Add top-level comments APIs

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

Information

Review Board
release-5.0.x

Reviewers

Comment resources are currently child resources of reviews. This limits the type
of queries we can do for comments, for example we can't query for all diff
comments created by a given user.

This change adds top-level API endpoints for comments. We register root list
resources for diff comments, file attachment comments and general comments and
support GET requests for the resources with the following query parameters:
- review-request-id=<review-request-id>, review-id=<review-id>,
user=<user-username>, is-reply for all
- file-diff-id, line and interdiff-revision for diff comments
- file-attachment-id and file-name for file attachment comments

This change also improves the docstrings and code readability for base diff
comments, base file attachment comments, and base general review comments.

Based on work by Chaoyu Xiang at /r/12037.

  • Created GET method unit tests for the new resources and ran them successfully
  • Manually tested the /api/general-comments/, /api/diff-comments/ and
    api/file-attachment-comments endpoints
Summary ID Author
style fixes and removed item mimetypes and urls
a5f42292a09663b7e512c6a9d002282dee719333 Michelle
Description From Last Updated

Along the lines of what I said in the Reviews API and CommentManager changes, we should ensure that all the …

chipx86chipx86

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

Should we specify what "more advanced queries" we made possible?

maubinmaubin

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

E999 SyntaxError: invalid syntax

reviewbotreviewbot

coming -> incoming

daviddavid

key argument pairs -> keyword arguments

daviddavid

This needs a type, and the description should be reworded. We don't need to say "Django's queryset"

daviddavid

Same comments here.

daviddavid

And here.

daviddavid

This can probably just be q = q.filter(review__review_request=review_request). No need for a Q object here.

daviddavid

Not sure how best to organize this, but maybe "All Comments". An argument could also be made to just put …

chipx86chipx86

"ID"

chipx86chipx86

This should have , optional after the type.

chipx86chipx86

This should be *args.

chipx86chipx86

This should be **kwargs.

chipx86chipx86

For multi-line conditionals, you can use parens: if ('review_request_id' in kwargs and kwargs['review_request_id'] is not None): Though in this case, …

chipx86chipx86

We can omit this and leave it up to the parent method.

chipx86chipx86

Same comments as above regarding the * in the names.

chipx86chipx86

This blank line can be removed.

chipx86chipx86

This should probably be in the parameter list.

chipx86chipx86

These can use q &=.

chipx86chipx86

When working with multiple keyword arguments, we often prefer to do one parameter per line to keep it readable and …

chipx86chipx86

Can you order these in alphabetical order? That'll help make it obvious where any new query arguments should go.

chipx86chipx86

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

chipx86chipx86

No blank line here.

chipx86chipx86

We probably don't need this anymore, if we're reusing the other mimetype constants.

chipx86chipx86

Private methods should always go after all the public methods in the class. We also try to add docstrings for …

chipx86chipx86

This can be one line. Some older tests didn't do this, but we're moving away from that.

chipx86chipx86

Assigning unused variables to _ is a convention many use, but it's harmful in our codebase because _ is (also …

chipx86chipx86

local variable 'comment2' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

local variable 'comment' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

local variable 'comment2' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

local variable 'comment2' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

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

reviewbotreviewbot

local variable 'comment' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

no newline at end of file Column: 67 Error code: W292

reviewbotreviewbot

local variable 'comment2' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

local variable 'comment2' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

local variable 'comment' is assigned to but never used Column: 9 Error code: F841

reviewbotreviewbot

Instead of "API Class", let's say "API resource"

daviddavid

I don't think "made on a review" makes sense here because we're not dealing with reviews.

daviddavid

Instead of "API Class", let's say "API resource"

daviddavid

Nix "made on a review"

daviddavid

Instead of "API Class", let's say "API resource"

daviddavid

Nix "made on a review"

daviddavid

"different comments" -> "diff comments"

daviddavid

Should be RootDiffCommentResource

daviddavid

Should be RootFileAttachmentCommentResource

daviddavid

RootGeneralCommentResource

daviddavid

Let's have this explicitly check against None (differentiating from 0, ensuring users can't bypass this with some provided parameter).

chipx86chipx86

Same as above, let's compare explicitly against None.

chipx86chipx86

Same as above, let's explicitly check against None.

chipx86chipx86

If we do __id=, we may force a JOIN to the reviews_review table. We can leave that off and just …

chipx86chipx86

This will also force a JOIN, and we want to avoid that. Except we can't just hard-code a comparison with …

chipx86chipx86

Let's pre-fetch the user (just the user's ID, actually, using .values_list('pk', flat=True)). If we get 0 results, we can explicitly …

chipx86chipx86

