Top Level Reviews API Endpoint

Review Request #12028 — Created Feb. 3, 2022 and discarded

TaylorChristie
Review Board
master
reviewboard
  • Added top level reviews endpoint.
  • Ability to filter by username using ?user=username parameter.
  • Ability to filter by repository name using ?repository=repoName parameter.
  • Ability to filter by date range using ?last-updated-from and ?last-updated-to parameters.
  • Ability to filter by review group using ?review-group=group_name parameter.
  • Added tests for the new root_reviews resource.
  • Added documentation for top level reviews endpoint.
  • Wrote unit tests for all new functionality
  • Ran unit tests for root_reviews resource and ensure they pass
  • Tested new API endpoint manually and confirmed results were as expected.
Summary
attempted adding root_review
got endpoint working (rudementarily), added some basic tests
get tests working
Added user filtering by username. Added repository filtering by repository name. Added unit tests for user filtering and repository filtering
added date filtering, added review group filtering, added blank unit tests
removed unneeded imports
added unit tests for last-updated-from and laste-updated-to parameters and addressed some patch feedback
finished review-group parameter, unit tests, and flake8 formatting
added documentation
finished up root review documentation and ensure it generates correctly
feedback from chaoyu
addressed feedback from David
addressed feedback from David and Sheena.
Description From Last Updated

F401 'django.utils.six' imported but unused

reviewbotreviewbot

F401 'djblets.util.decorators.augment_method_from' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.decorators.webapi_response_errors' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.errors.RevokeShipItError' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.decorators.webapi_check_local_site' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.errors.REVOKE_SHIP_IT_ERROR' imported but unused

reviewbotreviewbot

F401 'datetime.datetime' imported but unused

reviewbotreviewbot

F401 'datetime.timedelta' imported but unused

reviewbotreviewbot

F401 'django.core.mail' imported but unused

reviewbotreviewbot

F401 'django.utils.timezone' imported but unused

reviewbotreviewbot

F401 'djblets.util.dates.get_tz_aware_utcnow' imported but unused

reviewbotreviewbot

F401 'djblets.testing.decorators.add_fixtures' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.errors.PERMISSION_DENIED' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.errors.DOES_NOT_EXIST' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.testing.decorators.webapi_test_template' imported but unused

reviewbotreviewbot

F401 'kgb.SpyAgency' imported but unused

reviewbotreviewbot

F401 'kgb.spy_on' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.models.Review' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.models.ReviewRequest' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.signals.review_ship_it_revoking' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.errors.REVOKE_SHIP_IT_ERROR' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mimetypes.review_item_mimetype' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mixins.ReviewRequestChildItemMixin' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mixins.ReviewRequestChildListMixin' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mixins.BasicTestsMetaclass' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mixins_review.ReviewItemMixin' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mixins_review.ReviewListMixin' imported but unused

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

F401 'django.utils.six' imported but unused

reviewbotreviewbot

F401 'djblets.util.decorators.augment_method_from' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.decorators.webapi_response_errors' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.errors.RevokeShipItError' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.decorators.webapi_check_local_site' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.errors.REVOKE_SHIP_IT_ERROR' imported but unused

reviewbotreviewbot

F401 'datetime.datetime' imported but unused

reviewbotreviewbot

F401 'datetime.timedelta' imported but unused

reviewbotreviewbot

F401 'django.core.mail' imported but unused

reviewbotreviewbot

F401 'django.utils.timezone' imported but unused

reviewbotreviewbot

F401 'djblets.util.dates.get_tz_aware_utcnow' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.errors.PERMISSION_DENIED' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.errors.DOES_NOT_EXIST' imported but unused

reviewbotreviewbot

F401 'djblets.webapi.testing.decorators.webapi_test_template' imported but unused

reviewbotreviewbot

F401 'kgb.SpyAgency' imported but unused

reviewbotreviewbot

F401 'kgb.spy_on' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.models.Review' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.models.ReviewRequest' imported but unused

reviewbotreviewbot

F401 'reviewboard.reviews.signals.review_ship_it_revoking' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.errors.REVOKE_SHIP_IT_ERROR' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mimetypes.review_item_mimetype' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mixins.ReviewRequestChildItemMixin' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mixins.ReviewRequestChildListMixin' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mixins.BasicTestsMetaclass' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mixins_review.ReviewItemMixin' imported but unused

