mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod draft
authorMark Cote <mcote@mozilla.com>
Wed, 02 Mar 2016 21:19:16 -0500
changeset 7518 74e12a2fe1fffaad1c85bf0425affcc603b72141
parent 7517 de737936b4f18399d779327e50ddb042ac53f5c1
child 7519 1bb1d8c8974edb4f324824b2fff0f6bd8ddf5be7
push id700
push usermcote@mozilla.com
push dateFri, 18 Mar 2016 00:41:48 +0000
reviewerssmacleod
bugs1237491
mozreview: Use a single internal call for all attachment actions (bug 1237491). r?smacleod Step one in consolidating BMO attachment calls on review publishing is to move them all to a single function in rbbz/extension.py. MozReview-Commit-ID: LG5TAl9Uewu
pylib/mozreview/mozreview/bugzilla/attachments.py
pylib/mozreview/mozreview/signal_handlers.py
--- a/pylib/mozreview/mozreview/bugzilla/attachments.py
+++ b/pylib/mozreview/mozreview/bugzilla/attachments.py
@@ -6,20 +6,26 @@ from mozautomation.commitparser import (
 
 from mozreview.models import (
     BugzillaUserMap,
     get_or_create_bugzilla_users,
 )
 from mozreview.rb_utils import (
     get_obj_url,
 )
+from mozreview.review_helpers import (
+    gen_latest_reviews
+)
 
 
-def post_bugzilla_attachment(bugzilla, bug_id, review_request_draft,
-                             review_request):
+def update_bugzilla_attachments(bugzilla, bug_id, children_to_post,
+                                children_to_obsolete):
+    for child in children_to_obsolete:
+        bugzilla.obsolete_review_attachments(bug_id, get_obj_url(child))
+
     # We publish attachments for each commit/child to Bugzilla so that
     # reviewers can easily track their requests.
 
     # The review request exposes a list of usernames for reviewers. We need
     # to convert these to Bugzilla emails in order to make the request into
     # Bugzilla.
     #
     # It may seem like there is a data syncing problem here where usernames
@@ -31,75 +37,72 @@ def post_bugzilla_attachment(bugzilla, b
     # immutable.
     #
     # But we do have a potential data syncing problem with the stored email
     # address. Review Board's stored email address could be stale. So
     # 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.
-    reviewers = {}
+    user_email_cache = {}
 
-    for u in review_request_draft.target_people.all():
-        bum = BugzillaUserMap.objects.get(user=u)
-
-        user_data = bugzilla.get_user_from_userid(bum.bugzilla_user_id)
+    for review_request_draft, review_request in children_to_post:
+        reviewers = {}
 
-        # Since we're making the API call, we might as well ensure the
-        # local database is up to date.
-        users = get_or_create_bugzilla_users(user_data)
-        reviewers[users[0].email] = False
+        for u in review_request_draft.target_people.all():
+            bum = BugzillaUserMap.objects.get(user=u)
+            email = user_email_cache.get(bum.bugzilla_user_id)
 
-    last_user = None
-    relevant_reviews = review_request.get_public_reviews().order_by(
-        'user', '-timestamp')
+            if email is None:
+                user_data = bugzilla.get_user_from_userid(bum.bugzilla_user_id)
 
-    for review in relevant_reviews:
-        if review.user == last_user:
-            # We only care about the most recent review for each
-            # particular user.
-            continue
+                # Since we're making the API call, we might as well ensure the
+                # local database is up to date.
+                users = get_or_create_bugzilla_users(user_data)
+                email = users[0].email
+                user_email_cache[bum.bugzilla_user_id] = email
 
-        last_user = review.user
+            reviewers[email] = False
 
-        # The last review given by this reviewer had a ship-it, so we
-        # will carry their r+ forward. If someone had manually changed
-        # their flag on bugzilla, we may be setting it back to r+, but
-        # we will consider the manual flag change on bugzilla user
-        # error for now.
-        if review.ship_it:
-            reviewers[last_user.email] = True
+        for review in gen_latest_reviews(review_request):
+            # The last review given by this reviewer had a ship-it, so we
+            # will carry their r+ forward. If someone had manually changed
+            # their flag on bugzilla, we may be setting it back to r+, but
+            # we will consider the manual flag change on bugzilla user
+            # error for now.
+            if review.ship_it:
+                reviewers[review.user.email] = True
+
+        rr_url = get_obj_url(review_request)
+        diff_url = '%sdiff/#index_header' % rr_url
 
-    rr_url = get_obj_url(review_request)
-    diff_url = '%sdiff/#index_header' % rr_url
+        # Only post a comment if the diffset has actually changed
+        comment = ''
+        if review_request_draft.get_latest_diffset():
+            diffset_count = review_request.diffset_history.diffsets.count()
+            if diffset_count < 1:
+                # We don't need the first line, since it is also the attachment
+                # summary, which is displayed in the comment.
+                full_commit_msg = review_request_draft.description.partition(
+                    '\n')[2].strip()
 
-    # Only post a comment if the diffset has actually changed
-    comment = ''
-    if review_request_draft.get_latest_diffset():
-        diffset_count = review_request.diffset_history.diffsets.count()
-        if diffset_count < 1:
-            # We don't need the first line, since it is also the attachment
-            # summary, which is displayed in the comment.
-            extended_commit_msg = review_request_draft.description.partition(
-                '\n')[2].strip()
+                full_commit_msg = strip_commit_metadata(full_commit_msg)
 
-            extended_commit_msg = strip_commit_metadata(extended_commit_msg)
-
-            if extended_commit_msg:
-                extended_commit_msg += '\n\n'
+                if full_commit_msg:
+                    full_commit_msg += '\n\n'
 
-            comment = '%sReview commit: %s\nSee other reviews: %s' % (
-                extended_commit_msg,
-                diff_url,
-                rr_url
-            )
-        else:
-            comment = ('Review request updated; see interdiff: '
-                       '%sdiff/%d-%d/\n' % (rr_url,
-                                            diffset_count,
-                                            diffset_count + 1))
+                comment = '%sReview commit: %s\nSee other reviews: %s' % (
+                    full_commit_msg,
+                    diff_url,
+                    rr_url
+                )
+            else:
+                comment = ('Review request updated; see interdiff: '
+                           '%sdiff/%d-%d/\n' % (rr_url,
+                                                diffset_count,
+                                                diffset_count + 1))
 
-    bugzilla.post_rb_url(bug_id,
-                         review_request.id,
-                         review_request_draft.summary,
-                         comment,
-                         diff_url,
-                         reviewers)
+        bugzilla.post_rb_url(bug_id,
+                             review_request.id,
+                             review_request_draft.summary,
+                             comment,
+                             diff_url,
+                             reviewers)
--- a/pylib/mozreview/mozreview/signal_handlers.py
+++ b/pylib/mozreview/mozreview/signal_handlers.py
@@ -24,17 +24,17 @@ from reviewboard.reviews.models import (
 )
 from reviewboard.reviews.signals import (
     review_request_closed,
     review_request_publishing,
     review_request_reopened,
 )
 
 from mozreview.bugzilla.attachments import (
-    post_bugzilla_attachment,
+    update_bugzilla_attachments,
 )
 from mozreview.bugzilla.client import (
     Bugzilla,
 )
 from mozreview.bugzilla.errors import (
     BugzillaError,
     bugzilla_to_publish_errors,
 )
@@ -279,24 +279,31 @@ def on_review_request_publishing(user, r
 
         # Create or update Bugzilla attachments for each draft commit.  This
         # is done before the children are published to ensure that MozReview
         # doesn't get into a strange state if communication with Bugzilla is
         # broken or attachment creation otherwise fails.  The Bugzilla
         # attachments will then, of course, be in a weird state, but that
         # should be fixed by the next successful publish.
         if using_bugzilla:
+            children_to_post = []
+            children_to_obsolete = []
+
             for child in child_rrs:
                 child_draft = child.get_draft(user=user)
 
                 if child_draft:
                     if child.id in discard_on_publish_rids:
-                        b.obsolete_review_attachments(
-                            bug_id, get_obj_url(child))
-                    post_bugzilla_attachment(b, bug_id, child_draft, child)
+                        children_to_obsolete.append(child)
+
+                    children_to_post.append((child_draft, child))
+
+            if children_to_post or children_to_obsolete:
+                update_bugzilla_attachments(b, bug_id, children_to_post,
+                                            children_to_obsolete)
 
         # Publish draft commits. This will already include items that are in
         # unpublished_rids, so we'll remove anything we publish out of
         # unpublished_rids.
         for child in child_rrs:
             if child.get_draft(user=user) or not child.public:
                 def approve_publish(sender, user, review_request_draft,
                                     **kwargs):