Security group: Fix Display #2543

gildub · 2017-10-30T10:07:54Z

The security group rules are not displayed properly any more, this seems to be related with "selectpicker-for-select-tag" => "", removing them restores normal behaviour of ng-repeat and ng-options

Also

Removes ng-selected which is not needed and not recommended along ng-repeat which takes care of it.
Changes security_groups_list retrieval to proper synchronous flow.
Simplifies copy of data to Model using Object.assign and relationships in form.
Better reset using cloneDeep and cleanup for firewall_rules.

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

gildub · 2017-11-01T00:14:07Z

@dclarizio, @martinpovolny, @himdel, could you please review as this one high priority because Security Groups Rules CRUD is a new feature to the new release. This was working before some change impacted it (see description).

This is now working fine, besides some cosmetic effect on the table row (font), well I've no idea where selectpicker-for-select-tag css class is defined, it's not in bootstrap-select so I'm assuming there is some default vs configuration hell happening somewhere ;).

Thanks

himdel · 2017-11-07T16:19:18Z

@gildub selectpicker-for-select-tag is the directive used to initialize the selectpicker component. It is defined in app/assets/javascripts/directives/selectpickerForSelectTag.js .. and either selectpicker-for-select-tag or pf-select (from angular-patternfly 3.23) are needed, otherwise the selects are styled wrong.

(Also, you may need to rebase, looks like Travis is failing more than it should :).)

gildub · 2017-11-08T10:55:57Z

@himdel, yeah I saw Travis issues last week, BTW thanks for tackling it. Rebased. Thanks for the details about selectpicker. My tests shows that when using it it breaks the table. So that's a blocker.

himdel · 2017-11-08T12:15:55Z

My tests shows that when using it it breaks the table

Do you have a screenshot please? (Or, is the breakage visual or..?)

gildub · 2017-11-09T00:38:09Z

@himdel, the symptoms are that neither the data model are showing up and the select options are not available in the drop downs.

This is the actual broken situation:

And this is what it should look like with current patch, which actually fix other things as well but the selectpicker-for-select-tag removal fixes the display issue:

ZitaNemeckova · 2017-11-17T09:55:24Z

BZ has blocker? flag so adding corresponding label
@miq-bot add_label blocker

himdel · 2017-11-17T12:27:55Z

app/assets/javascripts/controllers/security_group/security_group_form_controller.js

+        miqService.sparkleOn();
+        API.get("/api/security_groups/" + securityGroupFormId + "?attributes=name,ext_management_system.name,description,cloud_tenant.name,firewall_rules").then(function(data) {
+          Object.assign(vm.securityGroupModel, data);
+          vm.securityGroupModel.firewall_rules_delete = false;        vm.securityGroupModel.name = data.name;


why that vm.securityGroupModel.name = data.name; ? Isn't that handled by the assign too? (But if not, formatting..)

Yes, that's a residual cut mistake, removed, thanks!

himdel · 2017-11-17T12:28:52Z

OK, tested in the UI, this is definitely better than before :).

I think loading the security group data should not have to wait for the security group list query, but if this fixes a bug.. :).

Also, the table is too wide now, if you have security groups with long names - previously, the select was limited to a specific width, now it grows so wide I can't see the right side.

I do agree that you don't want the old approach with selectpicker-for-select-tag.

But, I don't see why you can't use that pf-select...

From what I can tell, it actually makes the table a bit better:

current state (plain <select>):

pf-select:

himdel · 2017-11-17T12:41:33Z

Ok, some progres..

removing that "data-width" => "auto", from those select elements makes pf-select do the right thing:

himdel · 2017-11-17T12:50:36Z

app/assets/javascripts/controllers/security_group/security_group_form_controller.js

-      miqService.sparkleOn();
-      API.get("/api/security_groups/" + securityGroupFormId + "?attributes=name,ext_management_system.name,description,cloud_tenant.name,firewall_rules").then(function(data) {
-        vm.securityGroupModel.name = data.name;
-        vm.securityGroupModel.ems_name = data.ext_management_system.name;


It looks like you're removing ems_name completely - is that intentional?

The same goes for cloud_tenant_name.

(Though it looks like it's OK, here.. you're saving via the UI, and I don't see any params[:ems_name] or params[:cloud_tenant_name] in the controller.)

EDIT: verified only name, description, ems_id, cloud_tenant_id and firewall_rules seem to be used by ruby side.

The name is only for the display.

himdel · 2017-11-17T13:07:27Z

Ok, this is the diff for the style part..

diff --git a/app/views/security_group/_fw_rules.haml b/app/views/security_group/_fw_rules.haml
index 51bca0bd1..e9667cc93 100644
--- a/app/views/security_group/_fw_rules.haml
+++ b/app/views/security_group/_fw_rules.haml
@@ -23,32 +23,32 @@
       %tr{"ng-repeat" => "fw_rule in vm.securityGroupModel.firewall_rules",
           "ng-form"   => "rowForm",
           "ng-if"     => "!fw_rule.deleted"}
-        %td.table-view-pf-select
+        %td
           %select{"name"                        => "direction",
-            "data-width"                  => "auto",
-            "ng-model"                    => "vm.securityGroupModel.firewall_rules[$index].direction",
-            "ng-options"                  => "direction for direction in vm.directions",
-            "required"                    => ""}
-        %td.table-view-pf-select
+                  "ng-model"                    => "vm.securityGroupModel.firewall_rules[$index].direction",
+                  "ng-options"                  => "direction for direction in vm.directions",
+                  "required"                    => "",
+                  'pf-select'                   => true}
+        %td
           %select{"name"                        => "network_protocol",
-            "data-width"                  => "auto",
-            "ng-model"                    => "vm.securityGroupModel.firewall_rules[$index].network_protocol",
-            "ng-options"                  => "network_protocol for network_protocol in vm.networkProtocols"}
+                  "ng-model"                    => "vm.securityGroupModel.firewall_rules[$index].network_protocol",
+                  "ng-options"                  => "network_protocol for network_protocol in vm.networkProtocols",
+                  'pf-select'                   => true}
             %option{"value" => ""}
               = "<#{_('Choose')}>"
-        %td.table-view-pf-select
+        %td
           %select{"name"                        => "host_protocol",
-            "data-width"                  => "auto",
-            "ng-model"                    => "vm.securityGroupModel.firewall_rules[$index].host_protocol",
-            "ng-options"                  => "host_protocol for host_protocol in vm.hostProtocols"}
+                  "ng-model"                    => "vm.securityGroupModel.firewall_rules[$index].host_protocol",
+                  "ng-options"                  => "host_protocol for host_protocol in vm.hostProtocols",
+                  'pf-select'                   => true}
             %option{"value" => ""}
               = "<#{_('Choose')}>"
-        %td.table-view-pf-select
+        %td
           %select{"name"                        => "source_security_group_id",
-            "data-width"                  => "auto",
-            "ng-model"                    => "vm.securityGroupModel.firewall_rules[$index].source_security_group_id",
-            "ng-options"                  => "sg.id as sg.name + ' - ' + sg.ems_ref for sg in vm.security_groups_list",
-            "ng-change"                   => "vm.securityGroupModel.firewall_rules[$index].source_ip_range = null"}
+                  "ng-model"                    => "vm.securityGroupModel.firewall_rules[$index].source_security_group_id",
+                  "ng-options"                  => "sg.id as sg.name + ' - ' + sg.ems_ref for sg in vm.security_groups_list",
+                  "ng-change"                   => "vm.securityGroupModel.firewall_rules[$index].source_ip_range = null",
+                  'pf-select'                   => true}
             %option{"value" => ""}
               = "<#{_('Choose')}>"
         %td

Does that work for you? :)

(It also removes the table-view-pf-select class, since that's intended for the checkbox column in lists.)

himdel · 2017-11-17T13:39:23Z

I think loading the security group data should not have to wait for the security group list query, but if this fixes a bug.. :).

OK, from the BZ, I can't tell what the bug really was, but if having the requests go one by one is not part of the fix, I'd rather you made them run in parallel..

Does this work for you..?

diff --git a/app/assets/javascripts/controllers/security_group/security_group_form_controller.js b/app/assets/javascripts/controllers/security_group/security_group_form_controller.js
index c0b498731..290b6665d 100644
--- a/app/assets/javascripts/controllers/security_group/security_group_form_controller.js
+++ b/app/assets/javascripts/controllers/security_group/security_group_form_controller.js
@@ -1,4 +1,4 @@
-ManageIQ.angular.app.controller('securityGroupFormController', ['securityGroupFormId', 'miqService', 'API', function(securityGroupFormId, miqService, API) {
+ManageIQ.angular.app.controller('securityGroupFormController', ['securityGroupFormId', 'miqService', 'API', '$q', function(securityGroupFormId, miqService, API, $q) {
   var vm = this;
 
   var init = function() {
@@ -18,23 +18,38 @@ ManageIQ.angular.app.controller('securityGroupFormController', ['securityGroupFo
     vm.newRecord = securityGroupFormId === "new";
     vm.saveable = miqService.saveable;
 
-    API.get("/api/security_groups/?expand=resources&attributes=ems_ref,id,name").then(function(data) {
-      vm.security_groups_list = data.resources;
-    }).then(function() {
-      if (vm.newRecord) {
-        vm.afterGet = true;
-        vm.modelCopy = angular.copy( vm.securityGroupModel );
-      } else {
-        miqService.sparkleOn();
-        API.get("/api/security_groups/" + securityGroupFormId + "?attributes=name,ext_management_system.name,description,cloud_tenant.name,firewall_rules").then(function(data) {
+    if (vm.newRecord) {
+      vm.afterGet = true;
+      vm.modelCopy = angular.copy( vm.securityGroupModel );
+      getSecurityGroups()
+        .catch(miqService.handleFailure);
+    } else {
+      miqService.sparkleOn();
+
+      $q.all([getSecurityGroup(securityGroupFormId), getSecurityGroups()])
+        .then(function(array) {
+          var data = array[0];
+
           Object.assign(vm.securityGroupModel, data);
-          vm.securityGroupModel.firewall_rules_delete = false;        vm.securityGroupModel.name = data.name;
+          vm.securityGroupModel.firewall_rules_delete = false;
           vm.afterGet = true;
           vm.modelCopy = _.cloneDeep(vm.securityGroupModel);
+
           miqService.sparkleOff();
-        }).catch(miqService.handleFailure);
-      }
-    }).catch(miqService.handleFailure);
+        })
+        .catch(miqService.handleFailure);
+    }
+  };
+
+  var getSecurityGroup = function(id) {
+    return API.get("/api/security_groups/" + id + "?attributes=name,ext_management_system.name,description,cloud_tenant.name,firewall_rules");
+  };
+
+  var getSecurityGroups = function() {
+    return API.get("/api/security_groups/?expand=resources&attributes=ems_ref,id,name")
+      .then(function(data) {
+        vm.security_groups_list = data.resources;
+      });
   };
 
   vm.addClicked = function() {

himdel · 2017-11-17T13:46:02Z

.. alternately with this, if the idea was to wait for securityGroups with afterGet, in the new case...

diff --git a/app/assets/javascripts/controllers/security_group/security_group_form_controller.js b/app/assets/javascripts/controllers/security_group/security_group_form_controller.js
index 290b6665d..96a9cc8f0 100644
--- a/app/assets/javascripts/controllers/security_group/security_group_form_controller.js
+++ b/app/assets/javascripts/controllers/security_group/security_group_form_controller.js
@@ -19,9 +19,11 @@ ManageIQ.angular.app.controller('securityGroupFormController', ['securityGroupFo
     vm.saveable = miqService.saveable;
 
     if (vm.newRecord) {
-      vm.afterGet = true;
-      vm.modelCopy = angular.copy( vm.securityGroupModel );
       getSecurityGroups()
+        .then(function() {
+          vm.afterGet = true;
+          vm.modelCopy = angular.copy( vm.securityGroupModel );
+        })
         .catch(miqService.handleFailure);
     } else {
       miqService.sparkleOn();

miq-bot · 2017-11-20T06:58:18Z

Checked commits https://github.com/gildub/manageiq-ui-classic/compare/c204cd0922e1f44cf49bfc3c6a6ade65871f27bf~...108543cec2042f04e88846604d8e1bb60d2c418f with ruby 2.3.3, rubocop 0.47.1, haml-lint 0.20.0, and yamllint 1.10.0
0 files checked, 0 offenses detected
Everything looks fine. 🏆

gildub · 2017-11-20T09:40:25Z

@himdel, thanks for the investigation.

The initial display regression issue is addressed by the first commit: 99aac08. Sorry I didn't make that obvious enough.

I've restored the pf-select part and removed the auto-width, that works bit width size are not optimal.

I'm not sure why I shared the security-groups-list (getSecurityGroups) for both, it's only needed for edit, fixed that too.

himdel · 2017-11-20T13:50:56Z

OK, ignoring the parallel request issue and merging. (created #2771)

Security group: Fix Display (cherry picked from commit 87eee0f)

simaishi · 2017-11-21T15:18:49Z

Gaprindashvili backport details:

$ git log -1
commit 94cb7a03070cbdfb4dd7dded02f4a14f11126666
Author: Martin Hradil <himdel@seznam.cz>
Date:   Mon Nov 20 13:50:55 2017 +0000

    Merge pull request #2543 from gildub/security_group_fix
    
    Security group: Fix Display
    (cherry picked from commit 87eee0f227529e27cc87bcc6e566303a07c94a06)

miq-bot added the wip label Oct 30, 2017

gildub changed the title ~~[WIP] Security group fix~~ Security group fix Nov 1, 2017

gildub changed the title ~~Security group fix~~ Security group: Display Issue fixed Nov 1, 2017

miq-bot removed the wip label Nov 1, 2017

miq-bot added the bug label Nov 1, 2017

martinpovolny added the gaprindashvili/yes label Nov 1, 2017

dclarizio assigned himdel Nov 2, 2017

dclarizio added the compute/cloud label Nov 2, 2017

gildub changed the title ~~Security group: Display Issue fixed~~ Security group: Fix Display Nov 15, 2017

miq-bot added the blocker label Nov 17, 2017

himdel reviewed Nov 17, 2017

View reviewed changes

gildub added 4 commits November 20, 2017 17:12

no select-picker-for-select-tag and no ng-selected

c204cd0

security_group_list flow and cloneDeep use

273615d

Restore 'pf-select'; fix cut error

f509a05

security_groups_list only for edit

108543c

himdel merged commit 87eee0f into ManageIQ:master Nov 20, 2017

himdel added this to the Sprint 74 Ending Nov 27, 2017 milestone Nov 20, 2017

himdel mentioned this pull request Nov 20, 2017

Security groups - fix API access #2771

Merged

himdel added networks and removed compute/cloud labels Nov 20, 2017

gildub deleted the security_group_fix branch November 20, 2017 20:12

simaishi pushed a commit that referenced this pull request Nov 21, 2017

Merge pull request #2543 from gildub/security_group_fix

94cb7a0

Security group: Fix Display (cherry picked from commit 87eee0f)

simaishi added gaprindashvili/backported and removed gaprindashvili/yes labels Nov 21, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Security group: Fix Display #2543

Security group: Fix Display #2543

gildub commented Oct 30, 2017 •

edited

Loading

gildub commented Nov 1, 2017 •

edited

Loading

himdel commented Nov 7, 2017

gildub commented Nov 8, 2017 •

edited

Loading

himdel commented Nov 8, 2017

gildub commented Nov 9, 2017

ZitaNemeckova commented Nov 17, 2017

himdel Nov 17, 2017

gildub Nov 20, 2017

himdel commented Nov 17, 2017 •

edited

Loading

himdel commented Nov 17, 2017

himdel Nov 17, 2017 •

edited

Loading

gildub Nov 20, 2017

himdel commented Nov 17, 2017

himdel commented Nov 17, 2017

himdel commented Nov 17, 2017 •

edited

Loading

miq-bot commented Nov 20, 2017

gildub commented Nov 20, 2017

himdel commented Nov 20, 2017 •

edited

Loading

simaishi commented Nov 21, 2017

Security group: Fix Display #2543

Security group: Fix Display #2543

Conversation

gildub commented Oct 30, 2017 • edited Loading

gildub commented Nov 1, 2017 • edited Loading

himdel commented Nov 7, 2017

gildub commented Nov 8, 2017 • edited Loading

himdel commented Nov 8, 2017

gildub commented Nov 9, 2017

ZitaNemeckova commented Nov 17, 2017

himdel Nov 17, 2017

Choose a reason for hiding this comment

gildub Nov 20, 2017

Choose a reason for hiding this comment

himdel commented Nov 17, 2017 • edited Loading

himdel commented Nov 17, 2017

himdel Nov 17, 2017 • edited Loading

Choose a reason for hiding this comment

gildub Nov 20, 2017

Choose a reason for hiding this comment

himdel commented Nov 17, 2017

himdel commented Nov 17, 2017

himdel commented Nov 17, 2017 • edited Loading

miq-bot commented Nov 20, 2017

gildub commented Nov 20, 2017

himdel commented Nov 20, 2017 • edited Loading

simaishi commented Nov 21, 2017

gildub commented Oct 30, 2017 •

edited

Loading

gildub commented Nov 1, 2017 •

edited

Loading

gildub commented Nov 8, 2017 •

edited

Loading

himdel commented Nov 17, 2017 •

edited

Loading

himdel Nov 17, 2017 •

edited

Loading

himdel commented Nov 17, 2017 •

edited

Loading

himdel commented Nov 20, 2017 •

edited

Loading