MSI-1890: Elasticsearch don't work with Custom stock #1943

IvanPletnyov · 2018-12-14T14:03:19Z

Fixed Issues (if relevant)

Elasticsearch don't work with Custom stock #1890: Elasticsearch don't work with Custom stock

Contribution checklist

Pull request has a meaningful description of its purpose
All commits are accompanied by meaningful commit messages
All new or changed code is covered with unit/integration tests (if applicable)
All automated tests passed successfully (all builds on Travis CI are green)

maghamed · 2018-12-17T15:20:31Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+                    $select->where('product.entity_id IN (?)', $productIds);
+                    $productsSalable = $connection->fetchAssoc($select);
+                    $productsSalable = array_filter($productsSalable, function ($productSalable) {
+                        return StockStatusInterface::STATUS_IN_STOCK == $productSalable['is_salable'];


here we are working with Inventory indexes table structure, no need to use deprecated constant from CatalogInventory module

@maghamed I've deleted array filtration.

maghamed · 2018-12-17T15:24:17Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+
+            foreach ($this->getProductIdsByWebsiteIds($productIds) as $websiteId => $productIds) {
+                $stock = $this->stockByWebsiteIdResolver->execute($websiteId);
+                $stockTable = $this->stockIndexTableNameResolver->execute((int)$stock->getStockId());


looks like we have to provide conditional logic for Default Stock similar to this PR
#1937

to prevent performance degradation on Default Stock

maghamed · 2018-12-17T15:26:54Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+            ['website_id', 'product_id']
+        )->where('product_in_websites.product_id IN (?)', $entityIds)->distinct();
+        foreach ($connection->fetchAll($select) as $websiteData) {
+            $result[(int)$websiteData['website_id']][] = (int)$websiteData['product_id'];


probably it's more performance efficient to use GROUP BY + GROUP CONCAT on DB level

maghamed · 2018-12-17T15:28:22Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+                    $select->where('product.entity_id IN (?)', $productIds);
+                    $productsSalable = $connection->fetchAssoc($select);
+                    $productsSalable = array_filter($productsSalable, function ($productSalable) {
+                        return StockStatusInterface::STATUS_IN_STOCK == $productSalable['is_salable'];


always use === for comparison

why not use filtering as a part of MySQL query?
why do you filter by in stock in application?

maghamed · 2018-12-17T15:28:35Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+                    );
+                    $select->joinInner(
+                        ['stock' => $stockTable],
+                        "product.sku = stock.sku",


use single quotes

maghamed · 2018-12-17T17:16:36Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+                $stock = $this->stockByWebsiteIdResolver->execute($websiteId);
+                $stockTable = $this->stockIndexTableNameResolver->execute((int)$stock->getStockId());
+
+                if ($this->resourceConnection->getConnection()->isTableExists($stockTable)) {


No need to use this IF clause

if ($this->resourceConnection->getConnection()->isTableExists($stockTable)) {

maghamed · 2018-12-17T17:20:14Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+                    $select->where('product.entity_id IN (?)', $productIds);
+                    $productsSalable = $connection->fetchAssoc($select);
+                    $productsSalable = array_filter($productsSalable, function ($productSalable) {
+                        return StockStatusInterface::STATUS_IN_STOCK == $productSalable['is_salable'];


why not use filtering as a part of MySQL query?
why do you filter by in stock in application?

maghamed · 2018-12-17T17:25:02Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+     * @param array $entityIds
+     * @return array
+     */
+    private function getProductIdsByWebsiteIds(array $entityIds): array


why do we need this logic at all? as customer always run a query in the scope of one website

one and only

@maghamed Save product in admin panel or run reindex will be on admin scope

but you are still supposed to execute business logic in a single scope

so that this method looks excessive

…iew)

maghamed · 2018-12-20T11:07:38Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+        DataProvider $dataProvider,
+        array $indexData,
+        array $productData,
+        int $storeId


these are backward incompatible changes, as you are introducing strict typing for NON-strict typed contract

/** * Prepare Fulltext index value for product * * @param array $indexData * @param array $productData * @param int $storeId * @return array * @SuppressWarnings(PHPMD.CyclomaticComplexity) * @since 100.0.3 */ public function prepareProductIndex($indexData, $productData, $storeId)

maghamed · 2018-12-20T11:08:52Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+    }
+
+    /**
+     * Filter out of stock options for configurable product.


please remove "of"
and just leave
Filter out stock options for configurable product.

maghamed · 2018-12-20T11:32:11Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+            $productIds = array_keys($indexData);
+            $stockStatuses = [];
+
+            foreach ($this->getProductIdsByWebsiteIds($productIds) as $websiteId => $productIdsStr) {


we have to resolve Website by $storeId and then by Website (which in MSI plays a role of Sales Channel) we could resolve corresponding Stock

Thus, for $stockId you would have ONE and ONLY Stock

maghamed · 2018-12-20T11:36:32Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+
+            foreach ($this->getProductIdsByWebsiteIds($productIds) as $websiteId => $productIdsStr) {
+                $stock = $this->stockByWebsiteIdResolver->execute($websiteId);
+                $stockId = (int)$stock->getStockId();


why do you use type casting if we already use strict typing in out interfaces?

/** * Get stock id * * @return int|null */ public function getStockId(): ?int;

maghamed · 2018-12-20T11:37:16Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+     * @param array $entityIds
+     * @return array
+     */
+    private function getProductIdsByWebsiteIds(array $entityIds): array


but you are still supposed to execute business logic in a single scope

maghamed · 2018-12-20T11:37:36Z

...ogSearch/Model/Indexer/Fulltext/Action/DataProvider/StockedProductFilterByInventoryStock.php

+     * @param array $entityIds
+     * @return array
+     */
+    private function getProductIdsByWebsiteIds(array $entityIds): array


so that this method looks excessive

…iew)

maghamed · 2018-12-20T16:49:50Z

Jenkins User Request Build:
https://m2build-ur.devops.magento.com/job/All-User-Requested-Tests/10749/

Basic Green UR:
https://m2build-ur.devops.magento.com/job/All-User-Requested-Tests/10750/

PR
https://m2build.devops.magento.com/job/Existing-PR-Tests/10493/

… test \Magento\Elasticsearch\Model\Indexer\IndexHandlerTest)

maghamed · 2018-12-21T13:14:03Z

Acceptance:
https://m2build-ur.devops.magento.com/job/All-User-Requested-Tests/10805/

Basic green:
https://m2build-ur.devops.magento.com/job/All-User-Requested-Tests/10807/

PR
https://m2build.devops.magento.com/job/Existing-PR-Tests/10544/

IvanPletnyov added 2 commits December 14, 2018 16:01

MSI-1890: Elasticsearch don't work with Custom stock

93fa3c9

MSI-1890: Elasticsearch don't work with Custom stock (fix static test)

ccc9d3a

maghamed suggested changes Dec 17, 2018

View reviewed changes

maghamed reviewed Dec 17, 2018

View reviewed changes

IvanPletnyov added 2 commits December 19, 2018 14:36

MSI-1890: Elasticsearch don't work with Custom stock (refactor by rev…

9641e03

…iew)

MSI-1890: Elasticsearch don't work with Custom stock (refactor by rev…

8214fee

…iew)

maghamed suggested changes Dec 20, 2018

View reviewed changes

IvanPletnyov added 2 commits December 20, 2018 16:55

MSI-1890: Elasticsearch don't work with Custom stock (refactor by rev…

709712e

…iew)

MSI-1890: Elasticsearch don't work with Custom stock (refactor by rev…

32c68e4

…iew)

maghamed approved these changes Dec 20, 2018

View reviewed changes

Merge branch '2.3-develop' into MSI-1890

761edd4

MSI-1890: Elasticsearch don't work with Custom stock (fix integration…

81ab1bc

… test \Magento\Elasticsearch\Model\Indexer\IndexHandlerTest)

