-
-
Notifications
You must be signed in to change notification settings - Fork 2.1k
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
[Core] Taxable variant #3915
[Core] Taxable variant #3915
Conversation
e7f5ff2
to
526a36b
Compare
726d038
to
1b8f5d0
Compare
@@ -71,7 +71,7 @@ public function apply(OrderInterface $order, ZoneInterface $zone) | |||
continue; | |||
} | |||
|
|||
$taxRate = $this->taxRateResolver->resolve($item->getProduct(), ['zone' => $zone]); | |||
$taxRate = $this->taxRateResolver->resolve($item->getProduct()->getMasterVariant(), ['zone' => $zone]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That is wrong, we should use variant of this specific item $item->getVariant()
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
We should add Behat scenario for applying different tax rates on two variants of the same product. |
@michalmarcinkowski 👍 - only problem I see that in new Behat contexts we currently handle only single variant products. I'd suggest waiting till #4053 is merged and then add this scenario here, so that new steps for selecting options, etc. follow new conventions. |
58ce9ee
to
cb80747
Compare
And I am logged in as "john@example.com" | ||
|
||
Scenario: Proper taxes for different taxed variants | ||
When I add product "PHP Mug" to the cart, selecting "Medium Mug" variant |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When I add "Medium Mug" variant of product "PHP Mug" to cart
6feab52
to
527bf45
Compare
@@ -63,6 +63,11 @@ | |||
</cascade> | |||
</one-to-many> | |||
|
|||
<many-to-one field="taxCategory" target-entity="Sylius\Component\Taxation\Model\TaxCategoryInterface"> | |||
<join-column name="tax_category_id" referenced-column-name="id" nullable="true" on-delete="SET NULL" /> | |||
<gedmo:versioned /> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we versioned this field? Some of fields has it, some doesn't.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd remove it for now as we do not handle these properly yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
527bf45
to
c209873
Compare
Thanks Mateusz! 👍 |
To improve taxes management and make it more flexible, relation to
TaxCategory
has been moved fromProduct
toProductVariant
.