Bugzilla – Attachment 142592 Details for
Bug 31963
Only show hold fee message on OPAC and charge hold fee if HoldFeeMode conditions are true as described in the system preference
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 31963: Only show hold fee msg on OPAC if patron will be charged
Bug-31963-Only-show-hold-fee-msg-on-OPAC-if-patron.patch (text/plain), 10.81 KB, created by
David Nind
on 2022-10-25 01:38:27 UTC
(
hide
)
Description:
Bug 31963: Only show hold fee msg on OPAC if patron will be charged
Filename:
MIME Type:
Creator:
David Nind
Created:
2022-10-25 01:38:27 UTC
Size:
10.81 KB
patch
obsolete
>From c7f68ef4396ab4d9de5f5cc9597d87dca22db26d Mon Sep 17 00:00:00 2001 >From: Aleisha Amohia <aleishaamohia@hotmail.com> >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 <david@davidnind.com> >--- > 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 @@ > <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 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.30.2
<b>You cannot view the attachment while viewing its details because your browser does not support IFRAMEs. <a href="attachment.cgi?id=142592">View the attachment on a separate page</a>.</b>
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 31963
:
142585
|
142592
|
142985