From fc583dcb397169155d31447eaa843130e895de72 Mon Sep 17 00:00:00 2001
From: Lari Taskula <lari.taskula@hypernova.fi>
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' \
&& 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.

Sponsored-by: The National Library of Finland
---
 t/db_dependent/Circulation.t              |  3 +++
 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 ++++++++++++++++++++---
 5 files changed, 39 insertions(+), 4 deletions(-)

diff --git a/t/db_dependent/Circulation.t b/t/db_dependent/Circulation.t
index a51349b186..e7b62033ed 100755
--- a/t/db_dependent/Circulation.t
+++ b/t/db_dependent/Circulation.t
@@ -680,6 +680,7 @@ subtest "CanBookBeRenewed tests" => sub {
             categorycode => undef,
             branchcode   => undef,
             itemtype     => undef,
+            onsite_checkout => undef,
             rule_name    => 'norenewalbefore',
             rule_value   => '7',
         }
@@ -2159,6 +2160,7 @@ subtest 'AddReturn + suspension_chargeperiod' => sub {
             categorycode => undef,
             branchcode   => undef,
             itemtype     => undef,
+            onsite_checkout => undef,
             rule_name    => 'suspension_chargeperiod',
             rule_value   => '2',
         }
@@ -3438,6 +3440,7 @@ subtest 'Incremented fee tests' => sub {
             categorycode => $patron->categorycode,
             itemtype     => $itemtype->id,
             branchcode   => $library->id,
+            onsite_checkout => undef,
             rule_name    => 'lengthunit',
             rule_value   => 'hours',
         }
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 31abe27c4c..c9ec5429c9 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' );
     };
 };
 
-- 
2.17.1