Bug 27927 - longoverdue cronjob renews items before marking lost when both RenewAccruingItemWhenPaid and WhenLostForgiveFine are enabled
Summary: longoverdue cronjob renews items before marking lost when both RenewAccruingI...
Status: RESOLVED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Fines and fees (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Martin Renvoize
QA Contact: Nick Clemens
URL:
Keywords:
Depends on: 25508
Blocks: 24300
  Show dependency treegraph
 
Reported: 2021-03-11 12:51 UTC by Nick Clemens
Modified: 2021-04-25 20:39 UTC (History)
6 users (show)

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


Attachments
Bug 27927: Prevent renewal on lost forgive (1.85 KB, patch)
2021-03-24 17:16 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27927: Prevent renewal on lost forgive (1.06 KB, patch)
2021-03-25 11:30 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27927: Add Unit Tests (3.96 KB, patch)
2021-04-06 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27927: Prevent renewal on lost forgive (1.32 KB, patch)
2021-04-06 10:34 UTC, Martin Renvoize
Details | Diff | Splinter Review
Bug 27927: Add Unit Tests (4.03 KB, patch)
2021-04-06 16:14 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 27927: Prevent renewal on lost forgive (1.38 KB, patch)
2021-04-06 16:15 UTC, Andrew Fuerste-Henry
Details | Diff | Splinter Review
Bug 27927: Add Unit Tests (4.09 KB, patch)
2021-04-15 18:44 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 27927: Prevent renewal on lost forgive (1.44 KB, patch)
2021-04-15 18:44 UTC, Nick Clemens
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2021-03-11 12:51:55 UTC
longoverdue.pl calls 'LostItem'

C4::Circulation::LostItem calls _FixOverduesOnReturn

C4::Circulation::_FixOverduesOnReturn create a Koha::Account::Line for the amount of the accruing fine and calls apply passing the existing fine

Koha::Account::Line calls renew_item when the fee is paid

The item is then renewed, a notice (potentially) sent, and then the item marked lost and charged

The sending of a renewal notice is quite confusing to patrons
Comment 1 Martin Renvoize 2021-03-24 17:16:49 UTC
Created attachment 118766 [details] [review]
Bug 27927: Prevent renewal on lost forgive

The `RenewAccruingItemWhenPaid` preference should only renew upon
payment (and writeoff?) and not upon automatic forgival triggered by
`WhenLostForgiveFine`.
Comment 2 Martin Renvoize 2021-03-24 17:18:05 UTC
This will need some tests writing, but I wanted to get the patch up here and start proceeding with testing.

I've currently only prevented the renewal for 'FORGIVEN' credit types, I wonder if we aught to exclude any other credit types...?
Comment 3 Martin Renvoize 2021-03-25 11:30:06 UTC
Created attachment 118781 [details] [review]
Bug 27927: Prevent renewal on lost forgive

The `RenewAccruingItemWhenPaid` preference should only renew upon
payment (and writeoff?) and not upon automatic forgival triggered by
`WhenLostForgiveFine`.
Comment 4 Martin Renvoize 2021-04-06 10:34:16 UTC
Created attachment 119193 [details] [review]
Bug 27927: Add Unit Tests

Add a unit test to check that 'FORGIVEN' credits do not prompt an
auto-renewal with `RenewAccruingItemWhenPaid` enabled.

Test plan
1/ Apply the test patch
2/ Run t/db_dependent/Koha/Account/Line.t and confirm it fails
3/ Apply subsequent patch
4/ Re-run the test and confirm it passes
Comment 5 Martin Renvoize 2021-04-06 10:34:19 UTC
Created attachment 119194 [details] [review]
Bug 27927: Prevent renewal on lost forgive

The `RenewAccruingItemWhenPaid` preference should only renew upon
payment (and writeoff) and not upon automatic forgival triggered by
`WhenLostForgiveFine`.

Test plan
1/ Enable `WhenLostForgiveFine` and `RenewAccruingItemWhenPaid`
2/ Checkout an item and backdate it's due date
3/ Run longeroverdue.pl to mark the item as lost and create the fine
4/ Confirm that the item is not renewed as part of the process
5/ Signoff
Comment 6 Andrew Fuerste-Henry 2021-04-06 13:00:34 UTC
The behavior is all good, but I get a test failure:
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.
t/db_dependent/Koha/Account/Line.t .. 9/14 
#   Failed test 'checkout() tests'
#   at t/db_dependent/Koha/Account/Line.t line 643.
Can't call method "checkout" on an undefined value at t/db_dependent/Koha/Account/Line.t line 635.
# 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.13 cusr  0.59 csys =  3.75 CPU)
Result: FAIL
Comment 7 Martin Renvoize 2021-04-06 15:47:43 UTC
I'm really struggling to get this test to fail.. how are you running it?
Comment 8 Andrew Fuerste-Henry 2021-04-06 16:04:50 UTC
Going back through this, the test passes if FinesMode is set to Don't Calculate, but fails if FinesMode is set to Calculate and Charge.
Comment 9 Andrew Fuerste-Henry 2021-04-06 16:14:56 UTC
Created attachment 119235 [details] [review]
Bug 27927: Add Unit Tests

Add a unit test to check that 'FORGIVEN' credits do not prompt an
auto-renewal with `RenewAccruingItemWhenPaid` enabled.

Test plan
1/ Apply the test patch
2/ Run t/db_dependent/Koha/Account/Line.t and confirm it fails
3/ Apply subsequent patch
4/ Re-run the test and confirm it passes

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 10 Andrew Fuerste-Henry 2021-04-06 16:15:01 UTC
Created attachment 119236 [details] [review]
Bug 27927: Prevent renewal on lost forgive

The `RenewAccruingItemWhenPaid` preference should only renew upon
payment (and writeoff) and not upon automatic forgival triggered by
`WhenLostForgiveFine`.

Test plan
1/ Enable `WhenLostForgiveFine` and `RenewAccruingItemWhenPaid`
2/ Checkout an item and backdate it's due date
3/ Run longeroverdue.pl to mark the item as lost and create the fine
4/ Confirm that the item is not renewed as part of the process
5/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>
Comment 11 Andrew Fuerste-Henry 2021-04-06 16:15:35 UTC
Signed off because I get that same test failure with FinesMode on without these patches. Filing a new bug for that.
Comment 12 Andrew Fuerste-Henry 2021-04-06 16:18:05 UTC
Filed bug 28097 for that test
Comment 13 Martin Renvoize 2021-04-06 16:24:33 UTC
Aha, thanks Andrew.. I hadn't thought about changing FinesMode having been changed in the background... certainly a bug, the tests should lock the preference to ensure they pass regardless.
Comment 14 Nick Clemens 2021-04-15 18:44:16 UTC
Created attachment 119676 [details] [review]
Bug 27927: Add Unit Tests

Add a unit test to check that 'FORGIVEN' credits do not prompt an
auto-renewal with `RenewAccruingItemWhenPaid` enabled.

Test plan
1/ Apply the test patch
2/ Run t/db_dependent/Koha/Account/Line.t and confirm it fails
3/ Apply subsequent patch
4/ Re-run the test and confirm it passes

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 15 Nick Clemens 2021-04-15 18:44:20 UTC
Created attachment 119677 [details] [review]
Bug 27927: Prevent renewal on lost forgive

The `RenewAccruingItemWhenPaid` preference should only renew upon
payment (and writeoff) and not upon automatic forgival triggered by
`WhenLostForgiveFine`.

Test plan
1/ Enable `WhenLostForgiveFine` and `RenewAccruingItemWhenPaid`
2/ Checkout an item and backdate it's due date
3/ Run longeroverdue.pl to mark the item as lost and create the fine
4/ Confirm that the item is not renewed as part of the process
5/ Signoff

Signed-off-by: Andrew Fuerste-Henry <andrew@bywatersolutions.com>

Signed-off-by: Nick Clemens <nick@bywatersolutions.com>
Comment 16 Jonathan Druart 2021-04-16 10:29:45 UTC
Pushed to master for 21.05, thanks to everybody involved!
Comment 17 Fridolin Somers 2021-04-22 13:48:47 UTC
Pushed to 20.11.x for 20.11.05
Comment 18 Andrew Fuerste-Henry 2021-04-25 15:51:17 UTC
Pushed to 20.05.x for 20.05.11
Comment 19 Victor Grousset/tuxayo 2021-04-25 20:39:03 UTC
Missing dependencies for 19.11.x, it shouldn't be affected, no backport.