Bug 25417 - Backdating returns and forgiving fines causes and internal server error
Summary: Backdating returns and forgiving fines causes and internal server error
Status: Pushed to stable
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low blocker (vote)
Assignee: Kyle M Hall
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 22539
  Show dependency treegraph
 
Reported: 2020-05-07 14:31 UTC by Nick Clemens
Modified: 2020-05-18 09:48 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Who signed the patch off:
Text to go in the release notes:
Version(s) released in:
20.05.00, 19.11.06


Attachments
Bug 25417: Add unit test (1.80 KB, patch)
2020-05-07 18:24 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25417: Prevent backdating returns and forgiving fines as the same time from causing an internal server error (2.74 KB, patch)
2020-05-07 18:25 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25417: Add unit test (1.83 KB, patch)
2020-05-08 10:09 UTC, David Nind
Details | Diff | Splinter Review
Bug 25417: Prevent backdating returns and forgiving fines as the same time from causing an internal server error (2.77 KB, patch)
2020-05-08 10:09 UTC, David Nind
Details | Diff | Splinter Review
[ALTERNATIVE-PATCH] Bug 25417: Prevent negative debit amount on backdating returns (889 bytes, patch)
2020-05-08 10:30 UTC, Jonathan Druart
Details | Diff | Splinter Review
Bug 25417: Prevent negative debit amount on backdating returns (946 bytes, patch)
2020-05-08 10:59 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25417: Prevent negative debit amount on backdating returns (947 bytes, patch)
2020-05-08 11:00 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25417: Add unit test (1.89 KB, patch)
2020-05-11 06:17 UTC, didier
Details | Diff | Splinter Review
Bug 25417: Prevent negative debit amount on backdating returns (1011 bytes, patch)
2020-05-11 06:17 UTC, didier
Details | Diff | Splinter Review
Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test (2.17 KB, patch)
2020-05-11 20:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: (QA follow-up) Restore 'fixing' of fines (1.08 KB, patch)
2020-05-11 20:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: Add unit test (1.96 KB, patch)
2020-05-12 08:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: Prevent negative debit amount on backdating returns (1.05 KB, patch)
2020-05-12 08:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test (2.23 KB, patch)
2020-05-12 08:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: (QA follow-up) Restore 'fixing' of fines (1.15 KB, patch)
2020-05-12 08:25 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: Add integration test for AddReturn (3.10 KB, patch)
2020-05-12 08:26 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: Add unit test (1.96 KB, patch)
2020-05-12 08:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: Prevent negative debit amount on backdating returns (1.05 KB, patch)
2020-05-12 08:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test (2.23 KB, patch)
2020-05-12 08:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: (QA follow-up) Restore 'fixing' of fines (1.15 KB, patch)
2020-05-12 08:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: (QA follow-up) Add integration test for AddReturn (3.67 KB, patch)
2020-05-12 08:32 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 25417: Add unit test (2.03 KB, patch)
2020-05-12 10:58 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25417: Add unit test (2.03 KB, patch)
2020-05-12 11:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25417: Prevent negative debit amount on backdating returns (1.12 KB, patch)
2020-05-12 11:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test (2.29 KB, patch)
2020-05-12 11:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25417: (QA follow-up) Restore 'fixing' of fines (1.22 KB, patch)
2020-05-12 11:03 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 25417: (QA follow-up) Add integration test for AddReturn (3.65 KB, patch)
2020-05-12 11:03 UTC, Kyle M Hall
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2020-05-07 14:31:52 UTC
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
Comment 1 Kyle M Hall 2020-05-07 18:24:54 UTC
Created attachment 104535 [details] [review]
Bug 25417: Add unit test
Comment 2 Kyle M Hall 2020-05-07 18:25:03 UTC
Created attachment 104536 [details] [review]
Bug 25417: Prevent backdating returns and forgiving fines as the same time from causing an internal server error
Comment 3 David Nind 2020-05-08 10:09:09 UTC
Created attachment 104575 [details] [review]
Bug 25417: Add unit test

