Merge pull request #567 from projecthydra-labs/fix_564

Adding a second manager should not wipe out the access of the first
samvera · Mar 16, 2017 · 07b7c24 · 07b7c24
2 parents 413de6a + 4457721
commit 07b7c24
Show file tree

Hide file tree

Showing 2 changed files with 28 additions and 10 deletions.
diff --git a/app/forms/hyrax/forms/permission_template_form.rb b/app/forms/hyrax/forms/permission_template_form.rb
@@ -38,7 +38,9 @@ def initialize(model)
         select_release_varies_option(model)
       end
 
-      # @return [Hash{Symbol => String, Boolean}] { :content_tab (for confirmation message), :updated (true/false), :error_code (for flash error lookup) }
+      # @return [Hash{Symbol => String, Boolean}] { :content_tab (for confirmation message),
+      #                                             :updated (true/false),
+      #                                               :error_code (for flash error lookup) }
       def update(attributes)
         return_info = { content_tab: tab_to_update(attributes) }
         error_code = nil
@@ -66,8 +68,8 @@ def tab_to_update(attributes)
 
         # @return [Void]
         def update_participants_options(attributes)
-          update_admin_set(attributes)
           update_permission_template(attributes)
+          update_admin_set(attributes)
         end
 
         # @return [String, Nil] error_code if validation fails, nil otherwise
@@ -124,13 +126,19 @@ def update_permission_template(attributes)
           nil
         end
 
-        # Maps the raw form attributes into a hash useful for updating the admin set.
+        # 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)
-          manage_grants = grants_as_collection(attributes).select { |x| x[:access] == 'manage' }
-          return unless manage_grants.present?
-          { edit_users: manage_grants.select { |x| x[:agent_type] == 'user' }.map { |x| x[:agent_id] },
-            edit_groups: manage_grants.select { |x| x[:agent_type] == 'group' }.map { |x| x[:agent_id] } }
+          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
 
         # This allows the attributes

diff --git a/spec/forms/hyrax/forms/permission_template_form_spec.rb b/spec/forms/hyrax/forms/permission_template_form_spec.rb
@@ -17,12 +17,22 @@
   end
 
   describe "#update" do
+    subject { form.update(input_params) }
+
     let(:grant_attributes) { [] }
     let(:input_params) do
       ActionController::Parameters.new(access_grants_attributes: grant_attributes).permit!
     end
     let(:permission_template) { create(:permission_template, admin_set_id: admin_set.id) }
-    subject { form.update(input_params) }
+
+    before do
+      permission_template.access_grants.create([{ agent_type: 'user',
+                                                  agent_id: 'karen',
+                                                  access: 'manage' },
+                                                { agent_type: 'group',
+                                                  agent_id: 'archivists',
+                                                  access: 'manage' }])
+    end
 
     context "with a user manager" do
       let(:grant_attributes) do
@@ -32,7 +42,7 @@
       end
       it "also adds edit_access to the AdminSet itself" do
         expect { subject }.to change { permission_template.access_grants.count }.by(1)
-        expect(admin_set.reload.edit_users).to include 'bob'
+        expect(admin_set.reload.edit_users).to match_array ['bob', 'karen']
       end
     end
 
@@ -44,7 +54,7 @@
       end
       it "also adds edit_access to the AdminSet itself" do
         expect { subject }.to change { permission_template.access_grants.count }.by(1)
-        expect(admin_set.reload.edit_groups).to include 'bob'
+        expect(admin_set.reload.edit_groups).to match_array ['bob', 'archivists']
       end
     end