Summary

Issue 2766: Web api 'review-requests' not returning draft review requests

Review Request #6328 — Created Sept. 16, 2014 and submitted Sept. 20, 2014, 7:32 a.m.

Information

Owner

rmdone

Repository

Review Board

Branch

master

Bugs

2766

Depends On

Commit

9e9580a...

Reviewers

Groups

reviewboard, students

People

Description

Added check for whether the user is admin in webapi/review_request.py. If so, the _query function for ReviewRequestManager (reviews/managers.py) will query for both public and NON public (draft) review requests, as opposed to just querying for public ones.

Testing Done


Verified that admin accounts see draft reviews through /api/review-requests/?status=all, but normal accounts do not. 
This change only affects the webapi, and not any other module that might use the ReviewRequestManager. Verifiedthat the "All Review Requests" page is unaffected here. 
webapi and reviewboard module unit tests run and passed
Added unit tests for private review request visibility in the reviewboard and the webapi unit tests

Issues

Description	From	Last Updated
Can we call this new parameter show_private?	david	Sept. 16, 2014, 8:29 p.m.
In order to fix the line length issue, you can rewrap this as: queryset = self.model.objects.public( user=request.user, status=status, ...	david	Sept. 16, 2014, 8:31 p.m.
Col: 80 E501 line too long (80 > 79 characters)	reviewbot	Sept. 16, 2014, 8:29 p.m.
Pretty close. We put the closing paren at the end of the last parameter.	david	Sept. 17, 2014, 12:19 a.m.
We usually include the trailing comma on our list and dictionary literals.	SM smacleod	Sept. 20, 2014, 6:56 a.m.
'P' is the default value for status, but to keep consistent can we add it to this call anyways?	SM smacleod	Sept. 20, 2014, 6:56 a.m.

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/managers.py

reviewboard/webapi/resources/review_request.py (Diff revision 1)
Show all issues
```
Col: 80
 E501 line too long (80 > 79 characters)
```

Looks like a good start. Can you link to the bug number in the "Bugs" field?

reviewboard/reviews/managers.py (Diff revision 1)
Show all issues
```
Can we call this new parameter show_private?
```

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

Show all issues

In order to fix the line length issue, you can rewrap this as:

queryset = self.model.objects.public(
    user=request.user,
    status=status,
    ...

rmdone Sept. 16, 2014, 8:31 p.m.

I put the closing bracket on a newline for now.

Change Summary:

Added link to issue ticket in bugs field

Bugs:

+	https://code.google.com/p/reviewboard/issues/detail?id=2766

Change Summary:

Bug number not URL

Bugs:

-	https://code.google.com/p/reviewboard/issues/detail?id=2766
+	2766

Change Summary:

Changed is_admin flag to show_private, fixed line length issue by rewrapping "public" function call as David suggested.

Commit:

-	2ded3fd108d195dba5666aaeba2768e6f34919fc
+	a861facdd55f9944a9f1d5ff2ef118ac8c645753

Diff:

Revision 2 (+11 -5)

Show changes

	reviewboard/reviews/managers.py
	reviewboard/webapi/resources/review_request.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/managers.py

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

Show all issues

Pretty close. We put the closing paren at the end of the last parameter.

Change Summary:

Fixed closing brace positioning so it sits on same line as last parameter.

Commit:

-	a861facdd55f9944a9f1d5ff2ef118ac8c645753
+	64a59537687644520c0c28c3da75b3e0bc21eabe

Diff:

Revision 3 (+10 -5)

Show changes

	reviewboard/reviews/managers.py
	reviewboard/webapi/resources/review_request.py

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/managers.py

Change Summary:

Enhanced the unit tests for reviewboard ReviewRequestManager.public() method to verify that all private review requests are accessible under the correct conditions (the webapi user is admin, the call to .public() sets show_private=True)

Summary:

-	[WIP] Issue 2766: Web api 'review-requests' not returning draft review requests
+	Issue 2766: Web api 'review-requests' not returning draft review requests

Testing Done:

		Verified that admin accounts see draft reviews through /api/review-requests/?status=all, but normal accounts do not.
		This change only affects the webapi, and not any other module that might use the ReviewRequestManager. Verifiedthat the "All Review Requests" page is unaffected here.
~
~		TODO:
	~	webapi and reviewboard module unit tests run and passed
	~	Added unit tests for private review request visibility in the reviewboard and the webapi unit tests
-		- Run webapi unit tests
-		- Add some automated tests for this change in behaviour?

Commit:

-	64a59537687644520c0c28c3da75b3e0bc21eabe
+	71e61acc3ee682161d8625ae4ef6fd7405f972cd

Diff:

Revision 4 (+27 -5)

Show changes

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

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/tests/test_review_request.py
    reviewboard/reviews/tests.py
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/tests/test_review_request.py
    reviewboard/reviews/tests.py
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/managers.py

This is looking good! Just a couple of style nits.

reviewboard/reviews/tests.py (Diff revision 4)

Show all issues

We usually include the trailing comma on our list and dictionary literals.

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

Show all issues

'P' is the default value for status, but to keep consistent can we add it to this call anyways?

Change Summary:

Cleaned up some coding style issues.

Commit:

-	71e61acc3ee682161d8625ae4ef6fd7405f972cd
+	9e9580a2082ce8f02eb0cb30cd05a9895f2960c1

Diff:

Revision 5 (+28 -6)

Show changes

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

Tool: Pyflakes
Processed Files:
    reviewboard/webapi/tests/test_review_request.py
    reviewboard/reviews/tests.py
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/managers.py



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/webapi/tests/test_review_request.py
    reviewboard/reviews/tests.py
    reviewboard/webapi/resources/review_request.py
    reviewboard/reviews/managers.py

What is the difference between filter_private and show_private in def _query?

rmdone Sept. 20, 2014, 7:13 a.m.

As we discussed...
filter_private controls the visibilty for queries coming from NOT the currently logged in user (or an unauthenticated queries all together)
show_private controls whether review requests with Public=False are returned

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (28dbc4a)