search

msupply-foundation / open-msupply

Open mSupply represents our most recent advancement in the Logistics Management Information System (LMIS), expanding on more than two decades of development inherited from the well-established legacy of the original mSupply.
https://msupply.foundation/open-msupply/
Other
20 stars 12 forks source link

QueryErrorHandler doesn't report repeated errors #3984

Open lache-melvin opened 3 months ago

lache-melvin commented 3 months ago

What went wrong? 😲

Error snack does not display again after first error:

https://github.com/msupply-foundation/open-msupply/assets/55115239/a0df8303-bbe8-4465-bc5c-b42939c6b187

This appears to be because the QueryErrorHandler only calls the error snack if its errorMessage reference changes. (So e.g. first error was ReturnIsNotEditable, second error is NetworkError, that would display fine.)

The errorMessage state is reset when we change pages, but otherwise, if we encounter the same error message twice in a row, we will only get the snack the first time!

I think this is particularly bad because the snack disappears - maybe I didn't finish reading it in time, I want to click OK again to finish reading the error message.

OR I think I fixed my entered data (but actually I hadn't fixed it, BE returning same error) so I click OK again, and now not getting any error message, but the modal won't close.

Expected behaviour 🤔

If the same problem happens twice, I am told about it twice....

How to Reproduce 🔨

Steps to reproduce the behaviour:

I think stopping the server is the easiest/fastest - but have seen with many places - with either standard errors, or where we throw errors from the api.ts files...

  1. Create something e.g. an inbound return with at least one line
  2. Stop your server
  3. Try to edit the line
  4. First time, get NetworkError
  5. Click OK again
  6. BUG: no error snack

OR: this is what I did for the video:

  1. Create an inbound return with at least one line
  2. Open the edit modal for that line
  3. Now, in another window (incognito?) set that inbound return to verified
  4. Back in the first window, now try to click OK in your edit modal
  5. First time, get ReturnIsNotEditable
  6. Click it again
  7. BUG: no error snack

Your environment 🌱

mark-prins commented 3 months ago

That was deliberate - we were getting duplicate errors displayed on a single page and it looks a litle incompetent, or at the least hamfisted!

If fixing, we need to be able to distinguish between multiple calls to the same action / api resulting in the same error and the user initiating something that results in the same error. possibly useQuery is initiating an action multiple times and reporting the same error multiple times?

lache-melvin commented 3 months ago

If fixing, we need to be able to distinguish between multiple calls to the same action / api resulting in the same error and the user initiating something that results in the same error.

Hm, or a way to reset error state once the toast disappears?

Vaguely related - I've seen the multiple error toasts thing in a few places where we catch thrown errors in the calling component... i.e. API error thrown, QueryErrorHandler catches, toasts and rethrows, caught again by component, toast again - but I think that needs a separate solution

clemens-msupply commented 3 months ago

For this specific case the UI could prevent the action and give a reason why the action can't be applied.

andreievg commented 3 months ago

Sorry about the chatter in the triage meeting. I was worried that we are hot fixing the problem by hiding it. If we are getting multiple errors maybe there are too many queries being executed or the error handling is not optimal (we are toasting in more then one place) ?

I was wondering what area of the app this has happened previously ?

mark-prins commented 3 months ago

agree - if we are still getting multiple toasts showing the same error, when the user has only done one action: then we should look at why there are so many.

I just didn't want us enabling the multi-toast again without investigating. As a general rule - the user should only be notified once of an error cause by their action. if they perform the same action again, and the error state reoccurs, sure, they should be notified again.