Issue 1306 rename website breaks grid #1344

phoenix128 · 2018-06-09T10:36:15Z

Fixed Issues (if relevant)

Renaming website breaks MSI grid #1306: Renaming website breaks MSI grid

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)

If a website was renamed/removed, sales channels were still tying to search for it

…te code is changed See #1306

naydav · 2018-06-09T10:50:34Z

app/code/Magento/InventorySales/Plugin/Store/WebsiteResourcePlugin.php

+    private function getCodeFromDatabase(
+        \Magento\Store\Model\ResourceModel\Website $resourceModel,
+        int $websiteId
+    ) {


Pls,

use return type hinting

$qry is not clear name for the variable

Need to use $this->resource instead of $resourceModel->getConnection(),
generally we don't need dependency on ResourceModel\Website for obtaining website code

Also, need to think about extracting this logic to separate class

I used \Magento\Store\Model\ResourceModel\Website with intention because it can be potentially stored in a different database from the standard one.

naydav · 2018-06-09T10:54:05Z

app/code/Magento/InventorySales/Plugin/Store/WebsiteResourcePlugin.php

+        $tableName = $this->resource->getTableName('inventory_stock_sales_channel');
+        $connection->update($tableName, [
+            SalesChannelInterface::CODE => $newCode,
+        ], $connection->quoteInto(SalesChannelInterface::CODE . "=? and type='website'", $oldCode));


Use `\Magento\InventorySalesApi\Api\Data\SalesChannelInterface::TYPE_WEBSITE

Second parameter can be array with clauses instead of sql string

naydav · 2018-06-09T10:57:02Z

app/code/Magento/InventorySales/Plugin/Store/WebsiteResourcePlugin.php

+        if ($object->getId()) {
+            // Keep database consistency while updating the website code
+            // See https://github.com/magento-engcom/msi/issues/1306
+            $oldCode = $this->getCodeFromDatabase($subject, (int) $object->getId());


Bad interface of method

naydav · 2018-06-09T10:57:41Z

app/code/Magento/InventorySalesAdminUi/Ui/Component/Listing/Column/SalesChannels.php

+                        'code' => $code,
+                    ];
+                } catch (NoSuchEntityException $e) {
+                    // We must handle the scenario in which a previous website was removed/renamed


I'm not sure that is good idea to skip exception... especially if we fixed initial case

I agree, but this is the only way to fix the already done mistake.

maghamed · 2018-06-09T10:47:45Z

app/code/Magento/InventorySales/Plugin/Store/WebsiteResourcePlugin.php

+        int $websiteId
+    ) {
+        $connection = $resourceModel->getConnection();
+        $qry = $connection->select()->from($resourceModel->getMainTable(), 'code')->where('website_id = ?', $websiteId);


please don't use shortening in variable names
like $qry
it's better to have long variable names than shortening

maghamed · 2018-06-09T11:01:00Z

app/code/Magento/InventorySales/Plugin/Store/WebsiteResourcePlugin.php

+     * @param \Magento\Store\Model\ResourceModel\Website $subject
+     * @param callable $proceed
+     * @param \Magento\Store\Model\Website $object
+     * @return mixed


why do you have mixed as return type?
you can specify \Magento\Store\Model\ResourceModel\Website as resource model::save returns this

Phpstorm autocompletition... my fault I did not notice it.

…o around plugin to keep data consistent while chaning the website code on an existing website. This commit is realted to issue #1306

phoenix128 · 2018-06-11T09:20:07Z

@naydav @maghamed , tests are failing because Magento_Catalog does not have a sequence entry in module.xml for Magento_Store. It should be since tests and business logic require it.

Magento/Catalog/etc/module.xml:

<?xml version="1.0"?>
<!--
/**
 * Copyright © Magento, Inc. All rights reserved.
 * See COPYING.txt for license details.
 */
-->
<config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:Module/etc/module.xsd">
    <module name="Magento_Catalog" >
        <sequence>
            <module name="Magento_Eav"/>
            <module name="Magento_Cms"/>
            <module name="Magento_Indexer"/>
            <module name="Magento_Customer"/>
        </sequence>
    </module>
</config>

It happens on this module because the fixture dev/tests/integration/testsuite/Magento/Catalog/Model/ResourceModel/_files/website_attribute_sync.php it is updating core_website table through resource model:

$website = $objectManager->create(Website::class);
$website->setName('custom website for av test')
    ->setCode('customwebsite1');
$website->isObjectNew(true);
$websiteResourceModel->save($website);

Should I fix this issue in the core or temporary skip this exception?

Skipping the exception in \Magento\InventorySales\Plugin\Store\WebsiteResourcePlugin::aroundSave is not my best option of course, but looking at current tests is not a big issue.

naydav · 2018-06-11T10:33:22Z

app/code/Magento/InventorySales/Plugin/Store/WebsiteResourcePlugin.php

+    private function getCodeFromDatabase(int $websiteId): string
+    {
+        $connection = $this->resourceConnection->getConnection();
+        $tableName = $connection->getTableName('store_website');


We need to extract table name via $this->resourceConnection->getTableName()
Because $connection->getTableName is only wrapper which doesn't take into account table prefix

naydav · 2018-06-11T10:35:56Z

app/code/Magento/InventorySales/Plugin/Store/WebsiteResourcePlugin.php

+        $connection = $this->resourceConnection->getConnection();
+        $tableName = $connection->getTableName('store_website');
+        $selectQry = $connection->select()->from($tableName, 'code')->where('website_id = ?', $websiteId);
+        return (string) $connection->fetchOne($selectQry);


I'm not sure that we need to return ''(empty string) if the record was not found (website id is not existing)
More clear is return null in this case

That method can't return an empty string. If it is called, it is called from aroundSave and only if the website exists.

Maybe I should change the initialization of $oldCode = ''; to $oldCode = null;.
And change the if statement accordingly:

... if (($oldCode !== null) && ($oldCode !== $newCode)) { ...

naydav · 2018-06-11T10:38:23Z

app/code/Magento/InventorySales/Plugin/Store/WebsiteResourcePlugin.php

+use Magento\InventorySales\Model\AssignWebsiteToDefaultStock;
+use Magento\InventorySales\Model\ResourceModel\UpdateSalesChannelWebsiteCode;
+
+class WebsiteResourcePlugin


No bad to have what does the plugin do in the class name

naydav · 2018-06-11T10:42:08Z

app/code/Magento/InventorySales/etc/events.xml

@@ -6,9 +6,6 @@
 */
 -->
 <config xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="urn:magento:framework:Event/etc/events.xsd">
-    <event name="website_save_after">
-        <observer name="assign_website_to_default_stock" instance="Magento\InventorySales\Observer\Website\AssignWebsiteToDefaultStock"/>
-    </event>
    <event name="website_delete_after">


Looks like we also need to update delete logic website_delete_after

An observer after delete seems to be an efficient way and seems to be working, do you prefer to pluginize it?

Done in: #1356

…o correct getTableName method

… exist

…and eventual stock_link deletion This commit is related to PR #1344 . The logic was implmented with a plugin on website save and to keep code consistency I decided to apply the same logic to website delete.

magento-cicd2 · 2018-06-11T17:29:45Z

All committers have signed the CLA.

…ame-website-breaks-grid

-- fix static test

smoskaluk · 2018-06-12T12:50:55Z

@phoenix128 Problem is not fixed, please recheck. I changed code(or name) of Main Website and saved it - and 've got fatal error:
Fatal error: Uncaught Error: Call to undefined method Magento\Framework\App\ResourceConnection\Interceptor::execute() in /var/www/html/magento2ce/pr7/app/code/Magento/InventorySales/Plugin/Store/Model/ResourceModel/Website/UpdateSalesChannelWebsiteCodePlugin.php:64 Stack trace: #0 /var/www/html/magento2ce/pr7/lib/internal/Magento/Framework/Interception/Interceptor.php(135): Magento\InventorySales\Plugin\Store\Model\ResourceModel\Website\UpdateSalesChannelWebsiteCodePlugin->aroundSave(Object(Magento\Store\Model\ResourceModel\Website\Interceptor), Object(Closure), Object(Magento\Store\Model\Website\Interceptor)) #1 /var/www/html/magento2ce/pr7/lib/internal/Magento/Framework/Interception/Interceptor.php(153): Magento\Store\Model\ResourceModel\Website\Interceptor->Magento\Framework\Interception{closure}(Object(Magento\Store\Model\Website\Interceptor)) #2 /var/www/html/magento2ce/pr7/generated/code/Magento/Store/Model/ResourceModel/Website/Interceptor.php(130): Magento\Store\Model\ResourceModel\Website\Interceptor->___callPlu in /var/www/html/magento2ce/pr7/app/code/Magento/InventorySales/Plugin/Store/Model/ResourceModel/Website/UpdateSalesChannelWebsiteCodePlugin.php on line 64

phoenix128 · 2018-06-12T14:51:40Z

@smoskaluk , seems it is not coming from my original code, but from other commits, let me check it.

naydav · 2018-06-12T14:58:10Z

Sorry, It was a mistake from my side
Already fixed

phoenix128 added 2 commits June 9, 2018 11:01

FIX for issue #1306

00646b4

If a website was renamed/removed, sales channels were still tying to search for it

Keep data consistency with inventory_stock_sales_channel when a websi…

6397e66

…te code is changed See #1306

phoenix128 added Perugia Partner: MageSpecialist partners-contribution labels Jun 9, 2018

phoenix128 mentioned this pull request Jun 9, 2018

'NoSuchEntityException' even when full reindex was performed after website code was modified #1059

Closed

maghamed mentioned this pull request Jun 9, 2018

Need to remove MSI SalesChannels and their links to Stocks when Magento Website is deleted #1346

Closed

naydav suggested changes Jun 9, 2018

View reviewed changes

naydav self-assigned this Jun 9, 2018

maghamed reviewed Jun 9, 2018

View reviewed changes

maghamed mentioned this pull request Jun 9, 2018

MSI-1059: 'NoSuchEntityException' even when full reindex was performed after website code was modified. #1088

Closed

phoenix128 added 2 commits June 9, 2018 16:53

Refactoring for AssignWebsiteToDefaultStock moving it from observer t…

e22d31c

…o around plugin to keep data consistent while chaning the website code on an existing website. This commit is realted to issue #1306

FIX PHPMD + PHPCS validation

222da52

naydav suggested changes Jun 11, 2018

View reviewed changes

phoenix128 added 2 commits June 11, 2018 14:47

Modified plugin class name to reflect its responsability + switched t…

965dd8d

…o correct getTableName method

Return null from getCodeFromDatabase if the required website does not…

e079a73

… exist

phoenix128 mentioned this pull request Jun 11, 2018

Switched from observer based logic to plugin based logic for website … #1356

Merged

4 tasks

Created interface and preference for UpdateSalesChannelWebsiteCode

fabd833

phoenix128 force-pushed the issue-1306-rename-website-breaks-grid branch from 1f48ab7 to fabd833 Compare June 11, 2018 17:42

phoenix128 and others added 5 commits June 11, 2018 19:48

Integration test for website rename scenario and stock link consistency

9998600

MSI-1306: Rename website breaks grid

7166977

Merge remote-tracking branch 'origin/2.3-develop' into issue-1306-ren…

40fbe9d

…ame-website-breaks-grid

MSI-1306: Rename website breaks grid

389e299

MSI-1306: Rename website breaks grid

81d2cb8

-- fix static test

smoskaluk added the QA Failed label Jun 12, 2018

MSI-1306: Rename website breaks grid

3a44479

MSI-1306: Rename website breaks grid

ff0b71f

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

naydav merged commit 1a6334c into 2.3-develop Jun 12, 2018

naydav mentioned this pull request Jun 12, 2018

Renaming website breaks MSI grid #1306

Closed

magento-engcom-team added Award: bug fix Award: complex labels Jun 26, 2018

maghamed deleted the issue-1306-rename-website-breaks-grid branch March 20, 2019 15:13

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue 1306 rename website breaks grid #1344

Issue 1306 rename website breaks grid #1344

phoenix128 commented Jun 9, 2018 •

edited

Loading

naydav Jun 9, 2018

phoenix128 Jun 9, 2018

naydav Jun 9, 2018

naydav Jun 9, 2018

naydav Jun 9, 2018

phoenix128 Jun 9, 2018

maghamed Jun 9, 2018

maghamed Jun 9, 2018

phoenix128 Jun 9, 2018

phoenix128 commented Jun 11, 2018 •

edited

Loading

naydav Jun 11, 2018

naydav Jun 11, 2018

phoenix128 Jun 11, 2018 •

edited

Loading

naydav Jun 11, 2018

naydav Jun 11, 2018

phoenix128 Jun 11, 2018

phoenix128 Jun 11, 2018 •

edited

Loading

magento-cicd2 commented Jun 11, 2018 •

edited

Loading

smoskaluk commented Jun 12, 2018 •

edited

Loading

phoenix128 commented Jun 12, 2018

naydav commented Jun 12, 2018

Issue 1306 rename website breaks grid #1344

Issue 1306 rename website breaks grid #1344

Conversation

phoenix128 commented Jun 9, 2018 • edited Loading

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

phoenix128 commented Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

phoenix128 Jun 11, 2018 • 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

phoenix128 Jun 11, 2018 • edited Loading

Choose a reason for hiding this comment

magento-cicd2 commented Jun 11, 2018 • edited Loading

smoskaluk commented Jun 12, 2018 • edited Loading

phoenix128 commented Jun 12, 2018

naydav commented Jun 12, 2018

phoenix128 commented Jun 9, 2018 •

edited

Loading

phoenix128 commented Jun 11, 2018 •

edited

Loading

phoenix128 Jun 11, 2018 •

edited

Loading

phoenix128 Jun 11, 2018 •

edited

Loading

magento-cicd2 commented Jun 11, 2018 •

edited

Loading

smoskaluk commented Jun 12, 2018 •

edited

Loading