Argument "" isn't numeric in multiplication (*) at /usr/share/koha/lib/C4/Overdues.pm when fines.pl is run. The lines vary a little between our 20.11.17 and master. It's in CalcFine: In 20.11.7 it's here: 265: $amount = $charge_periods * $issuing_rule->{fine}; That would make it this line in master: 279: $amount = $charge_periods * $issuing_rule->{fine}; I think it might be linked to cases where fine is empty but an interval is defined? In the database rule_name = fine shows empty and NULL values. I think with issuingrules we used to have a numeric data type so that might be a side effect of the migration. This is quite noisy.
I think this is a possible fix: diff --git a/C4/Overdues.pm b/C4/Overdues.pm index 5db9c220416..a31d2b4f7ba 100644 --- a/C4/Overdues.pm +++ b/C4/Overdues.pm @@ -274,7 +274,7 @@ sub CalcFine { # biblio-level recall $amount = $charge_periods * $issuing_rule->{recall_overdue_fine}; } - if ( scalar @recalls == 0 ) { + if ( scalar @recalls == 0 && $issuing_rule->{fine}) { # no recall, use normal fine amount $amount = $charge_periods * $issuing_rule->{fine}; } Are you expecting a suspension (days) but no fine ($) with this rule?
Suspensions have a different rule for their interval, so it's not about this here. I think maybe we need to treat empty/null as 0, because with your fix the fine I believe the fine might not be recalculated. Use case would be: - There is an older fine > 0 - Library changes circulation rules to fine = 0 - fines.pl is rerun and reduces existing fines to 0 This works now (we have used it) and it should work after fixing the warn as well.
After discussing with Joubu we agreed the fix should not change things, there is also: my $amount = 0;
Created attachment 135225 [details] [review] Bug 30788: Fix warning in Overdues.pm when fine is empty in circ rules Argument "" isn't numeric in multiplication (*) at /usr/share/koha/lib/C4/Overdues.pm To test: * Create a issuing rule where fine is empty, but a fine interval is set. Fine amount: empty To do this, remove the 0 displayed in the input field before saving. Fine charging interval: 5 or any other numeric value When to charge: Start of interval Fine grace period: 0 * Check out an item with the due date yester (use specify due date) * Run misc/cronjobs/fines.pl -v * You should see above warn in the output * Apply patch * Rerun fines.pl, there should be no warn. * Run: t/db_dependent/Circulation/CalcFine.t t/db_dependent/Fines.t t/db_dependent/Circulation.t t/db_dependent/Overdues.t
Created attachment 135226 [details] [review] Bug 30788: Unit test Run: prove t/db_dependent/Circulation/CalcFine.t
Created attachment 135227 [details] [review] Bug 30788: Fix warning in Overdues.pm when fine is empty in circ rules Argument "" isn't numeric in multiplication (*) at /usr/share/koha/lib/C4/Overdues.pm To test: * Create a issuing rule where fine is empty, but a fine interval is set. Fine amount: empty To do this, remove the 0 displayed in the input field before saving. Fine charging interval: 5 or any other numeric value When to charge: Start of interval Fine grace period: 0 * Check out an item with the due date yester (use specify due date) * Run misc/cronjobs/fines.pl -v * You should see above warn in the output * Apply patch * Rerun fines.pl, there should be no warn. * Run: t/db_dependent/Circulation/CalcFine.t t/db_dependent/Fines.t t/db_dependent/Circulation.t t/db_dependent/Overdues.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Created attachment 135228 [details] [review] Bug 30788: Unit test Run: prove t/db_dependent/Circulation/CalcFine.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com>
Works great and is a nice cleanup. Unit tests are present and passing and QA script is happy (now that I fixed the first commit author to match the second commit author) Going for a straight to QA on this. Passed
For me, this test was failing: prove t/db_dependent/Circulation/CalcFine.t t/db_dependent/Circulation/CalcFine.t .. 1/5 # Looks like you planned 2 tests but ran 1. # Failed test 'Test with fine amount empty' # at t/db_dependent/Circulation/CalcFine.t line 142. t/db_dependent/Circulation/CalcFine.t .. 5/5 # Looks like you failed 1 test of 5. t/db_dependent/Circulation/CalcFine.t .. Dubious, test returned 1 (wstat 256, 0x100) Failed 1/5 subtests Test Summary Report ------------------- t/db_dependent/Circulation/CalcFine.t (Wstat: 256 Tests: 5 Failed: 1) Failed test: 2 Non-zero exit status: 1 Files=1, Tests=5, 4 wallclock secs ( 0.03 usr 0.01 sys + 2.78 cusr 0.26 csys = 3.08 CPU) Result: FAIL
Created attachment 135229 [details] [review] Bug 30788: Fix number of tests
Forgot to commit a last minute change - just the number of tests needed to be fixed.
Hi David, thx for testing! I think we overlapped, could you have a go with the follow-up patch? It should fix exactly that problem.
Just retested - everything passes now.
Created attachment 135554 [details] [review] Bug 30788: Unit test Run: prove t/db_dependent/Circulation/CalcFine.t Signed-off-by: Martin Renvoize <martin.renvoize@ptfs-europe.com> Signed-off-by: Tomas Cohen Arazi <tomascohen@theke.io>
Pushed to master for 22.11. Nice work everyone, thanks!
Pushed to 22.05.x for 22.05.01
this patch depends on recalls, released in 22.05 (bz19532) won't backport.