From 6192bfd7acba85c982498d1e9af9bd563f244ef4 Mon Sep 17 00:00:00 2001 From: Tyr Johanson Date: Fri, 11 Mar 2016 11:09:04 -0700 Subject: [PATCH] Images tables uses action promises This patch demonstrates how promises returned by actions are used by the images table. In particular, notice how specific action event handlers do not need to be registered. This allows a view to support a wide variety of actions without needing to setup a specific event listener for each type of event the action might emit. One could argue that if action events were standardized, they could still be used, which is true. Consider a generic SUCCESS event. However, promises have additional benefits and avoid some of the problems of events. One advantage is that promises make it easy to "chain" multiple handlers to a single success or failure. For example, once ALL of these actions have completed successfully, then update a status icon. We see another example of this in the delete-action.service which uses a promise chain to convert the data returned by the delete-modal.service into a standardized form used by actions. Also, promises don't require that the caller have a parent scope. In angular, events "bubble" from child scope to the parent scope. These scopes are effectively the view hierarchy, where a panel contains a table, which contains rows, which contain buttons. However, actions are not view elements lke a table or input box. Actions, like "delete image" are a behavior that may be invoked independently, and the code that cares about the success or failure of that action may not be a scope parent. Added an action-result service that provides a convenience object for creating such results. Co-Authored-By: Matt Borland Partially-Implements: blueprint angular-registry Change-Id: Id6725664e5654a4f75508993b9640a0de80c6884 --- .../util/actions/action-result.service.js | 147 ++++++++++++++++++ .../actions/action-result.service.spec.js | 87 +++++++++++ .../framework/util/actions/actions.module.js | 21 +++ horizon/static/framework/util/util.module.js | 1 + .../widgets/modal/delete-modal.service.js | 8 +- .../static/app/core/core.module.js | 8 +- .../images/actions/create-volume.service.js | 19 ++- .../images/actions/delete-image.service.js | 23 ++- .../actions/delete-image.service.spec.js | 22 +-- .../images/actions/edit.action.service.js | 47 ++---- .../actions/edit.action.service.spec.js | 71 +-------- .../images/actions/launch-instance.service.js | 4 +- .../actions/launch-instance.service.spec.js | 1 - .../actions/update-metadata.action.service.js | 25 ++- .../update-metadata.action.service.spec.js | 9 -- .../app/core/images/table/images-table.html | 8 +- .../core/images/table/images.controller.js | 88 +++++++---- .../images/table/images.controller.spec.js | 83 +++++----- .../action-results-303c282165b60f47.yaml | 12 ++ 19 files changed, 456 insertions(+), 228 deletions(-) create mode 100644 horizon/static/framework/util/actions/action-result.service.js create mode 100644 horizon/static/framework/util/actions/action-result.service.spec.js create mode 100644 horizon/static/framework/util/actions/actions.module.js create mode 100644 releasenotes/notes/action-results-303c282165b60f47.yaml diff --git a/horizon/static/framework/util/actions/action-result.service.js b/horizon/static/framework/util/actions/action-result.service.js new file mode 100644 index 0000000000..c7da5fc1be --- /dev/null +++ b/horizon/static/framework/util/actions/action-result.service.js @@ -0,0 +1,147 @@ +/** + * (c) Copyright 2016 Hewlett Packard Enterprise Development Company LP + * + * Licensed under the Apache License, Version 2.0 (the "License"); you may + * not use self file except in compliance with the License. You may obtain + * a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations + * under the License. + */ + +(function() { + 'use strict'; + + angular + .module('horizon.framework.util.actions') + .factory('horizon.framework.util.actions.action-result.service', factory); + + /** + * @ngdoc factory + * @name factory + * @description + * The purpose of this service is to conveniently create meaningful return + * values from an action. For example, if you perform an action that deletes + * three items, it may be useful for the action to return information + * that indicates which three items were deleted. + * + * The ActionResult object allows an action's code to easily indicate what + * items were affected. + * + * For example, let's say our action deleted three items. We would + * resolve the action's promise by appending three 'deleted' items, then + * conclude by returning the bare result object. + * @example + ``` + return actionResultService.getActionResult() + .deleted('OS::Glance::Image', id1) + .deleted('OS::Glance::Image', id2) + .deleted('OS::Glance::Image', id3) + .result; + ``` + * As an example of how this is consumed, imagine a situation where there is + * a display with a list of instances, each having actions. A user performs + * one action, let's say Edit Instance; then after the action completes, the + * user's expectation is that the list of instances is reloaded. The + * controller that is managing that display needs to have a hook into the + * user's action. This is achieved through returning a promise from the + * initiation of the action. In the case of the actions directive, the + * promise is handled through assigning a result-handler in the table row + * markup: + ``` + + ``` + * The controller places a handler (above, ctrl.actionResultHandler) on this + * promise which, when the promise is resolved, analyzes that resolution + * to figure out logically what to do. We want to make this logic simple and + * also capable of handling 'unknown' actions; that is, we want to generically + * handle any action that a third-party could add. The action result + * feature provides this generic mechanism. The Edit Instance action would + * resolve with {updated: [{type: 'OS::Nova::Server', id: 'some-uuid'}]}, + * which then can be handled by the controller as required. In a controller: + ``` + ctrl.actionResultHandler = function resultHandler(returnValue) { + return $q.when(returnValue, actionSuccessHandler); + }; + + function actionSuccessHandler(result) { + // simple logic to just reload any time there are updated results. + if (result.updated.length > 0) { + reloadTheList(); + } + } + ``` + * This logic of course should probably be more fine-grained than the example, + * but this demonstrates the basics of how you use action promises and provide + * appropriate behaviors. + */ + function factory() { + + return { + getActionResult: getActionResult, + getIdsOfType: getIdsOfType + }; + + // Given a list of objects (items) that each have an 'id' property, + // return a list of those id values for objects whose 'type' property + // matches the 'type' parameter. + // This is a convenience method used for extracting IDs from action + // result objects. For example, if you wanted to know the IDs of + // the deleted images (but didn't want to know about other deleted types), + // you'd use this function. + function getIdsOfType(items, type) { + return items ? items.reduce(typeIdReduce, []) : []; + + function typeIdReduce(accumulator, item) { + if (item.type === type) { + accumulator.push(item.id); + } + return accumulator; + } + } + + function getActionResult() { + return new ActionResult(); + } + + function ActionResult() { + this.result = { + created: [], + updated: [], + deleted: [], + failed: [] + }; + this.created = created; + this.updated = updated; + this.deleted = deleted; + this.failed = failed; + + function created(type, id) { + this.result.created.push({type: type, id: id}); + return this; + } + + function updated(type, id) { + this.result.updated.push({type: type, id: id}); + return this; + } + + function deleted(type, id) { + this.result.deleted.push({type: type, id: id}); + return this; + } + + function failed(type, id) { + this.result.failed.push({type: type, id: id}); + return this; + } + } + + } +})(); diff --git a/horizon/static/framework/util/actions/action-result.service.spec.js b/horizon/static/framework/util/actions/action-result.service.spec.js new file mode 100644 index 0000000000..e5b1be7087 --- /dev/null +++ b/horizon/static/framework/util/actions/action-result.service.spec.js @@ -0,0 +1,87 @@ +/* + * (c) Copyright 2016 Hewlett Packard Enterprise Development Company LP + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +(function() { + 'use strict'; + + describe('horizon.framework.util.actions.action-result.service', function() { + var service; + + beforeEach(module('horizon.framework.util.actions')); + + beforeEach(inject(function($injector) { + service = $injector.get('horizon.framework.util.actions.action-result.service'); + })); + + describe('getIdsOfType', function() { + + it('returns an empty array if no items', function() { + expect(service.getIdsOfType([], 'OS::Glance::Image')).toEqual([]); + }); + + it('returns an empty array if items is falsy', function() { + expect(service.getIdsOfType(false, 'OS::Glance::Image')).toEqual([]); + }); + + it('returns items with matching type', function() { + var items = [{type: 'No::Match'}, {type: 'OS::Glance::Image', id: 1}, + {type: 'OS::Glance::Image', id: 2}]; + expect(service.getIdsOfType(items, 'OS::Glance::Image')).toEqual([1, 2]); + }); + }); + + it('has getActionResult', function() { + expect(service.getActionResult).toBeDefined(); + }); + + describe('ActionResult', function() { + var actionResult; + var type = 'OS::Nova::Server'; + var id = 'the-id-value'; + + beforeEach(function() { + actionResult = service.getActionResult(); + }); + + it('.updated() adds updated items', function() { + actionResult.updated(type, id); + expect(actionResult.result.updated) + .toEqual([{type: 'OS::Nova::Server', id: 'the-id-value'}]); + }); + + it('.created() adds created items', function() { + actionResult.created(type, id); + expect(actionResult.result.created) + .toEqual([{type: 'OS::Nova::Server', id: 'the-id-value'}]); + }); + + it('.deleted() adds deleted items', function() { + actionResult.deleted(type, id); + expect(actionResult.result.deleted) + .toEqual([{type: 'OS::Nova::Server', id: 'the-id-value'}]); + }); + + it('.failed() adds failed items', function() { + actionResult.failed(type, id); + expect(actionResult.result.failed) + .toEqual([{type: 'OS::Nova::Server', id: 'the-id-value'}]); + }); + + }); + + }); + +})(); diff --git a/horizon/static/framework/util/actions/actions.module.js b/horizon/static/framework/util/actions/actions.module.js new file mode 100644 index 0000000000..90687a6908 --- /dev/null +++ b/horizon/static/framework/util/actions/actions.module.js @@ -0,0 +1,21 @@ +/* + * (c) Copyright 2016 Hewlett Packard Enterprise Development Company LP + * + * Licensed under the Apache License, Version 2.0 (the "License"); + * you may not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ +(function() { + 'use strict'; + + angular.module('horizon.framework.util.actions', []); + +})(); diff --git a/horizon/static/framework/util/util.module.js b/horizon/static/framework/util/util.module.js index 5bf2dbe1ac..6d95727f44 100644 --- a/horizon/static/framework/util/util.module.js +++ b/horizon/static/framework/util/util.module.js @@ -3,6 +3,7 @@ angular .module('horizon.framework.util', [ + 'horizon.framework.util.actions', 'horizon.framework.util.bind-scope', 'horizon.framework.util.file', 'horizon.framework.util.filters', diff --git a/horizon/static/framework/widgets/modal/delete-modal.service.js b/horizon/static/framework/widgets/modal/delete-modal.service.js index 0cbdbfc03f..a13ed5cdb0 100644 --- a/horizon/static/framework/widgets/modal/delete-modal.service.js +++ b/horizon/static/framework/widgets/modal/delete-modal.service.js @@ -115,12 +115,8 @@ scope.$emit(context.failedEvent, failEntities.map(getId)); toast.add('error', getMessage(context.labels.error, failEntities)); } - - return { - // Object intentionally left blank. This data is passed to - // code that holds this action's promise. In the future, it may - // contain entity IDs and types that were modified by this action. - }; + // Return the passed and failed entities as part of resolving the promise + return result; } } diff --git a/openstack_dashboard/static/app/core/core.module.js b/openstack_dashboard/static/app/core/core.module.js index 61b06cd474..2656594b02 100644 --- a/openstack_dashboard/static/app/core/core.module.js +++ b/openstack_dashboard/static/app/core/core.module.js @@ -16,6 +16,9 @@ (function () { 'use strict'; + // Constants used within this file + var VOLUME_RESOURCE_TYPE = 'OS::Cinder::Volume'; + /** * @ngdoc overview * @name horizon.app.core @@ -38,6 +41,9 @@ 'horizon.framework.widgets', 'horizon.dashboard.project.workflow' ], config) + // NOTE: this will move into the correct module as that resource type + // becomes available. For now there is no volumes module. + .constant('horizon.app.core.volumes.resourceType', VOLUME_RESOURCE_TYPE) .run([ 'horizon.framework.conf.resource-type-registry.service', performRegistrations @@ -79,7 +85,7 @@ registry.getResourceType('OS::Cinder::Snapshot', { names: [gettext('Volume Snapshot'), gettext('Volume Snapshots')] }); - registry.getResourceType('OS::Cinder::Volume', { + registry.getResourceType(VOLUME_RESOURCE_TYPE, { names: [gettext('Volume'), gettext('Volumes')] }); registry.getResourceType('OS::Nova::Flavor', { 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 139b988ab9..e6f751df48 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 @@ -27,9 +27,11 @@ 'horizon.app.core.openstack-service-api.serviceCatalog', 'horizon.app.core.images.workflows.create-volume.service', 'horizon.app.core.images.events', + 'horizon.framework.util.actions.action-result.service', 'horizon.framework.util.q.extensions', 'horizon.framework.widgets.modal.wizard-modal.service', - 'horizon.framework.widgets.toast.service' + 'horizon.framework.widgets.toast.service', + 'horizon.app.core.volumes.resourceType' ]; /** @@ -46,9 +48,11 @@ serviceCatalog, createVolumeWorkflowService, events, + actionResultService, $qExtensions, wizardModalService, - toast + toast, + volumeResourceType ) { var scope, createVolumePromise, volumeServiceEnabledPromise; var NON_BOOTABLE_IMAGE_TYPES = ['aki', 'ari']; @@ -105,11 +109,12 @@ function showSuccessMessage(response) { var volume = response.data; toast.add('success', interpolate(message.success, [volume.name])); - return { - // Object intentionally left blank. This data is passed to - // code that holds this action's promise. In the future, it may - // contain entity IDs and types that were modified by this action. - }; + + // To make the result of this action generically useful, reformat the return + // from the deleteModal into a standard form + return actionResultService.getActionResult() + .created(volumeResourceType, volume.id) + .result; } function imageBootable(image) { diff --git a/openstack_dashboard/static/app/core/images/actions/delete-image.service.js b/openstack_dashboard/static/app/core/images/actions/delete-image.service.js index c2e08a3b01..fbb1430d62 100644 --- a/openstack_dashboard/static/app/core/images/actions/delete-image.service.js +++ b/openstack_dashboard/static/app/core/images/actions/delete-image.service.js @@ -24,11 +24,12 @@ 'horizon.app.core.openstack-service-api.glance', 'horizon.app.core.openstack-service-api.userSession', 'horizon.app.core.openstack-service-api.policy', + 'horizon.framework.util.actions.action-result.service', 'horizon.framework.util.i18n.gettext', 'horizon.framework.util.q.extensions', 'horizon.framework.widgets.modal.deleteModalService', 'horizon.framework.widgets.toast.service', - 'horizon.app.core.images.events' + 'horizon.app.core.images.resourceType' ]; /** @@ -46,11 +47,12 @@ glance, userSessionService, policy, + actionResultService, gettext, $qExtensions, deleteModal, toast, - events + imagesResourceType ) { var scope, context, deleteImagePromise; var notAllowedMessage = gettext("You are not allowed to delete images: %s"); @@ -67,7 +69,7 @@ function initScope(newScope) { scope = newScope; - context = { successEvent: events.DELETE_SUCCESS }; + context = { }; deleteImagePromise = policy.ifAllowed({rules: [['image', 'delete_image']]}); } @@ -105,11 +107,24 @@ outcome = $q.reject(result.fail); } if (result.pass.length > 0) { - outcome = deleteModal.open(scope, result.pass.map(getEntity), context); + outcome = deleteModal.open(scope, result.pass.map(getEntity), context).then(createResult); } return outcome; } + function createResult(deleteModalResult) { + // To make the result of this action generically useful, reformat the return + // from the deleteModal into a standard form + var actionResult = actionResultService.getActionResult(); + deleteModalResult.pass.forEach(function markDeleted(item) { + actionResult.deleted(imagesResourceType, getEntity(item).id); + }); + deleteModalResult.fail.forEach(function markFailed(item) { + actionResult.failed(imagesResourceType, getEntity(item).id); + }); + return actionResult.result; + } + function labelize(count) { return { diff --git a/openstack_dashboard/static/app/core/images/actions/delete-image.service.spec.js b/openstack_dashboard/static/app/core/images/actions/delete-image.service.spec.js index 8011cad530..35944dad63 100644 --- a/openstack_dashboard/static/app/core/images/actions/delete-image.service.spec.js +++ b/openstack_dashboard/static/app/core/images/actions/delete-image.service.spec.js @@ -19,7 +19,11 @@ var deleteModalService = { open: function () { - return; + deferredModal.resolve({ + pass: [{context: {id: 'a'}}], + fail: [{context: {id: 'b'}}] + }); + return deferredModal.promise; } }; @@ -46,7 +50,7 @@ } }; - var deferred, service, $scope; + var deferred, service, $scope, deferredModal; /////////////////////// @@ -70,6 +74,7 @@ $scope = _$rootScope_.$new(); service = $injector.get('horizon.app.core.images.actions.delete-image.service'); deferred = $q.defer(); + deferredModal = $q.defer(); })); function generateImage(imageCount) { @@ -95,7 +100,7 @@ describe('perform method', function() { beforeEach(function() { - spyOn(deleteModalService, 'open'); + spyOn(deleteModalService, 'open').and.callThrough(); service.initScope($scope, labelize); }); @@ -114,7 +119,6 @@ it('should open the delete modal and show correct labels', testSingleLabels); it('should open the delete modal and show correct labels', testpluralLabels); it('should open the delete modal with correct entities', testEntities); - it('should pass the success and error events to be thrown', testEvents); it('should only delete images that are valid', testValids); it('should fail if this project is not owner', testOwner); it('should fail if images is protected', testProtected); @@ -154,16 +158,6 @@ expect(entities.length).toEqual(imageCount); } - function testEvents() { - var images = generateImage(1); - service.perform(images); - $scope.$apply(); - - var context = deleteModalService.open.calls.argsFor(0)[2]; - expect(deleteModalService.open).toHaveBeenCalled(); - expect(context.successEvent).toEqual('horizon.app.core.images.DELETE_SUCCESS'); - } - function testValids() { var imageCount = 2; var images = generateImage(imageCount); diff --git a/openstack_dashboard/static/app/core/images/actions/edit.action.service.js b/openstack_dashboard/static/app/core/images/actions/edit.action.service.js index 641e315642..e6b3e1b9ad 100644 --- a/openstack_dashboard/static/app/core/images/actions/edit.action.service.js +++ b/openstack_dashboard/static/app/core/images/actions/edit.action.service.js @@ -21,12 +21,14 @@ editService.$inject = [ '$q', + 'horizon.app.core.images.resourceType', 'horizon.app.core.images.events', 'horizon.app.core.images.actions.editWorkflow', 'horizon.app.core.metadata.service', 'horizon.app.core.openstack-service-api.glance', 'horizon.app.core.openstack-service-api.policy', 'horizon.app.core.openstack-service-api.userSession', + 'horizon.framework.util.actions.action-result.service', 'horizon.framework.util.q.extensions', 'horizon.framework.widgets.modal.wizard-modal.service', 'horizon.framework.widgets.toast.service' @@ -39,12 +41,14 @@ */ function editService( $q, + imageResourceType, events, editWorkflow, metadataService, glance, policy, userSessionService, + actionResultService, $qExtensions, wizardModalService, toast @@ -53,7 +57,7 @@ success: gettext('Image %s was successfully updated.'), successMetadata: gettext('Image metadata %s was successfully updated.') }; - var modifyImagePolicyCheck, scope; + var modifyImagePolicyCheck, scope, saveDeferred; var model = { image: {}, @@ -70,31 +74,9 @@ ////////////// - // include this function in your service - // if you plan to emit events to the parent controller function initScope($scope) { - var watchImageChange = $scope.$on(events.IMAGE_CHANGED, onImageChange); - var watchMetadataChange = $scope.$on(events.IMAGE_METADATA_CHANGED, onMetadataChange); - scope = $scope; modifyImagePolicyCheck = policy.ifAllowed({rules: [['image', 'modify_image']]}); - - $scope.$on('$destroy', destroy); - - function destroy() { - watchImageChange(); - watchMetadataChange(); - } - } - - function onImageChange(e, image) { - model.image = image; - e.stopPropagation(); - } - - function onMetadataChange(e, metadata) { - model.metadata = metadata; - e.stopPropagation(); } function allowed(image) { @@ -115,11 +97,14 @@ model.image = localImage; } - return wizardModalService.modal({ + wizardModalService.modal({ scope: scope, workflow: editWorkflow, submit: submit - }).result; + }); + + saveDeferred = $q.defer(); + return saveDeferred.promise; } function submit() { @@ -137,15 +122,15 @@ function onUpdateImageSuccess() { toast.add('success', interpolate(message.success, [model.image.name])); - scope.$emit(events.UPDATE_SUCCESS, model.image); - return { - // This will be filled out with useful information as it is - // decided upon. - }; + saveDeferred.resolve(actionResultService.getActionResult() + .updated(imageResourceType, model.image.id) + .result); } function onUpdateImageFail() { - scope.$emit(events.UPDATE_SUCCESS, model.image); + saveDeferred.reject(actionResultService.getActionResult() + .failed(imageResourceType, model.image.id) + .result); } function saveMetadata() { diff --git a/openstack_dashboard/static/app/core/images/actions/edit.action.service.spec.js b/openstack_dashboard/static/app/core/images/actions/edit.action.service.spec.js index 576896d049..c0d2a9580f 100644 --- a/openstack_dashboard/static/app/core/images/actions/edit.action.service.spec.js +++ b/openstack_dashboard/static/app/core/images/actions/edit.action.service.spec.js @@ -75,7 +75,7 @@ } }; - var service, events, $scope, $q, toast, deferred, testImage, $timeout; + var service, $scope, $q, toast, deferred, testImage, $timeout; /////////////////////// @@ -94,7 +94,6 @@ $scope = _$rootScope_.$new(); $q = _$q_; service = $injector.get('horizon.app.core.images.actions.edit.service'); - events = $injector.get('horizon.app.core.images.events'); toast = $injector.get('horizon.framework.widgets.toast.service'); service.initScope($scope); deferred = $q.defer(); @@ -118,42 +117,8 @@ expect(modalArgs.workflow).toBeDefined(); }); - it('should update image in glance, update metadata and raise event', function() { - testImage = { name: 'Test', id: '2' }; - var newImage = { name: 'Test2', id: '2' }; - var newMetadata = {p1: '11', p3: '3'}; - - spyOn($scope, '$emit').and.callThrough(); - spyOn(glanceAPI, 'updateImage').and.callThrough(); - spyOn(metadataService, 'editMetadata').and.callThrough(); - spyOn(toast, 'add').and.callThrough(); - spyOn(wizardModalService, 'modal').and.callThrough(); - - service.initScope($scope); - service.perform(testImage); - $timeout.flush(); - - $scope.$emit(events.IMAGE_CHANGED, newImage); - $scope.$emit(events.IMAGE_METADATA_CHANGED, newMetadata); - - var modalArgs = wizardModalService.modal.calls.argsFor(0)[0]; - modalArgs.submit(); - $scope.$apply(); - - expect(glanceAPI.updateImage).toHaveBeenCalledWith(newImage); - expect(metadataService.editMetadata) - .toHaveBeenCalledWith('image', '2', newMetadata, ['p2']); - expect(toast.add) - .toHaveBeenCalledWith('success', 'Image Test2 was successfully updated.'); - expect(toast.add.calls.count()).toBe(2); - expect($scope.$emit) - .toHaveBeenCalledWith('horizon.app.core.images.UPDATE_SUCCESS', newImage); - }); - it('should raise event even if update meta data fails', function() { var image = { name: 'Test', id: '2' }; - var newImage = { name: 'Test2', id: '2' }; - var newMetadata = {prop1: '11', prop3: '3'}; var failedPromise = function() { return { @@ -166,51 +131,17 @@ spyOn(wizardModalService, 'modal').and.callThrough(); spyOn(glanceAPI, 'updateImage').and.callThrough(); spyOn(metadataService, 'editMetadata').and.callFake(failedPromise); - spyOn($scope, '$emit').and.callThrough(); spyOn(toast, 'add').and.callThrough(); service.initScope($scope); service.perform(image); $scope.$apply(); - $scope.$emit(events.IMAGE_CHANGED, newImage); - $scope.$emit(events.IMAGE_METADATA_CHANGED, newMetadata); - var modalArgs = wizardModalService.modal.calls.argsFor(0)[0]; modalArgs.submit(); $scope.$apply(); expect(toast.add.calls.count()).toBe(1); - expect($scope.$emit) - .toHaveBeenCalledWith('horizon.app.core.images.UPDATE_SUCCESS', newImage); - }); - - it('should destroy the event watchers', function() { - testImage = { name: 'Test', id: '2' }; - var newImage = { name: 'Test2', id: '2' }; - var newMetadata = {p1: '11', p3: '3'}; - - spyOn(wizardModalService, 'modal').and.callThrough(); - spyOn(glanceAPI, 'updateImage').and.callThrough(); - spyOn(metadataService, 'editMetadata').and.callThrough(); - spyOn(toast, 'add').and.callThrough(); - - service.initScope($scope); - service.perform(testImage); - $scope.$apply(); - - $scope.$emit('$destroy'); - $scope.$emit(events.IMAGE_CHANGED, newImage); - $scope.$emit(events.IMAGE_METADATA_CHANGED, newMetadata); - - var modalArgs = wizardModalService.modal.calls.argsFor(0)[0]; - modalArgs.submit(); - $scope.$apply(); - - expect(glanceAPI.updateImage).toHaveBeenCalledWith(testImage); - expect(metadataService.editMetadata) - .toHaveBeenCalledWith('image', testImage.id, {}, ['p1', 'p2']); - expect(toast.add.calls.count()).toBe(2); }); }); diff --git a/openstack_dashboard/static/app/core/images/actions/launch-instance.service.js b/openstack_dashboard/static/app/core/images/actions/launch-instance.service.js index e2edd5df0f..1d17fbfe99 100644 --- a/openstack_dashboard/static/app/core/images/actions/launch-instance.service.js +++ b/openstack_dashboard/static/app/core/images/actions/launch-instance.service.js @@ -52,8 +52,10 @@ ////////////// function perform(image) { + // Previous uses of this relocated the display using the successUrl; + // in this case we leave the post-action behavior up to the result + // handler. return launchInstanceModal.open({ - successUrl: '/project/instances', 'imageId': image.id }); } diff --git a/openstack_dashboard/static/app/core/images/actions/launch-instance.service.spec.js b/openstack_dashboard/static/app/core/images/actions/launch-instance.service.spec.js index abf86665d9..91f8dbab71 100644 --- a/openstack_dashboard/static/app/core/images/actions/launch-instance.service.spec.js +++ b/openstack_dashboard/static/app/core/images/actions/launch-instance.service.spec.js @@ -45,7 +45,6 @@ expect(launchInstanceModalMock.open).toHaveBeenCalled(); expect(launchInstanceModalMock.open.calls.argsFor(0)).toEqual([{ - successUrl: '/project/instances', imageId: '1' }]); }); diff --git a/openstack_dashboard/static/app/core/images/actions/update-metadata.action.service.js b/openstack_dashboard/static/app/core/images/actions/update-metadata.action.service.js index 02b7479d95..9e34e47912 100644 --- a/openstack_dashboard/static/app/core/images/actions/update-metadata.action.service.js +++ b/openstack_dashboard/static/app/core/images/actions/update-metadata.action.service.js @@ -25,7 +25,9 @@ 'horizon.app.core.images.events', 'horizon.app.core.metadata.modal.service', 'horizon.app.core.openstack-service-api.userSession', - 'horizon.framework.util.q.extensions' + 'horizon.framework.util.actions.action-result.service', + 'horizon.framework.util.q.extensions', + 'horizon.app.core.images.resourceType' ]; /** @@ -42,12 +44,12 @@ events, metadataModalService, userSessionService, - $qExtensions + actionResultService, + $qExtensions, + imageResourceType ) { - var scope; var service = { - initScope: initScope, perform: perform, allowed: allowed }; @@ -56,22 +58,17 @@ ////////////// - function initScope(newScope) { - scope = newScope; - } - function perform(image) { return metadataModalService.open('image', image.id) .result .then(onSuccess); function onSuccess() { - scope.$emit(events.UPDATE_METADATA_SUCCESS, [image.id]); - return { - // Object intentionally left blank. This data is passed to - // code that holds this action's promise. In the future, it may - // contain entity IDs and types that were modified by this action. - }; + // To make the result of this action generically useful, reformat the return + // from the deleteModal into a standard form + return actionResultService.getActionResult() + .updated(imageResourceType, image.id) + .result; } } diff --git a/openstack_dashboard/static/app/core/images/actions/update-metadata.action.service.spec.js b/openstack_dashboard/static/app/core/images/actions/update-metadata.action.service.spec.js index 62913ec136..ca15412a7c 100644 --- a/openstack_dashboard/static/app/core/images/actions/update-metadata.action.service.spec.js +++ b/openstack_dashboard/static/app/core/images/actions/update-metadata.action.service.spec.js @@ -57,24 +57,17 @@ } }; - spyOn($scope, '$emit').and.callThrough(); spyOn(metadataModalMock, 'open').and.returnValue(fakeModalService); - service.initScope($scope); service.perform({id: '1', name: 'image1'}); expect(metadataModalMock.open).toHaveBeenCalled(); expect(metadataModalMock.open.calls.argsFor(0)).toEqual(['image', '1']); - expect($scope.$emit).toHaveBeenCalledWith( - 'horizon.app.core.images.UPDATE_METADATA_SUCCESS', - ['1'] - ); }); describe('Update Metadata', function() { it('should allow Update Metadata if image can be deleted', function() { var image = {owner: 'project', status: 'active'}; - service.initScope($scope); permissionShouldPass(service.allowed(image)); $scope.$apply(); }); @@ -82,14 +75,12 @@ it('should not allow Update Metadata if service call is rejected', function() { var image = {owner: 'doesnt_matter', status: 'active'}; deferred.reject(); - service.initScope($scope); permissionShouldFail(service.allowed(image)); $scope.$apply(); }); it('should not allow Update Metadata if image status is not active', function() { var image = {owner: 'project', status: 'not_active'}; - service.initScope($scope); permissionShouldFail(service.allowed(image)); $scope.$apply(); }); diff --git a/openstack_dashboard/static/app/core/images/table/images-table.html b/openstack_dashboard/static/app/core/images/table/images-table.html index e9a93752f7..c097cc6e65 100644 --- a/openstack_dashboard/static/app/core/images/table/images-table.html +++ b/openstack_dashboard/static/app/core/images/table/images-table.html @@ -13,7 +13,8 @@ --> - + @@ -55,7 +56,7 @@ Include action-col if you want to perform actions. rsp-p1 rsp-p2 are responsive priority as user resizes window. --> - +
@@ -85,7 +86,8 @@ Table-row-action-column: Actions taken here applies to a single item/row. --> - + diff --git a/openstack_dashboard/static/app/core/images/table/images.controller.js b/openstack_dashboard/static/app/core/images/table/images.controller.js index 2338c416d2..a1a43052b6 100644 --- a/openstack_dashboard/static/app/core/images/table/images.controller.js +++ b/openstack_dashboard/static/app/core/images/table/images.controller.js @@ -30,6 +30,7 @@ 'horizon.app.core.openstack-service-api.glance', 'horizon.app.core.openstack-service-api.userSession', 'horizon.framework.conf.resource-type-registry.service', + 'horizon.framework.util.actions.action-result.service', 'imageVisibilityFilter' ]; @@ -50,6 +51,7 @@ glance, userSession, typeRegistry, + actionResultService, imageVisibilityFilter ) { var ctrl = this; @@ -58,23 +60,19 @@ ctrl.checked = {}; - ctrl.images = []; - ctrl.imagesSrc = []; ctrl.metadataDefs = null; ctrl.imageResourceType = typeRegistry.getResourceType(imageResourceType); + ctrl.actionResultHandler = actionResultHandler; - var deleteWatcher = $scope.$on(events.DELETE_SUCCESS, onDeleteSuccess); - var updateWatcher = $scope.$on(events.UPDATE_SUCCESS, onUpdateSuccess); - - $scope.$on('$destroy', destroy); - - init(); + typeRegistry.initActions(imageResourceType, $scope); + loadImages(); //////////////////////////////// - function init() { - typeRegistry.initActions(imageResourceType, $scope); + function loadImages() { + ctrl.images = []; + ctrl.imagesSrc = []; $q.all( { images: glance.getImages(), @@ -96,21 +94,6 @@ applyMetadataDefinitions(); } - function onUpdateSuccess(e, image) { - e.stopPropagation(); - ctrl.imagesSrc = difference(ctrl.imagesSrc, [image.id], 'id'); - ctrl.imagesSrc.push(image); - } - - function onDeleteSuccess(e, removedImageIds) { - ctrl.imagesSrc = difference(ctrl.imagesSrc, removedImageIds, 'id'); - e.stopPropagation(); - - // after deleting the items - // we need to clear selected items from table controller - $scope.$emit('hzTable:clearSelected'); - } - function difference(currentList, otherList, key) { return currentList.filter(filter); @@ -121,11 +104,6 @@ } } - function destroy() { - updateWatcher(); - deleteWatcher(); - } - function applyMetadataDefinitions() { glance.getNamespaces({resource_type: imageResourceType}, true) .then(function setMetadefs(data) { @@ -133,6 +111,56 @@ }); } + function actionResultHandler(returnValue) { + return $q.when(returnValue, actionSuccessHandler); + } + + function actionSuccessHandler(result) { // eslint-disable-line no-unused-vars + + // The action has completed (for whatever "complete" means to that + // action. Notice the view doesn't really need to know the semantics of the + // particular action because the actions return data in a standard form. + // That return includes the id and type of each created, updated, deleted + // and failed item. + // + // This handler is also careful to check the type of each item. This + // is important because actions which create non-images are launched from + // the images page (like create "volume" from image). + var deletedIds, updatedIds, createdIds, failedIds; + + if ( result ) { + // Reduce the results to just image ids ignoring other types the action + // may have produced + deletedIds = actionResultService.getIdsOfType(result.deleted, imageResourceType); + updatedIds = actionResultService.getIdsOfType(result.updated, imageResourceType); + createdIds = actionResultService.getIdsOfType(result.created, imageResourceType); + failedIds = actionResultService.getIdsOfType(result.failed, imageResourceType); + + // Handle deleted images + if (deletedIds.length) { + ctrl.imagesSrc = difference(ctrl.imagesSrc, deletedIds,'id'); + } + + // Handle updated and created images + if ( updatedIds.length || createdIds.length ) { + // Ideally, get each created image individually, but + // this is simple and robust for the common use case. + // TODO: If we want more detailed updates, we could do so here. + loadImages(); + } + + // Handle failed images + if ( failedIds.length ) { + // Do nothing for now + } + + } else { + // promise resolved, but no result returned. Because the action didn't + // tell us what happened...reload the displayed images just in case. + loadImages(); + } + } + } })(); diff --git a/openstack_dashboard/static/app/core/images/table/images.controller.spec.js b/openstack_dashboard/static/app/core/images/table/images.controller.spec.js index 9200dc5747..dca9cc8c25 100644 --- a/openstack_dashboard/static/app/core/images/table/images.controller.spec.js +++ b/openstack_dashboard/static/app/core/images/table/images.controller.spec.js @@ -57,6 +57,9 @@ callback(input); } }; + }, + when: function (input, callback) { + return callback(input); } }; @@ -65,7 +68,7 @@ 2: {id: '2', is_public: false, owner: 'not_me', filtered_visibility: 'Shared with Me'} }; - var $scope, controller, events, detailsRoute; + var $scope, controller, detailsRoute; beforeEach(module('ui.bootstrap')); beforeEach(module('horizon.framework')); @@ -85,7 +88,6 @@ beforeEach(inject(function ($injector, _$rootScope_) { $scope = _$rootScope_.$new(); - events = $injector.get('horizon.app.core.images.events'); controller = $injector.get('$controller'); detailsRoute = $injector.get('horizon.app.core.images.detailsRoute'); @@ -121,37 +123,62 @@ expect(glanceAPI.getNamespaces).toHaveBeenCalled(); }); - it('should refresh images after delete', function() { + it('re-queries if no result', function() { + var ctrl = createController(); + glanceAPI.getImages.calls.reset(); + ctrl.actionResultHandler(); + expect(glanceAPI.getImages).toHaveBeenCalled(); + }); + + it('re-queries if updated', function() { + var ctrl = createController(); + glanceAPI.getImages.calls.reset(); + ctrl.actionResultHandler({updated: [{type: 'OS::Glance::Image', id: 'b'}]}); + expect(glanceAPI.getImages).toHaveBeenCalled(); + }); + + it('re-queries if created', function() { + var ctrl = createController(); + glanceAPI.getImages.calls.reset(); + ctrl.actionResultHandler({created: [{type: 'OS::Glance::Image', id: 'b'}]}); + expect(glanceAPI.getImages).toHaveBeenCalled(); + }); + + it('does not re-query if only failed', function() { + var ctrl = createController(); + glanceAPI.getImages.calls.reset(); + ctrl.actionResultHandler({failed: [{type: 'OS::Glance::Image', id: 'b'}]}); + expect(glanceAPI.getImages).not.toHaveBeenCalled(); + }); + + it('should remove deleted images', function() { var ctrl = createController(); expect(ctrl.imagesSrc).toEqual([ expectedImages['1'], expectedImages['2'] ]); - spyOn($scope, '$emit').and.callThrough(); - $scope.$emit(events.DELETE_SUCCESS, ['1']); + var result = { + deleted: [ {type: "OS::Glance::Image", id: '1'} ] + }; + ctrl.actionResultHandler(result); expect(ctrl.imagesSrc).toEqual([ expectedImages['2'] ]); - - expect($scope.$emit).toHaveBeenCalledWith('hzTable:clearSelected'); }); - it('should refresh images after update', function() { + it('should not remove deleted volumes', function() { var ctrl = createController(); - expect(ctrl.imagesSrc).toEqual(images); + expect(ctrl.imagesSrc).toEqual([ + expectedImages['1'], + expectedImages['2'] + ]); - $scope.$emit(events.UPDATE_SUCCESS, {id: '1', name: 'name_new'}); - - expect(ctrl.imagesSrc.filter(function (x) { return x.id === '1'; })[0].name).toBe('name_new'); - }); - - it('should destroy the event watcher for delete', function() { - var ctrl = createController(); - - $scope.$emit('$destroy'); - $scope.$emit(events.DELETE_SUCCESS, ['1']); + var result = { + deleted: [ {type: "OS::Cinder::Values", id: '1'} ] + }; + ctrl.actionResultHandler(result); expect(ctrl.imagesSrc).toEqual([ expectedImages['1'], @@ -159,23 +186,5 @@ ]); }); - it('should destroy the event watcher for update', function() { - var ctrl = createController(); - - $scope.$emit('$destroy'); - $scope.$emit(events.UPDATE_SUCCESS, {id: '1', name: 'name_new'}); - - expect(ctrl.imagesSrc).toEqual(images); - }); - - it('should destroy the event watcher for create', function() { - var ctrl = createController(); - - $scope.$emit('$destroy'); - $scope.$emit(events.createSuccess, {id: '3'}); - - expect(ctrl.imagesSrc).toEqual(images); - }); - }); })(); diff --git a/releasenotes/notes/action-results-303c282165b60f47.yaml b/releasenotes/notes/action-results-303c282165b60f47.yaml new file mode 100644 index 0000000000..f33fed94e3 --- /dev/null +++ b/releasenotes/notes/action-results-303c282165b60f47.yaml @@ -0,0 +1,12 @@ +--- +prelude: > + Angular actions now should return a promise + that resolves with an object structured in a way + to indicate what the action did (or didn't do). +features: + - An action-result service provides convenience methods + for construction of the result, and for parsing of + a resolved object +upgrade: + - Although it's not required, it's best to make your + actions return promises with the expected structure.