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

Improve the Hyrax form API #6419

Merged
merged 5 commits into from
Nov 13, 2023
Merged

Improve the Hyrax form API #6419

merged 5 commits into from
Nov 13, 2023

Conversation

marrus-sh
Copy link
Collaborator

@marrus-sh marrus-sh commented Nov 7, 2023

This PR contains three related changes aimed at polishing up the API of Hyrax::Forms::ResourceForm and related classes :⁠—

  • Make all of the core Valkyrie forms inherit from Hyrax::Forms::ResourceForm. The ones which didn’t previously are Hyrax::Forms::PcdmCollectionForm and Hyrax::Forms::AdministrativeSetForm. This required splitting off some of the existing behaviours which aren’t needed for collections into a separate module, Hyrax::ContainedInWorksBehavior, which is included in Hyrax::Forms::PcdmObjectForm, Hyrax::Forms::FileSetForm, and Hyrax::Forms::ResourceBatchEditForm.

    This module is a bit long and messy, and an alternative would be to have Hyrax::Forms::PcdmObjectForm, Hyrax::Forms::FileSetForm, and Hyrax::Forms::ResourceBatchEditForm inherit from a subclass of Hyrax::Forms::ResourceForm which defined those behaviours. But the module approach seemed more flexible.

    The module could also probably be broken down into more specific parts, but it seemed a bit unnecessary given that at this time all of the forms which include it require all of those behaviours.

  • Use resource_class.pcdm_collection?, etc, in Hyrax::Forms::ResourceForm.for instead of resource_class <= Hyrax::PcdmCollection. This offers more flexibility to Hyrax applications to define modules that aren’t strict subclasses of builtin Hyrax ones. The if checks are a bit complicated, because Hyrax recognizes two types of PCDM collection (AdministrativeSet and PcdmCollection), and two types of PCDM object (FileSet and application‐defined model classes). The default is now to return a generic Hyrax::Forms::ResourceForm rather than a Hyrax::Forms::PcdmObjectForm since all application‐defined models should be returning true for resource_class.pcdm_object?; if an application has defined a resource which does not inherit from Hyrax::Work, they may need to define self.pcdm_object? on their model classes.

  • Change the API from Hyrax::Forms::ResourceForm.for(_) to Hyrax::Forms::ResourceForm.for(resource:_). The former is still supported, with a deprecation warning. Most places in Hyrax code take a resource argument as an explicit keyword argument, notably Hyrax::ValkyrieIndexer.for(resource:_) which is a very similar API. I have always been bothered that forms do not follow this pattern, and pre‐5.0 seems like a good time to make the switch.

@marrus-sh marrus-sh force-pushed the refactor_forms branch 7 times, most recently from 28aa36c to 1c014ac Compare November 9, 2023 01:35
@marrus-sh marrus-sh marked this pull request as ready for review November 9, 2023 01:55
@marrus-sh marrus-sh added notes-major Release Notes: Potentially breaking changes notes-valkyrie Release Notes: Valkyrie specific labels Nov 9, 2023
dlpierce
dlpierce previously approved these changes Nov 9, 2023
Formerly, `Hyrax::Forms::ResourceForm` contained a bunch of shared
behaviours between objects and filesets which weren’t appropriate
for adminsets or collections. This commit splits those behaviours
out into a new `Hyrax::ContainedInWorksBehavior` module, which
allows all forms to inherit from the same base class.
Use `ResourceForm.for(resource: resource)` instead of
`ResourceForm.for(resource)` to match
`ValkyrieIndexer.for(resource: resource)` and other places in Hyrax code
which require an explicit `resource` keyword argument. The old behaviour
is still supported, but issues a deprecation warning.
@marrus-sh
Copy link
Collaborator Author

@no-reply I’ve added a new commit to split up Hyrax::ContainedInWorksBehavior. I’m not clear on which modules need Hyrax::DepositorAgreementBehavior, so I’ve just left it in everything for now. Actually, depositor is a part of Hyrax core_metadata, it just doesn’t have any form information in the schema, so every Valkyrie form needs to declare it separately. There’s probably a separate refactor involving the behaviour of depositor which could be done, but I wanted to keep the scope of this change small.

In discussion, it was decided that it was better to break this out into
separate modules.
@no-reply no-reply merged commit a96e0e4 into main Nov 13, 2023
4 checks passed
@no-reply no-reply deleted the refactor_forms branch November 13, 2023 19:35
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
notes-major Release Notes: Potentially breaking changes notes-valkyrie Release Notes: Valkyrie specific
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants