rework CollectionMembershipValidator to skip overwriting collections

the validator should limit itself to validating that the collections are
allowed.

whether or not validation is ever run, `#sync` should apply the form values to
the target resource. ensure this happens by moving the value population behavior
into a reform populator.

without this work, the `Hyrax::CollectionMembershipValidator` was erasing
existing collection membership. validators should never change the records being
validated.

app/forms/hyrax/forms/resource_form.rb

@@ -99,7 +99,7 @@ class ResourceForm < Hyrax::ChangeSet # rubocop:disable Metrics/ClassLength
       property :in_works_ids, virtual: true, prepopulator: InWorksPrepopulator
       property :member_ids, default: [], type: Valkyrie::Types::Array
       property :member_of_collection_ids, default: [], type: Valkyrie::Types::Array
-      property :member_of_collections_attributes, virtual: true
+      property :member_of_collections_attributes, virtual: true, populator: :in_collections_populator
       validates_with CollectionMembershipValidator
 
       property :representative_id, type: Valkyrie::Types::String
@@ -208,6 +208,20 @@ def display_additional_fields?
 
       private
 
+      def in_collections_populator(fragment:, **_options)
+        adds = []
+        deletes = []
+        fragment.each  do |_, h|
+          if h["_destroy"] == "true"
+            deletes << Valkyrie::ID.new(h["id"])
+          else
+            adds << Valkyrie::ID.new(h["id"])
+          end
+        end
+
+        self.member_of_collection_ids = ((member_of_collection_ids + adds) - deletes).uniq
+      end
+
       # https://trailblazer.to/2.1/docs/reform.html#reform-populators-populator-collections
       def permission_populator(collection:, index:, **)
         Hyrax::Forms::Permission.new(collection[index])

app/validators/hyrax/collection_membership_validator.rb

@@ -1,37 +1,38 @@
 # frozen_string_literal: true
 module Hyrax
-  # Validates that the record passes the multiple membership checker
+  ##
+  # Validates that the record passes the multiple membership requirements for collections.
   class CollectionMembershipValidator < ActiveModel::Validator
-    def validate(record)
-      update_collections(record)
-      validation = validate_multi_membership(record)
-      return if validation == true
-      record.errors[:member_of_collection_ids] << validation
+    ##
+    # @param multiple_membership_checker
+    def initialize(multiple_membership_checker: Hyrax::MultipleMembershipChecker, **options)
+      @multiple_membership_checker = multiple_membership_checker
+      super(options)
     end
 
-    private
-
-    def validate_multi_membership(record)
+    def validate(record)
       # collections-in-collections do not have multi-membership restrictions
       return true if record.is_a? Hyrax::Forms::PcdmCollectionForm
+      checker = @multiple_membership_checker.new(item: nil)
+      ids = collections_ids(record)
 
-      Hyrax::MultipleMembershipChecker.new(item: record).validate
+      errors = Array(checker.check(collection_ids: ids))
+      record.errors[:member_of_collection_ids].concat(errors)
     end
 
-    def update_collections(record)
-      record.member_of_collection_ids = collections_ids(record)
-      record.member_of_collection_ids.uniq!
-    end
+    private
 
     def collections_ids(record)
-      collection_ids = []
+      collection_ids = record.member_of_collection_ids.dup
+
       if record.member_of_collections_attributes.present?
         record.member_of_collections_attributes
               .each do |_k, h|
                 next if h["_destroy"] == "true"
                 collection_ids << Valkyrie::ID.new(h["id"])
               end
       end
+
       collection_ids
     end
   end

spec/controllers/concerns/hyrax/works_controller_behavior_spec.rb

@@ -123,6 +123,56 @@
         end
       end
 
+      context 'when adding a collection' do
+        let(:collection) { FactoryBot.valkyrie_create(:pcdm_collection) }
+
+        let(:create_params) do
+          { title: 'comet in moominland',
+            member_of_collection_ids: [collection.id.to_s] }
+        end
+
+        it 'adds to the collection' do
+          post :create, params: { test_simple_work: create_params }
+
+          expect(assigns[:curation_concern].member_of_collection_ids)
+            .to contain_exactly(collection.id)
+        end
+
+        context 'with attributes' do
+          let(:create_params) do
+            { title: 'comet in moominland',
+              member_of_collections_attributes:
+                { "0" =>
+                 { "id" => collection.id, "_destroy" => "false" } } }
+          end
+
+          it 'adds to the collection' do
+            post :create, params: { test_simple_work: create_params }
+
+            expect(assigns[:curation_concern].member_of_collection_ids)
+              .to contain_exactly(collection.id)
+          end
+        end
+
+        context 'with both setter styles' do
+          let(:other_collection) { FactoryBot.valkyrie_create(:pcdm_collection) }
+          let(:create_params) do
+            { title: 'comet in moominland',
+              member_of_collections_attributes:
+                { "0" =>
+                 { "id" => other_collection.id, "_destroy" => "false" } },
+              member_of_collection_ids: [collection.id] }
+          end
+
+          it 'adds to the collection' do
+            post :create, params: { test_simple_work: create_params }
+
+            expect(assigns[:curation_concern].member_of_collection_ids)
+              .to contain_exactly(collection.id, other_collection.id)
+          end
+        end
+      end
+
       context 'and files' do
         let(:uploads) { FactoryBot.create_list(:uploaded_file, 2, user: user) }
 

spec/factories/hyrax_collection.rb

@@ -3,7 +3,7 @@
 ##
 # Use this factory for generic Hyrax/HydraWorks Collections in valkyrie.
 FactoryBot.define do
-  factory :hyrax_collection, class: 'Hyrax::PcdmCollection' do
+  factory :hyrax_collection, class: 'Hyrax::PcdmCollection', aliases: [:pcdm_collection] do
     sequence(:title) { |n| ["The Tove Jansson Collection #{n}"] }
     collection_type_gid { Hyrax::CollectionType.find_or_create_default_collection_type.to_global_id.to_s }
 

spec/forms/hyrax/forms/resource_form_spec.rb

@@ -197,34 +197,44 @@
 
     context 'when collection membership is updated' do
       context 'from none to one' do
-        let(:work) { FactoryBot.valkyrie_create(:hyrax_work) }
+        let(:work) { FactoryBot.valkyrie_create(:hyrax_work, title: ['comet in moominland']) }
         let(:member_of_collections_attributes) do
           { "0" => { "id" => "123", "_destroy" => "false" } }
         end
 
         it 'is populated from member_of_collections_attributes' do
-          expect { form.validate(member_of_collections_attributes: member_of_collections_attributes) }
-            .to change { form.member_of_collection_ids&.map(&:id) }
+          form.validate(member_of_collections_attributes: member_of_collections_attributes)
+
+          expect { form.sync }
+            .to change { work.member_of_collection_ids }
             .to contain_exactly('123')
         end
       end
 
       context 'from 3 down to 2' do
-        let(:work) { FactoryBot.valkyrie_create(:hyrax_work, member_of_collection_ids: before_collection_ids) }
+        let(:work) do
+          FactoryBot.valkyrie_create(:hyrax_work,
+                                     member_of_collection_ids: before_collection_ids,
+                                     title: ['comet in moominland'])
+        end
+
         let(:col1) { FactoryBot.valkyrie_create(:hyrax_collection) }
         let(:col2) { FactoryBot.valkyrie_create(:hyrax_collection) }
         let(:col3) { FactoryBot.valkyrie_create(:hyrax_collection) }
         let(:before_collection_ids) { [col1.id, col2.id, col3.id] }
         let(:after_collection_ids) { [col1.id.to_s, col2.id.to_s] }
+
         let(:member_of_collections_attributes) do
           { "0" => { "id" => col1.id.to_s, "_destroy" => "false" },
             "1" => { "id" => col2.id.to_s, "_destroy" => "false" },
             "2" => { "id" => col3.id.to_s, "_destroy" => "true" } }
         end
 
         it 'is populated from member_of_collections_attributes' do
-          expect { form.validate(member_of_collections_attributes: member_of_collections_attributes) }
-            .to change { form.member_of_collection_ids }
+          form.validate(member_of_collections_attributes: member_of_collections_attributes)
+
+          expect { form.sync }
+            .to change { work.member_of_collection_ids }
             .to contain_exactly(*after_collection_ids)
         end
       end

spec/validators/hyrax/collection_membership_validator_spec.rb

@@ -0,0 +1,20 @@
+# frozen_string_literal: true
+
+RSpec.describe Hyrax::CollectionMembershipValidator do
+  subject(:validator) { described_class.new }
+  let(:work) { FactoryBot.build(:hyrax_work, :as_collection_member) }
+  let(:form) { Hyrax::Forms::ResourceForm(Monograph).new(work) }
+
+  describe '#validate' do
+    it 'is valid' do
+      expect { validator.validate(form) }
+        .not_to change { form.errors }
+        .from be_empty
+    end
+
+    it 'does not change existing collections' do
+      expect { validator.validate(form) }
+        .not_to change { form.member_of_collection_ids }
+    end
+  end
+end