mozreview: Reuse r? flags when publishing review requests (bug 1286740); r?glob r?imadueme draft
authorMark Cote <mcote@mozilla.com>
Tue, 11 Apr 2017 08:46:54 -0400 (2017-04-11)
changeset 10919 36c98366186fe0f546429ee392c79aa955e2bfd0
parent 10909 ef1a634e33fa4cdc735124a64df4c663be7e566e
push id1653
push usermcote@mozilla.com
push dateFri, 28 Apr 2017 17:34:34 +0000 (2017-04-28)
reviewersglob, imadueme
bugs1286740
mozreview: Reuse r? flags when publishing review requests (bug 1286740); r?glob r?imadueme If someone other than the review-request author changes the list of reviewers, clearing and then (re)creating the r? flags means that the flag creator will be set to the current user. We would like to preserve the request author as the flag creator. Hence, reuse existing flags if possible, changing only the requestee. MozReview-Commit-ID: 8IwHOr8dsqc
pylib/mozreview/mozreview/bugzilla/attachments.py
--- a/pylib/mozreview/mozreview/bugzilla/attachments.py
+++ b/pylib/mozreview/mozreview/bugzilla/attachments.py
@@ -48,16 +48,23 @@ def update_bugzilla_attachments(bugzilla
     # instead of using it directly, we query Bugzilla and map the stored,
     # immutable numeric Bugzilla userid into an email address. This lookup
     # could be avoided if Bugzilla accepted a numeric userid in the
     # requestee parameter when modifying an attachment.
     user_email_cache = {}
 
     for review_request_draft, review_request in children_to_post:
         carry_forward = {}
+
+        bum = BugzillaUserMap.objects.get(user=review_request_draft.submitter)
+        submitter_email = bugzilla.get_user_from_userid(
+            bum.bugzilla_user_id)['users'][0]['email']
+
+        user_email_cache[bum.bugzilla_user_id] = submitter_email
+
         has_code_changes = review_request_draft.diffset is not None
 
         for u in review_request_draft.target_people.all():
             bum = BugzillaUserMap.objects.get(user=u)
             email = user_email_cache.get(bum.bugzilla_user_id)
 
             if email is None:
                 user_data = bugzilla.get_user_from_userid(bum.bugzilla_user_id)
@@ -97,16 +104,29 @@ def update_bugzilla_attachments(bugzilla
             else:
                 # This is a meta data only change, don't touch any existing
                 # flags.
                 carry_forward[review.user.email] = True
 
         flags = []
         attachment = attachment_updates.get_attachment(review_request)
 
+        # Map of current r? flags, requestee -> flag object.
+        #
+        # If this review-request change is actually a reviewer delegating
+        # reviews, that is, they are removing themselves and adding another
+        # reviewer, we want to reuse the r? flag if possible.  In this case,
+        # the person submitting the review-request update is not the
+        # author, so they cannot create a new r? flag as though it were
+        # coming from the author.  However, the reviewer ("requestee") of an
+        # existing flag *can* be changed while preserving the flag creator.
+        # Thus, we record them here and then match them up later.
+
+        attachment_rq_flags = {}
+
         if attachment:
             # Update flags on an existing attachment.
             for f in attachment.get('flags', []):
                 if f['name'] not in ['review', 'feedback']:
                     # We only care about review and feedback flags.
                     continue
 
                 # When a new patch is pushed, we need to mimic what
@@ -141,41 +161,75 @@ def update_bugzilla_attachments(bugzilla
                                 'status': '?',
                                 'requestee': f['setter']
                             })
                         # else we leave the flag alone, carrying it forward.
 
                         # In either case, we've dealt with this reviewer, so
                         # remove it from the carry_forward dict.
                         carry_forward.pop(f['setter'])
+                elif f['status'] == '?' and f['setter'] == submitter_email:
+                    # Record this and sort out r? flags later.
+                    attachment_rq_flags[f['requestee']] = f
                 elif ('requestee' not in f or
                       f['requestee'] not in carry_forward):
                     # We clear review flags where the requestee is not
                     # a reviewer, or if there is some (possibly future) flag
                     # other than + or - that does not have a 'requestee' field.
                     flags.append({'id': f['id'], 'status': 'X'})
                 elif f['requestee'] in carry_forward:
                     # We're already waiting for a review from this user
                     # so don't touch the flag.
                     carry_forward.pop(f['requestee'])
 
         # Add flags for new reviewers.
+        #
         # We can't set a missing r+ (if it was manually removed) except in the
         # trivial (and useless) case that the setter and the requestee are the
         # same person.  We could set r? again, but in the event that the
         # reviewer is not accepting review requests, this will block
         # publishing, with no way for the author to fix it.  So we'll just
         # ignore manually removed r+s.
+        #
         # This is sorted so behavior is deterministic (this mucks with test
         # output otherwise).
+
+        # If there are existing r? flags and we want to re-request review for
+        # that reviewer, don't send any update to the server so that we keep
+        # the flag in place.
+
+        for reviewer, keep in sorted(carry_forward.iteritems()):
+            if not keep:
+                if reviewer in attachment_rq_flags:
+                    del(attachment_rq_flags[reviewer])
+                    del(carry_forward[reviewer])
+
+        # At this point, attachment_rq_flags contains r? flags that
+        # are no longer needed.  Reuse them for any new r? so that the
+        # creator is preserved.
+
         for reviewer, keep in sorted(carry_forward.iteritems()):
             if not keep:
-                flags.append({
-                    'name': 'review',
-                    'status': '?',
-                    'requestee': reviewer,
-                    'new': True
-                })
+                if attachment_rq_flags:
+                    rr_flag = attachment_rq_flags.values()[0]
+                    flags.append({
+                        'id': rr_flag['id'],
+                        'requestee': reviewer,
+                        'status': '?',
+                    })
+                    del(attachment_rq_flags[rr_flag['requestee']])
+                else:
+                    flags.append({
+                        'name': 'review',
+                        'status': '?',
+                        'requestee': reviewer,
+                        'new': True
+                    })
+
+        # Clear any remaining r? flags.
+
+        for reviewer, flag in attachment_rq_flags.iteritems():
+            flags.append({'id': flag['id'], 'status': 'X'})
 
         attachment_updates.create_or_update_attachment(
             review_request, review_request_draft, flags)
 
     attachment_updates.do_updates()