From dc1221b8ba614fb16d4c76a2d170b133351af863 Mon Sep 17 00:00:00 2001 From: Bryan Housel Date: Wed, 10 Dec 2014 00:12:32 -0500 Subject: [PATCH] pass graphs instead of entities to iD.actions.MergeRemoteChanges (realized that I will need to compare more stuff from the local and remote graphs in order to merge ways/relations) --- js/id/actions/merge_remote_changes.js | 12 +- js/id/modes/save.js | 13 +- test/spec/actions/merge_remote_changes.js | 229 +++++++++++++--------- 3 files changed, 152 insertions(+), 102 deletions(-) diff --git a/js/id/actions/merge_remote_changes.js b/js/id/actions/merge_remote_changes.js index 5eb7eba91..509d8187c 100644 --- a/js/id/actions/merge_remote_changes.js +++ b/js/id/actions/merge_remote_changes.js @@ -1,11 +1,10 @@ -iD.actions.MergeRemoteChanges = function(base, local, remote) { - var option = 'safe', // 'safe', 'force_local', 'force_remote' +iD.actions.MergeRemoteChanges = function(id, localGraph, remoteGraph) { + var base = localGraph.base().entities[id], + local = localGraph.entities[id], + remote = remoteGraph.entities[id], + option = 'safe', // 'safe', 'force_local', 'force_remote' target; - function assertIds() { - return (base.id === local.id) && (base.id === remote.id); - } - function sameLocation() { var epsilon = 1e-6; return (Math.abs(remote.loc[0] - local.loc[0]) < epsilon) && @@ -46,7 +45,6 @@ iD.actions.MergeRemoteChanges = function(base, local, remote) { } var action = function(graph) { - if (!assertIds()) { return graph; } target = iD.Entity(local, {version: remote.version}); if (option === 'force_remote') { return graph.replace(remote); } diff --git a/js/id/modes/save.js b/js/id/modes/save.js index 4a2e7a89a..185b7790d 100644 --- a/js/id/modes/save.js +++ b/js/id/modes/save.js @@ -10,7 +10,6 @@ iD.modes.Save = function(context) { function save(e) { var loading = iD.ui.Loading(context).message(t('save.uploading')).blocking(true), history = context.history(), - altGraph = iD.Graph(history.base(), true), toCheck = _.pluck(history.changes().modified, 'id'), errors = []; @@ -18,7 +17,7 @@ iD.modes.Save = function(context) { .call(loading); // check for version conflicts.. reload modified entities into an alternate graph. - context.altGraph(altGraph); + context.altGraph(iD.Graph(history.base(), true)); _.each(toCheck, check); function check(id) { @@ -29,13 +28,13 @@ iD.modes.Save = function(context) { errors.push(err.responseText); } else { - var base = history.base().entity(id), - local = context.graph().entity(id), - remote = context.altGraph().entity(id), - diff; + var graph = context.graph(), + altGraph = context.altGraph(), + local = graph.entity(id), + remote = altGraph.entity(id); if (local.version !== remote.version) { - diff = history.perform(iD.actions.MergeRemoteChanges(base, local, remote)); + var diff = history.perform(iD.actions.MergeRemoteChanges(id, graph, altGraph)); if (!diff.length) { errors.push('Version mismatch for ' + id + ': local=' + local.version + ', remote=' + remote.version); } diff --git a/test/spec/actions/merge_remote_changes.js b/test/spec/actions/merge_remote_changes.js index 7345254f2..68991d913 100644 --- a/test/spec/actions/merge_remote_changes.js +++ b/test/spec/actions/merge_remote_changes.js @@ -1,45 +1,75 @@ describe("iD.actions.MergeRemoteChanges", function () { + var base = iD.Graph([ + iD.Node({id: 'a', loc: [1, 1], version: '1', tags: {foo: 'foo'}}), + + iD.Node({id: 'p1', loc: [ 10, 10], version: '1', tags: {foo: 'foo'}}), + iD.Node({id: 'p2', loc: [ 10, -10], version: '1', tags: {foo: 'foo'}}), + iD.Node({id: 'p3', loc: [-10, -10], version: '1', tags: {foo: 'foo'}}), + iD.Node({id: 'p4', loc: [-10, 10], version: '1', tags: {foo: 'foo'}}), + iD.Way({ + id: 'w1', + nodes: ['p1', 'p2', 'p3', 'p4', 'p1'], + version: '1', + tags: {foo: 'foo', area: 'yes'} + }), + + iD.Node({id: 'q1', loc: [ 5, 5], version: '1', tags: {foo: 'foo'}}), + iD.Node({id: 'q2', loc: [ 5, -5], version: '1', tags: {foo: 'foo'}}), + iD.Node({id: 'q3', loc: [-5, -5], version: '1', tags: {foo: 'foo'}}), + iD.Node({id: 'q4', loc: [-5, 5], version: '1', tags: {foo: 'foo'}}), + iD.Way({ + id: 'w2', + nodes: ['q1', 'q2', 'q3', 'q4', 'q1'], + version: '1', + tags: {foo: 'foo', area: 'yes'} + }), + + iD.Relation({ + id: 'r', + members: [{id: 'w1', role: 'outer'}, {id: 'w2', role: 'inner'}], + version: '1', + tags: {type: 'multipolygon', foo: 'foo'} + }), + + ]); + + describe("non-destuctive merging", function () { it("doesn't merge nodes if location is different", function () { - var base = iD.Node({id: 'a', loc: [1, 1], version: '1', tags: {foo: 'foo'}}), - local = iD.Node({id: 'a', loc: [1, 1], version: '1', v: 2, tags: {foo: 'foo_v2'}}), - remote = iD.Node({id: 'a', loc: [3, 3], version: '2', tags: {bar: 'bar'}}), - graph = iD.Graph([local]), - action = iD.actions.MergeRemoteChanges(base, local, remote); + var local = iD.Node({id: 'a', loc: [1, 1], version: '1', v: 2, tags: {foo: 'foo_local'}}), + remote = iD.Node({id: 'a', loc: [3, 3], version: '2', tags: {foo: 'foo', bar: 'bar_remote'}}), + graph = iD.Graph(base).replace(local), + altGraph = iD.Graph(base).replace(remote), + action = iD.actions.MergeRemoteChanges('a', graph, altGraph); graph = action(graph); - expect(graph.entity('a').loc).to.eql([1, 1]); - expect(graph.entity('a').version).to.eql('1'); - expect(graph.entity('a').tags).to.eql({foo: 'foo_v2'}); + expect(graph.entity('a')).to.eql(local); }); it("doesn't merge nodes if changed tags conflict", function () { - var base = iD.Node({id: 'a', loc: [1, 1], version: '1', tags: {foo: 'foo'}}), - local = iD.Node({id: 'a', loc: [1, 1], version: '1', v: 2, tags: {foo: 'foo_v2'}}), - remote = iD.Node({id: 'a', loc: [1, 1], version: '2', tags: {foo: 'bar'}}), - graph = iD.Graph([local]), - action = iD.actions.MergeRemoteChanges(base, local, remote); + var local = iD.Node({id: 'a', loc: [1, 1], version: '1', v: 2, tags: {foo: 'foo_local'}}), + remote = iD.Node({id: 'a', loc: [1, 1], version: '2', tags: {foo: 'foo_remote', bar: 'bar_remote'}}), + graph = iD.Graph(base).replace(local), + altGraph = iD.Graph(base).replace(remote), + action = iD.actions.MergeRemoteChanges('a', graph, altGraph); graph = action(graph); - expect(graph.entity('a').loc).to.eql([1, 1]); - expect(graph.entity('a').version).to.eql('1'); - expect(graph.entity('a').tags).to.eql({foo: 'foo_v2'}); + expect(graph.entity('a')).to.eql(local); }); it("does merge nodes if location is same and changed tags don't conflict", function () { - var base = iD.Node({id: 'a', loc: [1, 1], version: '1', tags: {foo: 'foo'}}), - local = iD.Node({id: 'a', loc: [1, 1], version: '1', v: 2, tags: {foo: 'foo_v2'}}), - remote = iD.Node({id: 'a', loc: [1, 1], version: '2', tags: {foo: 'foo', bar: 'bar'}}), - graph = iD.Graph([local]), - action = iD.actions.MergeRemoteChanges(base, local, remote); + var local = iD.Node({id: 'a', loc: [1, 1], version: '1', v: 2, tags: {foo: 'foo_local'}}), + remote = iD.Node({id: 'a', loc: [1, 1], version: '2', tags: {foo: 'foo', bar: 'bar_remote'}}), + graph = iD.Graph(base).replace(local), + altGraph = iD.Graph(base).replace(remote), + action = iD.actions.MergeRemoteChanges('a', graph, altGraph); graph = action(graph); - expect(graph.entity('a').loc).to.eql([1, 1]); expect(graph.entity('a').version).to.eql('2'); - expect(graph.entity('a').tags).to.eql({foo: 'foo_v2', bar: 'bar'}); + expect(graph.entity('a').tags).to.eql({foo: 'foo_local', bar: 'bar_remote'}); }); // test merging ways @@ -50,108 +80,131 @@ describe("iD.actions.MergeRemoteChanges", function () { describe("destuctive merging", function () { it("merges nodes with 'force_local' option", function () { - var base = iD.Node({id: 'a', loc: [1, 1], version: '1', tags: {foo: 'foo'}}), - local = iD.Node({id: 'a', loc: [2, 2], version: '1', v: 2, tags: {foo: 'foo_v2'}}), - remote = iD.Node({id: 'a', loc: [3, 3], version: '2', tags: {foo: 'bar'}}), - graph = iD.Graph([local]), - action = iD.actions.MergeRemoteChanges(base, local, remote).withOption('force_local'); + var localTags = {foo: 'foo_local'}, // changed tag foo + remoteTags = {foo: 'foo_remote'}, // changed tag foo + local = iD.Node({id: 'a', loc: [2, 2], version: '1', v: 2, tags: localTags}), + remote = iD.Node({id: 'a', loc: [3, 3], version: '2', tags: remoteTags}), + graph = iD.Graph(base).replace(local), + altGraph = iD.Graph(base).replace(remote), + action = iD.actions.MergeRemoteChanges('a', graph, altGraph).withOption('force_local'); graph = action(graph); - expect(graph.entity('a').loc).to.eql([2, 2]); expect(graph.entity('a').version).to.eql('2'); - expect(graph.entity('a').tags).to.eql({foo: 'foo_v2'}); + expect(graph.entity('a').loc).to.eql([2, 2]); + expect(graph.entity('a').tags).to.eql(localTags); }); it("merges nodes with 'force_remote' option", function () { - var base = iD.Node({id: 'a', loc: [1, 1], version: '1', tags: {foo: 'foo'}}), - local = iD.Node({id: 'a', loc: [2, 2], version: '1', v: 2, tags: {foo: 'foo_v2'}}), - remote = iD.Node({id: 'a', loc: [3, 3], version: '2', tags: {foo: 'bar'}}), - graph = iD.Graph([local]), - action = iD.actions.MergeRemoteChanges(base, local, remote).withOption('force_remote'); + var localTags = {foo: 'foo_local'}, // changed tag foo + remoteTags = {foo: 'foo_remote'}, // changed tag foo + local = iD.Node({id: 'a', loc: [2, 2], version: '1', v: 2, tags: localTags}), + remote = iD.Node({id: 'a', loc: [3, 3], version: '2', tags: remoteTags}), + graph = iD.Graph(base).replace(local), + altGraph = iD.Graph(base).replace(remote), + action = iD.actions.MergeRemoteChanges('a', graph, altGraph).withOption('force_remote'); graph = action(graph); - expect(graph.entity('a').loc).to.eql([3, 3]); expect(graph.entity('a').version).to.eql('2'); - expect(graph.entity('a').tags).to.eql({foo: 'bar'}); + expect(graph.entity('a').loc).to.eql([3, 3]); + expect(graph.entity('a').tags).to.eql(remoteTags); }); it("merges ways with 'force_local' option", function () { - var a = iD.Node({id: 'a'}), - b = iD.Node({id: 'b'}), - c = iD.Node({id: 'c'}), - d = iD.Node({id: 'd'}), - e = iD.Node({id: 'e'}), - f = iD.Node({id: 'f'}), - base = iD.Way({id: 'w', nodes: ['a', 'b'], version: '1', tags: {foo: 'foo'}}), - local = iD.Way({id: 'w', nodes: ['c', 'd'], version: '1', v: 2, tags: {foo: 'foo_v2'}}), - remote = iD.Way({id: 'w', nodes: ['e', 'f'], version: '2', tags: {foo: 'bar'}}), - graph = iD.Graph([c, d, local]), - action = iD.actions.MergeRemoteChanges(base, local, remote).withOption('force_local'); + var x = iD.Node({id: 'x', loc: [5, 0], tags: {foo: 'foo_local'}}), + y = iD.Node({id: 'y', loc: [-5, 0], version: '2', tags: {foo: 'foo_remote'}}), + localNodes = ['p1', 'x', 'p2', 'p3', 'p4', 'p1'], // inserted node x + remoteNodes = ['p1', 'p2', 'p3', 'y', 'p4', 'p1'], // inserted node y + localTags = {foo: 'foo_local', area: 'yes'}, // changed tag foo + remoteTags = {foo: 'foo_remote', area: 'yes'}, // changed tag foo + local = iD.Way({id: 'w1', nodes: localNodes, version: '1', v: 2, tags: localTags}), + remote = iD.Way({id: 'w1', nodes: remoteNodes, version: '2', tags: remoteTags}), + graph = iD.Graph(base).replace(x).replace(local), + altGraph = iD.Graph(base).replace(y).replace(remote), + action = iD.actions.MergeRemoteChanges('w1', graph, altGraph).withOption('force_local'); graph = action(graph); - expect(graph.entity('w').nodes).to.eql(['c', 'd']); - expect(graph.entity('w').version).to.eql('2'); - expect(graph.entity('w').tags).to.eql({foo: 'foo_v2'}); + expect(graph.entity('w1').version).to.eql('2'); + // expect(graph.hasEntity('x')).to.be.true; + // expect(graph.hasEntity('y')).to.be.false; + expect(graph.entity('w1').nodes).to.eql(localNodes); + expect(graph.entity('w1').tags).to.eql(localTags); }); it("merges ways with 'force_remote' option", function () { - var a = iD.Node({id: 'a'}), - b = iD.Node({id: 'b'}), - c = iD.Node({id: 'c'}), - d = iD.Node({id: 'd'}), - e = iD.Node({id: 'e'}), - f = iD.Node({id: 'f'}), - base = iD.Way({id: 'w', nodes: ['a', 'b'], version: '1', tags: {foo: 'foo'}}), - local = iD.Way({id: 'w', nodes: ['c', 'd'], version: '1', v: 2, tags: {foo: 'foo_v2'}}), - remote = iD.Way({id: 'w', nodes: ['e', 'f'], version: '2', tags: {foo: 'bar'}}), - graph = iD.Graph([c, d, local]), - action = iD.actions.MergeRemoteChanges(base, local, remote).withOption('force_remote'); + var x = iD.Node({id: 'x', loc: [5, 0], tags: {foo: 'foo_local'}}), + y = iD.Node({id: 'y', loc: [-5, 0], version: '2', tags: {foo: 'foo_remote'}}), + localNodes = ['p1', 'x', 'p2', 'p3', 'p4', 'p1'], // inserted node x + remoteNodes = ['p1', 'p2', 'p3', 'y', 'p4', 'p1'], // inserted node y + localTags = {foo: 'foo_local', area: 'yes'}, // changed tag foo + remoteTags = {foo: 'foo_remote', area: 'yes'}, // changed tag foo + local = iD.Way({id: 'w1', nodes: localNodes, version: '1', v: 2, tags: localTags}), + remote = iD.Way({id: 'w1', nodes: remoteNodes, version: '2', tags: remoteTags}), + graph = iD.Graph(base).replace(x).replace(local), + altGraph = iD.Graph(base).replace(y).replace(remote), + action = iD.actions.MergeRemoteChanges('w1', graph, altGraph).withOption('force_remote'); graph = action(graph); - // expect(graph.hasEntity('e')).to.be.true; - // expect(graph.hasEntity('f')).to.be.true; - expect(graph.entity('w').nodes).to.eql(['e', 'f']); - expect(graph.entity('w').version).to.eql('2'); - expect(graph.entity('w').tags).to.eql({foo: 'bar'}); + expect(graph.entity('w1').version).to.eql('2'); + // expect(graph.hasEntity('x')).to.be.true; + // expect(graph.hasEntity('y')).to.be.true; + expect(graph.entity('w1').nodes).to.eql(remoteNodes); + expect(graph.entity('w1').tags).to.eql(remoteTags); }); it("merges relations with 'force_local' option", function () { - var a = iD.Node({id: 'a'}), - b = iD.Node({id: 'b'}), - c = iD.Node({id: 'c'}), - base = iD.Relation({id: 'r', members: [{id: 'a', type: 'node'}], version: '1', tags: {foo: 'foo'}}), - local = iD.Relation({id: 'r', members: [{id: 'b', type: 'node'}], version: '1', v: 2, tags: {foo: 'foo_v2'}}), - remote = iD.Relation({id: 'r', members: [{id: 'c', type: 'node'}], version: '2', tags: {foo: 'bar'}}), - graph = iD.Graph([b, local]), - action = iD.actions.MergeRemoteChanges(base, local, remote).withOption('force_local'); + var localNodes = ['p2', 'p3', 'p4', 'p1', 'p2'], // changed order + remoteNodes = ['p1', 'p4', 'p3', 'p2', 'p1'], // reversed order + localWayTags = {foo: 'foo_local'}, // changed tag foo + remoteWayTags = {foo: 'foo_remote'}, // changed tag foo + x = iD.Way({id: 'x', nodes: localNodes, tags: localWayTags}), + y = iD.Way({id: 'y', nodes: remoteNodes, version: '2', tags: remoteWayTags}), + localMembers = [{id: 'x', role: 'outer'}, {id: 'w2', role: 'inner'}], // changed outer to x + remoteMembers = [{id: 'y', role: 'outer'}, {id: 'w2', role: 'inner'}], // changed outer to y + localRelTags = {type: 'multipolygon', foo: 'foo_local'}, // changed tag foo + remoteRelTags = {type: 'multipolygon', foo: 'foo_remote'}, // changed tag foo + local = iD.Relation({id: 'r', members: localMembers, version: '1', v: 2, tags: localRelTags}), + remote = iD.Relation({id: 'r', members: remoteMembers, version: '2', tags: remoteRelTags}), + graph = iD.Graph(base).replace(x).replace(local), + altGraph = iD.Graph(base).replace(y).replace(remote), + action = iD.actions.MergeRemoteChanges('r', graph, altGraph).withOption('force_local'); graph = action(graph); - expect(graph.entity('r').members).to.eql([{id: 'b', type: 'node'}]); expect(graph.entity('r').version).to.eql('2'); - expect(graph.entity('r').tags).to.eql({foo: 'foo_v2'}); + // expect(graph.hasEntity('x')).to.be.true; + // expect(graph.hasEntity('y')).to.be.false; + expect(graph.entity('r').members).to.eql(localMembers); + expect(graph.entity('r').tags).to.eql(localRelTags); }); it("merges relations with 'force_remote' option", function () { - var a = iD.Node({id: 'a'}), - b = iD.Node({id: 'b'}), - c = iD.Node({id: 'c'}), - base = iD.Relation({id: 'r', members: [{id: 'a', type: 'node'}], version: '1', tags: {foo: 'foo'}}), - local = iD.Relation({id: 'r', members: [{id: 'b', type: 'node'}], version: '1', v: 2, tags: {foo: 'foo_v2'}}), - remote = iD.Relation({id: 'r', members: [{id: 'c', type: 'node'}], version: '2', tags: {foo: 'bar'}}), - graph = iD.Graph([b, local]), - action = iD.actions.MergeRemoteChanges(base, local, remote).withOption('force_remote'); + var localNodes = ['p2', 'p3', 'p4', 'p1', 'p2'], // changed order + remoteNodes = ['p1', 'p4', 'p3', 'p2', 'p1'], // reversed + localWayTags = {foo: 'foo_local'}, // changed tag foo + remoteWayTags = {foo: 'foo_remote'}, // changed tag foo + x = iD.Way({id: 'x', nodes: localNodes, tags: localWayTags}), + y = iD.Way({id: 'y', nodes: remoteNodes, version: '2', tags: remoteWayTags}), + localMembers = [{id: 'x', role: 'outer'}, {id: 'w2', role: 'inner'}], // changed outer to x + remoteMembers = [{id: 'y', role: 'outer'}, {id: 'w2', role: 'inner'}], // changed outer to y + localRelTags = {type: 'multipolygon', foo: 'foo_local'}, // changed tag foo + remoteRelTags = {type: 'multipolygon', foo: 'foo_remote'}, // changed tag foo + local = iD.Relation({id: 'r', members: localMembers, version: '1', v: 2, tags: localRelTags}), + remote = iD.Relation({id: 'r', members: remoteMembers, version: '2', tags: remoteRelTags}), + graph = iD.Graph(base).replace(x).replace(local), + altGraph = iD.Graph(base).replace(y).replace(remote), + action = iD.actions.MergeRemoteChanges('r', graph, altGraph).withOption('force_remote'); graph = action(graph); - // expect(graph.hasEntity('c')).to.be.true; - expect(graph.entity('r').members).to.eql([{id: 'c', type: 'node'}]); expect(graph.entity('r').version).to.eql('2'); - expect(graph.entity('r').tags).to.eql({foo: 'bar'}); + // expect(graph.hasEntity('x')).to.be.true; + // expect(graph.hasEntity('y')).to.be.true; + expect(graph.entity('r').members).to.eql(remoteMembers); + expect(graph.entity('r').tags).to.eql(remoteRelTags); }); });