-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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] Add new migrations and validations in core
to support new admin
Spree::Role
interface
#5833
[Admin] Add new migrations and validations in core
to support new admin
Spree::Role
interface
#5833
Conversation
560e327
to
45e4837
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5833 +/- ##
=======================================
Coverage 89.14% 89.14%
=======================================
Files 744 744
Lines 17343 17343
=======================================
Hits 15460 15460
Misses 1883 1883 ☔ View full report in Codecov by Sentry. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added just a nit about using def change
vs. def up / def down
, but otherwise looks great! 🙌
core/db/migrate/20240821173254_create_spree_permission_sets_in_core.rb
Outdated
Show resolved
Hide resolved
45e4837
to
98addef
Compare
core
to support new admin
Spree::Role
interface
core
to support new admin
Spree::Role
interface core
to support new admin
Spree::Role
interface
It doesn't make a lot of sense to allow blank on this considering it is the only defining feature of a Role currently. Co-authored-by: benjamin wil <[email protected]>
98addef
to
98dc7f6
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice. Could we remove the unless check? It should not be necessary for Rails 7.0 and above.
core/db/migrate/20240821173254_create_spree_permission_sets_in_core.rb
Outdated
Show resolved
Hide resolved
These migrations originally come from `solidus_user_roles`. We intend to migrate some of the functionality from that extension into `core`, and this is the first step. With the table check, these migrations should be safe to (re)run for any store which might already have `solidus_user_roles` installed.
98dc7f6
to
1aa18ee
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looking forward to this! Thanks for working on it 🍨
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.
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. Later commits also 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. Co-authored-by: benjamin wil <[email protected]>
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. Later commits also 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. Co-authored-by: benjamin wil <[email protected]>
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]>
Summary
This PR is part of issue: #5823
These new migrations and validations are being added to support the example designs for the new roles admin interface:
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: