Add flag to initialize field default values #485

d-m-u · 2018-10-04T20:17:12Z

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1635673

At this point if a service dialog gets ordered via the API, it will not honor defaults on static fields. To remedy this, this adds a flag to the call to also let us call load_values_into_fields(values, false), thus honoring the default values. This builds on the recent changes to ManageIQ/manageiq#17844

@@ -469,6 +476,8 @@ def init_st(service_template, resource_action)
    let(:ra1)     { FactoryGirl.create(:resource_action, :action => "Provision", :dialog => dialog1) }
    let(:st1)     { FactoryGirl.create(:service_template, :name => "service template 1") }
    let(:sc)      { FactoryGirl.create(:service_template_catalog, :name => "sc", :description => "sc description") }
+    let(:product_settings) { double(:run_automate_methods_on_service_catalog_api_submit => run_automate_methods_on_service_catalog_api_submit) }


I don't think product_settings is being used anywhere currently?

d-m-u · 2018-10-09T19:30:28Z

@miq-bot add_reviewer @tinaafitz

JPrause · 2018-10-09T19:42:56Z

@d-m-u if this will be able to be backported, please add the gaprindashvili/yes and hammer/yes labels

JPrause · 2018-10-09T19:43:03Z

@miq-bot add_label blocker

d-m-u · 2018-10-09T19:43:10Z

@miq-bot add_label bug

d-m-u · 2018-10-09T19:43:58Z

@miq-bot add_label gaprindashvili/yes, hammer/yes

eclarizio

Couple more spec comments, but the code itself should be fine

eclarizio · 2018-10-09T19:46:47Z

spec/requests/service_catalogs_spec.rb

@@ -469,6 +476,7 @@ def init_st(service_template, resource_action)
    let(:ra1)     { FactoryGirl.create(:resource_action, :action => "Provision", :dialog => dialog1) }
    let(:st1)     { FactoryGirl.create(:service_template, :name => "service template 1") }
    let(:sc)      { FactoryGirl.create(:service_template_catalog, :name => "sc", :description => "sc description") }
+    let(:run_automate_methods_on_service_catalog_api_submit) { true }


Now that you fixed the product_settings not being used anywhere, this variable isn't being used either. I figured you meant to change line 372 (https://github.com/ManageIQ/manageiq-api/pull/485/files#diff-e129a646c1b31a559a2192293c958cf9R372) to use product_settings which then uses this variable.

eclarizio · 2018-10-09T19:47:04Z

spec/requests/service_templates_spec.rb

+
+    context "with the product setting not allowing automate to run on submit" do
+      let(:template_no_display) { FactoryGirl.create(:service_template, :display => false) }
+      let(:run_automate_methods_on_service_catalog_api_submit) { false }


Is this variable being used anywhere?

AllenBW · 2018-10-11T13:32:49Z

Egad, 's a blocker here, we need eyes far sharper than mine, @abellotti if we turn this into a wrestling match, and I tag you in... 🤼‍♂️

bdunne · 2018-10-11T14:14:00Z

app/controllers/api/mixins/service_templates.rb

@@ -14,6 +15,16 @@ def order_service_template(id, data, scheduled_time = nil)

      private

+      def request_from_ui?


Can you use auth_mechanism here instead? https://github.com/ManageIQ/manageiq-api/pull/488/files#diff-a7e4db9275b9f3963c7d8d24685f6bb8R6

Didn't we determine that it would be better to verify the token instead of solely going on if it's an http basic request versus token to know if the request is coming from the UI? At least that was my understanding from the conversation at #476 (comment).

Maybe that method could be extended or used in another way, but I don't think simply changing to use auth_mechanism is the correct solution.

d-m-u · 2018-10-11T17:26:28Z

@miq-bot assign @abellotti
sorry, AB, I dunno who else to give this to

JPrause · 2018-10-12T14:22:00Z

@bdunne can you review,...Mr. Alberto is tied up at the moment. This is for a blocker issue.

bdunne · 2018-10-15T13:45:15Z

app/controllers/api/mixins/service_templates.rb

@@ -14,6 +15,16 @@ def order_service_template(id, data, scheduled_time = nil)

      private

+      def request_from_ui?
+        return false if request.headers["x-auth-token"].blank?
+        !token_info.empty?


Prefer .present? over !x.empty?

bdunne · 2018-10-15T13:47:47Z

spec/requests/service_catalogs_spec.rb

+    let(:product_settings)                                   { double(:run_automate_methods_on_service_api_submit => run_automate_methods_on_service_api_submit) }
+
+    before do
+      stub_settings_merge(:product => product_settings)


Shouldn't this be a hash rather than a double and let blocks?

bdunne · 2018-10-15T14:07:47Z

spec/requests/service_catalogs_spec.rb

+    let(:ra2)                                                { FactoryGirl.create(:resource_action, :action => "Provision", :dialog => dialog1) }
+    let(:st2)                                                { FactoryGirl.create(:service_template, :name => "service template 2", :display => true) }
+    let(:sc)                                                 { FactoryGirl.create(:service_template_catalog, :name => "sc", :description => "sc description") }
+    let(:run_automate_methods_on_service_api_submit)         { true }


Why is this in a let block?

miq-bot · 2018-10-15T14:12:57Z

Checked commit d-m-u@f2f024a with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
3 files checked, 0 offenses detected
Everything looks fine. 🏆

d-m-u · 2018-10-15T16:41:32Z

Thanks @bdunne!

Add flag to initialize field default values (cherry picked from commit 73eee92) https://bugzilla.redhat.com/show_bug.cgi?id=1635673

simaishi · 2018-10-15T16:45:16Z

Hammer backport details:

$ git log -1
commit d394c47a8c23092acfdd3812ce8cbde59c86f485
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Mon Oct 15 12:40:20 2018 -0400

    Merge pull request #485 from d-m-u/bz1635673
    
    Add flag to initialize field default values
    
    (cherry picked from commit 73eee92f0137aa8546c95e1e25a44e51e901cf90)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1635673

Add flag to initialize field default values (cherry picked from commit 73eee92) https://bugzilla.redhat.com/show_bug.cgi?id=1639413

simaishi · 2018-10-15T18:34:00Z

Gaprindashvili backport details:

$ git log -1
commit 28d00c9389c6e4c9a50a1f3882174ae2ec6e7fc0
Author: Brandon Dunne <brandondunne@hotmail.com>
Date:   Mon Oct 15 12:40:20 2018 -0400

    Merge pull request #485 from d-m-u/bz1635673
    
    Add flag to initialize field default values
    
    (cherry picked from commit 73eee92f0137aa8546c95e1e25a44e51e901cf90)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1639413

d-m-u mentioned this pull request Oct 4, 2018

Add flag for init of defaults in fields ManageIQ/manageiq#18061

Merged

eclarizio suggested changes Oct 4, 2018

View reviewed changes

d-m-u force-pushed the bz1635673 branch 2 times, most recently from 8941245 to 6311303 Compare October 4, 2018 21:12

d-m-u changed the title ~~Add flag to initialize field default values~~ [WIP] Add flag to initialize field default values Oct 4, 2018

d-m-u force-pushed the bz1635673 branch 2 times, most recently from d4c8f5d to b41704a Compare October 4, 2018 22:04

miq-bot added the wip label Oct 5, 2018

eclarizio suggested changes Oct 5, 2018

View reviewed changes

app/controllers/api/mixins/service_templates.rb Outdated Show resolved Hide resolved

d-m-u force-pushed the bz1635673 branch 2 times, most recently from 58c3f88 to 01c7c09 Compare October 8, 2018 12:51

d-m-u changed the title ~~[WIP] Add flag to initialize field default values~~ Add flag to initialize field default values Oct 8, 2018

miq-bot removed the wip label Oct 8, 2018

eclarizio reviewed Oct 9, 2018

View reviewed changes

d-m-u force-pushed the bz1635673 branch from 01c7c09 to b79d50e Compare October 9, 2018 19:30

miq-bot requested a review from tinaafitz October 9, 2018 19:31

miq-bot added blocker bug labels Oct 9, 2018

miq-bot added gaprindashvili/yes labels Oct 9, 2018

eclarizio suggested changes Oct 9, 2018

View reviewed changes

d-m-u force-pushed the bz1635673 branch from b79d50e to 83b764e Compare October 9, 2018 19:53

