Update the AdminSet permissions when removing a PermissionTemplateAccess

Fixes #570

app/controllers/hyrax/admin/permission_template_accesses_controller.rb

@@ -5,11 +5,23 @@ class PermissionTemplateAccessesController < ApplicationController
 
       def destroy
         @permission_template_access.destroy
+        update_admin_set if @permission_template_access.manage?
 
-        redirect_to hyrax.edit_admin_admin_set_path(@permission_template_access.permission_template.admin_set_id,
+        redirect_to hyrax.edit_admin_admin_set_path(admin_set_id,
                                                     anchor: 'participants'),
                     notice: translate('participants', scope: 'hyrax.admin.admin_sets.form.permission_update_notices')
       end
+
+      private
+
+        # @return [String] the identifier for the AdminSet for the currently loaded resource
+        def admin_set_id
+          @admin_set_id ||= @permission_template_access.permission_template.admin_set_id
+        end
+
+        def update_admin_set
+          AdminSet.find(admin_set_id).update_access_controls!
+        end
     end
   end
 end

app/forms/hyrax/forms/permission_template_form.rb

@@ -69,7 +69,8 @@ def tab_to_update(attributes)
         # @return [Void]
         def update_participants_options(attributes)
           update_permission_template(attributes)
-          update_admin_set(attributes)
+          # if managers were added, recalculate update the access controls on the AdminSet
+          admin_set.update_access_controls! if managers_updated?(attributes)
         end
 
         # @return [String, Nil] error_code if validation fails, nil otherwise
@@ -109,29 +110,12 @@ def grant_workflow_roles(attributes)
           end
         end
 
-        def update_admin_set(attributes)
-          update_params = admin_set_update_params(attributes)
-          return unless update_params
-          admin_set.update!(update_params)
-        end
-
         # @return [Nil]
         def update_permission_template(attributes)
           model.update(permission_template_update_params(attributes))
           nil
         end
 
-        # The attributes[:access_grants_attributes], only submits changes, not
-        # all of the managers, so we need to query the persisted access_grants on
-        # the permission_template to see who should be an edit user.
-        # This can only be used after the permission template has been updated
-        # @return [Hash] includes :edit_users and :edit_groups
-        def admin_set_update_params(attributes)
-          return unless managers_updated?(attributes)
-          { edit_users: model.access_grants.where(access: 'manage', agent_type: 'user').pluck(:agent_id),
-            edit_groups: model.access_grants.where(access: 'manage', agent_type: 'group').pluck(:agent_id) }
-        end
-
         def managers_updated?(attributes)
           grants_as_collection(attributes).any? { |x| x[:access] == 'manage' }
         end

app/models/concerns/hyrax/admin_set_behavior.rb

@@ -83,6 +83,13 @@ def active_workflow
       Sipity::Workflow.find_active_workflow_for(admin_set_id: id)
     end
 
+    # Calculate and update who should have edit access based on who
+    # has "manage" access in the PermissionTemplateAccess
+    def update_access_controls!
+      update!(edit_users: permission_template.access_grants.where(access: 'manage', agent_type: 'user').pluck(:agent_id),
+              edit_groups: permission_template.access_grants.where(access: 'manage', agent_type: 'group').pluck(:agent_id))
+    end
+
     private
 
       def destroy_permission_template

spec/actors/hyrax/apply_permission_template_actor_spec.rb

@@ -29,25 +29,25 @@
       let(:attributes) { { admin_set_id: admin_set.id } }
       before do
         create(:permission_template_access,
+               :manage,
                permission_template: permission_template,
                agent_type: 'user',
-               agent_id: 'hannah',
-               access: 'manage')
+               agent_id: 'hannah')
         create(:permission_template_access,
+               :manage,
                permission_template: permission_template,
                agent_type: 'group',
-               agent_id: 'librarians',
-               access: 'manage')
+               agent_id: 'librarians')
         create(:permission_template_access,
+               :view,
                permission_template: permission_template,
                agent_type: 'user',
-               agent_id: 'gary',
-               access: 'view')
+               agent_id: 'gary')
         create(:permission_template_access,
+               :view,
                permission_template: permission_template,
                agent_type: 'group',
-               agent_id: 'readers',
-               access: 'view')
+               agent_id: 'readers')
         allow(Hyrax::Actors::RootActor).to receive(:new).and_return(create_actor)
         allow(create_actor).to receive(:create).and_return(true)
       end

spec/controllers/hyrax/admin/permission_template_accesses_controller_spec.rb

@@ -9,8 +9,8 @@
   let(:permission_template_access) { create(:permission_template_access) }
   let(:admin_set_id) { permission_template_access.permission_template.admin_set_id }
 
-  context "without admin privleges" do
-    describe "destroy" do
+  describe "destroy" do
+    context "without admin privleges" do
       before do
         allow(controller.current_ability).to receive(:test_edit).with(admin_set_id).and_return(false)
       end
@@ -19,17 +19,26 @@
         expect(response).to be_unauthorized
       end
     end
-  end
 
-  context "when signed in as an admin" do
-    describe "update" do
+    context "when signed in as an admin" do
+      let(:permission_template_access) do
+        create(:permission_template_access,
+               :manage,
+               permission_template: permission_template,
+               agent_type: 'user',
+               agent_id: 'Liz')
+      end
+      let(:permission_template) { create(:permission_template, admin_set_id: admin_set.id) }
+      let(:admin_set) { create(:admin_set, edit_users: ['Liz']) }
+
       it "is successful" do
         expect(controller).to receive(:authorize!).with(:destroy, permission_template_access)
         expect do
           delete :destroy, params: { id: permission_template_access }
         end.to change { Hyrax::PermissionTemplateAccess.count }.by(-1)
         expect(response).to redirect_to(hyrax.edit_admin_admin_set_path(admin_set_id, locale: 'en', anchor: 'participants'))
         expect(flash[:notice]).to eq(I18n.t('participants', scope: 'hyrax.admin.admin_sets.form.permission_update_notices'))
+        expect(admin_set.reload.edit_users).to be_empty
       end
     end
   end

spec/factories/permission_template_accesses.rb

@@ -1,5 +1,13 @@
 FactoryGirl.define do
   factory :permission_template_access, class: Hyrax::PermissionTemplateAccess do
     permission_template
+
+    trait :manage do
+      access 'manage'
+    end
+
+    trait :view do
+      access 'view'
+    end
   end
 end

spec/forms/hyrax/forms/permission_template_form_spec.rb

@@ -26,12 +26,16 @@
     let(:permission_template) { create(:permission_template, admin_set_id: admin_set.id) }
 
     before do
-      permission_template.access_grants.create([{ agent_type: 'user',
-                                                  agent_id: 'karen',
-                                                  access: 'manage' },
-                                                { agent_type: 'group',
-                                                  agent_id: 'archivists',
-                                                  access: 'manage' }])
+      create(:permission_template_access,
+             :manage,
+             permission_template: permission_template,
+             agent_type: 'user',
+             agent_id: 'karen')
+      create(:permission_template_access,
+             :manage,
+             permission_template: permission_template,
+             agent_type: 'group',
+             agent_id: 'archivists')
     end
 
     context "with a user manager" do

spec/search_builder/hyrax/admin_set_search_builder_spec.rb

@@ -1,6 +1,6 @@
 require 'spec_helper'
 
-describe Hyrax::AdminSetSearchBuilder do
+RSpec.describe Hyrax::AdminSetSearchBuilder do
   let(:context) do
     double(blacklight_config: CatalogController.blacklight_config,
            current_ability: ability)
@@ -91,20 +91,21 @@
 
       before do
         create(:permission_template_access,
+               :manage,
                permission_template: permission_template1,
                agent_type: 'user',
                agent_id: user.user_key,
                access: 'deposit')
         create(:permission_template_access,
+               :manage,
                permission_template: permission_template2,
                agent_type: 'user',
-               agent_id: user.user_key,
-               access: 'manage')
+               agent_id: user.user_key)
         create(:permission_template_access,
+               :view,
                permission_template: permission_template3,
                agent_type: 'user',
-               agent_id: user.user_key,
-               access: 'view')
+               agent_id: user.user_key)
       end
 
       it 'is successful' do