Fixed issue #1660 - ObjectManager Attribute of type Tree-Select mixes up children.

app/assets/javascripts/app/controllers/object_manager.coffee

@@ -1,9 +1,8 @@
 # coffeelint: disable=duplicate_key
 treeParams = (e, params) ->
   tree = []
-  lastLevel = 0
   lastLevels = []
-  valueLevels = []
+  previousValueLevels = []
 
   $(e.target).closest('.modal').find('.js-treeTable .js-key').each( ->
     $element = $(@)
@@ -19,20 +18,19 @@ treeParams = (e, params) ->
       lastLevels[level-1].children.push item
     else
       console.log('ERROR', item)
-    if level is 0
-      valueLevels = []
-    else if lastLevel is level
-      valueLevels.pop()
-    else if lastLevel > level
-      down = lastLevel - level
-      for count in [1..down]
-        valueLevels.pop()
-    if lastLevel <= level
-      valueLevels.push name
+
+    valueLevels = []
+    # add previous level values
+    if level > 0
+      for previousLevel in [0..level - 1]
+        valueLevels.push(previousValueLevels[previousLevel])
+
+    # add current level value
+    valueLevels.push(name)
 
     item.value = valueLevels.join('::')
     lastLevels[level] = item
-    lastLevel = level
+    previousValueLevels = valueLevels
 
   )
   if tree[0]

db/migrate/20180226085743_issue_1660_fix_tree_select_configurations.rb

@@ -0,0 +1,44 @@
+class Issue1660FixTreeSelectConfigurations < ActiveRecord::Migration[5.1]
+  def change
+
+    # return if it's a new setup
+    return if !Setting.find_by(name: 'system_init_done')
+
+    return if attributes.blank?
+    attributes.each do |attribute|
+
+      next if attribute.data_option.blank?
+      next if attribute.data_option[:options].blank?
+
+      fixed_options = fix(attribute.data_option[:options])
+
+      attribute.data_option[:options] = fixed_options
+
+      attribute.save!
+    end
+  end
+
+  private
+
+  def attributes
+    @attributes ||= ObjectManager::Attribute.where(data_type: 'tree_select')
+  end
+
+  def fix(options, namespace = nil)
+
+    options.tap do |ref|
+
+      ref.each do |option|
+
+        option_namespace = Array(namespace.dup)
+        option_namespace.push(option['name'])
+
+        option['value'] = option_namespace.join('::')
+
+        next if option['children'].blank?
+
+        option['children'] = fix(option['children'], option_namespace)
+      end
+    end
+  end
+end

script/build/test_slice_tests.sh

@@ -16,6 +16,7 @@ if [ "$LEVEL" == '1' ]; then
   rm test/browser/abb_one_group_test.rb
   rm test/browser/admin_channel_email_test.rb
   rm test/browser/admin_object_manager_test.rb
+  rm test/browser/admin_object_manager_tree_select_test.rb
   rm test/browser/admin_overview_test.rb
   rm test/browser/admin_role_test.rb
   # test/browser/agent_navigation_and_title_test.rb
@@ -79,6 +80,7 @@ elif [ "$LEVEL" == '2' ]; then
   # test/browser/abb_one_group_test.rb
   rm test/browser/admin_channel_email_test.rb
   rm test/browser/admin_object_manager_test.rb
+  rm test/browser/admin_object_manager_tree_select_test.rb
   rm test/browser/admin_overview_test.rb
   #rm test/browser/admin_role_test.rb
   rm test/browser/agent_navigation_and_title_test.rb
@@ -142,6 +144,7 @@ elif [ "$LEVEL" == '3' ]; then
   # test/browser/abb_one_group_test.rb
   rm test/browser/admin_channel_email_test.rb
   rm test/browser/admin_object_manager_test.rb
+  rm test/browser/admin_object_manager_tree_select_test.rb
   rm test/browser/admin_overview_test.rb
   rm test/browser/admin_role_test.rb
   rm test/browser/agent_navigation_and_title_test.rb
@@ -205,6 +208,7 @@ elif [ "$LEVEL" == '4' ]; then
   # test/browser/abb_one_group_test.rb
   rm test/browser/admin_channel_email_test.rb
   rm test/browser/admin_object_manager_test.rb
+  rm test/browser/admin_object_manager_tree_select_test.rb
   rm test/browser/admin_overview_test.rb
   rm test/browser/admin_role_test.rb
   rm test/browser/agent_navigation_and_title_test.rb
@@ -267,6 +271,7 @@ elif [ "$LEVEL" == '5' ]; then
   # test/browser/abb_one_group_test.rb
   # test/browser/admin_channel_email_test.rb
   # test/browser/admin_object_manager_test.rb
+  # test/browser/admin_object_manager_tree_select_test.rb
   # test/browser/admin_overview_test.rb
   rm test/browser/admin_role_test.rb
   rm test/browser/agent_navigation_and_title_test.rb
@@ -332,6 +337,7 @@ elif [ "$LEVEL" == '6' ]; then
   rm test/browser/abb_one_group_test.rb
   rm test/browser/admin_channel_email_test.rb
   rm test/browser/admin_object_manager_test.rb
+  rm test/browser/admin_object_manager_tree_select_test.rb
   rm test/browser/admin_overview_test.rb
   rm test/browser/admin_role_test.rb
   rm test/browser/agent_navigation_and_title_test.rb

spec/db/migrate/issue_1660_fix_tree_select_configurations_spec.rb

