From c88d6cc27d6e202b54695274700a0179abfe97f3 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Wed, 22 Mar 2023 16:30:31 +0100 Subject: [PATCH] Bug 30860: Cache CanItemBeReserved return value This patch caches the return value of CanItemBeReserved that could be then returned *on demand* We don't want to introduce side-effects hard to catch from this simple change, so let's return the cache value only from the 2 scripts we are dealing with. This patch requests all item values from CanBookBeReserved on request.pl Before this we either: - Looped every item to find out that book could not be reserved - Looped until we found an item that could be reserved, then looped all items to get statuses In the worst case we avoid double processing a single item, in the best case we avoid double processing all items (if only last on record is holdable) To test: 1 - Find a record in staff client with several items 2 - Set AllowHoldsOnDamagedItems to 'Dont allow' 3 - Add a damaged item to record 4 - Set a hold rule to only allow holds form homebranch and ensure record has items from other branches 5 - Setup things to prevent more items from being held 6 - Attempt hold for patron 7 - Note item statuses 8 - Apply patch 9 - Confirm statuses are as they were before Signed-off-by: Nick Clemens Signed-off-by: Katrin Fischer --- C4/Reserves.pm | 58 ++++++++++++++++++++++++++++---------------- opac/opac-reserve.pl | 4 +-- reserve/request.pl | 2 +- 3 files changed, 40 insertions(+), 24 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 987543e1cd..10550b9b49 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -36,6 +36,7 @@ use Koha::Account::Lines; use Koha::BackgroundJob::BatchUpdateBiblioHoldsQueue; use Koha::Biblios; use Koha::Calendar; +use Koha::Cache::Memory::Lite; use Koha::CirculationRules; use Koha::Database; use Koha::DateUtils qw( dt_from_string output_pref ); @@ -419,9 +420,24 @@ sub CanBookBeReserved{ =cut +our $CanItemBeReserved_cache_key; +sub _cache { + my ( $return ) = @_; + my $memory_cache = Koha::Cache::Memory::Lite->get_instance(); + $memory_cache->set_in_cache( $CanItemBeReserved_cache_key, $return ); + return $return; +} + sub CanItemBeReserved { my ( $patron, $item, $pickup_branchcode, $params ) = @_; + my $memory_cache = Koha::Cache::Memory::Lite->get_instance(); + $CanItemBeReserved_cache_key = sprintf "Hold_CanItemBeReserved:%s:%s:%s", $patron->borrowernumber, $item->itemnumber, $pickup_branchcode || ""; + if ( $params->{get_from_cache} ) { + my $cached = $memory_cache->get_from_cache($CanItemBeReserved_cache_key); + return $cached if $cached; + } + my $dbh = C4::Context->dbh; my $ruleitemtype; # itemtype of the matching issuing rule my $allowedreserves = 0; # Total number of holds allowed across all records, default to none @@ -432,7 +448,7 @@ sub CanItemBeReserved { and !C4::Context->preference('canreservefromotherbranches') ) { if ( $item->homebranch ne $patron->branchcode ) { - return { status => 'cannotReserveFromOtherBranches' }; + return _cache { status => 'cannotReserveFromOtherBranches' }; } } @@ -441,7 +457,7 @@ sub CanItemBeReserved { my $borrower = $patron->unblessed; # If an item is damaged and we don't allow holds on damaged items, we can stop right here - return { status =>'damaged' } + return _cache { status =>'damaged' } if ( $item->damaged && !C4::Context->preference('AllowHoldsOnDamagedItems') ); @@ -450,21 +466,21 @@ sub CanItemBeReserved { # Check for the age restriction my ( $ageRestriction, $daysToAgeRestriction ) = C4::Circulation::GetAgeRestriction( $biblio->biblioitem->agerestriction, $borrower ); - return { status => 'ageRestricted' } if $daysToAgeRestriction && $daysToAgeRestriction > 0; + return _cache { status => 'ageRestricted' } if $daysToAgeRestriction && $daysToAgeRestriction > 0; } # Check that the patron doesn't have an item level hold on this item already - return { status =>'itemAlreadyOnHold' } + return _cache { status =>'itemAlreadyOnHold' } if ( !$params->{ignore_hold_counts} && Koha::Holds->search( { borrowernumber => $patron->borrowernumber, itemnumber => $item->itemnumber } )->count() ); # Check that patron have not checked out this biblio (if AllowHoldsOnPatronsPossessions set) if ( !C4::Context->preference('AllowHoldsOnPatronsPossessions') && C4::Circulation::CheckIfIssuedToPatron( $patron->borrowernumber, $item->biblionumber ) ) { - return { status =>'alreadypossession' }; + return _cache { status =>'alreadypossession' }; } # check if a recall exists on this item from this borrower - return { status => 'recall' } + return _cache { status => 'recall' } if $patron->recalls->filter_by_current->search({ item_id => $item->itemnumber })->count; my $controlbranch = C4::Context->preference('ReservesControlBranch'); @@ -508,7 +524,7 @@ sub CanItemBeReserved { if ( defined $holds_per_record && $holds_per_record ne '' ){ if ( $holds_per_record == 0 ) { - return { status => "noReservesAllowed" }; + return _cache { status => "noReservesAllowed" }; } if ( !$params->{ignore_hold_counts} ) { my $search_params = { @@ -516,7 +532,7 @@ sub CanItemBeReserved { biblionumber => $item->biblionumber, }; my $holds = Koha::Holds->search($search_params); - return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record } if $holds->count() >= $holds_per_record; + return _cache { status => "tooManyHoldsForThisRecord", limit => $holds_per_record } if $holds->count() >= $holds_per_record; } } @@ -526,13 +542,13 @@ sub CanItemBeReserved { borrowernumber => $patron->borrowernumber, reservedate => dt_from_string->date }); - return { status => 'tooManyReservesToday', limit => $holds_per_day } if $today_holds->count() >= $holds_per_day; + return _cache { status => 'tooManyReservesToday', limit => $holds_per_day } if $today_holds->count() >= $holds_per_day; } # we check if it's ok or not if ( defined $allowedreserves && $allowedreserves ne '' ){ if( $allowedreserves == 0 ){ - return { status => 'noReservesAllowed' }; + return _cache { status => 'noReservesAllowed' }; } if ( !$params->{ignore_hold_counts} ) { # we retrieve count @@ -577,7 +593,7 @@ sub CanItemBeReserved { $reservecount = $rowcount->{count}; } - return { status => 'tooManyReserves', limit => $allowedreserves } if $reservecount >= $allowedreserves; + return _cache { status => 'tooManyReserves', limit => $allowedreserves } if $reservecount >= $allowedreserves; } } @@ -596,26 +612,26 @@ sub CanItemBeReserved { } )->count(); - return { status => 'tooManyReserves', limit => $rule->rule_value} if $total_holds_count >= $rule->rule_value; + return _cache { status => 'tooManyReserves', limit => $rule->rule_value} if $total_holds_count >= $rule->rule_value; } my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->effective_itemtype ); if ( $branchitemrule->{holdallowed} eq 'not_allowed' ) { - return { status => 'notReservable' }; + return _cache { status => 'notReservable' }; } if ( $branchitemrule->{holdallowed} eq 'from_home_library' && $borrower->{branchcode} ne $item->homebranch ) { - return { status => 'cannotReserveFromOtherBranches' }; + return _cache { status => 'cannotReserveFromOtherBranches' }; } my $item_library = Koha::Libraries->find( {branchcode => $item->homebranch} ); if ( $branchitemrule->{holdallowed} eq 'from_local_hold_group') { if($patron->branchcode ne $item->homebranch && !$item_library->validate_hold_sibling( {branchcode => $patron->branchcode} )) { - return { status => 'branchNotInHoldGroup' }; + return _cache { status => 'branchNotInHoldGroup' }; } } @@ -625,23 +641,23 @@ sub CanItemBeReserved { }); unless ($destination) { - return { status => 'libraryNotFound' }; + return _cache { status => 'libraryNotFound' }; } unless ($destination->pickup_location) { - return { status => 'libraryNotPickupLocation' }; + return _cache { status => 'libraryNotPickupLocation' }; } unless ($item->can_be_transferred({ to => $destination })) { - return { status => 'cannotBeTransferred' }; + return _cache { status => 'cannotBeTransferred' }; } if ($branchitemrule->{hold_fulfillment_policy} eq 'holdgroup' && !$item_library->validate_hold_sibling( {branchcode => $pickup_branchcode} )) { - return { status => 'pickupNotInHoldGroup' }; + return _cache { status => 'pickupNotInHoldGroup' }; } if ($branchitemrule->{hold_fulfillment_policy} eq 'patrongroup' && !Koha::Libraries->find({branchcode => $borrower->{branchcode}})->validate_hold_sibling({branchcode => $pickup_branchcode})) { - return { status => 'pickupNotInHoldGroup' }; + return _cache { status => 'pickupNotInHoldGroup' }; } } - return { status => 'OK' }; + return _cache { status => 'OK' }; } =head2 CanReserveBeCanceledFromOpac diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index 0778ac5d04..b1eba4fd28 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -264,7 +264,7 @@ if ( $query->param('place_reserve') ) { my $biblio = Koha::Biblios->find($biblioNum); my $rank = $biblio->holds->search( { found => [ { "!=" => "W" }, undef ] } )->count + 1; if ( $item ) { - my $status = CanItemBeReserved( $patron, $item, $branch )->{status}; + my $status = CanItemBeReserved( $patron, $item, $branch, { get_from_cache => 1 } )->{status}; if( $status eq 'OK' ){ $canreserve = 1; } else { @@ -484,7 +484,7 @@ foreach my $biblioNum (@biblionumbers) { # items_any_available defined outside of the current loop, # so we avoiding loop inside IsAvailableForItemLevelRequest: my $policy_holdallowed = - CanItemBeReserved( $patron, $item )->{status} eq 'OK' && + CanItemBeReserved( $patron, $item, undef, { get_from_cache => 1 } )->{status} eq 'OK' && IsAvailableForItemLevelRequest($item, $patron, undef, $items_any_available); if ($policy_holdallowed) { diff --git a/reserve/request.pl b/reserve/request.pl index fce3a6e5b0..1e6786a7a6 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -486,7 +486,7 @@ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) $item->{'holdallowed'} = $branchitemrule->{'holdallowed'}; - my $can_item_be_reserved = CanItemBeReserved( $patron, $item_object )->{status}; + my $can_item_be_reserved = CanItemBeReserved( $patron, $item_object, undef, { get_from_cache => 1 } )->{status}; $item->{not_holdable} = $can_item_be_reserved unless ( $can_item_be_reserved eq 'OK' ); $item->{not_holdable} ||= 'notforloan' if ( $item->{notforloanitype} || $item->{notforloan} > 0 ); -- 2.30.2