diff --git a/data/core.yaml b/data/core.yaml index f1736102d..20f91e406 100644 --- a/data/core.yaml +++ b/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: diff --git a/modules/osm/tags.js b/modules/osm/tags.js index 32207091a..d9451f144 100644 --- a/modules/osm/tags.js +++ b/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'] +]; + diff --git a/modules/validations/index.js b/modules/validations/index.js index 985c1203d..0cc27ec55 100644 --- a/modules/validations/index.js +++ b/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'; diff --git a/modules/validations/mutually_exclusive_tags.js b/modules/validations/mutually_exclusive_tags.js new file mode 100644 index 000000000..a89b5d43c --- /dev/null +++ b/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; +} \ No newline at end of file diff --git a/modules/validations/suspicious_name.js b/modules/validations/suspicious_name.js index 77aea8236..369116bec 100644 --- a/modules/validations/suspicious_name.js +++ b/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,7 +104,6 @@ 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_-]+))?$/); @@ -159,15 +111,7 @@ export function validationSuspiciousName() { 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)); diff --git a/test/spec/validations/mutually_exclusive_tags.js b/test/spec/validations/mutually_exclusive_tags.js new file mode 100644 index 000000000..1639ea701 --- /dev/null +++ b/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); + }); +}); diff --git a/test/spec/validations/suspicious_name.js b/test/spec/validations/suspicious_name.js index bdd6d9cfd..f7696353d 100644 --- a/test/spec/validations/suspicious_name.js +++ b/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); - }); - });