From 2d741d4d7158eff6dbeaf64bfc7e1fd9db9458a1 Mon Sep 17 00:00:00 2001 From: Andrew Nugged Date: Mon, 20 Jul 2020 00:20:56 +0300 Subject: [PATCH] Bug 24683: Fix for take smart rules into account in "if all unavailable" Inside of ItemsAnyAvailableAndNotRestricted was no effect from main set of smart rules (per record and other limits): i.e. call to "CanItemBeReserved" was absent totally. Because of this there was a bug: for example none of two items were allowed to be held when first was allowed by one smart rule, BUT on loan, and second was disallowed by another smart rule (for example, 0 "Holds per record"), i.e. in this case both items unavailable: so on-shelf holds setting "allow hold if all unavailable" should allow to hold first one, and not the second one. But it was that both wasn't allowed to be held. Solution: call to sub "CanItemBeReserved" added so it checked for "...->{status} ne 'OK'" so now if item restricted by smart rule it also accounted as "unavailable" and "AnyAvailavble" not counts it. How to reproduce: 1. Add 2 smart rules (/cgi-bin/koha/admin/smart-rules.pl) with "on shelf holds": "if all unavailable" for all rules, no "item level holds", and set "holds per record" to 2 for "books" and "0" for "computer files". 2. Create only 2 items for one biblio, but different types, "book" and "computer file". For example in misc4dev env: /cgi-bin/koha/cataloguing/additem.pl?biblionumber=1#additem 3. Check out that item of type "book" to some person, for example, in misc4dev: /cgi-bin/koha/circ/circulation.pl?borrowernumber=2&barcode=3999900000001 4. Open reserve/request, for example, for item 1 and patron 1 in misc4dev env (/cgi-bin/koha/reserve/request.pl?biblionumber=1&borrowernumber=1) 5. It does not allow to hold, both red crossed, but computer file says "Exceeded max holds per record" because of "0" limit set on step 1. 6. Apply the patch. 7. Reload page on step 5 and see that "book" will be available for hold, but "computer file" still will be red-crossed "Exceeded max holds per record", now that's correct because both items unavailable: one because on load, another because of "0" limit for computer files. 8. Check-in book from step 3 so it will be returned to the library, 9. Reload page on step 5 and see that again no any holds available, but it's now also correct: "book" now returned but "on shelf holds" set to "if all unavailable". Signed-off-by: Agustin Moyano Signed-off-by: Katrin Fischer Signed-off-by: Jonathan Druart (cherry picked from commit 4599fcc59d4036cb5e3d776b9f162062968e8d98) Signed-off-by: Lucas Gass --- C4/Reserves.pm | 3 +- .../Holds/DisallowHoldIfItemsAvailable.t | 81 ++++++++++++++++++- 2 files changed, 82 insertions(+), 2 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index d2dd9f0efa..ab8f41c105 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1340,7 +1340,8 @@ sub ItemsAnyAvailableAndNotRestricted { && ! C4::Context->preference('AllowHoldsOnDamagedItems') ) || Koha::ItemTypes->find( $i->effective_itemtype() )->notforloan || $branchitemrule->{holdallowed} == 1 && $param->{patron}->branchcode ne $i->homebranch - || $branchitemrule->{holdallowed} == 3 && ! $item_library->validate_hold_sibling( { branchcode => $param->{patron}->branchcode } ); + || $branchitemrule->{holdallowed} == 3 && ! $item_library->validate_hold_sibling( { branchcode => $param->{patron}->branchcode } ) + || CanItemBeReserved( $param->{patron}->borrowernumber, $i->id )->{status} ne 'OK'; } return 0; diff --git a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t index e54efc327c..c10d2c5b62 100755 --- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t +++ b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t @@ -8,7 +8,7 @@ use C4::Items; use Koha::Items; use Koha::CirculationRules; -use Test::More tests => 10; +use Test::More tests => 11; use t::lib::TestBuilder; use t::lib::Mocks; @@ -285,6 +285,85 @@ is( $is, 1, "Items availability: 1 item is available, 1 item held in T" ); $is = IsAvailableForItemLevelRequest( $item3, $patron1); is( $is, 1, "Item can be held, items in transit are not available" ); +subtest 'Check holds availability with different item types' => sub { + plan tests => 6; + + # Check for holds availability when different item types have different + # smart rules assigned both with "if all unavailable" set, + # and $itemtype rule allows holds, $itemtype2 rule disallows holds. + # So, $item should be available for hold when checked out even if $item2 + # is not checked out, because anyway $item2 unavailable for holds by rule + # (Bug 24683): + + my $biblio2 = $builder->build_sample_biblio( { itemtype => $itemtype } ); + my $biblionumber1 = $biblio2->biblionumber; + my $item4 = $builder->build_sample_item( + { biblionumber => $biblionumber1, + itype => $itemtype, + homebranch => $library_A, + holdingbranch => $library_A + } + ); + my $item5 = $builder->build_sample_item( + { biblionumber => $biblionumber1, + itype => $itemtype2, + homebranch => $library_A, + holdingbranch => $library_A + } + ); + + # Test hold_fulfillment_policy + $dbh->do("DELETE FROM circulation_rules"); + Koha::CirculationRules->set_rules( + { categorycode => undef, + itemtype => $itemtype, + branchcode => undef, + rules => { + issuelength => 7, + lengthunit => 8, + reservesallowed => 99, + holds_per_record => 99, + onshelfholds => 2, + } + } + ); + Koha::CirculationRules->set_rules( + { categorycode => undef, + itemtype => $itemtype2, + branchcode => undef, + rules => { + issuelength => 7, + lengthunit => 8, + reservesallowed => 0, + holds_per_record => 0, + onshelfholds => 2, + } + } + ); + + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber1, patron => $patron1 } ); + is( $is, 1, "Items availability: 2 items, one allowed by smart rule but not checked out, another one not allowed to be held by smart rule" ); + + $is = IsAvailableForItemLevelRequest( $item4, $patron1 ); + is( $is, 0, "Item4 cannot be requested to hold: 2 items, Item4 available, Item5 restricted" ); + + $is = IsAvailableForItemLevelRequest( $item5, $patron1 ); + is( $is, 0, "Item5 cannot be requested to hold: 2 items, Item4 available, Item5 restricted" ); + + AddIssue( $patron2->unblessed, $item4->barcode ); + + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber1, patron => $patron1 } ); + is( $is, 0, "Items availability: 2 items, one allowed by smart rule and checked out, another one not allowed to be held by smart rule" ); + + $is = IsAvailableForItemLevelRequest( $item4, $patron1 ); + is( $is, 1, "Item4 can be requested to hold, 2 items, Item4 checked out, Item5 restricted" ); + + $is = IsAvailableForItemLevelRequest( $item5, $patron1 ); + # Note: read IsAvailableForItemLevelRequest sub description about CanItemBeReserved/CanBookBeReserved: + is( $is, 1, "Item5 can be requested to hold, 2 items, Item4 checked out, Item5 restricted" ); +}; + + # Cleanup $schema->storage->txn_rollback; -- 2.39.5