From 24c72b64d10a2ef8cc30fa692fb1e0c4138e286c Mon Sep 17 00:00:00 2001 From: Quincy Morgan Date: Wed, 13 Nov 2019 14:35:01 -0500 Subject: [PATCH] Load issue fixes dynamically instead of cacheing them (close #7037) --- modules/core/validation/models.js | 41 +++++++---- modules/core/validator.js | 32 +-------- modules/ui/entity_issues.js | 16 +++-- modules/validations/almost_junction.js | 71 +++++++++--------- modules/validations/close_nodes.js | 22 +++--- modules/validations/crossing_ways.js | 81 +++++++++++---------- modules/validations/disconnected_way.js | 84 ++++++++++++---------- modules/validations/help_request.js | 12 ++-- modules/validations/impossible_oneway.js | 63 ++++++++-------- modules/validations/incompatible_source.js | 12 ++-- modules/validations/mismatched_geometry.js | 82 +++++++++++---------- modules/validations/missing_role.js | 30 ++++---- modules/validations/missing_tag.js | 69 ++++++++++-------- modules/validations/outdated_tags.js | 40 ++++++----- modules/validations/private_data.js | 20 +++--- modules/validations/suspicious_name.js | 34 ++++----- modules/validations/unsquare_way.js | 66 ++++++++--------- test/spec/validations/almost_junction.js | 8 +-- 18 files changed, 413 insertions(+), 370 deletions(-) diff --git a/modules/core/validation/models.js b/modules/core/validation/models.js index 683971d11..c8551422a 100644 --- a/modules/core/validation/models.js +++ b/modules/core/validation/models.js @@ -1,4 +1,5 @@ import { geoExtent } from '../../geo'; +import { t } from '../../util/locale'; export function validationIssue(attrs) { this.type = attrs.type; // required - name of rule that created the issue (e.g. 'missing_tag') @@ -9,7 +10,7 @@ export function validationIssue(attrs) { this.entityIds = attrs.entityIds; // optional - array of IDs of entities involved in the issue this.loc = attrs.loc; // optional - [lon, lat] to zoom in on to see the issue this.data = attrs.data; // optional - object containing extra data for the fixes - this.fixes = attrs.fixes || []; // optional - array of validationIssueFix objects + this.dynamicFixes = attrs.dynamicFixes;// optional - function(context) returning fixes this.hash = attrs.hash; // optional - string to further differentiate the issue this.id = generateID.apply(this); // generated - see below @@ -50,25 +51,41 @@ export function validationIssue(attrs) { return null; }; + this.fixes = function(context) { + var fixes = this.dynamicFixes ? this.dynamicFixes(context) : []; + var issue = this; - if (this.fixes) { // add a reference in the fixes to the issue for use in fix actions - for (var i = 0; i < this.fixes.length; i++) { - var fix = this.fixes[i]; - fix.issue = this; - if (fix.autoArgs) { - this.autoFix = fix; - } + if (issue.severity === 'warning') { + // allow ignoring any issue that's not an error + fixes.push(new validationIssueFix({ + title: t('issues.fix.ignore_issue.title'), + icon: 'iD-icon-close', + onClick: function() { + context.validator().ignoreIssue(this.issue.id); + } + })); } - } + + fixes.forEach(function(fix) { + fix.id = fix.title; + // add a reference to the issue for use in actions + fix.issue = issue; + if (fix.autoArgs) { + issue.autoFix = fix; + } + }); + return fixes; + }; + } export function validationIssueFix(attrs) { this.title = attrs.title; // Required - this.onClick = attrs.onClick; // Required + this.onClick = attrs.onClick; // Optional - the function to run to apply the fix this.icon = attrs.icon; // Optional - shows 'iD-icon-wrench' if not set - this.entityIds = attrs.entityIds || []; // Optional - Used for hover-higlighting. + this.entityIds = attrs.entityIds || []; // Optional - used for hover-higlighting. this.autoArgs = attrs.autoArgs; // Optional - pass [actions, annotation] arglist if this fix can automatically run - this.issue = null; // Generated link - added by ValidationIssue constructor + this.issue = null; // Generated link - added by validationIssue } diff --git a/modules/core/validator.js b/modules/core/validator.js index 9caaf5073..e70a3f65a 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -5,7 +5,6 @@ import { geoExtent } from '../geo/extent'; import { modeSelect } from '../modes/select'; import { utilArrayGroupBy, utilRebind } from '../util'; import { t } from '../util/locale'; -import { validationIssueFix } from './validation/models'; import * as Validations from '../validations/index'; @@ -91,19 +90,7 @@ export function coreValidator(context) { buildings.forEach(function(entity) { var detected = checkUnsquareWay(entity, graph); if (detected.length !== 1) return; - var issue = detected[0]; - var ignoreFix = new validationIssueFix({ - title: t('issues.fix.ignore_issue.title'), - icon: 'iD-icon-close', - onClick: function() { - ignoreIssue(this.issue.id); - } - }); - ignoreFix.type = 'ignore'; - ignoreFix.issue = issue; - issue.fixes.push(ignoreFix); - if (!cache.issuesByEntityID[entity.id]) { cache.issuesByEntityID[entity.id] = new Set(); } @@ -275,9 +262,9 @@ export function coreValidator(context) { }; - function ignoreIssue(id) { + validator.ignoreIssue = function(id) { _ignoredIssueIDs[id] = true; - } + }; // @@ -296,21 +283,6 @@ export function coreValidator(context) { } var detected = fn(entity, graph); - detected.forEach(function(issue) { - var hasIgnoreFix = issue.fixes && issue.fixes.length && issue.fixes[issue.fixes.length - 1].type === 'ignore'; - if (issue.severity === 'warning' && !hasIgnoreFix) { - var ignoreFix = new validationIssueFix({ - title: t('issues.fix.ignore_issue.title'), - icon: 'iD-icon-close', - onClick: function() { - ignoreIssue(this.issue.id); - } - }); - ignoreFix.type = 'ignore'; - ignoreFix.issue = issue; - issue.fixes.push(ignoreFix); - } - }); entityIssues = entityIssues.concat(detected); } diff --git a/modules/ui/entity_issues.js b/modules/ui/entity_issues.js index 391601348..2fd0a5c3d 100644 --- a/modules/ui/entity_issues.js +++ b/modules/ui/entity_issues.js @@ -197,16 +197,17 @@ export function uiEntityIssues(context) { var fixLists = containers.selectAll('.issue-fix-list'); var fixes = fixLists.selectAll('.issue-fix-item') - .data(function(d) { return d.fixes ? d.fixes : []; }); + .data(function(d) { return d.fixes ? d.fixes(context) : []; }, function(fix) { return fix.id; }); + + fixes.exit() + .remove(); var fixesEnter = fixes.enter() .append('li') - .attr('class', function(d) { - return 'issue-fix-item ' + (d.onClick ? 'actionable' : ''); - }) + .attr('class', 'issue-fix-item') .on('click', function(d) { // not all fixes are actionable - if (!d.onClick) return; + if (!d3_select(this).classed('actionable') || !d.onClick) return; // Don't run another fix for this issue within a second of running one // (Necessary for "Select a feature type" fix. Most fixes should only ever run once) @@ -250,6 +251,11 @@ export function uiEntityIssues(context) { .append('span') .attr('class', 'fix-message') .text(function(d) { return d.title; }); + + fixesEnter.merge(fixes) + .classed('actionable', function(d) { + return d.onClick; + }); } diff --git a/modules/validations/almost_junction.js b/modules/validations/almost_junction.js index 11e844de8..210504243 100644 --- a/modules/validations/almost_junction.js +++ b/modules/validations/almost_junction.js @@ -42,9 +42,40 @@ export function validationAlmostJunction(context) { var extendableNodeInfos = findConnectableEndNodesByExtension(entity); extendableNodeInfos.forEach(function(extendableNodeInfo) { - var node = extendableNodeInfo.node; - var edgeHighway = graph.entity(extendableNodeInfo.wid); + issues.push(new validationIssue({ + type: type, + subtype: 'highway-highway', + severity: 'warning', + message: function(context) { + var entity1 = context.hasEntity(this.entityIds[0]); + if (this.entityIds[0] === this.entityIds[2]) { + return entity1 ? t('issues.almost_junction.self.message', { + feature: utilDisplayLabel(entity1, context) + }) : ''; + } else { + var entity2 = context.hasEntity(this.entityIds[2]); + return (entity1 && entity2) ? t('issues.almost_junction.message', { + feature: utilDisplayLabel(entity1, context), + feature2: utilDisplayLabel(entity2, context) + }) : ''; + } + }, + reference: showReference, + entityIds: [entity.id, extendableNodeInfo.node.id, extendableNodeInfo.wid], + loc: extendableNodeInfo.node.loc, + hash: JSON.stringify(extendableNodeInfo.node.loc), + data: { + edge: extendableNodeInfo.edge, + cross_loc: extendableNodeInfo.cross_loc + }, + dynamicFixes: makeFixes + })); + }); + return issues; + + + function makeFixes(context) { var fixes = [new validationIssueFix({ icon: 'iD-icon-abutment', title: t('issues.fix.connect_features.title'), @@ -73,7 +104,8 @@ export function validationAlmostJunction(context) { } })]; - if (Object.keys(node.tags).length === 0) { + var node = context.hasEntity(this.entityIds[1]); + if (node && Object.keys(node.tags).length === 0) { // node has no tags, suggest noexit fix fixes.push(new validationIssueFix({ icon: 'maki-barrier', @@ -88,37 +120,8 @@ export function validationAlmostJunction(context) { })); } - issues.push(new validationIssue({ - type: type, - subtype: 'highway-highway', - severity: 'warning', - message: function(context) { - var entity1 = context.hasEntity(this.entityIds[0]); - if (this.entityIds[0] === this.entityIds[2]) { - return entity1 ? t('issues.almost_junction.self.message', { - feature: utilDisplayLabel(entity1, context) - }) : ''; - } else { - var entity2 = context.hasEntity(this.entityIds[2]); - return (entity1 && entity2) ? t('issues.almost_junction.message', { - feature: utilDisplayLabel(entity1, context), - feature2: utilDisplayLabel(entity2, context) - }) : ''; - } - }, - reference: showReference, - entityIds: [entity.id, node.id, edgeHighway.id], - loc: extendableNodeInfo.node.loc, - hash: JSON.stringify(extendableNodeInfo.node.loc), - data: { - edge: extendableNodeInfo.edge, - cross_loc: extendableNodeInfo.cross_loc - }, - fixes: fixes - })); - }); - - return issues; + return fixes; + } function showReference(selection) { diff --git a/modules/validations/close_nodes.js b/modules/validations/close_nodes.js index fe4ca81d8..0e903c053 100644 --- a/modules/validations/close_nodes.js +++ b/modules/validations/close_nodes.js @@ -162,16 +162,18 @@ export function validationCloseNodes(context) { }, reference: showReference, entityIds: [node.id, nearby.id], - fixes: [ - new validationIssueFix({ - icon: 'iD-operation-disconnect', - title: t('issues.fix.move_points_apart.title') - }), - new validationIssueFix({ - icon: 'iD-icon-layers', - title: t('issues.fix.use_different_layers_or_levels.title') - }) - ] + dynamicFixes: function() { + return [ + new validationIssueFix({ + icon: 'iD-operation-disconnect', + title: t('issues.fix.move_points_apart.title') + }), + new validationIssueFix({ + icon: 'iD-icon-layers', + title: t('issues.fix.use_different_layers_or_levels.title') + }) + ]; + } })); } } diff --git a/modules/validations/crossing_ways.js b/modules/validations/crossing_ways.js index 1024633a4..8523039a6 100644 --- a/modules/validations/crossing_ways.js +++ b/modules/validations/crossing_ways.js @@ -359,44 +359,6 @@ export function validationCrossingWays(context) { crossingTypeID += '_connectable'; } - var fixes = []; - if (connectionTags) { - fixes.push(makeConnectWaysFix(connectionTags)); - } - - var useFixIcon = 'iD-icon-layers'; - var useFixID; - if (isCrossingIndoors) { - useFixID = 'use_different_levels'; - } else if (isCrossingTunnels || isCrossingBridges) { - useFixID = 'use_different_layers'; - // don't recommend bridges for waterways even though they're okay - } else if ((allowsBridge(featureType1) && featureType1 !== 'waterway') || - (allowsBridge(featureType2) && featureType2 !== 'waterway')) { - useFixID = 'use_bridge_or_tunnel'; - useFixIcon = 'maki-bridge'; - } else if (allowsTunnel(featureType1) || allowsTunnel(featureType2)) { - useFixID = 'use_tunnel'; - } else { - useFixID = 'use_different_layers'; - } - if (useFixID === 'use_different_layers' || - featureType1 === 'building' || - featureType2 === 'building') { - fixes.push(makeChangeLayerFix('higher')); - fixes.push(makeChangeLayerFix('lower')); - } - if (useFixID !== 'use_different_layers') { - fixes.push(new validationIssueFix({ - icon: useFixIcon, - title: t('issues.fix.' + useFixID + '.title') - })); - } - fixes.push(new validationIssueFix({ - icon: 'iD-operation-move', - title: t('issues.fix.reposition_features.title') - })); - return new validationIssue({ type: type, subtype: subtype, @@ -427,7 +389,48 @@ export function validationCrossingWays(context) { // ensure the correct connection tags are added in the fix JSON.stringify(connectionTags), loc: crossing.crossPoint, - fixes: fixes + dynamicFixes: function() { + var fixes = []; + + if (connectionTags) { + fixes.push(makeConnectWaysFix(connectionTags)); + } + + var useFixIcon = 'iD-icon-layers'; + var useFixID; + if (isCrossingIndoors) { + useFixID = 'use_different_levels'; + } else if (isCrossingTunnels || isCrossingBridges) { + useFixID = 'use_different_layers'; + // don't recommend bridges for waterways even though they're okay + } else if ((allowsBridge(featureType1) && featureType1 !== 'waterway') || + (allowsBridge(featureType2) && featureType2 !== 'waterway')) { + useFixID = 'use_bridge_or_tunnel'; + useFixIcon = 'maki-bridge'; + } else if (allowsTunnel(featureType1) || allowsTunnel(featureType2)) { + useFixID = 'use_tunnel'; + } else { + useFixID = 'use_different_layers'; + } + if (useFixID === 'use_different_layers' || + featureType1 === 'building' || + featureType2 === 'building') { + fixes.push(makeChangeLayerFix('higher')); + fixes.push(makeChangeLayerFix('lower')); + } + if (useFixID !== 'use_different_layers') { + fixes.push(new validationIssueFix({ + icon: useFixIcon, + title: t('issues.fix.' + useFixID + '.title') + })); + } + fixes.push(new validationIssueFix({ + icon: 'iD-operation-move', + title: t('issues.fix.reposition_features.title') + })); + + return fixes; + } }); function showReference(selection) { diff --git a/modules/validations/disconnected_way.js b/modules/validations/disconnected_way.js index 1ba1ab5dc..06df02926 100644 --- a/modules/validations/disconnected_way.js +++ b/modules/validations/disconnected_way.js @@ -18,44 +18,6 @@ export function validationDisconnectedWay() { var routingIslandWays = routingIslandForEntity(entity); if (!routingIslandWays) return []; - var fixes = []; - - var isSingle = routingIslandWays.size === 1; - - if (isSingle) { - - if (entity.type === 'way' && !entity.isClosed()) { - - var startFix = makeContinueDrawingFixIfAllowed(entity.first(), 'start'); - if (startFix) fixes.push(startFix); - - var endFix = makeContinueDrawingFixIfAllowed(entity.last(), 'end'); - if (endFix) fixes.push(endFix); - } - if (!fixes.length) { - fixes.push(new validationIssueFix({ - title: t('issues.fix.connect_feature.title') - })); - } - - fixes.push(new validationIssueFix({ - icon: 'iD-operation-delete', - title: t('issues.fix.delete_feature.title'), - entityIds: [entity.id], - onClick: function(context) { - var id = this.issue.entityIds[0]; - var operation = operationDelete([id], context); - if (!operation.disabled()) { - operation(); - } - } - })); - } else { - fixes.push(new validationIssueFix({ - title: t('issues.fix.connect_features.title') - })); - } - return [new validationIssue({ type: type, subtype: 'highway', @@ -69,10 +31,54 @@ export function validationDisconnectedWay() { }, reference: showReference, entityIds: Array.from(routingIslandWays).map(function(way) { return way.id; }), - fixes: fixes + dynamicFixes: makeFixes })]; + function makeFixes(context) { + + var fixes = []; + + var singleEntity = this.entityIds.length === 1 && context.hasEntity(this.entityIds[0]); + + if (singleEntity) { + + if (singleEntity.type === 'way' && !singleEntity.isClosed()) { + + var startFix = makeContinueDrawingFixIfAllowed(singleEntity.first(), 'start'); + if (startFix) fixes.push(startFix); + + var endFix = makeContinueDrawingFixIfAllowed(singleEntity.last(), 'end'); + if (endFix) fixes.push(endFix); + } + if (!fixes.length) { + fixes.push(new validationIssueFix({ + title: t('issues.fix.connect_feature.title') + })); + } + + fixes.push(new validationIssueFix({ + icon: 'iD-operation-delete', + title: t('issues.fix.delete_feature.title'), + entityIds: [singleEntity.id], + onClick: function(context) { + var id = this.issue.entityIds[0]; + var operation = operationDelete([id], context); + if (!operation.disabled()) { + operation(); + } + } + })); + } else { + fixes.push(new validationIssueFix({ + title: t('issues.fix.connect_features.title') + })); + } + + return fixes; + } + + function showReference(selection) { selection.selectAll('.issue-reference') .data([0]) diff --git a/modules/validations/help_request.js b/modules/validations/help_request.js index 5abe2b521..3ae0db3af 100644 --- a/modules/validations/help_request.js +++ b/modules/validations/help_request.js @@ -27,11 +27,13 @@ export function validationHelpRequest(context) { var entity = context.hasEntity(this.entityIds[0]); return entity ? t('issues.fixme_tag.message', { feature: utilDisplayLabel(entity, context) }) : ''; }, - fixes: [ - new validationIssueFix({ - title: t('issues.fix.address_the_concern.title') - }) - ], + dynamicFixes: function() { + return [ + new validationIssueFix({ + title: t('issues.fix.address_the_concern.title') + }) + ]; + }, reference: showReference, entityIds: [entity.id] })]; diff --git a/modules/validations/impossible_oneway.js b/modules/validations/impossible_oneway.js index 3cd6edc51..b03219c40 100644 --- a/modules/validations/impossible_oneway.js +++ b/modules/validations/impossible_oneway.js @@ -144,35 +144,6 @@ export function validationImpossibleOneway() { if (connectedEndpointsOkay) return []; } - var fixes = []; - - if (attachedOneways.length) { - fixes.push(new validationIssueFix({ - icon: 'iD-operation-reverse', - title: t('issues.fix.reverse_feature.title'), - entityIds: [way.id], - onClick: function(context) { - var id = this.issue.entityIds[0]; - context.perform(actionReverse(id), t('operations.reverse.annotation')); - } - })); - } - if (node.tags.noexit !== 'yes') { - var useLeftContinue = (isFirst && textDirection === 'ltr') || - (!isFirst && textDirection === 'rtl'); - fixes.push(new validationIssueFix({ - icon: 'iD-operation-continue' + (useLeftContinue ? '-left' : ''), - title: t('issues.fix.continue_from_' + (isFirst ? 'start' : 'end') + '.title'), - onClick: function(context) { - var entityID = this.issue.entityIds[0]; - var vertexID = this.issue.entityIds[1]; - var way = context.entity(entityID); - var vertex = context.entity(vertexID); - continueDrawing(way, vertex, context); - } - })); - } - var placement = isFirst ? 'start' : 'end', messageID = wayType + '.', referenceID = wayType + '.'; @@ -197,7 +168,39 @@ export function validationImpossibleOneway() { }, reference: getReference(referenceID), entityIds: [way.id, node.id], - fixes: fixes, + dynamicFixes: function() { + + var fixes = []; + + if (attachedOneways.length) { + fixes.push(new validationIssueFix({ + icon: 'iD-operation-reverse', + title: t('issues.fix.reverse_feature.title'), + entityIds: [way.id], + onClick: function(context) { + var id = this.issue.entityIds[0]; + context.perform(actionReverse(id), t('operations.reverse.annotation')); + } + })); + } + if (node.tags.noexit !== 'yes') { + var useLeftContinue = (isFirst && textDirection === 'ltr') || + (!isFirst && textDirection === 'rtl'); + fixes.push(new validationIssueFix({ + icon: 'iD-operation-continue' + (useLeftContinue ? '-left' : ''), + title: t('issues.fix.continue_from_' + (isFirst ? 'start' : 'end') + '.title'), + onClick: function(context) { + var entityID = this.issue.entityIds[0]; + var vertexID = this.issue.entityIds[1]; + var way = context.entity(entityID); + var vertex = context.entity(vertexID); + continueDrawing(way, vertex, context); + } + })); + } + + return fixes; + }, loc: node.loc })]; diff --git a/modules/validations/incompatible_source.js b/modules/validations/incompatible_source.js index 2ef790be6..ed1a9d3a9 100644 --- a/modules/validations/incompatible_source.js +++ b/modules/validations/incompatible_source.js @@ -40,11 +40,13 @@ export function validationIncompatibleSource() { }, reference: getReference(invalidSource.id), entityIds: [entity.id], - fixes: [ - new validationIssueFix({ - title: t('issues.fix.remove_proprietary_data.title') - }) - ] + dynamicFixes: function() { + return [ + new validationIssueFix({ + title: t('issues.fix.remove_proprietary_data.title') + }) + ]; + } })); }); diff --git a/modules/validations/mismatched_geometry.js b/modules/validations/mismatched_geometry.js index 4d2880078..a3ea1ce0a 100644 --- a/modules/validations/mismatched_geometry.js +++ b/modules/validations/mismatched_geometry.js @@ -72,37 +72,11 @@ export function validationMismatchedGeometry(context) { } } - function lineTaggedAsAreaIssue(entity, graph) { + function lineTaggedAsAreaIssue(entity) { var tagSuggestingArea = tagSuggestingLineIsArea(entity); if (!tagSuggestingArea) return null; - var fixes = []; - - var connectEndsOnClick = makeConnectEndpointsFixOnClick(entity, graph); - - fixes.push(new validationIssueFix({ - title: t('issues.fix.connect_endpoints.title'), - onClick: connectEndsOnClick - })); - - fixes.push(new validationIssueFix({ - icon: 'iD-operation-delete', - title: t('issues.fix.remove_tag.title'), - onClick: function(context) { - var entityId = this.issue.entityIds[0]; - var entity = context.entity(entityId); - var tags = Object.assign({}, entity.tags); // shallow copy - for (var key in tagSuggestingArea) { - delete tags[key]; - } - context.perform( - actionChangeTags(entityId, tags), - t('issues.fix.remove_tag.annotation') - ); - } - })); - return new validationIssue({ type: type, subtype: 'area_as_line', @@ -116,10 +90,38 @@ export function validationMismatchedGeometry(context) { }, reference: showReference, entityIds: [entity.id], - hash: JSON.stringify(tagSuggestingArea) + - // avoid stale "connect endpoints" fix - (typeof connectEndsOnClick), - fixes: fixes + hash: JSON.stringify(tagSuggestingArea), + dynamicFixes: function(context) { + + var fixes = []; + + var entity = context.entity(this.entityIds[0]); + var connectEndsOnClick = makeConnectEndpointsFixOnClick(entity, context.graph()); + + fixes.push(new validationIssueFix({ + title: t('issues.fix.connect_endpoints.title'), + onClick: connectEndsOnClick + })); + + fixes.push(new validationIssueFix({ + icon: 'iD-operation-delete', + title: t('issues.fix.remove_tag.title'), + onClick: function(context) { + var entityId = this.issue.entityIds[0]; + var entity = context.entity(entityId); + var tags = Object.assign({}, entity.tags); // shallow copy + for (var key in tagSuggestingArea) { + delete tags[key]; + } + context.perform( + actionChangeTags(entityId, tags), + t('issues.fix.remove_tag.annotation') + ); + } + })); + + return fixes; + } }); @@ -206,13 +208,15 @@ export function validationMismatchedGeometry(context) { .text(t('issues.point_as_vertex.reference')); }, entityIds: [entity.id], - fixes: [ - new validationIssueFix({ - icon: 'iD-operation-extract', - title: t('issues.fix.extract_point.title'), - onClick: extractOnClick - }) - ], + dynamicFixes: function() { + return [ + new validationIssueFix({ + icon: 'iD-operation-extract', + title: t('issues.fix.extract_point.title'), + onClick: extractOnClick + }) + ]; + }, hash: typeof extractOnClick, // avoid stale extraction fix }); } @@ -223,7 +227,7 @@ export function validationMismatchedGeometry(context) { var validation = function checkMismatchedGeometry(entity, graph) { var issues = [ vertexTaggedAsPointIssue(entity, graph), - lineTaggedAsAreaIssue(entity, graph) + lineTaggedAsAreaIssue(entity) ]; return issues.filter(Boolean); }; diff --git a/modules/validations/missing_role.js b/modules/validations/missing_role.js index 33bfe7758..093d416bb 100644 --- a/modules/validations/missing_role.js +++ b/modules/validations/missing_role.js @@ -55,20 +55,22 @@ export function validationMissingRole() { member: member }, hash: member.index.toString(), - fixes: [ - makeAddRoleFix('inner'), - makeAddRoleFix('outer'), - new validationIssueFix({ - icon: 'iD-operation-delete', - title: t('issues.fix.remove_from_relation.title'), - onClick: function(context) { - context.perform( - actionDeleteMember(this.issue.entityIds[0], this.issue.data.member.index), - t('operations.delete_member.annotation') - ); - } - }) - ] + dynamicFixes: function() { + return [ + makeAddRoleFix('inner'), + makeAddRoleFix('outer'), + new validationIssueFix({ + icon: 'iD-operation-delete', + title: t('issues.fix.remove_from_relation.title'), + onClick: function(context) { + context.perform( + actionDeleteMember(this.issue.entityIds[0], this.issue.data.member.index), + t('operations.delete_member.annotation') + ); + } + }) + ]; + } }); diff --git a/modules/validations/missing_tag.js b/modules/validations/missing_tag.js index 8b13ed39c..0d9b5ee7c 100644 --- a/modules/validations/missing_tag.js +++ b/modules/validations/missing_tag.js @@ -62,37 +62,11 @@ export function validationMissingTag() { if (!subtype) return []; - var selectFixType = subtype === 'highway_classification' ? 'select_road_type' : 'select_preset'; - - var fixes = [ - new validationIssueFix({ - icon: 'iD-icon-search', - title: t('issues.fix.' + selectFixType + '.title'), - onClick: function(context) { - context.ui().sidebar.showPresetList(); - } - }) - ]; - - // can always delete if the user created it in the first place.. - var canDelete = (entity.version === undefined || entity.v !== undefined); - fixes.push( - new validationIssueFix({ - icon: 'iD-operation-delete', - title: t('issues.fix.delete_feature.title'), - onClick: function(context) { - var id = this.issue.entityIds[0]; - var operation = operationDelete([id], context); - if (!operation.disabled()) { - operation(); - } - } - }) - ); - var messageID = subtype === 'highway_classification' ? 'unknown_road' : 'missing_tag.' + subtype; var referenceID = subtype === 'highway_classification' ? 'unknown_road' : 'missing_tag'; + // can always delete if the user created it in the first place.. + var canDelete = (entity.version === undefined || entity.v !== undefined); var severity = (canDelete && subtype !== 'highway_classification') ? 'error' : 'warning'; return [new validationIssue({ @@ -107,7 +81,44 @@ export function validationMissingTag() { }, reference: showReference, entityIds: [entity.id], - fixes: fixes + dynamicFixes: function(context) { + + var fixes = []; + + var selectFixType = subtype === 'highway_classification' ? 'select_road_type' : 'select_preset'; + + fixes.push(new validationIssueFix({ + icon: 'iD-icon-search', + title: t('issues.fix.' + selectFixType + '.title'), + onClick: function(context) { + context.ui().sidebar.showPresetList(); + } + })); + + var deleteOnClick; + + var id = this.entityIds[0]; + var operation = operationDelete([id], context); + if (!operation.disabled()) { + deleteOnClick = function(context) { + var id = this.issue.entityIds[0]; + var operation = operationDelete([id], context); + if (!operation.disabled()) { + operation(); + } + }; + } + + fixes.push( + new validationIssueFix({ + icon: 'iD-operation-delete', + title: t('issues.fix.delete_feature.title'), + onClick: deleteOnClick + }) + ); + + return fixes; + } })]; diff --git a/modules/validations/outdated_tags.js b/modules/validations/outdated_tags.js index f4a518975..79c6ed91b 100644 --- a/modules/validations/outdated_tags.js +++ b/modules/validations/outdated_tags.js @@ -144,15 +144,17 @@ export function validationOutdatedTags(context) { reference: showReference, entityIds: [entity.id], hash: JSON.stringify(tagDiff), - fixes: [ - new validationIssueFix({ - autoArgs: autoArgs, - title: t('issues.fix.upgrade_tags.title'), - onClick: function(context) { - context.perform(doUpgrade, t('issues.fix.upgrade_tags.annotation')); - } - }) - ] + dynamicFixes: function() { + return [ + new validationIssueFix({ + autoArgs: autoArgs, + title: t('issues.fix.upgrade_tags.title'), + onClick: function(context) { + context.perform(doUpgrade, t('issues.fix.upgrade_tags.annotation')); + } + }) + ]; + } })]; @@ -245,15 +247,17 @@ export function validationOutdatedTags(context) { message: showMessage, reference: showReference, entityIds: [outerWay.id, multipolygon.id], - fixes: [ - new validationIssueFix({ - autoArgs: [doUpgrade, t('issues.fix.move_tags.annotation')], - title: t('issues.fix.move_tags.title'), - onClick: function(context) { - context.perform(doUpgrade, t('issues.fix.move_tags.annotation')); - } - }) - ] + dynamicFixes: function() { + return [ + new validationIssueFix({ + autoArgs: [doUpgrade, t('issues.fix.move_tags.annotation')], + title: t('issues.fix.move_tags.title'), + onClick: function(context) { + context.perform(doUpgrade, t('issues.fix.move_tags.annotation')); + } + }) + ]; + } })]; diff --git a/modules/validations/private_data.js b/modules/validations/private_data.js index 3317ea991..03f3c57b9 100644 --- a/modules/validations/private_data.js +++ b/modules/validations/private_data.js @@ -63,15 +63,17 @@ export function validationPrivateData() { message: showMessage, reference: showReference, entityIds: [entity.id], - fixes: [ - new validationIssueFix({ - icon: 'iD-operation-delete', - title: t('issues.fix.' + fixID + '.title'), - onClick: function(context) { - context.perform(doUpgrade, t('issues.fix.upgrade_tags.annotation')); - } - }) - ] + dynamicFixes: function() { + return [ + new validationIssueFix({ + icon: 'iD-operation-delete', + title: t('issues.fix.' + fixID + '.title'), + onClick: function(context) { + context.perform(doUpgrade, t('issues.fix.upgrade_tags.annotation')); + } + }) + ]; + } })]; diff --git a/modules/validations/suspicious_name.js b/modules/validations/suspicious_name.js index 33153f5a4..28b061782 100644 --- a/modules/validations/suspicious_name.js +++ b/modules/validations/suspicious_name.js @@ -111,22 +111,24 @@ export function validationSuspiciousName() { reference: showReference, entityIds: [entityId], hash: nameKey + '=' + incorrectName, - fixes: [ - new validationIssueFix({ - icon: 'iD-operation-delete', - title: t('issues.fix.remove_the_name.title'), - onClick: function(context) { - var entityId = this.issue.entityIds[0]; - var entity = context.entity(entityId); - var tags = Object.assign({}, entity.tags); // shallow copy - delete tags[nameKey]; - context.perform( - actionChangeTags(entityId, tags), - t('issues.fix.remove_mistaken_name.annotation') - ); - } - }) - ] + dynamicFixes: function() { + return [ + new validationIssueFix({ + icon: 'iD-operation-delete', + title: t('issues.fix.remove_the_name.title'), + onClick: function(context) { + var entityId = this.issue.entityIds[0]; + var entity = context.entity(entityId); + var 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) { diff --git a/modules/validations/unsquare_way.js b/modules/validations/unsquare_way.js index 2f3e0fc43..a4331458a 100644 --- a/modules/validations/unsquare_way.js +++ b/modules/validations/unsquare_way.js @@ -80,38 +80,40 @@ export function validationUnsquareWay(context) { reference: showReference, entityIds: [entity.id], hash: JSON.stringify(autoArgs !== undefined) + degreeThreshold, - fixes: [ - new validationIssueFix({ - icon: 'iD-operation-orthogonalize', - title: t('issues.fix.square_feature.title'), - autoArgs: autoArgs, - onClick: function(context, completionHandler) { - var entityId = this.issue.entityIds[0]; - // use same degree threshold as for detection - context.perform( - actionOrthogonalize(entityId, context.projection, undefined, degreeThreshold), - t('operations.orthogonalize.annotation.area') - ); - // run after the squaring transition (currently 150ms) - window.setTimeout(function() { completionHandler(); }, 175); - } - }), - /* - new validationIssueFix({ - title: t('issues.fix.tag_as_unsquare.title'), - onClick: function(context) { - var entityId = this.issue.entityIds[0]; - var entity = context.entity(entityId); - var tags = Object.assign({}, entity.tags); // shallow copy - tags.nonsquare = 'yes'; - context.perform( - actionChangeTags(entityId, tags), - t('issues.fix.tag_as_unsquare.annotation') - ); - } - }) - */ - ] + dynamicFixes: function() { + return [ + new validationIssueFix({ + icon: 'iD-operation-orthogonalize', + title: t('issues.fix.square_feature.title'), + autoArgs: autoArgs, + onClick: function(context, completionHandler) { + var entityId = this.issue.entityIds[0]; + // use same degree threshold as for detection + context.perform( + actionOrthogonalize(entityId, context.projection, undefined, degreeThreshold), + t('operations.orthogonalize.annotation.area') + ); + // run after the squaring transition (currently 150ms) + window.setTimeout(function() { completionHandler(); }, 175); + } + }), + /* + new validationIssueFix({ + title: t('issues.fix.tag_as_unsquare.title'), + onClick: function(context) { + var entityId = this.issue.entityIds[0]; + var entity = context.entity(entityId); + var tags = Object.assign({}, entity.tags); // shallow copy + tags.nonsquare = 'yes'; + context.perform( + actionChangeTags(entityId, tags), + t('issues.fix.tag_as_unsquare.annotation') + ); + } + }) + */ + ]; + } })]; function showReference(selection) { diff --git a/test/spec/validations/almost_junction.js b/test/spec/validations/almost_junction.js index d8feec1ae..378665029 100644 --- a/test/spec/validations/almost_junction.js +++ b/test/spec/validations/almost_junction.js @@ -166,8 +166,8 @@ describe('iD.validations.almost_junction', function () { expect(issue.data.cross_loc[0]).to.eql(22.42356); expect(issue.data.cross_loc[1]).to.eql(0); - expect(issue.fixes).to.have.lengthOf(2); - issue.fixes[0].onClick(context); + expect(issue.fixes(context)).to.have.lengthOf(3); + issue.fixes(context)[0].onClick(context); issues = validate(); expect(issues).to.have.lengthOf(0); }); @@ -196,8 +196,8 @@ describe('iD.validations.almost_junction', function () { expect(issue.data.cross_loc[0]).to.eql(22.42356); expect(issue.data.cross_loc[1]).to.eql(0); - expect(issue.fixes).to.have.lengthOf(2); - issue.fixes[1].onClick(context); + expect(issue.fixes(context)).to.have.lengthOf(3); + issue.fixes(context)[1].onClick(context); issues = validate(); expect(issues).to.have.lengthOf(0); });