Bug 38302

Summary: Inconsistent delete confirmation dialog for "Delete list" buttons
Product: Koha Reporter: Yanjun Li <yanjun.li>
Component: ListsAssignee: 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: MainKeywords: 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
In the staff interface, there're two ways to delete a list: 

1) "Delete" button in "Your lists" table
2) "Delete List" button in "Edit" dropdown

Option 2) has a confirmation dialog "Are you sure you want to delete this list?" but option 1) does not. 


To recreate: 

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
Comment 1 Yanjun Li 2024-11-04 22:29:04 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
Comment 2 Lucas Gass (lukeg) 2024-11-05 23:54:15 UTC
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>
Comment 3 Brendan Lawlor 2024-11-15 22:13:54 UTC
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.
Comment 4 Yanjun Li 2024-11-15 23:24:57 UTC
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>
Comment 5 Yanjun Li 2024-11-15 23:24:59 UTC
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.
Comment 6 Yanjun Li 2024-11-15 23:34:47 UTC
(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.
Comment 7 Brendan Lawlor 2024-11-18 15:33:32 UTC
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>
Comment 8 Brendan Lawlor 2024-11-18 15:33:34 UTC
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>
Comment 9 Brendan Lawlor 2024-11-18 15:37:59 UTC
Trivial change, works, makes sense, qa scripts happy. Passing QA.

Thanks Yanjun!
Comment 10 Katrin Fischer 2024-11-18 15:44:14 UTC
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!
Comment 11 Katrin Fischer 2024-12-23 10:11:40 UTC
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.
Comment 12 Yanjun Li 2024-12-26 23:32:16 UTC
Created attachment 175951 [details] [review]
Bug 38302: (follow-up) Add t() to make sure delete button is translatable
Comment 13 Katrin Fischer 2024-12-27 14:09:28 UTC
Treating the small change as a follow-up.
Comment 14 Katrin Fischer 2024-12-27 14:11:28 UTC
As the change is rather small I have squashed the patches before pushing. 
Thanks for your perseverance!
Comment 15 Katrin Fischer 2024-12-27 14:12:11 UTC
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>
Comment 16 Katrin Fischer 2024-12-27 14:12:38 UTC
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>
Comment 17 Katrin Fischer 2024-12-27 14:20:23 UTC
Pushed for 25.05!

Well done everyone, thank you!