Bugzilla – Attachment 82975 Details for
Bug 18736
Problems in order calculations (rounding errors)
Home
|
New
|
Browse
|
Search
|
[?]
|
Reports
|
Help
|
New Account
|
Log In
[x]
|
Forgot Password
Login:
[x]
[patch]
Bug 18736: (follow-up) Remove duplicate code and adjust tests
Bug-18736-follow-up-Remove-duplicate-code-and-adju.patch (text/plain), 7.16 KB, created by
Nick Clemens (kidclamp)
on 2018-12-07 13:23:18 UTC
(
hide
)
Description:
Bug 18736: (follow-up) Remove duplicate code and adjust tests
Filename:
MIME Type:
Creator:
Nick Clemens (kidclamp)
Created:
2018-12-07 13:23:18 UTC
Size:
7.16 KB
patch
obsolete
>From 18cc77aac06840330beaa9ad930695090c9ef444 Mon Sep 17 00:00:00 2001 >From: Nick Clemens <nick@bywatersolutions.com> >Date: Thu, 20 Sep 2018 19:34:10 +0000 >Subject: [PATCH] Bug 18736: (follow-up) Remove duplicate code and adjust tests > >--- > C4/Acquisition.pm | 15 ++++++++------- > C4/Budgets.pm | 30 ++++++------------------------ > t/Acquisition.t | 6 +++--- > 3 files changed, 17 insertions(+), 34 deletions(-) > >diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm >index d58d3bd7ef..937386822d 100644 >--- a/C4/Acquisition.pm >+++ b/C4/Acquisition.pm >@@ -94,6 +94,7 @@ BEGIN { > &FillWithDefaultValues > > &get_rounded_price >+ &get_rounding_sql > ); > } > >@@ -1462,8 +1463,8 @@ sub ModReceiveOrder { > $dbh->do(q| > UPDATE aqorders > SET >- tax_value_on_ordering = quantity * | . _get_rounding_sql(q|ecost_tax_excluded|) . q| * tax_rate_on_ordering, >- tax_value_on_receiving = quantity * | . _get_rounding_sql(q|unitprice_tax_excluded|) . q| * tax_rate_on_receiving >+ tax_value_on_ordering = quantity * | . get_rounding_sql(q|ecost_tax_excluded|) . q| * tax_rate_on_ordering, >+ tax_value_on_receiving = quantity * | . get_rounding_sql(q|unitprice_tax_excluded|) . q| * tax_rate_on_receiving > WHERE ordernumber = ? > |, undef, $order->{ordernumber}); > >@@ -1646,8 +1647,8 @@ sub CancelReceipt { > $dbh->do(q| > UPDATE aqorders > SET >- tax_value_on_ordering = quantity * | . _get_rounding_sql(q|ecost_tax_excluded|) . q| * tax_rate_on_ordering, >- tax_value_on_receiving = quantity * | . _get_rounding_sql(q|unitprice_tax_excluded|) . q| * tax_rate_on_receiving >+ tax_value_on_ordering = quantity * | . get_rounding_sql(q|ecost_tax_excluded|) . q| * tax_rate_on_ordering, >+ tax_value_on_receiving = quantity * | . get_rounding_sql(q|unitprice_tax_excluded|) . q| * tax_rate_on_receiving > WHERE ordernumber = ? > |, undef, $parent_ordernumber); > >@@ -1999,15 +2000,15 @@ sub TransferOrder { > return $newordernumber; > } > >-=head3 _get_rounding_sql >+=head3 get_rounding_sql > >- $rounding_sql = _get_rounding_sql("mysql_variable_to_round_string"); >+ $rounding_sql = get_rounding_sql("mysql_variable_to_round_string"); > > returns the correct SQL routine based on OrderPriceRounding system preference. > > =cut > >-sub _get_rounding_sql { >+sub get_rounding_sql { > my ( $round_string ) = @_; > my $rounding_pref = C4::Context->preference('OrderPriceRounding'); > if ( $rounding_pref eq "nearest_cent" ) { return ("CAST($round_string*100 AS UNSIGNED)/100"); } >diff --git a/C4/Budgets.pm b/C4/Budgets.pm >index 870d8d277c..9b932996d7 100644 >--- a/C4/Budgets.pm >+++ b/C4/Budgets.pm >@@ -24,6 +24,7 @@ use Koha::Database; > use Koha::Patrons; > use Koha::Acquisition::Invoice::Adjustments; > use C4::Debug; >+use C4::Acquisition; > use vars qw(@ISA @EXPORT); > > BEGIN { >@@ -211,7 +212,7 @@ sub GetBudgetsPlanCell { > my ( $cell, $period, $budget ) = @_; #FIXME we don't use $period > my ($actual, $sth); > my $dbh = C4::Context->dbh; >- my $roundsql = _get_rounding_sql(qq|ecost_tax_included|); >+ my $roundsql = C4::Acquisition::get_rounding_sql(qq|ecost_tax_included|); > if ( $cell->{'authcat'} eq 'MONTHS' ) { > # get the actual amount > # FIXME we should consider quantity >@@ -336,7 +337,7 @@ sub GetBudgetSpent { > # unitprice_tax_included should always been set here > # we should not need to retrieve ecost_tax_included > my $sth = $dbh->prepare(qq| >- SELECT SUM( | . _get_rounding_sql("COALESCE(unitprice_tax_included, ecost_tax_included)") . qq| * quantity ) AS sum FROM aqorders >+ SELECT SUM( | . C4::Acquisition::get_rounding_sql("COALESCE(unitprice_tax_included, ecost_tax_included)") . qq| * quantity ) AS sum FROM aqorders > WHERE budget_id = ? AND > quantityreceived > 0 AND > datecancellationprinted IS NULL >@@ -367,7 +368,7 @@ sub GetBudgetOrdered { > my ($budget_id) = @_; > my $dbh = C4::Context->dbh; > my $sth = $dbh->prepare(qq| >- SELECT SUM(| . _get_rounding_sql(qq|ecost_tax_included|) . qq| * quantity) AS sum FROM aqorders >+ SELECT SUM(| . C4::Acquisition::get_rounding_sql(qq|ecost_tax_included|) . qq| * quantity) AS sum FROM aqorders > WHERE budget_id = ? AND > quantityreceived = 0 AND > datecancellationprinted IS NULL >@@ -561,14 +562,14 @@ sub GetBudgetHierarchy { > # Get all the budgets totals in as few queries as possible > my $hr_budget_spent = $dbh->selectall_hashref(q| > SELECT aqorders.budget_id, aqbudgets.budget_parent_id, >- SUM( | . _get_rounding_sql(qq|COALESCE(unitprice_tax_included, ecost_tax_included)|) . q| * quantity ) AS budget_spent >+ SUM( | . C4::Acquisition::get_rounding_sql(qq|COALESCE(unitprice_tax_included, ecost_tax_included)|) . q| * quantity ) AS budget_spent > FROM aqorders JOIN aqbudgets USING (budget_id) > WHERE quantityreceived > 0 AND datecancellationprinted IS NULL > GROUP BY budget_id, budget_parent_id > |, 'budget_id'); > my $hr_budget_ordered = $dbh->selectall_hashref(q| > SELECT aqorders.budget_id, aqbudgets.budget_parent_id, >- SUM( | . _get_rounding_sql(qq|ecost_tax_included|) . q| * quantity) AS budget_ordered >+ SUM( | . C4::Acquisition::get_rounding_sql(qq|ecost_tax_included|) . q| * quantity) AS budget_ordered > FROM aqorders JOIN aqbudgets USING (budget_id) > WHERE quantityreceived = 0 AND datecancellationprinted IS NULL > GROUP BY budget_id, budget_parent_id >@@ -1349,25 +1350,6 @@ sub MoveOrders { > return \@report; > } > >-=head1 INTERNAL FUNCTIONS >- >-=cut >- >-=head3 _get_rounding_sql >- >- $rounding_sql = _get_rounding_sql("mysql_variable_to_round_string"); >- >-returns the correct SQL routine based on OrderPriceRounding system preference. >- >-=cut >- >-sub _get_rounding_sql { >- my $to_round = shift; >- my $rounding_pref = C4::Context->preference('OrderPriceRounding'); >- if ($rounding_pref eq 'nearest_cent') { return "CAST($to_round*100 AS UNSIGNED)/100"; } >- else { return "$to_round"; } >-} >- > END { } # module clean-up code here (global destructor) > > 1; >diff --git a/t/Acquisition.t b/t/Acquisition.t >index 1acca9c26c..9c5102c9b1 100644 >--- a/t/Acquisition.t >+++ b/t/Acquisition.t >@@ -23,16 +23,16 @@ use t::lib::Mocks; > > use_ok( 'C4::Acquisition' ); > >-subtest 'Tests for _get_rounding_sql' => sub { >+subtest 'Tests for get_rounding_sql' => sub { > > plan tests => 2; > > my $value = '3.141592'; > > t::lib::Mocks::mock_preference( 'OrderPriceRounding', q{} ); >- my $no_rounding_result = C4::Acquisition::_get_rounding_sql($value); >+ my $no_rounding_result = C4::Acquisition::get_rounding_sql($value); > t::lib::Mocks::mock_preference( 'OrderPriceRounding', q{nearest_cent} ); >- my $rounding_result = C4::Acquisition::_get_rounding_sql($value); >+ my $rounding_result = C4::Acquisition::get_rounding_sql($value); > > ok( $no_rounding_result eq $value, "Value ($value) not to be rounded" ); > ok( $rounding_result =~ /CAST/, "Value ($value) will be rounded" ); >-- >2.11.0
You cannot view the attachment while viewing its details because your browser does not support IFRAMEs.
View the attachment on a separate page
.
View Attachment As Diff
View Attachment As Raw
Actions:
View
|
Diff
|
Splinter Review
Attachments on
bug 18736
:
64061
|
64065
|
70187
|
70188
|
70189
|
70768
|
71062
|
71063
|
71627
|
71628
|
71629
|
71630
|
71631
|
71632
|
74335
|
74336
|
74337
|
74338
|
74339
|
74340
|
74341
|
74342
|
74370
|
74371
|
74374
|
74397
|
74398
|
74422
|
74427
|
74438
|
76979
|
76980
|
76981
|
76982
|
76983
|
76984
|
76985
|
76986
|
76987
|
76988
|
76989
|
76990
|
76991
|
77614
|
77615
|
77616
|
77617
|
77618
|
78101
|
78120
|
78981
|
78982
|
78983
|
78984
|
78985
|
78986
|
78987
|
78988
|
79087
|
79088
|
79089
|
79090
|
79091
|
79092
|
79093
|
79094
|
79098
|
79099
|
79100
|
79101
|
79102
|
79103
|
79104
|
79105
|
79187
|
79223
|
79562
|
79563
|
82967
|
82968
|
82969
|
82970
|
82971
|
82972
|
82973
|
82974
|
82975
|
82976
|
82977
|
83212
|
83213
|
83214
|
83973
|
83974
|
83975
|
83976
|
83977
|
83978
|
83979
|
83980
|
83981
|
83982
|
83983
|
83984
|
83985
|
86646
|
86647
|
86648
|
86649
|
86650
|
86651
|
86652
|
86653
|
86654
|
86655
|
86656
|
86657
|
86658