804708 - Add a "Review" extension to customise the review flag for Mozilla's workflow (make requestee/reviewer mandatory, provide review suggestions, etc)

Status: RESOLVED FIXED

(bugzilla.mozilla.org :: Extensions, defect)

Description

[(no longer active)]

I've probably filed this bug in some form before...  Setting review flags without specifying the requestee doesn't mean anything, and we should disallow that footgun.

Comment 1

[Teemu Mannermaa (:wicked)]

Sure it does, it means "from the wind" so anybody with some free time can do those. It's just that most reviewers won't go through the wind review queue and so they are often left untouched. :( I've sometimes tried to empty that queue for Bugzilla with mixed results. (Oh, looks like only one pending at the moment. \o/)

If this is done, it should really be product specific setting so different projects can decide if they want to add this additional burden on patchers. Are you sure they (especially newcomers, if such people are desired by you) know who to ask a review from?

Comment 3

[:glob ✱]

until determining the appropriate reviewer is simpler (see bug 774145), i'm not sure this is a move in the right direction.


as per comment 1, it definitely needs to be per-product, not a bmo wide.

eg, the firefox product has a default requestee which hits a list (see bug 680122), which conflicts with this request.

can we get a list of the products this policy should apply to?  we'll probably have to split the review flag further to facilitate this.

Comment 4

[(no longer active)]

(In reply to comment #3)
> until determining the appropriate reviewer is simpler (see bug 774145), i'm not
> sure this is a move in the right direction.

That would only be the case if you assume that people look at the list of such requests from time to time.  At least in the Firefox development, they don't, so such requests are practically treated as if the patch did not exist, which does not help anybody.

> as per comment 1, it definitely needs to be per-product, not a bmo wide.
> 
> eg, the firefox product has a default requestee which hits a list (see bug
> 680122), which conflicts with this request.

Fair enough.

> can we get a list of the products this policy should apply to?  we'll probably
> have to split the review flag further to facilitate this.

Core, Toolkit, Firefox for Android at the very least.

Comment 5

[:glob ✱]

(In reply to Ehsan Akhgari [:ehsan] from comment #4)
> That would only be the case if you assume that people look at the list of
> such requests from time to time.  At least in the Firefox development, they
> don't, so such requests are practically treated as if the patch did not
> exist, which does not help anybody.

well, we could schedule a whine which is sent to [e-mail] ;)

Comment 6

[David Lawrence [:dkl]]

Having a default requestee per product with our current implementation would require creating a separate review flag for each product desired using the flag admin UI which is very tricky.

One thing we could do is to add a checkbox to the flag admin UI via the extension that says that a requestee is required if requested. Then maintain a constant in the extension to allow filtering based on product/component. If the product/component doesn't exist for the bugs product/component, then it is required for all. This is doable if we do not forsee it being changed very often and removes the need for a database schema change. If it will be changed often we could possibly do a UI just for the default requestee requirement.

dkl

Comment 7

[David Lawrence [:dkl]]

(In reply to Byron Jones ‹:glob› from comment #5)
> well, we could schedule a whine which is sent to
> [e-mail] ;)

+1

Comment 8

[(no longer active)]

(In reply to comment #7)
> (In reply to Byron Jones â¹:glob⺠from comment #5)
> > well, we could schedule a whine which is sent to
> > [e-mail] ;)
> 
> +1

[e-mail] is the list of *Firefox* reviewers, not Core, Toolkit, FF for Android reviewers so I don't think that is a good idea.

About the possibility of us wanting to change this in the future, I can't foresee why we would decide that ignoring patches in those three components is a good idea in the future, so I would suggest that we should be ok not worrying about that.

Comment 9

[:Gavin Sharp [email: [e-mail]]]

(In reply to Byron Jones ‹:glob› from comment #3)
> eg, the firefox product has a default requestee which hits a list (see bug
> 680122), which conflicts with this request.

That list filters out all "review requested" emails, and is only used to "watch" r+/r- activity. It's not being used for the purpose of letting people request review from the wind, and so it does not conflict with this request.

Comment 10

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

See also bug 781336 which can lead to people inadvertently requesting for review "from the wind".

Comment 11

[Kyle Lahnakoski [:ekyle]]

I personally have no issue with "?" review having no requestee.  But please prevent r+/r- from having no requestee.

Comment 12

[Reed Loden [:reed]]

(In reply to Kyle Lahnakoski from comment #11)
> I personally have no issue with "?" review having no requestee.  But please
> prevent r+/r- from having no requestee.

What do you mean?

Comment 13

[JP Rosevear [:jpr]]

(In reply to Reed Loden [:reed] from comment #12)
> (In reply to Kyle Lahnakoski from comment #11)
> > I personally have no issue with "?" review having no requestee.  But please
> > prevent r+/r- from having no requestee.
> 
> What do you mean?

I think he means from a metrics gathering perspective the person associated with doing the review only has to be there when the review request is "resolved".  However, that is not relevant to this bug in particular.

The goal should be to prevent "in the wind reviews" which can be generally agreed as broken.  As others have noted, we can refine this over time to default reviews, or finding the most likely candidate or whatever, but in the mean time let's make this simple improvement if possible.

Comment 14

[Gervase Markham [:gerv]]

Perhaps it's an edge case, but a couple of weeks ago I filed 90 bugs, one per locale, on getting the locale relicensed. In each case I attached a patch and requested review from the wind. This is because looking up the correct reviewer for all 90 would have taken even more time, and I assumed that putting the bug in the right product and component would have led someone appropriate to look at it (and in many cases this was true).

Do we want a warning rather than a prohibition?

Gerv

Comment 15

[JP Rosevear [:jpr]]

(In reply to Gervase Markham [:gerv] from comment #14)
> Perhaps it's an edge case, but a couple of weeks ago I filed 90 bugs, one
> per locale, on getting the locale relicensed. In each case I attached a
> patch and requested review from the wind. This is because looking up the
> correct reviewer for all 90 would have taken even more time, and I assumed
> that putting the bug in the right product and component would have led
> someone appropriate to look at it (and in many cases this was true).

I think that is an extreme edge case, plus it leads to 20 or so of those bugs still sitting around so its not even clear to me it is the right approach.

> Do we want a warning rather than a prohibition?

I would still argue for prohibition.  Make the tool help our process as best as possible.

Comment 16

[Gervase Markham [:gerv]]

Hardly "still sitting around"; I went back to the ones were I'd got no response and looked up people by hand to ping. It was certainly less work than having to do it for all 70.

Gerv

Comment 17

[(no longer active)]

(In reply to JP Rosevear [:jpr] from comment #15)
> > Do we want a warning rather than a prohibition?
> 
> I would still argue for prohibition.  Make the tool help our process as best
> as possible.

Agreed, no need to try to address every possible edgecase.

Comment 18

[Justin Dolske [:Dolske]]

One slight concern (which is already in a bad state, so we can only make it better), which I guess this echos glob's concern in comment 3:

New contributors seem most likely to be affected by this. Today they may know enough to set "r?", but not enough to select a reviewer. So they happily set the flag, and end up waiting years for a review. With this change, they're going get the frustration immediately. Which is better is probably a long and not terribly useful discussion. :)

Can bugzilla generate a customizable error message when this condition happens? If we can add some text / links explaining how to select a reviewer (and pointer to #introduction?), that would seem an adequate way to fix both problems.

Comment 19

[(no longer active)]

Many new contributors these days have mentors, so I don't think this is going to be a big problem in practice.

Comment 20

[JP Rosevear [:jpr]]

Discussed at engineering call today - no real dissent to doing outright prohibition (but of course this isn't a poll of *all* devs).

Comment 21

[JP Rosevear [:jpr]]

(In reply to Justin Dolske [:Dolske] from comment #18)
> One slight concern (which is already in a bad state, so we can only make it
> better), which I guess this echos glob's concern in comment 3:
> 
> New contributors seem most likely to be affected by this. Today they may
> know enough to set "r?", but not enough to select a reviewer. So they
> happily set the flag, and end up waiting years for a review. With this
> change, they're going get the frustration immediately. Which is better is
> probably a long and not terribly useful discussion. :)

We should absolutely do follow up bugs to make things even better, but let's do it a little at a time.

Something like bwinton's review chooser could be integrated/extended:
http://weblog.latte.ca/blake/employment/mozilla/thunderbird/reviewerChooser.html

Comment 22

[Blake Winton (:bwinton) (:☕️)]

On that note, I have a semi-working instance of it up at https://reviewers.vcap.mozillalabs.com/ but I really don't have time to maintain it, and the vcap.mozillalabs.com site doesn't have the uptime guarantees that bmo probably wants.  ;)

If someone (anyone?) would like to take it over from me and push it forward, I'll be more than happy to show them what I currently have, how it all fits together, and where I think it could go in the future.

Thanks,
Blake.

Comment 23

[Jesse Ruderman]

> Can bugzilla generate a customizable error message when this condition happens?

It could suggest the component owner, or a list of component peers, or 
"[e-mail]" for Firefox bugs.

Ideally this would all happen on the client side, before the file is attached, so it feels more like "Bugzilla is being helpful" than "Bugzilla gave me an error message and made me fix it in a way that generates embarrassing bugspam".

Comment 24

[(no longer active)]

(In reply to comment #23)
> > Can bugzilla generate a customizable error message when this condition happens?
> 
> It could suggest the component owner, or a list of component peers, or 
> "[e-mail]" for Firefox bugs.
> 
> Ideally this would all happen on the client side, before the file is attached,
> so it feels more like "Bugzilla is being helpful" than "Bugzilla gave me an
> error message and made me fix it in a way that generates embarrassing bugspam".

While this is definitely nice, it's outside the scope of this bug.  I'd really like us not make perfect the enemy of the good here, and just move along and disallow this.

Comment 25

[JP Rosevear [:jpr]]

DKL, is this on the radar anytime soon?  Is there more info required?

Comment 26

[David Lawrence [:dkl]]

(In reply to JP Rosevear [:jpr] from comment #25)
> DKL, is this on the radar anytime soon?  Is there more info required?

Look at this some more today. I have a pretty good idea on how this would be implemented from a Bugzilla coding standpoint. Couple things I need to iron out first are, a bug that we found recently where duplicate bugs are created if a person enters an invalid requestee when entering a bug, and the proper wordage for the error message that gets displayed when a requestee is left empty.

The duplicate issue (bug 823153), I will work on first. Basically any error message thrown when creating a new bug and setting a review flag will have this issue so we need to work it out first to make this new error message work properly. A possible option would be to thrown an error before the form is submitted which would work around the duplication issue but we try to make things work without JS as much as we can.

As for the other thing, I would need to know how you would like the error message worded and also some possible links to documentation directing the user on how to find out what the proper requestee should be. I don't want to just have a cryptic message without giving some direction such as asking a mentor, etc.

Jesse suggested "It could suggest the component owner, or a list of component peers, or "[e-mail]" for Firefox bugs." but we would need something that worked equally well for all products. Module owners page?

dkl

Comment 27

[David Lawrence [:dkl]]

Attachment: Patch to disallow setting specific flags without requestee (POC) (v1)

This is a proof of concept on a way to accomplish this feature. It works for the cases where we need this to fire. One thing I would still like to add is some jscript to put (* required) next to the requestee text box where the requestee is required to give a visual clue to the user. Please review what I have so far and let me know if it looks OK or if you have a better idea. 

Also when this lands I will extend the Data.pm file to allow for setting default requestees for specific flags (bug 816228)

dkl

Comment 28

[David Lawrence [:dkl]]

Oops. Sorry for the spam. Set the flag on the wrong bug.

Comment 29

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #27)
> Created attachment 703947 [details] [diff] [review]
> Patch to disallow setting specific flags without requestee (POC) (v1)

looks like you accidentally left FlagDefaultRequestee/lib/Data.pm out of the patch.

Comment 30

[:glob ✱]

Comment on attachment 703947 [details] [diff] [review]
Patch to disallow setting specific flags without requestee (POC) (v1)

this looks fine for setting new flags, however this won't work when an existing flag is changed to from +/- to ? .. they are 'flag-<id>' params, not 'flag_type-<id>' params.

when fixing this, take care not to force a requestee for any existing flags set to ? (ie. make sure you catch when the flag is changing).

Comment 32

[:glob ✱]

Attachment: patch v1

here's a review extension which performs the following:
- adds a "reviewer required" checkbox to each product
- adds "suggested reviewers" to both products and components
  - components inherit the suggestions from their product, but can override
- javascript and server-side checking for missing reviewer
  - for new review flags only
- adds a "suggested reviewers" drop-down menu next to the review field
  - if the bug has a mentor, they are included in the list
- updates the FlagDefaultRequestee extension to prevent using that for review flags
- throws an error on enter_bug if a requestee fails to match exactly one person
  - override the current behavour of setting the requestee to blank

notes:
- i plan on seeding the list of suggested reviews from the module owners wiki page
- on enter_bug, the suggestions won't appear until a component is selected

Comment 33

[Kyle Lahnakoski [:ekyle]]

I would not know how to begin notifying the proper authorities about some reviews still in limbo:

https://metrics.mozilla.com/bugzilla-analysis/Reviews_NoReviewer.html

Where can I get a list of component "owners"?

Thanks!!

Comment 34

[:glob ✱]

(In reply to Kyle Lahnakoski : ekyle from comment #33)
> I would not know how to begin notifying the proper authorities about some reviews still
> in limbo

nor do i, but that isn't an issue this patch is trying to address (in fact it explicitly prevents forcing a reviewer on existing review requests).

> Where can I get a list of component "owners"?

a big problem is there simply isn't a list of owners by component; that's an issue that we're trying to address in part here.

i'll be seeding the list of suggested reviewers from the module owners wiki page (https://wiki.mozilla.org/Modules/All), and advertising this feature on lists and blogs so the default suggestions can be adjusted as required.

Comment 35

[:glob ✱]

Comment on attachment 767635 [details] [diff] [review]
patch v1

as it hasn't been reviewed yet, i'm going to add a few more goodies into this extension.

Comment 36

[(no longer active)]

(In reply to Byron Jones ‹:glob› from comment #34)
> (In reply to Kyle Lahnakoski : ekyle from comment #33)
> > I would not know how to begin notifying the proper authorities about some reviews still
> > in limbo
> 
> nor do i, but that isn't an issue this patch is trying to address (in fact
> it explicitly prevents forcing a reviewer on existing review requests).

Maybe this is something which interests Mike?

Comment 37

[:glob ✱]

Attachment: patch v2

here's a review extension which performs the following:
- adds a "reviewer required" checkbox to each product
- adds "suggested reviewers" to both products and components
  - components inherit the suggestions from their product, but can override
- javascript and server-side checking for missing reviewer
  - for new review flags only
- adds a "suggested reviewers" drop-down menu next to the review field
  - if the bug has a mentor, they are included in the list
- updates the FlagDefaultRequestee extension to prevent using that for review flags
- throws an error on enter_bug if a requestee fails to match exactly one person
  - override the current behaviour of setting the requestee to blank
+ sets the patch description from the filename
+ automatically checks the "patch" checkbox for .diff and .patch files

notes:
- i plan on seeding the list of suggested reviews from the module owners wiki page
- on enter_bug, the suggestions won't appear until a component is selected

Comment 38

[Benjamin Smedberg]

glob, I think mhoye can get you a better seed list based on historical data per component, since the modules owners list doesn't map to bugzilla components especially well. Will the setting maintain ordering so that e.g. we can put the most common reviewers first in the suggestion list?

Also, it is possible to customize the name that appears in the suggestion list so that we can show things like "mccr8 (Cycle Collector) / dbaron (Strings)" and so forth in the suggestions?

This is a great thing to build on; in the future I know mhoye wants to try and build out better suggestions based on the files that the patch touches. Thanks!

Comment 39

[Mike Hoye [:mhoye]]

> Maybe this is something which interests Mike?

I've got a mostly-built tool here called git-coach that identifies that will suggest reviewers and additional files to investigate based on your current git diff. I think this is a better and more automatable approach than a static per-component approach. It's based on Git - bwinton has a comparable mostly-built thing based on Mercurial, and we were just talking yesterday about how great it would be if somebody actually made finishing them both off a quarterly goal, so I intend to do that.

In the meantime, I can modify what I've got in the current (sloooooow - O(N^2) on the number of files, boo-urns) code to give you a reasonably sane-looking per-component list of likely reviewers. It'll take a day or two.

Comment 40

[:glob ✱]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #38)
> Also, it is possible to customize the name that appears in the suggestion
> list so that we can show things like "mccr8 (Cycle Collector) / dbaron
> (Strings)" and so forth in the suggestions?

that isn't possible right now; it's just a list of users and they are displayed as per everywhere else in bugzilla.

once this is in we can look at that, but it's a reasonable amount more work and i don't think it should delay this patch -- i'll file another bug for that.

(In reply to Mike Hoye [:mhoye] from comment #39)
> I've got a mostly-built tool here called git-coach that identifies that will
> suggest reviewers and additional files to investigate based on your current
> git diff. I think this is a better and more automatable approach than a
> static per-component approach.

i agree.

unfortunately as the suggestion list in bugzilla needs to be determined prior to the file being uploaded, the component is the best thing we have.  happy to switch to something smarter once that exists :) (bug 774145 is relevant here).

> In the meantime, I can modify what I've got in the current (sloooooow -
> O(N^2) on the number of files, boo-urns) code to give you a reasonably
> sane-looking per-component list of likely reviewers. It'll take a day or two.

excellent :)

Comment 41

[:glob ✱]

(In reply to Benjamin Smedberg  [:bsmedberg] from comment #38)
> Will the setting maintain ordering so that e.g.
> we can put the most common reviewers first in the suggestion list?

sorry, missed this question.  yes, it maintains order.

Comment 42

[(no longer active)]

Yeah, let's not make the perfect the enemy of the good.  We can always add fancier tools to suggest reviewers based on the contents of the diff later on.

Actually I CCed Mike on this bug to see if he can help with redirecting the existing review requests from the wind to real people.  :-)

Comment 43

[:glob ✱]

(In reply to :Ehsan Akhgari (needinfo? me!) from comment #42)
> Actually I CCed Mike on this bug to see if he can help with redirecting the
> existing review requests from the wind to real people.  :-)

that's probably fodder for another bug, or forum :)

fwiw, right now there are only 18 bugs with untargetted review requests across all bugzilla products, so this isn't a large problem:

https://bugzil.la/808233,862662,870639,891766,892209,893802,821028,833611,495357,890224,886234,888460,386313,399775,802924,547236,630144,628033

Comment 44

[Mike Hoye [:mhoye]]

> that's probably fodder for another bug, or forum :)

It think it might actually just be my job. Once the coaching tool is set up, I'll make checking for unassigned reviews a daily thing.

Comment 45

[David Lawrence [:dkl]]

Comment on attachment 776244 [details] [diff] [review]
patch v2

Review of attachment 776244 [details] [diff] [review]:
-----------------------------------------------------------------

Code looks really good as always and everything mostly works from my testing.

nit: move web/review.css to web/styles/review.css and web/review.js to web/js/review.js respectively for consistency.

When a product requires a reviewer and a user hits submit without, it shows the text as red with words that the review is required. The words push the input box up. Attaching screenshot to illustrate.

Suggestion: When the use selects '?' for review flag, show red '*' next to the requestee input similar to other mandatory fields. Do not display unless status is '?'. Since it highlights the input red if you hit submit I am OK if you do not feel like doing this. But it would be consistent with other fields on the page.

When select '?' for review flag, and then typing a search string in the requestee field such as ':glob', I then select an item from the autocomplete list
and it autofills the login name with a ',' (comma) after the login. When submitting the bug, it complains that '[e-mail],' does not exist or I do not have permission to see the user. This doesn't occur for similar values for other flags with a requestee field such as feedback. This does not happen when creating a new attachment. Need to strip commas in post_bug_attachment_flags possibly.

r=dkl with the above fixed and the other possible nits.

dkl

::: extensions/Review/template/en/default/hook/global/user-error-errors.html.tmpl
@@ +7,5 @@
> +  #%]
> +
> +[% IF error == "reviewer_required" %]
> +  [% title = "Reviewer Required" %]
> +  You must provide a reviewer for review requests.

Nit: You must provide a reviewer for 'review' flag requests.

::: extensions/Review/web/review.js
@@ +36,5 @@
> +    },
> +
> +    init_enter_bug: function() {
> +        Event.addListener('component', 'change', REVIEW.component_change);
> +        BUGZILLA.string['reviewer_required'] = 'A reviewer is required.';

Nit: Maybe expand that the requestee field needs a valid Bugzilla account or select from suggested reviewers.

Comment 46

[:glob ✱]

thanks for the review dkl :)

> When a product requires a reviewer and a user hits submit without, it shows
> the text as red with words that the review is required. The words push the
> input box up. Attaching screenshot to illustrate.

i hate that too, but there's no easy fix i can do without making significant html changes to that page :(

> When select '?' for review flag, and then typing a search string in the
> requestee field such as ':glob', I then select an item from the autocomplete
> list and it autofills the login name with a ',' (comma) after the login.

ah, review is a multiselect field, which i wasn't supporting.  fixed with a split and a foreach :)

> > +  You must provide a reviewer for review requests.
> Nit: You must provide a reviewer for 'review' flag requests.

i think most users, especially new contributors, don't understand the difference between fields and flags on bugzilla, so i left that out on purpose.


schema-only commit:

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
added extensions/Review
added extensions/Review/Config.pm
added extensions/Review/Extension.pm
Committed revision 8878.

Comment 47

[:glob ✱]

and the rest:

Committing to: bzr+ssh://[e-mail]/bmo/4.2/
modified post_bug.cgi
modified extensions/FlagDefaultRequestee/Extension.pm
added extensions/FlagDefaultRequestee/template/en/default/hook/global
added extensions/FlagDefaultRequestee/template/en/default/hook/global/user-error-errors.html.tmpl
modified extensions/Review/Extension.pm
added extensions/Review/template
added extensions/Review/web
added extensions/Review/template/en
added extensions/Review/template/en/default
added extensions/Review/template/en/default/hook
added extensions/Review/template/en/default/hook/admin
added extensions/Review/template/en/default/hook/attachment
added extensions/Review/template/en/default/hook/bug
added extensions/Review/template/en/default/hook/flag
added extensions/Review/template/en/default/hook/global
added extensions/Review/template/en/default/hook/admin/components
added extensions/Review/template/en/default/hook/admin/products
added extensions/Review/template/en/default/hook/admin/components/edit-common-rows.html.tmpl
added extensions/Review/template/en/default/hook/admin/products/edit-common-rows.html.tmpl
added extensions/Review/template/en/default/hook/admin/products/updated-changes.html.tmpl
added extensions/Review/template/en/default/hook/attachment/create-end.html.tmpl
added extensions/Review/template/en/default/hook/attachment/edit-end.html.tmpl
added extensions/Review/template/en/default/hook/bug/create
added extensions/Review/template/en/default/hook/bug/create/create-end.html.tmpl
added extensions/Review/template/en/default/hook/flag/list-requestee.html.tmpl
added extensions/Review/template/en/default/hook/global/header-start.html.tmpl
added extensions/Review/template/en/default/hook/global/user-error-errors.html.tmpl
added extensions/Review/web/js
added extensions/Review/web/styles
added extensions/Review/web/js/review.js
added extensions/Review/web/styles/review.css
modified skins/standard/attachment.css
modified template/en/default/flag/list.html.tmpl
modified template/en/default/global/userselect.html.tmpl
Committed revision 8879.