Alert user when error requesting a report #12867
Conversation
dacook
added
the
technical changes only
|
Oh, that makes sense, thanks for checking that out. It's worrying that the specs didn't pick that up! |
I think the system specs accept alert by default if no behaviour specified. |
app/webpacker/js/turbo.js
Outdated
| if(status == 401) { | ||
| alert(I18n.t("errors.unauthorized.message")); | ||
| } else if(status === undefined) { | ||
| alert(I18n.t("errors.network_error.message")); | ||
| } else { |
There was a problem hiding this comment.
Looking at this again, I think we should not catch all 4xx (Client errors) with a generic message. They need specific handling and perhaps are handled differently in different parts of the app.
But we can catch all 5xx (server errors) and give a general message.
| } else { | |
| } else if (status >= 500) { |
(I'm not sure if status is a string or integer, but it will be sorted correctly either way)
How does that sound to you @rioug?
There was a problem hiding this comment.
I note that the existing frame-missing event handler would no longer cover the situation where an unexpected client error results in no turbo-frame response. But Gaetan already tested that a 422 is handled correctly, so it should be fine 👍
I'm not sure why, but Turbo was swallowing the unauthorized error, so I thought we should alert the user to help with debugging. Same with network error, it gave no feedback before. I think this is testable in theory, but I haven't invested the time on it.
dacook
force-pushed
the
turbo-error-messages
branch
from
32dac1a to
a3a7968
Compare
dacook
added
pr-staged-au
Testing:Network error ✅
Application error (422) ✅
Authentication error 🆇Not as I hoped, because when logged out it returns a 302. We don't handle those (before or after this PR). Maybe next time... Authorisation error (401) ✅I achieved this by removing admin permission on my user. Server error (503) ✅The alert appeared twice again. |
I'm not sure why, but Turbo was swallowing the
unauthorizederror, so I thought we should alert the user to help with debugging. Same with network error, it gave no feedback before.I think this can be tested with Capybara in theory, but I haven't invested the time on it.
What should we test?
This occurs whenever submitting a form with Turbo. The best example is running any report.
There are three types of errors:
This can be tested with bug Asking a report has no result when the producer is set as profile only (doesn't sell products) #12835. OtherwiseI think you could test this by log out in another tab then log in as a shoppersudo systemctl stop puma(then start it again).There are other types of errors that aren't handled by this and should be handled differently, for example: