-
-
Notifications
You must be signed in to change notification settings - Fork 157
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
[Draft] Allow super admin to reset passwords for community admin users #904
Conversation
def edit | ||
@user = User.find(params[:id]) | ||
|
||
if @user.role == 'admin' |
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.
Since the roles are handled by an enum, we can use the built-in helper methods to check the role:
if @user.role == 'admin' | |
if @user.admin? |
@user | ||
else | ||
flash[:alert] = 'You are not authorized to perform this action.' | ||
redirect_to(root_path) |
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.
I'm wondering if it would be better for us to redirect to the community path rather than to the root path which shows the metrics for all of the communities for the super admin. Since it seems the current path is: Metrics root page (after login) -> Communities -> Community -> Find the user in the tiny list -> edit. On save, going all the way back feels jarring.
Alternatively, it may also be worth creating a new list view under the Super Admin dashboard that lists all of the admin users in the system that we can use to redirect to and make it easier for super admins to reset admin passwords, regardless of the community.
@rudokemper thoughts?
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.
Sorry I missed this back in May. I think the first solution makes the most sense.
def update | ||
@user = User.find(params[:id]) | ||
if @user.role != 'admin' | ||
redirect_to(root_path) |
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.
Let's add a flash alert here to denote why it failed/they are being redirected.
</div> | ||
<% end %> | ||
|
||
<%= form_with url: member_path(@user), model: @user, multipart: true, class: "form aligned", data: {remote: false}, id: "userForm", locale: true do |f| %> |
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.
We can remove multipart
from this form since there are no file uploads.
describe "GET edit" do | ||
context "with a super admin privileges" do | ||
let(:super_admin) { FactoryBot.create(:user, role: 100, super_admin: true) } | ||
let(:user) { FactoryBot.create(:user, email: "[email protected]", username: "user_name", role: 2) } |
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.
In case you were unaware, you can reference the roles by their symbols. So, rather than having to integer map role: 2
, you can use role: :admin
.
login_as super_admin | ||
end | ||
|
||
it "should return success if the user is an admin" do |
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.
Let's also add a spec for when the logged-in user is a super admin, but the user itself is not an admin. The controller accounts for this with a redirect, but that is not encapsulated in a spec here.
|
||
expect(response).to have_http_status(302) | ||
expect(flash[:notice]).to match('User successfully updated') | ||
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.
Let's also add a failure spec here for when the user being updated is not an admin here.
Just saw this again! Wondering what we can do to finish this PR? @DuldR are you still able to work on it, or do you want to leave it here for someone else to finish the remaining tasks as indicated by @lauramosher? |
@rudokemper Yes sir, sorry, I can't finish it up. If anyone else wanted to take a whack at it, that'd be great. |
Fixes #802
Related PR: #905
I wasn't sure on how powerful we want a super admin to be, so I've essentially allowed them the max permissions for this PR. (More discussion: https://rubyforgood.slack.com/archives/CAH9XQX2A/p1675975728042129?thread_ts=1675971543.153629&cid=CAH9XQX2A)
Updated this PR to only allow a
super_admin
to update a communities admin password.Community User Page
Super Admin Reset Password Page
Admin Reset Password Page