Bug 38477 - Regression: new overdue fine applied incorrectly when using "Refund lost item charge and charge new overdue fine" option in circ rules
Summary: Regression: new overdue fine applied incorrectly when using "Refund lost item...
Status: Pushed to oldstable
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: Main
Hardware: All All
: P5 - low major
Assignee: Nick Clemens (kidclamp)
QA Contact: Pedro Amorim
URL:
Keywords: regression
Depends on: 30254
Blocks:
  Show dependency treegraph
 
Reported: 2024-11-18 20:10 UTC by Andrew Fuerste-Henry
Modified: 2025-06-19 08:55 UTC (History)
9 users (show)

See Also:
GIT URL:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Under certain circumstances, the existence of a lost charge for a patron that previously borrowed an item (which was later found) could lead to creating a new fine for a patron that borrowed and returned the item with no issues - if the item was lost and found again after they had returned it. This adds tests to cover this edge case, and fixes this edge case to ensure that a new fine is only charged if the patron charged the lost fine matches the patron who most recently returned the item.
Version(s) released in:
25.11.00,25.05.01,24.11.06
Circulation function:


Attachments
Bug 38477: UNit test (2.64 KB, patch)
2025-05-21 19:21 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38477: Unit test (2.64 KB, patch)
2025-05-22 11:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38477: Only charge a new lost fine when the last patron to return the item matches the original fined patron (4.05 KB, patch)
2025-05-22 11:32 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 38477: Only charge a new lost fine when the last patron to return the item matches the original fined patron (4.10 KB, patch)
2025-05-22 22:29 UTC, David Nind
Details | Diff | Splinter Review
Bug 38477: Unit test (2.69 KB, patch)
2025-05-28 15:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38477: Only charge a new lost fine when the last patron to return the item matches the original fined patron (4.16 KB, patch)
2025-05-28 15:56 UTC, Pedro Amorim
Details | Diff | Splinter Review
Bug 38477: (follow-up) Correct failing Item.t test (1.08 KB, patch)
2025-06-02 23:42 UTC, Lucas Gass (lukeg)
Details | Diff | Splinter Review
Bug 38477: (follow-up) Correct failing Item.t test (1.13 KB, patch)
2025-06-04 10:48 UTC, David Nind
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Andrew Fuerste-Henry 2024-11-18 20:10:10 UTC
Bug 30254 fixed this something like this previously. We're seeing erroneous fines created for patrons who previously returned something on time. In production, this occurs when an item is marked lost while not checked out to a patron. When that item is later found and checked in, Koha finds the most recent checkout and generates an overdue fine using the current date as the return date of the item (even though the item was returned at some point in the past).

Set these sysprefs:
- FinesMode: Calculate and Charge
- WhenLostForgiveFine: Forgive
- WhenLostChargeReplacementFee: Charge

In circ rules:
- Have a rule that charges fines
- Set Refund lost item replacement fee to "Refund lost item charge and charge new overdue fine"

1 - Have/create item with replacement price
2 - Check item out to patron 1, due date in past
3 - Run fines.pl
4 - confirm fines on patron 1
5 - mark item lost
6 - confirm replacement cost generated and fine forgiven on patron 1
7 - check item in 
8 - confirm replacement cost forgiven and new fine generated on patron 1
9 - check item out to patron 2, due date in future
10 - check item in
11 - mark item lost (no replacement fee generated because item is not checked out)
12 - in database, update date_due for checkout to patron 2, set to a date in the past
13 - check item in
14 - confirm patron 2 now has a fine

At step 12 we're pushing the date_due of the old issue into the past to make date_due<curdate(). In production, this comes up when there are days between steps 10 and 13.
Comment 1 Nick Clemens (kidclamp) 2024-11-21 18:06:20 UTC
AddReturn does some stuff, eventually calls Item->store

In Koha::Item->store we call _set_found_trigger 

_set_found_trigger  adds a 'lost_charge' message to the item if we have found a previous lost or overdue lost accountline and the rule is set to charge a new fine

This gets passed back to AddReturn which looks for the last issue of the item, and charges a new fine from the previous due_date until today

I believe what we need to do is:
1 - Verify the lost charge found in _set_found_trigger is the same patron we are charging now
2 - Verify the issue was not returned since the issue

Current thoughts:
Store the borrowernumber in the item_lost entry in statistics when marking an item lost - then we can verify that current lost is attributed to patron

Pass the patroid from the lost fine found in _set_found_trigger back to AddReturn and verify the patron is the same as the one with the issue
Comment 2 David Cook 2024-12-12 05:04:12 UTC
Marking as CONFIRMED on behalf of Nick
Comment 3 Nick Clemens (kidclamp) 2025-05-21 19:21:17 UTC
Created attachment 182701 [details] [review]
Bug 38477: UNit test
Comment 4 Nick Clemens (kidclamp) 2025-05-22 11:32:18 UTC
Created attachment 182726 [details] [review]
Bug 38477: Unit test
Comment 5 Nick Clemens (kidclamp) 2025-05-22 11:32:21 UTC
Created attachment 182727 [details] [review]
Bug 38477: Only charge a new lost fine when the last patron to return the item matches the original fined patron

Set these sysprefs:
- FinesMode: Calculate and Charge
- WhenLostForgiveFine: Forgive
- WhenLostChargeReplacementFee: Charge

In circ rules:
- Have a rule that charges fines
- Set Refund lost item replacement fee to "Refund lost item charge and charge new overdue fine"

1 - Have/create item with replacement price
2 - Check item out to patron 1, due date in past
3 - Run fines.pl
4 - confirm fines on patron 1
5 - mark item lost
6 - confirm replacement cost generated and fine forgiven on patron 1
7 - check item in
8 - confirm replacement cost forgiven and new fine generated on patron 1
9 - check item out to patron 2, due date in future
10 - check item in
11 - mark item lost (no replacement fee generated because item is not checked out)
12 - in database, update date_due for checkout to patron 2, set to a date in the past
13 - check item in
14 - confirm patron 2 now has a fine
15 - Apply patches, restart all
16 - Repeat test plan with another item and confirm no fine is added for patron 2

Note: At step 12 we're pushing the date_due of the old issue into the past to make date_due<curdate(). In production, this comes up when there are days between steps 10 and 13.
Comment 6 David Nind 2025-05-22 14:54:07 UTC
SQL for step 12?

