From 91c087cebcab7b005ced31c6deb9519dad3b3abf Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Thu, 3 Nov 2016 16:12:38 +0000 Subject: [PATCH] Bug 17560: Hold fee placement at point of checkout Currently, Koha charges all patrons a hold fee in all circumstances, if a hold fee is applicable to their patron category. This is immediately applied at point of request. However, it would be useful to let patrons make requests without a charge being incurred until they physically have the item in their hands and checked out to their cards. The hold fee will only be added to the account as soon as the item is checked out to the requesting patron. With this scenario, we will be certain that patrons have the correct item, and they are happy with what has been supplied. It also means that patrons can place holds via the OPAC without reaching the usage limit that has been selected. Test plan: 0/ All the following steps must be done with a patron using a patron category with a hold fee 1/ Make sure that the existing options for HoldFeeMode work as before 2/ Select the third option "any time a hold is collected" 3/ Place a hold on an item 4/ Note that the patron has not been charged 5/ Check this item from the staff interface 6/ Note that the patron has been charged 7/ Place another hold 8/ Use the self checkout feature at the OPAC for the checkin 9/ Note that the patron has been charged and a message is displayed to inform about the fee. Sponsored-by: Cheshire Libraries Signed-off-by: Josef Moravec Signed-off-by: Marcel de Rooy Signed-off-by: Kyle M Hall --- C4/Reserves.pm | 11 ++- t/db_dependent/Reserves/GetReserveFee.t | 106 +++++++++++++++++++----- 2 files changed, 96 insertions(+), 21 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index b42440712b..fff932cbf7 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -219,8 +219,10 @@ sub AddReserve { my $reserve_id = $hold->id(); # add a reserve fee if needed - my $fee = GetReserveFee( $borrowernumber, $biblionumber ); - ChargeReserveFee( $borrowernumber, $fee, $title ); + if ( C4::Context->preference('HoldFeeMode') ne 'any_time_is_collected' ) { + my $reserve_fee = GetReserveFee( $borrowernumber, $biblionumber ); + ChargeReserveFee( $borrowernumber, $reserve_fee, $title ); + } _FixPriority({ biblionumber => $biblionumber}); @@ -1181,6 +1183,11 @@ sub ModReserveFill { $hold->delete(); + if ( C4::Context->preference('HoldFeeMode') eq 'any_time_is_collected' ) { + my $reserve_fee = GetReserveFee( $hold->borrowernumber ); + ChargeReserveFee( $hold->borrowernumber, $reserve_fee, $hold->biblio->title ); + } + # now fix the priority on the others (if the priority wasn't # already sorted!).... unless ( $priority == 0 ) { diff --git a/t/db_dependent/Reserves/GetReserveFee.t b/t/db_dependent/Reserves/GetReserveFee.t index b86204f06e..31e45bb6a6 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 => 2; +use Test::More tests => 3; use Test::MockModule; use t::lib::TestBuilder; use t::lib::Mocks; @@ -64,6 +64,9 @@ my $patron2 = $builder->build({ categorycode => 'XYZ1', }, }); +my $patron3 = $builder->build({ + source => 'Borrower', +}); # One biblio and two items my $biblio = $builder->build({ @@ -76,30 +79,23 @@ my $item1 = $builder->build({ source => 'Item', value => { biblionumber => $biblio->{biblionumber}, + notforloan => 0, }, }); my $item2 = $builder->build({ source => 'Item', value => { biblionumber => $biblio->{biblionumber}, + notforloan => 0, }, }); - -# Actual testing starts here! -# Add reserve for patron1, no fee expected -# Note: AddReserve calls GetReserveFee and ChargeReserveFee -my $acc1 = acctlines( $patron1->{borrowernumber} ); -my $res1 = addreserve( $patron1->{borrowernumber} ); -is( acctlines( $patron1->{borrowernumber} ), $acc1, 'No fee charged for patron 1' ); - subtest 'GetReserveFee' => sub { - plan tests => 7; + plan tests => 5; - # 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} ); + my $res1 = addreserve( $patron1->{borrowernumber} ); t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always'); my $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); @@ -108,7 +104,7 @@ subtest 'GetReserveFee' => sub { 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 ) ); + $dbh->do( "DELETE FROM reserves WHERE borrowernumber = ?", undef, ( $patron1->{borrowernumber}) ); $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->{biblionumber} ); is( $fee, 0, 'HoldFeeMode=not_always, Patron 2 should not be charged' ); @@ -119,16 +115,88 @@ subtest 'GetReserveFee' => sub { 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' ); +}; + +subtest 'Integration with AddReserve' => sub { + plan tests => 2; + + my $dbh = C4::Context->dbh; + + subtest 'Items are not issued' => sub { + plan tests => 3; + + t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always'); + $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->{biblionumber} ); + $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} ); + addreserve( $patron1->{borrowernumber} ); + is( acctlines( $patron1->{borrowernumber} ), 0, 'not_always - No fee charged for patron 1 if not issued' ); + + t::lib::Mocks::mock_preference('HoldFeeMode', 'any_time_is_placed'); + $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->{biblionumber} ); + $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} ); + addreserve( $patron1->{borrowernumber} ); + is( acctlines( $patron1->{borrowernumber} ), 1, 'any_time_is_placed - Patron should be always charged' ); + + t::lib::Mocks::mock_preference('HoldFeeMode', 'any_time_is_collected'); + $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->{biblionumber} ); + $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} ); + addreserve( $patron1->{borrowernumber} ); + is( acctlines( $patron1->{borrowernumber} ), 0, 'any_time_is_collected - Patron should not be charged when placing a hold' ); + }; + + subtest 'Items are issued' => sub { + plan tests => 3; + + C4::Circulation::AddIssue( $patron2, $item1->{barcode}, '2015-12-31', 0, undef, 0, {} ); + + t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always'); + $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->{biblionumber} ); + $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} ); + addreserve( $patron1->{borrowernumber} ); + is( acctlines( $patron1->{borrowernumber} ), 0, 'not_always - Patron should not be charged if items are not all checked out' ); + + $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->{biblionumber} ); + $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} ); + addreserve( $patron3->{borrowernumber} ); + addreserve( $patron1->{borrowernumber} ); + # FIXME Are we sure it's the expected behavior? + is( acctlines( $patron1->{borrowernumber} ), 1, 'not_always - Patron should be charged if all the items are not checked out and at least 1 hold is already placed' ); + + C4::Circulation::AddIssue( $patron3, $item2->{barcode}, '2015-12-31', 0, undef, 0, {} ); + $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->{biblionumber} ); + $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} ); + addreserve( $patron1->{borrowernumber} ); + is( acctlines( $patron1->{borrowernumber} ), 1, 'not_always - Patron should be charged if all items are checked out' ); + }; +}; + +subtest 'Integration with AddIssue' => sub { + plan tests => 5; + + $dbh->do( "DELETE FROM issues WHERE borrowernumber = ?", undef, $patron1->{borrowernumber} ); + $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->{biblionumber} ); + $dbh->do( "DELETE FROM accountlines WHERE borrowernumber=?", undef, $patron1->{borrowernumber} ); + + t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always'); + C4::Circulation::AddIssue( $patron1, $item1->{barcode}, '2015-12-31', 0, undef, 0, {} ); + is( acctlines( $patron1->{borrowernumber} ), 0, 'not_always - Patron should not 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' ); + $dbh->do( "DELETE FROM issues WHERE borrowernumber = ?", undef, $patron1->{borrowernumber} ); + C4::Circulation::AddIssue( $patron1, $item1->{barcode}, '2015-12-31', 0, undef, 0, {} ); + is( acctlines( $patron1->{borrowernumber} ), 0, 'not_always - Patron should not 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' ); + $dbh->do( "DELETE FROM issues WHERE borrowernumber = ?", undef, $patron1->{borrowernumber} ); + C4::Circulation::AddIssue( $patron1, $item1->{barcode}, '2015-12-31', 0, undef, 0, {} ); + is( acctlines( $patron1->{borrowernumber} ), 0, 'any_time_is_collected - Patron should not be charged when checking out an item which was not placed hold for him' ); + + $dbh->do( "DELETE FROM issues WHERE borrowernumber = ?", undef, $patron1->{borrowernumber} ); + my $id = addreserve( $patron1->{borrowernumber} ); + my $r = C4::Reserves::GetReserveInfo($id); + is( acctlines( $patron1->{borrowernumber} ), 0, 'any_time_is_collected - Patron should not be charged yet (just checking to make sure)'); + C4::Circulation::AddIssue( $patron1, $item1->{barcode}, '2015-12-31', 0, undef, 0, {} ); + is( acctlines( $patron1->{borrowernumber} ), 1, 'any_time_is_collected - Patron should not be charged when checking out an item which was not placed hold for him' ); }; sub acctlines { #calculate number of accountlines for a patron -- 2.39.5