Bug 30254 - New overdue fine applied to incorrectly when using "Refund lost item charge and charge new overdue fine" option in circ rules
Summary: New overdue fine applied to incorrectly when using "Refund lost item charge a...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: Main
Hardware: All All
: P5 - low critical
Assignee: Nick Clemens (kidclamp)
QA Contact: Kyle M Hall (khall)
URL:
Keywords:
Depends on: 23091
Blocks: 38477
  Show dependency treegraph
 
Reported: 2022-03-09 12:58 UTC by Andrew Fuerste-Henry
Modified: 2024-11-18 20:10 UTC (History)
8 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
23.05.00,22.11.03, 22.05.10
Circulation function:


Attachments
Bug 30254: Unit tests (1.22 KB, patch)
2022-10-31 18:44 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 30254: Don't charge overdue fines unless some fine exists (5.26 KB, patch)
2022-10-31 18:44 UTC, Nick Clemens (kidclamp)
Details | Diff | Splinter Review
Bug 30254: Unit tests (1.29 KB, patch)
2022-11-01 10:21 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30254: Don't charge overdue fines unless some fine exists (5.32 KB, patch)
2022-11-01 10:21 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30254: (QA follow-up) Remove warn from tests (933 bytes, patch)
2022-11-01 10:21 UTC, Martin Renvoize (ashimema)
Details | Diff | Splinter Review
Bug 30254: Unit tests (1.34 KB, patch)
2023-01-20 18:23 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 30254: Unit tests (1.34 KB, patch)
2023-01-20 18:25 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 30254: Don't charge overdue fines unless some fine exists (5.38 KB, patch)
2023-01-20 18:25 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 30254: (QA follow-up) Remove warn from tests (989 bytes, patch)
2023-01-20 18:25 UTC, Kyle M Hall (khall)
Details | Diff | Splinter Review
Bug 30254: [22.05.x] Don't charge overdue fines unless some fine exists (6.77 KB, patch)
2023-02-22 19:57 UTC, Nick Clemens (kidclamp)
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 2022-03-09 12:58:20 UTC
I've only confirmed this on 21.05. I haven't been able to replicate on master.
When using the lost item fee refund feature to charge a new overdue fee when a lost item is found, Koha can end up creating an overdue fine on a patron who never actually had an overdue item.

To recreate:
- set Lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine"
- have an itemtype / patron combo that charges an overdue fine
- check item out to patron and then right back in again
- confirm patron doesn't have a fine because the item was not late
- set the item to Missing
- wait until after the item's due date from your previous checkout
- check the item out to a new patron
- confirm your original patron now has a fine

When the item is checked out and Koha clears its missing status, it looks back for the most recent checkout in old_issues and generates an overdue fine as if that item had been checked out the whole time. Even though this item wasn't lost by a patron and the first patron never had an overdue fine on it.

Things get more confusing if the first patron is anonymizing their reading history, as then the fine ends up on the anonymous borrower.
Comment 1 Andrew Fuerste-Henry 2022-04-25 15:22:35 UTC
Confirmed in master. To recreate:
1 - set Lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine", turn on FinesMode, make sure your circ rules charge fines
2 - have an itemtype / patron combo that charges an overdue fine
3 - check item out (with a due date in the future) and then right back in again
4 - confirm patron doesn't have a fine because the item was not late
5 - set the item to Lost
6 - in the database, edit the date_due of your checkout to a date in the past
7 - check the item in, it is marked found
8 - confirm your patron now has a fine
Comment 2 Sara Brown 2022-08-24 18:33:04 UTC
This remains an issue in 21.11.
Comment 3 Sara Brown 2022-10-28 17:50:53 UTC
Still applicable as of 22.05.05.
Comment 4 Nick Clemens (kidclamp) 2022-10-31 18:44:33 UTC
Created attachment 142839 [details] [review]
Bug 30254: Unit tests
Comment 5 Nick Clemens (kidclamp) 2022-10-31 18:44:37 UTC
Created attachment 142840 [details] [review]
Bug 30254: Don't charge overdue fines unless some fine exists

We need to determine if a book was lost by a patron, the clues we have
are previous charges. If we don't find any, we shouldn't charge a new fine

To test:
 1 - set Lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine", turn on FinesMode, make sure your circ rules charge fines
 2 - have an itemtype / patron combo that charges an overdue fine
 3 - check item out (with a due date in the future) and then right back in again
 4 - confirm patron doesn't have a fine because the item was not late
 5 - set the item to Lost
 6 - in the database, edit the date_due of your checkout to a date in the past
 7 - check the item in, it is marked found
 8 - confirm your patron now has a fine
 9 - Apply patch
10 - Repeat with a new item and patron
11 - Confirm no charges
Comment 6 Martin Renvoize (ashimema) 2022-11-01 10:21:33 UTC
Created attachment 142851 [details] [review]
Bug 30254: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 7 Martin Renvoize (ashimema) 2022-11-01 10:21:37 UTC
Created attachment 142852 [details] [review]
Bug 30254: Don't charge overdue fines unless some fine exists

We need to determine if a book was lost by a patron, the clues we have
are previous charges. If we don't find any, we shouldn't charge a new fine

To test:
 1 - set Lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine", turn on FinesMode, make sure your circ rules charge fines
 2 - have an itemtype / patron combo that charges an overdue fine
 3 - check item out (with a due date in the future) and then right back in again
 4 - confirm patron doesn't have a fine because the item was not late
 5 - set the item to Lost
 6 - in the database, edit the date_due of your checkout to a date in the past
 7 - check the item in, it is marked found
 8 - confirm your patron now has a fine
 9 - Apply patch
