[Bug 16202] New: Rental fees can be generated for fractions of a penny/cent
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Bug ID: 16202 Summary: Rental fees can be generated for fractions of a penny/cent Change sponsored?: --- Product: Koha Version: master Hardware: All OS: All Status: NEW Severity: enhancement Priority: P5 - low Component: Circulation Assignee: koha-bugs@lists.koha-community.org Reporter: colin.campbell@ptfs-europe.com QA Contact: testopia@bugs.koha-community.org CC: gmcharlt@gmail.com, kyle.m.hall@gmail.com 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Colin Campbell <colin.campbell@ptfs-europe.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|NEW |Needs Signoff Assignee|koha-bugs@lists.koha-commun |colin.campbell@ptfs-europe. |ity.org |com --- Comment #1 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Created attachment 49907 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49907&action=edit 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 -- You are receiving this mail because: You are the assignee for the bug. You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #2 from M. Tompsett <mtompset@hotmail.com> --- Comment on attachment 49907 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=49907 Proposed Patch Review of attachment 49907: --> (https://bugs.koha-community.org/bugzilla3/page.cgi?id=splinter.html&bug=16202&attachment=49907) ----------------------------------------------------------------- ::: 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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtompset@hotmail.com --- Comment #3 from M. Tompsett <mtompset@hotmail.com> --- (In reply to Colin Campbell from comment #0)
GetIssuingCharges ... Subroutine should do the rounding before returning the charge
I agree. :) -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Sally Healey <sally.healey@cheshiresharedservices.gov.uk> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |sally.healey@cheshireshared | |services.gov.uk -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Marc Véron <veron@veron.ch> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |veron@veron.ch -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #4 from Colin Campbell <colin.campbell@ptfs-europe.com> --- (In reply to M. Tompsett from comment #2)
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.
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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #5 from M. Tompsett <mtompset@hotmail.com> --- (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? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Jiri Kozlovsky <mail@jkozlovsky.cz> changed: What |Removed |Added ---------------------------------------------------------------------------- Blocks| |16652 Referenced Bugs: https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16652 [Bug 16652] Omnibus: RestfulAPI supporting services for Vufind and for xnciptoolkit drivers -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #6 from Colin Campbell <colin.campbell@ptfs-europe.com> --- Other uses of sprintf in C4::Circulation no loger have the superfluous 0 - can we fix the bug now -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #7 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Needs Signoff |Signed Off -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 M. Tompsett <mtompset@hotmail.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #49907|0 |1 is obsolete| | --- Comment #8 from M. Tompsett <mtompset@hotmail.com> --- Created attachment 59741 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=59741&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |jonathan.druart@bugs.koha-c | |ommunity.org --- Comment #9 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #10 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Should not we rephrase the comment without "penny", to make it more international? -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #11 from Colin Campbell <colin.campbell@ptfs-europe.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #12 from Colin Campbell <colin.campbell@ptfs-europe.com> --- (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 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #13 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Yes, but I was more referring to arbitrarily force the number of decimals. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #14 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #15 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 --- Comment #16 from M. Tompsett <mtompset@hotmail.com> --- (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. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Signed Off |Passed QA -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Jonathan Druart <jonathan.druart@bugs.koha-community.org> changed: What |Removed |Added ---------------------------------------------------------------------------- Attachment #59741|0 |1 is obsolete| | --- Comment #17 from Jonathan Druart <jonathan.druart@bugs.koha-community.org> --- Created attachment 60639 --> https://bugs.koha-community.org/bugzilla3/attachment.cgi?id=60639&action=edit 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> -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Kyle M Hall <kyle@bywatersolutions.com> changed: What |Removed |Added ---------------------------------------------------------------------------- Status|Passed QA |Pushed to Master CC| |kyle@bywatersolutions.com --- Comment #18 from Kyle M Hall <kyle@bywatersolutions.com> --- Pushed to master for 17.05, thanks Colin! -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Katrin Fischer <katrin.fischer@bsz-bw.de> changed: What |Removed |Added ---------------------------------------------------------------------------- Severity|enhancement |minor Status|Pushed to Master |Pushed to Stable CC| |katrin.fischer@bsz-bw.de --- Comment #19 from Katrin Fischer <katrin.fischer@bsz-bw.de> --- Considering this a bug fix. This patch has been pushed to 16.11.x and will be in 16.11.05. -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Julian Maurice <julian.maurice@biblibre.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |julian.maurice@biblibre.com --- Comment #20 from Julian Maurice <julian.maurice@biblibre.com> --- Pushed to 3.22.x, will be in 3.22.18 -- You are receiving this mail because: You are watching all bug changes.
https://bugs.koha-community.org/bugzilla3/show_bug.cgi?id=16202 Mason James <mtj@kohaaloha.com> changed: What |Removed |Added ---------------------------------------------------------------------------- CC| |mtj@kohaaloha.com --- Comment #21 from Mason James <mtj@kohaaloha.com> --- Pushed to 16.05.x, for 16.05.11 release -- You are receiving this mail because: You are watching all bug changes.
participants (1)
-
bugzilla-daemon＠bugs.koha-community.org