Refactoring FileSetActor

* Moving logic to create JobIoWrapper into a factory-like method in the
  JobIoWrapper class. The clue for this refactor was that there were
  several specs for a private method. Also, the private method in
  FileSetactor had intimate knowledge of files and the attributes of
  JobIoWrapper.
* Marking #import_url as deprecated, as it does not appear to be called
  in any tests in Hyrax.
* Adding a class attribute for .file_actor_class; I believe this creates
  a clearer definition of external collaborators.

The original impetus was to remove a Rubocop violation regarding class
length.

.rubocop_fixme.yml

@@ -4,7 +4,6 @@ Security/MarshalLoad:
 
 Metrics/ClassLength:
   Exclude:
-    - 'app/actors/hyrax/actors/file_set_actor.rb'
     - 'app/controllers/hyrax/file_sets_controller.rb'
     - 'app/forms/hyrax/forms/permission_template_form.rb'
     - 'app/presenters/hyrax/work_show_presenter.rb'

app/actors/hyrax/actors/file_set_actor.rb

@@ -22,25 +22,27 @@ def create_content(file, relation = :original_file)
         file_set.label ||= label_for(file)
         file_set.title = [file_set.label] if file_set.title.blank?
         return false unless file_set.save # Need to save to get an id
-        IngestJob.perform_later(wrapper!(file, relation))
+        IngestJob.perform_later(wrapper!(file: file, relation: relation))
       end
 
       # Spawns asynchronous IngestJob with user notification afterward
       # @param [Hyrax::UploadedFile, File, ActionDigest::HTTP::UploadedFile] file the file uploaded by the user
       # @param [Symbol, #to_s] relation
       # @return [IngestJob] the queued job
       def update_content(file, relation = :original_file)
-        IngestJob.perform_later(wrapper!(file, relation), notification: true)
+        IngestJob.perform_later(wrapper!(file: file, relation: relation), notification: true)
       end
 
       # Spawns async ImportUrlJob to attach remote file to fileset
       # @param [#to_s] url
       # @return [IngestUrlJob] the queued job
+      # @todo Remove as it appears to be untested and not called
       def import_url(url)
         file_set.update(import_url: url.to_s)
         operation = Hyrax::Operation.create!(user: user, operation_type: "Attach File")
         ImportUrlJob.perform_later(file_set, operation)
       end
+      deprecation_deprecate import_url: "appears to be untested and not used, and will be removed unless an issue/PR is submitted verifying otherwise"
 
       # @!endgroup
 
@@ -102,9 +104,8 @@ def destroy
         Hyrax.config.callback.run(:after_destroy, file_set.id, user)
       end
 
-      def file_actor_class
-        Hyrax::Actors::FileActor
-      end
+      class_attribute :file_actor_class
+      self.file_actor_class = Hyrax::Actors::FileActor
 
       private
 
@@ -117,25 +118,8 @@ def build_file_actor(relation)
         end
 
         # uses create! because object must be persisted to serialize for jobs
-        def wrapper!(file, relation)
-          JobIoWrapper.create!(wrapper_params(file, relation))
-        end
-
-        # helps testing
-        # @return [Hash] params for JobIoWrapper (new or create)
-        def wrapper_params(file, relation)
-          args = { user: user, relation: relation.to_s, file_set_id: file_set.id }
-          if file.is_a?(Hyrax::UploadedFile)
-            args[:uploaded_file] = file
-            args[:path] = file.uploader.path
-          elsif file.respond_to?(:path)
-            args[:path] = file.path
-            args[:original_name] = file.original_filename if file.respond_to?(:original_filename)
-            args[:original_name] ||= file.original_name if file.respond_to?(:original_name)
-          else
-            raise "Require Hyrax::UploadedFile or File-like object, received #{file.class} object: #{file}"
-          end
-          args
+        def wrapper!(file:, relation:)
+          JobIoWrapper.create_with_varied_file_handling!(user: user, file: file, relation: relation, file_set: file_set)
         end
 
         # For the label, use the original_filename or original_name if it's there.

app/models/job_io_wrapper.rb

@@ -24,6 +24,30 @@ class JobIoWrapper < ApplicationRecord
   after_initialize :static_defaults
   delegate :read, :size, to: :file
 
+  # Responsible for creating a JobIoWrapper from the given parameters, with a
+  # focus on sniffing out attributes from the given :file.
+  #
+  # @param [User] user - The user requesting to create this instance
+  # @param [#path, Hyrax::UploadedFile] file - The file that is to be uploaded
+  # @param [String] relation
+  # @param [FileSet] file_set - The associated file set
+  # @return [JobIoWrapper]
+  # @raise ActiveRecord::RecordInvalid - if the instance is not valid
+  def self.create_with_varied_file_handling!(user:, file:, relation:, file_set:)
+    args = { user: user, relation: relation.to_s, file_set_id: file_set.id }
+    if file.is_a?(Hyrax::UploadedFile)
+      args[:uploaded_file] = file
+      args[:path] = file.uploader.path
+    elsif file.respond_to?(:path)
+      args[:path] = file.path
+      args[:original_name] = file.original_filename if file.respond_to?(:original_filename)
+      args[:original_name] ||= file.original_name if file.respond_to?(:original_name)
+    else
+      raise "Require Hyrax::UploadedFile or File-like object, received #{file.class} object: #{file}"
+    end
+    create!(args)
+  end
+
   def original_name
     super || extracted_original_name
   end

