Remove use of scope from action service

The scope being handed to (and retained on) the action
service was being used for two purposes:

1. handing data down to the underlying wizard steps, and
2. receiving data back from the underlying wizard steps.

This patch provides a mechanism for passing data down through
injection, and removes the need for event-based data return
by explicitly passing the captured wizard data to the submit()
resolution.

It also adds the controller scope to the allowed() and
perform() action service handlers to grant access to
contextual information without needing to attach state
to the service.

Change-Id: Ieb293d0a849cd84d15e7aae0a68558fde80fd2c2
Fixes-Bug: 1640049

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
+       * If an action does not have an initAction() function, it is ignored.
-       * have an initScope() 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());
           }
         }

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({});

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;
       };

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
+   *   service: is the service expected to have two functions defined. Both of these
-   *   1. allowed: is expected to return a promise that resolves
+   *   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.
+   *      - When using 'row' or 'detail' type, the current 'item' will be passed to the function.
-   *   2. perform: is what gets called when the button is clicked. Also expected to return a
+   *      - When using 'batch' type, the item argument will be undefined.
-   *      promise that resolves when the action completes.
+   *
-   *      When using 'row' or 'detail' type, the current 'item' is evaluated and passed to the
+   *   2. perform(item[s], scope): is what gets called when the button is clicked. It is also
-   *      function.
+   *      expected to return a promise that resolves when the action completes.
-   *      When using 'batch' type, 'item' is not passed.
+   *
-   *      When using 'delete-selected' for 'batch' type, all selected rows are passed.
+   *      - When using 'row' or 'detail' type, the current 'item' is evaluated and passed
-   *   3. initScope: actions may perform post-config (in the angular sense) initialization by
+   *        to the function.
-   *      providing an initScope method. This might be typically invoked by initActions()
+   *      - When using 'batch' type, 'item' will be undefined.
-   *      on a ResourceType. Actions should not perform blocking operations in their
+   *      - When using 'delete-selected' for 'batch' type, all selected rows are passed as
-   *      construction, for example API calls, because as injectables their constructor
+   *        an array.
-   *      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
+   *   There is a third optional function, initAction (which was previously called initScope)
-   *      Angular context initialization, such as going to the login page.
+   *   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) {
+   *   allowed: function(ignored, scope) {
-   *     return $q.all(
+   *     // This function may use the information in the scope to check whether
-   *       isActive(image),
+   *     // an image may be created in a certain context.
-   *       imageServiceEnabledPromise
+   *     // We ignore the undefined "item" value passed in because as a "batch"
-   *     );
+   *     // action undefined will be passed in.
+   *     return imageServiceEnabledPromise;
    *   },
-   *   perform: function() {
+   *   perform: function(ignored, scope) {
-   *     //open the modal to create volume and return the modal's result promise
+   *     // 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');
    *   }
    * };

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: '<wizard></wizard>',
           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);
       }
     }

horizon/static/framework/widgets/modal/wizard.controller.js

@@ -24,8 +24,9 @@
   WizardModalController.$inject = [
     '$modalInstance',
     '$scope',
-    'workflow', // modal injected
+    'workflow', // WizardModalService injected
-    'submit'    // modal 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) {

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() {

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() {

openstack_dashboard/static/app/core/images/actions/create-volume.service.js

@@ -56,33 +56,23 @@
     toast,
     volumeResourceType
   ) {
-    var scope, createVolumePromise, volumeServiceEnabledPromise;
+    var createVolumePromise, volumeServiceEnabledPromise;
-
-    var volume = {};
 
     var message = {
       success: gettext('Volume %s was successfully created.')
     };
 
     var service = {
-      initScope: initScope,
+      initAction: initAction,
       allowed: allowed,
       perform: perform
     };
 
     return service;
 
-    function initScope(newScope) {
+    function initAction() {
-      scope = newScope;
-
-      var watchVolumeChange = scope.$on(events.VOLUME_CHANGED, onChangedVolume);
-      scope.$on('$destroy', destroy);
       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() {
+    function submit(stepModels) {
-      return cinder.createVolume(volume).then(showSuccessMessage);
+      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();
-    }
-
   }
 })();

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() {

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'

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.framework.widgets.charts.quotaChartDefaults'
-    'horizon.app.core.images.events'
   ];
 
   /**
@@ -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() {

openstack_dashboard/static/app/core/images/steps/create-volume/create-volume.controller.spec.js

@@ -34,9 +34,7 @@
           }
         };
       },
-      getAbsoluteLimits: function() {
+      getAbsoluteLimits: angular.noop
-        return;
-      }
     };
 
     beforeEach(module('horizon.app.core.images'));
@@ -60,10 +58,10 @@
         size: 1024
       };
 
+      $scope.stepModels = {};
+
       $scope.volumeForm = {
-        $setValidity : function() {
+        $setValidity : angular.noop
-          return;
-        }
       };
 
       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;
-
+      expect(ctrl.volume.availability_zone).toEqual('');
-      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);
     });
 
     it('should not update the graph if wrong values are given for volume size', function () {

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.