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.
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
This remains an issue in 21.11.
Still applicable as of 22.05.05.
Created attachment 142839 [details] [review] Bug 30254: Unit tests
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
Created attachment 142851 [details] [review] Bug 30254: Unit tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
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>
Created attachment 142853 [details] [review] Bug 30254: (QA follow-up) Remove warn from tests Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Thanks for beating me to this one Nick, much appreciated.. Simple fix, signing off.
I am sorry, looks good, passes QA tests and unit tests, but couldn't finish testing here (ran out of time).
I think this may have fallen off Katrin's radar... bump
Happy for someone else to take this on - please don't wait on me!
(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 :)
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.
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.
(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
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>
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>
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>
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>
Pushed to master for 23.05. Nice work everyone, thanks!
Nice work, thanks everyone! Pushed to 22.11.x for the next release.
Tests won't apply to 22.05.x, if needed please rebase.
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
Backported to 22.05.x for upcoming 22.05.10
Conflicts when trying to apply to 21.11.x from 22.05.x. Can you provide a backport patch if needed?