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

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

Open
MadelineCollier opened this issue Aug 13, 2024 · 6 comments
Assignees

Comments

@MadelineCollier
Copy link
Contributor

Description

Introduce a new interface for creating and editing roles.

Features

  • Implement the interface described in the design with specified fields.

Figma Design

Storyboard

Image

@MadelineCollier
Copy link
Contributor Author

MadelineCollier commented Aug 15, 2024

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:

  • Types
  • Descriptions
  • Any validation on "Name" except that it be unique (it is still allowed to be blank).

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
Copy link
Contributor

Just my two cents:

  • Descriptions are valuable for stores who have many custom roles. I think it's worth adding.
  • I do not think types add very much value.
  • Why the checkboxes? I don't think any bulk actions add much value at this stage.
  • 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.

@MadelineCollier
Copy link
Contributor Author

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

@kennyadsl
Copy link
Member

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
Copy link
Contributor Author

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
Copy link
Member

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Review
Development

No branches or pull requests

3 participants