Merge "Remove use of scope from action service"

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.