Skip to content
This repository has been archived by the owner on Dec 13, 2020. It is now read-only.

Error handling prettyfied #253

Closed
cadavre opened this issue Feb 7, 2017 · 11 comments
Closed

Error handling prettyfied #253

cadavre opened this issue Feb 7, 2017 · 11 comments

Comments

@cadavre
Copy link
Contributor

cadavre commented Feb 7, 2017

We're deep into project and I think it's final time to think about proper error handling across the system. We have different types of errors: server errors, errors generated by bad user input, instant errors provided when user action in not possible to accomplish or errors that are caused by lack of relations (like between BPartner, Pricing system and Product adding into Sales order)...

What we'd like to cover here is mostly ALL types of errors, when they happen, where should we display them and how we should display them.

It's also about UI enhancements to make error reading more user-friendly.

Error types

Global errors

Entered URL could not be resolved due to not found 404 of one of entities. I.e. /window/143/12345 – if 143 or 12345 are not found.

Solution: define "main" API calls that must be non-error to display particular page. If any failed = display connected whole-page error. That may mean this i.e. /layout and /data would be marked as main and if any of this calls fail with i.e. 404 – we will display generic 404 page.

Context errors

Lookup or Dropdown widget's loading of data caused error.

Solution: Display lookup errors from /typeahead calls into info container that is part of lookup (one that now displays "There are no results"). But! specify additional color (red) that indicated it is error text.

Phrase entered in Lookup is not valid. (relates to #223)

&&

Value entered in input (text, number etc.) widget is not valid.

Solution: We shall introduce input-context errors and display them next to particular input, with marking this input red (as below mentioned by @ damianprzygodzki ).

Will be solved by #446 .

@cadavre
Copy link
Contributor Author

cadavre commented Feb 7, 2017

About notifications enhancements, we shall add:

  1. As long as mouse is over notification – keep displaying it.
  2. Add "elapsed" bar that displays gently how long particular notification will be displayed.
  3. We shall stack multiple same error into one notification with "label" with number of errors.

@cadavre
Copy link
Contributor Author

cadavre commented Feb 7, 2017

What we also need is to define single JSON envelope for errors and decide how are we handling error codes. I strongly encourage to use HTTP codes, but I guess we shall also add internal codes for which we can provide human-readable translations.

I guess we could also add a ?dev=true query (or header) flag which would enable full error output, and if dev=false display only HTTP equivalents of errors. This would mean that user won't see:

Document /W143/1003511 state is invalid: Cannot create included document because it's not allowed. AllowCreateNewDocument: FALSE{ParentDocumentProcessed} EntityDescriptor: DocumentEntityDescriptor{tableName=C_OrderLine, fields.count=102}

but simple User error as a solution for the beginning.

Also with dev=true we could add possibility to click on notification to open modal with full error description, formatted in pretty way.

@teosarca
Copy link
Member

teosarca commented Feb 7, 2017

NOTE: following issue relates to this task too: #223

@cadavre
Copy link
Contributor Author

cadavre commented Feb 7, 2017

My proposal is to fit something like this:

"error": {
  "code": 400,
  "status": "Bad Request",
  "internal": 400111,
  "title": "Entered data is not valid",
  "detail": "Cannot add product to Completed sales order.",
  "source": {}
}
  • Error code and status are HTTP equivalents.
  • Error internal is metasfresh's error code.
  • Error title is something that can fit in any "title" (of notification, popup etc.).
  • Error detail is something that can fit error container.
  • Error source may be anything that helps developer to track the bug - stacktrace, SQL error info, validation results... This shall be appended only for dev=true.

@teosarca
Copy link
Member

teosarca commented Feb 7, 2017

Hi @cadavre , basically i agree with you... but i have to deep a bit more.
But, one this that i would like is to preserve the fields which are currently provided by spring,
e.g.

{
  "timestamp": 1486484620399,
  "status": 404,
  "error": "Not Found",
  "exception": "de.metas.ui.web.window.exceptions.DocumentNotFoundException",
  "message": "* Nicht gefunden * DocumentKey{type=Window, typeId=143, documentId=111}",
  "path": "/rest/api/window/143/111"
}

wdyt?

@cadavre
Copy link
Contributor Author

cadavre commented Feb 7, 2017

In 1st document you can find a list of errors. If any of you have something about errors to add – write it here and I'll rewrite to first post.

Another thing to add would be well-designed error pages, for example for 404 error.

@teosarca sure we can preserve them. One question: do you need them in root of JSON response or can we wrap it into "java" leaf? Or maybe you'd like to keep single error responses in Swing and frontend?

{
  "timestamp": 1486484620399,
  "status": 404,
  "error": "Not Found",
  "exception": "de.metas.ui.web.window.exceptions.DocumentNotFoundException",
  "message": "* Nicht gefunden * DocumentKey{type=Window, typeId=143, documentId=111}",
  "path": "/rest/api/window/143/111",
  "ui": {
    "internal": 400111,
    "title": "Entered data is not valid",
    "detail": "Cannot add product to Completed sales order."
  }
}

I think above would be acceptable too.

@damianprzygodzki
Copy link
Contributor

What i want to add, in addition to #223 issue too, we have to handle errors in widget in one generic way (and surely it should be dropdown - because it is not always visible, and for now there are only items)

Long time ago, we developped state for corrupted inputs:
screen shot 2017-02-08 at 10 56 12
screen shot 2017-02-08 at 10 56 26

It is already implemented, we can add error message for that, under the input.

What i want to say, user inputs errors, should be consistent for all of inputs. And we should not varying them.

@damianprzygodzki
Copy link
Contributor

Second case, is that we cannot catch each 404 and display it in generic way - because 404 is more about context of entity, and sometimes user should be notified.

So, we have to make assumption about handling 404:

  • what are the places that you want to display 404 notifications (i know that "window not found" is the place)
  • do we want to get rid of that? (notifications for 404, replaced by context handling, like 404 pages, or whatever similar)
  • and maybe we should go 100% into generic way of error notifications, and leave it for API to decide if we should return notification (error code independently)

@cadavre
Copy link
Contributor Author

cadavre commented Feb 14, 2017

@damianprzygodzki I guess it won't be possible for API to decide how UI error shall be presented. Actually I can imagine situation where one API call gives you good data but another failed for some reason (i.e. to generate breadcrumb) – they both may generate 404's.

@cadavre
Copy link
Contributor Author

cadavre commented Feb 14, 2017

I added some solution suggestions in 1st post.

@cadavre
Copy link
Contributor Author

cadavre commented Apr 19, 2017

Closing because two things are left:

  • 404
  • errors in lookups

Splitting into separate issues.

@cadavre cadavre closed this as completed Apr 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants