From 490634a688a3933e3a118e2265f395ba58a32553 Mon Sep 17 00:00:00 2001 From: Andrew Nugged Date: Fri, 5 Jun 2020 09:59:48 +0300 Subject: [PATCH] Bug 24683: Subroutine name changed (fix), no code logic changed This is the intermediate refactor: renamed subroutine only. Naming mistake came because this sub is used to detect if anything available for hold, but it used in "if ANY UNAVAILABLE rule", so actually results of this sub negated (see below "return" in the code). In details: when previous refactor was done, name for subroutine was chosen wrongly in "opposite" direction from what it actually does: it was named "ItemsAnyAvailableForHold", but this subroutine gave truth (1) if at least one of the items available on shelf, not lost, not on loan, not held, and not restricted by smart rules and damaged status. So, if this sub says that item is still "available", this actually PREVENTS item from hold in parent sub (see negated return): sub IsAvailableForItemLevelRequest { ... my $any_available = ItemsAnyAvailableAndNotRestricted... return $any_available ? 0 : 1; # ^^^ if any available and not restricted - we don't allow # on-shelf holds ... I.e. like it named now: "ItemsAnyAvailableAndNotRestricted". Small aside fix: white space for '&&' inside brackets added to join operation by priority visually. Testing plan not needed: all places where sub used it just renamed. More: all this places/code was introduced in one older commit so there is also no overlaps or other calls/uses for this subroutine. Signed-off-by: Agustin Moyano --- C4/Reserves.pm | 19 +++++++------ reserve/request.pl | 2 +- .../Holds/DisallowHoldIfItemsAvailable.t | 28 +++++++++---------- 3 files changed, 25 insertions(+), 24 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 979b3de244..9054e4fddb 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -122,7 +122,7 @@ BEGIN { &AutoUnsuspendReserves &IsAvailableForItemLevelRequest - ItemsAnyAvailableForHold + ItemsAnyAvailableAndNotRestricted &AlterPriority &ToggleLowestPriority @@ -1299,24 +1299,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} } ); @@ -1337,7 +1338,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 } ); diff --git a/reserve/request.pl b/reserve/request.pl index 26e40dd685..9c8e0090d7 100755 --- a/reserve/request.pl +++ b/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} } ) { diff --git a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t index ae5e96cb60..e54efc327c 100755 --- a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t +++ b/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); -- 2.25.0