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.

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

Client deletion implement #389

Merged
merged 13 commits into from
Jun 10, 2024
Merged

Client deletion implement #389

merged 13 commits into from
Jun 10, 2024

Conversation

Domejko
Copy link
Contributor

@Domejko Domejko commented May 25, 2024

Description

Regarding #386

  • Added delete feature for clients in backend and updated urls.py.
  • Enabled button in frontend/templates/pages/clients/dashboard/ _rows.html and connected it with backend.

TODO:

  • Display success message. At them moment I'm stuck in this matter since when I enable message about successful deletion by adding {% component "messages_list" %} at the end of _rows.html then message about successful creation of client gets 'bugged' and it just blinks for split of second. I don't know much about HTML so I'm learning on the fly.
  • Make sure that error messages as "Client not found" and "You do not have permission to delete this invoice" are displayed to the user.
  • Write test.

Checklist

  • Ran the Black Formatter and
    djLint-er on any new code
    (checks
    will
    fail without)
  • Made any changes or additions to the documentation where required
  • Changes generate no new warnings/errors
  • New and existing unit tests pass locally with my
    changes

What type of PR is this?

  • ✨ Feature

Added/updated tests?

  • 👍 yes

Related PRs, Issues etc

@Domejko
Copy link
Contributor Author

Domejko commented Jun 4, 2024

@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
Copy link
Owner

TreyWW commented Jun 4, 2024

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
Copy link
Contributor Author

Domejko commented Jun 5, 2024

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
Copy link
Owner

TreyWW commented Jun 5, 2024

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.

@Domejko Domejko marked this pull request as ready for review June 6, 2024 10:36
@TreyWW
Copy link
Owner

TreyWW commented Jun 7, 2024

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

@Domejko
Copy link
Contributor Author

Domejko commented Jun 8, 2024

@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 TreyWW merged commit 2e5dfa9 into TreyWW:main Jun 10, 2024
13 checks passed
@TreyWW
Copy link
Owner

TreyWW commented Jun 10, 2024

Great PR, thanks @Domejko! :)

@Domejko Domejko deleted the delete-client branch July 9, 2024 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: ✔ Done
Development

Successfully merging this pull request may close these issues.

[Feature] Implement Client Deletion via the Client Management Dashboard
2 participants