Bug 19296 - Tax is being subtracted from orders when vendor price does not include gst and ordering from a file
Summary: Tax is being subtracted from orders when vendor price does not include gst an...
Status: CLOSED FIXED
Alias: None
Product: Koha
Classification: Unclassified
Component: Acquisitions (show other bugs)
Version: master
Hardware: All All
: P5 - low major (vote)
Assignee: Nick Clemens
QA Contact: Marcel de Rooy
URL:
Keywords:
Depends on: 15503
Blocks:
  Show dependency treegraph
 
Reported: 2017-09-12 16:16 UTC by Nick Clemens
Modified: 2019-06-27 09:24 UTC (History)
6 users (show)

See Also:
Change sponsored?: ---
Patch complexity: Small patch
Documentation contact:
Documentation submission:
Text to go in the release notes:
Version(s) released in:


Attachments
Bug 19296 - Allow all tax processing to happen in C4::Acquisition::populate_order_with_prices (4.12 KB, patch)
2017-09-13 11:21 UTC, Nick Clemens
Details | Diff | Splinter Review
Bug 19296 - Allow all tax processing to happen in C4::Acquisition::populate_order_with_prices (4.17 KB, patch)
2017-10-08 13:29 UTC, Katrin Fischer
Details | Diff | Splinter Review
Bug 19296 - Allow all tax processing to happen in C4::Acquisition::populate_order_with_prices (4.27 KB, patch)
2017-10-13 07:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review
Bug 19296: (QA Follow-up) Correct comment in populate_orders (1.16 KB, patch)
2017-10-13 07:05 UTC, Marcel de Rooy
Details | Diff | Splinter Review

Note You need to log in before you can comment on or make changes to this bug.
Description Nick Clemens 2017-09-12 16:16:54 UTC
In the case of vendor prices don't include gst we are removing the gst from the order price, passing it to populate_order_with_prices and calculating tax on the price with tax removed!

Example:
Order with price of $10 and tax rate of 10%
We calculate ecost/rrp at 9.09
Then calculate tax on 9.09
We should be calculating tax on $10

In addorderiso2707.pl:
270                         if ( $bookseller->listincgst ) {
271                             if ( $c_discount ) {
272                                 $orderinfo{ecost} = $price;
273                                 $orderinfo{rrp}   = $orderinfo{ecost} / ( 1 - $c );
274                             } else {
275                                 $orderinfo{ecost} = $price * ( 1 - $c );
276                                 $orderinfo{rrp}   = $price;
277                             }
278                         } else {
279                             if ( $c_discount ) {
280                                 $orderinfo{ecost} = $price / ( 1 + $orderinfo{tax_rate} );
281                                 $orderinfo{rrp}   = $orderinfo{ecost} / ( 1 - $c );
282                             } else {
283                                 $orderinfo{rrp}   = $price / ( 1 + $orderinfo{tax_rate} );
284                                 $orderinfo{ecost} = $orderinfo{rrp} * ( 1 - $c );
285                             }
286                         }
...
296 
297                     %orderinfo = %{
298                         C4::Acquisition::populate_order_with_prices(
299                             {
300                                 order        => \%orderinfo,
301                                 booksellerid => $booksellerid,
302                                 ordering     => 1,
303                                 receiving    => 1,
304                             }
305                         )
306                     };

Then in C4 Acquisition:

2943     if ($ordering) {
2944         $order->{tax_rate_on_ordering} //= $order->{tax_rate};
2945         if ( $bookseller->listincgst ) {
2946             # The user entered the rrp tax included
2947             $order->{rrp_tax_included} = $order->{rrp};
2948 
2949             # rrp tax excluded = rrp tax included / ( 1 + tax rate )
2950             $order->{rrp_tax_excluded} = $order->{rrp_tax_included} / ( 1 + $order->{tax_rate_on_ordering} );
2951 
2952             # ecost tax excluded = rrp tax excluded * ( 1 - discount )
2953             $order->{ecost_tax_excluded} = $order->{rrp_tax_excluded} * ( 1 - $discount );
2954 
2955             # ecost tax included = rrp tax included  ( 1 - discount )
2956             $order->{ecost_tax_included} = $order->{rrp_tax_included} * ( 1 - $discount );
2957         }
2958         else {
2959             # The user entered the rrp tax excluded
2960             $order->{rrp_tax_excluded} = $order->{rrp};
2961 
2962             # rrp tax included = rrp tax excluded * ( 1 - tax rate )
2963             $order->{rrp_tax_included} = $order->{rrp_tax_excluded} * ( 1 + $order->{tax_rate_on_ordering} );
2964 
2965             # ecost tax excluded = rrp tax excluded * ( 1 - discount )
2966             $order->{ecost_tax_excluded} = $order->{rrp_tax_excluded} * ( 1 - $discount );
2967 
2968             # ecost tax included = rrp tax excluded * ( 1 - tax rate ) * ( 1 - discount )
2969             $order->{ecost_tax_included} =
2970                 $order->{rrp_tax_excluded} *
2971                 ( 1 + $order->{tax_rate_on_ordering} ) *
2972                 ( 1 - $discount );
2973         }
2974 
2975         # tax value = quantity * ecost tax excluded * tax rate
2976         $order->{tax_value_on_ordering} =
2977             $order->{quantity} * $order->{ecost_tax_excluded} * $order->{tax_rate_on_ordering};
2978     }
Comment 1 Nick Clemens 2017-09-13 11:21:09 UTC
Created attachment 67110 [details] [review]
Bug 19296 - Allow all tax processing to happen in C4::Acquisition::populate_order_with_prices

To test:
 1 - Create an order file that includes prices and items
(MarcFieldsToOrder)
 2 - Stage the file
 3 - Set vendor to 'prices exclude tax'
 4 - Open a basket and add from the file
 5 - View the items in the basket
 6 - Prices are reduced by the tax rate and tax is calculated to return
prices to the value in the file
 7 - Apply patch
 8 - Repeat steps 1-6
 9 - Prices should now calculate correctly
10 - Repeat with 'MarcItemFieldsToOrder'
Comment 2 Katrin Fischer 2017-10-08 13:26:16 UTC
Tested both variations: list price includes GST and list price doesn't include GST. Verified bug and made sure that price calculations for ordering from staged and ordering from new (empty) record now match and make sense.
Comment 3 Katrin Fischer 2017-10-08 13:29:41 UTC
Created attachment 67822 [details] [review]
Bug 19296 - Allow all tax processing to happen in C4::Acquisition::populate_order_with_prices

To test:
 1 - Create an order file that includes prices and items
(MarcFieldsToOrder)
 2 - Stage the file
 3 - Set vendor to 'prices exclude tax'
 4 - Open a basket and add from the file
 5 - View the items in the basket
 6 - Prices are reduced by the tax rate and tax is calculated to return
prices to the value in the file
 7 - Apply patch
 8 - Repeat steps 1-6
 9 - Prices should now calculate correctly
10 - Repeat with 'MarcItemFieldsToOrder'

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>
Comment 4 Marcel de Rooy 2017-10-13 06:28:48 UTC
QA: Looking here now
Comment 5 Marcel de Rooy 2017-10-13 07:05:53 UTC
Created attachment 68045 [details] [review]
Bug 19296 - Allow all tax processing to happen in C4::Acquisition::populate_order_with_prices

To test:
 1 - Create an order file that includes prices and items
(MarcFieldsToOrder)
 2 - Stage the file
 3 - Set vendor to 'prices exclude tax'
 4 - Open a basket and add from the file
 5 - View the items in the basket
 6 - Prices are reduced by the tax rate and tax is calculated to return
prices to the value in the file
 7 - Apply patch
 8 - Repeat steps 1-6
 9 - Prices should now calculate correctly
10 - Repeat with 'MarcItemFieldsToOrder'

Signed-off-by: Katrin Fischer <katrin.fischer.83@web.de>

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 6 Marcel de Rooy 2017-10-13 07:05:57 UTC
Created attachment 68046 [details] [review]
Bug 19296: (QA Follow-up) Correct comment in populate_orders

Just replacing a wrong - by a correct + sign in the comment. The code
itself is fine.

Signed-off-by: Marcel de Rooy <m.de.rooy@rijksmuseum.nl>
Comment 7 Jonathan Druart 2017-10-13 16:20:26 UTC
listprice is going to be different as well, is it expected?
Comment 8 Nick Clemens 2017-10-18 14:09:05 UTC
(In reply to Jonathan Druart from comment #7)
> listprice is going to be different as well, is it expected?

I think this is correct - if list price includes GST then it should already have it, if it doesn't it doesn't need it.

Looking harder I am not sure why list price isn't just passed through the form but not adding that here.
Comment 9 Jonathan Druart 2017-10-18 15:05:43 UTC
Pushed to master for 17.11, thanks to everybody involved!
Comment 10 Fridolin Somers 2017-10-25 07:55:54 UTC
Pushed to 17.05.x, will be in 17.05.06.
Comment 11 Katrin Fischer 2017-10-29 18:08:48 UTC
Dependent bug is not in 16.11.x, skipping.