Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Tax zones leading to bad tax calculation #11788

Open
jacquesbh opened this issue Sep 1, 2020 · 5 comments
Open

Tax zones leading to bad tax calculation #11788

jacquesbh opened this issue Sep 1, 2020 · 5 comments
Labels
Do not stale Important issues and PRs, that should not be stalled by Stale Bot RFC Discussions about potential changes or new features.

Comments

@jacquesbh
Copy link
Member

jacquesbh commented Sep 1, 2020

Sylius version affected: all since we have the tax zones I think

Description
Considering a country (let's say FRANCE) is in two zones (let's say ZONE A and then ZONE B, both type "country" so same priority, but A entered before B in database).
Considering a tax rate for ZONE A.

If I order with a billing address in FRANCE, then I don't get the tax, but I should because FRANCE is in ZONE A.

BUT !

public function match(AddressInterface $address, ?string $scope = null): ?ZoneInterface
{
$zones = [];
/** @var ZoneInterface $zone */
foreach ($this->getZones($scope) as $zone) {
if ($this->addressBelongsToZone($address, $zone)) {
$zones[$zone->getType()] = $zone;
}
}
foreach (self::PRIORITIES as $priority) {
if (isset($zones[$priority])) {
return $zones[$priority];
}
}
return null;
}

Here we take the last zone in the loop (line 46), which will lead to ZONE B.

And since ZONE B ≠ ZONE A (zone with the tax applied) then I won't have any tax applied.

The issue here is: we consider a country to be in ONE zone, but it can be in more than one zone.

Steps to reproduce

Add the ZONE A and ZONE B for the following countries: FRANCE. Add them in this order so ZONE B is the last one taken into account in the ZoneMatcher.
Add a tax rate applying on ZONE A.
Order something with FRANCE as billing country.
You won't have any tax, but you SHOULD.

Possible Solution

I think it's important to consider that a country can be in more than one zone and to compare all zones to the tax rates, instead of only one.

@lchrusciel lchrusciel added the RFC Discussions about potential changes or new features. label Sep 8, 2020
@stale
Copy link

stale bot commented Dec 25, 2020

This issue has been automatically marked as stale because it has not had any recent activity. It will be closed in a week if no further activity occurs. Thank you for your contributions.

@stale stale bot added the Stale Issues and PRs with no recent activity, about to be closed soon. label Dec 25, 2020
@jacquesbh
Copy link
Member Author

Do not stale.

This is still an issue, and can till lead to some errors in taxes.

@stale stale bot removed the Stale Issues and PRs with no recent activity, about to be closed soon. label Dec 25, 2020
@Zales0123 Zales0123 added the Do not stale Important issues and PRs, that should not be stalled by Stale Bot label Dec 29, 2020
@jacquesbh
Copy link
Member Author

Applying more than one tax will lead to more issues.
Like if we have 2 taxes included in price: what is the excluding price of the product? :p

Also having more than one zone in the match fixes the issue we have.

I think adding a priority to the Tax rates could help fixe the side effect of the first fix.

So, what I suggest:

  • Always use a matchAll to match zones.
  • Add a priority on Tax rates and then we apply only one tax per item (product, shipping)

@Sylius/core-team We need to discuss this, there is a big issue here and I am very surprised than nobody complained about it.

@jacquesbh
Copy link
Member Author

Or we can say that an item (product or shipping) can have only one "tax included in the price" applied.

@maximehuran
Copy link
Contributor

The snippet above correct the missing zones tax application but we can have some weird cases.
If 2 taxes are included in price, the calculation is not correct.
I think it could be great to have only one tax included in price applied, and the other (not included in price) can be applied too.

diff --git a/src/Sylius/Component/Core/OrderProcessing/OrderTaxesProcessor.php b/src/Sylius/Component/Core/OrderProcessing/OrderTaxesProcessor.php
index a39978abb4..52147c925e 100644
--- a/src/Sylius/Component/Core/OrderProcessing/OrderTaxesProcessor.php
+++ b/src/Sylius/Component/Core/OrderProcessing/OrderTaxesProcessor.php
@@ -58,34 +58,40 @@ final class OrderTaxesProcessor implements OrderProcessorInterface
             return;
         }
 
-        $zone = $this->getTaxZone($order);
+        $zones = $this->getTaxZones($order);
 
-        if (null === $zone) {
+        if (null === $zones) {
             return;
         }
 
+        $strategyApplied = false;
         /** @var TaxCalculationStrategyInterface $strategy */
         foreach ($this->strategyRegistry->all() as $strategy) {
-            if ($strategy->supports($order, $zone)) {
-                $strategy->applyTaxes($order, $zone);
-
-                return;
+            foreach ($zones as $zone) {
+                if ($strategy->supports($order, $zone)) {
+                    $strategy->applyTaxes($order, $zone);
+                    $strategyApplied = true;
+                }
             }
         }
 
-        throw new UnsupportedTaxCalculationStrategyException();
+        if (!$strategyApplied) {
+            throw new UnsupportedTaxCalculationStrategyException();
+        }
     }
 
-    private function getTaxZone(OrderInterface $order): ?ZoneInterface
+    private function getTaxZones(OrderInterface $order): ?array
     {
         $billingAddress = $order->getBillingAddress();
-        $zone = null;
+        $zones = null;
 
         if (null !== $billingAddress) {
-            $zone = $this->zoneMatcher->match($billingAddress, Scope::TAX);
+            $zones = $this->zoneMatcher->matchAll($billingAddress, Scope::TAX);
         }
 
-        return $zone ?: $this->defaultTaxZoneProvider->getZone($order);
+        $defaultTaxZone = $this->defaultTaxZoneProvider->getZone($order);
+
+        return $zones ?: ($defaultTaxZone ? [$defaultTaxZone] : null) ;
     }
 
     private function clearTaxes(OrderInterface $order): void

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Do not stale Important issues and PRs, that should not be stalled by Stale Bot RFC Discussions about potential changes or new features.
Projects
None yet
Development

No branches or pull requests

4 participants