From 40a663198c616e63ceabe53018ef437feddbe475 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 29 Jun 2018 08:14:18 +0200 Subject: [PATCH] Fixed issue #2099 - Changing data type of new object attributes will lead to errors. --- .../object_manager_attribute.coffee | 6 + app/models/object_manager/attribute.rb | 75 ++++++---- .../check_for_object_attributes_spec.rb | 47 +++--- ...660_fix_tree_select_configurations_spec.rb | 14 +- test/browser_test_helper.rb | 37 +++-- ...ject_manager_attributes_controller_test.rb | 134 ++++++++++++++++++ test/integration/object_manager_test.rb | 79 +++++++++++ 7 files changed, 310 insertions(+), 82 deletions(-) diff --git a/app/assets/javascripts/app/controllers/_ui_element/object_manager_attribute.coffee b/app/assets/javascripts/app/controllers/_ui_element/object_manager_attribute.coffee index 271ad8cd7..71fd11be6 100644 --- a/app/assets/javascripts/app/controllers/_ui_element/object_manager_attribute.coffee +++ b/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 }, ] diff --git a/app/models/object_manager/attribute.rb b/app/models/object_manager/attribute.rb index b9fec4173..0a4bcfc1c 100644 --- a/app/models/object_manager/attribute.rb +++ b/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 diff --git a/spec/db/migrate/check_for_object_attributes_spec.rb b/spec/db/migrate/check_for_object_attributes_spec.rb index bfb474194..55dc8517f 100644 --- a/spec/db/migrate/check_for_object_attributes_spec.rb +++ b/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) diff --git a/spec/db/migrate/issue_1660_fix_tree_select_configurations_spec.rb b/spec/db/migrate/issue_1660_fix_tree_select_configurations_spec.rb index 21df685ae..8e1871b08 100644 --- a/spec/db/migrate/issue_1660_fix_tree_select_configurations_spec.rb +++ b/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 diff --git a/test/browser_test_helper.rb b/test/browser_test_helper.rb index 2095ce1f5..75b22fafe 100644 --- a/test/browser_test_helper.rb +++ b/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' diff --git a/test/integration/object_manager_attributes_controller_test.rb b/test/integration/object_manager_attributes_controller_test.rb index 498c076b4..7b706efd2 100644 --- a/test/integration/object_manager_attributes_controller_test.rb +++ b/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 diff --git a/test/integration/object_manager_test.rb b/test/integration/object_manager_test.rb index a5f359807..e28cf1394 100644 --- a/test/integration/object_manager_test.rb +++ b/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