680122 - Create a [e-mail] email alias, and set it up as the default requestee field for the review flag in the Firefox product

Status: VERIFIED FIXED

(bugzilla.mozilla.org :: Administration, task)

Description

[(no longer active)]

We are changing the Firefox code review requirements according to this document:

https://wiki.mozilla.org/Firefox/Code_Review

As part of this change, we need to add a [e-mail] alias so that Firefox reviewers can follow it, and we need to set it up as the default requestee field for the review? flag in the Firefox product.  It would be good if the review requestee field would be invisible in the Firefox component, so that people would just ask for a patch to be reviewed and not worry about who they're asking review from, or whether they should change [e-mail] to something else, but I don't know how hard that would be to implement in Bugzilla.  At any rate, we need to make sure that the [e-mail] account receives bugmail on review requests, so that Firefox reviewers can watch that address and get bugmail from review requests.

This is vetted on by Shaver, the Firefox module owner, and everyone on the Firefox reviewers list.  I'm filing this bug on behalf of them!  :-)

Comment 1

[:glob ✱]

it is possible for an email to be triggered when a flag is requested, however currently bugzilla only supports valid email addresses in this field, not bugzilla logins.  this means if we're to use it, an mailing list will have to be created and managed instead of people watching [e-mail] on bmo :(

what i propose is:
  - exclude the current review flag from the firefox product
  - create a new review flag which is not specifically requestable (requestors
    will not have the option of setting a requestee)
  - add [e-mail] as a flag CC
  - investigate if it's possible to hook the flag mail code to automatically
    CC watchers of that login

ehsan, do you have an expected time-frame for this request?

Comment 2

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

We want the review flag to continue to be targetable - we just want it to also have a default requestee if none is chosen.

We are creating the [e-mail] list in bug 680141.

Comment 3

[(no longer active)]

Yeah, let's use [e-mail] as the list then...

(In reply to Byron Jones ‹:glob› from comment #1)
> ehsan, do you have an expected time-frame for this request?

As soon as possible?  :-)

Comment 4

[David Lawrence [:dkl]]

