Migrate Single Stock Data From CatalogInventory to MSI SourceItems #198

bartekszymanski · 2017-11-09T22:58:23Z

Description

Fixed Issues (if relevant)

Migrate Single Stock Data From CatalogInventory to MSI SourceItems #178: Migrate Single Stock Data From CatalogInventory to MSI SourceItems
Need to add "@magentoAppArea adminhtml" to all tests which are used with InventoryApi product fixtures #263: Need to add "@magentoAppArea adminhtml" to all tests which are used with InventoryApi product fixtures

Manual testing scenarios

Contribution checklist

Add data to default source (Data Install) when merchant migrates from Magento 2.0.X, 2.1.X, 2.2.X -> Magento 2.3 with MSI
Plugin when saving via StockItem resource model.
Test save Stock Data in the scope of Product. And check new storage
Test to save Old Stock Item via StockItem Repository and check new storage
Test to save Old Stock Item via Resource Model and check data in New storage

…ngle-stock-data

…Add data to default source (Data Install), split InstallData to processors

# Conflicts: # app/code/Magento/InventoryCatalog/Setup/InstallData.php

naydav · 2017-11-23T11:57:04Z

app/code/Magento/InventoryCatalog/Plugin/Model/ResourceModel/StockItemPlugin.php

+     * @throws AlreadyExistsException;
+     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
+     */
+    public function aroundSave(Interceptor $subject, callable $proceed, Item $stockItem): AbstractDb


Why do we use around plugin instead of after plugin?

Maybe it is better to use observer event? In this case, all operations will be in scope of one transaction (now our code will be performed in out of transaction)
@maghamed pls provide your opinion

with around plugin we could achieve stock deduction for MSI in the same DB transaction that stock deduction for CatalogInventory StockItem happened

with around plugin we could either achieve transactional consistency as we control state before the subject execution and after

naydav · 2017-11-23T12:47:57Z

app/code/Magento/InventoryCatalog/Plugin/Model/ResourceModel/StockItemPlugin.php

+/**
+ * Class provide Around Plugin on Stock::save to migrate single stock data
+ */
+class StockItemPlugin


Please, Provide more clear name (what is this plugin responsible for)

yes, better to sync up with existing plugin we have for Reservation placement.
We could give them similar names and more to the single location.
For example,

1. UpdateLegacyCatalogInventoryAtStockDeductionTime 2. UpdateLegacyCatalogInventoryAtStockSettingTime

Because we have two different business operations with stock:

We set stock data and overwrite existing data in StockItem (despite the data stored there before) , this happens for example during the Import or synchronization with ERP

Stock Deduction, which happens during the order placement, when we need to subtract given quantity from the data we have in Stock

…ck-data # Conflicts: # app/code/Magento/InventoryCatalog/etc/di.xml

naydav · 2017-11-23T13:06:34Z

app/code/Magento/InventoryCatalog/Setup/Operation/MigrateSingleStockData.php

