Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Convert uncaught exceptions to messages.error #1162

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
Open

Convert uncaught exceptions to messages.error #1162

Show file tree
Hide file tree
Changes from 2 commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a434cd5
Convert uncaught exceptions to messages.error
hmpf Jan 23, 2025
593f8ca
Add changelog fragment
hmpf Jan 24, 2025
120b1ef
hide exception without args
hmpf Jan 24, 2025
2fc059e
dump content of response if exists
hmpf Jan 24, 2025
6b68b12
change looks of error_msg
hmpf Jan 27, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions changelog.d/+convert-exceptions-to-messages.changed.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1 @@
Htmx: Otherwise uncaught exceptions are caught, logged and converted to messages.error.
17 changes: 16 additions & 1 deletion src/argus/htmx/middleware.py
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,3 +1,5 @@
import logging

from django.conf import settings
from django.contrib.auth.views import redirect_to_login
from django.http import HttpResponse
Expand All @@ -6,8 +8,11 @@
from django.utils.encoding import force_str
from django_htmx.http import HttpResponseClientRedirect
from django.contrib import messages

from .request import HtmxHttpRequest

LOG = logging.getLogger(__name__)


class LoginRequiredMiddleware:
def __init__(self, get_response):
Expand Down Expand Up @@ -61,6 +66,12 @@ class HtmxMessageMiddleware(MiddlewareMixin):

TEMPLATE = "messages/_notification_messages_htmx_append.html"

def process_exception(self, request, exception):
error_msg = f"500 Internal Server Error: {exception}"
podliashanyk marked this conversation as resolved.
Show resolved Hide resolved
messages.error(request, error_msg)
LOG.error(error_msg)
return None

def process_response(self, request: HtmxHttpRequest, response: HttpResponse) -> HttpResponse:
if not request.htmx:
return response
Expand All @@ -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}"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is used when no error message was passed!? That is: a fixed text is combined with what the response knows.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is also used on f.e. return HttpResponseBadRequest("Some detailed err msg"), but "Some detailed err msg" gets dropped

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wouldn't that become "An error occurred while processing your request, please try again: STATUSCODE Some detailed err msg"?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No, it becomes "An error occurred while processing your request, please try again: 400 BadRequest"

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See new commit

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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:".

messages.error(
request, f"An error occured while processing your request, please try again: {error_msg}"
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Small comment:

Suggested change
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"

)
LOG.error("Unhandled exception: %s", error_msg)
# HTMX doesn't swap content for response codes >=400. However, we do want to show
# the new messages, so we need to rewrite the response to 200, and make sure it only
# swaps the oob notification content
Expand Down
Loading