Summary: | Argument "" isn't numeric in multiplication (*) at /usr/share/koha/lib/C4/Overdues.pm | ||
---|---|---|---|
Product: | Koha | Reporter: | Katrin Fischer <katrin.fischer> |
Component: | Command-line Utilities | Assignee: | Katrin Fischer <katrin.fischer> |
Status: | CLOSED FIXED | QA Contact: | Martin Renvoize (ashimema) <martin.renvoize> |
Severity: | minor | ||
Priority: | P5 - low | CC: | arthur.suzuki, david, jonathan.druart, lucas, martin.renvoize, robin, tomascohen |
Version: | Main | ||
Hardware: | All | ||
OS: | All | ||
GIT URL: | Change sponsored?: | --- | |
Patch complexity: | --- | Documentation contact: | |
Documentation submission: | Text to go in the release notes: | ||
Version(s) released in: |
22.11.00, 22.05.01
|
Circulation function: | |
Bug Depends on: | 19532 | ||
Bug Blocks: | 25790 | ||
Attachments: |
Bug 30788: Fix warning in Overdues.pm when fine is empty in circ rules
Bug 30788: Unit test Bug 30788: Fix warning in Overdues.pm when fine is empty in circ rules Bug 30788: Unit test Bug 30788: Fix number of tests Bug 30788: Unit test |
Description
Katrin Fischer
2022-05-17 11:31:28 UTC
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. |