Merge remote-tracking branch 'mtmail/mutually-exclusive-tags-validator' into develop

Author: tyrasd

CHANGELOG.md

@@ -49,6 +49,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
 #### :camera: Street-Level
 * Fetch Microsoft Bing Streetlevel imagery layer via the Bing Maps API (from the previously used undocumented internal API endpoints), fixing reliability issues with the service's integration ([#10074]) 
 #### :white_check_mark: Validation
+* Warn when objects contain tags which are considered to be _mutually exclusive_ like for example `noname` and `name` (replaces previous `incorrect_name` validator) ([#10035], thanks [@mtmail])
 #### :bug: Bugfixes
 * Show turn restriction editor also when there is only one possible "to" way, as there might exist restrictions with that way as _via_ ([#9983])
 * Local photos: Fix bug which prevented the last image from being removed from the map when removed from the list
@@ -72,6 +73,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
 [#9891]: https://github.com/openstreetmap/iD/pull/9891
 [#9983]: https://github.com/openstreetmap/iD/issues/9983
 [#9992]: https://github.com/openstreetmap/iD/issues/9992
+[#10035]: https://github.com/openstreetmap/iD/pull/10035
 [#10054]: https://github.com/openstreetmap/iD/issues/10054
 [#10062]: https://github.com/openstreetmap/iD/pull/10062
 [#10066]: https://github.com/openstreetmap/iD/pull/10066
@@ -82,6 +84,7 @@ _Breaking developer changes, which may affect downstream projects or sites that
 [@ramith-kulal]: https://github.com/ramith-kulal
 [@mangerlahn]: https://github.com/mangerlahn
 [@NaVis0mple]: https://github.com/NaVis0mple
+[@mtmail]: https://github.com/mtmail
 
 
 # 2.27.3

data/core.yaml

@@ -1839,9 +1839,6 @@ en:
         amap: "Amap products are proprietary and must not be used as references."
         baidu: "Baidu products are proprietary and must not be used as references."
         google: "Google products are proprietary and must not be used as references."
-    incorrect_name:
-      message: '{feature} has the mistaken name "{name}"'
-      message_language: '{feature} has the mistaken name "{name}" in {language}'
     invalid_format:
       title: Invalid Formatting
       tip: Find tags with unexpected formats
@@ -1873,6 +1870,15 @@ en:
         message: "{feature} has no descriptive tags"
       relation_type:
         message: "{feature} is a relation without a type"
+    mutually_exclusive_tags:
+      title: Contradictory Tags
+      tip: "Find features that have contradictory tags."
+      default:
+        message: "{feature} has both {tag1} and {tag2}"
+        reference: 'The tags "{tag1}" and "{tag2}" contradict each other. Only one tag should be set.'
+      same_value:
+        message: "{feature}: {tag1} and {tag2} have same value"
+        reference: 'The tags "{tag1}" and "{tag2}" have the same value. Only one tag should be set.'
     old_multipolygon:
       message: "{multipolygon} has misplaced tags"
       reference: "Multipolygons should be tagged on their relation, not their outer way."
@@ -1998,6 +2004,9 @@ en:
       remove_tag:
         title: Remove the tag
         annotation: Removed tag.
+      remove_named_tag:
+        title: 'Remove the "{tag}" tag'
+        annotation: 'Removed "{tag}" tag.'
       remove_tags:
         title: Remove the tags
       remove_the_name:

modules/osm/tags.js

@@ -255,3 +255,15 @@ export function isColourValid(value) {
     }
     return true;
 }
+
+// https://wiki.openstreetmap.org/wiki/Special:WhatLinksHere/Property:P44
+export var osmMutuallyExclusiveTagPairs = [
+    ['noname', 'name'],
+    ['noref', 'ref'],
+    ['nohousenumber', 'addr:housenumber'],
+    ['noaddress', 'addr:housenumber'],
+    ['noaddress', 'addr:housename'],
+    ['noaddress', 'addr:unit'],
+    ['addr:nostreet', 'addr:street']
+];
+

modules/validations/index.js

@@ -10,6 +10,7 @@ export { validationMaprules } from './maprules';
 export { validationMismatchedGeometry } from './mismatched_geometry';
 export { validationMissingRole } from './missing_role';
 export { validationMissingTag } from './missing_tag';
+export { validationMutuallyExclusiveTags } from './mutually_exclusive_tags';
 export { validationOutdatedTags } from './outdated_tags';
 export { validationPrivateData } from './private_data';
 export { validationSuspiciousName } from './suspicious_name';

modules/validations/mutually_exclusive_tags.js

@@ -0,0 +1,93 @@
+import { actionChangeTags } from '../actions/change_tags';
+import { t } from '../core/localizer';
+import { utilDisplayLabel } from '../util';
+import { validationIssue, validationIssueFix } from '../core/validation';
+import { osmMutuallyExclusiveTagPairs } from '../osm/tags';
+
+export function validationMutuallyExclusiveTags(/* context */) {
+    const type = 'mutually_exclusive_tags';
+
+    // https://wiki.openstreetmap.org/wiki/Special:WhatLinksHere/Property:P44
+    const tagKeyPairs = osmMutuallyExclusiveTagPairs;
+
+    const validation = function checkMutuallyExclusiveTags(entity /*, graph */) {
+
+        let pairsFounds = tagKeyPairs.filter((pair) => {
+            return (pair[0] in entity.tags && pair[1] in entity.tags);
+        }).filter((pair) => {
+            // noname=no is double-negation, thus positive and not conflicting. We'll ignore those
+            return !((pair[0].match(/^(addr:)?no[a-z]/) && entity.tags[pair[0]] === 'no') ||
+                     (pair[1].match(/^(addr:)?no[a-z]/) && entity.tags[pair[1]] === 'no'));
+        });
+
+        // Additional:
+        // Check if name and not:name (and similar) are set and both have the same value
+        // not:name can actually have multiple values, separate by ;
+        // https://taginfo.openstreetmap.org/search?q=not%3A#keys
+        Object.keys(entity.tags).forEach((key) => {
+            let negative_key = 'not:' + key;
+            if (negative_key in entity.tags && entity.tags[negative_key].split(';').includes(entity.tags[key])) {
+                pairsFounds.push([negative_key, key, 'same_value']);
+            }
+            // For name:xx we also compare against the not:name tag
+            if (key.match(/^name:[a-z]+/)) {
+                negative_key = 'not:name';
+                if (negative_key in entity.tags && entity.tags[negative_key].split(';').includes(entity.tags[key])) {
+                    pairsFounds.push([negative_key, key, 'same_value']);
+                }
+            }
+        });
+
+        let issues = pairsFounds.map((pair) => {
+            const subtype = pair[2] || 'default';
+            return new validationIssue({
+                type: type,
+                subtype: subtype,
+                severity: 'warning',
+                message: function(context) {
+                    let entity = context.hasEntity(this.entityIds[0]);
+                    return entity ? t.append(`issues.${type}.${subtype}.message`, {
+                        feature: utilDisplayLabel(entity, context.graph()),
+                        tag1: pair[0],
+                        tag2: pair[1]
+                    }) : '';
+                },
+                reference: (selection) => showReference(selection, pair, subtype),
+                entityIds: [entity.id],
+                dynamicFixes: () => pair.slice(0,2).map((tagToRemove) => createIssueFix(tagToRemove))
+            });
+        });
+
+        function createIssueFix(tagToRemove) {
+            return new validationIssueFix({
+                icon: 'iD-operation-delete',
+                title: t.append('issues.fix.remove_named_tag.title', { tag: tagToRemove }),
+                onClick: function(context) {
+                    const entityId = this.issue.entityIds[0];
+                    const entity = context.entity(entityId);
+                    let tags = Object.assign({}, entity.tags);   // shallow copy
+                    delete tags[tagToRemove];
+                    context.perform(
+                        actionChangeTags(entityId, tags),
+                        t('issues.fix.remove_named_tag.annotation', { tag: tagToRemove })
+                    );
+                }
+            });
+        }
+
+        function showReference(selection, pair, subtype) {
+            selection.selectAll('.issue-reference')
+                .data([0])
+                .enter()
+                .append('div')
+                .attr('class', 'issue-reference')
+                .call(t.append(`issues.${type}.${subtype}.reference`, { tag1: pair[0], tag2: pair[1] }));
+        }
+
+        return issues;
+    };
+
+    validation.type = type;
+
+    return validation;
+}

modules/validations/suspicious_name.js

@@ -96,53 +96,6 @@ export function validationSuspiciousName() {
     }
   }
 
-  function makeIncorrectNameIssue(entityId, nameKey, incorrectName, langCode) {
-    return new validationIssue({
-      type: type,
-      subtype: 'not_name',
-      severity: 'warning',
-      message: function(context) {
-        const entity = context.hasEntity(this.entityIds[0]);
-        if (!entity) return '';
-        const preset = presetManager.match(entity, context.graph());
-        const langName = langCode && localizer.languageName(langCode);
-        return t.append('issues.incorrect_name.message' + (langName ? '_language' : ''),
-          { feature: preset.name(), name: incorrectName, language: langName }
-        );
-      },
-      reference: showReference,
-      entityIds: [entityId],
-      hash: `${nameKey}=${incorrectName}`,
-      dynamicFixes: function() {
-        return [
-          new validationIssueFix({
-            icon: 'iD-operation-delete',
-            title: t.append('issues.fix.remove_the_name.title'),
-            onClick: function(context) {
-              const entityId = this.issue.entityIds[0];
-              const entity = context.entity(entityId);
-              let tags = Object.assign({}, entity.tags);   // shallow copy
-              delete tags[nameKey];
-              context.perform(
-                actionChangeTags(entityId, tags), t('issues.fix.remove_mistaken_name.annotation')
-              );
-            }
-          })
-        ];
-      }
-    });
-
-    function showReference(selection) {
-      selection.selectAll('.issue-reference')
-        .data([0])
-        .enter()
-        .append('div')
-        .attr('class', 'issue-reference')
-        .call(t.append('issues.generic_name.reference'));
-    }
-  }
-
-
   let validation = function checkGenericName(entity) {
     const tags = entity.tags;
 
@@ -151,23 +104,14 @@ export function validationSuspiciousName() {
     if (hasWikidata) return [];
 
     let issues = [];
-    const notNames = (tags['not:name'] || '').split(';');
 
     for (let key in tags) {
       const m = key.match(/^name(?:(?::)([a-zA-Z_-]+))?$/);
       if (!m) continue;
 
       const langCode = m.length >= 2 ? m[1] : null;
       const value = tags[key];
-      if (notNames.length) {
-        for (let i in notNames) {
-          const notName = notNames[i];
-          if (notName && value === notName) {
-            issues.push(makeIncorrectNameIssue(entity.id, key, value, langCode));
-            continue;
-          }
-        }
-      }
+
       if (isGenericName(value, tags)) {
         issues.provisional = _waitingForNsi;  // retry later if we are waiting on NSI to finish loading
         issues.push(makeGenericNameIssue(entity.id, key, value, langCode));

test/spec/validations/mutually_exclusive_tags.js

@@ -0,0 +1,92 @@
+describe('iD.validations.mutually_exclusive_tags', function () {
+    var context;
+
+    beforeEach(function() {
+        context = iD.coreContext().init();
+    });
+
+    function createNode(tags) {
+        context.perform(
+            iD.actionAddEntity({id: 'n-1', loc: [4,4], tags: tags})
+        );
+    }
+
+    function validate(validator) {
+        var changes = context.history().changes();
+        var entities = changes.modified.concat(changes.created);
+        var issues = [];
+        entities.forEach(function(entity) {
+            issues = issues.concat(validator(entity, context.graph()));
+        });
+        return issues;
+    }
+
+
+    it('has no errors on init', function(done) {
+        var validator = iD.validationMutuallyExclusiveTags(context);
+        window.setTimeout(function() {   // async, so data will be available
+            var issues = validate(validator);
+            expect(issues).to.have.lengthOf(0);
+            done();
+        }, 20);
+    });
+
+    it('has no errors on good tags', function(done) {
+        createNode({'name': 'Trader Joe', 'not:name': 'Trader Jane'});
+        var validator = iD.validationMutuallyExclusiveTags(context);
+        window.setTimeout(function() {   // async, so data will be available
+            var issues = validate(validator);
+            expect(issues).to.have.lengthOf(0);
+            done();
+        }, 20);
+    });
+
+    it('flags mutually exclusive tags', function(done) {
+        createNode({'name': 'Trader Joe', 'noname': 'yes'});
+        var validator = iD.validationMutuallyExclusiveTags(context);
+        window.setTimeout(function() {   // async, so data will be available
+            var issues = validate(validator);
+            expect(issues).to.have.lengthOf(1);
+            var issue = issues[0];
+            expect(issue.type).to.eql('mutually_exclusive_tags');
+            expect(issue.subtype).to.eql('default');
+            expect(issue.severity).to.eql('warning');
+            expect(issue.entityIds).to.have.lengthOf(1);
+            expect(issue.entityIds[0]).to.eql('n-1');
+            done();
+        }, 20);
+    });
+
+    it('flags feature with a mutually exclusive `not:name` value', function(done) {
+        createNode({ shop: 'supermarket', name: 'Lous', 'not:name': 'Lous' });
+        var validator = iD.validationMutuallyExclusiveTags(context);
+        window.setTimeout(function() {   // async, so data will be available
+            var issues = validate(validator);
+            expect(issues).to.have.lengthOf(1);
+            var issue = issues[0];
+            expect(issue.type).to.eql('mutually_exclusive_tags');
+            expect(issue.subtype).to.eql('same_value');
+            expect(issue.severity).to.eql('warning');
+            expect(issue.entityIds).to.have.lengthOf(1);
+            expect(issue.entityIds[0]).to.eql('n-1');
+            done();
+        }, 20);
+    });
+
+
+    it('flags feature with a mutually exclusive semicolon-separated `not:name` value', function(done) {
+        createNode({ shop: 'supermarket', name: 'Lous', 'not:name': 'Louis\';Lous;Louis\'s' });
+        var validator = iD.validationMutuallyExclusiveTags(context);
+        window.setTimeout(function() {   // async, so data will be available
+            var issues = validate(validator);
+            expect(issues).to.have.lengthOf(1);
+            var issue = issues[0];
+            expect(issue.type).to.eql('mutually_exclusive_tags');
+            expect(issue.subtype).to.eql('same_value');
+            expect(issue.severity).to.eql('warning');
+            expect(issue.entityIds).to.have.lengthOf(1);
+            expect(issue.entityIds[0]).to.eql('n-1');
+            done();
+        }, 20);
+    });
+});

test/spec/validations/suspicious_name.js

@@ -183,45 +183,4 @@ describe('iD.validations.suspicious_name', function () {
             done();
         }, 20);
     });
-
-    it('ignores feature with a non-matching `not:name` tag', function(done) {
-        createWay({ shop: 'supermarket', name: 'Lou\'s', 'not:name': 'Lous' });
-        var validator = iD.validationSuspiciousName(context);
-        window.setTimeout(function() {   // async, so data will be available
-            var issues = validate(validator);
-            expect(issues).to.have.lengthOf(0);
-            done();
-        }, 20);
-    });
-
-    it('flags feature with a matching `not:name` tag', function(done) {
-        createWay({ shop: 'supermarket', name: 'Lous', 'not:name': 'Lous' });
-        var validator = iD.validationSuspiciousName(context);
-        window.setTimeout(function() {   // async, so data will be available
-            var issues = validate(validator);
-            expect(issues).to.have.lengthOf(1);
-            var issue = issues[0];
-            expect(issue.type).to.eql('suspicious_name');
-            expect(issue.subtype).to.eql('not_name');
-            expect(issue.entityIds).to.have.lengthOf(1);
-            expect(issue.entityIds[0]).to.eql('w-1');
-            done();
-        }, 20);
-    });
-
-    it('flags feature with a matching a semicolon-separated `not:name` tag', function(done) {
-        createWay({ shop: 'supermarket', name: 'Lous', 'not:name': 'Louis\';Lous;Louis\'s' });
-        window.setTimeout(function() {   // async, so data will be available
-            var validator = iD.validationSuspiciousName(context);
-            var issues = validate(validator);
-            expect(issues).to.have.lengthOf(1);
-            var issue = issues[0];
-            expect(issue.type).to.eql('suspicious_name');
-            expect(issue.subtype).to.eql('not_name');
-            expect(issue.entityIds).to.have.lengthOf(1);
-            expect(issue.entityIds[0]).to.eql('w-1');
-            done();
-        }, 20);
-    });
-
 });