Bug 35322 - AllowItemsOnHoldCheckoutSCO and AllowItemsOnHoldCheckoutSIP do not work
Summary: AllowItemsOnHoldCheckoutSCO and AllowItemsOnHoldCheckoutSIP do not work
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Hold requests (show other bugs)
Version: Main
Hardware: All All
: P5 - low major (vote)
Assignee: Kyle M Hall
QA Contact: Emily Lamancusa
URL:
Keywords:
: 30505 (view as bug list)
Depends on:
Blocks:
 
Reported: 2023-11-13 15:37 UTC by Kyle M Hall
Modified: 2024-04-26 20:30 UTC (History)
5 users (show)

See Also:
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


Attachments
Bug 35322: Fix the logic for checking for holds in CanBookBeIssued (1.44 KB, patch)
2023-11-13 15:45 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35322: Add unit tests (3.32 KB, patch)
2023-11-29 15:08 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 35322: Fix the logic for checking for holds in CanBookBeIssued (1.49 KB, patch)
2023-12-04 01:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 35322: Add unit tests (3.37 KB, patch)
2023-12-04 01:42 UTC, David Nind
Details | Diff | Splinter Review
Bug 35322: Fix the logic for checking for holds in CanBookBeIssued (1.56 KB, patch)
2024-01-11 21:14 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 35322: Add unit tests (3.44 KB, patch)
2024-01-11 21:14 UTC, Emily Lamancusa
Details | Diff | Splinter Review
Bug 35322: (QA follow-up) Perltidy (2.52 KB, patch)
2024-01-11 21:14 UTC, Emily Lamancusa
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 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 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 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 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 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 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 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 2024-01-11 22:07:10 UTC
*** Bug 30505 has been marked as a duplicate of this bug. ***
Comment 10 Emily Lamancusa 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 2024-02-02 18:00:25 UTC
Backported to 23.05.x for upcoming 23.05.09.