Bugzilla – Attachment 157551 Details for
Bug 32092
Improve circulation rules cache utilization
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 32092: Fix regression in TooMany and some missed cases
Bug-32092-Fix-regression-in-TooMany-and-some-misse.patch (text/plain), 18.54 KB, created by
David Gustafsson
on 2023-10-20 15:08:09 UTC
(
hide
)
Description:
Bug 32092: Fix regression in TooMany and some missed cases
Filename:
MIME Type:
Creator:
David Gustafsson
Created:
2023-10-20 15:08:09 UTC
Size:
18.54 KB
patch
obsolete
>From 769b6dc7674010bd7d6f7ba8ddb3ed4c09e3cddd Mon Sep 17 00:00:00 2001 >From: David Gustafsson <david.gustafsson@ub.gu.se> >Date: Thu, 19 Oct 2023 16:24:53 +0200 >Subject: [PATCH] Bug 32092: Fix regression in TooMany and some missed cases > >--- > C4/Circulation.pm | 87 +++++++++++++------------ > Koha/Biblio.pm | 6 +- > Koha/CirculationRules.pm | 4 +- > Koha/Item.pm | 7 +- > misc/cronjobs/recalls/expire_recalls.pl | 6 +- > opac/opac-recall.pl | 4 +- > t/db_dependent/Circulation/Branch.t | 4 +- > 7 files changed, 62 insertions(+), 56 deletions(-) > >diff --git a/C4/Circulation.pm b/C4/Circulation.pm >index 6f92b3e6001..d40bc8d6417 100644 >--- a/C4/Circulation.pm >+++ b/C4/Circulation.pm >@@ -432,8 +432,8 @@ sub TooMany { > # applicable issuing rule > > # If the parent rule is for default type we discount it >- my $parent_maxissueqty; >- $parent_maxissueqty = Koha::CirculationRules->get_effective_rule_value( >+ my $parent_maxissueqty_rule; >+ $parent_maxissueqty_rule = Koha::CirculationRules->get_effective_rule( > { > categorycode => $cat_borrower, > itemtype => $parent_type, >@@ -441,8 +441,10 @@ sub TooMany { > rule_name => 'maxissueqty', > } > ) if $parent_type; >+ # If the parent rule is for default type we discount it >+ $parent_maxissueqty_rule = undef if $parent_maxissueqty_rule && !defined $parent_maxissueqty_rule->itemtype; > >- my $maxissueqty = Koha::CirculationRules->get_effective_rule_value( >+ my $maxissueqty_rule = Koha::CirculationRules->get_effective_rule( > { > categorycode => $cat_borrower, > itemtype => $type, >@@ -451,7 +453,7 @@ sub TooMany { > } > ); > >- my $maxonsiteissueqty = Koha::CirculationRules->get_effective_rule_value( >+ my $maxonsiteissueqty_rule = Koha::CirculationRules->get_effective_rule( > { > categorycode => $cat_borrower, > itemtype => $type, >@@ -463,18 +465,18 @@ sub TooMany { > # if a rule is found and has a loan limit set, count > # how many loans the patron already has that meet that > # rule >- if (defined $maxissueqty and $maxissueqty ne "") { >+ if (defined $maxissueqty_rule and $maxissueqty_rule->rule_value ne "") { > > my $checkouts; >- if ( $branch ) { >+ if ( $maxissueqty_rule->branchcode ) { > if ( C4::Context->preference('CircControl') eq 'PickupLibrary' ) { > $checkouts = $patron->checkouts->search( >- { 'me.branchcode' => $branch } ); >+ { 'me.branchcode' => $maxissueqty_rule->branchcode } ); > } elsif (C4::Context->preference('CircControl') eq 'PatronLibrary') { > $checkouts = $patron->checkouts; # if branch is the patron's home branch, then count all loans by patron > } else { > my $branch_type = C4::Context->preference('HomeOrHoldingBranch') || 'homebranch'; >- $checkouts = $patron->checkouts->search( { "item.$branch_type" => $branch } ); >+ $checkouts = $patron->checkouts->search( { "item.$branch_type" => $maxissueqty_rule->branchcode } ); > } > } else { > $checkouts = $patron->checkouts; # if rule is not branch specific then count all loans by patron >@@ -484,20 +486,20 @@ sub TooMany { > my $sum_checkouts; > > my @types; >- unless ( $type ) { >+ unless ( $maxissueqty_rule->itemtype ) { > # matching rule has the default item type, so count only > # those existing loans that don't fall under a more > # specific rule > @types = Koha::CirculationRules->search( > { >- branchcode => $branch, >- categorycode => $cat_borrower, >+ branchcode => $maxissueqty_rule->branchcode, >+ categorycode => [ $maxissueqty_rule->categorycode, $cat_borrower ], > itemtype => { '!=' => undef }, > rule_name => 'maxissueqty' > } > )->get_column('itemtype'); > } else { >- if ( $parent_maxissueqty ) { >+ 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 > my $children = Koha::ItemTypes->search({ parent_type => $parent_type }); >@@ -516,7 +518,7 @@ sub TooMany { > while ( my $c = $checkouts->next ) { > my $itemtype = $c->item->effective_itemtype; > >- unless ( $type ) { >+ unless ( $maxissueqty_rule->itemtype ) { > next if grep {$_ eq $itemtype} @types; > } else { > next unless grep {$_ eq $itemtype} @types; >@@ -535,20 +537,23 @@ sub TooMany { > checkout_count => $checkout_count, > onsite_checkout_count => $onsite_checkout_count, > onsite_checkout => $onsite_checkout, >- max_checkouts_allowed => $maxissueqty,, >- max_onsite_checkouts_allowed => $maxonsiteissueqty, >+ max_checkouts_allowed => $maxissueqty_rule ? $maxissueqty_rule->rule_value : undef, >+ max_onsite_checkouts_allowed => $maxonsiteissueqty_rule ? $maxonsiteissueqty_rule->rule_value : undef, > switch_onsite_checkout => $switch_onsite_checkout, > }; > # If parent rules exists >- if ( defined $parent_maxissueqty ){ >- $checkout_rules->{max_checkouts_allowed} = $parent_maxissueqty; >+ 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; > > # If the parent rule is less than or equal to the child, we only need check the parent >- if( $maxissueqty < $parent_maxissueqty && defined($type) ) { >+ if( >+ $maxissueqty_rule->rule_value < $parent_maxissueqty_rule->rule_value >+ && defined($maxissueqty_rule->itemtype) >+ ) { > $checkout_rules->{checkout_count} = $checkout_count_type; >- $checkout_rules->{max_checkouts_allowed} = $maxissueqty; >+ $checkout_rules->{max_checkouts_allowed} = $maxissueqty_rule->rule_value; > my $qty_over = _check_max_qty($checkout_rules); > return $qty_over if defined $qty_over; > } >@@ -560,7 +565,10 @@ sub TooMany { > > # Now count total loans against the limit for the branch > my $branch_borrower_circ_rule = GetBranchBorrowerCircRule($branch, $cat_borrower); >- if (defined($branch_borrower_circ_rule->{patron_maxissueqty}) and $branch_borrower_circ_rule->{patron_maxissueqty} ne '') { >+ if ( >+ defined($branch_borrower_circ_rule->{patron_maxissueqty}) >+ and $branch_borrower_circ_rule->{patron_maxissueqty} ne '' >+ ) { > my $checkouts; > if ( C4::Context->preference('CircControl') eq 'PickupLibrary' ) { > $checkouts = $patron->checkouts->search( >@@ -592,7 +600,7 @@ sub TooMany { > return $qty_over if defined $qty_over; > } > >- unless ( defined $maxissueqty || 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 }; > } > >@@ -610,7 +618,7 @@ sub _check_max_qty { > my $switch_onsite_checkout = $params->{switch_onsite_checkout}; > > 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', >@@ -620,7 +628,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 { >@@ -631,10 +639,10 @@ 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 ) >- { >+ $checkout_count - $onsite_checkout_count >= $max_checkouts_allowed >+ ) { > return { > reason => 'TOO_MANY_CHECKOUTS', > count => $checkout_count - $onsite_checkout_count, >@@ -1416,22 +1424,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', > } >- ); >+ ) || C4::Context->preference('decreaseLoanHighHoldsDuration'); > >- my $duration; >- if ( defined($rule) && $rule ne '' ){ >- # overrides decreaseLoanHighHoldsDuration syspref >- $duration = $rule; >- } else { >- $duration = C4::Context->preference('decreaseLoanHighHoldsDuration'); >- } > my $reduced_datedue = $calendar->addDuration( $issuedate, $duration ); > $reduced_datedue->set_hour($orig_due->hour); > $reduced_datedue->set_minute($orig_due->minute); >@@ -1919,6 +1921,7 @@ wildcards. > > sub GetBranchBorrowerCircRule { > my ( $branchcode, $categorycode ) = @_; >+ > return Koha::CirculationRules->get_effective_rules( > { > categorycode => $categorycode, >@@ -2690,11 +2693,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 >@@ -3217,14 +3220,14 @@ 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, > branchcode => $circ_library->branchcode, > 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++; > } else { > # If the renewal is seen, unseen should revert to 0 >@@ -3583,14 +3586,14 @@ 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, > itemtype => $item_type, > rule_name => 'rentaldiscount' > }); > 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/Koha/Biblio.pm b/Koha/Biblio.pm >index 6cc40d7f5db..8144a922736 100644 >--- a/Koha/Biblio.pm >+++ b/Koha/Biblio.pm >@@ -1566,9 +1566,9 @@ sub can_be_recalled { > 'on_shelf_recalls', > ], > }); >- 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 >diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm >index 180ea807765..929f6cc6d97 100644 >--- a/Koha/CirculationRules.pm >+++ b/Koha/CirculationRules.pm >@@ -313,7 +313,9 @@ sub get_effective_rule_value { > if ($rule) { > $value = $rule->rule_value; > } >- $value //= 'undef'; >+ else { >+ $value = 'undef'; >+ } > $memory_cache->set_in_cache( $cache_key, $value ); > } > return if $value eq 'undef'; >diff --git a/Koha/Item.pm b/Koha/Item.pm >index 76844aaf02c..3e722c8abb2 100644 >--- a/Koha/Item.pm >+++ b/Koha/Item.pm >@@ -1902,12 +1902,13 @@ 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 ); >+ 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 >= $rule->{recalls_per_record} ); > >@@ -1976,12 +1977,12 @@ 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 ); >+ return 0 if !$rule->{recalls_allowed}; > > # can recall > return 1; >diff --git a/misc/cronjobs/recalls/expire_recalls.pl b/misc/cronjobs/recalls/expire_recalls.pl >index 71d9c2bcaef..a03c86864f6 100755 >--- a/misc/cronjobs/recalls/expire_recalls.pl >+++ b/misc/cronjobs/recalls/expire_recalls.pl >@@ -50,7 +50,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, > branchcode => $recall->pickup_library_id, >@@ -58,8 +58,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 2a8b2497c00..b54a02ea360 100755 >--- a/opac/opac-recall.pl >+++ b/opac/opac-recall.pl >@@ -48,13 +48,13 @@ 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, > itemtype => undef, > rule_name => 'recalls_per_record' > }); >- if ( defined $recalls_per_record and $recalls_per_record->rule_value and $recalled >= $recalls_per_record->rule_value ){ >+ if ( defined $recalls_per_record and $recalls_per_record and $recalled >= $recalls_per_record ) { > $error = 'duplicate'; > } > } >diff --git a/t/db_dependent/Circulation/Branch.t b/t/db_dependent/Circulation/Branch.t >index 53e26d2d6a5..b7d4f44c636 100755 >--- a/t/db_dependent/Circulation/Branch.t >+++ b/t/db_dependent/Circulation/Branch.t >@@ -143,8 +143,8 @@ my $borrower_id1 = Koha::Patron->new({ > > is_deeply( > GetBranchBorrowerCircRule(), >- { patron_maxissueqty => undef, patron_maxonsiteissueqty => undef }, >-"Without parameter, GetBranchBorrower returns undef (unilimited) for patron_maxissueqty and patron_maxonsiteissueqty if no rules defined" >+ undef, >+ "Without parameter, GetBranchBorrower returns undef (unilimited) for patron_maxissueqty and patron_maxonsiteissueqty if no rules defined" > ); > > Koha::CirculationRules->set_rules( >-- >2.42.0
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=157551">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 32092
:
143120
|
146956
|
147149
|
147150
|
147151
|
149782
|
149783
|
151195
|
151196
|
151197
|
151709
|
151710
|
151711
|
153168
|
153169
|
153170
|
157534
|
157535
|
157536
|
157537
| 157551