mozreview: carry forward approval for autolanding (Bug 1239798) r?smacleod draft
authorDan Minor <dminor@mozilla.com>
Mon, 25 Jan 2016 13:51:32 -0500
changeset 6944 1bfee6c9b44d613296e04b8385997540f8fc342e
parent 6915 606fc555aa716dc1a76fd525c219d6c587e682a4
child 6945 6da1817e43e9cbd8898077996db45b9caf242c6c
push id551
push userdminor@mozilla.com
push dateMon, 25 Jan 2016 20:21:17 +0000
reviewerssmacleod
bugs1239798
mozreview: carry forward approval for autolanding (Bug 1239798) r?smacleod Currently we require re-review on commits made by non-L3 users which have been amended. This limits the utility of autoland as a replacement for the manual Bugzilla "check-in needed" process. Instead, we will apply the same criteria we use for L3 users for non-L3 users and create a warning in the UI during the autoland approval process that commits have been amended and may need further examination prior to landing.
hgext/reviewboard/tests/test-review-request-approval.t
pylib/mozreview/mozreview/review_helpers.py
--- a/hgext/reviewboard/tests/test-review-request-approval.t
+++ b/hgext/reviewboard/tests/test-review-request-approval.t
@@ -305,17 +305,17 @@ One more ship it should switch it back t
     diff_comments: []
   - id: 4
     public: true
     ship_it: true
     body_top: NVM, Ship-it!
     body_top_text_type: plain
     diff_comments: []
 
-Since the author is L1, adding a new diff should cancel approval
+Even though the author is L1, adding a new diff will not cancel approval
 
   $ echo modified > foo
   $ hg commit --amend > /dev/null
   $ export SSH_KEYNAME=l1a@example.com
   $ hg --config bugzilla.username=l1a@example.com --config bugzilla.apikey=${l1aapikey} --config reviewboard.autopublish=true push > /dev/null
   $ rbmanage dumpreview 2
   id: 2
   status: pending
@@ -358,18 +358,18 @@ Since the author is L1, adding a new dif
     patch:
     - diff --git a/foo b/foo
     - '--- a/foo'
     - +++ b/foo
     - '@@ -1,1 +1,1 @@'
     - -foo
     - +modified
     - ''
-  approved: false
-  approval_failure: A suitable reviewer has not given a "Ship It!"
+  approved: true
+  approval_failure: null
   review_count: 4
   reviews:
   - id: 1
     public: true
     ship_it: true
     body_top: Ship-it!
     body_top_text_type: plain
     diff_comments: []
--- a/pylib/mozreview/mozreview/review_helpers.py
+++ b/pylib/mozreview/mozreview/review_helpers.py
@@ -65,25 +65,16 @@ def has_l3_shipit(review_request):
         # Although I'm not certain when this field will be empty
         # it has "blank=true, null=True" - we'll assume there is
         # no published diff.
         return False
 
     for review in gen_latest_reviews(review_request):
         if not review.ship_it:
             continue
-
-        # TODO: We might want to add a required delay between when the
-        # diff is posted and when a review is published - this would
-        # avoid a malicious user from timing a diff publish immediately
-        # before a reviewer publishes a ship-it on the previous diff
-        # (Making it look like the ship-it came after the new diff)
-        if review.timestamp <= diffset_history.last_diff_updated:
-            continue
-
         if get_profile(review.user).has_scm_ldap_group('scm_level_3'):
             return True
 
     return False
 
 
 def get_reviewers_status(review_request, reviewers=None):
     """Returns the latest review status for each reviewer."""
@@ -99,8 +90,44 @@ def get_reviewers_status(review_request,
         return reviewers_status
 
     for review in gen_latest_reviews(review_request):
 
         if review.user.username in reviewers_status:
             reviewers_status[review.user.username]['ship_it'] = review.ship_it
 
     return reviewers_status
+
+
+def has_shipit_carryforward(review_request):
+    """Return whether the review request has a carried forward ship-it
+
+    A ship-it is considered carried forward if the commit for which it was
+    granted has been amended since the ship-it was granted. This returns
+    True if a ship-it was carried forward, and false if no ship-its are
+    present or none have been carried forward.
+    """
+    diffset_history = review_request.diffset_history
+
+    if not diffset_history:
+        # There aren't any published diffs so we should just consider
+        # any ship-its meaningless.
+        return False
+
+    if not diffset_history.last_diff_updated:
+        # Although I'm not certain when this field will be empty
+        # it has "blank=true, null=True" - we'll assume there is
+        # no published diff.
+        return False
+
+    for review in gen_latest_reviews(review_request):
+        if not review.ship_it:
+            continue
+
+        # TODO: We might want to add a required delay between when the
+        # diff is posted and when a review is published - this would
+        # avoid a malicious user from timing a diff publish immediately
+        # before a reviewer publishes a ship-it on the previous diff
+        # (Making it look like the ship-it came after the new diff)
+        if review.timestamp <= diffset_history.last_diff_updated:
+            return True
+
+    return False