To recreate: 1 - Checkout an item to a patron and make it overdue (can backdate the checkout) 2 - Make sure the itemtype has fines that will be charged 3 - Charge the fines: Set finesMode = production perl misc/cronjobs/fines.pl -v 4 - Browse to Circulation->Check in 5 - In 'Checkin settings' backdate to before the due date and select forgive fines 6- Explode!!!!! Debit amount passed is not positive at /usr/share/perl5/Exception/Class/Base.pm line 88
Created attachment 104535 [details] [review] Bug 25417: Add unit test
Created attachment 104536 [details] [review] Bug 25417: Prevent backdating returns and forgiving fines as the same time from causing an internal server error
Created attachment 104575 [details] [review] Bug 25417: Add unit test Signed-off-by: David Nind <david@davidnind.com>
Created attachment 104576 [details] [review] Bug 25417: Prevent backdating returns and forgiving fines as the same time from causing an internal server error Signed-off-by: David Nind <david@davidnind.com>
Created attachment 104578 [details] [review] [ALTERNATIVE-PATCH] Bug 25417: Prevent negative debit amount on backdating returns Koha::Account->add_credit is expecting a positive amount.
Kyle, what are your thoughts on this alternative patch? I think it makes it more readable and avoid the ->store call as well.
Created attachment 104579 [details] [review] Bug 25417: Prevent negative debit amount on backdating returns
Created attachment 104580 [details] [review] Bug 25417: Prevent negative debit amount on backdating returns
Created attachment 104644 [details] [review] Bug 25417: Add unit test Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Created attachment 104645 [details] [review] Bug 25417: Prevent negative debit amount on backdating returns Koha::Account->add_credit is expecting a positive amount. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com>
Not sure I'm understanding the Unit test added here.. how, 'in real life' can you end up with a negative amountoutstanding amount?.. I ask as I'm not sure how it relates to the issue reported?
(In reply to Martin Renvoize from comment #11) > Not sure I'm understanding the Unit test added here.. how, 'in real life' > can you end up with a negative amountoutstanding amount?.. I ask as I'm not > sure how it relates to the issue reported? If someone pays an accruing fine and then the item is backdated via dropbox or something. It can easily happen that a patron drops something in the box, then comes inside and pays fines, not realizing one of them is the one they returned etc
This would leave us with a zero value 'OVERDUE' fine with an 'UNRETURNED' status on the item even though the item has been returned. Took me a while to work through it and I'm still struggling to actually replicate the issue. I'll work on improving the test to clarify it's intent for this bug report.
Created attachment 104715 [details] [review] Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test
Created attachment 104716 [details] [review] Bug 25417: (QA follow-up) Restore 'fixing' of fines
Some minor improvements to the test to highlight the requirement to 'fix' fines regardless of exemptfine. I'd still really love to see a test added to AddReturn to actually test the case highlighted by Nick in the first comment.. currently we still don't actually test that codepath as a whole. Let me know what you think of the final followup which I believe corrects the behavior as a whole.
Created attachment 104730 [details] [review] Bug 25417: Add unit test Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 104731 [details] [review] Bug 25417: Prevent negative debit amount on backdating returns Koha::Account->add_credit is expecting a positive amount. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 104732 [details] [review] Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 104733 [details] [review] Bug 25417: (QA follow-up) Restore 'fixing' of fines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 104734 [details] [review] Bug 25417: Add integration test for AddReturn Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
That makes sense to me Martin, but I would prefer Nick or Kyle to confirm it with their QA stamp.
Created attachment 104735 [details] [review] Bug 25417: Add unit test Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 104736 [details] [review] Bug 25417: Prevent negative debit amount on backdating returns Koha::Account->add_credit is expecting a positive amount. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 104737 [details] [review] Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 104738 [details] [review] Bug 25417: (QA follow-up) Restore 'fixing' of fines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 104739 [details] [review] Bug 25417: (QA follow-up) Add integration test for AddReturn Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
I'm happy with this now... This improves the behaviour to properly skip adding a FORGIVEN type credit for OVERDUES that have already been annulled by a backdated return with CalculateFinesOnBackdate. We still end up with a zero amount fine line, which I'm still not entirely sure about, but as that question has been unanswered in bugs 22539 and 8338 already I don't think to ask it here and holding up this fix is the correct action to take. I've also modified the clause to only skip on '0'.. if we somehow end up with a negative here we do still want to throw an exception as it highlights issues elsewhere in the codepaths prior to this point.. we should not just silently ignore that in my opinion. Passing QA
Holding off on pushing until I get a final stamp from Kyle, Nick or Tomas.. all whom I believe are close enough to accounts or this issue to be able to confirm the logic.
Created attachment 104749 [details] [review] Bug 25417: Add unit test Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 104750 [details] [review] Bug 25417: Add unit test Signed-off-by: David Nind <david@davidnind.com> Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 104751 [details] [review] Bug 25417: Prevent negative debit amount on backdating returns Koha::Account->add_credit is expecting a positive amount. Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com> Signed-off-by: Didier Gautheron <didier.gautheron@biblibre.com> Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 104752 [details] [review] Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 104753 [details] [review] Bug 25417: (QA follow-up) Restore 'fixing' of fines Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Created attachment 104754 [details] [review] Bug 25417: (QA follow-up) Add integration test for AddReturn Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Kyle M Hall <kyle@bywatersolutions.com>
Nice work everyone! Pushed to master for 20.05
Pushed to 19.11.x for 19.11.06
For 19.05.x unit tests are failing: ok 9 - Credit amountoutstanding is correctly set to 0 not ok 33 - _FixOverduesOnReturn # Looks like you planned 14 tests but ran 9. # Failed test '_FixOverduesOnReturn' # at t/db_dependent/Circulation.t line 2587. No property debit_type_code for Koha::Account::Line# Looks like your test exited with 255 just after 33. Dubious, test returned 255 (wstat 65280, 0xff00) Failed 14/46 subtests Not backporting