Skip to content

539 add apply to property #540

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

Conversation

ProkopovVitaliy
Copy link
Contributor

Description (*)

Added possibility set apply_to property for product eav attribute.

There are two variants how to use this property in data patch generation:

  1. Apply attribute for all available product types in the system:
    Screenshot 2021-04-12 at 13 53 43
    If Apply to all products types selected no specific product type will be specified for the attribute.

Screenshot 2021-04-12 at 13 54 15

  1. Specify product types:

Unselect Apply to all products types, and it will be possible to specify product types:
Screenshot 2021-04-12 at 13 55 11

Screenshot 2021-04-12 at 13 55 29

After that will be generated data patch with specified product types:

Screenshot 2021-04-12 at 13 55 44

Fixed Issues (if relevant)

  1. Fixes Eav attribute generation for specified products types. #539

Questions or comments

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 integration/functional tests (if applicable)
  • All automated tests passed successfully (all builds are green)

Copy link
Contributor

@eduard13 eduard13 left a comment

Choose a reason for hiding this comment

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

Hi @ProkopovVitaliy, thank you for the provided changes. But I guess providing a multiple select with available product types, would be much more user friendly. Additionally, we'll avoid entering some typos within that field.

Please let me know your thoughts.

Thank you.

@ProkopovVitaliy
Copy link
Contributor Author

ProkopovVitaliy commented Apr 12, 2021

Hi @ProkopovVitaliy, thank you for the provided changes. But I guess providing a multiple select with available product types, would be much more user friendly. Additionally, we'll avoid entering some typos within that field.

Please let me know your thoughts.

Thank you.

Hi @eduard13 Yes you are right, this is more user friendly. But if developer have custom product type in their store, for example sellable, we cannot predict such options and it can be some restriction for interact with our dialog window.So that why i added "help text block" which contain default types:

Enter comma separated product types for which attribute will available, for example: simple,configurable,bundle,virtual,downloadable,category. Or leave this property blank if you want apply attribute for all product types.

What about your opinion ?

@DmitryFurs
Copy link
Contributor

@ProkopovVitaliy @eduard13 Magento provides product_types.xml file that describes the product types. Is it possible to implement support for this file for the apply_to option? When typing manually, there is a chance of making a typo. There may also be third-party product types. If there was multiselect option, it would be very convenient. Thanks!

https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/Downloadable/etc/product_types.xml#L9
https://github.com/magento/magento2/blob/2.4-develop/app/code/Magento/ConfigurableProduct/etc/product_types.xml#L9

@VitaliyBoyko
Copy link
Contributor

@ProkopovVitaliy
So we can add a file indexer for product_types.xml, and build options accordingly.

@ProkopovVitaliy
Copy link
Contributor Author

ProkopovVitaliy commented Apr 13, 2021

@eduard13 I finalized this functionality. Could you please review it?

Main points:

  • Added indexer for product_types.xml
  • Added multi-select for apply_to property that based on parsed result from product_types.xml files.

Overview:

  1. Create attribute for all product types:
    Screenshot 2021-04-13 at 14 29 38
    Screenshot 2021-04-13 at 14 29 52
    There are no changes.

  2. Create attribute for specified product types:
    Screenshot 2021-04-13 at 14 27 33
    Screenshot 2021-04-13 at 14 28 32
    As we can see here added new multi-select field with product types.

  3. And the new one. When we decide to create new product type we will see him in the multi-select:
    Create new product type:
    Screenshot 2021-04-13 at 14 33 08

Run reindex for plugin:
Screenshot 2021-04-13 at 14 33 24

Open window for generation and try generate attribute for new created product type:
Screenshot 2021-04-13 at 14 35 34
Screenshot 2021-04-13 at 14 35 50

Comment on lines +75 to +76
mappedAttributes.put(EavAttribute.APPLY_TO.getAttribute(),
wrapStringValueForTemplate(eavEntityData.getApplyTo()));
Copy link
Contributor

@eduard13 eduard13 Apr 15, 2021

Choose a reason for hiding this comment

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

@ProkopovVitaliy, just want to bring your attention, that breaking a multiple arguments to a new line, should move all the arguments to a new empty line, like following.

Suggested change
mappedAttributes.put(EavAttribute.APPLY_TO.getAttribute(),
wrapStringValueForTemplate(eavEntityData.getApplyTo()));
mappedAttributes.put(
EavAttribute.APPLY_TO.getAttribute(),
wrapStringValueForTemplate(eavEntityData.getApplyTo())
);

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@eduard13 Thanks for recommendation, really it's looks better in your variant. I will take this into account.

Thanks!

Copy link
Contributor

@eduard13 eduard13 left a comment

Choose a reason for hiding this comment

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

No action is needed here, just wanted to add a remark, it also applies to PHP code.

Copy link
Contributor

@eduard13 eduard13 left a comment

Choose a reason for hiding this comment

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

Thank you for the feature 👍.

@eduard13 eduard13 merged commit b30c699 into magento:mainline-eav-attr-code-genearators Apr 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants