If AllowRenewalIfOtherItemsAvailable is set to Allow we are incorrectly (against the syspref description) allowing to renew some checkouts when the holds are coming from a single patron. When there is only 1 available item for checkout in the biblio and a single patron has made 2 biblio level holds we allow the patron who has a single item on loan to renew their item and disregard the fact the we are leaving one hold unfilled. According to the AllowRenewalIfOtherItemsAvailable syspref the renewal should only be possible when other items can fill all the unfilled holds. I have made a patch for this and will submit it shortly.
Created attachment 137826 [details] [review] Bug 31112: Remove unnecessary if-clause To test: 1) Please check manually that the logic stays the same, use git's -w command line parameter to ignore whitespace changes in the diff output. 2) prove t/db_dependent/Circulation.t
Created attachment 137827 [details] [review] Bug 31112: Remove unit test that forces specific internal implementation To test: 1) Make sure tests still pass: $ prove t/db_dependent/Circulation.t
Created attachment 137828 [details] [review] Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio If a single patron had more than 1 hold to a biblio and there was only one available item we allowed incorrectly renewing the checkout when AllowRenewalIfOtherItemsAvailable was set to "Allow". This changes CanBookBeRenewed so that it makes sure all the holds are filled and not just one per patron. To test: 1) prove t/db_dependent/Circulation.t 2) (Optional, as unit test is provided) - Set AllowRenewalIfOtherItemsAvailable = Allow - Create biblio with three items - Checkout one item to patron A - Add two biblio-level holds for patron B - Try to renew patron A's checkout with and without this patch. - Notice that without this patch the renewal succeeds even though we one unfilled hold left. After applying the patch the renewal should fail.
Created attachment 137856 [details] [review] Bug 31112: Remove unnecessary if-clause To test: 1) Please check manually that the logic stays the same, use git's -w command line parameter to ignore whitespace changes in the diff output. 2) prove t/db_dependent/Circulation.t Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Created attachment 137857 [details] [review] Bug 31112: Remove unit test that forces specific internal implementation To test: 1) Make sure tests still pass: $ prove t/db_dependent/Circulation.t Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Created attachment 137858 [details] [review] Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio If a single patron had more than 1 hold to a biblio and there was only one available item we allowed incorrectly renewing the checkout when AllowRenewalIfOtherItemsAvailable was set to "Allow". This changes CanBookBeRenewed so that it makes sure all the holds are filled and not just one per patron. To test: 1) prove t/db_dependent/Circulation.t 2) (Optional, as unit test is provided) - Set AllowRenewalIfOtherItemsAvailable = Allow - Create biblio with three items - Checkout one item to patron A - Add two biblio-level holds for patron B - Try to renew patron A's checkout with and without this patch. - Notice that without this patch the renewal succeeds even though we one unfilled hold left. After applying the patch the renewal should fail. Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk>
Works as described, signed off. Thanks for this, good improvement. There's a small typo in the test plan - either 2.2. should be: 'Create biblio with two items' or 2.3/2.4 should be: 'Checkout out one item to patron A, one item to patron B, and add two biblio-level holds for patron C'. To test: 1) prove t/db_dependent/Circulation.t 2) (Optional, as unit test is provided) - Set AllowRenewalIfOtherItemsAvailable = Allow - Create biblio with three items - Checkout one item to patron A - Add two biblio-level holds for patron B - Try to renew patron A's checkout with and without this patch. - Notice that without this patch the renewal succeeds even though we one unfilled hold left. After applying the patch the renewal should fail.
Created attachment 139492 [details] [review] Bug 31112: Remove unnecessary if-clause To test: 1) Please check manually that the logic stays the same, use git's -w command line parameter to ignore whitespace changes in the diff output. 2) prove t/db_dependent/Circulation.t Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 139493 [details] [review] Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio If a single patron had more than 1 hold to a biblio and there was only one available item we allowed incorrectly renewing the checkout when AllowRenewalIfOtherItemsAvailable was set to "Allow". This changes CanBookBeRenewed so that it makes sure all the holds are filled and not just one per patron. To test: 1) prove t/db_dependent/Circulation.t 2) (Optional, as unit test is provided) - Set AllowRenewalIfOtherItemsAvailable = Allow - Create biblio with three items - Checkout one item to patron A - Add two biblio-level holds for patron B - Try to renew patron A's checkout with and without this patch. - Notice that without this patch the renewal succeeds even though we one unfilled hold left. After applying the patch the renewal should fail. Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 139494 [details] [review] Bug 31112: (QA follow-up) Restore check to avoid extra processing We now count all holds for all patrons, we can still eject if we have more holds than we do items Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 139495 [details] [review] Bug 31112: (QA follow-up) Fetch patrons with reserves Previously we fetched all in a single call using biblionumbers Fetching each individually could be a performance hit on systems with large numbers of holds Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
I reinstated the test for early exit - this is still a valid performance boost - we can't fill all holds if we don't have enough items Also added a prefetch to avoid single calls for each borrower Kyle is going to followup after me
Thanks for the review Nick! :) (In reply to Nick Clemens from comment #12) > I reinstated the test for early exit - this is still a valid performance > boost - we can't fill all holds if we don't have enough items OK, works for me. I didn't know it was for performance reasons and seems to make the logic not work correctly. I could open another bug report to fix this as I feel like we need some better way to make it more performant while still keeping the check correct.
prove t/db_dependent/Holds.t t/db_dependent/Holds.t .. 69/75 # Failed test 'Can renew' # at t/db_dependent/Holds.t line 1528. # Failed test 'Item is on non priority hold' # at t/db_dependent/Holds.t line 1529. # got: 'on_reserve' # expected: undef # Looks like you failed 2 tests of 6. t/db_dependent/Holds.t .. 71/75 # Failed test 'non priority holds' # at t/db_dependent/Holds.t line 1556. t/db_dependent/Holds.t .. 75/75 # Looks like you failed 1 test of 75. t/db_dependent/Holds.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/75 subtests Test Summary Report ------------------- t/db_dependent/Holds.t (Wstat: 256 Tests: 75 Failed: 1) Failed test: 71 Non-zero exit status: 1 Files=1, Tests=75, 7 wallclock secs ( 0.03 usr 0.01 sys + 5.04 cusr 0.68 csys = 5.76 CPU) Result: FAIL
Created attachment 139622 [details] [review] Bug 31112: (follow-up) Bring back the check for non-priority holds Before the changes from bug 31112 when CheckReserves returned a non-priority hold we didn't return "on_reserve" status but checked in addition to that whether there are any priority holds and if there were, only then we returned the "on_reserve" error. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Created attachment 139623 [details] [review] Bug 31112: (follow-up) Don't return "on_reserve" when there are 0 possible holds It's possible that there could be 0 possible reserves, for example when the hold has already been filled, thus the check would fail as the item count can never be less than 0. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi>
Thanks Kyle for spotting the failing test. I fixed the issue by restoring the previous check that I had accidentally removed (btw, the check looks somewhat buggy actually but that's for another bug report). There was also a small issue with the follow-up patch from Nick, added a fix for that too.
Created attachment 139729 [details] [review] Bug 31112: Remove unnecessary if-clause To test: 1) Please check manually that the logic stays the same, use git's -w command line parameter to ignore whitespace changes in the diff output. 2) prove t/db_dependent/Circulation.t Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 139730 [details] [review] Bug 31112: CanBookBeRenewed: take into account patrons with more than 1 hold to a biblio If a single patron had more than 1 hold to a biblio and there was only one available item we allowed incorrectly renewing the checkout when AllowRenewalIfOtherItemsAvailable was set to "Allow". This changes CanBookBeRenewed so that it makes sure all the holds are filled and not just one per patron. To test: 1) prove t/db_dependent/Circulation.t 2) (Optional, as unit test is provided) - Set AllowRenewalIfOtherItemsAvailable = Allow - Create biblio with three items - Checkout one item to patron A - Add two biblio-level holds for patron B - Try to renew patron A's checkout with and without this patch. - Notice that without this patch the renewal succeeds even though we one unfilled hold left. After applying the patch the renewal should fail. Signed-off-by: Sally <sally.healey@cheshiresharedservices.gov.uk> Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 139731 [details] [review] Bug 31112: (QA follow-up) Restore check to avoid extra processing We now count all holds for all patrons, we can still eject if we have more holds than we do items Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 139732 [details] [review] Bug 31112: (QA follow-up) Fetch patrons with reserves Previously we fetched all in a single call using biblionumbers Fetching each individually could be a performance hit on systems with large numbers of holds Signed-off-by: Nick Clemens <nick@bywatersolutions.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 139733 [details] [review] Bug 31112: (follow-up) Bring back the check for non-priority holds Before the changes from bug 31112 when CheckReserves returned a non-priority hold we didn't return "on_reserve" status but checked in addition to that whether there are any priority holds and if there were, only then we returned the "on_reserve" error. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 139734 [details] [review] Bug 31112: (follow-up) Don't return "on_reserve" when there are 0 possible holds It's possible that there could be 0 possible reserves, for example when the hold has already been filled, thus the check would fail as the item count can never be less than 0. Signed-off-by: Joonas Kylmälä <joonas.kylmala@iki.fi> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 139735 [details] [review] Bug 31112: (QA follow-up) Reduce database queries Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Pushed to master for 22.11. Nice work everyone, thanks!
Backported to 22.05.x for upcoming 22.05.07 release
This one brings in conflicts because of "UseRecalls" present in 22.05 (and not 21.11.x). It is not easy to backport, I'm afraid to miss the logic here. Can I get some help with a backport patch for 21.11? Wont backport unless provided. Arthur
This is already pushed to stable but I think the AllowRenewalIfOtherItemsAvailable syspref is misleading, there is nowhere information that other items should fill all the unfilled holds. Our superlibrarians have activated this and they are reporting that it is not working. The bug name also indicates that renewal should be possible when holds exceeds available items.
(In reply to Johanna Räisä from comment #28) > This is already pushed to stable but I think the > AllowRenewalIfOtherItemsAvailable syspref is misleading, there is nowhere > information that other items should fill all the unfilled holds. Our > superlibrarians have activated this and they are reporting that it is not > working. > > The bug name also indicates that renewal should be possible when holds > exceeds available items. Hi Johanna, can you file a new bug, describing the behaviour your librarians are seeing vs what they expect to see?
(In reply to Nick Clemens from comment #29) > (In reply to Johanna Räisä from comment #28) > > This is already pushed to stable but I think the > > AllowRenewalIfOtherItemsAvailable syspref is misleading, there is nowhere > > information that other items should fill all the unfilled holds. Our > > superlibrarians have activated this and they are reporting that it is not > > working. > > > > The bug name also indicates that renewal should be possible when holds > > exceeds available items. > > Hi Johanna, can you file a new bug, describing the behaviour your librarians > are seeing vs what they expect to see? I will do that. I was also a bit confused how this feature should work.