From 1b45a7f1cc73a8f315b3fa0ed227810f873100a3 Mon Sep 17 00:00:00 2001 From: Andrew Myers Date: Mon, 19 Oct 2015 00:24:12 -0400 Subject: [PATCH] Implements a callback api * Allows callbacks for hooks to be defined in configuration block along with the rest of config options, for instance those generated by the installer. * Requires that callback hooks be 'enabled' so that we can all know what hooks are available to tie into. closes #378 --- .../curation_concerns/file_set_actor.rb | 12 ++--- .../app/jobs/audit_job.rb | 4 +- .../app/jobs/import_url_job.rb | 11 +--- .../app/jobs/ingest_file_job.rb | 4 +- .../app/jobs/ingest_local_file_job.rb | 12 +---- .../app/jobs/upload_set_update_job.rb | 30 +++++------ .../lib/curation_concerns/configuration.rb | 10 ++++ lib/curation_concerns/callbacks.rb | 26 +++++++++ lib/curation_concerns/callbacks/registry.rb | 48 +++++++++++++++++ spec/jobs/ingest_file_job_spec.rb | 5 ++ .../callbacks/registry_spec.rb | 54 +++++++++++++++++++ spec/lib/curation_concerns/callbacks_spec.rb | 27 ++++++++++ 12 files changed, 196 insertions(+), 47 deletions(-) create mode 100644 lib/curation_concerns/callbacks.rb create mode 100644 lib/curation_concerns/callbacks/registry.rb create mode 100644 spec/lib/curation_concerns/callbacks/registry_spec.rb create mode 100644 spec/lib/curation_concerns/callbacks_spec.rb diff --git a/curation_concerns-models/app/actors/curation_concerns/file_set_actor.rb b/curation_concerns-models/app/actors/curation_concerns/file_set_actor.rb index 3c58353d0f..107acfb509 100644 --- a/curation_concerns-models/app/actors/curation_concerns/file_set_actor.rb +++ b/curation_concerns-models/app/actors/curation_concerns/file_set_actor.rb @@ -73,8 +73,7 @@ def revert_content(revision_id) working_file = copy_repository_resource_to_working_directory(file_set) make_derivative(file_set.id, working_file) - return true unless CurationConcerns.config.respond_to?(:after_revert_content) - CurationConcerns.config.after_revert_content.call(file_set, user, revision_id) + CurationConcerns.config.callback.run(:after_revert_content, file_set, user, revision_id) true end @@ -82,8 +81,7 @@ def update_content(file) working_file = copy_file_to_working_directory(file, file_set.id) IngestFileJob.perform_later(file_set.id, working_file, file.content_type, user.user_key) make_derivative(file_set.id, working_file) - return true unless CurationConcerns.config.respond_to?(:after_update_content) - CurationConcerns.config.after_update_content.call(file_set, user) + CurationConcerns.config.callback.run(:after_update_content, file_set, user) true end @@ -93,16 +91,14 @@ def update_metadata(model_attributes, all_attributes) file_set.attributes = model_attributes file_set.date_modified = CurationConcerns::TimeService.time_in_utc save do - if CurationConcerns.config.respond_to?(:after_update_metadata) - CurationConcerns.config.after_update_metadata.call(file_set, user) - end + CurationConcerns.config.callback.run(:after_update_metadata, file_set, user) end end def destroy file_set.destroy # TODO: need to mend the linked list of proxies (possibly wrap with a lock) - CurationConcerns.config.after_destroy.call(file_set.id, user) if CurationConcerns.config.respond_to?(:after_destroy) + CurationConcerns.config.callback.run(:after_destroy, file_set.id, user) end private diff --git a/curation_concerns-models/app/jobs/audit_job.rb b/curation_concerns-models/app/jobs/audit_job.rb index 1a36d59a98..245e4937b8 100644 --- a/curation_concerns-models/app/jobs/audit_job.rb +++ b/curation_concerns-models/app/jobs/audit_job.rb @@ -18,10 +18,10 @@ def perform(id, file_id, uri) log = run_audit fixity_ok = log.pass == 1 unless fixity_ok - if CurationConcerns.config.respond_to?(:after_audit_failure) + if CurationConcerns.config.callback.set?(:after_audit_failure) login = file_set.depositor user = User.find_by_user_key(login) - CurationConcerns.config.after_audit_failure.call(file_set, user, log.created_at) + CurationConcerns.config.callback.run(:after_audit_failure, file_set, user, log.created_at) end end fixity_ok diff --git a/curation_concerns-models/app/jobs/import_url_job.rb b/curation_concerns-models/app/jobs/import_url_job.rb index 04a6c94386..fe7c5a1a4a 100644 --- a/curation_concerns-models/app/jobs/import_url_job.rb +++ b/curation_concerns-models/app/jobs/import_url_job.rb @@ -13,17 +13,10 @@ def perform(id) copy_remote_file(file_set.import_url, f) # attach downloaded file to generic file stubbed out if CurationConcerns::FileSetActor.new(file_set, user).create_content(f) - # send message to user on download success - if CurationConcerns.config.respond_to?(:after_import_url_success) - CurationConcerns.config.after_import_url_success.call(file_set, user) - end + CurationConcerns.config.callback.run(:after_import_url_success, file_set, user) else - - # send message to user on download failure - if CurationConcerns.config.respond_to?(:after_import_url_failure) - CurationConcerns.config.after_import_url_failure.call(file_set, user) - end + CurationConcerns.config.callback.run(:after_import_url_failure, file_set, user) end end end diff --git a/curation_concerns-models/app/jobs/ingest_file_job.rb b/curation_concerns-models/app/jobs/ingest_file_job.rb index fd6a4bf950..78f1409293 100644 --- a/curation_concerns-models/app/jobs/ingest_file_job.rb +++ b/curation_concerns-models/app/jobs/ingest_file_job.rb @@ -11,8 +11,6 @@ def perform(file_set_id, filename, mime_type, user_key) Hydra::Works::UploadFileToFileSet.call(file_set, file, versioning: false) file_set.save! CurationConcerns::VersioningService.create(file_set.original_file, user_key) - - return unless CurationConcerns.config.respond_to?(:after_create_content) - CurationConcerns.config.after_create_content.call(file_set, user_key) + CurationConcerns.config.callback.run(:after_create_content, file_set, user_key) end end diff --git a/curation_concerns-models/app/jobs/ingest_local_file_job.rb b/curation_concerns-models/app/jobs/ingest_local_file_job.rb index b3c992ee24..cce09ec55a 100644 --- a/curation_concerns-models/app/jobs/ingest_local_file_job.rb +++ b/curation_concerns-models/app/jobs/ingest_local_file_job.rb @@ -19,17 +19,9 @@ def perform(file_set_id, directory, filename, user_key) if actor.create_content(File.open(path)) FileUtils.rm(path) - - # send message to user on import success - if CurationConcerns.config.respond_to?(:after_import_local_file_success) - CurationConcerns.config.after_import_local_file_success.call(file_set, user, filename) - end + CurationConcerns.config.callback.run(:after_import_local_file_success, file_set, user, filename) else - - # send message to user on import failure - if CurationConcerns.config.respond_to?(:after_import_local_file_failure) - CurationConcerns.config.after_import_local_file_failure.call(file_set, user, filename) - end + CurationConcerns.config.callback.run(:after_import_local_file_failure, file_set, user, filename) end end end diff --git a/curation_concerns-models/app/jobs/upload_set_update_job.rb b/curation_concerns-models/app/jobs/upload_set_update_job.rb index 07a0dbf343..02647b9275 100644 --- a/curation_concerns-models/app/jobs/upload_set_update_job.rb +++ b/curation_concerns-models/app/jobs/upload_set_update_job.rb @@ -19,50 +19,50 @@ def perform(login, upload_set_id, title, file_attributes, visibility) upload_set = UploadSet.find_or_create(self.upload_set_id) user = User.find_by_user_key(self.login) - upload_set.file_sets.each do |gf| - update_file(gf, user) + upload_set.file_sets.each do |file| + update_file(file, user) end upload_set.update(status: ["Complete"]) if denied.empty? unless saved.empty? - if CurationConcerns.config.respond_to?(:after_upload_set_update_success) + if CurationConcerns.config.callback.set?(:after_upload_set_update_success) login = upload_set.depositor user = User.find_by_user_key(login) - CurationConcerns.config.after_upload_set_update_failure.call(user, upload_set, log.created_at) + CurationConcerns.config.callback.run(:after_upload_set_update_success, user, upload_set, log.created_at) end return true end else - if CurationConcerns.config.respond_to?(:after_upload_set_update_failure) + if CurationConcerns.config.callback.set?(:after_upload_set_update_failure) login = upload_set.depositor user = User.find_by_user_key(login) - CurationConcerns.config.after_upload_set_update_failure.call(user, upload_set, log.created_at) + CurationConcerns.config.callback.run(:after_upload_set_update_failure. user, upload_set, log.created_at) end return false end end - def update_file(gf, user) - unless user.can? :edit, gf - ActiveFedora::Base.logger.error "User #{user.user_key} DENIED access to #{gf.id}!" - denied << gf + def update_file(file, user) + unless user.can? :edit, file + ActiveFedora::Base.logger.error "User #{user.user_key} DENIED access to #{file.id}!" + denied << file return end # update the file using the actor after setting the title - gf.title = title[gf.id] if title[gf.id] - CurationConcerns::FileSetActor.new(gf, user).update_metadata(file_attributes, visibility: visibility) + file.title = title[file.id] if title[file.id] + CurationConcerns::FileSetActor.new(file, user).update_metadata(file_attributes, visibility: visibility) # update the work to the same metadata as the file. # NOTE: For the moment we are assuming copied metadata. This is likely to change. # NOTE2: TODO: stop assuming that files only belong to one work - work = gf.in_works.first + work = file.in_works.first unless work.nil? - work.title = title[gf.id] if title[gf.id] + work.title = title[file.id] if title[file.id] CurationConcerns::GenericWorkActor.new(work, user, work_attributes).update end - saved << gf + saved << file end end diff --git a/curation_concerns-models/lib/curation_concerns/configuration.rb b/curation_concerns-models/lib/curation_concerns/configuration.rb index a424d720d8..f588bb4312 100644 --- a/curation_concerns-models/lib/curation_concerns/configuration.rb +++ b/curation_concerns-models/lib/curation_concerns/configuration.rb @@ -1,3 +1,5 @@ +require 'curation_concerns/callbacks' + module CurationConcerns extend Deprecation class << self @@ -17,6 +19,8 @@ class Queue end class Configuration + include Callbacks + # An anonymous function that receives a path to a file # and returns AntiVirusScanner::NO_VIRUS_FOUND_RETURN_VALUE if no # virus is found; Any other returned value means a virus was found @@ -114,6 +118,12 @@ def lock_retry_delay @lock_retry_delay ||= 200 # milliseconds end + callback.enable :after_create_content, :after_update_content, + :after_revert_content, :after_update_metadata, + :after_import_local_file_success, + :after_import_local_file_failure, :after_audit_failure, + :after_destroy, :after_import_url_success, :after_import_url_failure + def register_curation_concern(*curation_concern_types) Array(curation_concern_types).flatten.compact.each do |cc_type| class_name = normalize_concern_name(cc_type) diff --git a/lib/curation_concerns/callbacks.rb b/lib/curation_concerns/callbacks.rb new file mode 100644 index 0000000000..28b40da3f8 --- /dev/null +++ b/lib/curation_concerns/callbacks.rb @@ -0,0 +1,26 @@ +require 'active_support/concern' +require 'curation_concerns/callbacks/registry' + +module CurationConcerns + module Callbacks + extend ActiveSupport::Concern + + included do + # Define class instance variable as endpoint to the + # Callback::Registry api. + @callback = Registry.new + end + + module ClassMethods + # Reader for class instance variable containing callback definitions. + def callback + @callback + end + end + + # Accessor to Callback::Registry api for instances. + def callback + self.class.callback + end + end +end diff --git a/lib/curation_concerns/callbacks/registry.rb b/lib/curation_concerns/callbacks/registry.rb new file mode 100644 index 0000000000..64bbaed72e --- /dev/null +++ b/lib/curation_concerns/callbacks/registry.rb @@ -0,0 +1,48 @@ +module CurationConcerns + module Callbacks + class Registry + attr_reader :callbacks + + def initialize + @callbacks = {} + end + + # Enables a callback by specifying one or more hooks. + def enable(hook, *more_hooks) + ([hook] + more_hooks).each { |h| @callbacks[h] ||= nil } + end + + # Returns all enabled callback hooks. + def enabled + @callbacks.keys + end + + # Returns true if the callback hook has been enabled. + def enabled?(hook) + @callbacks.key? hook + end + + # Defines a callback for a given hook. + def set(hook, &block) + raise NoBlockGiven, "a block is required when setting a callback" unless block_given? + @callbacks[hook] = proc(&block) + end + + # Returns true if a callback has been defined for a given hook. + def set?(hook) + enabled?(hook) && @callbacks[hook].respond_to?(:call) + end + + # Runs the callback defined for a given hook, with the arguments provided + def run(hook, *args) + raise NotEnabled unless enabled?(hook) + return nil unless set?(hook) + @callbacks[hook].call(*args) + end + end + + # Custom exceptions + class NotEnabled < StandardError; end + class NoBlockGiven < StandardError; end + end +end diff --git a/spec/jobs/ingest_file_job_spec.rb b/spec/jobs/ingest_file_job_spec.rb index 43c6cbb42f..89e6700acf 100644 --- a/spec/jobs/ingest_file_job_spec.rb +++ b/spec/jobs/ingest_file_job_spec.rb @@ -37,4 +37,9 @@ expect(VersionCommitter.where(version_id: versions.last.uri).pluck(:committer_login)).to eq ['bess'] end end + + it 'runs the after_create_content callback with file_set and user arguments' do + expect(CurationConcerns.config.callback).to receive(:run).with(:after_create_content, file_set, 'bob').exactly(1).times + described_class.perform_now(file_set.id, filename, 'image/png', 'bob') + end end diff --git a/spec/lib/curation_concerns/callbacks/registry_spec.rb b/spec/lib/curation_concerns/callbacks/registry_spec.rb new file mode 100644 index 0000000000..f9e1164b7b --- /dev/null +++ b/spec/lib/curation_concerns/callbacks/registry_spec.rb @@ -0,0 +1,54 @@ +require 'curation_concerns/callbacks/registry' + +describe CurationConcerns::Callbacks::Registry do + subject { described_class.new } + + describe "#enabled?" do + it "returns false if the specified callback has not been enabled" do + expect(subject.enabled?(:foo)) + end + + it "returns true after enabling the specified callback" do + subject.enable(:foo) + expect(subject.enabled?(:foo)).to eq true + end + end + + describe "#set?" do + it "returns false if the callback has not been set" do + expect(subject.set?(:foo)).to eq false + end + + it "returns true if the callback has been set" do + subject.set(:foo) { nil } + expect(subject.set?(:foo)).to eq true + end + end + + describe "#set" do + it "raises an error if a block is not given" do + expect { subject.set(:foo) }.to raise_error CurationConcerns::Callbacks::NoBlockGiven + end + + it "raises an error if given no arguments" do + expect { subject.set }.to raise_error ArgumentError + end + end + + describe "#run" do + it "raises a NotEnabled error if the callback has not been enabled" do + expect { subject.run(:foo) }.to raise_error CurationConcerns::Callbacks::NotEnabled + end + + it "runs the specified callback with parameters" do + subject.set(:foo) { |x, y| x + y } + expect(subject.run(:foo, 1, 2)).to eq 3 + end + + it "runs the most recently set callback" do + subject.set(:foo) { "first" } + subject.set(:foo) { "second" } + expect(subject.run(:foo)).to eq "second" + end + end +end diff --git a/spec/lib/curation_concerns/callbacks_spec.rb b/spec/lib/curation_concerns/callbacks_spec.rb new file mode 100644 index 0000000000..335f4c1682 --- /dev/null +++ b/spec/lib/curation_concerns/callbacks_spec.rb @@ -0,0 +1,27 @@ +require 'curation_concerns/callbacks' + +describe CurationConcerns::Callbacks do + context 'when included in a class,' do + before do + class TestClass + include CurationConcerns::Callbacks + end + end + + after do + Object.send(:remove_const, :TestClass) + end + + describe '.callback' do + it 'returns an instance of Callbacks::Registry' do + expect(TestClass.callback).to be_a CurationConcerns::Callbacks::Registry + end + end + + describe '#callback' do + it 'is an instance method shortcut to the class method of the same name' do + expect(TestClass.new.callback).to eq TestClass.callback + end + end + end +end