Convert uncaught exceptions to messages.error #1162
Conversation
hmpf
requested review from
lunkwill42,
elfjes,
stveit,
johannaengland and
podliashanyk
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1162 +/- ##
==========================================
- Coverage 78.48% 78.32% -0.17%
==========================================
Files 141 141
Lines 5442 5457 +15
==========================================
+ Hits 4271 4274 +3
- Misses 1171 1183 +12 ☔ View full report in Codecov by Sentry. |
src/argus/htmx/middleware.py
Outdated
| @@ -84,7 +95,11 @@ def process_response(self, request: HtmxHttpRequest, response: HttpResponse) -> | |||
| has_error_message = any("error" in message.tags for message in storage) | |||
| storage.used = False | |||
| if not has_error_message: | |||
| messages.error(request, "An error occured while processing your request, please try again") | |||
| error_msg = f"{response.status_code} {response.reason_phrase}" | |||
There was a problem hiding this comment.
For better UX I would prefer to show an original error msg if there is any in content, and only show status_code and reason_phrase if there is no error message that was passed.
There was a problem hiding this comment.
This is used when no error message was passed!? That is: a fixed text is combined with what the response knows.
There was a problem hiding this comment.
It is also used on f.e. return HttpResponseBadRequest("Some detailed err msg"), but "Some detailed err msg" gets dropped
There was a problem hiding this comment.
Wouldn't that become "An error occurred while processing your request, please try again: STATUSCODE Some detailed err msg"?
There was a problem hiding this comment.
No, it becomes "An error occurred while processing your request, please try again: 400 BadRequest"
There was a problem hiding this comment.
Works kinda, formatting is off though. The message reads: "An error occured while processing your request, please try again: b'Some detailed err msg'".
I would also suggest to shorten "An error occured while processing your request, please try again:" to "An error occured while processing your request:", or "An unexpected error occurred:".
src/argus/htmx/middleware.py
Outdated
| if not error_msg: | ||
| error_msg = f"{response.status_code} {response.reason_phrase}" | ||
| messages.error( | ||
| request, f"An error occured while processing your request, please try again: {error_msg}" |
There was a problem hiding this comment.
Small comment:
| request, f"An error occured while processing your request, please try again: {error_msg}" | |
| request, f"An error occured while processing your request: {error_msg}. Please try again" |
|
| def process_exception(self, request, exception): | ||
| error_msg = "500 Internal Server Error" | ||
| if str(exception): | ||
| error_msg += f": {exception}" | ||
| messages.error(request, error_msg) | ||
| LOG.error(error_msg) | ||
| return None |
There was a problem hiding this comment.
Wait, now that I think about this. I don't think we should be returning the raw exception message to the user. This could lead to leaking sensitve information to the user which is a security risk
To test this
DEBUGneeds to be set toFalseto force not showing the yellow debug page.