Do proxy deposit updates synchronously

Add a new transaction step.
Replace behavior of queuing a job that runs a service with running a
service that then queues a job

closes #5457

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

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

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

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
 

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
 

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

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

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
 

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

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' } }
 

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

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 <a href=\"/users/#{user.to_param}\">#{user.user_key}</a> " \
-                          "has transferred <a href=\"/concern/generic_works/#{generic_work.id}\">BethsMac</a> " \
-                          "to user <a href=\"/users/#{another_user.to_param}\">#{another_user.user_key}</a>",
+    { action:
+        "User <a href=\"/users/#{previous_user.to_param}\">#{previous_user.user_key}</a> " \
+        "has transferred <a href=\"/concern/generic_works/#{generic_work.id}\">BethsMac</a> " \
+        "to user <a href=\"/users/#{new_user.to_param}\">#{new_user.user_key}</a>",
       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 <a href=\"/users/#{user.to_param}\">#{user.user_key}</a> " \
+      { action: "User <a href=\"/users/#{previous_user.to_param}\">#{previous_user.user_key}</a> " \
                 "has transferred <a href=\"/concern/monographs/#{monograph.id}\">BethsMac</a> " \
-                "to user <a href=\"/users/#{another_user.to_param}\">#{another_user.user_key}</a>",
+                "to user <a href=\"/users/#{new_user.to_param}\">#{new_user.user_key}</a>",
         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

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