Refactors transaction step for saving admin data (#889)

* Refactors transaction step for saving admin data

By default, multi-valued input fields display an additional set of input fields that are blank.
These get submitted by the form and are usually discarded, but because we are treating Sony Ci
ID field special, it was erroneously saving additional blank values for Sony Ci ID, which was
resulting in multiple players of the same media dislaying on AAPB when the record was published.

Multiple Sony Ci IDs are allowed in cases of mult-part Assets. They are stored as a serialized JSON
array in the AdminData.sonyci_id field, which is related to Asset reords via a global ID field on
AdminData.gid. And the global ID fields was used instead of a normal foreign key because the
AssetResource used to be just Asset, and stored in Fedora, not the database, and a global ID was
the closest thing to a foreign key across 2 persistence layers that we could think of :)

Fixes #882.

* Handles non-multiple, non-blank (default) case

Co-authored-by: Kiah Stroud <[email protected]>

* Add debug log for when nothing gets written

Co-authored-by: Kiah Stroud <[email protected]>

* Enumerates over SERIALIZED_FIELDS explicitly rather than duck typing

* Another refactor/simplification

Uses AdminData.attributes_for_updates as a filter to the change set fields.
Filters out blank sony ci ids so they don't get serialized.
Adds validation routine to AdminData for guarding against fields that cannot
  be deleted once set: bulkrax_importer_id and batch_ingest_batch_id.
  These IDs are foreign keys and if they accidentally get unset, we loase
  relational integrity, so need to make sure they are not accidentally
  getting unset via ingest, or updating Assets via UI, or anywhere else.

* adds missing code that was supposed to be part of the last commit 🤦, please squash when merging and remove this msg.

* Do not persist data in CreateAapbAdminData#set_admin_data_attributes

---------

Co-authored-by: Kiah Stroud <[email protected]>

app/models/admin_data.rb

@@ -1,21 +1,29 @@
 class AdminData < ApplicationRecord
+  self.table_name = "admin_data"
+
+  include ::EmptyDetection
+
   attr_reader :asset_error
 
   belongs_to  :hyrax_batch_ingest_batch, optional: true
   belongs_to  :bulkrax_importer, optional: true, class_name: 'Bulkrax::Importer'
   has_many    :annotations, dependent: :destroy
   accepts_nested_attributes_for :annotations, allow_destroy: true
-
-  self.table_name = "admin_data"
-  include ::EmptyDetection
-
   serialize :sonyci_id, Array
+  validate :validate_undeletable_fields
+
+  # TODO: Only used in AssetActor, which is deprecated. One AssetActor is
+  # removed, remove SERIALIZED_FIELD as well.
 
   SERIALIZED_FIELDS = [ :sonyci_id ]
 
+  # Mark fields that should not be deletable once set. These are used in validation.
+  UNDELETABLE_FIELDS = %w(bulkrax_importer_id hyrax_batch_ingest_batch_id)
+
   # Find the admin data associated with the Global Identifier (gid)
   # @param [String] gid - Global Identifier for this admin_data (e.g.gid://ams/admindata/1)
-  # @return [AdminData] if record matching gid is found, an instance of AdminData with id = the model_id portion of the gid (e.g. 1)
+  # @return [AdminData] if record matching gid is found, an instance of
+  #   AdminData with id = the model_id portion of the gid (e.g. 1)
   # @return [False] if record matching gid is not found
   def self.find_by_gid(gid)
     find(GlobalID.new(gid).model_id)
@@ -24,8 +32,10 @@ def self.find_by_gid(gid)
   end
 
   # Find the admin data associated with the Global Identifier (gid)
-  # @param [String] gid - Global Identifier for this adimindata (e.g. gid://ams/admindata/1)
-  # @return [AdminData] an instance of AdminData with id = the model_id portion of the gid (e.g. 1)
+  # @param [String] gid - Global Identifier for this adimindata\
+  #   (e.g. gid://ams/admindata/1)
+  # @return [AdminData] an instance of AdminData with id = the model_id portion
+  #   of the gid (e.g. 1)
   # @raise [ActiveRecord::RecordNotFound] if record matching gid is not found
   def self.find_by_gid!(gid)
     result = find_by_gid(gid)
@@ -35,11 +45,12 @@ def self.find_by_gid!(gid)
 
   # These are the attributes that could be edited through a form or through ingest.
   def self.attributes_for_update
-    (AdminData.attribute_names.dup - ['id', 'created_at', 'updated_at']).map(&:to_sym)
+    AdminData.attribute_names.dup - ['id', 'created_at', 'updated_at']
   end
 
   # Return the Global Identifier for this admin data.
-  # @return [String] Global Identifier (gid) for this AdminData (e.g.gid://ams/admindata/1)
+  # @return [String] Global Identifier (gid) for this AdminData
+  #   (e.g.gid://ams/admindata/1)
   def gid
     URI::GID.build(app: GlobalID.app, model_name: model_name.name.parameterize.to_sym, model_id: id).to_s if id
   end
@@ -78,4 +89,14 @@ def sonyci_records
   def sony_ci_api
     @sony_ci_api ||= SonyCiApi::Client.new('config/ci.yml')
   end
+
+  def validate_undeletable_fields
+    UNDELETABLE_FIELDS.each do |field|
+      # NOTE: the {field}_was helper comes from ActiveModel::Dirty
+      new_val, old_val = send(field), send("#{field}_was")
+      if new_val.blank? && old_val.present?
+        errors.add(field, "can't change from #{old_val} to blank")
+      end
+    end
+  end
 end

app/services/aapb/batch_ingest/csv_item_ingester.rb

@@ -213,8 +213,8 @@ def set_batch_ingest_id_on_related_asset(work_id, ability)
         def set_admin_data_attributes(admin_data, attributes)
           # add existing admin_data values so they're preserved in the AssetActor
           AdminData.attributes_for_update.each do |admin_attr|
-            next unless attributes.keys.include?(admin_attr.to_s)
-            admin_data.send("#{admin_attr}=", attributes[admin_attr.to_s])
+            next unless attributes.keys.include?(admin_attr)
+            admin_data.send("#{admin_attr}=", attributes[admin_attr])
           end
         end
 

app/transactions/ams/steps/create_aapb_admin_data.rb

@@ -51,22 +51,12 @@ def save_aapb_admin_data(change_set)
       end
 
       def set_admin_data_attributes(admin_data, change_set)
-        AdminData.attributes_for_update.each do |k|
-          # Some attributes are serialized on AdminData, so always send an array
-          k_string = k.to_s
-          if should_empty_admin_data_value?(k, change_set)
-            AdminData::SERIALIZED_FIELDS.include?(k) ? admin_data.send("#{k}=", Array.new) : admin_data.send("#{k}=", nil)
-          elsif change_set.fields[k_string].present?
-            AdminData::SERIALIZED_FIELDS.include?(k) ? admin_data.send("#{k}=", Array(change_set.fields[k_string])) : admin_data.send("#{k}=", change_set.fields[k_string].to_s)
-          end
+        admin_data_values = change_set.fields.slice(*AdminData.attributes_for_update).compact
+        # If Sony Ci IDs are present, ensure there are no blank ones, such as empty strings.
+        if admin_data_values.key?('sonyci_id')
+          admin_data_values['sonyci_id'] = Array(admin_data_values['sonyci_id']).reject(&:blank?)
         end
-      end
-
-      def should_empty_admin_data_value?(key, change_set)
-        return false if %i[bulkrax_importer_id hyrax_batch_ingest_batch_id].include?(key)
-
-        # The presence of the key with a "blank" value indicates we're intentionally emptying the value
-        change_set.fields.key?(key) && change_set.fields[key].blank?
+        admin_data.assign_attributes(admin_data_values)
       end
 
       def delete_removed_annotations(admin_data, change_set)