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 Should we move this to Needs Sign-off for another round of testing or tests first? Created attachment 74335 [details] [review] Bug 18736: Add OrderPriceRounding syspref Created attachment 74336 [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 74337 [details] [review] Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t Created attachment 74338 [details] [review] Bug 18736: (follow-up) - Fix SQL and uncomment db update Created attachment 74339 [details] [review] Bug 18736: (follow-up) fix wrong call to _get_rounding_sql Created attachment 74340 [details] [review] Bug 18736: (follow-up) Use CAST instead of CEIL for appropriate rounding Created attachment 74341 [details] [review] Bug 18736: (follow-up) Correctly assign values in subroutines Created attachment 74342 [details] [review] Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t Created attachment 74370 [details] [review] Bug 18736: Follow to prevent duplicate run noise TEST PLAN --------- update the database twice -- noise apply patch update the database -- no noise Created attachment 74371 [details] [review] Bug 18736: Follow to clean up admin policy preference Two preferences were merged into the same box. This likely was unintended. This reformats them. TEST PLAN --------- Go to Acquisitions tab of the system preference -- in the policy section PurgeSuggestionsOlderThan and OrderPriceRounding are in the same box Apply this patch Refresh the page -- OrderPriceRounding is in its own box, and better formatted. Need test for GetLateOrders with both settings of OrderPriceRounding. Other feedback still may follow. I'll summarize in one final comment afterwards. It would be nice to also trigger the else case for the dbdriver, but that might take some expert mocking, which is low priority as mysql/mariadb are the preferred db drivers. Created attachment 74374 [details] [review] Bug 18736: [DO NOT PUSH] C4/Acquisition print scaffolding. Need to trigger all the print STDERR "C4/Acquisition.pm... cases. Created attachment 74397 [details] [review] Bug 18736: [DO NOT PUSH] Print scaffolding for C4/Budgets.pm The ELSE case is a pain. Please get it to return something rather than undef. Created attachment 74398 [details] [review] Bug 18736: Add missing test cases [BUGGY] The ELSE case is flawed. This needs fixing. The third parameter is budgets, and needs to be valid. It currently is passing, because undef+0= 0, and the expected values are set to zero. They should be non-zero! Created attachment 74422 [details] [review] Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests Created attachment 74427 [details] [review] Bug 18736: (follow-up) Undo changes to GetLateOrders GetLateOrders calculates subtotal based on rrp which is a 2 decimal precision field. If we need more precision here it should be fixed on a separate bug Created attachment 74438 [details] [review] Bug 18736: Follow up tests Test coverage for new C4::Acquisition functions. prove t/Acquisition/_get_rounding_sql.t prove t/Acquisition/get_rounded_price.t All Acquisition.pm and Budgets.pm changes are fully covered. Obsoleting the scaffolding. (In reply to M. Tompsett from comment #52) > prove t/Acquisition/_get_rounding_sql.t > prove t/Acquisition/get_rounded_price.t Create subtests in t/Acquisition.t instead. (In reply to Marc Véron from comment #9) > 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) Canada has something similar, but only when paying with cash. 1.00-1.02 -> 1.00 1.03-1.07 -> 1.05 1.08-1.12 -> 1.10 But I think improving the rounding for 5 cent issues should be another followup bug on this. Need to test with year end process. |