From 8108b201336d911f66bd1a33afc7f8f4f3d409c3 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 Signed-off-by: Sam Lau --- C4/Reserves.pm | 27 +++++---- ..._reserved_option_to_HoldFeeMode_syspref.pl | 10 ++-- .../admin/preferences/circulation.pref | 1 + t/db_dependent/Reserves/GetReserveFee.t | 56 +++++++++++++++++-- 4 files changed, 72 insertions(+), 22 deletions(-) mode change 100644 => 100755 installer/data/mysql/atomicupdate/bug_32142_-_add_issued_or_reserved_option_to_HoldFeeMode_syspref.pl diff --git a/C4/Reserves.pm b/C4/Reserves.pm index 729d5d0eae7..35daf89778e 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -736,21 +736,24 @@ SELECT COUNT(*) FROM reserves WHERE biblionumber=? AND borrowernumber<>? my ( $fee ) = $dbh->selectrow_array( $borquery, undef, ($borrowernumber) ); $fee += 0; 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 ) { + # items still available to be checked out OR record has no reserves + $fee = 0; + } + } elsif ( $hold_fee_mode eq 'not_always' ) { + unless ( $notissued == 0 and $reserved > 0 ) { + # items still available to be checked out AND record has no reserves + $fee = 0; + } } } return $fee; diff --git a/installer/data/mysql/atomicupdate/bug_32142_-_add_issued_or_reserved_option_to_HoldFeeMode_syspref.pl b/installer/data/mysql/atomicupdate/bug_32142_-_add_issued_or_reserved_option_to_HoldFeeMode_syspref.pl old mode 100644 new mode 100755 index e0df726935f..92903185e11 --- a/installer/data/mysql/atomicupdate/bug_32142_-_add_issued_or_reserved_option_to_HoldFeeMode_syspref.pl +++ b/installer/data/mysql/atomicupdate/bug_32142_-_add_issued_or_reserved_option_to_HoldFeeMode_syspref.pl @@ -1,12 +1,14 @@ use Modern::Perl; return { - bug_number => "32142", + bug_number => "32142", description => "Add issued_or_reserved option to HoldFeeMode system preference", - up => sub { + up => sub { my ($args) = @_; - my ($dbh, $out) = @$args{qw(dbh out)}; + my ( $dbh, $out ) = @$args{qw(dbh out)}; - $dbh->do(q{ UPDATE systempreferences SET options = 'any_time_is_placed|not_always|issued_or_reserved|any_time_is_collected' where variable = 'HoldFeeMode' }); + $dbh->do( + q{ UPDATE systempreferences SET options = 'any_time_is_placed|not_always|issued_or_reserved|any_time_is_collected' where variable = 'HoldFeeMode' } + ); }, }; 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 317e4238ec2..22f2874d472 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 @@ -1176,6 +1176,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 03029d83331..437ac6c6dff 100755 --- a/t/db_dependent/Reserves/GetReserveFee.t +++ b/t/db_dependent/Reserves/GetReserveFee.t @@ -97,19 +97,29 @@ my $item2 = $builder->build_sample_item( ); subtest 'GetReserveFee' => sub { - plan tests => 6; + plan tests => 8; 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', 'not_always' ); my $fee = C4::Reserves::GetReserveFee( $patron2->borrowernumber, $biblio->biblionumber ); + is( $fee, 0, 'HoldFeeMode=not_always, Patron 2 should not be charged' ); + + my $res1 = addreserve( $patron1->borrowernumber ); + + t::lib::Mocks::mock_preference( 'HoldFeeMode', 'issued_or_reserved' ); + $fee = C4::Reserves::GetReserveFee( $patron2->borrowernumber, $biblio->biblionumber ); + is( int($fee), 2, 'HoldFeeMode=issued_or_reserved, Patron 2 should 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' ); + t::lib::Mocks::mock_preference( 'HoldFeeMode', 'not_always' ); # If we delete the reserve, there should be no charge $dbh->do( "DELETE FROM reserves WHERE borrowernumber = ?", undef, ( $patron1->borrowernumber) ); $fee = C4::Reserves::GetReserveFee( $patron2->borrowernumber, $biblio->biblionumber ); @@ -156,7 +166,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); @@ -168,11 +178,25 @@ 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' ); - $dbh->do( "DELETE FROM reserves WHERE biblionumber=?", undef, $biblio->biblionumber ); + 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 ); addreserve( $patron1->borrowernumber ); - is( acctlines( $patron1->borrowernumber ), 0, 'not_always - Patron should not be charged if all the items are not checked out, even if 1 hold is already placed' ); + is( + acctlines( $patron1->borrowernumber ), 0, + 'not_always - Patron should not be charged if all the items are not checked out, even if 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 ); @@ -180,11 +204,31 @@ 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.39.5