@@ -0,0 +1,224 @@
+require 'rails_helper'
+
+RSpec.describe Issue1660FixTreeSelectConfigurations, type: :db_migration do
+
+  it 'corrects broken data_option options' do
+
+    # as provided in issue #1775
+    expected = [
+      {
+        'name'     => 'Blaak',
+        'value'    => 'Blaak',
+        'children' => [
+          {
+            'name'     => 'BL.-1',
+            'value'    => 'Blaak::BL.-1',
+            'children' => [
+              {
+                'name'  => 'BL.-1.3',
+                'value' => 'Blaak::BL.-1::BL.-1.3',
+              },
+              {
+                'name'  => 'BL.-1.19',
+                'value' => 'Blaak::BL.-1::BL.-1.19',
+              }
+            ]
+          },
+          {
+            'name'     => 'BL.0',
+            'value'    => 'Blaak::BL.0',
+            'children' => [
+              {
+                'name'  => 'BL.00.10a',
+                'value' => 'Blaak::BL.0::BL.00.10a',
+              },
+              {
+                'name'  => 'BL.00.7',
+                'value' => 'Blaak::BL.0::BL.00.7',
+              },
+              {
+                'name'  => 'BL.01.18',
+                'value' => 'Blaak::BL.0::BL.01.18',
+              },
+              {
+                'name'  => 'BL.00.12',
+                'value' => 'Blaak::BL.0::BL.00.12',
+              },
+              {
+                'name'  => 'BL.e0.3',
+                'value' => 'Blaak::BL.0::BL.e0.3',
+              },
+            ]
+          },
+          {
+            'name'     => 'BL.1',
+            'value'    => 'Blaak::BL.1',
+            'children' => [
+              {
+                'name'  => 'BL.01.i',
+                'value' => 'Blaak::BL.1::BL.01.i',
+              },
+            ]
+          },
+          {
+            'name'     => 'BL.2',
+            'value'    => 'Blaak::BL.2',
+            'children' => [
+              {
+                'name'  => 'BL.02.2',
+                'value' => 'Blaak::BL.2::BL.02.2',
+              },
+              {
+                'name'  => 'BL.02.4',
+                'value' => 'Blaak::BL.2::BL.02.4',
+              },
+              {
+                'name'  => 'BL.02.5',
+                'value' => 'Blaak::BL.2::BL.02.5',
+              },
+              {
+                'name'  => 'BL.02.6',
+                'value' => 'Blaak::BL.2::BL.02.6',
+              },
+              {
+                'name'  => 'BL.02.7',
+                'value' => 'Blaak::BL.2::BL.02.7',
+              },
+            ]
+          },
+        ],
+      }
+    ]
+
+    broken = [
+      {
+        'name'     => 'Blaak',
+        'value'    => 'Blaak',
+        'children' => [
+          {
+            'name'     => 'BL.-1',
+            'value'    => 'Blaak::BL.-1',
+            'children' => [
+              {
+                'name'  => 'BL.-1.3',
+                'value' => 'Blaak::BL.2::BL.-1.3',
+              },
+              {
+                'name'  => 'BL.-1.19',
+                'value' => 'Blaak::BL.2::BL.-1.19',
+              }
+            ]
+          },
+          {
+            'name'     => 'BL.0',
+            'value'    => 'Blaak::BL.0',
+            'children' => [
+              {
+                'name'  => 'BL.00.10a',
+                'value' => 'Blaak::BL.2::BL.00.10a',
+              },
+              {
+                'name'  => 'BL.00.7',
+                'value' => 'Blaak::BL.2::BL.00.7',
+              },
+              {
+                'name'  => 'BL.01.18',
+                'value' => 'Blaak::BL.2::BL.01.18',
+              },
+              {
+                'name'  => 'BL.00.12',
+                'value' => 'Blaak::BL.2::BL.00.12',
+              },
+              {
+                'name'  => 'BL.e0.3',
+                'value' => 'Blaak::BL.2::BL.e0.3',
+              },
+            ]
+          },
+          {
+            'name'     => 'BL.1',
+            'value'    => 'Blaak::BL.1',
+            'children' => [
+              {
+                'name'  => 'BL.01.i',
+                'value' => 'Blaak::BL.2::BL.01.i',
+              },
+            ]
+          },
+          {
+            'name'     => 'BL.2',
+            'value'    => 'Blaak::BL.2',
+            'children' => [
+              {
+                'name'  => 'BL.02.2',
+                'value' => 'Blaak::BL.2::BL.02.2',
+              },
+              {
+                'name'  => 'BL.02.4',
+                'value' => 'Blaak::BL.2::BL.02.4',
+              },
+              {
+                'name'  => 'BL.02.5',
+                'value' => 'Blaak::BL.2::BL.02.5',
+              },
+              {
+                'name'  => 'BL.02.6',
+                'value' => 'Blaak::BL.2::BL.02.6',
+              },
+              {
+                'name'  => 'BL.02.7',
+                'value' => 'Blaak::BL.2::BL.02.7',
+              },
+            ]
+          },
+        ],
+      }
+    ]
+
+    attribute = create(:object_manager_attribute_tree_select, data_option: { options: broken })
+
+    expect do
+      migrate
+    end.to change {
+      attribute.reload.data_option[:options]
+    }
+
+    expect(attribute.data_option[:options]).to eq(expected)
+  end
+
+  it 'performs no action for new systems', system_init_done: false do
+    migrate do |instance|
+      expect(instance).not_to receive(:attributes)
+    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: [] })
+
+    expect do
+      migrate
+    end.not_to change {
+      attribute.reload.data_option[:options]
+    }
+  end
+
+  it 'does not change correct data_option options' do
+    attribute = create(:object_manager_attribute_tree_select)
+
+    expect do
+      migrate
+    end.not_to change {
+      attribute.reload.data_option[:options]
+    }
+  end
+end

test/browser/admin_object_manager_tree_select_test.rb

@@ -0,0 +1,221 @@
+require 'browser_test_helper'
+
+class AdminObjectManagerTreeSelectTest < TestCase
+
+  def test_basic_a
+
+    @browser = browser_instance
+    login(
+      username: 'master@example.com',
+      password: 'test',
+      url: browser_url,
+    )
+    tasks_close_all()
+
+    object_manager_attribute_create(
+      data: {
+        name:      'browser_test_tree_select1',
+        display:   'Browser Test TreeSelect1',
+        data_type: 'Tree Select',
+        data_option: {
+          options: {
+            'Incident' => {
+              'Hardware' => {
+                'Monitor'  => {},
+                'Mouse'    => {},
+                'Keyboard' => {},
+              },
+              'Softwareproblem' => {
+                'CRM' => {},
+                'EDI' => {},
+                'SAP' => {
+                  'Authentication' => {},
+                  'Not reachable'  => {},
+                },
+                'MS Office' => {
+                  'Excel'      => {},
+                  'PowerPoint' => {},
+                  'Word'       => {},
+                  'Outlook'    => {},
+                },
+              },
+            },
+            'Service request' => {
+              'New software requirement' => {},
+              'New hardware'             => {},
+              'Consulting'               => {},
+            },
+            'Change request' => {},
+          },
+        },
+      },
+    )
+
+    correct_options = [
+      {
+        'name'     => 'Incident',
+        'value'    => 'Incident',
+        'children' => [
+          {
+            'name'     => 'Hardware',
+            'value'    => 'Incident::Hardware',
+            'children' => [
+              {
+                'name'  => 'Monitor',
+                'value' => 'Incident::Hardware::Monitor'
+              },
+              {
+                'name'  => 'Mouse',
+                'value' => 'Incident::Hardware::Mouse'
+              },
+              {
+                'name'  => 'Keyboard',
+                'value' => 'Incident::Hardware::Keyboard'
+              }
+            ]
+          },
+          {
+            'name'     => 'Softwareproblem',
+            'value'    => 'Incident::Softwareproblem',
+            'children' => [
+              {
+                'name'  => 'CRM',
+                'value' => 'Incident::Softwareproblem::CRM'
+              },
+              {
+                'name' => 'EDI',
+                'value' => 'Incident::Softwareproblem::EDI'
+              },
+              {
+                'name'     => 'SAP',
+                'value'    => 'Incident::Softwareproblem::SAP',
+                'children' => [
+                  {
+                    'name'  => 'Authentication',
+                    'value' => 'Incident::Softwareproblem::SAP::Authentication'
+                  },
+                  {
+                    'name'  => 'Not reachable',
+                    'value' => 'Incident::Softwareproblem::SAP::Not reachable'
+                  }
+                ]
+              },
+              {
+                'name'     => 'MS Office',
+                'value'    => 'Incident::Softwareproblem::MS Office',
+                'children' => [
+                  {
+                    'name'  => 'Excel',
+                    'value' => 'Incident::Softwareproblem::MS Office::Excel'
+                  },
+                  {
+                    'name'  => 'PowerPoint',
+                    'value' => 'Incident::Softwareproblem::MS Office::PowerPoint'
+                  },
+                  {
+                    'name'  => 'Word',
+                    'value' => 'Incident::Softwareproblem::MS Office::Word'
+                  },
+                  {
+                    'name'  => 'Outlook',
+                    'value' => 'Incident::Softwareproblem::MS Office::Outlook'
+                  }
+                ]
+              }
+            ]
+          }
+        ]
+      },
+      {
+        'name'     => 'Service request',
+        'value'    => 'Service request',
+        'children' => [
+          {
+            'name'  => 'New software requirement',
+            'value' => 'Service request::New software requirement'
+          },
+          {
+            'name'  => 'New hardware',
+            'value' => 'Service request::New hardware'
+          },
+          {
+            'name'  => 'Consulting',
+            'value' => 'Service request::Consulting'
+          }
+        ]
+      },
+      {
+        'name'  => 'Change request',
+        'value' => 'Change request'
+      }
+    ]
+
+    created_attribute = ObjectManager::Attribute.last
+    assert_equal(correct_options, created_attribute.data_option[:options])
+
+    watch_for(
+      css: '.content.active',
+      value: 'Database Update required',
+    )
+    click(css: '.content.active .tab-pane.active div.js-execute')
+    watch_for(
+      css: '.modal',
+      value: 'restart',
+    )
+    watch_for_disappear(
+      css:     '.modal',
+      timeout: 120,
+    )
+    sleep 5
+    watch_for(
+      css: '.content.active',
+    )
+
+    # discard new attribute
+    click(css: 'a[href="#manage"]')
+    click(css: 'a[href="#system/object_manager"]')
+    watch_for(
+      css: '.content.active table',
+      value: 'browser_test_tree_select1',
+    )
+    match_not(
+      css: '.content.active',
+      value: 'Database Update required',
+    )
+    object_manager_attribute_delete(
+      data: {
+        name: 'browser_test_tree_select1',
+      },
+    )
+    watch_for(
+      css: '.content.active',
+      value: 'Database Update required',
+    )
+    watch_for(
+      css: '.content.active table',
+      value: 'browser_test_tree_select1',
+    )
+    click(css: '.content.active .tab-pane.active div.js-execute')
+    watch_for(
+      css: '.modal',
+      value: 'restart',
+    )
+    watch_for_disappear(
+      css:     '.modal',
+      timeout: 120,
+    )
+    sleep 5
+    watch_for(
+      css: '.content.active',
+    )
+    match_not(
+      css: '.content.active',
+      value: 'Database Update required',
+    )
+    match_not(
+      css: '.content.active table',
+      value: 'browser_test_tree_select1',
+    )
+  end
+
+end

