From 68d299a73269cc2e16bdb73e549d2a6054136ecc Mon Sep 17 00:00:00 2001 From: Aleisha Amohia Date: Wed, 9 Nov 2022 01:30:55 +0000 Subject: [PATCH] Bug 32142: Implement issued_or_reserved HoldFeeMode option This enhancement adds an issued_or_reserved option to the HoldFeeMode system preference, so that a hold fee may be charged if either all items are check out, or the record has at least one hold already. To test: 1) Apply patch, update database, restart services 2) Go to Administration -> Patron categories 3) Edit Category A and add a hold fee of $1.00 4) Go to Administration -> System preferences and search for HoldFeeMode 5) Set HoldFeeMode to not_always - "only if all items are checked out and the record has at least one hold already" 6) In another tab, open the staff interface. Search for a record with one item attached to it (Biblio A) 7) Place a hold on this record for Patron B (any category). not_always, reserved but not issued 8) Place a hold on Biblio A for Patron A. (Make sure Patron A is of Category A which the hold fee applies to) 9) Check Patron A's accounting page. They should NOT have a hold fee issued_or_reserved, reserved but not issued 10) In your System preferences tab, change HoldFeeMode to issued_or_reserved - "either if all items are checked out, or the record has at least one hold already" 11) Delete Patron A's hold on Biblio A 12) Place a hold on Biblio A for Patron A 13) Check Patron A's accounting page. They SHOULD have a hold fee not_always, issued but not reserved 14) In your System preferences tab, change HoldFeeMode to not_always - "only if all items are checked out and the record has at least one hold already" 15) Delete all holds on Biblio A 16) Check out Biblio A's item (Item A) to Patron B 17) Place a hold on Biblio A for Patron A 18) Check Patron A's accounting page. They should NOT have a hold fee issued_or_reserved, issued but not reserved 19) In your System preferences tab, change HoldFeeMode to issued_or_reserved - "either if all items are checked out, or the record has at least one hold already" 20) Delete Patron A's hold on Biblio A 21) Place a hold on Biblio A for Patron A 22) Check Patron A's accounting page. They SHOULD have a hold fee not_always, issued and reserved 23) In your System preferences tab, change HoldFeeMode to not_always - "only if all items are checked out and the record has at least one hold already" 24) Delete all holds on Biblio A 25) Place a hold on Biblio A for Patron C (any category). 26) Place a hold on Biblio A for Patron A 27) Check Patron A's accounting page. They SHOULD have a hold fee issued_or_reserved, issued and reserved 28) In your System preferences tab, change HoldFeeMode to issued_or_reserved - "either if all items are checked out, or the record has at least one hold already" 29) Delete Patron A's hold on Biblio A. There should still be a hold for Patron C. 30) Place a hold on Biblio A for Patron A 31) Check Patron A's accounting page. They SHOULD have a hold fee 32) Ensure tests pass t/db_dependent/Reserves/GetReserveFee.t Sponsored-by: Horowhenua Libraries Trust --- C4/Reserves.pm | 25 +++++++------ .../admin/preferences/circulation.pref | 1 + t/db_dependent/Reserves/GetReserveFee.t | 37 +++++++++++++++++-- 3 files changed, 47 insertions(+), 16 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 6ff7149edcf..4afd585edd6 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -764,21 +764,22 @@ 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 eq 'not_always' ) { + if( $fee and $fee > 0 and ( $hold_fee_mode eq 'not_always' or $hold_fee_mode eq 'issued_or_reserved' ) ) { # 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 # NOTE: Lost, damaged, not-for-loan, etc. are just ignored here - my ( $notissued, $reserved ); - ( $notissued ) = $dbh->selectrow_array( $issue_qry, undef, - ( $biblionumber ) ); - if( $notissued == 0 ) { - # all items are issued - ( $reserved ) = $dbh->selectrow_array( $holds_qry, undef, - ( $biblionumber, $borrowernumber ) ); - $fee = 0 if $reserved == 0; - } else { - $fee = 0; + + my $notissued = $dbh->selectrow_array( $issue_qry, undef, ( $biblionumber ) ); + my $reserved = $dbh->selectrow_array( $holds_qry, undef, ( $biblionumber, $borrowernumber ) ); + + if ( $hold_fee_mode eq 'issued_or_reserved' ) { + unless ( $notissued == 0 or $reserved > 0 ) { + $fee = 0; + } + } elsif ( $hold_fee_mode eq 'not_always' ) { + unless ( $notissued == 0 and $reserved > 0 ) { + $fee = 0; + } } } return $fee; diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref index e025fc6e9d0..7e6ad30f799 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/admin/preferences/circulation.pref @@ -1124,6 +1124,7 @@ Circulation: choices: any_time_is_placed: "any time a hold is placed." not_always: "only if all items are checked out and the record has at least one hold already." + issued_or_reserved: "either if all items are checked out, or the record has at least one hold already." any_time_is_collected: "any time a hold is collected." - - pref: useDefaultReplacementCost diff --git a/t/db_dependent/Reserves/GetReserveFee.t b/t/db_dependent/Reserves/GetReserveFee.t index 958e6c35271..c94d3457a26 100755 --- a/t/db_dependent/Reserves/GetReserveFee.t +++ b/t/db_dependent/Reserves/GetReserveFee.t @@ -84,15 +84,22 @@ my $item2 = $builder->build_sample_item( ); subtest 'GetReserveFee' => sub { - plan tests => 5; + plan tests => 7; C4::Circulation::AddIssue( $patron1, $item1->barcode, '2015-12-31', 0, undef, 0, {} ); # the date does not really matter - C4::Circulation::AddIssue( $patron3, $item2->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'); + t::lib::Mocks::mock_preference('HoldFeeMode', 'issued_or_reserved'); my $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->biblionumber ); + is( int($fee), 2, 'HoldFeeMode=issued_or_reserved, Patron 2 should be charged' ); + + t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always'); + $fee = C4::Reserves::GetReserveFee( $patron2->{borrowernumber}, $biblio->biblionumber ); + is( $fee, 0, 'HoldFeeMode=issued_or_reserved, Patron 2 should not be charged' ); + + C4::Circulation::AddIssue( $patron3, $item2->barcode, '2015-12-31', 0, undef, 0, {} ); # the date does not really matter + $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' ); @@ -139,7 +146,7 @@ subtest 'Integration with AddReserve' => sub { }; subtest 'Items are issued' => sub { - plan tests => 4; + plan tests => 7; $dbh->do( "DELETE FROM issues WHERE itemnumber=?", undef, $item1->itemnumber); $dbh->do( "DELETE FROM issues WHERE itemnumber=?", undef, $item2->itemnumber); @@ -151,6 +158,14 @@ subtest 'Integration with AddReserve' => sub { addreserve( $patron1->{borrowernumber} ); is( acctlines( $patron1->{borrowernumber} ), 0, 'not_always - Patron should not be charged if items are not all checked out' ); + t::lib::Mocks::mock_preference('HoldFeeMode', 'issued_or_reserved'); + $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} ); + is( acctlines( $patron1->{borrowernumber} ), 1, 'issued_or_reserved - Patron should be charged if all the items are not checked out because 1 hold is placed' ); + + 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( $patron3->{borrowernumber} ); @@ -163,11 +178,25 @@ subtest 'Integration with AddReserve' => sub { addreserve( $patron1->{borrowernumber} ); is( acctlines( $patron1->{borrowernumber} ), 0, 'not_always - Patron should not be charged if all items are checked out but no holds are placed' ); + t::lib::Mocks::mock_preference('HoldFeeMode', 'issued_or_reserved'); + $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, 'issued_or_reserved - Patron should be charged if all items are checked out but no holds are placed' ); + + 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( $patron3->{borrowernumber} ); addreserve( $patron1->{borrowernumber} ); is( acctlines( $patron1->{borrowernumber} ), 1, 'not_always - Patron should only be charged if all items are checked out and at least 1 hold is already placed' ); + + t::lib::Mocks::mock_preference('HoldFeeMode', 'issued_or_reserved'); + $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} ); + is( acctlines( $patron1->{borrowernumber} ), 1, 'issued_or_reserved - Patron should be charged if all items are checked out and at least 1 hold is already placed' ); }; }; -- 2.20.1