Fix the order shown for file attachment diffs.

Review Request #7089 — Created March 19, 2015 and submitted

Information

Review Board
release-2.5.x
3e416f8...

Reviewers

When viewing diffs of file attachments, the displayed order between the
two revisions were reversed. For instance, on images, the new image was
being shown as the original version, and the original image was shown a
the new version.

This was due to the order in which the URL arguments were captured. In
file attachment terms, the original file is the one being diffed
against, but the URL was capturing the new file as the one being diffed
against.

Text attachments were correctly shown, but had reversed the IDs and
state. They're now using the correct variables.

This change fixes up the capture order, the template rendering for text
attachments, and the logic for text attachments, fixing all display
issues.

Tested with some revisioned images I had. The order was correct for all revisions.

Description From Last Updated

I don't like this change. We have it in the other order for diffs+interdiffs. I'd rather fix the other code …

daviddavid
reviewbot
  1. Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/urls.py
    
    
    
    Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/urls.py
    
    
  2. 
      
david
  1. Can you test that the text review UIs still behave appropriately?

    1. Hmm, yeah, they're broken.

      So, it seems we have half the codebase with one interpretation of these fields, and half with another.

  2. 
      
chipx86
reviewbot
  1. Tool: Pyflakes
    Processed Files:
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js
    
    
    
    Tool: PEP8 Style Checker
    Processed Files:
        reviewboard/reviews/ui/text.py
        reviewboard/reviews/urls.py
    
    Ignored Files:
        reviewboard/templates/reviews/ui/text.html
        reviewboard/static/rb/js/views/fileAttachmentRevisionLabelView.js
    
    
  2. 
      
david
  1. 
      
  2. reviewboard/reviews/urls.py (Diff revision 2)
     
     
     
    Show all issues

    I don't like this change. We have it in the other order for diffs+interdiffs. I'd rather fix the other code that generates these URLs.

    1. The problem isn't that the URLs are generated incorrectly, but rather that this view and the text-based review UI diff support were reversed in their interpretation of these IDs. To get a proper diff between IDs 3 and 4, you'd previously have to generate a URL like ..../file/4-3/, which is wrong. That's the case for images, anyway. Text-based attachments were using the right objects, but then swapping the display order in the template.

      This change fixes up everything to let you do ..../file/3-4/, as expected, matching the URL format of interdiffs.

  3. 
      
david
  1. Ship It!
  2. 
      
chipx86
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.5.x (34ce1d9)