-
Notifications
You must be signed in to change notification settings - Fork 357
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
Add cloud tenant filtering for various network object forms #1343
Conversation
.then(getCloudTenantsByEms) | ||
.catch(miqService.handleFailure); | ||
|
||
miqService.sparkleOff(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
.
@@ -22,16 +22,21 @@ | |||
%h3 | |||
= _('Placement') | |||
.form-horizontal | |||
.form-group{"ng-if" => "networkRouterModel.ems_id"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Probably wanted to add ng-if
to the next line, instead of adding an empty form-group
?
@@ -105,4 +105,18 @@ ManageIQ.angular.app.controller('networkRouterFormController', ['$http', '$scope | |||
$scope.available_subnets = data.available_subnets; | |||
miqService.sparkleOff(); | |||
} | |||
|
|||
$scope.filterNetworkManagerChanged = function(id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A method with this name already exists on line 64 (network_router)
@@ -66,4 +66,19 @@ ManageIQ.angular.app.controller('floatingIpFormController', ['$http', '$scope', | |||
$scope.available_networks = data.available_networks; | |||
miqService.sparkleOff(); | |||
} | |||
|
|||
|
|||
$scope.filterNetworkManagerChanged = function(id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A method with this name already exists on line 47 (floating_ip)
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 (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 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.) |
371fe64
to
47a2652
Compare
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. |
@@ -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); | |||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@himdel I'm a bit confused as to how to refactor this function to use miqService.getProviderTenants - any hints? : )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nevermind, figured it out!
@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! |
21ed898
to
a341b71
Compare
This pull request is not mergeable. Please rebase and repush. |
a341b71
to
cbc19a4
Compare
@@ -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; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry, just merged a PR converting floating ip to controllerAs
.. This will need to be vm.available_tenants = data.resources
now..
app/views/floating_ip/new.html.haml
Outdated
@@ -68,17 +68,20 @@ | |||
%h3 | |||
= _('Placement') | |||
.form-horizontal | |||
.form-group | |||
.form-group{"ng-if" => "floatingIpModel.ems_id"} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And this one will be vm.floatingIpModel.ems_id
app/views/floating_ip/new.html.haml
Outdated
"required" => "", | ||
:miqrequired => true, | ||
:checkchange => true, | ||
'ng-options' => 'tenant.id as tenant.name for tenant in available_tenants', |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
And vm.available_tenants
.
112b079
to
4f3934e
Compare
@himdel thanks for the heads up! changes made |
@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! 👍 |
@tzumainn Just one bug.. We need to deal with the undefined value - when you select a provider, and then change back to Can you add some condition to handle that? Perhaps something like... this.getProviderTenants = function(callback) {
return function(id) {
if (! id) {
callback([]);
return;
}
miqService.sparkleOn();
...... |
Ah, that makes sense - added the check! |
The test failure is unrelated, but can you please fix that |
@himdel Oh, whoops - fixed! |
Checked commits tzumainn/manageiq-ui-classic@5b9f9b6~...dd2e9b5 with ruby 2.2.6, rubocop 0.47.1, and haml-lint 0.20.0 app/controllers/application_controller/network.rb
|
Thanks, LGTM, restarting travis 👍 |
@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. |
@@ -124,4 +124,23 @@ ManageIQ.angular.app.service('miqService', ['$timeout', '$document', '$q', funct | |||
|
|||
return $q.reject(e); | |||
}; | |||
|
|||
this.getProviderTenants = function(callback) { | |||
return function(id) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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... :))
I guess "CloudService" would make the most sense? |
@tzumainn Marking as |
@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 😄 |
@miq-bot remove_label euwe/yes |
Add cloud tenant filtering for various network object forms (cherry picked from commit dc8cd44) https://bugzilla.redhat.com/show_bug.cgi?id=1458377
Fine backport details:
|
https://bugzilla.redhat.com/show_bug.cgi?id=1394289