bdunne reviewed Oct 11, 2018

View reviewed changes

miq-bot assigned abellotti Oct 12, 2018

d-m-u force-pushed the bz1635673 branch from 83b764e to c138ada Compare October 15, 2018 13:10

bdunne reviewed Oct 15, 2018

View reviewed changes

d-m-u force-pushed the bz1635673 branch from c138ada to 218c2f8 Compare October 15, 2018 13:57

bdunne reviewed Oct 15, 2018

View reviewed changes

Add flag to initialize field default values

f2f024a

d-m-u force-pushed the bz1635673 branch from 218c2f8 to f2f024a Compare October 15, 2018 14:09

simaishi closed this Oct 15, 2018

simaishi reopened this Oct 15, 2018

bdunne merged commit 73eee92 into ManageIQ:master Oct 15, 2018

bdunne added this to the Sprint 97 Ending Oct 22, 2018 milestone Oct 15, 2018

bdunne assigned bdunne and unassigned abellotti Oct 15, 2018

d-m-u deleted the bz1635673 branch October 15, 2018 16:40

simaishi pushed a commit that referenced this pull request Oct 15, 2018

Merge pull request #485 from d-m-u/bz1635673

d394c47

Add flag to initialize field default values (cherry picked from commit 73eee92) https://bugzilla.redhat.com/show_bug.cgi?id=1635673

simaishi added hammer/backported and removed hammer/yes labels Oct 15, 2018

simaishi pushed a commit that referenced this pull request Oct 15, 2018

Merge pull request #485 from d-m-u/bz1635673

28d00c9

Add flag to initialize field default values (cherry picked from commit 73eee92) https://bugzilla.redhat.com/show_bug.cgi?id=1639413

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Oct 15, 2018

eclarizio mentioned this pull request Oct 30, 2018

Ensure ServiceTemplate ordering passes through the submit_workflow flag #504

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add flag to initialize field default values #485

Add flag to initialize field default values #485

d-m-u commented Oct 4, 2018 •

edited

Loading

d-m-u commented Oct 4, 2018

eclarizio left a comment

eclarizio left a comment

d-m-u commented Oct 8, 2018

d-m-u commented Oct 9, 2018

eclarizio Oct 9, 2018

d-m-u commented Oct 9, 2018

JPrause commented Oct 9, 2018

JPrause commented Oct 9, 2018

d-m-u commented Oct 9, 2018

d-m-u commented Oct 9, 2018

eclarizio left a comment

eclarizio Oct 9, 2018

eclarizio Oct 9, 2018

AllenBW commented Oct 11, 2018

bdunne Oct 11, 2018

eclarizio Oct 11, 2018

d-m-u commented Oct 11, 2018

JPrause commented Oct 12, 2018

bdunne Oct 15, 2018

bdunne Oct 15, 2018

bdunne Oct 15, 2018

miq-bot commented Oct 15, 2018

d-m-u commented Oct 15, 2018

simaishi commented Oct 15, 2018

simaishi commented Oct 15, 2018

		@@ -14,6 +15,16 @@ def order_service_template(id, data, scheduled_time = nil)

		private

		def request_from_ui?

Add flag to initialize field default values #485

Add flag to initialize field default values #485

Conversation

d-m-u commented Oct 4, 2018 • edited Loading

Related:

d-m-u commented Oct 4, 2018

eclarizio left a comment

Choose a reason for hiding this comment

eclarizio left a comment

Choose a reason for hiding this comment

d-m-u commented Oct 8, 2018

d-m-u commented Oct 9, 2018

Choose a reason for hiding this comment

d-m-u commented Oct 9, 2018

JPrause commented Oct 9, 2018

JPrause commented Oct 9, 2018

d-m-u commented Oct 9, 2018

d-m-u commented Oct 9, 2018

eclarizio left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AllenBW commented Oct 11, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

d-m-u commented Oct 11, 2018

JPrause commented Oct 12, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

miq-bot commented Oct 15, 2018

d-m-u commented Oct 15, 2018

simaishi commented Oct 15, 2018

simaishi commented Oct 15, 2018

d-m-u commented Oct 4, 2018 •

edited

Loading