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 <josef.moravec@gmail.com>
Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index f73cc01..b424407 100644 (file)
--- 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 3b477ea..b86204f 100755 (executable)
--- 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] ) );