From 81c9b1b4dc88ac46e83d10cd890fcfed894d5021 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Wed, 8 Apr 2020 19:10:32 +0000 Subject: [PATCH] Bug 26814: Add onsite_checkout to get_effective_rule To test: 1. Find all occurrences of get_effective_rule that are missing onsite_checkout where required grep --exclude-dir '.git' --exclude-dir 'misc/translator' \ --exclude-dir 'koha-tmpl' \ -Przo '(?s)(::|->)get_effective_rule(?!s).*?\)' | \ grep -avz 'onsite_checkout' | \ grep -Pavz 'hold|reserves|article_requests|lostreturn' | less The only occasions this should return anything are cases where a HASH or HASHref is given to get_effective_rule(), or that we are explicitly testing missing parameters in an unit test. 1.2 Verify the HASH/HASHref cases. The hash should contain a onsite_checkout (unless the rule is related to holds) 1.3 Verify the other cases. If you find another result than what is mentioned above, then this test fails. 2. Find all subroutines using get_effective_rule() git grep --no-index -n -p -P 'get_effective_rule\s*\(' \ | grep -v 'sub {' | grep -P 'sub .*' This list should be returned (line numbers not meaningful): C4/Circulation.pm=394=sub TooMany { C4/Circulation.pm=1375=sub AddIssue { C4/Circulation.pm=1697=sub GetBranchBorrowerCircRule { C4/Circulation.pm=1753=sub GetBranchItemRule { C4/Circulation.pm=3040=sub GetRenewCount { C4/Reserves.pm=359=sub CanItemBeReserved { C4/Reserves.pm=2245=sub GetHoldRule { Koha/Biblio.pm=295=sub article_request_type_for_bib { Koha/Charges/Fees.pm=93=sub accumulate_rentalcharge { Koha/CirculationRules.pm=221=sub get_effective_rules { Koha/CirculationRules.pm=395=sub get_opacitemholds_policy { Koha/CirculationRules.pm=421=sub get_onshelfholds_policy { Koha/CirculationRules.pm=443=sub get_lostreturn_policy { Koha/CirculationRules.pm=538=sub get_effective_daysmode { Koha/Item.pm=617=sub article_request_type { Koha/REST/V1/Checkouts.pm=192=sub allows_renewal { Koha/Template/Plugin/CirculationRules.pm=26=sub Get { Check these subroutines and make sure they are using get_effective_rule() with the new scope, unless not needed. When fetching a hold-related rules, onsite_checkout is not needed. These subroutines and classes need changes to sub params: 2.1. Check modifications to Koha::Charges::Fees 2.2. Check modification made to C4::Circulation::_debar_user_on_return(). 2.3. Check modification made to C4::Circulation::_calculate_new_debar_dt(). 3. Find all occurrences of get_effective_rule with the following command: grep --exclude-dir='.git' -Prn 'get_effective_rule(?!s)' Make sure all matches (where rule scope allows onsite_checkout) are updated. Sponsored-by: The National Library of Finland --- C4/Circulation.pm | 21 ++++++--- Koha/Charges/Fees.pm | 25 ++++++++++ Koha/REST/V1/Checkouts.pm | 1 + Koha/Template/Plugin/CirculationRules.pm | 8 +++- .../swagger/definitions/circ-rule-kind.json | 2 +- .../IssuingRules/maxsuspensiondays.t | 4 +- t/db_dependent/Koha/Charges/Fees.t | 2 + t/db_dependent/Koha/IssuingRules.t | 47 ++++++++++++++++--- 8 files changed, 92 insertions(+), 18 deletions(-) diff --git a/C4/Circulation.pm b/C4/Circulation.pm index c5656705d5..9788401bfe 100644 --- a/C4/Circulation.pm +++ b/C4/Circulation.pm @@ -410,7 +410,7 @@ sub TooMany { my $child_types = Koha::ItemTypes->search({ parent_type => $type }); # Find any children if we are a parent_type; - # given branch, patron category, and item type, determine + # given branch, patron category, item type, and checkout type, determine # applicable issuing rule $parent_maxissueqty_rule = Koha::CirculationRules->get_effective_rule( @@ -1452,6 +1452,7 @@ sub AddIssue { patron => $patron, library => $library, item => $item_object, + onsite_checkout => $onsite_checkout, to_date => $datedue, } ); @@ -1493,6 +1494,7 @@ sub AddIssue { categorycode => $borrower->{categorycode}, itemtype => $item_object->effective_itemtype, branchcode => $branchcode, + onsite_checkout => $onsite_checkout, rule_name => 'auto_renew' } ); @@ -2139,7 +2141,7 @@ sub AddReturn { if ( $issue and $issue->is_overdue($return_date) ) { # fix fine days - my ($debardate,$reminder) = _debar_user_on_return( $patron_unblessed, $item->unblessed, dt_from_string($issue->date_due), $return_date ); + my ($debardate,$reminder) = _debar_user_on_return( $patron_unblessed, $item->unblessed, $issue->onsite_checkout, dt_from_string($issue->date_due), $return_date ); if ($reminder){ $messages->{'PrevDebarred'} = $debardate; } else { @@ -2357,12 +2359,14 @@ sub MarkIssueReturned { =head2 _debar_user_on_return - _debar_user_on_return($borrower, $item, $datedue, $returndate); + _debar_user_on_return($borrower, $item, $onsite_checkout, $datedue, $returndate); C<$borrower> borrower hashref C<$item> item hashref +C<$onsite_checkout> undef, 0 or 1 + C<$datedue> date due DateTime object C<$returndate> DateTime object representing the return time @@ -2378,7 +2382,7 @@ to ease testing. =cut sub _calculate_new_debar_dt { - my ( $borrower, $item, $dt_due, $return_date ) = @_; + my ( $borrower, $item, $onsite_checkout, $dt_due, $return_date ) = @_; my $branchcode = _GetCircControlBranch( $item, $borrower ); my $circcontrol = C4::Context->preference('CircControl'); @@ -2386,6 +2390,7 @@ sub _calculate_new_debar_dt { { categorycode => $borrower->{categorycode}, itemtype => $item->{itype}, branchcode => $branchcode, + onsite_checkout => $onsite_checkout, rules => [ 'finedays', 'lengthunit', @@ -2459,11 +2464,11 @@ sub _calculate_new_debar_dt { } sub _debar_user_on_return { - my ( $borrower, $item, $dt_due, $return_date ) = @_; + my ( $borrower, $item, $onsite_checkout, $dt_due, $return_date ) = @_; $return_date //= dt_from_string(); - my $new_debar_dt = _calculate_new_debar_dt ($borrower, $item, $dt_due, $return_date); + my $new_debar_dt = _calculate_new_debar_dt ($borrower, $item, $onsite_checkout, $dt_due, $return_date); return unless $new_debar_dt; @@ -3032,6 +3037,7 @@ sub AddRenewal { patron => $patron, library => $circ_library, item => $item_object, + onsite_checkout => $issue->onsite_checkout, from_date => dt_from_string( $issue->date_due, 'sql' ), to_date => dt_from_string($datedue), } @@ -3046,6 +3052,7 @@ sub AddRenewal { { categorycode => $patron->categorycode, itemtype => $item_object->effective_itemtype, branchcode => $circ_library->branchcode, + onsite_checkout => $issue->onsite_checkout, rule_name => 'unseen_renewals_allowed' } ); @@ -3178,12 +3185,14 @@ sub GetRenewCount { $unseencount = $data->{'unseen_renewals'} if $data->{'unseen_renewals'}; # $item and $borrower should be calculated my $branchcode = _GetCircControlBranch($item->unblessed, $patron->unblessed); + my $onsite_checkout = $data->{'onsite_checkout'}; my $rules = Koha::CirculationRules->get_effective_rules( { categorycode => $patron->categorycode, itemtype => $item->effective_itemtype, branchcode => $branchcode, + onsite_checkout => $onsite_checkout, rules => [ 'renewalsallowed', 'unseen_renewals_allowed' ] } ); diff --git a/Koha/Charges/Fees.pm b/Koha/Charges/Fees.pm index 7eff76ba79..b8b491960c 100644 --- a/Koha/Charges/Fees.pm +++ b/Koha/Charges/Fees.pm @@ -22,6 +22,7 @@ use Modern::Perl; use Carp qw( carp confess ); use Koha::Calendar; +use Koha::Checkouts; use Koha::DateUtils qw( dt_from_string ); use Koha::Exceptions; @@ -38,6 +39,7 @@ Koha::Charges::Fees->new( patron => $patron, library => $library, item => $item, + [ onsite_checkout => $onsite_checkout, ] to_date => $to_dt, [ from_date => $from_dt, ] } @@ -57,6 +59,8 @@ sub new { Koha::Exceptions::MissingParameter->throw("Missing mandatory parameter: to_date") unless $params->{to_date}; + $params->{onsite_checkout} = $params->{onsite_checkout} ? 1 : 0; + Carp::confess("Key 'patron' is not a Koha::Patron object!") unless $params->{patron}->isa('Koha::Patron'); Carp::confess("Key 'library' is not a Koha::Library object!") @@ -95,6 +99,7 @@ sub accumulate_rentalcharge { categorycode => $self->patron->categorycode, itemtype => $itemtype->id, branchcode => $self->library->id, + onsite_checkout => $self->onsite_checkout, rule_name => 'lengthunit', } ); @@ -188,6 +193,26 @@ sub item { return $self->{item}; } +=head3 onsite_checkout + +my $item = $fees->onsite_checkout( $onsite_checkout ); + +=cut + +sub onsite_checkout { + my ( $self, $onsite_checkout ) = @_; + + if ( $onsite_checkout ) { + my @valid_types = (0, 1); + Carp::carp("Given 'onsite_checkout' is not valid! Valid are: " . join(', ', @valid_types)) + unless grep { $_ eq $onsite_checkout } @valid_types; + + $self->{onsite_checkout} = $onsite_checkout if $onsite_checkout; + } + + return $self->{onsite_checkout}; +} + =head3 to_date my $to_date = $fees->to_date( $to_date ); diff --git a/Koha/REST/V1/Checkouts.pm b/Koha/REST/V1/Checkouts.pm index 93feeaeb24..7a6d1ddba0 100644 --- a/Koha/REST/V1/Checkouts.pm +++ b/Koha/REST/V1/Checkouts.pm @@ -222,6 +222,7 @@ sub allows_renewal { categorycode => $checkout->patron->categorycode, itemtype => $checkout->item->effective_itemtype, branchcode => $checkout->branchcode, + onsite_checkout => $checkout->onsite_checkout, rule_name => 'renewalsallowed', } ); diff --git a/Koha/Template/Plugin/CirculationRules.pm b/Koha/Template/Plugin/CirculationRules.pm index e82dce01ed..385b2d7d64 100644 --- a/Koha/Template/Plugin/CirculationRules.pm +++ b/Koha/Template/Plugin/CirculationRules.pm @@ -24,15 +24,17 @@ use base qw( Template::Plugin ); use Koha::CirculationRules; sub Get { - my ( $self, $branchcode, $categorycode, $itemtype, $rule_name ) = @_; + my ( $self, $branchcode, $onsite_checkout, $categorycode, $itemtype, $rule_name ) = @_; $branchcode = undef if $branchcode eq q{} or $branchcode eq q{*}; + $onsite_checkout = undef if $onsite_checkout eq {} or $onsite_checkout eq q{*}; $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( { branchcode => $branchcode, + onsite_checkout => $onsite_checkout, categorycode => $categorycode, itemtype => $itemtype, rule_name => $rule_name, @@ -43,15 +45,17 @@ sub Get { } sub Search { - my ( $self, $branchcode, $categorycode, $itemtype, $rule_name ) = @_; + my ( $self, $branchcode, $onsite_checkout, $categorycode, $itemtype, $rule_name ) = @_; $branchcode = undef if $branchcode eq q{} or $branchcode eq q{*}; + $onsite_checkout = undef if $onsite_checkout eq q{} or $onsite_checkout eq q{*}; $categorycode = undef if $categorycode eq q{} or $categorycode eq q{*}; $itemtype = undef if $itemtype eq q{} or $itemtype eq q{*}; my $rule = Koha::CirculationRules->search( { branchcode => $branchcode, + onsite_checkout => $onsite_checkout, categorycode => $categorycode, itemtype => $itemtype, rule_name => $rule_name, diff --git a/api/v1/swagger/definitions/circ-rule-kind.json b/api/v1/swagger/definitions/circ-rule-kind.json index 90b956f9f6..2fe89bc91b 100644 --- a/api/v1/swagger/definitions/circ-rule-kind.json +++ b/api/v1/swagger/definitions/circ-rule-kind.json @@ -6,7 +6,7 @@ "type": "array", "items": { "type": "string", - "enum": [ "branchcode", "categorycode", "itemtype" ] + "enum": [ "branchcode", "onsite_checkout", "categorycode", "itemtype" ] } } }, diff --git a/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t b/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t index 0198c3f219..78e59b53e8 100755 --- a/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t +++ b/t/db_dependent/Circulation/IssuingRules/maxsuspensiondays.t @@ -130,7 +130,7 @@ subtest "suspension_chargeperiod" => sub { my $last_year = dt_from_string->clone->subtract( years => 1 ); my $today = dt_from_string; - my $new_debar_dt = C4::Circulation::_calculate_new_debar_dt( $patron->unblessed, $item->unblessed, $last_year, $today ); + my $new_debar_dt = C4::Circulation::_calculate_new_debar_dt( $patron->unblessed, $item->unblessed, undef, $last_year, $today ); is( $new_debar_dt->truncate( to => 'day' ), $today->clone->add( days => 365 / 15 * 7 )->truncate( to => 'day' ) ); @@ -157,7 +157,7 @@ subtest "maxsuspensiondays" => sub { my $last_year = dt_from_string->clone->subtract( years => 1 ); my $today = dt_from_string; - my $new_debar_dt = C4::Circulation::_calculate_new_debar_dt( $patron->unblessed, $item->unblessed, $last_year, $today ); + my $new_debar_dt = C4::Circulation::_calculate_new_debar_dt( $patron->unblessed, $item->unblessed, undef, $last_year, $today ); is( $new_debar_dt->truncate( to => 'day' ), $today->clone->add( days => 333 )->truncate( to => 'day' ) ); }; diff --git a/t/db_dependent/Koha/Charges/Fees.t b/t/db_dependent/Koha/Charges/Fees.t index 0cc067a15c..dfd19adc36 100755 --- a/t/db_dependent/Koha/Charges/Fees.t +++ b/t/db_dependent/Koha/Charges/Fees.t @@ -29,6 +29,7 @@ use t::lib::Dates; use Time::Fake; use C4::Calendar; +use Koha::Checkouts; use Koha::DateUtils qw(dt_from_string); BEGIN { @@ -406,6 +407,7 @@ subtest 'accumulate_rentalcharge tests' => sub { patron => $patron, library => $library, item => $item, + onsite_checkout => 0, to_date => $dt_to, from_date => $dt_from, } diff --git a/t/db_dependent/Koha/IssuingRules.t b/t/db_dependent/Koha/IssuingRules.t index 074f75e6af..e65fad668c 100755 --- a/t/db_dependent/Koha/IssuingRules.t +++ b/t/db_dependent/Koha/IssuingRules.t @@ -25,6 +25,7 @@ use Test::Exception; use Benchmark; +use Koha::Checkouts; use Koha::CirculationRules; use t::lib::TestBuilder; @@ -41,6 +42,7 @@ subtest 'get_effective_issuing_rule' => sub { my $categorycode = $builder->build({ source => 'Category' })->{'categorycode'}; my $itemtype = $builder->build({ source => 'Itemtype' })->{'itemtype'}; my $branchcode = $builder->build({ source => 'Branch' })->{'branchcode'}; + my $onsite_checkout = 0; subtest 'Call with undefined values' => sub { plan tests => 5; @@ -54,6 +56,7 @@ subtest 'get_effective_issuing_rule' => sub { branchcode => undef, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'fine', rule_value => 1, }); @@ -67,6 +70,7 @@ subtest 'get_effective_issuing_rule' => sub { branchcode => undef, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'fine', rule_value => 2, } @@ -77,6 +81,7 @@ subtest 'get_effective_issuing_rule' => sub { branchcode => undef, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'fine', }); _is_row_match( @@ -85,6 +90,7 @@ subtest 'get_effective_issuing_rule' => sub { branchcode => undef, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'fine', rule_value => 2, }, @@ -95,31 +101,44 @@ subtest 'get_effective_issuing_rule' => sub { }; subtest 'Performance' => sub { - plan tests => 4; + plan tests => 5; my $worst_case = timethis(500, sub { Koha::CirculationRules->get_effective_rule({ branchcode => 'nonexistent', categorycode => 'nonexistent', itemtype => 'nonexistent', + onsite_checkout => 'nonexistent', rule_name => 'nonexistent', }); } ); - my $mid_case = timethis(500, + my $mid_case1 = timethis(500, sub { Koha::CirculationRules->get_effective_rule({ branchcode => $branchcode, categorycode => 'nonexistent', itemtype => 'nonexistent', + onsite_checkout => 'nonexistent', rule_name => 'nonexistent', }); } ); - my $sec_best_case = timethis(500, + my $mid_case2 = timethis(500, sub { Koha::CirculationRules->get_effective_rule({ branchcode => $branchcode, categorycode => $categorycode, itemtype => 'nonexistent', + onsite_checkout => 'nonexistent', + rule_name => 'nonexistent', + }); + } + ); + my $mid_case3 = timethis(500, + sub { Koha::CirculationRules->get_effective_rule({ + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + onsite_checkout => 'nonexistent', rule_name => 'nonexistent', }); } @@ -129,6 +148,7 @@ subtest 'get_effective_issuing_rule' => sub { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'nonexistent', }); } @@ -136,11 +156,14 @@ subtest 'get_effective_issuing_rule' => sub { ok($worst_case, 'In worst case, get_effective_issuing_rule finds matching' .' rule '.sprintf('%.2f', $worst_case->iters/$worst_case->cpu_a) .' times per second.'); - ok($mid_case, 'In mid case, get_effective_issuing_rule finds matching' - .' rule '.sprintf('%.2f', $mid_case->iters/$mid_case->cpu_a) + ok($mid_case1, 'In mid case 1, get_effective_issuing_rule finds matching' + .' rule '.sprintf('%.2f', $mid_case1->iters/$mid_case1->cpu_a) + .' times per second.'); + ok($mid_case2, 'In mid case 2, get_effective_issuing_rule finds matching' + .' rule '.sprintf('%.2f', $mid_case2->iters/$mid_case2->cpu_a) .' times per second.'); - ok($sec_best_case, 'In second best case, get_effective_issuing_rule finds matching' - .' rule '.sprintf('%.2f', $sec_best_case->iters/$sec_best_case->cpu_a) + ok($mid_case3, 'In mid case 3, get_effective_issuing_rule finds matching' + .' rule '.sprintf('%.2f', $mid_case3->iters/$mid_case3->cpu_a) .' times per second.'); ok($best_case, 'In best case, get_effective_issuing_rule finds matching' .' rule '.sprintf('%.2f', $best_case->iters/$best_case->cpu_a) @@ -304,6 +327,7 @@ subtest 'clone' => sub { my $branchcode = $builder->build({ source => 'Branch' })->{'branchcode'}; my $categorycode = $builder->build({ source => 'Category' })->{'categorycode'}; my $itemtype = $builder->build({ source => 'Itemtype' })->{'itemtype'}; + my $onsite_checkout = 0; subtest 'Clone multiple rules' => sub { plan tests => 4; @@ -314,6 +338,7 @@ subtest 'clone' => sub { branchcode => undef, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'fine', rule_value => 5, })->store; @@ -322,6 +347,7 @@ subtest 'clone' => sub { branchcode => undef, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'lengthunit', rule_value => 'days', })->store; @@ -332,12 +358,14 @@ subtest 'clone' => sub { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'fine', }); my $rule_lengthunit = Koha::CirculationRules->get_effective_rule({ branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'lengthunit', }); @@ -347,6 +375,7 @@ subtest 'clone' => sub { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'fine', rule_value => 5, }, @@ -359,6 +388,7 @@ subtest 'clone' => sub { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'lengthunit', rule_value => 'days', }, @@ -377,6 +407,7 @@ subtest 'clone' => sub { branchcode => undef, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'fine', rule_value => 5, })->store; @@ -388,6 +419,7 @@ subtest 'clone' => sub { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'fine', }); @@ -397,6 +429,7 @@ subtest 'clone' => sub { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'fine', rule_value => '5', }, -- 2.17.1