diff --git a/app/controllers/supplemental_files_controller.rb b/app/controllers/supplemental_files_controller.rb index a37de63b38..27b4bebcdd 100644 --- a/app/controllers/supplemental_files_controller.rb +++ b/app/controllers/supplemental_files_controller.rb @@ -110,6 +110,8 @@ def update @supplemental_file.attach_file(attachment) if attachment raise Avalon::SaveError, @supplemental_file.errors.full_messages unless @supplemental_file.save + # Updates parent object's solr document + @object.update_index flash[:success] = "Supplemental file successfully updated." respond_to do |format| diff --git a/app/models/concerns/supplemental_file_behavior.rb b/app/models/concerns/supplemental_file_behavior.rb index d911c693c8..e204a274ea 100644 --- a/app/models/concerns/supplemental_file_behavior.rb +++ b/app/models/concerns/supplemental_file_behavior.rb @@ -29,7 +29,20 @@ module SupplementalFileBehavior # @return [SupplementalFile] def supplemental_files(tag: '*') return [] if supplemental_files_json.blank? - files = JSON.parse(supplemental_files_json).collect { |file_gid| GlobalID::Locator.locate(file_gid) } + # If the supplemental_files_json becomes out of sync with the + # database after a delete, this check could fail. Have not + # encountered in a live environment but came up in automated + # testing. Adding a rescue on fail to locate allows us to skip + # these out of sync files. + files = JSON.parse(supplemental_files_json).collect do |file_gid| + begin + GlobalID::Locator.locate(file_gid) + rescue ActiveRecord::RecordNotFound + nil + end + end.compact + return [] if files.blank? + case tag when '*' files diff --git a/app/models/supplemental_file.rb b/app/models/supplemental_file.rb index 135f3f00b1..e000c73eb8 100644 --- a/app/models/supplemental_file.rb +++ b/app/models/supplemental_file.rb @@ -31,6 +31,7 @@ class SupplementalFile < ApplicationRecord # See https://github.com/rails/rails/issues/37304 after_create_commit :index_file, prepend: true after_update_commit :update_index, prepend: true + after_destroy_commit :remove_from_index def attach_file(new_file) file.attach(new_file) @@ -103,6 +104,10 @@ def update_index end alias index_file update_index + def remove_from_index + ActiveFedora::SolrService.delete(to_global_id) + end + # Creates a solr document hash for the {#object} # @return [Hash] the solr document def to_solr diff --git a/spec/models/supplemental_file_spec.rb b/spec/models/supplemental_file_spec.rb index fd50cc22d2..65a3e65fac 100644 --- a/spec/models/supplemental_file_spec.rb +++ b/spec/models/supplemental_file_spec.rb @@ -95,6 +95,38 @@ after_doc = ActiveFedora::SolrService.query("id:#{RSolr.solr_escape(transcript.to_global_id.to_s)}").first expect(after_doc["transcript_tsim"].first).to eq("00:00:01.200 --> 00:00:21.000 [music]") end + + context 'caption as transcript' do + let(:transcript) { FactoryBot.create(:supplemental_file, :with_caption_file, tags: ['caption', 'transcript']) } + + it 'removes the transcript_tsim content when transcript tag is removed' do + before_doc = ActiveFedora::SolrService.query("id:#{RSolr.solr_escape(transcript.to_global_id.to_s)}").first + expect(before_doc["transcript_tsim"].first).to eq "00:00:03.500 --> 00:00:05.000 Example captions" + transcript.tags = ['caption'] + transcript.save + after_doc = ActiveFedora::SolrService.query("id:#{RSolr.solr_escape(transcript.to_global_id.to_s)}").first + expect(after_doc["transcript_tsim"]).to be_nil + end + end + end + end + + describe "#remove_from_index" do + let(:transcript) { FactoryBot.create(:supplemental_file, :with_transcript_file, :with_transcript_tag) } + + context 'on delete' do + it 'triggers callback' do + expect(transcript).to receive(:remove_from_index) + transcript.destroy + end + + it 'removes the transcript from the index' do + before_doc = ActiveFedora::SolrService.query("id:#{RSolr.solr_escape(transcript.to_global_id.to_s)}").first + expect(before_doc['transcript_tsim']).to eq ["00:00:03.500 --> 00:00:05.000 Example captions"] + transcript.destroy + after_doc = ActiveFedora::SolrService.query("id:#{RSolr.solr_escape(transcript.to_global_id.to_s)}").first + expect(after_doc).to be_nil + end end end diff --git a/spec/support/supplemental_files_controller_examples.rb b/spec/support/supplemental_files_controller_examples.rb index a890957323..84b87795ea 100644 --- a/spec/support/supplemental_files_controller_examples.rb +++ b/spec/support/supplemental_files_controller_examples.rb @@ -485,11 +485,32 @@ end context "removing transcript designation" do let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_transcript_file, tags: ['caption', 'transcript'], label: 'label (machine generated)') } + + before do + if object.is_a?(MasterFile) + supplemental_file.parent_id = object.id + supplemental_file.save + mo = object.media_object + mo.sections = [object] + mo.save + end + end + it "removes transcript note from tags" do expect { put :update, params: { class_id => object.id, id: supplemental_file.id, supplemental_file: valid_update_attributes, format: :html }, session: valid_session }.to change { master_file.reload.supplemental_files.first.tags }.from(['caption', 'transcript']).to(['caption']) end + + it 'removes transcript from parent media object\'s index' do + # Media object only looks at section files for has_transcript check, + # so we only test against the MasterFile case. + if object.is_a?(MasterFile) + expect{ + put :update, params: { class_id => object.id, id: supplemental_file.id, supplemental_file:valid_update_attributes, format: :html}, session: valid_session + }.to change { object.media_object.to_solr(include_child_fields: true)['has_transcripts_bsi'] }.from(true).to(false) + end + end end end @@ -605,5 +626,29 @@ expect(JSON.parse(response.body)["errors"]).to be_present end end + + context 'supplemental file marked as transcript' do + let(:supplemental_file) { FactoryBot.create(:supplemental_file, :with_transcript_file, tags: ['caption', 'transcript']) } + + before do + if object.is_a?(MasterFile) + supplemental_file.parent_id = object.id + supplemental_file.save + mo = object.media_object + mo.sections = [object] + mo.save + end + end + + it 'removes transcript from parent media object\'s index' do + # Media object only looks at section files for has_transcript check, + # so we only test against the MasterFile case. + if object.is_a?(MasterFile) + expect{ + delete :destroy, params: { class_id => object.id, id: supplemental_file.id, format: :html}, session: valid_session + }.to change { object.media_object.to_solr(include_child_fields: true)['has_transcripts_bsi'] }.from(true).to(false) + end + end + end end end