[Core] Shipment taxes #3907
[Core] Shipment taxes #3907
Conversation
Zales0123
force-pushed
the
shipment-taxes
branch
from
efa9bff
to
4e7700b
Compare
| And the following payment methods exist: | ||
| | code | name | gateway | enabled | calculator | calculator_configuration | | ||
| | PM1 | Dummy | dummy | yes | fixed | amount: 0 | | ||
| | code | name | gateway | enabled | calculator | calculator_configuration | |
Zales0123
force-pushed
the
shipment-taxes
branch
from
4e7700b
to
5830f45
Compare
| * @param string $label | ||
| * @param bool $included | ||
| */ | ||
| private function addAdjustment($order, $taxAmount, $label, $included) |
There was a problem hiding this comment.
This addAdjustment method is a duplicate of what appears in the Sylius\Bundle\CoreBundle\OrderProcessing\OrderShipmentTaxesApplicator service. Consider moving this out into an adjustment factory service.
|
Thank you for adding this feature @Zales0123 👍 |
|
@tristanbes My pleasure ;) Hope, that together with other tax-related features and whole order logic refactoring it will be useful and powerful solution. |
|
@Zales0123 required rebase, as it will make the review easier. |
Zales0123
force-pushed
the
shipment-taxes
branch
from
5830f45
to
e0de8b1
Compare
|
|
||
| $percentageAmount = $taxRate->getAmountAsPercentage(); | ||
| $taxAmount = $this->calculator->calculate($lastShipping->getAmount(), $taxRate); | ||
| $label = sprintf('%s (%s%%)', $taxRate->getName(), (float) $percentageAmount); |
There was a problem hiding this comment.
This should be moved to getLabel method on TaxRate model.
|
I see it this way:
and then two implementations of
Then in
@Zales0123 WDYT? |
| * @param OrderInterface $order | ||
| * @param ZoneInterface $zone | ||
| */ | ||
| protected function processUnitsTaxes(OrderInterface $order, ZoneInterface $zone) |
There was a problem hiding this comment.
Should be extracted to OrderItemsByZoneTaxesApplicator
| } | ||
|
|
||
| $units = $item->getUnits(); | ||
| if ($units->isEmpty()) { |
There was a problem hiding this comment.
if (0 === $item->getQuantity())
There was a problem hiding this comment.
OK for now but we need to rethink the behavior of isEmpty method on the order. Normally we should use it here, but it checks if there are any items.
Zales0123
force-pushed
the
shipment-taxes
branch
from
086f990
to
c65077d
Compare
| /** | ||
| * @author Mateusz Zalewski <mateusz.zalewski@lakion.com> | ||
| */ | ||
| class OrderItemsByZoneTaxesApplicator implements OrderTaxesByZoneApplicatorInterface |
There was a problem hiding this comment.
IMO better name OrderItemsTaxesByZoneApplicator
It should implement OrderItemsByZoneTaxesApplicatorInterface
Zales0123
force-pushed
the
shipment-taxes
branch
2 times, most recently
from
edc90a4
to
190369d
Compare
Zales0123
force-pushed
the
shipment-taxes
branch
from
190369d
to
78253d3
Compare
|
Wooo, great work Mateusz! 👍 |
This PR is based on #3868.
Here there comes some next improvements with taxes ;)
ShippingMethodbecomes taxable so no we can apply some new taxes based on shipping fee. For now, taxes are applied onOrderlevel. There are also two new servicesOrderUnitsTaxesApplicatorandOrderShipmentTaxesApplicator(I think naming should be discussed), which are injected to mainOrderTaxesApplicator, to split taxes-application logic.