spec/actors/hyrax/actors/file_set_actor_spec.rb

@@ -6,7 +6,6 @@
   let(:user)          { create(:user) }
   let(:file_path)     { File.join(fixture_path, 'world.png') }
   let(:file)          { fixture_file_upload(file_path, 'image/png') } # we will override for the different types of File objects
-  let(:wrapper_path)  { file.path }                                   # override to match
   let(:local_file)    { File.open(file_path) }
   let(:file_set)      { create(:file_set, content: local_file) }
   let(:actor)         { described_class.new(file_set, user) }
@@ -18,37 +17,6 @@
   describe 'private' do
     let(:file_set) { build(:file_set) } # avoid 130+ LDP requests
 
-    describe '#wrapper_params' do
-      let(:baseline_args) { { user: user, file_set_id: file_set.id, relation: relation.to_s } }
-      let(:wrapper_args)  { baseline_args.merge(path: wrapper_path, original_name: actor.send(:label_for, file)) }
-
-      subject { actor.send(:wrapper_params, file, relation) }
-
-      it 'with Rack::Test::UploadedFile returns expected hash' do
-        expect(subject).to eq(wrapper_args)
-        expect(actor.send(:wrapper_params, file, :remastered)).to eq(wrapper_args.merge(relation: 'remastered'))
-      end
-
-      context 'with ::File' do
-        let(:file) { local_file }
-
-        it 'returns expected hash' do
-          wrapper_args.delete(:original_name)
-          expect(subject).to eq(wrapper_args)
-        end
-      end
-
-      context 'with Hyrax::UploadedFile' do
-        let(:file) { Hyrax::UploadedFile.new(user: user, file_set_uri: file_set.uri, file: local_file) }
-        let(:wrapper_path) { file.uploader.path }
-
-        it 'returns expected hash' do
-          wrapper_args.delete(:original_name)
-          expect(subject).to eq(wrapper_args.merge(uploaded_file: file))
-        end
-      end
-    end
-
     describe '#assign_visibility?' do
       let(:viz) { Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PUBLIC }
 
@@ -96,9 +64,7 @@
 
   describe '#create_content' do
     before do
-      expect(JobIoWrapper).to receive(:create!).with(any_args) do |args|
-        JobIoWrapper.new(args) # skip persistence
-      end
+      expect(JobIoWrapper).to receive(:create_with_varied_file_handling!).with(any_args).and_return(JobIoWrapper.new)
       expect(IngestJob).to receive(:perform_later).with(JobIoWrapper)
     end
 

spec/models/job_io_wrapper_spec.rb

@@ -14,6 +14,43 @@
     expect { subject.save! }.not_to raise_error
   end
 
+  describe '.create_with_wrapped_params!' do
+    let(:local_file) { File.open(path) }
+    let(:relation) { :remastered }
+
+    subject { described_class.create_with_varied_file_handling!(user: user, file_set: file_set, file: file, relation: relation) }
+
+    context 'with Rack::Test::UploadedFile' do
+      let(:file) { Rack::Test::UploadedFile.new(path, 'image/png') }
+
+      it 'creates a JobIoWrapper' do
+        expected_create_args = { user: user, relation: relation.to_s, file_set_id: file_set.id, path: file.path, original_name: 'world.png' }
+        expect(JobIoWrapper).to receive(:create!).with(expected_create_args)
+        subject
+      end
+    end
+
+    context 'with ::File' do
+      let(:file) { local_file }
+
+      it 'creates a JobIoWrapper' do
+        expected_create_args = { user: user, relation: relation.to_s, file_set_id: file_set.id, path: file.path }
+        expect(JobIoWrapper).to receive(:create!).with(expected_create_args)
+        subject
+      end
+    end
+
+    context 'with Hyrax::UploadedFile' do
+      let(:file) { Hyrax::UploadedFile.new(user: user, file_set_uri: file_set.uri, file: local_file) }
+
+      it 'creates a JobIoWrapper' do
+        expected_create_args = { user: user, relation: relation.to_s, file_set_id: file_set.id, uploaded_file: file, path: file.uploader.path }
+        expect(JobIoWrapper).to receive(:create!).with(expected_create_args)
+        subject
+      end
+    end
+  end
+
   describe 'uploaded_file' do
     let(:other_path) { fixture_path + '/image.jpg' }
     let(:uploaded_file) { Hyrax::UploadedFile.new(user: user, file_set_uri: file_set.uri, file: File.new(other_path)) }