From 68a034e1592a826c1ebbfbf144d72c8d57f3931b Mon Sep 17 00:00:00 2001 From: Rolf Schmidt Date: Mon, 4 Apr 2022 13:15:08 +0200 Subject: [PATCH] Fixes #4027 - undefined method `to_hash` on editing select fields in the admin interface after migration to 5.1. --- .../_application_ui_element.coffee | 34 +++++ .../_ui_element/multiselect.coffee | 28 ---- .../app/controllers/_ui_element/select.coffee | 29 +--- .../_ui_element/tree_select.coffee | 66 ++++----- .../javascripts/app/models/overview.coffee | 4 +- app/models/object_manager/attribute.rb | 36 ++++- public/assets/tests/qunit/form.js | 36 ++++- spec/models/object_manager/attribute_spec.rb | 131 ++++++++++++++++++ 8 files changed, 262 insertions(+), 102 deletions(-) diff --git a/app/assets/javascripts/app/controllers/_ui_element/_application_ui_element.coffee b/app/assets/javascripts/app/controllers/_ui_element/_application_ui_element.coffee index 3a3c55fb4..8024c0ba7 100644 --- a/app/assets/javascripts/app/controllers/_ui_element/_application_ui_element.coffee +++ b/app/assets/javascripts/app/controllers/_ui_element/_application_ui_element.coffee @@ -282,3 +282,37 @@ class App.UiElement.ApplicationUiElement record.disabled = 'disabled' else record.disabled = '' + + @findOption: (options, value) -> + return if !_.isArray(options) + for option in options + return option if option.value is value + if option.children + result = @findOption(option.children, value) + return result if result + + # 1. If attribute.value is not among the current options, then search within historical options + # 2. If attribute.value is not among current and historical options, then add the value itself as an option + @addDeletedOptions: (attribute) -> + return if !_.isEmpty(attribute.relation) # do not apply for attributes with relation, relations will fill options automatically + return if attribute.rejectNonExistentValues + value = attribute.value + return if !value + return if !attribute.options + + values = value + if !_.isArray(value) + values = [value] + + attribute.historical_options ||= {} + if _.isArray(attribute.options) + for value in values + continue if @findOption(attribute.options, value) + attribute.options.push( + value: value + name: attribute.historical_options[value] || value + ) + else + for value in values + continue if attribute.options[value] + attribute.options[value] = attribute.historical_options[value] || value diff --git a/app/assets/javascripts/app/controllers/_ui_element/multiselect.coffee b/app/assets/javascripts/app/controllers/_ui_element/multiselect.coffee index c633d789d..f8be70484 100644 --- a/app/assets/javascripts/app/controllers/_ui_element/multiselect.coffee +++ b/app/assets/javascripts/app/controllers/_ui_element/multiselect.coffee @@ -37,34 +37,6 @@ class App.UiElement.multiselect extends App.UiElement.ApplicationUiElement # return item $( App.view('generic/select')(attribute: attribute) ) - # 1. If attribute.value is not among the current options, then search within historical options - # 2. If attribute.value is not among current and historical options, then add the value itself as an option - @addDeletedOptions: (attribute) -> - return if !_.isEmpty(attribute.relation) # do not apply for attributes with relation, relations will fill options automatically - return if attribute.rejectNonExistentValues - value = attribute.value - return if !value - return if _.isArray(value) - return if !attribute.options - return if !_.isObject(attribute.options) - return if value of attribute.options - return if value in (temp for own prop, temp of attribute.options) - - if _.isArray(attribute.options) - # Array of Strings (value) - return if value of attribute.options - - # Array of Objects (for ordering purposes) - return if attribute.options.filter((elem) -> elem.value == value) isnt null - else - # regular Object - return if value in (temp for own prop, temp of attribute.options) - - if attribute.historical_options && value of attribute.historical_options - attribute.options[value] = attribute.historical_options[value] - else - attribute.options[value] = value - @_selectedOptionsIsSelected: (value, record) -> if _.isArray(value) for valueItem in value diff --git a/app/assets/javascripts/app/controllers/_ui_element/select.coffee b/app/assets/javascripts/app/controllers/_ui_element/select.coffee index 86d470416..d50dafd9a 100644 --- a/app/assets/javascripts/app/controllers/_ui_element/select.coffee +++ b/app/assets/javascripts/app/controllers/_ui_element/select.coffee @@ -59,7 +59,7 @@ class App.UiElement.select extends App.UiElement.ApplicationUiElement @shouldDisplayWarn: (selectedVal, attribute, params) -> return if !selectedVal return if !params - + params[attribute.name + '_is_display_warning'](selectedVal) @toggleDisplayWarn: (warn_visible, attribute, item) -> @@ -73,30 +73,3 @@ class App.UiElement.select extends App.UiElement.ApplicationUiElement warn_elem.html(attribute.warn) item.append(warn_elem) - # 1. If attribute.value is not among the current options, then search within historical options - # 2. If attribute.value is not among current and historical options, then add the value itself as an option - @addDeletedOptions: (attribute) -> - return if !_.isEmpty(attribute.relation) # do not apply for attributes with relation, relations will fill options automatically - return if attribute.rejectNonExistentValues - value = attribute.value - return if !value - return if _.isArray(value) - return if !attribute.options - return if !_.isObject(attribute.options) - return if value of attribute.options - return if value in (temp for own prop, temp of attribute.options) - - if _.isArray(attribute.options) - # Array of Strings (value) - return if value of attribute.options - - # Array of Objects (for ordering purposes) - return if attribute.options.filter((elem) -> elem.value == value) isnt null - else - # regular Object - return if value in (temp for own prop, temp of attribute.options) - - if attribute.historical_options && value of attribute.historical_options - attribute.options[value] = attribute.historical_options[value] - else - attribute.options[value] = value diff --git a/app/assets/javascripts/app/controllers/_ui_element/tree_select.coffee b/app/assets/javascripts/app/controllers/_ui_element/tree_select.coffee index 19ffac724..1c5a23d23 100644 --- a/app/assets/javascripts/app/controllers/_ui_element/tree_select.coffee +++ b/app/assets/javascripts/app/controllers/_ui_element/tree_select.coffee @@ -1,5 +1,40 @@ # coffeelint: disable=camel_case_classes class App.UiElement.tree_select extends App.UiElement.ApplicationUiElement + @render: (attribute, params) -> + + # set multiple option + if attribute.multiple + attribute.multiple = 'multiple' + else + attribute.multiple = '' + + # add deleted historical options if required + @addDeletedOptions(attribute, params) + + # build options list based on config + @getConfigOptionList(attribute, params) + + # build options list based on relation + @getRelationOptionList(attribute, params) + + # add null selection if needed + @addNullOption(attribute, params) + + # sort attribute.options + @sortOptions(attribute, params) + + # find selected/checked item of list + if attribute.options + @optionsSelect(attribute.options, attribute.value) + + # disable item of list + @disabledOptions(attribute, params) + + # filter attributes + @filterOption(attribute, params) + + new App.SearchableSelect(attribute: attribute).element() + @optionsSelect: (children, value) -> return if !children for child in children @@ -37,34 +72,3 @@ class App.UiElement.tree_select extends App.UiElement.ApplicationUiElement @filterOptionArray: (attribute) -> attribute.options = @filterTreeOptions(attribute.filter, 0, attribute.options, attribute.null) - @render: (attribute, params) -> - - # set multiple option - if attribute.multiple - attribute.multiple = 'multiple' - else - attribute.multiple = '' - - # build options list based on config - @getConfigOptionList(attribute, params) - - # build options list based on relation - @getRelationOptionList(attribute, params) - - # add null selection if needed - @addNullOption(attribute, params) - - # sort attribute.options - @sortOptions(attribute, params) - - # find selected/checked item of list - if attribute.options - @optionsSelect(attribute.options, attribute.value) - - # disable item of list - @disabledOptions(attribute, params) - - # filter attributes - @filterOption(attribute, params) - - new App.SearchableSelect(attribute: attribute).element() diff --git a/app/assets/javascripts/app/models/overview.coffee b/app/assets/javascripts/app/models/overview.coffee index cd72f7325..3fa9879be 100644 --- a/app/assets/javascripts/app/models/overview.coffee +++ b/app/assets/javascripts/app/models/overview.coffee @@ -31,7 +31,7 @@ class App.Overview extends App.Model name: 'order::direction' display: __('Order by Direction') tag: 'select' - default: 'down' + default: 'DESC' null: false translate: true options: @@ -57,7 +57,7 @@ class App.Overview extends App.Model name: 'group_direction' display: __('Group by Direction') tag: 'select' - default: 'down' + default: 'DESC' null: false translate: true options: diff --git a/app/models/object_manager/attribute.rb b/app/models/object_manager/attribute.rb index 5aa593a76..6431497c6 100644 --- a/app/models/object_manager/attribute.rb +++ b/app/models/object_manager/attribute.rb @@ -535,6 +535,31 @@ returns ObjectManager::Attribute.where('to_create = ? OR to_migrate = ? OR to_delete = ? OR to_config = ?', true, true, true, true) end + def self.attribute_historic_options(attribute) + historical_options = attribute.data_option[:historical_options] || {} + if attribute.data_option[:options].present? + historical_options = historical_options.merge(data_options_hash(attribute.data_option[:options])) + end + if attribute.data_option_new[:options].present? + historical_options = historical_options.merge(data_options_hash(attribute.data_option_new[:options])) + end + historical_options + end + + def self.data_options_hash(options, result = {}) + return options if options.is_a?(Hash) + return {} if !options.is_a?(Array) + + options.each do |option| + result[ option[:value] ] = option[:name] + if option[:children].present? + data_options_hash(option[:children], result) + end + end + + result + end + =begin start migration of pending attribute migrations @@ -573,11 +598,8 @@ to send no browser reload event, pass false # config changes if attribute.to_config execute_config_count += 1 - if attribute.data_type == 'select' && attribute.data_option[:options] - historical_options = attribute.data_option[:historical_options] || {} - historical_options.update(attribute.data_option[:options]) - historical_options.update(attribute.data_option_new[:options]) - attribute.data_option_new[:historical_options] = historical_options + if attribute.data_type =~ %r{^(multi|tree_)?select$} && attribute.data_option[:options] + attribute.data_option_new[:historical_options] = attribute_historic_options(attribute) end attribute.data_option = attribute.data_option_new attribute.data_option_new = {} @@ -586,8 +608,8 @@ to send no browser reload event, pass false next if !attribute.to_create && !attribute.to_migrate && !attribute.to_delete end - if attribute.data_type == 'select' && attribute.data_option[:options] - attribute.data_option[:historical_options] = attribute.data_option[:options] + if %r{^(multi|tree_)?select$}.match?(attribute.data_type) + attribute.data_option[:historical_options] = attribute_historic_options(attribute) end data_type = nil diff --git a/public/assets/tests/qunit/form.js b/public/assets/tests/qunit/form.js index 03821c2d3..80d74301e 100644 --- a/public/assets/tests/qunit/form.js +++ b/public/assets/tests/qunit/form.js @@ -1639,15 +1639,39 @@ QUnit.test("Fixes #4024 - Tree select value cannot be set to \"-\" (empty) with el: el, model: { configure_attributes: [ - { name: '4042_select', display: '4042_select', tag: 'select_search', null: true, nulloption: true, multiple: true, options: { 'a': 'a', 'b': 'b' } }, - { name: '4042_multiselect', display: '4042_multiselect', tag: 'multiselect_search', null: true, nulloption: true, multiple: true, options: { 'a': 'a', 'b': 'b' } }, - { name: '4042_tree_select', display: '4042_tree_select', tag: 'tree_select_search', null: true, nulloption: true, multiple: true, options: [{ 'value': 'a', 'name': 'a'}, { 'value': 'b', 'name': 'b'}] }, + { name: '4024_select', display: '4024_select', tag: 'select_search', null: true, nulloption: true, multiple: true, options: { 'a': 'a', 'b': 'b' } }, + { name: '4024_multiselect', display: '4024_multiselect', tag: 'multiselect_search', null: true, nulloption: true, multiple: true, options: { 'a': 'a', 'b': 'b' } }, + { name: '4024_tree_select', display: '4024_tree_select', tag: 'tree_select_search', null: true, nulloption: true, multiple: true, options: [{ 'value': 'a', 'name': 'a'}, { 'value': 'b', 'name': 'b'}] }, ], }, autofocus: true }); - assert.equal(el.find('select[name="4042_select"] option[value=""]').text(), '-', '4042_select has nulloption') - assert.equal(el.find('select[name="4042_multiselect"] option[value=""]').text(), '-', '4042_multiselect has nulloption') - assert.equal(el.find('select[name="4042_tree_select"] option[value=""]').text(), '-', '4042_tree_select has nulloption') + assert.equal(el.find('select[name="4024_select"] option[value=""]').text(), '-', '4024_select has nulloption') + assert.equal(el.find('select[name="4024_multiselect"] option[value=""]').text(), '-', '4024_multiselect has nulloption') + assert.equal(el.find('select[name="4024_tree_select"] option[value=""]').text(), '-', '4024_tree_select has nulloption') +}); + +QUnit.test("Fixes #4027 - undefined method `to_hash` on editing select fields in the admin interface after migration to 5.1.", assert => { + $('#qunit').append('

