Bug 37152 - Delete-confirm should not start with 'cud-'
Summary: Delete-confirm should not start with 'cud-'
Status: Pushed to main
Alias: None
Product: Koha
Classification: Unclassified
Component: Architecture, internals, and plumbing (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
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
Modified: 2024-06-27 09:52 UTC (History)
1 user (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
24.11.00


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