mozreview: default review flag to the last known status (
bug 1197879); r?mcote r?smacleod
When the user opens up the review dialog for the first time they will see the review flag
set to either 'r?' or ' ', depending on whether they were designated by the reviewee or not
(drive by reviewer). Once they leave a first review, the dialog will remember the last
choice and comment-only reviews will not mutate the review flag state,.
MozReview-Commit-ID: 6nzYJgLgEXI
--- a/hgext/reviewboard/tests/test-bugzilla-review-interaction.t
+++ b/hgext/reviewboard/tests/test-bugzilla-review-interaction.t
@@ -790,19 +790,19 @@ Verify that a single r+ affects only tha
summary: Parent Reviews
r+ reviews are not allowed on the parent.
$ rbmanage create-review 9 --body-top 'all good!' --public --review-flag='r+'
API Error: 500: 225: Error publishing: "Ship it" reviews on parent review requests are not allowed. Please review individual commits.
[1]
-A non 'clear flag' review shouldn't clear the attachment's review flag.
+A comment-only review shouldn't change the review flag.
- $ rbmanage create-review 11 --body-top 'there is a problem' --public
+ $ rbmanage create-review 11 --body-top 'I have another comment' --public
created review 7
$ bugzilla dump-bug 5
Bug 5:
attachments:
- attacher: author@example.com
content_type: text/x-review-board-request
data: http://$DOCKER_HOSTNAME:$HGPORT1/r/10/diff/#index_header
@@ -831,19 +831,19 @@ A non 'clear flag' review shouldn't clea
flags:
- id: 10
name: review
requestee: reviewer2@example.com
setter: author@example.com
status: '?'
- id: 11
name: review
- requestee: reviewer@example.com
+ requestee: null
setter: reviewer@example.com
- status: '?'
+ status: +
id: 6
is_obsolete: false
is_patch: false
summary: 'MozReview Request: Bug 5 - Parent reviews, second commit'
- attacher: author@example.com
content_type: text/x-review-board-request
data: http://$DOCKER_HOSTNAME:$HGPORT1/r/12/diff/#index_header
description: 'MozReview Request: Bug 5 - Parent reviews, third commit'
@@ -908,22 +908,19 @@ A non 'clear flag' review shouldn't clea
- ''
- http://$DOCKER_HOSTNAME:$HGPORT1/r/11/#review5
- ''
- land it!
- author: reviewer@example.com
id: 18
tags: []
text:
- - Comment on attachment 6
- - 'MozReview Request: Bug 5 - Parent reviews, second commit'
- - ''
- http://$DOCKER_HOSTNAME:$HGPORT1/r/11/#review7
- ''
- - there is a problem
+ - I have another comment
component: TestComponent
depends_on: []
platform: All
product: TestProduct
resolution: ''
status: UNCONFIRMED
summary: Parent Reviews
@@ -959,19 +956,19 @@ A 'clear flag' review on a child should
flags:
- id: 10
name: review
requestee: reviewer2@example.com
setter: author@example.com
status: '?'
- id: 11
name: review
- requestee: reviewer@example.com
+ requestee: null
setter: reviewer@example.com
- status: '?'
+ status: +
id: 6
is_obsolete: false
is_patch: false
summary: 'MozReview Request: Bug 5 - Parent reviews, second commit'
- attacher: author@example.com
content_type: text/x-review-board-request
data: http://$DOCKER_HOSTNAME:$HGPORT1/r/12/diff/#index_header
description: 'MozReview Request: Bug 5 - Parent reviews, third commit'
@@ -1036,22 +1033,19 @@ A 'clear flag' review on a child should
- ''
- http://$DOCKER_HOSTNAME:$HGPORT1/r/11/#review5
- ''
- land it!
- author: reviewer@example.com
id: 18
tags: []
text:
- - Comment on attachment 6
- - 'MozReview Request: Bug 5 - Parent reviews, second commit'
- - ''
- http://$DOCKER_HOSTNAME:$HGPORT1/r/11/#review7
- ''
- - there is a problem
+ - I have another comment
- author: reviewer2@example.com
id: 19
tags: []
text:
- Comment on attachment 5
- 'MozReview Request: Bug 5 - Parent reviews'
- ''
- http://$DOCKER_HOSTNAME:$HGPORT1/r/10/#review8
@@ -1097,19 +1091,19 @@ A non-r+ review on a parent should post
flags:
- id: 10
name: review
requestee: reviewer2@example.com
setter: author@example.com
status: '?'
- id: 11
name: review
- requestee: reviewer@example.com
+ requestee: null
setter: reviewer@example.com
- status: '?'
+ status: +
id: 6
is_obsolete: false
is_patch: false
summary: 'MozReview Request: Bug 5 - Parent reviews, second commit'
- attacher: author@example.com
content_type: text/x-review-board-request
data: http://$DOCKER_HOSTNAME:$HGPORT1/r/12/diff/#index_header
description: 'MozReview Request: Bug 5 - Parent reviews, third commit'
@@ -1174,22 +1168,19 @@ A non-r+ review on a parent should post
- ''
- http://$DOCKER_HOSTNAME:$HGPORT1/r/11/#review5
- ''
- land it!
- author: reviewer@example.com
id: 18
tags: []
text:
- - Comment on attachment 6
- - 'MozReview Request: Bug 5 - Parent reviews, second commit'
- - ''
- http://$DOCKER_HOSTNAME:$HGPORT1/r/11/#review7
- ''
- - there is a problem
+ - I have another comment
- author: reviewer2@example.com
id: 19
tags: []
text:
- Comment on attachment 5
- 'MozReview Request: Bug 5 - Parent reviews'
- ''
- http://$DOCKER_HOSTNAME:$HGPORT1/r/10/#review8
--- a/hgext/reviewboard/tests/test-review-request-approval.t
+++ b/hgext/reviewboard/tests/test-review-request-approval.t
@@ -200,17 +200,17 @@ Have an L3 user provide a r+ review whic
ship_it: true
extra_data:
p2rb.review_flag: r+
body_top: Ship-it!
body_top_text_type: plain
diff_comments: []
Posting a new review without r+ should cancel the previous approval
- $ rbmanage create-review 2 --body-top "Don't Land it!" --public
+ $ rbmanage create-review 2 --body-top "Don't Land it!" --public --review-flag='r-'
created review 3
$ rbmanage dumpreview 2
id: 2
status: pending
public: true
bugs:
- '1'
commit: null
@@ -265,17 +265,17 @@ Posting a new review without r+ should c
p2rb.review_flag: r+
body_top: Ship-it!
body_top_text_type: plain
diff_comments: []
- id: 3
public: true
ship_it: false
extra_data:
- p2rb.review_flag: r?
+ p2rb.review_flag: r-
body_top: Don't Land it!
body_top_text_type: plain
diff_comments: []
One more r+ should switch it back to approved
$ rbmanage create-review 2 --body-top "NVM, Ship-it!" --public --review-flag='r+'
created review 4
@@ -337,17 +337,17 @@ One more r+ should switch it back to app
p2rb.review_flag: r+
body_top: Ship-it!
body_top_text_type: plain
diff_comments: []
- id: 3
public: true
ship_it: false
extra_data:
- p2rb.review_flag: r?
+ p2rb.review_flag: r-
body_top: Don't Land it!
body_top_text_type: plain
diff_comments: []
- id: 4
public: true
ship_it: true
extra_data:
p2rb.review_flag: r+
@@ -432,17 +432,17 @@ Even though the author is L1, adding a n
p2rb.review_flag: r+
body_top: Ship-it!
body_top_text_type: plain
diff_comments: []
- id: 3
public: true
ship_it: false
extra_data:
- p2rb.review_flag: r?
+ p2rb.review_flag: r-
body_top: Don't Land it!
body_top_text_type: plain
diff_comments: []
- id: 4
public: true
ship_it: true
extra_data:
p2rb.review_flag: r+
@@ -525,17 +525,17 @@ A new r+ from L3 should give approval
p2rb.review_flag: r+
body_top: Ship-it!
body_top_text_type: plain
diff_comments: []
- id: 3
public: true
ship_it: false
extra_data:
- p2rb.review_flag: r?
+ p2rb.review_flag: r-
body_top: Don't Land it!
body_top_text_type: plain
diff_comments: []
- id: 4
public: true
ship_it: true
extra_data:
p2rb.review_flag: r+
@@ -549,17 +549,17 @@ A new r+ from L3 should give approval
p2rb.review_flag: r+
body_top: Update looks good!
body_top_text_type: plain
diff_comments: []
Opening issues, even from an L1 user, should revoke approval until they're fixed
$ exportbzauth l1b@example.com password
- $ rbmanage create-review 2 --body-top "I found issues"
+ $ rbmanage create-review 2 --body-top "I found issues" --review-flag="r-"
created review 6
$ rbmanage create-diff-comment 2 6 foo 1 "Issue Text" --open-issue
created diff comment 1
$ rbmanage publish-review 2 6
published review 6
$ rbmanage dumpreview 2
id: 2
status: pending
@@ -631,17 +631,17 @@ Opening issues, even from an L1 user, sh
p2rb.review_flag: r+
body_top: Ship-it!
body_top_text_type: plain
diff_comments: []
- id: 3
public: true
ship_it: false
extra_data:
- p2rb.review_flag: r?
+ p2rb.review_flag: r-
body_top: Don't Land it!
body_top_text_type: plain
diff_comments: []
- id: 4
public: true
ship_it: true
extra_data:
p2rb.review_flag: r+
@@ -655,17 +655,17 @@ Opening issues, even from an L1 user, sh
p2rb.review_flag: r+
body_top: Update looks good!
body_top_text_type: plain
diff_comments: []
- id: 6
public: true
ship_it: false
extra_data:
- p2rb.review_flag: r?
+ p2rb.review_flag: r-
body_top: I found issues
body_top_text_type: plain
diff_comments:
- id: 1
public: true
user: level1b
issue_opened: true
issue_status: open
@@ -752,17 +752,17 @@ Fixing the issue should restore approval
p2rb.review_flag: r+
body_top: Ship-it!
body_top_text_type: plain
diff_comments: []
- id: 3
public: true
ship_it: false
extra_data:
- p2rb.review_flag: r?
+ p2rb.review_flag: r-
body_top: Don't Land it!
body_top_text_type: plain
diff_comments: []
- id: 4
public: true
ship_it: true
extra_data:
p2rb.review_flag: r+
@@ -776,17 +776,17 @@ Fixing the issue should restore approval
p2rb.review_flag: r+
body_top: Update looks good!
body_top_text_type: plain
diff_comments: []
- id: 6
public: true
ship_it: false
extra_data:
- p2rb.review_flag: r?
+ p2rb.review_flag: r-
body_top: I found issues
body_top_text_type: plain
diff_comments:
- id: 1
public: true
user: level1b
issue_opened: true
issue_status: resolved
--- a/hgext/reviewboard/tests/test-reviewer-flags.t
+++ b/hgext/reviewboard/tests/test-reviewer-flags.t
@@ -87,17 +87,17 @@ There are no warnings for reviewers who
$ rbmanage list-reviewers 2
cthulhu
There are warnings for reviewers who haved granted a non ship-it review when
using r=.
$ exportbzauth cthulhu@example.com password
- $ rbmanage create-review 2 --body-top "No way you should ship-it!" --public
+ $ rbmanage create-review 2 --body-top "No way you should ship-it!" --public --review-flag='r-'
created review 2
$ exportbzauth default@example.com password
$ echo foo >> foo
$ hg commit --amend -l - << EOF
> bug 1 - even better stuff; r=cthulhu
>
> MozReview-Commit-ID: APOgLo
--- a/pylib/mozreview/mozreview/extension.py
+++ b/pylib/mozreview/mozreview/extension.py
@@ -274,16 +274,18 @@ class MozReviewExtension(Extension):
TemplateHook(self, 'before-login-form',
'mozreview/before-login-form.html', apply_to=['login'])
TemplateHook(self, 'after-login-form',
'mozreview/after-login-form.html', apply_to=['login'])
TemplateHook(self, 'base-after-content',
'mozreview/scm_level.html')
TemplateHook(self, 'base-after-content',
'mozreview/repository.html')
+ TemplateHook(self, 'base-after-content',
+ 'mozreview/user_review_flag.html')
ReviewRequestFieldsHook(self, 'main', [CommitsListField])
# This forces the Commits field to be the top item.
main_fieldset.field_classes.insert(0,
main_fieldset.field_classes.pop())
# The above hack forced Commits at the top, but the rest of these
# fields are fine below the Description.
--- a/pylib/mozreview/mozreview/fields.py
+++ b/pylib/mozreview/mozreview/fields.py
@@ -133,16 +133,17 @@ class CommitsListField(CommitDataBackedF
repo_urls.add(request.repository_url)
latest_autoland_requests.append(request)
return get_template('mozreview/commits.html').render(Context({
'review_request_details': self.review_request_details,
'parent_details': parent_details,
'children_details': children_details,
'latest_autoland_requests': latest_autoland_requests,
+ 'user': user
}))
class ImportCommitField(BaseReviewRequestField):
"""This field provides some information on how to pull down the commit"""
# RB validation requires this to be unique, so we fake a field id
field_id = "p2rb.ImportCommitField"
label = _("Import")
--- a/pylib/mozreview/mozreview/review_helpers.py
+++ b/pylib/mozreview/mozreview/review_helpers.py
@@ -72,37 +72,51 @@ def has_l3_shipit(review_request):
if not review.ship_it:
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."""
+def get_reviewers_status(review_request, reviewers=None, include_drive_by=False):
+ """Returns the latest review status for each reviewer.
+
+ If include_drive_by is False, only reviewers nominated by the reviewee are
+ considered. Set it to True to also report the status of `drive_by`
+ reviewers.
+
+ If a list of reviewers is provided, the returned dictionary will contain
+ those reviewers regardless the value of include_drive_by
+ """
# Don't show any status on drafts.
if type(review_request) == ReviewRequestDraft:
return {}
+ designated_reviewers = review_request.target_people.all()
if not reviewers:
- reviewers = review_request.target_people.all()
+ reviewers = designated_reviewers
+
reviewers_status = dict()
for r in reviewers:
# The initial state is r?
reviewers_status[r.username] = {
'ship_it': False,
- 'review_flag': 'r?',
+ 'review_flag': 'r?' if r in designated_reviewers else ' ',
}
for review in gen_latest_reviews(review_request):
review_flag = review.extra_data.get(REVIEW_FLAG_KEY)
user = review.user.username
+
+ if (user not in reviewers_status) and include_drive_by:
+ reviewers_status[user] = {}
+
if user in reviewers_status:
reviewers_status[user]['ship_it'] = review.ship_it
if review_flag:
reviewers_status[user]['review_flag'] = review_flag
return reviewers_status
--- a/pylib/mozreview/mozreview/signal_handlers.py
+++ b/pylib/mozreview/mozreview/signal_handlers.py
@@ -71,16 +71,19 @@ from mozreview.messages import (
from mozreview.models import (
CommitData,
DiffSetVerification,
get_bugzilla_api_key,
)
from mozreview.rb_utils import (
get_obj_url,
)
+from mozreview.review_helpers import (
+ get_reviewers_status,
+)
from mozreview.signals import (
commit_request_publishing,
)
logger = logging.getLogger(__name__)
@@ -554,20 +557,30 @@ def _close_child_review_requests(user, r
commit_data.extra_data[DISCARD_ON_PUBLISH_KEY] = '[]'
commit_data.save(update_fields=['extra_data'])
def pre_save_review(sender, *args, **kwargs):
"""Handle pre_save for a Review.
This is needed to give a default value to the REVIEW_FLAG_KEY
- extra_data key.
+ extra_data key. It tries to retrieve the last known review status,
+ falling back to r? if no status is found.
"""
review = kwargs["instance"]
+ if review.pk:
+ # The review create endpoint calls save twice: the first time it
+ # gets or creates the review and the second time it updates the
+ # object retrieved/created. This condition let us execute the code
+ # below only once.
- if (REVIEW_FLAG_KEY not in review.extra_data and
- not is_parent(review.review_request)):
- if review.pk:
- # The review create endpoint calls save twice: the first time it
- # gets or creates the review and the second time it updates the
- # object retrieved/created. This condition let us execute the code
- # below only when all the fields are set.
- review.extra_data[REVIEW_FLAG_KEY] = 'r?'
+ if not is_parent(review.review_request):
+
+ if REVIEW_FLAG_KEY not in review.extra_data:
+ # TODO: we should use a different query than going through
+ # 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+')
--- a/pylib/mozreview/mozreview/static/mozreview/js/review.js
+++ b/pylib/mozreview/mozreview/static/mozreview/js/review.js
@@ -26,17 +26,16 @@
var reviewRequest = RB.PageManager.getPage().reviewRequest;
RB.apiCall({
type: 'GET',
prefix: reviewRequest.get('sitePrefix'),
noActivityIndicator: true,
url: '/api/review-requests/'+reviewRequest.get('id')+'/reviews/'
+ '?max-results=200',
success: function(data) {
- console.log(data);
_.forEach(data.reviews, function(item) {
var flag = item.extra_data['p2rb.review_flag'];
var flagDesc = '';
var reviewText = $('#review'+item.id+' .body');
switch(flag){
case ' ':
flagDesc = 'Review flag cleared';
break;
--- a/pylib/mozreview/mozreview/static/mozreview/js/review_flag.js
+++ b/pylib/mozreview/mozreview/static/mozreview/js/review_flag.js
@@ -56,32 +56,38 @@ MRReviewFlag.View = Backbone.View.extend
this.model.save({
success: _.bind(function(){
$('#review-form-comments .body-top a.add-link').click();
}, this)
});
},
render: function() {
+ var lastKnownFlag = ' ';
+ var userReviewFlag = $('#user-review-flag');
+
+ if (userReviewFlag.length == 1) {
+ lastKnownFlag = $('#user-review-flag').data('reviewer-flag');
+ }
+
this.$el.html(this.template({
states: this.states,
- val: this.model.get('extraData')[this.key] || 'r?'
+ val: this.model.get('extraData')[this.key] || lastKnownFlag
}));
return this;
},
updateReviewState: function() {
var val = this.$el.find('#mr-review-flag').val();
// We have to send the 'clear the flag' as a non empty string because
// the api endpoint will ignore it otherwise.
if (val === '') {
val = ' ';
}
this.model.get('extraData')[this.key] = val;
- $('#id_shipit').prop('checked', (val == 'r+'));
this.save()
}
});
MRReviewFlag.Extension = RB.Extension.extend({
initialize: function() {
_super(this).initialize.call(this);
new file mode 100644
--- /dev/null
+++ b/pylib/mozreview/mozreview/templates/mozreview/user_review_flag.html
@@ -0,0 +1,6 @@
+{% load mozreview %}
+{% if user.is_authenticated %}
+ {% with status=review_request_details.get_review_request|reviewer_status_with_drive_by:user %}
+ <div id="user-review-flag" data-reviewer-flag="{{status.review_flag}}"></div>
+ {% endwith %}
+{% endif %}
--- a/pylib/mozreview/mozreview/templatetags/mozreview.py
+++ b/pylib/mozreview/mozreview/templatetags/mozreview.py
@@ -108,16 +108,23 @@ def ssh_to_https(landing_url):
@register.filter()
def reviewers_status(review_request):
return get_reviewers_status(review_request).items()
@register.filter()
+def reviewer_status_with_drive_by(review_request, reviewer):
+ reviewer_status = get_reviewers_status(review_request, reviewers=[reviewer],
+ include_drive_by=True)
+ return reviewer_status[reviewer.username]
+
+
+@register.filter()
def userid_to_user(user_id):
try:
return User.objects.get(pk=user_id)
except User.DoesNotExist:
return 'Unknown user'
@register.filter()