From a41e8c48d22ace150b3e2faac513f9dce4c73d43 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Ky=E2=84=93e=20Hensel?= Date: Wed, 5 Mar 2025 22:42:40 +1100 Subject: [PATCH] separate tag-upgrade warnings from NSI suggestions (#10801) --- CHANGELOG.md | 2 + modules/util/util.js | 7 +- modules/validations/outdated_tags.js | 156 +++++++++++++------------ test/spec/validations/outdated_tags.js | 119 +++++++++++++++++++ 4 files changed, 207 insertions(+), 77 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f065f2f0e..e72d7627e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -59,6 +59,7 @@ _Breaking developer changes, which may affect downstream projects or sites that * Revalidate ways that are added to or removed from relations ([#10786]) * Preserve `crossing:markings` tag when fixing missing connection of crossing path and road ([#9586], thanks [@jtracey]) * Add a dedicated description to fix waterway-road intersections by adding a _culvert_ ([#10778], thanks [@matkoniecz]) +* Separate tag-upgrade warnings from NSI suggestions ([#10800], thanks [@k-yle]) #### :bug: Bugfixes * Prevent degenerate ways caused by deleting a corner of a triangle ([#10003], thanks [@k-yle]) * Fix briefly disappearing data layer during background layer tile layer switching transition ([#10748]) @@ -104,6 +105,7 @@ _Breaking developer changes, which may affect downstream projects or sites that [#10776]: https://github.com/openstreetmap/iD/issues/10776 [#10778]: https://github.com/openstreetmap/iD/issues/10778 [#10798]: https://github.com/openstreetmap/iD/pull/10798 +[#10800]: https://github.com/openstreetmap/iD/pull/10800 [#10807]: https://github.com/openstreetmap/iD/issues/10807 [@hlfan]: https://github.com/hlfan [@Deeptanshu-sankhwar]: https://github.com/Deeptanshu-sankhwar diff --git a/modules/util/util.js b/modules/util/util.js index 0039ef152..68dc61d50 100644 --- a/modules/util/util.js +++ b/modules/util/util.js @@ -28,8 +28,13 @@ export function utilTotalExtent(array, graph) { return extent; } - +/** + * @typedef {{ type: '-' | '+'; key: string; oldVal: string; newVal: string; display: string; }} TagDiff + * @param {Tags} oldTags + * @param {Tags} newTags + */ export function utilTagDiff(oldTags, newTags) { + /** @type {TagDiff[]} */ var tagDiff = []; var keys = utilArrayUnion(Object.keys(oldTags), Object.keys(newTags)).sort(); keys.forEach(function(k) { diff --git a/modules/validations/outdated_tags.js b/modules/validations/outdated_tags.js index 046bb66ee..f1190e194 100644 --- a/modules/validations/outdated_tags.js +++ b/modules/validations/outdated_tags.js @@ -10,6 +10,8 @@ import { utilHashcode, utilTagDiff } from '../util'; import { utilDisplayLabel } from '../util/utilDisplayLabel'; import { validationIssue, validationIssueFix } from '../core/validation'; +/** @import { TagDiff } from '../util/util'. */ + export function validationOutdatedTags() { const type = 'outdated_tags'; @@ -30,7 +32,6 @@ export function validationOutdatedTags() { if (!preset) return []; const oldTags = Object.assign({}, entity.tags); // shallow copy - let subtype = 'deprecated_tags'; // Upgrade preset, if a replacement is available.. if (preset.replacement) { @@ -40,8 +41,6 @@ export function validationOutdatedTags() { preset = newPreset; } - const upgradeReasons = []; - // Upgrade deprecated tags.. if (_dataDeprecated) { const deprecatedTags = entity.deprecatedTags(_dataDeprecated); @@ -56,10 +55,6 @@ export function validationOutdatedTags() { if (deprecatedTags.length) { deprecatedTags.forEach(tag => { graph = actionUpgradeTags(entity.id, tag.old, tag.replace)(graph); - upgradeReasons.push({ - source: 'id-tagging-schema--deprecated', - data: tag - }); }); entity = graph.entity(entity.id); } @@ -75,14 +70,12 @@ export function validationOutdatedTags() { } else if (preset.addTags[k]) { newTags[k] = preset.addTags[k]; } - upgradeReasons.push({ - source: 'id-tagging-schema--preset-addTags', - data: preset - }); } }); } + const deprecationDiff = utilTagDiff(oldTags, newTags); + // Attempt to match a canonical record in the name-suggestion-index. const nsi = services.nsi; let waitingForNsi = false; @@ -91,81 +84,106 @@ export function validationOutdatedTags() { waitingForNsi = (nsi.status() === 'loading'); if (!waitingForNsi) { const loc = entity.extent(graph).center(); - nsiResult = nsi.upgradeTags(newTags, loc); - if (nsiResult) { - newTags = nsiResult.newTags; - subtype = 'noncanonical_brand'; - upgradeReasons.push({ - source: 'name-suggestion-index', - data: nsiResult - }); - } + nsiResult = nsi.upgradeTags(oldTags, loc); } } + const nsiDiff = nsiResult ? utilTagDiff(oldTags, nsiResult.newTags) : []; + let issues = []; issues.provisional = (_waitingForDeprecated || waitingForNsi); - // determine diff - const tagDiff = utilTagDiff(oldTags, newTags); - if (!tagDiff.length) return issues; + if (deprecationDiff.length) { + const isOnlyAddingTags = deprecationDiff.every(d => d.type === '+'); + const prefix = isOnlyAddingTags ? 'incomplete.' : ''; - const isOnlyAddingTags = tagDiff.every(d => d.type === '+'); + issues.push(new validationIssue({ + type: type, + subtype: isOnlyAddingTags ? 'incomplete_tags' : 'deprecated_tags', + severity: 'warning', + message: (context) => { + const currEntity = context.hasEntity(entity.id); + if (!currEntity) return ''; - let prefix = ''; - if (nsiResult) { - prefix = 'noncanonical_brand.'; - } else if (subtype === 'deprecated_tags' && isOnlyAddingTags) { - subtype = 'incomplete_tags'; - prefix = 'incomplete.'; + const feature = utilDisplayLabel(currEntity, context.graph(), /* verbose */ true); + + return t.append(`issues.outdated_tags.${prefix}message`, { feature }); + }, + reference: selection => showReference( + selection, + t.append(`issues.outdated_tags.${prefix}reference`), + deprecationDiff + ), + entityIds: [entity.id], + hash: utilHashcode(JSON.stringify(deprecationDiff)), + dynamicFixes: () => { + let fixes = [ + new validationIssueFix({ + title: t.append('issues.fix.upgrade_tags.title'), + onClick: (context) => { + context.perform(graph => doUpgrade(graph, deprecationDiff), t('issues.fix.upgrade_tags.annotation')); + } + }) + ]; + return fixes; + } + })); } - // don't allow autofixing brand tags - let autoArgs = subtype !== 'noncanonical_brand' ? [doUpgrade, t('issues.fix.upgrade_tags.annotation')] : null; + if (nsiDiff.length) { + const isOnlyAddingTags = nsiDiff.every(d => d.type === '+'); - issues.push(new validationIssue({ - type: type, - subtype: subtype, - severity: 'warning', - message: showMessage, - reference: showReference, - entityIds: [entity.id], - hash: utilHashcode(JSON.stringify(tagDiff)), - dynamicFixes: () => { - let fixes = [ - new validationIssueFix({ - autoArgs: autoArgs, - title: t.append('issues.fix.upgrade_tags.title'), - onClick: (context) => { - context.perform(doUpgrade, t('issues.fix.upgrade_tags.annotation')); - } - }) - ]; + issues.push(new validationIssue({ + type: type, + subtype: 'noncanonical_brand', + severity: 'warning', + message: (context) => { + const currEntity = context.hasEntity(entity.id); + if (!currEntity) return ''; - const item = nsiResult && nsiResult.matched; - if (item) { - fixes.push( + const feature = utilDisplayLabel(currEntity, context.graph(), /* verbose */ true); + + return isOnlyAddingTags + ? t.append('issues.outdated_tags.noncanonical_brand.message_incomplete', { feature }) + : t.append('issues.outdated_tags.noncanonical_brand.message', { feature }); + }, + reference: selection => showReference( + selection, + t.append('issues.outdated_tags.noncanonical_brand.reference'), + nsiDiff + ), + entityIds: [entity.id], + hash: utilHashcode(JSON.stringify(nsiDiff)), + dynamicFixes: () => { + let fixes = [ new validationIssueFix({ - title: t.append('issues.fix.tag_as_not.title', { name: item.displayName }), + title: t.append('issues.fix.upgrade_tags.title'), + onClick: (context) => { + context.perform(graph => doUpgrade(graph, nsiDiff), t('issues.fix.upgrade_tags.annotation')); + } + }), + new validationIssueFix({ + title: t.append('issues.fix.tag_as_not.title', { name: nsiResult.matched.displayName }), onClick: (context) => { context.perform(addNotTag, t('issues.fix.tag_as_not.annotation')); } }) - ); + ]; + return fixes; } - return fixes; + })); + } - } - })); return issues; - function doUpgrade(graph) { + /** @param {iD.Graph} graph @param {TagDiff[]} diff */ + function doUpgrade(graph, diff) { const currEntity = graph.hasEntity(entity.id); if (!currEntity) return graph; let newTags = Object.assign({}, currEntity.tags); // shallow copy - tagDiff.forEach(diff => { + diff.forEach(diff => { if (diff.type === '-') { delete newTags[diff.key]; } else if (diff.type === '+') { @@ -200,21 +218,7 @@ export function validationOutdatedTags() { } - function showMessage(context) { - const currEntity = context.hasEntity(entity.id); - if (!currEntity) return ''; - - let messageID = `issues.outdated_tags.${prefix}message`; - if (subtype === 'noncanonical_brand' && isOnlyAddingTags) { - messageID += '_incomplete'; - } - return t.append(messageID, { - feature: utilDisplayLabel(currEntity, context.graph(), true /* verbose */) - }); - } - - - function showReference(selection) { + function showReference(selection, reference, tagDiff) { let enter = selection.selectAll('.issue-reference') .data([0]) .enter(); @@ -222,7 +226,7 @@ export function validationOutdatedTags() { enter .append('div') .attr('class', 'issue-reference') - .call(t.append(`issues.outdated_tags.${prefix}reference`)); + .call(reference); enter .append('strong') diff --git a/test/spec/validations/outdated_tags.js b/test/spec/validations/outdated_tags.js index 1d2d04c26..ff0687ad3 100644 --- a/test/spec/validations/outdated_tags.js +++ b/test/spec/validations/outdated_tags.js @@ -5,13 +5,29 @@ describe('iD.validations.outdated_tags', function () { before(function() { iD.fileFetcher.cache().deprecated = [ + { old: { building: 'roof' }, replace: { building: 'roof', layer: '1' } }, { old: { highway: 'no' } }, { old: { highway: 'ford' }, replace: { ford: '*' } } ]; + iD.services.nsi = { + status: () => 'ok', + upgradeTags: (tags) => { + // mock implementation of NSI: All it does it suggest + // adding `brand:wikidata` if there's a matching `brand`. + const NSI = { 'Fish Bowl': 'Q110785465' }; + if (tags.brand && NSI[tags.brand] && tags['brand:wikidata'] !== NSI[tags.brand]) { + return { + matched: {}, + newTags: { ...tags, 'brand:wikidata': NSI[tags.brand] } + }; + } + }, + }; }); after(function() { iD.fileFetcher.cache().deprecated = []; + delete iD.services.nsi; }); beforeEach(function() { @@ -115,4 +131,107 @@ describe('iD.validations.outdated_tags', function () { var issues = validate(validator); expect(issues).to.have.lengthOf(0); }); + + it('flags suggestions from NSI', async () => { + createWay({ amenity: 'fast_food', brand: 'Fish Bowl' }); + const validator = iD.validationOutdatedTags(context); + await setTimeout(20); + const issues = validate(validator); + + expect(issues).toHaveLength(1); + expect(issues[0]).toMatchObject({ + type: 'outdated_tags', + subtype: 'noncanonical_brand', + severity: 'warning', + entityIds: ['w-1'], + }); + + // click on "Upgrade Tags" + issues[0].dynamicFixes()[0].onClick(context); + expect(context.graph().entity('w-1').tags).toStrictEqual({ + amenity: 'fast_food', + brand: 'Fish Bowl', + 'brand:wikidata': 'Q110785465', // added + }); + }); + + it('generates 2 separate issues for deprecated tags and NSI suggestions', async () => { + createWay({ highway: 'ford', amenity: 'fast_food', brand: 'Fish Bowl' }); + const validator = iD.validationOutdatedTags(context); + await setTimeout(20); + const issues = validate(validator); + + expect(issues).toHaveLength(2); + expect(issues[0]).toMatchObject({ + type: 'outdated_tags', + subtype: 'deprecated_tags', + severity: 'warning', + entityIds: ['w-1'], + }); + expect(issues[1]).toMatchObject({ + type: 'outdated_tags', + subtype: 'noncanonical_brand', + severity: 'warning', + entityIds: ['w-1'], + }); + + // click on "Upgrade Tags" (to fix the deprecated issue) + issues[0].dynamicFixes()[0].onClick(context); + expect(context.graph().entity('w-1').tags).toStrictEqual({ + amenity: 'fast_food', + brand: 'Fish Bowl', + // brand:wikidata not added yet + ford: 'yes' // tag upgraded + }); + + // click on "Upgrade Tags" (to fix the NSI suggestion) + issues[1].dynamicFixes()[0].onClick(context); + expect(context.graph().entity('w-1').tags).toStrictEqual({ + amenity: 'fast_food', + brand: 'Fish Bowl', + 'brand:wikidata': 'Q110785465', // added + ford: 'yes' // tag already added + }); + }); + + it('generates 2 separate issues for incomplete tags and NSI suggestions', async () => { + createWay({ building: 'roof', amenity: 'fast_food', brand: 'Fish Bowl' }); + const validator = iD.validationOutdatedTags(context); + await setTimeout(20); + const issues = validate(validator); + + expect(issues).toHaveLength(2); + expect(issues[0]).toMatchObject({ + type: 'outdated_tags', + subtype: 'incomplete_tags', + severity: 'warning', + entityIds: ['w-1'], + }); + expect(issues[1]).toMatchObject({ + type: 'outdated_tags', + subtype: 'noncanonical_brand', + severity: 'warning', + entityIds: ['w-1'], + }); + + // click on "Upgrade Tags" (to fix the incomplete_tags issue) + issues[0].dynamicFixes()[0].onClick(context); + expect(context.graph().entity('w-1').tags).toStrictEqual({ + amenity: 'fast_food', + brand: 'Fish Bowl', + // brand:wikidata not added yet + building: 'roof', + layer: '1', // tag added + }); + + // click on "Upgrade Tags" (to fix the NSI suggestion) + issues[1].dynamicFixes()[0].onClick(context); + expect(context.graph().entity('w-1').tags).toStrictEqual({ + amenity: 'fast_food', + brand: 'Fish Bowl', + 'brand:wikidata': 'Q110785465', // added + building: 'roof', + layer: '1', // tag already added + }); + }); });