1022707 - Duplicate review flags on attachments in Toolkit and Firefox for Metro

Status: RESOLVED FIXED

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

Description

[:Gijs (he/him)]

Per bug 1022015 comment #4.

Comment 1

[:glob ✱]

thanks gijs.

we need to first determine which review flag is the right one for those products.

there's probably reviews using the wrong flag, so we can't just update the flag exclusions as that will wipe those requests.  fixing this will probably require updating the database directly with a script (need to touch the flags and flag_state_activity tables, perhaps others) immediately prior to updating exclusions.

the main difference between the two review flags is setting one will trigger an email to '[e-mail]', while the other will not.

gavin - should reviews in the 'firefox for metro' and 'toolbox' products result in an email to the firefox-reviewers list?

Comment 2

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

(In reply to Byron Jones ‹:glob› from comment #1)
> gavin - should reviews in the 'firefox for metro' and 'toolbox' products
> result in an email to the firefox-reviewers list?

I assume you meant "toolkit". I think we should exclude them both (i.e. only have reviews in the "Firefox" product send mail to firefox-reviewers@). I asked for this in bug 834586 a while ago but apparently that was tricky (at least it was >1year ago).

Comment 3

[:glob ✱]

(In reply to :Gavin Sharp (email [e-mail]) from comment #2)
> I assume you meant "toolkit".

:)

> I think we should exclude them both (i.e. only
> have reviews in the "Firefox" product send mail to firefox-reviewers@). I
> asked for this in bug 834586 a while ago but apparently that was tricky (at
> least it was >1year ago).

ah, sorry about the lack of movement there.  i'll duplicate that bug to this one.

dkl's suggested fix in bug 834586 comment 1 is almost identical to my suggestion, except we don't need to create another review flag - those products should use the default review flag (id=4).


we need a script that:

updates all rows in the flags table which match:
  select flags.*
  from flags
  inner join bugs on bugs.bug_id = flags.bug_id
  where type_id = 748
  and bugs.product_id != 21
change flags.type_id to 4

updates type_id for the corresponding flag_state_activity rows

updates type_id for the corresponding nag_defer rows

clears memcached with Bugzilla->memcached->clear_all()


the ETL won't be impacted as it works from the bugs_activity table, so it's working with flag names not ids.


dylan - are you up to giving this script a shot?  create it in extensions/BMO/bin

Comment 5

[(not currently active) Ted Mielczarek]

This is breaking bzexport usage for bugs in the Toolkit product, can you prioritize it?

Comment 6

[Mark Côté [:mcote]]

Sure.

Comment 7

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1022707-v1.patch

> updates type_id for the corresponding nag_defer rows

There is no type_id in nag_defer, only (id, date, flag_id). I'm not sure I understand what you meant by this. :-)

Comment 8

[:glob ✱]

(In reply to Dylan William Hardison [:dylan] from comment #7)
> Created attachment 8461284 [details] [diff] [review]
> bug-1022707-v1.patch
> 
> > updates type_id for the corresponding nag_defer rows
> 
> There is no type_id in nag_defer, only (id, date, flag_id). I'm not sure I
> understand what you meant by this. :-)

sorry, that should be "updates flag_is for the corresponding nag_defer rows".

Comment 9

[:glob ✱]

(In reply to Byron Jones ‹:glob› from comment #8)
> sorry, that should be "updates flag_is for the corresponding nag_defer rows".

*sigh* flag_id

Comment 10

[:glob ✱]

dylan pointed out correctly over irc that we don't need to touch nag_defer because it's a fk to flags, not flagtypes.

Comment 11

[:glob ✱]

Comment on attachment 8461284 [details] [diff] [review]
bug-1022707-v1.patch

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

the logic here looks sane.

since it's a single-shot script, call it bug_1022707.pl

::: extensions/BMO/bin/fix-duplicate-review-flags.pl
@@ +18,5 @@
> +
> +use Bugzilla::Extension::BMO::Data;
> +use Bugzilla::Field;
> +use Bugzilla::User;
> +use Bugzilla::Util qw(trim);

most of these modules aren't used

@@ +19,5 @@
> +use Bugzilla::Extension::BMO::Data;
> +use Bugzilla::Field;
> +use Bugzilla::User;
> +use Bugzilla::Util qw(trim);
> +

you need to set the usage mode to ensure correct error management

@@ +29,5 @@
> +    WHERE type_id = 748
> +      AND bugs.product_id != 21
> +};
> +
> +print "Searching for suitable attachments..\n";

attachments?

@@ +34,5 @@
> +my $flag_ids = $dbh->selectcol_arrayref($sql);
> +my $total    = @$flag_ids;
> +
> +die "No suitable flags found\n" unless $total;
> +print "About to fix $total duplicate flags\n";

nit: these flags aren't duplicates; s/duplicate //;

Comment 13

[Dylan Hardison [:dylan] (he/him)]

Attachment: bug-1022707-v2.patch

Cleaned up per feedback suggestions.

Comment 14

[:glob ✱]

Comment on attachment 8475669 [details] [diff] [review]
bug-1022707-v2.patch

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

r=glob

Comment 15

[:glob ✱]

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   2bc51e2..3c28be9  master -> master

keeping open as a reminder to execute this script and update the flag exclusions following today's push.

Comment 16

[:glob ✱]

> $ sudo ./extensions/BMO/bin/bug_1022707.pl
> Searching for suitable flags..
> About to fix 343 flags
> Press <enter> to start, or ^C to cancel...
> Done

i've updated the configuration of the review flag, so there should now be a single review flag on those products.