Summary

Add root comments APIs

Review Request #12037 — Created Feb. 4, 2022 and discarded June 3, 2022, 10:17 a.m.

Information

Owner

Chaoyu

Repository

Review Board

Branch

master

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description


Registered root diff comments, root file attachment comments and root general comments API
Support GET method for list resources with query parameter:
Shared query parameters : review-request-id=<review-request-id> , review-id=<review-id>, user=<user-username>, is-reply
file-diff-id, line and interdiff-revision  for diff comments, and file-attachment-id file-name for file attachment comments
Unit tests suites for comments API
Added added webapi_request_fields decorator to get_list for request parameters documentation
Updated the WebAPI documentations

Testing Done


Added unit tests for diff-comment, general-comment and file-attachment-comment with list resource and GET method

Commits

Summary	Author
register comments root APIs	charlie-xiang
fix a bug in base file attachment comment from last commit	charlie-xiang
add user name filter and add diff comment test suites	charlie-xiang
add user tests to diff comment	charlie-xiang
add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
add last-updated-from, to and is_reply filters	charlie-xiang
address reviews feedback	charlie-xiang
added tests for last-updated and is-reply	charlie-xiang
add webapi_request_fields documentations and resource class docstring	charlie-xiang
update to use parsed parameters and added doc tree	charlie-xiang
fix review comments	charlie-xiang

Issues

