Fix Files header still appearing after all attachments are removed.

Review Request #8417 — Created Sept. 19, 2016 and submitted

finaiized
Review Board
release-2.5.x
4451
2e11cfb...
reviewboard

The review page still showed the "Files" header when all attachments were
removed and the page was not reloaded. This was because the visibility of
the header was determined by the template once on page load, but the
attachments removed themselves dynamically from the page.

We now listen to "attachment remove" events, and dynamically hide the header
as necessary. Note that it wasn't necessary to check if the header should be
re-shown if an attachment was added because it either
a) triggers a full-page reload (Update > Add File) or
b) Automatically reshows the Files section (drag-and-drop).

  • Created a new review request and added (multiple) files to it.
  • Deleted some of the files and confirmed deleting still works as before.
  • When there was only one attachment left, removed it and confirmed the
    "Files" header was also removed.
  • Uploaded another file attachment (now that there is no files/header)
    using both drag-and-drop and Update > Add File and confirmed Files header
    (and the file) reappears.
  • Repeated testing on a published review request.
Description From Last Updated

Oops, realized this wasn't updated for 2.5. Once that's confirmed to work and the branch info on here is correct, ...

chipx86chipx86

This is some older code, but these day we use: this.listenTo(fileAttachments, 'destroy', function() { ... }); instead of using on(). ...

chipx86chipx86

#file-list-contianer will be underneath the review request editor view's element, so you can use this.$('#file-list-container') to narrow the scope to ...

brenniebrennie

I believe you can just use this._$attachmentsContainer instead of re-querying.

chipx86chipx86

jQuery's .hide() is the same as .css('display', 'none')

daviddavid
chipx86
  1. Looks good! I have a change I'd like you to make to the code (not a bug on your end, just the result of this code being older).

    I'd also like to see two more things:

    1) Description and Testing Done lines wrapped to ~75 characters (so it'll sanely fit in a commit message).
    2) Testing showing that after you've erased the last file attachment and "Files" went away, you were able to upload a new file attachment (without reloading the page) and "Files" came back.

    1. I believe if you use "Update > Add File", it causes a reload. If you drag-and-drop a file, it doesn't.

  2. This is some older code, but these day we use:

    this.listenTo(fileAttachments, 'destroy', function() {
        ...
    });
    

    instead of using on(). This helps to avoid some circular reference issues by allowing this to intelligently disconnect any signal handlers when it wants to clean things up.

    Can you make that change for both of these fileAttachments.on statements, and re-test with those?

  3. 
      
FI
reviewbot
  1. Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
FI
brennie
  1. 
      
  2. #file-list-contianer will be underneath the review request editor view's element, so you can use this.$('#file-list-container') to narrow the scope to elements in that subtree.

  3. 
      
FI
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
david
  1. 
      
  2. jQuery's .hide() is the same as .css('display', 'none')

  3. 
      
chipx86
  1. 
      
    1. Oh, I noticed this was on master. You'll want to verify the fix on release-2.5.x as well (since this is the target for the fix).

  2. I believe you can just use this._$attachmentsContainer instead of re-querying.

  3. 
      
FI
reviewbot
  1. Tool: PEP8 Style Checker
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
    
    Tool: Pyflakes
    Ignored Files:
        reviewboard/static/rb/js/views/reviewRequestEditorView.js
    
    
  2. 
      
chipx86
  1. Ship It!
  2. 
      
chipx86
  1. 
      
  2. Oops, realized this wasn't updated for 2.5. Once that's confirmed to work and the branch info on here is correct, I'm happy.

    It's also good to say what changed in each iteration of your diff update (through the change description in the draft banner).

    1. Noted. Didn't realize my git commit messages weren't passed along after each rbt post update!

  3. 
      
FI
david
  1. Ship It!
  2. 
      
FI
Review request changed

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (a4af54f)
Loading...