Bring across fixes for #1468 and #2277 into TCR (#2386)

platform/commonUI/edit/src/services/TransactionManager.js

@@ -77,14 +77,19 @@ define([], function () {
                 return promiseFn().then(nextFn);
             };
         }
+        /**
+         * Clear any existing persistence calls for object with given ID. This ensures only the most recent persistence
+         * call is executed. This should prevent stale objects being persisted and overwriting fresh ones.
+         */
+        if (this.isScheduled(id)) {
+            this.clearTransactionsFor(id);
+        }
 
-        if (!this.isScheduled(id)) {
         this.clearTransactionFns[id] =
             this.transactionService.addToTransaction(
                 chain(onCommit, release),
                 chain(onCancel, release)
             );
-        }
     };
 
     /**

platform/commonUI/edit/test/services/TransactionManagerSpec.js

@@ -93,24 +93,33 @@ define(
                     expect(mockOnCancel).toHaveBeenCalled();
                 });
 
-                it("ignores subsequent calls for the same object", function () {
+                describe("Adds callbacks to transaction", function () {
+                    beforeEach(function () {
+                        spyOn(manager, 'clearTransactionsFor');
+                        manager.clearTransactionsFor.and.callThrough();
+                    });
+
+                    it("and clears pending calls if same object", function () {
                         manager.addToTransaction(
                             testId,
                             jasmine.createSpy(),
                             jasmine.createSpy()
                         );
-                    expect(mockTransactionService.addToTransaction.calls.count())
+                        expect(manager.clearTransactionsFor).toHaveBeenCalledWith(testId);
-                        .toEqual(1);
                     });
 
-                it("accepts subsequent calls for other objects", function () {
+                    it("and does not clear pending calls if different object", function () {
                         manager.addToTransaction(
                             'other-id',
                             jasmine.createSpy(),
                             jasmine.createSpy()
                         );
-                    expect(mockTransactionService.addToTransaction.calls.count())
+                        expect(manager.clearTransactionsFor).not.toHaveBeenCalled();
-                        .toEqual(2);
+                    });
+
+                    afterEach(function () {
+                        expect(mockTransactionService.addToTransaction.calls.count()).toEqual(2);
+                    });
                 });
 
                 it("does not remove callbacks from the transaction", function () {

platform/core/src/runs/TransactingMutationListener.js

@@ -43,23 +43,10 @@ define([], function () {
         var mutationTopic = topic('mutation');
         mutationTopic.listen(function (domainObject) {
             var persistence = domainObject.getCapability('persistence');
-            var wasActive = transactionService.isActive();
             cacheService.put(domainObject.getId(), domainObject.getModel());
 
             if (hasChanged(domainObject)) {
-
+                persistence.persist();
-                if (!wasActive) {
-                    transactionService.startTransaction();
-                }
-
-                transactionService.addToTransaction(
-                    persistence.persist.bind(persistence),
-                    persistence.refresh.bind(persistence)
-                );
-
-                if (!wasActive) {
-                    transactionService.commit();
-                }
             }
         });
     }

platform/core/test/runs/TransactingMutationListenerSpec.js

@@ -24,22 +24,27 @@ define(
     ["../../src/runs/TransactingMutationListener"],
     function (TransactingMutationListener) {
 
-        xdescribe("TransactingMutationListener", function () {
+        describe("TransactingMutationListener", function () {
             var mockTopic,
                 mockMutationTopic,
+                mockCacheService,
                 mockTransactionService,
                 mockDomainObject,
+                mockModel,
                 mockPersistence;
 
             beforeEach(function () {
                 mockTopic = jasmine.createSpy('topic');
                 mockMutationTopic =
                     jasmine.createSpyObj('mutation', ['listen']);
+                mockCacheService =
+                    jasmine.createSpyObj('cacheService', [
+                        'put'
+                    ]);
                 mockTransactionService =
                     jasmine.createSpyObj('transactionService', [
                         'isActive',
                         'startTransaction',
-                        'addToTransaction',
                         'commit'
                     ]);
                 mockDomainObject = jasmine.createSpyObj(
@@ -52,18 +57,24 @@ define(
                 );
 
                 mockTopic.and.callFake(function (t) {
-                    return (t === 'mutation') && mockMutationTopic;
+                    expect(t).toBe('mutation');
+                    return mockMutationTopic;
                 });
 
+                mockDomainObject.getId.and.returnValue('mockId');
                 mockDomainObject.getCapability.and.callFake(function (c) {
-                    return (c === 'persistence') && mockPersistence;
+                    expect(c).toBe('persistence');
+                    return mockPersistence;
                 });
+                mockModel = {};
+                mockDomainObject.getModel.and.returnValue(mockModel);
 
                 mockPersistence.persisted.and.returnValue(true);
 
                 return new TransactingMutationListener(
                     mockTopic,
-                    mockTransactionService
+                    mockTransactionService,
+                    mockCacheService
                 );
             });
 
@@ -72,48 +83,27 @@ define(
                     .toHaveBeenCalledWith(jasmine.any(Function));
             });
 
-            [false, true].forEach(function (isActive) {
+            it("calls persist if the model has changed", function () {
-                var verb = isActive ? "is" : "isn't";
+                mockModel.persisted = Date.now();
 
-                function onlyWhenInactive(expectation) {
+                //Mark the model dirty by setting the mutated date later than the last persisted date.
-                    return isActive ? expectation.not : expectation;
+                mockModel.modified = mockModel.persisted + 1;
-                }
 
-                describe("when a transaction " + verb + " active", function () {
-                    var innerVerb = isActive ? "does" : "doesn't";
-
-                    beforeEach(function () {
-                        mockTransactionService.isActive.and.returnValue(isActive);
-                    });
-
-                    describe("and mutation occurs", function () {
-                        beforeEach(function () {
                 mockMutationTopic.listen.calls.mostRecent()
                     .args[0](mockDomainObject);
+
+                expect(mockPersistence.persist).toHaveBeenCalled();
             });
 
+            it("does not call persist if the model has not changed", function () {
+                mockModel.persisted = Date.now();
 
-                        it(innerVerb + " start a new transaction", function () {
+                mockModel.modified = mockModel.persisted;
-                            onlyWhenInactive(
-                                expect(mockTransactionService.startTransaction)
-                            ).toHaveBeenCalled();
-                        });
 
-                        it("adds to the active transaction", function () {
+                mockMutationTopic.listen.calls.mostRecent()
-                            expect(mockTransactionService.addToTransaction)
+                    .args[0](mockDomainObject);
-                                .toHaveBeenCalledWith(
-                                    jasmine.any(Function),
-                                    jasmine.any(Function)
-                                );
-                        });
 
-                        it(innerVerb + " immediately commit", function () {
+                expect(mockPersistence.persist).not.toHaveBeenCalled();
-                            onlyWhenInactive(
-                                expect(mockTransactionService.commit)
-                            ).toHaveBeenCalled();
-                        });
-                    });
-                });
             });
         });
     }