Summary

Add an API for commenting on FileDiffs ranges

Review Request #10190 — Created Oct. 19, 2018 and submitted Oct. 26, 2018, 5:28 a.m.

Information

Owner

brennie

Repository

Review Board

Branch

release-4.0.x

Bugs

Depends On

~~10136~~

Blocks

10242

Commit

b959551...

Reviewers

Groups

reviewboard

People

Description

The ReviewDiffCommentResource is now capable of creating comments on
FileDiffs with a base FileDiff ID. This allows us to comment on
FileDiffs in commit ranges and keep the context of the base FileDiff.

Testing Done

Ran unit tests.

Issues

Description	From	Last Updated
E128 continuation line under-indented for visual indent	reviewbot	Oct. 23, 2018, 9:29 p.m.
F841 local variable 'rsp' is assigned to but never used	reviewbot	Oct. 23, 2018, 9:29 p.m.
Seems like this should be "The ID fo the base FileDiff" or "The base FileDiff ID"	chipx86	Oct. 24, 2018, 10:10 p.m.
This would be more concise as: return (self.extra_data and self.extra_data.get(...))	chipx86	Oct. 24, 2018, 10:11 p.m.
Mind putting :term: around "cumulative diff?" We'll still need to define it, but it's such an insider term that I …	chipx86	Oct. 24, 2018, 10:11 p.m.
I don't understand what this means.	chipx86	Oct. 24, 2018, 10:09 p.m.
We should handle the case of this not existing. I know it should exist, but we've seen cases like this …	chipx86	Oct. 24, 2018, 10:11 p.m.
Alphabetical order.	chipx86	Oct. 24, 2018, 10:12 p.m.
Same comments as above with the terms and referencing commit histories.	chipx86	Oct. 24, 2018, 10:20 p.m.
This really should have a period at the end. I know it didn't before, but most of our errors do. …	chipx86	Oct. 24, 2018, 10:22 p.m.
This gets pretty deeply nested, and the checking of base_filediff_id here and the setting in the else clause can initially …	chipx86	Oct. 24, 2018, 10:22 p.m.
This should be one statement.	chipx86	Oct. 24, 2018, 10:23 p.m.
Typo: "Comapare" -> "Compare" Also, I think the "with" before "response" shouldn't be there.	chipx86	Oct. 24, 2018, 10:23 p.m.
Let's just compare for truthiness. This is a bit fragile otherwise.	chipx86	Oct. 24, 2018, 10:23 p.m.
Missing a docstring.	chipx86	Oct. 24, 2018, 10:24 p.m.
"commit history"	chipx86	Oct. 24, 2018, 10:27 p.m.
E999 SyntaxError: invalid syntax	reviewbot	Oct. 24, 2018, 10:28 p.m.
F401 'reviewboard.diffviewer.models.FileDiff' imported but unused	reviewbot	Oct. 24, 2018, 10:29 p.m.
E303 too many blank lines (2)	reviewbot	Oct. 24, 2018, 10:29 p.m.
Let's use :term: here for "cumulative diff," since we now have it.	chipx86	Oct. 26, 2018, 4:50 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/webapi/resources/review_diff_comment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
E128 continuation line under-indented for visual indent
```
reviewboard/webapi/tests/test_review_comment.py (Diff revision 1)
The issue has been resolved. Show all issues
```
F841 local variable 'rsp' is assigned to but never used
```

Commit:

-	ba06d71ffbbe698c0343b46a122444c377c12d6b
+	2669689253ab702a180ed67ac95cc7ff2d7739bd

Diff:

Revision 2 (+871 -41)

Show changes

	reviewboard/reviews/models/diff_comment.py
	reviewboard/webapi/resources/base_comment.py
	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/review_diff_comment.py
	reviewboard/webapi/tests/test_review_comment.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/models/diff_comment.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Seems like this should be "The ID fo the base FileDiff" or "The base FileDiff ID"
```

reviewboard/reviews/models/diff_comment.py (Diff revision 2)

The issue has been resolved. Show all issues

This would be more concise as:

return (self.extra_data and
        self.extra_data.get(...))

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

The issue has been dropped. Show all issues

Mind putting :term: around "cumulative diff?" We'll still need to define it, but it's such an insider term that I think we'll need to if we're going to at all reference it publicly.

This should probably also mention that it's used for review requests with commit history.

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

The issue has been resolved. Show all issues

I don't understand what this means.

brennie Oct. 24, 2018, 5:15 p.m.

Unless I am mistaken, if we GET the list resource, each comment will be serialized and each one will do FileDiff.objects.get(...) which will result in N queries.

We can't augment the queryset to select_related because the relationship is stored in the JSON field. Thoughts?

