magento-lts icon indicating copy to clipboard operation
magento-lts copied to clipboard

GUI to Edit EAV Attributes & Sets - Customer

Open justinbeaty opened this issue 3 years ago • 18 comments

Description (*)

[WIP] This commit was previously part of #2317. It is broken out so that we can discuss ways to integrate the customer EAV attributes into customer groups, and customer forms. Discussions about this are in that PR.

Related Pull Requests

#2317 #2260

Fixed Issues (if relevant)

Manual testing scenarios (*)

Questions or comments

Todo:

  • [ ] Check translation strings
  • [x] Modify customer attributes to add use_in_forms option
  • [x] Modify Customers > Manage Groups to allow us to select an attribute set for each customer group (both customer and address set)

Contribution checklist (*)

  • [x] Pull request has a meaningful description of its purpose
  • [x] All commits are accompanied by meaningful commit messages
  • [ ] All automated tests passed successfully (all builds are green)
  • [x] Add yourself to contributors list

justinbeaty avatar Aug 01 '22 18:08 justinbeaty

In testing the use_in_forms function, it seems like adding attributes to any of the frontend forms does not work, and would involve editing template files as mentioned by fballiano here: https://github.com/OpenMage/magento-lts/pull/2317#issuecomment-1201662652

However it does seem to work perfectly in all of the adminhtml areas.

justinbeaty avatar Aug 02 '22 14:08 justinbeaty

I can confirm you that there's some work on the frontend that needs to be done for the use_in_forms to work :-)

fballiano avatar Aug 02 '22 14:08 fballiano

I've started support for use_in_forms to work on the frontend without the user having to edit templates:

image

Todo:

  • Compile the SCSS to make the date field show up correctly.
  • Date field if required doesn't pass validation even if filled 🤔
  • Add changes in app/design/frontend/rwd/default/template/customer/form/edit.phtml to app/design/frontend/base as well
  • Support the other frontend forms

justinbeaty avatar Aug 02 '22 17:08 justinbeaty

This pull request introduces 2 alerts and fixes 8 when merging e62967875febbf3d670aa7ae9e55790ef04a9b4a into cf82b8f623e754e63c9a64dbf3c21adaf46ddc56 - view on LGTM.com

new alerts:

  • 2 for Syntax error

fixed alerts:

  • 4 for Useless assignment to local variable
  • 2 for Missing variable declaration
  • 1 for Useless conditional
  • 1 for Unused variable, import, function or class

lgtm-com[bot] avatar Aug 02 '22 17:08 lgtm-com[bot]

wow!

fballiano avatar Aug 02 '22 17:08 fballiano

Working on the other frontend forms, checkout_register is a bit crowded, but it's just how Magento is with checkout & register I guess. I think a store owner should decide to not add too many fields to this form.

image

justinbeaty avatar Aug 02 '22 20:08 justinbeaty

I still have address forms to do tomorrow.

I am also thinking it might be nice to group these fields on the frontend based on the attribute set > groups. This way there is some more organization and the user can label the fieldset. For example in Account Information they could have a group called "Social Media" and let the user enter urls to their pages (just an example.)

But also, I am going to take a stab at allowing you to define attribute set per customer group. Thus it would be important that not only do you define use_in_forms but also put that attribute in a attribute set. Otherwise you cannot control what attributes show up for each customer group.

justinbeaty avatar Aug 02 '22 22:08 justinbeaty

@kiatng (and @fballiano) I'm seeking your opinion on the following (originally posted here)


I use attribute sets to organize the attributes for different customer groups. The project has over 200 customer attributes, each customer group having very different set of attributes.

My concern is that changes to EAV could make it very difficult to merge OM to existing projects with custom EAV.

The PR as it now should definitely not conflict with anything since it's all new code, but I see your point if we hook up a customer group -> customer attribute set relation. Still, I think it could be worth it maybe just waiting to merge this PR until the next major version.

How did you correlate attribute sets with customer groups? My first thought is to modify the customer_group table:

mysql> describe customer_group;
+---------------------+-------------------+------+-----+---------+----------------+
| Field               | Type              | Null | Key | Default | Extra          |
+---------------------+-------------------+------+-----+---------+----------------+
| customer_group_id   | smallint unsigned | NO   | PRI | NULL    | auto_increment |
| customer_group_code | varchar(32)       | NO   |     | NULL    |                |
| tax_class_id        | int unsigned      | NO   |     | 0       |                |
+---------------------+-------------------+------+-----+---------+----------------+
3 rows in set (0.00 sec)

To add something like customer_attribute_set_id and maybe even customer_address_attribute_set_id. Probably both are useful in the case you want to control both attribute sets per group.

If we can do it in a way that is maybe compatible with what you've done, then that's at least one site that wouldn't break.


In any case, I'll set up an observer so that users can change this logic.

justinbeaty avatar Aug 03 '22 14:08 justinbeaty

Unit Test Results

1 files  ±0  1 suites  ±0   0s :stopwatch: ±0s 0 tests ±0  0 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0  7 runs  ±0  7 :heavy_check_mark: ±0  0 :zzz: ±0  0 :x: ±0 

Results for commit cf82b8f6. ± Comparison against base commit cf82b8f6.

github-actions[bot] avatar Aug 03 '22 15:08 github-actions[bot]

I have some customer group -> attribute set integration. Here's screenshots:

image

image

So basically I made a new customer attribute set "Special Customer" and then I can add or remove certain attributes to that set. If I then set a group to use that attribute set, attributes will show or hide based on the set in adminhtml and frontend forms.

I will have some extra care to not BC break but not implemented yet.

I think this is a really nice feature because you can easily show or hide certain fields for customers on their edit account page.

Edit: err I just realized I didn't push an upgrade script to add the new columns to the database ( I just added them manually ) with this sql:

alter table customer_group add customer_attribute_set_id smallint unsigned not null default 0;
alter table customer_group add customer_address_attribute_set_id smallint unsigned not null default 0;

justinbeaty avatar Aug 03 '22 21:08 justinbeaty

Also in working on this, I realize there's a lot of eav_form_* tables which I assume were to implement similar functionality into EE. This PR doesn't use those, but it's a whole 'nother can of worms to try and implement that functionality into CE.

justinbeaty avatar Aug 03 '22 21:08 justinbeaty

@kiatng (and @fballiano) I'm seeking your opinion on the following (originally posted here)

How did you correlate attribute sets with customer groups? My first thought is to modify the customer_group table:

...

To add something like customer_attribute_set_id and maybe even customer_address_attribute_set_id. Probably both are useful in the case you want to control both attribute sets per group.

If we can do it in a way that is maybe compatible with what you've done, then that's at least one site that wouldn't break.

In any case, I'll set up an observer so that users can change this logic.

What I did 10 years ago was to add a column apply_to to the table eav_attribute_set. apply_to sets comma-delimited customer groups. I needed to also drop the unique index on array('entity_type_id', 'attribute_set_name'). It looks like this: image image

Since then, I also use the same apply_to for a custom EAV, which is not for customer group.

I do not see how you can make it compatible without using the same approach above.

kiatng avatar Aug 04 '22 02:08 kiatng

@kiatng So are you able to create two attribute sets that apply to the same customer group, but differ in the form setting? In other words, you can show different fields for the same customer group on different forms?

If so that's a nice feature that my PR doesn't have, but I am not sure it's something to introduce to the core this way. I would rather try and reverse engineer the eav_form tables.

So no this PR wouldn't be compatible with your way, but I think I can make it at least not interfere with what you've done.

justinbeaty avatar Aug 04 '22 17:08 justinbeaty

@kiatng So are you able to create two attribute sets that apply to the same customer group, but differ in the form setting? In other words, you can show different fields for the same customer group on different forms?

Yes, in fact sets 19 and 23 in the screenshot I attached are of the same customer group. The use case is for different backend users to view/edit a different set of attributes. At its height, there were a couple hundreds of backend users in this project.

There are other advantages to add apply_to at the attribute set level. Because it is upstream (EAV level), there is less coding downstream (customer level and other custom entity).

Your approach to match an attribute set at the customer group is similar to the product's attribute_set_id. However, number of customer groups (I have seen max 30 groups) is much less than the number of products (can be hundreds of thousands). It's easier to configure apply_to at the attribute set level for customer groups, but impractical for products.

But what I have is customized to a very particular use case. I only developed what is required of the project, I do not have anything on addresses and categories. Developing for general use cases is a lot more challenging.

kiatng avatar Aug 05 '22 00:08 kiatng

Will be ready for RC4?

luigifab avatar Apr 25 '23 10:04 luigifab

I wanted to revive this, I think they should target "next" but it's like my preference, because we're more free to choose how to act on the frontend.

fballiano avatar Apr 25 '23 10:04 fballiano

i've made some test on this PR:

  • for each type of attribute (custom/address), I can't set the scope value. It's always stuck on 'store view', and in the grid column, it's always null.
  • in the frontend checkout, the extra fields are not being printed

empiricompany avatar May 13 '23 10:05 empiricompany

there were some bad conflicts, I did what i could, not sure 100% was correct

fballiano avatar Jul 29 '23 23:07 fballiano