Bug 13592: Add an option to charge for any hold placed

Currently the fee is applied on if all items for the record are issued
and at least one hold already exists on the record.
This patch does not give a complete answer to the problem (see
discussion on bug 13592 for the other user's expectations).
It only adds the ability to charge for any hold placed regardless of the
conditions.

Test plan:
1) Execute the updatedb entry to insert the new pref
2) Confirm that the behavior is the same as before applying this patch
3) Change the HoldFeeMode pref to 'always'
4) Note that the fee is applied for any hold placed

Signed-off-by: Sally Healey <sally.healey@cheshiresharedservices.gov.uk>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

C4/Reserves.pm

@@ -708,7 +708,8 @@ SELECT COUNT(*) FROM reserves WHERE biblionumber=? AND borrowernumber<>?
 
     my $dbh = C4::Context->dbh;
     my ( $fee ) = $dbh->selectrow_array( $borquery, undef, ($borrowernumber) );
-    if( $fee && $fee > 0 ) {
+    my $hold_fee_mode = C4::Context->preference('HoldFeeMode') || 'not_always';
+    if( $fee and $fee > 0 and $hold_fee_mode ne '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

installer/data/mysql/atomicupdate/bug_13592_add_HoldFeeMode_pref.sql

@@ -0,0 +1 @@
+INSERT IGNORE INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `type` ) VALUES ('HoldFeeMode','not_always','always|not_always','Set the hold fee mode','Choice');

installer/data/mysql/sysprefs.sql

@@ -157,6 +157,7 @@ INSERT INTO systempreferences ( `variable`, `value`, `options`, `explanation`, `
 ('hide_marc','0',NULL,'If ON, disables display of MARC fields, subfield codes & indicators (still shows data)','YesNo'),
 ('HighlightOwnItemsOnOPAC','0','','If on, and a patron is logged into the OPAC, items from his or her home library will be emphasized and shown first in search results and item details.','YesNo'),
 ('HighlightOwnItemsOnOPACWhich','PatronBranch','PatronBranch|OpacURLBranch','Decides which branch\'s items to emphasize. If PatronBranch, emphasize the logged in user\'s library\'s items. If OpacURLBranch, highlight the items of the Apache var BRANCHCODE defined in Koha\'s Apache configuration file.','Choice'),
+('HoldFeeMode','not_always','always|not_always','Set the hold fee mode','Choice'),
 ('HoldsToPullStartDate','2',NULL,'Set the default start date for the Holds to pull list to this many days ago','Integer'),
 ('HomeOrHoldingBranch','holdingbranch','holdingbranch|homebranch','Used by Circulation to determine which branch of an item to check with independent branches on, and by search to determine which branch to choose for availability ','Choice'),
 ('HTML5MediaEnabled','not','not|opac|staff|both','Show a tab with a HTML5 media player for files catalogued in field 856','Choice'),

koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref

@@ -637,6 +637,12 @@ Circulation:
                   yes: Charge
                   no: "Don't Charge"
             - the replacement price when a patron loses an item.
+        -
+            - pref: HoldFeeMode
+              choices:
+                  always: Always
+                  not_always: "Don't always"
+            - Charge fee when a hold is placed.
     Self Checkout:
         -
             - "Include the following JavaScript on all pages in the web-based self checkout:"

t/db_dependent/Reserves/GetReserveFee.t

@@ -21,9 +21,10 @@
 
 use Modern::Perl;
 
-use Test::More tests => 5;
+use Test::More tests => 6;
 use Test::MockModule;
 use t::lib::TestBuilder;
+use t::lib::Mocks;
 
 use C4::Circulation;
 use C4::Reserves qw|AddReserve|;
@@ -48,7 +49,7 @@ $builder->build({
     source => 'Category',
     value  => {
         categorycode          => 'XYZ1',
-        reservefee            => 2.5,
+        reservefee            => 2,
     },
 });
 my $patron1 = $builder->build({
@@ -96,6 +97,7 @@ is( acctlines( $patron1->{borrowernumber} ), $acc1, 'No fee charged for patron 1
 # 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} );
@@ -104,12 +106,16 @@ is( acctlines( $patron2->{borrowernumber} ), $acc2 + 1, 'Patron 2 has been charg
 # 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, 'Patron 2 will not be charged now' );
+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( $fee > 0, 1, 'Patron 2 should be charged again this time' );
+is( int($fee), 2, 'Patron 2 should be charged again this time' );
 # End of tests
 
 sub acctlines { #calculate number of accountlines for a patron