Description	From	Last Updated
E501 line too long (85 > 79 characters)	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'django.core.exceptions.ObjectDoesNotExist' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.decorators.webapi_response_errors' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.decorators.webapi_login_required' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.decorators.webapi_request_fields' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.errors.PERMISSION_DENIED' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.errors.NOT_LOGGED_IN' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.errors.DOES_NOT_EXIST' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.fields.IntFieldType' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'reviewboard.diffviewer.models.FileDiff' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'reviewboard.webapi.resources.resources' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 11, 2022, 5:52 p.m.
E265 block comment should start with '# '	reviewbot	Feb. 11, 2022, 5:52 p.m.
W293 blank line contains whitespace	reviewbot	Feb. 11, 2022, 5:52 p.m.
E122 continuation line missing indentation or outdented	reviewbot	Feb. 11, 2022, 5:52 p.m.
E122 continuation line missing indentation or outdented	reviewbot	Feb. 11, 2022, 5:52 p.m.
E122 continuation line missing indentation or outdented	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'django.core.exceptions.ObjectDoesNotExist' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.util.decorators.augment_method_from' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.decorators.webapi_response_errors' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.decorators.webapi_login_required' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.decorators.webapi_request_fields' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.errors.PERMISSION_DENIED' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.errors.NOT_LOGGED_IN' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'djblets.webapi.errors.DOES_NOT_EXIST' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'reviewboard.webapi.decorators.webapi_check_local_site' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
F401 'reviewboard.webapi.resources.resources' imported but unused	reviewbot	Feb. 11, 2022, 5:52 p.m.
E225 missing whitespace around operator	reviewbot	Feb. 11, 2022, 5:52 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Feb. 11, 2022, 8:28 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Feb. 18, 2022, 3:44 p.m.
W291 trailing whitespace	reviewbot	Feb. 25, 2022, 8:19 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Feb. 25, 2022, 8:19 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Feb. 25, 2022, 8:19 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Feb. 25, 2022, 8:19 p.m.
W291 trailing whitespace	reviewbot	March 4, 2022, 8:46 p.m.
This is getting long enough that we should just wrap and list one argument per line.	david	March 4, 2022, 8:46 p.m.
While you're here, mind updating this docstring to follow our modern standards? "Returns" -> "Return", and add "Args" and "Returns" …	david	March 4, 2022, 8:46 p.m.
Instead of doing this, let's avoid listing review_request_id in the args list, and pull it out for our local use: …	david	March 4, 2022, 8:47 p.m.
Same comments here re: kwargs.	david	March 4, 2022, 8:47 p.m.
New files on master won't require the unicode_literals import, but we would like to include module docstrings at the top …	david	March 18, 2022, 2:53 p.m.
Same here re: unicode_literals, docstrings.	david	March 18, 2022, 2:53 p.m.
Same here re: unicode_literals, docstrings.	david	March 18, 2022, 2:53 p.m.
Same here re: unicode_literals, docstrings.	david	March 18, 2022, 2:53 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 4, 2022, 8:46 p.m.
And here.	david	March 18, 2022, 2:53 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 4, 2022, 8:46 p.m.
And here.	david	March 18, 2022, 2:53 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 4, 2022, 8:46 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 11, 2022, 4:47 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 11, 2022, 4:47 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 11, 2022, 4:47 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 11, 2022, 11:02 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 11, 2022, 11:02 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 11, 2022, 11:02 p.m.
W291 trailing whitespace	reviewbot	March 18, 2022, 2:52 p.m.
W291 trailing whitespace	reviewbot	March 18, 2022, 2:52 p.m.
W291 trailing whitespace	reviewbot	March 18, 2022, 2:52 p.m.
W291 trailing whitespace	reviewbot	March 18, 2022, 2:52 p.m.
W291 trailing whitespace	reviewbot	March 18, 2022, 2:52 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 18, 2022, 2:52 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 18, 2022, 2:52 p.m.
E999 SyntaxError: invalid syntax	reviewbot	March 18, 2022, 2:52 p.m.
I think there is a typo here. Should it be "list" resources?	sheenaNg	April 5, 2022, 3:56 p.m.
I know it was like this before, but can you add a docstring?	david	April 5, 2022, 3:03 p.m.
Add a blank line after this.	david	April 5, 2022, 3:04 p.m.
dict.get() returns None if the key doesn't exist, so we can simplify: if kwargs.get('review_request_id') is not None:	david	April 5, 2022, 3:04 p.m.
I know it was like this before, but can you add a docstring?	david	April 5, 2022, 3:08 p.m.
Add a blank line after this.	david	April 5, 2022, 3:08 p.m.
dict.get() returns None if the key doesn't exist, so we can simplify: if kwargs.get('review_request_id') is not None:	david	April 5, 2022, 3:09 p.m.
Instead of having multiple return paths, let's just reassign q. Please also add a blank line above the return: ```python …	david	April 5, 2022, 3:10 p.m.
It's not "different comments", it's "diff comments" :)	david	April 5, 2022, 3:10 p.m.
Imports should be in three groups, separated by blank lines and alphabetized within: Standard library Third-party "Current" module (anything in …	david	April 5, 2022, 4:09 p.m.
This is "new" right now but won't be new forever. Just call it "This top level ..."	david	April 5, 2022, 3:13 p.m.
Add a blank line between these two.	david	April 5, 2022, 3:14 p.m.
This should be in the imperative mood ("Return" instead of "Returns"). This docstring also needs an "Args" section.	david	April 5, 2022, 3:16 p.m.
Add a blank line between these two.	david	April 5, 2022, 3:16 p.m.
Can you add a line between these two lines?	sheenaNg	April 5, 2022, 3:56 p.m.
This is something minor, you missed the period at the end of the description.	sheenaNg	April 5, 2022, 3:57 p.m.
You missed the period at the end of the description.	sheenaNg	April 5, 2022, 3:57 p.m.
This should be in the imperative mood.	david	April 5, 2022, 3:16 p.m.
Same comments in this file as in the diff comments file.	david	April 5, 2022, 3:19 p.m.
There should be a blank line after the class docstring. Can you add a blank line here?	sheenaNg	April 5, 2022, 3:57 p.m.
Can you also add a blank line between these two lines?	sheenaNg	April 5, 2022, 3:57 p.m.
Can you add a blank line after q = Q() and also after each if statement?	sheenaNg	April 5, 2022, 3:58 p.m.
You missed the period at the end of the description.	sheenaNg	April 5, 2022, 3:58 p.m.
You missed the period at the end of the description.	sheenaNg	April 5, 2022, 3:58 p.m.
Same comments in this file as in the diff comments file.	david	April 5, 2022, 3:24 p.m.
There should be a blank line after the class docstring. Can you add a blank line here?	sheenaNg	April 5, 2022, 3:58 p.m.
Can you add a blank line between these two lines?	sheenaNg	April 5, 2022, 3:59 p.m.
Can you add a blank line after q = Q() and also after each if statement?	sheenaNg	April 5, 2022, 3:59 p.m.
You missed the period at the end of the description.	sheenaNg	April 5, 2022, 3:59 p.m.
You missed the period at the end of the description.	sheenaNg	April 5, 2022, 3:59 p.m.
I don't know that it makes sense to have new mimetypes for these endpoints, since the content is effectively the …	david	April 4, 2022, 4:23 p.m.
There's an extra space before "Unit"	david	April 5, 2022, 3:32 p.m.
No blank line here. Please also alphabetize the import list.	david	April 5, 2022, 3:34 p.m.
E999 SyntaxError: invalid syntax	reviewbot	April 5, 2022, 4:20 p.m.
Instead of writing "diff_comments/" in each of these docstrings, let's just put "<URL>/..." and then use the @webapi_test_template decorator.	david	April 5, 2022, 3:38 p.m.
I think it is great overall. One suggestion: Would it be easier to read if you add some blank lines …	sheenaNg	April 5, 2022, 4:08 p.m.
A few things here: Alphabetize the list No blank line within the section For the tests.urls import, put the import …	david	April 5, 2022, 4:10 p.m.
E999 SyntaxError: invalid syntax	reviewbot	April 5, 2022, 4:20 p.m.
Let's use @webapi_test_template for all these.	david	April 5, 2022, 3:47 p.m.
I do not think there should be a blank line after function docstring.	sheenaNg	April 5, 2022, 4:08 p.m.
Same comments here as in the other files.	david	April 5, 2022, 3:55 p.m.
E999 SyntaxError: invalid syntax	reviewbot	April 5, 2022, 4:20 p.m.
Let's use @webapi_test_template for all the docstrings here.	david	April 5, 2022, 3:49 p.m.
I do not think there should be a blank line after function docstring.	sheenaNg	April 5, 2022, 4:08 p.m.
E999 SyntaxError: invalid syntax	reviewbot	April 5, 2022, 4:20 p.m.
E999 SyntaxError: invalid syntax	reviewbot	April 5, 2022, 4:20 p.m.
E999 SyntaxError: invalid syntax	reviewbot	April 5, 2022, 4:20 p.m.

flake8 failed.

JSHint passed.

flake8

```
Warning: Showing 30 of 31 failures.
```
reviewboard/webapi/resources/base_diff_comment.py (Diff revision 1)
Show all issues
```
E501 line too long (85 > 79 characters)
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 1)
Show all issues
```
F401 'django.core.exceptions.ObjectDoesNotExist' imported but unused
```

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

Show all issues

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

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

Show all issues

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

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

Show all issues

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

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.errors.PERMISSION_DENIED' imported but unused
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.errors.NOT_LOGGED_IN' imported but unused
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.errors.DOES_NOT_EXIST' imported but unused
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.fields.IntFieldType' imported but unused
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 1)
Show all issues
```
F401 'reviewboard.diffviewer.models.FileDiff' imported but unused
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 1)
Show all issues
```
F401 'reviewboard.webapi.resources.resources' imported but unused
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 1)
Show all issues
```
E265 block comment should start with '# '
```
reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 1)
Show all issues
```
W293 blank line contains whitespace
```
reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 1)
Show all issues
```
E122 continuation line missing indentation or outdented
```
reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 1)
Show all issues
```
E122 continuation line missing indentation or outdented
```
reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 1)
Show all issues
```
E122 continuation line missing indentation or outdented
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
F401 'django.core.exceptions.ObjectDoesNotExist' imported but unused
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.util.decorators.augment_method_from' imported but unused
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.decorators.webapi_response_errors' imported but unused
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.decorators.webapi_login_required' imported but unused
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.decorators.webapi_request_fields' imported but unused
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.errors.PERMISSION_DENIED' imported but unused
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.errors.INVALID_FORM_DATA' imported but unused
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.errors.NOT_LOGGED_IN' imported but unused
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
F401 'djblets.webapi.errors.DOES_NOT_EXIST' imported but unused
```

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

Show all issues

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

reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
F401 'reviewboard.webapi.resources.resources' imported but unused
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 1)
Show all issues
```
E225 missing whitespace around operator
```

Commits:

	Summary	Author
-	register comments root APIs	charlie-xiang
+	register comments root APIs	charlie-xiang
+	fix a bug in base file attachment comment from last commit	charlie-xiang
+	add user name filter and add diff comment test suites	charlie-xiang

Diff:

Revision 2 (+798 -88)

Show changes

	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/base_file_attachment_comment.py
	reviewboard/webapi/resources/base_review_general_comment.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/root_diff_comment.py
	reviewboard/webapi/resources/root_file_attachment_comment.py
	reviewboard/webapi/resources/root_general_comment.py
	reviewboard/webapi/tests/mimetypes.py
	2 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 2)
Show all issues
```
E999 SyntaxError: invalid syntax
```

Commits:

	Summary	Author
-	register comments root APIs	charlie-xiang
-	fix a bug in base file attachment comment from last commit	charlie-xiang
-	add user name filter and add diff comment test suites	charlie-xiang
+	register comments root APIs	charlie-xiang
+	fix a bug in base file attachment comment from last commit	charlie-xiang
+	add user name filter and add diff comment test suites	charlie-xiang
+	add user tests to diff comment	charlie-xiang

Diff:

Revision 3 (+864 -110)

Show changes

	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/base_file_attachment_comment.py
	reviewboard/webapi/resources/base_review_general_comment.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/root_diff_comment.py
	reviewboard/webapi/resources/root_file_attachment_comment.py
	reviewboard/webapi/resources/root_general_comment.py
	reviewboard/webapi/tests/mimetypes.py
	2 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 3)
Show all issues
```
E999 SyntaxError: invalid syntax
```

Description:

		Registered root diff comments, root file attachment comments and root general comments API
~		Support GET method for list resources with query parameter: `review-request-id` , `review-id`
	~	Support GET method for list resources with query parameter: `review-request-id=<review-request-id>` , `review-id=<review-id>`, `user=<user-username>`
	+	Unit tests suites for comments API

Testing Done:

~		No unit test was added. Will look into how unit testw work and set up tests for comments api next week
	~	Added unit tests for `diff-comment`
	+	Still need to add tests for `general-comment` and `file-attachment-comment`

Change Summary:

add more query parameters and added tests for general-comment and file-attachment-comment

Description:

		Registered root diff comments, root file attachment comments and root general comments API
~		Support GET method for list resources with query parameter: `review-request-id=<review-request-id>` , `review-id=<review-id>`, `user=<user-username>`
	~	Support GET method for list resources with query parameter: `review-request-id=<review-request-id>` , `review-id=<review-id>`, `user=<user-username>`, `file-diff-id` for diff comments, and `file-attachment-id` `file-name` for file attachment comments
		Unit tests suites for comments API

Testing Done:

~		Added unit tests for `diff-comment`
	~	Added unit tests for `diff-comment`, `general-comment` and `file-attachment-comment` with list resource and GET method
-		Still need to add tests for `general-comment` and `file-attachment-comment`

Commits:

	Summary	Author
-	register comments root APIs	charlie-xiang
-	fix a bug in base file attachment comment from last commit	charlie-xiang
-	add user name filter and add diff comment test suites	charlie-xiang
-	add user tests to diff comment	charlie-xiang
+	register comments root APIs	charlie-xiang
+	fix a bug in base file attachment comment from last commit	charlie-xiang
+	add user name filter and add diff comment test suites	charlie-xiang
+	add user tests to diff comment	charlie-xiang
+	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang

Diff:

Revision 4 (+1720 -122)

Show changes

	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/base_file_attachment_comment.py
	reviewboard/webapi/resources/base_review_general_comment.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/root_diff_comment.py
	reviewboard/webapi/resources/root_file_attachment_comment.py
	reviewboard/webapi/resources/root_general_comment.py
	5 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/__init__.py (Diff revision 4)
Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 4)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 4)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 4)
Show all issues
```
E999 SyntaxError: invalid syntax
```

Change Summary:

add last-updated-from, to and is_reply filters

Commits:

	Summary	Author
-	register comments root APIs	charlie-xiang
-	fix a bug in base file attachment comment from last commit	charlie-xiang
-	add user name filter and add diff comment test suites	charlie-xiang
-	add user tests to diff comment	charlie-xiang
-	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
+	register comments root APIs	charlie-xiang
+	fix a bug in base file attachment comment from last commit	charlie-xiang
+	add user name filter and add diff comment test suites	charlie-xiang
+	add user tests to diff comment	charlie-xiang
+	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
+	add last-updated-from, to and is_reply filters	charlie-xiang

Diff:

Revision 5 (+1827 -141)

Show changes

	reviewboard/webapi/resources/__init__.py
	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/base_file_attachment_comment.py
	reviewboard/webapi/resources/base_review_general_comment.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/root_diff_comment.py
	reviewboard/webapi/resources/root_file_attachment_comment.py
	reviewboard/webapi/resources/root_general_comment.py
	5 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/__init__.py (Diff revision 5)
Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 5)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 5)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 5)
Show all issues
```
E999 SyntaxError: invalid syntax
```

```
Great start!
```

reviewboard/webapi/resources/base_diff_comment.py (Diff revision 5)

Show all issues

This is getting long enough that we should just wrap and list one argument per line.

reviewboard/webapi/resources/base_diff_comment.py (Diff revision 5)

Show all issues

While you're here, mind updating this docstring to follow our modern standards? "Returns" -> "Return", and add "Args" and "Returns" sections to the bottom.

reviewboard/webapi/resources/base_file_attachment_comment.py (Diff revision 5)

Show all issues

Instead of doing this, let's avoid listing review_request_id in the args list, and pull it out for our local use:

def get_queryset(self, request, *args, **kwargs):
    review_request_id = kwargs.get('review_request_id', None)


Since we're not actually using the ID, we might even be able to just do:

if 'review_request_id' in kwargs:
    review_request = ...
    q = q.filter(...)

reviewboard/webapi/resources/base_review_general_comment.py (Diff revision 5)
Show all issues
```
Same comments here re: kwargs.
```

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 5)

Show all issues

New files on master won't require the unicode_literals import, but we would like to include module docstrings at the top of the file.

reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 5)
Show all issues
```
Same here re: unicode_literals, docstrings.
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 5)
Show all issues
```
Same here re: unicode_literals, docstrings.
```
reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 5)
Show all issues
```
Same here re: unicode_literals, docstrings.
```
reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 5)
Show all issues
```
And here.
```
reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 5)
Show all issues
```
And here.
```

