Revokable ship it labels

Review Request #7170 — Created April 5, 2015 and discarded

Information

Review Board
master

Reviewers

Added removable ship it labels such that a ship it label can be revoked only by the person who endorsed it in the first place. To revoke a ship it, click
on the X to the right of a ship it label. Ship its can be revoked and the changes seen immediately. Furthermore, the ship-it counter corrresponding to the review request is duly updated(decremented) when a ship it is revoked.

Manual testing using different users and reviews, and checking if appropriate permissions are met.
Manual tests for ship its being revoked on click, and asynchronously. Furthermore, any checks to see if
review request is up to date returns true.
Unit tests for if ship it counter is being updated appropriately.

Description From Last Updated

Col: 13 E115 expected an indented block (comment)

reviewbotreviewbot

Missing one empty line

XU xuanyi

How about just _revokeShipIt ?

brenniebrennie

Missing one space between the key and value

XU xuanyi

Missing one empty line

XU xuanyi

How about _removeShipItLabel ?

brenniebrennie

We should also be able to remove Ship-Its from "Fix it, then Ship it" reviews.

brenniebrennie

The import should be in alphabetical order

XU xuanyi

This needs to be updated to reflect the fact that the review can be updated to revoke the ship-it ness …

brenniebrennie

Why this logic? By the time we get here, we've already determined that the user has to have modify permissions …

brenniebrennie

Col: 13 E115 expected an indented block (comment)

reviewbotreviewbot

What if review.ship_it = False prior to assining to it? I believe this can result in incorrect ship-it counters. As …

brenniebrennie

Left over from debugging?

brenniebrennie

Col: 80 E501 line too long (81 > 79 characters)

reviewbotreviewbot

Col: 80 E501 line too long (86 > 79 characters)

reviewbotreviewbot

Col: 9 E115 expected an indented block (comment)

reviewbotreviewbot

Col: 61 E231 missing whitespace after ','

reviewbotreviewbot

Col: 80 E501 line too long (84 > 79 characters)

reviewbotreviewbot

Col: 38 E127 continuation line over-indented for visual indent

reviewbotreviewbot

Col: 15 E225 missing whitespace around operator

reviewbotreviewbot

Col: 15 E225 missing whitespace around operator

reviewbotreviewbot

Col: 15 E225 missing whitespace around operator

reviewbotreviewbot
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/base_review.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/base_review.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxView.js
    
    
  2. Col: 13
     E115 expected an indented block (comment)
    
  3. 
      
RM
RM
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/base_review.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxView.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/base_review.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxView.js
    
    
  2. Col: 13
     E115 expected an indented block (comment)
    
  3. 
      
XU
  1. 
      
  2. Missing one empty line

  3. Missing one space between the key and value

  4. Missing one empty line

  5. The import should be in alphabetical order

  6. 
      
brennie
  1. 
      
  2. How about just _revokeShipIt ?

  3. How about _removeShipItLabel ?

  4. We should also be able to remove Ship-Its from "Fix it, then Ship it" reviews.

  5. reviewboard/webapi/resources/base_review.py (Diff revision 2)
     
     
     
     

    This needs to be updated to reflect the fact that the review can be updated to revoke the ship-it ness of the review.

  6. reviewboard/webapi/resources/base_review.py (Diff revision 2)
     
     
     

    Why this logic?
    By the time we get here, we've already determined that the user has to have modify permissions on the review.

    Also, this makes the ship_it key mandatory for updating the review. I don't think that is necessary.

  7. reviewboard/webapi/resources/base_review.py (Diff revision 2)
     
     
     
     
     
     
     
     
     
     

    What if review.ship_it = False prior to assining to it? I believe this can result in incorrect ship-it counters.

    As I understand it, the ReviewResource (which inherits from BaseReviewResource) is also used for review drafts (the ReviewDraftResource is just a redirection to it). Won't toggling the shipit-ness of a draft review end up decrementing the ship-it count?

  8. Left over from debugging?

  9. 
      
chipx86
  1. Make sure the students group is added! :)

  2. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/tests/test_review.py
        reviewboard/webapi/resources/base_review.py
        reviewboard/webapi/tests/test_review_request.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/tests/test_review.py
        reviewboard/webapi/resources/base_review.py
        reviewboard/webapi/tests/test_review_request.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxView.js
    
    
  2. Col: 80
     E501 line too long (81 > 79 characters)
    
  3. Col: 80
     E501 line too long (86 > 79 characters)
    
  4. Col: 9
     E115 expected an indented block (comment)
    
  5. Col: 61
     E231 missing whitespace after ','
    
  6. Col: 80
     E501 line too long (84 > 79 characters)
    
  7. reviewboard/webapi/tests/test_review.py (Diff revision 3)
     
     
    Col: 38
     E127 continuation line over-indented for visual indent
    
  8. Col: 15
     E225 missing whitespace around operator
    
  9. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/tests/test_review.py
        reviewboard/webapi/resources/base_review.py
        reviewboard/webapi/tests/test_review_request.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/tests/test_review.py
        reviewboard/webapi/resources/base_review.py
        reviewboard/webapi/tests/test_review_request.py
    
    Ignored Files:
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/js/views/reviewBoxView.js
    
    
  2. Col: 15
     E225 missing whitespace around operator
    
  3. 
      
RM
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/tests/test_review.py
        reviewboard/webapi/resources/base_review.py
        reviewboard/webapi/tests/test_review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/tests/test_review.py
        reviewboard/webapi/resources/base_review.py
        reviewboard/webapi/tests/test_review_request.py
    
    Ignored Files:
        reviewboard/static/rb/js/views/reviewBoxView.js
        reviewboard/templates/reviews/boxes/review.html
        reviewboard/static/rb/css/pages/reviews.less
    
    
  2. Col: 15
     E225 missing whitespace around operator
    
  3. 
      
RM
RM
david
Review request changed

Status: Discarded

Loading...