maghamed added 2 commits December 21, 2018 16:06

MSI-1890: Fix Static

95faeb2

MSI-1890: add readme

cab9d9e

maghamed merged commit 673e85c into 2.3-develop Dec 21, 2018

ishakhsuvarov deleted the MSI-1890 branch June 19, 2019 16:34

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

MSI-1890: Elasticsearch don't work with Custom stock #1943

MSI-1890: Elasticsearch don't work with Custom stock #1943

IvanPletnyov commented Dec 14, 2018

maghamed Dec 17, 2018

IvanPletnyov Dec 19, 2018

maghamed Dec 17, 2018

maghamed Dec 17, 2018

maghamed Dec 17, 2018

maghamed Dec 17, 2018

maghamed Dec 17, 2018

maghamed Dec 17, 2018

maghamed Dec 17, 2018

maghamed Dec 17, 2018

IvanPletnyov Dec 19, 2018

maghamed Dec 20, 2018

maghamed Dec 20, 2018

maghamed Dec 20, 2018

maghamed Dec 20, 2018

maghamed Dec 20, 2018

maghamed Dec 20, 2018

maghamed Dec 20, 2018

maghamed Dec 20, 2018

maghamed commented Dec 20, 2018

maghamed commented Dec 21, 2018

MSI-1890: Elasticsearch don't work with Custom stock #1943

MSI-1890: Elasticsearch don't work with Custom stock #1943

Conversation

IvanPletnyov commented Dec 14, 2018

Fixed Issues (if relevant)

Contribution checklist

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

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 commented Dec 20, 2018

maghamed commented Dec 21, 2018