Bug 20598 - Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled
Summary: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low normal (vote)
Assignee: Kyle M Hall
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2018-04-17 18:07 UTC by Kyle M Hall
Modified: 2020-01-06 20:15 UTC (History)
8 users (show)

See Also:
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


Attachments
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled (1.82 KB, patch)
2018-04-17 18:13 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled (1.89 KB, patch)
2018-06-08 19:38 UTC, Maryse Simard
Details | Diff | Splinter Review
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled (1.91 KB, patch)
2018-11-05 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20598: Unit test (1.08 KB, patch)
2018-11-05 13:33 UTC, Kyle M Hall
Details | Diff | Splinter Review
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled (1.93 KB, patch)
2018-11-16 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20598: Unit test (1.12 KB, patch)
2018-11-16 09:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20598: Accruing fines not closed out by longoverdue.pl if WhenLostForgiveFine is not enabled (1.98 KB, patch)
2018-11-16 10:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20598: Unit test (1.17 KB, patch)
2018-11-16 10:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 20598: (QA follow-up) Tiny fixes (1.89 KB, patch)
2018-11-16 10:50 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Kyle M Hall 2018-04-17 18:07:57 UTC

    
Comment 1 Kyle M Hall 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 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 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 2018-11-16 13:09:15 UTC
Awesome work all!

Pushed to master for 18.11
Comment 19 Lucas Gass 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