fix stock item configuration by seruymt · Pull Request #1353 · magento/inventory

seruymt · 2018-06-11T07:55:05Z

depends on #1236

Incorrect quantity deduction after shipment with mixed product types created. #1396

maghamed · 2018-06-12T20:08:13Z

-            // Sku is not assigned to Stock
-            return null;
-        }
-


looks like it would be great to throw NoSuchEntityException exception instead of removed code. It would be more reliable for a code of business logic.
But for doing this we need to introduce one more service isProductAssignedToStock::execute($sku, $stockId) to InventoryApi.

Because currently we create and return configuration with default values for a product which is actually not belonging to specified Stock

maghamed · 2018-06-12T20:09:53Z

+        $stockItemData = $this->getStockItemData->execute($sku, $stockId);
+        if (null === $stockItemData) {
+            // Sku is not assigned to Stock
+            return false;


good candidate for usage of isProductAssignedToStock::execute($sku, $stockId)

maghamed · 2018-06-12T20:12:19Z

        }

-        // iterate over the required conditions: return error as soon as a condition fails
+        $stockItemData = $this->getStockItemData->execute($sku, $stockId);


another candidate for isProductAssignedToStock::execute($sku, $stockId) usage

maghamed · 2018-06-12T20:16:56Z

+        $requiredConditionsErrors = array_merge(...$requiredConditionsErrors);
+        if (count($requiredConditionsErrors)) {
+            return $this->productSalableResultFactory->create(['errors' => $requiredConditionsErrors]);
+        }


+ $requiredConditionsErrors = array_merge(...$requiredConditionsErrors); + if (count($requiredConditionsErrors)) { + return $this->productSalableResultFactory->create(['errors' => $requiredConditionsErrors]); + }

this looks like a code which could be extracted to separate private method

maghamed · 2018-06-12T20:21:32Z

            );

