Implement Valkyrie native Collections as `PcdmCollection` #4076

no-reply · 2019-10-15T01:09:21Z

Add a PCDM style collection in native Valkyrie, with support for
membership. Extract Work membership tests into shared examples for use across
PCDM/Works models.

We name the Collection model Hyrax::PcdmCollection to avoid application layer
collisions with ::Collection when referring to it from a Hyrax module. That
is: we don't want to introduce a Hyrax::Collection without warning adopters.

@samvera/hyrax-code-reviewers

Add a PCDM style collection in native Valkyrie, with support for membership. Extract Work membership tests into shared examples for use across PCDM/Works models. We name the Collection model `Hyrax::PcdmCollection` to avoid application layer collisions with `::Collection` when referring to it from a Hyrax module. That is: we don't want to introduce a `Hyrax::Collection` without warning adopters.

`Schema` is a base level dependency for much of Hyrax post-Valkyrie, so putting it in `lib` seems to make sense. This makes it easy to require independently when it needs to be used in, e.g. `Wings`.

Collections in Hyrax need a collection type. We link them to a `GlobalID` for a `CollectionType` using `#collection_type_gid`. This closely mirrors the old `Hyrax::CollectionBehavior` approach.

Setup a mapping between `Hyrax::PcdmCollection` and `::Collection` for use in Wings adapters. Introduce a `:hyrax_collection` factory with some basic membership accessors.

hweng

It looks good to me.
Is this your intention to keep the model as short as only include attributes?

Can i know what's our plan for those callbacks, validation, indexer, member_object methods and permission templates, as well as those mixins modules in collection_behavior.rb?

hweng · 2019-10-15T22:43:09Z

Are we planning to use dry-transactions + pub/sub style for callbacks?and push indexer, member_object, permission templates to app/services/hyrax/collection? Correct me if i am wrong.

hweng · 2019-10-15T23:04:25Z

app/models/hyrax/pcdm_collection.rb

+    include Hyrax::Schema(:core_metadata)
+
+    attribute :collection_type_gid, Valkyrie::Types::String
+    attribute :member_ids, Valkyrie::Types::Array.of(Valkyrie::Types::ID).meta(ordered: true)


Is that fine to add collection_type, collection_type= in the model?

We don't supply a collection_type setter without the _gid suffix in the current CollectionBehavior implementation. There is a ticket to refine that implementation, though it might be a wontfix until after the ActiveFedora behavior is removed.

I'm not necessarily against putting them in now, but I'm a bit worried about the impact on Wings. Keeping symmetry between Valkyrie and ActiveFedora properties simplifies things a bit.

Gotcha. Sounds reasonable.

no-reply · 2019-10-16T18:35:14Z

Is this your intention to keep the model as short as only include attributes?

That's my notion, yes. Attributes and code explicitly involved in managing them.

Obviously, we inherit Valkyrie::Resource so other code directly impacting the concerns is bundled there (e.g. Draper decoration, we choose to use it). Is this your intention to keep the model as short as only include attributes?

no-reply · 2019-10-16T18:54:36Z

Are we planning to use dry-transactions + pub/sub style for callbacks?and push indexer, member_object, permission templates to app/services/hyrax/collection? Correct me if i am wrong.

Indexing is already a separate service in Hyrax (see: app/indexers), though there's some work we need to do to figure out the ideal approach for Valkyrie. see: samvera/valkyrie#598

I think not dry-transaction, since development has been discontinued. I'm playing with its replacement at https://github.com/samvera/hyrax/compare/transaction-to-monads. That's one pattern we could consider using.

Re: Callbacks, I think this might be a bit case-by-case.

The Do notation approach above could probably be used for many of these cases. the Figgy-style ChangeSetPersister approach is probably also worth considering. For some callbacks (e.g. those in Hyrax.config.callback), dry-events style pub-sub seem good.

For others (e.g. those in CollectionNesting), just an external service that needs to be called explicitly (and only sometimes) is probably fine.

Membership is already handled here. I'm not sure what else is needed on this front. Any ideas?

Permissions are handled by Hyrax::AccessControl and Hyrax::AccessControlList, while template application is also an external service in Hyrax already. I think some further work on admin sets and permission templates in native Valkyrie is still called for.

@hweng does this help answer your questions? I realize there's still a lot to decide here, but I think it's best to take these things one at a time.

hweng · 2019-10-16T20:40:19Z

@no-reply Your approach https://github.com/samvera/hyrax/compare/transaction-to-monads looks good to me.

Regarding the callbacks, do you consider to add them as steps in Transactions?
For instance, save lib/hyrax/transactions/steps/save.rb

module Hyrax
  module Transactions
    module Steps
       class Save
         include Dry::Monads[:result]

         step :before_save
         Step :call
         step :after_save

        def before_save(&block); end 
    
        def after_save(&block); end 
        .....

Or if you have any concern about it?

hweng · 2019-10-16T20:46:15Z

Regarding the Permissions:

There are some local Permissions code in collection_behavior.rb and admit_set.rb, do you think it is safe to remove or should put it somewhere?
https://github.com/samvera/hyrax/blob/master/app/models/concerns/hyrax/collection_behavior.rb#L131-L162

hweng · 2019-10-16T20:51:02Z

For Membership, Do we still want to keep these methods add_member_objects, member_objects:

https://github.com/samvera/hyrax/blob/master/app/models/concerns/hyrax/collection_behavior.rb#L55-L83

elrayle · 2019-10-16T21:01:06Z

app/models/hyrax/pcdm_collection.rb

+module Hyrax
+  ##
+  # Valkyrie model for Collection domain objects in the Hydra Works model.
+  class PcdmCollection < Hyrax::Resource


I am concerned that we are establishing inconsistent naming of the Works Model classes. See my comments in PR #4084 RE: name of PcdmFileSet

https://github.com/samvera/hyrax/pull/4084/files/a5ab3ca30ca078c8c4ced3da56f5ff6c9b9922d9#r335705853

See my follow-up on the linked discussion.

hweng · 2019-10-17T20:06:24Z

@no-reply This PR looks good to me. I agree with you that there is still a lot to decide, might be best to discuss in the separate tickets.

Tom Johnson added 4 commits October 14, 2019 17:20

Move Hyrax::Schema and valkyrie metadata specifications into lib

20a1cb3

`Schema` is a base level dependency for much of Hyrax post-Valkyrie, so putting it in `lib` seems to make sense. This makes it easy to require independently when it needs to be used in, e.g. `Wings`.

Add collection_type_gid to Valkyrie native collection

adf777e

Collections in Hyrax need a collection type. We link them to a `GlobalID` for a `CollectionType` using `#collection_type_gid`. This closely mirrors the old `Hyrax::CollectionBehavior` approach.

[wings] Enable Collection round-tripping with Wings

b2d1d24

Setup a mapping between `Hyrax::PcdmCollection` and `::Collection` for use in Wings adapters. Introduce a `:hyrax_collection` factory with some basic membership accessors.

hweng reviewed Oct 15, 2019

View reviewed changes

cjcolvar added the wings label Oct 16, 2019

no-reply mentioned this pull request Oct 16, 2019

add base FileSet resource; add find_files custom query #4084

Merged

elrayle reviewed Oct 16, 2019

View reviewed changes

hweng approved these changes Oct 17, 2019

View reviewed changes

no-reply merged commit 3a02960 into master Oct 17, 2019

no-reply deleted the native-collection branch October 17, 2019 20:55

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement Valkyrie native Collections as `PcdmCollection` #4076

Implement Valkyrie native Collections as `PcdmCollection` #4076

no-reply commented Oct 15, 2019

hweng left a comment •

edited

Loading

hweng commented Oct 15, 2019 •

edited

Loading

hweng Oct 15, 2019

no-reply Oct 16, 2019

hweng Oct 16, 2019

no-reply commented Oct 16, 2019

no-reply commented Oct 16, 2019

hweng commented Oct 16, 2019

hweng commented Oct 16, 2019

hweng commented Oct 16, 2019

elrayle Oct 16, 2019

no-reply Oct 16, 2019

hweng commented Oct 17, 2019

Implement Valkyrie native Collections as PcdmCollection #4076

Implement Valkyrie native Collections as PcdmCollection #4076

Conversation

no-reply commented Oct 15, 2019

hweng left a comment • edited Loading

Choose a reason for hiding this comment

hweng commented Oct 15, 2019 • edited Loading

hweng Oct 15, 2019

Choose a reason for hiding this comment

no-reply Oct 16, 2019

Choose a reason for hiding this comment

hweng Oct 16, 2019

Choose a reason for hiding this comment

no-reply commented Oct 16, 2019

no-reply commented Oct 16, 2019

hweng commented Oct 16, 2019

hweng commented Oct 16, 2019

hweng commented Oct 16, 2019

elrayle Oct 16, 2019

Choose a reason for hiding this comment

no-reply Oct 16, 2019

Choose a reason for hiding this comment

hweng commented Oct 17, 2019

Implement Valkyrie native Collections as `PcdmCollection` #4076

Implement Valkyrie native Collections as `PcdmCollection` #4076

hweng left a comment •

edited

Loading

hweng commented Oct 15, 2019 •

edited

Loading