Bug 16202 - Rental fees can be generated for fractions of a penny/cent
Summary: Rental fees can be generated for fractions of a penny/cent
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Circulation (show other bugs)
Version: master
Hardware: All All
: P5 - low minor (vote)
Assignee: Colin Campbell
QA Contact: Testopia
URL:
Keywords:
Depends on:
Blocks: 16652
  Show dependency treegraph
 
Reported: 2016-04-05 11:55 UTC by Colin Campbell
Modified: 2017-12-07 22:20 UTC (History)
10 users (show)

See Also:
Change sponsored?: ---
Patch complexity: ---
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Proposed Patch (1.66 KB, patch)
2016-04-05 12:23 UTC, Colin Campbell
Details | Diff | Splinter Review
Bug 16202: GetIssuingCharges should return a collectable amount (1.83 KB, patch)
2017-02-01 14:07 UTC, Mark Tompsett
Details | Diff | Splinter Review
Bug 16202: GetIssuingCharges should return a collectable amount (1.90 KB, patch)
2017-02-24 08:23 UTC, Jonathan Druart
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Colin Campbell 2016-04-05 11:55:05 UTC
GetIssuingCharges includes a calculation applying any discount to the item rentalcharge the resulting charge may well include parts of a penny e.g. 1.752643
in one call of the routine the result was being rounded to two decimal places but in other calls it was not and the resulting amount was saved to the database. This caused odd behaviour down the line as amounts accrued causing extra one penny fines to occur or fines not to revert to 0 after user paid the apparent amount owing. Subroutine should do the rounding before returning the charge
Comment 1 Colin Campbell 2016-04-05 12:23:51 UTC
Created attachment 49907 [details] [review]
Proposed Patch

Bug was observed in renewals where an item charge and discount were applied. In mysql amounts were recorded in accountlines using the decimal places undisplayed in the online. With patch applied only the first 2 decimal places can have values other than 0
Comment 2 Mark Tompsett 2016-04-05 13:20:30 UTC
Comment on attachment 49907 [details] [review]
Proposed Patch

Review of attachment 49907 [details] [review]:
-----------------------------------------------------------------

