From bd3dcca389a19fa660ea2cdf60627003a44ba076 Mon Sep 17 00:00:00 2001
From: Petro Vashchuk <stalkernoid@gmail.com>
Date: Tue, 21 Jul 2020 13:04:39 +0300
Subject: [PATCH] Bug 25711: add ExpireReservesMaxPickUpDelayCharge to
 circulation rules

Add a feature that allows to define ExpireReservesMaxPickUpDelayCharge
in circulation rules per specific items/patron categories.
When value not found in circulation rules it uses global
ExpireReservesMaxPickUpDelayCharge, as it was before this smart-rules
feature was introduced.

To check the feature after applying the patch:
    1) Go to /cgi-bin/koha/admin/preferences.pl and set
ExpireReservesMaxPickUpDelay to allow.
    1) Go to /cgi-bin/koha/admin/smart-rules.pl circulation rules table
and notice the newly added "Expire reserve charge" row.
    2) In that row add numerical values to type/patron categories
that you will be using later to test this feature.
    3) Prepare an item for our next step, pick a biblio that has item
with the same item type you altered in smart rules and check it out.
    3) Place a hold on that item and set the expiration date as the
next day.
    4) Check in it after that.
    5) Do SQL request to change expiration date manually to "yesterday":
in "reserves" table, find the id of the hold that you placed and
"UPDATE reserves SET `expirationdate`='2020-07-19' WHERE
`reserve_id`=329;"
 Or alternatively:
    5) Wait for tomorrow, change server time or invent a time machine.
    6) Run cancel_expired_holds.pl cron job.
    7) Check that patron got charged in "accounting" left tab and
"transactions" upper tab: your patron should have debt of that amount
of fee you assigned in that rule
(/cgi-bin/koha/members/boraccount.pl?borrowernumber=...YourPatronID...)
    8) You can repeat this with other rules and different
numbers/biblio/item types to check that different fees apply according
to smart rules.
    9) If there's no matching smart rule or smart rule have an empty
string as a fee - global ExpireReservesMaxPickUpDelayCharge variable
value will be used.
   10) But in case if matching rule has zero ("0") as a value - that
allows to have no-fee smart-rule results, as a feature.

IMPORTANT:
Smart-rules filters only applied to item-level holds, when item-type
for items is known. If there's biblio-level hold, then again global
ExpireReservesMaxPickUpDelayCharge value will be used (as well global
one can be empty or zero).

Mentored-by: Andrew Nugged <nugged@gmail.com>
---
 Koha/CirculationRules.pm                      |   3 +
 Koha/Hold.pm                                  |  20 +-
 admin/smart-rules.pl                          |   3 +
 .../prog/en/modules/admin/smart-rules.tt      |  16 +-
 t/db_dependent/Holds/Holdfine.t               | 187 ++++++++++++++++++
 5 files changed, 226 insertions(+), 3 deletions(-)
 create mode 100755 t/db_dependent/Holds/Holdfine.t

diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm
index 9bf1ab860e..7c084f7f6e 100644
--- a/Koha/CirculationRules.pm
+++ b/Koha/CirculationRules.pm
@@ -79,6 +79,9 @@ our $RULE_KINDS = {
     cap_fine_to_replacement_price => {
         scope => [ 'branchcode', 'categorycode', 'itemtype' ],
     },
+    expire_reserves_charge => {
+        scope => [ 'branchcode', 'categorycode', 'itemtype' ],
+    },
     chargeperiod => {
         scope => [ 'branchcode', 'categorycode', 'itemtype' ],
     },
diff --git a/Koha/Hold.pm b/Koha/Hold.pm
index 8969e32839..c30baaf6f2 100644
--- a/Koha/Hold.pm
+++ b/Koha/Hold.pm
@@ -372,7 +372,25 @@ sub cancel {
             C4::Reserves::_FixPriority({ biblionumber => $self->biblionumber });
 
             # and, if desired, charge a cancel fee
-            my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");
+            my $charge;
+            if (my $item = $self->item) {
+                my $branchcode = C4::Reserves::GetReservesControlBranch($item, $self->borrower);
+                # C4::Reserves::GetReservesControlBranch($item->unblessed, $self->borrower->unblessed);
+
+                my $rule = Koha::CirculationRules->get_effective_rule(
+                    {
+                        categorycode => $self->borrower->categorycode,
+                        itemtype     => $item->effective_itemtype,
+                        branchcode   => $branchcode,
+                        rule_name    => 'expire_reserves_charge',
+                    }
+                );
+                my $rule_value = $rule && $rule->rule_value // '';
+                $charge = $rule_value ne '' ? $rule_value : C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");
+            } else {
+                $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge");
+            }
+
             if ( $charge && $params->{'charge_cancel_fee'} ) {
                 my $account =
                   Koha::Account->new( { patron_id => $self->borrowernumber } );
diff --git a/admin/smart-rules.pl b/admin/smart-rules.pl
index 04ddb2698a..4db01633bf 100755
--- a/admin/smart-rules.pl
+++ b/admin/smart-rules.pl
@@ -110,6 +110,7 @@ if ($op eq 'delete') {
                 opacitemholds                    => undef,
                 overduefinescap                  => undef,
                 cap_fine_to_replacement_price    => undef,
+                expire_reserves_charge           => undef,
                 article_requests                 => undef,
                 note                             => undef,
             }
@@ -287,6 +288,7 @@ elsif ($op eq 'add') {
     my $article_requests = $input->param('article_requests') || 'no';
     my $overduefinescap = $input->param('overduefinescap') || '';
     my $cap_fine_to_replacement_price = ($input->param('cap_fine_to_replacement_price') || '') eq 'on';
+    my $expire_reserves_charge = $input->param('expire_reserves_charge') // '';
     my $note = $input->param('note');
     $debug and warn "Adding $br, $bor, $itemtype, $fine, $maxissueqty, $maxonsiteissueqty, $cap_fine_to_replacement_price";
 
@@ -319,6 +321,7 @@ elsif ($op eq 'add') {
         opacitemholds                 => $opacitemholds,
         overduefinescap               => $overduefinescap,
         cap_fine_to_replacement_price => $cap_fine_to_replacement_price,
+        expire_reserves_charge        => $expire_reserves_charge,
         article_requests              => $article_requests,
         note                          => $note,
     };
diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
index b34d02e2fa..ca2c4fd6b9 100644
--- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
+++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/smart-rules.tt
@@ -108,6 +108,7 @@
                 <th>Fine grace period</th>
                 <th>Overdue fines cap (amount)</th>
                 <th>Cap fine at replacement price</th>
+                <th>Expire reserve charge</th>
                 <th>Suspension in days (day)</th>
                 <th>Max. suspension duration (day)</th>
                 <th>Suspension charging interval</th>
@@ -147,6 +148,7 @@
                         [% SET firstremind = all_rules.$c.$i.firstremind %]
                         [% SET overduefinescap = all_rules.$c.$i.overduefinescap %]
                         [% SET cap_fine_to_replacement_price = all_rules.$c.$i.cap_fine_to_replacement_price %]
+                        [% SET expire_reserves_charge = all_rules.$c.$i.expire_reserves_charge %]
                         [% SET finedays = all_rules.$c.$i.finedays %]
                         [% SET maxsuspensiondays = all_rules.$c.$i.maxsuspensiondays %]
                         [% SET suspension_chargeperiod = all_rules.$c.$i.suspension_chargeperiod %]
@@ -164,7 +166,7 @@
                         [% SET article_requests = all_rules.$c.$i.article_requests %]
                         [% SET rentaldiscount = all_rules.$c.$i.rentaldiscount %]
 
-                        [% SET show_rule = note || maxissueqty || maxonsiteissueqty || issuelength || daysmode || lengthunit || hardduedate || hardduedatecompare || fine || chargeperiod || chargeperiod_charge_at || firstremind || overduefinescap || cap_fine_to_replacement_price || finedays || maxsuspensiondays || suspension_chargeperiod || renewalsallowed || renewalperiod || norenewalbefore || auto_renew || no_auto_renewal_after || no_auto_renewal_after_hard_limit || reservesallowed || holds_per_day || holds_per_record || onshelfholds || opacitemholds || article_requests || rentaldiscount %]
+                        [% SET show_rule = note || maxissueqty || maxonsiteissueqty || issuelength || daysmode || lengthunit || hardduedate || hardduedatecompare || fine || chargeperiod || chargeperiod_charge_at || firstremind || overduefinescap || cap_fine_to_replacement_price || expire_reserves_charge || finedays || maxsuspensiondays || suspension_chargeperiod || renewalsallowed || renewalperiod || norenewalbefore || auto_renew || no_auto_renewal_after || no_auto_renewal_after_hard_limit || reservesallowed || holds_per_day || holds_per_record || onshelfholds || opacitemholds || article_requests || rentaldiscount %]
                         [% IF show_rule %]
                             [% SET row_count = row_count + 1 %]
                             <tr row_countd="row_[% row_count | html %]">
@@ -254,6 +256,13 @@
                                             <input type="checkbox" disabled="disabled" />
                                         [% END %]
                                     </td>
+                                    <td>
+                                        [% IF expire_reserves_charge.defined && expire_reserves_charge != '' %]
+                                            [% expire_reserves_charge | html %]
+                                        [% ELSE %]
+                                            <a href="/cgi-bin/koha/admin/preferences.pl?op=search&amp;searchfield=ExpireReservesMaxPickUpDelayCharge">Use global</a>
+                                        [% END %]
+                                    </td>
                                     <td>[% finedays | html %]</td>
                                     <td>[% maxsuspensiondays | html %]</td>
                                     <td>[% suspension_chargeperiod | html %]</td>
@@ -402,6 +411,7 @@
                     <td><input type="text" name="firstremind" id="firstremind" size="2" /> </td>
                     <td><input type="text" name="overduefinescap" id="overduefinescap" size="6" /> </td>
                     <td><input type="checkbox" name="cap_fine_to_replacement_price" id="cap_fine_to_replacement_price" /></td>
+                    <td><input type="text" name="expire_reserves_charge" id="expire_reserves_charge" size="6"/></td>
                     <td><input type="text" name="finedays" id="fined" size="3" /> </td>
                     <td><input type="text" name="maxsuspensiondays" id="maxsuspensiondays" size="3" /> </td>
                     <td><input type="text" name="suspension_chargeperiod" id="suspension_chargeperiod" size="3" /> </td>
@@ -471,6 +481,7 @@
                       <th>Fine grace period</th>
                       <th>Overdue fines cap (amount)</th>
                       <th>Cap fine at replacement price</th>
+                      <th>Expire reserve charge</th>
                       <th>Suspension in days (day)</th>
                       <th>Max. suspension duration (day)</th>
                       <th>Suspension charging interval</th>
@@ -1081,10 +1092,11 @@
                             // Remove potential previous input added
                             $(current_column).find("input").remove();
                             $(current_column).append("<input type='hidden' name='"+name+"' value='"+val+"' />");
-                        } else if ( i == 5 || i == 6 || i == 25 || i == 26 || i == 27 ) {
+                        } else if ( i == 5 || i == 6 || i == 16 || i == 26 || i == 27 || i == 28 ) {
                             // If the value is not an integer for
                             //     - "Current checkouts allowed"
                             //     - "Current on-site checkouts allowed"
+                            //     - "Expire reserve charge"
                             //     - "Holds allowed (total)"
                             //     - "Holds allowed (daily)"
                             //     - "Holds per record (count)"
diff --git a/t/db_dependent/Holds/Holdfine.t b/t/db_dependent/Holds/Holdfine.t
new file mode 100755
index 0000000000..9d897545ad
--- /dev/null
+++ b/t/db_dependent/Holds/Holdfine.t
@@ -0,0 +1,187 @@
+#!/usr/bin/perl
+
+# Copyright The National Library of Finland, University of Helsinki 2020
+# Copyright Petro Vashchuk <stalkernoid@gmail.com> 2020
+#
+# This file 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 the Free Software
+# Foundation; either version 3 of the License, or (at your option) any later
+# version.
+#
+# Koha is distributed in the hope that it will be useful, but WITHOUT ANY
+# WARRANTY; without even the implied warranty of MERCHANTABILITY or FITNESS FOR
+# A PARTICULAR PURPOSE. See the GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License along
+# with Koha; if not, see <http://www.gnu.org/licenses>.
+
+use Modern::Perl;
+
+use C4::Context;
+use Koha::CirculationRules;
+
+use Test::More tests => 10;
+
+use t::lib::TestBuilder;
+use t::lib::Mocks;
+use Koha::Holds;
+
+use Koha::Account;
+use Koha::Account::DebitTypes;
+
+BEGIN {
+    use_ok('C4::Reserves');
+}
+
+my $schema = Koha::Database->schema;
+$schema->storage->txn_begin;
+my $dbh = C4::Context->dbh;
+
+my $builder = t::lib::TestBuilder->new;
+
+my $library1 = $builder->build({
+    source => 'Branch',
+});
+
+my $bib_title = "Test Title";
+
+my $borrower = $builder->build({
+    source => 'Borrower',
+    value => {
+        branchcode => $library1->{branchcode},
+    }
+});
+
+my $borrowernumber = $borrower->{borrowernumber};
+my $library_A_code = $library1->{branchcode};
+
+my $biblio = $builder->build_sample_biblio({itemtype=>'BK'});
+my $biblionumber = $biblio->biblionumber;
+my $item1 = $builder->build_sample_item({
+    biblionumber => $biblionumber,
+    itype => 'CF',
+    homebranch => $library_A_code,
+    holdingbranch => $library_A_code
+});
+my $item2 = $builder->build_sample_item({
+    biblionumber => $biblionumber,
+    itype => 'MU',
+    homebranch => $library_A_code,
+    holdingbranch => $library_A_code
+});
+my $item3 = $builder->build_sample_item({
+    biblionumber => $biblionumber,
+    itype => 'MX',
+    homebranch => $library_A_code,
+    holdingbranch => $library_A_code
+});
+
+$dbh->do("DELETE FROM circulation_rules");
+Koha::CirculationRules->set_rules(
+    {
+        itemtype     => 'CF',
+        categorycode => undef,
+        branchcode   => undef,
+        rules        => {
+            expire_reserves_charge => '111'
+        }
+    }
+);
+Koha::CirculationRules->set_rules(
+    {
+        itemtype     => 'MU',
+        categorycode => undef,
+        branchcode   => undef,
+        rules        => {
+            expire_reserves_charge => undef
+        }
+    }
+);
+
+my $reserve_id;
+my $account;
+my $status;
+my $start_balance;
+
+# TEST: Hold Computer File item
+$reserve_id = AddReserve(
+    {
+        branchcode       => $library_A_code,
+        borrowernumber   => $borrowernumber,
+        biblionumber     => $biblionumber,
+        priority         => 1,
+        itemnumber       => $item1->itemnumber,
+    }
+);
+
+$account = Koha::Account->new({ patron_id => $borrowernumber });
+
+( $status ) = CheckReserves($item1->id);
+is( $status, 'Reserved', "Hold for the CF created" );
+
+$start_balance = $account->balance();
+
+Koha::Holds->find( $reserve_id )->cancel({ charge_cancel_fee => 1 });
+
+( $status ) = CheckReserves($item1->id);
+is( $status, '', "Hold for the CF cancelled" );
+
+is( $account->balance() - $start_balance, 111, "Account debt is 111" );
+
+# TEST: Hold Music item that has 'expire_reserves_charge' set undef
+t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 222);
+
+$reserve_id = AddReserve(
+    {
+        branchcode       => $library_A_code,
+        borrowernumber   => $borrowernumber,
+        biblionumber     => $biblionumber,
+        priority         => 1,
+        itemnumber       => $item2->itemnumber,
+    }
+);
+
+$account = Koha::Account->new({ patron_id => $borrowernumber });
+
+( $status ) = CheckReserves($item2->id);
+is( $status, 'Reserved', "Hold for the MU created" );
+
+$start_balance = $account->balance();
+
+Koha::Holds->find( $reserve_id )->cancel({ charge_cancel_fee => 1 });
+
+( $status ) = CheckReserves($item2->id);
+is( $status, '', "Hold for the MU cancelled" );
+
+is( $account->balance() - $start_balance, 222, "Account debt is 222" );
+
+# TEST: Hold MX item that has no circulation rules
+t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 333);
+
+$reserve_id = AddReserve(
+    {
+        branchcode       => $library_A_code,
+        borrowernumber   => $borrowernumber,
+        biblionumber     => $biblionumber,
+        priority         => 1,
+        itemnumber       => $item3->itemnumber,
+    }
+);
+
+$account = Koha::Account->new({ patron_id => $borrowernumber });
+
+( $status ) = CheckReserves($item3->id);
+is( $status, 'Reserved', "Hold for the MX created" );
+
+$start_balance = $account->balance();
+
+Koha::Holds->find( $reserve_id )->cancel({ charge_cancel_fee => 1 });
+
+( $status ) = CheckReserves($item3->id);
+is( $status, '', "Hold for the MX cancelled" );
+
+is( $account->balance() - $start_balance, 333, "Account debt is 333" );
+
+$schema->storage->txn_rollback;
-- 
2.17.1