From e1a02dde8f7dbb09172071ebaec1338d58c2634f Mon Sep 17 00:00:00 2001 From: Aleisha Amohia Date: Mon, 24 Oct 2022 23:08:52 +0000 Subject: [PATCH] Bug 31963: Only show hold fee msg on OPAC if patron will be charged This patch ensures HoldFeeMode is considered when displaying a message to patrons on the OPAC that says they'll be charged a hold fee when placing or collecting the hold. When HoldFeeMode is set to not_always or "only if all items are checked out and the record has at least one hold already" then the hold fee message should only show if all items on the record are checked out, AND the record has at least one hold already - both of these conditions must be met. To test: 1. Go to Administration -> Patron categories 2. Edit your patron category and give a hold fee of $1. 3. Go to Administration -> System preferences and search for HoldFeeMode. Set to 'only if all items are checked out and the record has at least one hold already' if not already set. Keep this tab open. 4. In another tab, open the OPAC. 5. Search the OPAC for a record with one item which is NOT checked out. 6. Go to place a hold on this record. Confirm you see a message saying that you will be charged a hold fee, even though not all items are checked out and the record does not have a hold --> This is the bug. 7. Apply patch and restart services. Items available, no holds placed 8. Repeat steps 5-6. This time, you should NOT see the hold fee message. Items available, holds placed 9. In your staff interface tab, find the same record. 10. Place a hold for a different patron on this record. 11. In your OPAC tab, find this record again and go to place a hold. You should NOT see the hold fee message. No items available, no holds placed 12. In your staff interface tab, cancel the hold placed on this record. 13. Check out the item to a different patron. 14. In your OPAC tab, find this record again and go to place a hold. You should NOT see the hold fee message. No items available, holds placed 15. In your staff interface tab, keep the item checked out to another patron. 16. Place a hold for a third patron on this record. 17. In your OPAC tab, find this record again and go to place a hold. You SHOULD see the hold fee message. Multiple holds 18. Search the OPAC for a record. Make sure your search will return more than one result, including our test record. 19. Check the checkbox for our test record, plus another record where the item is not checked out. 20. Click the Place hold button to place holds on all of our selected records. You should only see the hold fee message above our test record. 21. In your staff interface tab, test setting HoldFeeMode to the other values and confirm the hold message shows on the OPAC as expected. 22. Confirm tests pass t/db_dependent/Reserves/GetReserveFee.t Sponsored-by: Horowhenua Libraries Trust Signed-off-by: David Nind Signed-off-by: Katrin Fischer Signed-off-by: Tomas Cohen Arazi --- C4/Reserves.pm | 5 ++++- .../bootstrap/en/modules/opac-reserve.tt | 21 ++++++++++--------- opac/opac-reserve.pl | 10 +++------ t/db_dependent/Reserves/GetReserveFee.t | 16 ++++++++++---- 4 files changed, 30 insertions(+), 22 deletions(-) diff --git a/C4/Reserves.pm b/C4/Reserves.pm index a03072d8df..d96d6ce43b 100644 --- a/C4/Reserves.pm +++ b/C4/Reserves.pm @@ -773,10 +773,13 @@ SELECT COUNT(*) FROM reserves WHERE biblionumber=? AND borrowernumber<>? my ( $notissued, $reserved ); ( $notissued ) = $dbh->selectrow_array( $issue_qry, undef, ( $biblionumber ) ); - if( $notissued ) { + if( $notissued == 0 ) { + # all items are issued ( $reserved ) = $dbh->selectrow_array( $holds_qry, undef, ( $biblionumber, $borrowernumber ) ); $fee = 0 if $reserved == 0; + } else { + $fee = 0; } } return $fee; diff --git a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt index ac5f1ccd8c..1965a0dd50 100644 --- a/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt +++ b/koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt @@ -141,16 +141,6 @@

Confirm holds for:[% INCLUDE 'patron-title.inc' patron = logged_in_user %] ([% logged_in_user.cardnumber | html %])

[% END # / UNLESS none_available %] - [% IF (RESERVE_CHARGE) %] -
- [% IF Koha.Preference('HoldFeeMode') == 'any_time_is_collected' %] - You will be charged a hold fee of [% RESERVE_CHARGE | $Price %] when you collect this item - [% ELSE %] - You will be charged a hold fee of [% RESERVE_CHARGE | $Price %] for placing this hold - [% END %] -
- [% END %] - [% IF ( new_reserves_allowed ) %]
Sorry, you can only place [% new_reserves_allowed | html %] more holds. Please uncheck the checkboxes for the items you wish to not place holds on. @@ -178,6 +168,17 @@ [% END %]
[% END %] + + [% IF ( bibitemloo.reserve_charge ) %] +
+ [% IF Koha.Preference('HoldFeeMode') == 'any_time_is_collected' %] + You will be charged a hold fee of [% bibitemloo.reserve_charge | $Price %] when you collect this item + [% ELSE %] + You will be charged a hold fee of [% bibitemloo.reserve_charge | $Price %] for placing this hold + [% END %] +
+ [% END %] +

[% IF ( bibitemloo.holdable ) %] diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl index ec3d6adcb8..1200ef0608 100755 --- a/opac/opac-reserve.pl +++ b/opac/opac-reserve.pl @@ -25,7 +25,7 @@ use CGI qw ( -utf8 ); use C4::Auth qw( get_template_and_user ); use C4::Koha qw( getitemtypeimagelocation getitemtypeimagesrc ); use C4::Circulation qw( GetBranchItemRule ); -use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve GetReservesControlBranch ItemsAnyAvailableAndNotRestricted IsAvailableForItemLevelRequest ); +use C4::Reserves qw( CanItemBeReserved CanBookBeReserved AddReserve GetReservesControlBranch ItemsAnyAvailableAndNotRestricted IsAvailableForItemLevelRequest GetReserveFee ); use C4::Biblio qw( GetBiblioData GetFrameworkCode ); use C4::Output qw( output_html_with_http_headers ); use C4::Context; @@ -72,12 +72,6 @@ unless ( $can_place_hold_if_available_at_pickup ) { } } -# Pass through any reserve charge -my $reservefee = $category->reservefee; -if ( $reservefee > 0){ - $template->param( RESERVE_CHARGE => $reservefee); -} - my $itemtypes = { map { $_->{itemtype} => $_ } @{ Koha::ItemTypes->search_with_localization->unblessed } }; # There are two ways of calling this script, with a single biblio num @@ -575,6 +569,8 @@ foreach my $biblioNum (@biblionumbers) { $biblioLoopIter{forced_hold_level} = $forced_hold_level; } + # Pass through any reserve charge + $biblioLoopIter{reserve_charge} = GetReserveFee( $patron->id, $biblioNum ); push @$biblioLoop, \%biblioLoopIter; diff --git a/t/db_dependent/Reserves/GetReserveFee.t b/t/db_dependent/Reserves/GetReserveFee.t index f53f7c154f..958e6c3527 100755 --- a/t/db_dependent/Reserves/GetReserveFee.t +++ b/t/db_dependent/Reserves/GetReserveFee.t @@ -87,6 +87,7 @@ subtest 'GetReserveFee' => sub { plan tests => 5; 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} ); @@ -138,8 +139,10 @@ subtest 'Integration with AddReserve' => sub { }; subtest 'Items are issued' => sub { - plan tests => 3; + plan tests => 4; + $dbh->do( "DELETE FROM issues WHERE itemnumber=?", undef, $item1->itemnumber); + $dbh->do( "DELETE FROM issues WHERE itemnumber=?", undef, $item2->itemnumber); C4::Circulation::AddIssue( $patron2, $item1->barcode, '2015-12-31', 0, undef, 0, {} ); t::lib::Mocks::mock_preference('HoldFeeMode', 'not_always'); @@ -152,14 +155,19 @@ subtest 'Integration with AddReserve' => sub { $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' ); + 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 ); $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' ); + is( acctlines( $patron1->{borrowernumber} ), 0, 'not_always - Patron should not be charged if all items are checked out but no holds are placed' ); + + $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' ); }; }; -- 2.39.5