reviewbotreviewbot

F401 'reviewboard.webapi.tests.mixins_review.ReviewListMixin' imported but unused

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

On the master branch, we no longer need the unicode_literals imports.

daviddavid

This was just copied from the other review resource, but should probably be clarified since this one is not a …

daviddavid

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (96 > 79 characters)

reviewbotreviewbot

E501 line too long (94 > 79 characters)

reviewbotreviewbot

E501 line too long (93 > 79 characters)

reviewbotreviewbot

E501 line too long (91 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (86 > 79 characters)

reviewbotreviewbot

E501 line too long (89 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

E501 line too long (80 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (90 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (83 > 79 characters)

reviewbotreviewbot

E501 line too long (95 > 79 characters)

reviewbotreviewbot

E501 line too long (96 > 79 characters)

reviewbotreviewbot

E501 line too long (94 > 79 characters)

reviewbotreviewbot

E501 line too long (109 > 79 characters)

reviewbotreviewbot

E501 line too long (121 > 79 characters)

reviewbotreviewbot

E501 line too long (117 > 79 characters)

reviewbotreviewbot

E501 line too long (82 > 79 characters)

reviewbotreviewbot

E501 line too long (139 > 79 characters)

reviewbotreviewbot

E501 line too long (84 > 79 characters)

reviewbotreviewbot

F401 'importlib' imported but unused

reviewbotreviewbot

F401 'django.utils.six' imported but unused

reviewbotreviewbot

Please add a module docstring at the top of this file. Something like """Root-level resource for querying reviews."""

daviddavid

Imports from reviewboard should be in a separate group after third-party packages. So: from django... from djblets... from reviewboard... Everything …

daviddavid

This can be wrapped much later. Line width is 79 columns.

daviddavid

I know you probably copied this from our other code, but this should use our new docstring standards: https://www.notion.so/reviewboard/Writing-Python-Docs-845c696271194d85b69503246db1d0e6

daviddavid

Looks great! Only one suggestion here, once speficied in webapi_request_fields, we can get parsed parameters in kwargds. Using kwargds['drafts'] here …

ChaoyuChaoyu

This needs a docstring.

daviddavid

Please update for new docstring standards.

daviddavid

This file needs a module docstring.

daviddavid

Please sort alphabetically - t before u

daviddavid

This needs a blank line between these two.

daviddavid

Please add a docstring.

daviddavid

When we wrap a single-line docstring, please put the final """ on its own line: def test_get_with_counts_only(self): """Testing the GET …

daviddavid

""" on its own line.

daviddavid

""" on its own line. Please go through the rest of this file and change where appropriate.

daviddavid

webapi.decorators before webapi.fields

daviddavid

This should be in the imperative mood ("Return" instead of "Returns")

daviddavid

This line can dedent 4 spaces.

daviddavid

Returns -> Return

daviddavid

Returns -> Return

daviddavid

Instead of listing "reviews/" in each docstring, let's use "<URL>/" and add on the @webapi_test_template decorator.

daviddavid

I think everything looks good overall. I just have a small suggestion to improve the unit tests readability: I think …

sheenaNgsheenaNg

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

maubinmaubin

I'm going to specify that this is the "review group name of users" rather than "the group of users"

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
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

TaylorChristie
Review request changed

Description:

   
  • Added rudimentary top level reviews endpoint.
~  
  • Added some basic tests for the new root_reviews resource.
~  
  • Still missing a lot of basic functionality, documentation, and tests
  ~
  • Ability to filter by username using ?user=username parameter
  ~
  • Ability to filter by repository name using ?repository=repoName parameter
  +
  • Added some basic tests for the new root_reviews resource.
  +
  • Still missing date range filtering, review group filtering, documentation, some unit tests

Testing Done:

~  
  • Ran unit tests for root_reviews resource.
~  
  • Tested API new endpoint and confirmed results were as expected.
  ~
  • Wrote unit tests for all new functionality
  ~
  • Ran unit tests for root_reviews resource and ensure they pass
  +
  • Tested new API endpoint manually and confirmed results were as expected.

Commits:

Summary
-
attempted adding root_review
-
got endpoint working (rudementarily), added some basic tests
-
get tests working
+
attempted adding root_review
+
got endpoint working (rudementarily), added some basic tests
+
get tests working
+
Added user filtering by username. Added repository filtering by repository name. Added unit tests for user filtering and repository filtering

Diff:

Revision 2 (+571 -61)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TaylorChristie
Review request changed

Description:

   
  • Added rudimentary top level reviews endpoint.
   
  • Ability to filter by username using ?user=username parameter
   
  • Ability to filter by repository name using ?repository=repoName parameter
~  
  • Added some basic tests for the new root_reviews resource.
~  
  • Still missing date range filtering, review group filtering, documentation, some unit tests
  ~
  • Ability to filter by date range using ?last-updated-from and ?last-updated-to
  ~
  • Ability to filter by review group using ?review-group=group_name
  +
  • Added some basic tests for the new root_reviews resource.
  +
  • Still missing documentation, some unit tests

Commits:

Summary
-
attempted adding root_review
-
got endpoint working (rudementarily), added some basic tests
-
get tests working
-
Added user filtering by username. Added repository filtering by repository name. Added unit tests for user filtering and repository filtering
+
attempted adding root_review
+
got endpoint working (rudementarily), added some basic tests
+
get tests working
+
Added user filtering by username. Added repository filtering by repository name. Added unit tests for user filtering and repository filtering
+
added date filtering, added review group filtering, added blank unit tests
+
removed unneeded imports

Diff:

Revision 3 (+625 -113)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
  1. Great start! Looks like you're on the right track, and I'll do a more thorough review once everything is more complete.

  2. On the master branch, we no longer need the unicode_literals imports.

  3. This was just copied from the other review resource, but should probably be clarified since this one is not a child of a review request.

  4. 
      
TaylorChristie
Review request changed

Description:

   
  • Added rudimentary top level reviews endpoint.
   
  • Ability to filter by username using ?user=username parameter
   
  • Ability to filter by repository name using ?repository=repoName parameter
   
  • Ability to filter by date range using ?last-updated-from and ?last-updated-to
   
  • Ability to filter by review group using ?review-group=group_name
~  
  • Added some basic tests for the new root_reviews resource.
  ~
  • Added tests for the new root_reviews resource.
   
  • Still missing documentation, some unit tests

Commits:

Summary
-
attempted adding root_review
-
got endpoint working (rudementarily), added some basic tests
-
get tests working
-
Added user filtering by username. Added repository filtering by repository name. Added unit tests for user filtering and repository filtering
-
added date filtering, added review group filtering, added blank unit tests
-
removed unneeded imports
+
attempted adding root_review
+
got endpoint working (rudementarily), added some basic tests
+
get tests working
+
Added user filtering by username. Added repository filtering by repository name. Added unit tests for user filtering and repository filtering
+
added date filtering, added review group filtering, added blank unit tests
+
removed unneeded imports
+
added unit tests for last-updated-from and laste-updated-to parameters and addressed some patch feedback

Diff:

Revision 4 (+718 -150)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TaylorChristie
TaylorChristie
TaylorChristie
Review request changed

Summary:

-[WIP] Top Level Reviews API Endpoint
+Top Level Reviews API Endpoint

Description:

~  
  • Added rudimentary top level reviews endpoint.
~  
  • Ability to filter by username using ?user=username parameter
~  
  • Ability to filter by repository name using ?repository=repoName parameter
~  
  • Ability to filter by date range using ?last-updated-from and ?last-updated-to
~  
  • Ability to filter by review group using ?review-group=group_name
  ~
  • Added top level reviews endpoint.
  ~
  • Ability to filter by username using ?user=username parameter.
  ~
  • Ability to filter by repository name using ?repository=repoName parameter.
  ~
  • Ability to filter by date range using ?last-updated-from and ?last-updated-to parameters.
  ~
  • Ability to filter by review group using ?review-group=group_name parameter.
   
  • Added tests for the new root_reviews resource.
~  
  • Still missing documentation
  ~
  • Added documentation for top level reviews endpoint.

Commits:

Summary
-
attempted adding root_review
-
got endpoint working (rudementarily), added some basic tests
-
get tests working
-
Added user filtering by username. Added repository filtering by repository name. Added unit tests for user filtering and repository filtering
-
added date filtering, added review group filtering, added blank unit tests
-
removed unneeded imports
-
added unit tests for last-updated-from and laste-updated-to parameters and addressed some patch feedback
-
finished review-group parameter, unit tests, and flake8 formatting
+
attempted adding root_review
+
got endpoint working (rudementarily), added some basic tests
+
get tests working
+
Added user filtering by username. Added repository filtering by repository name. Added unit tests for user filtering and repository filtering
+
added date filtering, added review group filtering, added blank unit tests
+
removed unneeded imports
+
added unit tests for last-updated-from and laste-updated-to parameters and addressed some patch feedback
+
finished review-group parameter, unit tests, and flake8 formatting
+
added documentation

Diff:

Revision 6 (+8188 -5886)

Show changes

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

TaylorChristie
TaylorChristie
Chaoyu
  1. 
      
  2. reviewboard/webapi/resources/root_review.py (Diff revision 8)
     
     
     

    Looks great! Only one suggestion here, once speficied in webapi_request_fields, we can get parsed parameters in kwargds. Using kwargds['drafts'] here should get parsed boolean value directly.

  3. 
      
david
  1. Looking great! I have a few comments but they're all trivial style things.

  2. Please add a module docstring at the top of this file. Something like """Root-level resource for querying reviews."""

  3. reviewboard/webapi/resources/root_review.py (Diff revision 8)
     
     
     
     
     

    Imports from reviewboard should be in a separate group after third-party packages. So:

    from django...
    from djblets...
    
    from reviewboard...
    

    Everything within those two groups should be sorted alphabetically.

  4. reviewboard/webapi/resources/root_review.py (Diff revision 8)
     
     
     
     

    This can be wrapped much later. Line width is 79 columns.

  5. I know you probably copied this from our other code, but this should use our new docstring standards: https://www.notion.so/reviewboard/Writing-Python-Docs-845c696271194d85b69503246db1d0e6

  6. This needs a docstring.

  7. Please update for new docstring standards.

  8. This file needs a module docstring.

  9. Please sort alphabetically - t before u

  10. This needs a blank line between these two.

  11. Please add a docstring.

  12. When we wrap a single-line docstring, please put the final """ on its own line:

    def test_get_with_counts_only(self):
        """Testing the GET reviews/?counts_only=1 API returns expected
        counts
        """
    
  13. """ on its own line.

  14. """ on its own line. Please go through the rest of this file and change where appropriate.

  15. 
      
TaylorChristie
TaylorChristie
david
  1. 
      
  2. reviewboard/webapi/resources/root_review.py (Diff revision 10)
     
     
     
     
     
     

    webapi.decorators before webapi.fields

  3. This should be in the imperative mood ("Return" instead of "Returns")

  4. This line can dedent 4 spaces.

  5. Returns -> Return

  6. Returns -> Return

  7. Instead of listing "reviews/" in each docstring, let's use "<URL>/" and add on the @webapi_test_template decorator.

  8. 
      
sheenaNg
  1. 
      
    1. I think it looks great overall! I just added a minor suggestion to add some blank lines in between groups of codes in the unit tests to improve readability and consistency in terms of styling.

  2. I think everything looks good overall.

    I just have a small suggestion to improve the unit tests readability: I think it would be easier to read if you add some blank lines to separate different groups of codes in the unit tests.

    For example, add blank line after the codes that create review_request, review etc. and before calling the API endpoint. Then, add another blank line after calling the API endpoint and before the assertions.

    I see that you have added some blank lines in some unit tests but not all. It will improve the consistency if this is applied to the rest of the unit tests.

  3. 
      
TaylorChristie
maubin
  1. 
      
  2. 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 or should we just specify in the description that this field is the name of the repository?

  3. I'm going to specify that this is the "review group name of users" rather than "the group of users"

  4. 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 I add these fields?

  5. 
      
maubin
Review request changed

Status: Discarded

Change Summary:

This work has moved to /r/12297

Loading...