Extract Relation#replaceMember

js/id/actions/connect.js

@@ -30,11 +30,7 @@ iD.actions.Connect = function(nodeIds) {
             });
 
             graph.parentRelations(node).forEach(function (parent) {
-                var memberA = parent.memberById(survivor.id),
-                    memberB = parent.memberById(node.id);
-                if (!memberA) {
-                    graph = graph.replace(parent.addMember({id: survivor.id, role: memberB.role, type: 'node'}));
-                }
+                graph = graph.replace(parent.replaceMember(node, survivor));
             });
 
             survivor = survivor.mergeTags(node.tags);

js/id/actions/join.js

@@ -10,7 +10,7 @@ iD.actions.Join = function(idA, idB) {
     var action = function(graph) {
         var a = graph.entity(idA),
             b = graph.entity(idB),
-            nodes, tags;
+            nodes;
 
         if (a.first() === b.first()) {
             // a <-- b ==> c
@@ -37,14 +37,9 @@ iD.actions.Join = function(idA, idB) {
             nodes = a.nodes.concat(b.nodes.slice().reverse().slice(1));
         }
 
-        graph.parentRelations(b)
-            .forEach(function (parent) {
-                var memberA = parent.memberById(idA),
-                    memberB = parent.memberById(idB);
-                if (!memberA) {
-                    graph = graph.replace(parent.addMember({id: idA, role: memberB.role, type: 'way'}));
-                }
-            });
+        graph.parentRelations(b).forEach(function (parent) {
+            graph = graph.replace(parent.replaceMember(b, a));
+        });
 
         graph = graph.replace(a.mergeTags(b.tags).update({nodes: nodes}));
         graph = iD.actions.DeleteWay(idB)(graph);

js/id/graph/relation.js

@@ -49,6 +49,16 @@ _.extend(iD.Relation.prototype, {
         }
     },
 
+    // Return the first member with the given id and role. A copy of the member object
+    // is returned, extended with an 'index' property whose value is the member index.
+    memberByIdAndRole: function(id, role) {
+        for (var i = 0; i < this.members.length; i++) {
+            if (this.members[i].id === id && this.members[i].role === role) {
+                return _.extend({}, this.members[i], {index: i});
+            }
+        }
+    },
+
     addMember: function(member, index) {
         var members = this.members.slice();
         members.splice(index === undefined ? members.length : index, 0, member);
@@ -66,6 +76,28 @@ _.extend(iD.Relation.prototype, {
         return this.update({members: members});
     },
 
+    // Wherever a member appears with id `needle.id`, replace it with a member
+    // with id `replacement.id`, type `replacement.type`, and the original role,
+    // unless a member already exists with that id and role. Return an updated
+    // relation.
+    replaceMember: function(needle, replacement) {
+        if (!this.memberById(needle.id))
+            return this;
+
+        var members = [];
+
+        for (var i = 0; i < this.members.length; i++) {
+            var member = this.members[i];
+            if (member.id !== needle.id) {
+                members.push(member);
+            } else if (!this.memberByIdAndRole(replacement.id, member.role)) {
+                members.push({id: replacement.id, type: replacement.type, role: member.role});
+            }
+        }
+
+        return this.update({members: members});
+    },
+
     asJXON: function(changeset_id) {
         var r = {
             relation: {

test/spec/actions/connect.js

@@ -99,7 +99,7 @@ describe("iD.actions.Connect", function() {
                 '-': iD.Way({id: '-', nodes: ['a', 'b']}),
                 '=': iD.Way({id: '=', nodes: ['c', 'd']}),
                 'r1': iD.Relation({id: 'r1', members: [{id: 'b', role: 'r1', type: 'node'}]}),
-                'r2': iD.Relation({id: 'r2', members: [{id: 'b', role: 'r1', type: 'node'}, {id: 'c', role: 'r2', type: 'node'}]})
+                'r2': iD.Relation({id: 'r2', members: [{id: 'b', role: 'r2', type: 'node'}, {id: 'c', role: 'r2', type: 'node'}]})
             });
 
         graph = iD.actions.Connect(['b', 'c'])(graph);

test/spec/actions/join.js

@@ -161,7 +161,7 @@ describe("iD.actions.Join", function () {
                 '-': iD.Way({id: '-', nodes: ['a', 'b']}),
                 '=': iD.Way({id: '=', nodes: ['b', 'c']}),
                 'r1': iD.Relation({id: 'r1', members: [{id: '=', role: 'r1', type: 'way'}]}),
-                'r2': iD.Relation({id: 'r2', members: [{id: '=', role: 'r1', type: 'way'}, {id: '-', role: 'r2', type: 'way'}]})
+                'r2': iD.Relation({id: 'r2', members: [{id: '=', role: 'r2', type: 'way'}, {id: '-', role: 'r2', type: 'way'}]})
             });
 
         graph = iD.actions.Join('-', '=')(graph);

test/spec/graph/relation.js

@@ -131,6 +131,35 @@ describe('iD.Relation', function () {
         });
     });
 
+    describe("#replaceMember", function () {
+        it("returns self if self does not contain needle", function () {
+            var r = iD.Relation({members: []});
+            expect(r.replaceMember({id: 'a'}, {id: 'b'})).to.equal(r);
+        });
+
+        it("replaces a member which doesn't already exist", function () {
+            var r = iD.Relation({members: [{id: 'a', role: 'a'}]});
+            expect(r.replaceMember({id: 'a'}, {id: 'b', type: 'node'}).members).to.eql([{id: 'b', role: 'a', type: 'node'}]);
+        });
+
+        it("preserves the existing role", function () {
+            var r = iD.Relation({members: [{id: 'a', role: 'a', type: 'node'}]});
+            expect(r.replaceMember({id: 'a'}, {id: 'b', type: 'node'}).members).to.eql([{id: 'b', role: 'a', type: 'node'}]);
+        });
+
+        it("uses the replacement type", function () {
+            var r = iD.Relation({members: [{id: 'a', role: 'a', type: 'node'}]});
+            expect(r.replaceMember({id: 'a'}, {id: 'b', type: 'way'}).members).to.eql([{id: 'b', role: 'a', type: 'way'}]);
+        });
+
+        it("removes members if replacing them would produce duplicates", function () {
+            var r = iD.Relation({members: [
+                {id: 'a', role: 'b', type: 'node'},
+                {id: 'b', role: 'b', type: 'node'}]});
+            expect(r.replaceMember({id: 'a'}, {id: 'b', type: 'node'}).members).to.eql([{id: 'b', role: 'b', type: 'node'}]);
+        });
+    });
+
     describe("#asJXON", function () {
         it('converts a relation to jxon', function() {
             var relation = iD.Relation({id: 'r-1', members: [{id: 'w1', role: 'forward', type: 'way'}], tags: {type: 'route'}});