From 347dd95c2fe1b2cc8d9c8c1293c9782fe1fde7fb Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 16 Feb 2018 09:08:08 +0100 Subject: [PATCH 1/8] Extended tests for checkbox and radio element. --- .../_application_controller_form.coffee | 20 ++++++ public/assets/tests/form.js | 69 +++++++++++++------ 2 files changed, 67 insertions(+), 22 deletions(-) diff --git a/app/assets/javascripts/app/controllers/_application_controller_form.coffee b/app/assets/javascripts/app/controllers/_application_controller_form.coffee index 91fdcbdb1..5dccdf588 100644 --- a/app/assets/javascripts/app/controllers/_application_controller_form.coffee +++ b/app/assets/javascripts/app/controllers/_application_controller_form.coffee @@ -454,6 +454,26 @@ class App.ControllerForm extends App.Controller else param[item.name] = value + # verify if we have not checked checkboxes + uncheckParam = {} + lookupForm.find('input[type=checkbox]').each( (index) -> + checked = $(@).attr('checked') + name = $(@).attr('name') + if !checked && !param[name] || param[name] is '' + if uncheckParam[name] is '' + uncheckParam[name] = [] + else + uncheckParam[name] = '' + ) + lookupForm.find('input[type=radio]').each( (index) -> + checked = $(@).attr('checked') + name = $(@).attr('name') + if !checked && !param[name] || param[name] is '' + uncheckParam[name] = '' + ) + for key, value of uncheckParam + param[key] = value + # data type conversion for key of param diff --git a/public/assets/tests/form.js b/public/assets/tests/form.js index e2971e153..6f3fc49f8 100644 --- a/public/assets/tests/form.js +++ b/public/assets/tests/form.js @@ -24,6 +24,8 @@ test("form elements check", function() { selectmultioption2: [ false, true ], richtext2: 'lalu lalu', datetime1: Date.parse('2015-01-11T12:40:00Z'), + checkbox1: [], + checkbox2: '1', } new App.ControllerForm({ el: el, @@ -33,18 +35,20 @@ test("form elements check", function() { { name: 'input2', display: 'Input2', tag: 'input', type: 'text', limit: 100, null: false, default: defaults['input2'] }, { name: 'password1', display: 'Password1', tag: 'input', type: 'password', limit: 100, null: true, default: defaults['password1'] }, { name: 'password2', display: 'Password2', tag: 'input', type: 'password', limit: 100, null: false, default: defaults['password2'] }, - { name: 'textarea1', display: 'Textarea1', tag: 'textarea', rows: 6, limit: 100, null: true, upload: true, default: defaults['textarea1'] }, - { name: 'textarea2', display: 'Textarea2', tag: 'textarea', rows: 6, limit: 100, null: false, upload: true, default: defaults['textarea2'] }, + { name: 'textarea1', display: 'Textarea1', tag: 'textarea', rows: 6, limit: 100, null: true, upload: true, default: defaults['textarea1'] }, + { name: 'textarea2', display: 'Textarea2', tag: 'textarea', rows: 6, limit: 100, null: false, upload: true, default: defaults['textarea2'] }, { name: 'select1', display: 'Select1', tag: 'select', null: true, options: { true: 'internal', false: 'public' }, default: defaults['select1'] }, { name: 'select2', display: 'Select2', tag: 'select', null: false, options: { true: 'internal', false: 'public' }, default: defaults['select2'] }, { name: 'selectmulti1', display: 'SelectMulti1', tag: 'select', null: true, multiple: true, options: { true: 'internal', false: 'public' }, default: defaults['selectmulti1'] }, { name: 'selectmulti2', display: 'SelectMulti2', tag: 'select', null: false, multiple: true, options: { true: 'internal', false: 'public' }, default: defaults['selectmulti2'] }, { name: 'selectmultioption1', display: 'SelectMultiOption1', tag: 'select', null: true, multiple: true, options: [{ value: true, name: 'internal' }, { value: false, name: 'public' }], default: defaults['selectmultioption1'] }, { name: 'selectmultioption2', display: 'SelectMultiOption2', tag: 'select', null: false, multiple: true, options: [{ value: true, name: 'A' }, { value: 1, name: 'B'}, { value: false, name: 'C' }], default: defaults['selectmultioption2'] }, - { name: 'richtext1', display: 'Richtext1', tag: 'richtext', limit: 100, null: true, upload: true, default: defaults['richtext1'] }, - { name: 'richtext2', display: 'Richtext2', tag: 'richtext', limit: 100, null: true, upload: true, default: defaults['richtext2'] }, - { name: 'datetime1', display: 'Datetime1', tag: 'datetime', null: true, default: defaults['datetime1'] }, - { name: 'datetime2', display: 'Datetime2', tag: 'datetime', null: false, default: defaults['datetime2'] }, + { name: 'richtext1', display: 'Richtext1', tag: 'richtext', limit: 100, null: true, upload: true, default: defaults['richtext1'] }, + { name: 'richtext2', display: 'Richtext2', tag: 'richtext', limit: 100, null: true, upload: true, default: defaults['richtext2'] }, + { name: 'datetime1', display: 'Datetime1', tag: 'datetime', null: true, default: defaults['datetime1'] }, + { name: 'datetime2', display: 'Datetime2', tag: 'datetime', null: false, default: defaults['datetime2'] }, + { name: 'checkbox1', display: 'Checkbox1', tag: 'checkbox', null: false, default: defaults['checkbox1'], options: { a: 'AA', b: 'BB' } }, + { name: 'checkbox2', display: 'Checkbox2', tag: 'checkbox', null: false, default: defaults['checkbox2'], options: { 1: '11' } }, ] }, autofocus: true @@ -100,6 +104,10 @@ test("form elements check", function() { //equal(el.find('[name="richtext2"]').prop('required'), true, 'check textarea2 required') equal(el.find('[name="richtext2"]').is(":focus"), false, 'check textarea2 focus') + equal(el.find('[name="checkbox1"]').first().is(":checked"), false) + equal(el.find('[name="checkbox1"]').last().is(":checked"), false) + equal(el.find('[name="checkbox2"]').is(":checked"), true) + }); test("form params check", function() { @@ -134,6 +142,11 @@ test("form params check", function() { date3: '2015-01-11', active1: true, active2: false, + checkbox1: [], + checkbox2: '', + checkbox3: 'd', + radiobox1: '', + radiobox2: 'a', } new App.ControllerForm({ el: el, @@ -155,24 +168,30 @@ test("form params check", function() { { name: 'selectmultioption2', display: 'SelectMultiOption2', tag: 'select', null: false, multiple: true, options: [{ value: true, name: 'A' }, { value: 1, name: 'B'}, { value: false, name: 'C' }] }, { name: 'autocompletion1', display: 'AutoCompletion1', tag: 'autocompletion', null: false, options: { true: 'internal', false: 'public' }, source: [ { label: "Choice1", value: "value1", id: "id1" }, { label: "Choice2", value: "value2", id: "id2" }, ], minLength: 1 }, { name: 'autocompletion2', display: 'AutoCompletion2', tag: 'autocompletion', null: false, options: { true: 'internal', false: 'public' }, source: [ { label: "Choice1", value: "value1", id: "id1" }, { label: "Choice2", value: "value2", id: "id2" }, ], minLength: 1 }, - { name: 'richtext1', display: 'Richtext1', tag: 'richtext', maxlength: 100, null: true, type: 'richtext', multiline: true, upload: true, default: defaults['richtext1'] }, - { name: 'richtext2', display: 'Richtext2', tag: 'richtext', maxlength: 100, null: true, type: 'richtext', multiline: true, upload: true, default: defaults['richtext2'] }, - { name: 'richtext3', display: 'Richtext3', tag: 'richtext', maxlength: 100, null: true, type: 'richtext', multiline: false, default: defaults['richtext3'] }, - { name: 'richtext4', display: 'Richtext4', tag: 'richtext', maxlength: 100, null: true, type: 'richtext', multiline: false, default: defaults['richtext4'] }, - { name: 'richtext5', display: 'Richtext5', tag: 'richtext', maxlength: 100, null: true, type: 'textonly', multiline: true, upload: true, default: defaults['richtext5'] }, - { name: 'richtext6', display: 'Richtext6', tag: 'richtext', maxlength: 100, null: true, type: 'textonly', multiline: true, upload: true, default: defaults['richtext6'] }, - { name: 'richtext7', display: 'Richtext7', tag: 'richtext', maxlength: 100, null: true, type: 'textonly', multiline: false, default: defaults['richtext7'] }, - { name: 'richtext8', display: 'Richtext8', tag: 'richtext', maxlength: 100, null: true, type: 'textonly', multiline: false, default: defaults['richtext8'] }, - { name: 'datetime1', display: 'Datetime1', tag: 'datetime', null: true, default: defaults['datetime1'] }, - { name: 'datetime2', display: 'Datetime2', tag: 'datetime', null: true, default: defaults['datetime2'] }, - { name: 'datetime3', display: 'Datetime3', tag: 'datetime', null: false, default: defaults['datetime3'] }, - { name: 'datetime4', display: 'Datetime4', tag: 'datetime', null: false, default: defaults['datetime4'] }, - { name: 'date1', display: 'Date1', tag: 'date', null: true, default: defaults['date1'] }, - { name: 'date2', display: 'Date2', tag: 'date', null: true, default: defaults['date2'] }, - { name: 'date3', display: 'Date3', tag: 'date', null: false, default: defaults['date3'] }, - { name: 'date4', display: 'Date4', tag: 'date', null: false, default: defaults['date4'] }, + { name: 'richtext1', display: 'Richtext1', tag: 'richtext', maxlength: 100, null: true, type: 'richtext', multiline: true, upload: true, default: defaults['richtext1'] }, + { name: 'richtext2', display: 'Richtext2', tag: 'richtext', maxlength: 100, null: true, type: 'richtext', multiline: true, upload: true, default: defaults['richtext2'] }, + { name: 'richtext3', display: 'Richtext3', tag: 'richtext', maxlength: 100, null: true, type: 'richtext', multiline: false, default: defaults['richtext3'] }, + { name: 'richtext4', display: 'Richtext4', tag: 'richtext', maxlength: 100, null: true, type: 'richtext', multiline: false, default: defaults['richtext4'] }, + { name: 'richtext5', display: 'Richtext5', tag: 'richtext', maxlength: 100, null: true, type: 'textonly', multiline: true, upload: true, default: defaults['richtext5'] }, + { name: 'richtext6', display: 'Richtext6', tag: 'richtext', maxlength: 100, null: true, type: 'textonly', multiline: true, upload: true, default: defaults['richtext6'] }, + { name: 'richtext7', display: 'Richtext7', tag: 'richtext', maxlength: 100, null: true, type: 'textonly', multiline: false, default: defaults['richtext7'] }, + { name: 'richtext8', display: 'Richtext8', tag: 'richtext', maxlength: 100, null: true, type: 'textonly', multiline: false, default: defaults['richtext8'] }, + { name: 'datetime1', display: 'Datetime1', tag: 'datetime', null: true, default: defaults['datetime1'] }, + { name: 'datetime2', display: 'Datetime2', tag: 'datetime', null: true, default: defaults['datetime2'] }, + { name: 'datetime3', display: 'Datetime3', tag: 'datetime', null: false, default: defaults['datetime3'] }, + { name: 'datetime4', display: 'Datetime4', tag: 'datetime', null: false, default: defaults['datetime4'] }, + { name: 'date1', display: 'Date1', tag: 'date', null: true, default: defaults['date1'] }, + { name: 'date2', display: 'Date2', tag: 'date', null: true, default: defaults['date2'] }, + { name: 'date3', display: 'Date3', tag: 'date', null: false, default: defaults['date3'] }, + { name: 'date4', display: 'Date4', tag: 'date', null: false, default: defaults['date4'] }, { name: 'active1', display: 'Active1', tag: 'active', default: defaults['active1'] }, { name: 'active2', display: 'Active2', tag: 'active', default: defaults['active2'] }, + { name: 'checkbox1', display: 'Checkbox1', tag: 'checkbox', null: false, default: defaults['checkbox1'], options: { a: 'AA', b: 'BB' } }, + { name: 'checkbox2', display: 'Checkbox2', tag: 'checkbox', null: false, default: defaults['checkbox2'], options: { 1: '11' } }, + { name: 'checkbox3', display: 'Checkbox3', tag: 'checkbox', null: false, default: defaults['checkbox3'], options: { c: 'CC', d: 'DD' } }, + { name: 'checkbox4', display: 'Checkbox4', tag: 'checkbox', null: false, default: defaults['checkbox4'], options: { aa: 'AA', bb: 'BB' } }, + { name: 'radiobox1', display: 'Radiobox1', tag: 'radio', null: false, default: defaults['radiobox1'], options: { a: 'AA', b: 'BB' } }, + { name: 'radiobox2', display: 'Radiobox2', tag: 'radio', null: false, default: defaults['radiobox2'], options: { a: '11' } }, ], }, params: defaults, @@ -271,6 +290,12 @@ test("form params check", function() { date4: undefined, active1: true, active2: false, + checkbox1: [], + checkbox2: '', + checkbox3: 'd', + checkbox4: [], + radiobox1: '', + radiobox2: 'a', } deepEqual(params, test_params, 'form param check') From db8b54f64cd1500ba704172e16d4439b61037c09 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 16 Feb 2018 09:41:50 +0100 Subject: [PATCH 2/8] WIP. --- .../app/controllers/_application_controller_form.coffee | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/app/assets/javascripts/app/controllers/_application_controller_form.coffee b/app/assets/javascripts/app/controllers/_application_controller_form.coffee index 5dccdf588..828c8784d 100644 --- a/app/assets/javascripts/app/controllers/_application_controller_form.coffee +++ b/app/assets/javascripts/app/controllers/_application_controller_form.coffee @@ -459,7 +459,7 @@ class App.ControllerForm extends App.Controller lookupForm.find('input[type=checkbox]').each( (index) -> checked = $(@).attr('checked') name = $(@).attr('name') - if !checked && !param[name] || param[name] is '' + if name && !checked && (!(name of param) || param[name] is '') if uncheckParam[name] is '' uncheckParam[name] = [] else @@ -468,11 +468,12 @@ class App.ControllerForm extends App.Controller lookupForm.find('input[type=radio]').each( (index) -> checked = $(@).attr('checked') name = $(@).attr('name') - if !checked && !param[name] || param[name] is '' + if name && !checked && !(name of param) uncheckParam[name] = '' ) for key, value of uncheckParam - param[key] = value + if !(key of param) + param[key] = value # data type conversion for key of param From 894afeeb421988448cd4abe608f620441a975b5b Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 16 Feb 2018 10:35:50 +0100 Subject: [PATCH 3/8] WIP. --- .../app/controllers/_application_controller_form.coffee | 8 ++++---- public/assets/tests/form.js | 8 ++++---- 2 files changed, 8 insertions(+), 8 deletions(-) diff --git a/app/assets/javascripts/app/controllers/_application_controller_form.coffee b/app/assets/javascripts/app/controllers/_application_controller_form.coffee index 828c8784d..7f7cc5991 100644 --- a/app/assets/javascripts/app/controllers/_application_controller_form.coffee +++ b/app/assets/javascripts/app/controllers/_application_controller_form.coffee @@ -460,16 +460,16 @@ class App.ControllerForm extends App.Controller checked = $(@).attr('checked') name = $(@).attr('name') if name && !checked && (!(name of param) || param[name] is '') - if uncheckParam[name] is '' - uncheckParam[name] = [] + if !(name of uncheckParam) + uncheckParam[name] = undefined else - uncheckParam[name] = '' + uncheckParam[name] = [] ) lookupForm.find('input[type=radio]').each( (index) -> checked = $(@).attr('checked') name = $(@).attr('name') if name && !checked && !(name of param) - uncheckParam[name] = '' + uncheckParam[name] = undefined ) for key, value of uncheckParam if !(key of param) diff --git a/public/assets/tests/form.js b/public/assets/tests/form.js index 6f3fc49f8..46d22e110 100644 --- a/public/assets/tests/form.js +++ b/public/assets/tests/form.js @@ -143,9 +143,9 @@ test("form params check", function() { active1: true, active2: false, checkbox1: [], - checkbox2: '', + checkbox2: undefined, checkbox3: 'd', - radiobox1: '', + radiobox1: undefined, radiobox2: 'a', } new App.ControllerForm({ @@ -291,10 +291,10 @@ test("form params check", function() { active1: true, active2: false, checkbox1: [], - checkbox2: '', + checkbox2: undefined, checkbox3: 'd', checkbox4: [], - radiobox1: '', + radiobox1: undefined, radiobox2: 'a', } deepEqual(params, test_params, 'form param check') From faad5f9ebda5f00540f70ca8d7151dd1ea6907c9 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 16 Feb 2018 11:19:26 +0100 Subject: [PATCH 4/8] WIP. --- .../app/controllers/_application_controller_form.coffee | 4 ++++ app/assets/javascripts/app/controllers/_channel/form.coffee | 2 +- 2 files changed, 5 insertions(+), 1 deletion(-) diff --git a/app/assets/javascripts/app/controllers/_application_controller_form.coffee b/app/assets/javascripts/app/controllers/_application_controller_form.coffee index 7f7cc5991..ee466374a 100644 --- a/app/assets/javascripts/app/controllers/_application_controller_form.coffee +++ b/app/assets/javascripts/app/controllers/_application_controller_form.coffee @@ -465,12 +465,16 @@ class App.ControllerForm extends App.Controller else uncheckParam[name] = [] ) + + # verify if we have not checked radios lookupForm.find('input[type=radio]').each( (index) -> checked = $(@).attr('checked') name = $(@).attr('name') if name && !checked && !(name of param) uncheckParam[name] = undefined ) + + # apply empty checkboxes & radio values to params for key, value of uncheckParam if !(key of param) param[key] = value diff --git a/app/assets/javascripts/app/controllers/_channel/form.coffee b/app/assets/javascripts/app/controllers/_channel/form.coffee index 2edaa3cdc..10394d8d3 100644 --- a/app/assets/javascripts/app/controllers/_channel/form.coffee +++ b/app/assets/javascripts/app/controllers/_channel/form.coffee @@ -55,7 +55,7 @@ class App.ChannelForm extends App.ControllerSubContent params = @formParam(@$('.js-paramsDesigner')) paramString = '' for key, value of params - if value != '' + if !_.isEmpty(value) if paramString != '' paramString += ",\n" if value == 'true' || value == 'false' From a1391a5cd319567efff0c26fb2b0ac5b423d0888 Mon Sep 17 00:00:00 2001 From: Martin Edenhofer Date: Fri, 16 Feb 2018 12:18:44 +0100 Subject: [PATCH 5/8] VIP. --- app/assets/javascripts/app/controllers/_channel/chat.coffee | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/app/assets/javascripts/app/controllers/_channel/chat.coffee b/app/assets/javascripts/app/controllers/_channel/chat.coffee index 4745483a4..86ad9fcc6 100644 --- a/app/assets/javascripts/app/controllers/_channel/chat.coffee +++ b/app/assets/javascripts/app/controllers/_channel/chat.coffee @@ -348,7 +348,7 @@ class App.ChannelChat extends App.ControllerSubContent params[key] = value paramString = '' for key, value of params - if value != '' + if !_.isEmpty(value) if paramString != '' # coffeelint: disable=no_unnecessary_double_quotes paramString += ",\n" From 0f28d1f0b6441730e24bc79d1525e44c3870f497 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Mon, 19 Feb 2018 20:48:09 +0100 Subject: [PATCH 6/8] WIP TE. --- .../application_model/can_associations.rb | 1 - app/models/concerns/has_groups.rb | 66 ++++++++++------- spec/models/concerns/has_groups_examples.rb | 72 ++++++++++++++++++- 3 files changed, 112 insertions(+), 27 deletions(-) diff --git a/app/models/application_model/can_associations.rb b/app/models/application_model/can_associations.rb index 2a625f19e..aca230bc8 100644 --- a/app/models/application_model/can_associations.rb +++ b/app/models/application_model/can_associations.rb @@ -23,7 +23,6 @@ returns group_ids: :group_ids_access_map= }.each do |param, setter| map = params[param] - next if map.blank? next if !respond_to?(setter) send(setter, map) end diff --git a/app/models/concerns/has_groups.rb b/app/models/concerns/has_groups.rb index 84fd3bbbe..8780d3bcf 100644 --- a/app/models/concerns/has_groups.rb +++ b/app/models/concerns/has_groups.rb @@ -211,46 +211,62 @@ module HasGroups end def groups_access_map_store(map) - map.each do |group_identifier, accesses| - # use given key as identifier or look it up - # via the given block which returns the identifier - group_id = block_given? ? yield(group_identifier) : group_identifier + fill_group_access_buffer do + map.each do |group_identifier, accesses| + # use given key as identifier or look it up + # via the given block which returns the identifier + group_id = block_given? ? yield(group_identifier) : group_identifier - if !accesses.is_a?(Array) - accesses = [accesses] - end + if !accesses.is_a?(Array) + accesses = [accesses] + end - accesses.each do |access| - push_group_access_buffer( - group_id: group_id, - access: access - ) + accesses.each do |access| + push_group_access_buffer( + group_id: group_id, + access: access + ) + end end end + end + def fill_group_access_buffer + @group_access_buffer = [] + yield check_group_access_buffer if id end def push_group_access_buffer(entry) - @group_access_buffer ||= [] @group_access_buffer.push(entry) end - def check_group_access_buffer - return if group_access_buffer.blank? - destroy_group_relations + def flushed_group_access_buffer + # group_access_buffer is at least an empty Array + # if changes to the map were performed + # otherwise it's just an update of other attributes + return if group_access_buffer.nil? + yield + group_access_buffer = nil + cache_delete + end - foreign_key = group_through.foreign_key - entries = group_access_buffer.collect do |entry| - entry[foreign_key] = id - entry + def check_group_access_buffer + + flushed_group_access_buffer do + destroy_group_relations + + break if group_access_buffer.blank? + + foreign_key = group_through.foreign_key + entries = group_access_buffer.collect do |entry| + entry[foreign_key] = id + entry + end + + group_through.klass.create!(entries) end - group_through.klass.create!(entries) - - group_access_buffer = nil - - cache_delete true end diff --git a/spec/models/concerns/has_groups_examples.rb b/spec/models/concerns/has_groups_examples.rb index f8bc0beb1..3b2ddaee2 100644 --- a/spec/models/concerns/has_groups_examples.rb +++ b/spec/models/concerns/has_groups_examples.rb @@ -229,6 +229,19 @@ RSpec.shared_examples 'HasGroups' do described_class.group_through.klass.count }.by(3) end + + it 'allows empty Hash value' do + group_access_instance.group_names_access_map = { + group_full.name => 'full', + group_read.name => %w[read change], + } + + expect do + group_access_instance.group_names_access_map = {} + end.to change { + described_class.group_through.klass.count + }.by(-3) + end end context 'new instance' do @@ -256,6 +269,16 @@ RSpec.shared_examples 'HasGroups' do described_class.group_through.klass.count }.by(2) end + + it 'allows empty Hash value' do + expect do + new_group_access_instance.group_names_access_map = {} + + new_group_access_instance.save + end.not_to change { + described_class.group_through.klass.count + } + end end end @@ -284,6 +307,18 @@ RSpec.shared_examples 'HasGroups' do expect(group_access_instance_inactive.group_names_access_map).to be_empty end + + it 'returns empty map if none is stored' do + + group_access_instance.group_names_access_map = { + group_full.name => 'full', + group_read.name => 'read', + } + + group_access_instance.group_names_access_map = {} + + expect(group_access_instance.group_names_access_map).to be_blank + end end context '#group_ids_access_map=' do @@ -305,7 +340,7 @@ RSpec.shared_examples 'HasGroups' do }.by(2) end - it 'stores Hash with String values' do + it 'stores Hash with Array values' do expect do group_access_instance.group_ids_access_map = { group_full.id => 'full', @@ -315,6 +350,19 @@ RSpec.shared_examples 'HasGroups' do described_class.group_through.klass.count }.by(3) end + + it 'allows empty Hash value' do + group_access_instance.group_ids_access_map = { + group_full.id => 'full', + group_read.id => %w[read change], + } + + expect do + group_access_instance.group_ids_access_map = {} + end.to change { + described_class.group_through.klass.count + }.by(-3) + end end context 'new instance' do @@ -342,6 +390,16 @@ RSpec.shared_examples 'HasGroups' do described_class.group_through.klass.count }.by(2) end + + it 'allows empty Hash value' do + expect do + new_group_access_instance.group_ids_access_map = {} + + new_group_access_instance.save + end.not_to change { + described_class.group_through.klass.count + } + end end end @@ -370,6 +428,18 @@ RSpec.shared_examples 'HasGroups' do expect(group_access_instance_inactive.group_ids_access_map).to be_empty end + + it 'returns empty map if none is stored' do + + group_access_instance.group_ids_access_map = { + group_full.id => 'full', + group_read.id => 'read', + } + + group_access_instance.group_ids_access_map = {} + + expect(group_access_instance.group_ids_access_map).to be_blank + end end context '#associations_from_param' do From 77ec399fe92bbdb10d94a8986491b35784a38edb Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Tue, 27 Feb 2018 16:52:30 +0100 Subject: [PATCH 7/8] Further refactoring and bugfixing. --- app/models/concerns/has_groups.rb | 20 ++++++++------------ 1 file changed, 8 insertions(+), 12 deletions(-) diff --git a/app/models/concerns/has_groups.rb b/app/models/concerns/has_groups.rb index 8780d3bcf..b04930570 100644 --- a/app/models/concerns/has_groups.rb +++ b/app/models/concerns/has_groups.rb @@ -7,8 +7,8 @@ module HasGroups attr_accessor :group_access_buffer - after_create :check_group_access_buffer - after_update :check_group_access_buffer + after_create :process_group_access_buffer + after_update :process_group_access_buffer association_attributes_ignored :groups, group_through_identifier @@ -212,16 +212,12 @@ module HasGroups def groups_access_map_store(map) fill_group_access_buffer do - map.each do |group_identifier, accesses| + Hash(map).each do |group_identifier, accesses| # use given key as identifier or look it up # via the given block which returns the identifier group_id = block_given? ? yield(group_identifier) : group_identifier - if !accesses.is_a?(Array) - accesses = [accesses] - end - - accesses.each do |access| + Array(accesses).each do |access| push_group_access_buffer( group_id: group_id, access: access @@ -234,14 +230,14 @@ module HasGroups def fill_group_access_buffer @group_access_buffer = [] yield - check_group_access_buffer if id + process_group_access_buffer if id end def push_group_access_buffer(entry) @group_access_buffer.push(entry) end - def flushed_group_access_buffer + def flush_group_access_buffer # group_access_buffer is at least an empty Array # if changes to the map were performed # otherwise it's just an update of other attributes @@ -251,9 +247,9 @@ module HasGroups cache_delete end - def check_group_access_buffer + def process_group_access_buffer - flushed_group_access_buffer do + flush_group_access_buffer do destroy_group_relations break if group_access_buffer.blank? From 16d89b1fa7439a71958f075fffd13f16f3dae745 Mon Sep 17 00:00:00 2001 From: Thorsten Eckel Date: Wed, 28 Feb 2018 12:58:10 +0100 Subject: [PATCH 8/8] Fixed bug: Empty given parameter overwrites/removes group access structure. --- app/models/application_model/can_associations.rb | 1 + 1 file changed, 1 insertion(+) diff --git a/app/models/application_model/can_associations.rb b/app/models/application_model/can_associations.rb index aca230bc8..d40273a1e 100644 --- a/app/models/application_model/can_associations.rb +++ b/app/models/application_model/can_associations.rb @@ -22,6 +22,7 @@ returns groups: :group_names_access_map=, group_ids: :group_ids_access_map= }.each do |param, setter| + next if !params.key?(param) map = params[param] next if !respond_to?(setter) send(setter, map)