139: Show only active categories #164
Conversation
|
|
|
Pls check |
Hello Valeriy , I tested default behaviour on develop "2.3-develop" branch without my fix . Query from test and answer So category "Category 1.1.1" is nested in category ""Category 1.2" but this is not true . and "Category 12" doesn't have child nodes in tree, but in answer we have it . |
| * @param \Iterator $iterator | ||
| * @return array | ||
| */ | ||
| private function processTree(\Iterator $iterator) : array |
There was a problem hiding this comment.
Why is this code not necessary anymore?
There was a problem hiding this comment.
I guess it was mergin with 2.3-develop, cause currently I dont see processTree in https://github.com/magento/graphql-ce/blob/2.3-develop/app/code/Magento/CatalogGraphQl/Model/Resolver/Products/DataProvider/CategoryTree.php
…hilds are always coming to last parent. Rewritte way of tree generation
| } | ||
|
|
||
| return $tree; | ||
| } | ||
|
|
||
| /** | ||
| * Merge together complex categories tree |
There was a problem hiding this comment.
How about categories tree => category trees ?
Same regarding method name itself.
| * @param $index | ||
| * @return array | ||
| */ | ||
| private function generateLevelTree($elements, $index): array |
There was a problem hiding this comment.
Method name is not reflecting its purpose, need to read the docs to understand what it is doing. Please consider improving method name.
|
@paliarush Tests are there and aligned, just pushed last update |
| $pathElements = explode("/", $category->getPath()); | ||
| $this->iteratingCategory = $category; | ||
|
|
||
| $currentLevelTree = $this->explodePathToArray($pathElements, self::START_CATEGORY_FETCH_LEVEL); |
There was a problem hiding this comment.
@paliarush @nuzil
This changes will be broke current logic
Query:
{
category(id: 334) {
id
name
}
}
The result before changes:
{
"data": {
"category": {
"id": 334,
"name": "Cat-1"
}
}
}
The result after changes:
{
"data": {
"category": {
"id": 2,
"name": null
}
}
}
The root category has always been loaded first.
Looks like we need to change
const START_CATEGORY_FETCH_LEVEL = 1;
on
const START_CATEGORY_FETCH_LEVEL = 2;
-- Fix static tests
…graphql-ce into 139-show-only-active-categories
|
Hey @naydav, you was close. reason in different deepness level calculation for each category. I extended code and also added one additional test for this case |
|
@nuzil From the quick glance this fix is still missing sorting by the position. If you re-arrange the categories the tree in GraphQl will not match without this filter. That is implemented here https://github.com/magento/graphql-ce/pull/257/files#diff-b63924c5648c0a0b6c687d44f7bb7db3R105 @naydav and @paliarush I think my solution for the ExtractDataFromCategoryTree.php is simpler and it also has unit tests to cover the basics of the implementation. |
|
@magento-engcom-team give me test instance |
…active-categories
…graphql-ce into 139-show-only-active-categories
|
We have static tests fail |
Remove dependency
…graphql-ce into 139-show-only-active-categories
|
Hi @naydav , I fixed test and removed dependency, let me know if there are some other changes required |
|
Hi @nuzil, thank you for your contribution! |
Description
We fixed this issue with this PR
https://app.zenhub.com/workspace/o/magento/graphql-ce/issues/139
https://app.zenhub.com/workspace/o/magento/graphql-ce/issues/246
So currently only Active categories will be delivered with request
Contribution checklist