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

Add ACL role ID to category tree cache id #27429

Conversation

quangdo-aligent
Copy link
Contributor

@quangdo-aligent quangdo-aligent commented Mar 25, 2020

Related Pull Requests

Description (*)

When an admin user views a product's category tree (by editing a product), the category tree is cached with an ID akin to CATALOG_PRODUCT_CATEGORY_TREE_0_ (the $filter option is not used in the Magento codebase).

This is not compatible with admin users that have limited Role Scopes. If the first admin user to view a product category tree has access to all websites (e.g. Administrator), this then caches the full category tree for all websites. Then the limited admin user will also see this full category tree even if they should be limited to a single website's category tree. Similarly, if the limited admin user views the category tree after the block cache is cleaned, the Administrator user will only see a limited category tree.

This pull request adds the admin's user's ACL role ID to the cache ID. This is probably about as performant as we can get.

Manual testing scenarios (*)

  1. Create a new category (and any sub-categories if desired) and assign it to a new website.
  2. Assign a few products to the new website.
  3. Create a new website-limited admin user with only access to the new website.
  4. Clean block_html cache and view a product's category tree as an Administrator admin user.
  5. Log in as the website-limited admin user and view the category tree for any product.
  6. Observe that the full category tree is visible.

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)

Resolved issues:

  1. resolves [Issue] Add ACL role ID to category tree cache id #28306: Add ACL role ID to category tree cache id

@m2-assistant
Copy link

m2-assistant bot commented Mar 25, 2020

Hi @quangdo-aligent. 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.

Copy link

@onkz19 onkz19 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it's good

@aleron75 aleron75 added Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests bugfix Award: bug fix labels Apr 3, 2020
@aleron75
Copy link
Contributor

aleron75 commented Apr 3, 2020

Hello @quangdo-aligent first of all, thanks for your contribution, this seems to be a nasty bug.

Due to Magento Definition of Done, all code must be covered by tests.

For this specific case, you should cover your fix by automated tests with the scenario which leads to an issue. Tests should fail on the mainline and pass with your fix.

To answer the question "which kind of tests should I write", the answer is:
pick the most lightweight (execution time-wise) test type that will provide sufficient coverage.

In Magento codebase, you already find two kinds of test for this class:

  • \Magento\Catalog\Test\Unit\Ui\DataProvider\Product\Form\Modifier\CategoriesTest
  • \Magento\Catalog\Ui\DataProvider\Product\Form\Modifier\CategoriesTest

So my suggestion would be to try with a unit test.

Thank you again!

@aleron75 aleron75 self-assigned this Apr 3, 2020
@ihor-sviziev ihor-sviziev removed their request for review April 3, 2020 12:20
@aleron75 aleron75 added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests and removed Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests labels Apr 7, 2020
@aleron75
Copy link
Contributor

@magento run all tests

@aleron75
Copy link
Contributor

Hello @quangdo-aligent can you please check why some tests fail?
Thank you!

@aleron75
Copy link
Contributor

@magento run all tests

aleron75
aleron75 previously approved these changes Apr 15, 2020
Copy link
Contributor

@aleron75 aleron75 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @quangdo-aligent all tests passed, well done!

@ghost ghost moved this from Pending Review to Ready for Testing in Pull Requests Dashboard Apr 15, 2020
@magento-engcom-team
Copy link
Contributor

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

@engcom-Alfa engcom-Alfa moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard May 21, 2020
@engcom-Alfa
Copy link
Contributor

✔️ QA Passed
The result the same as in comment

@engcom-Alfa engcom-Alfa moved this from Testing in Progress to Extended Testing (optional) in Pull Requests Dashboard May 21, 2020
@slavvka slavvka added the QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) label May 22, 2020
@engcom-Alfa engcom-Alfa added QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope and removed QA: Ready to add to Regression Scope Should be analyzed and added to Regression Testing Scope(if applicable) labels Jun 12, 2020
@ghost ghost added the Priority: P0 This generally occurs in cases when the entire functionality is blocked. label Jun 15, 2020
@okorshenko okorshenko removed the Priority: P0 This generally occurs in cases when the entire functionality is blocked. label Jun 15, 2020
@engcom-Kilo engcom-Kilo moved this from Extended Testing (optional) to Merge in Progress in Pull Requests Dashboard Jun 17, 2020
@ghost ghost moved this from Merge in Progress to Ready for Testing in Pull Requests Dashboard Jun 17, 2020
@ghost ghost moved this from Ready for Testing to Testing in Progress in Pull Requests Dashboard Jun 17, 2020
@ghost ghost moved this from Testing in Progress to Merge in Progress in Pull Requests Dashboard Jun 17, 2020
@magento-engcom-team magento-engcom-team merged commit 45f34a2 into magento:2.4-develop Jun 24, 2020
@m2-assistant
Copy link

m2-assistant bot commented Jun 24, 2020

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

@ghost ghost moved this from Merge in Progress to Recently Merged in Pull Requests Dashboard Jun 24, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix bugfix Component: Catalog Partner: Aligent Consulting partners-contribution Pull Request is created by Magento Partner Priority: P2 A defect with this priority could have functionality issues which are not to expectations. Progress: accept QA: Added to Regression Scope Scenario was analysed and added to Regression Testing Scope Release Line: 2.4 Severity: S1 Affects critical data or functionality and forces users to employ a workaround.
Projects
Pull Requests Dashboard
  
Recently Merged
Development

Successfully merging this pull request may close these issues.

[Issue] Add ACL role ID to category tree cache id
9 participants