Attachment 54532 Details for Bug 17138 – Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need

[patch] Bug 17138 - UpdateFine() modyfies existing fine records even when there is no need

Bug-17138---UpdateFine-modyfies-existing-fine-reco.patch (text/plain), 2.41 KB, created by Jacek Ablewicz on 2016-08-17 10:04:55 UTC

(hide)

Description:

Filename:

MIME Type:

Creator: Jacek Ablewicz

Created: 2016-08-17 10:04:55 UTC

Size: 2.41 KB

patch

obsolete

>From 484f33e93f2dde35a3637eec7d31e1e5fc3536ec Mon Sep 17 00:00:00 2001
>From: Jacek Ablewicz <abl@biblos.pk.edu.pl>
>Date: Wed, 17 Aug 2016 11:52:48 +0200
>Subject: [PATCH] Bug 17138 - UpdateFine() modyfies existing fine records even
> when there is no need
>
>In C4/Overdues.pm:
>
>  574:     # we're updating an existing fine.  Only modify if amount changed
>
>  577:     if ( $data->{'amount'} != $amount ) {
>
>for some numbers (e.g. 5.60) the comparison in line 577 fails
>($data->{'amount'} is decimal(28,6) i.e. a string, and $amount may be
>a floating point number with no exact binary representation).
>E.g. when the fine amounts are multiple of 0.20, it fails for the 33.3%
>of the amounts (on average).
>
>Depending on the fine charging settings, this may trigger unnecessary
>database writes (and a lot of them). If 'Fine Charging Interval' is
>1 day, this has very little practical consequences (misc/cronjobs/fines.pl
>script is usually run once per day, and all fines need an increase
>anyway). But if the charging interval is longer, e.g. 7 days, in a given
>day amount changes only for the 1/7 of the fines (again, on average),
>and over 60% of the database writes would be redundant.
>
>To reproduce:
>
>1) run misc/cronjobs/fines.pl
>2) add
>
>   warn "AMOUNT ".$data->{'amount'}." vs $amount";
>
>below line 577 in C4/Overdues.pm
>3) run misc/cronjobs/fines.pl again
>4) you'll see some warnings like 'AMOUNT 5.600000 vs 5.6'
>
>or: dump accountlines table after step 1), run the script 2nd time, dump
>the table again and compare the contents - some of the records would have
>a different timestamp.
>
>To test:
>
>1) apply patch
>2) redo steps 1) - 4) above
>3) no more warnings in step 4
>4) 2nd run of misc/cronjobs/fines.pl should be noticeably faster
>---
> C4/Overdues.pm |    2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
>diff --git a/C4/Overdues.pm b/C4/Overdues.pm
>index 3f0c32d..9665d7d 100644
>--- a/C4/Overdues.pm
>+++ b/C4/Overdues.pm
>@@ -574,7 +574,7 @@ sub UpdateFine {
>         # we're updating an existing fine.  Only modify if amount changed
>         # Note that in the current implementation, you cannot pay against an accruing fine
>         # (i.e. , of accounttype 'FU').  Doing so will break accrual.
>-        if ( $data->{'amount'} != $amount ) {
>+        if ( $data->{'amount'} ne sprintf('%.6f', $amount) ) {
>             my $accountline = Koha::Account::Lines->find( $data->{accountlines_id} );
>             my $diff = $amount - $data->{'amount'};
> 
>-- 
>1.7.10.4

Actions: View | Diff | Splinter Review

Attachments on bug 17138: 54532 | 58559 | 59755 | 59756