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

Use the ResourceForm for FileSets #5894

Merged
merged 5 commits into from
Nov 17, 2022
Merged

Use the ResourceForm for FileSets #5894

merged 5 commits into from
Nov 17, 2022

Conversation

marrus-sh
Copy link
Collaborator

@marrus-sh marrus-sh commented Oct 10, 2022

This PR makes the following changes (in the interest of resolving the remaining issues with #5575) :—

  • It refactors work‐specific form behaviours into a new class, Hyrax::Forms::PcdmObjectForm, to enable Hyrax::Forms::ResourceForm to be used with resource types other than PCDM objects @ 5c48b30. [Note №1]
  • It moves FileSet metadata into a new YAML‐based configuration, file_set_metadata, and updates the model and form to both use the same metadata (previously the model used basic_metadata, and the form had hardcoded fields) @ f700452. [Note №2]
  • It modifies Hyrax::Forms::FileSetForm to inherit from Hyrax::Forms::ResourceForm, with the intention of inheriting the lease and embargo handling behaviours provided there @ f63ca9c.
  • It uses the form, instead of the model object directly, in the FileSet edit view, to allow embargo information to be populated in a Valkyrie context @ d0b785e.

Notes:

  1. Hyrax::Forms::ResourceForm(…) now returns a class subclassing Hyrax::Forms::PcdmObjectForm, not Hyrax::Forms::ResourceForm directly. This is a bit awkward and confusing, but it preserves backwards‐compatibility with existing Hyrax applications which defined their models using the recommended class MyObjectForm < Hyrax::Forms::ResourceForm(MyObjectForm) syntax.

    FileSet forms (and other future forms which want to use the ResourceForm embargo/lease implementations but do not have PCDM object properties) consequently cannot use the function‐call syntax and should just subclass Hyrax::Forms::ResourceForm directly.

    See commit 5c48b30.

  2. Hyrax::FileSet formerly included Hyrax::Schema(:basic_metadata) in the model, but had a more limited set of metadata actually exposed through the Hyrax::FileSetForm. The two furthermore did not agree on license, in that it is optional in basic_metadata but was required in Hyrax::FileSetForm.

    I’ve resolved this by creating a new metadata YAML, file_set_metadata, with just the attributes listed in the form or required by tests, and license set to required. But another solution might be to keep the full basic_metadata schema and allow license to be optional. See

    RSpec.shared_examples 'a Hyrax::FileSet' do
    subject(:fileset) { described_class.new }
    let(:adapter) { Valkyrie::MetadataAdapter.find(:test_adapter) }
    let(:persister) { adapter.persister }
    let(:query_service) { adapter.query_service }
    it_behaves_like 'a Hyrax::Resource'
    it_behaves_like 'a model with core metadata'
    it_behaves_like 'a model with basic metadata'
    which implies general basic_metadata support, but not all attributes are tested and compare also 792ccfd which removed that requirement for collections considering that those attributes were never exposed in forms.

    (The attributes required by it_behaves_like 'a model with basic metadata' but not previously exposed on the form are: abstract, label, relative_path, resource_type, rights_notes, rights_statement, and source. If we’re comfortable revising this test, we could probably drop those from file_set_metadata.yaml.)

    By “form” I mean in a Reform/Hyrax::ChangeSet sense; the actual Edit page only has a field for title. That leaves open the question of which, if any, of these attributes actually need to be defined on the FileSet model?

    In the long term, I would like for the FileSet metadata to be configurable, but it’s hardcoded to whatever is in the YAML file for now.

    See commit f700452.

This method ensures that the correct `ResourceForm` subclass is
created for works.
This will enable other kinds of resources, like FileSets, to reuse the
lease & embargo functionality of `Hyrax::Forms::ResourceForm` without
implementing PCDM Object behaviours.

Awkwardly, this means that `Hyrax::Forms::ResourceForm(MyClass)` now
returns a `Hyrax::Forms::PcdmObjectForm`, not a
`Hyrax::Forms::ResourceForm`, but this is the only way to preserve
backwards‐compatibility.
@marrus-sh marrus-sh force-pushed the file-set-resource-form branch 10 times, most recently from 0cc7a34 to c129786 Compare October 17, 2022 21:14
This is a more restrictive set of attributes than was previously
present on the FileSet model, matching exactly those which were in the
old FileSet form. The form options differ from `basic_metadata` in the
case of `license`, which is required for FileSets but optional
in other places. If using `basic_metadata` for FileSets is desired,
resolving this difference will be necessary.
This allows the sharing of common pieces of implementation with other
Hyrax resource forms, like those for Hyrax::Works.
@marrus-sh marrus-sh changed the title [WIP] Use the Resource form for FileSets Use the ResourceForm for FileSets Oct 17, 2022
@marrus-sh marrus-sh marked this pull request as ready for review October 17, 2022 22:30
@jeremyf
Copy link
Contributor

jeremyf commented Oct 18, 2022

Can you talk a bit more about the desire for configurable FileSet metadata? I ask because this may drift the file_set towards a conceptual work.

Could these configurable attributes be better modeled in a work?

@no-reply
Copy link
Member

@jeremyf i'm not sure that i see that making FileSet metadata configurable changes FileSet's status. couldn't this configuration help us move away from the work-like description in the BasicMetadata we use now?

@marrus-sh
Copy link
Collaborator Author

marrus-sh commented Oct 18, 2022

@jeremyf as far as this PR is concerned, @no-reply is right in that the changes here move the FileSet away from a conceptual work, in that the model used to use the same metadata as works (:basic_metadata) and now uses different metadata (which will hopefully be more limited).

as far as my personal desire for configurable metadata is concerned, this is of course open to discussion :) . mostly, though, i think it is a gut feeling deriving from the large number of attributes already present on the FileSet model—if FileSet only included :core_metadata, i wouldn’t necessarily be asking for additional configurability, but considering it currently contains a number of additional properties beyond just :core_metadata, i feel like those properties should be configurable.

none of that configurability is in this PR, though.

@no-reply
Copy link
Member

no-reply commented Nov 2, 2022

@jeremyf do you have any thoughts about a path to move this one through?

@jeremyf
Copy link
Contributor

jeremyf commented Nov 15, 2022

I think this looks great and the docs and conversation help better understand what's happening!

@no-reply no-reply merged commit 16a7904 into main Nov 17, 2022
@no-reply no-reply deleted the file-set-resource-form branch November 17, 2022 18:50
@marrus-sh marrus-sh mentioned this pull request Nov 17, 2022
@dlpierce dlpierce added the notes-valkyrie Release Notes: Valkyrie specific label Mar 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-valkyrie Release Notes: Valkyrie specific valkyrization
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants