[RFC][Order] Introduce OrderItemUnit #3771
Conversation
pjedrzejewski
added
RFC
| @@ -24,7 +24,7 @@ | |||
|
|
|||
| <field name="inventoryState" column="inventory_state" type="string" /> | |||
|
|
|||
| <many-to-one field="stockable" target-entity="Sylius\Component\Inventory\Model\StockableInterface"> | |||
| <many-to-one field="stockable" target-entity="Sylius\Component\Core\Model\ProductVariant"> | |||
There was a problem hiding this comment.
InventoryBundle shouldn't know about Core :)
There was a problem hiding this comment.
Sure it should not! I'm aware about some decoupling issues that can appear in this PR and I will be grateful for pointing them out - it will make my work much easier ;)
There was a problem hiding this comment.
This relation should be just removed from here.
Zales0123
force-pushed
the
order-item-unit
branch
2 times, most recently
from
47c865b
to
f38baa4
Compare
Zales0123
changed the title
Zales0123
force-pushed
the
order-item-unit
branch
from
55012bd
to
ee8a81e
Compare
Zales0123
force-pushed
the
order-item-unit
branch
3 times, most recently
from
7a0b307
to
e1538c0
Compare
| @@ -1,6 +1,6 @@ | |||
| Sylius\Component\Core\Model\InventoryUnit: | |||
| Sylius\Component\Core\Model\OrderItemUnit: | |||
There was a problem hiding this comment.
File name should be Model.OrderItemUnit.yml
Zales0123
force-pushed
the
order-item-unit
branch
2 times, most recently
from
94f9f66
to
96a45ab
Compare
|
|
||
| namespace Sylius\Component\Core\Model; | ||
|
|
||
| use Sylius\Component\Inventory\Model\InventoryUnitInterface as BaseInventoryUnitInterface; |
There was a problem hiding this comment.
Do we need to make an alias here?
Zales0123
force-pushed
the
order-item-unit
branch
from
7a24658
to
991aa2a
Compare
|
👍 |
Zales0123
force-pushed
the
order-item-unit
branch
from
991aa2a
to
4e393fe
Compare
Zales0123
force-pushed
the
order-item-unit
branch
from
4e393fe
to
39fb1dd
Compare
| ### Order and SyliusOrderBundle | ||
|
|
||
| * Introduced ``OrderItemUnit``, which represents every single unit in ``Order`` | ||
| * Removed ``InventoryUnit``, as it's replaced by ``OrderItemUnit``, which can be used both as ``InventoryUnit`` and ``ShipmentItem`` |
There was a problem hiding this comment.
Replaced InventoryUnit with OrderItemUnit in the core. This entity will be used as InventoryUnit and ShipmentItem;
[RFC][Order] Introduce OrderItemUnit
|
Thank you Mateusz and the 2 additional brains behind making sure that Sylius handles $ well - @peteward and @michalmarcinkowski. 👍 Happy rebasing of related PRs. :D |
|
👍 👍 👍 Great work all. |
|
Great to see that this one is on such a good path to be finished :) |
Hello everybody!
This is a Proof of Concept PR that illustrates idea how to resolve issues with how adjustments (thus taxes, shipping fees and discounts) are handled on the Order model. It is based on excellent work and research of Peter from REISS.
See corresponding PRs/Issues:
Overview of how Adjustments are handled currently:
Full rationale for why this is wrong is available in the issues listed above.
Solution:
To fix this problem, we’ve created completely new
OrderItemUnit. In this highly preliminary PR, there is a proper model and interface introduced and used asInventoryUnitandShipmentIteminsylius.yml.The logic of splitting OrderItem’s quantity into separate OrderItemUnits will be part of Order Component. In Core, to avoid duplication and for performance reasons,
OrderItemUnitwill implementShipmentItemInterfaceandInventoryUnitInterface, thus our current logic regarding shipping and inventory can remain untouched.Having OrderItemUnits will allow us to apply adjustments more precisely and get the financials right. We can also provide two implementations for any service, like 2 TaxationProcessors, one which applies taxes on OrderItemUnit level (default) and optional for simpler use-cases that applies taxes to Order level.
There is plenty of work with adjustments to make them perfect and suitable for modern e-commerce system which Sylius obviously is, but it can be first, important step to do this. When we do this, we also want to finally clean up the way that totals are calculated and remove this non-sense with
setTotal()method. :)Let me hear your thoughts!
UPDATE:
Related docs PR: Sylius/Sylius-Docs#394