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

Review Request #13667 — Created March 25, 2024 and submitted

Information

Review Board
release-7.x

Reviewers

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.

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

undefined name 'diff_commits' Column: 49 Error code: F821

reviewbotreviewbot

undefined name 'interdiff_commits' Column: 49 Error code: F821

reviewbotreviewbot

'reviewboard.diffviewer.models.diffcommit.SerializedDiffCommit' imported but unused Column: 5 Error code: F401

reviewbotreviewbot

redefinition of unused '_get_comments_hint' from line 525 Column: 5 Error code: F811

reviewbotreviewbot

We're sort of undoing an important optimization here, going from one query for commits spanning diffsets to now 3 queries. …

chipx86chipx86

social-page_title -> social_page_title

chipx86chipx86

With the above fix, I'd expect all of these to go back to their original values.

chipx86chipx86

Summary must be one line.

chipx86chipx86

Can we pass these in as keyword-only args?

chipx86chipx86

Can we sort keys?

chipx86chipx86

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

chipx86chipx86

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

chipx86chipx86

Can we sort keys?

chipx86chipx86

We don't normally use the outer parens for the tuple in this case.

chipx86chipx86

Can we sort the keys?

chipx86chipx86

Same comment about parens.

chipx86chipx86

Blank line here.

chipx86chipx86

Probably no need for a blank line here.

chipx86chipx86

This can just be `bool(self.interdiffset and draft_diffset == self.interdiffset)

maubinmaubin

We access draft.diffset a lot here, you could pull that out into a variable, and have it be None if …

maubinmaubin

This can just be if draft_diffset:

maubinmaubin

Missing a "Version Added"

maubinmaubin
david
Review request changed

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

Checks run (1 failed, 1 succeeded)

flake8 failed.
JSHint passed.

flake8

david
chipx86
  1. 
      
  2. reviewboard/diffviewer/views.py (Diff revision 3)
     
     
     
     
     
     

    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)
    
    1. 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.

  3. reviewboard/reviews/context.py (Diff revision 3)
     
     

    social-page_title -> social_page_title

  4. With the above fix, I'd expect all of these to go back to their original values.

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

    Can we pass these in as keyword-only args?

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

    Can we sort keys?

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

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

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

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

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

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

    Can we sort keys?

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

    We don't normally use the outer parens for the tuple in this case.

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

    Can we sort the keys?

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

    Same comment about parens.

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

    Blank line here.

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

    Probably no need for a blank line here.

  15. 
      
david
chipx86
  1. 
      
  2. reviewboard/reviews/views/diffviewer.py (Diff revisions 3 - 4)
     
     
     

    Summary must be one line.

  3. 
      
maubin
  1. 
      
  2. reviewboard/reviews/views/diffviewer.py (Diff revision 4)
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     
     

    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.

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

    Missing a "Version Added"

  4. 
      
david
maubin
  1. 
      
  2. reviewboard/reviews/views/diffviewer.py (Diff revisions 4 - 5)
     
     
     
     

    This can just be `bool(self.interdiffset and draft_diffset == self.interdiffset)

  3. reviewboard/reviews/views/diffviewer.py (Diff revisions 4 - 5)
     
     

    This can just be if draft_diffset:

  4. 
      
david
maubin
  1. Ship It!
  2. 
      
david
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-7.x (5499649)
Loading...