MozReview: Move 'on_review_publishing' signal to mozreview (Bug 1262548). r?smacleod draft
authorDavid Walsh <dwalsh@mozilla.com>
Thu, 26 May 2016 15:48:23 -0500
changeset 8485 bf277a5218008275f9bdc4c09a93f32feacb6823
parent 8484 898c76e634748699818422d5cb0fbe5205fe5658
child 8486 402dbbb4642b08f8f8de0278945e952027732c52
push id917
push userbmo:dwalsh@mozilla.com
push dateThu, 09 Jun 2016 17:49:07 +0000
reviewerssmacleod
bugs1262548
MozReview: Move 'on_review_publishing' signal to mozreview (Bug 1262548). r?smacleod MozReview-Commit-ID: 51HqyHhUSlb
pylib/mozreview/mozreview/signal_handlers.py
pylib/rbbz/rbbz/extension.py
--- a/pylib/mozreview/mozreview/signal_handlers.py
+++ b/pylib/mozreview/mozreview/signal_handlers.py
@@ -1,14 +1,17 @@
 from __future__ import unicode_literals
 
 import copy
 import json
 import logging
 
+from django.contrib.sites.models import (
+    Site
+)
 from django.db.models.signals import (
     post_save,
     pre_delete,
     pre_save,
 )
 
 from djblets.siteconfig.models import (
     SiteConfiguration,
@@ -20,36 +23,41 @@ from reviewboard.extensions.hooks import
     SignalHook,
 )
 from reviewboard.reviews.models import (
     Review,
     ReviewRequest,
     ReviewRequestDraft,
 )
 from reviewboard.reviews.signals import (
+    review_publishing,
     review_request_closed,
     review_request_publishing,
     review_request_reopened,
 )
 
 from mozreview.bugzilla.attachments import (
     update_bugzilla_attachments,
 )
 from mozreview.bugzilla.client import (
     Bugzilla,
     BugzillaAttachmentUpdates,
 )
 from mozreview.bugzilla.errors import (
     BugzillaError,
     bugzilla_to_publish_errors,
 )
+from mozreview.diffs import (
+    build_plaintext_review,
+)
 from mozreview.errors import (
     CommitPublishProhibited,
     ConfidentialBugError,
     InvalidBugIdError,
+    ParentShipItError,
 )
 from mozreview.extra_data import (
     DISCARD_ON_PUBLISH_KEY,
     DRAFTED_COMMIT_DATA_KEYS,
     fetch_commit_data,
     gen_child_rrs,
     gen_rrs_by_extra_data_key,
     gen_rrs_by_rids,
@@ -131,16 +139,22 @@ def initialize_signal_handlers(extension
         sandbox_errors=False)
 
     SignalHook(
         extension,
         pre_save,
         pre_save_review,
         sender=Review)
 
+    SignalHook(
+        extension,
+        review_publishing,
+        on_review_publishing,
+        sandbox_errors=False)
+
 
 def post_save_review_request_draft(sender, **kwargs):
     """Handle post_save for a ReviewRequestDraft."""
     draft = kwargs["instance"]
 
     if kwargs["created"] and not kwargs["raw"]:
         copy_commit_data(draft)
 
@@ -579,8 +593,71 @@ def pre_save_review(sender, *args, **kwa
                 # all the reviews, which is what get_reviewers_status does.
                 reviewers_status = get_reviewers_status(review.review_request,
                                                         reviewers=[review.user])
                 user = review.user.username
                 flag = reviewers_status.get(user, {}).get('review_flag', ' ')
                 review.extra_data[REVIEW_FLAG_KEY] = flag
 
             review.ship_it = (review.extra_data[REVIEW_FLAG_KEY] == 'r+')
+
+
+@bugzilla_to_publish_errors
+def on_review_publishing(user, review, **kwargs):
+    """Comment in the bug and potentially r+ or clear a review flag.
+
+    Note that a reviewer *must* have editbugs to set an attachment flag on
+    someone else's attachment (i.e. the standard BMO review process).
+
+    TODO: Report lack-of-editbugs properly; see bug 1119065.
+    """
+    review_request = review.review_request
+    logger.info('Publishing review for user: %s review id: %s '
+                'review request id: %s' % (user, review.id,
+                                            review_request.id))
+
+    # skip review requests that were not pushed
+    if not is_pushed(review_request):
+        logger.info('Did not publish review: %s: for user: %d: review not '
+                    'pushed.' % (user, review.id))
+        return
+
+    site = Site.objects.get_current()
+    siteconfig = SiteConfiguration.objects.get_current()
+    comment = build_plaintext_review(review,
+                                     get_obj_url(review, site,
+                                                 siteconfig),
+                                     {"user": user})
+    b = Bugzilla(get_bugzilla_api_key(user))
+
+    if is_parent(review_request):
+        # Mirror the comment to the bug, unless it's a ship-it or the review
+        # flag was set, in which case throw an error.
+        # Ship-its and review flags are allowed only on child commits.
+        if review.ship_it or review.extra_data.get(REVIEW_FLAG_KEY):
+            raise ParentShipItError
+
+        [b.post_comment(int(bug_id), comment) for bug_id in
+         review_request.get_bug_list()]
+    else:
+        diff_url = '%sdiff/#index_header' % get_obj_url(review_request)
+        bug_id = int(review_request.get_bug_list()[0])
+
+        commented = False
+        flag = review.extra_data.get(REVIEW_FLAG_KEY)
+
+        if flag is not None:
+            commented = b.set_review_flag(bug_id, flag, review.user.email,
+                                              diff_url, comment)
+        else:
+            # If for some reasons we don't have the flag set in extra_data,
+            # fall back to ship_it
+            logger.warning('Review flag not set on review %s, '
+                           'updating attachment based on ship_it' % review.id)
+            if review.ship_it:
+                commented = b.r_plus_attachment(bug_id, review.user.email,
+                                                diff_url, comment)
+            else:
+                commented = b.cancel_review_request(bug_id, review.user.email,
+                                                    diff_url, comment)
+
+        if comment and not commented:
+            b.post_comment(bug_id, comment)
--- a/pylib/rbbz/rbbz/extension.py
+++ b/pylib/rbbz/rbbz/extension.py
@@ -1,37 +1,27 @@
 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 import logging
 
-from django.contrib.sites.models import Site
-
-from djblets.siteconfig.models import SiteConfiguration
-
 from reviewboard.extensions.base import Extension
 from reviewboard.extensions.hooks import SignalHook
 from reviewboard.reviews.signals import (
     reply_publishing,
-    review_publishing,
 )
 
 from mozreview.bugzilla.client import Bugzilla
 from mozreview.bugzilla.errors import (
     bugzilla_to_publish_errors,
 )
 from mozreview.diffs import build_plaintext_review
-from mozreview.errors import (
-    ParentShipItError,
-)
 from mozreview.extra_data import (
-    is_parent,
     is_pushed,
-    REVIEW_FLAG_KEY
 )
 from mozreview.models import (
     get_bugzilla_api_key,
 )
 from mozreview.rb_utils import (
     get_obj_url,
 )
 
@@ -39,98 +29,30 @@ logger = logging.getLogger(__name__)
 
 
 class BugzillaExtension(Extension):
 
     def initialize(self):
         # Any abortable signal hooks that talk to Bugzilla should have
         # sandbox_errors=False, since we don't want to complete the action if
         # updating Bugzilla failed for any reason.
-        SignalHook(self, review_publishing, on_review_publishing,
-                   sandbox_errors=False)
         SignalHook(self, reply_publishing, on_reply_publishing,
                    sandbox_errors=False)
 
 
 def get_reply_url(reply, site=None, siteconfig=None):
     """ Get the URL for a reply to a review.
 
     Since replies can have multiple comments, we can't link to a specific
     comment, so we link to the parent review which the reply is targeted at.
     """
     return get_obj_url(reply.base_reply_to, site=site, siteconfig=siteconfig)
 
 
 @bugzilla_to_publish_errors
-def on_review_publishing(user, review, **kwargs):
-    """Comment in the bug and potentially r+ or clear a review flag.
-
-    Note that a reviewer *must* have editbugs to set an attachment flag on
-    someone else's attachment (i.e. the standard BMO review process).
-
-    TODO: Report lack-of-editbugs properly; see bug 1119065.
-    """
-    review_request = review.review_request
-    logger.info('Publishing review for user: %s review id: %s '
-                'review request id: %s' % (user, review.id,
-                                            review_request.id))
-
-    # skip review requests that were not pushed
-    if not is_pushed(review_request):
-        logger.info('Did not publish review: %s: for user: %d: review not '
-                    'pushed.' % (user, review.id))
-        return
-
-    site = Site.objects.get_current()
-    siteconfig = SiteConfiguration.objects.get_current()
-    comment = build_plaintext_review(review,
-                                     get_obj_url(review, site,
-                                                 siteconfig),
-                                     {"user": user})
-    b = Bugzilla(get_bugzilla_api_key(user))
-
-    # TODO: Update all attachments in one call.  This is not possible right
-    # now because we have to potentially mix changing and creating flags.
-
-    if is_parent(review_request):
-        # Mirror the comment to the bug, unless it's a ship-it or the review
-        # flag was set, in which case throw an error.
-        # Ship-its and review flags are allowed only on child commits.
-        if review.ship_it or review.extra_data.get(REVIEW_FLAG_KEY):
-            raise ParentShipItError
-
-        [b.post_comment(int(bug_id), comment) for bug_id in
-         review_request.get_bug_list()]
-    else:
-        diff_url = '%sdiff/#index_header' % get_obj_url(review_request)
-        bug_id = int(review_request.get_bug_list()[0])
-
-        commented = False
-        flag = review.extra_data.get(REVIEW_FLAG_KEY)
-
-        if flag is not None:
-            commented = b.set_review_flag(bug_id, flag, review.user.email,
-                                              diff_url, comment)
-        else:
-            # If for some reasons we don't have the flag set in extra_data,
-            # fall back to ship_it
-            logger.warning('Review flag not set on review %s, '
-                           'updating attachment based on ship_it' % review.id)
-            if review.ship_it:
-                commented = b.r_plus_attachment(bug_id, review.user.email,
-                                                diff_url, comment)
-            else:
-                commented = b.cancel_review_request(bug_id, review.user.email,
-                                                    diff_url, comment)
-
-        if comment and not commented:
-            b.post_comment(bug_id, comment)
-
-
-@bugzilla_to_publish_errors
 def on_reply_publishing(user, reply, **kwargs):
     review_request = reply.review_request
     logger.info('Posting bugzilla reply for review request %s' % (
                 review_request.id))
 
     # skip review requests that were not pushed
     if not is_pushed(review_request):
         return