Bug 20598

Summary: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled
Product: Koha Reporter: Kyle M Hall (khall) <kyle>
Component: CirculationAssignee: Kyle M Hall (khall) <kyle>
Status: CLOSED FIXED QA Contact: Marcel de Rooy <m.de.rooy>
Severity: normal    
Priority: P5 - low CC: fridolin.somers, gmcharlt, jonathan.druart, kyle.m.hall, lucas, m.de.rooy, martin.renvoize, nick
Version: Main   
Hardware: All   
OS: All   
See Also: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=21849
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=22902
Change sponsored?: --- Patch complexity: Small patch
Documentation contact: Documentation submission:
Text to go in the release notes:
Version(s) released in:
18.11.00, 18.05.07, 17.11.13
Circulation function:
Attachments: Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled
Bug 20598: Unit test
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled
Bug 20598: Unit test
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled
Bug 20598: Unit test
Bug 20598: (QA follow-up) Tiny fixes

Description Kyle M Hall (khall) 2018-04-17 18:07:57 UTC

    
Comment 1 Kyle M Hall (khall) 2018-04-17 18:13:28 UTC
Created attachment 74377 [details] [review]
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled

Test Plan:
1) Ensure WhenLostForgiveFine is disabled
2) Create an overdue with a fine
3) Mark it lost with longoverdue.pl
4) Note it is still marked as an accruing fine
5) Apply this patch
6) Repeat steps 1-3
7) Note it is no longer an accruing fine!
Comment 2 Maryse Simard 2018-06-08 19:38:26 UTC
Created attachment 75932 [details] [review]
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled

Test Plan:
1) Ensure WhenLostForgiveFine is disabled
2) Create an overdue with a fine
3) Mark it lost with longoverdue.pl
4) Note it is still marked as an accruing fine
5) Apply this patch
6) Repeat steps 1-3
7) Note it is no longer an accruing fine!

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Followed the test plan and it works.
Comment 3 Jonathan Druart 2018-06-12 15:31:28 UTC
Please provide tests to cover this change.
Comment 4 Kyle M Hall (khall) 2018-11-05 13:33:15 UTC
Created attachment 81953 [details] [review]
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled

Test Plan:
1) Ensure WhenLostForgiveFine is disabled
2) Create an overdue with a fine
3) Mark it lost with longoverdue.pl
4) Note it is still marked as an accruing fine
5) Apply this patch
6) Repeat steps 1-3
7) Note it is no longer an accruing fine!

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Followed the test plan and it works.
Comment 5 Kyle M Hall (khall) 2018-11-05 13:33:45 UTC
Created attachment 81954 [details] [review]
Bug 20598: Unit test
Comment 6 Marcel de Rooy 2018-11-16 09:19:59 UTC
QA: Looking here
Comment 7 Marcel de Rooy 2018-11-16 09:23:41 UTC
Author: root <root@e7279b8b0511>
Comment 8 Marcel de Rooy 2018-11-16 09:25:10 UTC
kyle@bywatetsolutions.com

bywatet ??
Comment 9 Marcel de Rooy 2018-11-16 09:50:15 UTC
Created attachment 82391 [details] [review]
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled

Test Plan:
1) Ensure WhenLostForgiveFine is disabled
2) Create an overdue with a fine
3) Mark it lost with longoverdue.pl
4) Note it is still marked as an accruing fine
5) Apply this patch
6) Repeat steps 1-3
7) Note it is no longer an accruing fine!

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Followed the test plan and it works.
Comment 10 Marcel de Rooy 2018-11-16 09:50:20 UTC
Created attachment 82392 [details] [review]
Bug 20598: Unit test
Comment 11 Marcel de Rooy 2018-11-16 09:51:17 UTC
Not ready yet; _FixOverduesOnReturn needs fixing !
Comment 12 Marcel de Rooy 2018-11-16 10:12:10 UTC
(In reply to Marcel de Rooy from comment #11)
> Not ready yet; _FixOverduesOnReturn needs fixing !

See bug 21849 about useless calls of Koha::Account::Offset->new.
Comment 13 Marcel de Rooy 2018-11-16 10:48:56 UTC
Circulation.t

    t::lib::Mocks::mock_preference('WhenLostForgiveFine','0');
    t::lib::Mocks::mock_preference('WhenLostChargeReplacementFee','0');

   [...]

    is( $total_due, '15.000000', 'Borrower only charged replacement fee with both WhenLostForgiveFine and WhenLostChargeReplacementFee enabled' );

    [...]

    ok( $total_due == 15, 'Borrower only charged fine with both WhenLostForgiveFine and WhenLostChargeReplacementFee disabled' );

Hmm. Enabled? Replacement fee was zero ?
Comment 14 Marcel de Rooy 2018-11-16 10:50:31 UTC
Created attachment 82394 [details] [review]
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled

Test Plan:
1) Ensure WhenLostForgiveFine is disabled
2) Create an overdue with a fine
3) Mark it lost with longoverdue.pl
4) Note it is still marked as an accruing fine
5) Apply this patch
6) Repeat steps 1-3
7) Note it is no longer an accruing fine!

Signed-off-by: Maryse Simard <maryse.simard@inlibro.com>
Followed the test plan and it works.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 15 Marcel de Rooy 2018-11-16 10:50:36 UTC
Created attachment 82395 [details] [review]
Bug 20598: Unit test

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 16 Marcel de Rooy 2018-11-16 10:50:41 UTC
Created attachment 82396 [details] [review]
Bug 20598: (QA follow-up) Tiny fixes

[1] Correct POD for _FixOverduesOnReturn
Is called by AddReturn, AddRenewal and LostItem.
Also tested in Circulation.t btw

[2] $dbh is not used in _FixOverduesOnReturn
[3] Moving all parameters to the first line.
[4] Variable $uquery is not used too.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 17 Marcel de Rooy 2018-11-16 10:52:54 UTC
Passing QA. Tested a bit and works. Overall impression however is that things are handled a bit messy.Look at previous comments. Opened a new bug too.
Comment 18 Nick Clemens (kidclamp) 2018-11-16 13:09:15 UTC
Awesome work all!

Pushed to master for 18.11
Comment 19 Lucas Gass (lukeg) 2018-12-04 19:50:54 UTC
backported to 18.05 for 18.05.07
Comment 20 Fridolin Somers 2018-12-05 08:25:19 UTC
Pushed to 17.11.x for 17.11.13