:gift: WIP Flexible metadata for Valkyrie Objects

orangewolf commented 4 weeks ago

Summary

Beginning support for dynamic, user changeable metadata profiles for Valkyrie objects.

Issue:

https://github.com/scientist-softserv/amigos/issues/1

Note this should be rebased on main once double combo is in

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

*

Detailed Description

This is part of a broader feature to enable flexible metadata for Valkyrie objects. Essentially we are looking to be able to drive the fields in a work type (adminset, fileset, and collection) from an M3 profile. This is similar to the goals of Allinson flex. However, because this feature is often sought after, we are looking to put it in to Hyrax in some capacity.

This draft needs the following

These items are excluded from the PR to keep the PR manageable.

They are necessary for the feature to be released, but since it is behind a flag we recommend merging as we go.

@samvera/hyrax-code-reviewers

github-actions[bot] commented 2 weeks ago

Test Results

17 files ± 0 17 suites ±0 2h 17m 26s :stopwatch: - 2m 5s 6 731 tests +27 6 433 :white_check_mark: +27 297 :zzz: ±0 1 :x: ±0 13 238 runs +63 12 842 :white_check_mark: +63 395 :zzz: ±0 1 :x: ±0

For more details on these failures, see this check.

Results for commit 7f8525d3. ± Comparison against base commit 52af985f.

This pull request removes 274 and adds 301 tests. Note that renamed tests count towards both.

``` spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to destroy AdminSet: 4a8c8ac7-ff85-4afd-81a1-8a5c47c94249 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to destroy Hyrax::AdministrativeSet: 1946627f-8fdb-4dd7-93dc-cb7dff3176f4 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to edit AdminSet: 9ea91db6-37b3-488f-8659-94e064100920 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to edit Hyrax::AdministrativeSet: 16a18629-53fa-413e-9ca7-4ff9a8dd64f7 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to update AdminSet: a555b3ee-d4c3-4f79-b2ed-7afc9c518953 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to update Hyrax::AdministrativeSet: d214f880-a833-48ca-9005-99a1a070cf9d … ``` ``` spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to create # spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to destroy AdminSet: f87d9388-3184-420e-a19c-ba9a1779de30 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to destroy Hyrax::AdministrativeSet: 66ee3c7f-e567-4c8e-8902-1d4e2e0d4200 spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to edit AdminSet: e47218b0-0883-4754-a3e2-de0e8c84983c spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to edit Hyrax::AdministrativeSet: 3f352b5e-599d-4e01-b2c7-a2e82dba972a spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to update AdminSet: 51b738ef-6baa-42d0-94b3-08a53b84ce4c spec.abilities.ability_spec ‑ Hyrax::Ability AdminSets and PermissionTemplates a user without edit access is expected not to be able to update Hyrax::AdministrativeSet: 7e3cb648-8db7-455b-abcd-1a4601a67fc0 … ```

:recycle: This comment has been updated with latest results.

marrus-sh commented 1 week ago

The Surfliner M3 approach is a bit different because we don’t support versioning or live updating, but we do dynamically generate the model classes on startup rather than requiring them to be already present in the application. We use Valkyrie.config.resource_class_resolver for this, which winds up looking something like this…

def resolve(class_name)
  # Get the M3 definition of the class
  resource_class = resource_class_from_name(class_name)
  # “availability” refers to M3 `available_on`…
  availability = resource_class.name
  # Returns Hyrax::Work for works, Hyrax::Collection for collections…
  base_class = valkyrie_resource_class.call(resource_class)

  # Create the Hyrax model for the M3 class…
  Class.new(base_class) do |klass|
    # Define the Valkyrie attributes…
    klass.attributes(to_struct_attributes(availability: availability))
  end
end

Fundamentally, regarding the core Hyrax models, this is what we need to not break: We need to be able to dynamically define a new class extending a minimal Hyrax::Work, Hyrax::Collection… and add attributes to it in the ordinary Valkyrie way. I don’t think this MR does anything to get in the way of that, so I have no complaints there. The major difference in approach is that we aren’t doing any modification/overriding of the base Hyrax::Work or Hyrax::Collection models. Keeping those models as simple and vanilla‐Valkyrie as possible is ideal for us, which is part of why I’m hesitant towards the overriding of .new currently happening in Hyrax::Flexibility.

We aren’t currently using M3 for FileSets or AdminSets (and have no immediate plans to), and would like to keep using the upstream Hyrax models. One concern is that, if Hyrax moves towards a different M3 approach, we might want to support it in Hyrax::FileSet and Hyrax::AdministrativeSet without increasing the complexity of Hyrax::Work or Hyrax::Collection. (I mean, probably not, unless the old way of doing things is deprecated, but maybe it will be.) This is one of the motivating factors behind me asking for a more nuanced/modular approach. But this is perhaps a distant concern.

We do continue using the module builder approach for indexers and forms, and I would strongly encourage that here as well (but it seems like you’re doing that). In general, I agree with the approach being taken so far for indexing and form behaviors (as I understand it), altho it looks like there may be more work required to fully support M3 in this respect?

Regarding class/instance methods, we just do something like…

  def included(descendant)
    super

    descendant.singleton_class.include(ClassBehaviors)
    # etc
  end

inside the module builder to extend the singleton class whenever the module is included. We need this in our forms, for example, to define some form‐specific M3‐related class methods.

laritakr commented 1 week ago

I am in agreement with @marrus-sh. Enabling flexible metadata should be done selectively on a per-model basis. I want to have standard work models functioning next to flexible work models. Using a flexible work should not need flexible collections/admin sets/file sets.

This seems like a nice feature. I guess we had anticipated adding this later if needed. If you prefer it right at the start, we can do some brainstorming re: the best way to implement it.

dlpierce commented 1 week ago

I am in agreement with @marrus-sh. Enabling flexible metadata should be done selectively on a per-model basis. I want to have standard work models functioning next to flexible work models. Using a flexible work should not need flexible collections/admin sets/file sets.

This seems like a nice feature. I guess we had anticipated adding this later if needed. If you prefer it right at the start, we can do some brainstorming re: the best way to implement it.

Okay, let's get this merged, but I expect this to be addressed before this sees a release.

laritakr commented 6 days ago

This PR has been moved back to draft. We had hoped an initial PR that is behind a flag would allow some foundational behavior to get merged, allowing incremental subsequent small (more easily reviewed) PRs to build on that foundation.

While the requested changes are possibly reasonable requests, the details require more conversation to implement. We don't have the time right now to build the ideal solution and still meet our deadline, so we are reluctantly returning to the undesired "long-running branch" method.

samvera / hyrax