Bug #4924

[SDKs] Improve arv edit error handling UX

Added by Tom Clegg over 5 years ago. Updated over 5 years ago.

Status:
Resolved
Priority:
Normal
Assigned To:
Category:
-
Target version:
Start date:
09/02/2014
Due date:
% Done:

100%

Estimated time:
(Total: 0.00 h)
Story points:
0.5

Description

To do:
  • Error messages should be visible after a failure.
  • User should always be given a chance to re-edit after a failure. (Currently, (some?) API request failures just cause 'arv edit' to exit and save a temp file.)
  • Try: Show the errors by reopening the editor, with error messages in a comment block at the top. Strip the comment block when saving. This avoids the extra "try again? y/n" prompt which is nearly always what the user wants to do.
    • Exit if the temp file (minus leading comment block and whitespace) is empty.
    • Prompt (exit or try again) if the user saves the same bogus JSON twice in a row. (Maybe the user is just trying to escape!)
  • Unchanged attributes should not be sent in the API update call. (This is a common cause of failures.)

Subtasks

Task #4933: Review 4924-arv-edit-error-handlingResolvedPeter Amstutz

Task #3655: [SDKs] "arv edit" should not fail just because some special attribute (which I didn't edit) is rejected by the "update" call.ResolvedPeter Amstutz

Task #3786: [SDK] `arv edit` errors should be visible after failure, even if the edited JSON is longResolvedPeter Amstutz

Associated revisions

Revision 138a4b0a
Added by Peter Amstutz over 5 years ago

Merge branch '4924-arv-edit-error-handling' closes #4924

History

#1 Updated by Tom Clegg over 5 years ago

  • Description updated (diff)

#2 Updated by Tom Clegg over 5 years ago

  • Target version changed from Arvados Future Sprints to 2015-01-28 Sprint

#3 Updated by Brett Smith over 5 years ago

  • Assigned To set to Brett Smith

#4 Updated by Brett Smith over 5 years ago

  • Assigned To deleted (Brett Smith)

#5 Updated by Peter Amstutz over 5 years ago

  • Assigned To set to Peter Amstutz

#6 Updated by Brett Smith over 5 years ago

