From 0908a791acf10002bad56a3b5baf3f00d586e53f Mon Sep 17 00:00:00 2001 From: Andrew Nugged Date: Thu, 19 Dec 2019 16:39:04 +0000 Subject: [PATCH] Bug 24185: Make holds page fast when 'on shelf holds' set to 'If all unavailable' When "reserve/request.pl -> C4/Reserves.pm::IsAvailableForItemLevelRequest" called many times with hundred of items and "on shelf holds" parameter set to "If all unavailable" for these items + patron, it goes slow. It happens because in subloop there is checking if all items available so it is O(n^2) and it re-checks each time the same info for each item with repeating DB/data requests. Fix: The inner loop 1:1 picked out from another loop and put before that items processing loop, 'items_any_available' variable precalculated, this variable passed to IsAvailableForItemLevelRequest to be used inside as the precalculated result. On tests it gave a drastic improvement in execution time. How to reproduce: 1) on freshly installed kohadevbox create/import one book, remember that biblionumber for later use it in down below, 2) add 100 items for that book for some library, 3) find some patron, that patron's card number we will use as a borrower down below to open holds page, 4) check for the rule or set up a single circulation rule in admin "/cgi-bin/koha/admin/smart-rules.pl", that rule should match above book items/library/patron, check that rule to have a non-zero number of holds (total, daily, count) allowed, and, IMPORTANT: set up "On shelf holds allowed" to "If all unavailable", ("item level holds" doesn't matter). 5) open "Home > Catalog > THAT_BOOK > Place a hold on THAT_BOOK" page ("holds" tab), and enter patron code in the search field, or you can create a direct link by yourself, for example, in my case it was: /cgi-bin/koha/reserve/request.pl?biblionumber=4&findborrower=23529000686353 6) it should be pretty long page generation time on old code, densely increasing for every hundred items added. In the case of this solution, it's fast, and time increases a little only, linear. I tested on my computer in VirtualBox for page generation times, did 3-5 runs for same case to check if results are stable, and got such values: (old code): 100 items: 38 seconds 200 items: 2.5 minutes 300 items: 5.4 minutes (version with fix): 100 items: 3.7 seconds 200 items: 6.4 seconds 300 items: 8.7 seconds --- C4/Reserves.pm | 13 ++++++++++++- reserve/request.pl | 38 +++++++++++++++++++++++++++++++++++++- 2 files changed, 49 insertions(+), 2 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index b97f60d981..cad13136dc 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1188,7 +1188,12 @@ and canreservefromotherbranches. =cut sub IsAvailableForItemLevelRequest { - my ( $item, $patron, $pickup_branchcode ) = @_; + my $item = shift; + my $patron = shift; + my $pickup_branchcode = shift; + # items_any_available is precalculated status passed from request.pl when set of items + # looped outside of IsAvailableForItemLevelRequest to avoid nested loops: + my $items_any_available = shift; my $dbh = C4::Context->dbh; # must check the notforloan setting of the itemtype @@ -1217,6 +1222,12 @@ sub IsAvailableForItemLevelRequest { if ( $on_shelf_holds == 1 ) { return 1; } elsif ( $on_shelf_holds == 2 ) { + + # if we have this param predefined from outer caller sub, we just need + # to return it, so we saving from having loop inside other loop: + return $items_any_available ? 0 : 1 + if defined $items_any_available; + my @items = Koha::Items->search( { biblionumber => $item->biblionumber } ); diff --git a/reserve/request.pl b/reserve/request.pl index dc345900dc..69316633cf 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -444,6 +444,15 @@ foreach my $biblionumber (@biblionumbers) { $itemtypes->{ $biblioitem->{itemtype} }{imageurl} ); } + # iterating through all items first to check if any of them available + # to pass this value further inside down to IsAvailableForItemLevelRequest to + # it's complicated logic to analyse. + # (before this loop was inside that sub loop so it was O(n^2) ) + my $items_any_available; + + $items_any_available = items_any_available($biblioitemnumber) + if $patron; + foreach my $itemnumber ( @{ $itemnumbers_of_biblioitem{$biblioitemnumber} } ) { my $item = $iteminfos_of->{$itemnumber}; my $do_check; @@ -557,7 +566,9 @@ foreach my $biblionumber (@biblionumbers) { !$item->{cantreserve} && !$exceeded_maxreserves && $can_item_be_reserved eq 'OK' - && IsAvailableForItemLevelRequest($item_object, $patron) + # items_any_available defined outside of the current loop, + # so we avoiding loop inside IsAvailableForItemLevelRequest: + && IsAvailableForItemLevelRequest($item_object, $patron, undef, $items_any_available) ) { $item->{available} = 1; @@ -745,3 +756,28 @@ sub sort_borrowerlist { } @{$borrowerslist}; return $ref; } + +sub items_any_available { + my $biblioitemnumber = shift; + my $items_any_available = 0; + + my @items = Koha::Items->search( { biblionumber => $biblioitemnumber } ); + + foreach my $i (@items) { + my $reserves_control_branch = GetReservesControlBranch( $i->unblessed(), $patron->unblessed ); + my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $i->itype ); + + $items_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; + } + + return $items_any_available; +} -- 2.17.1