-
Notifications
You must be signed in to change notification settings - Fork 9.3k
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
Include custom source model options in attribute index - Up port #16727 #19176
Include custom source model options in attribute index - Up port #16727 #19176
Conversation
Hi @rain2o. Thank you for your contribution
For more details, please, review the Magento Contributor Assistant documentation |
The Travis CI build failed, but it looks like it just timed out. I don't see any errors. Is there a way to re-trigger the build? |
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.
Thanks for the contribution! Please take a look at the code review comments
app/code/Magento/Catalog/Model/ResourceModel/Product/Indexer/Eav/Source.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Catalog/Model/ResourceModel/Product/Indexer/Eav/Source.php
Outdated
Show resolved
Hide resolved
…st as opposed to direct query
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.
Thanks for collaboration. Please see my review notes, also it would be great to ensure integration tests coverage of this change
app/code/Magento/Catalog/Model/ResourceModel/Product/Indexer/Eav/Source.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Catalog/Model/ResourceModel/Product/Indexer/Eav/Source.php
Outdated
Show resolved
Hide resolved
app/code/Magento/Catalog/Model/ResourceModel/Product/Indexer/Eav/Source.php
Outdated
Show resolved
Hide resolved
@sivaschenko Regarding the integration tests, do you have any suggestions for a source model? I can't think of any existing source models in the Catalog module that would work for the tests. Or would it be best to create a test source model in the test suite? |
@rain2o if you cannot find appropriate source models for the test in the code base, a test source model would be fine |
Hi @sivaschenko, thank you for the review. |
Hi @rain2o. Thank you for your contribution. |
Description
This is an up-port of #16727 from 2.2 to 2.3
When a multiselect Product Attribute uses a custom source model those values are not included when indexing the attribute values for filters on the product grid. The result is those attributes cannot be used as a filter in the product grid.
This was originally opened as issue #417 but was closed as it was considered a feature request and moved to the forums here. The solution is based on @maderlock's comment here but replaced the use of
ObjectManager
with dependency injection ofAttributeRepositoryInterface
.Fixed Issues
Manual testing scenarios
Contribution checklist