From 1fc2c5e347440ae04fcbd738a76da19b7f51356c Mon Sep 17 00:00:00 2001 From: Nick Clemens 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 Signed-off-by: Jonathan Druart 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 Signed-off-by: Jonathan Druart 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 Signed-off-by: Jonathan Druart 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 Signed-off-by: Jonathan Druart --- 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