Show a message when a http error occured during a htmx request

[elfjes]

Whenever a htmx request returns a non 200 reponse, this is ignored by htmx and the user does not get an error notification. This PR adds some behaviour so that the user always gets a toast message

from Uninett/argus-htmx-frontend#241

[codecov-commenter]

Codecov Report

Attention: Patch coverage is 58.82353% with 7 lines in your changes missing coverage. Please review.

Project coverage is 81.08%. Comparing base (622cfad) to head (a13f9e1).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
src/argus/htmx/middleware.py	36.36%	7 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #1023      +/-   ##
==========================================
- Coverage   81.18%   81.08%   -0.10%     
==========================================
  Files         140      141       +1     
  Lines        5076     5087      +11     
==========================================
+ Hits         4121     4125       +4     
- Misses        955      962       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[hmpf]

See comment.

We should probably have a page that always returns an error > 400 so we can check that this works. Not strictly needed in this PR though, see #1024

[podliashanyk]

Looks good as a start of error feedback to user! 🙌
We will need more granular error handling for some 400 errors, f.e. input field errors like if user tries to create a media destination with invalid title. In that case we will want to show the error message under the input field itself. How should we go about to being able to disable showing some of the error messages? We will also need a customizable error text that is displayed in the alerts.

[podliashanyk]

Nice! 🙌

[elfjes]

Looks good as a start of error feedback to user! 🙌 We will need more granular error handling for some 400 errors, f.e. input field errors like if user tries to create a media destination with invalid title. In that case we will want to show the error message under the input field itself. How should we go about to being able to disable showing some of the error messages? We will also need a customizable error text that is displayed in the alerts.

I think it will be hard to generalize placement. I'm also not sure whether the messages framework is the most suitable for that job. I think we might be better off constructing an error dictionary, and processing that in the template. (Be sure to respond with 200 if it's a partial response, otherwise htmx won't process it)

The messages framework is good if you have messages coming from various sources and want to place them in a single place. You can have some categorization using tags, but that's really all. What we want in case of validation errors is have multiple messages from a single source (validation) and place them in various places.

Now, if we want to show messages in a different place then normal for a specific response, we just need to consume the messages in the template, and they will not be rerendered by HtmxMessageMiddleware. We also want to make sure to reply with a 200 to not trigger the code from this PR (and to have htmx actually process the response)

[podliashanyk]

Looks good as a start of error feedback to user! 🙌 We will need more granular error handling for some 400 errors, f.e. input field errors like if user tries to create a media destination with invalid title. In that case we will want to show the error message under the input field itself. How should we go about to being able to disable showing some of the error messages? We will also need a customizable error text that is displayed in the alerts.

I think it will be hard to generalize placement. I'm also not sure whether the messages framework is the most suitable for that job. I think we might be better off constructing an error dictionary, and processing that in the template. (Be sure to respond with 200 if it's a partial response, otherwise htmx won't process it)

The messages framework is good if you have messages coming from various sources and want to place them in a single place. You can have some categorization using tags, but that's really all. What we want in case of validation errors is have multiple messages from a single source (validation) and place them in various places.

Now, if we want to show messages in a different place then normal for a specific response, we just need to consume the messages in the template, and they will not be rerendered by HtmxMessageMiddleware. We also want to make sure to reply with a 200 to not trigger the code from this PR (and to have htmx actually process the response)

I was thinking of using the response-targets extension for that. So that we won't need a) to use messages framework for errors that need to be displayed elsewhere in UI; b) to respond with 200 on errors (I am not a fan of the idea of responding with 200 on errors. It is misleading, especially when debugging in the browser). Will consuming the messages in the template stop error alerts from rendering in that case as well?

[elfjes]

I was thinking of using the response-targets extension for that. So that we won't need a) to use messages framework for errors that need to be displayed elsewhere in UI; b) to respond with 200 on errors (I am not a fan of the idea of responding with 200 on errors. It is misleading, especially when debugging in the browser). Will consuming the messages in the template stop error alerts from rendering in that case as well?

Interesting extension, as it would allow for htmx to swap out non-200 responses, although we'd need to be explicit about it, either through a HX-Retarget header or a hx-target-... attribute. It can't be configured to have a single attribute accept both 200 and error responses. So while it is nice to be able to return non-200 htmx responses, it does have an impact on maintainability.

