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.
This would be a cool enhancement to see ... we want to prevent unrealized credits from popping up.
I agree that this would be a great enhancement.
Created attachment 167567 [details] [review] Bug 28575: Add new syspref 'NoRefundOnLostFinesPaidAge'
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
Created attachment 167569 [details] [review] Bug 28575: Add unit tests prove t/db_dependent/Circulation.t
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.
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.
(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.
Created attachment 167621 [details] [review] Bug 28575: Add new syspref 'NoRefundOnLostFinesPaidAge'
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
Created attachment 167623 [details] [review] Bug 28575: Add a new check-in message for the new syspref
Created attachment 167624 [details] [review] Bug 28575: Add unit tests prove t/db_dependent/Circulation.t
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
Created attachment 167627 [details] [review] Bug 28575: Add unit tests prove t/db_dependent/Circulation.t Sponsored-by: Rapid City Public Library
Created attachment 167635 [details] [review] Bug 28575: Add new syspref 'NoRefundOnLostFinesPaidAge' Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org>
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>
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>
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>
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"?
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
(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.
> 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
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
Created attachment 172252 [details] [review] Bug 28575: Add new syspref 'NoRefundOnLostFinesPaidAge' Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 172253 [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> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 172254 [details] [review] Bug 28575: Add a new check-in message for the new syspref Signed-off-by: Andrew Fuerste Henry <andrewfh@dubcolib.org> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 172255 [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> Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 172256 [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 Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 172257 [details] [review] Bug 28575: (QA follow-up) Tidy and test count Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Created attachment 172258 [details] [review] Bug 28575: (QA follow-up) Tidy more Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
1) Database update (solved) Please use the standardized example success messages when writing your database updates from the bottom of the skeleton.pl file. "Added system preference '...'" I updated it when moving the atomicupdate to dbrevs. 2) Question (not blocker) I was wondering when testing this if that and other preferences relating to the lost fee should be in the "lost item policy" section instead of the "fines policy" section. Just leaving this here to maybe get some opinions.
Pushed for 24.11! Well done everyone, thank you!
Enhancement, no 24.05.x backport