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

[FINE] Fix choose Provider reset bug #2301

Merged
merged 3 commits into from Nov 21, 2017
Merged

[FINE] Fix choose Provider reset bug #2301

merged 3 commits into from Nov 21, 2017

Conversation

gildub
Copy link
Contributor

@gildub gildub commented Oct 5, 2017

Fixes unexpected error message while adding new Cloud Subnet:

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

@martinpovolny
Copy link

if (id) {
  return;
}

maybe? given the whole function body is under the condition and we love to limit nesting?

@miq-bot miq-bot changed the title Fix choose Provider reset bug [FINE] Fix choose Provider reset bug Oct 5, 2017
@@ -11,10 +11,10 @@
options_for_select([["<#{_('Choose')}>", nil]] + @network_provider_choices.sort),
Copy link
Contributor

Choose a reason for hiding this comment

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

@gildub I also recommend using -

options_for_select([["<#{_('Choose')}>", nil]] + @network_provider_choices.sort, disabled: ["<#{_('Choose')}>", nil]),

So that the user is forced to make a valid selection and cannot choose <Choose>
This change alone should help address the BZ, and you probably don't even need the other changes.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@AparnaKarve, I was trying to align with the code on master branch but you're right that's better. Thanks

@gildub
Copy link
Contributor Author

gildub commented Oct 5, 2017

@martinpovolny, yes that sounds more solid, meanwhile for this BZ @AparnaKarve suggestion would do. Thanks

@simaishi
Copy link
Contributor

@AparnaKarve Is this good to go?

Copy link
Contributor

@AparnaKarve AparnaKarve left a comment

Choose a reason for hiding this comment

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

@simaishi Yes, this is good to go.
Thanks.

@simaishi
Copy link
Contributor

Oh no, there is a conflict now... @gildub please resolve the conflict. Thanks!

@miq-bot
Copy link
Member

miq-bot commented Nov 20, 2017

This pull request is not mergeable. Please rebase and repush.

@gildub
Copy link
Contributor Author

gildub commented Nov 20, 2017

@simaishi, conflict resolved.

@miq-bot
Copy link
Member

miq-bot commented Nov 20, 2017

Checked commits https://github.com/gildub/manageiq-ui-classic/compare/9083d1e0d123d52880ac53499def5cb4586a197f~...714162f54997f2dcc33488b797acea9e2a28ff55 with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
1 file checked, 0 offenses detected
Everything looks fine. 🍰

@simaishi simaishi merged commit 4146560 into ManageIQ:fine Nov 21, 2017
@simaishi simaishi added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 21, 2017
@gildub gildub deleted the cloud_subnet-fix-reset branch November 21, 2017 23:38
@himdel
Copy link
Contributor

himdel commented Jan 15, 2018

This PR introduces several bugs... the controller is using controllerAs even in fine, but this backport isn't.

 +                      "ng-model"                    => "cloudSubnetModel.ems_id",

should be vm.cloudSubnetModel.ems_id.

 +                      "ng-change"                   => "filterNetworkManagerChanged(cloudSubnetModel.ems_id)",

should be vm.filterNetworkManagerChanged(vm.cloudSubnetModel.ems_id) (vm twice).

EDIT: fixed by #3254

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

6 participants