WIP fix proxy depositor designation

refs #5457

app/controllers/concerns/hyrax/works_controller_behavior.rb

@@ -181,9 +181,11 @@ 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: 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

@@ -13,6 +13,8 @@ class ContentDepositorChangeEventJob < ContentEventJob
   # @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
+    # Note that this is the work from the method below, not the work passed into
+    # this intializer
     super(work, user)
   end
 
@@ -30,8 +32,16 @@ def log_work_event(work)
   end
   alias log_file_set_event log_work_event
 
+  # For Valklyrie resources, the code that updates the object has been moved to
+  # the transaction pipeline to avoid race conditions
   def work
-    @work ||= Hyrax::ChangeContentDepositorService.call(repo_object, depositor, reset)
+    @work ||=
+      case repo_object
+      when ActiveFedora::Base
+        Hyrax::ChangeContentDepositorService.call(repo_object, depositor, reset)
+      else
+        repo_object
+      end
   end
 
   # overriding default to log the event to the depositor instead of their profile

app/models/proxy_deposit_request.rb

@@ -129,6 +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)
+    Hyrax::ChangeContentDepositorService.call(work, receiving_user, reset)
     ContentDepositorChangeEventJob.perform_later(work, receiving_user, reset)
     fulfill!(status: ACCEPTED)
   end

app/services/hyrax/change_content_depositor_service.rb

@@ -1,5 +1,22 @@
 # frozen_string_literal: true
 module Hyrax
+  # TODO: call this service from the transaction pipeline, instead of from a
+  # background job invoked by a listener
+  #
+  # currently called from ContentDepositorChangeEventJob, app/jobs/content_depositor_change_event_job
+  #   which is called in:
+  #     app/models/proxy_deposit_request#transfer!,
+  #       which is called in:
+  # ***     app/controllers/hyrax/transfers_controller
+  #     app/actors/hyrax/actors/transfer_request_actor (basically a wrapper for this) -- old behavior,
+  #     app/services/hyrax/listeners/proxy_deposit_listener
+  #       which listens for object.deposited, which is published by:
+  # ***     lib/hyrax/transactions/steps/save.rb
+  #         app/services/hyrax/work_uploads_handler.rb -- doesn't look like this
+  #           will actually need the proxy depositor stuff so not worried about
+  #           moving it.
+  #         the after_create_concern callback, which is invoked by:
+  #           base_actor.rb -- old behavior
   class ChangeContentDepositorService
     # Set the given `user` as the depositor of the given `work`; If
     # `reset` is true, first remove all previous permissions.

app/services/hyrax/listeners/proxy_deposit_listener.rb

@@ -4,7 +4,12 @@ module Hyrax
   module Listeners
     ##
     # Listens for deposit events, and checks for proxy situations. When a user
-    # deposits an item `on_behalf_of` another, ensures transfer is handled.
+    # deposits an item `on_behalf_of` another, logs the event.
+    #
+    # Former behavior of the ContentDepositorChangeEventJob was to actually
+    # perform the transfer. That behavior is maintained for ActiveFedora code
+    # paths. Moving forward the transfer is performed separately from this
+    # listener and job, to avoid race conditions.
     class ProxyDepositListener
       ##
       # Called when 'object.deposited' event is published

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` via a ChangeSet.
+      # 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)
+          obj = Hyrax::ChangeContentDepositorService.call(obj, user, false)
+
+          Success(obj)
+        rescue NoMethodError => err
+          Failure([err.message, change_set])
+        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/controllers/concerns/hyrax/works_controller_behavior_spec.rb

@@ -102,6 +102,8 @@
         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
 

spec/models/proxy_deposit_request_spec.rb

@@ -78,10 +78,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).and_call_original
       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