446082 - Rejection/Approval notifications

Status: VERIFIED FIXED

(support.mozilla.org :: Knowledge Base Software, task, P1)

Description

[Chris Ilias [:cilias]]

Add email notifications for contributors, when their edit has been rejected/approved. If rejected, the person rejecting the edit should be able to explain why.

Comment 1

[David Tenser [:djst]]

Would definitely be useful. We need:

* A way for a reviewer to enter a reason for rejection
* Notification about rejection/approval sent to the email of the person making the edit.

Comment 2

[David Tenser [:djst]]

No time for 1.1.

Comment 3

[Chris Ilias [:cilias]]

Pushing to 1.2 (if this can be done within that time).
This bug creates a major disconnect with new contributors.

Comment 4

[Paul Craciunoiu [:paulc]]

Need to detail this a bit, including phrasing and maybe a mockup. It seems important so I can do it, but with more info please :)

Comment 5

[Laura Thomson :laura]

Chris, can you give us some verbage for this?  I don't know if this is going to get into 1.2.

Comment 6

[Chris Ilias [:cilias]]

I see this working similar to mailman.
When a non-[approver][locale leader][admin] edits an article, they are sent an email, telling them that their edit is awaiting review. Just use the email address from their user account info. It would include links to the staging copy and Contributors forum (for questions).
-------------------------------------------------------
Subject: Your edit to %ARTICLENAME% is awaiting review.
Body: 
Hi %USERNAME%,
Thank you for contributing to the Firefox Support Knowledge Base. Your edit to the article %ARTICLENAME% has been submitted and will be reviewed shortly. In order to keep the Knowledge Base from having any incorrect data, all edits to an article must go through a staging copy of the article and be reviewed. To view your edit, just go to the staging copy URL:
%STAGINGCOPY-URL%

If you have any questions, please feel free to ask in the Contributors forum: https://support.mozilla.com/tiki-view_forum.php?forumId=3
-------------------------------------------------------

If an edit gets approved, a second email should be sent regardless if the contributor has approver rights, with any comments from the approver and the name of the reviewer.
-------------------------------------------------------
Subject: Your edit to %ARTICLENAME% has been approved.
Body:
Hi %USERNAME%,
Thank you for contributing to the Firefox Support Knowledge Base. Your edit to the article %ARTICLENAME% was reviewed by %APPROVERNAME% and has been approved! You can view the updated article with your edit at %KBARTICLE-URL%

Comments from reviewer:
"%REVIEWER COMMENTS%"

If you have any questions, please feel free to ask in the Contributors forum: https://support.mozilla.com/tiki-view_forum.php?forumId=3
-------------------------------------------------------

If an edit gets rejected, a second email should be sent regardless if the contributor has approver rights, with any comments from the reviewer and the name of the reviewer.
-------------------------------------------------------
Subject: Your edit to %ARTICLENAME% has not been approved.
Body:
Hi %USERNAME%,
Thank you for contributing to the Firefox Support Knowledge Base. Your edit to the article %ARTICLENAME% was reviewed by %APPROVERNAME% and was not approved. Please read the reviewer's comments below to find out why your edit was not approved.

Comments from reviewer:
"%REVIEWER COMMENTS%"

If you have any questions, please feel free to ask in the Contributors forum: https://support.mozilla.com/tiki-view_forum.php?forumId=3
-------------------------------------------------------

