Bugzilla – Attachment 138058 Details for
Bug 30085
Improve performance of CanItemBeReserved
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 30085: [21.11.x] Reduce scope of holds count / today holds count
Bug-30085-2111x-Reduce-scope-of-holds-count--today.patch (text/plain), 10.91 KB, created by
Nick Clemens (kidclamp)
on 2022-07-22 19:29:41 UTC
(
hide
)
Description:
Bug 30085: [21.11.x] Reduce scope of holds count / today holds count
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2022-07-22 19:29:41 UTC
Size:
10.91 KB
patch
obsolete
>From 1fc2c5e347440ae04fcbd738a76da19b7f51356c Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Fri, 11 Feb 2022 19:21:22 +0000 >Subject: [PATCH] Bug 30085: [21.11.x] Reduce scope of holds count / today > holds count > >We retrieve two counts that are only needed if rules for hold limits are defined. >The DB counts should only be fetched once the rules are confirmed to exist > >Further improvement would be possiblke by allowing them to be passed in (or cached?) >from CanBookBeReserved as they rely only on patron/biblionumber and not item specific information > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> > >Bug 30085: Don't fetch ReservesControlBranch twice > >We essentially copy the code from GetReservesControlBranch here, because we >also calculate 'branchfield' > >Setting it to "" vs undef makes no difference in this code, so lets not fetch this >again later. > >Rename the variable to make it clearer where the branchcode came from > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> > >Bug 30085: Consolidate querycoutn code and only check if needed > >Similar to first patch, move a count only conditionally used into >the conditional > >This could be updated to use DBIC, but the itemtype conditionals >add complexity > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> > >Bug 30085: Move IndependentBranches check sooner > >This doesn't rely on the other statuses, and requires only cached >preference check, so lets allow the possibiliy of an early exit > >Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> > >Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >--- > C4/Reserves.pm | 144 ++++++++++++++++++++++--------------------------- > 1 file changed, 64 insertions(+), 80 deletions(-) > >diff --git a/C4/Reserves.pm b/C4/Reserves.pm >index 423e1494a2..769f232eb9 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -414,6 +414,16 @@ sub CanItemBeReserved { > my $patron = Koha::Patrons->find( $borrowernumber ); > my $borrower = $patron->unblessed; > >+ # We check item branch if IndependentBranches is ON >+ # and canreservefromotherbranches is OFF >+ if ( C4::Context->preference('IndependentBranches') >+ and !C4::Context->preference('canreservefromotherbranches') ) >+ { >+ if ( $item->homebranch ne $patron->branchcode ) { >+ return { status => 'cannotReserveFromOtherBranches' }; >+ } >+ } >+ > # If an item is damaged and we don't allow holds on damaged items, we can stop right here > return { status =>'damaged' } > if ( $item->damaged >@@ -439,25 +449,16 @@ sub CanItemBeReserved { > > my $controlbranch = C4::Context->preference('ReservesControlBranch'); > >- my $querycount = q{ >- SELECT count(*) AS count >- FROM reserves >- LEFT JOIN items USING (itemnumber) >- LEFT JOIN biblioitems ON (reserves.biblionumber=biblioitems.biblionumber) >- LEFT JOIN borrowers USING (borrowernumber) >- WHERE borrowernumber = ? >- }; >- >- my $branchcode = ""; >+ my $reserves_control_branch; > my $branchfield = "reserves.branchcode"; > > if ( $controlbranch eq "ItemHomeLibrary" ) { > $branchfield = "items.homebranch"; >- $branchcode = $item->homebranch; >+ $reserves_control_branch = $item->homebranch; > } > elsif ( $controlbranch eq "PatronLibrary" ) { > $branchfield = "borrowers.branchcode"; >- $branchcode = $borrower->{branchcode}; >+ $reserves_control_branch = $borrower->{branchcode}; > } > > # we retrieve rights >@@ -465,7 +466,7 @@ sub CanItemBeReserved { > my $reservesallowed = Koha::CirculationRules->get_effective_rule({ > itemtype => $item->effective_itemtype, > categorycode => $borrower->{categorycode}, >- branchcode => $branchcode, >+ branchcode => $reserves_control_branch, > rule_name => 'reservesallowed', > }) > ) { >@@ -479,81 +480,76 @@ sub CanItemBeReserved { > my $rights = Koha::CirculationRules->get_effective_rules({ > categorycode => $borrower->{'categorycode'}, > itemtype => $item->effective_itemtype, >- branchcode => $branchcode, >+ branchcode => $reserves_control_branch, > rules => ['holds_per_record','holds_per_day'] > }); > my $holds_per_record = $rights->{holds_per_record} // 1; > my $holds_per_day = $rights->{holds_per_day}; > >- my $search_params = { >- borrowernumber => $borrowernumber, >- biblionumber => $item->biblionumber, >- }; >- $search_params->{found} = undef if $params->{ignore_found_holds}; >- >- my $holds = Koha::Holds->search($search_params); > if ( defined $holds_per_record && $holds_per_record ne '' ){ > if ( $holds_per_record == 0 ) { > return { status => "noReservesAllowed" }; > } >- if ( !$params->{ignore_hold_counts} && $holds->count() >= $holds_per_record ) { >- return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record }; >+ if ( !$params->{ignore_hold_counts} ) { >+ my $search_params = { >+ borrowernumber => $borrowernumber, >+ biblionumber => $item->biblionumber, >+ }; >+ $search_params->{found} = undef if $params->{ignore_found_holds}; >+ my $holds = Koha::Holds->search($search_params); >+ return { status => "tooManyHoldsForThisRecord", limit => $holds_per_record } if $holds->count() >= $holds_per_record; > } > } > >- my $today_holds = Koha::Holds->search({ >- borrowernumber => $borrowernumber, >- reservedate => dt_from_string->date >- }); >- >- if (!$params->{ignore_hold_counts} && defined $holds_per_day && $holds_per_day ne '' >- && $today_holds->count() >= $holds_per_day ) >+ if (!$params->{ignore_hold_counts} && defined $holds_per_day && $holds_per_day ne '') > { >- return { status => 'tooManyReservesToday', limit => $holds_per_day }; >+ my $today_holds = Koha::Holds->search({ >+ borrowernumber => $borrowernumber, >+ reservedate => dt_from_string->date >+ }); >+ return { status => 'tooManyReservesToday', limit => $holds_per_day } if $today_holds->count() >= $holds_per_day; > } > >- # we retrieve count >- >- $querycount .= "AND ( $branchfield = ? OR $branchfield IS NULL )"; >- >- # If using item-level itypes, fall back to the record >- # level itemtype if the hold has no associated item >- if ( defined $ruleitemtype ) { >- if ( C4::Context->preference('item-level_itypes') ) { >- $querycount .= q{ >- AND ( COALESCE( items.itype, biblioitems.itemtype ) = ? >- OR reserves.itemtype = ? ) >- }; >+ # we check if it's ok or not >+ if ( defined $allowedreserves && $allowedreserves ne '' ){ >+ if( $allowedreserves == 0 ){ >+ return { status => 'noReservesAllowed' }; > } >- else { >- $querycount .= q{ >- AND ( biblioitems.itemtype = ? >- OR reserves.itemtype = ? ) >+ if ( !$params->{ignore_hold_counts} ) { >+ # we retrieve count >+ my $querycount = q{ >+ SELECT count(*) AS count >+ FROM reserves >+ LEFT JOIN items USING (itemnumber) >+ LEFT JOIN biblioitems ON (reserves.biblionumber=biblioitems.biblionumber) >+ LEFT JOIN borrowers USING (borrowernumber) >+ WHERE borrowernumber = ? > }; >- } >- } >+ $querycount .= "AND ( $branchfield = ? OR $branchfield IS NULL )"; > >- my $sthcount = $dbh->prepare($querycount); >+ # If using item-level itypes, fall back to the record >+ # level itemtype if the hold has no associated item >+ $querycount .= >+ C4::Context->preference('item-level_itypes') >+ ? " AND ( COALESCE( items.itype, biblioitems.itemtype ) = ? OR reserves.itemtype = ? )" >+ : " AND ( biblioitems.itemtype = ? OR reserves.itemtype = ? )" >+ if defined $ruleitemtype; > >- if ( defined $ruleitemtype ) { >- $sthcount->execute( $borrowernumber, $branchcode, $ruleitemtype, $ruleitemtype ); >- } >- else { >- $sthcount->execute( $borrowernumber, $branchcode ); >- } >+ my $sthcount = $dbh->prepare($querycount); > >- my $reservecount = "0"; >- if ( my $rowcount = $sthcount->fetchrow_hashref() ) { >- $reservecount = $rowcount->{count}; >- } >+ if ( defined $ruleitemtype ) { >+ $sthcount->execute( $borrowernumber, $reserves_control_branch, $ruleitemtype, $ruleitemtype ); >+ } >+ else { >+ $sthcount->execute( $borrowernumber, $reserves_control_branch ); >+ } > >- # we check if it's ok or not >- if ( defined $allowedreserves && $allowedreserves ne '' ){ >- if( $allowedreserves == 0 ){ >- return { status => 'noReservesAllowed' }; >- } >- if ( !$params->{ignore_hold_counts} && $reservecount >= $allowedreserves ) { >- return { status => 'tooManyReserves', limit => $allowedreserves }; >+ my $reservecount = "0"; >+ if ( my $rowcount = $sthcount->fetchrow_hashref() ) { >+ $reservecount = $rowcount->{count}; >+ } >+ >+ return { status => 'tooManyReserves', limit => $allowedreserves } if $reservecount >= $allowedreserves; > } > } > >@@ -561,7 +557,7 @@ sub CanItemBeReserved { > my $rule = Koha::CirculationRules->get_effective_rule( > { > categorycode => $borrower->{categorycode}, >- branchcode => $branchcode, >+ branchcode => $reserves_control_branch, > rule_name => 'max_holds', > } > ); >@@ -575,8 +571,6 @@ sub CanItemBeReserved { > return { status => 'tooManyReserves', limit => $rule->rule_value} if $total_holds_count >= $rule->rule_value; > } > >- my $reserves_control_branch = >- GetReservesControlBranch( $item->unblessed(), $borrower ); > my $branchitemrule = > C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->effective_itemtype ); > >@@ -597,16 +591,6 @@ sub CanItemBeReserved { > } > } > >- # If reservecount is ok, we check item branch if IndependentBranches is ON >- # and canreservefromotherbranches is OFF >- if ( C4::Context->preference('IndependentBranches') >- and !C4::Context->preference('canreservefromotherbranches') ) >- { >- if ( $item->homebranch ne $borrower->{branchcode} ) { >- return { status => 'cannotReserveFromOtherBranches' }; >- } >- } >- > if ($pickup_branchcode) { > my $destination = Koha::Libraries->find({ > branchcode => $pickup_branchcode, >-- >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 30085
:
130525
|
130526
|
130527
|
130528
|
130829
|
130830
|
130831
|
130832
|
130856
|
130857
|
130858
|
130859
| 138058