Fixes #4027 - undefined method `to_hash` on editing select fields in the admin interface after migration to 5.1.

') + var el = $('#form23') + new App.ControllerForm({ + el: el, + model: { + configure_attributes: [ + { name: '4027_selcet_hash', display: '4027_selcet_hash', tag: 'select', null: true, nulloption: true, options: { 'a': 'a', 'b': 'b' }, value: 'c', historical_options: { c: 'C' } }, + { name: '4027_selcet_array', display: '4027_selcet_array', tag: 'select', null: true, nulloption: true, options: [{ value: 'a', name: 'a' }, { value: 'b', name: 'b' } ], value: 'c', historical_options: { c: 'C' } }, + { name: '4027_multiselect_hash', display: '4027_multiselect_hash', tag: 'multiselect', null: true, nulloption: true, options: { 'a': 'a', 'b': 'b' }, value: ['c'], historical_options: { c: 'C' } }, + { name: '4027_multiselect_array', display: '4027_multiselect_array', tag: 'multiselect', null: true, nulloption: true, options: [{ value: 'a', name: 'a' }, { value: 'b', name: 'b' } ], value: ['c', 'd'], historical_options: { c: 'C', d: 'D' } }, + { name: '4027_tree_select_array', display: '4027_tree_select_array', tag: 'tree_select', null: true, nulloption: true, options: [{ value: 'a', name: 'a' }, { value: 'b', name: 'b' } ], value: 'b::c', historical_options: { 'b::c': 'C' } }, + ], + }, + autofocus: true + }); + + assert.equal(el.find('select[name="4027_selcet_hash"] option[selected]').text(), 'C', '4027_select has historic text') + assert.equal(el.find('select[name="4027_selcet_array"] option[selected]').text(), 'C', '4027_selcet_array has historic text') + assert.equal(el.find('select[name="4027_multiselect_hash"] option[selected]').text(), 'C', '4027_multiselect_hash has historic text') + assert.equal(el.find('select[name="4027_multiselect_array"] option[selected]').text(), 'CD', '4027_multiselect_array has historic text') + assert.equal(el.find('div[data-attribute-name="4027_tree_select_array"] .js-input').val(), 'C', '4027_tree_select_array has historic text') }); diff --git a/spec/models/object_manager/attribute_spec.rb b/spec/models/object_manager/attribute_spec.rb index 806650bcc..861e566df 100644 --- a/spec/models/object_manager/attribute_spec.rb +++ b/spec/models/object_manager/attribute_spec.rb @@ -171,6 +171,97 @@ RSpec.describe ObjectManager::Attribute, type: :model do expect(described_class.attribute_to_references_hash_objects).to match_array [Trigger, Overview, Job, Sla, Report::Profile ] end end + + describe '.data_options_hash' do + context 'when hash' do + let(:check) do + { + 'a' => 'A', + 'b' => 'B', + 'c' => 'c', + } + end + + it 'does return the options as hash' do + expect(described_class.data_options_hash(check)).to eq({ + 'a' => 'A', + 'b' => 'B', + 'c' => 'c', + }) + end + end + + context 'when array' do + let(:check) do + [ + { + value: 'a', + name: 'A', + }, + { + value: 'b', + name: 'B', + }, + { + value: 'c', + name: 'c', + }, + ] + end + + it 'does return the options as hash' do + expect(described_class.data_options_hash(check)).to eq({ + 'a' => 'A', + 'b' => 'B', + 'c' => 'c', + }) + end + end + + context 'when tree array' do + let(:check) do + [ + { + value: 'a', + name: 'A', + }, + { + value: 'b', + name: 'B', + }, + { + value: 'c', + name: 'c', + children: [ + { + value: 'c::a', + name: 'c sub a', + }, + { + value: 'c::b', + name: 'c sub b', + }, + { + value: 'c::c', + name: 'c sub c', + }, + ], + }, + ] + end + + it 'does return the options as hash' do + expect(described_class.data_options_hash(check)).to eq({ + 'a' => 'A', + 'b' => 'B', + 'c' => 'c', + 'c::a' => 'c sub a', + 'c::b' => 'c sub b', + 'c::c' => 'c sub c', + }) + end + end + end end describe '#data_option_validations' do @@ -343,4 +434,44 @@ RSpec.describe ObjectManager::Attribute, type: :model do include_examples 'tests the exception on missing past', 'datetime' end end + + describe 'undefined method `to_hash` on editing select fields in the admin interface after migration to 5.1 #4027', db_strategy: :reset do + let(:select_field) { create(:object_manager_attribute_select) } + + before do + described_class.migration_execute + end + + it 'does save the attribute with sorted options' do + add = select_field.attributes.deep_symbolize_keys + add[:data_option_new] = add[:data_option] + add[:data_option_new][:options] = [ + { + name: 'a', + value: 'a', + }, + { + name: 'b', + value: 'b', + }, + { + name: 'c', + value: 'c', + }, + ] + + described_class.add(add) + described_class.migration_execute + + expect_result = { + 'key_1' => 'value_1', + 'key_2' => 'value_2', + 'key_3' => 'value_3', + 'a' => 'a', + 'b' => 'b', + 'c' => 'c' + } + expect(select_field.reload.data_option[:historical_options]).to eq(expect_result) + end + end end