Do not overwrite URL Key with blank value

[josephmcdermott]

Description

• do not update existing products with a blank URL Key if no url_key value is provided in the import data
• do not update existing products with a new URL Key if a name but no url_key value is provided in the import data

Fixed Issues

• CSV Import of sku,attribute empties url_key value #17023: CSV Import of sku,attribute empties url_key value

Manual testing scenarios

See issue for details. Essentially if importing a CSV with sku,name or sku,description only, the result is the unexpected update or removal of the URL Key of the product.

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)

[magento-cicd2]

All committers have signed the CLA.

[magento-engcom-team]

Hi @josephmcdermott. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

• @magento-engcom-team give me test instance - deploy test instance based on PR changes
• @magento-engcom-team give me {$VERSION} instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

[nuzil]

Comments are good, can you please remove Magento issues links from the code. Let keep it clean.

[nuzil]

Comments are good, can you please remove Magento issues links from the code. Let keep it clean.

[nuzil]

Comments are good, can you please remove Magento issues links from the code. Let keep it clean.

[nuzil]

I would not recommend here to change return type of the method (line 2725, 2739). As user of getUrlKey method I expecting to receive back some url. Can you please change this implementation to check if URL is empty then do not overwrite it.

[nuzil]

see comment before about return type

[josephmcdermott]

Yes that's a good point, I was on the fence myself about this change.

[nuzil]

see comment before about return type

[magento-engcom-team]

@josephmcdermott thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

[josephmcdermott]

@nuzil looks like a test is now failing, but it is now an invalid test I believe:
\Magento\CatalogImportExport\Model\Import\ProductTest::testImportWithoutUrlKeys

This pre-existing test is to state that the URL Key SHOULD be updated if an import CSV does not contain a value for url_key (in this case the CSV contains url_key but empty value), whereas this PR states that those products URL Key should NOT be updated

I would say the expected result in the test is as follows, since the first two SKUs existed already, the third one did not:

• sku: simple1 / url_key: url-key
• sku: simple2 / url_key: url-key2
• sku: simple3 / url_key: simple-3

So perhaps the test data now should be:

$products = [
    'simple1' => 'url-key',
    'simple2' => 'url-key2',
    'simple3' => 'simple-3'
];

Obviously I don't just want to change an integration test though since it was added for reason, so it might be worth checking with the developer who wrote the now-failing test.

Over to you on how to proceed.

[nuzil]

@josephmcdermott
Thats generally purpose of integration tests to detect problems.
But in your case as you changed logic of urk_key import, definitely test also have to be changed according to new logic.
So I will ask you, can you please align tests so, that it correctly process new logic.
Feel free to do so.

[nuzil]

@josephmcdermott can you please sign CLA and also I see that one of integration tests are failed

[josephmcdermott]

OK tests passing @nuzil and CLA signed. The test itself was actually fine all along, but other tests were impacting the results (some tests check for 3 products instead of 4, despite the fact that other tests may have added products, etc.) - was a little frustrating to be honest, the test interdependencies seem a little too delicate.

[magento-engcom-team]

Hi @slavvka, thank you for the review.
ENGCOM-2916 has been created to process this Pull Request

[magento-engcom-team]

Hi @josephmcdermott. Thank you for your contribution.
We will aim to release these changes as part of 2.2.8.
Please check the release notes for final confirmation.

Please, consider to port this solution to 2.3 release line.
You may use Porting tool to port commits automatically.

[dzschille]

Is this PR released yet? I can see in the 2.2.8 CHANGELOG.md:

#17023 -- CSV Import of sku,attribute empties url_key value (fixed in magento/magento2#17882)

But when i look into the code the method getUrlKey() looks still like:

protected function getUrlKey($rowData)
{
    if (!empty($rowData[self::URL_KEY])) {
        return $this->productUrl->formatUrlKey($rowData[self::URL_KEY]);
    }

    if (!empty($rowData[self::COL_NAME])) {
        return $this->productUrl->formatUrlKey($rowData[self::COL_NAME]);
    }

    return '';
}