WIP: What Would Sandi Metz do?

app/controllers/concerns/curation_concerns/curation_concern_controller.rb

@@ -135,7 +135,7 @@ def after_create_response
 
     def after_update_response
       # TODO: visibility or lease/embargo status
-      if actor.visibility_changed? && curation_concern.file_sets.present?
+      if curation_concern.visibility_changed? && curation_concern.file_sets.present?
         redirect_to main_app.confirm_curation_concerns_permission_path(curation_concern)
       else
         respond_to do |wants|

app/services/curation_concerns/curation_concern.rb

@@ -1,9 +1,19 @@
 module CurationConcerns
   module CurationConcern
-    def self.actor(curation_concern, *args)
+    # Returns the top-level actor on the stack
+    def self.actor(curation_concern, current_user, attributes)
+      AddToCollectionActor.new(curation_concern, current_user, attributes,
+                               [AssignRepresentativeActor,
+                                AttachFilesActor,
+                                ApplyOrderActor,
+                                InterpretVisibilityActor,
+                                model_actor(curation_concern),
+                                AssignIdentifierActor])
+    end
+
+    def self.model_actor(curation_concern)
       actor_identifier = curation_concern.class.to_s.split('::').last
-      klass = "CurationConcerns::#{actor_identifier}Actor".constantize
-      klass.new(curation_concern, *args)
+      "CurationConcerns::#{actor_identifier}Actor".constantize
     end
   end
 end

curation_concerns-models/app/actors/concerns/curation_concerns/manages_embargoes_actor.rb

@@ -19,73 +19,10 @@ module CurationConcerns
   #
   module ManagesEmbargoesActor
     extend ActiveSupport::Concern
+    extend Deprecation
 
-    # Interprets embargo & lease visibility if necessary
-    # returns false if there are any errors
-    def interpret_visibility(attributes = self.attributes)
-      should_continue = interpret_embargo_visibility(attributes) && interpret_lease_visibility(attributes)
-      if attributes[:visibility]
-        curation_concern.visibility = attributes[:visibility]
-      end
-      should_continue
-    end
-
-    # If user has set visibility to embargo, interprets the relevant information and applies it
-    # Returns false if there are any errors and sets an error on the curation_concern
-    def interpret_embargo_visibility(attributes = self.attributes)
-      if attributes[:visibility] == Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_EMBARGO
-        if !attributes[:embargo_release_date]
-          curation_concern.errors.add(:visibility, 'When setting visibility to "embargo" you must also specify embargo release date.')
-          should_continue = false
-        else
-          attributes.delete(:visibility)
-          curation_concern.apply_embargo(attributes[:embargo_release_date], attributes.delete(:visibility_during_embargo),
-                                         attributes.delete(:visibility_after_embargo))
-          if curation_concern.embargo
-            curation_concern.embargo.save # See https://github.com/projecthydra/hydra-head/issues/226
-          end
-          should_continue = true
-        end
-      else
-        should_continue = true
-        # clear embargo_release_date if it isn't being used. Otherwise it sets the embargo_date
-        # even though they didn't select embargo on the form.
-        attributes.delete(:embargo_release_date)
-      end
-
-      attributes.delete(:visibility_during_embargo)
-      attributes.delete(:visibility_after_embargo)
-
-      should_continue
-    end
-
-    # If user has set visibility to lease, interprets the relevant information and applies it
-    # Returns false if there are any errors and sets an error on the curation_concern
-    def interpret_lease_visibility(attributes = self.attributes)
-      if attributes[:visibility] == Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_LEASE
-        if !attributes[:lease_expiration_date]
-          curation_concern.errors.add(:visibility, 'When setting visibility to "lease" you must also specify lease expiration date.')
-          should_continue = false
-        else
-          curation_concern.apply_lease(attributes[:lease_expiration_date], attributes.delete(:visibility_during_lease),
-                                       attributes.delete(:visibility_after_lease))
-          if curation_concern.lease
-            curation_concern.lease.save # See https://github.com/projecthydra/hydra-head/issues/226
-          end
-          attributes.delete(:visibility)
-          should_continue = true
-        end
-      else
-        # clear lease_expiration_date if it isn't being used. Otherwise it sets the lease_expiration
-        # even though they didn't select lease on the form.
-        attributes.delete(:lease_expiration_date)
-        should_continue = true
-      end
-
-      attributes.delete(:visibility_during_lease)
-      attributes.delete(:visibility_after_lease)
-
-      should_continue
+    included do
+      Deprecation.warn(ManagesEmbargoesActor, "ManagesEmbargoesActor is deprecated and will be removed in CurationConcerns 1.0")
     end
   end
 end

