• 
      

    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)