-
Notifications
You must be signed in to change notification settings - Fork 4
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
ARK minting #95
ARK minting #95
Conversation
@@ -56,6 +56,7 @@ def before_save(change_set:) | |||
|
|||
def after_save(change_set:, updated_resource:) | |||
append(append_id: change_set.append_id, updated_resource: updated_resource) if change_set.append_id.present? | |||
IdentifierService.mint_or_update(updated_resource) if change_set.try(:state_changed?) && change_set.new_state == 'complete' |
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.
I'm shocked this works - by the time after_save has worked the ChangeSet has sync
d already, right?
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.
It works because it saves the resource directly: https://github.com/pulibrary/figgy/pull/95/files#diff-41aae6e0457a6b7d231238cba013d0d1R8
Should we be minting the ARK earlier and getting it in before the change_set is persisted?
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.
Oh, I don't think this works. I don't think it ever gets called. There's only one spec, which sets a source_metadata_identifier, so it's being called as part of refreshing remote metadata. Part of why I don't like postfix conditionals - SimpleCov would have caught this
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.
Oh, I'll add another test that calls this one and see what we need to do to make sure it gets called when there's no remote metadata.
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.
Probably need a before_save hook to stash whether or not you need to identify.
app/models/remote_record.rb
Outdated
@@ -21,7 +21,7 @@ def success? | |||
end | |||
|
|||
def client_result | |||
@client_result ||= PulMetadataServices::Client.retrieve(source_metadata_identifier) | |||
@client_result ||= PulMetadataServices::Client.retrieve(Array.wrap(source_metadata_identifier).first) |
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 isn't necessary if you run prepopulate!
on the change_set
Fixes #54