curation_concerns-models/app/actors/curation_concerns/abstract_actor.rb

@@ -0,0 +1,30 @@
+module CurationConcerns
+  # The CurationConcern Abstract actor responds to two primary actions:
+  # * #create
+  # * #update
+  #
+  # and the following attributes
+  #
+  # * next_actor
+  # * curation_concern
+  # * user
+  #
+  # it must instantiate the next actor in the chain and instantiate it.
+  # it should respond to curation_concern, user and attributes.
+  # it ha to next_actor
+  class AbstractActor
+    attr_reader :next_actor, :attributes
+
+    def initialize(curation_concern, user, attributes, more_actors)
+      @attributes = attributes.with_indifferent_access
+      next_actor_class = more_actors.shift || RootActor
+      @next_actor = next_actor_class.new(curation_concern, user, attributes, more_actors)
+    end
+
+    delegate :curation_concern, :user, to: :next_actor
+
+    delegate :create, to: :next_actor
+
+    delegate :update, to: :next_actor
+  end
+end

curation_concerns-models/app/actors/curation_concerns/add_to_collection_actor.rb

@@ -0,0 +1,42 @@
+module CurationConcerns
+  class AddToCollectionActor < AbstractActor
+    def initialize(curation_concern, user, attributes, more_actors)
+      @collection_ids = attributes.delete(:collection_ids)
+      super
+    end
+
+    def create
+      true && next_actor.create
+    end
+
+    def update
+      add_to_collections(@collection_ids) &&
+        next_actor.create
+    end
+
+    private
+
+      # The default behavior of active_fedora's aggregates association,
+      # when assigning the id accessor (e.g. collection_ids = ['foo:1']) is to add
+      # to new collections, but not remove from old collections.
+      # This method ensures it's removed from the old collections.
+      def add_to_collections(new_collection_ids)
+        return true unless new_collection_ids
+        # remove from old collections
+        # TODO: Implement in_collection_ids https://github.com/projecthydra-labs/hydra-pcdm/issues/157
+        (curation_concern.in_collections.map(&:id) - new_collection_ids).each do |old_id|
+          collection = Collection.find(old_id)
+          collection.members.delete(curation_concern)
+          collection.save
+        end
+
+        # add to new
+        new_collection_ids.each do |coll_id|
+          collection = Collection.find(coll_id)
+          collection.members << curation_concern
+          collection.save
+        end
+        true
+      end
+  end
+end

curation_concerns-models/app/actors/curation_concerns/apply_order_actor.rb

@@ -0,0 +1,31 @@
+module CurationConcerns
+  class ApplyOrderActor < AbstractActor
+    attr_reader :next_actor
+    def initialize(curation_concern, user, attributes, more_actors)
+      @ordered_member_ids = attributes.delete(:ordered_member_ids)
+      super
+    end
+
+    delegate :create, to: :next_actor
+
+    def update
+      apply_order(@ordered_member_ids) && next_actor.update
+    end
+
+    private
+
+      def apply_order(new_order)
+        return true unless new_order
+        curation_concern.ordered_member_proxies.each_with_index do |proxy, index|
+          unless new_order[index]
+            proxy.prev.next = curation_concern.ordered_member_proxies.last.next
+            break
+          end
+          proxy.proxy_for = ActiveFedora::Base.id_to_uri(new_order[index])
+          proxy.target = nil
+        end
+        curation_concern.list_source.order_will_change!
+        true
+      end
+  end
+end

curation_concerns-models/app/actors/curation_concerns/assign_identifier_actor.rb

@@ -0,0 +1,7 @@
+module CurationConcerns
+  class AssignIdentifierActor < AbstractActor
+    def create
+      curation_concern.assign_id && next_actor.create
+    end
+  end
+end

curation_concerns-models/app/actors/curation_concerns/assign_representative_actor.rb

@@ -0,0 +1,18 @@
+module CurationConcerns
+  class AssignRepresentativeActor < AbstractActor
+    def create
+      next_actor.create && assign_representative
+    end
+
+    private
+
+      def assign_representative
+        unless curation_concern.representative_id
+          # TODO: Possible optimization here. Does this cause a fetch of ordered_members if they're already loaded?
+          representative = nil # curation_concern.ordered_members.association.reader.first.target
+          curation_concern.representative = representative if representative
+        end
+        curation_concern.save
+      end
+  end
+end

curation_concerns-models/app/actors/curation_concerns/attach_files_actor.rb

@@ -0,0 +1,41 @@
+module CurationConcerns
+  class AttachFilesActor < AbstractActor
+    attr_reader :next_actor
+    def initialize(curation_concern, user, attributes, more_actors)
+      @files = [attributes.delete(:files)].flatten.compact
+      super
+    end
+
+    def create
+      attach_files && next_actor.create
+    end
+
+    def update
+      next_actor.update && attach_files
+    end
+
+    private
+
+      def attach_files
+        @files.all? do |file|
+          attach_file(file)
+        end
+      end
+
+      def attach_file(file)
+        file_set = ::FileSet.new
+        file_set_actor = CurationConcerns::FileSetActor.new(file_set, user)
+        file_set_actor.create_metadata(curation_concern, visibility_attributes)
+        file_set_actor.create_content(file)
+      end
+
+      # The attributes used for visibility - used to send as initial params to
+      # created FileSets.
+      def visibility_attributes
+        attributes.slice(:visibility, :visibility_during_lease,
+                         :visibility_after_lease, :lease_expiration_date,
+                         :embargo_release_date, :visibility_during_embargo,
+                         :visibility_after_embargo)
+      end
+  end
+end

curation_concerns-models/app/actors/curation_concerns/base_actor.rb

@@ -1,34 +1,27 @@
-
 module CurationConcerns
-  # The CurationConcern base actor should respond to three primary actions:
+  # The CurationConcern base actor responds to two primary actions:
   # * #create
   # * #update
-  # * #delete
-  class BaseActor
-    attr_reader :curation_concern, :user, :attributes, :cloud_resources
-    def initialize(curation_concern, user, input_attributes)
-      @curation_concern = curation_concern
-      @user = user
-      @attributes = input_attributes.dup.with_indifferent_access
-      @visibility = attributes[:visibility]
+  # it must instantiate the next actor in the chain and instantiate it.
+  # it should respond to curation_concern, user and attributes.
+  class BaseActor < AbstractActor
+    attr_reader :cloud_resources
+
+    def initialize(curation_concern, user, attributes, more_actors)
       @cloud_resources = attributes.delete(:cloud_resources.to_s)
+      super
     end
 
-    attr_reader :visibility
-    protected :visibility
-
-    delegate :visibility_changed?, to: :curation_concern
-
     def create
       apply_creation_data_to_curation_concern
       apply_save_data_to_curation_concern
-      save
+      next_actor.create && save
     end
 
     def update
       apply_update_data_to_curation_concern
       apply_save_data_to_curation_concern
-      save
+      next_actor.update && save
     end
 
     protected

curation_concerns-models/app/actors/curation_concerns/file_set_actor.rb

@@ -1,15 +1,11 @@
 module CurationConcerns
   # Actions are decoupled from controller logic so that they may be called from a controller or a background job.
   class FileSetActor
-    include CurationConcerns::ManagesEmbargoesActor
     include CurationConcerns::Lockable
 
-    attr_reader :file_set, :user, :attributes, :curation_concern
+    attr_reader :file_set, :user, :attributes
 
     def initialize(file_set, user)
-      # we're setting attributes and curation_concern to bridge the difference
-      # between CurationConcerns::FileSetActor and ManagesEmbargoesActor
-      @curation_concern = file_set
       @file_set = file_set
       @user = user
     end
@@ -30,7 +26,7 @@ def create_metadata(work, file_set_params = {})
       file_set.date_modified = now
       file_set.creator = [user.user_key]
 
-      interpret_visibility file_set_params if assign_visibility?(file_set_params)
+      InterpretVisibilityActor.new(file_set, user, file_set_params, []).create if assign_visibility?(file_set_params)
       attach_file_to_work(work, file_set, file_set_params) if work
       yield(file_set) if block_given?
     end
@@ -70,9 +66,9 @@ def update_content(file, relation = 'original_file')
     end
 
     def update_metadata(attributes)
-      update_visibility(attributes)
-      # attributes.delete(:visibility) # Applying this attribute is handled by update_visibility
-      file_set.attributes = attributes
+      actor = InterpretVisibilityActor.new(file_set, user, attributes, [])
+      actor.update
+      file_set.attributes = actor.next_actor.attributes
       file_set.date_modified = CurationConcerns::TimeService.time_in_utc
       save do
         CurationConcerns.config.callback.run(:after_update_metadata, file_set, user)
@@ -128,11 +124,6 @@ def assign_visibility?(file_set_params = {})
         !((file_set_params || {}).keys & %w(visibility embargo_release_date lease_expiration_date)).empty?
       end
 
-      # This method can be overridden in case there is a custom approach for visibility (e.g. embargo)
-      def update_visibility(attributes)
-        interpret_visibility(attributes) # relies on CurationConcerns::ManagesEmbargoesActor to interpret and apply visibility
-      end
-
       # copy visibility from source_concern to destination_concern
       def copy_visibility(source_concern, destination_concern)
         destination_concern.visibility =  source_concern.visibility