Pass object instead of ID to avoid unnecessary lookups

app/controllers/concerns/curation_concerns/file_sets_controller_behavior.rb

@@ -164,7 +164,8 @@ def empty_file?(file)
 
       def process_file(file)
         update_metadata_from_upload_screen
-        actor.create_metadata(params[:upload_set_id], parent_id, params[:file_set])
+        parent = ActiveFedora::Base.find(parent_id)
+        actor.create_metadata(params[:upload_set_id], parent, params[:file_set])
         if actor.create_content(file)
           respond_to do |format|
             format.html do

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

@@ -20,10 +20,10 @@ def initialize(file_set, user)
     # we have to save both the parent work and the file_set in order to record the "metadata" relationship
     # between them.
     # @param [String] upload_set_id id of the batch of files that the file was uploaded with
-    # @param [String] work_id id of the parent work that will contain the file_set.
+    # @param [ActiveFedora::Base] work the parent work that will contain the file_set.
     # @param [Hash] file_set specifying the visibility, lease and/or embargo of the file set.  If you don't provide at least one of visibility, embargo_release_date or lease_expiration_date, visibility will be copied from the parent.
 
-    def create_metadata(upload_set_id, work_id, file_set_params = {})
+    def create_metadata(upload_set_id, work, file_set_params = {})
       file_set.apply_depositor_metadata(user)
       now = CurationConcerns::TimeService.time_in_utc
       file_set.date_uploaded = now
@@ -40,15 +40,15 @@ def create_metadata(upload_set_id, work_id, file_set_params = {})
       if assign_visibility?(file_set_params)
         interpret_visibility file_set_params
       end
-      # TODO: Why do we need to check if work_id is blank? Shoudn't that raise an error?
-      attach_file_to_work(work_id, file_set, file_set_params) unless work_id.blank?
+      # TODO: Why do we need to check if work is nil? Shoudn't that raise an error?
+      attach_file_to_work(work, file_set, file_set_params) if work
       yield(file_set) if block_given?
     end
 
     # Puts the uploaded content into a staging directory. Then kicks off a
     # job to characterize and create derivatives with this on disk variant.
     # Simultaneously moving a preservation copy to the repostiory.
-    # TODO create a job to monitor this directory and prune old files that
+    # TODO: create a job to monitor this directory and prune old files that
     # have made it to the repo
     # @param [ActionDigest::HTTP::UploadedFile, Tempfile] file the file uploaded by the user.
     def create_content(file)
@@ -162,10 +162,8 @@ def save
       # Adds a FileSet to the work using ore:Aggregations.
       # Locks to ensure that only one process is operating on
       # the list at a time.
-      def attach_file_to_work(work_id, file_set, file_set_params)
-        acquire_lock_for(work_id) do
-          work = ActiveFedora::Base.find(work_id)
-
+      def attach_file_to_work(work, file_set, file_set_params)
+        acquire_lock_for(work.id) do
           unless assign_visibility?(file_set_params)
             copy_visibility(work, file_set)
           end

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

@@ -69,12 +69,10 @@ def assign_representative
     def attach_file(file)
       file_set = ::FileSet.new
       file_set_actor = CurationConcerns::FileSetActor.new(file_set, user)
-      # TODO: we're passing an ID rather than an object. This means the actor does an unnecessary lookup
-      file_set_actor.create_metadata(curation_concern.id, curation_concern.id, visibility_attributes)
+      file_set_actor.create_metadata(curation_concern.id, curation_concern, visibility_attributes)
       file_set_actor.create_content(file)
       @file_sets ||= []
       @file_sets << file_set # This is so that other methods like assign_representative can access the file_sets without reloading them from fedora
-      curation_concern.members << file_set
     end
 
     # The attributes used for visibility - used to send as initial params to

spec/actors/curation_concerns/file_set_actor_spec.rb

@@ -10,7 +10,7 @@
 
   describe 'creating metadata and content' do
     let(:upload_set_id) { nil }
-    let(:work_id) { nil }
+    let(:work) { nil }
     subject { file_set.reload }
     let(:date_today) { DateTime.now }
 
@@ -22,11 +22,11 @@
       expect(CharacterizeJob).to receive(:perform_later)
       expect(IngestFileJob).to receive(:perform_later).with(file_set.id, /world\.png$/, 'image/png', user.user_key)
       allow(actor).to receive(:acquire_lock_for).and_yield
-      actor.create_metadata(upload_set_id, work_id)
+      actor.create_metadata(upload_set_id, work)
       actor.create_content(uploaded_file)
     end
 
-    context 'when an upload_set_id and work_id are not provided' do
+    context 'when an upload_set_id and work are not provided' do
       let(:upload_set_id) { nil }
       it "leaves the associations blank" do
         expect(subject.upload_set).to be_nil
@@ -41,9 +41,8 @@
       end
     end
 
-    context 'when a work_id is provided' do
+    context 'when a work is provided' do
       let(:work) { FactoryGirl.create(:generic_work) }
-      let(:work_id) { work.id }
 
       it 'adds the generic file to the parent work' do
         expect(subject.generic_works).to eq [work]
@@ -64,11 +63,10 @@
 
   describe "#create_metadata" do
     let(:work) { create(:public_generic_work) }
-    let(:work_id) { work.id }
 
     it 'copies visibility from the parent' do
       allow(actor).to receive(:acquire_lock_for).and_yield
-      actor.create_metadata(nil, work_id)
+      actor.create_metadata(nil, work)
       saved_file = file_set.reload
       expect(saved_file.visibility).to eq Hydra::AccessControls::AccessRight::VISIBILITY_TEXT_VALUE_PUBLIC
     end

spec/actors/curation_concerns/work_actor_spec.rb

@@ -1,11 +1,19 @@
 require 'spec_helper'
+require 'redlock'
 
 describe CurationConcerns::GenericWorkActor do
   include ActionDispatch::TestProcess
 
   let(:user) { FactoryGirl.create(:user) }
   let(:file) { curation_concerns_fixture_file_upload('files/image.png', 'image/png') }
 
+  let(:redlock_client_stub) { # stub out redis connection
+    client = double('redlock client')
+    allow(client).to receive(:lock).and_yield(true)
+    allow(Redlock::Client).to receive(:new).and_return(client)
+    client
+  }
+
   subject do
     CurationConcerns::CurationConcern.actor(curation_concern, user, attributes)
   end
@@ -56,6 +64,9 @@
                 ],
                 rights: ['http://creativecommons.org/licenses/by/3.0/us/'] }
             end
+
+            before { redlock_client_stub }
+
             it "applies it to attached files" do
               allow(CharacterizeJob).to receive(:perform_later).and_return(true)
               subject.create
@@ -117,6 +128,7 @@
         context 'authenticated visibility' do
           before do
             allow(CurationConcerns::TimeService).to receive(:time_in_utc) { xmas }
+            redlock_client_stub
           end
 
           it 'stamps each file with the access rights' do
@@ -149,6 +161,7 @@
         context 'authenticated visibility' do
           before do
             allow(CurationConcerns::TimeService).to receive(:time_in_utc) { xmas }
+            redlock_client_stub
           end
 
           it 'stamps each file with the access rights' do

spec/controllers/curation_concerns/file_sets_controller_json_spec.rb

@@ -38,7 +38,7 @@
 
     describe 'created' do
       it "returns 201, renders jq_upload json template and sets location header" do
-        expect(controller.send(:actor)).to receive(:create_metadata).with(nil, parent.id, hash_including(:files, title: ['a title']))
+        expect(controller.send(:actor)).to receive(:create_metadata).with(nil, parent, hash_including(:files, title: ['a title']))
         expect(controller.send(:actor)).to receive(:create_content).with(file).and_return(true)
 
         allow_any_instance_of(FileSet).to receive(:persisted?).and_return(true)

spec/controllers/curation_concerns/file_sets_controller_spec.rb

@@ -21,7 +21,7 @@
         end
 
         it 'calls the actor to create metadata and content' do
-          expect(controller.send(:actor)).to receive(:create_metadata).with(nil, parent.id, files: [file], title: ['test title'], visibility: 'restricted')
+          expect(controller.send(:actor)).to receive(:create_metadata).with(nil, parent, files: [file], title: ['test title'], visibility: 'restricted')
           expect(controller.send(:actor)).to receive(:create_content).with(file).and_return(true)
           xhr :post, :create, parent_id: parent,
                               file_set: { files: [file],

spec/features/create_work_spec.rb

@@ -1,13 +1,22 @@
 require 'spec_helper'
+require 'redlock'
 
 describe 'Creating a new Work' do
   let(:user) { FactoryGirl.create(:user) }
 
+  let(:redlock_client_stub) { # stub out redis connection
+    client = double('redlock client')
+    allow(client).to receive(:lock).and_yield(true)
+    allow(Redlock::Client).to receive(:new).and_return(client)
+    client
+  }
+
   before do
     sign_in user
 
     # stub out characterization. Travis doesn't have fits installed, and it's not relevant to the test.
     expect(CharacterizeJob).to receive(:perform_later)
+    redlock_client_stub
   end
 
   it 'creates the work and allow you to attach a file' do

spec/features/work_generator_spec.rb

@@ -1,9 +1,17 @@
 require 'spec_helper'
 require 'rails/generators'
+require 'redlock'
 
 describe 'Creating a new Work' do
   let(:user) { FactoryGirl.create(:user) }
 
+  let(:redlock_client_stub) { # stub out redis connection
+    client = double('redlock client')
+    allow(client).to receive(:lock).and_yield(true)
+    allow(Redlock::Client).to receive(:new).and_return(client)
+    client
+  }
+
   before do
     Rails::Generators.invoke('curation_concerns:work', ['Catapult'], destination_root: Rails.root)
     load 'spec/internal/app/models/catapult.rb'
@@ -17,6 +25,7 @@
 
     # stub out characterization. Travis doesn't have fits installed, and it's not relevant to the test.
     expect(CharacterizeJob).to receive(:perform_later)
+    redlock_client_stub
   end
 
   after do