test/browser_test_helper.rb

@@ -3362,6 +3362,11 @@ wait untill text in selector disabppears
           element.clear
           element.send_keys(data[:data_option][:options][:false])
           # rubocop:enable Lint/BooleanSymbol
+        elsif data[:data_type] == 'Tree Select'
+          add_tree_options(
+            instance: instance,
+            options:  data[:data_option][:options],
+          )
         else
           data[:data_option][:options].each do |key, value|
             element = instance.find_elements(css: '.modal .js-Table .js-key').last
@@ -3695,4 +3700,67 @@ wait untill text in selector disabppears
     return if params[:mute_log]
     puts "#{Time.zone.now}/#{method}: #{params.inspect}"
   end
+
+  private
+
+  def add_tree_options(instance:, options:)
+
+    # first level entries have to get added in regular order
+    options.each_key.with_index do |option, index|
+
+      if index != 0
+        element = instance.find_elements(css: '.modal .js-treeTable .js-addRow')[index - 1]
+        element.click
+      end
+
+      element = instance.find_elements(css: '.modal .js-treeTable .js-key')[index]
+      element.clear
+      element.send_keys(option)
+    end
+
+    add_sub_tree_recursion(
+      instance: instance,
+      options: options,
+    )
+  end
+
+  def add_sub_tree_recursion(instance:, options:, offset: 0)
+    options.each_value.inject(offset) do |child_offset, children|
+
+      child_offset += 1
+
+      # put your recursion glasses on 8-)
+      add_sub_tree_options(
+        instance: instance,
+        options:  children,
+        offset:   child_offset,
+      )
+    end
+  end
+
+  def add_sub_tree_options(instance:, options:, offset:)
+
+    # sub level entries have to get added in reversed order
+    level_options = options.to_a.reverse.to_h.keys
+
+    level_options.each do |option|
+
+      # sub level entries have to get added via 'add child row' link
+      click_index = offset - 1
+
+      element = instance.find_elements(css: '.modal .js-treeTable .js-addChild')[click_index]
+      element.click
+
+      element = instance.find_elements(css: '.modal .js-treeTable .js-key')[offset]
+      element.clear
+      element.send_keys(option)
+      sleep 0.25
+    end
+
+    add_sub_tree_recursion(
+      instance: instance,
+      options:  options,
+      offset:   offset,
+    )
+  end
 end