diff --git a/horizon/static/framework/conf/resource-type-registry.service.js b/horizon/static/framework/conf/resource-type-registry.service.js index c6367d3cb..19736adae 100644 --- a/horizon/static/framework/conf/resource-type-registry.service.js +++ b/horizon/static/framework/conf/resource-type-registry.service.js @@ -145,8 +145,7 @@ * @description * Performs initialization of all actions for the given type. * - * This requires the proper scope be passed. If an action does not - * have an initScope() function, it is ignored. + * If an action does not have an initAction() function, it is ignored. */ function initActions(scope) { angular.forEach(self.itemActions, setActionScope); @@ -154,7 +153,15 @@ angular.forEach(self.globalActions, setActionScope); function setActionScope(action) { - if (action.service.initScope) { + if (action.service.initAction) { + action.service.initAction(); + } else if (action.service.initScope) { + // The use of scope in action services breaks the singleton nature + // of the services, and should be stopped. State should be held on + // controllers instead; scope is now passed into allow() and perform() + // methods. + $log.warn('The initScope() method is deprecated. ' + + 'Invocation of it will stop in Queens.'); action.service.initScope(scope.$new()); } } diff --git a/horizon/static/framework/conf/resource-type-registry.service.spec.js b/horizon/static/framework/conf/resource-type-registry.service.spec.js index 67eb5612b..f4ce124e8 100644 --- a/horizon/static/framework/conf/resource-type-registry.service.spec.js +++ b/horizon/static/framework/conf/resource-type-registry.service.spec.js @@ -169,7 +169,21 @@ type = service.getResourceType('something'); }); - it('initActions calls initScope on item and batch actions', function () { + it('initActions calls initAction on item and batch actions', function () { + var action = {service: {initAction: angular.noop, initScope: angular.noop}}; + spyOn(action.service, 'initAction'); + spyOn(action.service, 'initScope'); + type.batchActions.push(action); + type.initActions({ + '$new': function () { + return 4; + } + }); + expect(action.service.initAction).toHaveBeenCalled(); + expect(action.service.initScope).not.toHaveBeenCalled(); + }); + + it('initActions calls initScope if initAction is not defined', function () { var action = {service: {initScope: angular.noop}}; spyOn(action.service, 'initScope'); type.batchActions.push(action); @@ -181,7 +195,7 @@ expect(action.service.initScope).toHaveBeenCalledWith(4); }); - it('initActions ignores initScope when not present', function () { + it('initActions ignores initAction and initScope when not present', function () { var action = {service: {}}; type.batchActions.push(action); var returned = type.initActions({}); diff --git a/horizon/static/framework/widgets/action-list/actions.controller.js b/horizon/static/framework/widgets/action-list/actions.controller.js index beb50e690..794f37968 100644 --- a/horizon/static/framework/widgets/action-list/actions.controller.js +++ b/horizon/static/framework/widgets/action-list/actions.controller.js @@ -19,7 +19,7 @@ .module('horizon.framework.widgets.action-list') .controller('horizon.framework.widgets.action-list.ActionsController', ActionsController); - ActionsController.$inject = ['$q']; + ActionsController.$inject = ['$q', '$scope']; /** * @ngdoc controller @@ -30,7 +30,7 @@ * functions and variables within this controller. * */ - function ActionsController($q) { + function ActionsController($q, $scope) { var ctrl = this; ctrl.disabled = false; ctrl.passThroughCallbacks = {}; @@ -93,7 +93,7 @@ ctrl.passThroughCallbacks[dynCallbackName] = function genPassThroughCallback(item) { if (ctrl.disabled) { return undefined; } preAction(); - var result = service.perform(item); + var result = service.perform(item, $scope.$new()); $q.when(result).then(postAction, postAction); return resultHandler ? resultHandler(result) : result; }; diff --git a/horizon/static/framework/widgets/action-list/actions.directive.js b/horizon/static/framework/widgets/action-list/actions.directive.js index 8d0b48e5e..e68e2f36c 100644 --- a/horizon/static/framework/widgets/action-list/actions.directive.js +++ b/horizon/static/framework/widgets/action-list/actions.directive.js @@ -95,25 +95,35 @@ * A title and description must be provided for the 'detail' type. These are used as * the title and description to display in the bootstrap panel. * - * service: is the service expected to have two functions - * 1. allowed: is expected to return a promise that resolves + * service: is the service expected to have two functions defined. Both of these + * functions take two arguments: the relevant item or items (if any) and the angular + * scope in which the action button appears. + * + * 1. allowed(item[s], scope): is expected to return a promise that resolves * if the action is permitted and is rejected if not. If there are multiple promises that * need to be resolved, you can $q.all to combine multiple promises into a single promise. - * When using 'row' or 'detail' type, the current 'item' will be passed to the function. - * When using 'batch' type, no arguments are provided. - * 2. perform: is what gets called when the button is clicked. Also expected to return a - * promise that resolves when the action completes. - * When using 'row' or 'detail' type, the current 'item' is evaluated and passed to the - * function. - * When using 'batch' type, 'item' is not passed. - * When using 'delete-selected' for 'batch' type, all selected rows are passed. - * 3. initScope: actions may perform post-config (in the angular sense) initialization by - * providing an initScope method. This might be typically invoked by initActions() - * on a ResourceType. Actions should not perform blocking operations in their - * construction, for example API calls, because as injectables their constructor - * is run during injection, meaning those calls would be executed as the module - * is initialized. This would mean those calls would be blocking on any - * Angular context initialization, such as going to the login page. + * + * - When using 'row' or 'detail' type, the current 'item' will be passed to the function. + * - When using 'batch' type, the item argument will be undefined. + * + * 2. perform(item[s], scope): is what gets called when the button is clicked. It is also + * expected to return a promise that resolves when the action completes. + * + * - When using 'row' or 'detail' type, the current 'item' is evaluated and passed + * to the function. + * - When using 'batch' type, 'item' will be undefined. + * - When using 'delete-selected' for 'batch' type, all selected rows are passed as + * an array. + * + * There is a third optional function, initAction (which was previously called initScope) + * Actions may perform post-config (in the angular sense) initialization by + * providing an initAction method. This might be typically invoked by initActions() + * on a ResourceType. Actions should not perform blocking operations in their + * construction, for example API calls, because as injectables their constructor + * is run during injection, meaning those calls would be executed as the module + * is initialized. This would mean those calls would be blocking on any + * Angular context initialization, such as going to the login page. + * * * @restrict E * @scope @@ -126,9 +136,13 @@ * * var batchDeleteService = { * allowed: function() { + * // This function ignores the "items" and scope arguments because + * // they're not needed for this action. * return policy.ifAllowed({ rules: [['image', 'delete_image']] }); * }, * perform: function(images) { + * // This function ignores the second scope argument because there's + * // no context information interesting to this action. * return $q.all(images.map(function(image){ * return glanceAPI.deleteImage(image.id); * })); @@ -141,16 +155,20 @@ * is executed. * * var createService = { - * allowed: function(image) { - * return $q.all( - * isActive(image), - * imageServiceEnabledPromise - * ); + * allowed: function(ignored, scope) { + * // This function may use the information in the scope to check whether + * // an image may be created in a certain context. + * // We ignore the undefined "item" value passed in because as a "batch" + * // action undefined will be passed in. + * return imageServiceEnabledPromise; * }, - * perform: function() { - * //open the modal to create volume and return the modal's result promise + * perform: function(ignored, scope) { + * // Open the modal to create volume and return the modal's result promise. + * // This function may use the information in the scope to create the + * // image in a certain context. Again, we ignore the undefined "item" + * // value passed in. * }, - * initScope: function() { + * initAction: function() { * imageServiceEnabledPromise = serviceCatalog.ifTypeEnabled('image'); * } * }; diff --git a/horizon/static/framework/widgets/modal/wizard-modal.service.js b/horizon/static/framework/widgets/modal/wizard-modal.service.js index 2a78287bb..56c8372ee 100644 --- a/horizon/static/framework/widgets/modal/wizard-modal.service.js +++ b/horizon/static/framework/widgets/modal/wizard-modal.service.js @@ -47,10 +47,11 @@ .factory('horizon.framework.widgets.modal.wizard-modal.service', WizardModalService); WizardModalService.$inject = [ + '$log', '$modal' ]; - function WizardModalService($modal) { + function WizardModalService($log, $modal) { var service = { modal: modal }; @@ -60,11 +61,10 @@ //////////////////// function modal(params) { - if (params && params.scope && params.workflow && params.submit) { + if (params && params.workflow && params.submit) { var options = { size: 'lg', controller: 'WizardModalController as modalCtrl', - scope: params.scope, template: '', backdrop: 'static', windowClass: 'modal-dialog-wizard', @@ -74,10 +74,20 @@ }, submit: function() { return params.submit; + }, + data: function() { + return params.data; } } }; + // backwards compatibility + if (angular.isDefined(params.scope)) { + $log.warn('The "scope" param to modal() is deprecated.' + + 'Handling of it will stop in Queens.'); + options.scope = params.scope; + } + return $modal.open(options); } } diff --git a/horizon/static/framework/widgets/modal/wizard.controller.js b/horizon/static/framework/widgets/modal/wizard.controller.js index 22b91e544..bf38d76e1 100644 --- a/horizon/static/framework/widgets/modal/wizard.controller.js +++ b/horizon/static/framework/widgets/modal/wizard.controller.js @@ -24,8 +24,9 @@ WizardModalController.$inject = [ '$modalInstance', '$scope', - 'workflow', // modal injected - 'submit' // modal injected + 'workflow', // WizardModalService injected + 'submit', // WizardModalService injected + 'data' // WizardModalService injected ]; /** @@ -38,13 +39,18 @@ * This controller sets the modal actions and workflow on the given scope * as the Wizard needs them defined on the scope. */ - function WizardModalController($modalInstance, $scope, workflow, submit) { + function WizardModalController($modalInstance, $scope, workflow, submit, data) { /* eslint-disable angular/controller-as */ $scope.close = close; $scope.cancel = cancel; $scope.submit = submit; $scope.workflow = workflow; + // copy over the data (we copy directly for backwards compatibility of access + // since these properties used to be assigned directly on the scope) + angular.forEach(data, function copy(value, key) { + $scope[key] = value; + }); /* eslint-enable angular/controller-as */ function close(args) { diff --git a/horizon/static/framework/widgets/modal/wizard.controller.spec.js b/horizon/static/framework/widgets/modal/wizard.controller.spec.js index d631af774..2ad0fa7a1 100644 --- a/horizon/static/framework/widgets/modal/wizard.controller.spec.js +++ b/horizon/static/framework/widgets/modal/wizard.controller.spec.js @@ -34,13 +34,14 @@ $modalInstance: modalInstance, $scope: scope, workflow: { steps: 'somestep' }, - submit: { api: 'someapi' } + submit: { api: 'someapi' }, + data: { spam: 'ham' } }); })); ////////// - it('should inject and assign workflow and submit', injectAssign); + it('should inject and assign workflow, submit and data', injectAssign); it('should forward call to modalInstance on close', closeModal); it('should forward call to modalInstance on cancel', cancelModal); @@ -49,6 +50,7 @@ function injectAssign() { expect(scope.workflow.steps).toEqual('somestep'); expect(scope.submit.api).toEqual('someapi'); + expect(scope.spam).toEqual('ham'); } function closeModal() { diff --git a/horizon/static/framework/widgets/wizard/wizard.controller.js b/horizon/static/framework/widgets/wizard/wizard.controller.js index a92b530ff..e9f586e36 100644 --- a/horizon/static/framework/widgets/wizard/wizard.controller.js +++ b/horizon/static/framework/widgets/wizard/wizard.controller.js @@ -50,6 +50,9 @@ var steps = $scope.steps = $scope.workflow.steps || []; $scope.wizardForm = {}; + // a place to keep each step's captured data, named for their step.formName + $scope.stepModels = {}; + $scope.switchTo = switchTo; $scope.showError = showError; /*eslint-enable angular/controller-as */ @@ -111,7 +114,7 @@ // prevent the finish button from being clicked again viewModel.isSubmitting = true; beforeSubmit(); - $scope.submit().then(afterSubmit, showError); + $scope.submit($scope.stepModels).then(afterSubmit, showError); } function onInitSuccess() { diff --git a/openstack_dashboard/static/app/core/images/actions/create-volume.service.js b/openstack_dashboard/static/app/core/images/actions/create-volume.service.js index 6d04514d0..594ecf1fc 100644 --- a/openstack_dashboard/static/app/core/images/actions/create-volume.service.js +++ b/openstack_dashboard/static/app/core/images/actions/create-volume.service.js @@ -56,33 +56,23 @@ toast, volumeResourceType ) { - var scope, createVolumePromise, volumeServiceEnabledPromise; - - var volume = {}; + var createVolumePromise, volumeServiceEnabledPromise; var message = { success: gettext('Volume %s was successfully created.') }; var service = { - initScope: initScope, + initAction: initAction, allowed: allowed, perform: perform }; return service; - function initScope(newScope) { - scope = newScope; - - var watchVolumeChange = scope.$on(events.VOLUME_CHANGED, onChangedVolume); - scope.$on('$destroy', destroy); + function initAction() { createVolumePromise = policy.ifAllowed({rules: [['volume', 'volume:create']]}); volumeServiceEnabledPromise = serviceCatalog.ifTypeEnabled('volume'); - - function destroy() { - watchVolumeChange(); - } } function allowed(image) { @@ -95,16 +85,15 @@ } function perform(image) { - scope.image = image; return wizardModalService.modal({ - scope: scope, + data: {image: image}, workflow: createVolumeWorkflowService, submit: submit }).result; } - function submit() { - return cinder.createVolume(volume).then(showSuccessMessage); + function submit(stepModels) { + return cinder.createVolume(stepModels.volumeForm).then(showSuccessMessage); } function showSuccessMessage(response) { @@ -127,12 +116,5 @@ function imageActive(image) { return $qExtensions.booleanAsPromise(image.status === 'active'); } - - //// scope functions //// - function onChangedVolume(e, newVolume) { - volume = newVolume; - e.stopPropagation(); - } - } })(); diff --git a/openstack_dashboard/static/app/core/images/actions/create-volume.service.spec.js b/openstack_dashboard/static/app/core/images/actions/create-volume.service.spec.js index d96936884..3c85b4d76 100644 --- a/openstack_dashboard/static/app/core/images/actions/create-volume.service.spec.js +++ b/openstack_dashboard/static/app/core/images/actions/create-volume.service.spec.js @@ -18,7 +18,7 @@ describe('horizon.app.core.images.actions.create-volume.service', function() { - var service, $scope, events, workflow; + var service, $scope, workflow; var wizardModalService = { modal: function () { return { @@ -72,7 +72,6 @@ beforeEach(inject(function($injector, _$rootScope_) { $scope = _$rootScope_.$new(); service = $injector.get('horizon.app.core.images.actions.create-volume.service'); - events = $injector.get('horizon.app.core.images.events'); workflow = $injector.get('horizon.app.core.images.workflows.create-volume.service'); })); @@ -81,55 +80,31 @@ spyOn(wizardModalService, 'modal').and.callThrough(); var image = {id: '12'}; - service.initScope($scope); + service.initAction(); service.perform(image); expect(wizardModalService.modal).toHaveBeenCalled(); var modalArgs = wizardModalService.modal.calls.argsFor(0)[0]; - expect(modalArgs.scope).toEqual($scope); + expect(modalArgs.data.image).toEqual(image); expect(modalArgs.workflow).toEqual(workflow); expect(modalArgs.submit).toBeDefined(); }); - it('should create volume in cinder and raise event', function() { + it('should create volume in cinder', function() { var volume = { name: 'Test', id: '2' }; spyOn(cinderAPI, 'createVolume').and.callThrough(); spyOn(wizardModalService, 'modal').and.callThrough(); - service.initScope($scope); + service.initAction(); service.perform(); - $scope.$emit(events.VOLUME_CHANGED, volume); - $scope.$apply(); - var modalArgs = wizardModalService.modal.calls.argsFor(0)[0]; - modalArgs.submit(); + modalArgs.submit({volumeForm: volume}); $scope.$apply(); expect(cinderAPI.createVolume).toHaveBeenCalledWith(volume); }); - - it('should destroy volume change watcher on exit', function() { - spyOn(cinderAPI, 'createVolume').and.callThrough(); - spyOn(wizardModalService, 'modal').and.callThrough(); - - service.initScope($scope); - service.perform(); - - var oldVolume = {id: 1}; - $scope.$emit(events.VOLUME_CHANGED, oldVolume); - - $scope.$emit('$destroy'); - - var newVolume = {id: 2}; - $scope.$emit(events.VOLUME_CHANGED, newVolume); - - var modalArgs = wizardModalService.modal.calls.argsFor(0)[0]; - modalArgs.submit(); - - expect(cinderAPI.createVolume).toHaveBeenCalledWith(oldVolume); - }); }); describe('allowed', function() { diff --git a/openstack_dashboard/static/app/core/images/images.module.js b/openstack_dashboard/static/app/core/images/images.module.js index 148bd17c1..102ed0b43 100644 --- a/openstack_dashboard/static/app/core/images/images.module.js +++ b/openstack_dashboard/static/app/core/images/images.module.js @@ -267,7 +267,6 @@ */ function events() { return { - VOLUME_CHANGED: 'horizon.app.core.images.VOLUME_CHANGED', IMAGE_CHANGED: 'horizon.app.core.images.IMAGE_CHANGED', IMAGE_METADATA_CHANGED: 'horizon.app.core.images.IMAGE_METADATA_CHANGED', IMAGE_UPLOAD_PROGRESS: 'horizon.app.core.images.IMAGE_UPLOAD_PROGRESS' diff --git a/openstack_dashboard/static/app/core/images/steps/create-volume/create-volume.controller.js b/openstack_dashboard/static/app/core/images/steps/create-volume/create-volume.controller.js index c140d7ca0..dc7dc4ec5 100644 --- a/openstack_dashboard/static/app/core/images/steps/create-volume/create-volume.controller.js +++ b/openstack_dashboard/static/app/core/images/steps/create-volume/create-volume.controller.js @@ -25,8 +25,7 @@ '$filter', 'horizon.app.core.openstack-service-api.cinder', 'horizon.app.core.openstack-service-api.nova', - 'horizon.framework.widgets.charts.quotaChartDefaults', - 'horizon.app.core.images.events' + 'horizon.framework.widgets.charts.quotaChartDefaults' ]; /** @@ -37,12 +36,11 @@ * @param {Object} cinder * @param {Object} nova * @param {Object} quotaChartDefaults - * @param {Object} events * @description * This controller is use for creating an image. * @return {undefined} No return value */ - function CreateVolumeController($scope, $filter, cinder, nova, quotaChartDefaults, events) { + function CreateVolumeController($scope, $filter, cinder, nova, quotaChartDefaults) { var ctrl = this; ctrl.volumeType = {}; @@ -57,7 +55,9 @@ var numberOfVolumesToAdd = 1; - ctrl.volume = { + // bind for local access and also hand back up to the wizard controller + // stepModels will be passed to the create volume action submit() + $scope.stepModels.volumeForm = ctrl.volume = { size: 1, name: ctrl.image.name, description: '', @@ -149,7 +149,6 @@ ctrl.volumeTypes.forEach(function(volumeType) { if (volumeType.name === response.name) { ctrl.volumeType = volumeType; - return; } }); } @@ -212,7 +211,6 @@ function volumeChangeEvent() { ctrl.volume.volume_type = ctrl.volumeType.name || ''; - $scope.$emit(events.VOLUME_CHANGED, ctrl.volume); } function deregisterWatchers() { diff --git a/openstack_dashboard/static/app/core/images/steps/create-volume/create-volume.controller.spec.js b/openstack_dashboard/static/app/core/images/steps/create-volume/create-volume.controller.spec.js index 3f0abd9dc..a142b6ddb 100644 --- a/openstack_dashboard/static/app/core/images/steps/create-volume/create-volume.controller.spec.js +++ b/openstack_dashboard/static/app/core/images/steps/create-volume/create-volume.controller.spec.js @@ -34,9 +34,7 @@ } }; }, - getAbsoluteLimits: function() { - return; - } + getAbsoluteLimits: angular.noop }; beforeEach(module('horizon.app.core.images')); @@ -60,10 +58,10 @@ size: 1024 }; + $scope.stepModels = {}; + $scope.volumeForm = { - $setValidity : function() { - return; - } + $setValidity : angular.noop }; controller = $injector.get('$controller'); @@ -276,16 +274,16 @@ expect(graph.maxLimit).toEqual(graph.maxLimit); }); - it('should emit volumeChanged event when a volume attribute is changed', function() { + it('should update volume type from volume name', function() { var ctrl = createController(); $scope.$apply(); - $scope.$emit.calls.reset(); + ctrl.volume.volume_type = 'spam'; ctrl.volume.name = 'nova2'; $scope.$apply(); - expect($scope.$emit.calls.count()).toEqual(1); + expect(ctrl.volume.volume_type).toEqual('lvmdriver-1'); }); it('should set the validity of the volume size input field based on the limit', function() { @@ -358,29 +356,6 @@ expect($scope.$emit).not.toHaveBeenCalled(); }); - it('should emit a changed volume event when the user changes the volume', function() { - var ctrl = createController(); - $scope.$apply(); - - ctrl.volume.size = 100; - - var emittedEventArgs = $scope.$emit.calls.argsFor(0); - var expectedVolume = { - size: 100, - name: ctrl.image.name, - description: '', - volume_type: 'lvmdriver-1', - availability_zone: 'zone1', // pre-selects first - metadata: {}, - image_id: ctrl.image.id, - snapshot_id: null, - source_volid: null - }; - - expect(emittedEventArgs[0]).toEqual('horizon.app.core.images.VOLUME_CHANGED'); - expect(emittedEventArgs[1]).toEqual(expectedVolume); - }); - it('not default the availability_zone if none present', function() { nova.getAvailabilityZones = function() { @@ -394,35 +369,7 @@ $scope.$apply(); ctrl.volume.size = 100; - - var emittedEventArgs = $scope.$emit.calls.argsFor(0); - var expectedVolume = { - size: 100, - name: ctrl.image.name, - description: '', - volume_type: 'lvmdriver-1', - availability_zone: '', - metadata: {}, - image_id: ctrl.image.id, - snapshot_id: null, - source_volid: null - }; - - expect(emittedEventArgs[0]).toEqual('horizon.app.core.images.VOLUME_CHANGED'); - expect(emittedEventArgs[1]).toEqual(expectedVolume); - }); - - it('should emit changed volume events even if the volume name is empty', function() { - var ctrl = createController(); - - $scope.$apply(); - ctrl.volumeType.name = ''; - $scope.$emit.calls.reset(); - - ctrl.volume.name = ''; - $scope.$apply(); - - expect($scope.$emit.calls.count()).toEqual(2); + expect(ctrl.volume.availability_zone).toEqual(''); }); it('should not update the graph if wrong values are given for volume size', function () { diff --git a/releasenotes/notes/bug-1640049-1195315b5f591ab0.yaml b/releasenotes/notes/bug-1640049-1195315b5f591ab0.yaml new file mode 100644 index 000000000..66c08d971 --- /dev/null +++ b/releasenotes/notes/bug-1640049-1195315b5f591ab0.yaml @@ -0,0 +1,10 @@ +--- +deprecations: + - The initScope method for action services has been + deprecated, with the new method initAction added + which does not get passed a scope. The allowed and + perform method are now passed a scope for context + in addition to the first item/items argument. + The "scope" parameter to the WizardModalService modal() + method has also been deprecated, and will be ignored + in a future release of Horizon.