From f1eb52b9a0a20d4464c15601d109e676145f60fe Mon Sep 17 00:00:00 2001
From: Justin Coyne <justin@curationexperts.com>
Date: Wed, 14 May 2014 22:54:59 -0500
Subject: [PATCH] Cease auto characterizing on save. Instead run the
 characterization from the FilesControllerBehavior. This makes testing far
 easier. Fixes #232

---
 .../sufia/files_controller_behavior.rb        |  7 ++++
 spec/controllers/batch_controller_spec.rb     |  1 -
 spec/controllers/catalog_controller_spec.rb   |  1 -
 spec/controllers/dashboard_controller_spec.rb |  1 -
 spec/controllers/downloads_controller_spec.rb |  1 -
 .../generic_files_controller_spec.rb          |  4 +-
 spec/controllers/homepage_controller_spec.rb  |  1 -
 spec/jobs/audit_job_spec.rb                   |  2 -
 spec/jobs/create_derivatives_job_spec.rb      |  8 ----
 spec/jobs/event_jobs_spec.rb                  |  1 -
 spec/models/batch_spec.rb                     |  6 ---
 spec/models/checksum_audit_log_spec.rb        |  1 -
 spec/models/featured_work_list_spec.rb        |  1 -
 spec/models/file_content_datastream_spec.rb   |  6 ---
 spec/models/generic_file_spec.rb              | 42 ++-----------------
 .../app/models/concerns/sufia/generic_file.rb |  2 +-
 .../sufia/generic_file/characterization.rb    |  6 ---
 17 files changed, 14 insertions(+), 77 deletions(-)

diff --git a/app/controllers/concerns/sufia/files_controller_behavior.rb b/app/controllers/concerns/sufia/files_controller_behavior.rb
index c06b0aa738..21373b61e1 100644
--- a/app/controllers/concerns/sufia/files_controller_behavior.rb
+++ b/app/controllers/concerns/sufia/files_controller_behavior.rb
@@ -138,9 +138,14 @@ def update
 
     protected
 
+    def push_characterize_job
+      Sufia.queue.push(CharacterizeJob.new(@generic_file.pid))
+    end
+
     def update_version
       Sufia::GenericFile::Actions.revert_content(@generic_file, params[:revision], datastream_id, current_user)
       return false unless @generic_file.save
+      push_characterize_job
       Sufia.queue.push(ContentRestoredVersionEventJob.new(@generic_file.pid, current_user.user_key, params[:revision]))
       @generic_file.record_version_committer(current_user)
     end
@@ -148,6 +153,7 @@ def update_version
     def update_file
       Sufia::GenericFile::Actions.update_content(@generic_file, params[:filedata], datastream_id, current_user)
       return false unless @generic_file.save
+      push_characterize_job
       Sufia.queue.push(ContentNewVersionEventJob.new(@generic_file.pid, current_user.user_key))
       @generic_file.record_version_committer(current_user)
     end
@@ -176,6 +182,7 @@ def process_file(file)
       update_metadata_from_upload_screen
       create_metadata(@generic_file)
       if Sufia::GenericFile::Actions.create_content(@generic_file, file, file.original_filename, datastream_id, current_user)
