From e5d1f6c70835f23465af0a50821888b6211db37d Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 3 Nov 2016 13:20:17 +0000 Subject: [PATCH] Bug 17560: Update current code This patch updates the current code to make it works with the new option's name of the syspref. It also refactor the tests to make them more reusable and robust. Sponsored-by: Cheshire Libraries Signed-off-by: Josef Moravec Signed-off-by: Marcel de Rooy Signed-off-by: Kyle M Hall --- C4/Reserves.pm | 2 +- t/db_dependent/Reserves/GetReserveFee.t | 63 +++++++++++++++---------- 2 files changed, 39 insertions(+), 26 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index f73cc013d3..b42440712b 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -664,7 +664,7 @@ SELECT COUNT(*) FROM reserves WHERE biblionumber=? AND borrowernumber<>? my $dbh = C4::Context->dbh; my ( $fee ) = $dbh->selectrow_array( $borquery, undef, ($borrowernumber) ); my $hold_fee_mode = C4::Context->preference('HoldFeeMode') || 'not_always'; - if( $fee and $fee > 0 and $hold_fee_mode ne 'always' ) { + if( $fee and $fee > 0 and $hold_fee_mode eq 'not_always' ) { # This is a reconstruction of the old code: # Compare number of items with items issued, and optionally check holds # If not all items are issued and there are no holds: charge no fee diff --git a/t/db_dependent/Reserves/GetReserveFee.t b/t/db_dependent/Reserves/GetReserveFee.t index 3b477ea295..b86204f06e 100755 --- a/t/db_dependent/Reserves/GetReserveFee.t +++ b/t/db_dependent/Reserves/GetReserveFee.t @@ -21,7 +21,7 @@ use Modern::Perl; -use Test::More tests => 6; +use Test::More tests => 2; use Test::MockModule; use t::lib::TestBuilder; use t::lib::Mocks; @@ -93,30 +93,43 @@ my $acc1 = acctlines( $patron1->{borrowernumber} ); my $res1 = addreserve( $patron1->{borrowernumber} ); is( acctlines( $patron1->{borrowernumber} ), $acc1, 'No fee charged for patron 1' ); -# Issue item1 to patron1. Since there is still a reserve too, we should -# expect a charge for patron2. -C4::Circulation::AddIssue( $patron1, $item1->{barcode}, '2015-12-31', 0, undef, 0, {} ); # the date does not really matter -my $acc2 = acctlines( $patron2->{borrowernumber} ); -t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always'); -my $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); -is( $fee > 0, 1, 'Patron 2 should be charged cf GetReserveFee' ); -C4::Reserves::ChargeReserveFee( $patron2->{borrowernumber}, $fee, $biblio->{title} ); -is( acctlines( $patron2->{borrowernumber} ), $acc2 + 1, 'Patron 2 has been charged by ChargeReserveFee' ); - -# If we delete the reserve, there should be no charge -$dbh->do( "DELETE FROM reserves WHERE reserve_id=?", undef, ( $res1 ) ); -$fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); -is( $fee, 0, 'HoldFeeMode=not_always, Patron 2 should not be charged' ); - -t::lib::Mocks::mock_preference('HoldFeeMode', 'always'); -$fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); -is( int($fee), 2, 'HoldFeeMode=always, Patron 2 should be charged' ); - -# If we delete the second item, there should be a charge -$dbh->do( "DELETE FROM items WHERE itemnumber=?", undef, ( $item2->{itemnumber} ) ); -$fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); -is( int($fee), 2, 'Patron 2 should be charged again this time' ); -# End of tests +subtest 'GetReserveFee' => sub { + plan tests => 7; + + # Issue item1 to patron1. Since there is still a reserve too, we should + # expect a charge for patron2. + C4::Circulation::AddIssue( $patron1, $item1->{barcode}, '2015-12-31', 0, undef, 0, {} ); # the date does not really matter + my $acc2 = acctlines( $patron2->{borrowernumber} ); + + t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always'); + my $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); + is( $fee > 0, 1, 'Patron 2 should be charged cf GetReserveFee' ); + C4::Reserves::ChargeReserveFee( $patron2->{borrowernumber}, $fee, $biblio->{title} ); + is( acctlines( $patron2->{borrowernumber} ), $acc2 + 1, 'Patron 2 has been charged by ChargeReserveFee' ); + + # If we delete the reserve, there should be no charge + $dbh->do( "DELETE FROM reserves WHERE reserve_id=?", undef, ( $res1 ) ); + $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); + is( $fee, 0, 'HoldFeeMode=not_always, Patron 2 should not be charged' ); + + t::lib::Mocks::mock_preference('HoldFeeMode', 'any_time_is_placed'); + $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); + is( int($fee), 2, 'HoldFeeMode=any_time_is_placed, Patron 2 should be charged' ); + + t::lib::Mocks::mock_preference('HoldFeeMode', 'any_time_is_collected'); + $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); + is( int($fee), 2, 'HoldFeeMode=any_time_is_collected, Patron 2 should be charged' ); + + # If we delete the second item, there should be a charge + t::lib::Mocks::mock_preference('HoldFeeMode', 'any_time_is_placed'); + $dbh->do( "DELETE FROM items WHERE itemnumber=?", undef, ( $item2->{itemnumber} ) ); + $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); + is( int($fee), 2, 'Patron 2 should be charged again this time' ); + + t::lib::Mocks::mock_preference('HoldFeeMode', 'any_time_is_collected'); + $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); + is( int($fee), 2, 'HoldFeeMode=any_time_is_collected, Patron 2 should be charged' ); +}; sub acctlines { #calculate number of accountlines for a patron my @temp = $dbh->selectrow_array( "SELECT COUNT(*) FROM accountlines WHERE borrowernumber=?", undef, ( $_[0] ) ); -- 2.20.1