From e887e24ead9695e17b627f5f2f878a7758dabed8 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 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Add a feature that allows to define ExpireReservesMaxPickUpDelayCharge in circulation rules per specific items/patron categories. When it’s undefined in circulation rules it uses global ExpireReservesMaxPickUpDelayCharge which was used before this 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 itemtype 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 no smart rule will be found or smart rules have an "empty" fee – global ExpireReservesMaxPickUpDelayCharge variable value will be used. IMPORTANT: Different actions when rules applied to biblio and item level holds: - if there is biblio-level hold ("next available") then biblio item type used for rule preselection - if there is direct item-level hold for particular barcode, then item "item type" will be used for rule preselection In most cases we have the same items as biblio item types, but in case there's different item types: be accurate and attentive with rules creation and overlapping. Mentored-by: Andrew Nugged --- Koha/CirculationRules.pm | 3 + Koha/Hold.pm | 22 +- admin/smart-rules.pl | 3 + installer/data/mysql/updatedatabase.pl | 1 + .../prog/en/modules/admin/smart-rules.tt | 17 +- t/db_dependent/Holds/Holdfine.t | 199 ++++++++++++++++++ 6 files changed, 241 insertions(+), 4 deletions(-) create mode 100755 t/db_dependent/Holds/Holdfine.t diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm index 790f30498d..e242cad26a 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..71290609f5 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -371,8 +371,28 @@ sub cancel { # now fix the priority on the others.... C4::Reserves::_FixPriority({ biblionumber => $self->biblionumber }); + my $item = $self->item; + + my $controlbranch = C4::Context->preference('ReservesControlBranch'); + my $branchcode = undef; + if ( $item && $controlbranch eq "ItemHomeLibrary" ) { + $branchcode = $item->homebranch; + } + elsif ( $controlbranch eq "PatronLibrary" ) { + $branchcode = $self->borrower->branchcode; + } + + my $rule = Koha::CirculationRules->get_effective_rule( + { + categorycode => $self->borrower->categorycode, + itemtype => $item ? $item->effective_itemtype : $self->biblio->itemtype, + branchcode => $branchcode, + rule_name => 'expire_reserves_charge', + } + ); # and, if desired, charge a cancel fee - my $charge = C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); + my $charge = $rule && $rule->rule_value // 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 0a33493623..90d0d283f4 100755 --- a/admin/smart-rules.pl +++ b/admin/smart-rules.pl @@ -111,6 +111,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'); $debug and warn "Adding $br, $bor, $itemtype, $fine, $maxissueqty, $maxonsiteissueqty, $cap_fine_to_replacement_price"; @@ -320,6 +322,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/installer/data/mysql/updatedatabase.pl b/installer/data/mysql/updatedatabase.pl index d2e18f7aba..36a6208029 100755 --- a/installer/data/mysql/updatedatabase.pl +++ b/installer/data/mysql/updatedatabase.pl @@ -20696,6 +20696,7 @@ if( CheckVersion( $DBversion ) ) { auto_renew no_auto_renewal_after no_auto_renewal_after_hard_limit + expire_reserves_charge reservesallowed holds_per_record holds_per_day 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 2b9f4513b2..67adbf141f 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 @@ -102,6 +102,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 @@ -141,6 +142,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 %] @@ -158,7 +160,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 %] @@ -246,6 +248,13 @@ [% END %] + + [% UNLESS expire_reserves_charge %] + Use global + [% ELSE %] + [% expire_reserves_charge | html %] + [% END %] + [% finedays | html %] [% maxsuspensiondays | html %] [% suspension_chargeperiod | html %] @@ -381,6 +390,7 @@ + @@ -448,6 +458,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 @@ -1020,7 +1031,7 @@ } $(current_column).find("input[type='text']").val(input_value); $(current_column).find("select").val(select_value); - } else if ( i == 15 ) { + } else if ( i == 16 ) { // specific processing for cap_fine_to_replacement_price var cap_fine_to_replacement_price = $(this).find("input[type='checkbox']"); $('#cap_fine_to_replacement_price').prop('checked', cap_fine_to_replacement_price.is(':checked') ); @@ -1045,7 +1056,7 @@ // Remove potential previous input added $(current_column).find("input").remove(); $(current_column).append(""); - } else if ( i == 5 || i == 6 || i == 25 || i == 26 || i == 27 ) { + } else if ( i == 5 || i == 6 || i == 26 || i == 27 || i == 28 ) { // If the value is not an integer for // - "Current checkouts allowed" // - "Current on-site checkouts allowed" diff --git a/t/db_dependent/Holds/Holdfine.t b/t/db_dependent/Holds/Holdfine.t new file mode 100755 index 0000000000..fcaa471ed8 --- /dev/null +++ b/t/db_dependent/Holds/Holdfine.t @@ -0,0 +1,199 @@ +#!/usr/bin/perl + +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 $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 => 'BK', + categorycode => undef, + branchcode => undef, + rules => { + expire_reserves_charge => '111' + } + } +); +Koha::CirculationRules->set_rules( + { + itemtype => 'CF', + categorycode => undef, + branchcode => undef, + rules => { + expire_reserves_charge => '222' + } + } +); +Koha::CirculationRules->set_rules( + { + itemtype => 'MU', + categorycode => undef, + branchcode => undef, + rules => { + expire_reserves_charge => undef + } + } +); + +# TEST: Hold first available +my $reserve_id = AddReserve( + { + branchcode => $library_A_code, + borrowernumber => $borrowernumber, + biblionumber => $biblionumber, + priority => 1, + } +); + +my $account = Koha::Account->new({ patron_id => $borrowernumber }); + +my ( $status ) = CheckReserves($item1->id); +is( $status, 'Reserved', "Hold for the first available created" ); + +my $start_balance = $account->balance(); + +Koha::Holds->find( $reserve_id )->cancel({ charge_cancel_fee => 1 }); + +( $status ) = CheckReserves($item1->id); +is( $status, '', "Hold for the first available cancelled" ); + +is( $account->balance() - $start_balance, 111, "Account debt is 111" ); + +# 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, 222, "Account debt is 222" ); + +# TEST: Hold Music item that has 'expire_reserves_charge' set undef +t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 333); + +$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, 333, "Account debt is 333" ); + +# TEST: Hold MX item that has no circulation rules +t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 444); + +$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, 444, "Account debt is 444" ); + +$schema->storage->txn_rollback; -- 2.17.1