This can just be file_attachment=.

chipx86chipx86

Same comments as above regarding the JOINs, Local Site handling, and pre-fetching/checking the user ID.

chipx86chipx86

We can probably just say: These are listed in :py:meth:`get_list`. Same elsewhere.

chipx86chipx86

Same comments as above regarding the JOINs, Local Site handling, and pre-fetching/checking the user ID.

chipx86chipx86

continuation line under-indented for visual indent Column: 34 Error code: E128

reviewbotreviewbot

While here, can you update this to include a blank line between class-level docstrings and members? Good opportunity to fix …

chipx86chipx86

Small nit: Rather than frm and to, let's go ahead and just make these something like timestamp_from and timestamp_to. Same …

chipx86chipx86
Checks run (1 failed, 1 succeeded)
flake8 failed.
JSHint passed.

flake8

maubin
Review request changed
Commits:
Summary ID Author
style fixes and removed item mimetypes and urls
a5b9010d4b02e1e6b67a87d07ff8487788e59ae1 Michelle
style fixes and removed item mimetypes and urls
5281065b69e86ae7d43aac81583424cbb614ca4b Michelle

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
Review request changed
Commits:
Summary ID Author
style fixes and removed item mimetypes and urls
5281065b69e86ae7d43aac81583424cbb614ca4b Michelle
style fixes and removed item mimetypes and urls
ecad7abf9fe6eed0dc0f5d125898d937b5e4916a Michelle

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
maubin
  1. 
      
  2. Show all issues

    Should we specify what "more advanced queries" we made possible?

    1. Yeah, I'd say something like "... makes it possible to query diffs across different review requests."

      These docs will appear on the website as the API docs, so we can definitely spend time elaborating and explaining to whatever degree is most helpful for people coming in without any prior knowledge.

  3. 
      
maubin
maubin
david
  1. 
      
  2. Show all issues

    coming -> incoming

  3. Show all issues

    key argument pairs -> keyword arguments

  4. Show all issues

    This needs a type, and the description should be reworded. We don't need to say "Django's queryset"

  5. Show all issues

    Same comments here.

  6. Show all issues

    And here.

  7. Show all issues

    This can probably just be q = q.filter(review__review_request=review_request). No need for a Q object here.

  8. 
      
maubin
chipx86
  1. Thanks for taking this on! It'll be great to have this in.

    Many of the comments here apply to all the comment type variations.

  2. Show all issues

    Not sure how best to organize this, but maybe "All Comments".

    An argument could also be made to just put this in "Reviews".

    1. I'm leaning towards "All Comments" or even just "Comments". If we put it in "Reviews", couldn't we just as equally put it in "Review Requests".

    2. All Comments works for me.

  3. Show all issues

    "ID"

  4. Show all issues

    This should have , optional after the type.

  5. Show all issues

    This should be *args.

  6. Show all issues

    This should be **kwargs.

  7. Show all issues

    For multi-line conditionals, you can use parens:

    if ('review_request_id' in kwargs and
        kwargs['review_request_id'] is not None):
    

    Though in this case, we should have review_request_id in the function's argument list.

  8. Show all issues

    We can omit this and leave it up to the parent method.

  9. Show all issues

    Same comments as above regarding the * in the names.

  10. Show all issues

    This blank line can be removed.

  11. Show all issues

    This should probably be in the parameter list.

  12. Show all issues

    These can use q &=.

  13. Show all issues

    When working with multiple keyword arguments, we often prefer to do one parameter per line to keep it readable and maintainable.

  14. reviewboard/webapi/resources/root_diff_comment.py (Diff revision 5)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Can you order these in alphabetical order? That'll help make it obvious where any new query arguments should go.

  15. reviewboard/webapi/resources/root_diff_comment.py (Diff revision 5)
     
     
     
     
     
     
    Show all issues

    Since we need a ' in the string, and don't use " anywhere, we can use " for the strings itself and avoid the escape.

    Same below.

  16. Show all issues

    No blank line here.

  17. reviewboard/webapi/tests/mimetypes.py (Diff revision 5)
     
     
    Show all issues

    We probably don't need this anymore, if we're reusing the other mimetype constants.

  18. Show all issues

    Private methods should always go after all the public methods in the class.

    We also try to add docstrings for helper methods in unit tests. We didn't always, so some older test modules lack this.

  19. Show all issues

    This can be one line.

    Some older tests didn't do this, but we're moving away from that.

  20. Show all issues

    Assigning unused variables to _ is a convention many use, but it's harmful in our codebase because _ is (also by convention) often mapped to gettext.

    Fun fact: A lot of online Python guides claim _ is a feature of the language for unused variables, but it's no different than any other variable name. Python doesn't treat it specially.

  21. 
      