Change Summary:

Added tests for last-updated-to, last-updated-from, is-reply, fixed the review issues

Commits:

	Summary	Author
-	register comments root APIs	charlie-xiang
-	fix a bug in base file attachment comment from last commit	charlie-xiang
-	add user name filter and add diff comment test suites	charlie-xiang
-	add user tests to diff comment	charlie-xiang
-	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
-	add last-updated-from, to and is_reply filters	charlie-xiang
+	register comments root APIs	charlie-xiang
+	fix a bug in base file attachment comment from last commit	charlie-xiang
+	add user name filter and add diff comment test suites	charlie-xiang
+	add user tests to diff comment	charlie-xiang
+	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
+	add last-updated-from, to and is_reply filters	charlie-xiang
+	address reviews feedback	charlie-xiang
+	added tests for last-updated and is-reply	charlie-xiang

Diff:

Revision 6 (+2255 -161)

Show changes

	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/base_file_attachment_comment.py
	reviewboard/webapi/resources/base_review_general_comment.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/root_diff_comment.py
	reviewboard/webapi/resources/root_file_attachment_comment.py
	reviewboard/webapi/resources/root_general_comment.py
	reviewboard/webapi/tests/mimetypes.py
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 6)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 6)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 6)
Show all issues
```
E999 SyntaxError: invalid syntax
```

Change Summary:

Added webapi_request_fields decorator to get_list method for API GET parameters documentation

Commits:

	Summary	Author
-	register comments root APIs	charlie-xiang
-	fix a bug in base file attachment comment from last commit	charlie-xiang
-	add user name filter and add diff comment test suites	charlie-xiang
-	add user tests to diff comment	charlie-xiang
-	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
-	add last-updated-from, to and is_reply filters	charlie-xiang
-	address reviews feedback	charlie-xiang
-	added tests for last-updated and is-reply	charlie-xiang
+	register comments root APIs	charlie-xiang
+	fix a bug in base file attachment comment from last commit	charlie-xiang
+	add user name filter and add diff comment test suites	charlie-xiang
+	add user tests to diff comment	charlie-xiang
+	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
+	add last-updated-from, to and is_reply filters	charlie-xiang
+	address reviews feedback	charlie-xiang
+	added tests for last-updated and is-reply	charlie-xiang
+	add webapi_request_fields documentations	charlie-xiang

Diff:

Revision 7 (+2664 -182)

Show changes

	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/base_file_attachment_comment.py
	reviewboard/webapi/resources/base_review_general_comment.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/root_diff_comment.py
	reviewboard/webapi/resources/root_file_attachment_comment.py
	reviewboard/webapi/resources/root_general_comment.py
	reviewboard/webapi/tests/mimetypes.py
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 7)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 7)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 7)
Show all issues
```
E999 SyntaxError: invalid syntax
```