For the web-UI, David created a mockup in attachment 384380 [details] in bug 469188. The difference being that I would like the reviewer to have the option of providing a comment when approving an edit. (Sometimes the edit just needs a tweak, so it gets approved and tweaked. Plus it's a good opportunity to open communication, and give an extra personal "thank you" and "great job")


I don't know how much of an issue this would be, but right now, the method of rejecting an edit is to rollback the edit. (bug 416422)

Comment 7

[Paul Craciunoiu [:paulc]]

How important is this? So I can tell how soon I should get to it.

Comment 8

[Chris Ilias [:cilias]]

This is a major disconnect with new contributors, so it is very important.

Comment 9

[Chris Ilias [:cilias]]

It seems that we need to separate the variables from rest of the text to make these notifications easy to translate, so here are the revised texts. Could someone provide feedback on the text:

-------------------------------------------------------
Subject: %ARTICLENAME% edit is awaiting review.
Body: 
Hi %USERNAME%,
Thank you for contributing to the Firefox Support Knowledge Base. Your edit to the following article has been submitted and will be reviewed shortly.
%ARTICLENAME%

In order to keep the Knowledge Base from having any incorrect data, all edits to an article must go through a staging copy of the article and be reviewed. To view your edit, just go to the staging copy URL:
%STAGINGCOPY-URL%

If you have any questions, please feel free to ask in the Contributors forum:
https://support.mozilla.com/tiki-view_forum.php?forumId=3
-------------------------------------------------------
Subject: %ARTICLENAME% edit has been approved.
Body:
Hi %USERNAME%,
Thank you for contributing to the Firefox Support Knowledge Base. Your edit to the following article was reviewed and has been approved! You can view the updated article with your edit at %KBARTICLE-URL%

Reviewer: %APPROVERNAME%
Comments from reviewer:
"%REVIEWER COMMENTS%"

If you have any questions, please feel free to ask in the Contributors forum:
https://support.mozilla.com/tiki-view_forum.php?forumId=3
-------------------------------------------------------
Subject: %ARTICLENAME% edit has not been approved.
Body:
Hi %USERNAME%,
Thank you for contributing to the Firefox Support Knowledge Base. Your edit to the following article was reviewed and was not approved. Please read the reviewer's comments below to find out why your edit was not approved.

Article: %STAGINGCOPY-URL%
Reviewer: %APPROVERNAME%
Comments from reviewer:
"%REVIEWER COMMENTS%"

If you have any questions, please feel free to ask in the Contributors forum:
https://support.mozilla.com/tiki-view_forum.php?forumId=3
-------------------------------------------------------

Comment 10

[[:Cww]]

Change the subjects to:

%ARTICLENAME%: under review

%ARTICLENAME%: edit approved

%ARTICLENAME%: edit not approved

or something like that.

In some languages, grammar may force the form: "The edit to %ARTICLENAME% is awaiting review" or even "Under review is %ARTICLENAME% edit" but you avoid all that if you avoid sentences.

Comment 11

[Chris Ilias [:cilias]]

Good stuff. I definitely agree.

Comment 12

[Paul Craciunoiu [:paulc]]

I also propose, for the second email, to change: "the following article was reviewed and has been approved! You can view the
updated article with your edit at %KBARTICLE-URL%"
to be:
"the following article was reviewed and has been approved! You can view the
updated article with your edit below:

Article: %KBARTICLE-URL%""

to match the same format as the rejected email.

Comment 13

[[:Cww]]

Also perhaps a little text about how you need to be logged in to see staging copy:

In order to keep the Knowledge Base free of incorrect data, all edits
to an article are put into a staging copy and undergo review. You can view the staging copy of the article (including your edit) by logging in and going to the following URL:
%STAGINGCOPY-URL%

Comment 14

[Chris Ilias [:cilias]]

Comments 12 and 13 sound good to me. I had another look at comment 10, and I think "%ARTICLENAME%: under review" makes it sound like someone reported the article as abuse. How about:
%ARTICLENAME%: edit waiting review

Comment 15

[[:Cww]]

That sounds good.  I mostly was commenting about making it not sentences in the subject.

Comment 16

[Paul Craciunoiu [:paulc]]

Attachment: patch, v1

This needs a lot of testing, so please review asap. Patch does the following:

* Adds email notifications for the three circumstances. The "waiting for review" email (user_review_notification) only applies to users without approval permissions.

* Adds the UI from David's mockup (attachment 384380 [details] from bug 469188). The "Mark out of date" checkbox should only display for English articles. I also rephrased the approval box's text for consistency, and turned the rejection box into a textarea rather than a row of input. The JS used for the UI degrades gracefully

* Adds a bit of padding to #tocnav elements, the text looks less strangled by the box :). This would apply to all of them all over the site. If that's undesirable, I can make it specific to this patch.

* Implements reject functionality, which is done by rolling back + removing the older versions. Rejecting removes all the versions since the last approval. 

It's not a small patch...

Comment 17

[Paul Craciunoiu [:paulc]]

(In reply to comment #16)
> * Implements reject functionality, which is done by rolling back + removing the
> older versions. Rejecting removes all the versions since the last approval. 
Just a concern here. As Chris requested, rejecting an edit deletes it. Should we provide some way for users to recover the edit? Such as, should we send it by email? Cheng raised a point if you reject an edit for a minor issue, all other changes will be erased.

Chris, you're probably in the best position to decide what the approach on this should be. Perhaps a separate bug to handle rejection behavior?

Comment 18

[Laura Thomson :laura]

Comment on attachment 392830 [details] [diff] [review]
patch, v1

Nitpicks, fix and resubmit:
* Does staging_rejectapprove_ui() belong in tiki-index?  Shouldn't it only be included/called for admins?  Also *cough*globals*cough*

* remove_mail_events_object() is a lot more generic than its name would imply (rename for reusab ility)

* In sendApprovalEmailNotification, why is there a switch statment with one case?
* In rollback_page_to_version, remove globals.

Comment 19

[Paul Craciunoiu [:paulc]]

Attachment: patch, v2

Thanks Laura! Good stuff :)

Comment 20

[Chris Ilias [:cilias]]

(In reply to comment #17)
> Just a concern here. As Chris requested, rejecting an edit deletes it. Should
> we provide some way for users to recover the edit? Such as, should we send it
> by email? Cheng raised a point if you reject an edit for a minor issue, all
> other changes will be erased.
> 
> Chris, you're probably in the best position to decide what the approach on this
> should be. Perhaps a separate bug to handle rejection behavior?

Send the page source in the "%ARTICLENAME%: edit waiting review" message.

We had a discussion a long time ago regarding rejections being per-edit vs. re-syncing with the KB version. In more cases than not, when there is more than one edit separating the staging copy from the KB version, it is by the same person, tweaking their work. If the reviewer sees a minor issue, then the reviewer should make that tweak and approve the previous edit, not reject it.

Comment 21

[Paul Craciunoiu [:paulc]]

Comment on attachment 393001 [details] [diff] [review]
patch, v2

Need to update the patch with this.

Comment 22

[Paul Craciunoiu [:paulc]]

Attachment: patch, v3

This adds the functionality Chris mentioned.

Comment 23

[Paul Craciunoiu [:paulc]]

r48827 / r48828

Comment 24

[Chris Ilias [:cilias]]

I tested this on:
https://support-stage.mozilla.org/en-US/kb/chris+tests+notification
and 
https://support-stage.mozilla.org/en-US/kb/Site+Identity+Button

- the "articlename: edit waiting review" notification does not contain the article source. However the rejection edit did contain it. No biggy.
- after rejecting an edit, I get a page that says "The staging article has been rolled back to the latest approved version. All the versions that were not approved have been removed.". That's it. If we don't take the contributor back to the staging copy, we should add a link to it.
- after I rejected an edit, the article was still in the "waiting for review" category.
- Here's the showstopper: All approved edits were removed, and rejected edit was still in the history of the staging copy.
Before: http://ilias.ca/bugs/446082/before.png
After: http://ilias.ca/bugs/446082/after.png

Comment 25

[Paul Craciunoiu [:paulc]]

Attachment: patch, v4

This fixes everything Chris mentioned:
* Adds article source to the "Waiting for review" notification
* Removes article from "Waiting for review" category on reject
* Removes the proper versions on reject (for this, the lastModif date needs to preserved, hence the modification to use_history() function). I tested this with both a single edit and multiple edits, with and without approvals beforehand.
* Adds a link to the staging article on the rejection page.

I've tested even more, so hopefully this time it works right. Thanks Chris!

Comment 26

[Paul Craciunoiu [:paulc]]

r49771 / r49772
Chris, go ahead and bang at it again! Stephen, feel free to join in! :)
There may still be problems, since this is a large change in the code.

Comment 27

[Chris Ilias [:cilias]]

Looks good to me. 

I tested on <https://support-stage.mozilla.org/en-US/kb/Right-clicking+does+not+work>

Comment 28

[David Tenser [:djst]]

support-stage seems to be down so I can't be more specific than this:

* Why are we showing the whole article source when an edit is rejected, rather than showing a diff of the edit? 
* If an edit is approved, we're not indicating which edit it was -- why aren't approvals and rejections treated equally here?
* "Reason for approval" is a little weird and makes it look like you have to provide one. This looks more like a way to provide optional feedback/comments for the author, so why not something like "Message to author (optional):" ?
* The multiline text box is under the label if you approve, but next to the label if you reject. Was this code tested locally??

Comment 29

[Paul Craciunoiu [:paulc]]

(In reply to comment #28)
> * Why are we showing the whole article source when an edit is rejected, rather
> than showing a diff of the edit? 
On rejection, the edits are being erased from staging history, hence the edit cannot be easily recovered using just the diff.
> * If an edit is approved, we're not indicating which edit it was -- why aren't
> approvals and rejections treated equally here?
I'm not sure I follow. What is indicated in one but not the other? (Except article source)
> * "Reason for approval" is a little weird and makes it look like you have to
> provide one. This looks more like a way to provide optional feedback/comments
> for the author, so why not something like "Message to author (optional):" ?
Since this bug has gone through several commits already, can you file a separate bug for changing the text (and anything else about the UI)
> * The multiline text box is under the label if you approve, but next to the
> label if you reject. Was this code tested locally??
I see what you mean here. The code was tested, but I guess on a mac the layout looks a bit different. On my machine, both text boxes wrap. I'll insert a <br/> before the latter. Thanks David!

Comment 30

[Paul Craciunoiu [:paulc]]

Done, filed bug 513013.

Comment 31

[David Tenser [:djst]]

Attachment: Inconsistency in line breaks

To clarify, when rejecting changes, the text area isn't under the label, but on the right side, which looks weird and is inconsistent with the approve changes appearance.

Also, in addition to my feedback in the previous comment: The first second of loading the page, you can see all the UI that's supposed to be hidden by default. Why isn't the default CSS to hide these things, and then use Javascript to show it, rather than the other way around (show by default, then hide it with code)?

Comment 32

[Paul Craciunoiu [:paulc]]

With javascript disabled, the UI still needs to be usable. Hence, it's shown by default.

Comment 33

[David Tenser [:djst]]

For users, support for non-javascript users is important for obvious reasons, but I don't see why we would need to support it for contributors. This is fine for now given the code freeze etc but let's discuss this at some point in a future sumodev meeting to change our javascript policy for contributors.

Comment 34

[Stephen Donner [:stephend] Not actively reading bugmail]

A few things:

[1] Still testing this; hope to be wrapped up by EOD today.
[2] Paul will address comment 31
[3] While testing this, I discovered bug 513752, which I think would be annoying to SUMO contributors/editors.

Comment 35

[Stephen Donner [:stephend] Not actively reading bugmail]

Bug 513840 should be resolved fixed before anyone tries verifying this; I think some JS is borked, somewhere.

Comment 36

[Stephen Donner [:stephend] Not actively reading bugmail]

Verified FIXED; tested all regressions, as well as re-certified functionality.  Will still keep an eye out, but am calling this good.

Comment 37

[David Tenser [:djst]]

Should be upstreamed to Tiki. See also bug 514054 which fixes a bug (are there other bugs related to Rejection/Approval notifications? Is it better to just take the latest code and upstream as a feature, rather than upstreaming every patch?)

Comment 38

[Paul Craciunoiu [:paulc]]

(In reply to comment #37)
Not sure I understand about "latest version". The series of patches results in a final "piece of code" that implements this feature. I would take that to start with and figure out where it goes in the new Tiki.

Comment 39

[Louis-Philippe Huberdeau]

I just took the tiki-approve_staging_page.php from SUMO and moved it to tiki trunk. No significant changes were made for that feature on our side. It also required to grab two new functions in categlib. I verified all commits that were made on that file as well and none of them need backporting from what I can tell. Most were related to permissions, which had a complete revamp.

I made basic tests to approve a page and it worked. In depth testing would be required.

Comment 40

[Marc Laporte]

Test staging and approval on tiki-trunk.mozilla.com ->
https://bugzilla.mozilla.org/show_bug.cgi?id=539171

This is the main test to see if we are close to being ready for SUMO &
Tiki5 code sync.