Bug 28097 - t/db_dependent/Koha/Account/Line.t test fails with FinesMode set to calculate
Summary: t/db_dependent/Koha/Account/Line.t test fails with FinesMode set to calculate
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: Main
Hardware: All All
: P5 - low normal (vote)
Assignee: Martin Renvoize
QA Contact: Testopia
URL:
Keywords:
Depends on: 8338
Blocks:
  Show dependency treegraph
 
Reported: 2021-04-06 16:17 UTC by Andrew Fuerste-Henry
Modified: 2021-12-13 21:10 UTC (History)
5 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:
21.05.00,20.11.05,20.05.11


Attachments
Bug 28097: Mock finesModes to disable recalculation (1.83 KB, patch)
2021-04-07 11:13 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 28097: Mock finesModes to disable recalculation (1.89 KB, patch)
2021-04-07 11:30 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 2021-04-06 16:17:37 UTC
To recreate:
- Confirm finesmode is set to do not calculate
- prove t/db_dependent/Koha/Account/Line.t
- it passes!
- set FinesMode ot calculate and charge
- prove t/db_dependent/Koha/Account/Line.t
- it fails 

root@kohadevbox:koha(master)$ prove t/db_dependent/Koha/Account/Line.t
t/db_dependent/Koha/Account/Line.t .. 7/14     # Looks like you planned 6 tests but ran 3.

#   Failed test 'checkout() tests'
#   at t/db_dependent/Koha/Account/Line.t line 615.
Can't call method "checkout" on an undefined value at t/db_dependent/Koha/Account/Line.t line 607.
# Looks like your test exited with 255 just after 9.
t/db_dependent/Koha/Account/Line.t .. Dubious, test returned 255 (wstat 65280, 0xff00)
Failed 6/14 subtests 

Test Summary Report
-------------------
t/db_dependent/Koha/Account/Line.t (Wstat: 65280 Tests: 9 Failed: 1)
  Failed test:  9
  Non-zero exit status: 255
  Parse errors: Bad plan.  You planned 14 tests but ran 9.
Files=1, Tests=9,  5 wallclock secs ( 0.02 usr  0.01 sys +  3.32 cusr  0.65 csys =  4.00 CPU)
Result: FAIL
Comment 1 Martin Renvoize 2021-04-07 11:05:20 UTC
This is a side effect of bug 8338.

With finesMode set to 'production' and CalculateFinesOnReturn set to 'enabled' upon return we will first re-calculate the fine amount (in this case that would result in a '0' amount fine).. we then call '_FixOverduesOnReturn' which subsequently fines the '0' amount fine and deletes it on the assumption it's a backdated return which had resulted in an errant fine.

I wonder if, rather than delete the original fine, we should void it which would give a more thorough audit trail (and fix this bug).. However, I think such a VOID would still appear on the patrons account in the OPAC and that was what I believe people wanted to avoid with bug 8338.

The easy fix here is to adjust the test to mock the two preferences to ensure they are disabled for the test case.
Comment 2 Martin Renvoize 2021-04-07 11:13:43 UTC
Created attachment 119275 [details] [review]
Bug 28097: Mock finesModes to disable recalculation

Since bug 8338 the recalcuated zero amount fine is subsequently removed
in _FixOverduesOnReturn which causes this test to fail if finesMode is
'production' and 'CalculateFinesOnReturn' is enabled.

Perhaps we should be 'VOID'ing fines rather than completely deleting
them, but that's a question for another day.

Test plan
1/ Enable finesMode in the staff client
2/ Run the test and watch it fail
3/ Apply the patch
4/ Run the test and watch it pass
Comment 3 Nick Clemens (kidclamp) 2021-04-07 11:30:53 UTC
Created attachment 119276 [details] [review]
Bug 28097: Mock finesModes to disable recalculation

Since bug 8338 the recalcuated zero amount fine is subsequently removed
in _FixOverduesOnReturn which causes this test to fail if finesMode is
'production' and 'CalculateFinesOnReturn' is enabled.

Perhaps we should be 'VOID'ing fines rather than completely deleting
them, but that's a question for another day.

Test plan
1/ Enable finesMode in the staff client
2/ Run the test and watch it fail
3/ Apply the patch
4/ Run the test and watch it pass

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 4 Nick Clemens (kidclamp) 2021-04-07 11:31:18 UTC
Small fix, passing QA
Comment 5 Jonathan Druart 2021-04-08 13:42:25 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 6 Fridolin Somers 2021-04-19 10:11:51 UTC
Pushed to 20.11.x for 20.11.05
Comment 7 Andrew Fuerste-Henry 2021-04-25 14:20:25 UTC
Pushed to 20.05.x for 20.05.11
Comment 8 Victor Grousset/tuxayo 2021-04-25 17:19:31 UTC
Not backported to oldoldstable (19.11.x). Feel free to ask if it's needed.