Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix for $evm.execute not honoring dialog options #17844

Merged
merged 2 commits into from
Aug 13, 2018

Conversation

eclarizio
Copy link
Member

This is kind of a replacement of #17839 after talking a bit about it with @d-m-u and @gmcculloug. It essentially takes the with_indifferent_access part and then forces the control flow by using a different option instead of submit_workflow.

The problem is that when ordering a service via the UI, there is a two step process, first the user loads the dialog which may need to go through automate and determine default values. The user is then presented with the loaded dialog and they can make changes. After making any changes, they can then submit, which is a separate call, but still is going through the same ResourceActionWorkflow logic. This is why submit_workflow was created in the first place.

However, doing this through automate, for example as in the BZ ($evm.execute), the dialog is never actually shown to the user in the first place. So, coming through the same code as the submit_workflow won't work, because it doesn't get the defaults they want, only the values they've supplied.

So, therefore, this PR creates another new flow option, named provision_workflow after the method, which first initializes the value context (which is what potentially calls automate to set up the default values), and then secondly loads the values provided by the $evm.execute call into the fields before being "submitted". We need the with_indifferent_access portion because we are trying to look up the values in the hash with either the field.automate_key_name or field.name, both of which are strings, where the values being passed in are symbols. This is only really relevant for this case because the other way uses the API which handles everything with strings.

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

@miq-bot assign @gmcculloug
@miq-bot add_label gaprindashvili/yes, bug, blocker
@miq-bot add_reviewer @d-m-u

@miq-bot miq-bot requested a review from d-m-u August 10, 2018 21:49
@eclarizio eclarizio changed the title Add specific funnel for provision_workflow Fix for $evm.execute not honoring dialog options Aug 10, 2018
@@ -381,6 +381,7 @@ def self.create_from_options(options)
private_class_method :create_from_options

def provision_request(user, options = nil, request_options = nil)
request_options[:provision_workflow] = true
Copy link
Contributor

@d-m-u d-m-u Aug 11, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do we not need to initialize the request_options if they're nil?

@d-m-u
Copy link
Contributor

d-m-u commented Aug 13, 2018

Other than the one small nit, looks great.

@@ -95,6 +95,9 @@ def load_dialog(resource_action, values, options)
dialog.target_resource = @target
if options[:display_view_only]
dialog.init_fields_with_values_for_request(values)
elsif options[:provision_workflow]
dialog.initialize_value_context(values)
dialog.load_values_into_fields(values)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Testing this in a local setup I see the dialog field is initialized with initialize_value_context but the value gets reset to nil on load_values_into_fields when the caller does not pass in a value.

user = User.current_user = User.find_by(:userid => "admin")
st = ServiceTemplate.find_by(:name => "Test Playbook Service")

# Good
vars = {:credential => nil, :hosts => "88.88.88.88"}
st.provision_request(User.current_user, vars)

# =>  options: {:dialog=>{"dialog_credential"=>nil, "dialog_hosts"=>"88.88.88.88", ...

# Bad
vars = {:credential => nil}
st.provision_request(User.current_user, vars)

# => options: {:dialog=>{"dialog_credential"=>nil, "dialog_hosts"=>nil, ...

@d-m-u
Copy link
Contributor

d-m-u commented Aug 13, 2018

@gmcculloug, per your request, I've confirmed that with these changes we're not introducing a regression about automate running eeetooomanytimes

@eclarizio
Copy link
Member Author

@d-m-u @gmcculloug Feedback should be addressed, re-review please? 😄

@d-m-u
Copy link
Contributor

d-m-u commented Aug 13, 2018

The only concern I have now, @eclarizio, is that now options: {:dialog=>{"dialog_credential"=>"" is empty string when you set it originally to nil. Previous behavior was to be nil if nil and empty string if you passed an empty hash. This might be okay behavior?

@miq-bot
Copy link
Member

miq-bot commented Aug 13, 2018

Checked commits eclarizio/manageiq@4b04629~...f15c18b with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
6 files checked, 0 offenses detected
Everything looks fine. 👍

@gmcculloug
Copy link
Member

Testing locally is now demonstrating the desired behavior of loading the dialog defaults as expected. 👍

@gmcculloug gmcculloug merged commit e8ff2a4 into ManageIQ:master Aug 13, 2018
@gmcculloug gmcculloug added this to the Sprint 92 Ending Aug 13, 2018 milestone Aug 13, 2018
simaishi pushed a commit that referenced this pull request Aug 14, 2018
Fix for $evm.execute not honoring dialog options
(cherry picked from commit e8ff2a4)

Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1615634
@simaishi
Copy link
Contributor

Gaprindashvili backport details:

$ git log -1
commit 3499a21949d4daebeccffcfdee332d44d0ffb327
Author: Greg McCullough <[email protected]>
Date:   Mon Aug 13 16:57:22 2018 -0400

    Merge pull request #17844 from eclarizio/BZ1613935
    
    Fix for $evm.execute not honoring dialog options
    (cherry picked from commit e8ff2a422a9965d700bac5331fd4c5093eb8b436)
    
    Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1615634

@pemcg
Copy link

pemcg commented Aug 15, 2018

Will this affect the behaviour of playbook services run from a button, where the dialog is shown to the user and the request is still submitted through automate?

@gmcculloug
Copy link
Member

@pemcg This change is restoring the previous functionality in that area after some major re-work in the dialog code. The end result is it should be the same as it was before. If you run into any issues please let us know.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants