-
-
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] Edit/Update roles via new admin UI #5828
[Admin] Edit/Update roles via new admin UI #5828
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #5828 +/- ##
==========================================
+ Coverage 89.12% 89.14% +0.01%
==========================================
Files 743 744 +1
Lines 17317 17343 +26
==========================================
+ Hits 15434 15460 +26
Misses 1883 1883 ☔ View full report in Codecov by Sentry. |
This should remain in draft until #5826 has been merged and the branch can be rebased to remove the extra (related) commits. |
7936e8f
to
93e3462
Compare
93e3462
to
ab155dc
Compare
ab155dc
to
76ae9f6
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.
Left a nit comment, but LGTM 👍
I'm also wondering if we should just enforce the presence of a name for roles 🤔
@@ -63,6 +98,10 @@ def destroy | |||
|
|||
private | |||
|
|||
def find_role |
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.
nit: thoughts on sticking to the convention used in rails scaffold code and use set_role
here?
from rails g scaffold
private
# Use callbacks to share common setup or constraints between actions.
def set_post
@post = Post.find(params[:id])
end
# Only allow a list of trusted parameters through.
def post_params
params.require(:post).permit(:title, :body)
end
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.
Oh for sure, I'd be happy to switch to that! And yes, I feel strongly that we should validate the presence of names on Roles given they are the only defining characteristic at the moment. It would be great if you could take a look at my draft PR: #5833 and let me know of anything else you think I should consider as part of my modifications. Part of that PR is working to pull in some of the most essential pieces from the solidus_user_roles
gem
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.
Actually on second thought, @elia I might leave this as find_role
for now, as you can see that this pattern of find_<resource_name>
persists across many of our admin controllers. I think if we do change it I should change it for all of them.
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.
Great call, yes, let's do it in one pass for everything 👍
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.
Sounds good, I can put that bulk rename change up today.
Summary
This PR is part 2 of issue #5823 That first PR adds the basic Role UI (creation/deletion/index), while this PR adds the modification logic for editing/updating.
The attached video shows the functionality visually:
(Note that currently
Spree::Role
does not requirename
to be present, but when it is present, it must be unique)Screen.Recording.2024-08-15.at.12.46.38.PM.mov
Checklist
Check out our PR guidelines for more details.
The following are mandatory for all PRs:
The following are not always needed: