Koha-community/Koha
13
7
Fork 0
Code Issues Releases Wiki Activity

Bug 31963: Only show hold fee msg on OPAC if patron will be charged

Browse source 
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 <david@davidnind.com>

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
This commit is contained in:
Aleisha Amohia 2022-10-24 23:08:52 +00:00 committed by Tomas Cohen Arazi
parent 9c6003621b
commit e1a02dde8f
Signed by: tomascohen
GPG key ID: 0A272EA1B2F3C15F
4 changed files with 30 additions and 22 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

5
C4/Reserves.pm
View file

@ -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;

21
koha-tmpl/opac-tmpl/bootstrap/en/modules/opac-reserve.tt
View file

@ -141,16 +141,6 @@
<h2>Confirm holds for:[% INCLUDE 'patron-title.inc' patron = logged_in_user %] ([% logged_in_user.cardnumber | html %])</h2>
[% END # / UNLESS none_available %]
[% IF (RESERVE_CHARGE) %]
<div class="alert" id="reserve_fee">
[% IF Koha.Preference('HoldFeeMode') == 'any_time_is_collected' %]
<span>You will be charged a hold fee of [% RESERVE_CHARGE | $Price %] when you collect this item</span>
[% ELSE %]
<span>You will be charged a hold fee of [% RESERVE_CHARGE | $Price %] for placing this hold</span>
[% END %]
</div>
[% END %]
[% IF ( new_reserves_allowed ) %]
<div id="new_reserves_allowed" class="alert">
<strong>Sorry,</strong> 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 %]
</div>
[% END %]
[% IF ( bibitemloo.reserve_charge ) %]
<div class="alert" id="reserve_fee">
[% IF Koha.Preference('HoldFeeMode') == 'any_time_is_collected' %]
<span>You will be charged a hold fee of [% bibitemloo.reserve_charge | $Price %] when you collect this item</span>
[% ELSE %]
<span>You will be charged a hold fee of [% bibitemloo.reserve_charge | $Price %] for placing this hold</span>
[% END %]
</div>
[% END %]
<p>
[% IF ( bibitemloo.holdable ) %]
<input class="reserve_mode" name="reserve_mode" type="hidden" value="single"/>

10
opac/opac-reserve.pl
View file

@ -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;

16
t/db_dependent/Reserves/GetReserveFee.t
View file

@ -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' );
};
};