Merge pull request #97 from mozilla-services/bug-96

Fixes #96 - Sync flow should stop when pushing changes fails.
Kinto · Jul 28, 2015 · 285119e · 285119e
2 parents df5e77c + dec3c66
commit 285119e
Show file tree

Hide file tree

Showing 3 changed files with 46 additions and 29 deletions.
diff --git a/src/collection.js b/src/collection.js
@@ -452,12 +452,14 @@ export default class Collection {
       .then(lastModified => this._lastModified = lastModified)
       .then(_ => this.pullChanges(result, options))
       .then(result => {
-        if (!result.ok) {
+        if (!result.ok)
           return result;
-        } else {
-          return this.pushChanges(result, options)
-            .then(result => this.pullChanges(result, options));
-        }
+        return this.pushChanges(result, options)
+          .then(result => {
+            if (!result.ok)
+              return result;
+            return this.pullChanges(result, options)
+          });
       });
   }
 }
diff --git a/test/adapters/base_test.js b/test/adapters/base_test.js
@@ -13,6 +13,7 @@ describe("adapters.BaseAdapter", () => {
     expect(() => adapter.clear()).to.Throw(Error, "Not Implemented.");
     expect(() => adapter.create()).to.Throw(Error, "Not Implemented.");
     expect(() => adapter.update()).to.Throw(Error, "Not Implemented.");
+    expect(() => adapter.delete()).to.Throw(Error, "Not Implemented.");
     expect(() => adapter.get()).to.Throw(Error, "Not Implemented.");
     expect(() => adapter.list()).to.Throw(Error, "Not Implemented.");
     expect(() => adapter.saveLastModified()).to.Throw(Error, "Not Implemented.");

diff --git a/test/collection_test.js b/test/collection_test.js
@@ -884,35 +884,49 @@ describe("Collection", () => {
         .should.eventually.become(result);
     });
 
-    it("should transfer the headers option", () => {
-      var pullChanges = sandbox.stub(articles, "pullChanges").returns(Promise.resolve({}));
-      return articles.sync({headers: {Foo: "Bar"}})
-        .then(() => {
-          expect(pullChanges.firstCall.args[1]).eql({headers: {Foo: "Bar"}});
-        })
+    it("should not execute a last pull on push failure", () => {
+      const pullResult = new SyncResultObject();
+      const pushResult = new SyncResultObject();
+      pushResult.add("conflicts", [1]);
+      sandbox.stub(articles, "pullChanges").returns(Promise.resolve(pullResult));
+      sandbox.stub(articles, "pushChanges").returns(Promise.resolve(pushResult));
+      return articles.sync()
+        .should.eventually.become(pushResult);
     });
 
-    it("should transfer the strategy option", () => {
-      var pullChanges = sandbox.stub(articles, "pullChanges").returns(Promise.resolve({}));
-      return articles.sync({strategy: Collection.strategy.SERVER_WINS})
-        .then(() => {
-          expect(pullChanges.firstCall.args[1]).eql({strategy: Collection.strategy.SERVER_WINS});
-        })
-    });
+    describe("Options", () => {
+      it("should transfer the headers option", () => {
+        var pullChanges = sandbox.stub(articles, "pullChanges").returns(Promise.resolve({}));
+        return articles.sync({headers: {Foo: "Bar"}})
+          .then(() => {
+            expect(pullChanges.firstCall.args[1]).eql({headers: {Foo: "Bar"}});
+          })
+      });
 
-    it("should reject on server backoff by default", () => {
-      articles.api = {backoff: 30000};
-      return articles.sync()
-        .should.be.rejectedWith(Error, /Server is backed off; retry in 30s/);
+      it("should transfer the strategy option", () => {
+        var pullChanges = sandbox.stub(articles, "pullChanges").returns(Promise.resolve({}));
+        return articles.sync({strategy: Collection.strategy.SERVER_WINS})
+          .then(() => {
+            expect(pullChanges.firstCall.args[1]).eql({strategy: Collection.strategy.SERVER_WINS});
+          })
+      });
     });
 
-    it("should perform sync on server backoff when ignoreBackoff is true", () => {
-      sandbox.stub(articles.db, "getLastModified").returns(Promise.resolve({}));
-      sandbox.stub(articles, "pullChanges").returns(Promise.resolve({}));
-      sandbox.stub(articles, "pushChanges").returns(Promise.resolve({}));
-      articles.api = {backoff: 30};
-      return articles.sync({ignoreBackoff: true})
-        .then(_ => sinon.assert.calledOnce(articles.db.getLastModified));
+    describe("Server backoff", () => {
+      it("should reject on server backoff by default", () => {
+        articles.api = {backoff: 30000};
+        return articles.sync()
+          .should.be.rejectedWith(Error, /Server is backed off; retry in 30s/);
+      });
+
+      it("should perform sync on server backoff when ignoreBackoff is true", () => {
+        sandbox.stub(articles.db, "getLastModified").returns(Promise.resolve({}));
+        sandbox.stub(articles, "pullChanges").returns(Promise.resolve({}));
+        sandbox.stub(articles, "pushChanges").returns(Promise.resolve({}));
+        articles.api = {backoff: 30};
+        return articles.sync({ignoreBackoff: true})
+          .then(_ => sinon.assert.calledOnce(articles.db.getLastModified));
+      });
     });
   });
 });