MC-40031: [UX] Configurations are not preserved on form reload when new configurable product creation fails #32194

namlier · 2021-02-16T17:45:59Z

Description (*)

This is a usability issue.

Fixed Issues (if relevant)

Fixes [UX] Configurations are not preserved on form reload when new configurable product creation fails #32102

Manual testing scenarios (*)

Open backend
Go to products grid
Create configurable product with 1 attribute named "Configurable One"
Save product
Create new configurable product configuration basing on 5 attributes with only 2 variations each
Enter "Configurable One" as name
Configure variations qty and prices (enter different price and qty for each vatiation)
CLick Save

Actual: Form is reloaded. Error is displayed:
The value specified in the URL Key field would generate a URL that already exists.
The variations you configured have disappeared. Information that has to be entered manually and requires significant time is lost.

Expected: To be identified by UX team.

Form is reloaded, configurations are preserved.
OR
Form is not reloaded, AJAX validation includes URL Key check.

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 are green)

m2-assistant · 2021-02-16T17:46:01Z

Hi @mastiuhin-olexandr. Thank you for your contribution
Here are 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 give me test instance - deploy test instance based on PR changes
@magento give me 2.4-develop instance - deploy vanilla Magento instance

❗ Automated tests can be triggered manually with an appropriate comment:

@magento run all tests - run or re-run all required tests against the PR changes
@magento run <test-build(s)> - run or re-run specific test build(s)
For example: @magento run Unit Tests

<test-build(s)> is a comma-separated list of build names. Allowed build names are:

Database Compare
Functional Tests CE
Functional Tests EE,
Functional Tests B2B
Integration Tests
Magento Health Index
Sample Data Tests CE
Sample Data Tests EE
Sample Data Tests B2B
Static Tests
Unit Tests
WebAPI Tests
Semantic Version Checker

You can find more information about the builds here

ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review.

For more details, please, review the Magento Contributor Guide documentation.

⚠️ According to the Magento Contribution requirements, all Pull Requests must go through the Community Contributions Triage process. Community Contributions Triage is a public meeting.

🕙 You can find the schedule on the Magento Community Calendar page.

📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket.

🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel

✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel

namlier · 2021-02-16T17:46:15Z

@magento run all tests

namlier · 2021-02-16T22:04:03Z

@magento run Functional Tests B2B

namlier · 2021-02-17T09:10:34Z

@magento run Functional Tests B2B

zakdma · 2021-02-17T13:36:16Z

app/code/Magento/Catalog/Controller/Adminhtml/Product/Validate.php

@@ -130,11 +158,18 @@ public function execute()
            $resource->getAttribute('news_from_date')->setMaxValue($product->getNewsToDate());
            $resource->getAttribute('custom_design_from')->setMaxValue($product->getCustomDesignTo());

+            $this->validateUrlKeyUniqueness($product);


You should not perform validation in controller
It is better to do it inside
\Magento\Catalog\Model\ResourceModel\Product::validate

I think it should stay in controller, because other requests also has this validation.

namlier · 2021-02-18T10:38:51Z

@magento run all tests

namlier · 2021-02-18T12:34:05Z

@magento run Functional Tests EE

namlier · 2021-02-18T13:22:20Z

@Magneto run Functional Tests B2B

namlier · 2021-02-18T14:02:42Z

@magento run Functional Tests B2B

namlier · 2021-02-18T15:42:49Z

@magento run Functional Tests B2B

namlier · 2021-02-18T17:50:16Z

@magento run Functional Tests B2B

namlier · 2021-02-18T19:50:38Z

@magento run Functional Tests B2B

namlier · 2021-02-19T00:16:30Z

@magento run Functional Tests B2B

namlier · 2021-02-19T09:08:19Z

@magento run Functional Tests B2B

namlier · 2021-02-19T12:24:24Z

@magento run Functional Tests B2B

namlier · 2021-02-19T15:45:21Z

@magento run Functional Tests B2B

zakdma · 2021-02-22T15:58:11Z

app/code/Magento/Catalog/Controller/Adminhtml/Product/Validate.php

+        \Magento\Catalog\Model\ProductFactory $productFactory,
+        ProductUrlPathGenerator $productUrlPathGenerator,
+        UrlFinderInterface $urlFinder,
+        GetStoresListByWebsiteIds $getStoresListByWebsiteIds