Sorry, I'm not familiar enough with the database structure to work out what table this data is in.
Comment 7 Andrew Fuerste-Henry 2025-05-22 16:21:22 UTC
(In reply to David Nind from comment #6)
> SQL for step 12?
> 
> Sorry, I'm not familiar enough with the database structure to work out what
> table this data is in.

UPDATE issues SET issues.date_due='2025-01-01 12:00:00' WHERE issues.borrowernumber=[your patron] AND issues.itemnumber=[your item];
Comment 8 Andrew Fuerste-Henry 2025-05-22 16:23:06 UTC
(In reply to Andrew Fuerste-Henry from comment #7)
> (In reply to David Nind from comment #6)
> > SQL for step 12?
> > 
> > Sorry, I'm not familiar enough with the database structure to work out what
> > table this data is in.
> 
> UPDATE issues SET issues.date_due='2025-01-01 12:00:00' WHERE
> issues.borrowernumber=[your patron] AND issues.itemnumber=[your item];

Oh, wait, sorry, I believe you want to change "issues" to "old_issues" in that. I didn't think about where this checkout was at that point in the test plan.
Comment 9 David Nind 2025-05-22 22:29:25 UTC
Created attachment 182745 [details] [review]
Bug 38477: Only charge a new lost fine when the last patron to return the item matches the original fined patron

Set these sysprefs:
- FinesMode: Calculate and Charge
- WhenLostForgiveFine: Forgive
- WhenLostChargeReplacementFee: Charge

In circ rules:
- Have a rule that charges fines
- Set Refund lost item replacement fee to "Refund lost item charge and charge new overdue fine"

1 - Have/create item with replacement price
2 - Check item out to patron 1, due date in past
3 - Run fines.pl
4 - confirm fines on patron 1
5 - mark item lost
6 - confirm replacement cost generated and fine forgiven on patron 1
7 - check item in
8 - confirm replacement cost forgiven and new fine generated on patron 1
9 - check item out to patron 2, due date in future
10 - check item in
11 - mark item lost (no replacement fee generated because item is not checked out)
12 - in database, update date_due for checkout to patron 2, set to a date in the past
13 - check item in
14 - confirm patron 2 now has a fine
15 - Apply patches, restart all
16 - Repeat test plan with another item and confirm no fine is added for patron 2

Note: At step 12 we're pushing the date_due of the old issue into the past to make date_due<curdate(). In production, this comes up when there are days between steps 10 and 13.
Signed-off-by: David Nind <david@davidnind.com>
Comment 10 David Nind 2025-05-22 22:37:42 UTC
Thanks Andrew!

Testing notes (using KTD):
1. Rule that charges fines:
   - Edit the All All rule
   - Fine amount: 1.00
   - Fine charging interval: 5
2. Scroll down to the heading "Default lost item fee refund on return policy" and change:
   - "Refund lost item charge" to "Refund lost item charge and charge new overdue fine"
3. I used Programming Perl (item 39999000011418):
   - Set 952$v - Cost, replacement price to 50.00
   - First check out to Mary Burton - set due date to 1 May 2025
   - Second check out to Lisa Charles - set due date to 
4. Running the fines cronjob:  misc/cronjobs/fines.pl -v
   Output:
     Fines assessment -- 2025-05-22T21:47:57
     Number of Overdue Items:
        counted 1
       reported 1
        updated 1
5. SQL (using the above patron and item number):
   - UPDATE old_issues SET old_issues.date_due='2025-01-01 12:00:00' WHERE old_issues.borrowernumber=37 AND old_issues.itemnumber=578;
6. After the patch:
   - I used CGI programming with Perl (item 39999000000498), set 952$v - Cost, replacement price to 25.00
   - SQL = UPDATE old_issues SET old_issues.date_due='2025-01-01 12:00:00' WHERE old_issues.borrowernumber=37 AND old_issues.itemnumber=32;
7. Run the tests, these should pass: prove t/db_dependent/Circulation.t
Comment 11 Pedro Amorim 2025-05-28 15:56:42 UTC
Created attachment 182827 [details] [review]
Bug 38477: Unit test

Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Comment 12 Pedro Amorim 2025-05-28 15:56:45 UTC
Created attachment 182828 [details] [review]
Bug 38477: Only charge a new lost fine when the last patron to return the item matches the original fined patron

Set these sysprefs:
- FinesMode: Calculate and Charge
- WhenLostForgiveFine: Forgive
- WhenLostChargeReplacementFee: Charge

In circ rules:
- Have a rule that charges fines
- Set Refund lost item replacement fee to "Refund lost item charge and charge new overdue fine"

1 - Have/create item with replacement price
2 - Check item out to patron 1, due date in past
3 - Run fines.pl
4 - confirm fines on patron 1
5 - mark item lost
6 - confirm replacement cost generated and fine forgiven on patron 1
7 - check item in
8 - confirm replacement cost forgiven and new fine generated on patron 1
9 - check item out to patron 2, due date in future
10 - check item in
11 - mark item lost (no replacement fee generated because item is not checked out)
12 - in database, update date_due for checkout to patron 2, set to a date in the past
13 - check item in
14 - confirm patron 2 now has a fine
15 - Apply patches, restart all
16 - Repeat test plan with another item and confirm no fine is added for patron 2

Note: At step 12 we're pushing the date_due of the old issue into the past to make date_due<curdate(). In production, this comes up when there are days between steps 10 and 13.
Signed-off-by: David Nind <david@davidnind.com>
Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk>
Comment 13 Lucas Gass (lukeg) 2025-06-02 20:58:42 UTC
Nice work everyone!

Pushed to main for 25.11
Comment 14 Lucas Gass (lukeg) 2025-06-02 23:39:18 UTC
This seems to be causing 't/db_dependent/Koha/Item.t' tests to fail.
Comment 15 Lucas Gass (lukeg) 2025-06-02 23:42:02 UTC
Created attachment 182916 [details] [review]
Bug 38477: (follow-up) Correct failing Item.t test

prove /kohadevbox/koha/t/db_dependent/Koha/Item.t
Comment 16 Lucas Gass (lukeg) 2025-06-02 23:43:29 UTC
Can someone please look at/sign-off my follow up before I push to main?
Comment 17 David Nind 2025-06-04 10:48:14 UTC
Created attachment 182937 [details] [review]
Bug 38477: (follow-up) Correct failing Item.t test

prove /kohadevbox/koha/t/db_dependent/Koha/Item.t

Signed-off-by: David Nind <david@davidnind.com>
Comment 18 Lucas Gass (lukeg) 2025-06-04 22:09:46 UTC
follow-up pushed to main
Comment 19 Fridolin Somers 2025-06-19 07:31:34 UTC
I see in 25.05.x
Comment 20 Fridolin Somers 2025-06-19 07:31:41 UTC
Pushed to 24.11.x for 24.11.06
Comment 21 Paul Derscheid 2025-06-19 08:55:09 UTC
Nice work everyone!

Pushed to 25.05.x for 25.05.03