Fix and modernise cloud volume forms #950
Conversation
|
@miha-plesko This is the one that we discussed earlier today. Please, have a look at the changes and do another round of testing. |
gberginc
force-pushed
the
fix/cloud_volume_forms
branch
from
bbc2fe3 to
128f446
Compare
gberginc
changed the title
| aws_encryption: false, | ||
| incremental: false, | ||
| // storage_manager_id: storageManagerId, |
gberginc
force-pushed
the
fix/cloud_volume_forms
branch
2 times, most recently
from
ba50938 to
2089688
Compare
|
@gberginc Thank you for those comprehensive videos that you put together. |
| break; | ||
| } | ||
| }; | ||
|
|
||
| $scope.canModifyVolumeSize = function() { |
There was a problem hiding this comment.
Can you define this function on vm?
vm.canModifyVolumeSize = function() {
| // editin Amazon EBS volume whose type is not magnetic (all other volume types | ||
| // can be resized on the fly). | ||
| return vm.newRecord || | ||
| (vm.cloudVolumeModel.emstype == 'ManageIQ::Providers::Amazon::StorageManager::Ebs' && |
| $scope.angularForm.$setPristine(true); | ||
| miqService.miqFlash("warn", "All changes have been reset"); | ||
| }; | ||
|
|
||
| $scope.storageManagerChanged = function(id) { |
There was a problem hiding this comment.
Define the function on vm instead of $scope.
vm.storageManagerChanged = function(id) {
| .then(getStorageManagerFormData) | ||
| .catch(miqService.handleFailure); | ||
| }; | ||
|
|
||
| $scope.sizeChanged = function(size) { |
There was a problem hiding this comment.
vm.sizeChanged = function(size) {
| "ng-model" => "vm.cloudVolumeModel.storage_manager_id", | ||
| "ng-options" => "mgr.id as mgr.name for mgr in vm.storageManagers", | ||
| "ng-change" => "storageManagerChanged(vm.cloudVolumeModel.storage_manager_id)", | ||
| "required" => "", |
There was a problem hiding this comment.
Even though you have the required here, for some reason it does not show up as a Required field for me.
There was a problem hiding this comment.
The problem was with the way I was setting the storage manager id. Thanks to @miha-plesko for discovering this: 7e54df7
| @@ -1,4 +1,8 @@ | |||
| %form#form_div{:name => "angularForm", 'ng-controller' => "cloudVolumeFormController"} | |||
| %form#form_div{:name => "angularForm", | |||
There was a problem hiding this comment.
Where exactly is this form in the UI?
There was a problem hiding this comment.
It's used for OpenStack provider: Restore from a backup of this Cloud Volume. I am unable to actually create a backup on our OpenStack testbed, so I could only ensure that the functions are being called exactly the same as they were called before.
|
@gberginc To complete the Functionality-wise, this is working great. |
| %input.form-control{"bs-switch" => "", | ||
| :data => {:on_text => _('Yes'), :off_text => _('No'), :size => 'mini'}, | ||
| :type => "checkbox", | ||
| :name => "encryption", |
There was a problem hiding this comment.
Please change the name to aws_encryption here.
Since the checkchange directive looks for the same name that has been specified in the model.
| :type => "checkbox", | ||
| :name => "encryption", | ||
| 'ng-model' => "vm.cloudVolumeModel.aws_encryption", | ||
| 'ng-disabled' => "!vm.newRecord", |
There was a problem hiding this comment.
Also noticed that ng-disabled does not seem to work.
I can change the value when I'm in edit mode.
If I remove the bs-switch and have a regular checkbox instead, ng-disabled works.
I'm looking into it...
There was a problem hiding this comment.
Ok, so to disable bs-switch, you need to use the switch-readonly directive in place of ng-disabled
So swap ng-disabled with switch-readonly
'switch-readonly' => "!vm.newRecord"
There was a problem hiding this comment.
True, this was something I forgot to ask you, what is the correct way to disable the switch. Thanks for finding this out: 1089882
There was a problem hiding this comment.
@AparnaKarve There is an issue with this as well. Pressing space key on the bs-switch field will still toggle it.
|
@gberginc I've attached a patch here that shows how to use the |
|
@AparnaKarve I've resolved your comments regarding transition to |
|
@AparnaKarve Since using the generic buttons for |
Yes, that looks just about right. Excellent work on removing Can you go through the codeclimate issues? Some of them seem quite legit. (like the missing semicolon ones) |
gberginc
force-pushed
the
fix/cloud_volume_forms
branch
from
8530f1b to
e622a62
Compare
|
@AparnaKarve codeclimate is now green, including that space after unary Please have a look at the comment regarding |
|
@miq-bot add_label blocker |
|
@miq-bot add_label fine/yes |
|
@AparnaKarve thx for all the reviewing, this looks like it's getting close. Ping me when ready . . . Thx, Dan |
|
@gberginc I was thinking that we could remove the In the JS -- - var managerId = parseInt(storageManagerId, 10);
- if (! isNaN(managerId)) {
- vm.cloudVolumeModel.storage_manager_id = managerId;
- }
+ vm.cloudVolumeModel.storage_manager_id = storageManagerId;In the haml -- - "ng-options" => "mgr.id as mgr.name for mgr in vm.storageManagers",
+ "ng-options" => "mgr.id|number as mgr.name for mgr in vm.storageManagers",
|
|
As far as the |
Introduction of the ability to create cloud volumes for a specific block storage manager broke the remaining forms dealing with cloud volumes. The reason for this was that the Angular controller required additional parameter specifying the target block storage manager. In an attempt to update these forms, various issues were raised (e.g. display of form fields in the new vs. edit form, use of legacy form control tags, mixing of the API and rails controller data, ...). This resulted in a more comprehensive update of the cloud volume forms, introducing two main enhancements (besides fixing the forms): * Use of APIs in Angular controller: some of the data was provided by rails controller while the other was already retrieved through and API. This commit tries to use API as much as possible. * Use of "controller as vm": when rewriting the controller it was easy to introduce this change to bring cloud volume views and controller closer to the current best practices This patch completely removes previous dependency on the `$scope` in the vm controller. Generic buttons are now used for `new` and `edit` forms For all other operations, the buttons section of forms have been updated to ensure `vm` can be used in all parts of the view controller.
gberginc
force-pushed
the
fix/cloud_volume_forms
branch
from
e622a62 to
24b23ea
Compare
|
@AparnaKarve fixed according to your comment and discussion in Gitter. Also tested the provisioning of the new cloud volume (with #1061 applied). |
|
Checked commit gberginc@24b23ea with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 |
|
@gberginc It looks like we have addressed all known issues and then some :) especially with the @dclarizio This is good to merge. |
|
Thanks @AparnaKarve, it really was a pleasure working on this one with you! |
Fix and modernise cloud volume forms (cherry picked from commit 6d5faea) https://bugzilla.redhat.com/show_bug.cgi?id=1444172
|
Fine backport details: |
Fixes https://bugzilla.redhat.com/show_bug.cgi?id=1443188
Introduction of the ability to create cloud volumes for a specific block storage manager broke the remaining forms dealing with cloud volumes. The reason for this was that the Angular controller required additional parameter specifying the target block storage manager.
In an attempt to update these forms, various issues were raised (e.g. display of form fields in the new vs. edit form, use of legacy form control tags, mixing of the API and rails controller data, ...). This resulted in a more comprehensive update of the cloud volume forms, introducing two main enhancements (besides fixing the forms):
Use of APIs in Angular controller: some of the data was provided by rails controller while the other was already retrieved through and API. This commit tries to use API as much as possible.
Use of "controller as vm": when rewriting the controller it was easy to introduce this change to bring cloud volume views and controller closer to the current best practices
I've made five videos showing different operations on cloud volumes (all show OpenStack Cinder and Amazon EBS):
This replaces two other PRs that actually lead to the decision to make more massive change:
@miq-bot add_label wip