+    public function execute()
+    {
+        $defaultSourceId = $this->defaultSourceProvider->getId();
+        $sql = "INSERT INTO inventory_source_item (source_id, sku, quantity, status) 


Need to resolve table name via setup object
example $sourceTable = $setup->getTable(SourceResourceModel::TABLE_NAME_SOURCE);
Now solution is not work if table prefixes are used

Also, need to use constants for fields

naydav · 2017-11-23T13:19:49Z

app/code/Magento/InventoryCatalog/Setup/Operation/MigrateSingleStockData.php

+    public function execute()
+    {
+        $defaultSourceId = $this->defaultSourceProvider->getId();
+        $sql = "INSERT INTO inventory_source_item (source_id, sku, quantity, status) 


Need to add dependency on Inventory in module sequence configuration in module.xml
Because we operate inventory_source_item table which is created in Inventory module

So we need to guarantee that install scheme from Inventory is finished before InventoryCatalog installation running

Looks like we have double saving of SourceItem
For example, I feel both fields in UI (old qty field and new in our grid) with different qty values

So we have save in:

In this plugin

In \Magento\InventoryCatalog\Observer\ProcessSourceItemsObserver::execute

And as result, we will have in old system(legacy stockItem) one value, and in new system(source item) different value

Maybe need to improve ProcessSourceItemsObserver (need to work only for multi stock system)
But looks like it will be better to do this in separate task
@maghamed

naydav · 2017-11-23T13:32:13Z

app/code/Magento/InventoryCatalog/Plugin/Model/ResourceModel/StockItemPlugin.php

+     * @throws AlreadyExistsException;
+     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
+     */
+    public function aroundSave(Interceptor $subject, callable $proceed, Item $stockItem): AbstractDb


The first parameter is \Magento\Framework\Model\ResourceModel\Db\AbstractDb (in type hinting)

The first parameter should have type Magento\CatalogInventory\Model\ResourceModel\Stock\Item

naydav · 2017-11-23T13:45:23Z

app/code/Magento/InventoryCatalog/Plugin/Model/ResourceModel/StockItemPlugin.php

+        $this->sourceItem->setSku($product->getSku());
+        $this->sourceItem->setQuantity($stockItem->getQty());
+        $this->sourceItem->setStatus((int)$stockItem->getIsInStock());
+        $this->sourceItemsSave->execute([$this->sourceItem]);


Need to add type casting to
$this->sourceItem->setQuantity($stockItem->getQty());

The case with creating product in admin panel:
If original field (old field in UI) is not filled then we have NULL value
And after that we will have fail in \Magento\Inventory\Model\SourceItem\Validator\QuantityValidator::validate

maghamed · 2017-11-17T11:34:29Z

app/code/Magento/InventoryCatalog/Plugin/Model/ResourceModel/StockItemPlugin.php

+use Magento\Framework\Exception\AlreadyExistsException;
+
+/**
+ * Class provide Around Plugin on Stock::save to migrate single stock data


provideS
not Stock::save but Magento\CatalogInventory\Model\ResourceModel\Stock\Item::save

maghamed · 2017-11-23T14:25:51Z

app/code/Magento/InventoryCatalog/Plugin/Model/ResourceModel/StockItemPlugin.php

+     * @throws AlreadyExistsException;
+     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
+     */
+    public function aroundSave(Interceptor $subject, callable $proceed, Item $stockItem): AbstractDb


The first parameter should have type Magento\CatalogInventory\Model\ResourceModel\Stock\Item

maghamed · 2017-11-23T14:28:47Z

app/code/Magento/InventoryCatalog/Plugin/Model/ResourceModel/StockItemPlugin.php

+     * @throws AlreadyExistsException;
+     * @SuppressWarnings(PHPMD.UnusedFormalParameter)
+     */
+    public function aroundSave(Interceptor $subject, callable $proceed, Item $stockItem): AbstractDb


with around plugin we could achieve stock deduction for MSI in the same DB transaction that stock deduction for CatalogInventory StockItem happened

with around plugin we could either achieve transactional consistency as we control state before the subject execution and after

maghamed · 2017-11-23T14:37:41Z

app/code/Magento/InventoryCatalog/Plugin/Model/ResourceModel/StockItemPlugin.php

+/**
+ * Class provide Around Plugin on Stock::save to migrate single stock data
+ */
+class StockItemPlugin


yes, better to sync up with existing plugin we have for Reservation placement.
We could give them similar names and more to the single location.
For example,

1. UpdateLegacyCatalogInventoryAtStockDeductionTime 2. UpdateLegacyCatalogInventoryAtStockSettingTime

Because we have two different business operations with stock:

We set stock data and overwrite existing data in StockItem (despite the data stored there before) , this happens for example during the Import or synchronization with ERP

Stock Deduction, which happens during the order placement, when we need to subtract given quantity from the data we have in Stock

maghamed · 2017-11-23T14:40:25Z

app/code/Magento/InventoryCatalog/Plugin/Model/ResourceModel/StockItemPlugin.php

+    public function aroundSave(Interceptor $subject, callable $proceed, Item $stockItem): AbstractDb
+    {
+        $product = $this->productRepository->getById($stockItem->getProductId());
+        $this->sourceItem->setSourceId($this->defaultSourceProvider->getId());


Ideally, you should use SourceItemFactory here, because currently you have the dependency in class on SourceItemInterface $sourceItem, which could be given to you in a dirty state.
But what you need is to construct new SourceItemInterface here - that's the responsibility of factories.

maghamed · 2017-11-23T14:44:01Z

app/code/Magento/InventoryCatalog/Setup/Operation/MigrateSingleStockData.php

+        $defaultSourceId = $this->defaultSourceProvider->getId();
+        $sql = "INSERT INTO inventory_source_item (source_id, sku, quantity, status) 
+                  SELECT $defaultSourceId AS source_id, product.sku, stock_item.qty, stock_item.is_in_stock 
+                  FROM cataloginventory_stock_item AS stock_item 


also where clause with website_id = 0 should be added.
Because some customizations duplicate data in our CatalogInventory cataloginventory_stock_item table and store Stock data per Website

maghamed · 2017-11-23T14:44:32Z

app/code/Magento/InventoryCatalog/Setup/Operation/MigrateSingleStockData.php

+        $defaultSourceId = $this->defaultSourceProvider->getId();
+        $sql = "INSERT INTO inventory_source_item (source_id, sku, quantity, status) 
+                  SELECT $defaultSourceId AS source_id, product.sku, stock_item.qty, stock_item.is_in_stock 
+                  FROM cataloginventory_stock_item AS stock_item 


Name of the table cataloginventory_stock_item should also be resolved

maghamed · 2017-11-23T14:58:25Z

app/code/Magento/InventoryCatalog/Plugin/Model/ResourceModel/StockItemPlugin.php

+        $this->sourceItem->setStatus((int)$stockItem->getIsInStock());
+        $this->sourceItemsSave->execute([$this->sourceItem]);
+
+        return $proceed($stockItem);


let's wrap this code into the DB transaction

…ngle-stock-data

maghamed · 2017-12-08T12:47:13Z

app/code/Magento/InventoryCatalog/etc/di.xml

+        <plugin name="update_legacy_catalog_inventory_plugin" type="Magento\InventoryCatalog\Plugin\InventoryApi\Api\UpdateLegacyCatalogInventoryAtStockDeductionTime"/>
+    </type>
+    <type name="Magento\CatalogInventory\Model\ResourceModel\Stock\Item">
+        <plugin name="migrate_single_stock_data" type="Magento\InventoryCatalog\Plugin\CatalogInventory\Model\ResourceModel\Stock\UpdateSourceItemAtLegacyCatalogInventoryStockSettingTime" sortOrder="1"/>


let's remove sortOrder="1"

maghamed · 2017-12-08T12:47:54Z

app/code/Magento/InventoryCatalog/etc/di.xml

+        <plugin name="migrate_single_stock_data" type="Magento\InventoryCatalog\Plugin\CatalogInventory\Model\ResourceModel\Stock\UpdateSourceItemAtLegacyCatalogInventoryStockSettingTime" sortOrder="1"/>
+    </type>
+    <type name="Magento\CatalogInventory\Model\ResourceModel\QtyCounterInterface">
+        <plugin name="migrate_single_stock_data_at_qty_counter" type="Magento\InventoryCatalog\Plugin\CatalogInventory\Model\ResourceModel\UpdateSourceItemAtLegacyCatalogInventoryQtyCounter" sortOrder="1"/>


let's remove sortOrder="1" as it's usage is a bad practice for plugin design

maghamed · 2017-12-08T13:04:59Z

app/code/Magento/InventoryCatalog/etc/module.xml

@@ -8,7 +8,7 @@
 <config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:Module/etc/module.xsd">
    <module name="Magento_InventoryCatalog" setup_version="2.0.0">
        <sequence>
-            <module name="InventorySales"/>
+            <module name="Magento_Inventory"/>


we should have a dependency on Magento_InventoryAPI but not on Inventory

It is not dependency
But it is sequence, so we need already installed tables which are described in Magento_Inventory
So now declaration is correct

maghamed · 2017-12-08T13:22:22Z

.../CatalogInventory/Model/ResourceModel/UpdateSourceItemAtLegacyCatalogInventoryQtyCounter.php

+     */
+    public function aroundCorrectItemsQty($subject, callable $proceed, array $items, $websiteId, $operator)
+    {
+        $proceed($items, $websiteId, $operator);


we need to wrap with transaction initial method as well
so,

$connection = $this->resourceConnection->getConnection(); + $connection->beginTransaction();

should be before $proceed

maghamed · 2017-12-08T13:24:30Z

.../CatalogInventory/Model/ResourceModel/UpdateSourceItemAtLegacyCatalogInventoryQtyCounter.php

+
+        if ($websiteId !== 0) {
+            return;
+        }


why do we return if websiteId <> 0?

Current implementation re-uses the same stock for all websites.

maghamed · 2017-12-08T13:29:34Z

.../CatalogInventory/Model/ResourceModel/UpdateSourceItemAtLegacyCatalogInventoryQtyCounter.php

+            $sourceItems = $this->sourceItemRepository->getList($searchCriteria)->getItems();
+            $sourceItems = array_map(
+                function (SourceItemInterface $item) use ($productsData, $operator) {
+                    $item->setQuantity($item->getQuantity() + (int)($operator . $productsData[$item->getSku()]));


why do we make casting to (int) but not to (float) ?
looks like we have unneeded rounding

naydav · 2017-12-08T14:34:39Z

app/code/Magento/Inventory/Test/Integration/Indexer/SourceItemIndexerTest.php

+     *
+     * @return int
+     */
+    private function getInventorySourceItemId(string $sku, int $sourceId): int


Looks like we can extract this code in separate object (in test namespace) and reuse this object in the other cases
After that we can remove $connection->query('ALTER TABLE ' . $resourceConnection->getTableName('inventory_source_item') . ' AUTO_INCREMENT 1;'); from fixtures

naydav · 2017-12-08T14:36:20Z

app/code/Magento/Inventory/Test/Integration/Indexer/SourceIndexerTest.php

@@ -13,6 +13,9 @@
 use Magento\TestFramework\Helper\Bootstrap;
 use PHPUnit\Framework\TestCase;

+/**
+ * @magentoAppArea adminhtml


Indexer test must work in any areas
If need you could use trick with is secure area

naydav · 2017-12-08T14:41:08Z

...Magento/InventoryCatalog/Model/Command/UpdateLegacyCatalogInventoryStockItemByPlainQuery.php

+            $connection->getTableName('cataloginventory_stock_item'),
+            [
+                StockItemInterface::QTY => new \Zend_Db_Expr(
+                    sprintf('%s%s', StockItemInterface::QTY, $reservation->getQuantity())


looks like need only concatenation

naydav · 2017-12-08T14:50:01Z

...Magento/InventoryCatalog/Model/Command/UpdateLegacyCatalogInventoryStockItemByPlainQuery.php

+    /**
+     * {@inheritdoc}
+     */
+    public function execute(ReservationInterface $reservation)


Need to unify interface
Method should work with sku and qty
In this case we could reuse this code when source item is updated

naydav · 2017-12-08T14:51:54Z

...gento/InventoryCatalog/Model/Command/UpdateLegacyCatalogInventoryStockStatusByPlainQuery.php

+            $connection->getTableName('cataloginventory_stock_status'),
+            [
+                StockStatusInterface::QTY => new \Zend_Db_Expr(
+                    sprintf('%s%s', StockStatusInterface::QTY, $reservation->getQuantity())


Looks like logic is not related to Interface
We need to update status

naydav · 2017-12-08T14:54:25Z

...ntory/Model/ResourceModel/Stock/UpdateSourceItemAtLegacyCatalogInventoryStockSettingTime.php

+
+            $product = $this->productRepository->getById($stockItem->getProductId());
+            $sourceItem = $this->sourceItemFactory->create();
+            $sourceItem->setSourceId($this->defaultSourceProvider->getId());


Need to think about case if product is not assigned to Default Source

naydav · 2017-12-08T14:55:57Z

app/code/Magento/InventoryCatalog/Setup/Operation/UpdateInventorySourceItem.php

+        $selectForInsert = $this->resourceConnection->getConnection()->select()->from(
+            $stockItemTable,
+            ['source_id' => new \Zend_Db_Expr($defaultSourceId), 'qty', 'is_in_stock']
+        )->join($productTable, 'entity_id = product_id', 'sku')->where('website_id = ?', 0);


Check behaviour with staging

naydav · 2017-12-08T17:50:42Z

...Magento/InventoryCatalog/Model/Command/UpdateLegacyCatalogInventoryStockItemByPlainQuery.php

+            $this->resourceConnection->getTableName('cataloginventory_stock_item'),
+            [
+                StockItemInterface::QTY => new \Zend_Db_Expr(
+                    sprintf('%s%s', StockItemInterface::QTY, $reservation->getQuantity())


Looks like we have logic error here
Maybe wright code is %s + %s

-- slight refactoring

naydav · 2017-12-08T18:11:50Z

app/code/Magento/InventoryCatalog/etc/module.xml

@@ -8,7 +8,7 @@
 <config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:Module/etc/module.xsd">
    <module name="Magento_InventoryCatalog" setup_version="2.0.0">
        <sequence>
-            <module name="InventorySales"/>
+            <module name="Magento_InventoryAPI"/>


Looks like is mistake
In this config we need to describe sequence but not dependency

-- slight refactoring

Migrate Single Stock Data From CatalogInventory to MSI SourceItems

e709586

bartekszymanski added the WIP label Nov 9, 2017

bartekszymanski self-assigned this Nov 9, 2017

bartekszymanski added 7 commits November 14, 2017 09:39

Merge remote-tracking branch 'remotes/origin/develop' into migrate-si…

79a26c3

…ngle-stock-data

Migrate Single Stock Data From CatalogInventory to MSI SourceItems - …

c390c2b

…Add data to default source (Data Install), split InstallData to processors

Migrate Single Stock Data From CatalogInventory to MSI SourceItems

d85d7a6

Merge branch 'install-data-code-quality' into migrate-single-stock-data

86f3ae9

# Conflicts: # app/code/Magento/InventoryCatalog/Setup/InstallData.php

Migrate Single Stock Data From CatalogInventory to MSI SourceItems

a371305

misspelling in namespace

cc3e005

misspelling in namespace

b19fcdb

naydav reviewed Nov 23, 2017

View reviewed changes

Merge remote-tracking branch 'origin/develop' into migrate-single-sto…

f33d780

…ck-data # Conflicts: # app/code/Magento/InventoryCatalog/etc/di.xml

naydav reviewed Nov 23, 2017

View reviewed changes

maghamed suggested changes Nov 23, 2017

View reviewed changes

maghamed reviewed Nov 23, 2017

View reviewed changes

naydav added the Award: special achievement label Nov 23, 2017

naydav mentioned this pull request Nov 23, 2017

MSI integration write #164

Closed

4 tasks

bartekszymanski added 9 commits November 24, 2017 11:17

Migrate Single Stock Data Fix from CR

8b0631f

change single delete to mass delete on source_items_rollback

03c1894

and webiste 0 to plain sql, format, remove unused plugin

210ede2

add strict types

c870ecb

legacy update

21c9661

add dependency in InventorySales on InventoryCatalog

92f6f15

add dependency in InventorySales on InventoryCatalog

e3560f2

Merge remote-tracking branch 'remotes/origin/develop' into migrate-si…

6765e91

…ngle-stock-data

Source Item Indexer Test

fe12129

bartekszymanski added 2 commits December 8, 2017 14:06

plain sql change

e8854f4

plain sql change

84caf5e

maghamed suggested changes Dec 8, 2017

View reviewed changes

CR

cd6c564

naydav reviewed Dec 8, 2017

View reviewed changes

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

c56a3c3

naydav reviewed Dec 8, 2017

View reviewed changes

Valeriy Nayda added 2 commits December 8, 2017 20:08

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

9f5b2bb

-- slight refactoring

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

4afe202

-- slight refactoring

naydav reviewed Dec 8, 2017

View reviewed changes

Valeriy Nayda and others added 15 commits December 8, 2017 20:40

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

59820d7

-- slight refactoring

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

b7fe73d

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

ef4ec1f

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

6c96461

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

f47b058

-- slight refactoring

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

0fb47e6

-- slight refactoring

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

9d1dd8b

-- slight refactoring

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

baa2d7d

-- slight refactoring

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

704020a

-- slight refactoring

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

85fc59e

-- slight refactoring

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

1bd31b1

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

6fa12c0

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

61de984

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

e2465cb

MSI: Migrate Single Stock Data From CatalogInventory to MSI SourceItems

3e6bae0

naydav merged commit befb126 into develop Dec 11, 2017

naydav mentioned this pull request Dec 11, 2017

Migrate Single Stock Data From CatalogInventory to MSI SourceItems #178

Closed

vkublytskyi added the Award: test coverage label Dec 14, 2017

maghamed added the Award: advanced label Dec 18, 2017

maghamed deleted the migrate-single-stock-data branch February 5, 2019 17:16

Migrate Single Stock Data From CatalogInventory to MSI SourceItems #198

Migrate Single Stock Data From CatalogInventory to MSI SourceItems #198

Conversation

bartekszymanski commented Nov 9, 2017 • edited Loading

Description

Fixed Issues (if relevant)

Manual testing scenarios

Contribution checklist

naydav Nov 23, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naydav Nov 23, 2017 • edited by maghamed Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

naydav Nov 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

naydav Nov 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

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

naydav Dec 9, 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

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

naydav Dec 8, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bartekszymanski commented Nov 9, 2017 •

edited

Loading

naydav Nov 23, 2017 •

edited

Loading

naydav Nov 23, 2017 •

edited by maghamed

Loading

naydav Nov 23, 2017 •

edited

Loading

naydav Nov 23, 2017 •

edited

Loading

naydav Dec 9, 2017 •

edited

Loading

naydav Dec 8, 2017 •

edited

Loading