Skip to content

MAGETWO-61422 Respect Category Top Navigation Max Depth setting #12640

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

Conversation

arnoudhgz
Copy link
Contributor

@arnoudhgz arnoudhgz commented Dec 11, 2017

Description

The setting Stores > Configuration > Catalog > Catalog > Category Top Navigation > Maximal Depth was actually never being used to limit the top menu maximum depth.

Now the setting will be used as a condition for the category collection when it is fetched.

Fixed Issues

  1. Category Top Navigation / Maximal Depth configuration not working #7543: Category Top Navigation / Maximal Depth configuration not working

Manual testing scenarios

  1. Change the setting in the backend
  2. Refresh the invalidated cache
  3. See that the setting actually is being used

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)

arnoudhgz and others added 2 commits December 10, 2017 21:37
The setting `Stores > Configuration > Catalog > Catalog > Category Top Navigation > Maximal Depth` was actually never being used to limit the topmenu.

Now the setting will be used as a condition for the category collection when it is fetched.
…x-depth-configuration

Respect Category Top Navigation Max Depth setting
@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Dec 11, 2017

CLA assistant check
All committers have signed the CLA.

@arnoudhgz
Copy link
Contributor Author

@lenlorijn can you sign the 'Contributor License Agreement'?

@magento-engcom-team magento-engcom-team added bugfix Component: Catalog Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release labels Dec 11, 2017
@mzeis mzeis self-assigned this Dec 11, 2017
@mzeis mzeis added this to the December 2017 milestone Dec 11, 2017
@lenlorijn
Copy link

I signed the Contributor License Agreement :)

@mzeis
Copy link
Contributor

mzeis commented Dec 21, 2017

FYI: I will fix the Travis error regarding maximum line length after the holidays. Suppressing CouplingBetweenObjects in this case is ok because it would mean a major refactoring for the frequently used parent classes.

Copy link
Contributor

@mzeis mzeis left a comment

Choose a reason for hiding this comment

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

@arnoudhgz I hope you had a good start into the new year! I finally got around to check your PR and think there might be a change necessary to make this work as intended.

Please can you have a look at the code review?

Thanks!

ScopeInterface::SCOPE_STORE
);
if ($navigationMaxDepth > 0) {
$this->addLevelFilter($navigationMaxDepth);
Copy link
Contributor

Choose a reason for hiding this comment

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

In my test, this looks like it doesn't work as intended. If I set a maximum depth of 1, no category is shown. If I set the value to 2, the top level category is displayed. If we add 1 to $navigationMaxDepth like $this->addLevelFilter($navigationMaxDepth + 1), it works like I would expect it to. I guess that's because the root category level also has to be considered.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No, that seems not like a good solution to me. I think we should adhere to the levels on http://docs.magento.com/m2/ee/user_guide/catalog/navigation-top.html

So in the admin configuration you have to use 2 for the first level.

Copy link
Contributor

Choose a reason for hiding this comment

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

You are absolutely right. Thank you for pointing this out. The PR works as intended.

@okorshenko okorshenko modified the milestones: December 2017, January 2018 Jan 8, 2018
@okorshenko okorshenko self-assigned this Jan 8, 2018
@magento-team magento-team merged commit 15523e8 into magento:2.3-develop Jan 9, 2018
@magento-engcom-team magento-engcom-team added the Fixed in 2.3.x The issue has been fixed in 2.3 release line label Jan 9, 2018
@mzeis
Copy link
Contributor

mzeis commented Jan 9, 2018

Thanks for your contribution @arnoudhgz! It has been merged into 2.3-develop.

@arnoudhgz arnoudhgz deleted the bugfix/MAGETWO-61422-category-max-depth-configuration branch August 10, 2018 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bugfix Component: Catalog Fixed in 2.3.x The issue has been fixed in 2.3 release line Progress: accept Release Line: 2.3 Reproduced on 2.1.x The issue has been reproduced on latest 2.1 release Reproduced on 2.2.x The issue has been reproduced on latest 2.2 release Reproduced on 2.3.x The issue has been reproduced on latest 2.3 release
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants