From a669f6ae6b3d007ddc40b44469c4781863adfd52 Mon Sep 17 00:00:00 2001 From: Nick Clemens 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 Signed-off-by: Martin Renvoize Signed-off-by: Kyle M Hall --- 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 9f2952d035..34d6e018ba 100644 --- a/Koha/CirculationRules.pm +++ b/Koha/CirculationRules.pm @@ -93,6 +93,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 764b1dce0a..023a978639 100644 --- a/Koha/Hold.pm +++ b/Koha/Hold.pm @@ -709,8 +709,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 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; diff --git a/t/db_dependent/Koha/Holds.t b/t/db_dependent/Koha/Holds.t index aec261dd1f..1413c29c71 100755 --- a/t/db_dependent/Koha/Holds.t +++ b/t/db_dependent/Koha/Holds.t @@ -23,7 +23,7 @@ use Test::More tests => 8; 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::Database; use Koha::DateUtils qw( dt_from_string ); @@ -132,38 +132,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.37.3