-
Notifications
You must be signed in to change notification settings - Fork 21
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
🧹 Make CreateRelationshipJob work for Valkyrie #908
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -80,8 +80,15 @@ def perform(parent_identifier:, importer_run_id:) # rubocop:disable Metrics/AbcS | |
# save record if members were added | ||
if @parent_record_members_added | ||
parent_record.save! | ||
# TODO: Push logic into Bulkrax.persistence_adapter | ||
# Ensure that the new relationship gets indexed onto the children | ||
@child_members_added.each(&:update_index) | ||
if parent_record.is_a?(Valkyrie::Resource) | ||
@child_members_added.each do |child| | ||
Hyrax.index_adapter.save(resource: child) | ||
end | ||
else | ||
@child_members_added.each(&:update_index) | ||
end | ||
end | ||
end | ||
else | ||
|
@@ -165,13 +172,34 @@ def add_to_collection(child_record, parent_record) | |
end | ||
|
||
def add_to_work(child_record, parent_record) | ||
return true if parent_record.ordered_members.to_a.include?(child_record) | ||
parent_record.is_a?(Valkyrie::Resource) ? add_to_valkyrie_work(child_record, parent_record) : add_to_af_work(child_record, parent_record) | ||
|
||
parent_record.ordered_members << child_record | ||
@parent_record_members_added = true | ||
@child_members_added << child_record | ||
end | ||
|
||
def add_to_valkyrie_work(child_record, parent_record) | ||
return true if parent_record.member_ids.include?(child_record.id) | ||
|
||
parent_record.member_ids << child_record.id | ||
|
||
# TODO: Hyrax is in the process of extracting an "Action" object that we could call. It does | ||
# provide validation that we may want to consider. | ||
# | ||
# NOTE: We may need to look at the step args we're passing, see | ||
# `Hyrax::WorksControllerBehavior#update_valkyrie_work` | ||
# Hyrax's `./app/controllers/concerns/hyrax/works_controller_behavior.rb` | ||
# | ||
change_set = Hyrax::ChangeSet.for(parent_record) | ||
Hyrax::Transactions::Container['change_set.update_work'].call(change_set) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. i'm curious about this choice. won't this ensure that the entire update work process (the same one that happens via the controller action) takes place just to change the work membership? Hyrax::Publisher provides an There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @no-reply, for our uses, we're trying to hit this transaction, so you're suggesting we put a publisher here and then that means this can become a listener? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. here was the original idea of the listener There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @kirkkwang yes. i think what i'm suggesting (and i'm just popping in as an outsider here), is that you want this behavior to trigger whenever an object's membership changes, not only when a given transaction runs. making it a i think a helpful line to draw around transactions and listeners is:
another way of thinking about it is:
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Thanks @no-reply that's super helpful! There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. FYI I made a ticket here scientist-softserv/iiif_print#330 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. |
||
end | ||
|
||
def add_to_af_work(child_record, parent_record) | ||
return true if parent_record.ordered_members.to_a.include?(child_record) | ||
|
||
parent_record.ordered_members << child_record | ||
end | ||
|
||
def reschedule(parent_identifier:, importer_run_id:) | ||
CreateRelationshipsJob.set(wait: 10.minutes).perform_later( | ||
parent_identifier: parent_identifier, | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,7 +6,7 @@ class ActiveFedoraAdapter < AbstractAdapter | |
def self.find(id) | ||
ActiveFedora::Base.find(id) | ||
rescue ActiveFedora::ObjectNotFoundError => e | ||
raise PersistenceLayer::RecordNotFound, e.message | ||
raise PersistenceLayer::ObjectNotFoundError, e.message | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Good catch! |
||
end | ||
|
||
def self.query(q, **kwargs) | ||
|
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 change fixed the reason why a link was not being displayed on a successful import. Before it would leave a message that the work didn't successfully import when that wasn't true.