Bug 35322

Summary: AllowItemsOnHoldCheckoutSCO and AllowItemsOnHoldCheckoutSIP do not work
Product: Koha Reporter: Kyle M Hall (khall) <kyle>
Component: Hold requestsAssignee: Kyle M Hall (khall) <kyle>
Status: RESOLVED FIXED QA Contact: Emily Lamancusa (emlam) <emily.lamancusa>
Severity: major    
Priority: P5 - low CC: clackman, emily.lamancusa, fridolin.somers, gmcharlt, lucas, nick, wainuiwitikapark
Version: Main   
Hardware: All   
OS: All   
GIT URL: Change sponsored?: ---
Patch complexity: --- Documentation contact:
Documentation submission: Text to go in the release notes:
Version(s) released in:
24.05.00,23.11.02,23.05.09
Circulation function:
Attachments: Bug 35322: Fix the logic for checking for holds in CanBookBeIssued
Bug 35322: Add unit tests
Bug 35322: Fix the logic for checking for holds in CanBookBeIssued
Bug 35322: Add unit tests
Bug 35322: Fix the logic for checking for holds in CanBookBeIssued
Bug 35322: Add unit tests
Bug 35322: (QA follow-up) Perltidy

Description Kyle M Hall (khall) 2023-11-13 15:37:09 UTC
We are having reports that AllowItemsOnHoldCheckoutSCO and AllowItemsOnHoldCheckoutSIP no longer work. It appreas that in CanBookBeIssued, the ignore reserves check was changed from "check reserves unless the ignore reserves flag was passed" to "check reserves unless the ignore reserves flag was passed *and* we have a recall". I think this was a logic mistake and we want to check reserves unless we have an ignore flag *or* there is a recall.
Comment 1 Kyle M Hall (khall) 2023-11-13 15:45:44 UTC
Created attachment 158886 [details] [review]
Bug 35322: Fix the logic for checking for holds in CanBookBeIssued

We are having reports that AllowItemsOnHoldCheckoutSCO and AllowItemsOnHoldCheckoutSIP no longer work. It appreas that in CanBookBeIssued, the ignore reserves check was changed from "check reserves unless the ignore reserves flag was passed" to "check reserves unless the ignore reserves flag was passed *and* we have a recall". I think this was a logic mistake and we want to check reserves unless we have an ignore flag *or* there is a recall.

Test Plan:
1) Enable AllowItemsOnHoldCheckoutSCO
2) Place a hold on an item
3) Attempt to check that item out to another patron
4) Note the checkout is blocked
5) Apply this patch
6) Restart all the things!
7) Attempt the checkout again
8) The checkout now succeeds!
Comment 2 Kyle M Hall (khall) 2023-11-29 15:08:42 UTC
Created attachment 159341 [details] [review]
Bug 35322: Add unit tests
Comment 3 David Nind 2023-12-04 01:42:28 UTC
Created attachment 159510 [details] [review]
Bug 35322: Fix the logic for checking for holds in CanBookBeIssued

We are having reports that AllowItemsOnHoldCheckoutSCO and AllowItemsOnHoldCheckoutSIP no longer work. It appreas that in CanBookBeIssued, the ignore reserves check was changed from "check reserves unless the ignore reserves flag was passed" to "check reserves unless the ignore reserves flag was passed *and* we have a recall". I think this was a logic mistake and we want to check reserves unless we have an ignore flag *or* there is a recall.

Test Plan:
1) Enable AllowItemsOnHoldCheckoutSCO
2) Place a hold on an item
3) Attempt to check that item out to another patron
4) Note the checkout is blocked
5) Apply this patch
6) Restart all the things!
7) Attempt the checkout again
8) The checkout now succeeds!

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2023-12-04 01:42:31 UTC
Created attachment 159511 [details] [review]
Bug 35322: Add unit tests

Signed-off-by: David Nind <david@davidnind.com>
Comment 5 Emily Lamancusa (emlam) 2024-01-11 21:14:17 UTC
Created attachment 160892 [details] [review]
Bug 35322: Fix the logic for checking for holds in CanBookBeIssued

We are having reports that AllowItemsOnHoldCheckoutSCO and AllowItemsOnHoldCheckoutSIP no longer work. It appreas that in CanBookBeIssued, the ignore reserves check was changed from "check reserves unless the ignore reserves flag was passed" to "check reserves unless the ignore reserves flag was passed *and* we have a recall". I think this was a logic mistake and we want to check reserves unless we have an ignore flag *or* there is a recall.

Test Plan:
1) Enable AllowItemsOnHoldCheckoutSCO
2) Place a hold on an item
3) Attempt to check that item out to another patron
4) Note the checkout is blocked
5) Apply this patch
6) Restart all the things!
7) Attempt the checkout again
8) The checkout now succeeds!

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 6 Emily Lamancusa (emlam) 2024-01-11 21:14:19 UTC
Created attachment 160893 [details] [review]
Bug 35322: Add unit tests

Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 7 Emily Lamancusa (emlam) 2024-01-11 21:14:21 UTC
Created attachment 160894 [details] [review]
Bug 35322: (QA follow-up) Perltidy

Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
Comment 8 Emily Lamancusa (emlam) 2024-01-11 21:28:57 UTC
Looks good, works, and passes the QA script!

This also cleans up the messages in the staff interface - without the patch, if you try to check out an item that has both a recall and a hold from the staff interface, you get a confusing message that mixes the two confirmation questions, and offers the option to cancel checkout and fill the hold but not the option to fill the recall. With the patch, you get one confirmation message that warns of the recall and provides the expected options. Given that recalls are meant to take precedence over holds, it's my understanding that this would be the expected behavior.

Passing QA
Comment 9 Emily Lamancusa (emlam) 2024-01-11 22:07:10 UTC
*** Bug 30505 has been marked as a duplicate of this bug. ***
Comment 10 Emily Lamancusa (emlam) 2024-01-11 22:19:41 UTC
Incidentally, while testing different scenarios with this patch to check for regressions, I found bug 35780 - HoldsAutoFill allows both a hold and a recall to be filled simultaneously by the same item

I can't justify spending time on it myself since MCPL doesn't use recalls, but it should definitely be addressed.
Comment 11 Katrin Fischer 2024-01-16 11:09:16 UTC
Pushed for 24.05!

Well done everyone, thank you!
Comment 12 Fridolin Somers 2024-01-17 09:52:07 UTC
Pushed to 23.11.x for 23.11.02
Comment 13 Lucas Gass (lukeg) 2024-02-02 18:00:25 UTC
Backported to 23.05.x for upcoming 23.05.09.
Comment 14 Wainui Witika-Park 2025-06-23 05:27:12 UTC
Not backporting to 22.11 unless requested