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
--- 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):