Bug 37152 - Delete-confirm should not start with 'cud-'
Summary: Delete-confirm should not start with 'cud-'
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Phil Ringnalda
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 36192
  Show dependency treegraph
 
Reported: 2024-06-21 13:50 UTC by Emily Lamancusa (emlam)
Modified: 2024-09-10 20:20 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
This fixes the delete actions in these areas so that they now work as they should: - Acquisitions: . Contracts - couldn't delete a contract (no confirmation message, didn't delete) (Acquisitions > search for a vendor > Contracts > Delete > Yes, delete contract) . Baskets - could still delete a basket (so no change in behavour), however it wasn't using the correct code to do this (Acquisitions > search for a vendor > select a basket > Delete basket) - MARC bibliographic frameworks and authority types: couldn't delete tags - the confirmation message didn't have the tag description, and didn't delete (there was no error message, it just didn't delete the tag) (Administration > Catalog > MARC bibliographic framework OR Authority types > Actions > MARC structure > [choose a tag] > Actions > Delete) - Patron categories: when attempting to delete a patron category that was still in use - it generated an error message, instead of a warning that it was still in use (and didn't delete the category) (Administration > Patron categories > Delete > [Warning that can't delete if in use OR Confirm deletion]) - Purchase suggestions in a patron's OPAC account: could still delete a purchase suggestion (so no change in behavour), however it wasn't using the correct code to do this (OPAC > Your account > Purchase suggestions > select suggestoon > Delete selected) (These fixes are related to the CSRF changes added in Koha 24.05 to improve form security.)
Version(s) released in:
24.11.00,24.05.02
Circulation function:


Attachments
Bug 37152: Can't delete an Acquisitions contract (1.53 KB, patch)
2024-06-21 19:34 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37152: Deleting framework tags expects a cud-delete-confirm op instead of delete-confirm (2.93 KB, patch)
2024-06-21 19:34 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37152: Deleting patron categories doesn't warn about categories in use (1.46 KB, patch)
2024-06-21 19:35 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37152: Aquisitions basket and OPAC suggestion deletion should use the op cud-delete (6.40 KB, patch)
2024-06-21 19:35 UTC, Phil Ringnalda
Details | Diff | Splinter Review
Bug 37152: Can't delete an Acquisitions contract (1.60 KB, patch)
2024-06-21 20:25 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 37152: Deleting framework tags expects a cud-delete-confirm op instead of delete-confirm (3.00 KB, patch)
2024-06-21 20:26 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 37152: Deleting patron categories doesn't warn about categories in use (1.53 KB, patch)
2024-06-21 20:26 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 37152: Aquisitions basket and OPAC suggestion deletion should use the op cud-delete (6.47 KB, patch)
2024-06-21 20:26 UTC, Emily Lamancusa (emlam)
Details | Diff | Splinter Review
Bug 37152: Can't delete an Acquisitions contract (1.66 KB, patch)
2024-06-25 22:26 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37152: Deleting framework tags expects a cud-delete-confirm op instead of delete-confirm (3.06 KB, patch)
2024-06-25 22:26 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37152: Deleting patron categories doesn't warn about categories in use (1.59 KB, patch)
2024-06-25 22:26 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 37152: Aquisitions basket and OPAC suggestion deletion should use the op cud-delete (6.54 KB, patch)
2024-06-25 22:26 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Emily Lamancusa (emlam) 2024-06-21 13:50:23 UTC
There are a number of pages that use the op delete_confirm to display a confirmation page before deleting an object, followed by the op cud-delete_confirm*ed* to actually perform the delete operation. A few pages slipped through that are expecting an op of cud-delete_confirm, which should not start with 'cud-':

  - acqui/basket.pl and basket.tt
  - opac/opac-suggestions.pl and opac-suggestions.tt
  - admin/auth-tag-structure.pl
  - admin/categories.pl
  - admin/aqcontract.pl
  - admin/marctagstructure.pl

None of these broke functionality for me when I tested them - the first two because at least the .pl and .tt files align, and I'm not sure why the last four didn't break. Either way, they should still be brought in line with the coding guideline.
Comment 1 Phil Ringnalda 2024-06-21 17:06:37 UTC
aqcontract.pl doesn't seem to actually work, but the other three "work" because nothing done in the elsif ( $op eq 'cud-delete_confirm' ) is actually required.

Deleting a tag, you just get a confirm page with "Tag: nnn" instead of "Tag: nnn liblibrarian" which isn't really noticeable, and if you delete an unused patron category it deletes, but if you delete a used patron category it tells you something went wrong and you should look at the logs to see why, because you were supposed to get a delete_confirm page telling you that you can't because it's used by 783 patrons.
Comment 2 Phil Ringnalda 2024-06-21 17:45:30 UTC
And for acqui/basket and opac/opac-suggestions the bug is that they are using cud-delete_confim when they mean cud-delete_confirmed because they already did the confirmation in a javascript modal. Or, depending on how you look at it, they mean cud-delete.
Comment 3 Phil Ringnalda 2024-06-21 18:22:05 UTC
cud-delete.

And major since you can't delete a contract.
Comment 4 Phil Ringnalda 2024-06-21 19:34:57 UTC
Created attachment 167987 [details] [review]
Bug 37152: Can't delete an Acquisitions contract

admin/aqcontract.pl expects the op cud-delete-confirm, but what's sent is
the op delete-confirm because it's just a GET of the page that will actually
do a cud op once it is confirmed.

Test plan:
1. Without the patch, Acquisitions - Vendor search for My Vendor
2. Left Sidebar menu - Contracts - Add a contract
3. Fill in Name, Start date, End date, Save
4. On the line for the contract, click Delete
5. Note that the page that loads has no button for "Yes, delete contract"
   (or for anything else)
6. Apply patch, restart_all
7. Acquisitions - Vendor search for My Vendor
8. Left Sidebar menu, Contracts, click Delete, then Yes, delete contract
9. Vendor search for My Vendor, Left Sidebar Contracts, verify the contract
   is gone
Comment 5 Phil Ringnalda 2024-06-21 19:34:59 UTC
Created attachment 167988 [details] [review]
Bug 37152: Deleting framework tags expects a cud-delete-confirm op instead of delete-confirm

The scripts for managing biblio and authority framework tag editing are
expecting an op of cud-delete-confirm, but the op that is passed is the
correct thing, delete-confirm, because it's a GET of a page where the
confirmation will happen, not the later cud- operation. You can only see
that the expected op is wrong because the confirmation page doesn't get
the description of the tag.

Test plan:
 1. Without the patch, Administration - MARC bibliographic framework
 2. Default framework Actions menu, MARC structure
 3. In the row for 025, Actions menu, Delete
 4. Note that the page which loads shows "Tag: 025" and "Description:"
    without actually having a description
 5. Administration - Authority types
 6. Default authority type Actions menu, MARC structure
 7. In the row for 046, Actions menu, Delete
 8. Note that the page that loads shows "Tag: 046" without the tag description
 9. Apply patch, restart_all
10. With the patch, repeat steps 1-8, but this time note that you see the
    description of the tag, not just the number. For both, click Yes, delete,
    and verify that the tag is deleted.
Comment 6 Phil Ringnalda 2024-06-21 19:35:01 UTC
Created attachment 167989 [details] [review]
Bug 37152: Deleting patron categories doesn't warn about categories in use

You can't delete a patron category when it is in use, but because the
script which handles delete confirmation is expecting the wrong op,
cud-delete_confirm rather than delete_confirm, you don't get any warning,
just a failure to delete error message.

Test plan:
 1. Without the patch, Administration - Patron categories
 2. On the row for Staff, click Delete, Delete this category
 3. Note an error message telling you to check the logs for details
 4. Apply patch, restart_all
 5. With the patch, repeat steps 1-2, note that instead of a button for
    Delete this category you get a warning that it is in use
 6. In the list of categories, row for Board, click Delete, Delete this category
 7. Note that the (unused) Board category was deleted
Comment 7 Phil Ringnalda 2024-06-21 19:35:04 UTC
Created attachment 167990 [details] [review]
Bug 37152: Aquisitions basket and OPAC suggestion deletion should use the op cud-delete

Both deleting a basket in Aquisitions and deleting a suggestion in the OPAC
take care of the confirmation in a javascript modal, rather than having a
whole separate page for confirmation, so they should be using the op
cud-delete rather than either cud-delete_confirm (which shouldn't ever be
used) or delete_confirm (which they aren't doing, they already confirmed).

Test plan:
1. There's no wrong behavior to see, so apply patch and restart_all
2. Aquisitions - Vendor search for My Vendor - Click My Basket
3. Click Delete basket, in the popup again Delete basket
4. Click Show baskets for vendor My Vendor and verify the basket is gone
5. OPAC - Your account - Purchase suggestions
6. Create a suggestion, then click the checkbox for it, Delete selected,
   confirm
7. Verify the suggestion was deleted
Comment 8 Emily Lamancusa (emlam) 2024-06-21 20:25:59 UTC
Created attachment 167993 [details] [review]
Bug 37152: Can't delete an Acquisitions contract

admin/aqcontract.pl expects the op cud-delete-confirm, but what's sent is
the op delete-confirm because it's just a GET of the page that will actually
do a cud op once it is confirmed.

Test plan:
1. Without the patch, Acquisitions - Vendor search for My Vendor
2. Left Sidebar menu - Contracts - Add a contract
3. Fill in Name, Start date, End date, Save
4. On the line for the contract, click Delete
5. Note that the page that loads has no button for "Yes, delete contract"
   (or for anything else)
6. Apply patch, restart_all
7. Acquisitions - Vendor search for My Vendor
8. Left Sidebar menu, Contracts, click Delete, then Yes, delete contract
9. Vendor search for My Vendor, Left Sidebar Contracts, verify the contract
   is gone

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 9 Emily Lamancusa (emlam) 2024-06-21 20:26:01 UTC
Created attachment 167994 [details] [review]
Bug 37152: Deleting framework tags expects a cud-delete-confirm op instead of delete-confirm

The scripts for managing biblio and authority framework tag editing are
expecting an op of cud-delete-confirm, but the op that is passed is the
correct thing, delete-confirm, because it's a GET of a page where the
confirmation will happen, not the later cud- operation. You can only see
that the expected op is wrong because the confirmation page doesn't get
the description of the tag.

Test plan:
 1. Without the patch, Administration - MARC bibliographic framework
 2. Default framework Actions menu, MARC structure
 3. In the row for 025, Actions menu, Delete
 4. Note that the page which loads shows "Tag: 025" and "Description:"
    without actually having a description
 5. Administration - Authority types
 6. Default authority type Actions menu, MARC structure
 7. In the row for 046, Actions menu, Delete
 8. Note that the page that loads shows "Tag: 046" without the tag description
 9. Apply patch, restart_all
10. With the patch, repeat steps 1-8, but this time note that you see the
    description of the tag, not just the number. For both, click Yes, delete,
    and verify that the tag is deleted.

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 10 Emily Lamancusa (emlam) 2024-06-21 20:26:04 UTC
Created attachment 167995 [details] [review]
Bug 37152: Deleting patron categories doesn't warn about categories in use

You can't delete a patron category when it is in use, but because the
script which handles delete confirmation is expecting the wrong op,
cud-delete_confirm rather than delete_confirm, you don't get any warning,
just a failure to delete error message.

Test plan:
 1. Without the patch, Administration - Patron categories
 2. On the row for Staff, click Delete, Delete this category
 3. Note an error message telling you to check the logs for details
 4. Apply patch, restart_all
 5. With the patch, repeat steps 1-2, note that instead of a button for
    Delete this category you get a warning that it is in use
 6. In the list of categories, row for Board, click Delete, Delete this category
 7. Note that the (unused) Board category was deleted

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 11 Emily Lamancusa (emlam) 2024-06-21 20:26:05 UTC
Created attachment 167996 [details] [review]
Bug 37152: Aquisitions basket and OPAC suggestion deletion should use the op cud-delete

Both deleting a basket in Aquisitions and deleting a suggestion in the OPAC
take care of the confirmation in a javascript modal, rather than having a
whole separate page for confirmation, so they should be using the op
cud-delete rather than either cud-delete_confirm (which shouldn't ever be
used) or delete_confirm (which they aren't doing, they already confirmed).

Test plan:
1. There's no wrong behavior to see, so apply patch and restart_all
2. Aquisitions - Vendor search for My Vendor - Click My Basket
3. Click Delete basket, in the popup again Delete basket
4. Click Show baskets for vendor My Vendor and verify the basket is gone
5. OPAC - Your account - Purchase suggestions
6. Create a suggestion, then click the checkbox for it, Delete selected,
   confirm
7. Verify the suggestion was deleted

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 12 Emily Lamancusa (emlam) 2024-06-21 20:26:43 UTC
Thanks, Phil! (for testing it more thoroughly than I did, the quick patchwriting, and the excellent commit messages :)  )

Testing notes:
After deleting a tag, marctagstructure.pl doesn't reload the framework properly - it has a heading showing the framework name, but nothing else. I think that's a separate bug, though.
Comment 13 Phil Ringnalda 2024-06-21 21:26:41 UTC
Filed that as bug 37161, since I don't see how the magic ever worked in the past, even though it did work.
Comment 14 Nick Clemens (kidclamp) 2024-06-25 22:26:40 UTC
Created attachment 168104 [details] [review]
Bug 37152: Can't delete an Acquisitions contract

admin/aqcontract.pl expects the op cud-delete-confirm, but what's sent is
the op delete-confirm because it's just a GET of the page that will actually
do a cud op once it is confirmed.

Test plan:
1. Without the patch, Acquisitions - Vendor search for My Vendor
2. Left Sidebar menu - Contracts - Add a contract
3. Fill in Name, Start date, End date, Save
4. On the line for the contract, click Delete
5. Note that the page that loads has no button for "Yes, delete contract"
   (or for anything else)
6. Apply patch, restart_all
7. Acquisitions - Vendor search for My Vendor
8. Left Sidebar menu, Contracts, click Delete, then Yes, delete contract
9. Vendor search for My Vendor, Left Sidebar Contracts, verify the contract
   is gone

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens (kidclamp) 2024-06-25 22:26:42 UTC
Created attachment 168105 [details] [review]
Bug 37152: Deleting framework tags expects a cud-delete-confirm op instead of delete-confirm

The scripts for managing biblio and authority framework tag editing are
expecting an op of cud-delete-confirm, but the op that is passed is the
correct thing, delete-confirm, because it's a GET of a page where the
confirmation will happen, not the later cud- operation. You can only see
that the expected op is wrong because the confirmation page doesn't get
the description of the tag.

Test plan:
 1. Without the patch, Administration - MARC bibliographic framework
 2. Default framework Actions menu, MARC structure
 3. In the row for 025, Actions menu, Delete
 4. Note that the page which loads shows "Tag: 025" and "Description:"
    without actually having a description
 5. Administration - Authority types
 6. Default authority type Actions menu, MARC structure
 7. In the row for 046, Actions menu, Delete
 8. Note that the page that loads shows "Tag: 046" without the tag description
 9. Apply patch, restart_all
10. With the patch, repeat steps 1-8, but this time note that you see the
    description of the tag, not just the number. For both, click Yes, delete,
    and verify that the tag is deleted.

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Nick Clemens (kidclamp) 2024-06-25 22:26:44 UTC
Created attachment 168106 [details] [review]
Bug 37152: Deleting patron categories doesn't warn about categories in use

You can't delete a patron category when it is in use, but because the
script which handles delete confirmation is expecting the wrong op,
cud-delete_confirm rather than delete_confirm, you don't get any warning,
just a failure to delete error message.

Test plan:
 1. Without the patch, Administration - Patron categories
 2. On the row for Staff, click Delete, Delete this category
 3. Note an error message telling you to check the logs for details
 4. Apply patch, restart_all
 5. With the patch, repeat steps 1-2, note that instead of a button for
    Delete this category you get a warning that it is in use
 6. In the list of categories, row for Board, click Delete, Delete this category
 7. Note that the (unused) Board category was deleted

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 17 Nick Clemens (kidclamp) 2024-06-25 22:26:46 UTC
Created attachment 168107 [details] [review]
Bug 37152: Aquisitions basket and OPAC suggestion deletion should use the op cud-delete

Both deleting a basket in Aquisitions and deleting a suggestion in the OPAC
take care of the confirmation in a javascript modal, rather than having a
whole separate page for confirmation, so they should be using the op
cud-delete rather than either cud-delete_confirm (which shouldn't ever be
used) or delete_confirm (which they aren't doing, they already confirmed).

Test plan:
1. There's no wrong behavior to see, so apply patch and restart_all
2. Aquisitions - Vendor search for My Vendor - Click My Basket
3. Click Delete basket, in the popup again Delete basket
4. Click Show baskets for vendor My Vendor and verify the basket is gone
5. OPAC - Your account - Purchase suggestions
6. Create a suggestion, then click the checkbox for it, Delete selected,
   confirm
7. Verify the suggestion was deleted

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 18 Katrin Fischer 2024-06-27 09:52:54 UTC
Pushed for 24.11!

Well done everyone, thank you!
Comment 19 Lucas Gass (lukeg) 2024-07-18 19:53:43 UTC
Backported to 24.05.x for upcoming 24.05.02
Comment 20 Fridolin Somers 2024-07-19 09:24:47 UTC
Full CSRF not in 23.11.x
Comment 21 Caroline Cyr La Rose 2024-09-10 20:20:26 UTC
This looks like a bug fix. Nothing to change/edit in the manual.