Summary: | Inconsistent delete confirmation dialog for "Delete list" buttons | ||
---|---|---|---|
Product: | Koha | Reporter: | Yanjun Li <yanjun.li> |
Component: | Lists | Assignee: | Yanjun Li <yanjun.li> |
Status: | Pushed to main --- | QA Contact: | Brendan Lawlor <blawlor> |
Severity: | enhancement | ||
Priority: | P5 - low | CC: | blawlor, david, lucas, m.de.rooy |
Version: | Main | Keywords: | Academy |
Hardware: | All | ||
OS: | All | ||
Change sponsored?: | --- | Patch complexity: | String patch |
Documentation contact: | Documentation submission: | ||
Text to go in the release notes: |
This enhancement adds a confirmation step when deleting a list from the "Your lists" and "Public lists" tabs in the staff interface. Previously, you were not asked to confirm the list deletion. This also makes it consistent with deleting a list from its contents page, where you are asked to confirm the list deletion.
|
Version(s) released in: |
25.05.00
|
Circulation function: | |||
Attachments: |
Bug 38302: Add delete confirmation dialog
Bug 38302: Add delete confirmation dialog Bug 38302: Add delete confirmation dialog Bug 38302: (follow-up) Remove duplicate click handler and modify button class Bug 38302: Add delete confirmation dialog Bug 38302: (follow-up) Remove duplicate click handler and modify button class Bug 38302: (follow-up) Add t() to make sure delete button is translatable Bug 38302: Add delete confirmation dialog Bug 38302: Add delete confirmation dialog |
Description
Yanjun Li
2024-10-30 15:23:18 UTC
Created attachment 173953 [details] [review] Bug 38302: Add delete confirmation dialog To test: 1. Create a list in the staff interface 2. Go to lists 3. See "Your lists" tab and find "Delete" button in "Actions" col, click "delete", no confirmation dialog pop up 4. Create another list 5. Go to lists and view the list detail 6. See "Edit" dropdown and click "Delete Lists" button, see confirmation dialog pop up 7. Apply patch 8. Try step 1 through 3 again 9. "Delete" button in "actions" column has the same confirmation dialog, and delete function works Created attachment 174052 [details] [review] Bug 38302: Add delete confirmation dialog To test: 1. Create a list in the staff interface 2. Go to lists 3. See "Your lists" tab and find "Delete" button in "Actions" col, click "delete", no confirmation dialog pop up 4. Create another list 5. Go to lists and view the list detail 6. See "Edit" dropdown and click "Delete Lists" button, see confirmation dialog pop up 7. Apply patch 8. Try step 1 through 3 again 9. "Delete" button in "actions" column has the same confirmation dialog, and delete function works Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> This works, but I think the code block in the patch is partly unnecessary. It looks like the code block directly above this patch is really supposed to be the click handler for this alert with the MSG_CONFIRM_DELETE_LIST, but the class on the button is different so it never runs. The class 'deleteshelf' is used on the opac side, but in shelves_results.tt the delete button has the class named 'delete-list' instead. Before the patch if you git grep -n 'delete-list' you can see that it only appears in the class on the button in shelves_results.tt I don't think the name of the class really matters, but we shouldn't have a duplicate click handler that does nothing. Thanks for finding and fixing this bug. If you resubmit a patch without the duplicate click handler I will pass QA. Created attachment 174639 [details] [review] Bug 38302: Add delete confirmation dialog To test: 1. Create a list in the staff interface 2. Go to lists 3. See "Your lists" tab and find "Delete" button in "Actions" col, click "delete", no confirmation dialog pop up 4. Create another list 5. Go to lists and view the list detail 6. See "Edit" dropdown and click "Delete Lists" button, see confirmation dialog pop up 7. Apply patch 8. Try step 1 through 3 again 9. "Delete" button in "actions" column has the same confirmation dialog, and delete function works Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Created attachment 174640 [details] [review] Bug 38302: (follow-up) Remove duplicate click handler and modify button class This follow-up patch removes the unnecessarily added click handler, and modifies the delete button class. The confirmation dialog and delete function work. (In reply to Brendan Lawlor from comment #3) > This works, but I think the code block in the patch is partly unnecessary. > > It looks like the code block directly above this patch is really supposed to > be the click handler for this alert with the MSG_CONFIRM_DELETE_LIST, but > the class on the button is different so it never runs. > > The class 'deleteshelf' is used on the opac side, but in shelves_results.tt > the delete button has the class named 'delete-list' instead. > > Before the patch if you git grep -n 'delete-list' you can see that it only > appears in the class on the button in shelves_results.tt > > I don't think the name of the class really matters, but we shouldn't have a > duplicate click handler that does nothing. > > Thanks for finding and fixing this bug. If you resubmit a patch without the > duplicate click handler I will pass QA. Hi Brendan, thank you so much for the explanation. I already deleted the duplicate click handler, and I will pay more attention next time to avoid writing duplicate code. Created attachment 174719 [details] [review] Bug 38302: Add delete confirmation dialog To test: 1. Create a list in the staff interface 2. Go to lists 3. See "Your lists" tab and find "Delete" button in "Actions" col, click "delete", no confirmation dialog pop up 4. Create another list 5. Go to lists and view the list detail 6. See "Edit" dropdown and click "Delete Lists" button, see confirmation dialog pop up 7. Apply patch 8. Try step 1 through 3 again 9. "Delete" button in "actions" column has the same confirmation dialog, and delete function works Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Created attachment 174720 [details] [review] Bug 38302: (follow-up) Remove duplicate click handler and modify button class This follow-up patch removes the unnecessarily added click handler, and modifies the delete button class. The confirmation dialog and delete function work. Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Trivial change, works, makes sense, qa scripts happy. Passing QA. Thanks Yanjun! Thanks Yanjun! As we are in feature freeze, this will have to wait until after the release, but will go into 25.05 early on! I am sorry, but this makes the "Delte" on the button untranslatable: - [%~ action_block = action_block _ '<button type="submit" class="delete-list btn btn-default btn-xs"><i class="fa fa-trash-can"></i> ' _ t("Delete") _ '</button>' ~%] + [%~ action_block = action_block _ '<button type="submit" class="deleteshelf btn btn-default btn-xs"><i class="fa fa-trash-can"></i> Delete</button>' ~%] The t() is needed for the translation. Created attachment 175951 [details] [review] Bug 38302: (follow-up) Add t() to make sure delete button is translatable Treating the small change as a follow-up. As the change is rather small I have squashed the patches before pushing. Thanks for your perseverance! Created attachment 175955 [details] [review] Bug 38302: Add delete confirmation dialog To test: 1. Create a list in the staff interface 2. Go to lists 3. See "Your lists" tab and find "Delete" button in "Actions" col, click "delete", no confirmation dialog pop up 4. Create another list 5. Go to lists and view the list detail 6. See "Edit" dropdown and click "Delete Lists" button, see confirmation dialog pop up 7. Apply patch 8. Try step 1 through 3 again 9. "Delete" button in "actions" column has the same confirmation dialog, and delete function works Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Created attachment 175956 [details] [review] Bug 38302: Add delete confirmation dialog To test: 1. Create a list in the staff interface 2. Go to lists 3. See "Your lists" tab and find "Delete" button in "Actions" col, click "delete", no confirmation dialog pop up 4. Create another list 5. Go to lists and view the list detail 6. See "Edit" dropdown and click "Delete Lists" button, see confirmation dialog pop up 7. Apply patch 8. Try step 1 through 3 again 9. "Delete" button in "actions" column has the same confirmation dialog, and delete function works Signed-off-by: Lucas Gass <lucas@bywatersolutions.com> Signed-off-by: Brendan Lawlor <blawlor@clamsnet.org> Pushed for 25.05! Well done everyone, thank you! |