chipx86 Oct. 24, 2018, 9:43 p.m.

Hmm. That is a problem, and will be an issue for larger customers and for RBCommons.

I have a possible solution in mind, but it's more work. So two questions for now:

Is this needed by consumers, or just nice info to have?
Is this represented as data in the payload, or a link to a resource?

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

The issue has been dropped. Show all issues

We should handle the case of this not existing. I know it should exist, but we've seen cases like this before with corruption/hand-modified tables.

reviewboard/webapi/resources/review_diff_comment.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Alphabetical order.
```
reviewboard/webapi/resources/review_diff_comment.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Same comments as above with the terms and referencing commit histories.
```

reviewboard/webapi/resources/review_diff_comment.py (Diff revision 2)

The issue has been resolved. Show all issues

This really should have a period at the end. I know it didn't before, but most of our errors do.

All the ones below should as well.

reviewboard/webapi/resources/review_diff_comment.py (Diff revision 2)

The issue has been resolved. Show all issues

This gets pretty deeply nested, and the checking of base_filediff_id here and the setting in the else clause can initially be confusing. How about:

if filediff is None or not dvcs_feaure.is_enabled(request=request):
    base_filediff_id = None

if base_filediff_id is not None:
    ...

reviewboard/webapi/resources/review_diff_comment.py (Diff revision 2)

The issue has been resolved. Show all issues

This should be one statement.

reviewboard/webapi/tests/test_review_comment.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Typo: "Comapare" -> "Compare"
Also, I think the "with" before "response" shouldn't be there.
```

reviewboard/webapi/tests/test_review_comment.py (Diff revision 2)

The issue has been dropped. Show all issues

Let's just compare for truthiness. This is a bit fragile otherwise.

brennie Oct. 24, 2018, 5:13 p.m.

How, exactly? If it is not None then it will be a dict. In which case we can do an assertNotIn.

Am I missing something?

chipx86 Oct. 24, 2018, 9:43 p.m.

Ah, I was reading that as an assertIn(), and was thinking this was assuming that if extra_data was converted to a dict, it would have this content. I felt the None vs. dict was an unsafe assumption. Disregard.

reviewboard/webapi/tests/test_review_comment.py (Diff revision 2)
The issue has been resolved. Show all issues
```
Missing a docstring.
```
reviewboard/webapi/tests/test_review_comment.py (Diff revision 2)
The issue has been resolved. Show all issues
```
"commit history"
```

Change Summary:

Addressed feedback.

Commit:

-	2669689253ab702a180ed67ac95cc7ff2d7739bd
+	23bc09a1302c353543e317f1d2ea99b9a3590a8a

Diff:

Revision 3 (+843 -40)

Show changes

	docs/manual/glossary.rst
	reviewboard/reviews/models/diff_comment.py
	reviewboard/webapi/resources/base_comment.py
	reviewboard/webapi/resources/base_diff_comment.py
	reviewboard/webapi/resources/review_diff_comment.py
	reviewboard/webapi/tests/test_review_comment.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/models/diff_comment.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E999 SyntaxError: invalid syntax
```
reviewboard/webapi/resources/base_diff_comment.py (Diff revision 3)
The issue has been resolved. Show all issues
```
F401 'reviewboard.diffviewer.models.FileDiff' imported but unused
```
reviewboard/webapi/resources/review_diff_comment.py (Diff revision 3)
The issue has been resolved. Show all issues
```
E303 too many blank lines (2)
```

Commit:

-	23bc09a1302c353543e317f1d2ea99b9a3590a8a
+	aab8ef49464cdbcf9dc7d4bfd10bc004600ba748

Diff:

Revision 4 (+824 -40)

Show changes

	docs/manual/glossary.rst
	reviewboard/reviews/models/diff_comment.py
	reviewboard/webapi/resources/base_comment.py
	reviewboard/webapi/resources/review_diff_comment.py
	reviewboard/webapi/tests/test_review_comment.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/webapi/resources/review_diff_comment.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Let's use :term: here for "cumulative diff," since we now have it.
```

Change Summary:

Addressed feedback

Commit:

-	aab8ef49464cdbcf9dc7d4bfd10bc004600ba748
+	b959551d51c5eeb67c598497d967f3c627e6dbb0

Diff:

Revision 5 (+824 -40)

Show changes

	docs/manual/glossary.rst
	reviewboard/reviews/models/diff_comment.py
	reviewboard/webapi/resources/base_comment.py
	reviewboard/webapi/resources/review_diff_comment.py
	reviewboard/webapi/tests/test_review_comment.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Status: Closed (submitted)

Change Summary:

Pushed to release-4.0.x (d14df94)