Summary

Fixed addFile form when uploading with no files selected

Review Request #5803 — Created May 12, 2014 and discarded Aug. 11, 2014, 12:18 p.m.

Information

Owner

salam0smy

Repository

Review Board

Branch

master

Bugs

3338

Depends On

~~5816~~

Commit

634ebba...

Reviewers

Groups

reviewboard, students

People

Description

If a file was not selected and the user clicks Upload button, the form sends an empty submit request. The form then changes to a loading state, while its waiting for a response. Since the request is empty, the form never gets a response.

Testing Done

1 - If you click Upload button without selecting files, the modal box will gets destroyed: this can be fixed in gravy.modalBox.
2 - The Upload button is enabled when files are selected.
2a - The form is submitted normally when clicking the Upload button.
3 - The Upload button is disabled when files are not selected.

Issues

Description	From	Last Updated
Make sure that the first sentence here is capitalized and ends in a period. Please also wrap this to fit …	david	May 14, 2014, 12:37 p.m.
Javascript should be indented 4 spaces. Your indentation here is inconsistent.	david	May 14, 2014, 12:37 p.m.
All of this could be one statement: $('input[type="button"][value="Upload"]') .attr('disabled', $('#id_path').get(0).files.length > 0); Please also include quotes around the type/value in …	david	May 14, 2014, 12:37 p.m.
It might be nice to just define the function inline here, instead of giving it a name.	david	May 14, 2014, 12:37 p.m.
Add a blank line before this.	david	May 14, 2014, 5:32 p.m.
Trailing whitespace.	david	May 14, 2014, 6:09 p.m.
Add a space between () and {	david	May 14, 2014, 6:09 p.m.
This needs to be indented to align with 'disabled' on the line above it (since it's another parameter to the …	david	May 14, 2014, 6:09 p.m.
Trailing whitespace.	david	May 14, 2014, 6:09 p.m.
.attr(...) should go on the next line.	david	May 15, 2014, 5:34 p.m.
This is somewhat expensive. Can we fetch the input up-front and hold onto that, so we don't have to search …	chipx86	May 21, 2014, 2:13 p.m.

Instead of letting the user click the "Upload" button and then showing them an error, how about disabling that button unless there are valid files selected?

salam0smy May 13, 2014, 10:23 a.m.

thats a good idea, I'll dig deeper in the code and try to make the button disabled until a file is selected

reviewboard/static/rb/js/common.js (Diff revision 1)

I bet that reviewboard people are pretty picky about code styles (well, they are building a code review tool after all :) ), so most likely you should fix indentation. I haven't found reviewboard's javascript conventions, but this is probably a pretty similar guide: http://javascript.crockford.com/code.html

mike_conley May 12, 2014, 7:11 p.m.

Volodymyr is right - we will definitely be harping on style. Might be a good idea to go over it with a fine-tooth comb and resubmit.

salam0smy May 13, 2014, 5:36 a.m.

thank you, I'll be more careful in future posts

Change Summary:

disable or enable the upload button on files selected

Description:

		fixed bug where the form starts loading after clicking submit, when no files are selected for upload.
~		there was no check on input files wheather they're empty or not, and no error message is displayed to the user.
	~	there was no check on input files wheather they're empty or not, and no error message is displayed to the user.
	+	The upload button now is initially disabled. It toggles as files get selected and unselected

Testing Done:

~		1- submit form with empty files,error message displayed, then select a file and submit it and check that they are uploaded.
~		2- submit form with files, check it proceeds normally, and files uploaded
	~	1- try to click upload with no file selected, the modal box gets destroyed: this can be fixed in gravy.modalBox
	~	2- select a file, check button is enabled
	+	3- unselect a file, check button is disabled
	+	4- submit form with files, check it proceeds normally, and files uploaded

Commit:

7174e6736f319028106e44c3e6801de99f9d2df1

0a3729a4c60be1e5800360b51a5cf03e4e0da215

Diff:

Revision 2 (+14 -1)

Show changes

reviewboard/static/rb/js/common.js

Depends On:: ~~5816 - Fix modalBox click handler to respect disabled state~~

reviewboard/static/rb/js/common.js (Diff revision 2)

The issue has been resolved. Show all issues

Make sure that the first sentence here is capitalized and ends in a period. Please also wrap this to fit in 80 columns.

reviewboard/static/rb/js/common.js (Diff revision 2)

The issue has been resolved. Show all issues

Javascript should be indented 4 spaces. Your indentation here is inconsistent.

reviewboard/static/rb/js/common.js (Diff revision 2)

The issue has been resolved. Show all issues

All of this could be one statement:

$('input[type="button"][value="Upload"]')
    .attr('disabled',
          $('#id_path').get(0).files.length > 0);


Please also include quotes around the type/value in the jquery selector.

reviewboard/static/rb/js/common.js (Diff revision 2)

The issue has been resolved. Show all issues

It might be nice to just define the function inline here, instead of giving it a name.

reviewboard/static/rb/js/common.js (Diff revision 2)

The issue has been resolved. Show all issues

Add a blank line before this.

david May 14, 2014, 2:56 p.m.

It looks like this is fixed, but you haven't closed out the issue. We'll often hold off on re-reviewing a change while there are still open issues, so make sure you mark them as fixed as you work through them.

Please also fix your summary, description, and testing done to use full sentences with capitalization and punctuation.

Change Summary:

The code is rewritten to fit one statement.

Commit:

0a3729a4c60be1e5800360b51a5cf03e4e0da215

e2ebca1a0adb9e18d72261a56c2d9a29303fe102

Diff:

Revision 3 (+13 -1)

Show changes

reviewboard/static/rb/js/common.js

reviewboard/static/rb/js/common.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```
reviewboard/static/rb/js/common.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Add a space between () and {
```

reviewboard/static/rb/js/common.js (Diff revision 3)

The issue has been resolved. Show all issues

This needs to be indented to align with 'disabled' on the line above it (since it's another parameter to the attr())

reviewboard/static/rb/js/common.js (Diff revision 3)
The issue has been resolved. Show all issues
```
Trailing whitespace.
```

Change Summary:

Fixed formatting issues

Summary:

fixed bug where it starts loading when no files selected for upload

Fixed addFile form when uploading with no files selected

Description:

~		fixed bug where the form starts loading after clicking submit, when no files are selected for upload.
	~	If a file was not selected and the user clicks Upload button, the form sends an empty submit request. The form then changes to a loading state, while its waiting for a response. Since the request is empty, the form never gets a response.
-		there was no check on input files wheather they're empty or not, and no error message is displayed to the user.
-		The upload button now is initially disabled. It toggles as files get selected and unselected

Testing Done:

~		1- try to click upload with no file selected, the modal box gets destroyed: this can be fixed in gravy.modalBox
~		2- select a file, check button is enabled
~		3- unselect a file, check button is disabled
~		4- submit form with files, check it proceeds normally, and files uploaded
	~	1 - If you click Upload button without selecting files, the modal box will gets destroyed: this can be fixed in gravy.modalBox.
	~	2 - The Upload button is enabled when files are selected.
	~	2a - The form is submitted normally when clicking the Upload button.
	~	3- The Upload button is disabled when files are not selected.

Commit:

e2ebca1a0adb9e18d72261a56c2d9a29303fe102

fdf840f438686e83a4e95e179963fe2651f36bae

Diff:

Revision 4 (+13 -1)

Show changes

reviewboard/static/rb/js/common.js

reviewboard/static/rb/js/common.js (Diff revision 4)
The issue has been resolved. Show all issues
```
.attr(...) should go on the next line.
```

Testing Done:

		1 - If you click Upload button without selecting files, the modal box will gets destroyed: this can be fixed in gravy.modalBox.
		2 - The Upload button is enabled when files are selected.
		2a - The form is submitted normally when clicking the Upload button.
~		3- The Upload button is disabled when files are not selected.
	~	3 - The Upload button is disabled when files are not selected.

Commit:

fdf840f438686e83a4e95e179963fe2651f36bae

634ebba628ef07c1e26bed9a72119966a8a4e48e

Diff:

Revision 5 (+13)

Show changes

reviewboard/static/rb/js/common.js

Ship it!

This looks good now. I'm going to hold off on pushing it because we're frozen for release.

Fix it!

reviewboard/static/rb/js/common.js (Diff revision 5)

An issue was opened. Show all issues

This is somewhat expensive. Can we fetch the input up-front and hold onto that, so we don't have to search for it every time we get a change event from the text field?

Same with id_path.

Status:: Discarded