| Summary: | Delete-confirm should not start with 'cud-' | ||
|---|---|---|---|
| Product: | Koha | Reporter: | Emily Lamancusa (emlam) <emily.lamancusa> |
| Component: | Architecture, internals, and plumbing | Assignee: | Phil Ringnalda <phil> |
| Status: | RESOLVED FIXED | QA Contact: | Testopia <testopia> |
| Severity: | major | ||
| Priority: | P5 - low | CC: | caroline.cyr-la-rose, david, fridolin.somers, lucas, phil |
| Version: | Main | ||
| Hardware: | All | ||
| OS: | All | ||
| See Also: |
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37161 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37163 https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=37162 |
||
| 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: | |||
| Bug Depends on: | |||
| Bug Blocks: | 36192 | ||
| Attachments: |
Bug 37152: Can't delete an Acquisitions contract
Bug 37152: Deleting framework tags expects a cud-delete-confirm op instead of delete-confirm Bug 37152: Deleting patron categories doesn't warn about categories in use Bug 37152: Aquisitions basket and OPAC suggestion deletion should use the op cud-delete Bug 37152: Can't delete an Acquisitions contract Bug 37152: Deleting framework tags expects a cud-delete-confirm op instead of delete-confirm Bug 37152: Deleting patron categories doesn't warn about categories in use Bug 37152: Aquisitions basket and OPAC suggestion deletion should use the op cud-delete Bug 37152: Can't delete an Acquisitions contract Bug 37152: Deleting framework tags expects a cud-delete-confirm op instead of delete-confirm Bug 37152: Deleting patron categories doesn't warn about categories in use Bug 37152: Aquisitions basket and OPAC suggestion deletion should use the op cud-delete |
||
|
Description
Emily Lamancusa (emlam)
2024-06-21 13:50:23 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. 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. cud-delete. And major since you can't delete a contract. 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 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. 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 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 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> 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> 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> 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> 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. Filed that as bug 37161, since I don't see how the magic ever worked in the past, even though it did work. 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> 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> 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> 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> Pushed for 24.11! Well done everyone, thank you! Backported to 24.05.x for upcoming 24.05.02 Full CSRF not in 23.11.x This looks like a bug fix. Nothing to change/edit in the manual. |