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

Correctly pass dialog field values through request #17839

Closed
wants to merge 2 commits into from

Conversation

d-m-u
Copy link
Contributor

@d-m-u d-m-u commented Aug 10, 2018

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

This is two small changes.

with_indifferent_access added to dialog field lookup

#17642 broke our setting values in fields because previously we were setting the names to strings in the lookup

submit_workflow utilized in requests

The submit_workflow option is used to funnel values correctly through requests.

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 10, 2018

@miq-bot add_label bug
@miq-bot assign @gmcculloug
@miq-bot add_reviewer @eclarizio

@@ -89,6 +89,7 @@ def validate_field_data
end

def load_values_into_fields(values)
values = values.with_indifferent_access
Copy link
Member

Choose a reason for hiding this comment

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

Would this change be required in initialize_with_given_values below where we are doing the same key lookup?

Copy link
Member

Choose a reason for hiding this comment

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

In theory, it wouldn't hurt I guess. In practice that path is only being used by reconfigure and uses the contents of options[:dialog] within a service.

Copy link
Member

Choose a reason for hiding this comment

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

Ok, wanted to make sure we had considered this code path. If you are saying it is good then we do not need the change.

Copy link
Member

@eclarizio eclarizio left a comment

Choose a reason for hiding this comment

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

👍

@d-m-u d-m-u changed the title Add indifferent access to load_vals_into_fields Correctly pass dialog field values through request Aug 10, 2018
@d-m-u d-m-u changed the title Correctly pass dialog field values through request [WIP] Correctly pass dialog field values through request Aug 10, 2018
@d-m-u d-m-u changed the title [WIP] Correctly pass dialog field values through request Correctly pass dialog field values through request Aug 10, 2018
@miq-bot miq-bot changed the title Correctly pass dialog field values through request [WIP] Correctly pass dialog field values through request Aug 10, 2018
@miq-bot miq-bot added the wip label Aug 10, 2018
@@ -380,6 +380,8 @@ def self.create_from_options(options)
private_class_method :create_from_options

def provision_request(user, options = nil, request_options = nil)
request_options ||= {}
request_options[:submit_workflow] = true
Copy link
Member

Choose a reason for hiding this comment

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

Let's change the method definition to use request_options = {}

Then you do not need the first line to initialize it.

@JPrause
Copy link
Member

JPrause commented Aug 10, 2018

@miq-bot add_label blocker

@JPrause
Copy link
Member

JPrause commented Aug 10, 2018

@d-m-u if this will be able to be backported, can you add the gaprindashvili/yes label

@miq-bot
Copy link
Member

miq-bot commented Aug 10, 2018

Checked commits d-m-u/manageiq@bbf06ef~...1d05596 with ruby 2.3.3, rubocop 0.52.1, haml-lint 0.20.0, and yamllint 1.10.0
4 files checked, 0 offenses detected
Everything looks fine. 🏆

@d-m-u d-m-u changed the title [WIP] Correctly pass dialog field values through request Correctly pass dialog field values through request Aug 10, 2018
@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 10, 2018

@miq-bot remove_label wip

@d-m-u
Copy link
Contributor Author

d-m-u commented Aug 10, 2018

@miq-bot add_label gaprindashvili/yes

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.

None yet

5 participants