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

Fixed checks around retirement dialog values. #768

Merged
merged 3 commits into from
Mar 23, 2017

Conversation

h-kataria
Copy link
Contributor

Retirement dialog is not a required field, so it is not always defined, checking if it is defined and has a value before setting it in retirement hash to be sent up via API call.

@syncrou please test this should fix the issue you found when trying to save new dialog name on Retirement tab.
@gmcculloug please review

@syncrou
Copy link
Contributor

syncrou commented Mar 22, 2017

@h-kataria - The :new_dialog_id is now passing as expected.

👍 LGTM

@h-kataria h-kataria force-pushed the fix_saving_of_dialog_value branch 2 times, most recently from 476823c to 6b0d2ae Compare March 22, 2017 21:27
- Retirement dialog is not a required field, so it is not always defined, checking if it is defined and has a value before setting it in retirement hash to be sent up via API call.
- Fixed spec test failure
@h-kataria h-kataria force-pushed the fix_saving_of_dialog_value branch from 6b0d2ae to 3ce463c Compare March 22, 2017 21:29
@syncrou
Copy link
Contributor

syncrou commented Mar 23, 2017

@h-kataria - These changes are working for me. I found an issue on my end where I'm not saving the new :dialog_id to the options hash of the current instance of ServiceTemplateAnsiblePlaybook - When I manually save that value, I see the correct Dialog name in the UI under the Retirement tab.

@h-kataria h-kataria force-pushed the fix_saving_of_dialog_value branch from f8124f6 to 60938b1 Compare March 23, 2017 15:52
Added spec test to verify that `fetch_playook_details` sets data for tabs correctly.
@h-kataria h-kataria force-pushed the fix_saving_of_dialog_value branch from 60938b1 to 9709fdf Compare March 23, 2017 15:57
@miq-bot
Copy link
Member

miq-bot commented Mar 23, 2017

Checked commits h-kataria/manageiq-ui-classic@b991bee~...9709fdf with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 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 minus the code climate nit.

catalog_item['config_info']['retirement']['dialog_id'] = configData.retirement_dialog_id;
} else if (configData.retirement_dialog_name !== '')
} else if (angular.isDefined(vm.catalogItemModel.retirement_dialog_name) && configData.retirement_dialog_name !== '')
Copy link
Contributor

Choose a reason for hiding this comment

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

@h-kataria - Seeing a code climate error about not having a { after this if condition.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@syncrou we usually add curly braces around if/else blocks if it has more than 1 statement in the block, so that comment can be ignored

Copy link
Contributor

Choose a reason for hiding this comment

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

@h-kataria - I see now - the removed else if was that way. Looks good to me then.

👍

@dclarizio dclarizio merged commit 2184f6d into ManageIQ:master Mar 23, 2017
@dclarizio dclarizio added this to the Sprint 57 Ending Mar 27, 2017 milestone Mar 23, 2017
@dclarizio dclarizio self-assigned this Mar 23, 2017
@h-kataria h-kataria deleted the fix_saving_of_dialog_value branch April 4, 2017 18:41
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.

4 participants