Bug 31963

Summary:	Only show hold fee message on OPAC and charge hold fee if HoldFeeMode conditions are true as described in the system preference
Product:	Koha	Reporter:	Aleisha Amohia <aleisha>
Component:	Hold requests	Assignee:	Aleisha Amohia <aleisha>
Status:	CLOSED FIXED	QA Contact:	Katrin Fischer <katrin.fischer>
Severity:	trivial
Priority:	P5 - low	CC:	caroline.cyr-la-rose, gmcharlt, lucas
Version:	Main
Hardware:	All
OS:	All
Change sponsored?:	Sponsored	Patch complexity:	Small patch
Documentation contact:		Documentation submission:
Text to go in the release notes:	When HoldFeeMode is set to not_always or "only if all items are checked out and the record has at least one hold already", Koha will unexpectedly show a hold fee message and charge a hold fee if at least one of those conditions are met. This patch fixes the behaviour so that a hold fee message is only shown, and hold fee only charged, 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, as the system preference implies.	Version(s) released in:	22.11.00
Circulation function:
Bug Depends on:
Bug Blocks:	32142, 33210
Attachments:	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 will be charged Bug 31963: Only show hold fee msg on OPAC if patron will be charged

Description Aleisha Amohia 2022-10-24 21:42:08 UTC

Hold fees can be set at the patron-category level, and if a patron who could be charged a reserve fee logs into the OPAC and tries the place a hold, a message about being charged a hold fee will show.

This message does not take HoldFeeMode into consideration.

When HoldFeeMode is set to "any time a reserve is collected" the message will be "You will be charged a hold fee of $X when you collect this item", but if HoldFeeMode is set to anything else, the message will be what we are seeing "You will be charged a hold fee of $X for placing this hold".

This message can be a bit unexpected, especially if HoldFeeMode is set so hold fees should only be charged if all items are checked out and the record has at least one hold already.

We don't expect we should see the message if there are items available and no holds on the record, when HoldFeeMode is set this way.

Comment 1 Aleisha Amohia 2022-10-24 23:22:55 UTC

Created attachment 142585 [details] [review]
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

Comment 2 David Nind 2022-10-25 01:38:27 UTC

Created attachment 142592 [details] [review]
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>

Comment 3 Katrin Fischer 2022-11-02 14:23:56 UTC

Created attachment 142985 [details] [review]
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>

Comment 4 Katrin Fischer 2022-11-02 14:24:38 UTC

It took me a bit to wrap my head around this.

I believe while this advertises as a display fix, it's also a behavior change:

GetReserveFee:

-        if( $notissued ) {
+        if( $notissued == 0 ) {
+            # all items are issued


And later this is also used to determine what is charged:

AddReserve:

    # add a reserve fee if needed
    if ( C4::Context->preference('HoldFeeMode') ne 'any_time_is_collected' ) {
        my $reserve_fee = GetReserveFee( $borrowernumber, $biblionumber );
        ChargeReserveFee( $borrowernumber, $reserve_fee, $title );
    }

The system preference states: "Charge a hold fee only if all items are checked out and the record has at least one hold already". So the change in behaviour makes this happen. 

Only problem: I am having a hard time to phrase out the actual difference in when a user is charged right now :(

Can you please update the bug description/title and release notes to be more clear about this not only affecting display please?

Comment 5 Aleisha Amohia 2022-11-03 01:17:51 UTC

(In reply to Katrin Fischer from comment #4)
> Can you please update the bug description/title and release notes to be more
> clear about this not only affecting display please?

I've done an update, hopefully that's better

Comment 6 Tomás Cohen Arazi (tcohen) 2022-11-03 12:05:45 UTC

Pushed to master for 22.11.

Nice work everyone, thanks!

Comment 7 Caroline Cyr La Rose 2023-05-05 19:31:06 UTC

This seems like a bug fix, nothing to add to the manual. Closing, but feel free to reopen with details if needed.