From eb51c011f366d365e38527978466919b91b63b50 Mon Sep 17 00:00:00 2001 From: Erik Clarizio Date: Thu, 18 Jul 2019 10:06:03 -0700 Subject: [PATCH 1/4] Initialize static field values before loading values into the field https://bugzilla.redhat.com/show_bug.cgi?id=1730813 --- app/models/dialog.rb | 8 +++ app/models/dialog_field.rb | 6 +++ app/models/resource_action_workflow.rb | 5 +- spec/models/dialog_field_spec.rb | 52 ++++++++++++++++++++ spec/models/dialog_spec.rb | 24 +++++++++ spec/models/resource_action_workflow_spec.rb | 5 ++ 6 files changed, 99 insertions(+), 1 deletion(-) diff --git a/app/models/dialog.rb b/app/models/dialog.rb index 821c1ab9bf9..1838933acd8 100644 --- a/app/models/dialog.rb +++ b/app/models/dialog.rb @@ -118,6 +118,14 @@ def initialize_value_context(_values) dialog_field_hash.each_value(&:initialize_value_context) end + def initialize_static_values + dialog_field_hash.each_value do |field| + field.dialog = self + end + + dialog_field_hash.each_value(&:initialize_static_values) + end + def init_fields_with_values_for_request(values) values = values.with_indifferent_access diff --git a/app/models/dialog_field.rb b/app/models/dialog_field.rb index 0d2ad2ed9f3..9d98cf44cf0 100644 --- a/app/models/dialog_field.rb +++ b/app/models/dialog_field.rb @@ -103,6 +103,12 @@ def initialize_value_context end end + def initialize_static_values + if @value.blank? && !dynamic + @value = default_value + end + end + def initialize_with_given_value(given_value) self.default_value = given_value end diff --git a/app/models/resource_action_workflow.rb b/app/models/resource_action_workflow.rb index afa343aa0fd..0e61a3c99fc 100644 --- a/app/models/resource_action_workflow.rb +++ b/app/models/resource_action_workflow.rb @@ -107,7 +107,10 @@ def load_dialog(resource_action, values, options) elsif options[:provision_workflow] || options[:init_defaults] dialog.initialize_value_context(values) dialog.load_values_into_fields(values, false) - elsif options[:refresh] || options[:submit_workflow] + elsif options[:submit_workflow] + dialog.load_values_into_fields(values) + elsif options[:refresh] + dialog.initialize_static_values dialog.load_values_into_fields(values) elsif options[:reconfigure] dialog.initialize_with_given_values(values) diff --git a/spec/models/dialog_field_spec.rb b/spec/models/dialog_field_spec.rb index a45d8b670d4..16049ec2182 100644 --- a/spec/models/dialog_field_spec.rb +++ b/spec/models/dialog_field_spec.rb @@ -162,6 +162,58 @@ end end + describe "#initialize_static_values" do + let(:field) { described_class.new(:dynamic => dynamic, :value => value) } + let(:field_with_default) { described_class.new(:dynamic => dynamic, :value => value, :default_value => "test") } + + context "when the field is dynamic" do + let(:dynamic) { true } + + context "when the value is blank" do + let(:value) { "" } + let(:automate_value) { "some value from automate" } + + it "does not set the value to the automate value" do + field.initialize_static_values + expect(field.instance_variable_get(:@value)).to eq("") + end + end + + context "when the value is not blank" do + let(:value) { "not blank" } + + it "does not set the value to the automate value" do + field.initialize_static_values + expect(field.instance_variable_get(:@value)).to eq("not blank") + end + end + end + + context "when the field is not dynamic" do + let(:dynamic) { false } + + context "with a user-adjusted value" do + let(:value) { "not dynamic" } + + it "does not adjust the value" do + field.initialize_static_values + expect(field.instance_variable_get(:@value)).to eq("not dynamic") + end + end + + context "without a user-adjusted value" do + context "with a default value" do + let(:value) { nil } + + it "does adjust the value" do + field_with_default.initialize_static_values + expect(field_with_default.instance_variable_get(:@value)).to eq("test") + end + end + end + end + end + describe "#initialize_with_given_value" do let(:field) { described_class.new(:default_value => "not the given value") } diff --git a/spec/models/dialog_spec.rb b/spec/models/dialog_spec.rb index 27d1b8457d3..e79caf5f4d2 100644 --- a/spec/models/dialog_spec.rb +++ b/spec/models/dialog_spec.rb @@ -59,6 +59,30 @@ end end + describe "#initialize_static_values" do + let(:dialog) { described_class.new } + let(:field1) { DialogField.new(:name => "name1") } + let(:field2) { DialogField.new(:name => "name2") } + + before do + allow(dialog).to receive(:dialog_fields).and_return([field1, field2]) + allow(field1).to receive(:initialize_static_values) + allow(field2).to receive(:initialize_static_values) + end + + it "sets the current dialog to each field" do + dialog.initialize_static_values + expect(field1.dialog).to eq(dialog) + expect(field2.dialog).to eq(dialog) + end + + it "calls initialize_static_values on each field" do + expect(field1).to receive(:initialize_static_values) + expect(field2).to receive(:initialize_static_values) + dialog.initialize_static_values + end + end + describe "#content" do it "returns the serialized content" do dialog = FactoryBot.create(:dialog, :description => "foo", :label => "bar") diff --git a/spec/models/resource_action_workflow_spec.rb b/spec/models/resource_action_workflow_spec.rb index f2ee3a74eac..47c2d51af4b 100644 --- a/spec/models/resource_action_workflow_spec.rb +++ b/spec/models/resource_action_workflow_spec.rb @@ -241,6 +241,11 @@ context "when the options are set to a refresh request" do let(:options) { {:refresh => true} } + it "initializes the static values" do + expect(dialog).to receive(:initialize_static_values) + ResourceActionWorkflow.new(values, nil, resource_action, options) + end + it "loads the values into fields" do expect(dialog).to receive(:load_values_into_fields).with(values) ResourceActionWorkflow.new(values, nil, resource_action, options) From bfaff4357f31319e627706b8303dd8476f30b562 Mon Sep 17 00:00:00 2001 From: Erik Clarizio Date: Thu, 18 Jul 2019 10:43:59 -0700 Subject: [PATCH 2/4] Refactor #load_dialog method into smaller manageable private methods https://bugzilla.redhat.com/show_bug.cgi?id=1730813 --- app/models/resource_action_workflow.rb | 60 +++++++++++--------- spec/models/resource_action_workflow_spec.rb | 1 + 2 files changed, 33 insertions(+), 28 deletions(-) diff --git a/app/models/resource_action_workflow.rb b/app/models/resource_action_workflow.rb index 0e61a3c99fc..81e8a332c43 100644 --- a/app/models/resource_action_workflow.rb +++ b/app/models/resource_action_workflow.rb @@ -93,34 +93,6 @@ def create_values_hash } end - def load_dialog(resource_action, values, options) - if resource_action.nil? - resource_action = load_resource_action(values) - @settings[:resource_action_id] = resource_action.id if resource_action - end - - dialog = resource_action.dialog if resource_action - if dialog - dialog.target_resource = @target - if options[:display_view_only] - dialog.init_fields_with_values_for_request(values) - elsif options[:provision_workflow] || options[:init_defaults] - dialog.initialize_value_context(values) - dialog.load_values_into_fields(values, false) - elsif options[:submit_workflow] - dialog.load_values_into_fields(values) - elsif options[:refresh] - dialog.initialize_static_values - dialog.load_values_into_fields(values) - elsif options[:reconfigure] - dialog.initialize_with_given_values(values) - else - dialog.initialize_value_context(values) - end - end - dialog - end - def init_field_hash @dialog.dialog_fields.each_with_object({}) { |df, result| result[df.name] = df } end @@ -151,6 +123,38 @@ def validate(_values = nil) private + def load_dialog(resource_action, values, options) + if resource_action.nil? + resource_action = load_resource_action(values) + @settings[:resource_action_id] = resource_action.id if resource_action + end + + dialog = resource_action.dialog if resource_action + load_proper_dialog_values(dialog, options, values) if dialog + + dialog + end + + def load_proper_dialog_values(dialog, options, values) + dialog.target_resource = @target + + if options[:display_view_only] + dialog.init_fields_with_values_for_request(values) + elsif options[:provision_workflow] || options[:init_defaults] + dialog.initialize_value_context(values) + dialog.load_values_into_fields(values, false) + elsif options[:submit_workflow] + dialog.load_values_into_fields(values) + elsif options[:refresh] + dialog.initialize_static_values + dialog.load_values_into_fields(values) + elsif options[:reconfigure] + dialog.initialize_with_given_values(values) + else + dialog.initialize_value_context(values) + end + end + def create_request?(values) ra = load_resource_action(values) !ra.resource.kind_of?(CustomButton) && has_request_class? diff --git a/spec/models/resource_action_workflow_spec.rb b/spec/models/resource_action_workflow_spec.rb index 47c2d51af4b..8b7764fb79a 100644 --- a/spec/models/resource_action_workflow_spec.rb +++ b/spec/models/resource_action_workflow_spec.rb @@ -215,6 +215,7 @@ allow(ResourceAction).to receive(:find).and_return(resource_action) allow(dialog).to receive(:load_values_into_fields).with(values) allow(dialog).to receive(:initialize_value_context).with(values) + allow(dialog).to receive(:initialize_static_values) allow(dialog).to receive(:init_fields_with_values_for_request).with(values) allow(dialog).to receive(:target_resource=) end From a86428f96982c4947052bdd0f65cdeb6b54d01c3 Mon Sep 17 00:00:00 2001 From: Erik Clarizio Date: Thu, 18 Jul 2019 11:13:36 -0700 Subject: [PATCH 3/4] Remove unnecessary spec https://bugzilla.redhat.com/show_bug.cgi?id=1730813 --- spec/models/dialog_field_spec.rb | 21 ++++----------------- 1 file changed, 4 insertions(+), 17 deletions(-) diff --git a/spec/models/dialog_field_spec.rb b/spec/models/dialog_field_spec.rb index 16049ec2182..0640cbce1e5 100644 --- a/spec/models/dialog_field_spec.rb +++ b/spec/models/dialog_field_spec.rb @@ -168,24 +168,11 @@ context "when the field is dynamic" do let(:dynamic) { true } + let(:value) { "value" } - context "when the value is blank" do - let(:value) { "" } - let(:automate_value) { "some value from automate" } - - it "does not set the value to the automate value" do - field.initialize_static_values - expect(field.instance_variable_get(:@value)).to eq("") - end - end - - context "when the value is not blank" do - let(:value) { "not blank" } - - it "does not set the value to the automate value" do - field.initialize_static_values - expect(field.instance_variable_get(:@value)).to eq("not blank") - end + it "does not change the value" do + field.initialize_static_values + expect(field.instance_variable_get(:@value)).to eq("value") end end From 7298e4f6cf952372e0ab96c25324598318eb30ce Mon Sep 17 00:00:00 2001 From: Erik Clarizio Date: Thu, 18 Jul 2019 15:48:50 -0700 Subject: [PATCH 4/4] Rename parameter for more accurate meaning and pass false for refresh https://bugzilla.redhat.com/show_bug.cgi?id=1730813 --- app/models/dialog.rb | 4 ++-- app/models/resource_action_workflow.rb | 2 +- spec/models/dialog_spec.rb | 8 ++++---- spec/models/resource_action_workflow_spec.rb | 10 +++------- 4 files changed, 10 insertions(+), 14 deletions(-) diff --git a/app/models/dialog.rb b/app/models/dialog.rb index 1838933acd8..d0ccf48f9a6 100644 --- a/app/models/dialog.rb +++ b/app/models/dialog.rb @@ -86,13 +86,13 @@ def validate_field_data end end - def load_values_into_fields(values, overwrite = true) + def load_values_into_fields(values, ignore_nils = true) values = values.with_indifferent_access dialog_field_hash.each_value do |field| field.dialog = self new_value = values[field.automate_key_name] || values[field.name] || values.dig("parameters", field.name) - new_value ||= field.value unless overwrite + new_value ||= field.value unless ignore_nils field.value = new_value end diff --git a/app/models/resource_action_workflow.rb b/app/models/resource_action_workflow.rb index 81e8a332c43..9f5f66a8071 100644 --- a/app/models/resource_action_workflow.rb +++ b/app/models/resource_action_workflow.rb @@ -147,7 +147,7 @@ def load_proper_dialog_values(dialog, options, values) dialog.load_values_into_fields(values) elsif options[:refresh] dialog.initialize_static_values - dialog.load_values_into_fields(values) + dialog.load_values_into_fields(values, false) elsif options[:reconfigure] dialog.initialize_with_given_values(values) else diff --git a/spec/models/dialog_spec.rb b/spec/models/dialog_spec.rb index e79caf5f4d2..bb37fb598f3 100644 --- a/spec/models/dialog_spec.rb +++ b/spec/models/dialog_spec.rb @@ -520,16 +520,16 @@ let(:dialog_group) { DialogGroup.new(:dialog_fields => [dialog_field1, dialog_field2]) } let(:dialog_field2) { DialogField.new(:value => "321", :name => "field2") } - context "when overwrite is true" do - it "sets nil values" do + context "when ignore_nils is true" do + it "accepts nil values as a value" do vars = {:field1 => "10.8.99.248"} dialog.load_values_into_fields(vars) expect(dialog_field2.value).to eq(nil) end end - context "when overwrite is false" do - it "does not set nil values" do + context "when ignore_nils is false" do + it "does not set values to nil" do vars = {:field1 => "10.8.99.248"} dialog.load_values_into_fields(vars, false) expect(dialog_field2.value).to eq("321") diff --git a/spec/models/resource_action_workflow_spec.rb b/spec/models/resource_action_workflow_spec.rb index 8b7764fb79a..a4b2dead4f5 100644 --- a/spec/models/resource_action_workflow_spec.rb +++ b/spec/models/resource_action_workflow_spec.rb @@ -242,13 +242,9 @@ context "when the options are set to a refresh request" do let(:options) { {:refresh => true} } - it "initializes the static values" do - expect(dialog).to receive(:initialize_static_values) - ResourceActionWorkflow.new(values, nil, resource_action, options) - end - - it "loads the values into fields" do - expect(dialog).to receive(:load_values_into_fields).with(values) + it "initializes the static values and then loads the values into fields" do + expect(dialog).to receive(:initialize_static_values).ordered + expect(dialog).to receive(:load_values_into_fields).with(values, false).ordered ResourceActionWorkflow.new(values, nil, resource_action, options) end end