Then, if non-200 htmx responses can have valid content, we'd need to update the logic in this PR to determine whether we need to respond with a (generic) error message notification. Since you're right in that if we already consumed the django messages in our initial rendering, with the current logic we'd falsly conclude that we need to add a new message, and worse: we'd throw out the now-perfectly-valid response content. This can be easily fixed though, by setting (and checking for) an attribute on the request, such as request._htmx_error=True. A view that returns a valid non-200 response can set it and then the middleware wouldn't replace the response or add any additional messages. (we could set that attribute to the response instead, but we don't have an explicit reference to the response in every view, such as when calling render, so we'd need to do response=render(...); response._htmx_error=True; return response or something, which is not as nice)

[podliashanyk]

I was thinking of using the response-targets extension for that. So that we won't need a) to use messages framework for errors that need to be displayed elsewhere in UI; b) to respond with 200 on errors (I am not a fan of the idea of responding with 200 on errors. It is misleading, especially when debugging in the browser). Will consuming the messages in the template stop error alerts from rendering in that case as well?

Interesting extension, as it would allow for htmx to swap out non-200 responses, although we'd need to be explicit about it, either through a HX-Retarget header or a hx-target-... attribute. It can't be configured to have a single attribute accept both 200 and error responses. So while it is nice to be able to return non-200 htmx responses, it does have an impact on maintainability.

The only maintainability overhead I immediately can see is that we will need to add and maintain the extension js file. Are there more?

Then, if non-200 htmx responses can have valid content, we'd need to update the logic in this PR to determine whether we need to respond with a (generic) error message notification. Since you're right in that if we already consumed the django messages in our initial rendering, with the current logic we'd falsly conclude that we need to add a new message, and worse: we'd throw out the now-perfectly-valid response content. This can be easily fixed though, by setting (and checking for) an attribute on the request, such as request._htmx_error=True. A view that returns a valid non-200 response can set it and then the middleware wouldn't replace the response or add any additional messages. (we could set that attribute to the response instead, but we don't have an explicit reference to the response in every view, such as when calling render, so we'd need to do response=render(...); response._htmx_error=True; return response or something, which is not as nice)

Sounds good to me even if it doesn't look the nicest. Especially since there are not that many htmx views that would need to return non-200 htmx responses I suspect (all of them being input field errors in different forms)

[elfjes]

The only maintainability overhead I immediately can see is that we will need to add and maintain the extension js file. Are there more?

It does introduce more methods of swapping out htmx content. Another cog in the machine, so to say. And that means that we have to be more careful about when to use which method, in order to keep consistency in the code base. Introducing another thing always has up- and downsides, and it's important to figure out whether upsides outweigh the downsides.

I'm not saying that we shouldn't add it as a dependency, but I think we'd need to have good use case before introducing it, and I'm not sure whether "so that I can see a 4xx in the debug console" is solid enough.

And there might very well be an excellent use case. I encourage you to experiment and discover the benefits of response-targets and how it provides a nicer way of dealing with certain (error-)situations. Or if you already have good examples, please share :)

Then, if non-200 htmx responses can have valid content, we'd need to update the logic in this PR to determine whether we need to respond with a (generic) error message notification. Since you're right in that if we already consumed the django messages in our initial rendering, with the current logic we'd falsly conclude that we need to add a new message, and worse: we'd throw out the now-perfectly-valid response content. This can be easily fixed though, by setting (and checking for) an attribute on the request, such as request._htmx_error=True. A view that returns a valid non-200 response can set it and then the middleware wouldn't replace the response or add any additional messages. (we could set that attribute to the response instead, but we don't have an explicit reference to the response in every view, such as when calling render, so we'd need to do response=render(...); response._htmx_error=True; return response or something, which is not as nice)

Sounds good to me even if it doesn't look the nicest. Especially since there are not that many htmx views that would need to return non-200 htmx responses I suspect (all of them being input field errors in different forms)

which part sounds good to you? about an _htmx_error attribute? on request or response?

btw. although I find it an interesting discussion. I'm not sure this PR is the place to have it. Maybe we can make a separate issue for it?

[podliashanyk]

It does introduce more methods of swapping out htmx content. Another cog in the machine, so to say. And that means that we have to be more careful about when to use which method, in order to keep consistency in the code base. Introducing another thing always has up- and downsides, and it's important to figure out whether upsides outweigh the downsides.

I'm not saying that we shouldn't add it as a dependency, but I think we'd need to have good use case before introducing it, and I'm not sure whether "so that I can see a 4xx in the debug console" is solid enough.

And there might very well be an excellent use case. I encourage you to experiment and discover the benefits of response-targets and how it provides a nicer way of dealing with certain (error-)situations. Or if you already have good examples, please share :)

