Mni/source selection #33

vrann · 2017-06-22T21:32:59Z

Design the extension points to inject multi-sources into the checkout process.
Issue: magento-engcom/magento2#30

Description

it should call Source Selection interface to allocate Order Items per sources
it should allow calculation shipping rate requests and aggregation of the results for the sources
it should display correct totals including the shipping cost
it should display the carriers and the sources for the merchant in the backend
It should reduce the stock number for the affected sources.

…i-Source - prototype of the rate request per package

ishakhsuvarov · 2017-06-23T07:25:28Z

app/code/Magento/Inventory/Model/PackageInterface.php

+     */
+    public function setAddress($address);
+
+    public function getQty();


Do we need qty here as it is already part of \Magento\Quote\Api\Data\CartItemInterface?

yes, because qty here is the number of the product in this particular package. It can be less or equal to the qty of the product in the cart. I.e. 10 products with sku1 were requested, 6 fulfilled from source 1, 4 from source 2.

Doesn't it make sense to just have different quote items in different packages? As it is already implemented even in the current version of the Shipping Assignments

Duplication of the Qty in multiple places leads to complicated code which would be potentially hard to maintain

agree with @ishakhsuvarov here.
Duplication of QTYs is misleading.
As QuoteItem (which represents single product SKU) has Qty
and Package which could consist of several SKUs has Qty (btw, why single, but not an array of quantities?)

So, it's hard to maintain and keep in consistent state.

that's why I proposed to consider usage of Visitor pattern here,
to separate Data from Algorithm implementation.
That could help us to live with current data structure of Quote

ishakhsuvarov · 2017-06-23T07:27:53Z

app/code/Magento/Quote/Api/Data/ShippingAssignmentInterface.php

@@ -34,6 +34,18 @@ public function getItems();
     */
    public function setItems($value);

+


I would suggest refactoring this interface in case we are already introducing BiC, as it does not reflect the desired state described in the latest version of the Multishipping design

ok, let me see

We had a discussion regarding implementation of the Visitor pattern to integrate MSI with Quote. Maybe it is better to research that solution?

ishakhsuvarov · 2017-06-23T07:29:57Z

app/code/Magento/Quote/Model/Quote/PackageCalculator.php

