Refactor process for resetting collection ACLs to PermissionTemplate settings

app/forms/hyrax/forms/permission_template_form.rb

@@ -12,7 +12,13 @@ class PermissionTemplateForm
       # @return [#to_s] the primary key of the associated admin_set or collection
       # def source_id (because you might come looking for this method)
       delegate :id, to: :source_model, prefix: :source
-      delegate :reset_access_controls!, to: :source_model
+
+      ##
+      # @deprecated  use PermissionTemplate#reset_access_controls instead.
+      def reset_access_controls!
+        Deprecation.warn("reset_access_controls! is deprecated; use PermissionTemplate#reset_access_controls instead.")
+        source_model.reset_access_controls!
+      end
 
       # Stores which radio button under release "Varies" option is selected
       attr_accessor :release_varies
@@ -67,7 +73,7 @@ def update(attributes)
       # Copy this access to the permissions of the Admin Set or Collection and to
       # the WorkflowResponsibilities of the active workflow if this is an Admin Set
       def update_access(remove_agent: false)
-        reset_access_controls!
+        source_model.reset_access_controls!
         update_workflow_responsibilities(remove_agent: remove_agent) if source_model.is_a?(AdminSet)
       end
 

app/models/admin_set.rb

@@ -75,37 +75,18 @@ def active_workflow
     Sipity::Workflow.find_active_workflow_for(admin_set_id: id)
   end
 
+  ##
+  # @deprecated use PermissionTemplate#reset_access_controls instead
+  #
   # Calculate and update who should have edit access based on who
   # has "manage" access in the PermissionTemplateAccess
   def reset_access_controls!
-    # NOTE: This is different from Collections in that it doesn't update read access based on visibility.
-    #       See also app/models/concerns/hyrax/collection_behavior.rb#reset_access_controls!
-    update!(edit_users: permission_template_edit_users,
-            edit_groups: permission_template_edit_groups,
-            read_users: permission_template_read_users,
-            read_groups: permission_template_read_groups)
-  end
-
-  private
-
-  def permission_template_edit_users
-    permission_template.agent_ids_for(access: 'manage', agent_type: 'user')
-  end
-
-  def permission_template_edit_groups
-    permission_template.agent_ids_for(access: 'manage', agent_type: 'group')
-  end
+    Deprecation.warn("reset_access_controls! is deprecated; use PermissionTemplate#reset_access_controls instead.")
 
-  def permission_template_read_users
-    (permission_template.agent_ids_for(access: 'view', agent_type: 'user') +
-        permission_template.agent_ids_for(access: 'deposit', agent_type: 'user')).uniq
+    permission_template.reset_access_controls_for(collection: self)
   end
 
-  def permission_template_read_groups
-    (permission_template.agent_ids_for(access: 'view', agent_type: 'group') +
-      permission_template.agent_ids_for(access: 'deposit', agent_type: 'group')).uniq -
-      [::Ability.registered_group_name, ::Ability.public_group_name]
-  end
+  private
 
   def destroy_permission_template
     permission_template.destroy

app/models/concerns/hyrax/collection_behavior.rb

@@ -125,41 +125,19 @@ def permission_template
       Hyrax::PermissionTemplate.find_by!(source_id: id)
     end
 
+    ##
+    # @deprecated use PermissionTemplate#reset_access_controls instead
+    #
     # Calculate and update who should have read/edit access to the collections based on who
     # has access in PermissionTemplateAccess
     def reset_access_controls!
-      update!(edit_users: permission_template_edit_users,
-              edit_groups: permission_template_edit_groups,
-              read_users: permission_template_read_users,
-              read_groups: (permission_template_read_groups + visibility_group).uniq)
-    end
-
-    private
+      Deprecation.warn("reset_access_controls! is deprecated; use PermissionTemplate#reset_access_controls instead.")
 
-    def permission_template_edit_users
-      permission_template.agent_ids_for(access: 'manage', agent_type: 'user')
+      permission_template
+        .reset_access_controls_for(collection: self, interpret_visibility: true)
     end
 
-    def permission_template_edit_groups
-      permission_template.agent_ids_for(access: 'manage', agent_type: 'group')
-    end
-
-    def permission_template_read_users
-      (permission_template.agent_ids_for(access: 'view', agent_type: 'user') +
-        permission_template.agent_ids_for(access: 'deposit', agent_type: 'user')).uniq
-    end
-
-    def permission_template_read_groups
-      (permission_template.agent_ids_for(access: 'view', agent_type: 'group') +
-        permission_template.agent_ids_for(access: 'deposit', agent_type: 'group')).uniq -
-        [::Ability.registered_group_name, ::Ability.public_group_name]
-    end
-
-    def visibility_group
-      return [Hydra::AccessControls::AccessRight::PERMISSION_TEXT_VALUE_PUBLIC] if visibility == Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PUBLIC
-      return [Hydra::AccessControls::AccessRight::PERMISSION_TEXT_VALUE_AUTHENTICATED] if visibility == Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_AUTHENTICATED
-      []
-    end
+    private
 
     # Solr field name works use to index member ids
     def member_ids_field

app/models/hyrax/permission_template.rb

@@ -172,6 +172,57 @@ def valid_visibility?(value)
       visibility == value
     end
 
+    ##
+    # @return [Array<String>]
+    def edit_users
+      agent_ids_for(access: 'manage', agent_type: 'user')
+    end
+
+    ##
+    # @return [Array<String>]
+    def edit_groups
+      agent_ids_for(access: 'manage', agent_type: 'group')
+    end
+
+    ##
+    # @return [Array<String>]
+    def read_users
+      (agent_ids_for(access: 'view', agent_type: 'user') +
+        agent_ids_for(access: 'deposit', agent_type: 'user')).uniq
+    end
+
+    ##
+    # @return [Array<String>]
+    def read_groups
+      (agent_ids_for(access: 'view', agent_type: 'group') +
+        agent_ids_for(access: 'deposit', agent_type: 'group')).uniq -
+        [::Ability.registered_group_name, ::Ability.public_group_name]
+    end
+
+    ##
+    # @return [Boolean]
+    def reset_access_controls(interpret_visibility: false)
+      reset_access_controls_for(collection: source_model,
+                                interpret_visibility: interpret_visibility)
+    end
+
+    ##
+    # @return [Boolean]
+    def reset_access_controls_for(collection:, interpret_visibility: false)
+      interpreted_read_groups = read_groups
+
+      if interpret_visibility
+        visibilities = Hyrax::VisibilityMap.instance
+        interpreted_read_groups -= visibilities.deletions_for(visibility: collection.visibility)
+        interpreted_read_groups += visibilities.additions_for(visibility: collection.visibility)
+      end
+
+      collection.update!(edit_users: edit_users,
+                         edit_groups: edit_groups,
+                         read_users: read_users,
+                         read_groups: interpreted_read_groups.uniq)
+    end
+
     private
 
     # If template requires no delays, check if date is exactly today

app/services/hyrax/admin_set_create_service.rb

@@ -81,9 +81,11 @@ def admin_group_name
       ::Ability.admin_group_name
     end
 
+    ##
+    # @return [PermissionTemplate]
     def create_permission_template
       permission_template = PermissionTemplate.create!(source_id: admin_set.id, access_grants_attributes: access_grants_attributes)
-      admin_set.reset_access_controls!
+      permission_template.reset_access_controls_for(collection: admin_set)
       permission_template
     end
 

app/services/hyrax/collections/migration_service.rb

@@ -40,6 +40,7 @@ def self.migrate_collection(collection)
       end
       private_class_method :migrate_collection
 
+      ##
       # @api private
       #
       # Migrate a single adminset to grant depositors and viewers read access to the admin set unless the grant is for
@@ -50,7 +51,8 @@ def self.migrate_collection(collection)
       def self.migrate_adminset(adminset)
         Hyrax::PermissionTemplateAccess.find_or_create_by(permission_template_id: adminset.permission_template.id,
                                                           agent_type: "group", agent_id: "admin", access: "manage")
-        adminset.reset_access_controls!
+
+        adminset.permission_template.reset_access_controls_for(collection: adminset)
       end
       private_class_method :migrate_adminset
 
@@ -84,7 +86,7 @@ def self.repair_migrated_collection(collection)
         collection.collection_type_gid = Hyrax::CollectionType.find_or_create_default_collection_type.to_global_id
         permission_template = Hyrax::PermissionTemplate.find_by(source_id: collection.id)
         if permission_template.present?
-          collection.reset_access_controls!
+          permission_template.reset_access_controls_for(collection: collection, interpret_visibility: true)
         else
           create_permissions(collection)
         end

app/services/hyrax/collections/permissions_create_service.rb

@@ -13,9 +13,10 @@ class PermissionsCreateService
       def self.create_default(collection:, creating_user:, grants: [])
         collection_type = Hyrax::CollectionType.find_by_gid!(collection.collection_type_gid)
         access_grants = access_grants_attributes(collection_type: collection_type, creating_user: creating_user, grants: grants)
-        PermissionTemplate.create!(source_id: collection.id,
-                                   access_grants_attributes: access_grants.uniq)
-        collection.reset_access_controls!
+        template = PermissionTemplate.create!(source_id: collection.id,
+                                              access_grants_attributes: access_grants.uniq)
+
+        template.reset_access_controls_for(collection: collection, interpret_visibility: true)
       end
 
       # @api public
@@ -38,7 +39,8 @@ def self.add_access(collection_id:, grants:)
                                                             agent_id: grant[:agent_id],
                                                             access: grant[:access])
         end
-        collection.reset_access_controls!
+
+        template.reset_access_controls_for(collection: collection, interpret_visibility: true)
       end
 
       # @api private

spec/services/hyrax/collections/permissions_create_service_spec.rb

@@ -42,7 +42,7 @@
 
   describe ".add_access" do
     subject { described_class.add_access(collection_id: collection.id, grants: grants) }
-    let(:collection) { build(:collection_lw, id: 'test_collection', with_permission_template: true) }
+    let(:collection) { FactoryBot.create(:collection_lw, with_permission_template: true) }
     let(:grants) do
       [{ agent_type: Hyrax::PermissionTemplateAccess::GROUP,
          agent_id: 'archivist',
@@ -53,7 +53,6 @@
 
     before do
       allow(ActiveFedora::Base).to receive(:find).with(collection.id).and_return(collection)
-      allow(collection).to receive(:reset_access_controls!).and_return true
       subject
       depositor_grants.each { |agent| array << agent.agent_id }
     end