From b5b7c77b8128619c5303817b689c04cd4bd63bd2 Mon Sep 17 00:00:00 2001 From: Petro Vashchuk 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, in that case global ExpireReservesMaxPickUpDelayCharge value will be used). Mentored-by: Andrew Nugged Signed-off-by: Martin Renvoize --- Koha/CirculationRules.pm | 3 + Koha/Hold.pm | 19 +- admin/smart-rules.pl | 3 + .../prog/en/modules/admin/smart-rules.tt | 16 +- t/db_dependent/Holds/Holdfine.t | 258 ++++++++++++++++++ 5 files changed, 296 insertions(+), 3 deletions(-) create mode 100755 t/db_dependent/Holds/Holdfine.t diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm index df1314613e..d677198e09 100644 --- a/Koha/CirculationRules.pm +++ b/Koha/CirculationRules.pm @@ -82,6 +82,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 a7455a44a4..7a57b28c98 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -539,7 +539,24 @@ 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->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 b1fa57b4da..68e4a20358 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, } @@ -288,6 +289,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'); my $decreaseloanholds = $input->param('decreaseloanholds') || undef; $debug and warn "Adding $br, $bor, $itemtype, $fine, $maxissueqty, $maxonsiteissueqty, $cap_fine_to_replacement_price"; @@ -322,6 +324,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, decreaseloanholds => $decreaseloanholds, 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 79a87ae164..147d1d14ab 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 @@ -123,6 +123,7 @@ Fine grace period Overdue fines cap (amount) Cap fine at replacement price + Expire reserve charge Suspension in days (day) Max. suspension duration (day) Suspension charging interval @@ -165,6 +166,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 %] @@ -184,7 +186,7 @@ [% SET rentaldiscount = all_rules.$c.$i.rentaldiscount %] [% SET decreaseloanholds = all_rules.$c.$i.decreaseloanholds %] - [% 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 || unseenrenewalsallowed || 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 || decreaseloanholds %] + [% 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 || unseenrenewalsallowed || 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 || decreaseloanholds %] [% IF show_rule %] [% SET row_count = row_count + 1 %] @@ -275,6 +277,13 @@ [% END %] + + [% IF expire_reserves_charge.defined && expire_reserves_charge != '' %] + [% expire_reserves_charge | html %] + [% ELSE %] + Use global + [% END %] + [% finedays | html %] [% maxsuspensiondays | html %] [% suspension_chargeperiod | html %] @@ -427,6 +436,7 @@ + @@ -500,6 +510,7 @@ Fine grace period Overdue fines cap (amount) Cap fine at replacement price + Expire reserve charge Suspension in days (day) Max. suspension duration (day) Suspension charging interval @@ -1138,10 +1149,11 @@ // Remove potential previous input added $(current_column).find("input").remove(); $(current_column).append(""); - } else if ( i == 5 || i == 6 || i == 26 || i == 27 || i == 28 ) { + } else if ( i == 5 || i == 6 || i == 17 || i == 27 || i == 28 || i == 29 ) { // 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..400410bd70 --- /dev/null +++ b/t/db_dependent/Holds/Holdfine.t @@ -0,0 +1,258 @@ +#!/usr/bin/perl + +# Copyright The National Library of Finland, University of Helsinki 2020 +# Copyright Petro Vashchuk 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 . + +use Modern::Perl; + +use C4::Context; +use Koha::CirculationRules; + +use Test::More tests => 13; + +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 $library2 = $builder->build({ + source => 'Branch', +}); + +my $bib_title = "Test Title"; + +my $borrower = $builder->build({ + source => 'Borrower', + value => { + branchcode => $library1->{branchcode}, + } +}); + +my $itemtype1 = $builder->build({ + source => 'Itemtype', + value => {} +}); +my $itemtype2 = $builder->build({ + source => 'Itemtype', + value => {} +}); +my $itemtype3 = $builder->build({ + source => 'Itemtype', + value => {} +}); +my $itemtype4 = $builder->build({ + source => 'Itemtype', + value => {} +}); + +my $borrowernumber = $borrower->{borrowernumber}; + +my $library_A_code = $library1->{branchcode}; + +my $biblio = $builder->build_sample_biblio({itemtype => $itemtype1->{itemtype}}); +my $biblionumber = $biblio->biblionumber; +my $item1 = $builder->build_sample_item({ + biblionumber => $biblionumber, + itype => $itemtype1->{itemtype}, + homebranch => $library_A_code, + holdingbranch => $library_A_code +}); +my $item2 = $builder->build_sample_item({ + biblionumber => $biblionumber, + itype => $itemtype2->{itemtype}, + homebranch => $library_A_code, + holdingbranch => $library_A_code +}); +my $item3 = $builder->build_sample_item({ + biblionumber => $biblionumber, + itype => $itemtype3->{itemtype}, + homebranch => $library_A_code, + holdingbranch => $library_A_code +}); + +my $library_B_code = $library2->{branchcode}; + +my $biblio2 = $builder->build_sample_biblio({itemtype => $itemtype4->{itemtype}}); +my $biblionumber2 = $biblio2->biblionumber; +my $item4 = $builder->build_sample_item({ + biblionumber => $biblionumber2, + itype => $itemtype4->{itemtype}, + homebranch => $library_B_code, + holdingbranch => $library_B_code +}); + +$dbh->do("DELETE FROM circulation_rules"); +Koha::CirculationRules->set_rules( + { + itemtype => $itemtype1->{itemtype}, + categorycode => undef, + branchcode => undef, + rules => { + expire_reserves_charge => '111' + } + } +); +Koha::CirculationRules->set_rules( + { + itemtype => $itemtype2->{itemtype}, + categorycode => undef, + branchcode => undef, + rules => { + expire_reserves_charge => undef + } + } +); +Koha::CirculationRules->set_rules( + { + itemtype => undef, + categorycode => undef, + branchcode => $library_B_code, + rules => { + expire_reserves_charge => '444' + } + } +); + +t::lib::Mocks::mock_preference('ReservesControlBranch', 'ItemHomeLibrary'); + +my $reserve_id; +my $account; +my $status; +my $start_balance; + +# TEST: Hold itemtype1 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 itemtype1 created" ); + +$start_balance = $account->balance(); + +Koha::Holds->find( $reserve_id )->cancel({ charge_cancel_fee => 1 }); + +( $status ) = CheckReserves($item1->id); +is( $status, '', "Hold for the itemtype1 cancelled" ); + +is( $account->balance() - $start_balance, 111, "Used circulation rule for itemtype1" ); + +# TEST: circulation rule for itemtype2 has 'expire_reserves_charge' set undef, so it should use ExpireReservesMaxPickUpDelayCharge preference +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 itemtype2 created" ); + +$start_balance = $account->balance(); + +Koha::Holds->find( $reserve_id )->cancel({ charge_cancel_fee => 1 }); + +( $status ) = CheckReserves($item2->id); +is( $status, '', "Hold for the itemtype2 cancelled" ); + +is( $account->balance() - $start_balance, 222, "Used ExpireReservesMaxPickUpDelayCharge preference as expire_reserves_charge set to undef" ); + +# TEST: no circulation rules for itemtype3, it should use ExpireReservesMaxPickUpDelayCharge preference +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 itemtype3 created" ); + +$start_balance = $account->balance(); + +Koha::Holds->find( $reserve_id )->cancel({ charge_cancel_fee => 1 }); + +( $status ) = CheckReserves($item3->id); +is( $status, '', "Hold for the itemtype3 cancelled" ); + +is( $account->balance() - $start_balance, 333, "Used ExpireReservesMaxPickUpDelayCharge preference as there's no circulation rules for itemtype3" ); + +# TEST: circulation rule for itemtype4 with library_B_code +t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 555); + +$reserve_id = AddReserve( + { + branchcode => $library_B_code, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber2, + priority => 1, + itemnumber => $item4->itemnumber, + } +); + +$account = Koha::Account->new({ patron_id => $borrowernumber }); + +( $status ) = CheckReserves($item4->id); +is( $status, 'Reserved', "Hold for the itemtype4 created" ); + +$start_balance = $account->balance(); + +Koha::Holds->find( $reserve_id )->cancel({ charge_cancel_fee => 1 }); + +( $status ) = CheckReserves($item4->id); +is( $status, '', "Hold for the itemtype4 cancelled" ); + +is( $account->balance() - $start_balance, 444, "Used circulation rule for itemtype4 with library_B_code" ); + +$schema->storage->txn_rollback; -- 2.20.1