maubin
Review request changed
Change Summary:
  • Changed the Root Comments category in the webapi resources index to Comments
  • Fixed up get_queryset method and docstrings for all base comment resources
  • Added interdiff-revision request field to RootDiffCommentResource
  • Applied query optimizations, alphabetized and small style changes to the root comment resources
  • Moved private methods so that they're after public ones in tests, added docstrings to private methods
  • Made tests docstrings one line when able to
  • Got rid of _ for unused variables
Commits:
Summary ID Author
style fixes and removed item mimetypes and urls
33a3cae263e543ea456b082140c42e521c6d66c3 Michelle
style fixes and removed item mimetypes and urls
007d73bdb4128b3b3c9fab81db7bc459ab2d691c Michelle

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
maubin
maubin
maubin
david
  1. Just some notes on docstrings. Code looks good!

  2. Show all issues

    Instead of "API Class", let's say "API resource"

  3. Show all issues

    I don't think "made on a review" makes sense here because we're not dealing with reviews.

  4. Show all issues

    Instead of "API Class", let's say "API resource"

  5. Show all issues

    Nix "made on a review"

  6. Show all issues

    Instead of "API Class", let's say "API resource"

  7. Show all issues

    Nix "made on a review"

  8. Show all issues

    "different comments" -> "diff comments"

  9. Show all issues

    Should be RootDiffCommentResource

  10. Show all issues

    Should be RootFileAttachmentCommentResource

  11. Show all issues

    RootGeneralCommentResource

  12. 
      
maubin
chipx86
  1. A few things that'll be important to fix regarding querying here, to avoid data leaks in results.

  2. Show all issues

    Along the lines of what I said in the Reviews API and CommentManager changes, we should ensure that all the unit tests have data that should not match based on access or filtering.

  3. Show all issues

    Let's have this explicitly check against None (differentiating from 0, ensuring users can't bypass this with some provided parameter).

  4. Show all issues

    Same as above, let's compare explicitly against None.

  5. Show all issues

    Same as above, let's explicitly check against None.

  6. Show all issues

    If we do __id=, we may force a JOIN to the reviews_review table. We can leave that off and just do review=, which will do the right thing and compare against review_id.

  7. Show all issues

    This will also force a JOIN, and we want to avoid that.

    Except we can't just hard-code a comparison with the ID. If using a Local Site, then the PK of the object is actually not the ID. The local_id field is. But only if on a Local Site. This gives us LocalSite-bound sequential IDs.

    So what we need to do is:

    if local_site is None:
        q &= Q(review__review_request=review_request_id)
    else:
        q &= (Q(review__review_request__local_id=review_request_id) &
              Q(review__review_request__local_site=local_site))
    

    We'll need to ensure that unit tests cover ID vs. Local ID testing, and that no data leaks between the two.

  8. reviewboard/webapi/resources/root_diff_comment.py (Diff revision 11)
     
     
     
     
    Show all issues

    Let's pre-fetch the user (just the user's ID, actually, using .values_list('pk', flat=True)). If we get 0 results, we can explicitly return self.model.objects.none(), since we know nothing will end up matching.

  9. Show all issues

    This can just be file_attachment=.

  10. reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Same comments as above regarding the JOINs, Local Site handling, and pre-fetching/checking the user ID.

  11. Show all issues

    We can probably just say:

    These are listed in :py:meth:`get_list`.
    

    Same elsewhere.

  12. reviewboard/webapi/resources/root_general_comment.py (Diff revision 11)
     
     
     
     
     
     
     
     
     
     
    Show all issues

    Same comments as above regarding the JOINs, Local Site handling, and pre-fetching/checking the user ID.

  13. 
      
maubin
Review request changed
Change Summary:
  • Better test coverage
  • Made some dosctrings better and more consistent
  • Removed a couple more JOINs
Commits:
Summary ID Author
style fixes and removed item mimetypes and urls
c9e9c7d0be9c7d3d26f0786d4a25fd678a577e36 Michelle
style fixes and removed item mimetypes and urls
d5b833fd2b7289a171f15f15aafb55607bbec57a Michelle

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

maubin
chipx86
  1. 
      
  2. Show all issues

    While here, can you update this to include a blank line between class-level docstrings and members? Good opportunity to fix this, since the lines are being modified here.

    There are some other spots where this can be done as well. We only have the trailing blank line for module-level and class-level docstrings (weird inconsistency, but it's standard in Python).

  3. Show all issues

    Small nit: Rather than frm and to, let's go ahead and just make these something like timestamp_from and timestamp_to.

    Same if we do this anywhere else.

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