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 vm_common controllers to use ControllerAs #1233

Merged

Conversation

ZitaNemeckova
Copy link
Contributor

@ZitaNemeckova ZitaNemeckova commented May 3, 2017

vmCloudAssociateFloatingIpFormController:
OpenStack Instance summary page -> Configuration -> Associate a Floating IP with this Instance

vmCloudDisassociateFloatingIpFormController:
OpenStack Instance summary page -> Configuration -> Disassociate a Floating IP with this Instance

vmCloudAttachFormController:
OpenStack Instance summary page -> Configuration -> Attach a Cloud Volume to this Instance

vmCloudDetachFormController:
OpenStack Instance summary page -> Configuration -> Detach a Cloud Volume to this Instance

vmCloudLiveMigrateFormController:
OpenStack Instance summary page -> Lifecycle -> Migrate selected Instance
vm_clouds_center_tb -> Lifecycle -> Migrate selected Instances

vmCloudEvacuateFormController:
OpenStack Instance summary page -> Lifecycle -> Evacuate selected Instance
vm_clouds_center_tb -> Lifecycle -> Evacuate selected Instances

vmCloudResizeFormController:
OpenStack Instance summary page -> Configuration -> Reconfigure this Instance
vm_clouds_center_tb -> Configuration -> Reconfigure this Instance

To have unless @explorer part of partial (vm_clouds_center_tb):
Compute -> Cloud -> Providers -> select OpenStack one -> in summary page select Instances/Vms -> select one or more Instances/VMs -> Lifecycle -> etc...

@miq-bot add_label angular dialogs, technical debt, wip

https://www.pivotaltracker.com/story/show/143026077
https://www.pivotaltracker.com/story/show/143026061
https://www.pivotaltracker.com/story/show/143026049
https://www.pivotaltracker.com/story/show/143026033
https://www.pivotaltracker.com/story/show/143026085

TODO:

  • uses as vm
  • ManageIQ.angular.scope points to vm
  • does not inject $scope unless needed for a $watch or events, or angularForm
  • using a common button partial - cannot :(
  • uses miq-form
  • uses form-changed, no checkchange
  • no separate edit & new, use the same partial
  • specs
  • no copying values one by one - use Object.assign - I think that's valid even when only one value is being copied.
  • no miqAjaxButton(url, true) - always submit the actual model, don't rely on magic form serialization
  • no extra $setPristine etc. where it doesn't make sense (form submit, form reset..)
  • no if (condition) return true; else return false;
  • no miqrequired, use required or ng-required

@miq-bot
Copy link
Member

miq-bot commented May 3, 2017

@ZitaNemeckova Cannot apply the following label because they are not recognized: angular_dialogs

@miq-bot miq-bot changed the title Convert associate_floating_ip to use ControllerAs [WIP] Convert associate_floating_ip to use ControllerAs May 3, 2017
@ZitaNemeckova ZitaNemeckova changed the title [WIP] Convert associate_floating_ip to use ControllerAs [WIP] Convert vm_common controllers to use ControllerAs May 3, 2017
@ZitaNemeckova
Copy link
Contributor Author

@miq-bot add_label angular dialogs

@ZitaNemeckova ZitaNemeckova force-pushed the controllerAs_associate_floating_ip branch from aeeb4d3 to 00ddbab Compare May 10, 2017 08:52
@ZitaNemeckova ZitaNemeckova force-pushed the controllerAs_associate_floating_ip branch from 00ddbab to d07261b Compare June 9, 2017 11:40
@ZitaNemeckova
Copy link
Contributor Author

@miq-bot remove_label wip

@miq-bot miq-bot changed the title [WIP] Convert vm_common controllers to use ControllerAs Convert vm_common controllers to use ControllerAs Jun 9, 2017
@miq-bot miq-bot removed the wip label Jun 9, 2017
@ZitaNemeckova
Copy link
Contributor Author

@lgalis please review, thanks :)

@lgalis
Copy link
Contributor

lgalis commented Jun 14, 2017

@ZitaNemeckova - this is what I see when selecting Cloud->Providers->Rellationshhips->Instances-> LiveMigrate for an instance
( Look at the buttons, I know the list of instances is a GTL issue - I have a separate GH issue opened for that). I see the same for Evacuate.
Update - I see the same issue for the buttons when I check an instance in Cloud->Instances _>Live Migrate or Evacuate ( but there is an additional error there for the ReportData Controller, unrelated to your changes).

screenshot from 2017-06-14 12-14-42

$scope.modelCopy = angular.copy( $scope.vmCloudModel );
vm.floating_ips = [];
vm.formId = vmCloudAssociateFloatingIpFormId;
vm.modelCopy = angular.copy( vm.vmCloudModel );

ManageIQ.angular.scope = $scope;
Copy link
Contributor

Choose a reason for hiding this comment

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

= vm

vm.vmCloudModel = { name: '' };
vm.formId = vmCloudAttachFormId;
vm.afterGet = false;
vm.modelCopy = angular.copy( vm.vmCloudModel );

ManageIQ.angular.scope = $scope;
Copy link
Contributor

Choose a reason for hiding this comment

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

= vm

vm.hosts = [];
vm.filtered_hosts = [];
vm.formId = vmCloudLiveMigrateFormId;
vm.modelCopy = angular.copy(vm.vmCloudModel);

ManageIQ.angular.scope = $scope;
Copy link
Contributor

Choose a reason for hiding this comment

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

= vm

@himdel
Copy link
Contributor

himdel commented Jun 22, 2017

.. and you should no longer have to inject $scope in any of those controllers if I'm seeing correctly ;).

@himdel
Copy link
Contributor

himdel commented Jun 22, 2017

The checklist..

EDIT: moved to the PR description

@ZitaNemeckova
Copy link
Contributor Author

@miq-bot add_label wip

@miq-bot miq-bot changed the title Convert vm_common controllers to use ControllerAs [WIP] Convert vm_common controllers to use ControllerAs Jun 23, 2017
@miq-bot miq-bot added the wip label Jun 23, 2017
@ZitaNemeckova ZitaNemeckova force-pushed the controllerAs_associate_floating_ip branch 3 times, most recently from 12d32f4 to 1fc2b79 Compare June 27, 2017 14:21
@ZitaNemeckova ZitaNemeckova mentioned this pull request Jul 12, 2017
@ZitaNemeckova ZitaNemeckova force-pushed the controllerAs_associate_floating_ip branch from 1fc2b79 to 7c4f9ba Compare July 12, 2017 11:21

%div_for_paging{'ng-controller' => "pagingDivButtonGroupController",
'paging_div_buttons_id' => "angular_paging_div_buttons",
'paging_div_buttons_type' => "Submit"}

- unless @explorer
Copy link
Contributor

Choose a reason for hiding this comment

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

This is definitely broken .. looks like you had to move this right in associate, so that the buttons are part of the form .. but forgot to do the same here?

Copy link
Contributor

Choose a reason for hiding this comment

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

Also, submitClicked() and cancelClicked() look suspiciously vm-less ;)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well, %div_for_paging is used elsewhere and it has vm-less submitClicked() and cancelClicked() so I left them to be on $scope for now.

:miqrequired => false,
:checkchange => true}
'ng-model' => "vm.vmCloudModel.device_path",
'ng-maxlength' => 128}
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be maxlength, 128 is a valid expression, but no need to use the ng- version with a constant...

'ng-model' => "vm.vmCloudModel.device_path",
'ng-maxlength' => 128}

%div_for_paging{'ng-controller' => "pagingDivButtonGroupController",
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this moved inside form-horizontal on purpose?

%div_for_paging{'ng-controller' => "pagingDivButtonGroupController",
'paging_div_buttons_id' => "angular_paging_div_buttons",
'paging_div_buttons_type' => "Submit"}
%div_for_paging{'ng-controller' => "pagingDivButtonGroupController",
Copy link
Contributor

Choose a reason for hiding this comment

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

And here..

@ZitaNemeckova ZitaNemeckova force-pushed the controllerAs_associate_floating_ip branch from c3b0a75 to 7c8250e Compare August 7, 2017 14:15
@ZitaNemeckova ZitaNemeckova reopened this Aug 11, 2017

%div_for_paging{'ng-controller' => "pagingDivButtonGroupController",
'paging_div_buttons_id' => "angular_paging_div_buttons",
'paging_div_buttons_type' => "Submit"}


- unless @explorer
Copy link
Contributor

Choose a reason for hiding this comment

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

You want to move this to the right too, otherwise it's not part of the form and can't access the controller..

@himdel
Copy link
Contributor

himdel commented Aug 18, 2017

Tested in the UI:

  • vmCloudAssociateFloatingIpFormController 👍
    • OpenStack Instance summary page -> Configuration -> Associate a Floating IP with this Instance
    • removed the unless @explorer bit
  • vmCloudDisassociateFloatingIpFormController 👍
    • OpenStack Instance summary page -> Configuration -> Disassociate a Floating IP with this Instance
    • removed the unless @explorer bit
  • vmCloudAttachFormController 👍
    • OpenStack Instance summary page -> Configuration -> Attach a Cloud Volume to this Instance
    • unless @explorer already removed
    • but missing sparkleOn - this is a missing { complete: false } in the miqAjaxButton call
  • vmCloudDetachFormController 👍
    • OpenStack Instance summary page -> Configuration -> Detach a Cloud Volume to this Instance
    • unless @explorer already removed
    • but missing sparkleOn - this is a missing { complete: false } in the miqAjaxButton call
  • vmCloudLiveMigrateFormController 👍
    • unless @explorer still present - outside %form, but miqAjaxButton
    • OpenStack Instance summary page -> Lifecycle -> Migrate selected Instance
    • vm_clouds_center tb -> Lifecycle -> Migrate selected Instances (1+!)
    • but doesn't show a list of hosts
  • vmCloudEvacuateFormController 👍
    • unless @explorer still present - outside %form, but miqAjaxButton
    • OpenStack Instance summary page -> Lifecycle -> Evacuate selected Instance
    • vm_clouds_center tb -> Lifecycle -> Evacuate selected Instances (1+!)
  • vmCloudResizeFormController 👍
    • unless @explorer still present - ng-click and inside %form
    • OpenStack Instance summary page -> Configuration -> Reconfigure this Instance
    • vm_clouds_center tb -> Configuration -> Reconfigure this Instance

@@ -45,8 +47,8 @@
= _('Destination Host')
.col-md-8
%select{:name => 'destination_host_id',
'ng-model' => 'vmCloudModel.host',
'ng-options' => 'host.name as host.name for host in hosts track by host.id'}
'ng-model' => 'vm.mCloudModel.host',
Copy link
Contributor

Choose a reason for hiding this comment

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

Typo - mCloudModel should be vmCloudModel

Copy link
Contributor

Choose a reason for hiding this comment

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

If you have trouble testing this...

--- a/app/controllers/mixins/actions/vm_actions/live_migrate.rb
+++ b/app/controllers/mixins/actions/vm_actions/live_migrate.rb
@@ -50,6 +50,10 @@ module Mixins
               hosts = []
             end
           end
+          hosts = [
+            {:name => "Karel", :id => 21},
+            {:name => "Lojza", :id => 123},
+          ]
           render :json => {
             :hosts => hosts
           }

@ZitaNemeckova ZitaNemeckova force-pushed the controllerAs_associate_floating_ip branch 2 times, most recently from 034afb9 to bbc703a Compare September 1, 2017 09:39
@ZitaNemeckova ZitaNemeckova force-pushed the controllerAs_associate_floating_ip branch from bbc703a to 8ba980d Compare September 1, 2017 11:42
@miq-bot
Copy link
Member

miq-bot commented Sep 1, 2017

Checked commits ZitaNemeckova/manageiq-ui-classic@3596313~...8ba980d with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
2 files checked, 1 offense detected

app/controllers/mixins/actions/vm_actions/disassociate_floating_ip.rb

@himdel himdel merged commit 04cbe32 into ManageIQ:master Sep 1, 2017
@himdel himdel added this to the Sprint 68 Ending Sep 4, 2017 milestone Sep 1, 2017
@ZitaNemeckova ZitaNemeckova deleted the controllerAs_associate_floating_ip branch September 12, 2017 14:56
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