Validate sample type changes against editing constraints

app/models/sample_attribute.rb

@@ -10,6 +10,7 @@ class SampleAttribute < ApplicationRecord
 
   validates :sample_type, presence: true
   validates :pid, format: { with: URI::regexp, allow_blank: true, allow_nil: true, message: 'not a valid URI' }
+  validate :validate_against_editing_constraints
 
   before_save :store_accessor_name
   before_save :default_pos, :force_required_when_is_title
@@ -70,4 +71,22 @@ def force_required_when_is_title
     true
   end
 
+  def validate_against_editing_constraints
+    c = sample_type.editing_constraints
+    error_message = "cannot be changed (#{title_was})" # Use pre-change title in error message.
+
+    errors.add(:title, error_message) if changed_attributes.key?(:title) && !c.allow_name_change?(self)
+
+    unless c.allow_required?(self)
+      errors.add(:is_title, error_message) if changed_attributes.key?(:is_title)
+      errors.add(:required, error_message) if changed_attributes.key?(:required)
+    end
+
+    unless c.allow_type_change?(self)
+      errors.add(:sample_attribute_type, error_message) if changed_attributes.key?(:sample_attribute_type_id)
+      errors.add(:sample_controlled_vocab, error_message) if changed_attributes.key?(:sample_controlled_vocab_id)
+      errors.add(:linked_sample_type, error_message) if changed_attributes.key?(:linked_sample_type_id)
+      errors.add(:unit, error_message) if changed_attributes.key?(:unit_id)
+    end
+  end
 end

app/models/sample_type.rb

@@ -47,8 +47,11 @@ class SampleType < ApplicationRecord
   validates :title, length: { maximum: 255 }
   validates :description, length: { maximum: 65_535 }
   validates :contributor, presence: true
-  validate :validate_one_title_attribute_present, :validate_attribute_title_unique, :validate_attribute_accessor_names_unique, 
-           :validate_title_is_not_type_of_seek_sample_multi
+  validate :validate_one_title_attribute_present,
+           :validate_attribute_title_unique,
+           :validate_attribute_accessor_names_unique,
+           :validate_title_is_not_type_of_seek_sample_multi,
+           :validate_against_editing_constraints
   validates :projects, presence: true, projects: { self: true }
 
   accepts_nested_attributes_for :sample_attributes, allow_destroy: true
@@ -196,6 +199,19 @@ def validate_attribute_accessor_names_unique
     end
   end
 
+  def validate_against_editing_constraints
+    c = editing_constraints
+    sample_attributes.each do |a|
+      if a.marked_for_destruction? && !c.allow_attribute_removal?(a)
+        errors.add(:base, "Cannot remove: \"#{a.title}\", there are existing samples using this attribute.")
+      end
+
+      if a.new_record? && !c.allow_new_attribute?
+        errors.add(:base, "Cannot add: \"#{a.title}\", new attributes are not allowed.")
+      end
+    end
+  end
+
   def attribute_search_terms
     attribute_titles
   end

lib/seek/samples/sample_type_editing_constraints.rb

@@ -82,11 +82,11 @@ def refresh_cache
       private
 
       def blanks?(attr)
-        analysis_hash[attr.to_sym][:has_blanks]
+        !analysis_hash.key?(attr.to_sym) || analysis_hash[attr.to_sym][:has_blanks]
       end
 
       def all_blank?(attr)
-        analysis_hash[attr.to_sym][:all_blank]
+        !analysis_hash.key?(attr.to_sym) || analysis_hash[attr.to_sym][:all_blank]
       end
 
       def analysis_hash

test/functional/sample_types_controller_test.rb

@@ -698,6 +698,22 @@ class SampleTypesControllerTest < ActionController::TestCase
     end
   end
 
+  test 'validates changes against editing constraints' do
+    @sample_type.samples.create!(data: { the_title: 'yes' }, sample_type: @sample_type, project_ids: @project_ids)
+
+    assert_no_difference('ActivityLog.count') do
+      put :update, params: { id: @sample_type, sample_type: {
+        sample_attributes_attributes: {
+          '0' => { id: @sample_type.sample_attributes.first.id, pos: '1', title: 'banana', required: '1' }
+        }
+      } }
+    end
+
+    assert_response :unprocessable_entity
+    assert_select 'div#error_explanation' do
+      assert_select 'ul > li', text: 'Sample attributes title cannot be changed (the_title)'
+    end
+  end
 
   private
 

test/unit/sample_type_test.rb

@@ -994,6 +994,104 @@ def setup
     refute sample_type.contributor_credited?
   end
 
+  test 'validates changes against editing constraints' do
+    sample_type = FactoryBot.create(:linked_optional_sample_type, contributor: @person)
+    different_sample_type = FactoryBot.create(:simple_sample_type, contributor: @person)
+    patient_sample = nil
+    User.with_current_user(@person.user) do
+      attr = sample_type.sample_attributes.detect { |t| t.accessor_name == 'patient' }
+      patient_sample = FactoryBot.create(:patient_sample, sample_type: attr.linked_sample_type)
+      sample_type.samples.create!(data: { title: 'Lib-4', patient: patient_sample.id }, sample_type: sample_type,
+                                  project_ids: @person.project_ids)
+    end
+
+    assert sample_type.valid?
+
+    # Adding attribute
+    sample_type.sample_attributes.build(title: 'test 123')
+    refute sample_type.valid?
+    assert sample_type.errors.added?(:'base', 'Cannot add: "test 123", new attributes are not allowed.')
+
+    sample_type.reload
+    assert sample_type.valid?
+
+    # Removing attribute (via nested attributes)
+    sample_type.sample_attributes_attributes = { id: sample_type.sample_attributes.last.id, _destroy: '1' }
+    refute sample_type.valid?
+    assert sample_type.errors.added?(:'base', 'Cannot remove: "patient", there are existing samples using this attribute.')
+
+    sample_type.reload
+    assert sample_type.valid?
+
+    # Changing attribute title
+    sample_type.sample_attributes.last.title = 'banana'
+    refute sample_type.valid?
+    assert sample_type.errors.added?(:'sample_attributes.title', 'cannot be changed (patient)')
+
+    sample_type.reload
+    assert sample_type.valid?
+
+    # Changing "required" attribute
+    User.with_current_user(@person.user) do
+      sample_type.samples.create!(data: { title: 'Lib-5', patient: nil }, sample_type: sample_type,
+                                  project_ids: @person.project_ids)
+    end
+    sample_type.sample_attributes.last.required = true
+    refute sample_type.valid?
+    assert sample_type.errors.added?(:'sample_attributes.required', 'cannot be changed (patient)')
+
+    sample_type.reload
+    assert sample_type.valid?
+
+    # Changing "title" attribute
+    sample_type.sample_attributes.last.is_title = true
+    refute sample_type.valid?
+    assert sample_type.errors.added?(:'sample_attributes.is_title', 'cannot be changed (patient)')
+
+    sample_type.reload
+    assert sample_type.valid?
+
+    # Changing sample attribute type
+    sample_type.sample_attributes.last.sample_attribute_type = FactoryBot.create(:integer_sample_attribute_type)
+    refute sample_type.valid?
+    assert sample_type.errors.added?(:'sample_attributes.sample_attribute_type', 'cannot be changed (patient)')
+
+    sample_type.reload
+    assert sample_type.valid?
+
+    # Changing linked sample type
+    attr = sample_type.sample_attributes.detect { |t| t.accessor_name == 'patient' }
+    attr.linked_sample_type = different_sample_type
+    refute sample_type.valid?
+    assert sample_type.errors.added?(:'sample_attributes.linked_sample_type', 'cannot be changed (patient)')
+
+    sample_type.reload
+    assert sample_type.valid?
+
+    # Changing sample controlled vocab
+    sample_type = FactoryBot.create(:apples_controlled_vocab_sample_type, contributor: @person)
+    User.with_current_user(@person.user) do
+      sample_type.samples.create!(data: { apples: 'Bramley' }, sample_type: sample_type, project_ids: @person.project_ids)
+    end
+
+    assert sample_type.valid?
+    attr = sample_type.sample_attributes.detect { |t| t.accessor_name == 'apples' }
+    attr.sample_controlled_vocab = FactoryBot.create(:sample_controlled_vocab)
+    refute sample_type.valid?
+    assert sample_type.errors.added?(:'sample_attributes.sample_controlled_vocab', 'cannot be changed (apples)')
+
+    sample_type.reload
+    assert sample_type.valid?
+
+    # Changing unit
+    sample_type = patient_sample.sample_type
+    assert sample_type.valid?
+    attr = sample_type.sample_attributes.detect { |t| t.accessor_name == 'weight' }
+    attr.unit = FactoryBot.create(:unit)
+    refute sample_type.valid?
+    assert sample_type.errors.added?(:'sample_attributes.unit', 'cannot be changed (weight)')
+  end
+
   private
 
   # sample type with 3 samples