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

Convert cloud key pair form to angular #9466

Merged

Conversation

dgutride
Copy link
Member

Purpose or Intent

Convert the Compute > Clouds > Key Pairs form to Angular.

screen shot 2016-06-27 at 3 48 38 pm

This included modifying the controller to remove any @edit variables and retrieve cloud providers for the dropdown. The create method was also changed to allow for a POST from the new Angular controller. All other functionality should be identical to the way it currently works.

Steps for Testing/QA

No additional testing is necessary other than verifying the key pair add form works the same way it used to (with the exception of having the required validation on the client side.

@chessbyte
Copy link
Member

/cc @martinpovolny

@AparnaKarve
Copy link
Contributor

@dgutride Thanks for the PR and the quick turnaround!
Overall, looks great and as discussed, I'll be sure to do a detailed review soon.

# REST call for provider choices
def ems_form_choices
assert_privileges("auth_key_pair_cloud_new")
ems_choices = []
Copy link
Member

Choose a reason for hiding this comment

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

no need for this line

@dgutride
Copy link
Member Author

@martinpovolny - I think I've addressed all of your comments (including using rubymine to clean up the double-quotes/single-quote/colon property inconsistencies in the haml).

@AparnaKarve
Copy link
Contributor

@dgutride Can you squash the last 2 Rubocop related commits?

"required" => "",
"pf-select" => true)
:javascript
miqInitSelectPicker();
Copy link
Contributor

Choose a reason for hiding this comment

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

@dgutride You can remove this JS method and instead use the directive selectpicker-for-select-tag for the select_tag

@AparnaKarve
Copy link
Contributor

If possible (without rocking the boat too much) ;), can you squash these 2 commits 760a257b18705c17d6194d4a8cd51b32b8ad699f and 4630ba37d3e27769d8caccff5ff18b1fc338f559 (since the second one is more of an 'undo' action)

@@ -200,6 +105,7 @@ def key_pair_save
:model => ui_lookup(:table => 'auth_key_pair_cloud')
}
end

Copy link
Contributor

Choose a reason for hiding this comment

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

blank line? Can you remove it?

@AparnaKarve
Copy link
Contributor

@dgutride This looks great! I also verified that it works well in the UI 👍
Once you address the comments above, this should be good for a merge!

@dgutride dgutride force-pushed the convert_cloud_key_pairs_to_angular branch from d0e7320 to 94a29cf Compare June 29, 2016 16:00
@dgutride
Copy link
Member Author

@AparnaKarve - I've tried to squash 760a257 and 4630ba3 (since the second one is more of an 'undo' action) but the squash command will only allow me to compress onto the previous commit. Would you like me to flatten that entire set?

@AparnaKarve
Copy link
Contributor

@dgutride Let's discuss this on gitter on how to squash those 2 non-consecutive commits.

:required => '',
'pf-select' => true)
:javascript
miqInitSelectPicker();
Copy link
Contributor

Choose a reason for hiding this comment

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

@dgutride Thought I made a comment here yesterday...but apparently not.
You can remove this :javascript block completely and instead use the directive selectpicker-for-select-tag for the select_tag. Also, you would not need the pf-select directive.

@dgutride dgutride force-pushed the convert_cloud_key_pairs_to_angular branch from 94a29cf to bf6605e Compare June 30, 2016 19:26
@miq-bot
Copy link
Member

miq-bot commented Jun 30, 2016

Checked commits dgutride/manageiq@3580a50~...bf6605e with ruby 2.2.4, rubocop 0.37.2, and haml-lint 0.16.1
2 files checked, 0 offenses detected
Everything looks good. 🍰

@AparnaKarve
Copy link
Contributor

@dclarizio Verified that the form works well in the UI.
So this is good for a merge.

@dclarizio dclarizio merged commit 0fa97dc into ManageIQ:master Jun 30, 2016
@dclarizio dclarizio added this to the Sprint 43 Ending July 11, 2016 milestone Jun 30, 2016
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