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.
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!
Created attachment 159341 [details] [review] Bug 35322: Add unit tests
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>
Created attachment 159511 [details] [review] Bug 35322: Add unit tests Signed-off-by: David Nind <david@davidnind.com>
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>
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>
Created attachment 160894 [details] [review] Bug 35322: (QA follow-up) Perltidy Signed-off-by: Emily Lamancusa <emily.lamancusa@montgomerycountymd.gov>
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
*** Bug 30505 has been marked as a duplicate of this bug. ***
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.
Pushed for 24.05! Well done everyone, thank you!
Pushed to 23.11.x for 23.11.02
Backported to 23.05.x for upcoming 23.05.09.