Bugzilla – Full Text Bug Listing

- Home
- | New
- | Browse
- | Search
- | [?]
- | Reports
- | Help
- | New Account
- | Log In
- | Forgot Password

Description
Nick Clemens
2017-06-06 14:36:49 UTC
Created attachment 64061 [details] [review] Bug 18736 - Problems in order calculations (rounding errors) Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t The issue we need to decide here is whether we store the extra digits and correct calculations based on these, or store the price with 2 digits precision We have a similar issue with fines: Bug 15741 - Incorrect rounding in total fines calculations (see comment #12) Bug 17140 - Incorrect rounding in total fines calculations, part 2 Created attachment 64065 [details] [review] Bug 18736: Correctly handle rounding The values displayed should be the sum of the rounded values, not the reverse. In ModReceiveOrder, i am wondering if: tax_value_on_ordering = quantity * format(ecost_tax_excluded) * format(tax_rate_on_ordering) is correct or if it must be: tax_value_on_ordering = quantity * format(ecost_tax_excluded * tax_rate_on_ordering) Note: the second form is used in 16.06.00.042 (bug 13321) Here is my try, for discussion. We must understand and fix everything in one go, we need people that have been involved in bug 13321 and related to confirm the correct behaviours. See also bug 18639. I think we shouldn't round the tax rates - they should be applied with as much precision as possible you have one ROUD where you want ROUND spent.pl will need an update too Hi, what we need in our library is to have the more precise price for order and the exact invoice price. So we sould surely store precise result rather than rounded price. But, people will not understand if we display rounded prices and if the total is calculated on prices with 4 decimals... Perhaps, we souhld display more precise price (4 decimals ?)... I'm not really sure to help in this discussion because I feel that I miss some technical points. Sonia Hi Sonia, So do your invoices from vendors include portions of a cent? It seems most libraries/vendors round to nearest penny in the final calculations so not rounding causes errors. If you have the opposite situation perhaps we need a system preference to control exact vs. rounded prices? -Nick (In reply to Koha Team Lyon 3 from comment #7) > Hi, > what we need in our library is to have the more precise price for order and > the exact invoice price. So we sould surely store precise result rather than > rounded price. > But, people will not understand if we display rounded prices and if the > total is calculated on prices with 4 decimals... > Perhaps, we souhld display more precise price (4 decimals ?)... > > I'm not really sure to help in this discussion because I feel that I miss > some technical points. > > Sonia (In reply to Nick Clemens from comment #8) > Hi Sonia, > > So do your invoices from vendors include portions of a cent? > > It seems most libraries/vendors round to nearest penny in the final > calculations so not rounding causes errors. If you have the opposite > situation perhaps we need a system preference to control exact vs. rounded > prices? > > -Nick > > (In reply to Koha Team Lyon 3 from comment #7) > > Hi, > > what we need in our library is to have the more precise price for order and > > the exact invoice price. So we sould surely store precise result rather than > > rounded price. > > But, people will not understand if we display rounded prices and if the > > total is calculated on prices with 4 decimals... > > Perhaps, we souhld display more precise price (4 decimals ?)... > > > > I'm not really sure to help in this discussion because I feel that I miss > > some technical points. > > > > Sonia To make things a little bit more complicated: Switzerland and Finland round to 5 cents. 1,000–1,024 → 1,00 1,025–1,074 → 1,05 1,075–1,099 → 1,10 See: https://de.wikipedia.org/wiki/Rundung ("Rappenrundung": 5 Rappen (cents) is the smallest coin in Switzerland) I think prices should be stored in 4 decimal digits precision, and so calculations made. Then for display purposes we could provide a way to specify how rounding should be done (number of digits). I am not sure if this is only a display issue. Thinking of the calculations we do like adding up the values of all orders to determine the available money. If you store exactly, display rounded, that will never 'add up correctly' and will make people wonder. I wonder how other ILS handle this? (In reply to Nick Clemens from comment #8) > Hi Sonia, > > So do your invoices from vendors include portions of a cent? > Hi Nick, Our invoice amount looks like 1453.78 for example. But our colleagues often say us that there's few penny difference between Koha and the invoice amount. It seems really tricky to have the exact calculation so I suppose that rounding only at the last step of calculation (addition of prices of an invoice) could reduce rounding error ? > It seems most libraries/vendors round to nearest penny in the final > calculations so not rounding causes errors. If you have the opposite > situation perhaps we need a system preference to control exact vs. rounded > prices? Yes, as Finland and Switserland are other rounding pratices, a syspref could be a solution... Sonia Independently of the discussion above: Typo in C4/Budgets.pm line 224 SELECT SUM(ROUD(ecost_tax_included, 2)) FROM aqorders Created attachment 70187 [details] [review] Bug 18736 - Add OrerPriceRounding syspref Created attachment 70188 [details] [review] Bug 18736 - Use rounding syspref to determine correct prices in calculations To test: Place an order (no tax just for simplicity) listprice/rrp = 16.99 discount = 42% quantity = 8 estimated calculated at 9.85 but order total is 78.83, but 8 times 9.85 = 78.80 Apply patches, set OrderPriceRounding syspref to 'Nearest cent' Not order total is now as expected View ordered.pl and confirm values are correct Complete order, view invoice and confirm values View spent.pl and confirm values Go through acquisitions module and confirm prices throughout are correct. Created attachment 70189 [details] [review] Bug 18736 - Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t This is an attempt to repair the calculations here by providing a system preference. I only add a 'nearest_cent' option, but I tried to leave things open for further options to be added later. Please test thouroughly and let me know any feedback. Comment on attachment 70187 [details] [review] Bug 18736 - Add OrerPriceRounding syspref Review of attachment 70187 [details] [review]: ----------------------------------------------------------------- ::: installer/data/mysql/atomicupdate/bug18736_add_rounding_syspref.perl @@ +1,3 @@ > +$DBversion = 'XXX'; # will be replaced by the RM > +if( CheckVersion( $DBversion ) ) { > + # $dbh->do( "INSERT INTO systempreferences (variable,value,explanation,options,type) VALUES ('OrderPriceRounding',NULL,'Local preference for rounding orders before calculations to ensure correct calculations','|nearest_cent','Choice')" ); This line is commented out. Why? Comment on attachment 70188 [details] [review] Bug 18736 - Use rounding syspref to determine correct prices in calculations Review of attachment 70188 [details] [review]: ----------------------------------------------------------------- ::: C4/Acquisition.pm @@ +2010,5 @@ > + > +sub _get_rounding_sql { > + my $round_string = @_; > + my $rounding_pref = C4::Context->preference('OrderPriceRounding'); > + if ( $rounding_pref eq "nearest_cent" ) { return ("ROUND($round_string,2)"); } ROUND() is not ANSI SQL. CEIL($round_string*100)/100 would be ANSI SQL 2003, I believe. ::: C4/Budgets.pm @@ +1291,5 @@ > + > +sub _get_rounding_sql { > + my $to_round = shift; > + my $rounding_pref = C4::Context->preference('OrderPriceRounding'); > + if ($rounding_pref eq 'nearest_cent') { return "ROUND($to_round,2)"; } Same problem here. Not ANSI SQL. Created attachment 70768 [details] [review] Bug 18736: Followup - Fix SQL and uncomment db update Created attachment 71062 [details] [review] Bug 18736: (follow-up) fix wrong call to _get_rounding_sql Created attachment 71063 [details] [review] Bug 18736 (follow-up) Use CAST instead of CEIL for appropriate rounding The only thing lacking is test coverage of C4::Budgets::GetBudgetsPlanCell, where some changes were made. This would be a Failed QA, but I know Nick is creating an improve test coverage bug elsewhere to link here, right? :) Rebasing. Created attachment 71627 [details] [review] Bug 18736 - Add OrerPriceRounding syspref Created attachment 71628 [details] [review] Bug 18736 - Use rounding syspref to determine correct prices in calculations To test: Place an order (no tax just for simplicity) listprice/rrp = 16.99 discount = 42% quantity = 8 estimated calculated at 9.85 but order total is 78.83, but 8 times 9.85 = 78.80 Apply patches, set OrderPriceRounding syspref to 'Nearest cent' Not order total is now as expected View ordered.pl and confirm values are correct Complete order, view invoice and confirm values View spent.pl and confirm values Go through acquisitions module and confirm prices throughout are correct. Created attachment 71629 [details] [review] Bug 18736 - Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t Created attachment 71630 [details] [review] Bug 18736: Followup - Fix SQL and uncomment db update Created attachment 71631 [details] [review] Bug 18736: (follow-up) fix wrong call to _get_rounding_sql Created attachment 71632 [details] [review] Bug 18736 (follow-up) Use CAST instead of CEIL for appropriate rounding The only thing lacking is test coverage of C4::Budgets::GetBudgetsPlanCell, where some changes were made. As a result, this is Failed QA. (In reply to M. Tompsett from comment #31) > The only thing lacking is test coverage of C4::Budgets::GetBudgetsPlanCell, > where some changes were made. As a result, this is Failed QA. Does it mean you tested everything and it works as you expect? (In reply to Jonathan Druart from comment #32) > Does it mean you tested everything and it works as you expect? No, that was from an eyeball stance only. I just tested what was there, and my Total tax exc. (USD) column is 8?! Regardless of system preference setting. There is a mistake somewhere. Hello, if someone is OK to work on this before the next hackfest in March, I'm OK to test it during the hackfest. Sonia |