Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Conversation

VitaliyBoyko
Copy link
Contributor

@VitaliyBoyko VitaliyBoyko commented Mar 27, 2019

Original PR: #20774

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)

  1. 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)

@magento-engcom-team
Copy link
Contributor

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.2-develop instance - deploy vanilla Magento instance

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

@magento-engcom-team magento-engcom-team added Component: CatalogUrlRewrite Release Line: 2.2 Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Mar 27, 2019
@rogyar rogyar self-assigned this Apr 3, 2019
@magento-engcom-team
Copy link
Contributor

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

@VasylShvorak
Copy link
Contributor

✔️ QA passed

@sivaschenko
Copy link
Member

The pull request is causing performance degradation on 2.2-develop branch

Save Attributes (Product Grid Mass Actions): 8.6% (mainline = 35 | team = 38) -> DEGRADATION
Save Attributes (Product Grid Mass Actions): 12.2% (mainline = 49 | team = 55) -> DEGRADATION
Save Attributes (Product Grid Mass Actions): 27.3% (mainline = 198 | team = 252) -> DEGRADATION

The degradation is caused by an additional observer to catalog_product_attribute_update_before event. @kandy can you please advise how this PAT failure can be handled

@sidolov
Copy link
Contributor

sidolov commented May 31, 2019

Hi @VitaliyBoyko , do you have any progress on this?

@VitaliyBoyko
Copy link
Contributor Author

Hi @sidolov
I believe the performance degradation is quite expected here.
Saving URL rewrites requires additional operations performed on DB that obviously increases the load. Thanks!

@sivaschenko
Copy link
Member

@kandy can you please take a look if the PAT failure can be approved for this PR?

@VladimirZaets
Copy link
Contributor

@magento run all tests

@orlangur orlangur self-assigned this Jul 15, 2019
@orlangur
Copy link
Contributor

Closing as corresponding 2.3 PR needs to be reverted: it introduced incorrect URL Rewrites behavior.

Correct behavior taking into consideration Piotr Kaminsky comment is being implemented in #23275.

@orlangur orlangur closed this Jul 15, 2019
@m2-assistant
Copy link

m2-assistant bot commented Jul 15, 2019

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: CatalogUrlRewrite Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner Release Line: 2.2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

10 participants