Update depositors (proxies, transfers) synchronously

.rubocop_fixme.yml

@@ -63,6 +63,7 @@ RSpec/ExampleLength:
     - 'spec/jobs/content_restored_version_event_job_spec.rb'
     - 'spec/jobs/content_new_version_event_job_spec.rb'
     - 'spec/jobs/content_depositor_change_event_job_spec.rb'
+    - 'spec/jobs/change_depositor_event_job_spec.rb'
     - 'spec/jobs/content_deposit_event_job_spec.rb'
     - 'spec/jobs/content_delete_event_job_spec.rb'
     - 'spec/jobs/ingest_file_job_spec.rb'
@@ -100,7 +101,6 @@ RSpec/SubjectStub:
     - 'spec/models/hyrax/operation_spec.rb'
     - 'spec/controllers/hyrax/accepts_batches_controller_spec.rb'
     - 'spec/indexers/hyrax/repository_reindexer_spec.rb'
-    - 'spec/jobs/content_depositor_change_event_job_spec.rb'
     - 'spec/lib/hyrax/analytics_spec.rb'
     - 'spec/models/job_io_wrapper_spec.rb'
     - 'spec/search_builders/hyrax/abstract_type_relation_spec.rb'

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::ChangeDepositorService.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_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/change_depositor_event_job.rb

@@ -0,0 +1,47 @@
+# frozen_string_literal: true
+# Log work depositor change to activity streams
+#
+# This class simply logs the transfer, pulling data from the object that was
+# just transferred. It does not perform the transfer.
+class ChangeDepositorEventJob < ContentEventJob
+  include Rails.application.routes.url_helpers
+  include ActionDispatch::Routing::PolymorphicRoutes
+
+  # @param [ActiveFedora::Base, Hyrax::Work] 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 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(repo_object)
+  end
+
+  # Log the event to the work's stream
+  def log_work_event(work)
+    work.log_event(event)
+  end
+  alias log_file_set_event log_work_event
+
+  # 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)
+    previous_owner.log_profile_event(event)
+    depositor.log_event(event)
+  end
+
+  private
+
+  def previous_owner
+    ::User.find_by_user_key(repo_object.proxy_depositor)
+  end
+
+  # used for @depositor
+  def new_owner(work)
+    ::User.find_by_user_key(work.depositor)
+  end
+end

app/jobs/content_depositor_change_event_job.rb

@@ -8,10 +8,11 @@ class ContentDepositorChangeEventJob < ContentEventJob
 
   attr_accessor :reset
 
-  # @param [ActiveFedora::Base] work the work to be transfered
+  # @param [ActiveFedora::Base, Hyrax::Work] work the work to be transferred
   # @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)
+    Deprecation.warn("This class will be removed in the next major release. Use Hyrax::ChangeDepositorService.call instead.")
     @reset = reset
     super(work, user)
   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::ChangeDepositorService.call(work, receiving_user, reset)
     fulfill!(status: ACCEPTED)
   end
 

app/services/hyrax/change_content_depositor_service.rb

@@ -13,6 +13,7 @@ class ChangeContentDepositorService
     #              sets; regardless of true/false make the given user
     #              the depositor of the given work
     def self.call(work, user, reset)
+      Deprecation.warn("This class will be removed in the next major release. Use Hyrax::ChangeDepositorService.call instead.")
       case work
       when ActiveFedora::Base
         call_af(work, user, reset)
@@ -49,7 +50,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
 

app/services/hyrax/change_depositor_service.rb

@@ -0,0 +1,86 @@
+# frozen_string_literal: true
+module Hyrax
+  class ChangeDepositorService
+    # 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
+    #             the given work
+    # @param reset [TrueClass, FalseClass] when true, first clear
+    #              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)
+      # 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
+      ChangeDepositorEventJob.perform_later(work)
+      work
+    end
+
+    def self.call_af(work, user, reset)
+      work.proxy_depositor = work.depositor
+      work.permissions = [] if reset
+      work.apply_depositor_metadata(user)
+      work.file_sets.each do |f|
+        f.permissions = [] if reset
+        f.apply_depositor_metadata(user)
+        f.save!
+      end
+      work.save!
+      work
+    end
+    private_class_method :call_af
+
+    # @todo Should this include some dependency injection regarding
+    # the Hyrax.persister and Hyrax.custom_queries?
+    def self.call_valkyrie(work, user, reset)
+      if reset
+        work.permission_manager.acl.permissions = []
+        work.permission_manager.acl.save
+      end
+
+      work.proxy_depositor = work.depositor
+      apply_depositor_metadata(work, user)
+
+      apply_valkyrie_changes_to_file_sets(work: work, user: user, reset: reset)
+
+      Hyrax.persister.save(resource: work)
+    end
+    private_class_method :call_valkyrie
+
+    def self.apply_depositor_metadata(resource, depositor)
+      depositor_id = depositor.respond_to?(:user_key) ? depositor.user_key : depositor
+      resource.depositor = depositor_id if resource.respond_to? :depositor=
+      Hyrax::AccessControlList.new(resource: resource).grant(:edit).to(::User.find_by_user_key(depositor_id)).save
+    end
+    private_class_method :apply_depositor_metadata
+
+    def self.apply_valkyrie_changes_to_file_sets(work:, user:, reset:)
+      Hyrax.custom_queries.find_child_file_sets(resource: work).each do |f|
+        if reset
+          f.permission_manager.acl.permissions = []
+          f.permission_manager.acl.save
+        end
+        apply_depositor_metadata(f, user)
+        Hyrax.persister.save(resource: f)
+      end
+    end
+    private_class_method :apply_valkyrie_changes_to_file_sets
+  end
+end

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_depositor'
       require 'hyrax/transactions/steps/check_for_empty_admin_set'
       require 'hyrax/transactions/steps/delete_access_control'
       require 'hyrax/transactions/steps/delete_resource'
@@ -205,6 +206,10 @@ class Container # rubocop:disable Metrics/ClassLength
           Steps::AddToParent.new
         end
 
+        ops.register 'change_depositor' do
+          Steps::ChangeDepositor.new
+        end
+
         ops.register 'delete' do
           Steps::DeleteResource.new
         end

lib/hyrax/transactions/steps/change_depositor.rb

@@ -0,0 +1,46 @@
+# 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 ChangeDepositor
+        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)
+          return Success(obj) unless user&.user_key
+
+          obj = Hyrax::ChangeDepositorService.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

@@ -14,6 +14,7 @@ class WorkCreate < Transaction
                        'change_set.apply',
                        'work_resource.save_acl',
                        'work_resource.add_file_sets',
+                       'work_resource.change_depositor',
                        '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(ChangeDepositorEventJob).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(ChangeDepositorEventJob)
+          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/controllers/hyrax/transfers_controller_spec.rb

@@ -79,7 +79,7 @@
       end
     end
 
-    describe "#accept", perform_enqueued: [ContentDepositorChangeEventJob] do
+    describe "#accept" do
       context "when I am the receiver" do
         let!(:incoming_work) do
           GenericWork.new(title: ['incoming']) do |w|

spec/features/create_work_spec.rb

@@ -53,7 +53,7 @@
     end
   end
 
-  context 'when the user is a proxy', perform_enqueued: [ContentDepositorChangeEventJob, AttachFilesToWorkJob, IngestJob] do
+  context 'when the user is a proxy', perform_enqueued: [AttachFilesToWorkJob, IngestJob] do
     let(:second_user) { create(:user) }
 
     before do

spec/hyrax/transactions/steps/change_depositor_spec.rb

@@ -0,0 +1,43 @@
+# frozen_string_literal: true
+require 'spec_helper'
+require 'hyrax/transactions'
+
+RSpec.describe Hyrax::Transactions::Steps::ChangeDepositor, valkyrie_adapter: :test_adapter do
+  subject(:step) { described_class.new }
+  let(:work)     { FactoryBot.valkyrie_create(:hyrax_work) }
+  let(:user)     { create(:user) }
+
+  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::ChangeDepositorService).to receive(:call).and_call_original
+      result = step.call(work, user: user).value!
+      expect(result.id).to eql(work.id)
+
+      expect(Hyrax::ChangeDepositorService).to have_received(:call)
+    end
+  end
+
+  context "when a nil user is passed" do
+    let(:user) { nil }
+
+    it "does not call the service" do
+      allow(Hyrax::ChangeDepositorService).to receive(:call).and_call_original
+      expect(step.call(work, user: nil).value!).to eql(work)
+
+      expect(Hyrax::ChangeDepositorService).not_to have_received(:call)
+    end
+  end
+
+  context "when there's an error" do
+    it 'returns a Failure' do
+      allow(Hyrax::ChangeDepositorService).to receive(:call).and_raise
+      result = step.call(work, user: user)
+
+      expect(result).to be_failure
+    end
+  end
+end