Bugzilla – Attachment 101615 Details for
Bug 24185
'If all unavailable' state for 'on shelf holds' makes holds page very slow if there's a lot of items
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 24185: Make holds page faster: preparatory refactoring
Bug-24185-Make-holds-page-faster-preparatory-refac.patch (text/plain), 15.37 KB, created by
Jonathan Druart
on 2020-03-24 15:47:57 UTC
(
hide
)
Description:
Bug 24185: Make holds page faster: preparatory refactoring
Filename:
MIME Type:
Creator:
Jonathan Druart
Created:
2020-03-24 15:47:57 UTC
Size:
15.37 KB
patch
obsolete
>From 96d75a38f6db2f186ed2d72af22d5207a668b610 Mon Sep 17 00:00:00 2001 >From: Andrew Nugged <nugged@gmail.com> >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 <fridolin.somers@biblibre.com> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Amended patch, I moved up the last line of the test, sounds more logical >to me. >--- > 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.20.1
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 24185
:
96079
|
96080
|
96151
|
96354
|
96460
|
96461
|
96464
|
96491
|
96492
|
96493
|
96500
|
97375
|
97376
|
97377
|
97378
|
101355
|
101356
|
101357
|
101358
|
101607
|
101608
|
101609
|
101611
|
101613
|
101614
| 101615 |
101616