Fix Issue 3217: Remove download link for a "new version" of a deleted file.

Review Request #5377 — Created Feb. 1, 2014 and submitted

Information

Review Board
master

Reviewers

We don't show the orig revision or a download link for it for new files, so we probably shouldn't show the new revision or a download link for deleted files, either.

Created a review with a deleted file.
Download link for a new revision of a deleted file no longer appears in the file revision header.
Headers for modified and new files remain unchanged.

Description From Last Updated

In the case where we don't have a download URL and the file isn't deleted, we should still show the …

daviddavid

The way we do alignment in templates is a weird, weird thing. You should think of the lines that have …

daviddavid

The </th> and {% endif %} should be swapped (since the opening <th> is outside the conditional).

daviddavid
david
  1. 
      
  2. reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 1)
     
     
     
     
     
     
     
    Show all issues

    In the case where we don't have a download URL and the file isn't deleted, we should still show the revision. Your logic should probably be something like:

    if not file.deleted:
       if download_modified_url:
           show download icon
       show revision
    
  3. 
      
OL
david
  1. 
      
  2. reviewboard/templates/diffviewer/diff_file_fragment.html (Diff revision 2)
     
     
     
     
     
     
     
     
     
    Show all issues

    The way we do alignment in templates is a weird, weird thing.

    You should think of the lines that have tags (the {% ... %} things) as being in an independent indentation namespace from the HTML. When there are tags, we put the {% on the left-hand side, and indent using single spaces within the tag.

    HTML inside the tags should then be indented with 1 space. That means the <a> tag should remain where it was.

    The result would look something like this:

       <th>
    {%   if not file.deleted %}
    {%    if download_modified_url %}
        <a class ... >...</a>
    {%    endif %}
        {{file.dest_revision }}
    {%   endif %}
       </th>
    {%  endif %}{# num_changes and moved #}
    

    The result is that if you just mentally filter out every line that starts with {%, you'd see a nicely indented HTML file. If you only look at the other lines, you'd see nicely indented logic.

    There are definitely several older templates that don't follow this pattern (or are inconsistent), but we're working to improve that.

    1. Thank you for taking the time to explain formatting, it makes a lot of sense, and I should have noted the pattern, and followed it.

  3. Show all issues

    The </th> and {% endif %} should be swapped (since the opening <th> is outside the conditional).

  4. 
      
OL
david
  1. Ship It!

  2. 
      
OL
Review request changed
Status:
Completed
Change Summary:
Pushed to master (8ac3484). Thanks!