Summary

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

Review Request #7196 — Created April 11, 2015 and submitted April 15, 2015, 3:24 p.m.

Information

Owner

halvorlu

Repository

Review Board

Branch

master

Bugs

Depends On

Blocks

7195

Commit

0babcc6...

Reviewers

Groups

reviewboard

People

Description

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.

Testing Done


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)

Issues

Description	From	Last Updated
This will need to take the local_site parameter as well.	chipx86	April 14, 2015, 9:38 a.m.
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.	brennie	April 14, 2015, 1:52 p.m.
We actually should probably coerce this to an integer. If show-all-unpublished=0 is given in the URL, then request_unpublished will be …	brennie	April 15, 2015, 11:54 a.m.
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 …	brennie	April 15, 2015, 12:08 p.m.
Can you pull this out into a separate test method?	david	April 15, 2015, 11:53 a.m.

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

<p>Seems reasonable. Thanks for the patch! I just have one comment.</p>

reviewboard/webapi/resources/review_request.py (Diff revision 1)
Show all issues
```
This will need to take the local_site parameter as well.
```

Change Summary:

Added local_site as argument to has_perm.

Commit:

-	be41b96761c538fd29a5414cfd5d57d7e23f4d35
+	c831c642aa91703a48c1e39f2d62e82bb08a6388

Diff:

Revision 2 (+28 -15)

Show changes

	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/tests/test_review_request.py

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

reviewboard/webapi/resources/review_request.py (Diff revision 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.

Change Summary:

Don't show unpublished reivew requests if show-all-unpublished is (e.g.) 0.

Commit:

-	c831c642aa91703a48c1e39f2d62e82bb08a6388
+	1b401f7654f92967645c00c52850df08200be87f

Diff:

Revision 3 (+30 -15)

Show changes

	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/tests/test_review_request.py

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

reviewboard/webapi/tests/test_review_request.py (Diff revision 3)

Show all issues

Can you pull this out into a separate test method?

halvorlu April 15, 2015, 11:58 a.m.

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.

reviewboard/webapi/resources/review_request.py (Diff revision 3)

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.

halvorlu April 15, 2015, 11:58 a.m.

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'.

brennie April 15, 2015, 12:01 p.m.
```
Alrighty, that seems reasonable.
```

Commit:

-	1b401f7654f92967645c00c52850df08200be87f
+	3c98714ebac8c6051e1eb9f91451364c4b85557f

Diff:

Revision 4 (+44 -16)

Show changes

	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/tests/test_review_request.py

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

Ship it!

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?

HA

halvorlu April 15, 2015, 12:08 p.m.
```
Good point. Fixed.
```

Commit:

-	3c98714ebac8c6051e1eb9f91451364c4b85557f
+	0babcc6a5574e1b9a0703fac845a5b5c3c07642f

Diff:

Revision 5 (+44 -16)

Show changes

	reviewboard/webapi/resources/review_request.py
	reviewboard/webapi/tests/test_review_request.py

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

Status: Closed (submitted)

Change Summary:

Pushed to master (af5fa9e)