From 087694cc0876818d9d511895584502ffff10066c Mon Sep 17 00:00:00 2001 From: Chris Want Date: Mon, 23 Jul 2018 11:23:47 -0600 Subject: [PATCH 01/21] Fix issue where `self.file_location` may not be set when `working_file_path` is called. --- app/models/master_file.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/master_file.rb b/app/models/master_file.rb index 7c41e4f0a4..e188711a29 100644 --- a/app/models/master_file.rb +++ b/app/models/master_file.rb @@ -647,9 +647,9 @@ def saveOriginal(file, original_name=nil) path = File.join(File.dirname(realpath), original_name) File.rename(realpath, path) realpath = path - self.file_location = realpath end + self.file_location = realpath newpath = working_file_path FileUtils.cp(realpath, newpath) unless newpath.blank? end From 4eb272f290b9a1d830b019f3a7e4a975026c0d5a Mon Sep 17 00:00:00 2001 From: Brian Keese Date: Mon, 23 Jul 2018 13:27:58 -0400 Subject: [PATCH 02/21] Fix inadvertantly renamed api option replace_masterfiles --- app/controllers/media_objects_controller.rb | 4 ++-- spec/controllers/media_objects_controller_spec.rb | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/media_objects_controller.rb b/app/controllers/media_objects_controller.rb index 4eaf4ab88d..16ffab0d83 100644 --- a/app/controllers/media_objects_controller.rb +++ b/app/controllers/media_objects_controller.rb @@ -255,7 +255,7 @@ def update_media_object end if error_messages.empty? - if api_params[:replace_master_files] + if api_params[:replace_masterfiles] old_ordered_master_files.each do |mf| p = MasterFile.find(mf) @media_object.master_files.delete(p) @@ -607,6 +607,6 @@ def master_files_params end def api_params - params.permit(:collection_id, :publish, :import_bib_record, :replace_master_files) + params.permit(:collection_id, :publish, :import_bib_record, :replace_masterfiles) end end diff --git a/spec/controllers/media_objects_controller_spec.rb b/spec/controllers/media_objects_controller_spec.rb index 5151fa4d2a..e3d6efffe0 100644 --- a/spec/controllers/media_objects_controller_spec.rb +++ b/spec/controllers/media_objects_controller_spec.rb @@ -439,7 +439,7 @@ ActiveJob::Base.queue_adapter = :inline end it "should delete existing master_files and add a new master_file to a media_object" do - put 'json_update', format: 'json', id: media_object.id, files: [master_file], collection_id: media_object.collection_id, replace_master_files: true + put 'json_update', format: 'json', id: media_object.id, files: [master_file], collection_id: media_object.collection_id, replace_masterfiles: true expect(JSON.parse(response.body)['id'].class).to eq String expect(JSON.parse(response.body)).not_to include('errors') media_object.reload From e2fbec3833bfcb7b3341462ee68d5d1c4dee8197 Mon Sep 17 00:00:00 2001 From: Chris Want Date: Mon, 23 Jul 2018 15:37:50 -0600 Subject: [PATCH 03/21] Another spot that needs care, and can fail in the test suite. --- app/jobs/cleanup_working_file_job.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/jobs/cleanup_working_file_job.rb b/app/jobs/cleanup_working_file_job.rb index 62bfde9834..3bb7afbf8e 100644 --- a/app/jobs/cleanup_working_file_job.rb +++ b/app/jobs/cleanup_working_file_job.rb @@ -16,6 +16,6 @@ class CleanupWorkingFileJob < ActiveJob::Base def perform(masterfile_id) masterfile = MasterFile.find(masterfile_id) path = masterfile.working_file_path - File.delete(path) if File.exist?(path) + File.delete(path) if path.present? && File.exist?(path) end end From 4f1810125f428ba1a229fa02dd026411c2b1657f Mon Sep 17 00:00:00 2001 From: Chris Want Date: Wed, 25 Jul 2018 11:41:45 -0600 Subject: [PATCH 04/21] Reduce two Fedora fetches into one. --- .../batch_registration_finished_mailer.html.erb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/views/batch_registries_mailer/batch_registration_finished_mailer.html.erb b/app/views/batch_registries_mailer/batch_registration_finished_mailer.html.erb index a246eaed10..70a2f65f4c 100644 --- a/app/views/batch_registries_mailer/batch_registration_finished_mailer.html.erb +++ b/app/views/batch_registries_mailer/batch_registration_finished_mailer.html.erb @@ -47,8 +47,8 @@ Unless required by applicable law or agreed to in writing, software distributed <% @completed_items.each do |be_completed| %> - <% if MediaObject.exists? be_completed.media_object_pid %> - <% media_object = MediaObject.find(be_completed.media_object_pid) %> + <% media_object = MediaObject.where(id: be_completed.media_object_pid).first %> + <% if media_object %> <% link_url = media_object.permalink %> <% link_url = media_object_url(media_object) if link_url.blank? %> <%= be_completed.position + 2 %> From ac05c0b92cd94c4ad2e2db61a9d8facc1c48be24 Mon Sep 17 00:00:00 2001 From: Phil Dinh Date: Mon, 6 Aug 2018 11:47:22 -0400 Subject: [PATCH 05/21] Update Fedora environment for newer 4.7.x image --- docker-compose.yml | 2 ++ 1 file changed, 2 insertions(+) diff --git a/docker-compose.yml b/docker-compose.yml index 3daa1e06bc..ba778ffa3a 100644 --- a/docker-compose.yml +++ b/docker-compose.yml @@ -25,6 +25,8 @@ services: - db volumes: - fedora:/data + environment: + - JAVA_OPTIONS=${JAVA_OPTIONS} -Dfcrepo.postgresql.password=${FEDORA_DB_PASSWORD} -Dfcrepo.postgresql.username=fedora -Dfcrepo.postgresql.host=db -Dfcrepo.modeshape.configuration=classpath:/config/jdbc-postgresql/repository.json ports: - "8984:8080" solr: From 98666f6f486f123947a912d72ab4e8dc10e79c02 Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Tue, 7 Aug 2018 17:05:34 -0400 Subject: [PATCH 06/21] Default to using the test ActiveJob adapter to avoid running background jobs --- spec/controllers/master_files_controller_spec.rb | 7 ------- spec/controllers/media_objects_controller_spec.rb | 2 -- spec/models/collection_spec.rb | 1 - spec/models/master_file_spec.rb | 4 ---- spec/rails_helper.rb | 2 +- 5 files changed, 1 insertion(+), 15 deletions(-) diff --git a/spec/controllers/master_files_controller_spec.rb b/spec/controllers/master_files_controller_spec.rb index 52aacc6b6e..e69cbfe42e 100644 --- a/spec/controllers/master_files_controller_spec.rb +++ b/spec/controllers/master_files_controller_spec.rb @@ -15,13 +15,6 @@ require 'rails_helper' describe MasterFilesController do - before do - ActiveJob::Base.queue_adapter = :test - end - - after do - ActiveJob::Base.queue_adapter = :inline - end describe "#create" do let(:media_object) { FactoryGirl.create(:media_object) } diff --git a/spec/controllers/media_objects_controller_spec.rb b/spec/controllers/media_objects_controller_spec.rb index e3d6efffe0..fc3cef96ab 100644 --- a/spec/controllers/media_objects_controller_spec.rb +++ b/spec/controllers/media_objects_controller_spec.rb @@ -430,13 +430,11 @@ expect(media_object.master_files.to_a.size).to eq 2 end it "should update the poster and thumbnail for its masterfile" do - ActiveJob::Base.queue_adapter = :test media_object = FactoryGirl.create(:media_object) put 'json_update', format: 'json', id: media_object.id, files: [master_file], collection_id: media_object.collection_id media_object.reload expect(media_object.master_files.to_a.size).to eq 1 expect(ExtractStillJob).to have_been_enqueued.with(media_object.master_files.first.id,{type:'both',offset:2000}) - ActiveJob::Base.queue_adapter = :inline end it "should delete existing master_files and add a new master_file to a media_object" do put 'json_update', format: 'json', id: media_object.id, files: [master_file], collection_id: media_object.collection_id, replace_masterfiles: true diff --git a/spec/models/collection_spec.rb b/spec/models/collection_spec.rb index a38cc0c008..2af2efff6d 100644 --- a/spec/models/collection_spec.rb +++ b/spec/models/collection_spec.rb @@ -500,7 +500,6 @@ describe "reindex_members" do before do @collection = FactoryGirl.create(:collection, items: 3) - ActiveJob::Base.queue_adapter = :test end it 'should queue a reindex job for all member objects' do @collection.reindex_members {} diff --git a/spec/models/master_file_spec.rb b/spec/models/master_file_spec.rb index 6d6777ef1c..8421b27de2 100644 --- a/spec/models/master_file_spec.rb +++ b/spec/models/master_file_spec.rb @@ -115,7 +115,6 @@ let!(:master_file) { FactoryGirl.create(:master_file) } # let(:encode_job) { ActiveEncodeJob::Create.new(master_file.id, nil, {}) } before do - ActiveJob::Base.queue_adapter = :test # allow(ActiveEncodeJob::Create).to receive(:new).and_return(encode_job) # allow(encode_job).to receive(:perform) end @@ -194,11 +193,9 @@ describe "update images" do before do - ActiveJob::Base.queue_adapter = :test MasterFile.set_callback(:save, :after, :update_stills_from_offset!) end after do - ActiveJob::Base.queue_adapter = :inline MasterFile.skip_callback(:save, :after, :update_stills_from_offset!) end it "should update on save" do @@ -488,7 +485,6 @@ class MyEncoder < ActiveEncode::Base subject(:master_file) { FactoryGirl.create(:master_file) } let(:working_dir) {'/path/to/working_dir/'} before do - ActiveJob::Base.queue_adapter = :test Settings.matterhorn.media_path = working_dir end diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index ca4a30e78f..7c4289d673 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -53,7 +53,7 @@ # If you are not using ActiveRecord, you can remove this line. ActiveRecord::Migration.maintain_test_schema! -ActiveJob::Base.queue_adapter = :inline +ActiveJob::Base.queue_adapter = :test Shoulda::Matchers.configure do |config| config.integrate do |with| From b8e1cfd82f7c3b8924ed50326159f8478992c444 Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Tue, 7 Aug 2018 22:25:11 -0400 Subject: [PATCH 07/21] Run jobs that need to run for tests to continue to pass --- spec/controllers/bookmarks_controller_spec.rb | 7 +++++++ spec/controllers/collections_controller_spec.rb | 6 ++---- spec/controllers/media_objects_controller_spec.rb | 7 +++++++ 3 files changed, 16 insertions(+), 4 deletions(-) diff --git a/spec/controllers/bookmarks_controller_spec.rb b/spec/controllers/bookmarks_controller_spec.rb index 8f913dd6f8..ad6a8a9fe9 100644 --- a/spec/controllers/bookmarks_controller_spec.rb +++ b/spec/controllers/bookmarks_controller_spec.rb @@ -16,8 +16,15 @@ require 'avalon/intercom' describe BookmarksController, type: :controller do + include ActiveJob::TestHelper + render_views + around(:example) do |example| + # In Rails 5.1+ this can be restricted to whitelist jobs allowed to be performed + perform_enqueued_jobs { example.run } + end + let!(:collection) { FactoryGirl.create(:collection) } let!(:media_objects) { [] } diff --git a/spec/controllers/collections_controller_spec.rb b/spec/controllers/collections_controller_spec.rb index d5649c68f9..b01411602e 100644 --- a/spec/controllers/collections_controller_spec.rb +++ b/spec/controllers/collections_controller_spec.rb @@ -259,7 +259,7 @@ # allow(mock_email).to receive(:deliver_later) # expect(NotificationsMailer).to receive(:new_collection).and_return(mock_email) # FIXME: This delivers two instead of one for some reason - expect {post 'create', format:'json', admin_collection: {name: collection.name, description: collection.description, unit: collection.unit, managers: collection.managers}}.to change { ActionMailer::Base.deliveries.count } + expect {post 'create', format:'json', admin_collection: {name: collection.name, description: collection.description, unit: collection.unit, managers: collection.managers}}.to have_enqueued_job(ActionMailer::DeliveryJob).twice # post 'create', format:'json', admin_collection: {name: collection.name, description: collection.description, unit: collection.unit, managers: collection.managers} end it "should create a new collection" do @@ -285,9 +285,7 @@ # expect(mock_delay).to receive(:update_collection) @collection = FactoryGirl.create(:collection) # put 'update', id: @collection.id, admin_collection: {name: "#{@collection.name}-new", description: @collection.description, unit: @collection.unit} - # FIXME: This delivers two instead of one for some reason - expect {put 'update', id: @collection.id, admin_collection: {name: "#{@collection.name}-new", description: @collection.description, unit: @collection.unit}}.to change { ActionMailer::Base.deliveries.count } - + expect {put 'update', id: @collection.id, admin_collection: {name: "#{@collection.name}-new", description: @collection.description, unit: @collection.unit}}.to have_enqueued_job(ActionMailer::DeliveryJob).once end context "update REST API" do diff --git a/spec/controllers/media_objects_controller_spec.rb b/spec/controllers/media_objects_controller_spec.rb index fc3cef96ab..28cc2f8cf4 100644 --- a/spec/controllers/media_objects_controller_spec.rb +++ b/spec/controllers/media_objects_controller_spec.rb @@ -15,6 +15,8 @@ require 'rails_helper' describe MediaObjectsController, type: :controller do + include ActiveJob::TestHelper + render_views before(:each) do @@ -868,6 +870,11 @@ login_user collection.managers.first end + around(:example) do |example| + # In Rails 5.1+ this can be restricted to whitelist jobs allowed to be performed + perform_enqueued_jobs { example.run } + end + it "should remove a MediaObject with a single MasterFiles" do media_object = FactoryGirl.create(:media_object, :with_master_file, collection: collection) delete :destroy, id: media_object.id From 77d72bbd8e7a6d8b6ad8f2d35b087a977ee29c45 Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Sun, 12 Aug 2018 10:48:35 -0400 Subject: [PATCH 08/21] Avoid calling fedora to only check existance --- app/models/ability.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/ability.rb b/app/models/ability.rb index 1783618e50..ca49cab949 100644 --- a/app/models/ability.rb +++ b/app/models/ability.rb @@ -219,7 +219,7 @@ def is_editor_of?(collection) end def is_member_of_any_collection? - @user.id.present? && Admin::Collection.where("inheritable_edit_access_person_ssim" => @user.user_key).first.present? + @user.id.present? && Admin::Collection.exists?("inheritable_edit_access_person_ssim" => @user.user_key) end def full_login? From 9f49678305dc9feed450c3bcc9fa6a57ea9d53ae Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Thu, 16 Aug 2018 11:21:23 -0400 Subject: [PATCH 09/21] Add missing require --- app/jobs/bulk_action_jobs.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/jobs/bulk_action_jobs.rb b/app/jobs/bulk_action_jobs.rb index 36baa9fff6..70398c629d 100644 --- a/app/jobs/bulk_action_jobs.rb +++ b/app/jobs/bulk_action_jobs.rb @@ -162,6 +162,8 @@ def perform documents, params end end + require 'avalon/intercom' + class IntercomPush < ActiveJob::Base def perform documents, user_key, params errors = [] From 8052a18ab98c4b917f64eb121c176bae0b5a56e2 Mon Sep 17 00:00:00 2001 From: Chris Want Date: Wed, 22 Aug 2018 14:45:23 -0600 Subject: [PATCH 10/21] Batch ingest was interpreting the terms of use field as an array (then converting it to a string, e.g. a spreadsheet entry with "My license" was being converted to "\[\"My license\"\]"). This change handles the terms of use field the same way as the title field is treated. --- app/models/concerns/media_object_mods.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/models/concerns/media_object_mods.rb b/app/models/concerns/media_object_mods.rb index 35802b1c93..1d22860ca9 100644 --- a/app/models/concerns/media_object_mods.rb +++ b/app/models/concerns/media_object_mods.rb @@ -315,7 +315,7 @@ def terms_of_use end def terms_of_use=(value) delete_all_values(:terms_of_use) - descMetadata.add_terms_of_use(value) if value.present? + descMetadata.add_terms_of_use(Array(value).first) if value.present? end # has_attributes :table_of_contents, datastream: :descMetadata, at: [:table_of_contents], multiple: true From 06c71e24e73ecf0d95a72e8e12904e2258b234d5 Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Thu, 23 Aug 2018 09:03:33 -0400 Subject: [PATCH 11/21] MasterFile#working_file_path turned into unique path(s) persisted on the master file and passed to Matterhorn. The working file path is only used when Settings.matterhorn.media_path is configured. The code changes in prior commits did not take into account all of the ways ingest happens: web upload, web upload skip transcoding, web dropbox, web dropbox skip transcoding, batch ingest, batch ingest skip transcoding, and batch ingest pre-transcoded derivatives. The approach to this commit was to write tests for each of these scenarios with both the Settings.matterhorn.media_path configuration set and not set then make all of the tests pass. The underlying use case for the working file path is when Matterhorn is running on a different server than Avalon and they don't share the same filesystem (and filesystem paths) for both the Rails web upload directory and dropbox. The working_file_path is a copy of the originally uploaded file to where Matterhorn has access and at a path shared by both servers. For example, if Matterhorn is running on a different server and Matterhorn's ingest directory is '/srv/matterhorn/ingest' and mounted on the Avalon server at the same location and Avalon's dropbox directory is '/srv/avalon/dropbox' then Settings.matterhorn.media_path should be configured to be '/srv/matterhorn/ingest'. Note that Matterhorn's ingset directory needs to be mounted at the same location on both the Avalon and Matterhorn servers for Avalon to be able to ingest due to the use of absolute paths and not relative paths. Co-authored-by: phuongdh --- app/jobs/cleanup_working_file_job.rb | 7 +- app/models/avalon/rdf_vocab.rb | 7 +- app/models/master_file.rb | 58 +++- lib/avalon/batch/entry.rb | 9 + spec/integration/working_file_path_spec.rb | 298 +++++++++++++++++++++ spec/jobs/cleanup_working_files_spec.rb | 13 +- spec/models/master_file_spec.rb | 15 +- 7 files changed, 379 insertions(+), 28 deletions(-) create mode 100644 spec/integration/working_file_path_spec.rb diff --git a/app/jobs/cleanup_working_file_job.rb b/app/jobs/cleanup_working_file_job.rb index 3bb7afbf8e..9fe0c00aca 100644 --- a/app/jobs/cleanup_working_file_job.rb +++ b/app/jobs/cleanup_working_file_job.rb @@ -15,7 +15,10 @@ class CleanupWorkingFileJob < ActiveJob::Base def perform(masterfile_id) masterfile = MasterFile.find(masterfile_id) - path = masterfile.working_file_path - File.delete(path) if path.present? && File.exist?(path) + masterfile.working_file_path.map do |path| + File.delete(path) if File.exist?(path) + end + masterfile.working_file_path = nil + masterfile.save! end end diff --git a/app/models/avalon/rdf_vocab.rb b/app/models/avalon/rdf_vocab.rb index 7ac233be2e..c96a0eb648 100644 --- a/app/models/avalon/rdf_vocab.rb +++ b/app/models/avalon/rdf_vocab.rb @@ -1,11 +1,11 @@ # Copyright 2011-2018, The Trustees of Indiana University and Northwestern # University. Licensed under the Apache License, Version 2.0 (the "License"); # you may not use this file except in compliance with the License. -# +# # You may obtain a copy of the License at -# +# # http://www.apache.org/licenses/LICENSE-2.0 -# +# # Unless required by applicable law or agreed to in writing, software distributed # under the License is distributed on an "AS IS" BASIS, WITHOUT WARRANTIES OR # CONDITIONS OF ANY KIND, either express or implied. See the License for the @@ -35,6 +35,7 @@ class Transcoding < RDF::StrictVocabulary("http://avalonmediasystem.org/rdf/voca class MasterFile < RDF::StrictVocabulary("http://avalonmediasystem.org/rdf/vocab/master_file#") property :posterOffset, "rdfs:isDefinedBy" => %(avr-master_file:).freeze, type: "rdfs:Class".freeze property :thumbnailOffset, "rdfs:isDefinedBy" => %(avr-master_file:).freeze, type: "rdfs:Class".freeze + property :workingFilePath, "rdfs:isDefinedBy" => %(avr-master_file:).freeze, type: "rdfs:Class".freeze end class Derivative < RDF::StrictVocabulary("http://avalonmediasystem.org/rdf/vocab/derivative#") diff --git a/app/models/master_file.rb b/app/models/master_file.rb index e188711a29..0ed32bf3c4 100644 --- a/app/models/master_file.rb +++ b/app/models/master_file.rb @@ -120,6 +120,9 @@ class MasterFile < ActiveFedora::Base index.as :stored_sortable end + # For working file copy when Settings.matterhorn.media_path is set + property :working_file_path, predicate: Avalon::RDFVocab::MasterFile.workingFilePath, multiple: true + validates :workflow_name, presence: true, inclusion: { in: proc { WORKFLOWS } } validates_each :date_digitized do |record, attr, value| unless value.nil? @@ -167,8 +170,7 @@ def save_parent def setContent(file) case file when Hash #Multiple files for pre-transcoded derivatives - saveOriginal( (file.has_key?('quality-high') && File.file?( file['quality-high'] )) ? file['quality-high'] : (file.has_key?('quality-medium') && File.file?( file['quality-medium'] )) ? file['quality-medium'] : file.values[0] ) - file.each_value {|f| f.close unless f.closed? } + saveDerivativesHash(file) when ActionDispatch::Http::UploadedFile #Web upload saveOriginal(file, file.original_filename) when URI, Addressable::URI @@ -180,7 +182,7 @@ def setContent(file) self.file_size = FileLocator::S3File.new(file).object.size end else #Batch - saveOriginal(file) + saveOriginal(file, File.basename(file.path)) end reloadTechnicalMetadata! end @@ -249,14 +251,22 @@ def process file=nil #Build hash for single file skip transcoding if !file.is_a?(Hash) && (self.workflow_name == 'avalon-skip-transcoding' || self.workflow_name == 'avalon-skip-transcoding-audio') - file = {'quality-high' => FileLocator.new(file_location).attachment} + if working_file_path.present? + file = {'quality-high' => FileLocator.new(working_file_path.first).attachment} + else + file = {'quality-high' => FileLocator.new(file_location).attachment} + end end input = if file.is_a? Hash file_dup = file.dup file_dup.each_pair {|quality, f| file_dup[quality] = FileLocator.new(f.to_path).uri.to_s } else - FileLocator.new(file_location).uri.to_s + if working_file_path.present? + FileLocator.new(working_file_path.first).uri.to_s + else + FileLocator.new(file_location).uri.to_s + end end ActiveEncodeJob::Create.perform_later(self.id, input, {preset: self.workflow_name}) @@ -501,11 +511,23 @@ def to_solr *args end end - def working_file_path - path = nil + def create_working_file!(full_path) + working_path = MasterFile.calculate_working_file_path(full_path) + return unless working_path.present? + + self.working_file_path = [working_path] + FileUtils.mkdir(File.dirname(working_path)) + FileUtils.cp(full_path, working_path) + working_path + end + + def self.calculate_working_file_path(old_path) config_path = Settings.matterhorn.media_path - path = File.join(config_path, File.basename(self.file_location)) if config_path.present? && File.directory?(config_path) - path + if config_path.present? && File.directory?(config_path) + File.join(config_path, SecureRandom.uuid, File.basename(old_path)) + else + nil + end end protected @@ -649,9 +671,7 @@ def saveOriginal(file, original_name=nil) realpath = path end - self.file_location = realpath - newpath = working_file_path - FileUtils.cp(realpath, newpath) unless newpath.blank? + create_working_file!(realpath) end self.file_location = realpath self.file_size = file.size.to_s @@ -659,6 +679,20 @@ def saveOriginal(file, original_name=nil) file.close end + def saveDerivativesHash(derivative_hash) + usable_files = derivative_hash.select { |quality, file| File.file?(file) } + self.working_file_path = usable_files.values.collect { |file| create_working_file!(File.realpath(file)) }.compact + + %w(quality-high quality-medium quality-low).each do |quality| + next unless usable_files.has_key?(quality) + self.file_location = File.realpath(usable_files[quality]) + self.file_size = usable_files[quality].size.to_s + break + end + ensure + derivative_hash.values.map { |file| file.close } + end + def reloadTechnicalMetadata! #Reset mediainfo @mediainfo = nil diff --git a/lib/avalon/batch/entry.rb b/lib/avalon/batch/entry.rb index 5fddd30802..04f52167c7 100644 --- a/lib/avalon/batch/entry.rb +++ b/lib/avalon/batch/entry.rb @@ -201,6 +201,15 @@ def process! files = self.class.gatherFiles(file_spec[:file]) self.class.attach_datastreams_to_master_file(master_file, file_spec[:file]) master_file.setContent(files) + + # Overwrite files hash with working file paths to pass to matterhorn + if files.is_a?(Hash) && master_file.working_file_path.present? + files.each do |quality, file| + working_path = master_file.working_file_path.find { |path| File.basename(file) == File.basename(path) } + files[quality] = File.new(working_path) + end + end + master_file.absolute_location = file_spec[:absolute_location] if file_spec[:absolute_location].present? master_file.title = file_spec[:label] if file_spec[:label].present? master_file.poster_offset = file_spec[:offset] if file_spec[:offset].present? diff --git a/spec/integration/working_file_path_spec.rb b/spec/integration/working_file_path_spec.rb new file mode 100644 index 0000000000..3fc9781fcc --- /dev/null +++ b/spec/integration/working_file_path_spec.rb @@ -0,0 +1,298 @@ +require 'rails_helper' + +# MasterFile#working_file_path has been a source of problems since it was introduced in 6.4.3 +# This spec file is meant to thoroughly test it in order to find any remaining bugs and to +# ensure the intended functionality doesn't get broken accidentally in the future. +# +# Need to test all of the possiblilites: +# batch ingest with single file +# batch ingest with single file skip transcoding +# batch ingest with pre-transcoded derivatives +# web upload +# web upload skip transcoding +# BE dropbox upload +# BE dropbox skip transcoding +# Repeat all of these with and without media path set. +# Also with dropbox mounted on matterhorn and not.? +# +# Pre-existing tests that are related (or duplicative) +# spec/models/master_file_spec.rb:262 +# spec/models/master_file_spec.rb:500 +# spec/lib/avalon/batch/entry_spec.rb:80 +# +describe "MasterFile#working_file_path" do + let(:master_file) { FactoryGirl.build(:master_file) } + let(:media_object) { FactoryGirl.create(:media_object) } + let(:workflow) { 'avalon' } + + context "with Settings.matterhorn.media_path set" do + let(:media_path) { Dir.mktmpdir } + + around(:example) do |example| + begin + old_media_path = Settings.matterhorn.media_path + Settings.matterhorn.media_path = media_path + + example.run + + Settings.matterhorn.media_path = old_media_path + ensure + FileUtils.remove_entry media_path + end + end + + describe '#calculate_working_file_path' do + let(:path1) { MasterFile.calculate_working_file_path('/dropbox/coll1/video.mp4') } + let(:path2) { MasterFile.calculate_working_file_path('/dropbox/coll2/video.mp4') } + + it 'returns a working file path' do + expect(File.fnmatch("#{File.absolute_path(media_path)}/*/video.mp4", path1)).to be true + end + + it 'creates a unique working_file_path for each file' do + expect(path1).not_to eq path2 + end + end + + it 'can recompute the working_file_path' do + file = fixture_file_upload('spec/fixtures/videoshort.mp4', 'video/mp4') + master_file.setContent(file) + original_path = master_file.working_file_path + master_file.save! + new_path = MasterFile.find(master_file.id).working_file_path + expect(original_path).to eq new_path + end + + context "using web upload" do + let(:file) { fixture_file_upload('spec/fixtures/videoshort.mp4', 'video/mp4') } + let(:params) { { Filedata: [file], original: nil, workflow: workflow } } + + it 'sends the working_file_path to matterhorn' do + MasterFileBuilder.build(media_object, params) + master_file = media_object.reload.master_files.first + expect(File.exists? master_file.working_file_path.first).to be true + input = FileLocator.new(master_file.working_file_path.first).uri.to_s + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: workflow}) + end + + context "with skip transcoding" do + let(:workflow) { 'skip_transcoding' } + + it 'sends the working_file_path to matterhorn' do + MasterFileBuilder.build(media_object, params) + master_file = media_object.reload.master_files.first + expect(File.exists? master_file.working_file_path.first).to be true + input = { "quality-high" => FileLocator.new(master_file.working_file_path.first).uri.to_s } + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: 'avalon-skip-transcoding'}) + end + end + end + + context "using dropbox upload" do + let(:file) { fixture_file_upload('spec/fixtures/videoshort.mp4', 'video/mp4') } + let(:url) { Addressable::URI.convert_path(File.absolute_path(file.to_path)) } + let(:params) { { selected_files: { "0" => { url: url, file_name: 'videoshort.mp4' } }, workflow: workflow } } + + it 'sends the working_file_path to matterhorn' do + MasterFileBuilder.build(media_object, params) + master_file = media_object.reload.master_files.first + expect(File.exists? master_file.working_file_path.first).to be true + input = FileLocator.new(master_file.working_file_path.first).uri.to_s + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: workflow}) + end + + context "with skip transcoding" do + let(:workflow) { 'skip_transcoding' } + + it 'sends the working_file_path to matterhorn' do + MasterFileBuilder.build(media_object, params) + master_file = media_object.reload.master_files.first + expect(File.exists? master_file.working_file_path.first).to be true + input = { "quality-high" => FileLocator.new(master_file.working_file_path.first).uri.to_s } + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: 'avalon-skip-transcoding'}) + end + end + end + + context "using batch ingest" do + let(:file) { fixture_file_upload('spec/fixtures/videoshort.mp4', 'video/mp4') } + let(:collection) { FactoryGirl.build(:collection) } + let(:entry_fields) { { title: Faker::Lorem.sentence, date_issued: "#{DateTime.now.strftime('%F')}" } } + let(:entry_files) { [{ file: File.absolute_path(file), skip_transcoding: false }] } + let(:entry_opts) { {user_key: 'archivist1@example.org', collection: collection} } + let(:entry) { Avalon::Batch::Entry.new(entry_fields, entry_files, entry_opts, nil, nil) } + + before do + allow(entry).to receive(:media_object).and_return(media_object) + end + + it 'sends the working_file_path to matterhorn' do + entry.process! + master_file = media_object.reload.master_files.first + expect(File.exists? master_file.working_file_path.first).to be true + input = FileLocator.new(master_file.working_file_path.first).uri.to_s + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: workflow}) + end + + context 'with skip transcoding' do + let(:entry_files) { [{ file: File.absolute_path(file), skip_transcoding: true }] } + + it 'sends the working_file_path to matterhorn' do + entry.process! + master_file = media_object.reload.master_files.first + expect(File.exists? master_file.working_file_path.first).to be true + input = { "quality-high" => FileLocator.new(master_file.working_file_path.first).uri.to_s } + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: 'avalon-skip-transcoding'}) + end + end + + context 'with pre-transcoded derivatives' do + let(:filename) {File.join(Rails.root, "spec/fixtures/videoshort.mp4")} + %w(low medium high).each do |quality| + let("filename_#{quality}".to_sym) {File.join(Rails.root, "spec/fixtures/videoshort.#{quality}.mp4")} + end + let(:derivative_paths) {[filename_low, filename_medium, filename_high]} + let(:derivative_hash) {{'quality-low' => File.new(filename_low), 'quality-medium' => File.new(filename_medium), 'quality-high' => File.new(filename_high)}} + + let(:entry_files) { [{ file: filename, skip_transcoding: true }] } + + let(:original_file_locator) { instance_double("FileLocator") } + before do + allow(FileLocator).to receive(:new).and_call_original + allow(FileLocator).to receive(:new).with(filename).and_return(original_file_locator) + allow(original_file_locator).to receive(:exist?).and_return(false) + end + + # All derivatives are copied to a working path + # TODO: Ensure all working file copies are cleaned up by the background job + it 'sends the working_file_path to matterhorn' do + entry.process! + master_file = media_object.reload.master_files.first + working_file_path_high = master_file.working_file_path.find { |file| file.include? "high" } + working_file_path_medium = master_file.working_file_path.find { |file| file.include? "medium" } + working_file_path_low = master_file.working_file_path.find { |file| file.include? "low" } + + [working_file_path_high, working_file_path_medium, working_file_path_low].each do |file| + expect(File.exists? file).to be true + end + input = { "quality-high" => FileLocator.new(working_file_path_high).uri.to_s, + "quality-medium" => FileLocator.new(working_file_path_medium).uri.to_s, + "quality-low" => FileLocator.new(working_file_path_low).uri.to_s } + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: 'avalon-skip-transcoding'}) + end + end + end + end + + context "without Settings.matterhorn.media_path set" do + it 'returns blank' do + expect(master_file.working_file_path).to be_blank + end + + context "using web upload" do + let(:file) { fixture_file_upload('spec/fixtures/videoshort.mp4', 'video/mp4') } + let(:params) { { Filedata: [file], original: nil, workflow: workflow } } + + it 'sends the file_location to matterhorn' do + MasterFileBuilder.build(media_object, params) + master_file = media_object.reload.master_files.first + input = FileLocator.new(master_file.file_location).uri.to_s + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: workflow}) + end + + context "with skip transcoding" do + let(:workflow) { 'skip_transcoding' } + + it 'sends the file_location to matterhorn' do + MasterFileBuilder.build(media_object, params) + master_file = media_object.reload.master_files.first + input = { "quality-high" => FileLocator.new(master_file.file_location).uri.to_s } + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: 'avalon-skip-transcoding'}) + end + end + end + + context "using dropbox upload" do + let(:file) { fixture_file_upload('spec/fixtures/videoshort.mp4', 'video/mp4') } + let(:url) { Addressable::URI.convert_path(File.absolute_path(file.to_path)) } + let(:params) { { selected_files: { "0" => { url: url, file_name: 'videoshort.mp4' } }, workflow: workflow } } + + it 'sends the file_location to matterhorn' do + MasterFileBuilder.build(media_object, params) + master_file = media_object.reload.master_files.first + input = FileLocator.new(master_file.file_location).uri.to_s + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: workflow}) + end + + context "with skip transcoding" do + let(:workflow) { 'skip_transcoding' } + + it 'sends the file_location to matterhorn' do + MasterFileBuilder.build(media_object, params) + master_file = media_object.reload.master_files.first + input = { "quality-high" => FileLocator.new(master_file.file_location).uri.to_s } + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: 'avalon-skip-transcoding'}) + end + end + end + + context "using batch ingest" do + let(:file) { fixture_file_upload('spec/fixtures/videoshort.mp4', 'video/mp4') } + let(:collection) { FactoryGirl.build(:collection) } + let(:entry_fields) { { title: Faker::Lorem.sentence, date_issued: "#{DateTime.now.strftime('%F')}" } } + let(:entry_files) { [{ file: File.absolute_path(file), skip_transcoding: false }] } + let(:entry_opts) { {user_key: 'archivist1@example.org', collection: collection} } + let(:entry) { Avalon::Batch::Entry.new(entry_fields, entry_files, entry_opts, nil, nil) } + + before do + allow(entry).to receive(:media_object).and_return(media_object) + end + + it 'sends the file_location to matterhorn' do + entry.process! + master_file = media_object.reload.master_files.first + input = FileLocator.new(master_file.file_location).uri.to_s + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: workflow}) + end + + context 'with skip transcoding' do + let(:entry_files) { [{ file: File.absolute_path(file), skip_transcoding: true }] } + + it 'sends the file_location to matterhorn' do + entry.process! + master_file = media_object.reload.master_files.first + input = { "quality-high" => FileLocator.new(master_file.file_location).uri.to_s } + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: 'avalon-skip-transcoding'}) + end + end + + context 'with pre-transcoded derivatives' do + let(:filename) {File.join(Rails.root, "spec/fixtures/videoshort.mp4")} + %w(low medium high).each do |quality| + let("filename_#{quality}".to_sym) {File.join(Rails.root, "spec/fixtures/videoshort.#{quality}.mp4")} + end + let(:derivative_paths) {[filename_low, filename_medium, filename_high]} + let(:derivative_hash) {{'quality-low' => File.new(filename_low), 'quality-medium' => File.new(filename_medium), 'quality-high' => File.new(filename_high)}} + + let(:entry_files) { [{ file: filename, skip_transcoding: true }] } + + let(:original_file_locator) { instance_double("FileLocator") } + before do + allow(FileLocator).to receive(:new).and_call_original + allow(FileLocator).to receive(:new).with(filename).and_return(original_file_locator) + allow(original_file_locator).to receive(:exist?).and_return(false) + end + + it 'sends the derivative locations to matterhorn' do + entry.process! + master_file = media_object.reload.master_files.first + input = {'quality-low' => Addressable::URI.convert_path(File.absolute_path(filename_low)).to_s, + 'quality-medium' => Addressable::URI.convert_path(File.absolute_path(filename_medium)).to_s, + 'quality-high' => Addressable::URI.convert_path(File.absolute_path(filename_high)).to_s + } + expect(ActiveEncodeJob::Create).to have_been_enqueued.with(master_file.id, input, {preset: 'avalon-skip-transcoding'}) + end + end + end + end +end diff --git a/spec/jobs/cleanup_working_files_spec.rb b/spec/jobs/cleanup_working_files_spec.rb index b032570263..dd82f5f118 100644 --- a/spec/jobs/cleanup_working_files_spec.rb +++ b/spec/jobs/cleanup_working_files_spec.rb @@ -15,13 +15,16 @@ require 'rails_helper' describe CleanupWorkingFileJob do - let(:working_file) {'/temp/working_file.mp4'} - let(:master_file) {instance_double('MasterFile')} + let(:working_file) { '/temp/working_file.mp4' } + let(:master_file) { FactoryGirl.build(:master_file, working_file_path: [working_file]) } + + before do + allow(MasterFile).to receive(:find).and_return(master_file) + allow(File).to receive(:exist?).and_return(true) + end + describe "perform" do it 'calls file delete when there is file to cleanup' do - allow(MasterFile).to receive(:find).and_return(master_file) - allow(master_file).to receive(:working_file_path).and_return(working_file) - allow(File).to receive(:exist?).and_return(true) expect(File).to receive(:delete).with(working_file).once CleanupWorkingFileJob.perform_now('abc123') end diff --git a/spec/models/master_file_spec.rb b/spec/models/master_file_spec.rb index 8421b27de2..2ce94640e7 100644 --- a/spec/models/master_file_spec.rb +++ b/spec/models/master_file_spec.rb @@ -317,7 +317,7 @@ it "should copy an uploaded file to the media path" do Settings.matterhorn.media_path = media_path - expect(subject.working_file_path).to eq(File.join(media_path,original)) + expect(File.fnmatch("#{media_path}/*/#{original}", subject.working_file_path.first)).to be true end end end @@ -483,7 +483,7 @@ class MyEncoder < ActiveEncode::Base context 'with a working directory' do subject(:master_file) { FactoryGirl.create(:master_file) } - let(:working_dir) {'/path/to/working_dir/'} + let(:working_dir) { Dir.mktmpdir } before do Settings.matterhorn.media_path = working_dir end @@ -498,13 +498,16 @@ class MyEncoder < ActiveEncode::Base end end describe '#working_file_path' do - it 'returns nil when the working directory is invalid' do - expect(master_file.working_file_path).to be_nil + it 'returns blank when the working directory is invalid' do + expect(master_file.working_file_path).to be_blank end it 'returns a path when the working directory is valid' do - allow(File).to receive(:directory?).and_return(true) - expect(master_file.working_file_path).to include(working_dir) + # allow(File).to receive(:directory?).and_call_original + # allow(File).to receive(:directory?).with(Settings.matterhorn.media_path).and_return(true) + file = File.new(Rails.root.join('spec', 'fixtures', 'videoshort.mp4')) + master_file.setContent(file) + expect(master_file.working_file_path.first).to include(Settings.matterhorn.media_path) end end end From d969257507472e13066e793d34125297b537273b Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Mon, 27 Aug 2018 11:07:56 -0400 Subject: [PATCH 12/21] Clarify test comments --- spec/integration/working_file_path_spec.rb | 5 ++--- spec/models/master_file_spec.rb | 2 -- 2 files changed, 2 insertions(+), 5 deletions(-) diff --git a/spec/integration/working_file_path_spec.rb b/spec/integration/working_file_path_spec.rb index 3fc9781fcc..5afff8033c 100644 --- a/spec/integration/working_file_path_spec.rb +++ b/spec/integration/working_file_path_spec.rb @@ -10,10 +10,9 @@ # batch ingest with pre-transcoded derivatives # web upload # web upload skip transcoding -# BE dropbox upload -# BE dropbox skip transcoding +# web dropbox upload +# web dropbox skip transcoding # Repeat all of these with and without media path set. -# Also with dropbox mounted on matterhorn and not.? # # Pre-existing tests that are related (or duplicative) # spec/models/master_file_spec.rb:262 diff --git a/spec/models/master_file_spec.rb b/spec/models/master_file_spec.rb index 2ce94640e7..37eb39472b 100644 --- a/spec/models/master_file_spec.rb +++ b/spec/models/master_file_spec.rb @@ -503,8 +503,6 @@ class MyEncoder < ActiveEncode::Base end it 'returns a path when the working directory is valid' do - # allow(File).to receive(:directory?).and_call_original - # allow(File).to receive(:directory?).with(Settings.matterhorn.media_path).and_return(true) file = File.new(Rails.root.join('spec', 'fixtures', 'videoshort.mp4')) master_file.setContent(file) expect(master_file.working_file_path.first).to include(Settings.matterhorn.media_path) From 0d00540f4b3c904cc5cb337d91a02cb95f12b8f8 Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Mon, 27 Aug 2018 11:27:10 -0400 Subject: [PATCH 13/21] Ensure that working file's parent directory is deleted. --- app/jobs/cleanup_working_file_job.rb | 2 ++ spec/jobs/cleanup_working_files_spec.rb | 2 ++ 2 files changed, 4 insertions(+) diff --git a/app/jobs/cleanup_working_file_job.rb b/app/jobs/cleanup_working_file_job.rb index 9fe0c00aca..501e64372d 100644 --- a/app/jobs/cleanup_working_file_job.rb +++ b/app/jobs/cleanup_working_file_job.rb @@ -16,7 +16,9 @@ class CleanupWorkingFileJob < ActiveJob::Base def perform(masterfile_id) masterfile = MasterFile.find(masterfile_id) masterfile.working_file_path.map do |path| + parent_directory = File.dirname(path) File.delete(path) if File.exist?(path) + Dir.delete(parent_directory) if Dir.exist?(parent_directory) end masterfile.working_file_path = nil masterfile.save! diff --git a/spec/jobs/cleanup_working_files_spec.rb b/spec/jobs/cleanup_working_files_spec.rb index dd82f5f118..ef60de1d3d 100644 --- a/spec/jobs/cleanup_working_files_spec.rb +++ b/spec/jobs/cleanup_working_files_spec.rb @@ -21,11 +21,13 @@ before do allow(MasterFile).to receive(:find).and_return(master_file) allow(File).to receive(:exist?).and_return(true) + allow(Dir).to receive(:exist?).and_return(true) end describe "perform" do it 'calls file delete when there is file to cleanup' do expect(File).to receive(:delete).with(working_file).once + expect(Dir).to receive(:delete).with(File.dirname(working_file)).once CleanupWorkingFileJob.perform_now('abc123') end end From c290665efb7dbb74d79ecc65a25e415114fc3aa8 Mon Sep 17 00:00:00 2001 From: Brian Keese Date: Wed, 29 Aug 2018 11:33:30 -0400 Subject: [PATCH 14/21] API update should force unpublish if passed publish=false --- app/controllers/media_objects_controller.rb | 10 +++++++--- lib/avalon/intercom.rb | 2 +- spec/controllers/media_objects_controller_spec.rb | 2 +- spec/lib/avalon/intercom_spec.rb | 2 +- 4 files changed, 10 insertions(+), 6 deletions(-) diff --git a/app/controllers/media_objects_controller.rb b/app/controllers/media_objects_controller.rb index 16ffab0d83..c2162439f3 100644 --- a/app/controllers/media_objects_controller.rb +++ b/app/controllers/media_objects_controller.rb @@ -272,9 +272,13 @@ def update_media_object if !@media_object.save error_messages += ['Failed to create media object:']+@media_object.errors.full_messages @media_object.destroy - elsif !!api_params[:publish] - @media_object.publish!('REST API') - @media_object.workflow.publish + else + if !!api_params[:publish] + @media_object.publish!('REST API') + @media_object.workflow.publish + else + @media_object.publish!('') + end end end end diff --git a/lib/avalon/intercom.rb b/lib/avalon/intercom.rb index 32a1e7a3b6..a76dc3a00e 100644 --- a/lib/avalon/intercom.rb +++ b/lib/avalon/intercom.rb @@ -33,7 +33,7 @@ def collection_valid?(collection) def push_media_object(media_object, collection_id, include_structure = true) return { message: 'Avalon intercom target is not configured.' } unless @avalon.present? - return { message: 'You are not autorized to push to this collection.' } unless collection_valid? collection_id + return { message: 'You are not authorized to push to this collection.' } unless collection_valid? collection_id begin resp = RestClient::Request.execute( method: :post, diff --git a/spec/controllers/media_objects_controller_spec.rb b/spec/controllers/media_objects_controller_spec.rb index 28cc2f8cf4..f0344564af 100644 --- a/spec/controllers/media_objects_controller_spec.rb +++ b/spec/controllers/media_objects_controller_spec.rb @@ -132,7 +132,7 @@ let(:target_link) { { link: 'http://link.to/media_object' } } let(:error_status) { { message: 'Not authorized', status: 401 } } let(:media_object_permission) { 'You do not have permission to push this media object.' } - let(:collection_permission) { 'You are not autorized to push to this collection.' } + let(:collection_permission) { 'You are not authorized to push to this collection.' } before do login_as(:administrator) session[:intercom_collections] = {} diff --git a/spec/lib/avalon/intercom_spec.rb b/spec/lib/avalon/intercom_spec.rb index 9158d47511..f7c2f066b4 100644 --- a/spec/lib/avalon/intercom_spec.rb +++ b/spec/lib/avalon/intercom_spec.rb @@ -72,7 +72,7 @@ it "should respond to unpermitted collection with error" do response = intercom.push_media_object(media_object, 'invalid_collection', false) - expect(response[:message]).to eq('You are not autorized to push to this collection.') + expect(response[:message]).to eq('You are not authorized to push to this collection.') end it "should respond to unconfigured intercom with error" do Settings.intercom = {} From 9a773eb60416f9fea16aaeaa0541019a2c73e33a Mon Sep 17 00:00:00 2001 From: Phuong Dinh Date: Mon, 20 Aug 2018 12:00:21 -0400 Subject: [PATCH 15/21] Add optional support for Google Analytics --- app/views/layouts/avalon.html.erb | 4 ++++ app/views/layouts/embed.html.erb | 4 ++++ app/views/modules/_google_analytics.html.erb | 9 +++++++++ config/settings.yml | 1 + 4 files changed, 18 insertions(+) create mode 100644 app/views/modules/_google_analytics.html.erb diff --git a/app/views/layouts/avalon.html.erb b/app/views/layouts/avalon.html.erb index 8622e7edd2..31688e973f 100644 --- a/app/views/layouts/avalon.html.erb +++ b/app/views/layouts/avalon.html.erb @@ -26,6 +26,10 @@ Unless required by applicable law or agreed to in writing, software distributed <%= stylesheet_link_tag "application", media: "all" %> <%= yield :page_styles %> <%= yield :additional_head_content %> +<<<<<<< HEAD +======= + <%= render "modules/google_analytics" %> +>>>>>>> c2f1f12a... DRY up analytics code diff --git a/app/views/layouts/embed.html.erb b/app/views/layouts/embed.html.erb index 23094d5a65..7eade1eb41 100644 --- a/app/views/layouts/embed.html.erb +++ b/app/views/layouts/embed.html.erb @@ -30,6 +30,10 @@ Unless required by applicable law or agreed to in writing, software distributed <%= stylesheet_link_tag "application", media: "all" %> <%= yield :page_styles %> <%= yield :additional_head_content %> +<<<<<<< HEAD +======= + <%= render "modules/google_analytics" %> +>>>>>>> c2f1f12a... DRY up analytics code diff --git a/app/views/modules/_google_analytics.html.erb b/app/views/modules/_google_analytics.html.erb new file mode 100644 index 0000000000..2c7b714b4c --- /dev/null +++ b/app/views/modules/_google_analytics.html.erb @@ -0,0 +1,9 @@ +<% if Settings.google_analytics_tracking_id.present? %> + + +<% end %> diff --git a/config/settings.yml b/config/settings.yml index 91ba8c0ee3..40a51ab933 100644 --- a/config/settings.yml +++ b/config/settings.yml @@ -82,3 +82,4 @@ auth: :oauth_credentials: <%= ENV['LTI_AUTH_KEY'] %>: <%= ENV['LTI_AUTH_SECRET'] %> <% end %> +# google_analytics_tracking_id: "someid" From 22ac1894a95fd6fbe7f0c7888bb02e0a9dd02e40 Mon Sep 17 00:00:00 2001 From: Phuong Dinh Date: Wed, 22 Aug 2018 13:33:47 -0400 Subject: [PATCH 16/21] Add view and controller tests for analytics --- app/views/layouts/avalon.html.erb | 3 --- app/views/layouts/embed.html.erb | 3 --- .../application_controller_spec.rb | 18 +++++++++++++ .../master_files_controller_spec.rb | 6 +++++ spec/views/google_analytics_spec.rb | 25 +++++++++++++++++++ 5 files changed, 49 insertions(+), 6 deletions(-) create mode 100644 spec/views/google_analytics_spec.rb diff --git a/app/views/layouts/avalon.html.erb b/app/views/layouts/avalon.html.erb index 31688e973f..f61aa0c461 100644 --- a/app/views/layouts/avalon.html.erb +++ b/app/views/layouts/avalon.html.erb @@ -26,10 +26,7 @@ Unless required by applicable law or agreed to in writing, software distributed <%= stylesheet_link_tag "application", media: "all" %> <%= yield :page_styles %> <%= yield :additional_head_content %> -<<<<<<< HEAD -======= <%= render "modules/google_analytics" %> ->>>>>>> c2f1f12a... DRY up analytics code diff --git a/app/views/layouts/embed.html.erb b/app/views/layouts/embed.html.erb index 7eade1eb41..f1fad73047 100644 --- a/app/views/layouts/embed.html.erb +++ b/app/views/layouts/embed.html.erb @@ -30,10 +30,7 @@ Unless required by applicable law or agreed to in writing, software distributed <%= stylesheet_link_tag "application", media: "all" %> <%= yield :page_styles %> <%= yield :additional_head_content %> -<<<<<<< HEAD -======= <%= render "modules/google_analytics" %> ->>>>>>> c2f1f12a... DRY up analytics code diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index d71bac3c5a..61e4ee5925 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -16,6 +16,10 @@ describe ApplicationController do controller do + def index + + end + def create render nothing: true end @@ -89,4 +93,18 @@ def show expect(response).not_to have_http_status(304) end end + + describe "layout" do + render_views + + it 'renders avalon layout' do + get :show, params: { id: 'abc1234' } + expect(response).to render_template("layouts/avalon") + end + + it 'renders google analytics partial' do + get :show, params: { id: 'abc1234' } + expect(response).to render_template("modules/_google_analytics") + end + end end diff --git a/spec/controllers/master_files_controller_spec.rb b/spec/controllers/master_files_controller_spec.rb index e69cbfe42e..de19ef8e30 100644 --- a/spec/controllers/master_files_controller_spec.rb +++ b/spec/controllers/master_files_controller_spec.rb @@ -233,6 +233,8 @@ class << file ordered_master_files: [master_file], master_file_ids: [master_file.id]) end + render_views + before do allow_any_instance_of(MasterFile).to receive(:media_object).and_return(media_object) allow_any_instance_of(MasterFile).to receive(:media_object_id).and_return(media_object.id) @@ -243,6 +245,10 @@ class << file expect(get(:embed, id: master_file.id)).to render_template(layout: 'embed') end + it "renders the Google Analytics partial" do + expect(get(:embed, params: { id: master_file.id })).to render_template('modules/_google_analytics') + end + context 'with fedora 3 pid' do let!(:master_file) { FactoryGirl.create(:master_file, identifier: [fedora3_pid]) } let(:fedora3_pid) { 'avalon:1234' } diff --git a/spec/views/google_analytics_spec.rb b/spec/views/google_analytics_spec.rb new file mode 100644 index 0000000000..025f3808e8 --- /dev/null +++ b/spec/views/google_analytics_spec.rb @@ -0,0 +1,25 @@ +require 'rails_helper' + +describe "modules/_google_analytics.html.erb", type: :view do + context "Google Analytics is configured" do + before do + Settings.google_analytics_tracking_id = "arandomid" + end + + it 'includes GA code' do + render + expect(rendered).to have_selector(:css, "script[src='https://www.googletagmanager.com/gtag/js?id=arandomid']", visible: false) + end + end + + context "Google Analytics is not configured" do + before do + Settings.google_analytics_tracking_id = nil + end + + it 'does not include GA code' do + render + expect(rendered).to have_none_of_selectors(:css, "script[src='https://www.googletagmanager.com/gtag/js?id=arandomid']", visible: false) + end + end +end From 8d9bebb93cc9575ee7115b6e92822b4f942c77de Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Wed, 29 Aug 2018 13:50:44 -0400 Subject: [PATCH 17/21] Fix BatchRegistries#batch_entries --- app/jobs/ingest_batch_status_email_jobs.rb | 2 +- app/models/batch_registries.rb | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) diff --git a/app/jobs/ingest_batch_status_email_jobs.rb b/app/jobs/ingest_batch_status_email_jobs.rb index cef72c1563..6cc9371640 100644 --- a/app/jobs/ingest_batch_status_email_jobs.rb +++ b/app/jobs/ingest_batch_status_email_jobs.rb @@ -24,7 +24,7 @@ def perform # Get the entries for the batch and see if they all complete complete = true errors = false - BatchEntries.where(batch_registries_id: br.id).each do |entry| + br.batch_entries.each do |entry| complete = false unless entry.complete || entry.error errors = true if entry.error end diff --git a/app/models/batch_registries.rb b/app/models/batch_registries.rb index b630a10f08..4e1d474363 100644 --- a/app/models/batch_registries.rb +++ b/app/models/batch_registries.rb @@ -17,7 +17,7 @@ class BatchRegistries < ActiveRecord::Base before_save :check_user validates :file_name, :user_id, :collection, presence: true - has_many :batch_entries, -> { order(position: :asc) } + has_many :batch_entries, -> { order(position: :asc) }, class_name: BatchEntries # For FactoryGirl's taps, document more TODO def file_name=(fn) From a61dbfc7e2959b77fc1fb1ab4b4683eeb680d55e Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Thu, 30 Aug 2018 13:40:43 -0400 Subject: [PATCH 18/21] Fix tests for Rails 4.2 --- spec/controllers/application_controller_spec.rb | 4 ++-- spec/controllers/master_files_controller_spec.rb | 2 +- spec/views/google_analytics_spec.rb | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/spec/controllers/application_controller_spec.rb b/spec/controllers/application_controller_spec.rb index 61e4ee5925..020a66fa82 100644 --- a/spec/controllers/application_controller_spec.rb +++ b/spec/controllers/application_controller_spec.rb @@ -98,12 +98,12 @@ def show render_views it 'renders avalon layout' do - get :show, params: { id: 'abc1234' } + get :show, id: 'abc1234' expect(response).to render_template("layouts/avalon") end it 'renders google analytics partial' do - get :show, params: { id: 'abc1234' } + get :show, id: 'abc1234' expect(response).to render_template("modules/_google_analytics") end end diff --git a/spec/controllers/master_files_controller_spec.rb b/spec/controllers/master_files_controller_spec.rb index de19ef8e30..bdd48c275f 100644 --- a/spec/controllers/master_files_controller_spec.rb +++ b/spec/controllers/master_files_controller_spec.rb @@ -246,7 +246,7 @@ class << file end it "renders the Google Analytics partial" do - expect(get(:embed, params: { id: master_file.id })).to render_template('modules/_google_analytics') + expect(get(:embed, id: master_file.id )).to render_template('modules/_google_analytics') end context 'with fedora 3 pid' do diff --git a/spec/views/google_analytics_spec.rb b/spec/views/google_analytics_spec.rb index 025f3808e8..84523e50e9 100644 --- a/spec/views/google_analytics_spec.rb +++ b/spec/views/google_analytics_spec.rb @@ -19,7 +19,7 @@ it 'does not include GA code' do render - expect(rendered).to have_none_of_selectors(:css, "script[src='https://www.googletagmanager.com/gtag/js?id=arandomid']", visible: false) + expect(rendered).not_to have_selector(:css, "script[src='https://www.googletagmanager.com/gtag/js?id=arandomid']", visible: false) end end end From 699b7cdd94d8d2779ec4537d66693d58e8d383bd Mon Sep 17 00:00:00 2001 From: Brian Keese Date: Tue, 4 Sep 2018 15:21:12 -0400 Subject: [PATCH 19/21] Add configurable recaptcha to comments page --- Gemfile | 1 + Gemfile.lock | 3 +++ app/controllers/comments_controller.rb | 9 ++++----- app/views/comments/index.html.erb | 9 +++++++-- config/initializers/recaptcha.rb | 8 ++++++++ 5 files changed, 23 insertions(+), 7 deletions(-) create mode 100644 config/initializers/recaptcha.rb diff --git a/Gemfile b/Gemfile index 5e7674e193..152afddb0c 100644 --- a/Gemfile +++ b/Gemfile @@ -40,6 +40,7 @@ gem 'avalon-about', git: 'https://github.com/avalonmediasystem/avalon-about.git' gem 'bootstrap-toggle-rails', git: 'https://github.com/rkallensee/bootstrap-toggle-rails.git', tag: 'v2.2.1.0' gem 'bootstrap_form' gem 'speedy-af', '~> 0.1.1' +gem 'recaptcha', require: 'recaptcha/rails' # Avalon Components gem 'avalon-workflow', git: "https://github.com/avalonmediasystem/avalon-workflow.git", tag: 'avalon-r6.2' diff --git a/Gemfile.lock b/Gemfile.lock index d6944fa5a5..33c5e753ec 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -633,6 +633,8 @@ GEM rdf (~> 2.1) rdf-xsd (2.2.0) rdf (~> 2.1) + recaptcha (4.12.0) + json redis (3.3.5) redis-actionpack (5.0.2) actionpack (>= 4.0, < 6) @@ -920,6 +922,7 @@ DEPENDENCIES rb-readline rdf (~> 2.2) rdf-rdfxml + recaptcha redis-rails resque (~> 1.27.0) resque-scheduler (~> 4.3.0) diff --git a/app/controllers/comments_controller.rb b/app/controllers/comments_controller.rb index 88f3a68316..11c53fd45b 100644 --- a/app/controllers/comments_controller.rb +++ b/app/controllers/comments_controller.rb @@ -29,17 +29,16 @@ def create @comment.subject = params[:comment][:subject] @comment.comment = params[:comment][:comment] - if (@comment.valid?) + if @comment.valid? and (Settings.recaptcha.blank? or verify_recaptcha(model: @comment)) begin CommentsMailer.contact_email(@comment.to_h).deliver_later rescue Errno::ECONNRESET => e logger.warn "The mail server does not appear to be responding \n #{e}" - - flash[:notice] = "The message could not be sent in a timely fashion. Contact us at #{Settings.email.support} to report the problem." - render action: "index" + flash[:notice] = "The message could not be sent in a timely fashion. Contact us at #{Settings.email.support} to report the problem." + render action: "index" end else - flash[:error] = "There were problems submitting your comment. Please correct the errors and try again." + flash[:error] = "Your comment was missing required information. Please complete all fields and resubmit." render action: "index" end end diff --git a/app/views/comments/index.html.erb b/app/views/comments/index.html.erb index d69a3aa52e..76a06de9c4 100644 --- a/app/views/comments/index.html.erb +++ b/app/views/comments/index.html.erb @@ -26,9 +26,14 @@ Unless required by applicable law or agreed to in writing, software distributed <%= comment.text_field :name, label: 'Name' %> <%= comment.hidden_field :nickname %> <%= comment.email_field :email, label: 'Email address' %> - <%= comment.email_field :email_confirmation, label: 'Confirm email address' %> - <%= comment.select :subject, @subjects, prompt: 'Select one' %> + <%= comment.email_field :email_confirmation, label: 'Confirm email address' %> + <%= comment.select :subject, @subjects, prompt: 'Select one' %> <%= comment.text_area :comment, rows: 10 %> + <% if Settings.recaptcha.present?%> +
+ <%= recaptcha_tags %> +
+ <% end %> <%= comment.primary "Submit comments" %> <% end %> diff --git a/config/initializers/recaptcha.rb b/config/initializers/recaptcha.rb new file mode 100644 index 0000000000..bf6bc58dcf --- /dev/null +++ b/config/initializers/recaptcha.rb @@ -0,0 +1,8 @@ + +# config/initializers/recaptcha.rb +if Settings.recaptcha.present? + Recaptcha.configure do |config| + config.site_key = Settings.recaptcha.site_key + config.secret_key = Settings.recaptcha.secret_key + end +end From 6ab1ecfdb1e997dcd499391a6740e18d1af8850e Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Wed, 5 Sep 2018 14:29:22 -0400 Subject: [PATCH 20/21] Bump version in anticipation of release --- config/application.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/config/application.rb b/config/application.rb index ac816f3cc3..6717aa49d0 100644 --- a/config/application.rb +++ b/config/application.rb @@ -8,7 +8,7 @@ Bundler.require(*Rails.groups) module Avalon - VERSION = '6.4.3' + VERSION = '6.4.4' class Application < Rails::Application require 'avalon/configuration' From d3a1182c29cf817a47fd88bab65879e3d182dc1c Mon Sep 17 00:00:00 2001 From: Chris Colvard Date: Thu, 6 Sep 2018 06:24:19 -0400 Subject: [PATCH 21/21] Bump version of rubyzip due to security vulnerability --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 33c5e753ec..5ee01833f8 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -726,7 +726,7 @@ GEM mime-types nokogiri rest-client - rubyzip (1.2.1) + rubyzip (1.2.2) rufus-scheduler (3.4.2) et-orbi (~> 1.0) safe_yaml (1.0.4)