response-targets has nothing extra to offer feature-wise that we wouldn't be able to replace with HX-Retarget. We will furthermore need to supplement response-targets with HX-Reswap (or django-htmx's reswap) in some use-cases. But I like the response-targets option more solely because of not needing to return 200 on errors. I recognize that I am being somewhat dogmatic about it yet I wouldn't downplay the importance of persisting http error code up to the browser.
What I am thinking of is this: the only way for us to quickly distinguish between "true 200" and "200 on error" is the color of an UI alert, in other words we assume that the visual alert component will always render as we expect (or render at all). Yes, it is unlikely that alert rendering will be often compromised in production (and if it is there are likely some big issues happening), but I can't say the same about the dev environment. We are constantly introducing instabilities during the dev process and I like my network tab to show me true HTTP codes (without me needing to dig into response bodies to see whether the request actually returned an error or not). In other words I don't assume that alert will always render correctly to quickly tell me what is error and what is 200 OK, I very much prefer for my true error codes to be there.
With that being said, I can accept "200 on error" if absolutely necessary.

which part sounds good to you? about an _htmx_error attribute? on request or response?

Both work well IMO, but I prefer the response one slightly more for an arbitrary reason that "it makes more sense to me to do it this way" 😄

btw. although I find it an interesting discussion. I'm not sure this PR is the place to have it. Maybe we can make a separate issue for it?

Sounds good to make a discussion out of it. My bottom line question for this PR is: do we need to update the code here so that it is easy to "disable" showing error alerts in some cases in the future or is it good to go (besides merge conflicts)?

[elfjes]

response-targets has nothing extra to offer feature-wise that we wouldn't be able to replace with HX-Retarget. We will furthermore need to supplement response-targets with HX-Reswap (or django-htmx's reswap) in some use-cases. But I like the response-targets option more solely because of not needing to return 200 on errors. I recognize that I am being somewhat dogmatic about it yet I wouldn't downplay the importance of persisting http error code up to the browser. What I am thinking of is this: the only way for us to quickly distinguish between "true 200" and "200 on error" is the color of an UI alert, in other words we assume that the visual alert component will always render as we expect (or render at all). Yes, it is unlikely that alert rendering will be often compromised in production (and if it is there are likely some big issues happening), but I can't say the same about the dev environment. We are constantly introducing instabilities during the dev process and I like my network tab to show me true HTTP codes (without me needing to dig into response bodies to see whether the request actually returned an error or not). In other words I don't assume that alert will always render correctly to quickly tell me what is error and what is 200 OK, I very much prefer for my true error codes to be there. With that being said, I can accept "200 on error" if absolutely necessary.

You have a point, I'll leave it up to you then :)

Both work well IMO, but I prefer the response one slightly more for an arbitrary reason that "it makes more sense to me to do it this way" 😄

yes doing it on the response makes more sense to me too. Just a bit more tedious to do. Although I guess we could wrap it in a function

def render_htmx_error(*args, **kwargs):
    response = render(*args, **kwargs)
    response._htmx_error = True
    return response

or something like that

Sounds good to make a discussion out of it.

can you do that?

My bottom line question for this PR is: do we need to update the code here so that it is easy to "disable" showing error alerts in some cases in the future or is it good to go (besides merge conflicts)?

See above, easy to accomodate for when we get there

[hmpf]

How does Django forms and the errors-attribute figure into this? I would prefer errors for an input field to be kept by that input field, those errors rendered by the template. Too much htmx-magic and you make it harder for the rest of us.

(Especially since we still don't have tests for the HTMx-bit of pages...)

[elfjes]

Short answer: Don't use the messages framework for form errors. Like I said previously:

The messages framework is good if you have messages coming from various sources and want to place them in a single place. You can have some categorization using tags, but that's really all. What we want in case of validation errors is have multiple messages from a single source (validation) and place them in various places.

For form errors, you'd want to stick with django's form validation functionality as much as possible (and access the errors through the form fields in your template)

Then the question is: after a form validation error, do you return 200 or 4xx? 4xx may be more appropiate and has some nice debugging benefits, but adds some complexity and requires the htmx response-targets extension. An interesting discussion that is however not relevant for this PR :)

[sonarqubecloud]

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

[podliashanyk]

Sounds good to make a discussion out of it.

can you do that?

Will do!

See above, easy to accomodate for when we get there

Great, then this PR is good to go. Nicely done! 🙌

... Too much htmx-magic and you make it harder for the rest of us.

response-targets shouldn't add to much magic when developing. Not much more magic and complexity than an already existing hx-target attribute anyways (what it does is lets you add several hx-targets to an element). Although it is an added complexity nonetheless. I will create a discussion on the topic