Add top-level reviews API endpoint
Review Request #12297 — Created May 23, 2022 and submitted
Information | |
---|---|
maubin | |
Review Board | |
release-5.0.x | |
|
|
Reviewers | |
reviewboard | |
Review resources are currently child resources of review requests. This limits
the type of queries we can do for reviews, for example we can't query for all
reviews created by a given user.This change adds a top-level API endpoint for reviews. We register a root list
resource for reviews and support GET requests with the following query
parameters:
-user=<username>
-repository=<repoName>
-last-updated-from
andlast-updated-to
-review-group=<groupName>
Based on work by Taylor Christie at /r/12028.
- Created unit tests for the root review resource and ran successfully
Summary | Author |
---|---|
Michelle | |
Michelle |
Description | From | Last Updated |
---|---|---|
I haven't dug into the unit testing on this much yet, but making sure we have this on the checklist … |
|
|
The review_request api endpoint filters by the ID of the repository instead of the name. Should we filter by ID … |
![]() |
|
It might be worthwhile to add fields for "time-added-to" and "time-added-from" just so that there's more options for date filtering. … |
![]() |
|
this doesn't actually add the user to the local site, is there another way that i should be adding the … |
![]() |
|
W292 no newline at end of file |
![]() |
|
W292 no newline at end of file |
![]() |
|
Can you order these alphabetically? |
|
|
Small nit, but for consistency, this is usually placed after the main resource definitions (so after added_in in this case). |
|
|
Can you order these alphabetically? |
|
|
We should probably stick with public instead of drafts for this, and take it as a boolean. |
|
|
We can use &= for these. |
|
|
We use a chaining format to keep these readable, like: users = ( Group.objects .filter(name=...) .values_list('users') ) |
|
|
Can you update this to do one argument per line? That'll help keep this readable/maintainable going forward. |
|
|
This should document *args and **kwargs. |
|
|
Can you order these keys alphabetically? |
|
|
Since we use a ' in the docstring, and don't use " anywhere, we can use " for the string … |
|
|
This should document the arguments and AssertionError. |
|
|
For each of these tests, we'll want to asser that we get exactly the reviews we expect, in the order … |
|
|
Can you update these to use user instead of u? Same elsewhere in tests. This helps keep things consistent and … |
|
|
Similar, review_request instead of rr. |
|
|
get_tz_aware_utcnow() is a bit legacy. We should be able to use Django's timezone.now() instead. |
|
|
We're going to want to avoid using _. I have some details on this in the other API change. |
|
|
Similarly, we should use review. |
|
|
line too long (82 > 79 characters) Column: 80 Error code: E501 |
![]() |
|
line too long (83 > 79 characters) Column: 80 Error code: E501 |
![]() |
|
Add a blank line between these two. |
|
|
Let's fix this to just say "Unit tests for RootReviewResource" |
|
|
An optimization I'll be working toward is to avoid these JOINs where possible, and since this code is new, we … |
|
|
This will need to be wrapped in list(...) in order to get the values directly. Otherwise this ends up being … |
|
|
The *args is off. Missing Args:. |
|
|
This test should have some public and non-public reviews that aren't accessible by the user, and ensure they don't show … |
|
|
This test should add a mixture of accessible/inaccessible review requests and accessible/inaccessible reviews, and non-public reviews on accessible review requests, … |
|
|
This test should add a mixture of accessible/inaccessible review requests and accessible/inaccessible reviews, and public reviews on accessible review requests, … |
|
|
This should have some inaccessible repositories and make sure reviews on them don't show up in the results. Same for … |
|
|
Same comments as above but for review groups. |
|

-
-
reviewboard/webapi/resources/root_review.py (Diff revision 1) The review_request api endpoint filters by the ID of the repository instead of the name. Should we filter by ID here too for consistency?
-
reviewboard/webapi/resources/root_review.py (Diff revision 1) It might be worthwhile to add fields for "time-added-to" and "time-added-from" just so that there's more options for date filtering. Should we add these fields?
-
-
I haven't dug into the unit testing on this much yet, but making sure we have this on the checklist for this feature...
One thing to make sure of is that we have very extensive test coverage of access controls, making sure that it won't return reviews if:
- It's unpublished and you are not the owner
- It's on a private repository you don't have access to
- It's on an invite-only review group you don't have access to
- It's on a Local Site you don't have access to

Commits: |
|
||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 2 (+1172 -38) |
Checks run (1 failed, 1 succeeded)
flake8

-
-
reviewboard/webapi/tests/test_root_review.py (Diff revision 2) this doesn't actually add the user to the local site, is there another way that i should be adding the user to the local site?

Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Depends On: |
|
|||||||||||||||
Diff: |
Revision 3 (+1179 -39) |
Checks run (1 failed, 1 succeeded)
flake8

Change Summary:
Ready for review
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 4 (+1183 -39) |
Checks run (2 succeeded)

Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 5 (+1171 -39) |
Checks run (2 succeeded)

Change Summary:
small styling changes
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 6 (+1175 -41) |
Checks run (2 succeeded)
-
Many of my comments are similar to the comments API change. There's more context in there for some of the issues opened here.
-
-
reviewboard/webapi/resources/root_review.py (Diff revision 6) Small nit, but for consistency, this is usually placed after the main resource definitions (so after
added_in
in this case). -
-
reviewboard/webapi/resources/root_review.py (Diff revision 6) We should probably stick with
public
instead ofdrafts
for this, and take it as a boolean. -
-
reviewboard/webapi/resources/root_review.py (Diff revision 6) We use a chaining format to keep these readable, like:
users = ( Group.objects .filter(name=...) .values_list('users') )
-
reviewboard/webapi/resources/root_review.py (Diff revision 6) Can you update this to do one argument per line? That'll help keep this readable/maintainable going forward.
-
reviewboard/webapi/resources/root_review.py (Diff revision 6) This should document
*args
and**kwargs
. -
reviewboard/webapi/resources/root_review.py (Diff revision 6) Can you order these keys alphabetically?
-
reviewboard/webapi/resources/root_review.py (Diff revision 6) Since we use a
'
in the docstring, and don't use"
anywhere, we can use"
for the string itself and avoid escaping the'
. -
reviewboard/webapi/tests/test_root_review.py (Diff revision 6) This should document the arguments and
AssertionError
. -
reviewboard/webapi/tests/test_root_review.py (Diff revision 6) For each of these tests, we'll want to asser that we get exactly the reviews we expect, in the order in which we expect them. You can use
compare_item
to help with this.This applies to all tests.
-
reviewboard/webapi/tests/test_root_review.py (Diff revision 6) Can you update these to use
user
instead ofu
? Same elsewhere in tests. This helps keep things consistent and greppable. -
reviewboard/webapi/tests/test_root_review.py (Diff revision 6) Similar,
review_request
instead ofrr
. -
reviewboard/webapi/tests/test_root_review.py (Diff revision 6) get_tz_aware_utcnow()
is a bit legacy. We should be able to use Django'stimezone.now()
instead. -
reviewboard/webapi/tests/test_root_review.py (Diff revision 6) We're going to want to avoid using
_
. I have some details on this in the other API change. -

Change Summary:
- Changed the
drafts
request field topublic
. Also changed the logic behind this field, previously the endpoint would return only public reviews by default. Now the default is to return both public and draft reviews. - Changed
time-added-from
andtime-added-to
request fields back tolast-updated-from
andlast-updated-to
sincereview.timestamp
can be updated, its not necessarily the time it was added - Switched
q = q &
toq &=
- Stopped using
_
for unused variables - Uses
timezone.now()
instead ofget_tz_aware_utcnow()
- Uses
compare_item
in mostly all tests - Uses consistent variable names in tests
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 7 (+1342 -130) |
Checks run (1 failed, 1 succeeded)
flake8
-
reviewboard/webapi/tests/test_root_review.py (Diff revision 7) line too long (82 > 79 characters) Column: 80 Error code: E501
-
reviewboard/webapi/tests/test_root_review.py (Diff revision 7) line too long (83 > 79 characters) Column: 80 Error code: E501

Change Summary:
fix line too long issues
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 8 (+1346 -130) |
Checks run (2 succeeded)
-
-
-
reviewboard/webapi/tests/test_root_review.py (Diff revision 8) Let's fix this to just say "Unit tests for RootReviewResource"

Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 9 (+1349 -131) |
Checks run (2 succeeded)
-
Most of my comments are about the test coverage. They're examples, but not the only cases, where we should probably expand our test data. We want to ensure that we never get any results we don't expect, so we should ensure there's enough test data in each test to ensure that we never unintentionally leak wrong information in the API calls.
-
reviewboard/webapi/resources/root_review.py (Diff revision 9) An optimization I'll be working toward is to avoid these JOINs where possible, and since this code is new, we might as well do this now.
We can fetch the
User
andRepository
up-front (returningself.model.objects.none()
if we fail to look those up, since we know we won't get results). Just the ID (usingvalues_list('pk', flat=True)
).We can then pass that ID in as
Q(user=<id>)
andQ(review_request__repository=<id>)
.This will optimistically save two JOINs, improving performance of this API.
-
reviewboard/webapi/resources/root_review.py (Diff revision 9) This will need to be wrapped in
list(...)
in order to get the values directly. Otherwise this ends up being a subquery, if we just pass it in. -
-
reviewboard/webapi/tests/test_root_review.py (Diff revision 9) This test should have some public and non-public reviews that aren't accessible by the user, and ensure they don't show up in results.
-
reviewboard/webapi/tests/test_root_review.py (Diff revision 9) This test should add a mixture of accessible/inaccessible review requests and accessible/inaccessible reviews, and non-public reviews on accessible review requests, and ensure nothing inaccessible/non-public shows up in results.
-
reviewboard/webapi/tests/test_root_review.py (Diff revision 9) This test should add a mixture of accessible/inaccessible review requests and accessible/inaccessible reviews, and public reviews on accessible review requests, and ensure nothing inaccessible/public shows up in results.
-
reviewboard/webapi/tests/test_root_review.py (Diff revision 9) This should have some inaccessible repositories and make sure reviews on them don't show up in the results.
Same for all other repository-related tests.
We're also going to need tests that show results only if:
- On the repository's user list, or
- In a group on the repository's review group list
-
reviewboard/webapi/tests/test_root_review.py (Diff revision 9) Same comments as above but for review groups.

Change Summary:
- Added more coverage when testing for access control
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 10 (+1620 -152) |
Checks run (2 succeeded)

Change Summary:
Got rid of a hardcoded key name and use
self.resource.list_result_key
instead
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 11 (+1653 -153) |
Checks run (2 succeeded)

Change Summary:
- Added a blank line between class-level docstrings and members
- Changed
frm
andto
variable names in timestamp tests to more descriptive ones
Commits: |
|
|||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Diff: |
Revision 12 (+1662 -156) |