Add cloud tenant filtering for various network object forms #1343

tzumainn · 2017-05-13T03:22:31Z

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

himdel · 2017-05-18T14:26:29Z

app/assets/javascripts/controllers/cloud_network/cloud_network_form_controller.js

+      .then(getCloudTenantsByEms)
+      .catch(miqService.handleFailure);
+
+    miqService.sparkleOff();


sparkleOff here doesn't make sense, calling sparkleOn and sparkleOff in the same tick pretty much means it will never start to spin.

I guess you want to sparkleOff in getCloudTenantsByEms.

himdel · 2017-05-18T14:28:12Z

app/views/network_router/new.html.haml

@@ -22,16 +22,21 @@
  %h3
    = _('Placement')
  .form-horizontal
+    .form-group{"ng-if" => "networkRouterModel.ems_id"}


Probably wanted to add ng-if to the next line, instead of adding an empty form-group?

himdel · 2017-05-18T14:31:12Z

app/assets/javascripts/controllers/network_router/network_router_form_controller.js

@@ -105,4 +105,18 @@ ManageIQ.angular.app.controller('networkRouterFormController', ['$http', '$scope
    $scope.available_subnets = data.available_subnets;
    miqService.sparkleOff();
  }
+
+  $scope.filterNetworkManagerChanged = function(id) {


A method with this name already exists on line 64 (network_router)

himdel · 2017-05-18T14:31:44Z

app/assets/javascripts/controllers/floating_ip/floating_ip_form_controller.js

@@ -66,4 +66,19 @@ ManageIQ.angular.app.controller('floatingIpFormController', ['$http', '$scope',
    $scope.available_networks = data.available_networks;
    miqService.sparkleOff();
  }
+
+
+  $scope.filterNetworkManagerChanged = function(id) {


A method with this name already exists on line 47 (floating_ip)

himdel · 2017-05-18T14:37:55Z

I'm also seeing a lot of the same code here being added to 5 or so places.

Any chance it could be shared? The views use different field names so maybe no point in doing a partial with magic params there.. but for example the JS code is always the same, and could be shared via miqService for example..

(Something like..

miqService.filterNetworkManagerChanged = function(scope) {
  return function(id) {
    miqService.sparkleOn();

    API.get("/api/providers/" + id + "/cloud_tenants?expand=resources&attributes=id,name")
      .then(getCloudTenantsByEms)
      .catch(miqService.handleFailure);
  };

  function getCloudTenantsByEms(data) {
    scope.available_tenants = data.resources;
    miqService.sparkleOff();
  }
};

(in miq_service.js)

And then you can do $scope.filterNetworkManagerChanged = miqService.filterNetworkManagerChanged($scope); in all those controllers and that's it :).

And maybe a similar thing could be done on the Ruby side?
)

EDIT: or maybe..

miqService.getProviderTenants = function(callback) {
  return function(id) {
    miqService.sparkleOn();

    API.get("/api/providers/" + id + "/cloud_tenants?expand=resources&attributes=id,name")
      .then(getCloudTenantsByEms)
      .catch(miqService.handleFailure);
  };

  function getCloudTenantsByEms(data) {
    callback(data);
    miqService.sparkleOff();
  }
};

...

$scope.filterNetworkManagerChanged = miqService.getProviderTenants(function(data) {
  $scope.available_tenants = data;
});

(If we want to be able to customize the field name on the controller, or add custom per-controller actions.)

tzumainn · 2017-05-18T17:02:04Z

Thanks for the review! I've done some initial cleanup; I'll work on reducing code duplication next, and let you know when it's ready for another review.

tzumainn · 2017-05-18T18:51:17Z

app/assets/javascripts/controllers/network_router/network_router_form_controller.js

@@ -66,6 +66,10 @@ ManageIQ.angular.app.controller('networkRouterFormController', ['$http', '$scope
    $http.get('/network_router/network_router_networks_by_ems/' + id)
      .then(getNetworkRouterFormByEmsData)
      .catch(miqService.handleFailure);
+


@himdel I'm a bit confused as to how to refactor this function to use miqService.getProviderTenants - any hints? : )

Nevermind, figured it out!

tzumainn · 2017-05-19T00:07:59Z

@himdel I believe I've resolved the issues you brought up, so I think this is ready for another look. Thanks again for your comments, and just let me know if there's anything I'm missing!

miq-bot · 2017-05-19T15:13:49Z

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

himdel · 2017-05-19T15:19:42Z

app/assets/javascripts/controllers/floating_ip/floating_ip_form_controller.js

@@ -51,6 +51,10 @@ ManageIQ.angular.app.controller('floatingIpFormController', ['$http', '$scope',
    $http.get('/floating_ip/networks_by_ems/' + id)
      .then(getNetworkByEmsFormData)
      .catch(miqService.handleFailure);
+
+    miqService.getProviderTenants(function(data) {
+      $scope.available_tenants = data.resources;


Sorry, just merged a PR converting floating ip to controllerAs.. This will need to be vm.available_tenants = data.resources now..

himdel · 2017-05-19T15:20:17Z

app/views/floating_ip/new.html.haml

@@ -68,17 +68,20 @@
  %h3
    = _('Placement')
  .form-horizontal
-    .form-group
+    .form-group{"ng-if" => "floatingIpModel.ems_id"}


And this one will be vm.floatingIpModel.ems_id

himdel · 2017-05-19T15:20:37Z

app/views/floating_ip/new.html.haml

+                "required"                    => "",
+                :miqrequired                  => true,
+                :checkchange                  => true,
+                'ng-options'                  => 'tenant.id as tenant.name for tenant in available_tenants',


And vm.available_tenants.

tzumainn · 2017-05-19T15:23:58Z

@himdel thanks for the heads up! changes made

himdel · 2017-05-19T15:39:03Z

@tzumainn And thanks for the quick fix :)

I will wait for travis and give this a spin in the UI but I think it looks good! 👍

himdel · 2017-05-22T18:07:29Z

@tzumainn Just one bug.. We need to deal with the undefined value - when you select a provider, and then change back to <Choose>, it will now generate a request to /api/providers/undefined/cloud_tenants?expand=resources&attributes=id,name (note that undefined), and error with "Invalid ExtManagementSystem id undefined specified".

Can you add some condition to handle that?

Perhaps something like...

this.getProviderTenants = function(callback) {
  return function(id) {
    if (! id) {
      callback([]);
      return;
    }

    miqService.sparkleOn();
......

tzumainn · 2017-05-22T18:52:35Z

Ah, that makes sense - added the check!

himdel · 2017-05-22T19:22:13Z

The test failure is unrelated, but can you please fix that "Expected indentation of 8 spaces but found 1 tab" CC complains about? (Don't care about the others, but we really shouldn't be mixing tabs and spaces :).)

tzumainn · 2017-05-22T23:22:23Z

@himdel Oh, whoops - fixed!

miq-bot · 2017-05-22T23:24:02Z

Checked commits tzumainn/manageiq-ui-classic@5b9f9b6~...dd2e9b5 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0
6 files checked, 1 offense detected

app/controllers/application_controller/network.rb

❗ - Line 5, Col 96 - Style/BlockDelimiters - Avoid using {...} for multi-line blocks.

himdel · 2017-05-23T15:31:11Z

Thanks, LGTM, restarting travis 👍

tzumainn · 2017-05-24T13:06:41Z

@dclarizio I don't think this can make it into euwe easily, as there's quite a long line of dependencies and the UI has changed quite a bit since.

AparnaKarve · 2017-05-24T15:35:03Z

app/assets/javascripts/services/miq_service.js

@@ -124,4 +124,23 @@ ManageIQ.angular.app.service('miqService', ['$timeout', '$document', '$q', funct

    return $q.reject(e);
  };
+
+  this.getProviderTenants = function(callback) {
+    return function(id) {


@himdel @tzumainn I just happened to notice these functions in miqService that are very specific to a particular controller.
miqService needs to be as generic as possible I think.

@tzumainn Would it be possible to take these functions out of miqService and create a new service (tenantService maybe?) that you could inject in the angular controllers that need it?

Well, this is specific to 4 different controllers, but yeah, maybe a separate service would make sense.

@tzumainn Any idea what's common in those 4 controllers? (Thinking about naming, should that be a CloudService or a NetworkingService or... :))

tzumainn · 2017-05-24T16:04:34Z

I guess "CloudService" would make the most sense?

simaishi · 2017-05-24T17:23:02Z

@tzumainn Marking as euwe/conflict... as you pointed out, there are conflicts in multiple files.

dclarizio · 2017-05-25T16:29:42Z

@dclarizio I don't think this can make it into euwe easily, as there's quite a long line of dependencies and the UI has changed quite a bit since.

@tzumainn well, then we either have to close the 5.7 BZ as WONTFIX or create a special euwe PR . . . up to you as you opened the BZ I think 😄

tzumainn · 2017-06-02T17:21:41Z

@miq-bot remove_label euwe/yes

simaishi · 2017-06-08T22:26:10Z

@tzumainn Looks like some of the changes here depend on #791, which is fine/no. Please create a separate Fine specific PR (referencing this one).

tzumainn · 2017-06-13T12:05:20Z

@simaishi Looks like #791 should be fine/yes now, so hopefully this can be merged without conflict!

Add cloud tenant filtering for various network object forms (cherry picked from commit dc8cd44) https://bugzilla.redhat.com/show_bug.cgi?id=1458377

simaishi · 2017-06-14T13:08:15Z

Fine backport details:

$ git log -1
commit e42426967f90f057fbc0f23e1611c84a59c1349e
Author: Martin Hradil <himdel@seznam.cz>
Date:   Wed May 24 09:07:23 2017 +0000

    Merge pull request #1343 from tzumainn/network-tenant-filtering
    
    Add cloud tenant filtering for various network object forms
    (cherry picked from commit dc8cd44621f0515959ec6f274132205ee9b3a0a4)
    
    https://bugzilla.redhat.com/show_bug.cgi?id=1458377

dclarizio added compute/cloud bug euwe/yes labels May 15, 2017

dclarizio assigned himdel May 16, 2017

himdel reviewed May 18, 2017

View reviewed changes

tzumainn force-pushed the network-tenant-filtering branch from 371fe64 to 47a2652 Compare May 18, 2017 17:01

tzumainn commented May 18, 2017

View reviewed changes

tzumainn force-pushed the network-tenant-filtering branch from 21ed898 to a341b71 Compare May 19, 2017 04:34

miq-bot added the unmergeable label May 19, 2017

tzumainn force-pushed the network-tenant-filtering branch from a341b71 to cbc19a4 Compare May 19, 2017 15:19

himdel reviewed May 19, 2017

View reviewed changes

tzumainn force-pushed the network-tenant-filtering branch from 112b079 to 4f3934e Compare May 19, 2017 15:23

miq-bot removed the unmergeable label May 19, 2017

tzumainn added 4 commits May 22, 2017 20:24

Add cloud tenant filtering for various network object forms

5b9f9b6

code cleanup

51d5380

consolidated tenant API call into miq_service

9935a98

unified shared network manager code

5b5d690

whitespace issue

dd2e9b5

himdel closed this May 23, 2017

himdel reopened this May 23, 2017

himdel merged commit dc8cd44 into ManageIQ:master May 24, 2017

himdel added this to the Sprint 62 Ending Jun 5, 2017 milestone May 24, 2017

tzumainn mentioned this pull request May 24, 2017

Add cloud tenants to API ManageIQ/manageiq#14731

Merged

AparnaKarve reviewed May 24, 2017

View reviewed changes

simaishi added the euwe/conflict label May 24, 2017

lpichler mentioned this pull request May 25, 2017

Add rbac for network managers #1434

Merged

miq-bot removed the euwe/yes label Jun 2, 2017

simaishi removed the euwe/conflict label Jun 2, 2017

simaishi added the fine/conflict label Jun 8, 2017

tzumainn mentioned this pull request Jun 8, 2017

Scope to vm - floating IP #791

Merged

simaishi removed the fine/conflict label Jun 13, 2017

simaishi pushed a commit that referenced this pull request Jun 14, 2017

Merge pull request #1343 from tzumainn/network-tenant-filtering

e424269

Add cloud tenant filtering for various network object forms (cherry picked from commit dc8cd44) https://bugzilla.redhat.com/show_bug.cgi?id=1458377

simaishi added fine/backported and removed fine/yes labels Jun 14, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add cloud tenant filtering for various network object forms #1343

Add cloud tenant filtering for various network object forms #1343

tzumainn commented May 13, 2017

himdel May 18, 2017

himdel May 18, 2017

himdel May 18, 2017

himdel May 18, 2017

himdel commented May 18, 2017 •

edited

tzumainn commented May 18, 2017

tzumainn May 18, 2017

tzumainn May 18, 2017

tzumainn commented May 19, 2017

miq-bot commented May 19, 2017

himdel May 19, 2017

himdel May 19, 2017

himdel May 19, 2017

tzumainn commented May 19, 2017

himdel commented May 19, 2017

himdel commented May 22, 2017 •

edited

tzumainn commented May 22, 2017

himdel commented May 22, 2017 •

edited

tzumainn commented May 22, 2017

miq-bot commented May 22, 2017

himdel commented May 23, 2017

tzumainn commented May 24, 2017

AparnaKarve May 24, 2017

himdel May 24, 2017

tzumainn commented May 24, 2017

simaishi commented May 24, 2017

dclarizio commented May 25, 2017

tzumainn commented Jun 2, 2017

simaishi commented Jun 8, 2017

tzumainn commented Jun 13, 2017

simaishi commented Jun 14, 2017

Add cloud tenant filtering for various network object forms #1343

Add cloud tenant filtering for various network object forms #1343

Conversation

tzumainn commented May 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

himdel commented May 18, 2017 • edited

tzumainn commented May 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzumainn commented May 19, 2017

miq-bot commented May 19, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzumainn commented May 19, 2017

himdel commented May 19, 2017

himdel commented May 22, 2017 • edited

tzumainn commented May 22, 2017

himdel commented May 22, 2017 • edited

tzumainn commented May 22, 2017

miq-bot commented May 22, 2017

himdel commented May 23, 2017

tzumainn commented May 24, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tzumainn commented May 24, 2017

simaishi commented May 24, 2017

dclarizio commented May 25, 2017

tzumainn commented Jun 2, 2017

simaishi commented Jun 8, 2017

tzumainn commented Jun 13, 2017

simaishi commented Jun 14, 2017

himdel commented May 18, 2017 •

edited

himdel commented May 22, 2017 •

edited

himdel commented May 22, 2017 •

edited