Bug 30254

Summary: New overdue fine applied to incorrectly when using "Refund lost item charge and charge new overdue fine" option in circ rules
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: Fines and feesAssignee: Nick Clemens <nick>
Status: Pushed to stable --- QA Contact: Kyle M Hall <kyle>
Severity: critical    
Priority: P5 - low CC: hebah, kyle, m.de.rooy, martin.renvoize, nick, sbrown
Version: master   
Hardware: All   
OS: All   
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
Bug Depends on: 23091    
Bug Blocks:    
Attachments: Bug 30254: Unit tests
Bug 30254: Don't charge overdue fines unless some fine exists
Bug 30254: Unit tests
Bug 30254: Don't charge overdue fines unless some fine exists
Bug 30254: (QA follow-up) Remove warn from tests
Bug 30254: Unit tests
Bug 30254: Unit tests
Bug 30254: Don't charge overdue fines unless some fine exists
Bug 30254: (QA follow-up) Remove warn from tests

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 2022-10-31 18:44:33 UTC
Created attachment 142839 [details] [review]
Bug 30254: Unit tests
Comment 5 Nick Clemens 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 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 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 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 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 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 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 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 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 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 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 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.