The way we are calculating and/or storing our prices in acquisitions is causing some odd issues. For example: Place an order (no tax, let's not even deal with that right now please) 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
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.
Sorry guys, can't get this set to apply right now: Applying: Bug 18736: (follow-up) Undo changes to GetLateOrders error: sha1 information is lacking or useless (C4/Acquisition.pm). error: could not build fake ancestor
Created attachment 76979 [details] [review] Bug 18736: Add OrderPriceRounding syspref
Created attachment 76980 [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 76981 [details] [review] Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t
Created attachment 76982 [details] [review] Bug 18736: (follow-up) - Fix SQL and uncomment db update
Created attachment 76983 [details] [review] Bug 18736: (follow-up) fix wrong call to _get_rounding_sql
Created attachment 76984 [details] [review] Bug 18736: (follow-up) Use CAST instead of CEIL for appropriate rounding
Created attachment 76985 [details] [review] Bug 18736: (follow-up) Correctly assign values in subroutines
Created attachment 76986 [details] [review] Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t
Created attachment 76987 [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 76988 [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.
Created attachment 76989 [details] [review] Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests
Created attachment 76990 [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 76991 [details] [review] Bug 18736: (follow-up) Unit tests Test coverage for new C4::Acquisition functions. prove t/Acquisition.t
Applied cleanly on master for me - I moved additional tests to subtest in Acquisition.t
There is no signoff! Please test!
Patches do not apply. Please rebase
Created attachment 77614 [details] [review] Bug 18736: Add OrderPriceRounding syspref
Created attachment 77615 [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 77616 [details] [review] Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t prove t/Acquisition.t
Created attachment 77617 [details] [review] Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t
Created attachment 77618 [details] [review] Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests
There are a few problems: - 'use C4::Acquisition' is missing in acqui/spent.pl - acqui/order.pl uses aqorders.ecost, which for me is already rounded in the database ('9.85'), so the amount doesn't change when I change the syspref value (it's like syspref is always "Round") - same for reports/orders_by_fund.pl - In admin/aqbudgets.pl, the amount doesn't change when I change the syspref value (it's like syspref is always "Don't round") - The result of get_rounded_price is often used in multiplication, but this is wrong because get_rounded_price use Koha::Number::Format and this can return strings that are not valid numbers (ex: "1,234.000")
Created attachment 78101 [details] [review] Bug 18736: (follow-up) Fix missing rounding and bad formatting This patch: Adds a missing use Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric returns Ensures too big numbers don't crash round() Uses syspref in 'GetBudgetHierarchy' To test: Follow previous test plan Check values on admin/aqbudgets.pl are affected by syspref Ensure values throughout acquisitions are correctly calculated/displayed (even when greater than 1,000)
(In reply to Julian Maurice from comment #79) > There are a few problems: > - 'use C4::Acquisition' is missing in acqui/spent.pl Fixed! > - acqui/order.pl uses aqorders.ecost, which for me is already rounded in the > database ('9.85'), so the amount doesn't change when I change the syspref > value (it's like syspref is always "Round") > - same for reports/orders_by_fund.pl ecost is a 2 decimal precision number - beyond the scope here > - In admin/aqbudgets.pl, the amount doesn't change when I change the syspref > value (it's like syspref is always "Don't round") Fixed > - The result of get_rounded_price is often used in multiplication, but this > is wrong because get_rounded_price use Koha::Number::Format and this can > return strings that are not valid numbers (ex: "1,234.000") I switched to round(), this should return numeric values
Created attachment 78120 [details] [review] Bug 18736: Unit tests doe GetBudgetHierarchy prove -v t/db_dependent/Budgets.t
Applying: Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell fatal: sha1 information is lacking or useless (C4/Budgets.pm).
Created attachment 78981 [details] [review] Bug 18736: Add OrderPriceRounding syspref
Created attachment 78982 [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 78983 [details] [review] Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t prove t/Acquisition.t
Created attachment 78984 [details] [review] Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t
Created attachment 78985 [details] [review] Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests
Created attachment 78986 [details] [review] Bug 18736: (follow-up) Fix missing rounding and bad formatting This patch: Adds a missing use Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric returns Ensures too big numbers don't crash round() Uses syspref in 'GetBudgetHierarchy' To test: Follow previous test plan Check values on admin/aqbudgets.pl are affected by syspref Ensure values throughout acquisitions are correctly calculated/displayed (even when greater than 1,000)
Created attachment 78987 [details] [review] Bug 18736: Unit tests for GetBudgetHierarchy prove -v t/db_dependent/Budgets.t
Created attachment 78988 [details] [review] Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER
Comment on attachment 78986 [details] [review] Bug 18736: (follow-up) Fix missing rounding and bad formatting Review of attachment 78986 [details] [review]: ----------------------------------------------------------------- ::: Koha/Number/Price.pm @@ +19,4 @@ > > use Modern::Perl; > > +use Number::Format qw( format_price round ); As these subs are used as Number::Format object's methods, I don't think importing them is necessary.
(In reply to Nick Clemens from comment #91) > Created attachment 78988 [details] [review] [review] > Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER Can you tell in the commit message why you are doing that ?
Created attachment 79087 [details] [review] Bug 18736: Add OrderPriceRounding syspref
Created attachment 79088 [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 79089 [details] [review] Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t prove t/Acquisition.t
Created attachment 79090 [details] [review] Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t
Created attachment 79091 [details] [review] Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests
Created attachment 79092 [details] [review] Bug 18736: (follow-up) Fix missing rounding and bad formatting This patch: Adds a missing use Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric returns Ensures too big numbers don't crash round() Uses syspref in 'GetBudgetHierarchy' To test: Follow previous test plan Check values on admin/aqbudgets.pl are affected by syspref Ensure values throughout acquisitions are correctly calculated/displayed (even when greater than 1,000)
Created attachment 79093 [details] [review] Bug 18736: Unit tests for GetBudgetHierarchy prove -v t/db_dependent/Budgets.t
Created attachment 79094 [details] [review] Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER More recent MySql versions require the use of SIGNED or UNSIGNED rather than INTEGER. UNSIGNED will still work in older installs
Thanks Nick! Reviewing this patchset once again, for the last time hopefully :)
Created attachment 79098 [details] [review] Bug 18736: Add OrderPriceRounding syspref Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 79099 [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. Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 79100 [details] [review] Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t prove t/Acquisition.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 79101 [details] [review] Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 79102 [details] [review] Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 79103 [details] [review] Bug 18736: (follow-up) Fix missing rounding and bad formatting This patch: Adds a missing use Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric returns Ensures too big numbers don't crash round() Uses syspref in 'GetBudgetHierarchy' To test: Follow previous test plan Check values on admin/aqbudgets.pl are affected by syspref Ensure values throughout acquisitions are correctly calculated/displayed (even when greater than 1,000) Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 79104 [details] [review] Bug 18736: Unit tests for GetBudgetHierarchy prove -v t/db_dependent/Budgets.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 79105 [details] [review] Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER More recent MySql versions require the use of SIGNED or UNSIGNED rather than INTEGER. UNSIGNED will still work in older installs Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
The bug status was 'Signed off' but it looks like it was never signed off... Well, it is now :-)
QA Comment: _get_rounding_sql is present in both C4::Budgets and C4::Acquisitions, should this not be factored out somewhere to prevent it falling out of sync across the two locations?
Created attachment 79187 [details] [review] Bug 18736: (follow-up) Remove duplicate code
QA: Looking here
ok 1 - use C4::Acquisition; # Subtest: Tests for _get_rounding_sql 1..2 # No tests run! not ok 2 - No tests run for subtest "Tests for _get_rounding_sql" # Failed test 'No tests run for subtest "Tests for _get_rounding_sql"' # at t/Acquisition.t line 40. Undefined subroutine &C4::Acquisition::_get_rounding_sql called at t/Acquisition.t line 33.
Created attachment 79223 [details] [review] Bug 18736: (follow-up) Remove duplicate code and adjust tests
QA: Giving this another try
No blocker: Koha/Number/Price.pm return $self->value if $self->value > Number::Format::MAX_INT/100; What about self->value < -2**53 ? Big credit note..
Would have expected a small change in t/Prices.t too btw
Got across this internal server error while testing: Undefined subroutine &C4::Acquisition::IsMarcStructureInternal called at /usr/share/koha/devclone/C4/Acquisition.pm line 3209. The famous bug to resolve with C4::Biblio:: Seems strictly unrelated however
Created attachment 79562 [details] [review] Bug 18736: (QA follow-up) Cosmetic changes [1] Resolve warnings like: Use of uninitialized value $rounding_pref in string eq at /usr/share/koha/devclone/C4/Acquisition.pm line 2040. [2] Fixing unusual use of whitespace too. [3] Remove list operator from get_rounding_sql return. Only used in scalar context. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 79563 [details] [review] Bug 18736: (QA follow-up) Change to signed, add large int test [1] Add trivial subtest in t/Number/Price.t Checking a negative number and a large number for the MAX_INT change. Note: Confusing to have t/Prices.t too. [2] Change UNSIGNED to SIGNED in get_rounding_sql. Although I did not spot problems with negative prices, we theoretically could while casting. cast(-2 as unsigned) == 18446744073709551614 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Unable to finish this one right now, still in interface testing. Leaving it in blocked, hope to return soon..
(In reply to Marcel de Rooy from comment #123) > Unable to finish this one right now, still in interface testing. Leaving it > in blocked, hope to return soon.. Resuming now
QA Comment: Kind of walking in a minefield here. So very brave try! Think we are doing fine and we have the pref to control it. One area did not have attention yet: C4/Budgets.pm needs similar adjustments: Look at e.g. SUM( COALESCE(unitprice_tax_included, ecost_tax_included) * quantity ) AS budget_spent or SUM(ecost_tax_included * quantity) AS budget_ordered With the pref to nearest cent, the total ordered drops one cent, while it should drop three cent (using your example where 78,83 becomes 78,80). Another note: I still have the Internal Server Error: Undefined subroutine &C4::Acquisition::IsMarcStructureInternal called at /usr/share/koha/devclone/C4/Acquisition.pm line 3211. I added the C4::Biblio prefix here to resolve it. Please check if you need it too. Changing status.
(In reply to Marcel de Rooy from comment #125) > One area did not have attention yet: > > C4/Budgets.pm needs similar adjustments: > Look at e.g. SUM( COALESCE(unitprice_tax_included, ecost_tax_included) * > quantity ) AS budget_spent > or SUM(ecost_tax_included * quantity) AS budget_ordered > > With the pref to nearest cent, the total ordered drops one cent, while it > should drop three cent (using your example where 78,83 becomes 78,80). > Can you point out some line numbers? I don't think I see where you are takling about?
Look in C4::Budgets not Acquisition GetBudgetsPlanCell line 238, 251 ? GetBudgetSpent line 335 GetBudgetOrdered line 366 ? GetBudgetHierarchy line 560, line 567
Whats the status of bug 18736? Carnegie Stout Team
Created attachment 82967 [details] [review] Bug 18736: Add OrderPriceRounding syspref Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 82968 [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. Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 82969 [details] [review] Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t prove t/Acquisition.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 82970 [details] [review] Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 82971 [details] [review] Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 82972 [details] [review] Bug 18736: (follow-up) Fix missing rounding and bad formatting This patch: Adds a missing use Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric returns Ensures too big numbers don't crash round() Uses syspref in 'GetBudgetHierarchy' To test: Follow previous test plan Check values on admin/aqbudgets.pl are affected by syspref Ensure values throughout acquisitions are correctly calculated/displayed (even when greater than 1,000) Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 82973 [details] [review] Bug 18736: Unit tests for GetBudgetHierarchy prove -v t/db_dependent/Budgets.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 82974 [details] [review] Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER More recent MySql versions require the use of SIGNED or UNSIGNED rather than INTEGER. UNSIGNED will still work in older installs Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 82975 [details] [review] Bug 18736: (follow-up) Remove duplicate code and adjust tests
Created attachment 82976 [details] [review] Bug 18736: (QA follow-up) Cosmetic changes [1] Resolve warnings like: Use of uninitialized value $rounding_pref in string eq at /usr/share/koha/devclone/C4/Acquisition.pm line 2040. [2] Fixing unusual use of whitespace too. [3] Remove list operator from get_rounding_sql return. Only used in scalar context. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 82977 [details] [review] Bug 18736: (QA follow-up) Change to signed, add large int test [1] Add trivial subtest in t/Number/Price.t Checking a negative number and a large number for the MAX_INT change. Note: Confusing to have t/Prices.t too. [2] Change UNSIGNED to SIGNED in get_rounding_sql. Although I did not spot problems with negative prices, we theoretically could while casting. cast(-2 as unsigned) == 18446744073709551614 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Rebased on master (In reply to Marcel de Rooy from comment #127) > Look in C4::Budgets not Acquisition > > GetBudgetsPlanCell line 238, 251 ? > GetBudgetSpent line 335 > GetBudgetOrdered line 366 ? > GetBudgetHierarchy line 560, line 567 Sorry Marcel, all of the occurrences I see use get_rounding_sql() >With the pref to nearest cent, the total ordered drops one cent, while it should >drop three cent (using your example where 78,83 becomes 78,80). Can you let me know where you are seeing this? Apologies if I am missing something obvious
Created attachment 83212 [details] [review] Bug 18736: (QA follow-up) Resolve uninitialized warn from Biblio.pm L3334 Running t/db/Budgets.t: Use of uninitialized value in join or string at C4/Biblio.pm line 3334. Test plan: Run again. Verify no warnings left. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 83213 [details] Test basket
Created attachment 83214 [details] Test budget
(In reply to Nick Clemens from comment #140) > >With the pref to nearest cent, the total ordered drops one cent, while it should >drop three cent (using your example where 78,83 becomes 78,80). > > Can you let me know where you are seeing this? Apologies if I am missing > something obvious Still seeing this. Best to illustrate by two images (see atts). The basket shows 78,80 and 15,33 tax. (94,13) The budget shows 92,16 (note that includes a -2, so 94,16) Three cents difference. This comes from rounding at different moments. The db shows: +-----------------------+ | tax_value_on_ordering | +-----------------------+ | 15.326600 | +-----------------------+ +--------------------+ | ecost_tax_excluded | +--------------------+ | 9.854200 | +--------------------+ +--------------------+ | ecost_tax_included | +--------------------+ | 11.770842 | +--------------------+ 8 * 11,77 = 94,16 8 * 9,85[rounded] * 1,1945 (tax percentage ) = 94,13 Difference: For 94,16 tax is calculated on not rounded price (9,8542). But for 94,13 tax is based on rounded price (9,85). How much tax do you want to pay ? ;) Not a real blocker for me btw. But this shows that we are not making consistent calculations in Acqui. If we still want to push, we should be aware of it at least. Will ask Joubu to comment on this finding too. Changing status for now.
Created attachment 83973 [details] [review] Bug 18736: Add OrderPriceRounding syspref Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 83974 [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. Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 83975 [details] [review] Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t prove t/Acquisition.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 83976 [details] [review] Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 83977 [details] [review] Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 83978 [details] [review] Bug 18736: (follow-up) Fix missing rounding and bad formatting This patch: Adds a missing use Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric returns Ensures too big numbers don't crash round() Uses syspref in 'GetBudgetHierarchy' To test: Follow previous test plan Check values on admin/aqbudgets.pl are affected by syspref Ensure values throughout acquisitions are correctly calculated/displayed (even when greater than 1,000) Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 83979 [details] [review] Bug 18736: Unit tests for GetBudgetHierarchy prove -v t/db_dependent/Budgets.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 83980 [details] [review] Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER More recent MySql versions require the use of SIGNED or UNSIGNED rather than INTEGER. UNSIGNED will still work in older installs Signed-off-by: Julian Maurice <julian.maurice@biblibre.com>
Created attachment 83981 [details] [review] Bug 18736: (follow-up) Remove duplicate code and adjust tests
Created attachment 83982 [details] [review] Bug 18736: (QA follow-up) Cosmetic changes [1] Resolve warnings like: Use of uninitialized value $rounding_pref in string eq at /usr/share/koha/devclone/C4/Acquisition.pm line 2040. [2] Fixing unusual use of whitespace too. [3] Remove list operator from get_rounding_sql return. Only used in scalar context. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 83983 [details] [review] Bug 18736: (QA follow-up) Change to signed, add large int test [1] Add trivial subtest in t/Number/Price.t Checking a negative number and a large number for the MAX_INT change. Note: Confusing to have t/Prices.t too. [2] Change UNSIGNED to SIGNED in get_rounding_sql. Although I did not spot problems with negative prices, we theoretically could while casting. cast(-2 as unsigned) == 18446744073709551614 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 83984 [details] [review] Bug 18736: (QA follow-up) Resolve uninitialized warn from Biblio.pm L3334 Running t/db/Budgets.t: Use of uninitialized value in join or string at C4/Biblio.pm line 3334. Test plan: Run again. Verify no warnings left. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 83985 [details] [review] Bug 18736: Calculate tax depending on rounding Marcel's comments pointed out that while I tried to avoid storing rounded values it is required for tax generation. This patch makes that change and adds test coverage and POD for populate_order_with_prices To test: Follow plan on other patches, ensure that orders and totals match on the basket, invoice, and budget pages prove -v t/db_dependent/Acquisition/populate_order_with_prices.t
(In reply to Marcel de Rooy from comment #144) > (In reply to Nick Clemens from comment #140) Ah, okay, I understand now - it seems we cannot leave the tax values untouched when enabling rounding, I added a followup for this. Let me know if the new calculations and tests match your expectations.
Spending a bit of time here now
Small textual problems: + receiving => 1 # boolean representing order stage, should pass only this or ordering + ordering => 1 # boolean representing order stage except fot
Test pass. QA pass. Hmm. Not wanting to be too precise, but we still have some subtle differences in some lines of the last patch. The first line rounds at the unit; with a whole quantity, it will be rounded. The second line is not rounded since you multiply with tax rate. The algorithm of tax = incl - excl is only used in the first line. The third line shows rounding at final stage. + $order->{tax_value_on_ordering} = ( get_rounded_price($order->{ecost_tax_included}) - get_rounded_price($order->{ecost_tax_excluded}) ) * $order->{quantity}; + $order->{tax_value_on_ordering} = $order->{quantity} * get_rounded_price($order->{ecost_tax_excluded}) * $order->{tax_rate_on_ordering}; + my $subtotal = get_rounded_price( $left * $data->{'ecost_tax_included'} ); Please explain why you do what you do.
(In reply to Marcel de Rooy from comment #161) > Test pass. QA pass. > Hmm. Not wanting to be too precise, but we still have some subtle > differences in some lines of the last patch. The first line rounds at the > unit; with a whole quantity, it will be rounded. The second line is not > rounded since you multiply with tax rate. The algorithm of tax = incl - excl > is only used in the first line. > The third line shows rounding at final stage. > + $order->{tax_value_on_ordering} = ( > get_rounded_price($order->{ecost_tax_included}) - > get_rounded_price($order->{ecost_tax_excluded}) ) * $order->{quantity}; > + $order->{tax_value_on_ordering} = $order->{quantity} * > get_rounded_price($order->{ecost_tax_excluded}) * > $order->{tax_rate_on_ordering}; > > + my $subtotal = get_rounded_price( $left * > $data->{'ecost_tax_included'} ); > > Please explain why you do what you do. The first line is when the user entered prices including tax - so we have to calculate excluding tax(In reply to Marcel de Rooy from comment #161) > Test pass. QA pass. > Hmm. Not wanting to be too precise, but we still have some subtle > differences in some lines of the last patch. The first line rounds at the > unit; with a whole quantity, it will be rounded. The second line is not > rounded since you multiply with tax rate. The algorithm of tax = incl - excl > is only used in the first line. > The third line shows rounding at final stage. > + $order->{tax_value_on_ordering} = ( > get_rounded_price($order->{ecost_tax_included}) - > get_rounded_price($order->{ecost_tax_excluded}) ) * $order->{quantity}; > + $order->{tax_value_on_ordering} = $order->{quantity} * > get_rounded_price($order->{ecost_tax_excluded}) * > $order->{tax_rate_on_ordering}; > > + my $subtotal = get_rounded_price( $left * > $data->{'ecost_tax_included'} ); > > Please explain why you do what you do. In the first line we are looking at prices supplied including tax - I assumed we get a rounded number from the vendor - I also assume they charge the tax to a single item then round - so we need to back calculate the tax, not knowing the unrounded end product - then charge this same amount per item In the second case (lines 2 and 3) we are given prices without tax, so we can calculate it on our own - and I assume they would perform the same caulcation - round discounted price, apply quantity and tax The issue is really that when getting a price including tax I assume we don't have full info, so do my best to calculate what I expect. Can you explain how you would expect to calculate that and what numbers you would expect to see? The tests illustrate that I do expect different numbers in the different scenarios, I honestly don't know what the vendors or librarians expect and would accept any quidance
I think I'm happy with Nicks logic here.. Unfortunately, as we store calculated values we're open to having issues like this. EDI for example can often give you both tax inclusive and tax exclusive values and pretty often the rounding math at their end will be different from at our end and so yield different results. I think this patch improves the situation a fair amount and is a good compromise given our existing codebase.. perhaps it's the foundations for more work in a followup bug but I don't think that should hold up this work any longer really.. Did you have any more specific issues Marcel, that Nick could go on for keeping this moving?
(In reply to Martin Renvoize from comment #163) > I think I'm happy with Nicks logic here.. > > Unfortunately, as we store calculated values we're open to having issues > like this. EDI for example can often give you both tax inclusive and tax > exclusive values and pretty often the rounding math at their end will be > different from at our end and so yield different results. > > I think this patch improves the situation a fair amount and is a good > compromise given our existing codebase.. perhaps it's the foundations for > more work in a followup bug but I don't think that should hold up this work > any longer really.. > > Did you have any more specific issues Marcel, that Nick could go on for > keeping this moving? Will have a look this week
Back here again ;)
Created attachment 86646 [details] [review] Bug 18736: Add OrderPriceRounding syspref Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86647 [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. Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86648 [details] [review] Bug 18736: Unit tests for rounding Unit tests to highlight the issue To test: Prove t/db_dependent/Budgets.t prove t/Acquisition.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86649 [details] [review] Bug 18736: (follow-up) Add tests and FIXME for GetbudgetsPlanCell prove t/db_dependent/Budgets.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86650 [details] [review] Bug 18736: (follow-up) Add missing test cases We now fully cover GetBudgetsPlanCell in these tests Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86651 [details] [review] Bug 18736: (follow-up) Fix missing rounding and bad formatting This patch: Adds a missing use Uses 'Koha::Number::Price->round()' instead of 'format()' to ensure numeric returns Ensures too big numbers don't crash round() Uses syspref in 'GetBudgetHierarchy' To test: Follow previous test plan Check values on admin/aqbudgets.pl are affected by syspref Ensure values throughout acquisitions are correctly calculated/displayed (even when greater than 1,000) Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86652 [details] [review] Bug 18736: Unit tests for GetBudgetHierarchy prove -v t/db_dependent/Budgets.t Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86653 [details] [review] Bug 18736: (follow-up) cast as UNSIGNED instead of INTEGER More recent MySql versions require the use of SIGNED or UNSIGNED rather than INTEGER. UNSIGNED will still work in older installs Signed-off-by: Julian Maurice <julian.maurice@biblibre.com> Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86654 [details] [review] Bug 18736: (follow-up) Remove duplicate code and adjust tests Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86655 [details] [review] Bug 18736: (QA follow-up) Cosmetic changes [1] Resolve warnings like: Use of uninitialized value $rounding_pref in string eq at /usr/share/koha/devclone/C4/Acquisition.pm line 2040. [2] Fixing unusual use of whitespace too. [3] Remove list operator from get_rounding_sql return. Only used in scalar context. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86656 [details] [review] Bug 18736: (QA follow-up) Change to signed, add large int test [1] Add trivial subtest in t/Number/Price.t Checking a negative number and a large number for the MAX_INT change. Note: Confusing to have t/Prices.t too. [2] Change UNSIGNED to SIGNED in get_rounding_sql. Although I did not spot problems with negative prices, we theoretically could while casting. cast(-2 as unsigned) == 18446744073709551614 Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86657 [details] [review] Bug 18736: (QA follow-up) Resolve uninitialized warn from Biblio.pm L3334 Running t/db/Budgets.t: Use of uninitialized value in join or string at C4/Biblio.pm line 3334. Test plan: Run again. Verify no warnings left. Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Created attachment 86658 [details] [review] Bug 18736: Calculate tax depending on rounding Marcel's comments pointed out that while I tried to avoid storing rounded values it is required for tax generation. This patch makes that change and adds test coverage and POD for populate_order_with_prices To test: Follow plan on other patches, ensure that orders and totals match on the basket, invoice, and budget pages prove -v t/db_dependent/Acquisition/populate_order_with_prices.t Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
QA Comment: I re-entered the example with 8 * 16,99 and 42% discount, 19.45% tax. With the following results when patches applied: OrderPriceRounding=Dont Basket page 78,83 + 15,33 tax. == 94,16 Fund page 94,17 => No match between basket and fund ! OrderPriceRounding=Nearest cent Basket page: 78,80 + 15,33 tax == 94,13 Fund page: 94,16 => No match either between basket and fund ! And without these patches: Basket page 78,83 + 15,33 tax. == 94,16 Fund page: 94,17 => No match either. Conclusion: Please see also my previous comments. I am not repeating them here. With the default setting of Don't round we make no difference in this example; so no behavior change.. The Nearest cent choice does not resolve the discrepancy between basket and fund, as noted before. As already mentioned, I do not object to push this patch set. But this simple exercise already shows that we did not solve the rounding issues. Passed QA
Awesome work all! Pushed to master for 19.05
(In reply to Marcel de Rooy from comment #179) > QA Comment: > I re-entered the example with 8 * 16,99 and 42% discount, 19.45% tax. > > With the following results when patches applied: > OrderPriceRounding=Dont > Basket page 78,83 + 15,33 tax. == 94,16 > Fund page 94,17 > => No match between basket and fund ! > > OrderPriceRounding=Nearest cent > Basket page: 78,80 + 15,33 tax == 94,13 > Fund page: 94,16 > => No match either between basket and fund ! > > And without these patches: > Basket page 78,83 + 15,33 tax. == 94,16 > Fund page: 94,17 > => No match either. > > Conclusion: Please see also my previous comments. I am not repeating them > here. With the default setting of Don't round we make no difference in this > example; so no behavior change.. The Nearest cent choice does not resolve > the discrepancy between basket and fund, as noted before. > > As already mentioned, I do not object to push this patch set. But this > simple exercise already shows that we did not solve the rounding issues. > > Passed QA You are right :-) I talked to others though and general consensus was this is a big change and should tackle issues raised on follow-ups. I think this does some good things and paves the way for more work forward so pushed. Thanks Marcel
Pushed to 18.11.x for 18.11.04!
not backporting enhancement to 18.05.x