-            if ($stockItemConfiguration === null || $stockItemConfiguration->isManageStock()) {
+            if ($stockItemConfiguration->isManageStock()) {


this is not correct fix, because of condition $stockItemConfiguration === null was added to support Order processing for deleted products from Catalog.

and condition $stockItemConfiguration === null checks that product does not exists anymore

isProductAssignedToStock::execute($sku, $stockId) should be used here as well, but Exception should be caught here and Reservation should be appended anyway

maghamed · 2018-06-12T20:23:48Z


-            if ($stockItemConfiguration === null || !$stockItemConfiguration->isManageStock()) {
-                //Product not assigned to Given Stock or we No need to Manage Stock
+            if (!$stockItemConfiguration->isManageStock()) {


same as above

…types created

naydav · 2018-06-21T11:22:30Z

            $productId = $this->getProductIdsBySkus->execute([$sku])[$sku];
        } catch (NoSuchEntityException $skuNotFoundInCatalog) {
-            $stockItem = \Magento\Framework\App\ObjectManager::getInstance()->create(StockItemInterface::class);
+            $stockItem = ObjectManager::getInstance()->create(StockItemInterface::class);


Why do we use ObjectManager manager directly instead of construct dependency? Or is it a temporal solution?

naydav · 2018-06-21T11:23:36Z

+ *
+ * @api
+ */
+interface IsSourceItemManagementAllowedForSkuInterface


I am not sure that we need to introduce this interface

naydav · 2018-06-21T11:24:59Z

+                    return true;
+                }
            }
+        } catch (NoSuchEntityException $e) {


Looks like we need to move this logic in some of a condition instead of applying it under all of the conditions

naydav · 2018-06-21T11:27:11Z

+            return $this->productSalableResultFactory->create(['errors' => $requiredConditionsErrors]);
+        }
+
+        $sufficientConditionsErrors = $this->processSufficientConditions($sku, $stockId, $requestedQty);


Not bad to describe in dockblock
What is a difference between requiredConditions and sufficientConditions

naydav · 2018-06-21T11:43:36Z


-            if ($stockItemConfiguration === null || !$stockItemConfiguration->isManageStock()) {
-                //Product not assigned to Given Stock or we No need to Manage Stock
+            try {


Generally, we have a lot of boilerplate code like

try { + $stockItemConfig = $this->getStockItemConfiguration->execute($sku, $stockId); + } catch (NoSuchEntityException $e) { + // GetStockItemConfiguration throw NoSuchEntityException when SKU is not assigned to Stock return false; }

Maybe it will be better to return some $stockItemConfiguration with predefined values manage_stock=0 (like as Special Case pattern)
If some sku doesn't have configuration for some stock will be sensible use fallback on global configuration

In another words, move concept is sku assigned to stock our of get stock item configuration
And you should always wrap work this service in try...catch

naydav · 2018-06-21T11:51:56Z

+            }
+
+            if (!$stockItemConfiguration->isManageStock()) {
+                //We No need to Manage Stock


don't need to manage

naydav · 2018-06-21T11:54:40Z

-        if (null === $stockItemData) {
-            // Sku is not assigned to Stock
-            return null;
+        if ($this->defaultStockProvider->getId() !== $stockId


Why do we need $this->defaultStockProvider->getId() !== $stockId condition?

…gacyCatalogInventoryAtSourceItemsSavePlugin

…anagementAllowedForSku

fix stock item configuration

3c9892b

seruymt added the WIP label Jun 11, 2018

seruymt added this to the MSI Part I milestone Jun 11, 2018

seruymt self-assigned this Jun 11, 2018

seruymt requested a review from maghamed June 11, 2018 07:55

maghamed suggested changes Jun 12, 2018

View reviewed changes

seruymt added 5 commits June 13, 2018 11:30

merge 2.3-develop and resolve conflicts

270040f

refactoring

8ee0f94

remove empty line

a1bb8b8

fix static test

d8d1997

throw exception only on non default stock

92b08f4

smoskaluk added the QA Passed label Jun 14, 2018

stock item configuration: refactoring

a7b4fad

seruymt force-pushed the fix-stock-item-configuration branch from 6054445 to a7b4fad Compare June 16, 2018 17:04

stock item configuration: fix tests

015fc13

maghamed added QA Failed and removed QA Passed labels Jun 17, 2018

seruymt added 5 commits June 17, 2018 19:29

stock item configuration: fix getItemsFromShipment

d5f61e4

stock item configuration: fix tests

77cea50

1396: Incorrect quantity deduction after shipment with mixed product …

a52bf4a

…types created

Merge branch '2.3-develop' into fix-stock-item-configuration

17890cc

skip test

12b4ce5

naydav reviewed Jun 21, 2018

View reviewed changes

maghamed added the Dare to Share label Jun 22, 2018

seruymt added 2 commits June 22, 2018 14:28

stock item configuration: refactoring

bcb0b50

stock item configuration: move getLegacyStockItem in separate class

bd1de50

smoskaluk added QA Passed and removed QA Failed QA Passed labels Jun 22, 2018

seruymt added 2 commits June 22, 2018 16:11

stock item configuration: fix error with product types in SetDataToLe…

b1f0b42

…gacyCatalogInventoryAtSourceItemsSavePlugin

stock item configuration: fix error wiwith product type IsSourceItemM…

7e3945b

…anagementAllowedForSku

maghamed removed the WIP label Jun 22, 2018

stock item configuration: introduce SkuIsNotAssignedToStockException

d7e070d

maghamed merged commit 90a0ccd into 2.3-develop Jun 26, 2018

magento-engcom-team added Award: advanced Award: special achievement Award: test coverage Award: bug fix labels Jun 26, 2018

ishakhsuvarov deleted the fix-stock-item-configuration branch June 19, 2019 16:34

Conversation

seruymt commented Jun 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naydav Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naydav Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naydav Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

naydav Jun 21, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

seruymt commented Jun 11, 2018 •

edited

Loading

naydav Jun 21, 2018 •

edited

Loading

naydav Jun 21, 2018 •

edited

Loading

naydav Jun 21, 2018 •

edited

Loading

naydav Jun 21, 2018 •

edited

Loading