Implemented the "Disable users" feature #240
Conversation
| current_user.admin? && User.admins.count == 1 | ||
| else | ||
| # Only admin users can disable other users. | ||
| !current_user.admin? |
There was a problem hiding this comment.
This part of the branch is useless because we have the check_admin filter in place. So a non admin user is never going to reach this part of the code
There was a problem hiding this comment.
Note that a regular user should be able to disable himself, therefore non-admin users should be able to perform this action. I'd say to just remove :disable from the check_admin hook (which by the way is not preventing anything, since the test in line 145 of spec/controllers/auth/registrations_controller_spec.rb passes ;) ).
There was a problem hiding this comment.
Good point, ignore all my comments about this part of the code 👍
|
The code looks good, I have just one doubt about dropping all the team memberships |
| @@ -42,6 +42,11 @@ def self.find_from_event(event) | |||
| actor | |||
| end | |||
|
|
|||
| # Returns all the enabled admins in the system. | |||
There was a problem hiding this comment.
Use ActiveRecord Scopes ?
I've also taken the opportunity to add the `enabled` scope, which can be convenient.
| @@ -2,7 +2,8 @@ class Admin::UsersController < Admin::BaseController | |||
| respond_to :html, :js | |||
|
|
|||
| def index | |||
| @users = User.all.page(params[:page]) | |||
| @users = User.where(enabled: true).page(params[:page]) | |||
There was a problem hiding this comment.
User has the enabled scope now, we should use it
|
Everything looks right. I have a couple of questions though before we can merge it:
|
|
So, right now the feature was meant to disable the selected users, there was some discussion about enabling them back, but I think that it was dismissed (I don't recall now :/). Maybe we can add this for admins later on. If so, then I say that we can do that in another PR (just to not complicate this PR further). I can submit the issue, and assign myself to it. |
Fine, what about the other question? |
|
Oh yeah. Admins now are not able to see disabled users. Maybe that can be added to the proposed new issue, since listing disabled users would be annoying if anyways you cannot enable them back, no ? |
|
Clicked the wrong button :_) |
Sounds good to me, please create the new issue |
Implemented the "Disable users" feature
Fixes #196