From fd0e6d1b36b18b1884acccff6bba1f8bb92fb467 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Fri, 23 Mar 2018 15:26:09 +0100 Subject: [PATCH] Fixed issue #1838 - Follow up issue #1752: Unable to migrate old object manager attribute fields. --- .../object_manager_attributes_controller.rb | 147 +++++++++--------- ...80220171219_check_for_object_attributes.rb | 37 +++++ .../check_for_object_attributes_spec.rb | 120 ++++++++++++++ spec/factories/object_manager_attribute.rb | 75 +++++++++ spec/support/db_migration.rb | 34 ++++ spec/support/system_init_done.rb | 9 ++ 6 files changed, 350 insertions(+), 72 deletions(-) create mode 100644 db/migrate/20180220171219_check_for_object_attributes.rb create mode 100644 spec/db/migrate/check_for_object_attributes_spec.rb create mode 100644 spec/factories/object_manager_attribute.rb create mode 100644 spec/support/db_migration.rb create mode 100644 spec/support/system_init_done.rb diff --git a/app/controllers/object_manager_attributes_controller.rb b/app/controllers/object_manager_attributes_controller.rb index 34361f174..a61e8516b 100644 --- a/app/controllers/object_manager_attributes_controller.rb +++ b/app/controllers/object_manager_attributes_controller.rb @@ -22,55 +22,51 @@ class ObjectManagerAttributesController < ApplicationController # POST /object_manager_attributes def create - check_params - # check if attribute already exists exists = ObjectManager::Attribute.get( - object: params[:object], - name: params[:name], + object: permitted_params[:object], + name: permitted_params[:name], ) raise Exceptions::UnprocessableEntity, 'already exists' if exists - local_params = params.permit!.to_h begin object_manager_attribute = ObjectManager::Attribute.add( - object: local_params[:object], - name: local_params[:name], - display: local_params[:display], - data_type: local_params[:data_type], - data_option: local_params[:data_option], - active: local_params[:active], - screens: local_params[:screens], + object: permitted_params[:object], + name: permitted_params[:name], + display: permitted_params[:display], + data_type: permitted_params[:data_type], + data_option: permitted_params[:data_option], + active: permitted_params[:active], + screens: permitted_params[:screens], position: 1550, editable: true, ) render json: object_manager_attribute.attributes_with_association_ids, status: :created rescue => e + logger.error e raise Exceptions::UnprocessableEntity, e end end # PUT /object_manager_attributes/1 def update - check_params - local_params = params.permit!.to_h - begin - object_manager_attribute = ObjectManager::Attribute.add( - object: local_params[:object], - name: local_params[:name], - display: local_params[:display], - data_type: local_params[:data_type], - data_option: local_params[:data_option], - active: local_params[:active], - screens: local_params[:screens], - position: 1550, - editable: true, - ) - render json: object_manager_attribute.attributes_with_association_ids, status: :ok - rescue => e - raise Exceptions::UnprocessableEntity, e - end + object_manager_attribute = ObjectManager::Attribute.add( + object: permitted_params[:object], + name: permitted_params[:name], + display: permitted_params[:display], + data_type: permitted_params[:data_type], + data_option: permitted_params[:data_option], + active: permitted_params[:active], + screens: permitted_params[:screens], + position: 1550, + editable: true, + ) + render json: object_manager_attribute.attributes_with_association_ids, status: :ok + rescue => e + logger.error e + raise Exceptions::UnprocessableEntity, e + end # DELETE /object_manager_attributes/1 @@ -97,50 +93,57 @@ class ObjectManagerAttributesController < ApplicationController private - def check_params - if params[:data_type].match?(/^(boolean)$/) - if params[:data_option][:options] - # rubocop:disable Lint/BooleanSymbol - if params[:data_option][:options][:false] - params[:data_option][:options][false] = params[:data_option][:options].delete(:false) + def permitted_params + @permitted_params ||= begin + permitted = params.permit!.to_h + + if permitted[:data_type].match?(/^(boolean)$/) + if permitted[:data_option][:options] + # rubocop:disable Lint/BooleanSymbol + if permitted[:data_option][:options][:false] + permitted[:data_option][:options][false] = permitted[:data_option][:options].delete(:false) + end + if permitted[:data_option][:options][:true] + permitted[:data_option][:options][true] = permitted[:data_option][:options].delete(:true) + end + if permitted[:data_option][:default] == 'true' + permitted[:data_option][:default] = true + elsif permitted[:data_option][:default] == 'false' + permitted[:data_option][:default] = false + end + # rubocop:enable Lint/BooleanSymbol end - if params[:data_option][:options][:true] - params[:data_option][:options][true] = params[:data_option][:options].delete(:true) - end - if params[:data_option][:default] == 'true' - params[:data_option][:default] = true - elsif params[:data_option][:default] == 'false' - params[:data_option][:default] = false - end - # rubocop:enable Lint/BooleanSymbol end + + if permitted[:data_option] + + if !permitted[:data_option].key?(:default) + permitted[:data_option][:default] = if permitted[:data_type].match?(/^(input|select|tree_select)$/) + '' + end + end + + if permitted[:data_option][:null].nil? + permitted[:data_option][:null] = true + end + + if !permitted[:data_option][:options].is_a?(Hash) + permitted[:data_option][:options] = {} + end + + if !permitted[:data_option][:relation].is_a?(String) + permitted[:data_option][:relation] = '' + end + else + permitted[:data_option] = { + default: '', + options: {}, + relation: '', + null: true + } + end + + permitted end - - if params[:data_option] - - if !params[:data_option].key?(:default) - params[:data_option][:default] = if params[:data_type].match?(/^(input|select|tree_select)$/) - '' - end - end - - if params[:data_option][:null].nil? - params[:data_option][:null] = true - end - if params[:data_option][:options].nil? - params[:data_option][:options] = '' - end - if params[:data_option][:relation].nil? - params[:data_option][:relation] = '' - end - else - params[:data_option] = { - default: '', - options: '', - relation: '', - null: true - } - end - end end diff --git a/db/migrate/20180220171219_check_for_object_attributes.rb b/db/migrate/20180220171219_check_for_object_attributes.rb new file mode 100644 index 000000000..ec2ebe05a --- /dev/null +++ b/db/migrate/20180220171219_check_for_object_attributes.rb @@ -0,0 +1,37 @@ +class CheckForObjectAttributes < ActiveRecord::Migration[5.1] + def change + return if !Setting.find_by(name: 'system_init_done') + + attributes.each do |attribute| + + fix_nil_data_option(attribute) + fix_options(attribute) + fix_relation(attribute) + + next if !attribute.changed? + + attribute.save! + end + end + + private + + def attributes + ObjectManager::Attribute.all + end + + def fix_nil_data_option(attribute) + return if attribute[:data_option].is_a?(Hash) + attribute[:data_option] = {} + end + + def fix_options(attribute) + return if attribute[:data_option][:options].is_a?(Hash) + attribute[:data_option][:options] = {} + end + + def fix_relation(attribute) + return if attribute[:data_option][:relation].is_a?(String) + attribute[:data_option][:relation] = '' + end +end diff --git a/spec/db/migrate/check_for_object_attributes_spec.rb b/spec/db/migrate/check_for_object_attributes_spec.rb new file mode 100644 index 000000000..45cd23889 --- /dev/null +++ b/spec/db/migrate/check_for_object_attributes_spec.rb @@ -0,0 +1,120 @@ +require 'rails_helper' + +RSpec.describe CheckForObjectAttributes, type: :db_migration do + + it 'performs no action for new systems' do + system_init_done(false) + + migrate do |instance| + expect(instance).not_to receive(:attributes) + end + end + + context 'valid [:data_option]' do + + it 'does not change converted text attribute' do + system_init_done + + attribute = create(:object_manager_attribute_text) + + expect do + migrate + end.not_to change { + attribute.reload.data_option + } + end + + it 'does not change select attribute' do + system_init_done + + attribute = create(:object_manager_attribute_select) + + expect do + migrate + end.not_to change { + attribute.reload.data_option + } + end + end + + context '[:data_option]' do + + it 'ensures an empty Hash' do + system_init_done + + 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 + system_init_done + + 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 + system_init_done + + wrong = { + default: '', + options: '', + relation: '', + 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 + system_init_done + + wrong = { + default: '', + options: {}, + null: true + } + + 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 + system_init_done + + wrong = { + default: '', + options: {}, + relation: {}, + 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 +end diff --git a/spec/factories/object_manager_attribute.rb b/spec/factories/object_manager_attribute.rb new file mode 100644 index 000000000..0a644dcae --- /dev/null +++ b/spec/factories/object_manager_attribute.rb @@ -0,0 +1,75 @@ +FactoryBot.define do + sequence :object_manager_attribute_name do |n| + "internal_name#{n}" + end + + sequence :object_manager_attribute_display do |n| + "Display Name #{n}" + end +end + +FactoryBot.define do + factory :object_manager_attribute, class: ObjectManager::Attribute do + + object_lookup_id 2 + name { generate(:object_manager_attribute_name) } + display { generate(:object_manager_attribute_display) } + data_option_new do + {} + end + editable false + active true + screens do + { + 'create_top' => { + '-all-' => { + 'null' => false + } + }, + 'edit' => {} + } + end + add_attribute(:to_create) { false } + to_migrate false + to_delete false + to_config false + position 15 + updated_by_id 1 + created_by_id 1 + end + + factory :object_manager_attribute_text, parent: :object_manager_attribute do + data_type 'input' + data_option do + { + 'type' => 'text', + 'maxlength' => 200, + 'null' => true, + 'translate' => false, + 'default' => '', + 'options' => {}, + 'relation' => '', + } + end + end + + factory :object_manager_attribute_select, parent: :object_manager_attribute do + data_type 'select' + data_option do + { + 'default' => '', + 'options' => { + 'key_1' => 'value_1', + 'key_2' => 'value_2', + 'key_3' => 'value_3', + }, + 'relation' => '', + 'nulloption' => true, + 'multiple' => false, + 'null' => true, + 'translate' => true, + 'maxlength' => 255 + } + end + end +end diff --git a/spec/support/db_migration.rb b/spec/support/db_migration.rb new file mode 100644 index 000000000..6efcc302d --- /dev/null +++ b/spec/support/db_migration.rb @@ -0,0 +1,34 @@ +# require all database migrations so we can test them without manual require +Rails.root.join('db', 'migrate').children.each do |migration| + require migration.to_s +end + +module DbMigrationHelper + + # Provides a helper method to execute a migration for the current class. + # Make sure to define type: :db_migration in your RSpec.describe call. + # + # @param [Symbol] direction the migration should take (:up or :down) + # @yield [instance] Yields the created instance of the + # migration to allow expectations or other changes to it + # + # @example + # migrate + + # @example + # migrate(:down) + # + # @return [nil] + def migrate(direction = :up) + instance = described_class.new + yield(instance) if block_given? + + instance.suppress_messages do + instance.migrate(direction) + end + end +end + +RSpec.configure do |config| + config.include DbMigrationHelper, type: :db_migration +end diff --git a/spec/support/system_init_done.rb b/spec/support/system_init_done.rb new file mode 100644 index 000000000..ffadc12e8 --- /dev/null +++ b/spec/support/system_init_done.rb @@ -0,0 +1,9 @@ +module SystemInitDoneHelper + def system_init_done(state = true) + expect(Setting).to receive(:find_by).with(name: 'system_init_done').and_return(state) + end +end + +RSpec.configure do |config| + config.include SystemInitDoneHelper +end