Summary:

Fixed ungraceful failure when failing to connect to web server during file upload.

Review Request #9208 — Created Sept. 22, 2017 and submitted Oct. 18, 2017, 3:40 p.m.

Information
Owner:	nicho
Repository:	Review Board
Branch:	release-2.5.x
Bugs:	4467
Depends On:
Commit:	1b7561b...
Reviewers
Groups:	reviewboard, students
People:

Description

In the Review Request screen, failing to connect to the web server while
uploading a file using the Update -> Add File dialog would result in a
JavaScript error.

The server response message is parsed as a JSON object. If the server does
not have a response message, which is the case when the connection to the
web server fails, the JSON object parsed from the response message would be
null, resulting in an error when trying to access members of the object
later.

Null checks were added prior to accessing members of the JSON object. This
prevents the JavaScript from simply crashing. A default error message was
added, reading "Unknown Error" if the JSON object is null.

With the fix in place, failing to connect to the web server during a file
upload in the Review Request screen displays "Unknown Error", without any
JavaScript errors.

Testing Done

Ran JSTests.

Tested manually by attempting to upload a file while the server was off.
There were no JavaScript errors, and the displayed error message read
"Unknown Error".

Issues

Description	From	Last Updated
Looks like the wrong summary for this change, and some of the code content? You may not have posted explicitly …	chipx86	Sept. 23, 2017, 8:34 a.m.
Same comments apply about the description/testing as in my other review(s). I'd like to see more about when/why this problem …	chipx86	Sept. 23, 2017, 8:34 a.m.
Oh, this change should also probably be targeting release-2.5.x.	chipx86	Sept. 24, 2017, 8:20 a.m.
The description looks great! Just what we're looking for :) A couple small wording tweaks: "web-server" should be "web server". …	chipx86	Sept. 24, 2017, 3:51 p.m.
Single quotes. This string should be marked for translation with gettext().	brennie	Sept. 24, 2017, 3:50 p.m.

flake8 passed.

JSHint passed.

Show all issues

Looks like the wrong summary for this change, and some of the code content? You may not have posted explicitly this branch, getting the parent branch as well.

Show all issues

Same comments apply about the description/testing as in my other review(s). I'd like to see more about when/why this problem occurs and what things look like with the fix in place when it does happen. Something that isn't clear from the description is that this isn't site-wide, and is specific to a certain piece of UI (it sounds like it addresses this across the board).

Change Summary:

Updated descriptions to be more specific, and fixed the code content.

Summary:

-	Fixed Update -> Add File not doing anything.
+	Fixed ungraceful failure when failing to connect to web-server during file upload.

Description:

~		When failing to connect to the web-server, the server response message is parsed as a json object. If the server does not have a response message, the json object would be null, resulting in an error when trying to access members of the object.
	~	In the Review Request screen, failing to connect to the web-server while
	+	uploading a file using the Update -> Add File dialog would result in a
	+	javascript error.

~		Null checks were added for the json object, and if the object is null, the error message would default to "Unknown Error".
	~	The server response message is parsed as a JSON object. If the server does
	+	not have a response message, which is the case when the connection to the
	+	web-server fails, the JSON object parsed from the response message would be
	+	null, resulting in an error when trying to access members of the object
	+	later.
	+
	+	Null checks were added prior to accessing members of the JSON object. This
	+	prevents the javascript from simply crashing. A default error message was
	+	added, reading "Unknown Error" if the JSON object is null.
	+
	+	With the fix in place, failing to connect to the web-server during a file
	+	upload in the Review Request screen displays "Unknown Error", without any
	+	javascript errors.

Testing Done:

		Ran JSTests.

~		Tested manually by attempting to upload a file while the server was off. There were no javascript errors, and the displayed error message read "Unknown Error".
	~	Tested manually by attempting to upload a file while the server was off.
	+	There were no javascript errors, and the displayed error message read
	+	"Unknown Error".

Commit:

-	b33696beb3cc811caa9b0966b31d70157e9978b2
+	56dd99fbfc779a088477fa653ce28f3946749cbb

Diff:

Revision 2 (+2 -2)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Show all issues

The description looks great! Just what we're looking for :)

A couple small wording tweaks:

"web-server" should be "web server".
"javascript" should be "JavaScript".

chipx86 Sept. 24, 2017, 2:29 p.m.
```
Still a problem in the summary.
```

Show all issues

Oh, this change should also probably be targeting release-2.5.x.

Change Summary:

Fixed grammar, and targetted version release-2.5.x instead.

Description:

~		In the Review Request screen, failing to connect to the web-server while
	~	In the Review Request screen, failing to connect to the web server while
		uploading a file using the Update -> Add File dialog would result in a
~		javascript error.
	~	JavaScript error.

		The server response message is parsed as a JSON object. If the server does
		not have a response message, which is the case when the connection to the
~		web-server fails, the JSON object parsed from the response message would be
	~	web server fails, the JSON object parsed from the response message would be
		null, resulting in an error when trying to access members of the object
		later.

		Null checks were added prior to accessing members of the JSON object. This
~		prevents the javascript from simply crashing. A default error message was
	~	prevents the JavaScript from simply crashing. A default error message was
		added, reading "Unknown Error" if the JSON object is null.

~		With the fix in place, failing to connect to the web-server during a file
	~	With the fix in place, failing to connect to the web server during a file
		upload in the Review Request screen displays "Unknown Error", without any
~		javascript errors.
	~	JavaScript errors.

Testing Done:

		Ran JSTests.

		Tested manually by attempting to upload a file while the server was off.
~		There were no javascript errors, and the displayed error message read
	~	There were no JavaScript errors, and the displayed error message read
		"Unknown Error".

Branch:

-	master
+	release-2.5.x

Commit:

-	56dd99fbfc779a088477fa653ce28f3946749cbb
+	08cd3b864f1b0942aa0ad2ef2d0ed09ebe0b2b94

Diff:

Revision 3 (+2 -2)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

Ship it!

```
Looks good to me!
```
reviewboard/static/rb/js/views/uploadAttachmentView.js (Diff revision 3)
Show all issues
```
Single quotes.
This string should be marked for translation with gettext().
```

Change Summary:

Changed 'Unknown Error' string to single quotes, and used gettext() around it for translations down the line.

Summary:

-	Fixed ungraceful failure when failing to connect to web-server during file upload.
+	Fixed ungraceful failure when failing to connect to web server during file upload.

Commit:

-	08cd3b864f1b0942aa0ad2ef2d0ed09ebe0b2b94
+	1b7561b429d3a8ded38676585ed65731a4ec150f

Diff:

Revision 4 (+2 -2)

Show changes

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

Checks run (2 succeeded)

flake8 passed.

JSHint passed.

```
Ship It!
```

Change Summary:

Added a review request dependency.

Depends On:

+	~~9207 - Fixed the Update -> Add File review request action not responding to clicks.~~

Change Summary:

Removed the dependency (turns out it wasn't actually dependent).

Depends On:

-	~~9207 - Fixed the Update -> Add File review request action not responding to clicks.~~

Ship it!

```
Ship It!
```

Status: Closed (submitted)

Change Summary:

Pushed to release-2.5.x (751d099)