mozreview: default review flag to the last known status (bug 1197879); r?mcote r?smacleod draft
authorMauro Doglio <mdoglio@mozilla.com>
Wed, 13 Apr 2016 16:58:33 +0100
changeset 8124 d89d4f303e8dd5d424b5954bb16e7e4feef163b3
parent 8123 05d98e90e38c46a40e56e9ea8326571f9251839c
push id830
push usermdoglio@mozilla.com
push dateTue, 17 May 2016 10:03:33 +0000
reviewersmcote, smacleod
bugs1197879
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
hgext/reviewboard/tests/test-bugzilla-review-interaction.t
hgext/reviewboard/tests/test-review-request-approval.t
hgext/reviewboard/tests/test-reviewer-flags.t
pylib/mozreview/mozreview/extension.py
pylib/mozreview/mozreview/fields.py
pylib/mozreview/mozreview/review_helpers.py
pylib/mozreview/mozreview/signal_handlers.py
pylib/mozreview/mozreview/static/mozreview/js/review.js
pylib/mozreview/mozreview/static/mozreview/js/review_flag.js
pylib/mozreview/mozreview/templates/mozreview/user_review_flag.html
pylib/mozreview/mozreview/templatetags/mozreview.py
--- 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()