469188 - Reviewers should be able to veto "Mark other translations as out of date"

Status: VERIFIED FIXED

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

Description

[Chris Ilias [:cilias]]

If someone edits an article, and check marks "Mark other translations as out of date", the reviewer should be able to uncheck before approving it.

Comment 1

[Underpass]

IMO when someone check marks that checkbox, approvers and locale leaders should also be warned by email, since that check could easily slip away unseen.

Comment 2

[David Tenser [:djst]]

How could we make sure that the reviewer sees the option being used when reviewing the change? Where in the review process should it be shown? Right now there is just an "Approve changes" link.

How about this: when you're viewing a staging article (as a reviewer), at the top of the article we could show that checkbox and allow people to uncheck it before approving. Let me attach screenshot mockup of what I mean.

Comment 3

[David Tenser [:djst]]

Attachment: Mockup UI

Rather than just showing a small line of text with a small link to "Approve changes", we could make it more of a full-size form with the ability to both veto a previous "significant edit" option, or add it (if the editor forgot to check it and it is indeed significant).

Comment 4

[David Tenser [:djst]]

Attachment: If the editor didn't check the checkbox

This is how it would look like if the checkbox wasn't checked by the article editor. The reviewer can still check the checkbox before approving the changes.

Note also that several people could have made edits on an article before the reviewer approves/rejects. If person A edits and marks the edit as significant, then person B corrects a typo on the staging copy and doesn't mark his/her change as significant, the overall approve will still be significant, so the checkbox should be checked for the approver.

Comment 5

[Underpass]

Can we decide - for example - that only an approver (and higher) can judge if
an edit is "significant"?

In other words: an editor changes the staging copy of a page, the approver
looks at the page and, if he decides that the changes is "significant", check
marks the "Mark other translations as out of date" box.

In my (personal) vision of the process, an editor never sees that checkbox with staging copies of an article.

Comment 6

[David Tenser [:djst]]

Attachment: Off-topic: Article approval/rejection mockup

OK, I started to think some more about the functionality we'd like and created a mockup that doesn't just solve this bug, but rejecting edits as well.

* It allows you to select whether you want to approve or reject the edit
* If you approve it, you also have the ability to correct the (already entered) change summary - maybe the original person making the edits didn't specify a clear enough message? Having a clear summary of what was changes is especially important if the edit marks translations as outdated.
* If you reject it, you have the ability to specify why it was rejected. This message should of course be sent to the person(s) making the edit(s).

Submitting this patch here in lack of a better place. This fix would involve more than this bug.

Comment 7

[David Tenser [:djst]]

Attachment: Off-topic: Article approval/rejection mockup

Better version.

Comment 8

[David Tenser [:djst]]

(In reply to comment #5)
> Can we decide - for example - that only an approver (and higher) can judge if
> an edit is "significant"?

What would be the benefit of that? I view the editor's checkbox as a "nomination", and ultimately it should be the reviewer that has to decide (as shown in the mockup I made). 

We could remove the possibility for an editor to check the checkbox altogether, but then we would lose the ability for the editor to "alert" the reviewer. Essentially, the reviewer would then have to pay more attention so an important edit is really making translations outdated. Otherwise, an important edit could be made and no localizers would ever be notified.

So:

* by keeping the option for editors, the risk is that a reviewer doesn't pay attention to the checked checkbox and marks all translations as outdated by accident. 

* by removing the option for editors, the risk is that a reviewer doesn't notice that the edit is significant, and no localizers are informed of the change.

Chris, do you have any opinion here?

Comment 9

[Underpass]

Sorry David,

The problem is only the fact that now this checkbox is too small and hidden to make a visible warn for the reviewer.

If this check triggers a notification and is visible enough (as it is with the mockup you made), there is no problem to leave it for both the editor and the reviewer.

Bye :-)

Comment 10

[David Tenser [:djst]]

Yeah, so my comment above was assuming that the mockup I made and attached to this bug would be implemented. I totally agree that the situation today is not good. :) 

The question is if we still would like to remove the option to mark a translation as outdated for non-reviewers/admins even if the above UI is implemented, but my opinion is that it won't be necessary as long as there is a way for reviewers to veto it.

Comment 11

[Paul Craciunoiu [:paulc]]

As per attachment 384380 [details], the UI will be added in bug 446082.

I think the approach here should be to simply remove that option from editing staging pages. The "out of date" action can then only be taken on approval. Sound good?