::: C4/Circulation.pm
@@ +3209,5 @@
>              my $discount = _get_discount_from_rule($discount_rules, $branch, $item_type);
>              $charge = ( $charge * ( 100 - $discount ) ) / 100;
>          }
> +        if ($charge) {
> +            $charge = sprintf '%.2f', $charge; # ensure no fractions of a penny returned

This is not identical to the %.02f from line 984 before. Please add the 0 back.
Comment 3 Mark Tompsett 2016-04-05 13:21:54 UTC
(In reply to Colin Campbell from comment #0)
> GetIssuingCharges ... Subroutine should do the rounding
> before returning the charge

I agree. :)
Comment 4 Colin Campbell 2016-07-22 14:32:58 UTC
(In reply to M. Tompsett from comment #2)
> Comment on attachment 49907 [details] [review] [review]
> Proposed Patch
> 
> Review of attachment 49907 [details] [review] [review]:
> -----------------------------------------------------------------
> 
> ::: C4/Circulation.pm
> @@ +3209,5 @@
> >              my $discount = _get_discount_from_rule($discount_rules, $branch, $item_type);
> >              $charge = ( $charge * ( 100 - $discount ) ) / 100;
> >          }
> > +        if ($charge) {
> > +            $charge = sprintf '%.2f', $charge; # ensure no fractions of a penny returned
> 
> This is not identical to the %.02f from line 984 before. Please add the 0
> back.

The value after the point is the number of decimal places presented the 0 is meaningless in this context see the printf/sprintf documentation where it is not used. ( 0 can be used on its own to suppress the decimal part). Using the precision as documented is less ambiguous
Comment 5 Mark Tompsett 2016-07-23 03:28:00 UTC
(In reply to Colin Campbell from comment #4)
[SNIP]
> The value after the point is the number of decimal places presented the 0 is
> meaningless in this context see the printf/sprintf documentation where it is
> not used. ( 0 can be used on its own to suppress the decimal part). Using
> the precision as documented is less ambiguous

Okay, then should they not be both the same?
Comment 6 Colin Campbell 2017-02-01 13:13:37 UTC
Other uses of sprintf in C4::Circulation no loger have the superfluous 0 - can we fix the bug now
Comment 7 Mark Tompsett 2017-02-01 13:54:28 UTC
(In reply to Colin Campbell from comment #6)
> Other uses of sprintf in C4::Circulation no loger have the superfluous 0 -
> can we fix the bug now

Valid counter point. Okay.
Comment 8 Mark Tompsett 2017-02-01 14:07:49 UTC
Created attachment 59741 [details] [review]
Bug 16202: GetIssuingCharges should return a collectable amount

Charges should not include elements less than a penny/cent
they are not displayed but can be saved to the database
causing "odd" behaviour down the line

Make the routine round the resultant charge to nearest cent,
so consistent values are returned.
Removed the one case where it was rounded post call.
Although the main danger is values generated by the discount
calculation apply the rounding to all returned charges in case
the item charge is defined using the 3rd or 4th decimal
places.

NOTE: prove -v t/db_dependent/Circulation.t triggers the change.
      Though, all the returned amounts are 0.00 only.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>
Comment 9 Jonathan Druart 2017-02-02 09:09:33 UTC
(In reply to M. Tompsett from comment #8)
> NOTE: prove -v t/db_dependent/Circulation.t triggers the change.
>       Though, all the returned amounts are 0.00 only.

Where? I did not find it.

I am not sure this is the correct way to fix it, but definitely the quicker and more efficient.
Comment 10 Jonathan Druart 2017-02-02 09:10:09 UTC
Should not we rephrase the comment without "penny", to make it more international?
Comment 11 Colin Campbell 2017-02-02 11:51:14 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to M. Tompsett from comment #8)
> > NOTE: prove -v t/db_dependent/Circulation.t triggers the change.
> >       Though, all the returned amounts are 0.00 only.
> 
> Where? I did not find it.
> 
> I am not sure this is the correct way to fix it, but definitely the quicker
> and more efficient.

perldoc -q round

sprintf behaves as expected in normal round (i.e) to closest integer
Comment 12 Colin Campbell 2017-02-02 11:55:13 UTC
(In reply to Jonathan Druart from comment #10)
> Should not we rephrase the comment without "penny", to make it more
> international?

I think penny is fairly universal in US, Commonwealth, and Irish English although most are more formally cent. both are in the commit message
Comment 13 Jonathan Druart 2017-02-02 11:56:29 UTC
Yes, but I was more referring to arbitrarily force the number of decimals.
Comment 14 Jonathan Druart 2017-02-02 11:57:29 UTC
(In reply to Colin Campbell from comment #12)
> (In reply to Jonathan Druart from comment #10)
> > Should not we rephrase the comment without "penny", to make it more
> > international?
> 
> I think penny is fairly universal in US, Commonwealth, and Irish English
> although most are more formally cent. both are in the commit message

Ho yes sorry, I thought it was from UK only.
Comment 15 Jonathan Druart 2017-02-14 13:41:17 UTC
(In reply to Jonathan Druart from comment #9)
> (In reply to M. Tompsett from comment #8)
> > NOTE: prove -v t/db_dependent/Circulation.t triggers the change.
> >       Though, all the returned amounts are 0.00 only.
> 
> Where? I did not find it.

Still waiting for an answer.
Comment 16 Mark Tompsett 2017-02-14 14:24:00 UTC
(In reply to Jonathan Druart from comment #15)
> (In reply to Jonathan Druart from comment #9)
> > (In reply to M. Tompsett from comment #8)
> > > NOTE: prove -v t/db_dependent/Circulation.t triggers the change.
> > >       Though, all the returned amounts are 0.00 only.
> > 
> > Where? I did not find it.
> 
> Still waiting for an answer.

The tests only generated 0's. I put in debugging sprintf's to see it.
Comment 17 Jonathan Druart 2017-02-24 08:23:54 UTC
Created attachment 60639 [details] [review]
Bug 16202: GetIssuingCharges should return a collectable amount

Charges should not include elements less than a penny/cent
they are not displayed but can be saved to the database
causing "odd" behaviour down the line

Make the routine round the resultant charge to nearest cent,
so consistent values are returned.
Removed the one case where it was rounded post call.
Although the main danger is values generated by the discount
calculation apply the rounding to all returned charges in case
the item charge is defined using the 3rd or 4th decimal
places.

NOTE: prove -v t/db_dependent/Circulation.t triggers the change.
      Though, all the returned amounts are 0.00 only.

Signed-off-by: Mark Tompsett <mtompset@hotmail.com>

Signed-off-by: Jonathan Druart <jonathan.druart@bugs.koha-community.org>
Comment 18 Kyle M Hall 2017-03-03 19:29:16 UTC
Pushed to master for 17.05, thanks Colin!
Comment 19 Katrin Fischer 2017-03-14 16:34:39 UTC
Considering this a bug fix.

This patch has been pushed to 16.11.x and will be in 16.11.05.
Comment 20 Julian Maurice 2017-03-15 11:36:16 UTC
Pushed to 3.22.x, will be in 3.22.18
Comment 21 Mason James 2017-04-03 14:36:52 UTC
Pushed to 16.05.x, for 16.05.11 release