+        push_characterize_job
         respond_to do |format|
           format.html {
             render :json => [@generic_file.to_jq_upload],
diff --git a/spec/controllers/batch_controller_spec.rb b/spec/controllers/batch_controller_spec.rb
index 4e7ea489a3..355f20c667 100644
--- a/spec/controllers/batch_controller_spec.rb
+++ b/spec/controllers/batch_controller_spec.rb
@@ -111,7 +111,6 @@
   describe "#edit" do
     before do
       User.any_instance.stub(:display_name).and_return("Jill Z. User")
-      GenericFile.any_instance.stub(:characterize_if_changed).and_yield
       @b1 = Batch.new
       @b1.save
       @file = GenericFile.new(:batch=>@b1, :label=>'f1')
diff --git a/spec/controllers/catalog_controller_spec.rb b/spec/controllers/catalog_controller_spec.rb
index 90d20bcaa7..79722f1812 100644
--- a/spec/controllers/catalog_controller_spec.rb
+++ b/spec/controllers/catalog_controller_spec.rb
@@ -6,7 +6,6 @@
   let(:user) { @user }
 
   before do
-    GenericFile.any_instance.stub(:characterize_if_changed).and_yield
     sign_in user
   end
 
diff --git a/spec/controllers/dashboard_controller_spec.rb b/spec/controllers/dashboard_controller_spec.rb
index fddf975d4b..88ec050611 100644
--- a/spec/controllers/dashboard_controller_spec.rb
+++ b/spec/controllers/dashboard_controller_spec.rb
@@ -2,7 +2,6 @@
 
 describe DashboardController do
   before do
-    GenericFile.any_instance.stub(:terms_of_service).and_return('1')
     User.any_instance.stub(:groups).and_return([])
     controller.stub(:clear_session_user) ## Don't clear out the authenticated session
   end
diff --git a/spec/controllers/downloads_controller_spec.rb b/spec/controllers/downloads_controller_spec.rb
index f038eb7e8d..b6caf07dd5 100644
--- a/spec/controllers/downloads_controller_spec.rb
+++ b/spec/controllers/downloads_controller_spec.rb
@@ -7,7 +7,6 @@
       @f = GenericFile.new(:pid => 'sufia:test1')
       @f.apply_depositor_metadata('archivist1@example.com')
       @f.add_file(File.open(fixture_path + '/world.png'), 'content', 'world.png')
-      @f.should_receive(:characterize_if_changed).and_yield
       @f.save!
     end
 
diff --git a/spec/controllers/generic_files_controller_spec.rb b/spec/controllers/generic_files_controller_spec.rb
index 2ea8d3fb82..0379e0a263 100644
--- a/spec/controllers/generic_files_controller_spec.rb
+++ b/spec/controllers/generic_files_controller_spec.rb
@@ -386,7 +386,7 @@
         end
       end
       before do
-        GenericFile.any_instance.stub(:characterize_if_changed).and_yield
+        allow(controller).to receive(:push_characterize_job)
         sign_in user
       end
 
@@ -491,9 +491,9 @@
       f.add_file(File.open(fixture_path + '/world.png'), 'content', 'world.png')
       # grant public read access explicitly
       f.read_groups = ['public']
-      f.should_receive(:characterize_if_changed).and_yield
       f.save
       @file = f
+      allow(controller).to receive(:push_characterize_job)
     end
     after do
       GenericFile.find('sufia:test5').destroy
diff --git a/spec/controllers/homepage_controller_spec.rb b/spec/controllers/homepage_controller_spec.rb
index d7c153b3c9..627e090dc0 100644
--- a/spec/controllers/homepage_controller_spec.rb
+++ b/spec/controllers/homepage_controller_spec.rb
@@ -21,7 +21,6 @@
 
     let(:user) { FactoryGirl.find_or_create(:jill) }
     before do
-      allow_any_instance_of(GenericFile).to receive(:characterize_if_changed).and_yield
       sign_in user
     end
 
diff --git a/spec/jobs/audit_job_spec.rb b/spec/jobs/audit_job_spec.rb
index b0136428ec..96751c0e25 100644
--- a/spec/jobs/audit_job_spec.rb
+++ b/spec/jobs/audit_job_spec.rb
@@ -4,8 +4,6 @@
   before do
     @user = FactoryGirl.find_or_create(:jill)
     @inbox = @user.mailbox.inbox
-    GenericFile.any_instance.should_receive(:characterize_if_changed).and_yield
-    GenericFile.any_instance.stub(:terms_of_service).and_return('1')
     @file = GenericFile.new
     @file.apply_depositor_metadata(@user)
     @file.save
diff --git a/spec/jobs/create_derivatives_job_spec.rb b/spec/jobs/create_derivatives_job_spec.rb
index 80d1826885..ecc402a4a3 100644
--- a/spec/jobs/create_derivatives_job_spec.rb
+++ b/spec/jobs/create_derivatives_job_spec.rb
@@ -21,7 +21,6 @@
     context 'with a video (.avi) file', unless: $in_travis do
       before do
         @generic_file.add_file(File.open(fixture_path + '/countdown.avi'), 'content', 'countdown.avi')
-        allow(@generic_file).to receive(:characterize_if_changed).and_yield
         allow_any_instance_of(GenericFile).to receive(:mime_type).and_return('video/avi')
         @generic_file.save!
       end
@@ -41,7 +40,6 @@
     context 'with an audio (.wav) file', unless: $in_travis do
       before do
         @generic_file.add_file(File.open(fixture_path + '/piano_note.wav'), 'content', 'piano_note.wav')
-        allow(@generic_file).to receive(:characterize_if_changed).and_yield
         allow_any_instance_of(GenericFile).to receive(:mime_type).and_return('audio/wav')
         @generic_file.save!
       end
@@ -60,7 +58,6 @@
     context 'with an image (.jp2) file' do
       before do
         @generic_file.add_file(File.open(fixture_path + '/image.jp2'), 'content', 'image.jp2')
-        allow(@generic_file).to receive(:characterize_if_changed).and_yield
         allow_any_instance_of(GenericFile).to receive(:mime_type).and_return('image/jp2')
         @generic_file.save!
       end
@@ -80,7 +77,6 @@
     context 'with an office document (.docx) file', unless: $in_travis do
       before do
         @generic_file.add_file(File.open(fixture_path + '/charter.docx'), 'content', 'charter.docx')
-        allow(@generic_file).to receive(:characterize_if_changed).and_yield
         allow_any_instance_of(GenericFile).to receive(:mime_type).and_return('application/vnd.openxmlformats-officedocument.wordprocessingml.document')
         @generic_file.save!
       end
@@ -102,7 +98,6 @@
     context 'with a video (.avi) file', unless: $in_travis do
       before do
         @generic_file.add_file(File.open(fixture_path + '/countdown.avi'), 'content', 'countdown.avi')
-        allow(@generic_file).to receive(:characterize_if_changed).and_yield
         allow_any_instance_of(GenericFile).to receive(:mime_type).and_return('video/avi')
         @generic_file.save!
       end
@@ -125,7 +120,6 @@
     context 'with an audio (.wav) file', unless: $in_travis do
       before do
         @generic_file.add_file(File.open(fixture_path + '/piano_note.wav'), 'content', 'piano_note.wav')
-        allow(@generic_file).to receive(:characterize_if_changed).and_yield
         allow_any_instance_of(GenericFile).to receive(:mime_type).and_return('audio/wav')
         @generic_file.save!
       end
@@ -149,7 +143,6 @@
       # Uncomment when this is no longer pending
       # before do
       #   @generic_file.add_file(File.open(fixture_path + '/sufia/sufia_test5.mp3'), 'content', 'sufia_test5.mp3')
-      #   allow(@generic_file).to receive(:characterize_if_changed).and_yield
       #   @generic_file.save!
       # end
 
@@ -171,7 +164,6 @@
       # Uncomment when this is no longer pending
       # before do
       #   @generic_file.add_file(File.open(fixture_path + '/Example.ogg'), 'content', 'Example.ogg')
-      #   allow(@generic_file).to receive(:characterize_if_changed).and_yield
       #   @generic_file.save!
       # end
 
diff --git a/spec/jobs/event_jobs_spec.rb b/spec/jobs/event_jobs_spec.rb
index 258b6e367b..90ed79844a 100644
--- a/spec/jobs/event_jobs_spec.rb
+++ b/spec/jobs/event_jobs_spec.rb
@@ -5,7 +5,6 @@
     @user = FactoryGirl.find_or_create(:jill)
     @another_user = FactoryGirl.find_or_create(:archivist)
     @third_user = FactoryGirl.find_or_create(:curator)
-    GenericFile.any_instance.stub(:terms_of_service).and_return('1')
     @gf = GenericFile.new(pid: 'test:123')
     @gf.apply_depositor_metadata(@user)
     @gf.title = 'Hamlet'
diff --git a/spec/models/batch_spec.rb b/spec/models/batch_spec.rb
index ec1bd66933..609005048d 100644
--- a/spec/models/batch_spec.rb
+++ b/spec/models/batch_spec.rb
@@ -3,8 +3,6 @@
 describe Batch do
   before(:all) do
     @user = FactoryGirl.find_or_create(:jill)
-    GenericFile.any_instance.should_receive(:characterize_if_changed).and_yield
-    GenericFile.any_instance.stub(:terms_of_service).and_return('1')
     @file = GenericFile.new
     @file.apply_depositor_metadata('mjg36')
     @file.save
@@ -36,11 +34,7 @@
     @batch.part.should == [@file.pid]
   end
   it "should be able to have more than one file" do
-    # not sure why this is needed here too, but when the test runs alone it is not needed but when run in the group it is needed
-    GenericFile.any_instance.stub(:terms_of_service).and_return('1')
-    #logger.info "before create"
     gf = GenericFile.new
-    #logger.info "after create"
     gf.apply_depositor_metadata('mjg36')
     gf.save
     @batch.part << gf.pid
diff --git a/spec/models/checksum_audit_log_spec.rb b/spec/models/checksum_audit_log_spec.rb
index 42589c3cda..f92d67ea77 100644
--- a/spec/models/checksum_audit_log_spec.rb
+++ b/spec/models/checksum_audit_log_spec.rb
@@ -5,7 +5,6 @@
     @f = GenericFile.new
     @f.add_file(File.open(fixture_path + '/world.png'), 'content', 'world.png')
     @f.apply_depositor_metadata('mjg36')
-    @f.stub(:characterize_if_changed).and_yield #don't run characterization
     @f.save!
     @version = @f.datastreams['content'].versions.first
     @old = ChecksumAuditLog.create(:pid=>@f.pid, :dsid=>@version.dsid, :version=>@version.versionID, :pass=>1, :created_at=>2.minutes.ago)
diff --git a/spec/models/featured_work_list_spec.rb b/spec/models/featured_work_list_spec.rb
index b665ba8e72..6b6ad914a0 100644
--- a/spec/models/featured_work_list_spec.rb
+++ b/spec/models/featured_work_list_spec.rb
@@ -5,7 +5,6 @@
   let(:file2) { FactoryGirl.create(:generic_file) }
 
   before do
-    GenericFile.any_instance.stub(:characterize_if_changed).and_yield
     FeaturedWork.create(generic_file_id: file1.noid)
     FeaturedWork.create(generic_file_id: file2.noid)
   end
diff --git a/spec/models/file_content_datastream_spec.rb b/spec/models/file_content_datastream_spec.rb
index f9e15811a7..3ea8008cc1 100644
--- a/spec/models/file_content_datastream_spec.rb
+++ b/spec/models/file_content_datastream_spec.rb
@@ -1,15 +1,11 @@
 require 'spec_helper'
 
 describe FileContentDatastream do
-  before do
-    Sufia.queue.stub(:push).with(an_instance_of CharacterizeJob) #don't run characterization
-  end
   describe "version control" do
     before do
       f = GenericFile.new
       f.add_file(File.open(fixture_path + '/world.png'), 'content', 'world.png')
       f.apply_depositor_metadata('mjg36')
-      f.stub(:characterize_if_changed).and_yield #don't run characterization
       f.save
       @file = f.reload
     end
@@ -34,7 +30,6 @@
     describe "add a version" do
       before do
         @file.add_file(File.open(fixture_path + '/world.png'), 'content', 'world.png')
-        @file.stub(:characterize_if_changed).and_yield #don't run characterization
         @file.save
       end
       it "should return two versions" do
@@ -77,7 +72,6 @@
     before do
       @generic_file = GenericFile.new
       @generic_file.apply_depositor_metadata('mjg36')
-      @generic_file.stub(:characterize_if_changed).and_yield #don't run characterization
     end
     after do
       @generic_file.delete
diff --git a/spec/models/generic_file_spec.rb b/spec/models/generic_file_spec.rb
index 9220b8395b..8ed3e0c7e1 100644
--- a/spec/models/generic_file_spec.rb
+++ b/spec/models/generic_file_spec.rb
@@ -320,7 +320,6 @@
   describe "create_thumbnail" do
     before do
       @f = GenericFile.new
-      #@f.stub(:characterize_if_changed).and_yield #don't run characterization
       @f.apply_depositor_metadata('mjg36')
     end
     after do
@@ -344,7 +343,6 @@
       u = FactoryGirl.create(:jill)
       @f = GenericFile.new.tap do |gf|
         gf.apply_depositor_metadata(u)
-        gf.stub(:characterize_if_changed).and_yield #don't run characterization
         gf.save!
       end
       @t = Trophy.create(user_id: u.id, generic_file_id: @f.pid)
@@ -365,7 +363,6 @@
       f = GenericFile.new
       f.add_file(File.open(fixture_path + '/world.png'), 'content', 'world.png')
       f.apply_depositor_metadata(u)
-      f.stub(:characterize_if_changed).and_yield #don't run characterization
       f.save!
       @f = f.reload
     end
@@ -412,7 +409,6 @@
       @f = GenericFile.new
       @f.add_file(File.open(fixture_path + '/world.png'), 'content', 'world.png')
       @f.apply_depositor_metadata('mjg36')
-      @f.stub(:characterize_if_changed).and_yield #don't run characterization
       @f.save!
       @version = @f.datastreams['content'].versions.first
       @old = ChecksumAuditLog.create(:pid=>@f.pid, :dsid=>@version.dsid, :version=>@version.versionID, :pass=>1, :created_at=>2.minutes.ago)
@@ -439,18 +435,6 @@
 
   end
 
-  describe "save" do
-    after do
-      subject.destroy
-    end
-    it "should schedule a characterization job" do
-      subject.add_file(File.open(fixture_path + '/world.png'), 'content', 'world.png')
-      s1 = double('one')
-      allow(CharacterizeJob).to receive(:new).and_return(s1)
-      expect(Sufia.queue).to receive(:push).with(s1).once
-      subject.save
-    end
-  end
   describe "related_files" do
     let(:batch_id) { "foobar:100" }
     before(:each) do
@@ -488,7 +472,6 @@
   end
   describe "noid integration" do
     before(:all) do
-      GenericFile.any_instance.should_receive(:characterize_if_changed).and_yield
       @new_file = GenericFile.new(:pid => 'ns:123')
       @new_file.apply_depositor_metadata('mjg36')
       @new_file.save
@@ -511,29 +494,17 @@
   describe "characterize" do
     it "should return expected results when called", :unless => $in_travis do
       subject.add_file(File.open(fixture_path + '/world.png'), 'content', 'world.png')
-      # Without the stub(:save), the after save callback was being triggered
-      # which resulted the characterize_if_changed method being called; which
-      # enqueued a job for characterizing
-      subject.stub(:save)
       subject.characterize
       doc = Nokogiri::XML.parse(subject.characterization.content)
       doc.root.xpath('//ns:imageWidth/text()', {'ns'=>'http://hul.harvard.edu/ois/xml/ns/fits/fits_output'}).inner_text.should == '50'
     end
-    it "is not triggered unless the content datastream is changed" do
-      expect(Sufia.queue).to receive(:push).once
-      subject.content.content = "hey"
-      subject.save
-      subject.related_url = 'http://example.com'
-      expect(Sufia.queue).to receive(:push).never
-      subject.save
-      subject.destroy
-    end
-    describe "after job runs" do
+    context "after characterization" do
       before(:all) do
         myfile = GenericFile.new
-        myfile.add_file(File.open(fixture_path + '/sufia/sufia_test4.pdf'), 'content', 'sufia_test4.pdf')
+        myfile.add_file(File.open(fixture_path + '/sufia/sufia_test4.pdf', 'rb').read, 'content', 'sufia_test4.pdf')
         myfile.label = 'label123'
         myfile.apply_depositor_metadata('mjg36')
+        myfile.characterize
         myfile.save
         @myfile = myfile.reload
       end
@@ -556,9 +527,7 @@
         @myfile.title.should include("Microsoft Word - sample.pdf.docx")
         @myfile.filename[0].should == @myfile.label
       end
-      it "should include thumbnail generation in characterization job" do
-        @myfile.thumbnail.size.should_not be_nil
-      end
+
       it "should append each term only once" do
         @myfile.append_metadata
         @myfile.format_label.should == ["Portable Document Format"]
@@ -999,9 +968,6 @@
     end
   end
   describe "file content validation" do
-    before do
-      allow(Sufia.queue).to receive(:push).with(an_instance_of CharacterizeJob) # don't run characterization
-    end
     context "when file contains a virus" do
       let(:f) { File.new(fixture_path + '/small_file.txt') }
       after(:each) do
diff --git a/sufia-models/app/models/concerns/sufia/generic_file.rb b/sufia-models/app/models/concerns/sufia/generic_file.rb
index 55e6ef0914..0a364ffbf1 100644
--- a/sufia-models/app/models/concerns/sufia/generic_file.rb
+++ b/sufia-models/app/models/concerns/sufia/generic_file.rb
@@ -23,7 +23,7 @@ module GenericFile
     included do
       belongs_to :batch, :property => :is_part_of
 
-      around_save :characterize_if_changed, :retry_warming
+      around_save :retry_warming
 
       #attr_accessible *(ds_specs['descMetadata'][:type].fields + [:permissions])
       attr_accessible *(terms_for_display + [:part_of, :permissions])
diff --git a/sufia-models/app/models/concerns/sufia/generic_file/characterization.rb b/sufia-models/app/models/concerns/sufia/generic_file/characterization.rb
index 8d2c019312..3dda65d4df 100644
--- a/sufia-models/app/models/concerns/sufia/generic_file/characterization.rb
+++ b/sufia-models/app/models/concerns/sufia/generic_file/characterization.rb
@@ -40,12 +40,6 @@ def sample_rate
         characterization.sample_rate.blank? ? characterization.video_sample_rate : characterization.sample_rate
       end
 
-      def characterize_if_changed
-        content_changed = self.content.changed?
-        yield
-        Sufia.queue.push(CharacterizeJob.new(self.pid)) if content_changed
-      end
-
       ## Extract the metadata from the content datastream and record it in the characterization datastream
       def characterize
         self.characterization.ng_xml = self.content.extract_metadata