From 7e89301ab2eca57aa13256f9ed4c8351834476e3 Mon Sep 17 00:00:00 2001 From: Jonathan Druart Date: Mon, 17 Nov 2014 12:06:09 +0100 Subject: [PATCH] Bug 13321: Fix the prices calculation method Well, we have finally arrived here \o/ The method where the prices are calculated uses the equations listed on the wiki page (http://wiki.koha-community.org/wiki/GST_Rewrite_RFC). The ecost is calculated from the rrp (using the discount and the tax rate). That's why we removed the ability to edit this value. That's why we remove the ability to edit the ecost on ordering in a previous commit (bug 12840). The total is now calculated in the scripts. That's why this patch removes lines in the test file. In C4::Acquisition::populate_order_with_prices, the calculation on receiving must depend on the 'invoiceincgst' supplier parameter, and not listincgst (which is used on ordering). It also removes the rounding errors, now we store "exact" values in DB (10^-6). The values will be displayed using the Price TT plugin it will round the values for us. Signed-off-by: Laurence Rault Signed-off-by: Francois Charbonnier Signed-off-by: Sonia Bouis Signed-off-by: Sonia Bouis Signed-off-by: Nick Clemens Signed-off-by: Kyle M Hall --- C4/Acquisition.pm | 70 ++++--- acqui/basket.pl | 6 +- acqui/basketgroup.pl | 4 - acqui/finishreceive.pl | 20 +- acqui/invoice.pl | 11 +- acqui/parcel.pl | 4 +- .../prog/en/modules/acqui/orderreceive.tt | 9 +- t/Prices.t | 179 ++---------------- 8 files changed, 75 insertions(+), 228 deletions(-) diff --git a/C4/Acquisition.pm b/C4/Acquisition.pm index c40fb33c83..d04e673ae4 100644 --- a/C4/Acquisition.pm +++ b/C4/Acquisition.pm @@ -2823,9 +2823,6 @@ sub GetBiblioCountByBasketno { return $sth->fetchrow; } -# This is *not* the good way to calcul prices -# But it's how it works at the moment into Koha -# This will be fixed later. # Note this subroutine should be moved to Koha::Acquisition::Order # Will do when a DBIC decision will be taken. sub populate_order_with_prices { @@ -2842,48 +2839,59 @@ sub populate_order_with_prices { my $discount = $order->{discount}; $discount /= 100 if $discount > 1; - $order->{rrp} = Koha::Number::Price->new( $order->{rrp} )->round; - $order->{ecost} = Koha::Number::Price->new( $order->{ecost} )->round; if ($ordering) { if ( $bookseller->{listincgst} ) { + # The user entered the rrp tax included $order->{rrp_tax_included} = $order->{rrp}; - $order->{rrp_tax_excluded} = Koha::Number::Price->new( - $order->{rrp_tax_included} / ( 1 + $order->{tax_rate} ) )->round; - $order->{ecost_tax_included} = $order->{ecost}; - $order->{ecost_tax_excluded} = Koha::Number::Price->new( - $order->{ecost} / ( 1 + $order->{tax_rate} ) )->round; - $order->{tax_value} = Koha::Number::Price->new( - ( $order->{ecost_tax_included} - $order->{ecost_tax_excluded} ) * - $order->{quantity} )->round; + + # rrp tax excluded = rrp tax included / ( 1 + tax rate ) + $order->{rrp_tax_excluded} = $order->{rrp_tax_included} / ( 1 + $order->{tax_rate} ); + + # ecost tax excluded = rrp tax excluded * ( 1 - discount ) + $order->{ecost_tax_excluded} = $order->{rrp_tax_excluded} * ( 1 - $discount ); + + # ecost tax included = rrp tax included ( 1 - discount ) + $order->{ecost_tax_included} = $order->{rrp_tax_included} * ( 1 - $discount ); } else { + # The user entered the rrp tax excluded $order->{rrp_tax_excluded} = $order->{rrp}; - $order->{rrp_tax_included} = Koha::Number::Price->new( - $order->{rrp} * ( 1 + $order->{tax_rate} ) )->round; - $order->{ecost_tax_excluded} = $order->{ecost}; - $order->{ecost_tax_included} = Koha::Number::Price->new( - $order->{ecost} * ( 1 + $order->{tax_rate} ) )->round; - $order->{tax_value} = Koha::Number::Price->new( - ( $order->{ecost_tax_included} - $order->{ecost_tax_excluded} ) * - $order->{quantity} )->round; + + # rrp tax included = rrp tax excluded * ( 1 - tax rate ) + $order->{rrp_tax_included} = $order->{rrp_tax_excluded} * ( 1 + $order->{tax_rate} ); + + # ecost tax excluded = rrp tax excluded * ( 1 - discount ) + $order->{ecost_tax_excluded} = $order->{rrp_tax_excluded} * ( 1 - $discount ); + + # ecost tax included = rrp tax excluded * ( 1 - tax rate ) * ( 1 - discount ) + $order->{ecost_tax_included} = + $order->{rrp_tax_excluded} * + ( 1 + $order->{tax_rate} ) * + ( 1 - $discount ); } + + # tax value = quantity * ecost tax excluded * tax rate + $order->{tax_value} = $order->{quantity} * $order->{ecost_tax_excluded} * $order->{tax_rate}; } if ($receiving) { - if ( $bookseller->{listincgst} ) { - $order->{unitprice_tax_included} = Koha::Number::Price->new( $order->{unitprice} )->round; - $order->{unitprice_tax_excluded} = Koha::Number::Price->new( - $order->{unitprice_tax_included} / ( 1 + $order->{tax_rate} ) )->round; + if ( $bookseller->{invoiceincgst} ) { + # The user entered the unit price tax included + $order->{unitprice_tax_included} = $order->{unitprice}; + + # unit price tax excluded = unit price tax included / ( 1 + tax rate ) + $order->{unitprice_tax_excluded} = $order->{unitprice_tax_included} / ( 1 + $order->{tax_rate} ); } else { - $order->{unitprice_tax_excluded} = Koha::Number::Price->new( $order->{unitprice} )->round; - $order->{unitprice_tax_included} = Koha::Number::Price->new( - $order->{unitprice_tax_excluded} * ( 1 + $order->{tax_rate} ) )->round; + # The user entered the unit price tax excluded + $order->{unitprice_tax_excluded} = $order->{unitprice}; + + # unit price tax included = unit price tax included * ( 1 + tax rate ) + $order->{unitprice_tax_included} = $order->{unitprice_tax_excluded} * ( 1 + $order->{tax_rate} ); } - $order->{tax_value} = Koha::Number::Price->new( - ( $order->{unitprice_tax_included} - $order->{unitprice_tax_excluded} ) - * $order->{quantityreceived} )->round; + # tax value = quantity * unit price tax excluded * tax rate + $order->{tax_value} = $order->{quantity} * $order->{unitprice_tax_excluded} * $order->{tax_rate}; } return $order; diff --git a/acqui/basket.pl b/acqui/basket.pl index e116fd3e5e..a7a01fd655 100755 --- a/acqui/basket.pl +++ b/acqui/basket.pl @@ -329,9 +329,6 @@ if ( $op eq 'list' ) { $template->param( uncertainprices => 1 ); } - $line->{total_tax_excluded} = Koha::Number::Price->new( $line->{ecost_tax_excluded} * $line->{quantity} )->format; - $line->{total_tax_included} = Koha::Number::Price->new( $line->{ecost_tax_included} * $line->{quantity} )->format; - push @books_loop, $line; $foot{$$line{tax_rate}}{tax_rate} = $$line{tax_rate}; @@ -440,6 +437,9 @@ sub get_order_infos { $line{basketno} = $basketno; $line{budget_name} = $budget->{budget_name}; + $line{total_tax_included} = $line{ecost_tax_included} * $line{quantity}; + $line{total_tax_excluded} = $line{ecost_tax_excluded} * $line{quantity}; + if ( $line{uncertainprice} ) { $line{rrp_tax_excluded} .= ' (Uncertain)'; } diff --git a/acqui/basketgroup.pl b/acqui/basketgroup.pl index 232547c767..a4994d3afb 100755 --- a/acqui/basketgroup.pl +++ b/acqui/basketgroup.pl @@ -80,10 +80,6 @@ sub BasketTotal { } else { $total = $total + ( $order->{ecost_tax_excluded} * $order->{quantity} ); } - if ($bookseller->{invoiceincgst} && ! $bookseller->{listincgst} && ( $bookseller->{tax_rate} // C4::Context->preference("gist") )) { - my $gst = $bookseller->{tax_rate} // C4::Context->preference("gist"); - $total = $total * ( $gst / 100 +1); - } } $total .= " " . ($bookseller->{invoiceprice} // 0); return $total; diff --git a/acqui/finishreceive.pl b/acqui/finishreceive.pl index 53a7412a41..35fa7504e0 100755 --- a/acqui/finishreceive.pl +++ b/acqui/finishreceive.pl @@ -54,8 +54,6 @@ my $invoice = GetInvoice($invoiceid); my $invoiceno = $invoice->{invoicenumber}; my $booksellerid = $input->param('booksellerid'); my $cnt = 0; -my $ecost = $input->param('ecost'); -my $rrp = $input->param('rrp'); my $bookfund = $input->param("bookfund"); my $order = GetOrder($ordernumber); my $new_ordernumber = $ordernumber; @@ -83,25 +81,9 @@ if ($quantityrec > $origquantityrec ) { } $order->{order_internalnote} = $input->param("order_internalnote"); - $order->{rrp} = $rrp; - $order->{ecost} = $ecost; $order->{unitprice} = $unitprice; - # FIXME is it still useful? my $bookseller = Koha::Acquisition::Bookseller->fetch({ id => $booksellerid }); - if ( $bookseller->{listincgst} ) { - if ( not $bookseller->{invoiceincgst} ) { - $order->{rrp} = $order->{rrp} * ( 1 + $order->{tax_rate} ); - $order->{ecost} = $order->{ecost} * ( 1 + $order->{tax_rate} ); - $order->{unitprice} = $order->{unitprice} * ( 1 + $order->{tax_rate} ); - } - } else { - if ( $bookseller->{invoiceincgst} ) { - $order->{rrp} = $order->{rrp} / ( 1 + $order->{tax_rate} ); - $order->{ecost} = $order->{ecost} / ( 1 + $order->{tax_rate} ); - $order->{unitprice} = $order->{unitprice} / ( 1 + $order->{tax_rate} ); - } - } $order = C4::Acquisition::populate_order_with_prices( { @@ -171,7 +153,7 @@ ModItem( booksellerid => $booksellerid, dateaccessioned => $datereceived, price => $unitprice, - replacementprice => $rrp, + replacementprice => $order->{rrp}, replacementpricedate => $datereceived, }, $biblionumber, diff --git a/acqui/invoice.pl b/acqui/invoice.pl index 0672866de4..438407c667 100755 --- a/acqui/invoice.pl +++ b/acqui/invoice.pl @@ -143,7 +143,6 @@ foreach my $order (@$orders) { push @foot_loop, map {$_} values %foot; -my $format = "%.2f"; my $budgets = GetBudgets(); my @budgets_loop; my $shipmentcost_budgetid = $details->{shipmentcost_budgetid}; @@ -170,11 +169,11 @@ $template->param( orders_loop => \@orders_loop, foot_loop => \@foot_loop, total_quantity => $total_quantity, - total_tax_excluded => sprintf( $format, $total_tax_excluded ), - total_tax_included => sprintf( $format, $total_tax_included ), - total_tax_value => sprintf( $format, $total_tax_value ), - total_tax_excluded_shipment => sprintf( $format, $total_tax_excluded + $details->{shipmentcost}), - total_tax_included_shipment => sprintf( $format, $total_tax_included + $details->{shipmentcost}), + total_tax_excluded => $total_tax_excluded, + total_tax_included => $total_tax_included, + total_tax_value => $total_tax_value, + total_tax_excluded_shipment => $total_tax_excluded + $details->{shipmentcost}, + total_tax_included_shipment => $total_tax_included + $details->{shipmentcost}, invoiceincgst => $bookseller->{invoiceincgst}, currency => Koha::Acquisition::Currencies->get_active, budgets_loop => \@budgets_loop, diff --git a/acqui/parcel.pl b/acqui/parcel.pl index ffe41d85fc..79c56040b3 100755 --- a/acqui/parcel.pl +++ b/acqui/parcel.pl @@ -149,8 +149,8 @@ for my $order ( @orders ) { $line{budget} = GetBudgetByOrderNumber( $line{ordernumber} ); $foot{$line{tax_rate}}{tax_rate} = $line{tax_rate}; $foot{$line{tax_rate}}{tax_value} += $line{tax_value}; - $total_tax_excluded += Koha::Number::Price->new( $line{ecost_tax_excluded} * $line{quantity} )->format; - $total_tax_included += Koha::Number::Price->new( $line{ecost_tax_included} * $line{quantity} )->format; + $total_tax_excluded += $line{unitprice_tax_excluded} * $line{quantity}; + $total_tax_included += $line{unitprice_tax_included} * $line{quantity}; my $suggestion = GetSuggestionInfoFromBiblionumber($line{biblionumber}); $line{suggestionid} = $suggestion->{suggestionid}; diff --git a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt index 477094f365..afbcfb952e 100644 --- a/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt +++ b/koha-tmpl/intranet-tmpl/prog/en/modules/acqui/orderreceive.tt @@ -328,13 +328,14 @@ [% END %][%# IF (AcqCreateItemReceiving) %] -
    • -
    • + +
  • [% rrp | $Price %]
    • +
  • [% ecost | $Price %]
  • [% IF ( unitprice ) %] - + [% ELSE %] - + [% END %]
    • [% IF order_vendornote %] diff --git a/t/Prices.t b/t/Prices.t index 61513d41e6..c5c744f7bb 100644 --- a/t/Prices.t +++ b/t/Prices.t @@ -47,7 +47,7 @@ my $today; for my $currency_format ( qw( US FR ) ) { t::lib::Mocks::mock_preference( 'CurrencyFormat', $currency_format ); subtest 'Configuration 1: 0 0' => sub { - plan tests => 12; + plan tests => 8; $bookseller_module->mock( 'fetch', sub { @@ -79,7 +79,6 @@ for my $currency_format ( qw( US FR ) ) { } ); - # Note that this configuration is correct \o/ compare( { got => $order_0_0->{rrp_tax_included}, @@ -120,22 +119,6 @@ for my $currency_format ( qw( US FR ) ) { field => 'tax_value' } ); - compare( - { - got => $order_0_0->{total_tax_included}, - expected => 154.98, - conf => '0 0', - field => 'total_tax_included' - } - ); - compare( - { - got => $order_0_0->{total_tax_excluded}, - expected => 147.60, - conf => '0 0', - field => 'total_tax_excluded' - } - ); $order_0_0 = C4::Acquisition::populate_order_with_prices( { @@ -145,7 +128,6 @@ for my $currency_format ( qw( US FR ) ) { } ); - # Note that this configuration is correct \o/ compare( { got => $order_0_0->{unitprice_tax_included}, @@ -170,26 +152,10 @@ for my $currency_format ( qw( US FR ) ) { field => 'tax_value' } ); - compare( - { - got => $order_0_0->{total_tax_included}, - expected => 154.98, - conf => '0 0', - field => 'total_tax_included' - } - ); - compare( - { - got => $order_0_0->{total_tax_excluded}, - expected => 147.60, - conf => '0 0', - field => 'total_tax_excluded' - } - ); }; subtest 'Configuration 1: 1 1' => sub { - plan tests => 12; + plan tests => 8; $bookseller_module->mock( 'fetch', sub { @@ -221,8 +187,6 @@ for my $currency_format ( qw( US FR ) ) { } ); - # Note that this configuration is *not* correct - # tax_value should be 7.03 instead of 7.02 compare( { got => $order_1_1->{rrp_tax_included}, @@ -258,27 +222,11 @@ for my $currency_format ( qw( US FR ) ) { compare( { got => $order_1_1->{tax_value}, - expected => 7.02, + expected => 7.03, conf => '1 1', field => 'tax_value' } ); - compare( - { - got => $order_1_1->{total_tax_included}, - expected => 147.60, - conf => '1 1', - field => 'total_tax_included' - } - ); - compare( - { - got => $order_1_1->{total_tax_excluded}, - expected => 140.58, - conf => '1 1', - field => 'total_tax_excluded' - } - ); $order_1_1 = C4::Acquisition::populate_order_with_prices( { @@ -287,8 +235,7 @@ for my $currency_format ( qw( US FR ) ) { receiving => 1, } ); - # Note that this configuration is *not* correct! - # tax_value should be 7.03 + compare( { got => $order_1_1->{unitprice_tax_included}, @@ -308,31 +255,15 @@ for my $currency_format ( qw( US FR ) ) { compare( { got => $order_1_1->{tax_value}, - expected => 7.02, + expected => 7.03, conf => '1 1', field => 'tax_value' } ); - compare( - { - got => $order_1_1->{total_tax_included}, - expected => 147.60, - conf => '1 1', - field => 'total_tax_included' - } - ); - compare( - { - got => $order_1_1->{total_tax_excluded}, - expected => 140.58, - conf => '1 1', - field => 'total_tax_excluded' - } - ); }; subtest 'Configuration 1: 1 0' => sub { - plan tests => 12; + plan tests => 8; $bookseller_module->mock( 'fetch', sub { @@ -345,11 +276,11 @@ for my $currency_format ( qw( US FR ) ) { biblionumber => $biblionumber_1_0, quantity => 2, listprice => 82.000000, - unitprice => 73.804500, + unitprice => 70.290000, quantityreceived => 2, basketno => $basketno_1_1, invoiceid => $invoiceid_1_1, - rrp => 82.01, + rrp => 82.00, ecost => 73.80, tax_rate => 0.0500, discount => 10.0000, @@ -364,14 +295,10 @@ for my $currency_format ( qw( US FR ) ) { } ); - # Note that this configuration is *not* correct! - # rrp_tax_included should be 82 (what we inserted!) - # tax_value should be 7.03 instead of 7.02 - compare( { got => $order_1_0->{rrp_tax_included}, - expected => 82.01, + expected => 82, conf => '1 0', field => 'rrp_tax_included' } @@ -403,27 +330,11 @@ for my $currency_format ( qw( US FR ) ) { compare( { got => $order_1_0->{tax_value}, - expected => 7.02, + expected => 7.03, conf => '1 0', field => 'tax_value' } ); - compare( - { - got => $order_1_0->{total_tax_included}, - expected => 147.60, - conf => '1 0', - field => 'total_tax_included' - } - ); - compare( - { - got => $order_1_0->{total_tax_excluded}, - expected => 140.58, - conf => '1 0', - field => 'total_tax_excluded' - } - ); $order_1_0 = C4::Acquisition::populate_order_with_prices( { @@ -432,8 +343,7 @@ for my $currency_format ( qw( US FR ) ) { receiving => 1, } ); - # Note that this configuration is *not* correct! - # gstvalue should be 7.03 + compare( { got => $order_1_0->{unitprice_tax_included}, @@ -453,31 +363,15 @@ for my $currency_format ( qw( US FR ) ) { compare( { got => $order_1_0->{tax_value}, - expected => 7.02, + expected => 7.03, conf => '1 0', field => 'tax_value' } ); - compare( - { - got => $order_1_0->{total_tax_included}, - expected => 147.60, - conf => '1 0', - field => 'total_tax_included' - } - ); - compare( - { - got => $order_1_0->{total_tax_excluded}, - expected => 140.58, - conf => '1 0', - field => 'total_tax_excluded' - } - ); }; subtest 'Configuration 1: 0 1' => sub { - plan tests => 12; + plan tests => 8; $bookseller_module->mock( 'fetch', sub { @@ -490,7 +384,7 @@ for my $currency_format ( qw( US FR ) ) { biblionumber => $biblionumber_0_1, quantity => 2, listprice => 82.000000, - unitprice => 73.800000, + unitprice => 77.490000, quantityreceived => 2, basketno => $basketno_1_1, invoiceid => $invoiceid_1_1, @@ -509,12 +403,11 @@ for my $currency_format ( qw( US FR ) ) { } ); - # Note that this configuration is correct \o/ compare( { got => $order_0_1->{rrp_tax_included}, expected => 86.10, - conf => '1 0', + conf => '0 1', field => 'rrp_tax_included' } ); @@ -522,7 +415,7 @@ for my $currency_format ( qw( US FR ) ) { { got => $order_0_1->{rrp_tax_excluded}, expected => 82.00, - conf => '1 0', + conf => '0 1', field => 'rrp_tax_excluded' } ); @@ -530,7 +423,7 @@ for my $currency_format ( qw( US FR ) ) { { got => $order_0_1->{ecost_tax_included}, expected => 77.49, - conf => '1 0', + conf => '0 1', field => 'ecost_tax_included' } ); @@ -538,7 +431,7 @@ for my $currency_format ( qw( US FR ) ) { { got => $order_0_1->{ecost_tax_excluded}, expected => 73.80, - conf => '1 0', + conf => '0 1', field => 'ecost_tax_excluded' } ); @@ -546,26 +439,10 @@ for my $currency_format ( qw( US FR ) ) { { got => $order_0_1->{tax_value}, expected => 7.38, - conf => '1 0', + conf => '0 1', field => 'tax_value' } ); - compare( - { - got => $order_0_1->{total_tax_included}, - expected => 154.98, - conf => '1 0', - field => 'total_tax_included' - } - ); - compare( - { - got => $order_0_1->{total_tax_excluded}, - expected => 147.60, - conf => '1 0', - field => 'total_tax_excluded' - } - ); $order_0_1 = C4::Acquisition::populate_order_with_prices( { @@ -574,7 +451,7 @@ for my $currency_format ( qw( US FR ) ) { receiving => 1, } ); - # Note that this configuration is correct + compare( { got => $order_0_1->{unitprice_tax_included}, @@ -599,22 +476,6 @@ for my $currency_format ( qw( US FR ) ) { field => 'tax_value' } ); - compare( - { - got => $order_0_1->{total_tax_included}, - expected => 154.98, - conf => '0 1', - field => 'total_tax_included' - } - ); - compare( - { - got => $order_0_1->{total_tax_excluded}, - expected => 147.60, - conf => '0 1', - field => 'total_tax_excluded' - } - ); }; } -- 2.39.5