Bugzilla – Attachment 176245 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: (QA follow-up) Move tests and disallow blank rule
Bug-25711-QA-follow-up-Move-tests-and-disallow-bla.patch (text/plain), 21.17 KB, created by
Peter Vashchuk
on 2025-01-08 15:55:58 UTC
(
hide
)
Description:
Bug 25711: (QA follow-up) Move tests and disallow blank rule
Filename:
MIME Type:
Creator:
Peter Vashchuk
Created:
2025-01-08 15:55:58 UTC
Size:
21.17 KB
patch
obsolete
>From 6c6b3e8ac5997cb1dbfe3b2abaa918ea21f757f1 Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Fri, 30 Jul 2021 13:54:38 +0000 >Subject: [PATCH] Bug 25711: (QA follow-up) Move tests and disallow blank rule > >The tests can go where the existing cancellation charge tests are. >If we define the default rule we do not need to delete all circulation rules >as the itemtypes and branches are all created for the tests > >The rule should not be saved if blank, this allows simplifying the logic >when getting the charge >--- > Koha/CirculationRules.pm | 1 + > Koha/Hold.pm | 3 +- > t/db_dependent/Holds/Holdfine.t | 258 ------------------------------ > t/db_dependent/Koha/Holds.t | 271 ++++++++++++++++++++++++++++---- > 4 files changed, 245 insertions(+), 288 deletions(-) > delete mode 100755 t/db_dependent/Holds/Holdfine.t > >diff --git a/Koha/CirculationRules.pm b/Koha/CirculationRules.pm >index 2acc63b7c7..72be23f642 100644 >--- a/Koha/CirculationRules.pm >+++ b/Koha/CirculationRules.pm >@@ -98,6 +98,7 @@ our $RULE_KINDS = { > }, > expire_reserves_charge => { > scope => [ 'branchcode', 'categorycode', 'itemtype' ], >+ can_be_blank => 0, > }, > chargeperiod => { > scope => [ 'branchcode', 'categorycode', 'itemtype' ], >diff --git a/Koha/Hold.pm b/Koha/Hold.pm >index 1602c2f9ec..ea7bcac62c 100644 >--- a/Koha/Hold.pm >+++ b/Koha/Hold.pm >@@ -798,8 +798,7 @@ sub cancel { > rule_name => 'expire_reserves_charge', > } > ); >- my $rule_value = $rule && $rule->rule_value // ''; >- $charge = $rule_value ne '' ? $rule_value : C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); >+ $charge = $rule ? $rule->rule_value : C4::Context->preference("ExpireReservesMaxPickUpDelayCharge"); > > my $account = > Koha::Account->new( { patron_id => $self->borrowernumber } ); >diff --git a/t/db_dependent/Holds/Holdfine.t b/t/db_dependent/Holds/Holdfine.t >deleted file mode 100755 >index 400410bd70..0000000000 >--- a/t/db_dependent/Holds/Holdfine.t >+++ /dev/null >@@ -1,258 +0,0 @@ >-#!/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 => 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; >diff --git a/t/db_dependent/Koha/Holds.t b/t/db_dependent/Koha/Holds.t >index e1d90102dc..de8ec9fc18 100755 >--- a/t/db_dependent/Koha/Holds.t >+++ b/t/db_dependent/Koha/Holds.t >@@ -23,7 +23,7 @@ use Test::More tests => 12; > use Test::Warn; > > use C4::Circulation qw( AddIssue ); >-use C4::Reserves qw( AddReserve ModReserve ModReserveCancelAll ); >+use C4::Reserves qw( AddReserve CheckReserves ModReserve ModReserveCancelAll ); > use Koha::AuthorisedValueCategory; > use Koha::Biblio::ItemGroups; > use Koha::Database; >@@ -134,38 +134,253 @@ subtest 'cancel' => sub { > is( $third_hold->discard_changes->priority, 2, 'Third hold should now be second' ); > > subtest 'charge_cancel_fee parameter' => sub { >- plan tests => 4; >- my $patron_category = $builder->build_object({ class => 'Koha::Patron::Categories', value => { reservefee => 0 } } ); >- my $patron = $builder->build_object({ class => 'Koha::Patrons', value => { categorycode => $patron_category->categorycode } }); >- is( $patron->account->balance, 0, 'A new patron does not have any charges' ); >+ plan tests => 15; >+ 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 >+ }); >+ >+ Koha::CirculationRules->set_rules( >+ { >+ itemtype => undef, >+ categorycode => undef, >+ branchcode => undef, >+ rules => { >+ expire_reserves_charge => undef >+ } >+ } >+ ); >+ 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' >+ } >+ } >+ ); > >- my $hold_info = { >- branchcode => $library->branchcode, >- borrowernumber => $patron->borrowernumber, >- biblionumber => $item->biblionumber, >- priority => 1, >- title => "title for fee", >- itemnumber => $item->itemnumber, >- }; >+ 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" ); >+ >+ $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" ); > >- # First, test cancelling a reserve when there's no charge configured. >- t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 0); >- my $reserve_id = C4::Reserves::AddReserve( $hold_info ); >- Koha::Holds->find( $reserve_id )->cancel( { charge_cancel_fee => 1 } ); >- is( $patron->account->balance, 0, 'ExpireReservesMaxPickUpDelayCharge=0 - The patron should not have been charged' ); >+ $start_balance = $account->balance(); > >- # Then, test cancelling a reserve when there's no charge desired. >- t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 42); >- $reserve_id = C4::Reserves::AddReserve( $hold_info ); >- Koha::Holds->find( $reserve_id )->cancel(); # charge_cancel_fee => 0 >- is( $patron->account->balance, 0, 'ExpireReservesMaxPickUpDelayCharge=42, but charge_cancel_fee => 0, The patron should not have been charged' ); >+ Koha::Holds->find( $reserve_id )->cancel({ charge_cancel_fee => 0 }); > >+ ( $status ) = CheckReserves($item4->id); >+ is( $status, '', "Hold for the itemtype4 cancelled" ); > >- # Finally, test cancelling a reserve when there's a charge desired and configured. >- t::lib::Mocks::mock_preference('ExpireReservesMaxPickUpDelayCharge', 42); >- $reserve_id = C4::Reserves::AddReserve( $hold_info ); >- Koha::Holds->find( $reserve_id )->cancel( { charge_cancel_fee => 1 } ); >- is( int($patron->account->balance), 42, 'ExpireReservesMaxPickUpDelayCharge=42 and charge_cancel_fee => 1, The patron should have been charged!' ); >+ is( $account->balance() - $start_balance, 0, "Patron not charged when charge_cancel_fee is 0" ); > }; > > subtest 'waiting hold' => sub { >-- >2.47.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