Skip to content
This repository has been archived by the owner on Dec 19, 2019. It is now read-only.

Fix regexp when filter by root category id #121

Merged

Conversation

mhhansen
Copy link
Contributor

@mhhansen mhhansen commented Jul 12, 2018

Description

At present, if you query the category tree requesting the Root Category (id = 1), query returns empty.
This is caused by the regular expression that filters the catalog_category_entity.path column table, which includes for all cases, a slash (/) at the beginning.
In the case of the Root Category, there's no slash at the beginning.

This has been tested for this scenarios:

  • A fresh 2.3-alpha install, with no content created prior or after install.
  • A fresh 2.3-alpha install, with other Root Category created post install.
  • A fresh 2.3-alpha install, with sample-data created after installation.

For all cases, in the catalog_category_entity.path column table, categories are always created as a 1/ children.

Fixed Issues (if relevant)

Fixed issue: No issue created to this PR.
Issue related: Category tree depth calculation issue #100

Manual testing scenarios

To be able to test this PR, you have to first apply the PR #102, which fixes the category deph children param.

  1. Query the category(id: 1), to get the entire Category
  2. Expected result: get the Root Category with all the children.
  3. Actual result:

Query the Root Category

{
category(id: 1) {
    name
    path
    children_count
    children {
      id
      name
      level
      children {
        id
        name
      }
    }
  }
}

Expected result

01-expected-result

Actual result

02-actual-result

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)

@magento-cicd2
Copy link
Contributor

magento-cicd2 commented Jul 12, 2018

CLA assistant check
All committers have signed the CLA.

@mhhansen mhhansen changed the title Fix regexp by removing first slash when filter by root category id Fix regexp when filter by root category id Jul 12, 2018
@@ -94,8 +94,17 @@ public function getTree(ResolveInfo $resolveInfo, int $rootCategoryId) : array
$this->joinAttributesRecursively($collection, $categoryQuery);
$depth = $this->depthCalculator->calculate($categoryQuery);
$level = $this->levelCalculator->calculate($rootCategoryId);

// If root category is being filter, we've to remove first slash
if ($rootCategoryId == 1) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like we need to use \Magento\Catalog\Model\Category::TREE_ROOT_ID instead of 1

Copy link
Contributor Author

Choose a reason for hiding this comment

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

that's exactly what I didn't found when I did that fix..
awesome! I will be pushing the update

@naydav naydav added the requires-tests PRs which require GraphQL tests label Aug 21, 2018
@mhhansen
Copy link
Contributor Author

@naydav welcome back!
I remember we talk that this one would require to break the class down (or something like that),
to avoid coupling 13 deps in the CategoryTree. ping me at anytime!

Valeriy Nayda added 2 commits October 1, 2018 13:36
@magento-engcom-team
Copy link
Contributor

@mhhansen thank you for contributing. Please accept Community Contributors team invitation here to gain extended permissions for this repository.

Valeriy Nayda added 4 commits October 1, 2018 14:03
…ee-root-regexp

# Conflicts:
#	app/code/Magento/CatalogGraphQl/Model/Resolver/Products/DataProvider/CategoryTree.php
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Imported Release Line: 2.3 requires-tests PRs which require GraphQL tests
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants