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
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!