20434 consider url rewrite when change product visibility attribute 2 3 #20774

VitaliyBoyko · 2019-01-29T17:36:59Z

Description (*)

Has been implemented observing the saving attribute via mass action, creating/deleting URL rewrites appropriately on the product visibility attribute change.
For more details see the issue (#20434).

Fixed Issues (if relevant)

Product URL duplicate when changing visibility via mass action #20434: Product URL duplicate M2.2.6

Manual testing scenarios (*)

CASE 1:

Step 1. Create a product with the following URL (or any of your choice): « mon-produit-de-test » - Visible product (catalog, search)
Step 2. Create a second one with the same values but invisible individually then URL « mon-produit-de-test »
Step 3. Change visibility of the second product to visible via mass action.

The Url Key exist exception should be thrown.

CASE 2:
Step 1. Create a product with the following URL (or any of your choice): « mon-produit-de-test » - Visible product (catalog, search)
Step 2. Set visibility attribute to invisible via product grid mass action

Product URL rewrite should be removed.

CASE 3:
Step 1. Create a product with the following URL (or any of your choice): « mon-produit-de-test » - Not visible
Step 2. Set visibility attribute to visible via product grid mass action

Product URL rewrite should be created.

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)

…roduct attribute visibility via mass action

magento-engcom-team · 2019-01-29T17:37:02Z

Hi @VitaliyBoyko. 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 2.3-develop instance - deploy vanilla Magento instance

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

orlangur

@VitaliyBoyko,

The Url Key exist exception should be thrown.

I don't think this is correct.

Exception should only occur when there is conflicting URL Rewrite, for example, when you try to make second product visible - no earlier than that.

…rt tests)

…rt unit test)

…consider-url-rewrite-when-change-product-visibility-attribute-2-3

VitaliyBoyko · 2019-02-04T06:44:34Z

HI @sidolov @orlangur!
I've removed validation on the product page.
Feel free to check once again.

Thank you for the review!

VitaliyBoyko · 2019-02-07T18:40:13Z

Hi @orlangur
Any updates regarding this one?

sivaschenko

Hi @VitaliyBoyko thanks for update!

If the purpose of this observer is to throw an exception about duplicate URLs - I think it's better to keep only validation functionality here and do not persist anything.

Also, I'd move the validation implementation to a separate model and move the observer to backend area (according to best practives https://devdocs.magento.com/guides/v2.3/ext-best-practices/extension-coding/observers-bp.html)

Finally, please consider covering the introduced functionality with at least unit tests.

sivaschenko · 2019-02-12T21:17:06Z

...de/Magento/CatalogUrlRewrite/Observer/ProcessUrlRewriteOnChangeProductVisibilityObserver.php

+        $visibility = $attrData[ProductInterface::VISIBILITY] ?? null;
+
+        if (!$visibility) {
+            return [$attrData, $productIds, $storeId];


The interface method (\Magento\Framework\Event\ObserverInterface::execute) is void. Why do you need to return this array?

orlangur · 2019-02-13T13:05:25Z

...de/Magento/CatalogUrlRewrite/Observer/ProcessUrlRewriteOnChangeProductVisibilityObserver.php

+        $storeId = $event->getStoreId();
+        $visibility = $attrData[ProductInterface::VISIBILITY] ?? null;
+
+        if (!$visibility) {


@VitaliyBoyko I don't really get this logic. URL Rewrites for invisible products should still be created so that they can be made visible at any moment without conflict.

Please update description with current/changed behavior.

Hi @orlangur
I've updated the PR description. Hope it is more comprehensive now. You may also check bugs
#10725
#20434 for additional information.
Thank you!

sivaschenko

Hi @VitaliyBoyko thanks for the updates! Great test coverage! Please see my review notes, also it'll be great if you can add short method descriptions.

sivaschenko · 2019-02-21T11:21:03Z

app/code/Magento/CatalogUrlRewrite/Model/Products/AdaptUrlRewritesToVisibilityAttribute.php

+     * @param array $productIds
+     * @return array
+     */
+    protected function getProductsByIds(array $productIds): array


Please change access level to private for this method

@sivaschenko Hi. Thank you. Could you please check once again?

magento-engcom-team · 2019-02-22T14:47:43Z

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

sivaschenko

Hi @VitaliyBoyko, there is a small issue in the pull request, please see my review comment

sivaschenko · 2019-03-01T13:26:05Z

...de/Magento/CatalogUrlRewrite/Observer/ProcessUrlRewriteOnChangeProductVisibilityObserver.php

+            return;
+        }
+
+        $this->adaptUrlRewritesToVisibility->execute($productIds, $visibility);


Please ensure $visibility is an integer: execute method expects the second argument to be an integer, but actually, it is a string in $attrData[ProductInterface::VISIBILITY]

Hi @sivaschenko
I've fixed possible type inconsistency. Thank you!

VitaliyBoyko · 2019-03-13T07:40:50Z

Hi @sivaschenko
Should I update something more here?

magento-engcom-team · 2019-03-16T14:11:37Z

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

VasylShvorak · 2019-03-21T13:14:09Z

✔️ QA passed

ghost · 2019-03-26T16:56:06Z

Hi @VitaliyBoyko, 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.

…y attribute 2 3 #20774

eerohakio · 2020-11-10T12:45:56Z

app/code/Magento/CatalogUrlRewrite/Model/Products/AdaptUrlRewritesToVisibilityAttribute.php

+                    ]
+                );
+            } elseif ($visibility !== Visibility::VISIBILITY_NOT_VISIBLE) {
+                $product->setVisibility($visibility);


Should not touch visibility here as the point of whole model is to adapt the rewrite to visibility changes and not change the visibility?

vitaliyboyko added 2 commits January 29, 2019 19:34

ENGCOM-20434: Consider deleting/creating url rewrites on change the p…

24382e3

…roduct attribute visibility via mass action

ENGCOM-20434: Added accidentally removed code

4d18c9a

magento-engcom-team added Component: CatalogUrlRewrite Release Line: 2.3 Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Jan 29, 2019

orlangur self-assigned this Jan 29, 2019

orlangur requested changes Jan 29, 2019

View reviewed changes

sidolov added the Progress: needs update label Jan 30, 2019

Vitaliy Boyko added 5 commits February 2, 2019 20:02

ENGCOM-20434: Removed url key validation for invisible products

6a5776c

ENGCOM-20434: Removed url key validation for invisible products (reve…

f2088d9

…rt tests)

ENGCOM-20434: Removed url key validation for invisible products (reve…

4edec38

…rt unit test)

ENGCOM-20434: static fixes

a8d1dc0

Merge branch '2.3-develop' of github.com:magento/magento2 into 20434-…

5687704

…consider-url-rewrite-when-change-product-visibility-attribute-2-3

sivaschenko requested changes Feb 12, 2019

View reviewed changes

sivaschenko self-assigned this Feb 12, 2019

orlangur requested changes Feb 13, 2019

View reviewed changes

ENGCOM-20434: refactoring, test coverage

c865bdc

sivaschenko requested changes Feb 21, 2019

View reviewed changes

ENGCOM-20434: refactoring, doc blocks adjustments

1c608d7

sivaschenko approved these changes Feb 22, 2019

View reviewed changes

sivaschenko removed the Progress: needs update label Feb 22, 2019

sivaschenko requested changes Mar 1, 2019

View reviewed changes

ghost added the Progress: needs update label Mar 1, 2019

ENGCOM-20434: fixed type inconsistency

a80943f

sivaschenko approved these changes Mar 16, 2019

View reviewed changes

ghost added Progress: ready for testing and removed Progress: needs update labels Mar 16, 2019

VasylShvorak self-assigned this Mar 21, 2019

ghost added Progress: testing in progress and removed Progress: ready for testing labels Mar 21, 2019

ghost added Progress: extended testing and removed Progress: testing in progress labels Mar 21, 2019

p-bystritsky self-assigned this Mar 21, 2019

ENGCOM-4357: Static tests fix.

a8d9be1

ghost added Progress: accept and removed Progress: extended testing labels Mar 22, 2019

magento-engcom-team added Award: bug fix Award: test coverage labels Mar 22, 2019

magento-engcom-team mentioned this pull request Mar 26, 2019

Product URL duplicate when changing visibility via mass action #20434

Closed

magento-engcom-team merged commit a8d9be1 into magento:2.3-develop Mar 26, 2019

magento-engcom-team pushed a commit that referenced this pull request Mar 26, 2019

ENGCOM-4357: 20434 consider url rewrite when change product visibilit…

e38f5a8

…y attribute 2 3 #20774

magento-engcom-team added this to the Release: 2.3.2 milestone Mar 26, 2019

VitaliyBoyko mentioned this pull request Mar 27, 2019

[Backport] 20434 consider url rewrite when change product visibility attribute #21977

Closed

4 tasks

eerohakio reviewed Nov 10, 2020

View reviewed changes

m2-community-project bot removed the Progress: accept label Nov 10, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

20434 consider url rewrite when change product visibility attribute 2 3 #20774

20434 consider url rewrite when change product visibility attribute 2 3 #20774

VitaliyBoyko commented Jan 29, 2019 •

edited

Loading

magento-engcom-team commented Jan 29, 2019

orlangur left a comment

VitaliyBoyko commented Feb 4, 2019 •

edited

Loading

VitaliyBoyko commented Feb 7, 2019

sivaschenko left a comment

sivaschenko Feb 12, 2019

orlangur Feb 13, 2019

VitaliyBoyko Feb 13, 2019 •

edited

Loading

sivaschenko left a comment •

edited

Loading

sivaschenko Feb 21, 2019

VitaliyBoyko Feb 22, 2019

magento-engcom-team commented Feb 22, 2019

sivaschenko left a comment

sivaschenko Mar 1, 2019 •

edited

Loading

VitaliyBoyko Mar 6, 2019 •

edited

Loading

VitaliyBoyko commented Mar 13, 2019

magento-engcom-team commented Mar 16, 2019

VasylShvorak commented Mar 21, 2019

ghost commented Mar 26, 2019

eerohakio Nov 10, 2020 •

edited

Loading

20434 consider url rewrite when change product visibility attribute 2 3 #20774

20434 consider url rewrite when change product visibility attribute 2 3 #20774

Conversation

VitaliyBoyko commented Jan 29, 2019 • edited Loading

Description (*)

Fixed Issues (if relevant)

Manual testing scenarios (*)

Contribution checklist (*)

magento-engcom-team commented Jan 29, 2019

orlangur left a comment

Choose a reason for hiding this comment

VitaliyBoyko commented Feb 4, 2019 • edited Loading

VitaliyBoyko commented Feb 7, 2019

sivaschenko left a comment

Choose a reason for hiding this comment

sivaschenko Feb 12, 2019

Choose a reason for hiding this comment

orlangur Feb 13, 2019

Choose a reason for hiding this comment

VitaliyBoyko Feb 13, 2019 • edited Loading

Choose a reason for hiding this comment

sivaschenko left a comment • edited Loading

Choose a reason for hiding this comment

sivaschenko Feb 21, 2019

Choose a reason for hiding this comment

VitaliyBoyko Feb 22, 2019

Choose a reason for hiding this comment

magento-engcom-team commented Feb 22, 2019

sivaschenko left a comment

Choose a reason for hiding this comment

sivaschenko Mar 1, 2019 • edited Loading

Choose a reason for hiding this comment

VitaliyBoyko Mar 6, 2019 • edited Loading

Choose a reason for hiding this comment

VitaliyBoyko commented Mar 13, 2019

magento-engcom-team commented Mar 16, 2019

VasylShvorak commented Mar 21, 2019

ghost commented Mar 26, 2019

eerohakio Nov 10, 2020 • edited Loading

Choose a reason for hiding this comment

VitaliyBoyko commented Jan 29, 2019 •

edited

Loading

VitaliyBoyko commented Feb 4, 2019 •

edited

Loading

VitaliyBoyko Feb 13, 2019 •

edited

Loading

sivaschenko left a comment •

edited

Loading

sivaschenko Mar 1, 2019 •

edited

Loading

VitaliyBoyko Mar 6, 2019 •

edited

Loading

eerohakio Nov 10, 2020 •

edited

Loading