From 9f6c4fbf096a15eadfbc9a56d99e57c2c1d75659 Mon Sep 17 00:00:00 2001 From: Lari Taskula Date: Fri, 10 Apr 2020 00:02:24 +0000 Subject: [PATCH] Bug 26814: Add onsite_checkout to set_rule To test: 1. Find all occurrences of set_rule that are missing onsite_checkout where required grep --exclude-dir '.git' --exclude-dir 'misc/translator' \ --exclude-dir 'koha-tmpl' -Przo '(?s)(::|->)set_rule(?!s).*?\)' | \ grep -avz 'onsite_checkout' | grep -Pavz 'hold|reserves|article_requests|lostreturn' \ && echo "" The only occasions this should return anything are cases where a HASH or HASHref is given to set_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) 2. Find all subroutines using set_rule() git grep --no-index -n -p -P 'set_rule\s*\(' | grep -v 'sub {' \ | grep -P 'sub .*' 2.1 The only return should be Koha/CirculationRules.pm=331=sub set_rules { 3. The boring step. Find all occurrences of set_rule() with the following command: grep --exclude-dir='.git' -Prn 'set_rule' | grep -v set_rules Go through this list and make sure all neccessary locations are updated. A change is required when the rule scope includes onsite_checkout. See Koha/CirculationRules.pm for scopes. Hold/reserve related rules do not require it. 4. Check changes to CirculationRule objects built with TestBuilder cd t && grep -Prn "CirculationRules?'" Make sure all these cases are updated with onsite_checkout => undef, otherwise tests will fail later on. Sponsored-by: The National Library of Finland --- t/db_dependent/Circulation.t | 9 +++++++ t/db_dependent/Circulation/CalcFine.t | 5 +++- t/db_dependent/Circulation/ReturnClaims.t | 1 + t/db_dependent/Circulation/Returns.t | 1 + t/db_dependent/Koha/IssuingRules.t | 33 ++++++++++++++++++++--- t/db_dependent/Koha/Items.t | 3 +++ 6 files changed, 48 insertions(+), 4 deletions(-) diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t index f29c175aef..be320000af 100755 --- a/t/db_dependent/Circulation.t +++ b/t/db_dependent/Circulation.t @@ -686,6 +686,7 @@ subtest "CanBookBeRenewed tests" => sub { categorycode => undef, branchcode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'norenewalbefore', rule_value => '7', } @@ -2227,6 +2228,7 @@ subtest 'AddReturn + suspension_chargeperiod' => sub { categorycode => undef, branchcode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'suspension_chargeperiod', rule_value => '2', } @@ -2746,6 +2748,7 @@ subtest 'AddReturn | is_overdue' => sub { branchcode => $branchcode_false, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'lostreturn', rule_value => 0 } @@ -2760,6 +2763,7 @@ subtest 'AddReturn | is_overdue' => sub { branchcode => $branchcode_refund, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'lostreturn', rule_value => 'refund' } @@ -2774,6 +2778,7 @@ subtest 'AddReturn | is_overdue' => sub { branchcode => $branchcode_restore, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'lostreturn', rule_value => 'restore' } @@ -2788,6 +2793,7 @@ subtest 'AddReturn | is_overdue' => sub { branchcode => $branchcode_charge, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'lostreturn', rule_value => 'charge' } @@ -3956,6 +3962,7 @@ subtest 'Incremented fee tests' => sub { categorycode => $patron->categorycode, itemtype => $itemtype->id, branchcode => $library->id, + onsite_checkout => undef, rule_name => 'lengthunit', rule_value => 'hours', } @@ -4206,6 +4213,7 @@ subtest 'Tests for NoRefundOnLostReturnedItemsAge with AddReturn' => sub { branchcode => undef, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'lostreturn', rule_value => 'refund' } @@ -4399,6 +4407,7 @@ subtest 'Tests for NoRefundOnLostReturnedItemsAge with AddIssue' => sub { branchcode => undef, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'lostreturn', rule_value => 'refund' } diff --git a/t/db_dependent/Circulation/CalcFine.t b/t/db_dependent/Circulation/CalcFine.t index 85f38ea42e..c443cd16a9 100755 --- a/t/db_dependent/Circulation/CalcFine.t +++ b/t/db_dependent/Circulation/CalcFine.t @@ -69,6 +69,7 @@ subtest 'Test basic functionality' => sub { branchcode => undef, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rules => { fine => '1.00', lengthunit => 'days', @@ -109,6 +110,7 @@ subtest 'Test cap_fine_to_replacement_price' => sub { branchcode => undef, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rules => { fine => '1.00', lengthunit => 'days', @@ -162,6 +164,7 @@ subtest 'Test cap_fine_to_replacement_pricew with overduefinescap' => sub { branchcode => undef, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rules => { fine => '1.00', lengthunit => 'days', @@ -189,7 +192,7 @@ subtest 'Test cap_fine_to_replacement_pricew with overduefinescap' => sub { my ($amount) = CalcFine( $item->unblessed, $patron->{categorycode}, $branch->{branchcode}, $start_dt, $end_dt ); is( int($amount), 3, 'Got the lesser of overduefinescap and replacement price where overduefinescap < replacement price' ); - Koha::CirculationRules->set_rule({ rule_name => 'overduefinescap', rule_value => 6, branchcode => undef, categorycode => undef, itemtype => undef }); + Koha::CirculationRules->set_rule({ rule_name => 'overduefinescap', rule_value => 6, branchcode => undef, onsite_checkout => undef, categorycode => undef, itemtype => undef }); ($amount) = CalcFine( $item->unblessed, $patron->{categorycode}, $branch->{branchcode}, $start_dt, $end_dt ); is( int($amount), 5, 'Get the lesser of overduefinescap and replacement price where overduefinescap > replacement price' ); diff --git a/t/db_dependent/Circulation/ReturnClaims.t b/t/db_dependent/Circulation/ReturnClaims.t index 7d3b52128e..c5f6622b6d 100755 --- a/t/db_dependent/Circulation/ReturnClaims.t +++ b/t/db_dependent/Circulation/ReturnClaims.t @@ -51,6 +51,7 @@ Koha::CirculationRules->set_rule( branchcode => undef, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'issuelength', rule_value => 1 } diff --git a/t/db_dependent/Circulation/Returns.t b/t/db_dependent/Circulation/Returns.t index 79ae8555bf..5a2c4ded1b 100755 --- a/t/db_dependent/Circulation/Returns.t +++ b/t/db_dependent/Circulation/Returns.t @@ -63,6 +63,7 @@ Koha::CirculationRules->set_rule( categorycode => undef, itemtype => undef, branchcode => undef, + onsite_checkout => undef, rule_name => 'issuelength', rule_value => 1, } diff --git a/t/db_dependent/Koha/IssuingRules.t b/t/db_dependent/Koha/IssuingRules.t index afcb2867b7..074f75e6af 100755 --- a/t/db_dependent/Koha/IssuingRules.t +++ b/t/db_dependent/Koha/IssuingRules.t @@ -154,9 +154,10 @@ subtest 'set_rule' => 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 'Correct call' => sub { - plan tests => 4; + plan tests => 5; Koha::CirculationRules->delete; @@ -172,6 +173,7 @@ subtest 'set_rule' => sub { Koha::CirculationRules->set_rule( { branchcode => $branchcode, categorycode => $categorycode, + onsite_checkout => $onsite_checkout, rule_name => 'patron_maxissueqty', rule_value => '', } ); @@ -191,10 +193,22 @@ subtest 'set_rule' => sub { branchcode => $branchcode, categorycode => $categorycode, itemtype => $itemtype, + onsite_checkout => $onsite_checkout, rule_name => 'fine', rule_value => '', } ); - }, 'setting fine with branch/category/itemtype succeeds' ); + }, 'setting fine with branch/category/itemtype/onsite_checkout succeeds' ); + + lives_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + categorycode => $categorycode, + itemtype => $itemtype, + onsite_checkout => $onsite_checkout, + rule_name => 'maxissueqty', + rule_value => 5, + } ); + }, 'setting maxissueqty with branch/category/itemtype/onsite_checkout succeeds' ); }; subtest 'Call with missing params' => sub { @@ -212,6 +226,7 @@ subtest 'set_rule' => sub { throws_ok( sub { Koha::CirculationRules->set_rule( { branchcode => $branchcode, + onsite_checkout => $onsite_checkout, rule_name => 'patron_maxissueqty', rule_value => '', } ); @@ -229,6 +244,7 @@ subtest 'set_rule' => sub { Koha::CirculationRules->set_rule( { branchcode => $branchcode, categorycode => $categorycode, + onsite_checkout => undef, rule_name => 'fine', rule_value => '', } ); @@ -236,7 +252,7 @@ subtest 'set_rule' => sub { }; subtest 'Call with extra params' => sub { - plan tests => 3; + plan tests => 4; Koha::CirculationRules->delete; @@ -252,6 +268,7 @@ subtest 'set_rule' => sub { throws_ok( sub { Koha::CirculationRules->set_rule( { branchcode => $branchcode, + onsite_checkout => undef, categorycode => $categorycode, itemtype => $itemtype, rule_name => 'patron_maxissueqty', @@ -268,6 +285,16 @@ subtest 'set_rule' => sub { rule_value => '', } ); }, qr/categorycode/, 'setting holdallowed with categorycode fails' ); + + throws_ok( sub { + Koha::CirculationRules->set_rule( { + branchcode => $branchcode, + onsite_checkout => undef, + rule_name => 'holdallowed', + itemtype => $itemtype, + rule_value => '', + } ); + }, qr/onsite_checkout/, 'setting holdallowed with onsite_checkout fails' ); }; }; diff --git a/t/db_dependent/Koha/Items.t b/t/db_dependent/Koha/Items.t index 0737881609..4ffbb38e6c 100755 --- a/t/db_dependent/Koha/Items.t +++ b/t/db_dependent/Koha/Items.t @@ -778,6 +778,7 @@ subtest 'store' => sub { branchcode => $manager->branchcode, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'lostreturn', rule_value => 'restore' } @@ -870,6 +871,7 @@ subtest 'store' => sub { branchcode => $manager->branchcode, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'lostreturn', rule_value => 'restore' } @@ -981,6 +983,7 @@ subtest 'store' => sub { branchcode => $manager->branchcode, categorycode => undef, itemtype => undef, + onsite_checkout => undef, rule_name => 'lostreturn', rule_value => 'restore' } -- 2.17.1