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
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 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.
(In reply to Colin Campbell from comment #0) > GetIssuingCharges ... Subroutine should do the rounding > before returning the charge I agree. :)
(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
(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?
Other uses of sprintf in C4::Circulation no loger have the superfluous 0 - can we fix the bug now
(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.
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>
(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.
Should not we rephrase the comment without "penny", to make it more international?
(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
(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
Yes, but I was more referring to arbitrarily force the number of decimals.
(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.
(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.
(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.
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>
Pushed to master for 17.05, thanks Colin!
Considering this a bug fix. This patch has been pushed to 16.11.x and will be in 16.11.05.
Pushed to 3.22.x, will be in 3.22.18
Pushed to 16.05.x, for 16.05.11 release