Bug 28097

Summary: t/db_dependent/Koha/Account/Line.t test fails with FinesMode set to calculate
Product: Koha Reporter: Andrew Fuerste-Henry <andrewfh>
Component: Fines and feesAssignee: Martin Renvoize <martin.renvoize>
Status: CLOSED FIXED QA Contact: Testopia <testopia>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, kyle, martin.renvoize, nick, victor
Version: Main   
Hardware: All   
OS: All   
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
Bug Depends on: 8338    
Bug Blocks:    
Attachments: Bug 28097: Mock finesModes to disable recalculation
Bug 28097: Mock finesModes to disable recalculation

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 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 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.