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

Review Request #9208 — Created Sept. 22, 2017 and submitted

Information

Review Board
release-2.5.x
1b7561b...

Reviewers

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.

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

Description From Last Updated

Looks like the wrong summary for this change, and some of the code content? You may not have posted explicitly …

chipx86chipx86

Same comments apply about the description/testing as in my other review(s). I'd like to see more about when/why this problem …

chipx86chipx86

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

chipx86chipx86

The description looks great! Just what we're looking for :) A couple small wording tweaks: "web-server" should be "web server". …

chipx86chipx86

Single quotes. This string should be marked for translation with gettext().

brenniebrennie
chipx86
  1. 
      
  2. 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.

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

  4. 
      
NI
chipx86
  1. 
      
  2. 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".
    1. Still a problem in the summary.

  3. 
      
chipx86
  1. 
      
  2. Show all issues

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

  3. 
      
NI
brennie
  1. Looks good to me!

  2. Show all issues

    Single quotes.

    This string should be marked for translation with gettext().

  3. 
      
NI
NI
  1. Ship It!

  2. 
      
NI
NI
chipx86
  1. Ship It!
  2. 
      
NI
Review request changed
Status:
Completed
Change Summary:
Pushed to release-2.5.x (751d099)