Signed-off-by: David Nind <david@davidnind.com>
Comment 4 David Nind 2020-05-08 10:09:12 UTC
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>
Comment 5 Jonathan Druart 2020-05-08 10:30:11 UTC
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.
Comment 6 Jonathan Druart 2020-05-08 10:30:54 UTC
Kyle, what are your thoughts on this alternative patch? I think it makes it more readable and avoid the ->store call as well.
Comment 7 Kyle M Hall 2020-05-08 10:59:44 UTC
Created attachment 104579 [details] [review]
Bug 25417: Prevent negative debit amount on backdating returns
Comment 8 Kyle M Hall 2020-05-08 11:00:55 UTC
Created attachment 104580 [details] [review]
Bug 25417: Prevent negative debit amount on backdating returns
Comment 9 didier 2020-05-11 06:17:02 UTC
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>
Comment 10 didier 2020-05-11 06:17:06 UTC
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>
Comment 11 Martin Renvoize 2020-05-11 14:04:34 UTC
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?
Comment 12 Nick Clemens 2020-05-11 17:20:35 UTC
(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
Comment 13 Martin Renvoize 2020-05-11 18:24:09 UTC
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.
Comment 14 Martin Renvoize 2020-05-11 20:16:31 UTC
Created attachment 104715 [details] [review]
Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test
Comment 15 Martin Renvoize 2020-05-11 20:16:35 UTC
Created attachment 104716 [details] [review]
Bug 25417: (QA follow-up) Restore 'fixing' of fines
Comment 16 Martin Renvoize 2020-05-11 20:22:28 UTC
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.
Comment 17 Martin Renvoize 2020-05-12 08:25:49 UTC
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>
Comment 18 Martin Renvoize 2020-05-12 08:25:53 UTC
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>
Comment 19 Martin Renvoize 2020-05-12 08:25:56 UTC
Created attachment 104732 [details] [review]
Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 20 Martin Renvoize 2020-05-12 08:25:59 UTC
Created attachment 104733 [details] [review]
Bug 25417: (QA follow-up) Restore 'fixing' of fines

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 21 Martin Renvoize 2020-05-12 08:26:03 UTC
Created attachment 104734 [details] [review]
Bug 25417: Add integration test for AddReturn

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 22 Jonathan Druart 2020-05-12 08:30:53 UTC
That makes sense to me Martin, but I would prefer Nick or Kyle to confirm it with their QA stamp.
Comment 23 Martin Renvoize 2020-05-12 08:32:00 UTC
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>
Comment 24 Martin Renvoize 2020-05-12 08:32:04 UTC
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>
Comment 25 Martin Renvoize 2020-05-12 08:32:08 UTC
Created attachment 104737 [details] [review]
Bug 25417: (QA follow-up) Clarify _FixOverduesOnReturn test

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 26 Martin Renvoize 2020-05-12 08:32:14 UTC
Created attachment 104738 [details] [review]
Bug 25417: (QA follow-up) Restore 'fixing' of fines

Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Comment 27 Martin Renvoize 2020-05-12 08:32:17 UTC
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>
Comment 28 Martin Renvoize 2020-05-12 08:36:53 UTC
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
Comment 29 Martin Renvoize 2020-05-12 08:38:26 UTC
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.
Comment 30 Kyle M Hall 2020-05-12 10:58:25 UTC
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>
Comment 31 Kyle M Hall 2020-05-12 11:03:12 UTC
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>
Comment 32 Kyle M Hall 2020-05-12 11:03:21 UTC
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>
Comment 33 Kyle M Hall 2020-05-12 11:03:27 UTC
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>
Comment 34 Kyle M Hall 2020-05-12 11:03:30 UTC
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>
Comment 35 Kyle M Hall 2020-05-12 11:03:34 UTC
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>
Comment 36 Martin Renvoize 2020-05-12 12:47:16 UTC
Nice work everyone!

Pushed to master for 20.05
Comment 37 Joy Nelson 2020-05-13 19:21:20 UTC
Pushed to 19.11.x for 19.11.06
Comment 38 Lucas Gass 2020-05-15 22:42:00 UTC
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