mozreview: Use BMO MozReview.attachments() API (bug 1237491). r?smacleod draft
authorMark Cote <mcote@mozilla.com>
Thu, 03 Mar 2016 19:50:47 -0500
changeset 7522 6d28f90f0fbda2c1206bb5c4fd235820b5a25078
parent 7521 415fadef62d44b97df92d3445aec65b00b625a2e
push id700
push usermcote@mozilla.com
push dateFri, 18 Mar 2016 00:41:48 +0000
reviewerssmacleod
bugs1237491
mozreview: Use BMO MozReview.attachments() API (bug 1237491). r?smacleod MozReview-Commit-ID: APK5D9iN9q3
pylib/mozreview/mozreview/bugzilla/client.py
--- a/pylib/mozreview/mozreview/bugzilla/client.py
+++ b/pylib/mozreview/mozreview/bugzilla/client.py
@@ -138,22 +138,21 @@ class BugzillaAttachmentUpdates(object):
             flags.append({
                 'name': 'review',
                 'status': '?',
                 'requestee': r,
                 'new': True
             })
 
         if rb_attachment:
-            params['ids'] = [rb_attachment['id']]
+            params['attachment_id'] = rb_attachment['id']
 
             if rb_attachment['is_obsolete']:
                 params['is_obsolete'] = False
         else:
-            params['ids'] = [self.bug_id]
             params['data'] = url
             params['content_type'] = 'text/x-review-board-request'
 
         params['file_name'] = 'reviewboard-%d-url.txt' % review_request_id
         params['summary'] = "MozReview Request: %s" % summary
         params['comment'] = comment
         if flags:
             params['flags'] = flags
@@ -164,41 +163,63 @@ class BugzillaAttachmentUpdates(object):
             self.creates.append(params)
 
     def obsolete_review_attachments(self, 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()
 
         for a in self.attachments:
             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']:
-            logger.info('Obsoleting attachments on bug %d: %s' % (
-                        self.bug_id, params['ids']))
-            self.updates.append(params)
+                logger.info('Obsoleting attachment %s on bug %d:' % (
+                            a['id'], self.bug_id))
+                self.updates.append({
+                    'attachment_id': a['id'],
+                    'is_obsolete': True
+                })
 
     @xmlrpc_to_bugzilla_errors
     def do_updates(self):
-        for params in self.creates:
-            self.bugzilla.proxy.Bug.add_attachment(
-                self.bugzilla._auth_params(params))
-            self.creates = self.creates[1:]
+        logger.info('Doing attachment updates for bug %s' % self.bug_id)
+        params = self.bugzilla._auth_params({
+            'bug_id': self.bug_id,
+            'attachments': self.creates + self.updates,
+        })
+
+        results = self.bugzilla.proxy.MozReview.attachments(params)
+
+        # The above Bugzilla call is wrapped in a single database transaction
+        # and should thus either succeed in creating and updating all
+        # attachments or will throw an exception and roll back all changes.
+        # However, just to be sure, we check the results.  There's not much
+        # we can do in this case, but we'll log an error for investigative
+        # purposes.
+        # TODO: Display an error in the UI (no easy way to do this without
+        # failing the publish).
 
-        for params in self.updates:
-            self.bugzilla.proxy.Bug.update_attachment(
-                self.bugzilla._auth_params(params))
-            self.updates = self.updates[1:]
+        ids_to_update = set(u['attachment_id'] for u in self.updates)
+        ids_to_update.difference_update(results['attachments_modified'].keys())
+
+        if ids_to_update:
+            logger.error('Failed to update the following attachments: %s' %
+                         ids_to_update)
+
+        num_to_create = len(self.creates)
+        num_created = len(results['attachments_created'])
+
+        if num_to_create != num_created:
+            logger.error('Tried to create %s attachments but %s reported as '
+                         'created.' % (num_to_create, num_created))
+
+        self.creates = []
+        self.updates = []
 
     def _update_attachments(self):
         if not self.attachments:
             self.attachments = self.bugzilla.get_rb_attachments(self.bug_id)
 
 
 class Bugzilla(object):
     """