Change Summary:

added webapi_request_fields decorator to get_list and resource class docstring

Description:

		Registered root diff comments, root file attachment comments and root general comments API
~		Support GET method for list resources with query parameter: `review-request-id=<review-request-id>` , `review-id=<review-id>`, `user=<user-username>`, `file-diff-id` for diff comments, and `file-attachment-id` `file-name` for file attachment comments
~		Unit tests suites for comments API
	~	Support GET method for list resources with query parameter:
	~	Shared query parameters : `review-request-id=<review-request-id>` , `review-id=<review-id>`, `user=<user-username>`, `is-reply`
	+	`file-diff-id`, `line` and `interdiff-revision` for diff comments, and `file-attachment-id` `file-name` for file attachment comments
	+	Unit tests suites for comments API
	+	Added added `webapi_request_fields` decorator to `get_list` for request parameters documentation
	+	Added resource class docstrings.

Commits:

	Summary	Author
-	register comments root APIs	charlie-xiang
-	fix a bug in base file attachment comment from last commit	charlie-xiang
-	add user name filter and add diff comment test suites	charlie-xiang
-	add user tests to diff comment	charlie-xiang
-	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
-	add last-updated-from, to and is_reply filters	charlie-xiang
-	address reviews feedback	charlie-xiang
-	added tests for last-updated and is-reply	charlie-xiang
-	add webapi_request_fields documentations	charlie-xiang
+	register comments root APIs	charlie-xiang
+	fix a bug in base file attachment comment from last commit	charlie-xiang
+	add user name filter and add diff comment test suites	charlie-xiang
+	add user tests to diff comment	charlie-xiang
+	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
+	add last-updated-from, to and is_reply filters	charlie-xiang
+	address reviews feedback	charlie-xiang
+	added tests for last-updated and is-reply	charlie-xiang
+	add webapi_request_fields documentations and resource class docstring	charlie-xiang

Diff:

Revision 8 (+2700 -182)

Show changes

	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/base_file_attachment_comment.py
	reviewboard/webapi/resources/base_review_general_comment.py
	reviewboard/webapi/resources/root.py
	reviewboard/webapi/resources/root_diff_comment.py
	reviewboard/webapi/resources/root_file_attachment_comment.py
	reviewboard/webapi/resources/root_general_comment.py
	reviewboard/webapi/tests/mimetypes.py
	4 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 8)
Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 8)
Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 8)
Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 8)
Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 8)
Show all issues
```
W291 trailing whitespace
```
reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 8)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 8)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 8)
Show all issues
```
E999 SyntaxError: invalid syntax
```

The documentation stuff looks cool - I'm going to implement it the same way on my project!

Change Summary:

update to use parsed parameters and added doc tree

Commits:

	Summary	Author
-	register comments root APIs	charlie-xiang
-	fix a bug in base file attachment comment from last commit	charlie-xiang
-	add user name filter and add diff comment test suites	charlie-xiang
-	add user tests to diff comment	charlie-xiang
-	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
-	add last-updated-from, to and is_reply filters	charlie-xiang
-	address reviews feedback	charlie-xiang
-	added tests for last-updated and is-reply	charlie-xiang
-	add webapi_request_fields documentations and resource class docstring	charlie-xiang
+	register comments root APIs	charlie-xiang
+	fix a bug in base file attachment comment from last commit	charlie-xiang
+	add user name filter and add diff comment test suites	charlie-xiang
+	add user tests to diff comment	charlie-xiang
+	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
+	add last-updated-from, to and is_reply filters	charlie-xiang
+	address reviews feedback	charlie-xiang
+	added tests for last-updated and is-reply	charlie-xiang
+	add webapi_request_fields documentations and resource class docstring	charlie-xiang
+	update to use parsed parameters and added doc tree	charlie-xiang

Diff:

Revision 9 (+2770 -248)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/root-diff-comment-list.rst
	docs/manual/webapi/2.0/resources/root-file-attachment-comment-list.rst
	docs/manual/webapi/2.0/resources/root-general-comment-list.rst
	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/base_file_attachment_comment.py
	reviewboard/webapi/resources/base_review_general_comment.py
	reviewboard/webapi/resources/root.py
	8 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 9)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 9)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 9)
Show all issues
```
E999 SyntaxError: invalid syntax
```

Summary:

-	[WIP]: Add root comments APIs
+	Add root comments APIs

Description:

		Registered root diff comments, root file attachment comments and root general comments API
		Support GET method for list resources with query parameter:
		Shared query parameters : `review-request-id=<review-request-id>` , `review-id=<review-id>`, `user=<user-username>`, `is-reply`
		`file-diff-id`, `line` and `interdiff-revision` for diff comments, and `file-attachment-id` `file-name` for file attachment comments
		Unit tests suites for comments API
		Added added `webapi_request_fields` decorator to `get_list` for request parameters documentation
~		Added resource class docstrings.
	~	Updated the WebAPI documentations

Fix it!

reviewboard/webapi/resources/base_file_attachment_comment.py (Diff revision 9)
Show all issues
```
I know it was like this before, but can you add a docstring?
```
reviewboard/webapi/resources/base_file_attachment_comment.py (Diff revision 9)
Show all issues
```
Add a blank line after this.
```

reviewboard/webapi/resources/base_file_attachment_comment.py (Diff revision 9)

Show all issues

dict.get() returns None if the key doesn't exist, so we can simplify:

if kwargs.get('review_request_id') is not None:

reviewboard/webapi/resources/base_review_general_comment.py (Diff revision 9)
Show all issues
```
I know it was like this before, but can you add a docstring?
```
reviewboard/webapi/resources/base_review_general_comment.py (Diff revision 9)
Show all issues
```
Add  a blank line after this.
```

reviewboard/webapi/resources/base_review_general_comment.py (Diff revision 9)

Show all issues

dict.get() returns None if the key doesn't exist, so we can simplify:

if kwargs.get('review_request_id') is not None:

reviewboard/webapi/resources/base_review_general_comment.py (Diff revision 9)

Show all issues

Instead of having multiple return paths, let's just reassign q. Please also add a blank line above the return:
```python

    q = q.filter(

        Q(review__review_request=review_request))
return q

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 9)
Show all issues
```
It's not "different comments", it's "diff comments" :)
```

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 9)

Show all issues

Imports should be in three groups, separated by blank lines and alphabetized within:

Standard library
Third-party
"Current" module (anything in reviewboard).

There aren't any for group 1, so in this case, we should have two:

All django and djblets imports
All review board imports

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 9)

Show all issues

This is "new" right now but won't be new forever. Just call it "This top level ..."

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 9)
Show all issues
```
Add a blank line between these two.
```

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 9)

Show all issues

This should be in the imperative mood ("Return" instead of "Returns"). This docstring also needs an "Args" section.

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 9)
Show all issues
```
Add a blank line between these two.
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 9)
Show all issues
```
This should be in the imperative mood.
```
reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 9)
Show all issues
```
Same comments in this file as in the diff comments file.
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 9)
Show all issues
```
Same comments in this file as in the diff comments file.
```

reviewboard/webapi/tests/mimetypes.py (Diff revision 9)

Show all issues

I don't know that it makes sense to have new mimetypes for these endpoints, since the content is effectively the same as the other endpoints for the same objects.

Chaoyu April 7, 2022, 9:45 p.m.

I removed the new mimetypes for file_attachment_comment and general_comment but kept the one for diff_comment as there's no existing mimetype for diff_comment

reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 9)
Show all issues
```
There's an extra space before "Unit"
```
reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 9)
Show all issues
```
No blank line here. Please also alphabetize the import list.
```

reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 9)

Show all issues

Instead of writing "diff_comments/" in each of these docstrings, let's just put "<URL>/..." and then use the @webapi_test_template decorator.

reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 9)

Show all issues

A few things here:

Alphabetize the list
No blank line within the section
For the tests.urls import, put the import on the first line rather than the second.

reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 9)
Show all issues
```
Let's use @webapi_test_template for all these.
```

reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 9)

Show all issues

Same comments here as in the other files.

reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 9)
Show all issues
```
Let's use @webapi_test_template for all the docstrings here.
```

sheenaNg April 4, 2022, 7:38 p.m.

I think you did a great job overall! I added some comments but they are mostly minor styling issues such as missing blank lines and minor typo.

Chaoyu April 7, 2022, 9:45 p.m.
```
Good catch! Thanks
```

reviewboard/webapi/resources/base_diff_comment.py (Diff revision 9)
Show all issues
```
I think there is a typo here. Should it be "list" resources?
```
reviewboard/webapi/resources/root_diff_comment.py (Diff revision 9)
Show all issues
```
Can you add a line between these two lines?
```

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 9)

Show all issues

This is something minor, you missed the period at the end of the description.

reviewboard/webapi/resources/root_diff_comment.py (Diff revision 9)
Show all issues
```
You missed the period at the end of the description.
```

reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 9)

Show all issues

There should be a blank line after the class docstring. Can you add a blank line here?

reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 9)
Show all issues
```
Can you also add a blank line between these two lines?
```

reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 9)

Show all issues

Can you add a blank line after q = Q() and also after each if statement?

reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 9)
Show all issues
```
You missed the period at the end of the description.
```
reviewboard/webapi/resources/root_file_attachment_comment.py (Diff revision 9)
Show all issues
```
You missed the period at the end of the description.
```

reviewboard/webapi/resources/root_general_comment.py (Diff revision 9)

Show all issues

There should be a blank line after the class docstring. Can you add a blank line here?

reviewboard/webapi/resources/root_general_comment.py (Diff revision 9)
Show all issues
```
Can you add a blank line between these two lines?
```

reviewboard/webapi/resources/root_general_comment.py (Diff revision 9)

Show all issues

Can you add a blank line after q = Q() and also after each if statement?

reviewboard/webapi/resources/root_general_comment.py (Diff revision 9)
Show all issues
```
You missed the period at the end of the description.
```
reviewboard/webapi/resources/root_general_comment.py (Diff revision 9)
Show all issues
```
You missed the period at the end of the description.
```

reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 9)

Show all issues

I think it is great overall.
One suggestion: Would it be easier to read if you add some blank lines to separate different groups of codes?

For example, add blank line after the codes to create diff_review, reviews/ comments and before calling the API endpoint. Then, add another blank line after calling the API endpoint and before the assertions.

This could be applied to the rest of the unit tests in multiple files.

reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 9)
Show all issues
```
I do not think there should be a blank line after function docstring.
```
reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 9)
Show all issues
```
I do not think there should be a blank line after function docstring.
```

Change Summary:

worked on review comments

Commits:

	Summary	Author
-	register comments root APIs	charlie-xiang
-	fix a bug in base file attachment comment from last commit	charlie-xiang
-	add user name filter and add diff comment test suites	charlie-xiang
-	add user tests to diff comment	charlie-xiang
-	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
-	add last-updated-from, to and is_reply filters	charlie-xiang
-	address reviews feedback	charlie-xiang
-	added tests for last-updated and is-reply	charlie-xiang
-	add webapi_request_fields documentations and resource class docstring	charlie-xiang
-	update to use parsed parameters and added doc tree	charlie-xiang
+	register comments root APIs	charlie-xiang
+	fix a bug in base file attachment comment from last commit	charlie-xiang
+	add user name filter and add diff comment test suites	charlie-xiang
+	add user tests to diff comment	charlie-xiang
+	add filediff id query parameter for diff comment, file attachment parameters for file attachment, add tests	charlie-xiang
+	add last-updated-from, to and is_reply filters	charlie-xiang
+	address reviews feedback	charlie-xiang
+	added tests for last-updated and is-reply	charlie-xiang
+	add webapi_request_fields documentations and resource class docstring	charlie-xiang
+	update to use parsed parameters and added doc tree	charlie-xiang
+	fix review comments	charlie-xiang

Diff:

Revision 10 (+3190 -370)

Show changes

	docs/manual/webapi/2.0/resources/index.rst
	docs/manual/webapi/2.0/resources/root-diff-comment-list.rst
	docs/manual/webapi/2.0/resources/root-file-attachment-comment-list.rst
	docs/manual/webapi/2.0/resources/root-general-comment-list.rst
	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/base_file_attachment_comment.py
	reviewboard/webapi/resources/base_review_general_comment.py
	reviewboard/webapi/resources/root.py
	8 more

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/tests/test_root_diff_comment.py (Diff revision 10)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_file_attachment_comment.py (Diff revision 10)
Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/tests/test_root_general_comment.py (Diff revision 10)
Show all issues
```
E999 SyntaxError: invalid syntax
```

Status: Discarded

Change Summary:

This work has moved to /r/12296