TreyWW / MyFinances

MyFinances is a web application that can help you as an individual, or team, manage your finances!
https://docs.myfinances.cloud
GNU Affero General Public License v3.0
85 stars 121 forks source link

Client deletion implement #389

Closed Domejko closed 4 weeks ago

Domejko commented 1 month ago

Description

Regarding #386

TODO:

Checklist

What type of PR is this?

Added/updated tests?

Related PRs, Issues etc

Domejko commented 1 month ago

@TreyWW When we return error message with status 404 we get thrown into a loop. One of solutions to avoid this and display error toast message to a user is:

messages.error(request, 'Client not found')
return render(request, "pages/clients/dashboard/_table.html", {"delete": True})

If returning a 404 status is essential I can further investigate that issue. Let me know.

As it comes to JsonResponse from what I have searched we don't have a JS to handle that. We would need a function that will take message from JsonResponse and return it to the user as a error toast message.

TreyWW commented 1 month ago

If returning a 404 status is essential I can further investigate that issue. Let me know.

As it comes to JsonResponse from what I have searched we don't have a JS to handle that. We would need a function that will take message from JsonResponse and return it to the user as a error toast message.

Yeah this is a big issue throughout the project. We, like you say, have two solutions.

  1. Return HTMX code with a django message (show user an error) Issues with this though is that it'll remove the row, and be seen by HTMX as a "success response" even though its an error
  2. Make a HTMX callback handler (like here) to catch error responses and make a custom function to handle a popup. Could be nice cause once this "foundation" is made we can also show errors for ANY failed requests, or retries.

I'm completely happy with option 1 for now though, feel free to just return a message and let the row delete, since the client is "non existent" anyways. It's just I find errors like this mean I've messed up (developer), that a bug caused it (generally at least).

So yeah, fine by me.

Domejko commented 1 month ago

I understand. I will implement and push 1st solution for the time been and in some spare time I will try to work on the 2nd solution but it might take a while since I don't have experience with JS.

TreyWW commented 1 month ago

Update: For option 2 with deleting content, I just checked the HTMX docs and we can actually send off a HX-Reswap and HX-Retarget to send off the notifications still! It would need some django work, but no JS! Woo!

I can provide more details later, just about to head home.

TreyWW commented 1 month ago

@Domejko sorry about the delay. Could you run py manage.py lint

Domejko commented 1 month ago

@TreyWW no problem. Linting and formatting have been fixed now.

What about that HX-Reswap and HX-Retarget you have some more details on that ?

TreyWW commented 4 weeks ago

Great PR, thanks @Domejko! :)