Summary

Fix comment links for file attachments in diffs.

Review Request #13681 — Created March 29, 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 is pretty complex because there are a lot of corner cases. In
particular, we have to deal with three annoying things:

We sometimes elide the base commit ID or tip commit ID from the URL,
depending on what commits are selected.
When a review request has commit history, the attachment is uploaded
for the commit-specific filediff, and then we do some magic to find
it when the user is viewing the cumulative diff. In this case, we
have to do the opposite and figure out if the user had commented on
the file while viewing the cumulative diff so we can link back to
that rather than the commit range.
When we do that backwards lookup for annoying thing #2, we need to
make sure that we set the anchor to the correct filediff ID.

Unit tests have been added to cover all of these corner cases.

Testing Done


Created a review request with a ton of comments on binary files across

  various revision and commit ranges. Checked that clicking on the

  filename listed in the comments correctly opened the corresponding

  diff view.
Ran unit tests.

Commits

Summary	ID
Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	b656e376010e8a741d0b4277b5def441de9b0225

Summary

Fix comment links for file attachments in diffs.

This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.

b656e376010e8a741d0b4277b5def441de9b0225

Issues

Description	From	Last Updated
local variable 'upstream_attachment' is assigned to but never used Column: 9 Error code: F841	reviewbot	March 29, 2024, 10:55 a.m.
'typing.cast' imported but unused Column: 1 Error code: F401	reviewbot	March 29, 2024, 11:29 a.m.
These should be in alphabetical order.	chipx86	April 2, 2024, 11:30 a.m.
"For comments file attachments" -> "For file attachment comments" ?	chipx86	April 2, 2024, 11:30 a.m.
These are missing docs.	chipx86	April 2, 2024, 11:30 a.m.
We aren't returning anything in this case.	chipx86	April 2, 2024, 11:31 a.m.
Looks like the line join got broken when wrapping.	chipx86	April 2, 2024, 11:31 a.m.
We aren't returning anything in this case.	chipx86	April 2, 2024, 11:31 a.m.
We aren't returning anything in this case.	chipx86	April 2, 2024, 11:31 a.m.
This is still going through all the logic of constructing an object and setting up deferred attribute values. We should …	chipx86	April 2, 2024, 11:32 a.m.
This should go in the previous import group.	chipx86	April 2, 2024, 11:32 a.m.
We can wrap this at a . and beanbag-docutils will do the right thing. Although this is a standard comment …	chipx86	April 2, 2024, 11:32 a.m.
Let's sort these in alphabetical order.	chipx86	April 2, 2024, 11:33 a.m.
This should say "get_comment_link_url" instead of "get_comment_thumbnail".	maubin	April 10, 2024, 9:47 a.m.
This is wrong.	maubin	April 10, 2024, 9:48 a.m.

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/tests/test_review_ui.py (Diff revision 1)

The issue has been dropped. Show all issues

local variable 'upstream_attachment' is assigned to but never used

Column: 9
Error code: F841

Commits:

	Summary	ID
	Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	3797485cc2a5ab0b03b80d11f5530f5722686c03
	Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	38d04ea4b32057b8a6e11e03886715db413d8a98

Diff:

Revision 2 (+986 -22)

Show changes

	reviewboard/reviews/tests/test_review_ui.py
	reviewboard/reviews/ui/base.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Change Summary:

Move revision computation into FileAttachmentComment so we can use it elsewhere.

Commits:

	Summary	ID
	Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	38d04ea4b32057b8a6e11e03886715db413d8a98
	Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	ca928728603066426e91cfec3fae190fcdf8f196

Diff:

Revision 3 (+1202 -56)

Show changes

	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/tests/test_review_ui.py
	reviewboard/reviews/ui/base.py

Checks run (1 failed, 1 succeeded)

flake8 failed.

JSHint passed.

flake8

reviewboard/reviews/ui/base.py (Diff revision 3)
The issue has been dropped. Show all issues
```
'typing.cast' imported but unused

Column: 1
Error code: F401
```

Commits:

	Summary	ID
	Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	ca928728603066426e91cfec3fae190fcdf8f196
	Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	56a785a3da2fea4dc2a480a571c9a1001d851782

Diff:

Revision 4 (+1206 -54)

Show changes

	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/tests/test_review_ui.py
	reviewboard/reviews/ui/base.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

reviewboard/reviews/models/file_attachment_comment.py (Diff revision 4)
The issue has been resolved. Show all issues
```
These should be in alphabetical order.
```
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 4)
The issue has been resolved. Show all issues
```
"For comments file attachments" -> "For file attachment comments" ?
```
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 4)
The issue has been resolved. Show all issues
```
These are missing docs.
```
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 4)
The issue has been resolved. Show all issues
```
We aren't returning anything in this case.
```
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Looks like the line join got broken when wrapping.
```
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 4)
The issue has been resolved. Show all issues
```
We aren't returning anything in this case.
```
reviewboard/reviews/models/file_attachment_comment.py (Diff revision 4)
The issue has been resolved. Show all issues
```
We aren't returning anything in this case.
```

reviewboard/reviews/models/file_attachment_comment.py (Diff revision 4)

The issue has been resolved. Show all issues

This is still going through all the logic of constructing an object and setting up deferred attribute values. We should just use values_list:

last_commit_id = (
      modified_diffset.commits
      .order_by('-pk')
      .values_list('pk', flat=True)
)[0]

This will still only fetch one result, since values_list will evaluate on access, and a direct access of an item will evaluate only to the item needed unless the cache is already populated.

reviewboard/reviews/tests/test_review_ui.py (Diff revision 4)
The issue has been resolved. Show all issues
```
This should go in the previous import group.
```

reviewboard/reviews/ui/base.py (Diff revision 4)

The issue has been resolved. Show all issues

We can wrap this at a . and beanbag-docutils will do the right thing.

Although this is a standard comment and not processed by ReST.

reviewboard/reviews/ui/base.py (Diff revision 4)
The issue has been resolved. Show all issues
```
Let's sort these in alphabetical order.
```

Commits:

	Summary	ID
	Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	56a785a3da2fea4dc2a480a571c9a1001d851782
	Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	43ce20313daaff948f7dbc92017039d961eb548c

Diff:

Revision 5 (+1222 -64)

Show changes

	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/tests/test_review_ui.py
	reviewboard/reviews/ui/base.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

reviewboard/reviews/models/file_attachment_comment.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This should say "get_comment_link_url" instead of "get_comment_thumbnail".
```
reviewboard/reviews/ui/base.py (Diff revision 5)
The issue has been resolved. Show all issues
```
This is wrong.
```

Commits:

	Summary	ID
	Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	43ce20313daaff948f7dbc92017039d961eb548c
	Fix comment links for file attachments in diffs. This change fixes the links for file attachments which are attached to binary files in a diff. The Review UI implementation was previously just linking to the file attachment page, but that's a pretty crummy experience when the comments were made on the diff. This is pretty complex because there are a lot of corner cases. In particular, we have to deal with three annoying things: 1. We sometimes elide the base commit ID or tip commit ID from the URL, depending on what commits are selected. 2. When a review request has commit history, the attachment is uploaded for the commit-specific filediff, and then we do some magic to find it when the user is viewing the cumulative diff. In this case, we have to do the opposite and figure out if the user had commented on the file while viewing the cumulative diff so we can link back to that rather than the commit range. 3. When we do that backwards lookup for annoying thing #2, we need to make sure that we set the anchor to the correct filediff ID. Unit tests have been added to cover all of these corner cases. Testing Done: - Created a review request with a ton of comments on binary files across various revision and commit ranges. Checked that clicking on the filename listed in the comments correctly opened the corresponding diff view. - Ran unit tests.	b656e376010e8a741d0b4277b5def441de9b0225

Diff:

Revision 6 (+1224 -64)

Show changes

	reviewboard/reviews/models/file_attachment_comment.py
	reviewboard/reviews/tests/test_review_ui.py
	reviewboard/reviews/ui/base.py

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (fb144e3)