Browse Source

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 <fridolin.somers@biblibre.com>
Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
20.05.x
Andrew Nugged 4 years ago
committed by Martin Renvoize
parent
commit
d3a37911cf
Signed by: martin.renvoize GPG Key ID: 422B469130441A0F
  1. 66
      C4/Reserves.pm
  2. 62
      t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t

66
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 );

62
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" );

Loading…
Cancel
Save