Build out Hyrax::ResourceForm to (mostly) support create views #4307
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
no-reply
force-pushed
the
form-model-class
branch
2 times, most recently
from
75466ce
to
7d193e8
Compare
jeremyf
requested changes
May 12, 2020
| let(:options_presenter) { double(select_options: []) } | ||
|
|
||
| before do | ||
| # mock the admin set options presenter to avoid hitting Solr |
There was a problem hiding this comment.
An ovation for removing a significant chunk of allow type mocking!
| context 'with an existing object' do | ||
| let(:work) { FactoryBot.valkyrie_create(:monograph) } | ||
|
|
||
| xit 'renders a form' do |
There was a problem hiding this comment.
Is this expected? Maybe a comment as to why it's a xit
There was a problem hiding this comment.
there's some discussion on this in the commit message. passing this test requires grappling with #version/optimistic locking, which i'm considering out of scope here.
|
the failing tests are because of |
no-reply
force-pushed
the
form-model-class
branch
from
ee52537
to
bd16279
Compare
added 5 commits
May 12, 2020 10:45
tests existing behavior on the controller side for the edit form. view tests are still needed, and are the next step. probably, there are behavioral problems there.
adds valkyrie work and ChangeSet/Hyrax::ResourceForm support for the `embargo/lease_enforced?` helper methods. this generalizes certain view partials to support a new form style.
since the new valkyrie models don't have pass-through attributes for embargo/lease settings, this form uses Reform's virtual properties to accept this input from the existing views. when saving the form, it will be necessary to interpret the virtual property states to determine appropriate changes to embargoes and leases. the values are prepopulated appropriately when `#prepopulate!` is called. for `#agreeement_accepted` we use a similar approach, setting to `false` by default, and to `true` when prepopulating with an existing model object.
no-reply
force-pushed
the
form-model-class
branch
from
bd16279
to
700c160
Compare
no-reply
force-pushed
the
form-model-class
branch
from
700c160
to
25baa5d
Compare
|
👍 the specs are mad, and Jeremy's comments are spot on, but over all this is a solid next step. Great work Tom |
no-reply
force-pushed
the
form-model-class
branch
3 times, most recently
from
c755640
to
e7eadb8
Compare
jeremyf
previously approved these changes
May 12, 2020
added 5 commits
May 13, 2020 14:21
adds support for permissions to change set style forms for use with valkyrie objects.
recent work has extended the ChangeSet style `Hyrax::ResourceForm` to support most of the view behavior for work create forms. this adds tests of the relevant form partials with the new form object. the partial that handles collection membership is stubbed, since its behavior is more complex and reproducing it in the new form should be considered separately. likewise, tests for these partials as an edit form (for existing objects) are left out; mainly because versions/optimistic locking becomes an issue in this case. there's more groundwork to lay there. to support this, a registration step is added to the new model generator. this sets up the routes when we bulid the test app.
Ensures `GenericWork` remains the "primary concern" for Hyrax, by keeping it at the top of the registry. We should really consider a better approach for determining the "primary concern", and also reducing the amount code that depends on this dubious concept.
`FilterByType` used to rely on `.to_rdf_representation`, which is `ActiveFedora`
specific. the default implementation is just `.name` ("RDF representation" means
here "unique name referring to the model type", Class.name fits this bill fine).
Add support for Valkyrie works in this context by using `.name` as a
fallback. possibly we'll want a different name for *actual* RDF representations,
but since this is really just for the solr index, probably we can just use
`.name` forever for the new models.
extends `EmbargoHelper#embargo_enforced?` to handle the case when a form object is explictly checked for an enforced embargo, instead of its model. rather than counting on the presence of a delegated method, we call recursively with `resource.model`. since `#model` is core to the `HydraEditor::Form` interface, this should be very resiliant.
no-reply
force-pushed
the
form-model-class
branch
from
e7eadb8
to
a609e40
Compare
jeremyf
approved these changes
May 14, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extends Valkyrie forms to handle visibility, lease/embargo, and permissions (called "sharing" by the relevant form partials).
Ignored here are:
class MonographForm < Hyrax::ResourceForm(Monograph); endAll of these are future work that could benefit from ongoing discussion.
@samvera/hyrax-code-reviewers