Bugzilla – Attachment 57243 Details for
Bug 17560
Hold fee placement at point of checkout
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
[SIGNED-OFF] Bug 17560: Hold fee placement at point of checkout
SIGNED-OFF-Bug-17560-Hold-fee-placement-at-point-o.patch (text/plain), 11.31 KB, created by
Josef Moravec
on 2016-11-07 07:46:25 UTC
(
hide
)
Description:
[SIGNED-OFF] Bug 17560: Hold fee placement at point of checkout
Filename:
MIME Type:
Creator:
Josef Moravec
Created:
2016-11-07 07:46:25 UTC
Size:
11.31 KB
patch
obsolete
>From 21bb1fdbbb439771a039a65ae2b2018d97057a2f Mon Sep 17 00:00:00 2001 >From: Jonathan Druart <jonathan.druart@bugs.koha-community.org> >Date: Thu, 3 Nov 2016 16:12:38 +0000 >Subject: [PATCH] [SIGNED-OFF] 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 <josef.moravec@gmail.com> >--- > 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 9283d1a..ebbf2d0 100644 >--- a/C4/Reserves.pm >+++ b/C4/Reserves.pm >@@ -220,8 +220,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}); > >@@ -1250,6 +1252,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 b86204f..31e45bb 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.1.4
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=57243">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 17560
:
57233
|
57234
|
57235
|
57236
|
57237
|
57241
|
57242
|
57243
|
57244
|
57245
|
60993
|
61301
|
61669
|
61691
|
61744
|
61745
|
61746
|
61747
|
61748
|
61749
|
61750