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.
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
Valkyrize batch editing #6148
Valkyrize batch editing #6148
Changes from all commits
17b9f7e
cf502da
356b0b7
df8e737
9f82151
bfc509c
49a5c9e
8dfb21e
6f3099a
6b2c3ba
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nitpick: How's Hyrax feel about ternaries over explicit branches? We use an if/else above, should we use one here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if it's not being flagged by rubocop, i think we don't block on style.
on the other hand, i'm wondering if we could just drop all the old handling? if the valkyrie code path is working, do we need the opt-in?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Suggestion: Can we get a comment about why this is necessary? Why doesn't the form handle this somewhere? Does this mean the form's not useful outside of this controller?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
i'm also curious about this.
it does seem like the form should handle these transformations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure what is the best way to handle it. The another way that we can try is to merge the submitted params to the resource attributes in form.validate(params). Please advice.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Action item: Fix this, I can take a look at it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this form only support the primary work type?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No. It'll work for any models, even with a batch that mixes with different objects types. The model_class here is only served for retrieving a consistent
param_key
for different object types.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why is it okay to hard code these values? shouldn't they be derived from configuration?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Action item: See if we can use the model configurable fields. Pull @no-reply in if we get stuck with this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: I just don't know much about hyrax forms anymore - if there's form objects with a restricted subset of fields, why do they also need to define their permitted params? Where's this get used - do all the form objects have these or is it something special about bulk edit?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tpendragon you probably know more about this form setup than most of us. these are valkyrie
ChangeSet
s.i have the same question about why this is needed, and where it's used other than the tests below.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe a little comment about why I'd want it to do this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: I missed it, where did this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The reason is that the monograph https://github.com/samvera/hyrax/blob/main/.dassie/app/models/monograph.rb model won't include Hyrax::WorkBehavior, which will cause tests related to visibility changes failed for fileset visibility propagating.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, the generated model doesn't include
WorkBehavior
. it seems like the weird thing here is that theVisibilityCopyJob
is casting the valkyrie resource back to an AF::Base, even whenuse_valkyrie
istrue
(?)i think in the ideal case, both of the following would be true:
VisibilityCopyJob
should support works generated by Wings; being generous about the type check as proposed here seems good.use_valkyrie
code path here would leverage valkyrie objects throughout.i think it would be worth checking up about the second point as part of this work.