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.
--- 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,