Let users with submit_as permission list non-public review requests.

Review Request #7196 — Created April 11, 2015 and submitted

Information

Review Board
master
0babcc6...

Reviewers

Until now, rbt post with the submit-as option could not update unpublished review requests. I've proposed that users with the can_submit_as_another_user permission are allowed to list unpublished review requests, so that rbt post knows about these and can update them. Previously only superusers could list unpublished review requests.

  • Expanded unit test to test that can_submit_as_another_user gives user access to unpublished review requests
  • Tested with rbt post -u --submit-as=submitter to update unpublished review request (see also #7195)
Description From Last Updated

This will need to take the local_site parameter as well.

chipx86chipx86

There is a possibility that the url will have show-all-unpublished=0, so this should probably use request.GET.get('show-all-unpublished', False) instead.

brenniebrennie

We actually should probably coerce this to an integer. If show-all-unpublished=0 is given in the URL, then request_unpublished will be …

brenniebrennie

Can we assign request.GET.get('show-all-unpublished, '0') to a variable (e.g. show_all_unpublished) before doing the in operation so that we don't need …

brenniebrennie

Can you pull this out into a separate test method?

daviddavid
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. 
      
chipx86
  1. <p>Seems reasonable. Thanks for the patch! I just have one comment.</p>

  2. Show all issues

    This will need to take the local_site parameter as well.

  3. 
      
HA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. 
      
brennie
  1. 
      
  2. Show all issues

    There is a possibility that the url will have show-all-unpublished=0, so this should probably use request.GET.get('show-all-unpublished', False) instead.

  3. 
      
HA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/webapi/tests/test_review_request.py (Diff revision 3)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can you pull this out into a separate test method?

    1. I chose to extract also a few more lines testing show-all-unpublished for an admin user. This collects all tests of the show-all-unpublished in one test method, which I thought was a good idea. Hope you agree.

  3. 
      
brennie
  1. 
      
  2. Show all issues

    We actually should probably coerce this to an integer. If show-all-unpublished=0 is given in the URL, then request_unpublished will be 0, which is truthy in Python.

    If we cannot coerce to an int, we can probably safely assume it is false.

    1. Inspired by how the ship-it parameter was treated, I chose to treat values '0', 'false' and 'False' as False, and all others as True. The documentation for show-all-unpublished says that unpublished requests should be returned if the parameter 'is set'.

    2. Alrighty, that seems reasonable.

  3. 
      
HA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. 
      
brennie
  1. 
      
  2. reviewboard/webapi/resources/review_request.py (Diff revisions 3 - 4)
     
     
     
    Show all issues

    Can we assign request.GET.get('show-all-unpublished, '0') to a variable (e.g. show_all_unpublished) before doing the in operation so that we don't need the backslash?

    1. Good point. Fixed.

  3. 
      
HA
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/webapi/resources/review_request.py
        reviewboard/webapi/tests/test_review_request.py
    
    
  2. 
      
HA
Review request changed
Status:
Completed
Change Summary:
Pushed to master (af5fa9e)