Merge pull request #1084 from nasa/transaction-clearing-1059

[Persistence] Clear transactions selectively

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",
+                        "copyService"
-                        "transactionService"
                     ],
                     "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"
+                    ]
+                }
             ]
         }
     });

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,
+                dialog = new SaveInProgressDialog(this.dialogService),
-                cancelOldTransaction,
+                toUndirty = [];
-                dialog = new SaveInProgressDialog(this.dialogService);
 
             function doWizardSave(parent) {
                 var wizard = self.createWizard(parent);
@@ -147,29 +144,33 @@ define([
             }
 
             function allowClone(objectToClone) {
-                return (objectToClone.getId() === domainObject.getId()) ||
+                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);
         };

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() {
+            if (this.transactionManager.isActive()) {
-                return self.persistenceCapability.persist().then(function (result) {
+                this.transactionManager.addToTransaction(
-                    self.persistPending = false;
+                    this.domainObject.getId(),
-                    return result;
+                    wrappedPersistence.persist.bind(wrappedPersistence),
-                });
+                    wrappedPersistence.refresh.bind(wrappedPersistence)
-            }
+                );
-
-            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;
-                }
                 //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();
         };
 

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

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

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(
+                action = new SaveAsAction(undefined, undefined, mockDialogService, undefined, mockCopyService, actionContext);
-                    undefined,
-                    undefined,
-                    mockDialogService,
-                    undefined,
-                    mockCopyService,
-                    mockTransactionService,
-                    actionContext
-                );
 
                 spyOn(action, "getObjectService");
                 action.getObjectService.andReturn(mockObjectService);
@@ -195,10 +179,16 @@ define(
             });
 
             it("hides the blocking dialog after saving", function () {
-                action.perform();
+                var mockCallback = jasmine.createSpy();
+                action.perform().then(mockCallback);
                 expect(mockDialogService.showBlockingMessage).toHaveBeenCalled();
+                waitsFor(function () {
+                    return mockCallback.calls.length > 0;
+                });
+                runs(function () {
                     expect(mockDialogService.dismiss).toHaveBeenCalled();
                 });
+            });
 
         });
     }

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", "getId"]
-                        "getModel"
-                    ]
                 );
                 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();
+                expect(mockTransactionManager.addToTransaction).toHaveBeenCalled();
-                mockTransactionService.addToTransaction.mostRecentCall.args[0]();
+                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 () {
+            it("wraps getSpace", function () {
-                mockTransactionService.isActive.andReturn(true);
+                mockPersistence.getSpace.andReturn('foo');
-                capability.persist();
+                expect(capability.getSpace()).toEqual('foo');
-                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("persist call is only added to transaction once", function () {
+            it("clears transactions and delegates refresh calls", function () {
-                mockTransactionService.isActive.andReturn(true);
+                capability.refresh();
-                capability.persist();
+                expect(mockTransactionManager.clearTransactionsFor)
-                expect(mockTransactionService.addToTransaction).toHaveBeenCalled();
+                    .toHaveBeenCalledWith(testId);
-                capability.persist();
+                expect(mockPersistence.refresh)
-                expect(mockTransactionService.addToTransaction.calls.length).toBe(1);
+                    .toHaveBeenCalled();
-
             });
 
         });

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

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

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