Summary

Fix Files header still appearing after all attachments are removed.

Review Request #8417 — Created Sept. 19, 2016 and submitted Oct. 28, 2016, 2:14 p.m.

Information

Owner

finaiized

Repository

Review Board

Branch

release-2.5.x

Bugs

4451

Depends On

Commit

2e11cfb...

Reviewers

Groups

reviewboard

People

Description

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).

Testing Done


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.

Issues

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, …	chipx86	Sept. 23, 2016, 10:26 a.m.
This is some older code, but these day we use: this.listenTo(fileAttachments, 'destroy', function() { ... }); instead of using on(). …	chipx86	Sept. 20, 2016, 5:16 p.m.
#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 …	brennie	Sept. 20, 2016, 6:42 p.m.
I believe you can just use this._$attachmentsContainer instead of re-querying.	chipx86	Sept. 23, 2016, 9:42 a.m.
jQuery's .hide() is the same as .css('display', 'none')	david	Sept. 23, 2016, 9:42 a.m.

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.

david Sept. 20, 2016, 9:25 a.m.

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

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 1)

The issue has been resolved. Show all issues

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?

Description:

~		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.
	~	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 that triggers a full-page reload and thus causes the template to re-render the header with the correct visibility.
	~	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).

Testing Done:

		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.
~		Repeated testing on a published review request.
	~	When there was only one attachment left, removed it and confirmed the "Files" header was also removed.
	~	Uploaded another file attachments (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.

Commit:

-	79270a1799fae42de7e372545bac9bed03e5638c
+	9a447bf1cd8bbb627f8aa559d421ebf16d38a8d8

Diff:

Revision 2 (+6 -1)

Show changes

reviewboard/static/rb/js/views/reviewRequestEditorView.js

Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

Testing Done:

		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 attachments (now that there is no files/header) using both drag-and-drop and Update > Add File and confirmed Files header (and the file) reappears.
	~	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.

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 2)

The issue has been resolved. Show all issues

#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.

Commit:

-	9a447bf1cd8bbb627f8aa559d421ebf16d38a8d8
+	106d33fa71e79a97288fe8f0af7204e4d598b406

Diff:

Revision 3 (+6 -1)

Show changes

reviewboard/static/rb/js/views/reviewRequestEditorView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
jQuery's .hide() is the same as .css('display', 'none')
```

chipx86 Sept. 23, 2016, 8:47 a.m.

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).

reviewboard/static/rb/js/views/reviewRequestEditorView.js (Diff revision 3)
The issue has been resolved. Show all issues
```
I believe you can just use this._$attachmentsContainer instead of re-querying.
```

Commit:

-	106d33fa71e79a97288fe8f0af7204e4d598b406
+	2e11cfb748aab58220f4bd88afeff2dc2f9ff746

Diff:

Revision 4 (+6 -1)

Show changes

reviewboard/static/rb/js/views/reviewRequestEditorView.js

Tool: PEP8 Style Checker
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js



Tool: Pyflakes
Ignored Files:
    reviewboard/static/rb/js/views/reviewRequestEditorView.js

Ship it!

```
Ship It!
```

The issue has been resolved. Show all issues

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).

finaiized Sept. 23, 2016, 10:03 a.m.

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

Branch:

-	master
+	release-2.5.x

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (a4af54f)