Bugzilla – Attachment 149377 Details for
Bug 30860
Performance: Add option for CanBookBeReserved to return all item values
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 30860: Cache CanItemBeReserved return value
Bug-30860-Cache-CanItemBeReserved-return-value.patch (text/plain), 11.59 KB, created by
Katrin Fischer
on 2023-04-09 15:56:28 UTC
(
hide
)
Description:
Bug 30860: Cache CanItemBeReserved return value
Filename:
MIME Type:
Creator:
Katrin Fischer
Created:
2023-04-09 15:56:28 UTC
Size:
11.59 KB
patch
obsolete
>From c88d6cc27d6e202b54695274700a0179abfe97f3 Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >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 <nick@bywatersolutions.com> >Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de> >--- > 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 30860
:
135426
|
135427
|
135428
|
135429
|
144480
|
144481
|
144482
|
144483
|
144506
|
144507
|
144508
|
144509
|
145577
|
145578
|
145579
|
145580
|
145581
|
145582
|
145583
|
145584
|
147695
|
148562
|
148812
|
149124
| 149377