Merge pull request #161 from Kinto/sync-result-updated-list

Refs #160: Sync flow and result object format optimizations.

docs/api.md

@@ -274,7 +274,7 @@ For publication conflicts, the `sync()` method accepts a `strategy` option, whic
 > `strategy` only applies to *outgoing* conflicts. *Incoming* conflicts will still
 > be reported in the `conflicts` array. See [`resolving conflicts section`](#resolving-conflicts-manually).
 
-You can override default options by passing `#sync()` a new `options` object; Kinto will merge these new values with the default ones:
+You can override default options by passing `#sync()` a new `options` object; Kinto.js will merge these new values with the default ones:
 
 ```js
 import Collection from "kinto/lib/collection";
@@ -291,24 +291,40 @@ articles.sync({
   });
 ```
 
-The synchronization updates the local data, and provides information about performed operations.
-Sample result:
+## The synchronization result object
+
+When the `#sync()` promise is fulfilled, a result object is returned, providing information about the performed operations.
+
+Here's a sample result object:
 
 ```js
 {
   ok: true,
   lastModified: 1434270764485,
-  conflicts: [], // Outgoing and incoming conflicts
-  errors:    [], // Errors encountered, if any
-  created:   [], // Created locally
-  updated:   [], // Updated locally
-  deleted:   [], // Deleted locally
-  skipped:   [], // Skipped imports
-  published: [], // Successfully published
-  resolved:  [], // Resolved conflicts, according to selected strategy
+  conflicts: [],
+  errors:    [],
+  created:   [],
+  updated:   [],
+  deleted:   [],
+  skipped:   [],
+  published: [],
+  resolved:  [],
 }
 ```
 
+The synchronization result object exposes the following properties:
+
+- `ok`: The boolean status of the synchronization operation; `true` if no unresolved conflicts and no errors were encountered.
+- `lastModified`: The timestamp of the latest known successful synchronization operation (no error and no conflict encountered).
+- `conflicts`: The list of unresolved conflicts encountered during both import and export operations (see *[Resolving conflicts manually](#resolving-conflicts-manually)*);
+- `errors`:    The list of encountered errors, if any.
+- `created`:   The list of remote records which have been successfully imported into the local database.
+- `updated`:   The list of remote record updates which have been successfully reflected into the local database.
+- `deleted`:   The list of remotely deleted records which have been successfully deleted as well locally.
+- `skipped`:   The list of remotely deleted records missing locally.
+- `published`: The list of locally modified records (created, updated, or deleted) which have been successfully pushed to the remote server.
+- `resolved`:  The list of conflicting records which have been successfully resolved according to the selected [strategy](#synchronization-strategies) (note that when using the default `MANUAL` strategy, this list is always empty).
+
 ## Resolving conflicts manually
 
 When using `Kinto.syncStrategy.MANUAL`, if conflicts occur, they're listed in the `conflicts` property; they must be resolved locally and `sync()` called again.

src/collection.js

@@ -304,19 +304,21 @@ export default class Collection {
   }
 
   /**
-   * Attempts to apply a remote change to its local matching record.
+   * Attempts to apply a remote change to its local matching record. Note that
+   * at this point, remote record data are already decoded.
    *
    * @param  {Object} local  The local record object.
    * @param  {Object} remote The remote change object.
    * @return {Promise}
    */
   _processChangeImport(local, remote) {
+    const identical = deepEquals(cleanRecord(local), cleanRecord(remote));
     if (local._status !== "synced") {
       // Locally deleted, unsynced: scheduled for remote deletion.
       if (local._status === "deleted") {
         return {type: "skipped", data: local};
       }
-      if (deepEquals(cleanRecord(local), cleanRecord(remote))) {
+      if (identical) {
         // If records are identical, import anyway, so we bump the
         // local last_modified value from the server and set record
         // status to "synced".
@@ -334,8 +336,10 @@ export default class Collection {
         return {type: "deleted", data: res.data};
       });
     }
-    return this.update(remote, {synced: true}).then(_ => {
-      return {type: "updated", data: local};
+    return this.update(remote, {synced: true}).then(updated => {
+      // if identical, simply exclude it from all lists
+      const type = identical ? "void" : "updated";
+      return {type, data: updated.data};
     });
   }
 
@@ -388,7 +392,9 @@ export default class Collection {
     }))
       .then(imports => {
         for (let imported of imports) {
-          syncResultObject.add(imported.type, imported.data);
+          if (imported.type !== "void") {
+            syncResultObject.add(imported.type, imported.data);
+          }
         }
         return syncResultObject;
       })
@@ -505,8 +511,9 @@ export default class Collection {
               return {data: {id: res.data.id, deleted: true}};
             });
           } else {
-            // Remote update was successful, reflect it locally
-            return this.update(record, {synced: true});
+            // Remote create/update was successful, reflect it locally
+            return this._decodeRecord("remote", record)
+              .then(record => this.update(record, {synced: true}));
           }
         })).then(published => {
           syncResultObject.add("published", published.map(res => res.data));

test/collection_test.js

@@ -689,18 +689,21 @@ describe("Collection", () => {
       const id_4 = uuid4();
       const id_5 = uuid4();
       const id_6 = uuid4();
+      const id_7 = uuid4();
 
       const localData = [
         {id: id_1, title: "art1"},
         {id: id_2, title: "art2"},
         {id: id_4, title: "art4"},
         {id: id_5, title: "art5"},
+        {id: id_7, title: "art7-a"},
       ];
       const serverChanges = [
-        {id: id_2, title: "art2"}, // existing, should simply be marked as synced
+        {id: id_2, title: "art2"}, // existing & untouched, skipped
         {id: id_3, title: "art3"}, // to be created
         {id: id_4, deleted: true}, // to be deleted
-        {id: id_6, deleted: true}, // remotely deleted, missing locally
+        {id: id_6, deleted: true}, // remotely deleted & missing locally, skipped
+        {id: id_7, title: "art7-b"},
       ];
 
       beforeEach(() => {
@@ -714,6 +717,12 @@ describe("Collection", () => {
         }));
       });
 
+      it("should not fetch remote records if result status isn't ok", () => {
+        result.ok = false;
+        return articles.pullChanges(result)
+          .then(_ => sinon.assert.notCalled(fetchChangesSince));
+      });
+
       it("should fetch remote changes from the server", () => {
         return articles.pullChanges(result)
           .then(_ => {
@@ -748,7 +757,7 @@ describe("Collection", () => {
         return articles.pullChanges(result)
           .then(res => res.updated)
           .should.eventually.become([
-            {id: id_2, title: "art2", _status: "synced"}
+            {id: id_7, title: "art7-b", _status: "synced"}
           ]);
       });
 
@@ -775,6 +784,7 @@ describe("Collection", () => {
             {id: id_2, title: "art2", _status: "synced"},
             {id: id_3, title: "art3", _status: "synced"},
             {id: id_5, title: "art5", _status: "synced"},
+            {id: id_7, title: "art7-b", _status: "synced"},
           ]);
       });
 
@@ -785,6 +795,16 @@ describe("Collection", () => {
           .then(res => res.data.title)
           .should.eventually.become("foo");
       });
+
+      it("should not list identical records as skipped", () => {
+        return articles.pullChanges(result)
+          .then(res => res.skipped)
+          .should.eventually.not.contain({
+            id: id_2,
+            title: "art2",
+            _status: "synced"
+          });
+      });
     });
 
     describe("When a conflict occured", () => {