10 - Repeat with a new item and patron
11 - Confirm no charges

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 8 Martin Renvoize (ashimema) 2022-11-01 10:21:42 UTC
Created attachment 142853 [details] [review]
Bug 30254: (QA follow-up) Remove warn from tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 9 Martin Renvoize (ashimema) 2022-11-01 10:22:33 UTC
Thanks for beating me to this one Nick, much appreciated..

Simple fix, signing off.
Comment 10 Katrin Fischer 2022-11-03 11:06:09 UTC
I am sorry, looks good, passes QA tests and unit tests, but couldn't finish testing here (ran out of time).
Comment 11 Martin Renvoize (ashimema) 2022-11-23 10:06:22 UTC
I think this may have fallen off Katrin's radar... bump
Comment 12 Katrin Fischer 2022-12-04 12:50:56 UTC
Happy for someone else to take this on - please don't wait on me!
Comment 13 Marcel de Rooy 2022-12-16 08:25:47 UTC
(In reply to Martin Renvoize from comment #9)
> Thanks for beating me to this one Nick, much appreciated..
> 
> Simple fix, signing off.

Looking here now. If this is a simple fix, I may not understand Koha :)
Comment 14 Marcel de Rooy 2022-12-16 08:27:34 UTC
A first glance tells me: The unit test does not cover all changes made in the second patch. There is a huge amount of changed lines and only one very trivial test that does not convince me at all.
Comment 15 Marcel de Rooy 2022-12-16 08:51:54 UTC
This is a bit arbitrary, but looking at sub _set_found_trigger and the associated tests in subtest '_set_found_trigger() tests', I think that we should improve the tests here. It looks like that we do not even check the account balances while we are refunding and charging..

Changing status for need of feedback/further attention.
Comment 16 Nick Clemens (kidclamp) 2022-12-16 14:30:25 UTC
(In reply to Marcel de Rooy from comment #14)
> A first glance tells me: The unit test does not cover all changes made in
> the second patch. There is a huge amount of changed lines and only one very
> trivial test that does not convince me at all.

git diff -w HEAD~3.. Koha/Item.pm

It changes three lines to prevent sending a 'lost_charge' message when there is no charge to replace  

The test fails before, and passes after and other behaviour is covered by _set_found_trigger tests. I agree, expanding coverage is good, but should be on another bug
Comment 17 Kyle M Hall (khall) 2023-01-20 18:23:48 UTC
Created attachment 145549 [details] [review]
Bug 30254: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 18 Kyle M Hall (khall) 2023-01-20 18:25:33 UTC
Created attachment 145550 [details] [review]
Bug 30254: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 19 Kyle M Hall (khall) 2023-01-20 18:25:46 UTC
Created attachment 145551 [details] [review]
Bug 30254: Don't charge overdue fines unless some fine exists

We need to determine if a book was lost by a patron, the clues we have
are previous charges. If we don't find any, we shouldn't charge a new fine

To test:
 1 - set Lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine", turn on FinesMode, make sure your circ rules charge fines
 2 - have an itemtype / patron combo that charges an overdue fine
 3 - check item out (with a due date in the future) and then right back in again
 4 - confirm patron doesn't have a fine because the item was not late
 5 - set the item to Lost
 6 - in the database, edit the date_due of your checkout to a date in the past
 7 - check the item in, it is marked found
 8 - confirm your patron now has a fine
 9 - Apply patch
10 - Repeat with a new item and patron
11 - Confirm no charges

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 20 Kyle M Hall (khall) 2023-01-20 18:25:50 UTC
Created attachment 145552 [details] [review]
Bug 30254: (QA follow-up) Remove warn from tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Comment 21 Tomás Cohen Arazi (tcohen) 2023-01-30 15:15:40 UTC
Pushed to master for 23.05.

Nice work everyone, thanks!
Comment 22 Jacob O'Mara 2023-01-31 17:19:27 UTC
Nice work, thanks everyone!

Pushed to 22.11.x for the next release.
Comment 23 Lucas Gass (lukeg) 2023-02-01 19:07:27 UTC
Tests won't apply to 22.05.x, if needed please rebase.
Comment 24 Nick Clemens (kidclamp) 2023-02-22 19:57:34 UTC
Created attachment 147185 [details] [review]
Bug 30254: [22.05.x] Don't charge overdue fines unless some fine exists

We need to determine if a book was lost by a patron, the clues we have
are previous charges. If we don't find any, we shouldn't charge a new fine

To test:
 1 - set Lost item fee refund on return policy to "Refund lost item charge and charge new overdue fine", turn on FinesMode, make sure your circ rules charge fines
 2 - have an itemtype / patron combo that charges an overdue fine
 3 - check item out (with a due date in the future) and then right back in again
 4 - confirm patron doesn't have a fine because the item was not late
 5 - set the item to Lost
 6 - in the database, edit the date_due of your checkout to a date in the past
 7 - check the item in, it is marked found
 8 - confirm your patron now has a fine
 9 - Apply patch
10 - Repeat with a new item and patron
11 - Confirm no charges

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 30254: Unit tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

Bug 30254: (QA follow-up) Remove warn from tests

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>

Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>

https://bugs.koha-community.org/show_bug.cgi?id=32054
Comment 25 Lucas Gass (lukeg) 2023-02-24 13:36:41 UTC
Backported to 22.05.x for upcoming 22.05.10
Comment 26 Arthur Suzuki 2023-02-28 07:46:40 UTC
Conflicts when trying to apply to 21.11.x from 22.05.x.
Can you provide a backport patch if needed?