Skip to content

Add option to index child attributes or not#2443

Open
fredden wants to merge 7 commits into
Smile-SA:2.10.xfrom
fredden:include-child-attributes-toggle
Open

Add option to index child attributes or not#2443
fredden wants to merge 7 commits into
Smile-SA:2.10.xfrom
fredden:include-child-attributes-toggle

Conversation

@fredden

@fredden fredden commented Feb 9, 2022

Copy link
Copy Markdown
Contributor

While working with bundle products on a website, the merchant wants to only include attributes of the parent/bundle product not any of its options/children. This configuration option adds the ability for website administrators to make this decision. By default, the existing behaviour is preserved.

@romainruaud

Copy link
Copy Markdown
Collaborator

Hi @fredden,

having such a config globally is not enough for me, I'd like to be able to also control which attributes are mergeable, and which are not, as it's done "hardcoded" actually, but with the ability to inject the list through DI.

Can you rework this PR ? Anyway, thank you for proposing it.

Best regards

@fredden

fredden commented Feb 15, 2022

Copy link
Copy Markdown
Contributor Author

Hi @romainruaud. Thanks for the feedback.

Making the list of forbidenChildrenAttributeCodes configurable sounds like a good idea. If that feature existed, we may have been able to use that instead of the approach here.

I think having the setting exposed in the admin makes more sense than via dependency injection though. This way, if a website administrator adds a new attribute, they'll be able to select its index status right away, and not require a deployment to set this.

With an admin configuration option per attribute, I can see it being a chore marking each and every attribute individually as indexable or not; having a global toggle would help then - which is what's being introduced here.

What do you think about putting this global toggle in for now, and working on getting the list configurable as a separate task?

@robaimes

robaimes commented May 6, 2022

Copy link
Copy Markdown

@fredden Leaving this here for notes, but following the merge of #2465, this file requires it's constructor modifying to also accommodate for the new Config parameter.

@fredden

fredden commented May 6, 2022

Copy link
Copy Markdown
Contributor Author

Thanks @robaimes. I've updated this pull request to be compatible with the changes introduced in #2465.

@romainruaud I still think that this should be merged, and the list of attributes should be set in admin configuration (not via dependency injection) as a separate task / feature.

@fredden

fredden commented Aug 22, 2022

Copy link
Copy Markdown
Contributor Author

Hi @rbayet. What can I do to help get this merged in?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants