Summary: | Regression: new overdue fine applied incorrectly when using "Refund lost item charge and charge new overdue fine" option in circ rules | ||
---|---|---|---|
Product: | Koha | Reporter: | Andrew Fuerste-Henry <andrew> |
Component: | Circulation | Assignee: | Nick Clemens (kidclamp) <nick> |
Status: | Pushed to oldstable --- | QA Contact: | Pedro Amorim <pedro.amorim> |
Severity: | major | ||
Priority: | P5 - low | CC: | david, dcook, fridolin.somers, gmcharlt, kyle.m.hall, lisette, lucas, nick, pedro.amorim |
Version: | Main | Keywords: | regression |
Hardware: | All | ||
OS: | All | ||
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: | |
Bug Depends on: | 30254 | ||
Bug Blocks: | |||
Attachments: |
Bug 38477: UNit test
Bug 38477: Unit test Bug 38477: Only charge a new lost fine when the last patron to return the item matches the original fined patron Bug 38477: Only charge a new lost fine when the last patron to return the item matches the original fined patron Bug 38477: Unit test Bug 38477: Only charge a new lost fine when the last patron to return the item matches the original fined patron Bug 38477: (follow-up) Correct failing Item.t test Bug 38477: (follow-up) Correct failing Item.t test |
Description
Andrew Fuerste-Henry
2024-11-18 20:10:10 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 Marking as CONFIRMED on behalf of Nick Created attachment 182701 [details] [review] Bug 38477: UNit test Created attachment 182726 [details] [review] Bug 38477: Unit test 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. SQL for step 12? Sorry, I'm not familiar enough with the database structure to work out what table this data is in. (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]; (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. 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> 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 Created attachment 182827 [details] [review] Bug 38477: Unit test Signed-off-by: Pedro Amorim <pedro.amorim@openfifth.co.uk> 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> Nice work everyone! Pushed to main for 25.11 This seems to be causing 't/db_dependent/Koha/Item.t' tests to fail. Created attachment 182916 [details] [review] Bug 38477: (follow-up) Correct failing Item.t test prove /kohadevbox/koha/t/db_dependent/Koha/Item.t Can someone please look at/sign-off my follow up before I push to main? 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> follow-up pushed to main I see in 25.05.x Pushed to 24.11.x for 24.11.06 Nice work everyone! Pushed to 25.05.x for 25.05.03 |