In the case where an item cannot be held 'AllowRenewalIfOtherItemsAvailable' will deny renewal if no other items are available. Since the holds don't affect this item, it is invalid. There is a FIXME in the code about this: # FIXME: We are not checking whether the item we are renewing can fill the hold
Created attachment 163236 [details] [review] Bug 36331: Don't check reserves that an item cannot fill when checking if it can be renewed Before this patch we get all holds on a record and see if we can fill them with available items. This means we check to fill holds that the item in questoion may not be able to fill, especially in the case where no holds are allowed on the item type, this is wrong To test: 1 - Find or create a biblio with two items of different item types 2 - Make sure one item type allows holds, and the other has: "Default holds policy by item type" Set to "No holds allowed" 3 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow" 4 - Check out the unholdable item to a patron 5 - Set a hold for a different patron on the next available item 6 - Confirm the checked out item can be renewed (don't renew, just view the checkouts page) 7 - Checkout the other item to a third patron 8 - Confirm the first item can still be renewed 9 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Allow" 10 - Confirm the item cannot be renewed now 11 - Apply patch, restart all 12 - Confirm the item can be renewed 13 - Set the item type to a type that allows holds 14 - Confirm the item can no longer be renewed 15 - Restore the item type 16 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow" 17 - Confirm the item can be renewed 18 - Check in the item from the third patron 19 - Confirm the item can still be renewed 20 - prove -v t/db_dependent/Circulation.t - test still pass
Created attachment 163254 [details] [review] Bug 36331: Don't check reserves that an item cannot fill when checking if it can be renewed Before this patch we get all holds on a record and see if we can fill them with available items. This means we check to fill holds that the item in questoion may not be able to fill, especially in the case where no holds are allowed on the item type, this is wrong To test: 1 - Find or create a biblio with two items of different item types 2 - Make sure one item type allows holds, and the other has: "Default holds policy by item type" Set to "No holds allowed" 3 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow" 4 - Check out the unholdable item to a patron 5 - Set a hold for a different patron on the next available item 6 - Confirm the checked out item can be renewed (don't renew, just view the checkouts page) 7 - Checkout the other item to a third patron 8 - Confirm the first item can still be renewed 9 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Allow" 10 - Confirm the item cannot be renewed now 11 - Apply patch, restart all 12 - Confirm the item can be renewed 13 - Set the item type to a type that allows holds 14 - Confirm the item can no longer be renewed 15 - Restore the item type 16 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow" 17 - Confirm the item can be renewed 18 - Check in the item from the third patron 19 - Confirm the item can still be renewed 20 - prove -v t/db_dependent/Circulation.t - test still pass Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Looking here
next unless CanItemBeReserved($patron_with_reserve,$other_item,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy' # CanItemBeReserved checks 'rules' and 'policies' which means # items will fill holds at checkin that are rejected here Stumbling over that note while looking here.. Why do we actually need that check? Trying to understand altough we could say that it is outside scope ? Perhaps.
Also wondering (but very theoretical here) if walking thru items and holds in another order could possibly produce other results.. But not for now :)
Created attachment 163673 [details] [review] Bug 36331: Don't check reserves that an item cannot fill when checking if it can be renewed Before this patch we get all holds on a record and see if we can fill them with available items. This means we check to fill holds that the item in questoion may not be able to fill, especially in the case where no holds are allowed on the item type, this is wrong To test: 1 - Find or create a biblio with two items of different item types 2 - Make sure one item type allows holds, and the other has: "Default holds policy by item type" Set to "No holds allowed" 3 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow" 4 - Check out the unholdable item to a patron 5 - Set a hold for a different patron on the next available item 6 - Confirm the checked out item can be renewed (don't renew, just view the checkouts page) 7 - Checkout the other item to a third patron 8 - Confirm the first item can still be renewed 9 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Allow" 10 - Confirm the item cannot be renewed now 11 - Apply patch, restart all 12 - Confirm the item can be renewed 13 - Set the item type to a type that allows holds 14 - Confirm the item can no longer be renewed 15 - Restore the item type 16 - Set system preference "AllowRenewalIfOtherItemsAvailable" to "Don't allow" 17 - Confirm the item can be renewed 18 - Check in the item from the third patron 19 - Confirm the item can still be renewed 20 - prove -v t/db_dependent/Circulation.t - test still pass Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
(In reply to Marcel de Rooy from comment #4) > next unless > CanItemBeReserved($patron_with_reserve,$other_item,undef, > {ignore_hold_counts=>1})->{status} eq 'OK'; > # NOTE: At checkin we call 'CheckReserves' which checks > hold 'policy' > # CanItemBeReserved checks 'rules' and 'policies' which > means > # items will fill holds at checkin that are rejected here > > Stumbling over that note while looking here.. Why do we actually need that > check? Trying to understand altough we could say that it is outside scope ? > Perhaps. (In reply to Marcel de Rooy from comment #4) > next unless > CanItemBeReserved($patron_with_reserve,$other_item,undef, > {ignore_hold_counts=>1})->{status} eq 'OK'; > # NOTE: At checkin we call 'CheckReserves' which checks > hold 'policy' > # CanItemBeReserved checks 'rules' and 'policies' which > means > # items will fill holds at checkin that are rejected here > > Stumbling over that note while looking here.. Why do we actually need that > check? Trying to understand altough we could say that it is outside scope ? > Perhaps. I do think it is outside of the scope, it is really meant to be a for the coders - essentially this routine is more strict, but I think that makes sense. This has come up on other reports, we have two concepts: Can a hold be placed on this item Can this item fill a hold My patch here simply limits the holds checked against renewal to the ones that the item being renewed would fill. This is pre-existing logic that won't assume another item on the record can fill a hold that can't be placed (by a patron, staff can override) Librarians want this for the situation I describe in the test plan - a record with a mix of items that can and cannot be held. Generally these are new books, or 'lucky day' books as some libraries call them - books that won't be on hold so you get 'lucky' and find them in the library. These items cannot have holds, but currently holds can prevent their renewal - even with AllowRenewalIfOtherItemsAvailable (In reply to Marcel de Rooy from comment #5) > Also wondering (but very theoretical here) if walking thru items and holds > in another order could possibly produce other results.. But not for now :) Very true, perhaps once we get 35826 we could use that logic too: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=35826
Maybe a unit test testing this specific case would be good?
Pushed for 24.05! Well done everyone, thank you!
It looks like this actually breaks unit tests: prove t/db_dependent/Holds.t t/db_dependent/Holds.t .. 68/74 Use of uninitialized value in numeric le (<=) at /kohadevbox/koha/C4/Circulation.pm line 3014. Use of uninitialized value in addition (+) at /kohadevbox/koha/C4/Circulation.pm line 3021. Use of uninitialized value in numeric le (<=) at /kohadevbox/koha/C4/Circulation.pm line 3014. Use of uninitialized value in addition (+) at /kohadevbox/koha/C4/Circulation.pm line 3021. # Failed test 'Can renew' # at t/db_dependent/Holds.t line 1531. # Failed test 'Item is on non priority hold' # at t/db_dependent/Holds.t line 1532. # got: 'on_reserve' # expected: undef Use of uninitialized value in numeric le (<=) at /kohadevbox/koha/C4/Circulation.pm line 3014. Use of uninitialized value in addition (+) at /kohadevbox/koha/C4/Circulation.pm line 3021. # Looks like you failed 2 tests of 6. # Failed test 'non priority holds' # at t/db_dependent/Holds.t line 1559. t/db_dependent/Holds.t .. 73/74 # Looks like you failed 1 test of 74. t/db_dependent/Holds.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/74 subtests Test Summary Report ------------------- t/db_dependent/Holds.t (Wstat: 256 Tests: 74 Failed: 1) Failed test: 69 Non-zero exit status: 1 Files=1, Tests=74, 7 wallclock secs ( 0.02 usr 0.01 sys + 4.67 cusr 0.98 csys = 5.68 CPU) Result: FAIL Please follow-up!
Created attachment 163706 [details] [review] Bug 36331: (follow-up) Ignore non_priority holds when checking renewability When changing the fetch of holds, the check for non-priority was lost - added a loop to pull those out so the totals and checks are correct
Created attachment 163707 [details] [review] Bug 36331: (follow-up) Ignore non_priority holds when checking renewability When changing the fetch of holds, the check for non-priority was lost - added a loop to pull those out so the totals and checks are correct Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io> Tidied (tcohen)
Thanks for the fast reaction! Follow-up pushed to master.
Pushed to 23.11.x for 23.11.05
Backported to 23.05.x for upcoming 23.05.12