Reviewing 8295ba7

  • If the API server goes away, I get this message:
    # Connection refused - connect(2) for "localhost" port 3030
    # Please fix the error and try again.
    

    Most users probably won't be able to make any fix in this situation. I realize I'm breaking from the story a little bit here, but I wonder if this retry logic should only be used when there's an error in the user input, or the API server returns a well-formed error? In other cases, the old behavior of saying "I can't do that right now; I saved your edits here" seems more appropriate.
  • If we keep retrying for any non-200 API response, perhaps the rest of the status line ("Forbidden", "Unprocessable Entity", whatever) could be a better default error message when none were provided in the JSON body? (Currently it's an empty string.)
  • I think it could be extra helpful if the error message briefly described the category of error: "There was an error parsing your input," "The API server rejected your changes," etc. Some of the more specific error messages can be cryptic, so providing category information could help a user understand more quickly whether they broke the rules of JSON or Arvados.
  • arv edit doesn't write any message if it successfully updates the object. Should it? That seems like it better follows the general UX patterns for these tools.
  • Not a blocker, just FYI, any? optionally takes a block, so you can write .any? { condition } rather than .select { condition }.any?

Thanks.

#7 Updated by Peter Amstutz over 5 years ago

Brett Smith wrote:

Reviewing 8295ba7

  • If the API server goes away, I get this message:
    [...]
    Most users probably won't be able to make any fix in this situation. I realize I'm breaking from the story a little bit here, but I wonder if this retry logic should only be used when there's an error in the user input, or the API server returns a well-formed error? In other cases, the old behavior of saying "I can't do that right now; I saved your edits here" seems more appropriate.

Done.

  • If we keep retrying for any non-200 API response, perhaps the rest of the status line ("Forbidden", "Unprocessable Entity", whatever) could be a better default error message when none were provided in the JSON body? (Currently it's an empty string.)

I had this thought too, but the HTTP library documentation doesn't say how to get the status line (let me know if you find it: https://github.com/lostisland/faraday)

  • I think it could be extra helpful if the error message briefly described the category of error: "There was an error parsing your input," "The API server rejected your changes," etc. Some of the more specific error messages can be cryptic, so providing category information could help a user understand more quickly whether they broke the rules of JSON or Arvados.

Done.

  • arv edit doesn't write any message if it successfully updates the object. Should it? That seems like it better follows the general UX patterns for these tools.

Done.

  • Not a blocker, just FYI, any? optionally takes a block, so you can write .any? { condition } rather than @.select { condition }.

Done.

Thanks.

#8 Updated by Brett Smith over 5 years ago

Peter Amstutz wrote:

Brett Smith wrote:

  • If we keep retrying for any non-200 API response, perhaps the rest of the status line ("Forbidden", "Unprocessable Entity", whatever) could be a better default error message when none were provided in the JSON body? (Currently it's an empty string.)

I had this thought too, but the HTTP library documentation doesn't say how to get the status line (let me know if you find it: https://github.com/lostisland/faraday)

Pfft. We can require net/http and ask the standard library for the normal message, at least:

2.1.1 :037 > status = 422
 => 422
2.1.1 :038 > Net::HTTPResponse::CODE_TO_OBJ[status.to_s].to_s.sub(/^Net::HTTP/, "").titleize
 => "Unprocessable Entity" 

I don't exactly like this code, but one grody line seems worthwhile to give the user something to work with.

The names HttpResponse and its status attribute make it seem like I'm dealing with an arbitrary HTTP response, rather than an exception and its message. Please consider a different name for the class (ArvadosAPIError?). And then I don't think you have to do anything else to get the messaging behavior you want:

2.1.1 :040 > class BCSError < RuntimeError
2.1.1 :041?>   end
 => nil
2.1.1 :042 > e = BCSError.new("foobar")
 => #<BCSError: foobar>
2.1.1 :043 > "#{e.class}: #{e}" 
 => "BCSError: foobar" 

Thanks.

#9 Updated by Peter Amstutz over 5 years ago

Brett Smith wrote:

Pfft. We can require net/http and ask the standard library for the normal message, at least:

[...]

I don't exactly like this code, but one grody line seems worthwhile to give the user something to work with.

Done.

The names HttpResponse and its status attribute make it seem like I'm dealing with an arbitrary HTTP response, rather than an exception and its message. Please consider a different name for the class (ArvadosAPIError?). And then I don't think you have to do anything else to get the messaging behavior you want:

Done.

#10 Updated by Brett Smith over 5 years ago

Peter Amstutz wrote:

Brett Smith wrote:

Pfft. We can require net/http and ask the standard library for the normal message, at least:

[...]

I don't exactly like this code, but one grody line seems worthwhile to give the user something to work with.

Done.

  • The titleize is on the wrong side of the parentheses. When the API server returns a full error, that gets titleized into something like "Properties Must Be A Hash, Not A Nil Class." Which Provides It With An Amusing Coat Of Inappropriate Formality.
  • Please move the require statement up near the top of the files with the others, for consistency.

Other bugs found while I was kicking these tires:

  • JSON parse errors aren't being retried, because Oj.parse raises an Oj::ParseError, rather than a JSON::ParseError. (Huh, it's almost like arbitrarily switching between two different JSON modules could confuse programmers…)
  • The successful update message for arv edit assumes that the UUID of the object hasn't been changed, but if I'm an admin I can totally do that. It should render the UUID from the object returned in the response, just like arv create.

Thanks.

#11 Updated by Peter Amstutz over 5 years ago

Brett Smith wrote:

  • The titleize is on the wrong side of the parentheses. When the API server returns a full error, that gets titleized into something like "Properties Must Be A Hash, Not A Nil Class." Which Provides It With An Amusing Coat Of Inappropriate Formality.

Fixed.

  • Please move the require statement up near the top of the files with the others, for consistency.

Fixed.

Other bugs found while I was kicking these tires:

  • JSON parse errors aren't being retried, because Oj.parse raises an Oj::ParseError, rather than a JSON::ParseError. (Huh, it's almost like arbitrarily switching between two different JSON modules could confuse programmers…)

Fixed.

  • The successful update message for arv edit assumes that the UUID of the object hasn't been changed, but if I'm an admin I can totally do that. It should render the UUID from the object returned in the response, just like arv create.

Fixed.

#12 Updated by Brett Smith over 5 years ago

Peter Amstutz wrote:

Brett Smith wrote:

  • JSON parse errors aren't being retried, because Oj.parse raises an Oj::ParseError, rather than a JSON::ParseError. (Huh, it's almost like arbitrarily switching between two different JSON modules could confuse programmers…)

Fixed.

Please collapse the two JSON rescue blocks in check_result to reduce duplicate code (rescue JSON::ParserError, Oj::ParseError => e) and then merge. Thanks.

#13 Updated by Peter Amstutz over 5 years ago

  • Status changed from New to Resolved
  • % Done changed from 67 to 100

Applied in changeset arvados|commit:138a4b0a5de177faede5255841a5c6fca06b31f4.

Also available in: Atom PDF