solidusio / solidus

🛒 Solidus, the open-source eCommerce framework for industry trailblazers.
https://solidus.io
Other
4.97k stars 1.29k forks source link

[Admin][Settings] Introduce role creation and modification capability #5823

Open MadelineCollier opened 1 month ago

MadelineCollier commented 1 month ago

Description

Introduce a new interface for creating and editing roles.

Features

Figma Design

Storyboard

Image

MadelineCollier commented 4 weeks ago

Going to repost my questions form this PR for visibility!

Additional Questions

This is the first piece for the new [Admin][Settings] Introduce role creation and modification capability ticket.

Example designs:

Screenshot 2024-08-14 at 7 43 40 PM

Spree::Role currently lacks:

I am assuming that to support the example designs, I will be adding those attributes to the Spree::Role model in a future PR, but I am unsure whether that should go in solidus_admin or whether that should be added to core.

Additionally, the example designs seem to differentiate between "custom" and "standard" roles. Will we be creating new stock roles with default permissions sets and adding them to core/db/default/spree/roles.rb? If so, what will those default roles and permissions be?

benjaminwil commented 4 weeks ago

Just my two cents:

MadelineCollier commented 3 weeks ago

@rainerdema @kennyadsl What do you guys think of the above? Was any of this discussed internally during the sprint planning/design stages?

kennyadsl commented 3 weeks ago

Good points @MadelineCollier and @benjaminwil.

@MadelineCollier

I think that the design in Figma is an implementation of Solidus with the solidus_user_roles gem installed. That's where the confusion comes from, I guess.

Now, it's still to be discovered how we want to get to the state described by the design. We could incorporate the code of the gem into Solidus core/admin. @MadelineCollier can you please explore if that's doable? That gem doesn't have a lot of code, I think the trickiest part will be keeping backward compatibility with people with custom roles and people who already have that gem installed.

@benjaminwil

Descriptions are valuable for stores who have many custom roles. I think it's worth adding.

👍 I also do

I do not think types add very much value.

It could be useful to differentiate the roles that come with Solidus and those made custom by the merchant, but yeah, we can live without it.

Why the checkboxes? I don't think any bulk actions add much value at this stage.

Right, I think this was more to keep all the tables uniform, we can get rid of that part

Any roles that come with Solidus or solidus_admin or a Solidus extension--or at the very least: the Admin role--should not be able to be destroyed via the web view. Alternatively, no roles should be destroyable via the web view.

Absolutely, maybe only custom roles (with no user associated) can be destroyed? That's where the type might be used.

MadelineCollier commented 3 weeks ago

I think that the design in Figma is an implementation of Solidus with the solidus_user_roles gem installed. That's where the confusion comes from, I guess.

Now, it's still to be discovered how we want to get to the state described by the design. We could incorporate the code of the gem into Solidus core/admin. @MadelineCollier can you please explore if that's doable? That gem doesn't have a lot of code, I think the trickiest part will be keeping backward compatibility with people with custom roles and people who already have that gem installed.

For sure I can look into this. Is the main backward compatibility issue just with re-running migrations on tables/columns that already exist? If so that should be easy to work around. Anything else I should look out for give me a shout but I'll get started.

Descriptions are valuable for stores who have many custom roles. I think it's worth adding.

👍 I also do

Definitely, should be pretty easy. Same question though @kennyadsl should these new additions be added to core or to solidus_admin ?

I do not think types add very much value.

It could be useful to differentiate the roles that come with Solidus and those made custom by the merchant, but yeah, we can live without it.

I agree that this is probably most valuable when comparing stock vs custom roles, so if core remains as is then it provides little value, but if solidus_user_roles gets folded into core, then i'd say it's worth implementing this, and it's not that tough to do either.

Why the checkboxes? I don't think any bulk actions add much value at this stage.

Right, I think this was more to keep all the tables uniform, we can get rid of that part

Sounds good, I can remove these once the other work is out of the way.

Any roles that come with Solidus or solidus_admin or a Solidus extension--or at the very least: the Admin role--should not be able to be destroyed via the web view. Alternatively, no roles should be destroyable via the web view.

Absolutely, maybe only custom roles (with no user associated) can be destroyed? That's where the type might be used.

Agreed that we should just make "standard" or "stock" roles non-destroyable (at least non-destroyable from the web UI). Also agree that we can and should add validations that prevent any roles with associated users from being destroyed.

kennyadsl commented 3 weeks ago

Definitely, should be pretty easy. Same question though @kennyadsl should these new additions be added to core or to solidus_admin ?

I'd say that the changes to the models and business logic should be done in core. solidus_admin should just be the UI to interact with those models. In fact, we should also update the solidus API to reflect the new changes to roles.