Bug 38268 - Callers of confirmModal need to remove the modal as the first step in their callback function
Summary: Callers of confirmModal need to remove the modal as the first step in their c...
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Templates (show other bugs)
Version: Main
Hardware: All All
: P2 major
Assignee: Phil Ringnalda
QA Contact: Martin Renvoize (ashimema)
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2024-10-24 21:56 UTC by Phil Ringnalda
Modified: 2025-03-21 03:52 UTC (History)
7 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Trivial patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes confirm dialog boxes in the OPAC to prevent unintended actions being taken, such as accidentally deleting a list. This specifically fixes lists, and makes a technical change to prevent this happening in the future for other areas of the OPAC (such as suggestions, tags, and self-checkout). Example of issue fixed for lists: 1. Create a list with several items. 2. From the new list, select a couple of the items. 3. Click "Delete list" and then select "No, do not delete". 4. Then select "Remove from list", and confirm by clicking "Yes, remove from list". 5. Result: Instead of removing the items selected, the whole list was incorrectly deleted!
Version(s) released in:
25.05.00,24.11.02
Circulation function:


Attachments
Bug 38268: Callers of confirmModal need to remove the modal as the first step in their callback function (6.43 KB, patch)
2024-10-25 18:58 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 38268: Callers of confirmModal need to remove the modal as the first step in their callback function (6.48 KB, patch)
2024-11-27 04:48 UTC, Victor Grousset/tuxayo
Details | Diff | Splinter Review
Bug 38268: (Alternate patch) Prevent callbacks from being duplicated (7.74 KB, patch)
2024-12-09 11:00 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 38268: (Follow-ups) Prevent callbacks from being duplicated (7.73 KB, patch)
2024-12-09 13:58 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 38268: (Follow-up) Prevent callbacks from being duplicated (7.83 KB, patch)
2024-12-09 13:58 UTC, Matt Blenkinsop
Details | Diff | Splinter Review
Bug 38268: Callers of confirmModal need to remove the modal as the first step in their callback function (6.52 KB, patch)
2024-12-09 17:15 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38268: (Follow-up) Prevent callbacks from being duplicated (7.85 KB, patch)
2024-12-09 17:15 UTC, Owen Leonard
Details | Diff | Splinter Review
Bug 38268: Callers of confirmModal need to remove the modal as the first step in their callback function (6.60 KB, patch)
2024-12-16 18:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 38268: (Follow-up) Prevent callbacks from being duplicated (7.93 KB, patch)
2024-12-16 18:56 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Phil Ringnalda 2024-10-24 21:56:50 UTC
Steps to reproduce:

1. Add several items to a list
2. Click the checkbox for an item, then accidentally click 'Delete list' rather than 'Delete selected items'
3. Click the 'No, do not delete' button in the confirmation asking if you want to delete the list
4. Click 'Delete selected items' like you meant to
5. Click 'Yes, remove from list' in the confirmation which lists the two items you selected

Expected result: the two selected items are removed from the list, leaving the rest and the list intact

Actual result: the entire list is deleted
Comment 1 Phil Ringnalda 2024-10-24 22:54:14 UTC
Missing part of step 1: "in the OPAC", since the staff interface uses a completely different approach.
Comment 2 Phil Ringnalda 2024-10-25 16:29:39 UTC
Le sigh.

I was starting to suspect that just hiding the modal was the problem, though why is beyond my pay-grade.

Then I went looking for other places that should be affected (by having more than one caller of confirmModal), and saw that opac_user.tt should confuse cancel and suspend holds and cancel article requests. But it doesn't, because in bug 27947 three years ago Agustín apparently realized this was a problem and how to fix it, silently fixed it in opac_user.tt by making the first step in every callback be '$("#bootstrap-confirm-box-modal").remove()', and nobody reviewing the fix noticed it and considered other places that might need the same fix.
Comment 3 Phil Ringnalda 2024-10-25 18:58:45 UTC
Created attachment 173410 [details] [review]
Bug 38268: Callers of confirmModal need to remove the modal as the first step in their callback function

