Summary

Backbone view for attachment upload

Review Request #5911 — Created May 31, 2014 and submitted Aug. 10, 2014, 3:22 p.m.

Information

Owner

volodymyr

Repository

Review Board

Branch

Bugs

Depends On

Blocks

5824

Reviewers

Groups

reviewboard, students

People

Description

A working version of a Backbone view for file uploads (the one that you see when you click Update -> Add File). Previously that popup is used gReviewRequest hack. Now it's a standalone view, as that view is also needed for attachment revisioning.

Side note: The only other thing that needs gReviewRequest hack is "Update Diff" popup. Once someone writes a view for that, gReviewRequest can be removed.

Testing Done

Tested: * Uploading new attachments. * Error when no attachment is specified. * Multiple errors returned from server side (see screenshots).

Files

Issues

Description	From	Last Updated
Styles should be defined in the .less files. That said, a lot of this looks like you're manually copying stuff …	david	July 6, 2014, 8:28 a.m.
All of the HTML in these strings should be indented 1 space, not 4.	david	June 7, 2014, 12:12 p.m.
It's not important during prototyping, but we'll eventually want any user-visible strings to be passed into the template as variables, …	david	June 7, 2014, 12:12 p.m.
You've got some badly defined HTML in here (all the <col> elements are un-closed).	david	June 7, 2014, 12:12 p.m.
Make sure never to have a trailing comma on the last entry in a dictionary or list. It'll break some …	chipx86	June 14, 2014, 10:51 a.m.
variables should always be nameed likeThis.	chipx86	June 7, 2014, 12:12 p.m.
initialize should never call render. That's up to the caller who's constructing the view. Same with setting el.	chipx86	June 7, 2014, 12:13 p.m.
Instead of assigning that, pass this as the last parameter to save(), and the callbacks will end up having this …	chipx86	June 14, 2014, 10:51 a.m.
gReviewRequest is deprecated and was supposed to be removed. This view should be associated with a ReviewRequestEditor, which will have …	chipx86	June 14, 2014, 10:51 a.m.
Would you mind wrapping this to try to keep it under 80 columns? ' <form encoding="multipart/form-data"', ' enctype="multipart/form-data"', ' id="attachment-upload-form">',	david	June 29, 2014, 12:36 p.m.
Can you wrap this?	david	June 29, 2014, 12:36 p.m.
Arrays in javascript can't have trailing commas (it breaks on some browsers)	david	June 29, 2014, 12:36 p.m.
Can you add method comments for this and all the others that don't have them?	david	June 29, 2014, 12:36 p.m.
If you use this.$(...), it will be more efficient, since it doesn't have to scan the whole document.	david	June 29, 2014, 12:35 p.m.
This should use gettext()	david	June 29, 2014, 12:35 p.m.
'errorStr' is not HTML, so this should use .text() instead of .html().	david	July 19, 2014, 9:01 p.m.
You should be able to use the 'events' mapping for this: events: { ... existing other stuff ... 'click #upload-file-link': …	david	July 27, 2014, 2:44 p.m.
You shouldn't need to pass el yourself. By default, a new div element will be created.	david	July 25, 2014, 5:12 p.m.
I just committed a change to common.js that fixes some logic in this code (and simplifies things). Can you look …	david	July 25, 2014, 5:12 p.m.
Can we name this $list, since it's a jquery object?	david	July 25, 2014, 5:12 p.m.
Can we swap these (set HTML first, then append?)	david	July 25, 2014, 5:12 p.m.
No trailing comma (breaks IE)	david	July 25, 2014, 5:12 p.m.
No trailing comma (breaks IE)	david	July 25, 2014, 5:12 p.m.
Either you should move the variable definition to the top of this method, or, more likely, just avoid assigning anything: …	david	Aug. 2, 2014, 10:44 p.m.
You can get rid of the uploadObject parameter by just passing in this.displayErrors as the value for error	david	Aug. 4, 2014, 12:01 p.m.
Get rid of the rsp parameter here (it's not used)	david	Aug. 2, 2014, 10:44 p.m.
What is this? I don't see any options anywhere.	david	Aug. 2, 2014, 10:44 p.m.
What is errors? I don't see where it's defined.	david	Aug. 2, 2014, 10:44 p.m.
Once the change in send above is made, this anonymous function could be replaced by _.bind(this.send, this).	david	Aug. 4, 2014, 12:01 p.m.
If you pass this as a second paramater to .save(), you don't have to create a local variable for displayErrors, …	david	Aug. 6, 2014, 11:28 a.m.
Variable definitions should be the first line in the function.	david	Aug. 6, 2014, 11:28 a.m.
Can you call this variable self instead of uploadObject?	david	Aug. 6, 2014, 8:53 p.m.
These two functions should probably be chained: this.$el .append(this.template({ ... })) .modalBox({ ... });	david	Aug. 6, 2014, 8:53 p.m.

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/staticbundles.py
  Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/staticbundles.py
  Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js

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

The issue has been resolved. Show all issues

Styles should be defined in the .less files.

That said, a lot of this looks like you're manually copying stuff that's autogenerated. Your view should use $.modalBox to create the modal, which will handle the background, etc.

volodymyr June 14, 2014, 10:52 a.m.

I still have some style="..." stuff left there, but it's a couple of thing only. Is it tolerable or no?

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
All of the HTML in these strings should be indented 1 space, not 4.
```

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

The issue has been resolved. Show all issues

It's not important during prototyping, but we'll eventually want any user-visible strings to be passed into the template as variables, so that we can call gettext() on them.

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
You've got some badly defined HTML in here (all the <col> elements are un-closed).
```

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

The issue has been resolved. Show all issues

Make sure never to have a trailing comma on the last entry in a dictionary or list. It'll break some browsers.

It's good to get in the habit of running 'jshint' within the js/ directory. (You'll need to install that.)

VO

volodymyr June 7, 2014, 12:13 p.m.
```
Will do.
```

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 1)
The issue has been resolved. Show all issues
```
variables should always be nameed likeThis.
```
1. david June 1, 2014, 2:30 p.m.
  In JavaScript, that is. Python uses underscores (yeah, yeah).

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

The issue has been resolved. Show all issues

initialize should never call render. That's up to the caller who's constructing the view.

Same with setting el.

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

The issue has been resolved. Show all issues

Instead of assigning that, pass this as the last parameter to save(), and the callbacks will end up having this set correctly.

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

The issue has been resolved. Show all issues

gReviewRequest is deprecated and was supposed to be removed. This view should be associated with a ReviewRequestEditor, which will have a handle to the ReviewRequest model.

VO

volodymyr June 7, 2014, 12:13 p.m.
```
Ok, I will look into that.
```

Change Summary:

Adressed some feedback.

Diff:

Revision 2 (+75)

Show changes

reviewboard/static/rb/js/views/uploadAttachmentView.js

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
  Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js

Change Summary:

Addressed another portion of review comments.

Diff:

Revision 3 (+74)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/views/uploadAttachmentView.js

This is a review from Review Bot.
  Tool: PEP8 Style Checker
  Processed Files:
    reviewboard/staticbundles.py
  Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js

This is a review from Review Bot.
  Tool: Pyflakes
  Processed Files:
    reviewboard/staticbundles.py
  Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js

Diff:

Revision 4 (+117)

Show changes

reviewboard/static/rb/js/views/uploadAttachmentView.js

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



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

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 4)

The issue has been resolved. Show all issues

Would you mind wrapping this to try to keep it under 80 columns?

' <form encoding="multipart/form-data"',
'       enctype="multipart/form-data"',
'       id="attachment-upload-form">',

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you wrap this?
```
reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Arrays in javascript can't have trailing commas (it breaks on some browsers)
```
reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
Can you add method comments for this and all the others that don't have them?
```
reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
If you use this.$(...), it will be more efficient, since it doesn't have to scan the whole document.
```
reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 4)
The issue has been resolved. Show all issues
```
This should use gettext()
```

Diff:

Revision 5 (+134)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/views/uploadAttachmentView.js

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 5)
The issue has been resolved. Show all issues
```
'errorStr' is not HTML, so this should use .text() instead of .html().
```

Change Summary:

Moved in some changes that previously were at https://reviews.reviewboard.org/r/5824/

Diff:

Revision 6 (+150 -14)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/uploadAttachmentView.js
	reviewboard/templates/reviews/review_request_dlgs.html

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_dlgs.html



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_dlgs.html

Summary:: [WIP] Backbone view for attachment upload
Backbone view for attachment upload

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 6)

The issue has been dropped. Show all issues

You should be able to use the 'events' mapping for this:

events: {
    ... existing other stuff ...
    'click #upload-file-link': '_onUploadFileClicked'
}

volodymyr July 25, 2014, 5:16 p.m.

This is the same issue that I keep encountering over and over again - #upload-file-link is not a child of the view element, since it's hidden inside a list that gets displayed when you hover over "Update" button. Therefore I have to assign the handler myself.

I really wonder why Backbone creators haven't added a way to do this in the 'events' mapping, but most likely I just don't know enough to see all tradeoffs.

david July 25, 2014, 5:28 p.m.

I don't understand. You're using this.$(...), which means that #upload-file-link has to be nested (however deeply) within this.$el. Anything that can be accessed through this.$(...) should also be fine to use with events.

chipx86 July 25, 2014, 5:30 p.m.

It won't work in this case.

events doesn't attach to those selectors. It only listens on this.$el, and checks the targets of any events that bubble up to it.

The drop-down menus disable propagation of events, so they'll never reach this.$el.