(In reply to Ehsan Akhgari [:ehsan] from comment #3)
> Yeah, let's use [e-mail] as the list then...
> 
> (In reply to Byron Jones ‹:glob› from comment #1)
> > ehsan, do you have an expected time-frame for this request?
> 
> As soon as possible?  :-)

Ok. So to reiterate what glob mentioned, what we can do at this moment without coding changes:

1. Create a new review flag specifically for Firefox.
2. Set it to be requestable and optionally allow it to be requestable to a specific person.
3. Add [e-mail] to the cc list for the flag.

One downside: [e-mail] will be Cc'ed on all changes to the flag as well including ?, +, -, and the clearing of it. This might not be a problem but it seemed from the initial comment that you were mainly only concerned with 'review?' changes.

Sound good? We can try this for a while and make tweaks later if something is working out.

dkl

Comment 5

[Mike Shaver (:shaver emeritus)]

So we can't make f-r@m.o get the same email effects as "mail me when someone requests a flag of me", then?

Failing that, it's probably fine.  I can probably futz with mailman filters to restrict to the ? cases, though reviewers (and others!) might benefit from reading each other's reviews anyway...

Comment 6

[David Lawrence [:dkl]]

(In reply to Mike Shaver (:shaver) from comment #5)
> So we can't make f-r@m.o get the same email effects as "mail me when someone
> requests a flag of me", then?
> 
> Failing that, it's probably fine.  I can probably futz with mailman filters
> to restrict to the ? cases, though reviewers (and others!) might benefit
> from reading each other's reviews anyway...

It will get that plus the others. It will get it whether the request was to the wind or to someone specific. There is not currently anything in the code to allow specifying what type of flag activity per Cc user. So if you only want the list to see review? type emails, you may need to set up the filtering for now. 

Having said that, given some more time, we could add code to the BMO extension that can look at each email that goes out and pick out f-r@m.o if it doesn't match some set criteria. That is an option.

dkl

Comment 7

[(no longer active)]

Let's go with the simple case for now then, and not block on further improvements at this point.

Comment 8

[David Lawrence [:dkl]]

Hmm, something I didn't think about before that needs to be mentioned before turning this on. 

Currently there is a similar review flag that is enabled for _AllProducts_/_AllComponents_ for quite some time. Unfortunately it is also called 'review' so if I just add a new 'review' flag there will be confusion between the two in the UI. And if I add the new one and turn off the old one for the Firefox product only (exclude), then anyone who has used the old review flag up to this point, it will be deleted from the database. I can't simply marked is as inactive (which preserves all old data) since it a shared flag with other products.

Possible choices:

1. Name the new flag something else like firefox-review, fx-review, etc. 
2. Not worry about preserving old 'review' set flags for attachments currently under the Firefox product and excluding it. Then create the new one for Firefox only. Currently there are greater than 5000 bugs with the current review flag set under Firefox.
3. Migrate the old review flags for Firefox bugs over to the new Firefox only review flag. 

Of course number 2 may irritate some. Maybe glob can add more to this, but it seems to me that number 1 and number 3 may be the only good options. Number 3 will take a little more time to get the script ready and tested to do the migration.

Thoughts?
dkl

Comment 9

[Justin Dolske [:Dolske]]

I'd think retaining the flag name to be relatively important, particularly wrt not breaking existing searches/queries. Isn't there some way to modify the current "review" flag, and only have it do the new magic when Product == Firefox?

Also, how should restricted-access (eg security) bugs/attachments be handled? Should it send mail (in which case we need to lock down the mailing list), or do nothing? Either is fine with me, though I'd suspect that "do nothing" is less work, and I'd hope that security patches tend to be filed with a particular reviewer in mind or otherwise watched closely that a review-to-the-wind shouldn't languish long.

Comment 10

[(no longer active)]

(In reply to comment #8)
> 3. Migrate the old review flags for Firefox bugs over to the new Firefox only
> review flag. 

This, please!

Comment 11

[(no longer active)]

Ping?

Comment 12

[David Lawrence [:dkl]]

Sorry for any delay. Just working on the migration script right now that is needed to move the old Firefox/review flags over as we don't want to lose any current data. Once the script is done, I will be running some test runs on my local instance and then have it ran on our staging environment. When everything looks proper, we should be able to get it out to the live site pretty quickly.

dkl

Comment 13

[David Lawrence [:dkl]]

Attachment: Patch to add move_flag_types.pl reorg script (v1)

Patch that adds a new contrib/reorg-tools script called move_flag_types.pl that moves set flags from one type to another so that the old one can be dropped/altered without loss of data.

The new flag needs to be created before the script is run and the two type_ids needs to be plugged into the script code itself. If it looks good to you I will get IT to run it on staging. It works for me on my local installation.

dkl

Comment 14

[:glob ✱]

Comment on attachment 555011 [details] [diff] [review]
Patch to add move_flag_types.pl reorg script (v1)

nice, the logic here looks and works well.

i'd prefer to see the following changes before we deploy it (for safety):

- change the config from editing source to command line args.  this will make execution easier, and may avoid bzr issues when updating.

- provide a clear summary of the flags/types that match the specified ids and require confirmation before starting.  currently the only output is the flag count and ids, neither do much in terms of validating that the correct flagtype.id has been provided.

Comment 15

[David Lawrence [:dkl]]

Attachment: Patch to add move_flag_types.pl reorg script (v2)

Thanks glob. New patch that addresses your suggestions.

dkl

Comment 16

[:glob ✱]

Comment on attachment 555329 [details] [diff] [review]
Patch to add move_flag_types.pl reorg script (v2)

r=glob

Comment 17

[David Lawrence [:dkl]]

Thanks glob. Script committed to bzr.

Committing to: bzr+ssh://[e-mail]/bmo/4.0
added contrib/reorg-tools/move_flag_types.pl
Committed revision 7851.

The next steps are:

1. create a new flag on BMO called fx-review with the proper product:component and cc list members.
2. Set the new flag to inactive so it doesn't yet show up in the UI.
3. File a bug to have IT run the script on a live server to move the old Firefox review set flags to the new flag.
4. Update the old review flag to no longer include the Firefox product. 
5. Rename the new fx-review flag to just review. Since the name will be the same as the old flag, bugs_activity entries will still be valid. (bugs_activity only contains the textual name of the flag and not the id)
6. Activate the new review flag so it displays in the UI.
7. profit!

Sound sane?

dkl

Comment 18

[:glob ✱]

(In reply to David Lawrence [:dkl] from comment #17)
> Sound sane?

yes.

Comment 19

[David Lawrence [:dkl]]

News Update: Flag move script went well on bugzilla-stage.mozilla.org. We require new code to be pushed to the live BMO server for the same to be ran on production. IT likes to do it off-peak so hopefully this will happen tonight and we can get the flags move over either tonight or first thing tomorrow. Then it is just a quick rename and we can close this out.

Comment 20

[David Lawrence [:dkl]]

Complete. IT has now moved the old flags to the new one and all is done. I have set the CC of the new 'review' flag to [e-mail] so let me know if you do not see any emails going to that address.

dkl

Comment 21

[(no longer active)]

This has added 28.34% to the volume of my bugmail ;-)

Confirming, thanks!