Templates that use confirmModal to show a pretty confirm() need to remove the
modal in their callback function, or they are at risk of having the first
callback called for subsequent calls that think they will get their own
callback called. opac-user.tt is already fixed, opac-shelves.tt is currently
affected, and opace-suggestions.tt, opac-tags.tt, and sco/sco-main.tt are at
risk of being affected if anyone adds another confirmModal caller.

Test plan:
 1. Without the patch, open the OPAC, log in, search for something that will
    return several results
 2. Select all - Add to list - New list
 3. Lists - {your list name}
 4. Click the checkboxes for the first two items
 5. Instead of clicking Remove from list, accidentally click Delete list
 6. In the confirmation popup, realize it was a mistake and click No, do not
    delete
 7. Click Remove from list
 8. Poof, your whole list is gone
 9. Apply patch
10. Repeat steps 1-7, but instead of the whole list being deleted, see that
    only the selected two items were removed
11. Now intentionally click Delete list and click Yes, delete, you'll see that
    it is deleted rather than getting a message about nothing being removed
    like you would have gotten without the patch if you started by removing
    an item
12. On any bib detail page, click Add tag(s) and add one
13. Your account - Tags, check the checkbox for the tag, Remove selected tags,
    Yes, delete tag and verify that it was deleted
14. Purchase suggestions - New purchase suggestion, fill in a title and submit
    your suggestion, click the checkbox for your suggestion, Delete selected,
    Yes, delete suggestion and verify it was deleted
15. In the staff interface, find and copy the barcode for any item that can
    be checked out, then go to /cgi-bin/koha/sco/sco-main.pl with the OPAC
    hostname and port, not the staff, and log in
16. Paste the barcode in the input, click Submit, then click Finish, and in
    the confirmation popup click Print recept and end session, and verify that
    it tries to print

Sponsored-by: Chetco Community Public Library
Comment 4 Phil Ringnalda 2024-11-13 03:13:32 UTC
Huh, I called it "normal" that if you cancel one operation and perform another, the first (destructive enough to require confirmation) operation will be done without any warning. My standards for severity are slipping.
Comment 5 Victor Grousset/tuxayo 2024-11-27 04:48:19 UTC
Created attachment 174995 [details] [review]
Bug 38268: Callers of confirmModal need to remove the modal as the first step in their callback function

Templates that use confirmModal to show a pretty confirm() need to remove the
modal in their callback function, or they are at risk of having the first
callback called for subsequent calls that think they will get their own
callback called. opac-user.tt is already fixed, opac-shelves.tt is currently
affected, and opace-suggestions.tt, opac-tags.tt, and sco/sco-main.tt are at
risk of being affected if anyone adds another confirmModal caller.

Test plan:
 1. Without the patch, open the OPAC, log in, search for something that will
    return several results
 2. Select all - Add to list - New list
 3. Lists - {your list name}
 4. Click the checkboxes for the first two items
 5. Instead of clicking Remove from list, accidentally click Delete list
 6. In the confirmation popup, realize it was a mistake and click No, do not
    delete
 7. Click Remove from list
 8. Poof, your whole list is gone
 9. Apply patch
10. Repeat steps 1-7, but instead of the whole list being deleted, see that
    only the selected two items were removed
11. Now intentionally click Delete list and click Yes, delete, you'll see that
    it is deleted rather than getting a message about nothing being removed
    like you would have gotten without the patch if you started by removing
    an item
12. On any bib detail page, click Add tag(s) and add one
13. Your account - Tags, check the checkbox for the tag, Remove selected tags,
    Yes, delete tag and verify that it was deleted
14. Purchase suggestions - New purchase suggestion, fill in a title and submit
    your suggestion, click the checkbox for your suggestion, Delete selected,
    Yes, delete suggestion and verify it was deleted
15. In the staff interface, find and copy the barcode for any item that can
    be checked out, then go to /cgi-bin/koha/sco/sco-main.pl with the OPAC
    hostname and port, not the staff, and log in
16. Paste the barcode in the input, click Submit, then click Finish, and in
    the confirmation popup click Print recept and end session, and verify that
    it tries to print

Sponsored-by: Chetco Community Public Library
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Comment 6 Victor Grousset/tuxayo 2024-11-27 04:48:41 UTC
It works! :)

> 7. Click Remove from list
> 8. Poof, your whole list is gone

