From 075b85c81d50fceb4fa247d173e991678586c833 Mon Sep 17 00:00:00 2001
From: Bryan Housel <bryan@mapbox.com>
Date: Sun, 14 Jan 2018 14:49:57 -0500
Subject: [PATCH] Apply reversal actions in `actionJoin` (closes #4688)

---
 modules/actions/join.js       | 34 +++++++++++++++++-----------------
 test/spec/actions/join.js     | 12 ++++++------
 test/spec/osm/multipolygon.js |  4 ++--
 3 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/modules/actions/join.js b/modules/actions/join.js
index 5749ffbba..421cda3ce 100644
--- a/modules/actions/join.js
+++ b/modules/actions/join.js
@@ -1,13 +1,8 @@
 import _extend from 'lodash-es/extend';
 import _groupBy from 'lodash-es/groupBy';
-import _map from 'lodash-es/map';
 
 import { actionDeleteWay } from './delete_way';
-
-import {
-    osmIsInterestingTag,
-    osmJoinWays
-} from '../osm';
+import { osmIsInterestingTag, osmJoinWays } from '../osm';
 
 
 // Join ways at the end node they share.
@@ -27,25 +22,30 @@ export function actionJoin(ids) {
 
 
     var action = function(graph) {
-        var ways = ids.map(graph.entity, graph),
-            survivor = ways[0];
+        var ways = ids.map(graph.entity, graph);
+        var survivorID = ways[0].id;
 
         // Prefer to keep an existing way.
         for (var i = 0; i < ways.length; i++) {
             if (!ways[i].isNew()) {
-                survivor = ways[i];
+                survivorID = ways[i].id;
                 break;
             }
         }
 
-        var joined = osmJoinWays(ways, graph)[0];
+        var sequences = osmJoinWays(ways, graph);
+        var joined = sequences[0];
 
-        survivor = survivor.update({nodes: _map(joined.nodes, 'id')});
+        // We might need to reverse some of these ways before joining them.  #4688
+        // `joined.actions` property will contain any actions we need to apply.
+        graph = sequences.actions.reduce(function(g, action) { return action(g); }, graph);
+
+        var survivor = graph.entity(survivorID);
+        survivor = survivor.update({ nodes: joined.nodes.map(function(n) { return n.id; }) });
         graph = graph.replace(survivor);
 
         joined.forEach(function(way) {
-            if (way.id === survivor.id)
-                return;
+            if (way.id === survivorID) return;
 
             graph.parentRelations(way).forEach(function(parent) {
                 graph = graph.replace(parent.replaceMember(way, survivor));
@@ -70,10 +70,10 @@ export function actionJoin(ids) {
         if (joined.length > 1)
             return 'not_adjacent';
 
-        var nodeIds = _map(joined[0].nodes, 'id').slice(1, -1),
-            relation,
-            tags = {},
-            conflicting = false;
+        var nodeIds = joined[0].nodes.map(function(n) { return n.id; }).slice(1, -1);
+        var relation;
+        var tags = {};
+        var conflicting = false;
 
         joined[0].forEach(function(way) {
             var parents = graph.parentRelations(way);
diff --git a/test/spec/actions/join.js b/test/spec/actions/join.js
index d591c4d55..8b1378187 100644
--- a/test/spec/actions/join.js
+++ b/test/spec/actions/join.js
@@ -274,25 +274,25 @@ describe('iD.actionJoin', function () {
 
         graph = iD.actionJoin(['-', '='])(graph);
 
-        expect(graph.entity('-').nodes).to.eql(['c', 'b', 'a']);
+        expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']);
         expect(graph.hasEntity('=')).to.be.undefined;
     });
 
     it('joins a <-- b ==> c', function () {
         // Expected result:
-        // a <-- b <-- c
-        // tags on === reversed
+        // a --> b --> c
+        // tags on --- reversed
         var graph = iD.Graph([
                 iD.Node({id: 'a'}),
                 iD.Node({id: 'b'}),
                 iD.Node({id: 'c'}),
-                iD.Way({id: '-', nodes: ['b', 'a']}),
-                iD.Way({id: '=', nodes: ['b', 'c'], tags: {'lanes:forward': 2}})
+                iD.Way({id: '-', nodes: ['b', 'a'], tags: {'lanes:forward': 2}}),
+                iD.Way({id: '=', nodes: ['b', 'c']})
             ]);
 
         graph = iD.actionJoin(['-', '='])(graph);
 
-        expect(graph.entity('-').nodes).to.eql(['c', 'b', 'a']);
+        expect(graph.entity('-').nodes).to.eql(['a', 'b', 'c']);
         expect(graph.hasEntity('=')).to.be.undefined;
         expect(graph.entity('-').tags).to.eql({'lanes:backward': 2});
     });
diff --git a/test/spec/osm/multipolygon.js b/test/spec/osm/multipolygon.js
index 1cb467d72..f3bba303b 100644
--- a/test/spec/osm/multipolygon.js
+++ b/test/spec/osm/multipolygon.js
@@ -407,8 +407,8 @@ describe('iD.osmJoinWays', function() {
         var graph = iD.coreGraph([a, b, c, d, e, w1, w2, w3, w4, w5, r]);
 
         var result = iD.osmJoinWays(r.members, graph);
-        expect(result.length).to.equal(3);
-        expect(result.actions.length).to.equal(1);
+        expect(result.length).to.equal(1);
+        expect(result.actions.length).to.equal(3);
 
         expect(getIDs(result[0].nodes)).to.eql(['a', 'b', 'c', 'd', 'e', 'c', 'b', 'a']);
         expect(result[0].length).to.equal(7);