Various fixes in Magento 2 core to support MSI #13434

vrann · 2018-01-31T03:30:59Z

Set of changes in the Magento core used by MSI

Description

add Delete Button UI Component
add stock item importer interface and injected to importing logic
added RegisterProductSaleInterface, RevertProductSaleInterface and implementation for them
added possibility to run integration and api-functional tests from the module directory
added possibility to generate and provide sample file for Import
added Multi-Dimensional Indexer Framework
fixed order of applying of data fixtures in integration/api-functional
added ability to change white list file path Magento/Test/Php/LiveCodeTest::getWhitelist()
added Magento/Test/Php/_files/blacklist/strict_type.txt for strict type test.

Manual testing scenarios

Covered by automated tests

Bamboo Tests:

l2 - https://bamboo.corp.magento.com/browse/MCCE23-L2431/latest;
l3 -https://bamboo.corp.magento.com/browse/MCCE23-L3404/latest;
l4 -https://bamboo.corp.magento.com/browse/MCCE23-L4408/latest;
Performance Acceptance Test - https://bamboo.corp.magento.com/browse/MCCE23-PAT367/latest;
Semantic Version Checker https://bamboo.corp.magento.com/browse/MCCE23-SDT393/latest.

Jenkins:

https://m2build.devops.magento.com/job/Public-Pull-Request/206/ .

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)

ihor-sviziev · 2018-01-31T06:41:13Z

app/code/Magento/Backend/Ui/Component/Control/DeleteButton.php

+                'label' => __('Delete'),
+                'class' => 'delete',
+                'on_click' =>
+                    "deleteConfirm('{$this->confirmationMessage}', '{$url}', {data:{{$this->idFieldName}:{$id}}})",


Shouldn't these variables to be escaped?

@ihor-sviziev escaped confirmationMessage.

maghamed

We can move to Lib Predefined ID trait which we use in MSI

maghamed · 2018-01-31T15:58:16Z

.../integration/testsuite/Magento/CatalogInventory/Model/StockItemSave/StockItemDataChecker.php

 use Magento\Framework\EntityManager\HydratorInterface;
+use Magento\InventoryApi\Api\Data\SourceItemInterface;
+use Magento\InventoryApi\Api\SourceItemRepositoryInterface;


dependency on MSI in test

@maghamed removed.

maghamed · 2018-01-31T15:59:39Z

lib/internal/Magento/Framework/MultiDimensionalIndexer/IndexNameResolver.php

+class IndexNameResolver implements IndexNameResolverInterface
+{
+    /**
+     * TODO: move to separate configurable interface (https://github.com/magento-engcom/msi/issues/213)


todo should be removed

Consider not removing but fixing todo

@maghamed removed

maghamed · 2018-01-31T15:59:54Z

lib/internal/Magento/Framework/MultiDimensionalIndexer/IndexNameResolver.php

+    }
+
+    /**
+     * TODO: move to separate configurable interface (https://github.com/magento-engcom/msi/issues/213)


todo should be removed

@maghamed removed

maghamed · 2018-01-31T16:00:04Z

lib/internal/Magento/Framework/MultiDimensionalIndexer/IndexTableSwitcher.php

+    private $replicaTableSuffix = '_replica';
+
+    /**
+     * TODO: move to separate configurable interface (https://github.com/magento-engcom/msi/issues/213)


todo should be removed

@maghamed removed

naydav · 2018-01-31T15:57:35Z

dev/tests/integration/phpunit.xml.dist

@@ -21,8 +21,12 @@
        </testsuite>
        <testsuite name="Magento Integration Tests">
            <directory suffix="Test.php">testsuite</directory>
+            <directory suffix="Test.php">../../../app/code/*/*/Test/Integration</directory>


Need to remove all mentions about MSI from phpunit.xml.dist (the same for WebApi)

@naydav but there is no mentioning of MSI in this test configuration

@naydav removed

naydav · 2018-01-31T15:58:24Z

.../integration/testsuite/Magento/CatalogInventory/Model/StockItemSave/StockItemDataChecker.php

@@ -74,6 +103,7 @@ public function checkStockItemData($sku, array $expectedData)
        $productLoadedByModel = $this->productFactory->create();
        $productLoadedByModel->load($product->getId());
        $this->doCheckStockItemData($product, $expectedData);
+        $this->checkIntegrityWithInventory($product, $expectedData);


Need to remove this check
This is related only to MSI project

@naydav

this check fully relies on Legacy CatalogInventory

/** * @param Product $product * @param array $expectedData * @return void */ private function checkIntegrityWithInventory(Product $product, array $expectedData) { $searchCriteria = $this->searchCriteriaBuilder ->addFilter(SourceItemInterface::SOURCE_CODE, $this->defaultSourceProvider->getCode()) ->addFilter(SourceItemInterface::SKU, $product->getSku()) ->create(); $sourceItems = $this->sourceItemRepository->getList($searchCriteria)->getItems(); Assert::assertCount(1, $sourceItems); $sourceItem = reset($sourceItems); Assert::assertEquals( $expectedData[StockItemInterface::QTY], $sourceItem->getQuantity() ); Assert::assertEquals( $expectedData[StockItemInterface::IS_IN_STOCK], (int)$sourceItem->getStatus() ); }

there are no problems to have it in CE

@maghamed $this->sourceItemRepository is instance of Magento\InventoryApi\Api\SourceItemRepositoryInterface.

@naydav removed

naydav · 2018-01-31T16:02:15Z

dev/tests/integration/framework/Magento/TestFramework/Annotation/DataFixture.php

@@ -203,7 +203,8 @@ protected function _applyFixtures(array $fixtures)
     */
    protected function _revertFixtures()
    {
-        foreach ($this->_appliedFixtures as $fixture) {
+        $appliedFixtures = array_reverse($this->_appliedFixtures);


Need to do same for WebApi framework

@naydav done

maghamed · 2018-01-31T16:02:27Z

lib/internal/Magento/Framework/MultiDimensionalIndexer/README.md

@@ -0,0 +1 @@
+MultiDimensionalIndexer


it's better to provide more descriptive read me

maghamed · 2018-01-31T16:15:43Z

app/code/Magento/CatalogImportExport/Model/StockItemImporterInterface.php

+ *
+ * @api
+ */
+interface StockItemImporterInterface


there is no default implementation of this interface in CE, just in MSI

that will lead to Fatal Error

magento/inventory#496

ishakhsuvarov · 2018-01-31T17:48:16Z

@vrann @maghamed @naydav I strongly suggest to try and keep information on original history from the MSI repository, possible cherry-picking related commits, or, at least, giving credits to the original authors in the commit message to this change set.

Additionally, provided description is not clear on what was changed in the scope of this PR, making it hard to identify and test components affected by this commit.

ishakhsuvarov

Please update Pull Request description to describe the affected components and possible impact.

ishakhsuvarov · 2018-03-13T10:20:55Z

app/code/Magento/ImportExport/Controller/Adminhtml/Import/Download.php

    /**
     * Constructor
     *
     * @param \Magento\Backend\App\Action\Context $context
     * @param \Magento\Framework\App\Response\Http\FileFactory $fileFactory
     * @param \Magento\Framework\Controller\Result\RawFactory $resultRawFactory
     * @param \Magento\Framework\Filesystem\Directory\ReadFactory $readFactory
+     * @param \Magento\ImportExport\Model\Import\SampleFileProvider $sampleFileProvider


Incorrect order of params in docBlock

@ishakhsuvarov fixed

In constructor signature SampleFileProvider is after ComponentRegistrar

ishakhsuvarov · 2018-03-13T10:22:49Z

app/code/Magento/Store/Setup/InstallSchema.php

@@ -11,6 +11,7 @@
 use Magento\Framework\Setup\InstallSchemaInterface;


InstallSchema is obsolete. Please update with latest mainline and use declarative schema

ishakhsuvarov · 2018-03-13T10:23:53Z

dev/tests/api-functional/phpunit_rest.xml.dist

@@ -19,6 +19,10 @@
        <testsuite name="Magento REST web API functional tests">
            <directory suffix="Test.php">testsuite</directory>
            <exclude>testsuite/Magento/GraphQl</exclude>
+            <directory suffix="Test.php">../../../app/code/*/*/Test/Api</directory>
+        </testsuite>
+        <testsuite name="msi">


Looks like this was only needed to execute tests on MSI fork

@ishakhsuvarov removed

Where is it removed?

ishakhsuvarov · 2018-03-13T10:27:03Z

dev/tests/static/testsuite/Magento/Test/Php/LiveCodeTest.php

     * @return array
     */
-    public static function getWhitelist($fileTypes = ['php'], $changedFilesBaseDir = '', $baseFilesFolder = '')
-    {
+    public static function getWhitelist(


Updates to this test are not reflected in PR description.
Please update with relevant information

ishakhsuvarov · 2018-03-13T10:28:00Z

dev/tests/static/testsuite/Magento/Test/Php/LiveCodeTest.php

+    /**
+     * Tests whitelisted files for strict type declarations.
+     */
+    public function testStrictTypes()


This may be implemented consistently with other static tests using PHPCS

ishakhsuvarov · 2018-03-13T10:29:17Z

dev/tests/static/testsuite/Magento/Test/Php/_files/blacklist/strict_type.txt

@@ -0,0 +1 @@
+# Format: <componentType=module|library|theme|language|*> <componentName> <globPattern> or simply <globPattern>


Please reflect these changes in PR description

ishakhsuvarov · 2018-03-13T10:29:42Z

dev/tests/static/testsuite/Magento/Test/Php/_files/whitelist/strict_type.txt

@@ -0,0 +1 @@
+# Format: <componentType=module|library|theme|language|*> <componentName> <globPattern> or simply <globPattern>


Is it required to have blacklist and whitelist at the same time?

@ishakhsuvarov blacklist removed

@ishakhsuvarov, blacklist returned. Yes it is required.

There is no value in delivering test which has empty whitelist. I assume it would not test anything.

Blacklist deleted.

ishakhsuvarov · 2018-03-13T10:30:40Z

dev/travis/before_script.sh

@@ -71,7 +71,7 @@ case $TEST_SUITE in
            --output-file="$changed_files_ce" \
            --base-path="$TRAVIS_BUILD_DIR" \
            --repo='https://github.com/magento/magento2.git' \
-            --branch="$TRAVIS_BRANCH"
+            --branch='$TRAVIS_BRANCH'


This change only seems to bring more inconsistency to this script

ishakhsuvarov · 2018-03-13T10:39:12Z

lib/internal/Magento/Framework/MultiDimensionalIndexer/IndexStructureInterface.php

+     * @param string $connectionName
+     * @return bool
+     */
+    public function isExist(IndexName $indexName, string $connectionName): bool;


Is there any obstacle in making create and delete idempotent eliminating this method?

…nges-no-history # Conflicts: # app/code/Magento/CatalogImportExport/Model/Import/Product.php # app/code/Magento/Store/Setup/InstallSchema.php # app/code/Magento/Ui/view/base/web/js/dynamic-rows/dynamic-rows.js # app/etc/di.xml # composer.lock

…nges-no-history # Conflicts: # app/code/Magento/Store/Setup/Patch/Schema/InitializeStoresAndWebsites.php # app/etc/di.xml

ishakhsuvarov · 2018-03-14T08:14:28Z

@p-bystritsky Please avoid commit messages like Some fixes

ishakhsuvarov · 2018-03-14T12:08:33Z

app/code/Magento/Backend/Ui/Component/Control/DeleteButton.php

+    /**
+     * {@inheritdoc}
+     */
+    public function getButtonData()


This class seems to be unused in the current implementation.
I would suggest delivering it with the code which actually uses it and provides functional test coverage or with unit/integration test which would ensure that it is not broken in the future.

naydav · 2018-03-15T22:15:33Z

app/code/Magento/ImportExport/Model/Import/SampleFileProvider.php

+    private function getModuleName(string $entityName): string
+    {
+        if (!isset($this->samples[$entityName])) {
+            throw new NoSuchEntityException();


Why do we use NoSuchEntityException for missed array key?
Maybe do we need to use something like \Magento\Framework\Exception\InputException?

naydav · 2018-03-15T22:18:51Z

app/code/Magento/ImportExport/Model/Import/SampleFileProvider.php

+     */
+    private function getPath(string $entityName): string
+    {
+        $directoryRead = $this->getDirectoryRead($entityName);


Underhood of this method, we create the directory, but in getModuleName(next call) we can throw the exception
So maybe do we need to finish all of the checks before performing some actions?

mslabko · 2018-03-16T17:03:14Z

lib/internal/Magento/Framework/MultiDimensionalIndexer/IndexHandlerInterface.php

+     * @param string $connectionName
+     * @return void
+     */
+    public function saveIndex(IndexName $indexName, \Traversable $documents, string $connectionName);


What the purpose of using connectionName as an argument for index handler?
Why this is not a responsibility of index class?

mslabko · 2018-03-16T17:08:20Z

lib/internal/Magento/Framework/MultiDimensionalIndexer/IndexHandlerInterface.php

+     * @param string $connectionName
+     * @return void
+     */
+    public function cleanIndex(IndexName $indexName, \Traversable $documents, string $connectionName);


I see, that you use indexName with pre-build information about current dimension - it forse us to build it in client code. But we can pass DImensionInterface (specific for concrete index) which will have all required information for build index table name + the same approach can be use for resolve table name on front

Note, this approach we use in draft implementation: https://github.com/magento-performance/magento2ce/blob/eac69331ff5208ab28ab5ad10db340c3d9674d8a/app/code/Magento/Catalog/Model/Indexer/Product/Price/Shard/ShardTableNameResolver.php#L20https://github.com/magento-performance/magento2ce/blob/eac69331ff5208ab28ab5ad10db340c3d9674d8a/app/code/Magento/Catalog/Model/Indexer/Product/Price/Shard/ShardTableNameResolver.php#L20

Shard - this is a presentation of specific dimensions (sevaral dimenstion can be used, e.g. for Price Indexer)

…ock_item table

magento-cicd2 · 2018-03-17T12:09:01Z

Thank you for your submission, we really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
4 out of 5 committers have signed the CLA.

✅ vrann
✅ p-bystritsky
✅ RomaKis
✅ vkublytskyi
❌ maghamed

…nges-no-history # Conflicts: # composer.lock

…o msi-core-changes-no-history

…E_ID into the db_schema_whitelist.json as it should always exists even if we don't have index anymore

…nges-no-history

…o msi-core-changes-no-history

…nges-no-history # Conflicts: # composer.lock

…nges-no-history

…nly for added files.

…nges-no-history

MSI: update Magento 2 core to support MSI

992bb67

ihor-sviziev reviewed Jan 31, 2018

View reviewed changes

maghamed suggested changes Jan 31, 2018

View reviewed changes

naydav reviewed Jan 31, 2018

View reviewed changes

maghamed suggested changes Jan 31, 2018

View reviewed changes

ishakhsuvarov previously requested changes Jan 31, 2018

View reviewed changes

maghamed mentioned this pull request Feb 2, 2018

Provide StockImporter implementation for Magento CE magento/inventory#496

Closed

p-bystritsky assigned p-bystritsky and vrann Mar 12, 2018

ishakhsuvarov reviewed Mar 13, 2018

View reviewed changes

p-bystritsky and others added 8 commits March 13, 2018 15:09

Some fixes

420afc8

MSI: 496: Provide StockImporter implementation for Magento CE

13efce8

MSI: Update Magento 2 core to support MSI

0067244

Merge remote-tracking branch 'magento2/2.3-develop' into msi-core-cha…

ad91424

…nges-no-history # Conflicts: # app/code/Magento/Store/Setup/Patch/Schema/InitializeStoresAndWebsites.php # app/etc/di.xml

MSI: Update Magento 2 core to support MSI

1b8cc6a

MSI: Update Magento 2 core to support MSI

36098f4

MSI: Update Magento 2 core to support MSI

f016278

p-bystritsky removed their assignment Mar 14, 2018

ishakhsuvarov reviewed Mar 14, 2018

View reviewed changes

naydav reviewed Mar 15, 2018

View reviewed changes

RomaKis added 2 commits March 16, 2018 11:54

MSI: Update Magento 2 core to support MSI

c084214

MSI: Update Magento 2 core to support MSI

3a2533b

mslabko reviewed Mar 16, 2018

View reviewed changes

Remove unique key for (product_id, website_id) on cataloginventory_st…

6a3835f

…ock_item table

RomaKis and others added 12 commits March 19, 2018 13:25

Merge remote-tracking branch 'magento2/2.3-develop' into msi-core-cha…

0a49140

…nges-no-history # Conflicts: # composer.lock

MSI: Update Magento 2 core to support MSI

aba9b73

Merge remote-tracking branch 'origin/msi-core-changes-no-history' int…

7fc59e7

…o msi-core-changes-no-history

get back declaration of CATALOGINVENTORY_STOCK_ITEM_PRODUCT_ID_WEBSIT…

661541a

…E_ID into the db_schema_whitelist.json as it should always exists even if we don't have index anymore

MSI: Update Magento 2 core to support MSI

bb6bac3

Merge remote-tracking branch 'magento2/2.3-develop' into msi-core-cha…

6f22a17

…nges-no-history

Merge remote-tracking branch 'origin/msi-core-changes-no-history' int…

e4eab13

…o msi-core-changes-no-history

MSI: Update Magento 2 core to support MSI

28b07ed

Merge remote-tracking branch 'magento2/2.3-develop' into msi-core-cha…

a509c71

…nges-no-history # Conflicts: # composer.lock

MSI: Update Magento 2 core to support MSI

be29aa7

Merge remote-tracking branch 'mainline/2.3-develop' into msi-core-cha…

aa365e0

…nges-no-history

magento/magento2#13434: Fix strict type checking for legacy code

af7737c

magento/magento2#13434: Strict type static test should be preformed o…

e949bdb

…nly for added files.

vkublytskyi force-pushed the msi-core-changes-no-history branch from c114abd to e949bdb Compare March 27, 2018 09:15

Remove Support of PHP 7.0.* on Travis Buils

0dceea8

maghamed approved these changes Mar 27, 2018

View reviewed changes

magento-engcom-team assigned maghamed Mar 27, 2018

magento-engcom-team added this to the March 2018 milestone Mar 27, 2018

magento-engcom-team added the Release Line: 2.3 label Mar 27, 2018

Merge remote-tracking branch 'mainline/2.3-develop' into msi-core-cha…

d62db82

…nges-no-history

magento-engcom-team merged commit d62db82 into magento:2.3-develop Mar 28, 2018

vkublytskyi deleted the msi-core-changes-no-history branch April 26, 2018 09:19

		@@ -11,6 +11,7 @@
		use Magento\Framework\Setup\InstallSchemaInterface;

		@@ -0,0 +1 @@
		# Format: <componentType=module\|library\|theme\|language\|*> <componentName> <globPattern> or simply <globPattern>

		@@ -0,0 +1 @@
		MultiDimensionalIndexer

Various fixes in Magento 2 core to support MSI #13434

Various fixes in Magento 2 core to support MSI #13434

Uh oh!

Conversation

vrann commented Jan 31, 2018 • edited by RomaKis Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description

Manual testing scenarios

Bamboo Tests:

Jenkins:

Contribution checklist

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

maghamed left a comment

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!

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!

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!

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!

ishakhsuvarov commented Jan 31, 2018

Uh oh!

ishakhsuvarov left a comment • 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!

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

vrann commented Jan 31, 2018 •

edited by RomaKis

Loading

ishakhsuvarov left a comment •

edited

Loading