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

Only update affected media list when deleting destination in HTMX frontend #1120

Merged
merged 2 commits into from
Jan 16, 2025

Conversation

stveit
Copy link
Contributor

@stveit stveit commented Jan 10, 2025

Fixes #1128

Makes it so only the media list for the destination you try to delete is updated with HTMX. If it is deleted succesfully, it just disappears. If it cannot be deleted, a error message is shown at the top of the list.

There was supposed to already be an error message if a destination could not be deleted, but the location of the error message in the templates did not match up with the template used for HTMX when deleting, so the error message was never actually shown prior to this PR, so this PR is also effectively a bugfix to that.

@stveit stveit self-assigned this Jan 10, 2025
Copy link

github-actions bot commented Jan 10, 2025

Test results

   10 files  1 060 suites   38m 7s ⏱️
  536 tests   535 ✅  1 💤 0 ❌
5 360 runs  5 350 ✅ 10 💤 0 ❌

Results for commit 4e46505.

♻️ This comment has been updated with latest results.

@codecov-commenter
Copy link

codecov-commenter commented Jan 10, 2025

Codecov Report

Attention: Patch coverage is 9.09091% with 10 lines in your changes missing coverage. Please review.

Project coverage is 79.67%. Comparing base (2b8a879) to head (4e46505).

Files with missing lines Patch % Lines
src/argus/htmx/destination/views.py 9.09% 10 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1120      +/-   ##
==========================================
- Coverage   79.71%   79.67%   -0.05%     
==========================================
  Files         141      141              
  Lines        5285     5288       +3     
==========================================
  Hits         4213     4213              
- Misses       1072     1075       +3     

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

@stveit stveit force-pushed the htmx-destination-delete-only-affected-row branch from a4325bf to fdfda05 Compare January 10, 2025 15:11
Copy link
Contributor Author

@stveit stveit left a comment

Choose a reason for hiding this comment

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

Self-review: This PR does not update the number in the media header that says how many emaiil/sms destinations exist. So if you have 5, then delete them all, it will still say Email(5)

Copy link
Contributor

@johannaengland johannaengland left a comment

Choose a reason for hiding this comment

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

Agree on that it does not update the number of destinations of that media. But that could also be a polish issue.

Otherwise it looks good, just lacks a news fragment

@stveit stveit force-pushed the htmx-destination-delete-only-affected-row branch 2 times, most recently from e1fba78 to 796cf8c Compare January 15, 2025 08:07
@stveit stveit changed the title Only update affected row when deleting destination in HTMX frontend Only update affected media list when deleting destination in HTMX frontend Jan 15, 2025
@stveit
Copy link
Contributor Author

stveit commented Jan 15, 2025

Updated this PR quite drastically: Now refreshes the list and not only the row

@stveit stveit force-pushed the htmx-destination-delete-only-affected-row branch from 71b7284 to 1cccf5e Compare January 15, 2025 13:56
This was added so the
"This destination cannot be deleted error" could be shown
separate from field specific errors, but after the changes
done to the delete endpoint this is no longer used.
_render_destination_list is complicated enough as-is that
removing this makes it a bit nicer
@stveit stveit force-pushed the htmx-destination-delete-only-affected-row branch from 1cccf5e to 4e46505 Compare January 16, 2025 08:21
@hmpf hmpf merged commit 5154e2c into master Jan 16, 2025
17 checks passed
@hmpf hmpf deleted the htmx-destination-delete-only-affected-row branch January 16, 2025 12:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Update only the related media list when deleting a destination
4 participants