From 105750acb100aa618f3cfe8f3a1dcb5071b287ed Mon Sep 17 00:00:00 2001 From: Nick Clemens Date: Fri, 30 Jun 2023 19:26:40 +0000 Subject: [PATCH] Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate There are several places in the code where we precalculate ItemsAnyAvailableAndNotRestricted to avoid looping on this routine when calling IsAvailableForItemLevelRequest on a list of items form a biblio The value of ItemsAnyAvailableAndNotRestricted is only used when there is a circulation rule for 'onshelfholds' with a value of '2' (If all unavailable) Rather than calculate a value that may never be used, let's cache this value per request when we do calculate it - and reuse the cached value To test: 1 - Apply patch 2 - Set circulation rule 'On shelf holds allowed' as 'If all unavailable' make sure the rule applies to all of the items/patrons you test with 3 - Find a record with two items that are available 4 - Try to place a hold for a patron - not allowed 5 - Check out one item to another patron 6 - Attempt hold - still not allowed 7 - Check out second item to another patron 8 - Attempt hold - allowed! 9 - Apply patch 10 - Cancel and replace hold - it is allowed! 11 - Check in one item, and cancel hold 12 - Place hold - not allowed! 13 - Check in second item 14 - Place hold - not allowed! 15 - prove -v t/db_dependent/Holds/DisallowHoldIfItemsAvailable.t Signed-off-by: Sam Lau Signed-off-by: Martin Renvoize Signed-off-by: Tomas Cohen Arazi --- C4/Circulation.pm | 8 ++------ C4/Reserves.pm | 19 +++++++++++-------- opac/opac-reserve.pl | 8 ++------ reserve/request.pl | 9 ++------- 4 files changed, 17 insertions(+), 27 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 5c895ed660..5a6e036685 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -26,7 +26,7 @@ use Encode; use C4::Context; use C4::Stats qw( UpdateStats ); -use C4::Reserves qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority RevertWaitingStatus IsItemOnHoldAndFound IsAvailableForItemLevelRequest ItemsAnyAvailableAndNotRestricted ); +use C4::Reserves qw( CheckReserves CanItemBeReserved MoveReserve ModReserve ModReserveMinusPriority RevertWaitingStatus IsItemOnHoldAndFound IsAvailableForItemLevelRequest ); use C4::Biblio qw( UpdateTotalIssues ); use C4::Items qw( ModItemTransfer ModDateLastSeen CartToShelf ); use C4::Accounts; @@ -3056,17 +3056,13 @@ sub CanBookBeRenewed { foreach my $possible_hold (@possible_holds) { my $fillable = 0; my $patron_with_reserve = Koha::Patrons->find($possible_hold->borrowernumber); - my $items_any_available = ItemsAnyAvailableAndNotRestricted({ - biblionumber => $item->biblionumber, - patron => $patron_with_reserve - }); # FIXME: We are not checking whether the item we are renewing can fill the hold foreach my $other_item (@other_items) { next if defined $matched_items{$other_item->itemnumber}; next if IsItemOnHoldAndFound( $other_item->itemnumber ); - next unless IsAvailableForItemLevelRequest($other_item, $patron_with_reserve, undef, $items_any_available); + next unless IsAvailableForItemLevelRequest($other_item, $patron_with_reserve, undef); next unless CanItemBeReserved($patron_with_reserve,$other_item,undef,{ignore_hold_counts=>1})->{status} eq 'OK'; # NOTE: At checkin we call 'CheckReserves' which checks hold 'policy' # CanItemBeReserved checks 'rules' and 'policies' which means diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 01aa726322..4b0b526a74 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -1336,9 +1336,6 @@ sub IsAvailableForItemLevelRequest { 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 @@ -1376,13 +1373,19 @@ sub IsAvailableForItemLevelRequest { 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; + # These calculations work at the biblio level, and can be expensive + # we use the in-memory cache to avoid calling once per item when looping items on a biblio - my $any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron }); + my $memory_cache = Koha::Cache::Memory::Lite->get_instance(); + my $cache_key = sprintf "ItemsAnyAvailableAndNotRestricted:%s:%s", $patron->id, $item->biblionumber; + + my $any_available = $memory_cache->get_from_cache( $cache_key ); + return $any_available ? 0 : 1 if defined( $any_available ); + + $any_available = ItemsAnyAvailableAndNotRestricted( { biblionumber => $item->biblionumber, patron => $patron }); + $memory_cache->set_in_cache( $cache_key, $any_available ); 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 ); } diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index b1eba4fd28..118a63d1a7 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -25,7 +25,7 @@ use CGI qw ( -utf8 ); use C4::Auth qw( get_template_and_user ); use C4::Koha qw( getitemtypeimagelocation getitemtypeimagesrc ); use C4::Circulation qw( GetBranchItemRule ); -use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve GetReservesControlBranch ItemsAnyAvailableAndNotRestricted IsAvailableForItemLevelRequest GetReserveFee ); +use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve GetReservesControlBranch IsAvailableForItemLevelRequest GetReserveFee ); use C4::Biblio qw( GetBiblioData GetFrameworkCode ); use C4::Output qw( output_html_with_http_headers ); use C4::Context; @@ -441,8 +441,6 @@ foreach my $biblioNum (@biblionumbers) { # 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 = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblioNum, patron => $patron }) if $patron; foreach my $item (@{$biblioData->{items}}) { my $item_info = $item->unblessed; @@ -481,11 +479,9 @@ foreach my $biblioNum (@biblionumbers) { my $branch = GetReservesControlBranch( $item_info, $patron_unblessed ); - # items_any_available defined outside of the current loop, - # so we avoiding loop inside IsAvailableForItemLevelRequest: my $policy_holdallowed = CanItemBeReserved( $patron, $item, undef, { get_from_cache => 1 } )->{status} eq 'OK' && - IsAvailableForItemLevelRequest($item, $patron, undef, $items_any_available); + IsAvailableForItemLevelRequest($item, $patron, undef); if ($policy_holdallowed) { my $opac_hold_policy = Koha::CirculationRules->get_opacitemholds_policy( { item => $item, patron => $patron } ); diff --git a/reserve/request.pl b/reserve/request.pl index ec75d049e0..f9c51cb4a4 100755 --- a/reserve/request.pl +++ b/reserve/request.pl @@ -33,7 +33,7 @@ use List::MoreUtils qw( uniq ); use Date::Calc qw( Date_to_Days ); use C4::Output qw( output_html_with_http_headers ); use C4::Auth qw( get_template_and_user ); -use C4::Reserves qw( RevertWaitingStatus AlterPriority ToggleLowestPriority ToggleSuspend CanBookBeReserved GetMaxPatronHoldsForRecord ItemsAnyAvailableAndNotRestricted CanItemBeReserved IsAvailableForItemLevelRequest ); +use C4::Reserves qw( RevertWaitingStatus AlterPriority ToggleLowestPriority ToggleSuspend CanBookBeReserved GetMaxPatronHoldsForRecord CanItemBeReserved IsAvailableForItemLevelRequest ); use C4::Items qw( get_hostitemnumbers_of ); use C4::Koha qw( getitemtypeimagelocation ); use C4::Serials qw( CountSubscriptionFromBiblionumber ); @@ -393,9 +393,6 @@ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) # 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 = ItemsAnyAvailableAndNotRestricted( { biblionumber => $biblio->biblionumber, patron => $patron }) - if $patron; for my $item_object ( @items ) { my $do_check; @@ -512,9 +509,7 @@ if ( ( $findborrower && $borrowernumber_hold || $findclub && $club_hold ) !$item->{cantreserve} && !$exceeded_maxreserves && $can_item_be_reserved eq 'OK' - # items_any_available defined outside of the current loop, - # so we avoiding loop inside IsAvailableForItemLevelRequest: - && IsAvailableForItemLevelRequest($item_object, $patron, undef, $items_any_available) + && IsAvailableForItemLevelRequest($item_object, $patron, undef) ) || C4::Context->preference('AllowHoldPolicyOverride') # If AllowHoldPolicyOverride is set, it overrides EVERY restriction # not just branch item rules -- 2.39.5