Add top-level reviews API endpoint

Review Request #12297 — Created May 23, 2022 and submitted

Information

Review Board
release-5.0.x

Reviewers

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 and last-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 ID Author
style fixes
8f401d529c308b4b4bb1fef11ec67df5b78e37ba Michelle
added tests for excluding reviews that the requester does not have access to
12ddd87cbd945920d04200e17ce508fadb085adc 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 …

chipx86chipx86

The review_request api endpoint filters by the ID of the repository instead of the name. Should we filter by ID …

maubinmaubin

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

maubinmaubin

this doesn't actually add the user to the local site, is there another way that i should be adding the …

maubinmaubin

W292 no newline at end of file

reviewbotreviewbot

W292 no newline at end of file

reviewbotreviewbot

Can you order these alphabetically?

chipx86chipx86

Small nit, but for consistency, this is usually placed after the main resource definitions (so after added_in in this case).

chipx86chipx86

Can you order these alphabetically?

chipx86chipx86

We should probably stick with public instead of drafts for this, and take it as a boolean.

chipx86chipx86

We can use &= for these.

chipx86chipx86

We use a chaining format to keep these readable, like: users = ( Group.objects .filter(name=...) .values_list('users') )

chipx86chipx86

Can you update this to do one argument per line? That'll help keep this readable/maintainable going forward.

chipx86chipx86

This should document *args and **kwargs.

chipx86chipx86

Can you order these keys alphabetically?

chipx86chipx86

Since we use a ' in the docstring, and don't use " anywhere, we can use " for the string …

chipx86chipx86

This should document the arguments and AssertionError.

chipx86chipx86

For each of these tests, we'll want to asser that we get exactly the reviews we expect, in the order …

chipx86chipx86

Can you update these to use user instead of u? Same elsewhere in tests. This helps keep things consistent and …

chipx86chipx86

Similar, review_request instead of rr.

chipx86chipx86

get_tz_aware_utcnow() is a bit legacy. We should be able to use Django's timezone.now() instead.

chipx86chipx86

We're going to want to avoid using _. I have some details on this in the other API change.

chipx86chipx86

Similarly, we should use review.

chipx86chipx86

line too long (82 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

line too long (83 > 79 characters) Column: 80 Error code: E501

reviewbotreviewbot

Add a blank line between these two.

daviddavid

Let's fix this to just say "Unit tests for RootReviewResource"

daviddavid

An optimization I'll be working toward is to avoid these JOINs where possible, and since this code is new, we …

chipx86chipx86

This will need to be wrapped in list(...) in order to get the values directly. Otherwise this ends up being …

chipx86chipx86

The *args is off. Missing Args:.

chipx86chipx86

This test should have some public and non-public reviews that aren't accessible by the user, and ensure they don't show …

chipx86chipx86

This test should add a mixture of accessible/inaccessible review requests and accessible/inaccessible reviews, and non-public reviews on accessible review requests, …

chipx86chipx86

This test should add a mixture of accessible/inaccessible review requests and accessible/inaccessible reviews, and public reviews on accessible review requests, …

chipx86chipx86

This should have some inaccessible repositories and make sure reviews on them don't show up in the results. Same for …

chipx86chipx86

Same comments as above but for review groups.

chipx86chipx86
maubin
  1. 
      
  2. Show all issues

    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?

    1. We should prioritize filtering by name. IDs are a holdover from ages ago. Generally-speaking, names are what people are going to work with.

  3. Show all issues

    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?

    1. I think that's worth doing.

  4. 
      
chipx86
  1. 
      
  2. Show all issues

    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:

    1. It's unpublished and you are not the owner
    2. It's on a private repository you don't have access to
    3. It's on an invite-only review group you don't have access to
    4. It's on a Local Site you don't have access to
  3. 
      
maubin
Review request changed
Commits:
Summary ID Author
style fixes
f51a3f8266cdc437adad47b077640c67303d48a6 Michelle
style fixes
f51a3f8266cdc437adad47b077640c67303d48a6 Michelle
added tests for excluding reviews that the requester does not have access to
2a13f607d8c71ce674dfd8e76ce5e9eaedde280c Michelle

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
  1. 
      
  2. Show all issues

    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?

  3. 
      
maubin
Review request changed
Commits:
Summary ID Author
style fixes
f51a3f8266cdc437adad47b077640c67303d48a6 Michelle
added tests for excluding reviews that the requester does not have access to
2a13f607d8c71ce674dfd8e76ce5e9eaedde280c Michelle
style fixes
f51a3f8266cdc437adad47b077640c67303d48a6 Michelle
added tests for excluding reviews that the requester does not have access to
21cd27249c868378fa540a824b70165b54ccfdec Michelle

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
maubin
maubin
david
  1. Ship It!
  2. 
      
chipx86
  1. Many of my comments are similar to the comments API change. There's more context in there for some of the issues opened here.

  2. reviewboard/webapi/resources/root_review.py (Diff revision 6)
     
     
     
     
    Show all issues

    Can you order these alphabetically?

  3. Show all issues

    Small nit, but for consistency, this is usually placed after the main resource definitions (so after added_in in this case).

  4. reviewboard/webapi/resources/root_review.py (Diff revision 6)
     
     
     
     
     
     
    Show all issues

    Can you order these alphabetically?

  5. reviewboard/webapi/resources/root_review.py (Diff revision 6)
     
     
     
    Show all issues

    We should probably stick with public instead of drafts for this, and take it as a boolean.

  6. Show all issues

    We can use &= for these.

  7. reviewboard/webapi/resources/root_review.py (Diff revision 6)
     
     
     
    Show all issues

    We use a chaining format to keep these readable, like:

    users = (
        Group.objects
        .filter(name=...)
        .values_list('users')
    )
    
  8. reviewboard/webapi/resources/root_review.py (Diff revision 6)
     
     
     
    Show all issues

    Can you update this to do one argument per line? That'll help keep this readable/maintainable going forward.

  9. reviewboard/webapi/resources/root_review.py (Diff revision 6)
     
     
     
     
     
     
     
     
    Show all issues

    This should document *args and **kwargs.

  10. reviewboard/webapi/resources/root_review.py (Diff revision 6)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can you order these keys alphabetically?

  11. reviewboard/webapi/resources/root_review.py (Diff revision 6)
     
     
     
     
     
    Show all issues

    Since we use a ' in the docstring, and don't use " anywhere, we can use " for the string itself and avoid escaping the '.

  12. Show all issues

    This should document the arguments and AssertionError.

  13. Show all issues

    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.

    1. Since this test is for counts-only I can't compare reviews here, but I will use compare_item for the rest of the tests.

  14. Show all issues

    Can you update these to use user instead of u? Same elsewhere in tests. This helps keep things consistent and greppable.

  15. Show all issues

    Similar, review_request instead of rr.

  16. Show all issues

    get_tz_aware_utcnow() is a bit legacy. We should be able to use Django's timezone.now() instead.

  17. Show all issues

    We're going to want to avoid using _. I have some details on this in the other API change.

  18. Show all issues

    Similarly, we should use review.

  19. 
      
maubin
Review request changed
Change Summary:
  • Changed the drafts request field to public. 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 and time-added-to request fields back to last-updated-from and last-updated-to since review.timestamp can be updated, its not necessarily the time it was added
  • Switched q = q & to q &=
  • Stopped using _ for unused variables
  • Uses timezone.now() instead of get_tz_aware_utcnow()
  • Uses compare_item in mostly all tests
  • Uses consistent variable names in tests
Commits:
Summary ID Author
style fixes
76f92ec1c9e7f2db453f99e15bea82b09cd21c78 Michelle
added tests for excluding reviews that the requester does not have access to
f5485492d261cb96a1394e40e17ab97ac5bc1171 Michelle
style fixes
b6c09163cab5c656015ee6192b1f5ac419ca4547 Michelle
added tests for excluding reviews that the requester does not have access to
52feb4f50bf103427913dc764ecac19639483011 Michelle

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
david
  1. 
      
  2. reviewboard/webapi/resources/root_review.py (Diff revision 8)
     
     
     
    Show all issues

    Add a blank line between these two.

    1. I thought we didn't need to add a blank line between class docstrings and their attributes?

    2. There's a blank line after module and class docstrings, but not function docstrings.

      Feels a bit inconsistent, but this is the standard convention out there.

    3. Ah got it.

    4. I think it's because there's expected to be one blank line between class members, and the docstring is considered a member.

  3. Show all issues

    Let's fix this to just say "Unit tests for RootReviewResource"

  4. 
      
maubin
chipx86
  1. 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.

    1. Basically anything we don't think we should get in a result should be part of the test data, for any given test.

  2. reviewboard/webapi/resources/root_review.py (Diff revision 9)
     
     
     
     
     
     
     
    Show all issues

    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 and Repository up-front (returning self.model.objects.none() if we fail to look those up, since we know we won't get results). Just the ID (using values_list('pk', flat=True)).

    We can then pass that ID in as Q(user=<id>) and Q(review_request__repository=<id>).

    This will optimistically save two JOINs, improving performance of this API.

  3. reviewboard/webapi/resources/root_review.py (Diff revision 9)
     
     
     
     
     
     
    Show all issues

    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.

  4. reviewboard/webapi/resources/root_review.py (Diff revision 9)
     
     
     
     
     
     
     
    Show all issues

    The *args is off. Missing Args:.

  5. reviewboard/webapi/tests/test_root_review.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

  6. reviewboard/webapi/tests/test_root_review.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

  7. reviewboard/webapi/tests/test_root_review.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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.

  8. reviewboard/webapi/tests/test_root_review.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    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:

    1. On the repository's user list, or
    2. In a group on the repository's review group list
  9. reviewboard/webapi/tests/test_root_review.py (Diff revision 9)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Same comments as above but for review groups.

  10. 
      
maubin
maubin
maubin
david
  1. Ship It!
  2. 
      
maubin
Review request changed
Status:
Completed
Change Summary:
Pushed to release-5.0.x (1be0f6d)