-
Notifications
You must be signed in to change notification settings - Fork 124
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Update depositors (proxies, transfers) synchronously #5537
Conversation
This job now only handles the events sent out aftet the work has been changed.
efdfaa5
to
491d022
Compare
This way it only happens if the code path gets by the guards.
491d022
to
c007962
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is looking really good. I had a couple of suggestions and a question.
def perform(work, user, reset = false) | ||
@reset = reset | ||
super(work, user) | ||
# @param [ActiveFedora::Base] work the work that's been transfered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can this parameter be a Hyrax::Work (i.e., Valkyrie::Resource)?
# @param [ActiveFedora::Base] work the work that's been transfered | |
# @param [ActiveFedora::Base,Hyrax::Work] work the work that's been transferred |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes thank you
depositor.log_event(event) | ||
end | ||
|
||
def proxy_depositor | ||
@proxy_depositor ||= ::User.find_by_user_key(work.proxy_depositor) | ||
private def previous_owner |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
# 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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nice to have exit checks and avoid unnecessary work
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
good catch
# | ||
# @return [Dry::Monads::Result] | ||
def call(obj, user: NullUser.new, reset: false) | ||
obj = Hyrax::ChangeContentDepositorService.call(obj, user, reset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should this bail if user is null?
obj = Hyrax::ChangeContentDepositorService.call(obj, user, reset) | |
return Success(obj) unless user&.user_key | |
obj = Hyrax::ChangeContentDepositorService.call(obj, user, reset) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the service has this guard, do you think we should do it both places?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We agreed to add it to make the code more explicit.
@@ -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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
superceded by #5553; code review addressed there. |
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: