diff --git a/app/models/object_manager/attribute.rb b/app/models/object_manager/attribute.rb index 547595c5d..d348379ac 100644 --- a/app/models/object_manager/attribute.rb +++ b/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 + # 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 local_data_option.blank? - raise 'Need data_option param' + case data_type + when /^((tree_)?select|checkbox)$/ + local_data_option[:nulloption] = true if local_data_option[:nulloption].nil? + local_data_option[:maxlength] ||= 255 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 - - 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 - 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 - - 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 - - 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 - - true end - def verify_possible_type_change - return true if changes_to_save['data_type'].blank? + def data_option_must_have_appropriate_values + data_option_validations + .select { |validation| validation[:failed] } + .each { |validation| errors.add(local_data_attr, validation[:message]) } + 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 data_type_must_not_change + allowable_changes = %w[tree_select select input checkbox] - return true if possible[changes_to_save['data_type'][0]]&.include?(changes_to_save['data_type'][1]) + return if !data_type_changed? + return if (data_type_change - allowable_changes).empty? - raise 'Can\'t be changed data_type of attribute. Drop the attribute and recreate it with new data_type.' + errors.add(:data_type, "can't be altered after creation " \ + '(delete the attribute and create another with the desired value)') + end + + def local_data_option + @local_data_option ||= send(local_data_attr) + end + + def local_data_attr + @local_data_attr ||= to_config ? :data_option_new : :data_option + end + + def local_data_option=(val) + send("#{local_data_attr}=", val) + end + + 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 diff --git a/spec/db/migrate/check_for_object_attributes_spec.rb b/spec/db/migrate/check_for_object_attributes_spec.rb index 55dc8517f..7b60955b4 100644 --- a/spec/db/migrate/check_for_object_attributes_spec.rb +++ b/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 + expect { migrate } + .not_to change { attribute.reload.data_option } + end + end + + 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 + } + + attribute = create(:object_manager_attribute_text, data_option: wrong) migrate - end.not_to change { - attribute.reload.data_option - } - end - end + attribute.reload - context '[:data_option][:options]' do - - 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 - - expect(attribute[:data_option][:options]).to be_a(Hash) - expect(attribute[:data_option][:options]).to be_blank - end - end - - context '[:data_option][:relation]' do - - 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 - - expect(attribute[:data_option][:relation]).to be_a(String) + expect(attribute[:data_option][:options]).to be_a(Hash) + expect(attribute[:data_option][:options]).to be_blank + end end - it 'converts Hash to String' do - wrong = { - default: '', - options: {}, - relation: {}, - type: 'text', - maxlength: 255, - null: true - } + context ':relation' do - attribute = create(:object_manager_attribute_text, data_option: wrong) - migrate - attribute.reload + it 'ensures an empty String' do + wrong = { + default: '', + options: {}, + type: 'text', + maxlength: 255, + null: true + } - expect(attribute[:data_option][:relation]).to be_a(String) - expect(attribute[:data_option][:relation]).to be_blank + 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_blank + end + end + + # 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 diff --git a/spec/models/object_manager/attribute_spec.rb b/spec/models/object_manager/attribute_spec.rb new file mode 100644 index 000000000..6f7b285e7 --- /dev/null +++ b/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 diff --git a/test/integration/object_manager_attributes_controller_test.rb b/test/integration/object_manager_attributes_controller_test.rb index 7b706efd2..799b31ea8 100644 --- a/test/integration/object_manager_attributes_controller_test.rb +++ b/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 diff --git a/test/integration/object_manager_test.rb b/test/integration/object_manager_test.rb index 8bde45c19..725f2f56e 100644 --- a/test/integration/object_manager_test.rb +++ b/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',