+            /**
+             * Skip if this item is virtual
+             */
+            if ($item->getProduct()->isVirtual()) {


Should virtual items be even included in the package?

maghamed

I looked through the interfaces added and modified.

For now Package interface introduces duplications of Qty data (Qty already represented in QuoteItem).
Also In ShippingAssignment we have a duplication of ShippingAddress.

We should have the only source of truth.
And taking into account that modification of Quote/QuoteItem interfaces is painful, it makes sense to consider Visitor pattern, which will provide an ability of segregation algorithm and Quote data

maghamed · 2017-06-23T12:47:39Z

app/code/Magento/Inventory/Model/PackageInterface.php

+    /**
+     * @return \Magento\InventoryApi\Api\Data\SourceInterface
+     */
+    public function getSource();


we no need to return SourceInterface in this interface, because Package can't contain the whole Source entity, it's not an Aggregate Root for the source.

What it can contain is a reference to SourceId.

maghamed · 2017-06-23T12:52:54Z

app/code/Magento/Inventory/Model/PackageInterface.php

+     */
+    public function setAddress($address);
+
+    public function getQty();


agree with @ishakhsuvarov here.
Duplication of QTYs is misleading.
As QuoteItem (which represents single product SKU) has Qty
and Package which could consist of several SKUs has Qty (btw, why single, but not an array of quantities?)

So, it's hard to maintain and keep in consistent state.

maghamed · 2017-06-23T12:54:21Z

app/code/Magento/Inventory/Model/PackageInterface.php

+
+    public function getWeight();
+
+    public function getItemQty();


what's the difference between Qty and ItemQuantity?

maghamed · 2017-06-23T12:55:24Z

app/code/Magento/Inventory/Model/PackageInterface.php

+    public function getItems();
+
+    /**
+     * @return \Magento\Quote\Model\Quote\Address


it should be Magento\Quote\Api\Data\AddressInterface
not the implementation

maghamed · 2017-06-23T12:56:48Z

app/code/Magento/Inventory/Model/PackageInterface.php

+
+    public function getItemQty();
+
+    public function getPhysicalValue();


what is business data represented with this Interface method?

PhysicalValue of the package?

maghamed · 2017-06-23T12:58:19Z

app/code/Magento/Inventory/Model/PackageInterface.php

+     */
+    public function setAddress($address);
+
+    public function getQty();


that's why I proposed to consider usage of Visitor pattern here,
to separate Data from Algorithm implementation.
That could help us to live with current data structure of Quote

maghamed · 2017-06-23T13:05:23Z

app/code/Magento/Inventory/Model/SourceSelectionInterface.php

+     * @param \Magento\Quote\Model\Quote\Address $destinationAddress
+     * @return Package[]
+     */
+    public function getPackages($store, $items, $destinationAddress);


why do we accept $store parameter here?
Just to read the configuration data? Why can't we resolve it?

in any case if we want introduce scoping , which we usually not do - it should be called $scope , but not $store.

maghamed · 2017-06-23T13:06:27Z

app/code/Magento/Inventory/Model/SourceSelectionInterface.php

+     * @param \Magento\Quote\Model\Quote\Address $destinationAddress
+     * @return Package[]
+     */
+    public function getPackages($store, $items, $destinationAddress);


is this method supposed to be called several times in case of multi-shipping delivery?

maghamed · 2017-06-23T13:10:40Z

app/code/Magento/Quote/Api/Data/ShippingAssignmentInterface.php

+    /**
+     * @return \Magento\Inventory\Model\PackageInterface[]
+     */
+    public function getPackages();


again ShippingAssignment interface has get/setQuoteItems
you also intorduce get/setPackages which also have get/setQuoteItems

maghamed · 2017-06-23T13:12:24Z

app/code/Magento/Quote/Api/Data/ShippingAssignmentInterface.php

+    /**
+     * @return \Magento\Inventory\Model\PackageInterface[]
+     */
+    public function getPackages();


Moreover there is a get/setShipping method here which returns
\Magento\Quote\Api\Data\ShippingInterface
so, you have also duplication of the shipping address here and in Package.

… to use plugins Void methods were breaking Interceptor classes because of the presence of a return statement. Return statements with any kind of value (even null) is forbiddent by PHP7.2 . Interceptor generation clas has been modified to check the void return type and unit tests have been adjusted accordingly.

vrann added 2 commits June 12, 2017 19:20

magento-engcom/magento2#30: Design Checkout Extension Points for Mult…

c5aed6b

…i-Source - prototype of the rate request per package

magento-engcom/magento2#30: Design Checkout Extension Points for Mult…

3c6b8db

…i-Source - prototype of the rate request per package

vrann added the prototype/investigation label Jun 22, 2017

vrann self-assigned this Jun 22, 2017

vrann requested a review from maghamed June 22, 2017 21:34

ishakhsuvarov reviewed Jun 23, 2017

View reviewed changes

maghamed suggested changes Jun 23, 2017

View reviewed changes

naydav closed this Oct 12, 2017

This was referenced Dec 11, 2017

Exception during creating New Stock #280

Closed

Exception during editing Default Stock #281

Closed

Exception during creating New Source #282

Closed

smoskaluk mentioned this pull request Mar 12, 2018

Exception when trying to create Grouped or Bundle product in admin #654

Closed

smoskaluk mentioned this pull request Mar 23, 2018

Exception in Low stock report for product with another name on additional storeview #737

Closed

phoenix128 mentioned this pull request Apr 21, 2018

FIX issue #33 - Returning void on method makes the whole class unable… #993

Closed

4 tasks

smoskaluk mentioned this pull request May 18, 2018

Installation B2b with MSI fails #1198

Closed

maghamed deleted the mni/source-selection branch December 11, 2018 18:12

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Mni/source selection #33

Mni/source selection #33

vrann commented Jun 22, 2017

ishakhsuvarov Jun 23, 2017

vrann Jun 23, 2017

ishakhsuvarov Jun 23, 2017 •

edited

Loading

maghamed Jun 23, 2017

maghamed Jun 23, 2017

ishakhsuvarov Jun 23, 2017

vrann Jun 23, 2017

ishakhsuvarov Jun 23, 2017

ishakhsuvarov Jun 23, 2017

maghamed left a comment

maghamed Jun 23, 2017

maghamed Jun 23, 2017

maghamed Jun 23, 2017

maghamed Jun 23, 2017

maghamed Jun 23, 2017

maghamed Jun 23, 2017

maghamed Jun 23, 2017

maghamed Jun 23, 2017

maghamed Jun 23, 2017

maghamed Jun 23, 2017

		@@ -34,6 +34,18 @@ public function getItems();
		*/
		public function setItems($value);


		public function getItemQty();

		public function getPhysicalValue();

Mni/source selection #33

Mni/source selection #33

Conversation

vrann commented Jun 22, 2017

Description

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishakhsuvarov Jun 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

maghamed left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ishakhsuvarov Jun 23, 2017 •

edited

Loading