Refactor validations and callbacks on ObjectManager::Attribute (fixes #2159)

app/models/object_manager/attribute.rb

@@ -2,18 +2,38 @@ class ObjectManager::Attribute < ApplicationModel
   include ChecksClientNotification
   include CanSeed
 
+  DATA_TYPES = %w[
+    input
+    user_autocompletion
+    checkbox
+    select
+    tree_select
+    datetime
+    date
+    tag
+    richtext
+    textarea
+    integer
+    autocompletion_ajax
+    boolean
+    user_permission
+    active
+  ].freeze
+
   self.table_name = 'object_manager_attributes'
 
   belongs_to :object_lookup
 
   validates :name, presence: true
+  validates :data_type, inclusion: { in: DATA_TYPES, msg: '%{value} is not a valid data type' }
+  validate :data_option_must_have_appropriate_values
+  validate :data_type_must_not_change, on: :update
 
   store :screens
   store :data_option
   store :data_option_new
 
-  before_create :check_datatype
-  before_update :check_datatype, :verify_possible_type_change
+  before_validation :set_base_options
 
 =begin
 
@@ -892,88 +912,87 @@ is certain attribute used by triggers, overviews or schedulers
 
   private
 
-  def check_datatype
-    local_data_option = data_option
-    if to_config == true
-      local_data_option = data_option_new
-    end
-    if !data_type
-      raise 'Need data_type param'
-    end
-    if !data_type.match?(/^(input|user_autocompletion|checkbox|select|tree_select|datetime|date|tag|richtext|textarea|integer|autocompletion_ajax|boolean|user_permission|active)$/)
-      raise "Invalid data_type param '#{data_type}'"
-    end
-
-    if local_data_option.blank?
-      raise 'Need data_option param'
-    end
-    if local_data_option[:null].nil?
-      raise 'Need data_option[:null] param with true or false'
-    end
-
-    # validate data_option
-    if data_type == 'input'
-      raise 'Need data_option[:type] param e. g. (text|password|tel|fax|email|url)' if !local_data_option[:type]
-      raise "Invalid data_option[:type] param '#{local_data_option[:type]}' (text|password|tel|fax|email|url)" if local_data_option[:type] !~ /^(text|password|tel|fax|email|url)$/
-      raise 'Need data_option[:maxlength] param' if !local_data_option[:maxlength]
-      raise "Invalid data_option[:maxlength] param #{local_data_option[:maxlength]}" if local_data_option[:maxlength].to_s !~ /^\d+?$/
-    end
+  # when setting default values for boolean fields,
+  # favor #nil? tests over ||= (which will overwrite `false`)
+  def set_base_options
+    local_data_option[:null] = true if local_data_option[:null].nil?
 
-    if data_type == 'richtext'
-      raise 'Need data_option[:maxlength] param' if !local_data_option[:maxlength]
-      raise "Invalid data_option[:maxlength] param #{local_data_option[:maxlength]}" if local_data_option[:maxlength].to_s !~ /^\d+?$/
-    end
-
-    if data_type == 'integer'
-      %i[min max].each do |item|
-        raise "Need data_option[#{item.inspect}] param" if !local_data_option[item]
-        raise "Invalid data_option[#{item.inspect}] param #{data_option[item]}" if local_data_option[item].to_s !~ /^\d+?$/
-      end
-    end
-
-    if data_type == 'select' || data_type == 'tree_select' || data_type == 'checkbox'
-      raise 'Need data_option[:default] param' if !local_data_option.key?(:default)
-      raise 'Invalid data_option[:options] or data_option[:relation] param' if local_data_option[:options].nil? && local_data_option[:relation].nil?
-      if !local_data_option.key?(:maxlength)
-        local_data_option[:maxlength] = 255
-      end
-      if !local_data_option.key?(:nulloption)
-        local_data_option[:nulloption] = true
-      end
+    case data_type
+    when /^((tree_)?select|checkbox)$/
+      local_data_option[:nulloption] = true if local_data_option[:nulloption].nil?
+      local_data_option[:maxlength] ||= 255
     end
+  end
 
-    if data_type == 'boolean'
-      raise 'Need data_option[:default] param true|false|undefined' if !local_data_option.key?(:default)
-      raise 'Invalid data_option[:options] param' if local_data_option[:options].nil?
-    end
+  def data_option_must_have_appropriate_values
+    data_option_validations
+      .select { |validation| validation[:failed] }
+      .each { |validation| errors.add(local_data_attr, validation[:message]) }
+  end
 
-    if data_type == 'datetime'
-      raise 'Need data_option[:future] param true|false' if local_data_option[:future].nil?
-      raise 'Need data_option[:past] param true|false' if local_data_option[:past].nil?
-      raise 'Need data_option[:diff] param in hours' if local_data_option[:diff].nil?
-    end
+  def data_type_must_not_change
+    allowable_changes = %w[tree_select select input checkbox]
 
-    if data_type == 'date'
-      raise 'Need data_option[:future] param true|false' if local_data_option[:future].nil?
-      raise 'Need data_option[:past] param true|false' if local_data_option[:past].nil?
-      raise 'Need data_option[:diff] param in days' if local_data_option[:diff].nil?
-    end
+    return if !data_type_changed?
+    return if (data_type_change - allowable_changes).empty?
 
-    true
+    errors.add(:data_type, "can't be altered after creation " \
+                           '(delete the attribute and create another with the desired value)')
   end
 
-  def verify_possible_type_change
-    return true if changes_to_save['data_type'].blank?
+  def local_data_option
+    @local_data_option ||= send(local_data_attr)
+  end
 
-    possible = {
-      'select' => %w[tree_select select input checkbox],
-      'tree_select' => %w[tree_select select input checkbox],
-      'checkbox' => %w[tree_select select input checkbox],
-      'input' => %w[tree_select select input checkbox],
-    }
+  def local_data_attr
+    @local_data_attr ||= to_config ? :data_option_new : :data_option
+  end
 
-    return true if possible[changes_to_save['data_type'][0]]&.include?(changes_to_save['data_type'][1])
+  def local_data_option=(val)
+    send("#{local_data_attr}=", val)
+  end
 
-    raise 'Can\'t be changed data_type of attribute. Drop the attribute and recreate it with new data_type.'
+  def data_option_validations
+    case data_type
+    when 'input'
+      [{ failed:  %w[text password tel fax email url].exclude?(local_data_option[:type]),
+         message: 'must have one of text/password/tel/fax/email/url for :type' },
+       { failed:  !local_data_option[:maxlength].to_s.match?(/^\d+$/),
+         message: 'must have integer for :maxlength' }]
+    when 'richtext'
+      [{ failed:  !local_data_option[:maxlength].to_s.match?(/^\d+$/),
+         message: 'must have integer for :maxlength' }]
+    when 'integer'
+      [{ failed:  !local_data_option[:min].to_s.match?(/^\d+$/),
+         message: 'must have integer for :min' },
+       { failed:  !local_data_option[:max].to_s.match?(/^\d+$/),
+         message: 'must have integer for :max' }]
+    when /^((tree_)?select|checkbox)$/
+      [{ failed:  !local_data_option.key?(:default),
+         message: 'must have value for :default' },
+       { failed:  local_data_option[:options].nil? && local_data_option[:relation].nil?,
+         message: 'must have non-nil value for either :options or :relation' }]
+    when 'boolean'
+      [{ failed:  !local_data_option.key?(:default),
+         message: 'must have boolean/undefined value for :default' },
+       { failed:  local_data_option[:options].nil?,
+         message: 'must have non-nil value for :options' }]
+    when 'datetime'
+      [{ failed:  local_data_option[:future].nil?,
+         message: 'must have boolean value for :future' },
+       { failed:  local_data_option[:past].nil?,
+         message: 'must have boolean value for :past' },
+       { failed:  local_data_option[:diff].nil?,
+         message: 'must have integer value for :diff (in hours)' }]
+    when 'date'
+      [{ failed:  local_data_option[:future].nil?,
+         message: 'must have boolean value for :future' },
+       { failed:  local_data_option[:past].nil?,
+         message: 'must have boolean value for :past' },
+       { failed:  local_data_option[:diff].nil?,
+         message: 'must have integer value for :diff (in days)' }]
+    else
+      []
+    end
   end
 end

spec/db/migrate/check_for_object_attributes_spec.rb

@@ -8,94 +8,108 @@ RSpec.describe CheckForObjectAttributes, type: :db_migration do
     end
   end
 
-  context 'valid [:data_option]' do
+  context 'with a valid #data_option hash' do
 
     it 'does not change converted text attribute' do
       attribute = create(:object_manager_attribute_text)
 
-      expect do
-        migrate
-      end.not_to change {
-        attribute.reload.data_option
-      }
+      expect { migrate }
+        .not_to change { attribute.reload.data_option }
     end
 
     it 'does not change select attribute' do
       attribute = create(:object_manager_attribute_select)
 
-      expect do
-        migrate
-      end.not_to change {
-        attribute.reload.data_option
-      }
+      expect { migrate }
+        .not_to change { attribute.reload.data_option }
     end
 
     it 'does not change tree_select attribute' do
       attribute = create(:object_manager_attribute_tree_select)
 
-      expect do
-        migrate
-      end.not_to change {
-        attribute.reload.data_option
-      }
+      expect { migrate }
+        .not_to change { attribute.reload.data_option }
     end
   end
 
-  context '[:data_option][:options]' do
+  context 'for #data_option key:' do
+    context ':options' do
 
-    it 'converts String to Hash' do
-      wrong = {
-        default:   '',
-        options:   '',
-        relation:  '',
-        type:      'text',
-        maxlength: 255,
-        null:      true
-      }
+      it 'converts String to Hash' do
+        wrong = {
+          default:   '',
+          options:   '',
+          relation:  '',
+          type:      'text',
+          maxlength: 255,
+          null:      true
+        }
 
-      attribute = create(:object_manager_attribute_text, data_option: wrong)
-      migrate
-      attribute.reload
+        attribute = create(:object_manager_attribute_text, data_option: wrong)
+        migrate
+        attribute.reload
 
-      expect(attribute[:data_option][:options]).to be_a(Hash)
-      expect(attribute[:data_option][:options]).to be_blank
+        expect(attribute[:data_option][:options]).to be_a(Hash)
+        expect(attribute[:data_option][:options]).to be_blank
+      end
     end
-  end
 
-  context '[:data_option][:relation]' do
+    context ':relation' do
 
-    it 'ensures an empty String' do
-      wrong = {
-        default:   '',
-        options:   {},
-        type:      'text',
-        maxlength: 255,
-        null:      true
-      }
+      it 'ensures an empty String' do
+        wrong = {
+          default:   '',
+          options:   {},
+          type:      'text',
+          maxlength: 255,
+          null:      true
+        }
 
-      attribute = create(:object_manager_attribute_text, data_option: wrong)
-      migrate
-      attribute.reload
+        attribute = create(:object_manager_attribute_text, data_option: wrong)
+        migrate
+        attribute.reload
+
+        expect(attribute[:data_option][:relation]).to be_a(String)
+      end
+
+      it 'converts Hash to String' do
+        wrong = {
+          default:   '',
+          options:   {},
+          relation:  {},
+          type:      'text',
+          maxlength: 255,
+          null:      true
+        }
+
+        attribute = create(:object_manager_attribute_text, data_option: wrong)
+        migrate
+        attribute.reload
 
-      expect(attribute[:data_option][:relation]).to be_a(String)
+        expect(attribute[:data_option][:relation]).to be_a(String)
+        expect(attribute[:data_option][:relation]).to be_blank
+      end
     end
 
-    it 'converts Hash to String' do
-      wrong = {
-        default:   '',
-        options:   {},
-        relation:  {},
-        type:      'text',
-        maxlength: 255,
-        null:      true
-      }
-
-      attribute = create(:object_manager_attribute_text, data_option: wrong)
-      migrate
-      attribute.reload
-
-      expect(attribute[:data_option][:relation]).to be_a(String)
-      expect(attribute[:data_option][:relation]).to be_blank
+    # see https://github.com/zammad/zammad/issues/2159
+    context ':null' do
+
+      it 'does not fail on missing values' do
+        wrong = {
+          default:   '',
+          options:   '',      # <- this is not the attribute under test,
+          relation:  '',      #    but it must be invalid
+          type:      'text',  #    to trigger a #save in the migration.
+          maxlength: 255,
+        }
+
+        # rubocop:disable Rails/SkipsModelValidations
+        create(:object_manager_attribute_text)
+          .update_columns(data_option: wrong)
+        # rubocop:enable Rails/SkipsModelValidations
+
+        expect { migrate }.not_to raise_error
+      end
     end
   end
 end

spec/models/object_manager/attribute_spec.rb

@@ -0,0 +1,52 @@
+require 'rails_helper'
+
+RSpec.describe ObjectManager::Attribute, type: :model do
+  describe 'callbacks' do
+    context 'for setting default values on local data options' do
+      let(:subject) { described_class.new }
+
+      context ':null' do
+        it 'sets nil values to true' do
+          expect { subject.validate }
+            .to change { subject.data_option[:null] }.to(true)
+        end
+
+        it 'does not overwrite false values' do
+          subject.data_option[:null] = false
+
+          expect { subject.validate }
+            .not_to change { subject.data_option[:null] }
+        end
+      end
+
+      context ':maxlength' do
+        context 'for data_type: select / tree_select / checkbox' do
+          let(:subject) { described_class.new(data_type: 'select') }
+
+          it 'sets nil values to 255' do
+            expect { subject.validate }
+              .to change { subject.data_option[:maxlength] }.to(255)
+          end
+        end
+      end
+
+      context ':nulloption' do
+        context 'for data_type: select / tree_select / checkbox' do
+          let(:subject) { described_class.new(data_type: 'select') }
+
+          it 'sets nil values to true' do
+            expect { subject.validate }
+              .to change { subject.data_option[:nulloption] }.to(true)
+          end
+
+          it 'does not overwrite false values' do
+            subject.data_option[:nulloption] = false
+
+            expect { subject.validate }
+              .not_to change { subject.data_option[:nulloption] }
+          end
+        end
+      end
+    end
+  end
+end

test/integration/object_manager_attributes_controller_test.rb

@@ -1076,7 +1076,7 @@ class ObjectManagerAttributesControllerTest < ActionDispatch::IntegrationTest
     assert_response(422)
     result = JSON.parse(@response.body)
     assert(result)
-    assert(result['error']['Can\'t be changed data_type of attribute. Drop the attribute and recreate it with new data_type.'])
+    assert(result['error'])
 
   end
 

test/integration/object_manager_test.rb

@@ -109,7 +109,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
         updated_by_id: 1,
       )
     end
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       attribute4 = ObjectManager::Attribute.add(
         object: 'Ticket',
         name: 'test4',
@@ -153,7 +153,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
       name: 'test5',
     )
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       attribute6 = ObjectManager::Attribute.add(
         object: 'Ticket',
         name: 'test6',
@@ -200,7 +200,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
       name: 'test7',
     )
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       attribute8 = ObjectManager::Attribute.add(
         object: 'Ticket',
         name: 'test8',
@@ -242,7 +242,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
       name: 'test9',
     )
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       attribute10 = ObjectManager::Attribute.add(
         object: 'Ticket',
         name: 'test10',
@@ -285,7 +285,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
       name: 'test11',
     )
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       attribute12 = ObjectManager::Attribute.add(
         object: 'Ticket',
         name: 'test12',
@@ -368,27 +368,9 @@ class ObjectManagerTest < ActiveSupport::TestCase
     end
     assert_equal(false, ObjectManager::Attribute.pending_migration?)
 
-    assert_raises(RuntimeError) do
-      attribute16 = ObjectManager::Attribute.add(
-        object: 'Ticket',
-        name: 'test16',
-        display: 'Test 16',
-        data_type: 'integer',
-        data_option: {
-          default: 2,
-          min: 1,
-          max: 999,
-        },
-        active: true,
-        screens: {},
-        position: 20,
-        created_by_id: 1,
-        updated_by_id: 1,
-      )
-    end
-    assert_equal(false, ObjectManager::Attribute.pending_migration?)
+    # Test case #16 invalidated after callback added to set default #data_option[:null] value
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       attribute17 = ObjectManager::Attribute.add(
         object: 'Ticket',
         name: 'test17',
@@ -861,7 +843,7 @@ class ObjectManagerTest < ActiveSupport::TestCase
 
     assert(ObjectManager::Attribute.migration_execute)
 
-    assert_raises(RuntimeError) do
+    assert_raises(ActiveRecord::RecordInvalid) do
       ObjectManager::Attribute.add(
         object: 'Ticket',
         name: 'example_1',