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

FIX #14080 Added improvements to Category repository (save method) #27304

Conversation

sergiy-v
Copy link
Contributor

Description

Added improvements to category repository (save method) to fix level issue.

Fixed Issues (if relevant)

#14080

Manual testing scenarios

See #14080 issue description for details.

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
Copy link

m2-assistant bot commented Mar 16, 2020

Hi @sergiy-v. 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.

@magento-engcom-team magento-engcom-team added Component: Catalog Release Line: 2.4 Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner labels Mar 16, 2020
@sergiy-v sergiy-v linked an issue Mar 16, 2020 that may be closed by this pull request
@lbajsarowicz lbajsarowicz changed the title 14080 Added improvements to category repository (save method) FIX #14080 Added improvements to Category repository (save method) Mar 16, 2020
@lbajsarowicz
Copy link
Contributor

Related PR #27291

@sergiy-v
Copy link
Contributor Author

@magento run all tests

@eduard13 eduard13 self-assigned this Mar 17, 2020
@eduard13
Copy link
Contributor

@magento run all tests

@sergiy-v
Copy link
Contributor Author

@eduard13 PR is ready to review, the functional tests errors are not related to the changes.
Thank you.

lbajsarowicz
lbajsarowicz previously approved these changes Mar 17, 2020
@magento-engcom-team
Copy link
Contributor

Hi @lbajsarowicz, thank you for the review.
ENGCOM-7112 has been created to process this Pull Request
✳️ @lbajsarowicz, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

@lbajsarowicz
Copy link
Contributor

@sergiy-v I really like the fact that you covered that change with MFTF tests. However (!) I'm not sure if it's the best approach to cover that kind of change. What I mean is that in this specific case Integration Tests would give cleaner overview what is happening under the hood.

Thanks once again for well-done Functional Test.

@sergiy-v
Copy link
Contributor Author

@sergiy-v I really like the fact that you covered that change with MFTF tests. However (!) I'm not sure if it's the best approach to cover that kind of change. What I mean is that in this specific case Integration Tests would give cleaner overview what is happening under the hood.

Thanks once again for well-done Functional Test.

@lbajsarowicz Thanks for your thoughts, will keep it in mind next time)

eduard13
eduard13 previously approved these changes Mar 17, 2020
@eduard13 eduard13 added Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Award: test coverage labels Mar 17, 2020
@magento-engcom-team
Copy link
Contributor

Hi @eduard13, thank you for the review.
ENGCOM-7112 has been created to process this Pull Request
✳️ @eduard13, could you please add one of the following labels to the Pull Request?

Label Description
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests
Auto-Tests: Not Covered Changes in Pull Request requires coverage by auto-tests
Auto-Tests: Not Required Changes in Pull Request does not require coverage by auto-tests

Copy link
Contributor

@engcom-Alfa engcom-Alfa left a comment

Choose a reason for hiding this comment

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

Hi, @sergiy-v

During testing, we faced the issue.

Problem: The category is saved with the wrong url_path

Manual testing scenario:
1. Create any category on the second level (with parent_id =2).
Name: Test category 1, Url key: test-category-1.
2. Create subcategory in "Test category 2" with level = 2 by rest api:
/V1/categories

{
	"category": {
		"name": "Test category 2",
		"parent_id":"3",  /* Test category 1 */
		"is_active":"true",
		"include_in_menu": true,
		"level":"2",
		"custom_attributes": [{ "attribute_code": "url_key", "value": "test-category-2" }]
	}
}

Actual Result:
✖️ Now we have the url_key: "test-category-2" and path: "test-category-2"
Screenshot from 2020-03-17 15-45-36

Expected Result:
✔️ The url_path should be "/test-category-1/test-category-2"

3. Change the URL key of the Test category 2 either in the backend or via the api to something like "test-category-2-changed"

Actual Result:

✖️ Category "Test category 2" has attribute values:
url_key: "test-category-2-changed"
url_path: "test-category-2-changed"
Screenshot from 2020-03-17 16-10-58
Or Am I wrong?

@sergiy-v Could you take a look?

Thanks!

@engcom-Alfa
Copy link
Contributor

@sergiy-v Thank you for the clarification!
I checked again and everything works correctly.
Thanks!

@engcom-Alfa
Copy link
Contributor

@lbajsarowicz @eduard13 Could you approve again?
Thank you!

@magento-engcom-team
Copy link
Contributor

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

@engcom-Alfa
Copy link
Contributor

✔️ QA Passed

Before:
before

After:
after

Copy link
Contributor

@lbajsarowicz lbajsarowicz left a comment

Choose a reason for hiding this comment

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

✔️ Thank you for your contribution.

@magento-engcom-team
Copy link
Contributor

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

@engcom-Echo
Copy link
Contributor

Failed functional tests not related to the changes in this PR

@magento-engcom-team magento-engcom-team merged commit 92a4477 into magento:2.4-develop Mar 21, 2020
@m2-assistant
Copy link

m2-assistant bot commented Mar 21, 2020

Hi @sergiy-v, 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
Auto-Tests: Covered All changes in Pull Request is covered by auto-tests Award: bug fix Award: test coverage Component: Catalog Partner: Atwix Pull Request is created by partner Atwix partners-contribution Pull Request is created by Magento Partner Progress: accept Release Line: 2.4
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Category path is the same as key producing duplicate URL issue
6 participants