From 5069334e0f6f373f036f230e8ec0aa34bcf8c9e9 Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Wed, 16 Jan 2019 14:15:33 +0000 Subject: [PATCH 1/7] CustomButtonSet - make sure children follow button_order CustomButtonSet has currently 2 independent lists of children: - `set_data[:button_order]` is an array of ids with the right order, - `children` (or `members`) is a collection of custom buttons, in no order Currently, in the UI, the tree is reading button_order, the GTL is reading `members`, the SUI toolbar is using button_order, the OPS UI toolbar is using both. It's essential to keep the 2 lists the same, and there are 2 ways of achieving it: * adding methods to add items to specific position, that will handle both button_order and children, and expose those via the API * deciding `button_order` is the canonical collection, and we can always replace children based on the ids in button order. The first option would mean doing N+1 API requests when creating a button group with N assigned buttons, the second option means we only have to edit the custom button set, without any per-button requests. (We never create a new custom button as part of editing/creating a set.) --- app/models/custom_button_set.rb | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/app/models/custom_button_set.rb b/app/models/custom_button_set.rb index 8c533c07fc0..a22d6207d2c 100644 --- a/app/models/custom_button_set.rb +++ b/app/models/custom_button_set.rb @@ -1,6 +1,11 @@ class CustomButtonSet < ApplicationRecord acts_as_miq_set + before_save :update_children + def update_children + replace_children Rbac.filtered(CustomButton.where(:id => set_data[:button_order])) + end + def self.find_all_by_class_name(class_name, class_id = nil) ordering = ->(o) { [o.set_data[:group_index].to_s, o.name] } From 213f45eee63d0dff04c643ff57bd801220e5e61b Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Wed, 16 Jan 2019 14:53:18 +0000 Subject: [PATCH 2/7] CustomButtonSet - don't allow saving if buttons specified in button_order don't exist without this, code like... ``` cbs.set_data[:button_order] = [1,2,3] cbs.save! ``` would suceed in saving a CustomButtonSet with that button order, even when some of the buttons don't exist. Since that would mean that children no longer match the button order, we need to throw. `throw(:abort)` is the only valid way of stopping the chain in Rails 5, and seems to work find in older rails too. --- app/models/custom_button_set.rb | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/models/custom_button_set.rb b/app/models/custom_button_set.rb index a22d6207d2c..921d4321160 100644 --- a/app/models/custom_button_set.rb +++ b/app/models/custom_button_set.rb @@ -4,6 +4,8 @@ class CustomButtonSet < ApplicationRecord before_save :update_children def update_children replace_children Rbac.filtered(CustomButton.where(:id => set_data[:button_order])) + + throw(:abort) if children.pluck(:id).sort != set_data[:button_order].sort end def self.find_all_by_class_name(class_name, class_id = nil) From dafc6a7f100921a0e76eedc60c88124b6b066f85 Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Wed, 23 Jan 2019 11:04:53 +0000 Subject: [PATCH 3/7] CustomButton#update_children - handle new custom buttons having set_data nil no set_data => no children --- app/models/custom_button_set.rb | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/app/models/custom_button_set.rb b/app/models/custom_button_set.rb index 921d4321160..d2066fe8b2e 100644 --- a/app/models/custom_button_set.rb +++ b/app/models/custom_button_set.rb @@ -3,9 +3,15 @@ class CustomButtonSet < ApplicationRecord before_save :update_children def update_children - replace_children Rbac.filtered(CustomButton.where(:id => set_data[:button_order])) + if set_data.try(:[], :button_order).nil? + remove_all_children + return + end + children = Rbac.filtered(CustomButton.where(:id => set_data[:button_order])) throw(:abort) if children.pluck(:id).sort != set_data[:button_order].sort + + replace_children Rbac.filtered(CustomButton.where(:id => set_data[:button_order])) end def self.find_all_by_class_name(class_name, class_id = nil) From 31d6d329584f9f554643b8f81695566536fa75ba Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Wed, 23 Jan 2019 11:06:55 +0000 Subject: [PATCH 4/7] ServiceTemplate spec - don't use add_children on custom buttons, set order instead adding a child won't actually affect any custom button toolbar that relies on the order (such as SUI) --- spec/models/service_template_spec.rb | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/spec/models/service_template_spec.rb b/spec/models/service_template_spec.rb index 2ddb4910dec..b1caa8b650d 100644 --- a/spec/models/service_template_spec.rb +++ b/spec/models/service_template_spec.rb @@ -8,7 +8,8 @@ FactoryBot.create(:custom_button, :name => "generic_no_group", :applies_to_class => "Service") generic_group = FactoryBot.create(:custom_button, :name => "generic_group", :applies_to_class => "Service") generic_group_set = FactoryBot.create(:custom_button_set, :name => "generic_group_set") - generic_group_set.add_member(generic_group) + generic_group_set.set_data = {:button_order => [generic_group.id]} + generic_group_set.save! service_template = FactoryBot.create(:service_template) FactoryBot.create( @@ -24,7 +25,9 @@ :applies_to_id => service_template.id ) assigned_group_set = FactoryBot.create(:custom_button_set, :name => "assigned_group_set") - assigned_group_set.add_member(assigned_group) + assigned_group_set.set_data = {:button_order => [assigned_group.id]} + assigned_group_set.save! + service_template.update(:custom_button_sets => [assigned_group_set]) expected = { From e517591a43e5890356fed79f4dcbcd8c78ce5544 Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Wed, 23 Jan 2019 11:41:48 +0000 Subject: [PATCH 5/7] TaskHelpers::Imports::CustomButtons - drop button_order before importing CustomButtonSet button_order is ignored for imports anyway, `custom_button_set_post` rewrites it. But having it set during `create!` causes `update_children` to fail because it can't find those nonexistent CustomButtons. --- lib/task_helpers/imports/custom_buttons.rb | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/lib/task_helpers/imports/custom_buttons.rb b/lib/task_helpers/imports/custom_buttons.rb index 14c23a82444..bf009d727be 100644 --- a/lib/task_helpers/imports/custom_buttons.rb +++ b/lib/task_helpers/imports/custom_buttons.rb @@ -30,14 +30,15 @@ def create_object(class_name, obj_array) klass = class_name.camelize.constantize obj_array.collect do |obj| - begin - klass.create!(obj['attributes']&.except('guid')).tap do |new_obj| - add_children(obj, new_obj) - add_associations(obj, new_obj) - try("#{class_name}_post", new_obj) - end - rescue StandardError - raise + if klass.name == "CustomButtonSet" + order = obj.fetch_path('attributes', 'set_data', :button_order) + obj['attributes']['set_data'][:button_order] = nil if order.present? + end + + klass.create!(obj['attributes']&.except('guid')).tap do |new_obj| + add_children(obj, new_obj) + add_associations(obj, new_obj) + try("#{class_name}_post", new_obj) end end end From cb46b8cd215a2c11bc438b6c8282fe324a07e78f Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Wed, 23 Jan 2019 12:23:45 +0000 Subject: [PATCH 6/7] CustomButtonSet - only set children in after_save otherwise, creating a CustomButtonSet with nonempty button_order fails: ActiveRecord::RecordNotSaved: You cannot call create unless the parent is saved # ./app/models/mixins/relationship_mixin.rb:446:in `add_relationship' --- app/models/custom_button_set.rb | 15 +++++++++++---- spec/models/custom_button_set_spec.rb | 24 ++++++++++++++++++++++++ 2 files changed, 35 insertions(+), 4 deletions(-) diff --git a/app/models/custom_button_set.rb b/app/models/custom_button_set.rb index d2066fe8b2e..f3b5e4a00ac 100644 --- a/app/models/custom_button_set.rb +++ b/app/models/custom_button_set.rb @@ -1,7 +1,16 @@ class CustomButtonSet < ApplicationRecord acts_as_miq_set - before_save :update_children + before_save :validate_children + after_save :update_children + + def validate_children + return if set_data.try(:[], :button_order).nil? + + children = Rbac.filtered(CustomButton.where(:id => set_data[:button_order])) + throw(:abort) if children.pluck(:id).sort != set_data[:button_order].sort + end + def update_children if set_data.try(:[], :button_order).nil? remove_all_children @@ -9,9 +18,7 @@ def update_children end children = Rbac.filtered(CustomButton.where(:id => set_data[:button_order])) - throw(:abort) if children.pluck(:id).sort != set_data[:button_order].sort - - replace_children Rbac.filtered(CustomButton.where(:id => set_data[:button_order])) + replace_children(children) end def self.find_all_by_class_name(class_name, class_id = nil) diff --git a/spec/models/custom_button_set_spec.rb b/spec/models/custom_button_set_spec.rb index 8a9a0e8a1e6..5f9e51778a9 100644 --- a/spec/models/custom_button_set_spec.rb +++ b/spec/models/custom_button_set_spec.rb @@ -79,4 +79,28 @@ expect(CustomButton.count).to eq(2) expect(CustomButtonSet.count).to eq(2) end + + context '#update_children' do + let(:vm) { FactoryBot.create(:vm_vmware, :name => 'vm') } + let(:custom_button_1) { FactoryBot.create(:custom_button, :applies_to => vm) } + let(:custom_button_2) { FactoryBot.create(:custom_button, :applies_to => vm) } + let(:custom_button_3) { FactoryBot.create(:custom_button, :applies_to => vm) } + let(:set_data) { {:applies_to_class => "Vm", :button_order => [custom_button_1.id, custom_button_2.id]} } + let(:custom_button_set) { FactoryBot.create(:custom_button_set, :name => "set_1", :set_data => set_data) } + + it "updates children after setting button_order" do + expect(custom_button_set.children.count).to eq(2) + + custom_button_set.set_data[:button_order] = [ + custom_button_2.id, + custom_button_3.id, + ] + custom_button_set.save! + + expect(custom_button_set.children.count).to eq(2) + expect(custom_button_set.children).not_to include(custom_button_1) + expect(custom_button_set.children).to include(custom_button_2) + expect(custom_button_set.children).to include(custom_button_3) + end + end end From 1c4ea876a1dfb0a8a6ba3e0ac54c739a8484d1e8 Mon Sep 17 00:00:00 2001 From: Martin Hradil Date: Tue, 12 Feb 2019 12:26:30 +0000 Subject: [PATCH 7/7] GOD spec - don't use add_member for custom button sets --- spec/models/generic_object_definition_spec.rb | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/spec/models/generic_object_definition_spec.rb b/spec/models/generic_object_definition_spec.rb index 0db9ab514d6..3983acc1997 100644 --- a/spec/models/generic_object_definition_spec.rb +++ b/spec/models/generic_object_definition_spec.rb @@ -297,8 +297,7 @@ it "returns the custom actions in a hash grouped by buttons and button groups" do FactoryBot.create(:custom_button, :name => "generic_no_group", :applies_to_class => "GenericObject") generic_group = FactoryBot.create(:custom_button, :name => "generic_group", :applies_to_class => "GenericObject") - generic_group_set = FactoryBot.create(:custom_button_set, :name => "generic_group_set") - generic_group_set.add_member(generic_group) + generic_group_set = FactoryBot.create(:custom_button_set, :name => "generic_group_set", :set_data => {:button_order => [generic_group.id]}) FactoryBot.create( :custom_button, @@ -312,8 +311,7 @@ :applies_to_class => "GenericObjectDefinition", :applies_to_id => definition.id ) - assigned_group_set = FactoryBot.create(:custom_button_set, :name => "assigned_group_set") - assigned_group_set.add_member(assigned_group) + assigned_group_set = FactoryBot.create(:custom_button_set, :name => "assigned_group_set", :set_data => {:button_order => [assigned_group.id]}) definition.update(:custom_button_sets => [assigned_group_set]) expected = {