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 Service Dialog not saving default value <None> for drop down or radio button #14240

Merged
merged 3 commits into from
Mar 9, 2017

Conversation

eclarizio
Copy link
Member

This will allow nil to be a default value for static sorted item fields. When the field is required, the user will be able to pick a "" options which corresponds to the nil option, and a "" option otherwise.

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

/cc @gmcculloug Please review/test

@miq-bot assign @gmcculloug
@miq-bot add_label bug, euwe/yes

@eclarizio eclarizio force-pushed the BZ1428133 branch 2 times, most recently from 20c839e to 0227e27 Compare March 9, 2017 19:03
def raw_values
@raw_values ||= dynamic ? values_from_automate : static_raw_values

self.value ||= default_value if @raw_values.collect { |value_pair| value_pair[0] }.include?(default_value)
Copy link
Member

Choose a reason for hiding this comment

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

@eclarizio Would suggest pulling this logic out into a separate private method with a descriptive name. Would make the raw_values method easier to read and make the logic of this line clearer as well.

Copy link
Member

@gmcculloug gmcculloug left a comment

Choose a reason for hiding this comment

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

Minor suggestion for readability but overall this looks good.

Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

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

Looks great minus a small request to extract behavior out of raw_values

@@ -91,7 +91,7 @@ def sort_data(data_to_sort)
end

def raw_values
@raw_values ||= dynamic ? values_from_automate : self[:values].to_miq_a
@raw_values ||= dynamic ? values_from_automate : static_raw_values
unless @raw_values.collect { |value_pair| value_pair[0] }.include?(default_value)
Copy link
Contributor

Choose a reason for hiding this comment

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

@eclarizio - I would second @gmcculloug - Can we extract this out into a private method as well - I'm getting lost in what the block is trying to accomplish.

@miq-bot
Copy link
Member

miq-bot commented Mar 9, 2017

Checked commits eclarizio/manageiq@c1a02a6~...d530fb7 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
5 files checked, 0 offenses detected
Everything looks good. 🍪

Copy link
Contributor

@syncrou syncrou left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me.

@gmcculloug gmcculloug merged commit a912f50 into ManageIQ:master Mar 9, 2017
@gmcculloug gmcculloug added this to the Sprint 56 Ending Mar 13, 2017 milestone Mar 9, 2017
eclarizio pushed a commit to eclarizio/manageiq that referenced this pull request Mar 9, 2017
Fix for Service Dialog not saving default value <None> for drop down or radio button
(cherry-picked from commit a912f50)

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

Backported to Euwe via #14259

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.

5 participants