From d3a37911cfbddfa99f50d06e9c423a4a05e89557 Mon Sep 17 00:00:00 2001 From: Andrew Nugged Date: Tue, 14 Jan 2020 22:51:57 +0200 Subject: [PATCH] Bug 24185: Make holds page faster: Preparatory refactoring This is just refactoring. extracting logically independent code to separate sub + tests update. No logic change yet. Searching for "any_available" item among all biblionumber items was done inside of "elsif on_shelf_holds == 2", and it is logically very independent piece of code (this "@items" loop), it needs just biblionumber and patron as parameters so it can be extracted into separate subroutine, and later also called/reused from somewhere else. This ability to call from another place also made for future patch to remove O(n^2) problem with nested loops. Signed-off-by: Fridolin Somers Signed-off-by: Jonathan Druart Signed-off-by: Martin Renvoize --- C4/Reserves.pm | 66 ++++++++++++------- .../Holds/DisallowHoldIfItemsAvailable.t | 62 +++++++++++++++-- 2 files changed, 100 insertions(+), 28 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 437180ea76..177fce0e20 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -122,6 +122,7 @@ BEGIN { &AutoUnsuspendReserves &IsAvailableForItemLevelRequest + ItemsAnyAvailableForHold &AlterPriority &ToggleLowestPriority @@ -1259,36 +1260,53 @@ sub IsAvailableForItemLevelRequest { if ( $on_shelf_holds == 1 ) { return 1; } elsif ( $on_shelf_holds == 2 ) { - my @items = - Koha::Items->search( { biblionumber => $item->biblionumber } ); - - my $any_available = 0; - - foreach my $i (@items) { - my $reserves_control_branch = GetReservesControlBranch( $i->unblessed(), $patron->unblessed ); - my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $i->itype ); - my $item_library = Koha::Libraries->find( {branchcode => $i->homebranch} ); - - - $any_available = 1 - unless $i->itemlost - || $i->notforloan > 0 - || $i->withdrawn - || $i->onloan - || IsItemOnHoldAndFound( $i->id ) - || ( $i->damaged - && !C4::Context->preference('AllowHoldsOnDamagedItems') ) - || Koha::ItemTypes->find( $i->effective_itemtype() )->notforloan - || $branchitemrule->{holdallowed} == 1 && $patron->branchcode ne $i->homebranch - || $branchitemrule->{holdallowed} == 3 && !$item_library->validate_hold_sibling( {branchcode => $patron->branchcode} ); - } - + my $any_available = ItemsAnyAvailableForHold( { 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 + + ItemsAnyAvailableForHold( { 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 + +=cut + +sub ItemsAnyAvailableForHold { + my $param = shift; + + my @items = Koha::Items->search( { biblionumber => $param->{biblionumber} } ); + + my $any_available = 0; + + foreach my $i (@items) { + my $reserves_control_branch = + GetReservesControlBranch( $i->unblessed(), $param->{patron}->unblessed ); + my $branchitemrule = + C4::Circulation::GetBranchItemRule( $reserves_control_branch, $i->itype ); + my $item_library = Koha::Libraries->find( { branchcode => $i->homebranch } ); + + $any_available = 1 + unless $i->itemlost + || $i->notforloan > 0 + || $i->withdrawn + || $i->onloan + || IsItemOnHoldAndFound( $i->id ) + || ( $i->damaged + && ! 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 } ); + } + + return $any_available; +} + =head2 AlterPriority AlterPriority( $where, $reserve_id, $prev_priority, $next_priority, $first_priority, $last_priority ); diff --git a/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t b/t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t index 86725cb678..ae5e96cb60 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 => 6; +use Test::More tests => 10; use t::lib::TestBuilder; use t::lib::Mocks; @@ -96,16 +96,27 @@ Koha::CirculationRules->set_rules( } ); -my $is = IsAvailableForItemLevelRequest( $item1, $patron1); +my $is; + +$is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); +is( $is, 1, "Items availability: both of 2 items are available" ); + +$is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Item cannot be held, 2 items available" ); my $issue1 = AddIssue( $patron2->unblessed, $item1->barcode ); +$is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); +is( $is, 1, "Items availability: one item is available" ); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Item cannot be held, 1 item available" ); AddIssue( $patron2->unblessed, $item2->barcode ); +$is = ItemsAnyAvailableForHold( { biblionumber => $biblionumber, patron => $patron1 }); +is( $is, 0, "Items availability: none of items are available" ); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 1, "Item can be held, no items available" ); @@ -119,7 +130,7 @@ AddReturn( $item1->barcode ); my $hold_allowed_from_any_libraries = 2; subtest 'Item is available at a different library' => sub { - plan tests => 7; + plan tests => 13; $item1->set({homebranch => $library_B, holdingbranch => $library_B })->store; #Scenario is: @@ -132,20 +143,36 @@ AddReturn( $item1->barcode ); set_holdallowed_rule( $hold_allowed_from_home_library ); 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( $is, 0, "Items availability: hold allowed from home + ReservesControlBranch=ItemHomeLibrary + one item is available at different library" ); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 1, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at different library, not holdable = none available => the hold is allowed at item level" ); $is = IsAvailableForItemLevelRequest( $item1, $patron2); is( $is, 1, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at home library, holdable = one available => the hold is not allowed at item level" ); 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( $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); is( $is, 0, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at different library, holdable = one available => the hold is not allowed at item level" ); 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( $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); is( $is, 1, "Hold allowed from home library + ReservesControlBranch=PatronLibrary, One item is available at different library, not holdable = none available => the hold is allowed at item level" ); #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( $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); is( $is, 0, "Hold allowed from home library + ReservesControlBranch=PatronLibrary, One item is available at different library, holdable = one available => the hold is not allowed at item level" ); } @@ -154,17 +181,25 @@ AddReturn( $item1->barcode ); set_holdallowed_rule( $hold_allowed_from_any_libraries ); 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( $is, 1, "Items availability: hold allowed from any library + ReservesControlBranch=ItemHomeLibrary + one item is available at different library" ); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Hold allowed from any library + ReservesControlBranch=ItemHomeLibrary, One item is available at the diff library, holdable = 1 available => the hold is not allowed at item level" ); 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( $is, 1, "Items availability: hold allowed from any library + ReservesControlBranch=PatronLibrary + one item is available at different library" ); + $is = IsAvailableForItemLevelRequest( $item1, $patron1); is( $is, 0, "Hold allowed from any library + ReservesControlBranch=PatronLibrary, One item is available at the diff library, holdable = 1 available => the hold is not allowed at item level" ); } }; subtest 'Item is available at the same library' => sub { - plan tests => 4; + plan tests => 8; $item1->set({homebranch => $library_A, holdingbranch => $library_A })->store; #Scenario is: @@ -179,10 +214,18 @@ AddReturn( $item1->barcode ); set_holdallowed_rule( $hold_allowed_from_home_library ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); + + $is = ItemsAnyAvailableForHold( { 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); is( $is, 0, "Hold allowed from home library + ReservesControlBranch=ItemHomeLibrary, One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); + + $is = ItemsAnyAvailableForHold( { 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); is( $is, 0, "Hold allowed from home library + ReservesControlBranch=PatronLibrary, One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); } @@ -191,10 +234,18 @@ AddReturn( $item1->barcode ); set_holdallowed_rule( $hold_allowed_from_any_libraries ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); + + $is = ItemsAnyAvailableForHold( { 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); is( $is, 0, "Hold allowed from any library + ReservesControlBranch=ItemHomeLibrary, One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); t::lib::Mocks::mock_preference('ReservesControlBranch', 'PatronLibrary'); + + $is = ItemsAnyAvailableForHold( { 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); is( $is, 0, "Hold allowed from any library + ReservesControlBranch=PatronLibrary, One item is available at the same library, holdable = 1 available => the hold is not allowed at item level" ); } @@ -228,6 +279,9 @@ Koha::CirculationRules->set_rules( } ); +$is = ItemsAnyAvailableForHold( { 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); is( $is, 1, "Item can be held, items in transit are not available" ); -- 2.39.5