https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 --- Comment #1 from Nick Clemens <nick@bywatersolutions.com> --- Created attachment 163236 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163236&action=edit 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 -- You are receiving this mail because: You are watching all bug changes. You are the assignee for the bug.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=29531 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 Andrew Fuerste-Henry <andrewfh@dubcolib.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163236|0 |1 is obsolete| | --- Comment #2 from Andrew Fuerste-Henry <andrewfh@dubcolib.org> --- Created attachment 163254 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163254&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 Nick Clemens <nick@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |martin.renvoize@ptfs-europe | |.com, | |pedro.amorim@ptfs-europe.co | |m -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 --- Comment #4 from Marcel de Rooy <m.de.rooy@rijksmuseum.nl> --- 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. -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 Marcel de Rooy <m.de.rooy@rijksmuseum.nl> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|BLOCKED |Passed QA Patch complexity|--- |Small patch -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 --- Comment #7 from Nick Clemens <nick@bywatersolutions.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)| |24.05.00 released in| | Status|Passed QA |Pushed to master -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Keywords| |additional_work_needed, | |RM_priority --- Comment #10 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- 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! -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 Tomás Cohen Arazi <tomascohen@gmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #163706|0 |1 is obsolete| | --- Comment #12 from Tomás Cohen Arazi <tomascohen@gmail.com> --- Created attachment 163707 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=163707&action=edit 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) -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 Fridolin Somers <fridolin.somers@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- See Also| |https://bugs.koha-community | |.org/bugzilla3/show_bug.cgi | |?id=26440 CC| |fridolin.somers@biblibre.co | |m -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 Lucas Gass <lucas@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Version(s)|24.05.00,23.11.05 |24.05.00,23.11.05,23.05.12 released in| | CC| |lucas@bywatersolutions.com Status|Pushed to stable |Pushed to oldstable --- Comment #15 from Lucas Gass <lucas@bywatersolutions.com> --- Backported to 23.05.x for upcoming 23.05.12 -- You are receiving this mail because: You are watching all bug changes.

https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=36331 Martin Renvoize <martin.renvoize@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC|martin.renvoize@ptfs-europe | |.com | -- You are receiving this mail because: You are watching all bug changes.