Skip to content
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

GUI to edit "Category Attributes" #2264

Closed
wants to merge 15 commits into from
Closed

GUI to edit "Category Attributes" #2264

wants to merge 15 commits into from

Conversation

fballiano
Copy link
Contributor

This PR give backend users the possibility to create/edit/delete "category attributes".

This feature was developed mostly using the code of the "product attributes" edit feature, that's why I've copied the license in the file header.

Notes:

  • It's a big PR so suggestions/fixes/additions are welcome.
  • We'd probably need to block the user from editing/deleting "system" attributes.
  • At the moment the newly created attributes will not appear in the "category edit" masks nor on the frontend.

Manual testing scenarios (*)

  1. open the backend
  2. open the menu "Catalog -> Attributes -> Manage Category Attributes"
  3. Add, edit or delete category attributes

Screenshots

The new menu element
Schermata 2022-06-29 alle 13 32 38

Category attributes grid
Schermata 2022-06-29 alle 13 32 52

Add a new attribute
Schermata 2022-06-29 alle 13 33 00

@github-actions github-actions bot added Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Template : admin Relates to admin template labels Jun 29, 2022
@addison74
Copy link
Contributor

... another important feature missing from Magento. I like such modules that improve functionality. Don't stop your PR's here :)

Copy link
Contributor

@luigifab luigifab 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 so much for this missing feature. I will test it at production level and deploy it!

@justinbeaty
Copy link
Contributor

Very interesting feature! Pulled into local env and playing around with it.

@justinbeaty
Copy link
Contributor

justinbeaty commented Jul 12, 2022

I added some changes to your branch @fballiano here: https://github.com/fballiano/openmage/pull/2

Mainly, I copy pasted the Attribute Set editor so you can actually see the custom attributes on the category edit page.

image

image

image

image

Still probably needs work and testing before this feature hits a release, but I found it interesting enough to help working on. For example, should users be able to even create a different attribute set for categories? If so, then they should be able to set a category to a different set, but what does this accomplish?

One more note, maybe consider that we should update the database with this:

UPDATE eav_attribute SET frontend_input = 'boolean' WHERE source_model = 'eav/entity_attribute_source_boolean' AND entity_type_id = 3;

because right now many system attributes are using select when it should be boolean. So, it shows the option to "Manage Options" of those attributes when it should not be possible.

Edit: maybe a few other manual upgrade SQL commands:

DELETE eag.* FROM eav_attribute_set eas LEFT JOIN eav_attribute_group eag ON eas.attribute_set_id = eag.attribute_set_id WHERE eas.entity_type_id = 3 AND eag.attribute_group_name = 'General';
UPDATE eav_attribute_set eas LEFT JOIN eav_attribute_group eag ON eas.attribute_set_id = eag.attribute_set_id SET eag.default_id = 1 WHERE eas.entity_type_id = 3 AND eag.attribute_group_name = 'General Information';

Because by default, Magento created a "General" group for each attribute set, but then they did not use the "General" group for categories, they used "General Information"... So this deletes the General one and makes General Information default. This could be destructive maybe, so it could be better to first check if General had any attributes set to it (probably via PHP code). Someone would have to done this manually since the GUI never existed.

@fballiano
Copy link
Contributor Author

Thanks!! I'll check it out ASAP!

@justinbeaty
Copy link
Contributor

justinbeaty commented Jul 12, 2022

I kind of wonder if we can abstract this code and re-use for all EAV entities? (Except product, probably best to leave that as it is)

mysql> select entity_type_id,entity_type_code from eav_entity_type;
+----------------+------------------+
| entity_type_id | entity_type_code |
+----------------+------------------+
|              3 | catalog_category |
|              4 | catalog_product  |
|              7 | creditmemo       |
|              1 | customer         |
|              2 | customer_address |
|              6 | invoice          |
|              5 | order            |
|              8 | shipment         |
+----------------+------------------+
8 rows in set (0.00 sec)

Edit: I'm having some success with generic GUI code in the Mage/Eav module that may be able to simplify this and #2260. I will keep testing and see what I can come up with.

@fballiano
Copy link
Contributor Author

a generic EAV module would be possibile and nice, I didn't want to use the "catalog" classes for that not to couple this new functionality to the catalog module, but if we do it from scratch in a generic way and remove the current implementation from the catalog module... that'd be awesome (althought removing the current one should be considered BC so v20...)

@justinbeaty
Copy link
Contributor

@fballiano When you say the current implementation in the catalog module, do you mean for the manage product attributes / sets?

If so, I think that is okay to stay because the product attributes have a lot of special conditions on it such as how it's used in search, etc.

@fballiano
Copy link
Contributor Author

also customer attributes have the same things although different, like to show an attribute in a particular form (subscribe in checkout or others), so I that may be needed for all the different EAV components.

anyway I was referring to extending the "catalog module" classes, that would be kinda wrong because it would make the EAV dependant on catalog.

@justinbeaty
Copy link
Contributor

Agreed on not Eav should not depend on Catalog, that's definitely wrong. I think I have a pretty good abstraction which I'll push as a commit on my personal branch soon today and link it here.

Also yes about used_in_forms part. What I think is best then is to have the base code in Mage_Eav and then extend it in Mage_Customer to add such functionality. I probably won't tackle making the products attribute code in Mage_Catalog extend these new base classes in Mage_Eav, but I suppose that could be done.

@justinbeaty
Copy link
Contributor

@fballiano I just pushed my progress as a new PR #2317 to make it easier for comments or reviews. I think this general purpose solution is the way to go, but am interested in your feedback.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Component: Adminhtml Relates to Mage_Adminhtml Component: Catalog Relates to Mage_Catalog Don't forget this PR Template : admin Relates to admin template
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants