From 36cfd800ff92a20b31231f04398e059e5c5e3bd9 Mon Sep 17 00:00:00 2001 From: David Gustafsson 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 | 10 ++- 7 files changed, 67 insertions(+), 57 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..ab434689abd 100755 --- a/t/db_dependent/Circulation/Branch.t +++ b/t/db_dependent/Circulation/Branch.t @@ -1,6 +1,7 @@ #!/usr/bin/perl -# This file is part of Koha. +# Thi +# le is part of Koha. # # Koha is free software; you can redistribute it and/or modify it # under the terms of the GNU General Public License as published by @@ -143,8 +144,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( @@ -337,6 +338,9 @@ subtest "GetBranchItemRule() tests" => sub { $schema->storage->txn_begin; $dbh->do('DELETE FROM circulation_rules'); + my $memory_cache = Koha::Cache::Memory::Lite->get_instance; + $memory_cache->flush(); + my $homebranch = $builder->build( { source => 'Branch' } )->{branchcode}; my $holdingbranch = $builder->build( { source => 'Branch' } )->{branchcode}; -- 2.42.0