From 99d037e97f98eb205bceea59d16f005381821323 Mon Sep 17 00:00:00 2001
From: Bryan Housel <bryan@7thposition.com>
Date: Wed, 22 Apr 2015 15:15:12 -0400
Subject: [PATCH] Better logic for adding `area=yes` (closes #2578)

This fixes a few issues:

1. before: checked first key in applyTags and break loop, now: check all of them
(this was what caused `area=yes` to be added to 'branded' presets:
the first key is for these is `name` which isn't in areaKeys.)

2. add `area=yes` if user is drawing an area but the preset can be an
area or a line (e.g. `barrier=city_wall`)

3. remove `area=yes` when switching to another preset
---
 js/id/presets/preset.js     | 22 ++++++++++++++++++----
 test/spec/presets/preset.js | 24 ++++++++++++++++++------
 2 files changed, 36 insertions(+), 10 deletions(-)

diff --git a/js/id/presets/preset.js b/js/id/presets/preset.js
index dded14da8..c893eac06 100644
--- a/js/id/presets/preset.js
+++ b/js/id/presets/preset.js
@@ -3,6 +3,7 @@ iD.presets.Preset = function(id, preset, fields) {
 
     preset.id = id;
     preset.fields = (preset.fields || []).map(getFields);
+    preset.geometry = (preset.geometry || []);
 
     function getFields(f) {
         return fields[f];
@@ -76,6 +77,7 @@ iD.presets.Preset = function(id, preset, fields) {
             }
         }
 
+        delete tags.area;
         return tags;
     };
 
@@ -93,11 +95,23 @@ iD.presets.Preset = function(id, preset, fields) {
             }
         }
 
-        // Add area=yes if necessary
-        for (k in applyTags) {
-            if (geometry === 'area' && !(k in iD.areaKeys))
+        // Add area=yes if necessary.
+        // This is necessary if the geometry is already an area (e.g. user drew an area) AND any of:
+        // 1. chosen preset could be either an area or a line (`barrier=city_wall`)
+        // 2. chosen preset doesn't have a key in areaKeys (`railway=station`)
+        if (geometry === 'area') {
+            var needsAreaTag = true;
+            if (preset.geometry.indexOf('line') === -1) {
+                for (k in applyTags) {
+                    if (k in iD.areaKeys) {
+                        needsAreaTag = false;
+                        break;
+                    }
+                }
+            }
+            if (needsAreaTag) {
                 tags.area = 'yes';
-            break;
+            }
         }
 
         for (var f in preset.fields) {
diff --git a/test/spec/presets/preset.js b/test/spec/presets/preset.js
index b517deb86..6ccc77cae 100644
--- a/test/spec/presets/preset.js
+++ b/test/spec/presets/preset.js
@@ -77,7 +77,7 @@ describe('iD.presets.Preset', function() {
         it("adds default tags of fields with matching geometry", function() {
             var field = iD.presets.Field('field', {key: 'building', geometry: 'area', default: 'yes'}),
                 preset = iD.presets.Preset('test', {fields: ['field']}, {field: field});
-            expect(preset.applyTags({}, 'area')).to.eql({building: 'yes'});
+            expect(preset.applyTags({}, 'area')).to.eql({area: 'yes', building: 'yes'});
         });
 
         it("adds no default tags of fields with non-matching geometry", function() {
@@ -86,15 +86,22 @@ describe('iD.presets.Preset', function() {
             expect(preset.applyTags({}, 'point')).to.eql({});
         });
 
-        context("with an area preset whose primary tag is not in areaKeys", function() {
-            var preset = iD.presets.Preset('test', {geometry: ['line', 'area'], tags: {highway: 'pedestrian'}});
+        context("for a preset with no tag in areaKeys", function() {
+            var preset = iD.presets.Preset('test', {geometry: ['line', 'area'], tags: {name: 'testname', highway: 'pedestrian'}});
 
-            it("adds no area=yes to non-areas", function() {
-                expect(preset.applyTags({}, 'line')).to.eql({highway: 'pedestrian'});
+            it("doesn't add area=yes to non-areas", function() {
+                expect(preset.applyTags({}, 'line')).to.eql({name: 'testname', highway: 'pedestrian'});
             });
 
             it("adds area=yes to areas", function() {
-                expect(preset.applyTags({}, 'area')).to.eql({highway: 'pedestrian', area: 'yes'});
+                expect(preset.applyTags({}, 'area')).to.eql({name: 'testname', highway: 'pedestrian', area: 'yes'});
+            });
+        });
+
+        context("for a preset with a tag in areaKeys", function() {
+            var preset = iD.presets.Preset('test', {geometry: ['area'], tags: {name: 'testname', natural: 'water'}});
+            it("doesn't add area=yes", function() {
+                expect(preset.applyTags({}, 'area')).to.eql({name: 'testname', natural: 'water'});
             });
         });
     });
@@ -111,6 +118,11 @@ describe('iD.presets.Preset', function() {
             expect(preset.removeTags({building: 'yes'}, 'area')).to.eql({});
         });
 
+        it('removes area=yes', function() {
+            var preset = iD.presets.Preset('test', {tags: {highway: 'pedestrian'}});
+            expect(preset.removeTags({highway: 'pedestrian', area: 'yes'}, 'area')).to.eql({});
+        });
+
         it('preserves tags that do not match field default tags', function() {
             var field = iD.presets.Field('field', {key: 'building', geometry: 'area', default: 'yes'}),
                 preset = iD.presets.Preset('test', {fields: ['field']}, {field: field});