Bug 28575

Summary: Add ability to choose if lost fee is refunded based on when lost fee was paid off
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: System AdministrationAssignee: Matt Blenkinsop <matt.blenkinsop>
Status: Signed Off --- QA Contact: Testopia <testopia>
Severity: enhancement    
Priority: P5 - low CC: barbara.johnson, bdaeuber, christofer.zorn, clackman, gmcharlt, hebah, jheltibridle, lisettepalouse+koha, matt.blenkinsop, nick, rcoert, saylett
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=20815
Change sponsored?: --- Patch complexity: ---
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
Attachments: Bug 28575: Add new syspref 'NoRefundOnLostFinesPaidAge'
Bug 28575: Stop lost fine refund if fine is older than syspref value
Bug 28575: Add unit tests
Bug 28575: Add new syspref 'NoRefundOnLostFinesPaidAge'
Bug 28575: Stop lost fine refund if fine is older than syspref value
Bug 28575: Add a new check-in message for the new syspref
Bug 28575: Add unit tests
Bug 28575: Add unit tests
Bug 28575: Add new syspref 'NoRefundOnLostFinesPaidAge'
Bug 28575: Stop lost fine refund if fine is older than syspref value
Bug 28575: Add a new check-in message for the new syspref
Bug 28575: Add unit tests
Bug 28575: (QA follow-up): Add further unit tests

Description Andrew Fuerste-Henry 2021-06-16 15:02:36 UTC
In bug 20815 we added the NoRefundOnLostReturnedItemsAge system preference to block lost item refunds if the item was lost more than X daya ago.

Many libraries would like a comparable system to block refund if the lost item fee has been paid in full more than X days ago.

Further, even more libraries have asked for the ability to simply not refund paid lost items at all (they only want to refund/forgive lost fees that are not yet paid). Enabling this new syspref to block refunds on lost items paid off more than 0 days ago would support this behavior.
Comment 1 Christofer Zorn 2024-04-30 12:50:34 UTC
This would be a cool enhancement to see ... we want to prevent unrealized credits from popping up.
Comment 2 saylett 2024-05-07 15:02:26 UTC
I agree that this would be a great enhancement.
Comment 3 Matt Blenkinsop 2024-06-07 10:44:20 UTC
Created attachment 167567 [details] [review]
Bug 28575: Add new syspref 'NoRefundOnLostFinesPaidAge'
Comment 4 Matt Blenkinsop 2024-06-07 10:44:22 UTC
Created attachment 167568 [details] [review]
Bug 28575: Stop lost fine refund if fine is older than syspref value

Test plan:
1) Apply patch and reset_all
2) Checkout an item to a patron
3) Mark that item as lost
4) Add a manual invoice for that item's barcode to the patron's account
5) Pay that fine in the Make a payment tab
6) In system preferences, search for NoRefundOnLostFinesPaidAge
7) Set this to -1. I use this value here to avoid needing to go into the database to change the date of the payment we made in step 5. Any fines older than -1 days (i.e. all fines) will be caught by the syspref which is what we want to test)
8) Check in the item
9) The check in message should display "Any lost item fees for this item will remain on the patron's account."
10) Navigate to the Patron's account and confirm that no credit has been added and that the lost fee has therefore not been refunded
11) Run unit test: prove t/db_dependent/Circulation.t
Comment 5 Matt Blenkinsop 2024-06-07 10:44:25 UTC
Created attachment 167569 [details] [review]
Bug 28575: Add unit tests

prove t/db_dependent/Circulation.t
Comment 6 Andrew Fuerste-Henry 2024-06-07 20:44:59 UTC
I'm not quite sure if this is behaving as desired or not. So, slightly different test plan to get more granular:

1: apply patch and reset_all
2: set WhenLostChargeReplacementFee=charge, NoRefundOnLostFinesPaidAge=1, circ rules set to refund paid lost fees when item returned
3: check out 5 items to a patron, mark all 5 lost, incurring 5 lost charges
4: make payments and edit the account_offsets.created_on date for them via the database such that your five charges match this:
 -- charge 1: fully paid 1+ days ago
 -- charge 2: partially paid 1+ days ago
 -- charge 3: partially paid 1+ days ago, remaining payment < 1 day ago
 -- charge 4: full payment < 1 day ago
 -- charge 5: partial payment < 1 day ago
5: Check all items in

I got no refund on charge 1, as expected.

I got no refund on charge 2 -- the system preference says this applies to charges paid in full, should it apply here?.

I got no refund on charge 3 -- I'm not sure if I should have expect a full refund because my charge reached $0 outstanding within the 1 day window, or just a refund of the payment that happened within the 1 day window. What actually happened was I got no refund at all because some portion of the charge had been paid more than a day ago. 

Charges 4 and 5 refunded the portion of the charge I had paid, as expected.
Comment 7 Andrew Fuerste-Henry 2024-06-07 20:46:54 UTC
If nothing else, the system preference needs to be reworded to accurately reflect that it's not just for fully paid charges.

Additionally, I don't think the message "Any lost item fees for this item will remain on the patron's account." conveys accurately what is happening here. We're not leaving fees on their account, we're leaving payments on their account.
Comment 8 Andrew Fuerste-Henry 2024-06-10 14:27:10 UTC
(In reply to Andrew Fuerste-Henry from comment #7)
> If nothing else, the system preference needs to be reworded to accurately
> reflect that it's not just for fully paid charges.
> 
> Additionally, I don't think the message "Any lost item fees for this item
> will remain on the patron's account." conveys accurately what is happening
> here. We're not leaving fees on their account, we're leaving payments on
> their account.

Our desired functionality here is that when a lost item is found the entire paid portion of the lost fee should be refunded unless the payment through which the debit reached $0 outstanding was more than X days ago. That would mean:
 - this system preference would have no bearing on refunds on items that have not reached $0 outstanding
 - if a lost item was partially paid >X days ago, then paid off the rest of the way  <X days ago, and then returned today, the full paid amount would be refunded

All that said, I don't know if we have a great way to determine the date on which a debit reached $0 outstanding.
Comment 9 Matt Blenkinsop 2024-06-11 10:46:07 UTC
Created attachment 167621 [details] [review]
Bug 28575: Add new syspref 'NoRefundOnLostFinesPaidAge'
Comment 10 Matt Blenkinsop 2024-06-11 10:46:10 UTC
Created attachment 167622 [details] [review]
Bug 28575: Stop lost fine refund if fine is older than syspref value

Test plan:
1) Apply patch and reset_all
2) Checkout an item to a patron
3) Mark that item as lost
4) Add a manual invoice for that item's barcode to the patron's account
5) Pay that fine in the Make a payment tab
6) In system preferences, search for NoRefundOnLostFinesPaidAge
7) Set this to -1. I use this value here to avoid needing to go into the database to change the date of the payment we made in step 5. Any fines older than -1 days (i.e. all fines) will be caught by the syspref which is what we want to test)
8) Check in the item
9) The check in message should display "Any lost item fees for this item will remain on the patron's account."
10) Navigate to the Patron's account and confirm that no credit has been added and that the lost fee has therefore not been refunded
11) Run unit test: prove t/db_dependent/Circulation.t
Comment 11 Matt Blenkinsop 2024-06-11 10:46:13 UTC
Created attachment 167623 [details] [review]
Bug 28575: Add a new check-in message for the new syspref
Comment 12 Matt Blenkinsop 2024-06-11 10:46:16 UTC
Created attachment 167624 [details] [review]
Bug 28575: Add unit tests

prove t/db_dependent/Circulation.t
Comment 13 Matt Blenkinsop 2024-06-11 10:48:09 UTC
Hi Andrew, thanks for the feedback and for testing! I've made changes as you suggested, so it should now only operate on charges that have a £0 balance. I've also updated the syspref description and added a new checkin message to account for this as well
Comment 14 Matt Blenkinsop 2024-06-11 12:03:04 UTC
Created attachment 167627 [details] [review]
Bug 28575: Add unit tests

prove t/db_dependent/Circulation.t

Sponsored-by: Rapid City Public Library
Comment 15 Andrew Fuerste-Henry 2024-06-11 14:48:24 UTC
Created attachment 167635 [details] [review]
Bug 28575: Add new syspref 'NoRefundOnLostFinesPaidAge'

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 16 Andrew Fuerste-Henry 2024-06-11 14:48:27 UTC
Created attachment 167636 [details] [review]
Bug 28575: Stop lost fine refund if fine is older than syspref value

Test plan:
1) Apply patch and reset_all
2) Checkout an item to a patron
3) Mark that item as lost
4) Add a manual invoice for that item's barcode to the patron's account
5) Pay that fine in the Make a payment tab
6) In system preferences, search for NoRefundOnLostFinesPaidAge
7) Set this to -1. I use this value here to avoid needing to go into the database to change the date of the payment we made in step 5. Any fines older than -1 days (i.e. all fines) will be caught by the syspref which is what we want to test)
8) Check in the item
9) The check in message should display "Any lost item fees for this item will remain on the patron's account."
10) Navigate to the Patron's account and confirm that no credit has been added and that the lost fee has therefore not been refunded
11) Run unit test: prove t/db_dependent/Circulation.t

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 17 Andrew Fuerste-Henry 2024-06-11 14:48:30 UTC
Created attachment 167637 [details] [review]
Bug 28575: Add a new check-in message for the new syspref

Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 18 Andrew Fuerste-Henry 2024-06-11 14:48:32 UTC
Created attachment 167638 [details] [review]
Bug 28575: Add unit tests

prove t/db_dependent/Circulation.t

Sponsored-by: Rapid City Public Library
Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
Comment 19 Andrew Fuerste-Henry 2024-06-11 15:55:04 UTC
This worked as desired and as described. The wording at checkin is clear and accurate.

I'm still unsure of the wording on the syspref, but figured that could get addressed moving forward. You've got "Don't refund lost fees if the fee was paid in full or if the balance of the fee was paid more than X days ago." Strictly speaking, the code says "Don't refund lost fees if the fee is at amountoutstanding=0 and the most recently applied credit was applied more than X days ago."

Maybe we could go with "Don't refund lost fees if the fee was paid in full and the most recently applied credit was applied more than X days ago"?
Comment 20 Nick Clemens (kidclamp) 2024-06-13 18:29:40 UTC
I wonder how this will play with lostreturn settings of:
Refund lost item charge and charge new overdue fine
Refund lost item charge and restore overdue fine

The new code returns from the subroutine before hitting these, but I am not sure how the logic should work?

If I paid the lost fee more than X days ago, and we aren't refunding, then do we expect to sustain those charges?

I think we should have additional tests in t/db_dependent/Koha/Item.t for _set_found_trigger directly
Comment 21 Andrew Fuerste-Henry 2024-06-13 18:50:04 UTC
(In reply to Nick Clemens (kidclamp) from comment #20)
> I wonder how this will play with lostreturn settings of:
> Refund lost item charge and charge new overdue fine
> Refund lost item charge and restore overdue fine
> 
> The new code returns from the subroutine before hitting these, but I am not
> sure how the logic should work?
> 
> If I paid the lost fee more than X days ago, and we aren't refunding, then
> do we expect to sustain those charges?

The ability to restore overdues when refunding a lost item charge was added in bug 23091. Discussion there suggests we should *not* reinstate overdues when a lost fee is not refunded due to having been paid too long ago.

Bug 20815 allows one to prevent refunds based on how long ago the item became lost. How does it handle whether or not overdues are reinstated/recreated? Consistency seems ideal.
Comment 22 Matt Blenkinsop 2024-06-18 08:29:20 UTC
> Bug 20815 allows one to prevent refunds based on how long ago the item
> became lost. How does it handle whether or not overdues are
> reinstated/recreated? Consistency seems ideal.

The syspref this introduces (NoRefundOnLostReturnedItemsAge) is processed right at the start of the _set_found_trigger method so returns before any overdues are handled - looks like they're not considered at all for that system preference
Comment 23 Matt Blenkinsop 2024-06-26 13:56:18 UTC
Created attachment 168146 [details] [review]
Bug 28575: (QA follow-up): Add further unit tests

This patch adds further unit tests for the messaging in _set_found_trigger

prove t/db_dependent/Koha/Item.t