[Admin] Allow assignment of permission sets when creating/editing admin roles #5846

MadelineCollier · 2024-09-02T16:43:54Z

Note for Reviewers

This PR is part of this issue: #5823

I went with checkboxes instead of the view/edit dropdowns as they were becoming complicated to un-pack on the backend with each select having its own form input and array of permission_set_ids.

I also added a component for the CheckboxRow as that whole chunk of code repeated itself 14 times, and any other methods of handling that repetition (partial, helper) proved awful to work with and messy to implement & maintain.

The attached video shows the functionality visually.

Screen.Recording.2024-09-09.at.5.16.48.PM.mov

Screenshots:

Component preview

"All" scope

"Admin" scope

Summary

Checklist

Check out our PR guidelines for more details.

The following are mandatory for all PRs:

I have written a thorough PR description.
I have kept my commits small and atomic.
I have used clear, explanatory commit messages.

The following are not always needed:

📖 I have updated the README to account for my changes.
📑 I have documented new code with YARD.
🛣️ I have opened a PR to update the guides.
✅ I have added automated tests to cover my changes.
📸 I have attached screenshots to demo visual changes.

codecov · 2024-09-03T18:32:49Z

Codecov Report

Attention: Patch coverage is 98.34711% with 2 lines in your changes missing coverage. Please review.

Project coverage is 89.24%. Comparing base (d81043a) to head (3239325).
Report is 7 commits behind head on main.

Files with missing lines	Patch %	Lines
...lib/spree/permission_sets/configuration_display.rb	60.00%	2 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #5846      +/-   ##
==========================================
+ Coverage   89.17%   89.24%   +0.06%     
==========================================
  Files         749      753       +4     
  Lines       17395    17515     +120     
==========================================
+ Hits        15512    15631     +119     
- Misses       1883     1884       +1

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

benjaminwil

Honestly, I think having checkboxes makes more sense than dropdowns, as this maps better to the way permission sets are actually stored on users.

So I'm in favour of all these changes. As far as your helper method goes, I have some ideas. But nothing huge.

core/db/default/spree/permission_sets.rb

admin/app/helpers/solidus_admin/permission_sets_helper.rb

tvdeyen

Wow. This is amazing work 👏🏻 but I am a bit worried about the maintenance burden we have with this change. I added a lot of comments for potential solutions.

To your concerns:

Yes, checkboxes are the preferred UI for such interfaces (not sure why designers love them drop downs so much nowadays.) We won't have many new permission set types in the future.
The way we identify role sets by a class name match is not something I would like to maintain in the future. I propose a permission_type (please can someone find a better name 🙏🏻?) column on the permission set classes and the new PermissionSet model.

Again, thanks for all the work you put in the new admin.

admin/app/helpers/solidus_admin/permission_sets_helper.rb

core/db/default/spree/permission_sets.rb

core/app/models/spree/permission_set.rb

admin/app/helpers/solidus_admin/permission_sets_helper.rb

admin/app/components/solidus_admin/roles/new/component.html.erb

admin/app/components/solidus_admin/roles/edit/component.html.erb

admin/app/components/solidus_admin/roles/index/component.rb

tvdeyen

This goes in really good direction, but please be patient with me, since this role management feature is new to Solidus I want us to go an extra mile so it's thoroughly build.

admin/app/components/solidus_admin/roles/edit/component.rb

admin/app/components/solidus_admin/roles/new/component.rb

admin/app/helpers/solidus_admin/permission_sets_helper.rb

core/db/default/spree/permission_sets.rb

This is building off of the new migrations in solidusio#5833 These originate from `solidus_user_roles`. We intend to migrate some of the functionality from that extension into `core`, and this is the (second) step. Co-authored-by: benjamin wil <[email protected]>

tvdeyen

Amazing. Thanks for addressing my concerns. I found two small things, that I would like to be addressed before we finally can merge this very welcomed and well written feature.

admin/app/components/solidus_admin/ui/checkbox_row/component.html.erb

admin/app/components/solidus_admin/ui/checkbox_row/component.rb

core/app/models/spree/permission_set.rb

Later commits proved the need for an easier method of identifying and categorizing permission sets more cleanly on the front-end, which is why we added the category & privilege columns. Each subclass (eg. `Spree::PermissionSets::ConfigurationDisplay`) has its own privilege and category, which is then used to build the instance (Spree::PermissionSet) with the correct identifying features.

admin/app/components/solidus_admin/ui/checkbox_row/component.html.erb

admin/app/helpers/solidus_admin/permission_sets_helper.rb

core/spec/models/spree/permission_set_spec.rb

While I am not sure how frequently this will be re-used, the fact that it comes up 14 times across the roles/new and roles/edit pages makes this extraction worthwhile. I also tried out a partial and a helper method previously but both approaches were messy and poorly architected. This should serve us much better in the long run and be easier to maintain.

MadelineCollier · 2024-09-09T11:05:01Z

I'll just fix these failing specs and then it should be good to re-review :)

Building off of the previous commit as well as the migrations that were added to core to pull in some of the key features of solidus_user_roles we can now allow admins to assign permission sets when creating a role.

The role edit modal now also has the functionality to allow admins to assign permission sets.

I would not normally write specs to ensure associations, as that seems like it's not my responsibility to test, but Codecov seems to think otherwise and would like a spec for `Spree::RolePermission`, a model with nothing on it except some `belongs_to`s. I also added specs for the other models that were flagged. The association specs are a bit uglier than they could be if we were using the `shoulda_matchers` and their `is_expected.to belong_to` but it seems like it's not worth it to install it just to beautify 3 specs!

tvdeyen

Nice work!

kennyadsl

Super nice work! For the sake of documentation, if you can update the screen recording with the last version, that would be super nice!

MadelineCollier · 2024-09-09T15:21:38Z

Updated the screenshots and recordings, thanks for all the time put into many rounds of reviews everyone :) Much appreciated!

github-actions bot added the changelog:solidus_admin label Sep 2, 2024

MadelineCollier force-pushed the admin-role-creation-with-permissions-round-2 branch 2 times, most recently from 2b1baa5 to 751784f Compare September 3, 2024 18:09

github-actions bot added the changelog:solidus_core Changes to the solidus_core gem label Sep 3, 2024

MadelineCollier force-pushed the admin-role-creation-with-permissions-round-2 branch 2 times, most recently from dcb32c3 to 578d332 Compare September 3, 2024 18:23

MadelineCollier changed the title ~~[DRAFT] [Admin] Role creation with permissions~~ [DRAFT] [Admin] Allow assignment of permission sets when creating/editing admin roles Sep 3, 2024

MadelineCollier changed the title ~~[DRAFT] [Admin] Allow assignment of permission sets when creating/editing admin roles~~ [Admin] Allow assignment of permission sets when creating/editing admin roles Sep 3, 2024

MadelineCollier marked this pull request as ready for review September 3, 2024 19:01

MadelineCollier requested a review from a team as a code owner September 3, 2024 19:01

benjaminwil reviewed Sep 3, 2024

View reviewed changes

core/db/default/spree/permission_sets.rb Outdated Show resolved Hide resolved

admin/app/helpers/solidus_admin/permission_sets_helper.rb Outdated Show resolved Hide resolved

admin/app/helpers/solidus_admin/permission_sets_helper.rb Outdated Show resolved Hide resolved

tvdeyen reviewed Sep 4, 2024

View reviewed changes

MadelineCollier marked this pull request as draft September 4, 2024 18:36

MadelineCollier force-pushed the admin-role-creation-with-permissions-round-2 branch 5 times, most recently from 7a73e2c to 915e9f6 Compare September 5, 2024 12:27

MadelineCollier commented Sep 5, 2024

View reviewed changes

admin/app/components/solidus_admin/roles/index/component.rb Outdated Show resolved Hide resolved

MadelineCollier force-pushed the admin-role-creation-with-permissions-round-2 branch from 915e9f6 to 82f691d Compare September 5, 2024 12:56

MadelineCollier requested review from tvdeyen and benjaminwil September 5, 2024 12:58

MadelineCollier marked this pull request as ready for review September 5, 2024 12:59

MadelineCollier requested review from a team and removed request for benjaminwil September 6, 2024 07:36

tvdeyen reviewed Sep 6, 2024

View reviewed changes

MadelineCollier force-pushed the admin-role-creation-with-permissions-round-2 branch 2 times, most recently from e02310a to 3ea93ea Compare September 6, 2024 14:03

MadelineCollier force-pushed the admin-role-creation-with-permissions-round-2 branch from 3ea93ea to 618edfc Compare September 6, 2024 14:10

MadelineCollier requested a review from tvdeyen September 6, 2024 14:17

tvdeyen reviewed Sep 6, 2024

View reviewed changes

MadelineCollier force-pushed the admin-role-creation-with-permissions-round-2 branch from 618edfc to a6a3474 Compare September 6, 2024 16:20

tvdeyen reviewed Sep 6, 2024

View reviewed changes

admin/app/components/solidus_admin/ui/checkbox_row/component.html.erb Outdated Show resolved Hide resolved

admin/app/helpers/solidus_admin/permission_sets_helper.rb Show resolved Hide resolved

core/spec/models/spree/permission_set_spec.rb Outdated Show resolved Hide resolved

MadelineCollier force-pushed the admin-role-creation-with-permissions-round-2 branch from a6a3474 to 1a82402 Compare September 9, 2024 10:55

MadelineCollier requested a review from tvdeyen September 9, 2024 10:55

MadelineCollier added 3 commits September 9, 2024 13:11

Add permission set management to roles admin

dec39cf

Building off of the previous commit as well as the migrations that were added to core to pull in some of the key features of solidus_user_roles we can now allow admins to assign permission sets when creating a role.

Add permission set management when editing roles

0567e80

The role edit modal now also has the functionality to allow admins to assign permission sets.

MadelineCollier force-pushed the admin-role-creation-with-permissions-round-2 branch from 1a82402 to 3239325 Compare September 9, 2024 11:11

tvdeyen approved these changes Sep 9, 2024

View reviewed changes

kennyadsl approved these changes Sep 9, 2024

View reviewed changes

MadelineCollier merged commit 3151b14 into solidusio:main Sep 9, 2024
14 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Admin] Allow assignment of permission sets when creating/editing admin roles #5846

[Admin] Allow assignment of permission sets when creating/editing admin roles #5846

MadelineCollier commented Sep 2, 2024 •

edited

Loading

codecov bot commented Sep 3, 2024 •

edited

Loading

benjaminwil left a comment

tvdeyen left a comment

tvdeyen left a comment

tvdeyen left a comment

MadelineCollier commented Sep 9, 2024

tvdeyen left a comment

kennyadsl left a comment

MadelineCollier commented Sep 9, 2024

[Admin] Allow assignment of permission sets when creating/editing admin roles #5846

[Admin] Allow assignment of permission sets when creating/editing admin roles #5846

Conversation

MadelineCollier commented Sep 2, 2024 • edited Loading

Note for Reviewers

Screenshots:

Summary

Checklist

codecov bot commented Sep 3, 2024 • edited Loading

Codecov Report

benjaminwil left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

tvdeyen left a comment

Choose a reason for hiding this comment

MadelineCollier commented Sep 9, 2024

tvdeyen left a comment

Choose a reason for hiding this comment

kennyadsl left a comment

Choose a reason for hiding this comment

MadelineCollier commented Sep 9, 2024

MadelineCollier commented Sep 2, 2024 •

edited

Loading

codecov bot commented Sep 3, 2024 •

edited

Loading