Fix comment links for file attachments in diffs.

Review Request #13681 — Created March 29, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

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.

  • 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.
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
Description From Last Updated

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

reviewbotreviewbot

'typing.cast' imported but unused Column: 1 Error code: F401

reviewbotreviewbot

These should be in alphabetical order.

chipx86chipx86

"For comments file attachments" -> "For file attachment comments" ?

chipx86chipx86

These are missing docs.

chipx86chipx86

We aren't returning anything in this case.

chipx86chipx86

Looks like the line join got broken when wrapping.

chipx86chipx86

We aren't returning anything in this case.

chipx86chipx86

We aren't returning anything in this case.

chipx86chipx86

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

chipx86chipx86

This should go in the previous import group.

chipx86chipx86

We can wrap this at a . and beanbag-docutils will do the right thing. Although this is a standard comment …

chipx86chipx86

Let's sort these in alphabetical order.

chipx86chipx86

This should say "get_comment_link_url" instead of "get_comment_thumbnail".

maubinmaubin

This is wrong.

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

flake8

david
david
Review request changed
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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. Show all issues

    These should be in alphabetical order.

  3. Show all issues

    "For comments file attachments" -> "For file attachment comments" ?

  4. Show all issues

    These are missing docs.

  5. Show all issues

    We aren't returning anything in this case.

  6. Show all issues

    Looks like the line join got broken when wrapping.

  7. Show all issues

    We aren't returning anything in this case.

  8. Show all issues

    We aren't returning anything in this case.

  9. reviewboard/reviews/models/file_attachment_comment.py (Diff revision 4)
     
     
     
     
     
     
     
    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.

  10. Show all issues

    This should go in the previous import group.

  11. reviewboard/reviews/ui/base.py (Diff revision 4)
     
     
    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.

  12. reviewboard/reviews/ui/base.py (Diff revision 4)
     
     
     
     
    Show all issues

    Let's sort these in alphabetical order.

  13. 
      
david
chipx86
  1. Ship It!
  2. 
      
maubin
  1. 
      
  2. Show all issues

    This should say "get_comment_link_url" instead of "get_comment_thumbnail".

  3. reviewboard/reviews/ui/base.py (Diff revision 5)
     
     
    Show all issues

    This is wrong.

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