Update depositors (proxies, transfers) synchronously #5537

hackartisan · 2022-03-15T18:07:56Z

This PR is in draft since it depends on merge of #5529. It should be rebased on main once that happens.

Follow-up to #5529
closes #5457

The ChangeContentDepositorService is where the correct depositor and
proxy_depositor values are set. the ContentDepositorChangeEventJob emits events related to depositor changes. Before this PR the service was always called by the job, which ran the service before emitting the related events. After this PR, that's flipped and the service enqueues the job after it's finished making the updates. Where the job was previously invoked, now we invoke the service.

Previous to #5529, the job (and thus the service) was called by a
listener or a callback in response to object deposit events. The changes in
#5529, intended to resolve a race condition bug, also made it easier to decouple
the AF behavior from the Valkyrie behavior, making the change in synchronous code in both cases.

Previous code state and summary of changes:

ChangeContentDepositorService was called from
  ContentDepositorChangeEventJob which is called from:

    app/models/proxy_deposit_request#transfer! (used by transfers_controller)

      - Call the depositor service instead of the change event job


    app/actors/hyrax/actors/transfer_request_actor

      - Call the depositor service instead of the change event job


    app/services/hyrax/listeners/proxy_deposit_listener
      listened for object.deposited, which is published by:

        lib/hyrax/transactions/steps/save.rb

          - Create a new transaction step to call invoke the service


        app/services/hyrax/work_uploads_handler.rb

          - I don't think the work uploads handler has any implications for
            proxy depositors so I think it's fine to leave this alone -- it can
            still publish the object.deposited message but with the proxy depositor
            listener removed I don't think there's any impact to behavior for
            this workflow.

        the after_create_concern callback, which is invoked by base_actor.rb

           - With the transfer request actor back in use, there's no impact here
             either.

Update the ChangeContentDepositorService to enqueue the ContentDepositorChangeEventJob. Update the ContentDepositorChangeEventJob to expect an work that has already been transferred, and to simply emit the required events rather than changing the objects first.

refs #5457

This job now only handles the events sent out aftet the work has been changed.

This way it only happens if the code path gets by the guards.

elrayle

This is looking really good. I had a couple of suggestions and a question.

elrayle · 2022-03-16T15:40:37Z

app/jobs/content_depositor_change_event_job.rb

-  def perform(work, user, reset = false)
-    @reset = reset
-    super(work, user)
+  # @param [ActiveFedora::Base] work the work that's been transfered


Can this parameter be a Hyrax::Work (i.e., Valkyrie::Resource)?

Suggested change

# @param [ActiveFedora::Base] work the work that's been transfered

# @param [ActiveFedora::Base,Hyrax::Work] work the work that's been transferred

yes thank you

elrayle · 2022-03-16T15:55:32Z

app/jobs/content_depositor_change_event_job.rb

    depositor.log_event(event)
  end

-  def proxy_depositor
-    @proxy_depositor ||= ::User.find_by_user_key(work.proxy_depositor)
+  private def previous_owner


Just curious, I haven't seen including private on the line with the method def. Has this always worked or did this start in a specific Rails version? Typically, the private keyword is on a line by itself with all methods that follow being private. In Hyrax, that is the only pattern I see in use.

Yeah, I'm not sure when. I've been thinking it would be nice to use it because it's more explicit than a private floating around somewhere in the file, especially if extra indentation isn't enforced. But it's not idiomatic; I'll change it.

elrayle · 2022-03-16T15:57:47Z

app/services/hyrax/change_content_depositor_service.rb

+      # 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


nice to have exit checks and avoid unnecessary work

elrayle · 2022-03-16T15:59:49Z

app/services/hyrax/change_content_depositor_service.rb

@@ -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


elrayle · 2022-03-16T16:05:17Z

lib/hyrax/transactions/steps/change_content_depositor.rb

+        #
+        # @return [Dry::Monads::Result]
+        def call(obj, user: NullUser.new, reset: false)
+          obj = Hyrax::ChangeContentDepositorService.call(obj, user, reset)


should this bail if user is null?

Suggested change

obj = Hyrax::ChangeContentDepositorService.call(obj, user, reset)

return Success(obj) unless user&.user_key

obj = Hyrax::ChangeContentDepositorService.call(obj, user, reset)

the service has this guard, do you think we should do it both places?

We agreed to add it to make the code more explicit.

elrayle · 2022-03-16T20:36:09Z

app/jobs/content_depositor_change_event_job.rb

@@ -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


This method call has been used for over 5 years. So it is making less sense to me now that this would be a problem. I originally thought this was moved here when the listener was created. Still not ruling out the various methods are chasing each other.

hackartisan · 2022-03-17T20:53:07Z

superceded by #5553; code review addressed there.

hackartisan marked this pull request as draft March 15, 2022 18:08

hackartisan changed the base branch from main to 5457-revert-proxy-deposit-listener March 15, 2022 18:14

hackartisan added 6 commits March 15, 2022 16:27

Tests for new pulling proxy depositor out of background job

3ea6965

Do proxy deposit updates synchronously

56e1425

refs #5457

Update content depositor change event job to assume updated work

1802349

This job now only handles the events sent out aftet the work has been changed.

Fix all the tests

1f37f24

Add more tests for the change content depositor service

af782ec

Add basic test for change content depositor transaction

7351d71

hackartisan force-pushed the 5457-add-proxy-deposit-transaction-step branch from efdfaa5 to 491d022 Compare March 15, 2022 20:41

Invoke content depositor change event job from service

c007962

This way it only happens if the code path gets by the guards.

hackartisan force-pushed the 5457-add-proxy-deposit-transaction-step branch from 491d022 to c007962 Compare March 15, 2022 21:03

hackartisan changed the title ~~Separate content depositor change from content depositor change event job~~ Update depositors (proxies, transfers) synchronously Mar 15, 2022

elrayle reviewed Mar 16, 2022

View reviewed changes

Base automatically changed from 5457-revert-proxy-deposit-listener to main March 16, 2022 17:07

elrayle reviewed Mar 16, 2022

View reviewed changes

hackartisan mentioned this pull request Mar 17, 2022

Update depositors (proxies, transfers) synchronously #5553

Merged

hackartisan closed this Mar 17, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Update depositors (proxies, transfers) synchronously #5537

Update depositors (proxies, transfers) synchronously #5537

hackartisan commented Mar 15, 2022 •

edited

elrayle left a comment

elrayle Mar 16, 2022

hackartisan Mar 17, 2022

elrayle Mar 16, 2022

hackartisan Mar 17, 2022

elrayle Mar 16, 2022

elrayle Mar 16, 2022

elrayle Mar 16, 2022

hackartisan Mar 17, 2022

hackartisan Mar 17, 2022

elrayle Mar 16, 2022

hackartisan commented Mar 17, 2022

	# @param [ActiveFedora::Base] work the work that's been transfered
	# @param [ActiveFedora::Base,Hyrax::Work] work the work that's been transferred

Update depositors (proxies, transfers) synchronously #5537

Update depositors (proxies, transfers) synchronously #5537

Conversation

hackartisan commented Mar 15, 2022 • edited

elrayle left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hackartisan commented Mar 17, 2022

hackartisan commented Mar 15, 2022 •

edited