Fixes #150 — Consistent conflicts resolution strategies. #152

n1k0 · 2015-09-08T16:27:33Z

Refs #150 - WiP, do not merge yet.

Fix incoming conflicts resolution according to selected strategy
Fix outgoing conflicts resolution according to selected strategy
Ensure conflicting scenarios are exhaustively covered by integration tests
Expose a new resolved property to the sync result object
Update documentation
Update sync flow diagram
Generate dist files

leplatrem · 2015-09-09T15:08:09Z

test/transformers/remote_test.js

+    expect(() => transformer.encode()).to.Throw(Error, "Not implemented.");
+    expect(() => transformer.decode()).to.Throw(Error, "Not implemented.");
+  });
+});


choucroute?

cassoulet !

choucroute?

cassoulet !

Both are fine, though this was for improving code coverage only :)

n1k0 · 2015-09-11T08:08:43Z

r=? @Natim

Natim · 2015-09-11T08:45:17Z

test/integration_test.js

+            });
+          });
+
+          it("should not have an ok status", () => {


should have an ok status ?

n1k0 · 2015-09-14T06:52:41Z

Then what happens in case options.resolved is set and there is another conflict?

Well, we've just called _handleConflicts in the previous promise step, so it's very unlikely (read impossible) that conflicts remain when checking for this option with an automatic resolution strategy set.

leplatrem · 2015-09-14T08:28:59Z

I would r+

Regarding the tests @Natim mentions, I would say they are covered by functional tests. At first, I also thought there was a need to re-sync and check the server state... But after digging a bit, I realized both strategies lead to a consistent state with the server :) Since this is the result of some code that is already being unit tested, I would say we're fine here!

almet · 2015-09-14T08:59:59Z

docs/api.md


-If conflicts occured, they're listed in the `conflicts` property; they must be resolved locally and `sync()` called again.
+When using `Kinto.syncStrategy.MANUAL` and conflicts occur, they're listed in the `conflicts` property; they must be resolved locally and `sync()` called again.


nit: consider "When using [...], if conflicts occurs, they're [...]".

almet · 2015-09-14T09:03:21Z

Interestingly, on a second read, the code looks clear to me (it wasn't the case the first time I read. The schema helped a lot to bring context, thanks).

The changes looks good to me. I would go for a merge (with my few nits addressed).

Fixes #150 — Consistent conflicts resolution strategies.

Natim · 2015-09-16T13:17:21Z

we've just called _handleConflicts in the previous promise step, so it's very unlikely (read impossible) that conflicts remain when checking for this option with an automatic resolution strategy set.

Well, if client 1 published something and client 2 is fast enough to update that record in between the first and the second call to pullChanges, then it could end up in another conflict.

n1k0 force-pushed the 150-consistent-sync-strategies branch 2 times, most recently from 57a53b1 to 931726d Compare September 9, 2015 14:32

leplatrem reviewed Sep 9, 2015
View reviewed changes

n1k0 changed the title ~~[WiP] Consistent incoming conflicts resolution strategies.~~ Fixes #150 — Consistent conflicts resolution strategies. Sep 9, 2015

n1k0 force-pushed the 150-consistent-sync-strategies branch from 9d8cc09 to 8e9670b Compare September 11, 2015 08:08

Natim reviewed Sep 11, 2015
View reviewed changes

test/integration_test.js

});

});

it("should not have an ok status", () => {

Copy link

Member

Natim Sep 11, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should have an ok status ?

n1k0 force-pushed the 150-consistent-sync-strategies branch from a7d558c to 15a03ff Compare September 14, 2015 06:59

almet reviewed Sep 14, 2015
View reviewed changes

n1k0 force-pushed the 150-consistent-sync-strategies branch 2 times, most recently from 5ce8ffa to 8b73c8e Compare September 14, 2015 09:31

Fixes #150: Consistent conflicts resolution strategies.

741b24e

n1k0 force-pushed the 150-consistent-sync-strategies branch from 8b73c8e to 741b24e Compare September 14, 2015 10:50

n1k0 added a commit that referenced this pull request Sep 14, 2015

Merge pull request #152 from Kinto/150-consistent-sync-strategies

8900071

Fixes #150 — Consistent conflicts resolution strategies.

n1k0 merged commit 8900071 into master Sep 14, 2015

n1k0 deleted the 150-consistent-sync-strategies branch September 14, 2015 11:17

n1k0 mentioned this pull request Sep 29, 2015

Bump 1.0.0-rc.4. #187

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fixes #150 — Consistent conflicts resolution strategies. #152

Fixes #150 — Consistent conflicts resolution strategies. #152

n1k0 commented Sep 8, 2015

leplatrem Sep 9, 2015

almet Sep 9, 2015

n1k0 Sep 9, 2015

n1k0 commented Sep 11, 2015

Natim Sep 11, 2015

n1k0 commented Sep 14, 2015

leplatrem commented Sep 14, 2015

almet Sep 14, 2015

almet commented Sep 14, 2015

Natim commented Sep 16, 2015


		If conflicts occured, they're listed in the `conflicts` property; they must be resolved locally and `sync()` called again.
		When using `Kinto.syncStrategy.MANUAL` and conflicts occur, they're listed in the `conflicts` property; they must be resolved locally and `sync()` called again.

Fixes #150 — Consistent conflicts resolution strategies. #152

Fixes #150 — Consistent conflicts resolution strategies. #152

Conversation

n1k0 commented Sep 8, 2015

leplatrem Sep 9, 2015

Choose a reason for hiding this comment

almet Sep 9, 2015

Choose a reason for hiding this comment

n1k0 Sep 9, 2015

Choose a reason for hiding this comment

n1k0 commented Sep 11, 2015

Natim Sep 11, 2015

Choose a reason for hiding this comment

n1k0 commented Sep 14, 2015

leplatrem commented Sep 14, 2015

almet Sep 14, 2015

Choose a reason for hiding this comment

almet commented Sep 14, 2015

Natim commented Sep 16, 2015