mozreview: allow r? to cancel carrying forward r+ (
bug 1195661) r?smacleod
This changes the r+ carry forward behaviour to not carry forward r+ if the
reviewer was specified with r? in the commit summary. Doing this from the
UI will be fixed in another bug.
MozReview-Commit-ID: L9gDbgtUMz5
--- a/docs/mozreview/commits.rst
+++ b/docs/mozreview/commits.rst
@@ -158,23 +158,22 @@ The `test corpus <https://dxr.mozilla.or
demonstrates the abilities of reviewer parsing.
When commits are pushed for review, the server will parse the commit
message and assign reviewers as requested. This should *just work*.
.. important::
``r=`` for specifying reviewers, while supported, is not recommended
- and may result in a warning when submitting review requests.
+ and may result in a warning when submitting review requests. You should
+ instead use ``r=`` after review has been granted.
- This is because ``r=`` is the syntax for denoting that review has
- been *granted*. Adding ``r=`` before review has been granted is
- effectively lying. MozReview doesn't want to encourage this practice,
- as it may result in confusion. Instead, the ``r?`` syntax should be
- used to denote that review is pending.
+ ``r?`` will always request review, even if the reviewer has previously
+ granted a ``r+`` which would be carried forward. If you do not wish to
+ request a new review, use ``r=``.
Autoland will automatically rewrite ``r?`` to ``r=`` when landing
commits, so using ``r?`` should be no extra work for you.
Bug References
--------------
Commit messages may reference Bugzila bugs.
--- a/hgext/reviewboard/tests/test-bugzilla-review-flag-cleared.t
+++ b/hgext/reviewboard/tests/test-bugzilla-review-flag-cleared.t
@@ -540,14 +540,88 @@ We should have an r+ flag already set.
component: TestComponent
depends_on: []
platform: All
product: TestProduct
resolution: ''
status: UNCONFIRMED
summary: Second Bug
+Using r? syntax should stop review carry forward
+
+ $ MESSAGE=`hg log -r . --template "{desc}" | sed -e "s/to review/to review; r?reviewer/"`
+ $ hg commit --amend -m "$MESSAGE" > /dev/null
+ $ hg --config bugzilla.username=l3author@example.com --config bugzilla.apikey=${l3key} --config reviewboard.autopublish=true push > /dev/null
+ $ bugzilla dump-bug 2
+ Bug 2:
+ attachments:
+ - attacher: l3author@example.com
+ content_type: text/x-review-board-request
+ data: http://*:$HGPORT1/r/4/diff/#index_header (glob)
+ description: 'MozReview Request: Bug 2 - Modified commit to review; r?reviewer'
+ file_name: reviewboard-4-url.txt
+ flags:
+ - id: 3
+ name: review
+ requestee: reviewer@example.com
+ setter: l3author@example.com
+ status: '?'
+ id: 2
+ is_obsolete: false
+ is_patch: false
+ summary: 'MozReview Request: Bug 2 - Modified commit to review; r?reviewer'
+ blocks: []
+ cc:
+ - reviewer@example.com
+ comments:
+ - author: l3author@example.com
+ id: 7
+ tags: []
+ text: ''
+ - author: l3author@example.com
+ id: 8
+ tags: []
+ text:
+ - Created attachment 2
+ - 'MozReview Request: Bug 2 - Modified commit to review; r?reviewer'
+ - ''
+ - 'Review commit: http://*:$HGPORT1/r/4/diff/#index_header' (glob)
+ - 'See other reviews: http://*:$HGPORT1/r/4/' (glob)
+ - author: reviewer@example.com
+ id: 9
+ tags: []
+ text:
+ - Comment on attachment 2
+ - 'MozReview Request: Bug 2 - Modified commit to review; r?reviewer'
+ - ''
+ - http://*:$HGPORT1/r/4/#review4 (glob)
+ - ''
+ - LGTM
+ - author: l3author@example.com
+ id: 10
+ tags: []
+ text:
+ - Comment on attachment 2
+ - 'MozReview Request: Bug 2 - Modified commit to review; r?reviewer'
+ - ''
+ - 'Review request updated; see interdiff: http://*:$HGPORT1/r/4/diff/1-2/' (glob)
+ - author: l3author@example.com
+ id: 11
+ tags: []
+ text:
+ - Comment on attachment 2
+ - 'MozReview Request: Bug 2 - Modified commit to review; r?reviewer'
+ - ''
+ - 'Review request updated; see interdiff: http://*:$HGPORT1/r/4/diff/2-3/' (glob)
+ component: TestComponent
+ depends_on: []
+ platform: All
+ product: TestProduct
+ resolution: ''
+ status: UNCONFIRMED
+ summary: Second Bug
+
$ cd ..
Cleanup
$ mozreview stop
stopped 9 containers
--- a/hgext/reviewboard/tests/test-review-request-approval.t
+++ b/hgext/reviewboard/tests/test-review-request-approval.t
@@ -191,16 +191,17 @@ Have an L3 user provide a ship it review
- id: 2
public: true
ship_it: true
body_top: Ship-it!
body_top_text_type: plain
diff_comments: []
Posting a new review without ship it should cancel the previous approval
+
$ rbmanage create-review 2 --body-top "Don't Land it!" --public
created review 3
$ rbmanage dumpreview 2
id: 2
status: pending
public: true
bugs:
- '1'
@@ -899,14 +900,94 @@ ship-its. Posting a new diff should not
reviews:
- id: 7
public: true
ship_it: true
body_top: Ship-it!
body_top_text_type: plain
diff_comments: []
+Using r? syntax should stop approval carry forward
+
+ $ MESSAGE=`hg log -r . --template "{desc}" | sed -e "s/to review/to review; r?level1a/"`
+ $ hg commit --amend -m "$MESSAGE" > /dev/null
+ $ export SSH_KEYNAME=l3@example.com
+ $ hg --config bugzilla.username=l3@example.com --config bugzilla.apikey=${l3apikey} --config reviewboard.autopublish=true push > /dev/null
+ $ rbmanage dumpreview 4
+ id: 4
+ status: pending
+ public: true
+ bugs:
+ - '2'
+ commit: null
+ submitter: level3
+ summary: Bug 2 - initial commit to review; r?level1a
+ description:
+ - Bug 2 - initial commit to review; r?level1a
+ - ''
+ - 'MozReview-Commit-ID: F63vXs'
+ target_people:
+ - level1a
+ extra_data:
+ calculated_trophies: true
+ commit_extra_data:
+ p2rb: true
+ p2rb.commit_id: 355e0073711503a0226badb08ddaf3fa5376eb10
+ p2rb.first_public_ancestor: 3a9f6899ef84c99841f546030b036d0124a863cf
+ p2rb.identifier: bz://2/mynick
+ p2rb.is_squashed: false
+ diffs:
+ - id: 6
+ revision: 1
+ base_commit_id: 3a9f6899ef84c99841f546030b036d0124a863cf
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo b/foo
+ - '--- a/foo'
+ - +++ b/foo
+ - '@@ -1,1 +1,1 @@'
+ - -foo
+ - +author2
+ - ''
+ - id: 8
+ revision: 2
+ base_commit_id: 3a9f6899ef84c99841f546030b036d0124a863cf
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo b/foo
+ - '--- a/foo'
+ - +++ b/foo
+ - '@@ -1,1 +1,1 @@'
+ - -foo
+ - +modified2
+ - ''
+ - id: 10
+ revision: 3
+ base_commit_id: 3a9f6899ef84c99841f546030b036d0124a863cf
+ name: diff
+ extra: {}
+ patch:
+ - diff --git a/foo b/foo
+ - '--- a/foo'
+ - +++ b/foo
+ - '@@ -1,1 +1,1 @@'
+ - -foo
+ - +modified2
+ - ''
+ approved: false
+ approval_failure: '"Ship-it" not carried forward because of r? in latest commit'
+ review_count: 1
+ reviews:
+ - id: 7
+ public: true
+ ship_it: true
+ body_top: Ship-it!
+ body_top_text_type: plain
+ diff_comments: []
+
$ cd ..
Cleanup
$ mozreview stop
stopped 9 containers
--- a/pylib/mozreview/mozreview/bugzilla/attachments.py
+++ b/pylib/mozreview/mozreview/bugzilla/attachments.py
@@ -1,11 +1,12 @@
from __future__ import unicode_literals
from mozautomation.commitparser import (
+ parse_rquestion_reviewers,
strip_commit_metadata,
)
from mozreview.models import (
BugzillaUserMap,
get_or_create_bugzilla_users,
)
from mozreview.rb_utils import (
@@ -47,30 +48,38 @@ def post_bugzilla_attachment(bugzilla, b
# local database is up to date.
users = get_or_create_bugzilla_users(user_data)
reviewers[users[0].email] = False
last_user = None
relevant_reviews = review_request.get_public_reviews().order_by(
'user', '-timestamp')
+ # We track who reviewers flagged with r? so that we won't carry forward
+ # review in that case. To match behaviour when the review request is
+ # created this needs to be case-insensitive.
+ desc = review_request_draft.description
+ rquestion_reviewers = set(parse_rquestion_reviewers(desc))
+
for review in relevant_reviews:
if review.user == last_user:
# We only care about the most recent review for each
# particular user.
continue
last_user = review.user
# 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:
+ # We ignore previous ship-its if the user specifically used r?
+ # in their commit description.
+ if review.ship_it and last_user.username not in rquestion_reviewers:
reviewers[last_user.email] = True
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():
--- a/pylib/mozreview/mozreview/hooks.py
+++ b/pylib/mozreview/mozreview/hooks.py
@@ -1,27 +1,30 @@
from __future__ import unicode_literals
import logging
from reviewboard.extensions.hooks import ReviewRequestApprovalHook
+from mozautomation.commitparser import parse_rquestion_reviewers
+
from mozreview.extra_data import (
COMMIT_ID_KEY,
fetch_commit_data,
gen_child_rrs,
is_parent,
is_pushed,
)
from mozreview.models import (
get_profile,
)
from mozreview.review_helpers import (
has_valid_shipit,
has_l3_shipit,
+ has_shipit_carryforward,
)
logger = logging.getLogger(__name__)
class MozReviewApprovalHook(ReviewRequestApprovalHook):
"""Calculates landing approval for review requests.
@@ -93,16 +96,22 @@ class MozReviewApprovalHook(ReviewReques
def is_approved_child(self, review_request):
"""Check approval for a child review request"""
if review_request.shipit_count == 0:
return False, 'A suitable reviewer has not given a "Ship It!"'
if review_request.issue_open_count > 0:
return False, 'The review request has open issues.'
+ # We don't carry forward a ship-it if the latest commit has a
+ # r? in the commit summary.
+ r = parse_rquestion_reviewers(review_request.description)
+ if len(list(r)) > 0 and has_shipit_carryforward(review_request):
+ return False, '"Ship-it" not carried forward because of r? in latest commit'
+
# TODO: Add a check that we have executed a try build of some kind.
author_mrp = get_profile(review_request.submitter)
# TODO: Make these "has_..." methods return the set of reviews
# which match the criteria so we can indicate which reviews
# actually gave the permission to land.
if author_mrp.has_scm_ldap_group('scm_level_3'):