Fixed URL Rewrite addition/removal on product website add/remove

[gwharton]

Related Pull Requests

magento/inventory#2887

Description (*)

Moved scope of ProductToWebsiteChangeObserver to base instead of adminhtml

as this functionality has been moved to a consumer. With the observer set
to adminhtml, this observer was not being called properly on changing
the websites for a particular product in the consumer.

Corrected ProductProcessUrlRewriteSavingObserver to correctly add and remove
rewrite URLs when products are added and removed from websites using the
Product Edit Admin Page.

Corrceted ProductToWebsiteChangeObserver to correctly add and remove
rewrite URLs when products are added and removed using the Products
Grid Mass Action function.

Extended unit tests to cover both Observers.

Fixed Issues (if relevant)

1. Closes Product URL Rewrites are not removed when product removed from website #24184: Product URL Rewrites are not removed when product removed from website
   This also fixes the issue with URL rewrites not being generated correctly using the mass action remove/add from website.

Manual testing scenarios (*)

1. See issue

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]

Hi @gwharton. 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 give me test instance - deploy test instance based on PR changes
• @magento give me 2.4-develop instance - deploy vanilla Magento instance

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

[ihor-sviziev]

@magento run Database Compare

[ihor-sviziev]

Hi @gwharton,
Thank you so much for the fix! From first view your fix looks good, but url rewrites quite complex thing and usually there are no issues with single unit, but issues appears when few components works together. For such cases unit tests much less effective than integration tests, as there you’re having real objects, real db, etc and you could do some queries to db and compare with what you expect.
Could you review my comments and cover your changes with integration tests?

[gwharton]

I have added a new function to the class Magento/UrlRewrite/Model/Storage/DbStorage.php.

Previously this class only provided the ability to remove a single url rewrite at a time, which lead to lengthy and nested foreach loops, with many many database transactions to process for a single operation where multiple url rewrites needed removing.

I added a new function which allows you to remove multiple url rewrite's based on arrays of entity_ids and websites_ids, allowing you to remove multiple items from multiple websites in a single operation.

Question, Would it be worth adding this new deleteEntitiesFromStores function to the UrlPersistInterface so it can sit alongside the existing deleteByData function already in that interface, and provided by DbStorage. It could be useful elsewhere.

Changes incorporated

[gwharton]

Apologies for the large number of additions of <group value="urlRewrite"/> in the MFTF tests. Group allocation was inconsistent amongst UrlRewrite and CatalogUrlRewrite modules, so I took the opportunity to tag them all in a consistent group to aid running of UrlRewrite related MFTF tests.

[gwharton]

Let me know if you want me to cleanup commit history and squash.

[ihor-sviziev]

Looks like this is the last iteration :)

Let's squash all changes once everything will be ready

[engcom-Echo]

@magento run all tests

[engcom-Echo]

Failed functional, static tests not related to the changes in this PR

[gwharton]

The integration test that used to fail now passes.
The functional tests seem to pass if you re-run them enough times.
The static test failiure is nothing to do with my code.

I guess this is what you get by running the tests against the PR merged with 2.4-develop as opposed to running against the forked repo as if someone manages to commit something bad to 2.4-develop, everyones tests fail.

[ihor-sviziev]

@engcom-Echo thank you so much!

[magento-engcom-team]

Hi @ihor-sviziev, thank you for the review.
ENGCOM-7162 has been created to process this Pull Request

[engcom-Bravo]

✔️ QA Passed

The record for the second website disappears from the catalog_url_rewrite_product_category table after the product is unassigned

[engcom-Echo]

Failed functional, static tests not related to the changes in this PR

[m2-assistant]

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

[naydav]

@gwharton
Hi Graham,
We run Performance build with medium profile on 2.4-develop branch and we discovered huge degradation during Product saving in Admin panel (approximately 70% of degradation).

Server Side EE API
Create product with extensible data objects	+77.7 %	[+205 ms]
Create product	+160.8 %	[+201 ms]
Server Side EE Add to Wishlist

Server Side EE Admin Create Order
Save Shipment	+7.4 %	[+27 ms]
Add Products	+5.7 %	[+18 ms]

Server Side EE Admin Create Product
New Configurable Product Save	+85.4 %	[+698 ms]
Server Side EE Admin Customer Management

Server Side EE Admin Edit Product
Edit Simple Product Save	+13 %	[+47 ms]
Edit Configurable Product Save

Thus we had to revert changes.

We need to find a more appreciative solution or make refact of current.
Thanks

[gwharton]

Understood. We did do some work on optimising it, but obviously not enough.