david July 25, 2014, 5:45 p.m.
```
Ah, it makes sense now.
```

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
You shouldn't need to pass el yourself. By default, a new div element will be created.
```

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 6)

The issue has been resolved. Show all issues

I just committed a change to common.js that fixes some logic in this code (and simplifies things). Can you look at that change (76caaa7) and copy the changes into your code here?

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Can we name this $list, since it's a jquery object?
```
reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
Can we swap these (set HTML first, then append?)
```
reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
No trailing comma (breaks IE)
```
reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 6)
The issue has been resolved. Show all issues
```
No trailing comma (breaks IE)
```

Diff:

Revision 7 (+143 -14)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/uploadAttachmentView.js
	reviewboard/templates/reviews/review_request_dlgs.html

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_dlgs.html



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_dlgs.html

reviewboard/static/rb/js/pages/views/reviewablePageView.js (Diff revision 7)

The issue has been resolved. Show all issues

Either you should move the variable definition to the top of this method, or, more likely, just avoid assigning anything:

this.$('#upload-file-link')
    .click(_.bind(this._onUploadFileClicked, this));


This code should probably also go into the render method.

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 7)

The issue has been resolved. Show all issues

You can get rid of the uploadObject parameter by just passing in this.displayErrors as the value for error

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
Get rid of the rsp parameter here (it's not used)
```
reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
What is this? I don't see any options anywhere.
```
reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 7)
The issue has been resolved. Show all issues
```
What is errors? I don't see where it's defined.
```

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 7)

The issue has been resolved. Show all issues

Once the change in send above is made, this anonymous function could be replaced by _.bind(this.send, this).

Change Summary:

Addressed feedback

Diff:

Revision 8 (+140 -14)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/uploadAttachmentView.js
	reviewboard/templates/reviews/review_request_dlgs.html

Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_dlgs.html



Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_dlgs.html

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 8)

The issue has been resolved. Show all issues

If you pass this as a second paramater to .save(), you don't have to create a local variable for displayErrors, because it will use the right context.

volodymyr Aug. 6, 2014, 11:29 a.m.

Sorry about that (it's not the first time you're telling me about this trick). I think I tried to do this and it didn't work, but I have a feeling I did it in the wrong place. All good now.

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 8)
The issue has been resolved. Show all issues
```
Variable definitions should be the first line in the function.
```

Change Summary:

Modified displayErrors a bit and retested. Made sure that it works with both single errors and multiple errors (single errors by not including file, multiple errors by sending appropriate reply from the python side). Attached screenshots of errors.

Diff:

Revision 9 (+137 -14)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/uploadAttachmentView.js
	reviewboard/templates/reviews/review_request_dlgs.html

Added Files:

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_dlgs.html



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_dlgs.html

Just a couple trivial coding style comments left. Nice job!

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 9)
The issue has been resolved. Show all issues
```
Can you call this variable self instead of uploadObject?
```

reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 9)

The issue has been resolved. Show all issues

These two functions should probably be chained:

this.$el
    .append(this.template({
        ...
    }))
    .modalBox({
        ...
    });

Change Summary:

Addressed feedback, retested.

Diff:

Revision 10 (+137 -14)

Show changes

	reviewboard/staticbundles.py
	reviewboard/static/rb/js/pages/views/reviewablePageView.js
	reviewboard/static/rb/js/views/uploadAttachmentView.js
	reviewboard/templates/reviews/review_request_dlgs.html

Tool: PEP8 Style Checker
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_dlgs.html



Tool: Pyflakes
Processed Files:
    reviewboard/staticbundles.py

Ignored Files:
    reviewboard/static/rb/js/views/uploadAttachmentView.js
    reviewboard/static/rb/js/pages/views/reviewablePageView.js
    reviewboard/templates/reviews/review_request_dlgs.html

The code looks good, but the description seems out of date. Can you rewrite that, and then I'll do some final testing and push?

VO

volodymyr Aug. 7, 2014, 5 p.m.
```
Done.
```

Change Summary:: Description updated.
Description:: ~
A working version of a Backbone view for file uploads. It's not written very well and will be improved (since I need it for my project, I need a partial version).
~
A working version of a Backbone view for file uploads (the one that you see when you click Update -> Add File). Previously that popup is used gReviewRequest hack. Now it's a standalone view, as that view is also needed for attachment revisioning.
+
+
Side note: The only other thing that needs gReviewRequest hack is "Update Diff" popup. Once someone writes a view for that, gReviewRequest can be removed.
Testing Done:: ~
Performed upload of new attachments, as well as updating existing versions.
~
Tested:
+ * Uploading new attachments.
+ * Error when no attachment is specified.
+ * Multiple errors returned from server side (see screenshots).

Ship it!

```
Ship It!
```

Status:: Completed
Change Summary:: Pushed to master (8c368ca)

~		Performed upload of new attachments, as well as updating existing versions.
	~	Tested:
	+	* Uploading new attachments.
	+	* Error when no attachment is specified.
	+	* Multiple errors returned from server side (see screenshots).