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 <david@davidnind.com>
Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>

diff --git a/C4/Reserves.pm b/C4/Reserves.pm
index a03072d..d96d6ce 100644 (file)
--- 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 ac5f1cc..1965a0d 100644 (file)
--- 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 @@
                     <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"/>

diff --git a/opac/opac-reserve.pl b/opac/opac-reserve.pl
index ec3d6ad..1200ef0 100755 (executable)
--- 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 f53f7c1..958e6c3 100755 (executable)
--- 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' );
     };
 };