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

Angular Form with REST API calls for Playbook Service Template type. #262

Merged
merged 13 commits into from
Feb 22, 2017

Conversation

h-kataria
Copy link
Contributor

@h-kataria h-kataria commented Jan 26, 2017

This is a work in Progress, changes in this PR load an Angular form that consumes REST API to populate/save data in form when user tries to add an 'Ansible Playbook' type Service Template or edit an existing 'Ansible Playbook' type Service Template.

https://www.pivotaltracker.com/story/show/138302747

catalog_item_add

edit_catalog_item

cc @lgalis

@miq-bot
Copy link
Member

miq-bot commented Feb 7, 2017

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

@h-kataria
Copy link
Contributor Author

Need to grab changes from ManageIQ/manageiq#13820 to be able to see "Ansible Playbook" entry in the Catalog Item type drop down when adding a new Catalog Item.

@bzwei
Copy link
Contributor

bzwei commented Feb 9, 2017

Is label Key Value Pairs appropriate? Variables and default values might be a better label.

@h-kataria h-kataria force-pushed the ansible_catalog_item_changes branch from f3cef87 to 2e3de74 Compare February 9, 2017 22:00
@h-kataria
Copy link
Contributor Author

@bzwei updated label on the Key/Value Pairs table.

@lgalis lgalis force-pushed the ansible_catalog_item_changes branch 4 times, most recently from 6be21e2 to 1b2a6f7 Compare February 10, 2017 15:11
@bzwei
Copy link
Contributor

bzwei commented Feb 13, 2017

@h-kataria Please change label Inventory to Hosts.

h-kataria and others added 10 commits February 17, 2017 10:40
This is a work in Progress, changes in this PR load an Angular form that consumes REST API to populate/save data in form when user tries to add an 'Ansible Playbook' type Service Template or edit an existing 'Ansible Playbook' type Service Template.

https://www.pivotaltracker.com/story/show/138302747
Also fixed adding/editing of key/value pairs fields in the form, changed label from "Key Value Pairs" to "Variables and Default values"

https://www.pivotaltracker.com/story/show/138302747
Made changes to mark fields on Provisioning tab as required fields, fields on Retirement tab are not required for now.

https://www.pivotaltracker.com/story/show/138302747
Did some cleanup of redundant code. Added changes to consume API to populate and when add/save form buttons are clicked. Added cloud type drop down to the form to be able to show cloud credentials for only selected type.

https://www.pivotaltracker.com/story/show/138302747
@h-kataria h-kataria force-pushed the ansible_catalog_item_changes branch from f53149a to 22ee313 Compare February 17, 2017 15:41
Made changes to disable save/reset form button on initial load of Edit form

https://www.pivotaltracker.com/story/show/138302747
@h-kataria h-kataria force-pushed the ansible_catalog_item_changes branch from 22ee313 to 2f6125b Compare February 17, 2017 16:53
@h-kataria h-kataria removed the wip label Feb 17, 2017
@h-kataria h-kataria changed the title [WIP] - Angular Form with REST API calls for Playbook Service Template type. Angular Form with REST API calls for Playbook Service Template type. Feb 17, 2017
@h-kataria
Copy link
Contributor Author

@himdel @martinpovolny @gmcculloug ready for review.

@h-kataria h-kataria force-pushed the ansible_catalog_item_changes branch from 179ac25 to 091c1db Compare February 18, 2017 17:27
minor cleanup to address rubocop warnings. Fixed nil error on edit screen

https://www.pivotaltracker.com/story/show/138302747
@h-kataria h-kataria force-pushed the ansible_catalog_item_changes branch from 091c1db to f90a520 Compare February 18, 2017 18:57
@h-kataria
Copy link
Contributor Author

@himdel @martinpovolny please review this is an Ansible PR

@martinpovolny
Copy link
Member

@h-kataria: with my limited Angular skills I see that you use the API, which is very cool! 👍 We need more people writing the UI code this way.

But I have noticed that you use the $scope and that we should avoid, especially for new code.

@himdel
Copy link
Contributor

himdel commented Feb 21, 2017

@h-kataria Agreed, I think this looks good :)

(@martinpovolny, this is using controllerAs, the remaining $scope references will have to stay for a while, until we can get all the directives ready for controllerAs.)

I think there will be some code climate issues though, mostly that we should prefer if (x) { y(); } over if (x) y();.

Also, the list of provider types should probably come from the API at some point instead of being hardcoded here, but not sure if possible right now...

@h-kataria
Copy link
Contributor Author

h-kataria commented Feb 21, 2017

@himdel getting list of Cloud credential types is being worked on, i will be making more changes in a follow up PR once API PR gets merged.

@miq-bot
Copy link
Member

miq-bot commented Feb 21, 2017

Checked commits h-kataria/manageiq-ui-classic@3c5455c~...d39b62c with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 18 offenses detected

app/controllers/catalog_controller.rb

app/views/catalog/_st_angular_form.html.haml

  • ⚠️ - Line 15 - Prefer to_s over string interpolation.
  • ⚠️ - Line 29 - Prefer to_s over string interpolation.

app/views/layouts/angular/_ansible_form_options_angular.html.haml

  • ⚠️ - Line 150 - Line is too long. [204/160]
  • ⚠️ - Line 151 - The = symbol should have one space separating it from code
  • ⚠️ - Line 153 - The = symbol should have one space separating it from code
  • ⚠️ - Line 155 - Avoid defining class in attributes hash for static class names
  • ⚠️ - Line 155 - Line is too long. [163/160]
  • ⚠️ - Line 156 - Line is too long. [201/160]
  • ⚠️ - Line 158 - Line is too long. [203/160]
  • ⚠️ - Line 166 - Line is too long. [240/160]
  • ⚠️ - Line 169 - Don't use parentheses around a literal.
  • ⚠️ - Line 169 - Line is too long. [236/160]

app/views/layouts/angular/_multi_tab_ansible_form_options.html.haml

  • ⚠️ - Line 14 - Prefer to_s over string interpolation.
  • ⚠️ - Line 21 - Prefer to_s over string interpolation.
  • ⚠️ - Line 27 - Prefer to_s over string interpolation.

@gmcculloug
Copy link
Member

Tested PR and looks good. Will need some followup PRs to limit resources but this is great progress and will help others get started with testing. Leave it to the UI team to determine when this is ready to merge.

@h-kataria
Copy link
Contributor Author

@himdel @martinpovolny @dclarizio can you please merge, if it looks good to you guys.

@himdel himdel self-assigned this Feb 22, 2017
@himdel himdel added this to the Sprint 55 Ending Feb 27, 2017 milestone Feb 22, 2017
@himdel himdel merged commit 54b4e77 into ManageIQ:master Feb 22, 2017
@himdel himdel added the euwe/no label Feb 22, 2017
@h-kataria h-kataria deleted the ansible_catalog_item_changes branch March 15, 2017 20:59
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.

7 participants