diff --git a/app/actors/hyrax/actors/transfer_request_actor.rb b/app/actors/hyrax/actors/transfer_request_actor.rb index 6ac589b869..e04c60f9a7 100644 --- a/app/actors/hyrax/actors/transfer_request_actor.rb +++ b/app/actors/hyrax/actors/transfer_request_actor.rb @@ -15,8 +15,9 @@ def create(env) def create_proxy_deposit_request(env) proxy = env.curation_concern.on_behalf_of return true if proxy.blank? - ContentDepositorChangeEventJob.perform_later(env.curation_concern, - ::User.find_by_user_key(proxy)) + work = env.curation_concern + user = ::User.find_by_user_key(proxy) + Hyrax::ChangeContentDepositorService.call(work, user, false) true end end diff --git a/app/controllers/concerns/hyrax/works_controller_behavior.rb b/app/controllers/concerns/hyrax/works_controller_behavior.rb index a07f97d125..85a86a21c0 100644 --- a/app/controllers/concerns/hyrax/works_controller_behavior.rb +++ b/app/controllers/concerns/hyrax/works_controller_behavior.rb @@ -181,9 +181,12 @@ def create_valkyrie_work result = transactions['change_set.create_work'] - .with_step_args('work_resource.add_to_parent' => { parent_id: params[:parent_id], user: current_user }, - 'work_resource.add_file_sets' => { uploaded_files: uploaded_files, file_set_params: params[hash_key_for_curation_concern][:file_set] }, - 'change_set.set_user_as_depositor' => { user: current_user }) + .with_step_args( + 'work_resource.add_to_parent' => { parent_id: params[:parent_id], user: current_user }, + 'work_resource.add_file_sets' => { uploaded_files: uploaded_files, file_set_params: params[hash_key_for_curation_concern][:file_set] }, + 'change_set.set_user_as_depositor' => { user: current_user }, + 'work_resource.change_content_depositor' => { user: ::User.find_by_user_key(form.on_behalf_of) } + ) .call(form) @curation_concern = result.value_or { return after_create_error(transaction_err_msg(result)) } after_create_response diff --git a/app/jobs/content_depositor_change_event_job.rb b/app/jobs/content_depositor_change_event_job.rb index 1daf416685..e6d03867a3 100644 --- a/app/jobs/content_depositor_change_event_job.rb +++ b/app/jobs/content_depositor_change_event_job.rb @@ -1,27 +1,24 @@ # frozen_string_literal: true # Log work depositor change to activity streams # -# @attr [Boolean] reset (false) should the access controls be reset. This means revoking edit access from the depositor +# This class simply logs the transfer, pulling data from the object that was +# just transferred. It does not perform the transfer. class ContentDepositorChangeEventJob < ContentEventJob include Rails.application.routes.url_helpers include ActionDispatch::Routing::PolymorphicRoutes - attr_accessor :reset - - # @param [ActiveFedora::Base] work the work to be transfered - # @param [User] user the user the work is being transfered to. - # @param [TrueClass,FalseClass] reset (false) if true, reset the access controls. This revokes edit access from the depositor - def perform(work, user, reset = false) - @reset = reset - super(work, user) + # @param [ActiveFedora::Base] work the work that's been transfered + def perform(work) + # these get set to repo_object and depositor + super(work, new_owner(work)) end def action - "User #{link_to_profile work.proxy_depositor} has transferred #{link_to_work work.title.first} to user #{link_to_profile depositor}" + "User #{link_to_profile repo_object.proxy_depositor} has transferred #{link_to_work repo_object.title.first} to user #{link_to_profile depositor}" end def link_to_work(text) - link_to text, polymorphic_path(work) + link_to text, polymorphic_path(repo_object) end # Log the event to the work's stream @@ -30,18 +27,19 @@ def log_work_event(work) end alias log_file_set_event log_work_event - def work - @work ||= Hyrax::ChangeContentDepositorService.call(repo_object, depositor, reset) - end - # overriding default to log the event to the depositor instead of their profile + # and to log the event for both users def log_user_event(depositor) - # log the event to the proxy depositor's profile - proxy_depositor.log_profile_event(event) + previous_owner.log_profile_event(event) depositor.log_event(event) end - def proxy_depositor - @proxy_depositor ||= ::User.find_by_user_key(work.proxy_depositor) + private def previous_owner + ::User.find_by_user_key(repo_object.proxy_depositor) + end + + # used for @depositor + private def new_owner(work) + ::User.find_by_user_key(work.depositor) end end diff --git a/app/models/proxy_deposit_request.rb b/app/models/proxy_deposit_request.rb index da538b20af..3cf0c94a82 100644 --- a/app/models/proxy_deposit_request.rb +++ b/app/models/proxy_deposit_request.rb @@ -129,7 +129,7 @@ def send_request_transfer_message_as_part_of_update # @param [TrueClass,FalseClass] reset (false) if true, reset the access controls. This revokes edit access from the depositor def transfer!(reset = false) - ContentDepositorChangeEventJob.perform_later(work, receiving_user, reset) + Hyrax::ChangeContentDepositorService.call(work, receiving_user, reset) fulfill!(status: ACCEPTED) end diff --git a/app/services/hyrax/change_content_depositor_service.rb b/app/services/hyrax/change_content_depositor_service.rb index f06feb3075..261abf5915 100644 --- a/app/services/hyrax/change_content_depositor_service.rb +++ b/app/services/hyrax/change_content_depositor_service.rb @@ -4,6 +4,10 @@ class ChangeContentDepositorService # Set the given `user` as the depositor of the given `work`; If # `reset` is true, first remove all previous permissions. # + # Used to transfer a an existing work, and to set + # depositor / proxy_depositor on a work newly deposited + # on_behalf_of another user + # # @param work [ActiveFedora::Base, Valkyrie::Resource] the work # that is receiving a change of depositor # @param user [User] the user that will "become" the depositor of @@ -12,13 +16,21 @@ class ChangeContentDepositorService # permissions for the given work and contained file # sets; regardless of true/false make the given user # the depositor of the given work + # @return work, updated if necessary def self.call(work, user, reset) - case work - when ActiveFedora::Base - call_af(work, user, reset) - when Valkyrie::Resource - call_valkyrie(work, user, reset) - end + # user_key is nil when there was no `on_behalf_of` in the form + return work unless user&.user_key + # Don't transfer to self + return work if user.user_key == work.depositor + + work = case work + when ActiveFedora::Base + call_af(work, user, reset) + when Valkyrie::Resource + call_valkyrie(work, user, reset) + end + ContentDepositorChangeEventJob.perform_later(work) + work end def self.call_af(work, user, reset) @@ -49,7 +61,6 @@ def self.call_valkyrie(work, user, reset) apply_valkyrie_changes_to_file_sets(work: work, user: user, reset: reset) Hyrax.persister.save(resource: work) - work end private_class_method :call_valkyrie diff --git a/lib/hyrax/transactions/container.rb b/lib/hyrax/transactions/container.rb index 008acbc81a..9c2762ea7f 100644 --- a/lib/hyrax/transactions/container.rb +++ b/lib/hyrax/transactions/container.rb @@ -33,6 +33,7 @@ class Container # rubocop:disable Metrics/ClassLength require 'hyrax/transactions/steps/add_to_collections' require 'hyrax/transactions/steps/add_to_parent' require 'hyrax/transactions/steps/apply_collection_type_permissions' + require 'hyrax/transactions/steps/change_content_depositor' require 'hyrax/transactions/steps/check_for_empty_admin_set' require 'hyrax/transactions/steps/delete_access_control' require 'hyrax/transactions/steps/delete_resource' @@ -204,6 +205,10 @@ class Container # rubocop:disable Metrics/ClassLength Steps::AddToParent.new end + ops.register 'change_content_depositor' do + Steps::ChangeContentDepositor.new + end + ops.register 'delete' do Steps::DeleteResource.new end diff --git a/lib/hyrax/transactions/steps/change_content_depositor.rb b/lib/hyrax/transactions/steps/change_content_depositor.rb new file mode 100644 index 0000000000..a620ea1615 --- /dev/null +++ b/lib/hyrax/transactions/steps/change_content_depositor.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true +require 'dry/monads' + +module Hyrax + module Transactions + module Steps + ## + # Add a given `::User` as the `#depositor` + # Move the previous value of that property to `#proxy_depositor` + # + # + # If no user is given, simply passes as a `Success`. + # + # @since 3.4.0 + class ChangeContentDepositor + include Dry::Monads[:result] + + ## + # @param [Hyrax::Work] obj + # @param user [User] the user that will "become" the depositor of + # the given work + # + # @return [Dry::Monads::Result] + def call(obj, user: NullUser.new, reset: false) + obj = Hyrax::ChangeContentDepositorService.call(obj, user, reset) + + Success(obj) + rescue StandardError => err + Failure([err.message, obj]) + end + + ## + # @api private + class NullUser + ## + # @return [nil] + def user_key + nil + end + end + end + end + end +end diff --git a/lib/hyrax/transactions/work_create.rb b/lib/hyrax/transactions/work_create.rb index 719e06dd35..89479c00fe 100644 --- a/lib/hyrax/transactions/work_create.rb +++ b/lib/hyrax/transactions/work_create.rb @@ -13,6 +13,7 @@ class WorkCreate < Transaction 'change_set.set_user_as_depositor', 'change_set.apply', 'work_resource.save_acl', + 'work_resource.change_content_depositor', 'work_resource.add_file_sets', 'work_resource.add_to_parent'].freeze diff --git a/spec/actors/hyrax/actors/transfer_request_actor_spec.rb b/spec/actors/hyrax/actors/transfer_request_actor_spec.rb index fcc20f5b3b..fa4d3105c3 100644 --- a/spec/actors/hyrax/actors/transfer_request_actor_spec.rb +++ b/spec/actors/hyrax/actors/transfer_request_actor_spec.rb @@ -36,7 +36,7 @@ end it "adds the template users to the work" do - expect(ContentDepositorChangeEventJob).to receive(:perform_later).with(work, User) + expect(ContentDepositorChangeEventJob).to receive(:perform_later).with(work) expect(middleware.create(env)).to be true end end diff --git a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb index bdd2ec5e6e..c5e8d39772 100644 --- a/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb +++ b/spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb @@ -95,6 +95,17 @@ expect(Sipity::Entity(assigns[:curation_concern]).workflow_state).to have_attributes(name: "deposited") end + context 'when depositing as a proxy for (on_behalf_of) another user' do + let(:create_params) { { title: 'comet in moominland', on_behalf_of: target_user.user_key } } + let(:target_user) { FactoryBot.create(:user) } + it 'transfers depositor status to proxy target' do + expect { post :create, params: { test_simple_work: create_params } } + .to have_enqueued_job(ContentDepositorChangeEventJob) + expect(assigns[:curation_concern]).to have_attributes(depositor: target_user.user_key) + expect(assigns[:curation_concern]).to have_attributes(proxy_depositor: user.user_key) + end + end + context 'when setting visibility' do let(:create_params) { { title: 'comet in moominland', visibility: 'open' } } diff --git a/spec/hyrax/transactions/steps/change_content_depositor_spec.rb b/spec/hyrax/transactions/steps/change_content_depositor_spec.rb new file mode 100644 index 0000000000..bd80e24b32 --- /dev/null +++ b/spec/hyrax/transactions/steps/change_content_depositor_spec.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true +require 'spec_helper' +require 'hyrax/transactions' + +RSpec.describe Hyrax::Transactions::Steps::ChangeContentDepositor, valkyrie_adapter: :test_adapter do + subject(:step) { described_class.new } + let(:work) { FactoryBot.valkyrie_create(:hyrax_work) } + + it 'gives Success(obj) in basic case' do + expect(step.call(work).value!).to eql(work) + end + + context "when the depositor update is successful" do + it "calls the service" do + allow(Hyrax::ChangeContentDepositorService).to receive(:call) + step.call(work) + + expect(Hyrax::ChangeContentDepositorService).to have_received(:call) + end + end + + context "when there's an error" do + it 'returns a Failure' do + allow(Hyrax::ChangeContentDepositorService).to receive(:call).and_raise + result = step.call(work) + + expect(result).to be_failure + end + end +end diff --git a/spec/jobs/content_depositor_change_event_job_spec.rb b/spec/jobs/content_depositor_change_event_job_spec.rb index a7d8b5ed4a..8f4f5b6cb5 100644 --- a/spec/jobs/content_depositor_change_event_job_spec.rb +++ b/spec/jobs/content_depositor_change_event_job_spec.rb @@ -1,12 +1,13 @@ # frozen_string_literal: true RSpec.describe ContentDepositorChangeEventJob do - let(:user) { create(:user) } - let(:another_user) { create(:user) } + let(:previous_user) { create(:user) } + let(:new_user) { create(:user) } let(:mock_time) { Time.zone.at(1) } let(:event) do - { action: "User #{user.user_key} " \ - "has transferred BethsMac " \ - "to user #{another_user.user_key}", + { action: + "User #{previous_user.user_key} " \ + "has transferred BethsMac " \ + "to user #{new_user.user_key}", timestamp: '1' } end @@ -15,42 +16,42 @@ end context "when passing an ActiveFedora work" do - let(:generic_work) { create(:generic_work, title: ['BethsMac'], user: user) } + let(:generic_work) { create(:generic_work, title: ['BethsMac'], user: new_user, proxy_depositor: previous_user.user_key) } it "logs the event to the proxy depositor's profile, the depositor's dashboard, and the FileSet" do - expect { described_class.perform_now(generic_work, another_user) } - .to change { user.profile_events.length } + expect { described_class.perform_now(generic_work) } + .to change { previous_user.profile_events.length } .by(1) - .and change { another_user.events.length } + .and change { new_user.events.length } .by(1) .and change { generic_work.events.length } .by(1) - expect(user.profile_events.first).to eq(event) - expect(another_user.events.first).to eq(event) + expect(previous_user.profile_events.first).to eq(event) + expect(new_user.events.first).to eq(event) expect(generic_work.events.first).to eq(event) end end context "when passing a valkyrie work" do - let(:monograph) { valkyrie_create(:monograph, title: ['BethsMac'], depositor: user.user_key) } + let(:monograph) { valkyrie_create(:monograph, title: ['BethsMac'], depositor: new_user.user_key, proxy_depositor: previous_user.user_key) } let(:event) do - { action: "User #{user.user_key} " \ + { action: "User #{previous_user.user_key} " \ "has transferred BethsMac " \ - "to user #{another_user.user_key}", + "to user #{new_user.user_key}", timestamp: '1' } end it "logs the event to the proxy depositor's profile, the depositor's dashboard, and the FileSet" do - expect { subject.perform(monograph, another_user) } - .to change { user.profile_events.length } + expect { subject.perform(monograph) } + .to change { previous_user.profile_events.length } .by(1) - .and change { another_user.events.length } + .and change { new_user.events.length } .by(1) .and change { monograph.events.length } .by(1) - expect(user.profile_events.first).to eq(event) - expect(another_user.events.first).to eq(event) + expect(previous_user.profile_events.first).to eq(event) + expect(new_user.events.first).to eq(event) expect(monograph.events.first).to eq(event) end end diff --git a/spec/models/proxy_deposit_request_spec.rb b/spec/models/proxy_deposit_request_spec.rb index 92a5710ad4..1f929ca1c5 100644 --- a/spec/models/proxy_deposit_request_spec.rb +++ b/spec/models/proxy_deposit_request_spec.rb @@ -77,11 +77,12 @@ describe '#transfer!' do it 'will change the status, fulfillment_date, and perform later the ContentDepositorChangeEventJob' do - allow(ContentDepositorChangeEventJob).to receive(:perform_later) + allow(Hyrax::ChangeContentDepositorService).to receive(:call) subject.transfer! expect(subject.status).to eq(described_class::ACCEPTED) expect(subject.fulfillment_date).to be_a(Time) expect(subject).to be_accepted + expect(Hyrax::ChangeContentDepositorService).to have_received(:call) end end diff --git a/spec/services/hyrax/change_content_depositor_service_spec.rb b/spec/services/hyrax/change_content_depositor_service_spec.rb index 368f036e4a..1bc1355fdf 100644 --- a/spec/services/hyrax/change_content_depositor_service_spec.rb +++ b/spec/services/hyrax/change_content_depositor_service_spec.rb @@ -62,6 +62,7 @@ context "by default, when permissions are not reset" do it "changes the depositor and records an original depositor" do + expect(ContentDepositorChangeEventJob).to receive(:perform_later) described_class.call(base_work, receiver, false) work = Hyrax.query_service.find_by_alternate_identifier(alternate_identifier: base_work.id, use_valkyrie: true) expect(work.depositor).to eq receiver.user_key @@ -83,6 +84,7 @@ context "when permissions are reset" do it "changes the depositor and records an original depositor" do + expect(ContentDepositorChangeEventJob).to receive(:perform_later) described_class.call(base_work, receiver, true) work = Hyrax.query_service.find_by_alternate_identifier(alternate_identifier: base_work.id, use_valkyrie: true) expect(work.depositor).to eq receiver.user_key @@ -101,5 +103,31 @@ end end end + + context "when no user is provided" do + it "does not update the work" do + expect(ContentDepositorChangeEventJob).not_to receive(:perform_later) + persister = double("Valkyrie Persister") + allow(Hyrax).to receive(:persister).and_return(persister) + allow(persister).to receive(:save) + + described_class.call(base_work, nil, false) + expect(persister).not_to have_received(:save) + end + end + + context "when transfer is requested to the existing owner" do + let!(:base_work) { valkyrie_create(:hyrax_work, :with_member_file_sets, title: ['AlreadyMine'], depositor: depositor.user_key, edit_users: [depositor]) } + + it "does not update the work" do + expect(ContentDepositorChangeEventJob).not_to receive(:perform_later) + persister = double("Valkyrie Persister") + allow(Hyrax).to receive(:persister).and_return(persister) + allow(persister).to receive(:save) + + described_class.call(base_work, depositor, false) + expect(persister).not_to have_received(:save) + end + end end end