1169131 - add-mozreview-children.pl production run issues

Status: RESOLVED FIXED

(bugzilla.mozilla.org Graveyard :: Extensions: MozReview Integration, defect)

Description

[:glob ✱]

two issues encountered during the add-mozreview-children.pl run on production:

1. users with review requests disabled cause the script to fail:

Blair McBride [:Unfocused] (UNAVAILABLE) <[e-mail]> is not currently accepting 'review' requests.

2. the script is generating flagmail for every change.  imho we shouldn't generate _any_ emails for this migration (bugmail or flagmail).


(2) will need some changes to bugzilla's internals.

Comment 1

[:glob ✱]

3. the script is naively resetting cancelled flags to r?s

Comment 2

[Mark Côté [:mcote]]

Furthering point 3: the easiest method is to only set r? flags on each commit for anyone who still has a r? on the parent.

Comment 3

[Mark Côté [:mcote]]

4. Because the time between deploying the MozReview changes and running the script is now stretched a fair bit, we should ensure that we don't create duplicate commit attachments, which will happen if someone publishes a change to a review request created before the switchover.

Comment 4

[Mark Côté [:mcote]]

4, amended: Don't make any guesses as to state.  Just ensure that parents are gone and all commits are there.

Comment 5

[Mark Côté [:mcote]]

Attachment: Fix script issues

I inverted the logic here, creating a map of attachments by URL and then going through all review requests associated with the bug, obsoleting parents and creating children where necessary.

Comment 6

[:glob ✱]

Comment on attachment 8613647 [details] [diff] [review]
Fix script issues

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

most of these changes look good, however it appears to be relying on a field that rb does not return.

::: extensions/MozReview/bin/add-mozreview-children.pl
@@ +121,5 @@
> +
> +    foreach my $summary (@$summaries) {
> +        my $parent = $summary->{"parent"};
> +        my $attacher = Bugzilla::User->new({ id => $parent->{"submitter_bmo_id"},
> +                                             cache => 1 });

submitter_bmo_id isn't returned by rb:

Bug 35 (1/826)
  Fetching reviews from https://reviewboard-dev.allizom.org/api/extensions/mozreview.extension.MozReviewExtension/summary/?bug=35...
  Attachment 8591313 [details]: https://reviewboard-dev.allizom.org/r/430/
Bad argument param sent to Bugzilla::User::new function.

Comment 7

[Mark Côté [:mcote]]

Attachment: MozReview Request: mozreview: Include request submitter's BMO id in review-request summary API (bug 1169131). r?dminor

mozreview: Include request submitter's BMO id in review-request summary API (bug 1169131). r?dminor

Like the reviewers' BMO ids, this should be in a structure with the username,
but we're keeping them separate, for now, for backwards compatibility.

Comment 8

[Mark Côté [:mcote]]

(In reply to Byron Jones ‹:glob› from comment #6)
> submitter_bmo_id isn't returned by rb:

Doh, thanks, I forgot that I hadn't committed that change yet.

Comment 9

[Dan Minor [:dminor]]

Comment on attachment 8614028 [details]
MozReview Request: mozreview: Include request submitter's BMO id in review-request summary API (bug 1169131). r?dminor

https://reviewboard.mozilla.org/r/9839/#review8651

Ship It!

Comment 10

[Mark Côté [:mcote]]

Comment on attachment 8613647 [details] [diff] [review]
Fix script issues

Required change deployed to dev.

Comment 11

[Mark Côté [:mcote]]

Attachment: Fix script issues, v2

Removed pointless =cut from Flag.pm and added __END__ as suggested.

Comment 12

[:glob ✱]

Comment on attachment 8614060 [details] [diff] [review]
Fix script issues, v2

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

r=glob

Comment 13

[:glob ✱]

i made one minor change before committing.. i moved the memcached->clear_all to after the commit_transaction.
while it means that we'll be clearing the cache _a lot_ it protects us from having an inconsistent cache should the script fail mid-way.

To ssh://[e-mail]/webtools/bmo/bugzilla.git
   805054a..5cdb187  master -> master