for kiah

app/controllers/admin/users_controller.rb

@@ -26,6 +26,20 @@ def activate
       end
     end
 
+    def remove_role
+      user = User.find(params[:user_id])
+      role = Role.find(params[:role_id])
+
+      if user&.roles&.include?(role)
+        user.remove_role(role.name)
+        flash[:notice] = "Role '#{role.name}' was successfully removed from user #{user.email}."
+      else
+        flash[:alert] = "Failed to remove role '#{role.name}' from user #{user.email}."
+      end
+
+      redirect_back(fallback_location: root_path)
+    end
+
     private
 
       def load_user

app/controllers/roles_controller.rb

@@ -26,20 +26,6 @@ def update
     end
   end
 
-  def remove_role
-    user = User.find(params[:user_id])
-    role_name = params[:role_name]
-
-    if user&.roles&.exists?(name: role_name)
-      user.remove_role(role_name)
-      flash[:notice] = "Role '#{role_name}' was successfully removed from user #{user.email}."
-    else
-      flash[:alert] = "Failed to remove role '#{role_name}' from user #{user.email}."
-    end
-
-    redirect_back(fallback_location: root_path)
-  end
-
   protected
 
     def user_params

app/models/ability.rb

@@ -25,18 +25,6 @@ class Ability
     self.ability_logic += %i[everyone_can_create_curation_concerns]
   end
 
-  ##
-  # OVERRIDE:
-  #   Calls Hydra::Ability's initialize method to set up special user_manager permissions
-  #   so user managers can remove roles from users
-  def initialize(user)
-    super(user)
-    return unless user.has_role? "user_manager", Site.instance
-
-    can :manage, [User, Role]
-    cannot :destroy, [User, Role]
-  end
-
   # OVERRIDE METHOD from blacklight-access_controls v0.6.2
   #
   # NOTE: DO NOT RENAME THIS METHOD - it is required for permissions to function properly.

app/models/concerns/hyrax/ability/user_ability.rb

@@ -8,6 +8,7 @@ def user_roles
         if user_manager?
           can %i[create read update edit remove], User
           can %i[create read update edit remove destroy], Hyrax::Group
+          can :manage, [User, Role]
         # Can read all Users and Groups
         elsif user_reader?
           can %i[read], User

app/presenters/hyrax/admin/users_presenter.rb

@@ -20,14 +20,14 @@ def user_roles(user)
 
       # @return [Array] an array of user group role names
       def user_group_roles(user)
-        user.group_roles.map(&:name)
+        user.group_roles
       end
 
       # @return [Array] an array of user added role names
       def user_site_roles(user)
         # if the user has a group role that is the same as the site role, we don't want to show the site role
         # because if it shows up as a site role and we can delete it, it will cause funky behavior
-        user.site_roles.map(&:name) - user_group_roles(user)
+        user.site_roles - user_group_roles(user)
       end
 
       def user_groups(user)

app/views/hyrax/admin/users/index.html.erb

@@ -74,7 +74,7 @@
 
               <td class="roles"><% roles = @presenter.user_group_roles(user) %>
                   <ul><% roles.each do |role| %>
-                    <li><%= role.titleize %></li>
+                    <li><%= role.name.titleize %></li>
                     <% end %>
                   </ul>
               </td>
@@ -83,10 +83,10 @@
                 <ul>
                   <% roles.each do |role| %>
                     <li>
-                      <%= role.titleize %>
-                      <%= link_to main_app.remove_role_site_roles_path(user_id: user.id, role_name: role), method: :delete, data: { confirm: t('hyrax.admin.users.roles.remove.confirmation', user: user.email, role: role.titleize) } do %>
-                        <% if current_ability.admin? || (can?(:edit, User) && role != 'admin') %>
-                          <span class="glyphicon glyphicon-remove"></span>  
+                      <%= role.name.titleize %>
+                      <%= link_to main_app.admin_user_remove_role_path(user_id: user.id, role_id: role.id), method: :delete, data: { confirm: t('hyrax.admin.users.roles.remove.confirmation', user: user.email, role: role.name.titleize) } do %>
+                        <% if current_ability.admin? || (current_ability.can?(:edit, User) && role.name != 'admin') %>
+                          <span class="glyphicon glyphicon-remove"></span>
                         <% end %>
                       <% end %>
                     </li>

config/routes.rb

@@ -56,9 +56,7 @@
   mount BrowseEverything::Engine => '/browse'
 
   resource :site, only: [:update] do
-    resources :roles, only: %i[index update] do
-      delete 'remove_role/:user_id/:role_name', on: :collection, to: 'roles#remove_role', as: :remove_role
-    end
+    resources :roles, only: %i[index update]
     resource :labels, only: %i[edit update]
   end
 
@@ -128,6 +126,7 @@
     resource :work_types, only: %i[edit update]
     resources :users, only: [:index, :destroy] do
       post 'activate', on: :member
+      delete 'remove_role/:role_id', to: 'users#remove_role', as: 'remove_role'
     end
     resources :groups do
       member do

spec/models/ability_spec.rb

@@ -91,6 +91,38 @@
     it { is_expected.to be_able_to(:manage, :all) }
   end
 
+  describe 'a user_manager user' do
+    let(:user) { FactoryBot.create(:user) }
+    let(:ordinary_role) { FactoryBot.create(:role, name: 'ordinary_role') }
+
+    before do
+      user.add_role :user_manager, Site.instance
+    end
+
+    context 'when managing User and Role' do
+      it 'can create, read, update, and edit User and Role' do
+        expect(ability).to be_able_to(:create, User.new)
+        expect(ability).to be_able_to(:read, User.new)
+        expect(ability).to be_able_to(:update, User.new)
+        expect(ability).to be_able_to(:edit, User.new)
+
+        expect(ability).to be_able_to(:create, Role.new)
+        expect(ability).to be_able_to(:read, Role.new)
+        expect(ability).to be_able_to(:update, Role.new)
+        expect(ability).to be_able_to(:edit, Role.new)
+      end
+
+      it 'cannot destroy User and Role' do
+        expect(ability).not_to be_able_to(:destroy, User.new)
+        expect(ability).not_to be_able_to(:destroy, Role.new)
+      end
+
+      it 'can manage an ordinary role' do
+        expect(ability).to be_able_to(:manage, ordinary_role)
+      end
+    end
+  end
+
   # Brought over from blacklight-access_controls v0.6.2
   describe '#user_groups' do
     subject { ability.user_groups }