@@ -, +, @@ changed This is the intermediate refactor: renamed subroutine only. sub IsAvailableForItemLevelRequest { ... my $any_available = ItemsAnyAvailableAndNotRestricted... return $any_available ? 0 : 1; # ^^^ if any available and not restricted - we don't allow # on-shelf holds ... --- C4/Reserves.pm | 19 ++++++++------- reserve/request.pl | 2 +- .../Holds/DisallowHoldIfItemsAvailable.t | 28 +++++++++++----------- 3 files changed, 25 insertions(+), 24 deletions(-) --- a/C4/Reserves.pm +++ a/C4/Reserves.pm @@ -123,7 +123,7 @@ BEGIN { &AutoUnsuspendReserves &IsAvailableForItemLevelRequest - ItemsAnyAvailableForHold + ItemsAnyAvailableAndNotRestricted &AlterPriority &ToggleLowestPriority @@ -1302,24 +1302,25 @@ sub IsAvailableForItemLevelRequest { return $items_any_available ? 0 : 1 if defined $items_any_available; - my $any_available = ItemsAnyAvailableForHold( { biblionumber => $item->biblionumber, patron => $patron }); + my $any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron }); return $any_available ? 0 : 1; } else { # on_shelf_holds == 0 "If any unavailable" (the description is rather cryptic and could still be improved) return $item->onloan || IsItemOnHoldAndFound( $item->itemnumber ); } } -=head2 ItemsAnyAvailableForHold +=head2 ItemsAnyAvailableAndNotRestricted - ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron }); + ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron }); -This function checks all items for specified biblionumber (num) / patron (object) -and returns true (1) or false (0) depending if any of rules allows at least of -one item to be available for hold including lots of parameters/logic +This function checks all items for specified biblionumber (numeric) against patron (object) +and returns true (1) if at least one item available for loan/check out/present/not held +and also checks other parameters logic which not restricts item for hold at all (for ex. +AllowHoldsOnDamagedItems or 'holdallowed' own/sibling library) =cut -sub ItemsAnyAvailableForHold { +sub ItemsAnyAvailableAndNotRestricted { my $param = shift; my @items = Koha::Items->search( { biblionumber => $param->{biblionumber} } ); @@ -1340,7 +1341,7 @@ sub ItemsAnyAvailableForHold { || $i->onloan || IsItemOnHoldAndFound( $i->id ) || ( $i->damaged - && ! C4::Context->preference('AllowHoldsOnDamagedItems') ) + && ! 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 } ); --- a/reserve/request.pl +++ a/reserve/request.pl @@ -450,7 +450,7 @@ foreach my $biblionumber (@biblionumbers) { # (before this loop was inside that sub loop so it was O(n^2) ) my $items_any_available; - $items_any_available = ItemsAnyAvailableForHold( { biblionumber => $biblioitemnumber, patron => $patron }) + $items_any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblioitemnumber, patron => $patron }) if $patron; foreach my $itemnumber ( @{ $itemnumbers_of_biblioitem{$biblioitemnumber} } ) { --- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t +++ a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t @@ -98,7 +98,7 @@ Koha::CirculationRules->set_rules( my $is; -$is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); +$is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); is( $is, 1, "Items availability: both of 2 items are available" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -106,7 +106,7 @@ is( $is, 0, "Item cannot be held, 2 items available" ); my $issue1 = AddIssue( $patron2->unblessed, $item1->barcode ); -$is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); +$is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); is( $is, 1, "Items availability: one item is available" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -114,7 +114,7 @@ is( $is, 0, "Item cannot be held, 1 item available" ); AddIssue( $patron2->unblessed, $item2->barcode ); -$is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); +$is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); is( $is, 0, "Items availability: none of items are available" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -144,7 +144,7 @@ AddReturn( $item1->barcode ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); - $is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item is( $is, 0, "Items availability: hold allowed from home + ReservesControlBranch=ItemHomeLibrary + one item is available at different library" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -154,7 +154,7 @@ AddReturn( $item1->barcode ); set_holdallowed_rule( $hold_allowed_from_any_libraries, $library_B ); #Adding a rule for the item's home library affects the availability for a borrower from another library because ReservesControlBranch is set to ItemHomeLibrary - $is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item is( $is, 1, "Items availability: hold allowed from any library for library B + ReservesControlBranch=ItemHomeLibrary + one item is available at different library" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -162,7 +162,7 @@ AddReturn( $item1->barcode ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); - $is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item is( $is, 0, "Items availability: hold allowed from any library for library B + ReservesControlBranch=PatronLibrary + one item is available at different library" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -170,7 +170,7 @@ AddReturn( $item1->barcode ); #Adding a rule for the patron's home library affects the availability for an item from another library because ReservesControlBranch is set to PatronLibrary set_holdallowed_rule( $hold_allowed_from_any_libraries, $library_A ); - $is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item is( $is, 1, "Items availability: hold allowed from any library for library A + ReservesControlBranch=PatronLibrary + one item is available at different library" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -182,7 +182,7 @@ AddReturn( $item1->barcode ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); - $is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item is( $is, 1, "Items availability: hold allowed from any library + ReservesControlBranch=ItemHomeLibrary + one item is available at different library" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -190,7 +190,7 @@ AddReturn( $item1->barcode ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); - $is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 0 items, library B 1 item is( $is, 1, "Items availability: hold allowed from any library + ReservesControlBranch=PatronLibrary + one item is available at different library" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -215,7 +215,7 @@ AddReturn( $item1->barcode ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); - $is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 1 item + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 1 item is( $is, 1, "Items availability: hold allowed from home library + ReservesControlBranch=ItemHomeLibrary + one item is available at home library" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -223,7 +223,7 @@ AddReturn( $item1->barcode ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); - $is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 1 item + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 1 item is( $is, 1, "Items availability: hold allowed from home library + ReservesControlBranch=PatronLibrary + one item is available at home library" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -235,7 +235,7 @@ AddReturn( $item1->barcode ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); - $is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 1 item + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 1 item is( $is, 1, "Items availability: hold allowed from any library + ReservesControlBranch=ItemHomeLibrary + one item is available at home library" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -243,7 +243,7 @@ AddReturn( $item1->barcode ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); - $is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 1 item + $is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 1 item is( $is, 1, "Items availability: hold allowed from any library + ReservesControlBranch=PatronLibrary + one item is available at home library" ); $is = IsAvailableForItemLevelRequest( $item1, $patron1); @@ -279,7 +279,7 @@ Koha::CirculationRules->set_rules( } ); -$is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 1 item +$is = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblionumber, patron => $patron1 }); # patron1 in library A, library A 1 item is( $is, 1, "Items availability: 1 item is available, 1 item held in T" ); $is = IsAvailableForItemLevelRequest( $item3, $patron1); --