diff --git a/platform/commonUI/edit/bundle.js b/platform/commonUI/edit/bundle.js index 880e61f5da..2141110409 100644 --- a/platform/commonUI/edit/bundle.js +++ b/platform/commonUI/edit/bundle.js @@ -42,6 +42,7 @@ define([ "./src/representers/EditToolbarRepresenter", "./src/capabilities/EditorCapability", "./src/capabilities/TransactionCapabilityDecorator", + "./src/services/TransactionManager", "./src/services/TransactionService", "./src/creation/CreateMenuController", "./src/creation/LocatorController", @@ -80,6 +81,7 @@ define([ EditToolbarRepresenter, EditorCapability, TransactionCapabilityDecorator, + TransactionManager, TransactionService, CreateMenuController, LocatorController, @@ -222,8 +224,7 @@ define([ "policyService", "dialogService", "creationService", - "copyService", - "transactionService" + "copyService" ], "priority": "mandatory" }, @@ -321,7 +322,7 @@ define([ "implementation": TransactionCapabilityDecorator, "depends": [ "$q", - "transactionService" + "transactionManager" ], "priority": "fallback" }, @@ -406,6 +407,15 @@ define([ "key": "locator", "template": locatorTemplate } + ], + "services": [ + { + "key": "transactionManager", + "implementation": TransactionManager, + "depends": [ + "transactionService" + ] + } ] } }); diff --git a/platform/commonUI/edit/src/actions/SaveAsAction.js b/platform/commonUI/edit/src/actions/SaveAsAction.js index 4e2c5d870b..57a055429a 100644 --- a/platform/commonUI/edit/src/actions/SaveAsAction.js +++ b/platform/commonUI/edit/src/actions/SaveAsAction.js @@ -44,7 +44,6 @@ define([ dialogService, creationService, copyService, - transactionService, context ) { this.domainObject = (context || {}).domainObject; @@ -55,7 +54,6 @@ define([ this.dialogService = dialogService; this.creationService = creationService; this.copyService = copyService; - this.transactionService = transactionService; } /** @@ -113,9 +111,8 @@ define([ var self = this, domainObject = this.domainObject, copyService = this.copyService, - transactionService = this.transactionService, - cancelOldTransaction, - dialog = new SaveInProgressDialog(this.dialogService); + dialog = new SaveInProgressDialog(this.dialogService), + toUndirty = []; function doWizardSave(parent) { var wizard = self.createWizard(parent); @@ -147,29 +144,33 @@ define([ } function allowClone(objectToClone) { - return (objectToClone.getId() === domainObject.getId()) || - objectToClone.getCapability('location').isOriginal(); + var allowed = + (objectToClone.getId() === domainObject.getId()) || + objectToClone.getCapability('location').isOriginal(); + if (allowed) { + toUndirty.push(objectToClone); + } + return allowed; } function cloneIntoParent(parent) { return copyService.perform(domainObject, parent, allowClone); } + function undirty(object) { + return object.getCapability('persistence').refresh(); + } + + function undirtyOriginals(object) { + return Promise.all(toUndirty.map(undirty)) + .then(resolveWith(object)); + } + function commitEditingAfterClone(clonedObject) { return domainObject.getCapability("editor").save() .then(resolveWith(clonedObject)); } - function restartTransaction(object) { - cancelOldTransaction = transactionService.restartTransaction(); - return object; - } - - function doCancelOldTransaction(object) { - cancelOldTransaction(); - return object; - } - function onFailure() { hideBlockingDialog(); return false; @@ -179,10 +180,9 @@ define([ .then(doWizardSave) .then(showBlockingDialog) .then(getParent) - .then(restartTransaction) .then(cloneIntoParent) + .then(undirtyOriginals) .then(commitEditingAfterClone) - .then(doCancelOldTransaction) .then(hideBlockingDialog) .catch(onFailure); }; diff --git a/platform/commonUI/edit/src/capabilities/TransactionalPersistenceCapability.js b/platform/commonUI/edit/src/capabilities/TransactionalPersistenceCapability.js index a85e177f9e..bcda009c56 100644 --- a/platform/commonUI/edit/src/capabilities/TransactionalPersistenceCapability.js +++ b/platform/commonUI/edit/src/capabilities/TransactionalPersistenceCapability.js @@ -33,22 +33,21 @@ define( * called. * @memberof platform/commonUI/edit/capabilities * @param $q - * @param transactionService + * @param transactionManager * @param persistenceCapability * @param domainObject * @constructor */ function TransactionalPersistenceCapability( $q, - transactionService, + transactionManager, persistenceCapability, domainObject ) { - this.transactionService = transactionService; + this.transactionManager = transactionManager; this.persistenceCapability = persistenceCapability; this.domainObject = domainObject; this.$q = $q; - this.persistPending = false; } /** @@ -57,34 +56,14 @@ define( * @returns {*} */ TransactionalPersistenceCapability.prototype.persist = function () { - var self = this; + var wrappedPersistence = this.persistenceCapability; - function onCommit() { - return self.persistenceCapability.persist().then(function (result) { - self.persistPending = false; - return result; - }); - } - - function onCancel() { - if (self.domainObject.getModel().persisted !== undefined) { - //Fetch clean model from persistence - return self.persistenceCapability.refresh().then(function (result) { - self.persistPending = false; - return result; - }); - } else { - self.persistPending = false; - //Model is undefined in persistence, so return undefined. - return self.$q.when(undefined); - } - } - - if (this.transactionService.isActive()) { - if (!this.persistPending) { - this.transactionService.addToTransaction(onCommit, onCancel); - this.persistPending = true; - } + if (this.transactionManager.isActive()) { + this.transactionManager.addToTransaction( + this.domainObject.getId(), + wrappedPersistence.persist.bind(wrappedPersistence), + wrappedPersistence.refresh.bind(wrappedPersistence) + ); //Need to return a promise from this function return this.$q.when(true); } else { @@ -93,6 +72,8 @@ define( }; TransactionalPersistenceCapability.prototype.refresh = function () { + this.transactionManager + .clearTransactionsFor(this.domainObject.getId()); return this.persistenceCapability.refresh(); }; diff --git a/platform/commonUI/edit/src/services/TransactionManager.js b/platform/commonUI/edit/src/services/TransactionManager.js new file mode 100644 index 0000000000..d27ba95ced --- /dev/null +++ b/platform/commonUI/edit/src/services/TransactionManager.js @@ -0,0 +1,113 @@ +/***************************************************************************** + * Open MCT, Copyright (c) 2014-2016, United States Government + * as represented by the Administrator of the National Aeronautics and Space + * Administration. All rights reserved. + * + * Open MCT is 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. + * + * Open MCT includes source code licensed under additional open source + * licenses. See the Open Source Licenses file (LICENSES.md) included with + * this source code distribution or the Licensing information page available + * at runtime from the About dialog for additional information. + *****************************************************************************/ + +define([], function () { + /** + * Manages transactions to support the TransactionalPersistenceCapability. + * This assumes that all commit/cancel callbacks for a given domain + * object are equivalent, and only need to be added once to any active + * transaction. Violating this assumption may cause unexpected behavior. + * @constructor + * @memberof platform/commonUI/edit + */ + function TransactionManager(transactionService) { + this.transactionService = transactionService; + this.clearTransactionFns = {}; + } + + /** + * Check if a transaction is currently active. + * @returns {boolean} true if there is a transaction active + */ + TransactionManager.prototype.isActive = function () { + return this.transactionService.isActive(); + }; + + /** + * Check if callbacks associated with this domain object have already + * been added to the active transaction. + * @private + * @param {string} id the identifier of the domain object to check + * @returns {boolean} true if callbacks have been added + */ + TransactionManager.prototype.isScheduled = function (id) { + return !!this.clearTransactionFns[id]; + }; + + /** + * Add callbacks associated with this domain object to the active + * transaction. Both callbacks are expected to return promises that + * resolve when their associated behavior is complete. + * + * If callbacks associated with this domain object have already been + * added to the active transaction, this call will be ignored. + * + * @param {string} id the identifier of the associated domain object + * @param {Function} onCommit behavior to invoke when committing transaction + * @param {Function} onCancel behavior to invoke when cancelling transaction + */ + TransactionManager.prototype.addToTransaction = function ( + id, + onCommit, + onCancel + ) { + var release = this.releaseClearFn.bind(this, id); + + function chain(promiseFn, nextFn) { + return function () { + return promiseFn().then(nextFn); + }; + } + + if (!this.isScheduled(id)) { + this.clearTransactionFns[id] = + this.transactionService.addToTransaction( + chain(onCommit, release), + chain(onCancel, release) + ); + } + }; + + /** + * Remove any callbacks associated with this domain object from the + * active transaction. + * @param {string} id the identifier for the domain object + */ + TransactionManager.prototype.clearTransactionsFor = function (id) { + if (this.isScheduled(id)) { + this.clearTransactionFns[id](); + this.releaseClearFn(id); + } + }; + + /** + * Release the cached "remove from transaction" function that has been + * stored in association with this domain object. + * @param {string} id the identifier for the domain object + * @private + */ + TransactionManager.prototype.releaseClearFn = function (id) { + delete this.clearTransactionFns[id]; + }; + + return TransactionManager; +}); diff --git a/platform/commonUI/edit/src/services/TransactionService.js b/platform/commonUI/edit/src/services/TransactionService.js index b5439d4491..119e314b17 100644 --- a/platform/commonUI/edit/src/services/TransactionService.js +++ b/platform/commonUI/edit/src/services/TransactionService.js @@ -81,6 +81,15 @@ define( //Log error because this is a programming error if it occurs. this.$log.error("No transaction in progress"); } + + return function () { + this.onCommits = this.onCommits.filter(function (callback) { + return callback !== onCommit; + }); + this.onCancels = this.onCancels.filter(function (callback) { + return callback !== onCancel; + }); + }.bind(this); }; /** @@ -140,38 +149,9 @@ define( }); }; - /** - * Clear and restart the active transaction. - * - * This neither cancels nor commits the active transaction; - * instead, it returns a function that can be used to cancel that - * transaction. - * - * @returns {Function} a function to cancel the prior transaction - * @private - */ - TransactionService.prototype.restartTransaction = function () { - var oldOnCancels = this.onCancels; - - this.onCommits = []; - this.onCancels = []; - - return function () { - while (oldOnCancels.length > 0) { - var onCancel = oldOnCancels.pop(); - try { - onCancel(); - } catch (error) { - this.$log.error("Error cancelling transaction."); - } - } - }; - }; - TransactionService.prototype.size = function () { return this.onCommits.length; }; return TransactionService; - } -); + }); diff --git a/platform/commonUI/edit/test/actions/SaveAsActionSpec.js b/platform/commonUI/edit/test/actions/SaveAsActionSpec.js index ec649571ee..4ad8ffa215 100644 --- a/platform/commonUI/edit/test/actions/SaveAsActionSpec.js +++ b/platform/commonUI/edit/test/actions/SaveAsActionSpec.js @@ -34,7 +34,6 @@ define( mockCopyService, mockParent, mockUrlService, - mockTransactionService, actionContext, capabilities = {}, action; @@ -120,26 +119,11 @@ define( ["urlForLocation"] ); - mockTransactionService = jasmine.createSpyObj( - "transactionService", - ["restartTransaction"] - ); - mockTransactionService.restartTransaction - .andReturn(jasmine.createSpy()); - actionContext = { domainObject: mockDomainObject }; - action = new SaveAsAction( - undefined, - undefined, - mockDialogService, - undefined, - mockCopyService, - mockTransactionService, - actionContext - ); + action = new SaveAsAction(undefined, undefined, mockDialogService, undefined, mockCopyService, actionContext); spyOn(action, "getObjectService"); action.getObjectService.andReturn(mockObjectService); @@ -195,9 +179,15 @@ define( }); it("hides the blocking dialog after saving", function () { - action.perform(); + var mockCallback = jasmine.createSpy(); + action.perform().then(mockCallback); expect(mockDialogService.showBlockingMessage).toHaveBeenCalled(); - expect(mockDialogService.dismiss).toHaveBeenCalled(); + waitsFor(function () { + return mockCallback.calls.length > 0; + }); + runs(function () { + expect(mockDialogService.dismiss).toHaveBeenCalled(); + }); }); }); diff --git a/platform/commonUI/edit/test/capabilities/TransactionalPersistenceCapabilitySpec.js b/platform/commonUI/edit/test/capabilities/TransactionalPersistenceCapabilitySpec.js index 1157ba5892..e5129f0055 100644 --- a/platform/commonUI/edit/test/capabilities/TransactionalPersistenceCapabilitySpec.js +++ b/platform/commonUI/edit/test/capabilities/TransactionalPersistenceCapabilitySpec.js @@ -37,77 +37,74 @@ define( describe("The transactional persistence decorator", function () { var mockQ, - mockTransactionService, + mockTransactionManager, mockPersistence, mockDomainObject, + testId, capability; beforeEach(function () { + testId = "test-id"; + mockQ = jasmine.createSpyObj("$q", ["when"]); mockQ.when.andCallFake(function (val) { return fastPromise(val); }); - mockTransactionService = jasmine.createSpyObj( + mockTransactionManager = jasmine.createSpyObj( "transactionService", - ["isActive", "addToTransaction"] + ["isActive", "addToTransaction", "clearTransactionsFor"] ); mockPersistence = jasmine.createSpyObj( "persistenceCapability", - ["persist", "refresh"] + ["persist", "refresh", "getSpace"] ); mockPersistence.persist.andReturn(fastPromise()); mockPersistence.refresh.andReturn(fastPromise()); mockDomainObject = jasmine.createSpyObj( "domainObject", - [ - "getModel" - ] + ["getModel", "getId"] ); mockDomainObject.getModel.andReturn({persisted: 1}); + mockDomainObject.getId.andReturn(testId); - capability = new TransactionalPersistenceCapability(mockQ, mockTransactionService, mockPersistence, mockDomainObject); + capability = new TransactionalPersistenceCapability( + mockQ, + mockTransactionManager, + mockPersistence, + mockDomainObject + ); }); it("if no transaction is active, passes through to persistence" + " provider", function () { - mockTransactionService.isActive.andReturn(false); + mockTransactionManager.isActive.andReturn(false); capability.persist(); expect(mockPersistence.persist).toHaveBeenCalled(); }); it("if transaction is active, persist and cancel calls are" + " queued", function () { - mockTransactionService.isActive.andReturn(true); + mockTransactionManager.isActive.andReturn(true); capability.persist(); - expect(mockTransactionService.addToTransaction).toHaveBeenCalled(); - mockTransactionService.addToTransaction.mostRecentCall.args[0](); + expect(mockTransactionManager.addToTransaction).toHaveBeenCalled(); + mockTransactionManager.addToTransaction.mostRecentCall.args[1](); expect(mockPersistence.persist).toHaveBeenCalled(); - mockTransactionService.addToTransaction.mostRecentCall.args[1](); + mockTransactionManager.addToTransaction.mostRecentCall.args[2](); expect(mockPersistence.refresh).toHaveBeenCalled(); }); - it("if transaction is active, cancel call is queued that refreshes model when appropriate", function () { - mockTransactionService.isActive.andReturn(true); - capability.persist(); - expect(mockTransactionService.addToTransaction).toHaveBeenCalled(); - - mockDomainObject.getModel.andReturn({}); - mockTransactionService.addToTransaction.mostRecentCall.args[1](); - expect(mockPersistence.refresh).not.toHaveBeenCalled(); - - mockDomainObject.getModel.andReturn({persisted: 1}); - mockTransactionService.addToTransaction.mostRecentCall.args[1](); - expect(mockPersistence.refresh).toHaveBeenCalled(); + it("wraps getSpace", function () { + mockPersistence.getSpace.andReturn('foo'); + expect(capability.getSpace()).toEqual('foo'); }); - it("persist call is only added to transaction once", function () { - mockTransactionService.isActive.andReturn(true); - capability.persist(); - expect(mockTransactionService.addToTransaction).toHaveBeenCalled(); - capability.persist(); - expect(mockTransactionService.addToTransaction.calls.length).toBe(1); - + it("clears transactions and delegates refresh calls", function () { + capability.refresh(); + expect(mockTransactionManager.clearTransactionsFor) + .toHaveBeenCalledWith(testId); + expect(mockPersistence.refresh) + .toHaveBeenCalled(); }); }); diff --git a/platform/commonUI/edit/test/services/TransactionManagerSpec.js b/platform/commonUI/edit/test/services/TransactionManagerSpec.js new file mode 100644 index 0000000000..1b616fc44b --- /dev/null +++ b/platform/commonUI/edit/test/services/TransactionManagerSpec.js @@ -0,0 +1,132 @@ +/***************************************************************************** + * Open MCT, Copyright (c) 2014-2016, United States Government + * as represented by the Administrator of the National Aeronautics and Space + * Administration. All rights reserved. + * + * Open MCT is 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. + * + * Open MCT includes source code licensed under additional open source + * licenses. See the Open Source Licenses file (LICENSES.md) included with + * this source code distribution or the Licensing information page available + * at runtime from the About dialog for additional information. + *****************************************************************************/ +/*global define,describe,it,expect,beforeEach,jasmine*/ + +define( + ["../../src/services/TransactionManager"], + function (TransactionManager) { + describe("TransactionManager", function () { + var mockTransactionService, + testId, + mockOnCommit, + mockOnCancel, + mockRemoves, + mockPromise, + manager; + + beforeEach(function () { + mockRemoves = []; + mockTransactionService = jasmine.createSpyObj( + "transactionService", + ["addToTransaction", "isActive"] + ); + mockOnCommit = jasmine.createSpy('commit'); + mockOnCancel = jasmine.createSpy('cancel'); + testId = 'test-id'; + mockPromise = jasmine.createSpyObj('promise', ['then']); + + mockOnCommit.andReturn(mockPromise); + mockOnCancel.andReturn(mockPromise); + + mockTransactionService.addToTransaction.andCallFake(function () { + var mockRemove = + jasmine.createSpy('remove-' + mockRemoves.length); + mockRemoves.push(mockRemove); + return mockRemove; + }); + + manager = new TransactionManager(mockTransactionService); + }); + + it("delegates isActive calls", function () { + [false, true].forEach(function (state) { + mockTransactionService.isActive.andReturn(state); + expect(manager.isActive()).toBe(state); + }); + }); + + describe("when addToTransaction is called", function () { + beforeEach(function () { + manager.addToTransaction( + testId, + mockOnCommit, + mockOnCancel + ); + }); + + it("adds callbacks to the active transaction", function () { + expect(mockTransactionService.addToTransaction) + .toHaveBeenCalledWith( + jasmine.any(Function), + jasmine.any(Function) + ); + }); + + it("invokes passed-in callbacks from its own callbacks", function () { + expect(mockOnCommit).not.toHaveBeenCalled(); + mockTransactionService.addToTransaction + .mostRecentCall.args[0](); + expect(mockOnCommit).toHaveBeenCalled(); + + expect(mockOnCancel).not.toHaveBeenCalled(); + mockTransactionService.addToTransaction + .mostRecentCall.args[1](); + expect(mockOnCancel).toHaveBeenCalled(); + }); + + it("ignores subsequent calls for the same object", function () { + manager.addToTransaction( + testId, + jasmine.createSpy(), + jasmine.createSpy() + ); + expect(mockTransactionService.addToTransaction.calls.length) + .toEqual(1); + }); + + it("accepts subsequent calls for other objects", function () { + manager.addToTransaction( + 'other-id', + jasmine.createSpy(), + jasmine.createSpy() + ); + expect(mockTransactionService.addToTransaction.calls.length) + .toEqual(2); + }); + + it("does not remove callbacks from the transaction", function () { + expect(mockRemoves[0]).not.toHaveBeenCalled(); + }); + + describe("and clearTransactionsFor is subsequently called", function () { + beforeEach(function () { + manager.clearTransactionsFor(testId); + }); + + it("removes callbacks from the transaction", function () { + expect(mockRemoves[0]).toHaveBeenCalled(); + }); + }); + }); + }); + } +); diff --git a/platform/core/src/capabilities/PersistenceCapability.js b/platform/core/src/capabilities/PersistenceCapability.js index e58d569005..275080368b 100644 --- a/platform/core/src/capabilities/PersistenceCapability.js +++ b/platform/core/src/capabilities/PersistenceCapability.js @@ -152,6 +152,10 @@ define( }, modified); } + if (domainObject.getModel().persisted === undefined) { + return this.$q.when(true); + } + return this.persistenceService.readObject( this.getSpace(), this.getKey() diff --git a/platform/core/test/capabilities/PersistenceCapabilitySpec.js b/platform/core/test/capabilities/PersistenceCapabilitySpec.js index 0947a7bef7..5dc8486401 100644 --- a/platform/core/test/capabilities/PersistenceCapabilitySpec.js +++ b/platform/core/test/capabilities/PersistenceCapabilitySpec.js @@ -74,7 +74,7 @@ define( ); mockQ = jasmine.createSpyObj( "$q", - ["reject"] + ["reject", "when"] ); mockNofificationService = jasmine.createSpyObj( "notificationService", @@ -103,6 +103,7 @@ define( mockIdentifierService.parse.andReturn(mockIdentifier); mockIdentifier.getSpace.andReturn(SPACE); mockIdentifier.getKey.andReturn(key); + mockQ.when.andCallFake(asPromise); persistence = new PersistenceCapability( mockCacheService, mockPersistenceService, @@ -156,6 +157,7 @@ define( }); it("refreshes the domain object model from persistence", function () { var refreshModel = {someOtherKey: "some other value"}; + model.persisted = 1; mockPersistenceService.readObject.andReturn(asPromise(refreshModel)); persistence.refresh(); expect(model).toEqual(refreshModel);