Valkyrize batch editing #6148

no-reply · 2023-08-10T21:29:12Z

The PR is to support batch editing while valkyrie is enabled. This can be tested under dassie (w/o valkyrie) and koppie.

@samvera/hyrax-code-reviewers

Hydra works has this, and it will be nice to provide it to Wings users handling Valkyrie native models cast back to AF.

i don't know why these older tests run into LDP errors during setup after the rewrite in 361b113, but we don't want to use these factories in setup anyway. since the issue occurs before any production code is executed, it's safe to just refactor to setup using the Valkyrie based factories. we'll want to complete this refactor throughout this file anyway.

* Fix transaction error undefined method `<' for nil:NilClass * Fix transaction error implicit conversion of Symbol into Integer. * Refactor to fix lint error with the Metrics/AbcSize cop.

tpendragon

This does appear to work, and closes #4913.

I don't work in Hyrax much (once every..couple years?) so some of my comments probably come from just not knowing the code base really well. If so please just say so and I'll happily approve.

spec/support/features/batch_edit_actions.rb

app/controllers/hyrax/batch_edits_controller.rb

tpendragon · 2023-08-21T23:20:46Z

app/controllers/hyrax/batch_edits_controller.rb

+
+    # Clean up form fields
+    # @param form Hyrax::Froms::ResourceBatchEditForm
+    def cleanup_form_fields(form)


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?

i'm also curious about this.
it does seem like the form should handle these transformations.

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.

Action item: Fix this, I can take a look at it

tpendragon · 2023-08-21T23:21:15Z

app/services/hyrax/visibility_propagator.rb

@@ -10,7 +10,7 @@ class VisibilityPropagator
    # @return [#propagate]
    def self.for(source:)
      case source
-      when Hyrax::WorkBehavior # ActiveFedora
+      when ActiveFedora::Base # ActiveFedora


Question: I missed it, where did this change?

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.

ah, the generated model doesn't include WorkBehavior. it seems like the weird thing here is that the VisibilityCopyJob is casting the valkyrie resource back to an AF::Base, even when use_valkyrie is true(?)

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.

the 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.

lib/wings/active_fedora_converter/default_work.rb

tpendragon · 2023-08-21T23:24:37Z

app/controllers/hyrax/batch_edits_controller.rb

@@ -97,15 +116,15 @@ def destroy_batch
    end

    def form_class
-      Forms::BatchEditForm
+      Hyrax.config.use_valkyrie? ? Forms::ResourceBatchEditForm : Forms::BatchEditForm


nitpick: How's Hyrax feel about ternaries over explicit branches? We use an if/else above, should we use one here?

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?

tpendragon · 2023-08-21T23:26:01Z

app/forms/hyrax/forms/resource_batch_edit_form.rb

+
+      # Returns a list of parameters we accept from the form
+      # rubocop:disable Metrics/MethodLength
+      def self.build_permitted_params


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?

@tpendragon you probably know more about this form setup than most of us. these are valkyrie ChangeSets.

i have the same question about why this is needed, and where it's used other than the tests below.

tpendragon · 2023-08-21T23:27:18Z

app/forms/hyrax/forms/resource_batch_edit_form.rb

+      # override this method if you need to initialize more complex RDF assertions (b-nodes)
+      # @return [Hash<String, Array>] the list of unique values per field
+      def initialize_combined_fields
+        # For each of the files in the batch, set the attributes to be the concatenation of all the attributes


Maybe a little comment about why I'd want it to do this?

no-reply · 2023-08-22T00:08:12Z

app/forms/hyrax/forms/resource_batch_edit_form.rb

+        end
+      end
+
+      def terms


why is it okay to hard code these values? shouldn't they be derived from configuration?

Action item: See if we can use the model configurable fields. Pull @no-reply in if we get stuck with this.

no-reply · 2023-08-22T01:22:01Z

app/forms/hyrax/forms/resource_batch_edit_form.rb

+      include Hyrax::FormFields(:basic_metadata)
+
+      self.required_fields = []
+      self.model_class = Hyrax.primary_work_type


does this form only support the primary work type?

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.

tpendragon

We're going to merge this because it works and we're going to do an RC, and this feature hasn't worked really well in the past. We'll write a ticket for the adjustments we might want to do here.

lsitu force-pushed the valk-batch-edit-old branch from a4d9643 to 338fc48 Compare August 10, 2023 22:50

no-reply force-pushed the valk-batch-edit-old branch from 338fc48 to 9c1c3a6 Compare August 10, 2023 23:25

lsitu force-pushed the valk-batch-edit-old branch 2 times, most recently from 44ba338 to e47826f Compare August 15, 2023 23:29

cjcolvar and others added 6 commits August 21, 2023 09:52

Valkyrize batch editing

17b9f7e

wings: provide a convenience method DefaultWork#file_sets

cf502da

Hydra works has this, and it will be nice to provide it to Wings users handling Valkyrie native models cast back to AF.

Fix failed controller tests for batch edit.

df8e737

Fix the inconsistent param_key issue.

9f82151

Add column_for_attribute for active fedora compatibility.

bfc509c

no-reply force-pushed the valk-batch-edit-old branch from e47826f to d7a4196 Compare August 21, 2023 16:52

lsitu added 3 commits August 21, 2023 11:36

Clean up valkyrie form fields to avoid transaction errors:

49a5c9e

* Fix transaction error undefined method `<' for nil:NilClass * Fix transaction error implicit conversion of Symbol into Integer. * Refactor to fix lint error with the Metrics/AbcSize cop.

Correct permissions_attributes value format.

8dfb21e

Fix failed integration tests for valkyrie.

6f3099a

lsitu force-pushed the valk-batch-edit-old branch from d7a4196 to 6f3099a Compare August 21, 2023 18:39

no-reply marked this pull request as ready for review August 21, 2023 19:11

tpendragon requested changes Aug 21, 2023

View reviewed changes

no-reply commented Aug 22, 2023

View reviewed changes

Merge branch 'main' into valk-batch-edit-old

6b2c3ba

tpendragon approved these changes Aug 22, 2023

View reviewed changes

tpendragon merged commit e285a4a into main Aug 22, 2023
8 checks passed

tpendragon deleted the valk-batch-edit-old branch August 22, 2023 22:00

dlpierce added the notes-valkyrie Release Notes: Valkyrie specific label Aug 25, 2023

This was referenced Sep 5, 2023

Generate terms and build permitted params for valkyrie ResourceBatchEditForm dynamically. #6289

Merged

Derive terms for valkyrie batch edit from configuration #6293

Closed

Resource form should handle transformation for Valkyrie batch edit #6295

Open

tpendragon mentioned this pull request Sep 8, 2023

Valkyrize batch editing #5864

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Valkyrize batch editing #6148

Valkyrize batch editing #6148

no-reply commented Aug 10, 2023 •

edited by lsitu

Loading

tpendragon left a comment

tpendragon Aug 21, 2023

no-reply Aug 21, 2023

lsitu Aug 22, 2023

tpendragon Aug 22, 2023

tpendragon Aug 21, 2023

lsitu Aug 22, 2023

no-reply Aug 22, 2023

tpendragon Aug 21, 2023

no-reply Aug 21, 2023

tpendragon Aug 21, 2023

no-reply Aug 22, 2023

tpendragon Aug 21, 2023

no-reply Aug 22, 2023

tpendragon Aug 22, 2023

no-reply Aug 22, 2023

lsitu Aug 22, 2023

tpendragon left a comment

Valkyrize batch editing #6148

Valkyrize batch editing #6148

Conversation

no-reply commented Aug 10, 2023 • edited by lsitu Loading

tpendragon left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tpendragon left a comment

Choose a reason for hiding this comment

no-reply commented Aug 10, 2023 •

edited by lsitu

Loading