From 7600456dadd7b075246f829217a97a228c0752fe Mon Sep 17 00:00:00 2001 From: Andrew Myers Date: Thu, 6 Jun 2024 13:55:05 -0400 Subject: [PATCH 1/7] 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. --- .../ams/steps/create_aapb_admin_data.rb | 28 +++++++++++-------- 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/app/transactions/ams/steps/create_aapb_admin_data.rb b/app/transactions/ams/steps/create_aapb_admin_data.rb index 1bb8546a..f8249b54 100644 --- a/app/transactions/ams/steps/create_aapb_admin_data.rb +++ b/app/transactions/ams/steps/create_aapb_admin_data.rb @@ -51,22 +51,26 @@ 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) + AdminData.attributes_for_update.each do |field| + field = field.to_s + # Convert emtpy strings to nil, whether the value is a scalar or an array + new_admin_data_value = if change_set.fields[field].respond_to?(:select) + change_set.fields[field].reject {|v| v.blank? } + elsif change_set.fields[field].blank? + nil + end + + if can_empty_field?(field) || new_admin_data_value.present? + admin_data.write_attribute(field, new_admin_data_value) end 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? + def can_empty_field?(field) + %w( + bulkrax_importer_id + hyrax_batch_ingest_batch_id + ).exclude?(field.to_s) end def delete_removed_annotations(admin_data, change_set) From 269defafbd58d4d33122bbf9166fe5ec6d5eb12c Mon Sep 17 00:00:00 2001 From: Andrew Myers Date: Tue, 11 Jun 2024 15:49:03 -0400 Subject: [PATCH 2/7] Handles non-multiple, non-blank (default) case Co-authored-by: Kiah Stroud <32469930+bkiahstroud@users.noreply.github.com> --- app/transactions/ams/steps/create_aapb_admin_data.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/transactions/ams/steps/create_aapb_admin_data.rb b/app/transactions/ams/steps/create_aapb_admin_data.rb index f8249b54..c064b2c4 100644 --- a/app/transactions/ams/steps/create_aapb_admin_data.rb +++ b/app/transactions/ams/steps/create_aapb_admin_data.rb @@ -58,6 +58,8 @@ def set_admin_data_attributes(admin_data, change_set) change_set.fields[field].reject {|v| v.blank? } elsif change_set.fields[field].blank? nil + else # non-multiple with value present + change_set.fields[field] end if can_empty_field?(field) || new_admin_data_value.present? From b780e98469913d1726ddab19d9cb9a30fc7372f3 Mon Sep 17 00:00:00 2001 From: Andrew Myers Date: Tue, 11 Jun 2024 16:40:35 -0400 Subject: [PATCH 3/7] Add debug log for when nothing gets written Co-authored-by: Kiah Stroud <32469930+bkiahstroud@users.noreply.github.com> --- app/transactions/ams/steps/create_aapb_admin_data.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/transactions/ams/steps/create_aapb_admin_data.rb b/app/transactions/ams/steps/create_aapb_admin_data.rb index c064b2c4..89e53b1b 100644 --- a/app/transactions/ams/steps/create_aapb_admin_data.rb +++ b/app/transactions/ams/steps/create_aapb_admin_data.rb @@ -64,6 +64,8 @@ def set_admin_data_attributes(admin_data, change_set) if can_empty_field?(field) || new_admin_data_value.present? admin_data.write_attribute(field, new_admin_data_value) + else + Rails.logger.debug("Chose not to update AdminData #{admin_data.id}'s field #{field} with value(s): #{change_set.fields[field]}") end end end From 0eb35a6d4162aad7d219cbb362580aa23815f449 Mon Sep 17 00:00:00 2001 From: Andrew Myers Date: Tue, 11 Jun 2024 17:18:02 -0400 Subject: [PATCH 4/7] Enumerates over SERIALIZED_FIELDS explicitly rather than duck typing --- app/transactions/ams/steps/create_aapb_admin_data.rb | 11 ++++++++--- 1 file changed, 8 insertions(+), 3 deletions(-) diff --git a/app/transactions/ams/steps/create_aapb_admin_data.rb b/app/transactions/ams/steps/create_aapb_admin_data.rb index 89e53b1b..146526cf 100644 --- a/app/transactions/ams/steps/create_aapb_admin_data.rb +++ b/app/transactions/ams/steps/create_aapb_admin_data.rb @@ -53,15 +53,20 @@ def save_aapb_admin_data(change_set) def set_admin_data_attributes(admin_data, change_set) AdminData.attributes_for_update.each do |field| field = field.to_s - # Convert emtpy strings to nil, whether the value is a scalar or an array - new_admin_data_value = if change_set.fields[field].respond_to?(:select) + + # Conditionally set new value for admin data field + new_admin_data_value = if AdminData::SERIALIZED_FIELDS.include?(field.to_sym) + # Filter out blanks if the field is a serialized field, e.g. sonyci_id. change_set.fields[field].reject {|v| v.blank? } elsif change_set.fields[field].blank? + # If not serialized and blank, convert to nil nil - else # non-multiple with value present + else + # Otherwise keep value as is. change_set.fields[field] end + # If the AdminData field can be emptied OR if we have a non-blank value, then update the AdminData field. if can_empty_field?(field) || new_admin_data_value.present? admin_data.write_attribute(field, new_admin_data_value) else From c5fb57a1c056f9196f0351c97c742ff4f16a132a Mon Sep 17 00:00:00 2001 From: Andrew Myers Date: Thu, 13 Jun 2024 10:11:11 -0400 Subject: [PATCH 5/7] 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. --- .../aapb/batch_ingest/csv_item_ingester.rb | 4 +-- .../ams/steps/create_aapb_admin_data.rb | 33 +++---------------- 2 files changed, 7 insertions(+), 30 deletions(-) diff --git a/app/services/aapb/batch_ingest/csv_item_ingester.rb b/app/services/aapb/batch_ingest/csv_item_ingester.rb index 0ba9b8c6..35ae6cab 100644 --- a/app/services/aapb/batch_ingest/csv_item_ingester.rb +++ b/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 diff --git a/app/transactions/ams/steps/create_aapb_admin_data.rb b/app/transactions/ams/steps/create_aapb_admin_data.rb index 146526cf..b8106c7c 100644 --- a/app/transactions/ams/steps/create_aapb_admin_data.rb +++ b/app/transactions/ams/steps/create_aapb_admin_data.rb @@ -51,35 +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 |field| - field = field.to_s - - # Conditionally set new value for admin data field - new_admin_data_value = if AdminData::SERIALIZED_FIELDS.include?(field.to_sym) - # Filter out blanks if the field is a serialized field, e.g. sonyci_id. - change_set.fields[field].reject {|v| v.blank? } - elsif change_set.fields[field].blank? - # If not serialized and blank, convert to nil - nil - else - # Otherwise keep value as is. - change_set.fields[field] - end - - # If the AdminData field can be emptied OR if we have a non-blank value, then update the AdminData field. - if can_empty_field?(field) || new_admin_data_value.present? - admin_data.write_attribute(field, new_admin_data_value) - else - Rails.logger.debug("Chose not to update AdminData #{admin_data.id}'s field #{field} with value(s): #{change_set.fields[field]}") - 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 can_empty_field?(field) - %w( - bulkrax_importer_id - hyrax_batch_ingest_batch_id - ).exclude?(field.to_s) + admin_data.update!(admin_data_values) end def delete_removed_annotations(admin_data, change_set) From a2fb7f808a73622045caf723c155c306ab34b7ae Mon Sep 17 00:00:00 2001 From: Andrew Myers Date: Sat, 15 Jun 2024 12:53:37 -0400 Subject: [PATCH 6/7] adds missing code that was supposed to be part of the last commit :facepalm:, please squash when merging and remove this msg. --- app/models/admin_data.rb | 39 ++++++++++++++++++++++++++++++--------- 1 file changed, 30 insertions(+), 9 deletions(-) diff --git a/app/models/admin_data.rb b/app/models/admin_data.rb index 37cd3a07..c7d3d9d0 100644 --- a/app/models/admin_data.rb +++ b/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 From 4d775e1f6e0ba619dad0a0641865ee0d11b6d101 Mon Sep 17 00:00:00 2001 From: Andrew Myers Date: Wed, 19 Jun 2024 12:39:05 -0400 Subject: [PATCH 7/7] Do not persist data in CreateAapbAdminData#set_admin_data_attributes --- app/transactions/ams/steps/create_aapb_admin_data.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/transactions/ams/steps/create_aapb_admin_data.rb b/app/transactions/ams/steps/create_aapb_admin_data.rb index b8106c7c..2e1dd61b 100644 --- a/app/transactions/ams/steps/create_aapb_admin_data.rb +++ b/app/transactions/ams/steps/create_aapb_admin_data.rb @@ -56,7 +56,7 @@ def set_admin_data_attributes(admin_data, change_set) if admin_data_values.key?('sonyci_id') admin_data_values['sonyci_id'] = Array(admin_data_values['sonyci_id']).reject(&:blank?) end - admin_data.update!(admin_data_values) + admin_data.assign_attributes(admin_data_values) end def delete_removed_annotations(admin_data, change_set)