Bugzilla – Attachment 152981 Details for
Bug 34178
Add caching of ItemsAnyAvailableAndNotRestricted to IsAvailableForItemLevelRequest
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate
Bug-34178-Cache-ItemsAnyAvailableAndNotRestricted-.patch (text/plain), 9.93 KB, created by
Sam Lau
on 2023-07-03 14:57:53 UTC
(
hide
)
Description:
Bug 34178: Cache ItemsAnyAvailableAndNotRestricted in memory and don't precalculate
Filename:
MIME Type:
Creator:
Sam Lau
Created:
2023-07-03 14:57:53 UTC
Size:
9.93 KB
patch
obsolete
>From 15b70917655f17440a652360f0b5f3cbd7833118 Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >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 <samalau@gmail.com> >--- > 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 4fdc525330..f9450d86d5 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; >@@ -3050,17 +3050,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.30.2
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=152981">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 34178
:
152913
|
152914
|
152981
|
152982
|
153628
|
153629
|
153630