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

Improve category url integrity #38672

Open
wants to merge 4 commits into
base: 2.4-develop
Choose a base branch
from

Conversation

digitalrisedorset
Copy link

@digitalrisedorset digitalrisedorset commented Apr 29, 2024

When the category is saved in Magento backend and particularly when the scope is a custom store, some use cases leave some url key or url path with data that break features like the canonical url in the category page on the frontend.

Description (*)

The changes are twofold:

  1. ensure the url path is valid for the global scope and for all the store scope when a category url key is changed
  2. ensure the url path is removed for a store scope if the category does not have custom url key for the store scope

Related Pull Requests

Fixed Issues (if relevant)

  1. Fixes Category URL Path issue in multi-store #38627

Manual testing scenarios (*)

use case 1: the category url key is set for the global scope and a different value is set for the store scope 2

global scope: store id = 0, url path: cat1
custom scope: store id = 2, url path: cat1_2
  • we want to validate the custom url path is removed correctly if the the custom store scope is reverted to use the global scope

use case 2: starting with a parent category like the above (with 2 scope set), the child category url key is set for the global scope and the category url key is not set for either of the default store or custom store: only store 0 has a url key

global scope: store id = 0, url path: cat1/cat12

  • we want to validate the custom url path for the store 2 is removed when the parent category custom scope is reverted

use case 3: the child category uses a url key for the global scope (store 0) and uses a custom url key (cat12_2) for the custom store (store 2)

global scope: store id = 0, url path: cat1/cat12
custom store scope: store id = 2, url path: cat1/cat12_2

use case 4: the child category uses a url key for the default store scope (store = 1) (cat12_1) and uses no custom url key for the custom store (store 2)

global scope: store id = 0, url path: cat1/cat12
default store scope: store id = 1, url path: cat1/cat12_1

use case 5: the child category uses a custom url key (cat12_1) for the global scope and uses a custom url key (cat12_2) for the custom store (store 2)

global scope: store id = 0, url path: cat1/cat12
default store scope: store id = 1, url path: cat1/cat12_1
custom store scope: store id = 2, url path: cat1/cat12_2

Questions or comments

The PR changes resolve the issue with canonical url being invalid. However, if the parent category url is changed afterwards, it does break the category path. I have attempted to add integration tests but this attempt was shortlived once I realised the category save process cannot be triggered by a single method but rather by using a list of events/plugins and resources. If we wanted this area of the system bug free, we'd need this area (category backend save controller) to be refactored

The changes duplicate existing snippets to try to keep the other logic to work like before as much as possible.

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)
  • README.md files for modified modules are updated and included in the pull request if any README.md predefined sections require an update
  • All automated tests passed successfully (all builds are green)

Copy link

m2-assistant bot commented Apr 29, 2024

Hi @digitalrisedorset. Thank you for your contribution!
Here are some useful tips on 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:
  1. Database Compare
  2. Functional Tests CE
  3. Functional Tests EE
  4. Functional Tests B2B
  5. Integration Tests
  6. Magento Health Index
  7. Sample Data Tests CE
  8. Sample Data Tests EE
  9. Sample Data Tests B2B
  10. Static Tests
  11. Unit Tests
  12. WebAPI Tests
  13. Semantic Version Checker

You can find more information about the builds here
ℹ️ Run only required test builds during development. Run all test builds before sending your pull request for review.


For more details, review the Code Contributions documentation.
Join Magento Community Engineering Slack and ask your questions in #github channel.

@m2-community-project m2-community-project bot added the Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. label Apr 29, 2024
@m2-community-project m2-community-project bot added this to Pending Review in Pull Requests Dashboard Apr 29, 2024
@digitalrisedorset digitalrisedorset marked this pull request as draft April 29, 2024 06:49
@digitalrisedorset digitalrisedorset marked this pull request as ready for review May 1, 2024 07:46
@digitalrisedorset
Copy link
Author

@magento run all tests

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Priority: P1 Once P0 defects have been fixed, a defect having this priority is the next candidate for fixing. Progress: pending review
Projects
Pull Requests Dashboard
  
Pending Review
Development

Successfully merging this pull request may close these issues.

Category URL Path issue in multi-store
1 participant