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