OMG that's vicious, thanks for finding it out and the fix!
Comment 7 Matt Blenkinsop 2024-12-09 10:59:46 UTC
Started having a look at this for QA and everything works as advertised but I'm hesitant to pass QA. This fixes the existing cases but I worry about what happens the next time someone who isn't aware of this bug adds a call to confirmModal - does it not make more sense to move the removal of the existing modal up a level into the confirmModal function itself so that it is handled each time without the need to implicitly handle it in each individual callback?

I'm adding an alternate patch for testing that does this - let me know your thoughts and if the behaviour works as expected
Comment 8 Matt Blenkinsop 2024-12-09 11:00:34 UTC
Created attachment 175294 [details] [review]
Bug 38268: (Alternate patch)  Prevent callbacks from being duplicated
Comment 9 Owen Leonard 2024-12-09 12:34:13 UTC
(In reply to Matt Blenkinsop from comment #7)
> I'm adding an alternate patch for testing that does this - let me know your
> thoughts and if the behaviour works as expected

The alternate makes sense to me, and thanks to both of you for cleaning up my mess.

Matt could you either replace your patch with one which doesn't depend on the first one, or update your patch so that it says it's a follow-up?

To me "Alternate" means the other patch isn't needed. But I like the option that gives Phil credit for providing a fix.
Comment 10 Matt Blenkinsop 2024-12-09 13:58:00 UTC
Created attachment 175310 [details] [review]
Bug 38268: (Follow-ups) Prevent callbacks from being duplicated
Comment 11 Matt Blenkinsop 2024-12-09 13:58:55 UTC
Created attachment 175311 [details] [review]
Bug 38268: (Follow-up) Prevent callbacks from being duplicated

This patch moves the logic up one level into the confirmModal function to prevent future regressions
Comment 12 Matt Blenkinsop 2024-12-09 14:00:15 UTC
Added the patch as a follow-up and set it back to NSO, Owen would you mind doing the SO?

I've removed myself from QA as I've authored one of the patches, I'll pass it back into the QA pool
Comment 13 Owen Leonard 2024-12-09 17:15:49 UTC
Created attachment 175325 [details] [review]
Bug 38268: Callers of confirmModal need to remove the modal as the first step in their callback function

Templates that use confirmModal to show a pretty confirm() need to
remove the modal in their callback function, or they are at risk of
having the first callback called for subsequent calls that think they
will get their own callback called. opac-user.tt is already fixed,
opac-shelves.tt is currently affected, and opace-suggestions.tt,
opac-tags.tt, and sco/sco-main.tt are at risk of being affected if
anyone adds another confirmModal caller.

Test plan:
 1. Without the patch, open the OPAC, log in, search for something that
    will return several results
 2. Select all - Add to list - New list
 3. Lists - {your list name}
 4. Click the checkboxes for the first two items
 5. Instead of clicking Remove from list, accidentally click Delete list
 6. In the confirmation popup, realize it was a mistake and click No, do
    not delete
 7. Click Remove from list
 8. Poof, your whole list is gone
 9. Apply patch
10. Repeat steps 1-7, but instead of the whole list being deleted, see
    that only the selected two items were removed
11. Now intentionally click Delete list and click Yes, delete, you'll
    see that it is deleted rather than getting a message about nothing
    being removed like you would have gotten without the patch if you
    started by removing an item
12. On any bib detail page, click Add tag(s) and add one
13. Your account - Tags, check the checkbox for the tag, Remove selected
    tags, Yes, delete tag and verify that it was deleted
14. Purchase suggestions - New purchase suggestion, fill in a title and
    submit your suggestion, click the checkbox for your suggestion,
    Delete selected, Yes, delete suggestion and verify it was deleted
15. In the staff interface, find and copy the barcode for any item that can
    be checked out, then go to /cgi-bin/koha/sco/sco-main.pl with the
    OPAC hostname and port, not the staff, and log in
16. Paste the barcode in the input, click Submit, then click Finish, and
    in the confirmation popup click Print recept and end session, and
    verify that it tries to print

Sponsored-by: Chetco Community Public Library
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 14 Owen Leonard 2024-12-09 17:15:52 UTC
Created attachment 175326 [details] [review]
Bug 38268: (Follow-up) Prevent callbacks from being duplicated

This patch moves the logic up one level into the confirmModal function
to prevent future regressions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Comment 15 Martin Renvoize (ashimema) 2024-12-16 18:56:12 UTC
Created attachment 175565 [details] [review]
Bug 38268: Callers of confirmModal need to remove the modal as the first step in their callback function

Templates that use confirmModal to show a pretty confirm() need to
remove the modal in their callback function, or they are at risk of
having the first callback called for subsequent calls that think they
will get their own callback called. opac-user.tt is already fixed,
opac-shelves.tt is currently affected, and opace-suggestions.tt,
opac-tags.tt, and sco/sco-main.tt are at risk of being affected if
anyone adds another confirmModal caller.

Test plan:
 1. Without the patch, open the OPAC, log in, search for something that
    will return several results
 2. Select all - Add to list - New list
 3. Lists - {your list name}
 4. Click the checkboxes for the first two items
 5. Instead of clicking Remove from list, accidentally click Delete list
 6. In the confirmation popup, realize it was a mistake and click No, do
    not delete
 7. Click Remove from list
 8. Poof, your whole list is gone
 9. Apply patch
10. Repeat steps 1-7, but instead of the whole list being deleted, see
    that only the selected two items were removed
11. Now intentionally click Delete list and click Yes, delete, you'll
    see that it is deleted rather than getting a message about nothing
    being removed like you would have gotten without the patch if you
    started by removing an item
12. On any bib detail page, click Add tag(s) and add one
13. Your account - Tags, check the checkbox for the tag, Remove selected
    tags, Yes, delete tag and verify that it was deleted
14. Purchase suggestions - New purchase suggestion, fill in a title and
    submit your suggestion, click the checkbox for your suggestion,
    Delete selected, Yes, delete suggestion and verify it was deleted
15. In the staff interface, find and copy the barcode for any item that can
    be checked out, then go to /cgi-bin/koha/sco/sco-main.pl with the
    OPAC hostname and port, not the staff, and log in
16. Paste the barcode in the input, click Submit, then click Finish, and
    in the confirmation popup click Print recept and end session, and
    verify that it tries to print

Sponsored-by: Chetco Community Public Library
Signed-off-by: Victor Grousset/tuxayo <victor@tuxayo.net>
Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 16 Martin Renvoize (ashimema) 2024-12-16 18:56:15 UTC
Created attachment 175566 [details] [review]
Bug 38268: (Follow-up) Prevent callbacks from being duplicated

This patch moves the logic up one level into the confirmModal function
to prevent future regressions

Signed-off-by: Owen Leonard <oleonard@myacpl.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 17 Martin Renvoize (ashimema) 2024-12-16 18:56:36 UTC
Nice cleanup and good golfing.
Comment 18 Katrin Fischer 2024-12-23 08:38:05 UTC
Pushed for 25.05!

Well done everyone, thank you!
Comment 19 Paul Derscheid 2025-02-03 19:09:07 UTC
Nice work everyone!

Pushed to 24.11.x for 24.11.01
Comment 20 Alex Buckley 2025-03-19 01:59:40 UTC
Hi all, 

I have been testing these two patches for 24.05.x backport.

When I have the first patch applied on 24.05.x all of the patch 1 test plan steps work exactly as noted.

However, when I apply the second patch and run through the patch 1 test plan steps again I have found that step 10 fails.

If I only remove two items from my list (which has more than 2 items in it) the whole list is deleted.

Could someone else please apply both patches on 24.05.x and confirm if this happens for you as well?

Thanks in advance,
Alex
Comment 21 Phil Ringnalda 2025-03-21 00:57:12 UTC
I applied both patches on 24.05.x, and the test plan worked for me, only two items were deleted from the list.
Comment 22 Alex Buckley 2025-03-21 03:52:58 UTC
(In reply to Phil Ringnalda from comment #21)
> I applied both patches on 24.05.x, and the test plan worked for me, only two
> items were deleted from the list.

Thanks Phil.

That's strange we are seeing different behaviour. We have made a note to revisit this bug report next month, when we have more time to see if we can get it backported to 24.05.09