Bug 30788

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 UtilitiesAssignee: Katrin Fischer <katrin.fischer>
Status: CLOSED FIXED QA Contact: Martin Renvoize <martin.renvoize>
Severity: minor    
Priority: P5 - low CC: arthur.suzuki, david, jonathan.druart, lucas, martin.renvoize, robin, tomascohen
Version: Main   
Hardware: All   
OS: All   
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
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
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.
Comment 1 Jonathan Druart 2022-05-17 12:19:52 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?
Comment 2 Katrin Fischer 2022-05-17 12:48:10 UTC
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.
Comment 3 Katrin Fischer 2022-05-17 13:04:29 UTC
After discussing with Joubu we agreed the fix should not change things, there is also:

my $amount = 0;
Comment 4 Katrin Fischer 2022-05-20 08:07:02 UTC
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
Comment 5 Katrin Fischer 2022-05-20 08:07:06 UTC
Created attachment 135226 [details] [review]
Bug 30788: Unit test

Run:
  prove t/db_dependent/Circulation/CalcFine.t
Comment 6 Martin Renvoize 2022-05-20 08:50:48 UTC
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>
Comment 7 Martin Renvoize 2022-05-20 08:50:52 UTC
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>
Comment 8 Martin Renvoize 2022-05-20 08:51:51 UTC
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
Comment 9 David Nind 2022-05-20 09:04:04 UTC
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
Comment 10 Katrin Fischer 2022-05-20 09:08:52 UTC
Created attachment 135229 [details] [review]
Bug 30788: Fix number of tests
Comment 11 Katrin Fischer 2022-05-20 09:11:58 UTC
Forgot to commit a last minute change - just the number of tests needed to be fixed.
Comment 12 Katrin Fischer 2022-05-20 09:28:37 UTC
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.
Comment 13 David Nind 2022-05-20 09:32:50 UTC
Just retested - everything passes now.
Comment 14 Tomás Cohen Arazi 2022-06-01 19:08:14 UTC
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>
Comment 15 Tomás Cohen Arazi 2022-06-01 19:10:08 UTC
Pushed to master for 22.11.

Nice work everyone, thanks!
Comment 16 Lucas Gass 2022-06-10 14:55:41 UTC
Pushed to 22.05.x for 22.05.01
Comment 17 Arthur Suzuki 2022-06-21 11:57:31 UTC
this patch depends on recalls, released in 22.05 (bz19532) won't backport.