rbbz: Do all Bugzilla attachment actions together (bug 1211791). r?smacleod draft
authorMark Cote <mcote@mozilla.com>
Wed, 06 Jan 2016 14:21:07 -0500
changeset 6642 e7ea0f8389e3e58d8fc20da736b85e9ee789fb25
parent 6641 b911a834cd5c2a6e3ac854c1edd82c80bf1acd30
push id501
push usermcote@mozilla.com
push dateThu, 07 Jan 2016 02:34:13 +0000
reviewerssmacleod
bugs1211791
rbbz: Do all Bugzilla attachment actions together (bug 1211791). r?smacleod This is another step toward switching to a single Bugzilla API call for attachment actions. We now prep all the attachment adds and updates and then execute them sequentially.
pylib/mozreview/mozreview/bugzilla/client.py
pylib/rbbz/rbbz/extension.py
--- a/pylib/mozreview/mozreview/bugzilla/client.py
+++ b/pylib/mozreview/mozreview/bugzilla/client.py
@@ -41,16 +41,158 @@ def xmlrpc_to_bugzilla_errors(func):
 
             if e.args:
                 msg = e.args[0]
 
             raise BugzillaError('IOError: %s' % msg)
     return _transform_errors
 
 
+class BugzillaAttachmentUpdates(object):
+
+    def __init__(self, bugzilla):
+        self.bugzilla = bugzilla
+        self.attachments = {}
+        self.updates = []
+        self.creates = []
+
+    @xmlrpc_to_bugzilla_errors
+    def create_or_update_attachment(self, bug_id, review_id, summary, comment,
+                                    url, reviewers):
+        """Creates or updates an attachment containing a review-request URL.
+
+        The reviewers argument should be a dictionary mapping reviewer email
+        to a boolean indicating if that reviewer has given an r+ on the
+        attachment in the past that should be left untouched.
+        """
+
+        # Copy because we modify it.
+        reviewers = reviewers.copy()
+        params = {}
+        flags = []
+        rb_attachment = None
+
+        self._update_attachments(bug_id)
+
+        # Find the associated attachment, then go through the review flags.
+        for a in self.attachments[bug_id]:
+
+            # Make sure we check for old-style URLs as well.
+            if not self.bugzilla._rb_attach_url_matches(a['data'], url):
+                continue
+
+            rb_attachment = a
+
+            for f in a.get('flags', []):
+                if f['name'] not in ['review', 'feedback']:
+                    # We only care about review and feedback flags.
+                    continue
+                elif f['name'] == 'feedback':
+                    # We always clear feedback flags.
+                    flags.append({'id': f['id'], 'status': 'X'})
+                elif f['status'] == '+' and f['setter'] not in reviewers:
+                    # This r+ flag was set manually on bugzilla rather
+                    # then through a review on Review Board. Always
+                    # clear these flags.
+                    flags.append({'id': f['id'], 'status': 'X'})
+                elif f['status'] == '+':
+                    if not reviewers[f['setter']]:
+                        # We should not carry this r+ forward so
+                        # re-request review.
+                        flags.append({
+                            'id': f['id'],
+                            'name': 'review',
+                            'status': '?',
+                            'requestee': f['setter']
+                        })
+
+                    reviewers.pop(f['setter'])
+                elif 'requestee' not in f or f['requestee'] not in reviewers:
+                    # We clear review flags where the requestee is not
+                    # a reviewer or someone has manually set r- on the
+                    # attachment.
+                    flags.append({'id': f['id'], 'status': 'X'})
+                elif f['requestee'] in reviewers:
+                    # We're already waiting for a review from this user
+                    # so don't touch the flag.
+                    reviewers.pop(f['requestee'])
+
+            break
+
+        # Add flags for new reviewers.
+
+        # Sorted so behavior is deterministic (this mucks with test output
+        # otherwise).
+        for r in sorted(reviewers.keys()):
+            flags.append({
+                'name': 'review',
+                'status': '?',
+                'requestee': r,
+                'new': True
+            })
+
+        if rb_attachment:
+            params['ids'] = [rb_attachment['id']]
+
+            if rb_attachment['is_obsolete']:
+                params['is_obsolete'] = False
+        else:
+            params['ids'] = [bug_id]
+            params['data'] = url
+            params['content_type'] = 'text/x-review-board-request'
+
+        params['file_name'] = 'reviewboard-%d-url.txt' % review_id
+        params['summary'] = "MozReview Request: %s" % summary
+        params['comment'] = comment
+        if flags:
+            params['flags'] = flags
+
+        if rb_attachment:
+            self.updates.append(params)
+        else:
+            self.creates.append(params)
+
+    @xmlrpc_to_bugzilla_errors
+    def obsolete_review_attachments(self, bug_id, rb_url):
+        """Mark any attachments for a given bug and review request as obsolete.
+
+        This is called when review requests are discarded or deleted. We don't
+        want to leave any lingering references in Bugzilla.
+        """
+        params = {'ids': [], 'is_obsolete': True}
+
+        self._update_attachments(bug_id)
+
+        for a in self.attachments[bug_id]:
+            if (self.bugzilla._rb_attach_url_matches(a.get('data'), rb_url) and
+                not a.get('is_obsolete')):
+                params['ids'].append(a['id'])
+
+        if params['ids']:
+            self.updates.append(params)
+
+    @xmlrpc_to_bugzilla_errors
+    def do_updates(self):
+        for params in self.creates:
+            self.bugzilla.proxy.Bug.add_attachment(
+                self.bugzilla._auth_params(params))
+
+        for params in self.updates:
+            self.bugzilla.proxy.Bug.update_attachment(
+                self.bugzilla._auth_params(params))
+
+        self.creates = []
+        self.updates = []
+
+    @xmlrpc_to_bugzilla_errors
+    def _update_attachments(self, bug_id):
+        if bug_id not in self.attachments:
+            self.attachments[bug_id] = self.bugzilla.get_rb_attachments(bug_id)
+
+
 class Bugzilla(object):
     """
     Interface to a Bugzilla system.
 
     At the moment this uses the XMLRPC API.  It should probably be converted
     to REST at some point.
 
     General FIXME: try to get more specific errors out of xmlrpclib.Fault
@@ -159,111 +301,16 @@ class Bugzilla(object):
         except xmlrpclib.Fault as e:
             if e.faultCode == 102:
                 return True
             raise
 
         return bool(groups)
 
     @xmlrpc_to_bugzilla_errors
-    def post_rb_url(self, bug_id, review_id, summary, comment, url,
-                    reviewers):
-        """Creates or updates an attachment containing a review-request URL.
-
-        The reviewers argument should be a dictionary mapping reviewer email
-        to a boolean indicating if that reviewer has given an r+ on the
-        attachment in the past that should be left untouched.
-        """
-
-        # Copy because we modify it.
-        reviewers = reviewers.copy()
-        params = self._auth_params({})
-        flags = []
-        rb_attachment = None
-        attachments = self.get_rb_attachments(bug_id)
-
-        # Find the associated attachment, then go through the review flags.
-        for a in attachments:
-
-            # Make sure we check for old-style URLs as well.
-            if not self._rb_attach_url_matches(a['data'], url):
-                continue
-
-            rb_attachment = a
-
-            for f in a.get('flags', []):
-                if f['name'] not in ['review', 'feedback']:
-                    # We only care about review and feedback flags.
-                    continue
-                elif f['name'] == 'feedback':
-                    # We always clear feedback flags.
-                    flags.append({'id': f['id'], 'status': 'X'})
-                elif f['status'] == '+' and f['setter'] not in reviewers:
-                    # This r+ flag was set manually on bugzilla rather
-                    # then through a review on Review Board. Always
-                    # clear these flags.
-                    flags.append({'id': f['id'], 'status': 'X'})
-                elif f['status'] == '+':
-                    if not reviewers[f['setter']]:
-                        # We should not carry this r+ forward so
-                        # re-request review.
-                        flags.append({
-                            'id': f['id'],
-                            'name': 'review',
-                            'status': '?',
-                            'requestee': f['setter']
-                        })
-
-                    reviewers.pop(f['setter'])
-                elif 'requestee' not in f or f['requestee'] not in reviewers:
-                    # We clear review flags where the requestee is not
-                    # a reviewer or someone has manually set r- on the
-                    # attachment.
-                    flags.append({'id': f['id'], 'status': 'X'})
-                elif f['requestee'] in reviewers:
-                    # We're already waiting for a review from this user
-                    # so don't touch the flag.
-                    reviewers.pop(f['requestee'])
-
-            break
-
-        # Add flags for new reviewers.
-
-        # Sorted so behavior is deterministic (this mucks with test output
-        # otherwise).
-        for r in sorted(reviewers.keys()):
-            flags.append({
-                'name': 'review',
-                'status': '?',
-                'requestee': r,
-                'new': True
-            })
-
-        if rb_attachment:
-            params['ids'] = [rb_attachment['id']]
-
-            if rb_attachment['is_obsolete']:
-                params['is_obsolete'] = False
-        else:
-            params['ids'] = [bug_id]
-            params['data'] = url
-            params['content_type'] = 'text/x-review-board-request'
-
-        params['file_name'] = 'reviewboard-%d-url.txt' % review_id
-        params['summary'] = "MozReview Request: %s" % summary
-        params['comment'] = comment
-        if flags:
-            params['flags'] = flags
-
-        if rb_attachment:
-            self.proxy.Bug.update_attachment(params)
-        else:
-            self.proxy.Bug.add_attachment(params)
-
-    @xmlrpc_to_bugzilla_errors
     def get_rb_attachments(self, bug_id):
         """Get all attachments that contain review-request URLs."""
 
         params = self._auth_params({
             'ids': [bug_id],
             'include_fields': ['id', 'data', 'content_type', 'is_obsolete',
                                'flags']
         })
@@ -366,36 +413,16 @@ class Bugzilla(object):
 
         if comment:
             params['comment'] = comment
 
         self.proxy.Bug.update_attachment(params)
         return True
 
     @xmlrpc_to_bugzilla_errors
-    def obsolete_review_attachments(self, bug_id, rb_url):
-        """Mark any attachments for a given bug and review request as obsolete.
-
-        This is called when review requests are discarded or deleted. We don't
-        want to leave any lingering references in Bugzilla.
-        """
-        params = self._auth_params({
-            'ids': [],
-            'is_obsolete': True,
-        })
-
-        for a in self.get_rb_attachments(bug_id):
-            if (self._rb_attach_url_matches(a.get('data'), rb_url) and
-                not a.get('is_obsolete')):
-                params['ids'].append(a['id'])
-
-        if params['ids']:
-            self.proxy.Bug.update_attachment(params)
-
-    @xmlrpc_to_bugzilla_errors
     def valid_api_key(self, username, api_key):
         try:
             return self.proxy.User.valid_login({
                 'login': username,
                 'api_key': api_key,
             })
         except xmlrpclib.Fault as e:
             # Invalid API-key formats (e.g. not 40 characters long) or expired
--- a/pylib/rbbz/rbbz/extension.py
+++ b/pylib/rbbz/rbbz/extension.py
@@ -19,17 +19,17 @@ from reviewboard.reviews.models import (
                                         ReviewRequestDraft)
 from reviewboard.reviews.signals import (reply_publishing,
                                          review_publishing,
                                          review_request_closed,
                                          review_request_publishing,
                                          review_request_reopened)
 from reviewboard.site.urlresolvers import local_site_reverse
 
-from mozreview.bugzilla.client import Bugzilla
+from mozreview.bugzilla.client import Bugzilla, BugzillaAttachmentUpdates
 from mozreview.bugzilla.errors import BugzillaError
 from mozreview.errors import (CommitPublishProhibited,
                               ParentShipItError)
 from mozreview.extra_data import (UNPUBLISHED_RRIDS_KEY,
                                   gen_child_rrs,
                                   gen_rrs_by_rids,
                                   gen_rrs_by_extra_data_key,
                                   is_parent)
@@ -203,18 +203,21 @@ def bugzilla_to_publish_errors(func):
             return func(*args, **kwargs)
         except BugzillaError as e:
             raise PublishError('Bugzilla error: %s' % e.msg)
     return _transform_errors
 
 
 def update_bugzilla_attachments(bugzilla, bug_id, children_to_post,
                                 children_to_obsolete):
+    attachment_updates = BugzillaAttachmentUpdates(bugzilla)
+
     for child in children_to_obsolete:
-        bugzilla.obsolete_review_attachments(bug_id, get_obj_url(child))
+        attachment_updates.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.
     #
@@ -294,22 +297,25 @@ def update_bugzilla_attachments(bugzilla
                     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)
+        attachment_updates.create_or_update_attachment(
+            bug_id,
+            review_request.id,
+            review_request_draft.summary,
+            comment,
+            diff_url,
+            reviewers)
+
+    attachment_updates.do_updates()
 
 
 @bugzilla_to_publish_errors
 def on_review_request_publishing(user, review_request_draft, **kwargs):
     # There have been strange cases (all local, and during development), where
     # when attempting to publish a review request, this handler will fail
     # because the draft does not exist. This is a really strange case, and not
     # one we expect to happen in production. However, since we've seen it
@@ -537,17 +543,19 @@ def on_review_request_closed_discarded(u
                                     ReviewRequest.DISCARDED,
                                     AUTO_CLOSE_DESCRIPTION)
     else:
         # TODO: Remove this once we properly prevent users from closing
         # commit review requests.
         b = Bugzilla(get_bugzilla_api_key(user))
         bug = int(review_request.get_bug_list()[0])
         diff_url = '%sdiff/#index_header' % get_obj_url(review_request)
-        b.obsolete_review_attachments(bug, diff_url)
+        attachment_updates = BugzillaAttachmentUpdates(b)
+        attachment_updates.obsolete_review_attachments(bug, diff_url)
+        attachment_updates.do_updates()
 
 
 def on_review_request_closed_submitted(user, review_request, type, **kwargs):
     if (not is_review_request_squashed(review_request) or
         type != ReviewRequest.SUBMITTED):
         return
 
     close_child_review_requests(user, review_request, ReviewRequest.SUBMITTED,