-
Notifications
You must be signed in to change notification settings - Fork 9.4k
30349: Product filter with category_id does not work as expected - fi… #30375
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
30349: Product filter with category_id does not work as expected - fi… #30375
Conversation
…xed total_count and items issues
Hi @sudheers-kensium. Thank you for your contribution
❗ Automated tests can be triggered manually with an appropriate comment:
You can find more information about the builds here ℹ️ Please run only needed test builds instead of all when developing. Please run all test builds before sending your PR for review. For more details, please, review the Magento Contributor Guide documentation. 🕙 You can find the schedule on the Magento Community Calendar page. 📞 The triage of Pull Requests happens in the queue order. If you want to speed up the delivery of your contribution, please join the Community Contributions Triage session to discuss the appropriate ticket. 🎥 You can find the recording of the previous Community Contributions Triage on the Magento Youtube Channel ✏️ Feel free to post questions/proposals/feedback related to the Community Contributions Triage process to the corresponding Slack Channel |
@@ -61,7 +61,6 @@ public function apply(Filter $filter, AbstractDb $collection) | |||
$category = $this->categoryFactory->create(); | |||
$this->categoryResourceModel->load($category, $categoryId); | |||
$categoryProducts[$categoryId] = $category->getProductCollection()->getAllIds(); | |||
$collection->addCategoryFilter($category); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sudheers-kensium. Thank you for your contribution. I'm not sure that by removing this filter we won't introduce an additional issue. This filter has been recently added for solving the sorting issue. Details are below
dfcf474#diff-8a27c9476b188f4e8830e9872984bdfbR64
@cpartica could you provide more information on the consequences, please? Unfortunately, I have a lack of details about MC-32278.
Thank you!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rogyar I have changed the code. Please check once.
Hi @sudheers-kensium, can you please look at #30375 (comment)? Otherwise, we can't proceed with your PR. |
…xed query returns missing items
@magento run all tests |
Hi @sudheers-kensium. Thanks for the adjustments. Now it looks much better. Thank you! |
…ded API functional test
Hi @rogyar. I have added API-functional test. Please check once. |
@magento run all tests |
@magento run integration tests |
@magento run Integration Tests |
@magento import PR to magento-honey-badgers/magento2ce |
@dthampy the branch with code successfully imported into |
Hi @rogyar, thank you for the review. |
@magento import PR to magento-honey-badgers/magento2ce |
@cpartica the branch with code successfully imported into |
Hi @sudheers-kensium, thank you for your contribution! |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@rogyar @cpartica and @prabhuram93 could you please review my observation
@@ -113,7 +113,7 @@ public function getList( | |||
$searchResults = $this->searchResultsFactory->create(); | |||
$searchResults->setSearchCriteria($searchCriteriaForCollection); | |||
$searchResults->setItems($collection->getItems()); | |||
$searchResults->setTotalCount($searchResult->getTotalCount()); | |||
$searchResults->setTotalCount($collection->getSize()); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @sudheers-kensium ,
Here why we have changed $searchResult->getTotalCount() to $collection->getSize() ?
Actually my concern is when we call getSize() on collection then it will execute one sql query to the database and it will take some time but in $searchResult is itself an object and all the data is present in that object. so, to execute $searchResult->getTotalCount() system will take negligible time where as $collection->getSize() will take some time.
I checked the performance of both code by adding below code in /vendor/magento/module-catalog-graph-ql/Model/Resolver/Products/DataProvider/ProductSearch.php file of "getList" method after "$this->collectionPostProcessor->process($collection, $attributes);" code and checked the response time of both getSize and getTotalCount method.
$time1 = microtime(true);
echo "Total Count: ".$collection->getSize()."
";
$time2 = microtime(true);
echo "time taken in getSize is: ".($time2- $time1)."
";
$time3 = microtime(true);
echo "Total Count: ".$searchResult->getTotalCount()."
";
$time4 = microtime(true);
echo "time taken in getTotalCount is: ".($time4 - $time3);
Response on my local :
Total Count: 32310
time taken in getSize is: 3.1487860679626
Total Count: 32310
time taken in getTotalCount is: 3.4093856811523E-5
@magento give me 2.4-develop instance |
Hi @rani-webkul. Thank you for your request. I'm working on Magento instance for you. |
Hi @rani-webkul, here is your Magento Instance: https://ba7f2d19bd12e7c6503a3b919866d314-2-4-develop.instances.magento-community.engineering |
Hi @rogyar, could you please check the above observation by @rani-webkul? Looks like it might cause performance issues when scaling up. |
Product filter with category_id does not work as expected
Description (*)
{ products(filter: {category_id: {in: ["<cat1>", "<cat2>"]}}) { items { sku name } total_count } }
Related Pull Requests
Fixed Issues (if relevant)
Manual testing scenarios (*)
Questions or comments
Contribution checklist (*)