Fixed issue #2099 - Changing data type of new object attributes will lead to errors.

app/assets/javascripts/app/controllers/_ui_element/object_manager_attribute.coffee

@@ -28,6 +28,12 @@ class App.UiElement.object_manager_attribute extends App.UiElement.ApplicationUi
       boolean: 'Boolean'
       integer: 'Integer'
 
+    # if attribute already exists, do not allow to change it anymore
+    if params.data_type
+      for key, value of options
+        if key isnt params.data_type
+          delete options[key]
+
     configureAttributes = [
       { name: attribute.name, display: '', tag: 'select', null: false, options: options, translate: true, default: 'input', disabled: attribute.disabled },
     ]

app/models/object_manager/attribute.rb

@@ -12,6 +12,9 @@ class ObjectManager::Attribute < ApplicationModel
   store :data_option
   store :data_option_new
 
+  before_create :check_datatype
+  before_update :check_datatype, :verify_possible_type_change
+
 =begin
 
 list of all attributes
@@ -324,7 +327,6 @@ possible types
         record.check_editable
         record.check_name
       end
-      record.check_datatype
       record.save!
       return record
     end
@@ -344,7 +346,6 @@ possible types
       record.check_editable
       record.check_name
     end
-    record.check_datatype
     record.save!
     record
   end
@@ -878,7 +879,13 @@ is certain attribute used by triggers, overviews or schedulers
     raise 'Attribute not editable!'
   end
 
+  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
@@ -886,62 +893,76 @@ is certain attribute used by triggers, overviews or schedulers
       raise "Invalid data_type param '#{data_type}'"
     end
 
-    if !data_option
-      raise 'Need data_type param'
+    if local_data_option.blank?
+      raise 'Need data_option param'
     end
-    if data_option[:null].nil?
+    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 !data_option[:type]
-      raise "Invalid data_option[:type] param '#{data_option[:type]}' (text|password|tel|fax|email|url)" if data_option[:type] !~ /^(text|password|tel|fax|email|url)$/
-      raise 'Need data_option[:maxlength] param' if !data_option[:maxlength]
-      raise "Invalid data_option[:maxlength] param #{data_option[:maxlength]}" if data_option[:maxlength].to_s !~ /^\d+?$/
+      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 !data_option[:maxlength]
-      raise "Invalid data_option[:maxlength] param #{data_option[:maxlength]}" if data_option[:maxlength].to_s !~ /^\d+?$/
+      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 !data_option[item]
-        raise "Invalid data_option[#{item.inspect}] param #{data_option[item]}" if data_option[item].to_s !~ /^\d+?$/
+        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 !data_option.key?(:default)
-      raise 'Invalid data_option[:options] or data_option[:relation] param' if data_option[:options].nil? && data_option[:relation].nil?
-      if !data_option.key?(:maxlength)
-        data_option[:maxlength] = 255
+      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 !data_option.key?(:nulloption)
-        data_option[:nulloption] = true
+      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 !data_option.key?(:default)
-      raise 'Invalid data_option[:options] param' if data_option[:options].nil?
+      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 data_option[:future].nil?
-      raise 'Need data_option[:past] param true|false' if data_option[:past].nil?
-      raise 'Need data_option[:diff] param in hours' if data_option[:diff].nil?
+      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 data_option[:future].nil?
-      raise 'Need data_option[:past] param true|false' if data_option[:past].nil?
-      raise 'Need data_option[:diff] param in days' if data_option[:diff].nil?
+      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?
+
+    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],
+    }
+
+    return true if possible[changes_to_save['data_type'][0]]&.include?(changes_to_save['data_type'][1])
+
+    raise 'Can\'t be changed data_type of attribute. Drop the attribute and recreate it with new data_type.'
+  end
 end

spec/db/migrate/check_for_object_attributes_spec.rb

@@ -41,33 +41,16 @@ RSpec.describe CheckForObjectAttributes, type: :db_migration do
     end
   end
 
-  context '[:data_option]' do
-
-    it 'ensures an empty Hash' do
-      attribute = create(:object_manager_attribute_text, data_option: nil)
-      migrate
-      attribute.reload
-
-      expect(attribute[:data_option]).to be_a(Hash)
-    end
-  end
-
   context '[:data_option][:options]' do
 
-    it 'ensures an empty Hash' do
-      attribute = create(:object_manager_attribute_text, data_option: {})
-      migrate
-      attribute.reload
-
-      expect(attribute[:data_option][:options]).to be_a(Hash)
-    end
-
     it 'converts String to Hash' do
       wrong = {
-        default:  '',
-        options:  '',
-        relation: '',
-        null:     true
+        default:   '',
+        options:   '',
+        relation:  '',
+        type:      'text',
+        maxlength: 255,
+        null:      true
       }
 
       attribute = create(:object_manager_attribute_text, data_option: wrong)
@@ -83,9 +66,11 @@ RSpec.describe CheckForObjectAttributes, type: :db_migration do
 
     it 'ensures an empty String' do
       wrong = {
-        default: '',
-        options: {},
-        null:    true
+        default:   '',
+        options:   {},
+        type:      'text',
+        maxlength: 255,
+        null:      true
       }
 
       attribute = create(:object_manager_attribute_text, data_option: wrong)
@@ -97,10 +82,12 @@ RSpec.describe CheckForObjectAttributes, type: :db_migration do
 
     it 'converts Hash to String' do
       wrong = {
-        default:  '',
-        options:  {},
-        relation: {},
-        null:     true
+        default:   '',
+        options:   {},
+        relation:  {},
+        type:      'text',
+        maxlength: 255,
+        null:      true
       }
 
       attribute = create(:object_manager_attribute_text, data_option: wrong)

spec/db/migrate/issue_1660_fix_tree_select_configurations_spec.rb

@@ -175,7 +175,7 @@ RSpec.describe Issue1660FixTreeSelectConfigurations, type: :db_migration do
       }
     ]
 
-    attribute = create(:object_manager_attribute_tree_select, data_option: { options: broken })
+    attribute = create(:object_manager_attribute_tree_select, data_option: { options: broken, null: true, default: '' })
 
     expect do
       migrate
@@ -192,18 +192,8 @@ RSpec.describe Issue1660FixTreeSelectConfigurations, type: :db_migration do
     end
   end
 
-  it 'skips blank data_option' do
-    attribute = create(:object_manager_attribute_tree_select, data_option: {})
-
-    expect do
-      migrate
-    end.not_to change {
-      attribute.reload.data_option
-    }
-  end
-
   it 'skips blank data_option options' do
-    attribute = create(:object_manager_attribute_tree_select, data_option: { options: [] })
+    attribute = create(:object_manager_attribute_tree_select, data_option: { options: [], null: true, default: '' })
 
     expect do
       migrate

test/browser_test_helper.rb

@@ -3601,19 +3601,22 @@ wait untill text in selector disabppears
     data     = params[:data]
 
     click(
-      browser: instance,
-      css:  'a[href="#manage"]',
+      browser:  instance,
+      css:      'a[href="#manage"]',
       mute_log: true,
     )
     click(
-      browser: instance,
-      css:  '.content.active a[href="#system/object_manager"]',
+      browser:  instance,
+      css:      '.content.active a[href="#system/object_manager"]',
       mute_log: true,
     )
-    sleep 4
-    click(
+    watch_for(
       browser: instance,
-      css:  '.content.active .js-new',
+      css:     '.content.active .js-new',
+    )
+    click(
+      browser:  instance,
+      css:      '.content.active .js-new',
       mute_log: true,
     )
     modal_ready(browser: instance)
@@ -3736,17 +3739,19 @@ wait untill text in selector disabppears
     data     = params[:data]
 
     click(
-      browser: instance,
-      css:  'a[href="#manage"]',
+      browser:  instance,
+      css:      'a[href="#manage"]',
       mute_log: true,
     )
     click(
-      browser: instance,
-      css:  '.content.active a[href="#system/object_manager"]',
+      browser:  instance,
+      css:      '.content.active a[href="#system/object_manager"]',
       mute_log: true,
     )
-    sleep 4
-
+    watch_for(
+      browser: instance,
+      css:     '.content.active .js-new',
+    )
     instance.execute_script("$(\".content.active td:contains('#{data[:name]}')\").first().click()")
     modal_ready(browser: instance)
     element = instance.find_elements(css: '.modal input[name=display]')[0]
@@ -3758,6 +3763,12 @@ wait untill text in selector disabppears
       value:    data[:data_type],
       mute_log: true,
     )
+
+    # if attribute is created, do not be able to select other types anymore
+    if instance.find_elements(css: '.modal select[name="data_type"] option').count > 1
+      assert(false, 'able to change the data_type of existing attribute which should not be allowed')
+    end
+
     if data[:data_option]
       if data[:data_option][:options]
         if data[:data_type] == 'Boolean'

test/integration/object_manager_attributes_controller_test.rb

@@ -1014,4 +1014,138 @@ class ObjectManagerAttributesControllerTest < ActionDispatch::IntegrationTest
     assert @response.body.include?('cannot be deleted!')
   end
 
+  test '07 verify if attribute type can not be changed' do
+    credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-admin@example.com', 'adminpw')
+
+    params = {
+      'name': "customerdescription_#{rand(999_999_999)}",
+      'object': 'Ticket',
+      'display': "custom description #{rand(999_999_999)}",
+      'active': true,
+      'data_type': 'boolean',
+      'data_option': {
+        'options': {
+          'true': '',
+          'false': '',
+        },
+        'default': 'false',
+        'screens': {
+          'create_middle': {
+            'ticket.customer': {
+              'shown': true,
+              'item_class': 'column'
+            },
+            'ticket.agent': {
+              'shown': true,
+              'item_class': 'column'
+            }
+          },
+          'edit': {
+            'ticket.customer': {
+              'shown': true
+            },
+            'ticket.agent': {
+              'shown': true
+            }
+          }
+        }
+      },
+    }
+
+    post '/api/v1/object_manager_attributes', params: params.to_json, headers: @headers.merge('Authorization' => credentials)
+
+    assert_response(201) # created
+    result = JSON.parse(@response.body)
+
+    assert(result)
+    assert_not(result['data_option']['default'])
+    assert_equal(result['data_option']['default'], false)
+    assert_equal(result['data_type'], 'boolean')
+
+    migration = ObjectManager::Attribute.migration_execute
+    assert_equal(migration, true)
+
+    params['data_type'] = 'input'
+    params['data_option'] = {
+      'default': 'test',
+      'type': 'text',
+      'maxlength': 120
+    }
+
+    put "/api/v1/object_manager_attributes/#{result['id']}", params: params.to_json, headers: @headers.merge('Authorization' => credentials)
+    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.'])
+
+  end
+
+  test '08 verify if attribute type can be changed' do
+    credentials = ActionController::HttpAuthentication::Basic.encode_credentials('tickets-admin@example.com', 'adminpw')
+
+    params = {
+      'name': "customerdescription_#{rand(999_999_999)}",
+      'object': 'Ticket',
+      'display': "custom description #{rand(999_999_999)}",
+      'active': true,
+      'data_type': 'input',
+      'data_option': {
+        'default': 'test',
+        'type': 'text',
+        'maxlength': 120,
+      },
+      'screens': {
+        'create_middle': {
+          'ticket.customer': {
+            'shown': true,
+            'item_class': 'column'
+          },
+          'ticket.agent': {
+            'shown': true,
+            'item_class': 'column'
+          }
+        },
+        'edit': {
+          'ticket.customer': {
+            'shown': true
+          },
+          'ticket.agent': {
+            'shown': true
+          }
+        },
+      },
+    }
+
+    post '/api/v1/object_manager_attributes', params: params.to_json, headers: @headers.merge('Authorization' => credentials)
+
+    assert_response(201) # created
+    result = JSON.parse(@response.body)
+
+    assert(result)
+    assert_equal(result['data_option']['default'], 'test')
+    assert_equal(result['data_type'], 'input')
+
+    migration = ObjectManager::Attribute.migration_execute
+    assert_equal(migration, true)
+
+    params['data_type'] = 'select'
+    params['data_option'] = {
+      'default': 'fuu',
+      'options': {
+        'key1': 'foo',
+        'key2': 'fuu',
+      }
+    }
+
+    put "/api/v1/object_manager_attributes/#{result['id']}", params: params.to_json, headers: @headers.merge('Authorization' => credentials)
+
+    assert_response(200)
+    result = JSON.parse(@response.body)
+    assert(result)
+    assert_equal(result['data_option']['default'], 'test')
+    assert_equal(result['data_option_new']['default'], 'fuu')
+    assert_equal(result['data_type'], 'select')
+
+  end
+
 end

test/integration/object_manager_test.rb

@@ -788,4 +788,83 @@ class ObjectManagerTest < ActiveSupport::TestCase
     assert_equal(1, overview[:count])
     assert_equal(ticket1.id, overview[:tickets][0][:id])
   end
+
+  test 'd object manager attribute - update attribute type' do
+
+    attribute1 = ObjectManager::Attribute.add(
+      object: 'Ticket',
+      name: 'example_1',
+      display: 'example_1',
+      data_type: 'input',
+      data_option: {
+        default: '',
+        maxlength: 200,
+        type: 'text',
+        null: true,
+        options: {},
+      },
+      active: true,
+      screens: {},
+      position: 20,
+      created_by_id: 1,
+      updated_by_id: 1,
+    )
+
+    assert_equal(true, ObjectManager::Attribute.pending_migration?)
+    assert_equal(1, ObjectManager::Attribute.migrations.count)
+
+    assert(ObjectManager::Attribute.migration_execute)
+
+    assert_raises(RuntimeError) do
+      ObjectManager::Attribute.add(
+        object: 'Ticket',
+        name: 'example_1',
+        display: 'example_1',
+        data_type: 'boolean',
+        data_option: {
+          default: true,
+          options: {
+            true: 'Yes',
+            false: 'No',
+          },
+          null: false,
+        },
+        active: true,
+        screens: {},
+        position: 200,
+        created_by_id: 1,
+        updated_by_id: 1,
+      )
+    end
+
+    attribute2 = ObjectManager::Attribute.add(
+      object: 'Ticket',
+      name: 'example_1',
+      display: 'example_1',
+      data_type: 'select',
+      data_option: {
+        default: '',
+        maxlength: 200,
+        type: 'text',
+        null: true,
+        options: {
+          aa: 'aa',
+          bb: 'bb',
+        },
+      },
+      active: true,
+      screens: {},
+      position: 20,
+      created_by_id: 1,
+      updated_by_id: 1,
+    )
+
+    assert_equal(attribute1.id, attribute2.id)
+    assert_equal(true, ObjectManager::Attribute.pending_migration?)
+    assert_equal(1, ObjectManager::Attribute.migrations.count)
+
+    assert(ObjectManager::Attribute.migration_execute)
+
+  end
+
 end