Fix caching issues in the diff viewer with interdiffs.

Review Request #7709 — Created Oct. 17, 2015 and submitted

Information

Review Board
release-2.0.x

Reviewers

The diff viewer had a caching bug that people noticed when using
interdiffs. Essentially, updating the diff and viewing its interdiff
resulted in the old diff being displayed.

The reason this happened is that DiffFragmentView.make_etag() was
expecting a DiffSet ID, but this wasn't actually available at this
point. We didn't have DiffSet information until later, after the caching
checks, so we couldn't include any of its identifying information.

Rather than regress the performance of the diff viewer by moving the
database access before the caching checks, I've opted to extend the URL
format to take an interfilediff ID. This gives us all the information
needed to sanely cache fragments without incurring any performance
penalties.

Tested diffs and interdiffs of different types in the diff viewer. Saw that
the content had a new ETag every time a diff was updated, and for every
diff revision range.

Unit tests pass (both Python and JavaScript).

Description From Last Updated

Two typos: "a a subclass's" should be "a subclass'".

AD adriano

Care to add Args and Returns here?

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js
        reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js
        reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js
    
    
  2. 
      
AD
  1. 
      
  2. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
    Show all issues

    Two typos: "a a subclass's" should be "a subclass'".

    1. Removed the "a", but "subclass's" is correct. As opposed to "the birds' flight path."

  3. 
      
david
  1. Bug 3993?

  2. reviewboard/diffviewer/views.py (Diff revision 1)
     
     
    Show all issues

    Care to add Args and Returns here?

  3. 
      
chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js
        reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/diffviewer/views.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/static/rb/js/diffviewer/models/diffReviewableModel.js
        reviewboard/static/rb/js/diffviewer/models/tests/diffReviewableModelTests.js
    
    
  2. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.0.x (dd7dcbd)
Loading...