rbbz: Use a single internal call for all attachment actions (bug 1249403). r?tif draft
authorMark Cote <mcote@mozilla.com>
Thu, 18 Feb 2016 16:00:53 -0500
changeset 7278 1e3a215aa76dcc12c42f32cfcd105b8a49ae0c12
parent 7265 8f4dfd0518b2575239bebbbe32cfc090a4febd45
child 7279 c806ab060d82cb734a945dada15e0aa5ddc63b12
push id646
push usermcote@mozilla.com
push dateThu, 18 Feb 2016 21:04:33 +0000
reviewerstif
bugs1249403
rbbz: Use a single internal call for all attachment actions (bug 1249403). r?tif 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: EzmvSdMSXhS
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
@@ -8,18 +8,21 @@ from mozreview.models import (
     BugzillaUserMap,
     get_or_create_bugzilla_users,
 )
 from mozreview.rb_utils import (
     get_obj_url,
 )
 
 
-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 +34,83 @@ 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)
+    for review_request_draft, review_request in children_to_post:
+        reviewers = {}
+
+        for u in review_request_draft.target_people.all():
+            bum = BugzillaUserMap.objects.get(user=u)
+            email = user_email_cache.get(bum.bugzilla_user_id)
 
-        user_data = bugzilla.get_user_from_userid(bum.bugzilla_user_id)
+            if email is None:
+                user_data = bugzilla.get_user_from_userid(bum.bugzilla_user_id)
 
-        # 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)
+                # 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
+
         reviewers[users[0].email] = False
 
-    last_user = None
-    relevant_reviews = review_request.get_public_reviews().order_by(
-        'user', '-timestamp')
+        last_user = None
+        relevant_reviews = review_request.get_public_reviews().order_by(
+            'user', '-timestamp')
 
-    for review in relevant_reviews:
-        if review.user == last_user:
-            # We only care about the most recent review for each
-            # particular user.
-            continue
+        for review in relevant_reviews:
+            if review.user == last_user:
+                # We only care about the most recent review for each
+                # particular user.
+                continue
 
-        last_user = review.user
+            last_user = review.user
 
-        # 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
+            # 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
 
-    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.
-            extended_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.
+                ext_commit_msg = review_request_draft.description.partition(
+                    '\n')[2].strip()
 
-            extended_commit_msg = strip_commit_metadata(extended_commit_msg)
+                ext_commit_msg = strip_commit_metadata(ext_commit_msg)
 
-            if extended_commit_msg:
-                extended_commit_msg += '\n\n'
+                if ext_commit_msg:
+                    ext_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' % (
+                        ext_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):