Skip to content
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

Add find_by_alternate_identifier, for #207 #419

Merged
merged 3 commits into from
Mar 27, 2018
Merged

Conversation

stkenny
Copy link
Contributor

@stkenny stkenny commented Mar 16, 2018

Test based on success criteria pass for all adapters.

@stkenny stkenny added the Review label Mar 16, 2018
@tpendragon
Copy link
Collaborator

@stkenny Is this still a WIP?

@stkenny
Copy link
Contributor Author

stkenny commented Mar 16, 2018

I think the fedora adaptor needs to delete the extra created resource in delete.

@stkenny stkenny changed the title [WIP] Add find_by_alternate_identifier, for #207 Add find_by_alternate_identifier, for #207 Mar 16, 2018
Copy link
Collaborator

@tpendragon tpendragon left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Really small change, but then this looks great! No data migrations necessary too!

@@ -52,7 +52,7 @@ def generate_id(resource)
end

def ensure_multiple_values!(resource)
bad_keys = resource.attributes.except(:internal_resource, :created_at, :updated_at, :new_record, :id).select do |_k, v|
bad_keys = resource.attributes.except(:internal_resource, :created_at, :updated_at, :new_record, :id, :alternate_identifier).select do |_k, v|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't have to add this anymore right? The special value is alternate_ids

@@ -40,7 +40,7 @@ def wipe!
private

def ensure_multiple_values!(resource)
bad_keys = resource.attributes.except(:internal_resource, :created_at, :updated_at, :new_record, :id).select do |_k, v|
bad_keys = resource.attributes.except(:internal_resource, :created_at, :updated_at, :new_record, :id, :alternate_identifier).select do |_k, v|
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same thing here - not necessary anymore.

@@ -13,7 +13,8 @@ def initialize(resource, resource_factory:)
def convert!
orm_class.find_or_initialize_by(id: resource.id && resource.id.to_s).tap do |orm_object|
orm_object.internal_resource = resource.internal_resource
orm_object.metadata.merge!(resource.attributes.except(:id, :internal_resource, :created_at, :updated_at))
orm_object.alternate_identifier = resource.alternate_identifier if resource.respond_to?(:alternate_identifier)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think these are necessary anymore either.

@tpendragon tpendragon merged commit 818d42c into master Mar 27, 2018
@tpendragon tpendragon deleted the alternate_identifier branch March 27, 2018 16:13
@tpendragon tpendragon removed the Review label Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants