[Admin] Allow assignment of permission sets when creating/editing admin roles#5846
Merged
MadelineCollier merged 6 commits intosolidusio:mainfrom Sep 9, 2024
Conversation
MadelineCollier
force-pushed
the
admin-role-creation-with-permissions-round-2
branch
2 times, most recently
from
2b1baa5 to
751784f
Compare
github-actions
Bot
added
the
changelog:solidus_core
MadelineCollier
force-pushed
the
admin-role-creation-with-permissions-round-2
branch
2 times, most recently
from
dcb32c3 to
578d332
Compare
MadelineCollier
changed the title
Codecov ReportAttention: Patch coverage is
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. |
MadelineCollier
changed the title
benjaminwil
reviewed
Sep 3, 2024
Contributor
benjaminwil
left a comment
benjaminwil
left a comment
There was a problem hiding this comment.
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.
tvdeyen
reviewed
Sep 4, 2024
Member
tvdeyen
left a comment
tvdeyen
left a comment
There was a problem hiding this comment.
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 newPermissionSetmodel.
Again, thanks for all the work you put in the new admin.
MadelineCollier
force-pushed
the
admin-role-creation-with-permissions-round-2
branch
5 times, most recently
from
7a73e2c to
915e9f6
Compare
MadelineCollier
commented
Sep 5, 2024
MadelineCollier
force-pushed
the
admin-role-creation-with-permissions-round-2
branch
from
915e9f6 to
82f691d
Compare
MadelineCollier
requested review from
a team
and removed request for
benjaminwil
tvdeyen
reviewed
Sep 6, 2024
Member
tvdeyen
left a comment
tvdeyen
left a comment
There was a problem hiding this comment.
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.
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 <benjamin@super.gd>
MadelineCollier
force-pushed
the
admin-role-creation-with-permissions-round-2
branch
2 times, most recently
from
e02310a to
3ea93ea
Compare
MadelineCollier
force-pushed
the
admin-role-creation-with-permissions-round-2
branch
from
3ea93ea to
618edfc
Compare
tvdeyen
reviewed
Sep 6, 2024
Member
tvdeyen
left a comment
tvdeyen
left a comment
There was a problem hiding this comment.
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.
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.
MadelineCollier
force-pushed
the
admin-role-creation-with-permissions-round-2
branch
from
618edfc to
a6a3474
Compare
tvdeyen
reviewed
Sep 6, 2024
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
force-pushed
the
admin-role-creation-with-permissions-round-2
branch
from
a6a3474 to
1a82402
Compare
Contributor
Author
|
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!
MadelineCollier
force-pushed
the
admin-role-creation-with-permissions-round-2
branch
from
1a82402 to
3239325
Compare
kennyadsl
approved these changes
Sep 9, 2024
Member
kennyadsl
left a comment
kennyadsl
left a comment
There was a problem hiding this comment.
Super nice work! For the sake of documentation, if you can update the screen recording with the last version, that would be super nice!
Contributor
Author
|
Updated the screenshots and recordings, thanks for all the time put into many rounds of reviews everyone :) Much appreciated! |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
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
CheckboxRowas 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:
The following are not always needed: