Fixed issue #1838 - Follow up issue #1752: Unable to migrate old object manager attribute fields.

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

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

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

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

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

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