added constructor parameters should be nullable and should be initialized inside constructor

Thank you, fixed.

namlier · 2021-02-22T16:07:40Z

@magento run all tests

gabrieldagama · 2021-03-31T10:51:00Z

app/code/Magento/CatalogUrlRewrite/Model/Product/Validator.php

+     * @param Product $product
+     * @return array Array of conflicting Url Keys.
+     */
+    public function findUrlKeyConflicts(Product $product): array


A Validator class with a method findUrlKeyConflicts probably doesn't make much sense, I would recommend actually changing it to validateUrlKeyConflicts and throw the exception here. Make this method a void instead, there is no point to return the URL keys if they won't be used.

Also, agree with Myroslav, the generation should happen on controller and be a parameter on this function. If the information from product that we need is just ID, probably just use it as parameter instead of whole product object.

I am not sure that rework this to passing UrlKey parameter is a good idea.

In this case the most of logic will stay in the controller. Because there is a function that is using for UrlPath generation
$this->productUrlPathGenerator->getUrlPathWithSuffix($product, $store->getId());,
that uses a product model. The only what will left in the validator is "findAllByData" and the pieace of program that checks found rewrites are not belong to a product.

The other pieace of program generating urlKey, generating urlPathes of stores, searching storeIds of websites (as workaround for performance) will be in the controller.

Please check the last changes and then confirm or deny that it should be reworked like you said before.

Thank you, fixed. Validator now throws exception. And method renamed.

gabrieldagama · 2021-03-31T10:52:43Z

app/code/Magento/CatalogUrlRewrite/Model/Product/Validator.php

+        foreach ($urlRewrites as $urlRewrite) {
+            if (in_array($urlRewrite->getRequestPath(), $storeIdsToPathForSave)
+                && isset($storeIdsToPathForSave[$urlRewrite->getStoreId()])
+                && $storeIdsToPathForSave[$urlRewrite->getStoreId()] == $urlRewrite->getRequestPath()


Please use ===

gabrieldagama · 2021-03-31T10:52:55Z

app/code/Magento/CatalogUrlRewrite/Model/Product/Validator.php

+            if (in_array($urlRewrite->getRequestPath(), $storeIdsToPathForSave)
+                && isset($storeIdsToPathForSave[$urlRewrite->getStoreId()])
+                && $storeIdsToPathForSave[$urlRewrite->getStoreId()] == $urlRewrite->getRequestPath()
+                && $product->getId() != $urlRewrite->getEntityId()) {


Please use !==

zakdma · 2021-03-31T11:03:04Z

app/code/Magento/CatalogUrlRewrite/Model/Product/Validator.php

+        $storeIdsToPathForSave = [];
+        $searchData = [
+            UrlRewrite::ENTITY_TYPE => ProductUrlRewriteGenerator::ENTITY_TYPE,
+            UrlRewrite::REQUEST_PATH => []


…ew configurable product creation fails

namlier · 2021-03-31T14:35:30Z

@magento run all tests

namlier · 2021-03-31T16:24:29Z

@magento run Functional Tests B2B

namlier · 2021-03-31T19:33:42Z

@magento run Functional Tests B2B, WebAPI Tests

namlier · 2021-04-01T08:48:46Z

@magento run WebAPI Tests

magento-engcom-team · 2021-04-01T13:14:41Z

Hi @gabrieldagama, thank you for the review.
ENGCOM-8808 has been created to process this Pull Request
✳️ @gabrieldagama, could you please add one of the following labels to the Pull Request?

Label	Description
`Auto-Tests: Covered`	All changes in Pull Request is covered by auto-tests
`Auto-Tests: Not Covered`	Changes in Pull Request requires coverage by auto-tests
`Auto-Tests: Not Required`	Changes in Pull Request does not require coverage by auto-tests

zakdma · 2021-04-01T13:15:20Z

@magento import code to https://github.com/magento-tsg/magento2ce

magento-engcom-team · 2021-04-01T13:17:46Z

@zakdma the branch with code successfully imported intomagento-tsg/magento2ce repository. Branch name: imported-magento-magento2-32194.

zakdma · 2021-04-06T11:32:31Z

@magento import code to https://github.com/magento-tsg/magento2ce

magento-engcom-team · 2021-04-06T11:35:27Z

@zakdma, an error occurred during the Pull Request import.

magento-engcom-team · 2021-04-06T11:36:00Z

@zakdma, an error occurred during the Pull Request import.

zakdma · 2021-04-06T11:36:40Z

@magento import code to https://github.com/magento-tsg/magento2ce

magento-engcom-team · 2021-04-06T11:38:46Z

@zakdma the branch with code successfully imported intomagento-tsg/magento2ce repository. Branch name: imported-magento-magento2-32194.

m2-assistant · 2021-04-06T20:32:09Z

Hi @mastiuhin-olexandr, thank you for your contribution!
Please, complete Contribution Survey, it will take less than a minute.
Your feedback will help us to improve contribution process.

m2-community-project bot added Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Severity: S2 Major restrictions or short-term circumventions are required until a fix is available. labels Feb 16, 2021

magento-engcom-team added Component: Catalog Release Line: 2.4 labels Feb 16, 2021

m2-community-project bot added the Progress: pending review label Feb 16, 2021

zakdma suggested changes Feb 17, 2021

View reviewed changes

m2-community-project bot assigned zakdma Feb 17, 2021

m2-community-project bot added Progress: needs update and removed Progress: pending review labels Feb 17, 2021

m2-community-project bot added Progress: review and removed Progress: needs update labels Feb 19, 2021

zakdma suggested changes Feb 22, 2021

View reviewed changes

m2-community-project bot added Progress: needs update and removed Progress: review labels Feb 22, 2021

m2-community-project bot added Progress: needs update and removed Progress: review labels Mar 31, 2021

gabrieldagama suggested changes Mar 31, 2021

View reviewed changes

zakdma suggested changes Mar 31, 2021

View reviewed changes

MC-40031: [UX] Configurations are not preserved on form reload when n…

e50d9a4

…ew configurable product creation fails

m2-community-project bot added Progress: review and removed Progress: needs update labels Mar 31, 2021

mastiuhin-olexandr added 2 commits March 31, 2021 17:32

MC-40031: [UX] Configurations are not preserved on form reload when n…

f4c4955

…ew configurable product creation fails

Merge remote-tracking branch 'origin/2.4-develop' into MC-40031

89c122d

namlier requested review from zakdma and gabrieldagama March 31, 2021 15:25

zakdma approved these changes Apr 1, 2021

View reviewed changes

m2-community-project bot added Progress: ready for testing and removed Progress: review labels Apr 1, 2021

gabrieldagama approved these changes Apr 1, 2021

View reviewed changes

magento-engcom-team added the Component: CatalogUrlRewrite label Apr 1, 2021

magento-engcom-team merged commit 0b36bd2 into magento:2.4-develop Apr 6, 2021

MC-40031: [UX] Configurations are not preserved on form reload when new configurable product creation fails #32194

MC-40031: [UX] Configurations are not preserved on form reload when new configurable product creation fails #32194

Uh oh!

Conversation

namlier commented Feb 16, 2021

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

Uh oh!

m2-assistant bot commented Feb 16, 2021

Uh oh!

namlier commented Feb 16, 2021

Uh oh!

namlier commented Feb 16, 2021

Uh oh!

namlier commented Feb 17, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namlier commented Feb 18, 2021

Uh oh!

namlier commented Feb 18, 2021

Uh oh!

namlier commented Feb 18, 2021

Uh oh!

namlier commented Feb 18, 2021

Uh oh!

namlier commented Feb 18, 2021

Uh oh!

namlier commented Feb 18, 2021

Uh oh!

namlier commented Feb 18, 2021

Uh oh!

namlier commented Feb 19, 2021

Uh oh!

namlier commented Feb 19, 2021

Uh oh!

namlier commented Feb 19, 2021

Uh oh!

namlier commented Feb 19, 2021

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

namlier commented Feb 22, 2021

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!

namlier commented Mar 31, 2021

Uh oh!

namlier commented Mar 31, 2021

Uh oh!

namlier commented Mar 31, 2021

Uh oh!

namlier commented Apr 1, 2021

Uh oh!

magento-engcom-team commented Apr 1, 2021

Uh oh!