Order total wrong from AdminOrder when VAT number used

[ckubitza]

Describe the bug
Wrong tax is applied to order totals when order is created from admin page with a foreign address which has a VAT-number. Product taxes are correct but not the totals. Frontend orders are not affected.

To Reproduce
Steps to reproduce the behavior:

1. Have products with taxes and VAT-number module is active
2. Place an order from admin orders page with an invoice address in another EU country and a VAT-number so no taxes should be applied (correctly shown in the order summary at the bottom)
3. The order is created with correct order details (..._price_tax_incl = ..._price_tax_excl in table ps_order_details), but the order totals are wrong (e.g. total_products != total_products_wt in table ps_orders). These wrong values lead to an invalid invoice because the product sum is wrong.

Additional information
PrestaShop version: 1.7.6.0
PHP version: 7.2
VAT-number module: 2.0.0

Technical Details
It seams that the order total is calculated here https://github.com/PrestaShop/PrestaShop/blob/c389f90984cdf46c42ecf970342553b6db867aee/controllers/admin/AdminOrdersController.php#L1248 which internally uses the Product::getPriceStatic() function but without an address_id. In this situation, the function tries to get it from the cart object in the context. But the context does not have a cart object yet, so a default address is used with a wrong country. You can see that $id_address is NULL here https://github.com/PrestaShop/PrestaShop/blob/b2f4a3508462790192259028e2e39c3f58a4baca/classes/Product.php#L3218 (compare it to a frontend order which is working correctly).

Possible Solution
Add this

Context::getContext()->cart = $cart;

after this line: https://github.com/PrestaShop/PrestaShop/blob/c389f90984cdf46c42ecf970342553b6db867aee/controllers/admin/AdminOrdersController.php#L1231 to set the correct cart in the context before the totals are calculated.
I could provide a pull request for this.

[khouloudbelguith]

Hi @ckubitza,

I did not manage to reproduce the issue with PS1.7.6.1 & the module VAT v2.1.0.
I attached a screen record
https://drive.google.com/file/d/12IMIdYjdCSRzUALvq6x3gkwJT4AABptO/view
Thanks to check & feedback.

[ckubitza]

Oh, I didn't noticed that there is an update for the vatmodule, I was using v2.0.0. So I tried the new version and indeed, the issue doesn't show up now.
I looked at the changes in the module, there some kind of caching were added, but only if the address is valid. So this filters out wrong calls and seems to lead to a valid cart in the context at some point, but I currently can not say how exactly.
Anyway, the comments there are saying that this behaviour is almost unclear, so maybe my observations can help clearing this out.

[khouloudbelguith]

@ckubitza, so the issue is fixed after upgrading the module?

Thanks!

[ckubitza]

Well, yes. But I tend to think that the update in the module is just obfuscating the real bug.

[khouloudbelguith]

@ckubitza,

the real issue is when making an order with an address in another EU country => the taxes are applied => which is wrong

With the new version there are no taxes applied => so can you explain to me how the module is just obfuscating the real bug & not fixing the issue?

Thanks!

[ckubitza]

In my observations I have found out, that the total price calculation is not given an address, so it tries to get it from the cart in the context, which is not set at this time when the adminorder is used.
The module update does not change this behaviour, it only "filters out" invalid calls through a cache, and I can only guess how this leads to a solution for this problem.
Also, the detailed comment in this module update shows, that the motivation for this update were just these multiple calls to this function with frequent invalid arguments and unknown reason. When you set the cart object in the context as I propose in my report, the calls with invalid arguments are (almost) eliminated. As I don't have a complete insight in all the mechanisms while making an order, I can not say for sure that this change is a "good" solution or a "better" solution than the update in the module. But I tend to be not fully "satisfied" with just the module update ;)

Here I made some logs for the calls in the vatnumber module (edit: v2.0.1 v2.1.0) and in the product-price functions with valid/invalid addresses while making an order over admin page:

Original:

VATNumberTaxManager::isAvailableForThisAddress() $address->id_customer: "1051"
Product::getPriceStatic() $id_address: ""
VATNumberTaxManager::isAvailableForThisAddress() $address->id_customer: ""
Product::getPriceStatic() $id_address: ""
VATNumberTaxManager::isAvailableForThisAddress() $address->id_customer: ""
Product::getPriceStatic() $id_address: ""
Product::getPriceStatic() $id_address: ""
Product::getPriceStatic() $id_address: ""
Product::getPriceStatic() $id_address: ""
VATNumberTaxManager::isAvailableForThisAddress() $address->id_customer: ""
VATNumberTaxManager::isAvailableForThisAddress() $address->id_customer: "1051"
Product::getPriceStatic() $id_address: "10234"
VATNumberTaxManager::isAvailableForThisAddress() $address->id_customer: ""
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: ""
Product::getPriceStatic() $id_address: ""
Product::getPriceStatic() $id_address: ""
Product::getPriceStatic() $id_address: ""
Product::getPriceStatic() $id_address: ""
Product::getPriceStatic() $id_address: ""
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"

... and here with my changes (setting the cart in context before getting totals):

VATNumberTaxManager::isAvailableForThisAddress() $address->id_customer: "1051"
Product::getPriceStatic() $id_address: "10234"
VATNumberTaxManager::isAvailableForThisAddress() $address->id_customer: "1051"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
VATNumberTaxManager::isAvailableForThisAddress() $address->id_customer: ""
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"
Product::getPriceStatic() $id_address: "10234"

[khouloudbelguith]

@ckubitza, thanks for your feedback.
I think this issue is fixed with this PR: #8
Ping @Quetzacoalt91 @PrestaShop/prestashop-core-developers what do you think?
The issue is not reproduced with the new version but @ckubitza thinks that does not really fix that.

Thanks!