Summary

Update and improve the comments hint computation for the diff viewer.

Review Request #13667 — Created March 25, 2024 and submitted April 12, 2024, 12:38 p.m.

Information

Owner

david

Repository

Review Board

Branch

release-7.x

Bugs

Depends On

Reviewers

Groups

reviewboard

People

Description

This new implementation simplifies things a little bit by compting more
information at the ReviewsDiffViewerView level, passing it down to the
DiffViewerView, instead of letting the superclass do things and then
pull the data back out of the context structure.

This makes a couple other small improvements:

The social page info definitions have been consolidated into the base
ReviewRequestContext, and arguments added to
make_review_request_context to set them there.
The way that diff commits and other objects are queried has been
simplified so we don't fetch things more than once.

This does not yet add UI for commit-specific comments.

Testing Done


Ran unit tests.
Tested all the different diff viewer views, including single diffs,

  interdiffs, and commit diffs.
Tested that the commit list and commit history diffs worked correctly.
Tested that the comments hint (in a future change) correctly listed

  all diffs, interdiffs, and commit range comments included in the

  pending review.

Commits

Summary	ID
Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly. - Tested that the comments hint (in a future change) correctly listed all diffs, interdiffs, and commit range comments included in the pending review. Reviewed at https://reviews.reviewboard.org/r/13667/	f0ef9f21fc06d3d2daa32bfae039c5539abd6c47

Summary

Update and improve the comments hint computation for the diff viewer.

This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly. - Tested that the comments hint (in a future change) correctly listed all diffs, interdiffs, and commit range comments included in the pending review. Reviewed at https://reviews.reviewboard.org/r/13667/

f0ef9f21fc06d3d2daa32bfae039c5539abd6c47

Issues

Description	From	Last Updated
undefined name 'diff_commits' Column: 49 Error code: F821	reviewbot	March 27, 2024, 1:51 p.m.
undefined name 'interdiff_commits' Column: 49 Error code: F821	reviewbot	March 27, 2024, 1:51 p.m.
'reviewboard.diffviewer.models.diffcommit.SerializedDiffCommit' imported but unused Column: 5 Error code: F401	reviewbot	March 27, 2024, 1:51 p.m.
redefinition of unused '_get_comments_hint' from line 525 Column: 5 Error code: F811	reviewbot	March 27, 2024, 1:51 p.m.
We're sort of undoing an important optimization here, going from one query for commits spanning diffsets to now 3 queries. …	chipx86	April 2, 2024, 5:29 p.m.
social-page_title -> social_page_title	chipx86	April 2, 2024, 10:46 a.m.
With the above fix, I'd expect all of these to go back to their original values.	chipx86	April 2, 2024, 5:28 p.m.
Summary must be one line.	chipx86	April 10, 2024, 9:20 a.m.
Can we pass these in as keyword-only args?	chipx86	April 2, 2024, 10:47 a.m.
Can we sort keys?	chipx86	April 2, 2024, 10:47 a.m.
To check: Are we pre-fetching associated reviews before this happens?	chipx86	April 2, 2024, 10:48 a.m.
Since we only use this when we append, we could just use this inline below. Makes it a bit more …	chipx86	April 2, 2024, 10:49 a.m.
Can we sort keys?	chipx86	April 2, 2024, 10:49 a.m.
We don't normally use the outer parens for the tuple in this case.	chipx86	April 2, 2024, 10:49 a.m.
Can we sort the keys?	chipx86	April 2, 2024, 10:49 a.m.
Same comment about parens.	chipx86	April 2, 2024, 10:49 a.m.
Blank line here.	chipx86	April 2, 2024, 10:50 a.m.
Probably no need for a blank line here.	chipx86	April 2, 2024, 10:50 a.m.
This can just be `bool(self.interdiffset and draft_diffset == self.interdiffset)	maubin	April 10, 2024, 1:13 p.m.
We access draft.diffset a lot here, you could pull that out into a variable, and have it be None if …	maubin	April 10, 2024, 11:14 a.m.
This can just be if draft_diffset:	maubin	April 10, 2024, 1:13 p.m.
Missing a "Version Added"	maubin	April 10, 2024, 11:12 a.m.

flake8 passed.

JSHint passed.

Change Summary:


Get all commits when we get the filediffs.
Properly change DiffViewerView.get_context to be keyword-only.
Include the new comments hint computation.

Testing Done:

		Ran unit tests.
		Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs.
		Tested that the commit list and commit history diffs worked correctly.
	+	Tested that the comments hint (in a future change) correctly listed all diffs, interdiffs, and commit range comments included in the pending review.

Commits:

	Summary	ID
	Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the `ReviewsDiffViewerView` level, passing it down to the `DiffViewerView`, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to `make_review_request_context` to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly.	ebed0eaa70128e03697028bbf80f2ff63383bc18
	Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly.	0b6025eaf431c465d976eea4c44c88751be7c207

Diff:

Revision 2 (+1386 -404)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/context.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/tests/test_review_request_page_data.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/reviews/views/review_request_detail.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/diffviewer/views.py (Diff revision 2)
The issue has been dropped. Show all issues
```
undefined name 'diff_commits'

Column: 49
Error code: F821
```
reviewboard/diffviewer/views.py (Diff revision 2)
The issue has been dropped. Show all issues
```
undefined name 'interdiff_commits'

Column: 49
Error code: F821
```

reviewboard/reviews/views/diffviewer.py (Diff revision 2)

The issue has been dropped. Show all issues

'reviewboard.diffviewer.models.diffcommit.SerializedDiffCommit' imported but unused

Column: 5
Error code: F401

reviewboard/reviews/views/diffviewer.py (Diff revision 2)
The issue has been dropped. Show all issues
```
redefinition of unused '_get_comments_hint' from line 525

Column: 5
Error code: F811
```

Change Summary:

Fix a merge-o

Commits:

	Summary	ID
	Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly.	0b6025eaf431c465d976eea4c44c88751be7c207
	Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly. - Tested that the comments hint (in a future change) correctly listed all diffs, interdiffs, and commit range comments included in the pending review.	c226b91431518075dcc07b7b2e29917f7ff1d285

Diff:

Revision 3 (+958 -406)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/context.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/tests/test_review_request_page_data.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/reviews/views/review_request_detail.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/diffviewer/views.py (Diff revision 3)

The issue has been resolved. Show all issues

We're sort of undoing an important optimization here, going from one query for commits spanning diffsets to now 3 queries.
Right off the bat, one of those can be eliminated (we do interdiffset.commits.all() twice). 
Both can be consolidated by looking up DiffCommit entries spanning either diffset ID, and then grouping together in a post-process. This is what we had up above, and it brings us back down to 1.
Relevant logic from before:

diffset_pks = [diffset.pk]

if interdiffset:
    diffset_pks.append(interdiffset.pk)

commits = DiffCommit.objects.filter(diffset_id__in=diffset_pks)

for commit in commits:
    commits_by_diffset_id.setdefault(commit.diffset_id, []).append(
        commit)

david April 2, 2024, 5:30 p.m.

So I had added commits to the prefetch_related in ReviewRequest.get_diffsets, which meant that these repeated calls weren't actually hitting the DB. That said, I'll move it back into here.

reviewboard/reviews/context.py (Diff revision 3)
The issue has been resolved. Show all issues
```
social-page_title -> social_page_title
```
reviewboard/reviews/tests/test_review_request_page_data.py (Diff revision 3)
The issue has been resolved. Show all issues
```
With the above fix, I'd expect all of these to go back to their original values.
```
reviewboard/reviews/views/diffviewer.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we pass these in as keyword-only args?
```
reviewboard/reviews/views/diffviewer.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we sort keys?
```

reviewboard/reviews/views/diffviewer.py (Diff revision 3)

The issue has been dropped. Show all issues

To check: Are we pre-fetching associated reviews before this happens?

david April 2, 2024, 5:30 p.m.

Yes, that happens above where we're querying Review.comments.through.objects

reviewboard/reviews/views/diffviewer.py (Diff revision 3)

The issue has been resolved. Show all issues

Since we only use this when we append, we could just use this inline below. Makes it a bit more readable.

reviewboard/reviews/views/diffviewer.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we sort keys?
```
reviewboard/reviews/views/diffviewer.py (Diff revision 3)
The issue has been resolved. Show all issues
```
We don't normally use the outer parens for the tuple in this case.
```
reviewboard/reviews/views/diffviewer.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Can we sort the keys?
```
reviewboard/reviews/views/diffviewer.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Same comment about parens.
```
reviewboard/reviews/views/diffviewer.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Blank line here.
```
reviewboard/reviews/views/diffviewer.py (Diff revision 3)
The issue has been resolved. Show all issues
```
Probably no need for a blank line here.
```

Commits:

	Summary	ID
	Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly. - Tested that the comments hint (in a future change) correctly listed all diffs, interdiffs, and commit range comments included in the pending review.	c226b91431518075dcc07b7b2e29917f7ff1d285
	Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly. - Tested that the comments hint (in a future change) correctly listed all diffs, interdiffs, and commit range comments included in the pending review.	7ae9fd2ab375579b2da8a879c0492bcbdd5ca88e

Diff:

Revision 4 (+1006 -422)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/context.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/reviews/views/review_request_detail.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

reviewboard/reviews/views/diffviewer.py (Diff revisions 3 - 4)
The issue has been resolved. Show all issues
```
Summary must be one line.
```

reviewboard/reviews/views/diffviewer.py (Diff revision 4)

The issue has been resolved. Show all issues

We access draft.diffset a lot here, you could pull that out into a variable, and have it be None if draft is None. That'd also avoid the repeated draft and draft.diffset checks.

reviewboard/reviews/views/diffviewer.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Missing a "Version Added"
```

Commits:

	Summary	ID
	Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly. - Tested that the comments hint (in a future change) correctly listed all diffs, interdiffs, and commit range comments included in the pending review.	7ae9fd2ab375579b2da8a879c0492bcbdd5ca88e
	Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly. - Tested that the comments hint (in a future change) correctly listed all diffs, interdiffs, and commit range comments included in the pending review. Reviewed at https://reviews.reviewboard.org/r/13667/	9129080f53b94d141dadcd352b26efdf675d894f

Diff:

Revision 5 (+1006 -422)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/context.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/reviews/views/review_request_detail.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/views/diffviewer.py (Diff revisions 4 - 5)
The issue has been resolved. Show all issues
```
This can just be `bool(self.interdiffset and draft_diffset == self.interdiffset)
```
reviewboard/reviews/views/diffviewer.py (Diff revisions 4 - 5)
The issue has been resolved. Show all issues
```
This can just be if draft_diffset:
```

Commits:

	Summary	ID
	Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly. - Tested that the comments hint (in a future change) correctly listed all diffs, interdiffs, and commit range comments included in the pending review. Reviewed at https://reviews.reviewboard.org/r/13667/	9129080f53b94d141dadcd352b26efdf675d894f
	Update and improve the comments hint computation for the diff viewer. This change makes major updates to the comments hint structure that gets included in the diff viewer page data. The major driver for this is that the old implementation did not include correct information about comments that were made against commit ranges. It was also doing a bunch of extra queries, using the database to generate the comments information when we already had most of what we needed fetched. This new implementation simplifies things a little bit by compting more information at the ReviewsDiffViewerView level, passing it down to the DiffViewerView, instead of letting the superclass do things and then pull the data back out of the context structure. This makes a couple other small improvements: - The social page info definitions have been consolidated into the base ReviewRequestContext, and arguments added to make_review_request_context to set them there. - The way that diff commits and other objects are queried has been simplified so we don't fetch things more than once. This does not yet add UI for commit-specific comments. Testing Done: - Ran unit tests. - Tested all the different diff viewer views, including single diffs, interdiffs, and commit diffs. - Tested that the commit list and commit history diffs worked correctly. - Tested that the comments hint (in a future change) correctly listed all diffs, interdiffs, and commit range comments included in the pending review. Reviewed at https://reviews.reviewboard.org/r/13667/	f0ef9f21fc06d3d2daa32bfae039c5539abd6c47

Diff:

Revision 6 (+1006 -424)

Show changes

	reviewboard/diffviewer/views.py
	reviewboard/reviews/context.py
	reviewboard/reviews/models/review_request.py
	reviewboard/reviews/ui/base.py
	reviewboard/reviews/views/diffviewer.py
	reviewboard/reviews/views/review_request_detail.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (5499649)