Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate

kidclamp · tomascohen · commit 105750acb100 · 2023-07-19T13:00:42.000-03:00
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 &lt;samalau@gmail.com&gt;
Signed-off-by: Martin Renvoize &lt;martin.renvoize@ptfs-europe.com&gt;
Signed-off-by: Tomas Cohen Arazi &lt;tomascohen@theke.io&gt;
diff --git 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
@@ -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
@@ -25,7 +25,7 @@
 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 @@
     # 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 @@
 
         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
@@ -33,7 +33,7 @@ =head1 request.pl
 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 @@ =head1 request.pl
             # 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 @@ =head1 request.pl
                                !$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
