Bugzilla – Attachment 107844 Details for
Bug 25711
Move ExpireReservesMaxPickUpDelayCharge to the circulation rules
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Bug-25711-add-ExpireReservesMaxPickUpDelayCharge-t.patch (text/plain), 17.50 KB, created by
Peter Vashchuk
on 2020-08-05 15:48:09 UTC
(
hide
)
Description:
Bug 25711: add ExpireReservesMaxPickUpDelayCharge to circulation rules
Filename:
MIME Type:
Creator:
Peter Vashchuk
Created:
2020-08-05 15:48:09 UTC
Size:
17.50 KB
patch
obsolete
>From c24e5a8c92480a2d1c2fe0dd52a1c97422727494 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 | 186 ++++++++++++++++++ > 5 files changed, 225 insertions(+), 3 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..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 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/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..b03b59efd8 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 @@ > <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> >@@ -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 %] > <tr row_countd="row_[% row_count | html %]"> >@@ -246,6 +248,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&searchfield=ExpireReservesMaxPickUpDelayCharge">Use global</a> >+ [% END %] >+ </td> > <td>[% finedays | html %]</td> > <td>[% maxsuspensiondays | html %]</td> > <td>[% suspension_chargeperiod | html %]</td> >@@ -381,6 +390,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> >@@ -448,6 +458,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> >@@ -1045,10 +1056,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..2fb31e0082 >--- /dev/null >+++ b/t/db_dependent/Holds/Holdfine.t >@@ -0,0 +1,186 @@ >+#!/usr/bin/perl >+ >+# Copyright The National Library of Finland, University of Helsinki 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
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 25711
:
107144
|
107407
|
107683
|
107844
|
108689
|
119851
|
119911
|
119912
|
120164
|
123309
|
123310
|
123326
|
123327
|
123328
|
123452
|
123456
|
123498
|
123568
|
123610
|
123653
|
123661
|
135060
|
135061
|
135062
|
135063
|
135064
|
135065
|
135156
|
135157
|
135158
|
135159
|
135160
|
135161
|
135950
|
135951
|
135952
|
135953
|
135954
|
135955
|
136769
|
140377
|
140378
|
140379
|
140380
|
140381
|
140382
|
140383
|
141270
|
157632
|
157633
|
157634
|
157635
|
157636
|
157637
|
157638
|
157639
|
171054
|
171055
|
171056
|
171057
|
171058
|
171059
|
176243
|
176244
|
176245
|
176246
|
176247
|
176248
|
176408
|
176409
|
176410
|
176411
|
176412
|
176413
|
176414
|
176415
|
176416
|
176417
|
176418
|
176419
|
176420
|
176421