From b7fccca23ab57568b4b399124fd0fcc21ca4066d Mon Sep 17 00:00:00 2001
From: Alex Zolotarev <alex@maps.me>
Date: Sun, 20 Mar 2016 16:54:56 +0300
Subject: [PATCH] [editor] Fixed duplication of similar tags like phone and
 contact:phone.

---
 editor/editor_config.hpp                 |  4 ++
 editor/editor_tests/xml_feature_test.cpp | 70 ++++++++++++++++++++++++
 editor/xml_feature.cpp                   | 27 ++++++++-
 3 files changed, 100 insertions(+), 1 deletion(-)

diff --git a/editor/editor_config.hpp b/editor/editor_config.hpp
index 6dde9cefac..7985185af4 100644
--- a/editor/editor_config.hpp
+++ b/editor/editor_config.hpp
@@ -50,6 +50,10 @@ public:
 
   void Reload();
 
+  // TODO(mgsergio): Implement this getter to avoid hard-code in XMLFeature::ApplyPatch.
+  // It should return [[phone, contact:phone], [website, contact:website, url], ...].
+  //vector<vector<string>> GetAlternativeFields() const;
+
 private:
   string const m_fileName;
   pugi::xml_document m_document;
diff --git a/editor/editor_tests/xml_feature_test.cpp b/editor/editor_tests/xml_feature_test.cpp
index 6ccc0e713f..dd6f19091e 100644
--- a/editor/editor_tests/xml_feature_test.cpp
+++ b/editor/editor_tests/xml_feature_test.cpp
@@ -253,3 +253,73 @@ UNIT_TEST(XMLFeature_Geometry)
   feature.SetGeometry(geometry);
   TEST_EQUAL(feature.GetGeometry(), geometry, ());
 }
+
+UNIT_TEST(XMLFeature_ApplyPatch)
+{
+  auto const kOsmFeature = R"(<?xml version="1.0"?>
+  <osm>
+  <node id="1" lat="1" lon="2" timestamp="2015-11-27T21:13:32Z" version="1">
+    <tag k="amenity" v="cafe"/>
+  </node>
+  </osm>
+  )";
+
+  auto const kPatch = R"(<?xml version="1.0"?>
+  <node lat="1" lon="2" timestamp="2015-11-27T21:13:32Z">
+    <tag k="website" v="maps.me"/>
+  </node>
+  )";
+
+  XMLFeature const baseOsmFeature = XMLFeature::FromOSM(kOsmFeature).front();
+
+  {
+    XMLFeature noAnyTags = baseOsmFeature;
+    noAnyTags.ApplyPatch(XMLFeature(kPatch));
+    TEST(noAnyTags.HasKey("website"), ());
+  }
+
+  {
+    XMLFeature hasMainTag = baseOsmFeature;
+    hasMainTag.SetTagValue("website", "mapswith.me");
+    hasMainTag.ApplyPatch(XMLFeature(kPatch));
+    TEST_EQUAL(hasMainTag.GetTagValue("website"), "maps.me", ());
+    size_t tagsCount = 0;
+    hasMainTag.ForEachTag([&tagsCount](string const &, string const &){ ++tagsCount; });
+    TEST_EQUAL(2, tagsCount, ("website should be replaced, not duplicated."));
+  }
+
+  {
+    XMLFeature hasAltTag = baseOsmFeature;
+    hasAltTag.SetTagValue("contact:website", "mapswith.me");
+    hasAltTag.ApplyPatch(XMLFeature(kPatch));
+    TEST(!hasAltTag.HasTag("website"), ("Existing alt tag should be used."));
+    TEST_EQUAL(hasAltTag.GetTagValue("contact:website"), "maps.me", ());
+  }
+
+  {
+    XMLFeature hasAltTag = baseOsmFeature;
+    hasAltTag.SetTagValue("url", "mapswithme.com");
+    hasAltTag.ApplyPatch(XMLFeature(kPatch));
+    TEST(!hasAltTag.HasTag("website"), ("Existing alt tag should be used."));
+    TEST_EQUAL(hasAltTag.GetTagValue("url"), "maps.me", ());
+  }
+
+  {
+    XMLFeature hasTwoAltTags = baseOsmFeature;
+    hasTwoAltTags.SetTagValue("contact:website", "mapswith.me");
+    hasTwoAltTags.SetTagValue("url", "mapswithme.com");
+    hasTwoAltTags.ApplyPatch(XMLFeature(kPatch));
+    TEST(!hasTwoAltTags.HasTag("website"), ("Existing alt tag should be used."));
+    TEST_EQUAL(hasTwoAltTags.GetTagValue("contact:website"), "maps.me", ());
+    TEST_EQUAL(hasTwoAltTags.GetTagValue("url"), "mapswithme.com", ());
+  }
+
+  {
+    XMLFeature hasMainAndAltTag = baseOsmFeature;
+    hasMainAndAltTag.SetTagValue("website", "osmrulezz.com");
+    hasMainAndAltTag.SetTagValue("url", "mapswithme.com");
+    hasMainAndAltTag.ApplyPatch(XMLFeature(kPatch));
+    TEST_EQUAL(hasMainAndAltTag.GetTagValue("website"), "maps.me", ());
+    TEST_EQUAL(hasMainAndAltTag.GetTagValue("url"), "mapswithme.com", ());
+  }
+}
diff --git a/editor/xml_feature.cpp b/editor/xml_feature.cpp
index 0f0b24c8d5..3b7abad3b6 100644
--- a/editor/xml_feature.cpp
+++ b/editor/xml_feature.cpp
@@ -161,8 +161,33 @@ string XMLFeature::ToOSMString() const
 
 void XMLFeature::ApplyPatch(XMLFeature const & featureWithChanges)
 {
-  featureWithChanges.ForEachTag([this](string const & k, string const & v)
+  // TODO(mgsergio): Get these alt tags from the config.
+  vector<vector<string>> const alternativeTags =
   {
+    {"phone", "contact:phone"},
+    {"website", "contact:website", "url"},
+    {"fax", "contact:fax"},
+    {"email", "contact:email"}
+  };
+  featureWithChanges.ForEachTag([&alternativeTags, this](string const & k, string const & v)
+  {
+    // Avoid duplication for similar alternative osm tags.
+    for (auto const & alt : alternativeTags)
+    {
+      ASSERT(!alt.empty(), ());
+      if (k == alt.front())
+      {
+        for (auto const & tag : alt)
+        {
+          // Reuse already existing tag if it's present.
+          if (HasTag(tag))
+          {
+            SetTagValue(tag, v);
+            return;
+          }
+        }
+      }
+    }
     SetTagValue(k, v);
   });
 }