From 302ce950044dcf6b4e1702800e8a4c112e9a79c2 Mon Sep 17 00:00:00 2001 From: David Gustafsson Date: Thu, 3 Nov 2022 14:26:34 +0100 Subject: [PATCH] Bug 32092: Improve circulation rules cache utilization To test: 1) Go to the page for placing a hold for a biblio with a large number of items 2) Depending on the number of items this page can be very slow to load 3) Ensure the following tests pass: t/db_dependent/Circulation.t t/db_dependent/Reserves.t t/db_dependent/Biblio.t t/db_dependent/Koha/Charges/Fees.t t/db_dependent/Items.t t/db_dependent/Koha/Patron.t t/db_dependent/api/v1/checkouts.t t/db_dependent/Koha/Template/Plugin/CirculationRules.t t/db_dependent/Template/Plugin/CirculationRules.t t/db_dependent/Koha/CirculationRules.t t/db_dependent/Circulation/Branch.t t/db_dependent/Circulation/TooMany.t 3) Apply the patch 4) The page loading time should now be significantly faster 5) Ensure all the tests in 3) still pass Sponsored-by: Gothenburg University Library Signed-off-by: Emily Lamancusa Signed-off-by: Kyle M Hall --- C4/Circulation.pm | 86 ++++++++++-------------- C4/Reserves.pm | 76 ++++++++++----------- Koha/Biblio.pm | 28 +++----- Koha/Charges/Fees.pm | 7 +- Koha/CirculationRules.pm | 36 +++++----- Koha/Item.pm | 17 ++--- Koha/Patron.pm | 10 +-- Koha/REST/V1/Checkouts.pm | 6 +- Koha/Recall.pm | 10 ++- Koha/Recalls.pm | 9 +-- Koha/Template/Plugin/CirculationRules.pm | 4 +- misc/cronjobs/recalls/expire_recalls.pl | 6 +- opac/opac-recall.pl | 7 +- t/db_dependent/Koha/CirculationRules.t | 6 ++ 14 files changed, 133 insertions(+), 175 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index 0a8df342638..30cf1324175 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -438,9 +438,8 @@ sub TooMany { my $branch = _GetCircControlBranch( $item, $patron ); my $type = $item->effective_itemtype; - my ( $type_object, $parent_type, $parent_maxissueqty_rule ); - $type_object = Koha::ItemTypes->find($type); - $parent_type = $type_object->parent_type if $type_object; + my $type_object = Koha::ItemTypes->find($type); + my $parent_type = $type_object->parent_type if $type_object; my $child_types = Koha::ItemTypes->search( { parent_type => $type } ); # Find any children if we are a parent_type; @@ -448,6 +447,7 @@ sub TooMany { # given branch, patron category, and item type, determine # applicable issuing rule + my $parent_maxissueqty_rule; $parent_maxissueqty_rule = Koha::CirculationRules->get_effective_rule( { categorycode => $cat_borrower, @@ -499,10 +499,9 @@ sub TooMany { $checkouts = $checkouts->search( undef, { prefetch => 'item' } ); my $sum_checkouts; - my $rule_itemtype = $maxissueqty_rule->itemtype; my @types; - unless ( $rule_itemtype || $parent_maxissueqty_rule ) { + unless ( $maxissueqty_rule->itemtype || $parent_maxissueqty_rule ) { # matching rule has the default item type, so count only # those existing loans that don't fall under a more @@ -516,7 +515,7 @@ sub TooMany { } )->get_column('itemtype'); } else { - if ($parent_maxissueqty_rule) { + if ( defined $parent_maxissueqty_rule ) { # if we have a parent item type then we count loans of the # specific item type or its siblings or parent @@ -538,7 +537,7 @@ sub TooMany { while ( my $c = $checkouts->next ) { my $itemtype = $c->item->effective_itemtype; - unless ( $rule_itemtype || $parent_maxissueqty_rule ) { + unless ( $maxissueqty_rule->itemtype || $parent_maxissueqty_rule ) { next if grep { $_ eq $itemtype } @types; } else { next unless grep { $_ eq $itemtype } @types; @@ -565,9 +564,8 @@ sub TooMany { }; # If parent rules exists - if ( defined($parent_maxissueqty_rule) and defined( $parent_maxissueqty_rule->rule_value ) ) { - $checkout_rules->{max_checkouts_allowed} = - $parent_maxissueqty_rule ? $parent_maxissueqty_rule->rule_value : undef; + if ( defined $parent_maxissueqty_rule ) { + $checkout_rules->{max_checkouts_allowed} = $parent_maxissueqty_rule->rule_value; my $qty_over = _check_max_qty($checkout_rules); return $qty_over if defined $qty_over; @@ -576,7 +574,7 @@ sub TooMany { && defined( $maxissueqty_rule->itemtype ) ) { $checkout_rules->{checkout_count} = $checkout_count_type; - $checkout_rules->{max_checkouts_allowed} = $maxissueqty_rule ? $maxissueqty_rule->rule_value : undef; + $checkout_rules->{max_checkouts_allowed} = $maxissueqty_rule->rule_value; my $qty_over = _check_max_qty($checkout_rules); return $qty_over if defined $qty_over; } @@ -623,7 +621,7 @@ sub TooMany { return $qty_over if defined $qty_over; } - if ( not defined($maxissueqty_rule) and not defined( $branch_borrower_circ_rule->{patron_maxissueqty} ) ) { + unless ( defined $maxissueqty_rule || defined $branch_borrower_circ_rule->{patron_maxissueqty} ) { return { reason => 'NO_RULE_DEFINED', max_allowed => 0 }; } @@ -643,7 +641,7 @@ sub _check_max_qty { my $onsite_circulation_rule = $params->{onsite_circulation_rule}; if ( $onsite_checkout and defined $max_onsite_checkouts_allowed ) { - if ( $max_onsite_checkouts_allowed eq '' ) { return; } + return if $max_onsite_checkouts_allowed eq ''; if ( $onsite_checkout_count >= $max_onsite_checkouts_allowed ) { return { reason => 'TOO_MANY_ONSITE_CHECKOUTS', @@ -654,7 +652,7 @@ sub _check_max_qty { } } if ( C4::Context->preference('ConsiderOnSiteCheckoutsAsNormalCheckouts') ) { - if ( $max_checkouts_allowed eq '' ) { return; } + return if $max_checkouts_allowed eq ''; my $delta = $switch_onsite_checkout ? 1 : 0; if ( $checkout_count >= $max_checkouts_allowed + $delta ) { return { @@ -665,7 +663,7 @@ sub _check_max_qty { }; } } elsif ( not $onsite_checkout ) { - if ( $max_checkouts_allowed eq '' ) { return; } + return if $max_checkouts_allowed eq ''; if ( $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed ) { return { reason => 'TOO_MANY_CHECKOUTS', @@ -1520,23 +1518,16 @@ sub checkHighHolds { my $orig_due = C4::Circulation::CalcDateDue( $issuedate, $itype, $branchcode, $patron ); - my $rule = Koha::CirculationRules->get_effective_rule_value( + # overrides decreaseLoanHighHoldsDuration syspref + my $duration = Koha::CirculationRules->get_effective_rule_value( { categorycode => $patron->categorycode, itemtype => $item->effective_itemtype, branchcode => $branchcode, rule_name => 'decreaseloanholds', } - ); - - my $duration; - if ( defined($rule) && $rule ne '' ) { + ) || C4::Context->preference('decreaseLoanHighHoldsDuration'); - # overrides decreaseLoanHighHoldsDuration syspref - $duration = $rule; - } else { - $duration = C4::Context->preference('decreaseLoanHighHoldsDuration') || 0; - } my $reduced_datedue = $calendar->addDuration( $issuedate, $duration ); $reduced_datedue->set_hour( $orig_due->hour ); $reduced_datedue->set_minute( $orig_due->minute ); @@ -2097,27 +2088,21 @@ wildcards. sub GetBranchBorrowerCircRule { my ( $branchcode, $categorycode ) = @_; - # Initialize default values - my $rules = { + my $rules = Koha::CirculationRules->get_effective_rules( + { + categorycode => $categorycode, + itemtype => undef, + branchcode => $branchcode, + rules => [ 'patron_maxissueqty', 'patron_maxonsiteissueqty' ] + } + ) // {}; + + # Initialize default values and return + return { patron_maxissueqty => undef, patron_maxonsiteissueqty => undef, + %{$rules} }; - - # Search for rules! - foreach my $rule_name (qw( patron_maxissueqty patron_maxonsiteissueqty )) { - my $rule = Koha::CirculationRules->get_effective_rule( - { - categorycode => $categorycode, - itemtype => undef, - branchcode => $branchcode, - rule_name => $rule_name, - } - ); - - $rules->{$rule_name} = $rule->rule_value if defined $rule; - } - - return $rules; } =head2 GetBranchItemRule @@ -2909,12 +2894,11 @@ sub _calculate_new_debar_dt { ] } ); - my $finedays = $issuing_rule ? $issuing_rule->{finedays} : undef; - my $unit = $issuing_rule ? $issuing_rule->{lengthunit} : undef; + return unless $issuing_rule->{finedays}; + my $finedays = $issuing_rule->{finedays}; + my $unit = $issuing_rule->{lengthunit}; my $chargeable_units = C4::Overdues::get_chargeable_units( $unit, $dt_due, $return_date, $branchcode ); - return unless $finedays; - # finedays is in days, so hourly loans must multiply by 24 # thus 1 hour late equals 1 day suspension * finedays rate $finedays = $finedays * 24 if ( $unit eq 'hours' ); @@ -3493,7 +3477,7 @@ sub AddRenewal { # a maximum value has been set in the circ rules my $unseen_renewals = $issue->unseen_renewals; if ( C4::Context->preference('UnseenRenewals') ) { - my $rule = Koha::CirculationRules->get_effective_rule( + my $unseen_renewals_allowed = Koha::CirculationRules->get_effective_rule_value( { categorycode => $patron->categorycode, itemtype => $item_object->effective_itemtype, @@ -3501,7 +3485,7 @@ sub AddRenewal { rule_name => 'unseen_renewals_allowed' } ); - if ( !$seen && $rule && looks_like_number( $rule->rule_value ) ) { + if ( !$seen && $unseen_renewals_allowed && looks_like_number($unseen_renewals_allowed) ) { $unseen_renewals++; push @{$confirmations}, 'UNSEEN'; } else { @@ -3907,7 +3891,7 @@ sub GetIssuingCharges { # FIXME This should follow CircControl my $branch = C4::Context::mybranch(); $patron //= Koha::Patrons->find($borrowernumber); - my $discount = Koha::CirculationRules->get_effective_rule( + my $discount = Koha::CirculationRules->get_effective_rule_value( { categorycode => $patron->categorycode, branchcode => $branch, @@ -3916,7 +3900,7 @@ sub GetIssuingCharges { } ); if ($discount) { - $charge = ( $charge * ( 100 - $discount->rule_value ) ) / 100; + $charge = ( $charge * ( 100 - $discount ) ) / 100; } $charge = sprintf '%.2f', $charge; # ensure no fractions of a penny returned } diff --git a/C4/Reserves.pm b/C4/Reserves.pm index c4eb3b7ff69..b070ae9768a 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -397,16 +397,16 @@ sub CanBookBeReserved { # biblio-level, item type-contrained my $patron = Koha::Patrons->find($borrowernumber); - my $reservesallowed = Koha::CirculationRules->get_effective_rule( + my $reservesallowed = Koha::CirculationRules->get_effective_rule_value( { itemtype => $params->{itemtype}, categorycode => $patron->categorycode, branchcode => $pickup_branchcode, rule_name => 'reservesallowed', } - )->rule_value; + ); - $reservesallowed = ( $reservesallowed eq '' ) ? undef : $reservesallowed; + $reservesallowed = $reservesallowed eq '' ? undef : $reservesallowed; my $count = $patron->holds->count_holds( { @@ -492,9 +492,9 @@ sub CanItemBeReserved { 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 + my $dbh = C4::Context->dbh; + my $effective_itemtype = $item->effective_itemtype; + my $allowedreserves = 0; # Total number of holds allowed across all records, default to none # We check item branch if IndependentBranches is ON # and canreservefromotherbranches is OFF @@ -534,8 +534,11 @@ sub CanItemBeReserved { } # check if a recall exists on this item from this borrower - return _cache { status => 'recall' } - if $patron->recalls->filter_by_current->search( { item_id => $item->itemnumber } )->count; + if ( C4::Context->preference('UseRecalls') + && $patron->recalls->filter_by_current->search( { item_id => $item->itemnumber } )->count ) + { + return _cache { status => 'recall' }; + } my $controlbranch = C4::Context->preference('ReservesControlBranch'); @@ -550,28 +553,20 @@ sub CanItemBeReserved { $reserves_control_branch = $patron->branchcode; } - # we retrieve rights - if ( - my $reservesallowed = Koha::CirculationRules->get_effective_rule( - { - itemtype => $item->effective_itemtype, - categorycode => $patron->categorycode, - branchcode => $reserves_control_branch, - rule_name => 'reservesallowed', - } - ) - ) - { - $ruleitemtype = $reservesallowed->itemtype; - $allowedreserves = $reservesallowed->rule_value // 0; #undefined is 0, blank is unlimited - } else { - $ruleitemtype = undef; - } + #undefined is 0, blank is unlimited + $allowedreserves = Koha::CirculationRules->get_effective_rule_value( + { + itemtype => $effective_itemtype, + categorycode => $patron->categorycode, + branchcode => $reserves_control_branch, + rule_name => 'reservesallowed' + } + ) // 0; my $rights = Koha::CirculationRules->get_effective_rules( { categorycode => $patron->categorycode, - itemtype => $item->effective_itemtype, + itemtype => $effective_itemtype, branchcode => $reserves_control_branch, rules => [ 'holds_per_record', 'holds_per_day' ] } @@ -606,7 +601,7 @@ sub CanItemBeReserved { } # we check if it's ok or not - if ( defined $allowedreserves && $allowedreserves ne '' ) { + if ( $allowedreserves ne '' ) { if ( $allowedreserves == 0 ) { return _cache { status => 'noReservesAllowed' }; } @@ -625,7 +620,7 @@ sub CanItemBeReserved { # If using item-level itypes, fall back to the record # level itemtype if the hold has no associated item - if ( defined $ruleitemtype ) { + if ( defined $effective_itemtype ) { if ( C4::Context->preference('item-level_itypes') ) { $querycount .= q{ AND ( COALESCE( items.itype, biblioitems.itemtype ) = ? @@ -640,14 +635,16 @@ sub CanItemBeReserved { } my $sthcount = $dbh->prepare($querycount); - - if ( defined $ruleitemtype ) { - $sthcount->execute( $patron->borrowernumber, $reserves_control_branch, $ruleitemtype, $ruleitemtype ); + if ( defined $effective_itemtype ) { + $sthcount->execute( + $patron->borrowernumber, $reserves_control_branch, $effective_itemtype, + $effective_itemtype + ); } else { $sthcount->execute( $patron->borrowernumber, $reserves_control_branch ); } - my $reservecount = "0"; + my $reservecount = 0; if ( my $rowcount = $sthcount->fetchrow_hashref() ) { $reservecount = $rowcount->{count}; } @@ -658,21 +655,21 @@ sub CanItemBeReserved { } # Now we need to check hold limits by patron category - my $rule = Koha::CirculationRules->get_effective_rule( + my $max_holds = Koha::CirculationRules->get_effective_rule_value( { categorycode => $patron->categorycode, branchcode => $reserves_control_branch, rule_name => 'max_holds', } ); - if ( !$params->{ignore_hold_counts} && $rule && defined( $rule->rule_value ) && $rule->rule_value ne '' ) { + if ( !$params->{ignore_hold_counts} && defined $max_holds && $max_holds ne '' ) { my $total_holds_count = Koha::Holds->search( { borrowernumber => $patron->borrowernumber } )->count(); - return _cache { status => 'tooManyReserves', limit => $rule->rule_value } - if $total_holds_count >= $rule->rule_value; + return _cache { status => 'tooManyReserves', limit => $max_holds } + if $total_holds_count >= $max_holds; } - my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $item->effective_itemtype ); + my $branchitemrule = C4::Circulation::GetBranchItemRule( $reserves_control_branch, $effective_itemtype ); if ( $branchitemrule->{holdallowed} eq 'not_allowed' ) { return _cache { status => 'notReservable' }; @@ -2409,15 +2406,14 @@ sub GetMaxPatronHoldsForRecord { $branchcode = $item->homebranch if ( $controlbranch eq "ItemHomeLibrary" ); - my $rule = Koha::CirculationRules->get_effective_rule( + my $holds_per_record = Koha::CirculationRules->get_effective_rule_value( { categorycode => $categorycode, itemtype => $itemtype, branchcode => $branchcode, rule_name => 'holds_per_record' } - ); - my $holds_per_record = $rule ? $rule->rule_value : 0; + ) // 0; $max = $holds_per_record if $holds_per_record > $max; } diff --git a/Koha/Biblio.pm b/Koha/Biblio.pm index 67ea254c63d..f5f19ca2931 100644 --- a/Koha/Biblio.pm +++ b/Koha/Biblio.pm @@ -566,16 +566,13 @@ $borrower must be a Koha::Patron object sub article_request_type { my ( $self, $borrower ) = @_; - return q{} unless $borrower; + return unless $borrower; - my $rule = $self->article_request_type_for_items($borrower); - return $rule if $rule; + my $type = $self->article_request_type_for_items( $borrower ); + return $type if $type; # If the record has no items that are requestable, go by the record itemtype - $rule = $self->article_request_type_for_bib($borrower); - return $rule if $rule; - - return q{}; + return $self->article_request_type_for_bib($borrower); } =head3 article_request_type_for_bib @@ -589,21 +586,18 @@ Returns the article request type 'yes', 'no', 'item_only', 'bib_only', for the g sub article_request_type_for_bib { my ( $self, $borrower ) = @_; - return q{} unless $borrower; + return unless $borrower; my $borrowertype = $borrower->categorycode; my $itemtype = $self->itemtype(); - my $rule = Koha::CirculationRules->get_effective_rule( + return Koha::CirculationRules->get_effective_rule_value( { rule_name => 'article_requests', categorycode => $borrowertype, itemtype => $itemtype, } ); - - return q{} unless $rule; - return $rule->rule_value || q{}; } =head3 article_request_type_for_items @@ -2207,9 +2201,9 @@ sub can_be_recalled { ], } ); - push( @recalls_allowed, $rule->{recalls_allowed} ) if $rule; - push( @recalls_per_record, $rule->{recalls_per_record} ) if $rule; - push( @on_shelf_recalls, $rule->{on_shelf_recalls} ) if $rule; + push( @recalls_allowed, $rule->{recalls_allowed} ) if $rule->{recalls_allowed}; + push( @recalls_per_record, $rule->{recalls_per_record} ) if $rule->{recalls_per_record}; + push( @on_shelf_recalls, $rule->{on_shelf_recalls} ) if $rule->{on_shelf_recalls}; } my $recalls_allowed = ( sort { $b <=> $a } @recalls_allowed )[0]; # take highest my $recalls_per_record = ( sort { $b <=> $a } @recalls_per_record )[0]; # take highest @@ -2220,8 +2214,8 @@ sub can_be_recalled { my $on_shelf_recalls = ( sort { $on_shelf_recalls_count{$b} <=> $on_shelf_recalls_count{$a} } @on_shelf_recalls )[0] ; # take most common - # check recalls allowed has been set and is not zero - return 0 if ( !defined($recalls_allowed) || $recalls_allowed == 0 ); + # check if recalls are not allowed + return 0 if !$recalls_allowed; if ($patron) { diff --git a/Koha/Charges/Fees.pm b/Koha/Charges/Fees.pm index 53ff799d1cf..d23c0702947 100644 --- a/Koha/Charges/Fees.pm +++ b/Koha/Charges/Fees.pm @@ -88,8 +88,8 @@ sub new { sub accumulate_rentalcharge { my ($self) = @_; - my $itemtype = Koha::ItemTypes->find( $self->item->effective_itemtype ); - my $lengthunit_rule = Koha::CirculationRules->get_effective_rule( + my $itemtype = Koha::ItemTypes->find( $self->item->effective_itemtype ); + my $units = Koha::CirculationRules->get_effective_rule_value( { categorycode => $self->patron->categorycode, itemtype => $itemtype->id, @@ -97,9 +97,8 @@ sub accumulate_rentalcharge { rule_name => 'lengthunit', } ); - return 0 unless $lengthunit_rule; + return 0 unless $units; - my $units = $lengthunit_rule->rule_value; my $rentalcharge_increment = ( $units eq 'days' ) ? $itemtype->rentalcharge_daily diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm index b0b2bd00c2b..8b9d3134b5b 100644 --- a/Koha/CirculationRules.pm +++ b/Koha/CirculationRules.pm @@ -323,13 +323,18 @@ sub get_effective_rule_value { my $cache_key = sprintf "CircRules:%s:%s:%s:%s", $rule_name // q{}, $categorycode // q{}, $branchcode // q{}, $itemtype // q{}; - my $cached = $memory_cache->get_from_cache($cache_key); - return $cached if $cached; - - my $rule = $self->get_effective_rule($params); - - my $value = $rule ? $rule->rule_value : undef; - $memory_cache->set_in_cache( $cache_key, $value ); + my $value = $memory_cache->get_from_cache($cache_key); + unless (defined $value) { + my $rule = $self->get_effective_rule($params); + if ($rule) { + $value = $rule->rule_value; + } + else { + $value = 'undef'; + } + $memory_cache->set_in_cache( $cache_key, $value ); + } + return if $value eq 'undef'; return $value; } @@ -567,7 +572,7 @@ sub get_opacitemholds_policy { return unless $item or $patron; - my $rule = Koha::CirculationRules->get_effective_rule( + return Koha::CirculationRules->get_effective_rule_value( { categorycode => $patron->categorycode, itemtype => $item->effective_itemtype, @@ -575,8 +580,6 @@ sub get_opacitemholds_policy { rule_name => 'opacitemholds', } ); - - return $rule ? $rule->rule_value : undef; } =head3 get_onshelfholds_policy @@ -590,15 +593,14 @@ sub get_onshelfholds_policy { my $item = $params->{item}; my $itemtype = $item->effective_itemtype; my $patron = $params->{patron}; - my $rule = Koha::CirculationRules->get_effective_rule( + return Koha::CirculationRules->get_effective_rule_value( { categorycode => ( $patron ? $patron->categorycode : undef ), itemtype => $itemtype, branchcode => $item->holdingbranch, rule_name => 'onshelfholds', } - ); - return $rule ? $rule->rule_value : 0; + ) // 0; } =head3 get_lostreturn_policy @@ -744,7 +746,7 @@ sub get_effective_daysmode { my $itemtype = $params->{itemtype}; my $branchcode = $params->{branchcode}; - my $daysmode_rule = $class->get_effective_rule( + my $daysmode = $class->get_effective_rule_value( { categorycode => $categorycode, itemtype => $itemtype, @@ -752,11 +754,7 @@ sub get_effective_daysmode { rule_name => 'daysmode', } ); - - return ( defined($daysmode_rule) and $daysmode_rule->rule_value ne '' ) - ? $daysmode_rule->rule_value - : C4::Context->preference('useDaysMode'); - + return $daysmode || C4::Context->preference('useDaysMode'); } =head3 get_effective_expire_reserves_charge diff --git a/Koha/Item.pm b/Koha/Item.pm index d2b92c19568..bd119af5c88 100644 --- a/Koha/Item.pm +++ b/Koha/Item.pm @@ -1163,7 +1163,7 @@ sub article_request_type { : undef; my $borrowertype = $borrower->categorycode; my $itemtype = $self->effective_itemtype(); - my $rule = Koha::CirculationRules->get_effective_rule( + return Koha::CirculationRules->get_effective_rule_value( { rule_name => 'article_requests', categorycode => $borrowertype, @@ -1171,9 +1171,6 @@ sub article_request_type { branchcode => $branchcode } ); - - return q{} unless $rule; - return $rule->rule_value || q{}; } =head3 current_holds @@ -2502,14 +2499,15 @@ sub can_be_recalled { } ); - # check recalls allowed has been set and is not zero - return 0 if ( !defined( $rule->{recalls_allowed} ) || $rule->{recalls_allowed} == 0 ); + # check if recalls are not allowed + return 0 if !$rule->{recalls_allowed}; if ($patron) { # check borrower has not reached open recalls allowed limit return 0 if ( $patron->recalls->filter_by_current->count >= $rule->{recalls_allowed} ); + $rule->{recalls_per_record} //= 0; # check borrower has not reach open recalls allowed per record limit return 0 if ( $patron->recalls->filter_by_current->search( { biblio_id => $self->biblionumber } )->count >= @@ -2595,14 +2593,11 @@ sub can_be_waiting_recall { branchcode => $branchcode, categorycode => $most_relevant_recall ? $most_relevant_recall->patron->categorycode : undef, itemtype => $self->effective_itemtype, - rules => [ 'recalls_allowed', ], + rules => [ 'recalls_allowed' ], } ); - # check recalls allowed has been set and is not zero - return 0 if ( !defined( $rule->{recalls_allowed} ) || $rule->{recalls_allowed} == 0 ); - - # can recall + return 0 if !$rule->{recalls_allowed}; return 1; } diff --git a/Koha/Patron.pm b/Koha/Patron.pm index 162ea03630c..a6f15bb4227 100644 --- a/Koha/Patron.pm +++ b/Koha/Patron.pm @@ -1545,7 +1545,7 @@ sub can_request_article { $library_id //= C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef; - my $rule = Koha::CirculationRules->get_effective_rule( + my $limit = Koha::CirculationRules->get_effective_rule_value( { branchcode => $library_id, categorycode => $self->categorycode, @@ -1553,8 +1553,6 @@ sub can_request_article { } ); - my $limit = ($rule) ? $rule->rule_value : undef; - return 1 unless defined $limit; my $count = Koha::ArticleRequests->search( @@ -1590,7 +1588,7 @@ sub article_request_fee { $library_id //= C4::Context->userenv ? C4::Context->userenv->{'branch'} : undef; - my $rule = Koha::CirculationRules->get_effective_rule( + my $value = Koha::CirculationRules->get_effective_rule_value( { branchcode => $library_id, categorycode => $self->categorycode, @@ -1598,9 +1596,7 @@ sub article_request_fee { } ); - my $fee = ($rule) ? $rule->rule_value + 0 : 0; - - return $fee; + return $value ? $value + 0 : 0; } =head3 add_article_request_fee_if_needed diff --git a/Koha/REST/V1/Checkouts.pm b/Koha/REST/V1/Checkouts.pm index 21b7303a903..6fce7246a4f 100644 --- a/Koha/REST/V1/Checkouts.pm +++ b/Koha/REST/V1/Checkouts.pm @@ -423,19 +423,19 @@ sub allows_renewal { my $renewable = Mojo::JSON->false; $renewable = Mojo::JSON->true if $can_renew; - my $rule = Koha::CirculationRules->get_effective_rule( + my $renewalsallowed = Koha::CirculationRules->get_effective_rule_value( { categorycode => $checkout->patron->categorycode, itemtype => $checkout->item->effective_itemtype, branchcode => $checkout->branchcode, - rule_name => 'renewalsallowed', + rule_name => 'renewalsallowed' } ); return $c->render( status => 200, openapi => { allows_renewal => $renewable, - max_renewals => $rule->rule_value, + max_renewals => $renewalsallowed, current_renewals => $checkout->renewals_count, unseen_renewals => $checkout->unseen_renewals, error => $error diff --git a/Koha/Recall.pm b/Koha/Recall.pm index 5ccb4b18ebd..7c8f96a02a7 100644 --- a/Koha/Recall.pm +++ b/Koha/Recall.pm @@ -116,7 +116,7 @@ sub checkout { my @items = Koha::Items->search( { biblionumber => $self->biblio_id } )->as_list; my @itemnumbers; foreach (@items) { - my $recalls_allowed = Koha::CirculationRules->get_effective_rule( + my $recalls_allowed = Koha::CirculationRules->get_effective_rule_value( { branchcode => C4::Context->userenv->{'branch'}, categorycode => $self->patron->categorycode, @@ -124,7 +124,7 @@ sub checkout { rule_name => 'recalls_allowed', } ); - if ( defined $recalls_allowed and $recalls_allowed->rule_value > 0 ) { + if ( defined $recalls_allowed and $recalls_allowed > 0 ) { push( @itemnumbers, $_->itemnumber ); } } @@ -265,16 +265,14 @@ sub calc_expirationdate { $branchcode = C4::Circulation::_GetCircControlBranch( $item, $self->patron ); } - my $rule = Koha::CirculationRules->get_effective_rule( + my $shelf_time = Koha::CirculationRules->get_effective_rule_value( { categorycode => $self->patron->categorycode, branchcode => $branchcode, itemtype => $item ? $item->effective_itemtype : undef, rule_name => 'recall_shelf_time' } - ); - - my $shelf_time = defined $rule ? $rule->rule_value : C4::Context->preference('RecallsMaxPickUpDelay'); + ) // C4::Context->preference('RecallsMaxPickUpDelay'); my $expirationdate = dt_from_string->add( days => $shelf_time ); return $expirationdate; diff --git a/Koha/Recalls.pm b/Koha/Recalls.pm index 2b139f166a7..518702e8cef 100644 --- a/Koha/Recalls.pm +++ b/Koha/Recalls.pm @@ -138,20 +138,17 @@ sub add_recall { # get checkout and adjust due date based on circulation rules my $checkout = $recall->checkout; - my $recall_due_date_interval = Koha::CirculationRules->get_effective_rule( + my $due_interval = Koha::CirculationRules->get_effective_rule_value( { categorycode => $checkout->patron->categorycode, itemtype => $checkout->item->effective_itemtype, branchcode => $branchcode, rule_name => 'recall_due_date_interval', } - ); - my $due_interval = 5; - $due_interval = $recall_due_date_interval->rule_value - if defined $recall_due_date_interval && $recall_due_date_interval->rule_value; + ) // 5; my $timestamp = dt_from_string( $recall->timestamp ); my $checkout_due_date = dt_from_string( $checkout->date_due ); - my $recall_due_date = $timestamp->set( + my $recall_due_date = $timestamp->set( { hour => $checkout_due_date->hour, minute => $checkout_due_date->minute, second => $checkout_due_date->second diff --git a/Koha/Template/Plugin/CirculationRules.pm b/Koha/Template/Plugin/CirculationRules.pm index d19c27b543b..287d8b63be4 100644 --- a/Koha/Template/Plugin/CirculationRules.pm +++ b/Koha/Template/Plugin/CirculationRules.pm @@ -46,7 +46,7 @@ sub Get { $categorycode = undef if $categorycode eq q{} or $categorycode eq q{*}; $itemtype = undef if $itemtype eq q{} or $itemtype eq q{*}; - my $rule = Koha::CirculationRules->get_effective_rule( + return Koha::CirculationRules->get_effective_rule_value( { branchcode => $branchcode, categorycode => $categorycode, @@ -54,8 +54,6 @@ sub Get { rule_name => $rule_name, } ); - - return $rule->rule_value if $rule; } =head3 Search diff --git a/misc/cronjobs/recalls/expire_recalls.pl b/misc/cronjobs/recalls/expire_recalls.pl index cec8f76537f..fd58df0111b 100755 --- a/misc/cronjobs/recalls/expire_recalls.pl +++ b/misc/cronjobs/recalls/expire_recalls.pl @@ -51,7 +51,7 @@ while ( my $recall = $recalls->next ) { } } if ( $recall->waiting ) { - my $recall_shelf_time = Koha::CirculationRules->get_effective_rule( + my $recall_shelf_time = Koha::CirculationRules->get_effective_rule_value( { categorycode => $recall->patron->categorycode, itemtype => $recall->item->effective_itemtype, @@ -61,8 +61,8 @@ while ( my $recall = $recalls->next ) { ); my $waitingdate = dt_from_string( $recall->waiting_date )->truncate( to => 'day' ); my $days_waiting = $today->subtract_datetime($waitingdate); - if ( defined $recall_shelf_time and $recall_shelf_time->rule_value >= 0 ) { - if ( $days_waiting->days > $recall_shelf_time->rule_value ) { + if ( defined $recall_shelf_time and $recall_shelf_time >= 0 ) { + if ( $days_waiting->days > $recall_shelf_time ) { # recall has been awaiting pickup for longer than the circ rules allow $recall->set_expired( { interface => 'COMMANDLINE' } ); diff --git a/opac/opac-recall.pl b/opac/opac-recall.pl index aba53ae86eb..960588a1704 100755 --- a/opac/opac-recall.pl +++ b/opac/opac-recall.pl @@ -48,7 +48,7 @@ if ( C4::Context->preference('UseRecalls') ) { # check if already recalled my $recalled = $biblio->recalls->filter_by_current->search( { patron_id => $borrowernumber } )->count; if ( defined $recalled and $recalled > 0 ) { - my $recalls_per_record = Koha::CirculationRules->get_effective_rule( + my $recalls_per_record = Koha::CirculationRules->get_effective_rule_value( { categorycode => $patron->categorycode, branchcode => undef, @@ -56,10 +56,7 @@ if ( C4::Context->preference('UseRecalls') ) { rule_name => 'recalls_per_record' } ); - if ( defined $recalls_per_record - and $recalls_per_record->rule_value - and $recalled >= $recalls_per_record->rule_value ) - { + if ( $recalls_per_record and $recalled >= $recalls_per_record ) { $error = 'duplicate'; } } diff --git a/t/db_dependent/Koha/CirculationRules.t b/t/db_dependent/Koha/CirculationRules.t index bde48da84bc..b05f3eeead7 100755 --- a/t/db_dependent/Koha/CirculationRules.t +++ b/t/db_dependent/Koha/CirculationRules.t @@ -758,6 +758,9 @@ subtest 'get_onshelfholds_policy() tests' => sub { } ); + my $memory_cache = Koha::Cache::Memory::Lite->get_instance; + $memory_cache->flush(); + is( $circ_rules->get_onshelfholds_policy( { item => $item } ), 1, 'If rule_value is set on a matching rule, return it' @@ -765,6 +768,7 @@ subtest 'get_onshelfholds_policy() tests' => sub { # Delete the rule (i.e. get_effective_rule returns undef) $circ_rules->delete; + $memory_cache->flush(); is( $circ_rules->get_onshelfholds_policy( { item => $item } ), 0, 'If no matching rule, fallback to 0' ); $schema->storage->txn_rollback; @@ -1132,6 +1136,8 @@ subtest 'get_lostreturn_policy() tests' => sub { rule_name => 'processingreturn' } )->next->delete; + $memory_cache->flush(); + is_deeply( Koha::CirculationRules->get_lostreturn_policy($params), { lostreturn => 'refund', processingreturn => 'refund' }, -- 2.34.1