Comment 12

[Paul Craciunoiu [:paulc]]

Since bug 467554 removes the option, the ability to veto won't be required. Reviewers can decide when to mark articles out of date, which makes sense to me. If that's okay, then this bug will be solved in bug 467554.

Comment 13

[Paul Craciunoiu [:paulc]]

Calling this fixed as per bugs 467554 (this removes the ability to mark staging articles as out of date) and 446082 (this has the UI).
Henceforth, it's all up to the approver when to mark out of date. Sounds good?

Comment 14

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

Chris, would you mind taking a look at this?  Thanks!

Comment 15

[Chris Ilias [:cilias]]

I tried this on <https://support-stage.mozilla.org/en-US/kb/Closing+the+only+tab+closes+the+window>.

It seems to have worked, but the es version still doesn't have the warning. <https://support-stage.mozilla.org/en-US/kb/Cerrar+la+%C3%BAnica+pesta%C3%B1a+cierra+la+ventana?bl=n>
And the nl version
<https://support-stage.mozilla.org/en-US/kb/Cerrar+la+%C3%BAnica+pesta%C3%B1a+cierra+la+ventana?bl=n>.

Most other translations of that article have the warning. Maybe the reason it is not on the es and nl versions is due to another bug. I don't know.

Comment 16

[Paul Craciunoiu [:paulc]]

Strange indeed. Can you try on older articles? (before July 1st, my dump is old -- I'll try to update it Monday) and perhaps on sumotools (I'd like to not resync the stage database so often)

Post the articles here and I'll try to reproduce on my copy and see what's up.

Comment 17

[Chris Ilias [:cilias]]

Whoa! Check it out! Older article, same locales don't get the warning. The rest do!
<https://support-stage.mozilla.org/en-US/kb/Firefox+has+just+updated+tab+shows+each+time+you+start+Firefox>

German has it: <https://support-stage.mozilla.org/en-US/kb/Tab+Firefox+wurde+aktualisiert+wird+bei+jedem+Firefox-Start+angezeigt?bl=n>

Finnish has it: <https://support-stage.mozilla.org/en-US/kb/Firefox+on+p%C3%A4ivitetty+-v%C3%A4lilehti+n%C3%A4kyy+aina+Firefoxin+k%C3%A4ynnistyess%C3%A4?bl=n>

Italian has it: <https://support-stage.mozilla.org/en-US/kb/Ad+ogni+avvio+compare+una+scheda+in+cui+si+dice+che+Firefox+%C3%A8+stato+aggiornato?bl=n>

Spanish does not have it: <https://support-stage.mozilla.org/en-US/kb/Firefox+ha+actualizado++las+pesta%C3%B1as+al+iniciar+?bl=n>

Dutch does not have it: <https://support-stage.mozilla.org/en-US/kb/Tabblad+Firefox+is+bijgewerkt+verschijnt+telkens+als+u+Firefox+start?bl=n>

Indonesian does not have it: <https://support-stage.mozilla.org/en-US/kb/Tab+%27Firefox+has+just+updated%27+muncul+setiap+kali+memulai+Firefox?bl=n>

Comment 18

[Paul Craciunoiu [:paulc]]

It seems like maybe this is a different bug -- some locales don't get a warning. I filed bug 512630 to reflect this.

I think is fixed. Only reviewers can mark articles as out of date on approving an article edit. Or do we want to provide a way to remove the warning? If so, does that functionality require removing it on a per-translation basis or for all translations?

Comment 19

[Paul Craciunoiu [:paulc]]

s/is fixed/this bug is fixed

Comment 20

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

Chris, are we happy with this now, with the information from comment 18?

Comment 21

[Chris Ilias [:cilias]]

(In reply to comment #20)
> Chris, are we happy with this now, with the information from comment 18?

Yes

Comment 22

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

Thx -- verified.

Comment 23

[Louis-Philippe Huberdeau]

No link to commit or patch attached. Can't upstream.

If this is part of staging & approval, it might already be upstreamed.

Comment 24

[Paul Craciunoiu [:paulc]]

LPH: see comment 13

Comment 25

[James Socol [:jsocol, :james]]

comment 11, comment 12, comment 18 all have links to relevant bugs, as well.

Comment 26

[Louis-Philippe Huberdeau]

Will track the other bugs instead. This one is just a shell.