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

[wings] Round trip registered valkyrie native classes #4012

Merged
merged 2 commits into from
Sep 18, 2019
Merged

Conversation

no-reply
Copy link
Contributor

Fixes #3671; supersedes #3937

Users will want to define their models with native Valkyrie. This supports a transition by allowing them to define Valkyrie analogues for their existing ActiveFedora models.

New Valkyrie models need to be registered with Wings to cast to the correct ActiveFedora models within Wings. Wings continues to return meta-programmed model objects, but these now subclass the user's native Valkyrie models.

Changes proposed in this pull request:

  • Add a registry for Wings models
  • support and test round-tripping of native Valkyrie models in Wings

Guidance for testing, such as acceptance criteria or new user interface behaviors:

@samvera/hyrax-code-reviewers

@no-reply no-reply force-pushed the native-models-2 branch 2 times, most recently from ac966b8 to 1478d9c Compare September 18, 2019 16:56
#
# # `Wings` will cast the `BookResource` to a `Book` to persist via `ActiveFedora`
# resource = BookResource.new(author: 'Tove Jansson', title: 'Comet in Moominland')
# adapter = Wings::Valkyrie::MetadataAdapter.new
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i chose to use Wings::Valkyrie::MetadataAdapter.new here to show this working explicitly with the wings adapter. i guess we could change either this or the Hyrax.persister on L25, but honestly I'm not thinking it matters. It might be good to show the ways of selecting persisters, and just get better inline documentation for Hyrax.persister in at the Hyrax level.

@no-reply
Copy link
Contributor Author

@elrayle @cjcolvar i've made the suggested changes, and rebased on @lsitu's ModelTransformer refactor

elrayle
elrayle previously approved these changes Sep 18, 2019
Copy link
Contributor

@elrayle elrayle left a comment

Choose a reason for hiding this comment

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

Nice implementation!

end
end

after do
Object.send(:remove_const, :Book)
Object.send(:remove_const, :Image)
Hyrax::Test.send(:remove_const, :QueryService)
Copy link
Contributor

Choose a reason for hiding this comment

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

I like this! Makes for easier removal of constants defined in tests.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

yeah, we should probably start using Hyrax::Test everywhere test constants are needed

@VivianChu VivianChu self-requested a review September 18, 2019 17:36
Users will want to define their models with native Valkyrie. This supports a
transition by allowing them to define Valkyrie analogues for their existing
ActiveFedora models.
@VivianChu
Copy link
Contributor

👍

@no-reply no-reply merged commit 11f167a into master Sep 18, 2019
@no-reply no-reply deleted the native-models-2 branch September 18, 2019 18:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

wings persister: can find that resource again
3 participants