From 07a53fe6ea785c62ee2cedbf9fa8454ec7861fa4 Mon Sep 17 00:00:00 2001 From: Xiaoming Gao Date: Tue, 18 Dec 2018 20:03:57 -0500 Subject: [PATCH] Extend data model for validation issues Add the Issues pane 1. Add a class to represent the validation issue 2. Extend the data model for an validation issue to (1) add a severity level field (useful for identify save-blocking issues later) (2) replace single entity with an array of entities (useful for issues involving multiple entities) (3) add a coordinates field for highlighting the location of the issue on the map (4) add a fixes field for possible automatic fixes 3. Update existing validation modules to use the new data model --- dist/locales/en.json | 62 +++++++++++++++++++-- modules/core/history.js | 9 ++- modules/services/maprules.js | 17 +++--- modules/ui/commit_warnings.js | 35 +++++++----- modules/ui/issues.js | 16 ++++-- modules/validations/deprecated_tag.js | 18 ++++-- modules/validations/disconnected_highway.js | 21 ++++--- modules/validations/index.js | 1 + modules/validations/many_deletions.js | 23 +++++--- modules/validations/mapcss_checks.js | 6 +- modules/validations/missing_tag.js | 19 ++++--- modules/validations/old_multipolygon.js | 19 ++++--- modules/validations/tag_suggests_area.js | 18 ++++-- modules/validations/validation_issue.js | 41 ++++++++++++++ 14 files changed, 225 insertions(+), 80 deletions(-) create mode 100644 modules/validations/validation_issue.js diff --git a/dist/locales/en.json b/dist/locales/en.json index 0a04218ef..8434bdd1a 100644 --- a/dist/locales/en.json +++ b/dist/locales/en.json @@ -7579,9 +7579,6 @@ "SPW_PICC": { "name": "SPW(allonie) PICC numerical imagery" }, - "US-TIGER-Roads-2012": { - "name": "TIGER Roads 2012" - }, "US-TIGER-Roads-2014": { "description": "At zoom level 16+, public domain map data from the US Census. At lower zooms, only changes since 2006 minus changes already incorporated into OpenStreetMap", "name": "TIGER Roads 2014" @@ -7590,6 +7587,10 @@ "description": "Yellow = Public domain map data from the US Census. Red = Data not found in OpenStreetMap", "name": "TIGER Roads 2017" }, + "US-TIGER-Roads-2018": { + "description": "Yellow = Public domain map data from the US Census. Red = Data not found in OpenStreetMap", + "name": "TIGER Roads 2018" + }, "US_Forest_Service_roads_overlay": { "description": "Highway: Green casing = unclassified. Brown casing = track. Surface: gravel = light brown fill, Asphalt = black, paved = gray, ground =white, concrete = blue, grass = green. Seasonal = white bars", "name": "U.S. Forest Roads Overlay" @@ -7606,6 +7607,12 @@ }, "name": "UrbIS-Ortho 2017" }, + "UrbISOrtho2018": { + "attribution": { + "text": "Realized by means of Brussels UrbIS®© - Distribution & Copyright CIRB" + }, + "name": "UrbIS-Ortho 2018" + }, "UrbisAdmFR": { "attribution": { "text": "Realized by means of Brussels UrbIS®© - Distribution & Copyright CIRB" @@ -7690,11 +7697,33 @@ "description": "Japan GSI Standard Map. Widely covered.", "name": "Japan GSI Standard Map" }, - "hike_n_bike": { + "helsingborg-orto": { "attribution": { - "text": "© OpenStreetMap contributors" + "text": "© Helsingborg municipality" }, - "name": "Hike & Bike" + "description": "Orthophotos from the municipality of Helsingborg 2016, public domain", + "name": "Helsingborg Orthophoto" + }, + "kalmar-orto-2014": { + "attribution": { + "text": "© Kalmar municipality" + }, + "description": "Orthophotos for the north coast of the municipality of Kalmar 2014", + "name": "Kalmar North Orthophoto 2014" + }, + "kalmar-orto-2016": { + "attribution": { + "text": "© Kalmar municipality" + }, + "description": "Orthophotos for the south coast of the municipality of Kalmar 2016", + "name": "Kalmar South Orthophoto 2016" + }, + "kalmar-orto-2018": { + "attribution": { + "text": "© Kalmar municipality" + }, + "description": "Orthophotos for urban areas of the municipality of Kalmar 2018", + "name": "Kalmar Urban Orthophoto 2018" }, "kelkkareitit": { "attribution": { @@ -7717,6 +7746,20 @@ "description": "Mosaic of Swedish orthophotos from the period 1970–1980. Is under construction.", "name": "Lantmäteriet Historic Orthophoto 1975" }, + "lantmateriet-topowebb": { + "attribution": { + "text": "© Lantmäteriet, CC0" + }, + "description": "Topographic map of Sweden 1:50 000", + "name": "Lantmäteriet Topographic Map" + }, + "linkoping-orto": { + "attribution": { + "text": "© Linköping municipality" + }, + "description": "Orthophotos from the municipality of Linköping 2010, open data", + "name": "Linköping Orthophoto" + }, "mapbox_locator_overlay": { "attribution": { "text": "Terms & Feedback" @@ -7781,6 +7824,13 @@ }, "name": "Stamen Terrain" }, + "stockholm-orto": { + "attribution": { + "text": "© Stockholm municipality, CC0" + }, + "description": "Orthophotos from the municipality of Stockholm 2015, CC0 license", + "name": "Stockholm Orthophoto" + }, "tf-cycle": { "attribution": { "text": "Maps © Thunderforest, Data © OpenStreetMap contributors" diff --git a/modules/core/history.js b/modules/core/history.js index 2699b9200..7db328d11 100644 --- a/modules/core/history.js +++ b/modules/core/history.js @@ -281,9 +281,12 @@ export function coreHistory(context) { validate: function(changes) { - return _flatten(_map(Validations, function(fn) { - return fn(context)(changes, _stack[_index].graph); - })); + return _flatten(_map( + _filter(Validations, _isFunction), + function(fn) { + return fn(context)(changes, _stack[_index].graph); + } + )); }, diff --git a/modules/services/maprules.js b/modules/services/maprules.js index 9e2f44a4f..f3b34450d 100644 --- a/modules/services/maprules.js +++ b/modules/services/maprules.js @@ -206,14 +206,17 @@ export default { } }, // when geometries match and tag matches are present, return a warning... - findWarnings: function (entity, graph, warnings) { + findIssues: function (entity, graph, issues) { if (this.geometryMatches(entity, graph) && this.matches(entity)) { - var type = Object.keys(selector).indexOf('error') > -1 ? 'error' : 'warning'; - warnings.push({ - severity: type, - message: selector[type], - entity: entity - }); + var severity = Object.keys(selector).indexOf('error') > -1 + ? ValidationIssueSeverity.error + : ValidationIssueSeverity.warning; + issues.push(new validationIssue({ + type: ValidationIssueType.map_rule_issue, + severity: severity, + message: selector[severity], + entities: [entity], + })); } } }; diff --git a/modules/ui/commit_warnings.js b/modules/ui/commit_warnings.js index 29ecf44a1..537288ea3 100644 --- a/modules/ui/commit_warnings.js +++ b/modules/ui/commit_warnings.js @@ -1,3 +1,5 @@ +import _map from 'lodash-es/map'; + import { t } from '../util/locale'; import { modeSelect } from '../modes'; import { svgIcon } from '../svg'; @@ -11,10 +13,9 @@ export function uiCommitWarnings(context) { function commitWarnings(selection) { - // maybe call these issues now? - var validations = context.issueManager().validate(); + var issues = context.issueManager().validate(); - validations = _reduce(validations, function(validations, val) { + validations = _reduce(issues, function(validations, val) { var severity = val.severity; if (validations.hasOwnProperty(severity)) { validations[severity].push(val); @@ -24,10 +25,10 @@ export function uiCommitWarnings(context) { return validations; }, {}); - _forEach(validations, function(instances, type) { + _forEach(validations, function(instances, severity) { instances = _uniqBy(instances, function(val) { return val.id + '_' + val.message.replace(/\s+/g,''); }); - var section = type + '-section'; - var instanceItem = type + '-item'; + var section = severity + '-section'; + var instanceItem = severity + '-item'; var container = selection.selectAll('.' + section) .data(instances.length ? [0] : []); @@ -41,7 +42,7 @@ export function uiCommitWarnings(context) { containerEnter .append('h3') - .text(type === 'warning' ? t('commit.warnings') : t('commit.errors')); + .text(severity === 'warning' ? t('commit.warnings') : t('commit.errors')); containerEnter .append('ul') @@ -77,31 +78,35 @@ export function uiCommitWarnings(context) { items = itemsEnter .merge(items); + items .on('mouseover', mouseover) .on('mouseout', mouseout) .on('click', warningClick); - function mouseover(d) { - if (d.entity) { + if (d.entities) { context.surface().selectAll( - utilEntityOrMemberSelector([d.entity.id], context.graph()) + utilEntityOrMemberSelector( + _map(d.entities, function(e) { return e.id; }), + context.graph() + ) ).classed('hover', true); } } - function mouseout() { context.surface().selectAll('.hover') .classed('hover', false); } - function warningClick(d) { - if (d.entity) { - context.map().zoomTo(d.entity); - context.enter(modeSelect(context, [d.entity.id])); + if (d.entities) { + context.map().zoomTo(d.entities[0]); + context.enter(modeSelect( + context, + _map(d.entities, function(e) { return e.id; }), + )); } } }); diff --git a/modules/ui/issues.js b/modules/ui/issues.js index 1660aeba5..531bec958 100644 --- a/modules/ui/issues.js +++ b/modules/ui/issues.js @@ -1,3 +1,4 @@ +import _map from 'lodash-es/map'; import { event as d3_event, select as d3_select @@ -104,9 +105,9 @@ export function uiIssues(context) { var name = 'issues_list'; var changes = context.history().changes(); - var validations = context.history().validate(changes); + var issues = context.history().validate(changes); - /*validations = _reduce(validations, function(validations, val) { + /*validations = _reduce(issues, function(validations, val) { var severity = val.severity; if (validations.hasOwnProperty(severity)) { validations[severity].push(val); @@ -117,7 +118,7 @@ export function uiIssues(context) { }, {});*/ var items = selection.selectAll('li') - .data(validations); + .data(issues); // Exit items.exit() @@ -130,13 +131,18 @@ export function uiIssues(context) { .call(tooltip() .html(true) .title(function(d) { - var tip = d.tooltip; + var tip = d.tooltip ? d.tooltip : ''; return uiTooltipHtml(tip); }) .placement('bottom') ) .on('click', function(d) { - context.enter(modeSelect(context, [d.entity.id])); + if (d.entities) { + context.enter(modeSelect( + context, + _map(d.entities, function(e) { return e.id; }) + )); + } }); var label = enter diff --git a/modules/validations/deprecated_tag.js b/modules/validations/deprecated_tag.js index 3690e714e..50f8606c0 100644 --- a/modules/validations/deprecated_tag.js +++ b/modules/validations/deprecated_tag.js @@ -2,27 +2,33 @@ import _isEmpty from 'lodash-es/isEmpty'; import { t } from '../util/locale'; import { utilTagText } from '../util/index'; +import { + ValidationIssueType, + ValidationIssueSeverity, + validationIssue, +} from './validation_issue'; export function validationDeprecatedTag() { var validation = function(changes) { - var warnings = []; + var issues = []; for (var i = 0; i < changes.created.length; i++) { var change = changes.created[i], deprecatedTags = change.deprecatedTags(); if (!_isEmpty(deprecatedTags)) { var tags = utilTagText({ tags: deprecatedTags }); - warnings.push({ - id: 'deprecated_tags', + issues.push(new validationIssue({ + type: ValidationIssueType.deprecated_tags, + severity: ValidationIssueSeverity.warning, message: t('validations.deprecated_tags', { tags: tags }), - entity: change - }); + entities: [change], + })); } } - return warnings; + return issues; }; diff --git a/modules/validations/disconnected_highway.js b/modules/validations/disconnected_highway.js index cc07dba85..1691c1755 100644 --- a/modules/validations/disconnected_highway.js +++ b/modules/validations/disconnected_highway.js @@ -1,5 +1,10 @@ import { t } from '../util/locale'; import { utilDisplayName } from '../util'; +import { + ValidationIssueType, + ValidationIssueSeverity, + validationIssue, +} from './validation_issue'; export function validationDisconnectedHighway(context) { @@ -24,7 +29,7 @@ export function validationDisconnectedHighway(context) { var validation = function(changes, graph) { - var warnings = []; + var issues = []; for (var i = 0; i < changes.created.length; i++) { var entity = changes.created[i]; @@ -38,16 +43,18 @@ export function validationDisconnectedHighway(context) { entityLabel = utilDisplayType(entity.id) } } - warnings.push({ - id: 'disconnected_highway', - message: t('validations.disconnected_highway', {entityLabel: entityLabel}), + + issues.push(new validationIssue({ + type: ValidationIssueType.disconnected_highway, + severity: ValidationIssueSeverity.error, + message: t('validations.disconnected_highway'), tooltip: t('validations.disconnected_highway_tooltip'), - entity: entity - }); + entities: [entity], + })); } } - return warnings; + return issues; }; diff --git a/modules/validations/index.js b/modules/validations/index.js index 112b38e6c..ead99f117 100644 --- a/modules/validations/index.js +++ b/modules/validations/index.js @@ -1,5 +1,6 @@ export { validationDeprecatedTag } from './deprecated_tag'; export { validationDisconnectedHighway } from './disconnected_highway'; +export { ValidationIssueType, ValidationIssueSeverity } from './validation_issue'; export { validationManyDeletions } from './many_deletions'; export { validationMapCSSChecks } from './mapcss_checks'; export { validationMissingTag } from './missing_tag'; diff --git a/modules/validations/many_deletions.js b/modules/validations/many_deletions.js index b0b0c437e..9624c2baa 100644 --- a/modules/validations/many_deletions.js +++ b/modules/validations/many_deletions.js @@ -1,11 +1,15 @@ import { t } from '../util/locale'; - +import { + ValidationIssueType, + ValidationIssueSeverity, + validationIssue, +} from './validation_issue'; export function validationManyDeletions() { var threshold = 100; var validation = function(changes, graph) { - var warnings = []; + var issues = []; var nodes=0, ways=0, areas=0, relations=0; changes.deleted.forEach(function(c) { @@ -15,14 +19,17 @@ export function validationManyDeletions() { else if (c.type === 'relation') {relations++;} }); if (changes.deleted.length > threshold) { - warnings.push({ - id: 'many_deletions', - message: t('validations.many_deletions', - { n: changes.deleted.length, p: nodes, l: ways, a:areas, r: relations }) - }); + issues.push(new validationIssue({ + type: ValidationIssueType.many_deletions, + severity: ValidationIssueSeverity.warning, + message: t( + 'validations.many_deletions', + { n: changes.deleted.length, p: nodes, l: ways, a:areas, r: relations } + ), + })); } - return warnings; + return issues; }; diff --git a/modules/validations/mapcss_checks.js b/modules/validations/mapcss_checks.js index 13fa6fe2b..6804279da 100644 --- a/modules/validations/mapcss_checks.js +++ b/modules/validations/mapcss_checks.js @@ -5,7 +5,7 @@ export function validationMapCSSChecks() { if (!services.maprules) return []; var rules = services.maprules.validationRules(); - var warnings = []; + var issues = []; var createdModified = ['created', 'modified']; for (var i = 0; i < rules.length; i++) { @@ -14,12 +14,12 @@ export function validationMapCSSChecks() { var type = createdModified[j]; var entities = changes[type]; for (var k = 0; k < entities.length; k++) { - rule.findWarnings(entities[k], graph, warnings); + rule.findIssues(entities[k], graph, issues); } } } - return warnings; + return issues; }; return validation; } diff --git a/modules/validations/missing_tag.js b/modules/validations/missing_tag.js index 6785ab247..53f977e3b 100644 --- a/modules/validations/missing_tag.js +++ b/modules/validations/missing_tag.js @@ -1,6 +1,10 @@ import _without from 'lodash-es/without'; import { t } from '../util/locale'; - +import { + ValidationIssueType, + ValidationIssueSeverity, + validationIssue, +} from './validation_issue'; export function validationMissingTag() { @@ -12,23 +16,24 @@ export function validationMissingTag() { var validation = function(changes, graph) { var types = ['point', 'line', 'area', 'relation'], - warnings = []; + issues = []; for (var i = 0; i < changes.created.length; i++) { var change = changes.created[i], geometry = change.geometry(graph); if (types.indexOf(geometry) !== -1 && !hasTags(change, graph)) { - warnings.push({ - id: 'missing_tag', + issues.push(new validationIssue({ + type: ValidationIssueType.missing_tag, + severity: ValidationIssueSeverity.error, message: t('validations.untagged_' + geometry), tooltip: t('validations.untagged_' + geometry + '_tooltip'), - entity: change - }); + entities: [change], + })); } } - return warnings; + return issues; }; diff --git a/modules/validations/old_multipolygon.js b/modules/validations/old_multipolygon.js index 6e6c640b8..e1b2da7be 100644 --- a/modules/validations/old_multipolygon.js +++ b/modules/validations/old_multipolygon.js @@ -1,23 +1,28 @@ import { t } from '../util/locale'; import { osmIsSimpleMultipolygonOuterMember } from '../osm'; - +import { + ValidationIssueType, + ValidationIssueSeverity, + validationIssue, +} from './validation_issue'; export function validationOldMultipolygon() { return function validation(changes, graph) { - var warnings = []; + var issues = []; for (var i = 0; i < changes.created.length; i++) { var entity = changes.created[i]; var parent = osmIsSimpleMultipolygonOuterMember(entity, graph); if (parent) { - warnings.push({ - id: 'old_multipolygon', + issues.push(new validationIssue({ + type: ValidationIssueType.old_multipolygon, + severity: ValidationIssueSeverity.warning, message: t('validations.old_multipolygon'), tooltip: t('validations.old_multipolygon_tooltip'), - entity: parent - }); + entities: [parent], + })); } } - return warnings; + return issues; }; } diff --git a/modules/validations/tag_suggests_area.js b/modules/validations/tag_suggests_area.js index a631f4a79..4ef06f39c 100644 --- a/modules/validations/tag_suggests_area.js +++ b/modules/validations/tag_suggests_area.js @@ -1,5 +1,10 @@ import _isEmpty from 'lodash-es/isEmpty'; import { t } from '../util/locale'; +import { + ValidationIssueType, + ValidationIssueSeverity, + validationIssue, +} from './validation_issue'; // https://github.com/openstreetmap/josm/blob/mirror/src/org/ @@ -25,22 +30,23 @@ export function validationTagSuggestsArea() { var validation = function(changes, graph) { - var warnings = []; + var issues = []; for (var i = 0; i < changes.created.length; i++) { var change = changes.created[i], geometry = change.geometry(graph), suggestion = (geometry === 'line' ? tagSuggestsArea(change.tags) : undefined); if (suggestion) { - warnings.push({ - id: 'tag_suggests_area', + issues.push(new validationIssue({ + type: ValidationIssueType.tag_suggests_area, + severity: ValidationIssueSeverity.warning, message: t('validations.tag_suggests_area', { tag: suggestion }), - entity: change - }); + entities: [change], + })); } } - return warnings; + return issues; }; diff --git a/modules/validations/validation_issue.js b/modules/validations/validation_issue.js new file mode 100644 index 000000000..c671a0bd3 --- /dev/null +++ b/modules/validations/validation_issue.js @@ -0,0 +1,41 @@ +import _isObject from 'lodash-es/isObject'; + + +var ValidationIssueType = Object.freeze({ + deprecated_tags: 'deprecated_tags', + disconnected_highway: 'disconnected_highway', + many_deletions: 'many_deletions', + missing_tag: 'missing_tag', + old_multipolygon: 'old_multipolygon', + tag_suggests_area: 'tag_suggests_area', + map_rule_issue: 'map_rule_issue', +}); + + +var ValidationIssueSeverity = Object.freeze({ + warning: 'warning', + error: 'error', +}); + + +export { ValidationIssueType, ValidationIssueSeverity }; + + +export function validationIssue(attrs) { + if (!_isObject(attrs)) throw new Error('Input attrs is not an object'); + if (!attrs.type || !ValidationIssueType.hasOwnProperty(attrs.type)) { + throw new Error('Invalid attrs.type: ' + attrs.type); + } + if (!attrs.severity || !ValidationIssueSeverity.hasOwnProperty(attrs.severity)) { + throw new Error('Invalid attrs.severity: ' + attr.severity); + } + if (!attrs.message) throw new Error('attrs.message is empty'); + + this.type = attrs.type; + this.severity = attrs.severity; + this.message = attrs.message; + this.tooltip = attrs.tooltip; + this.entities = attrs.entities; // expect an array of entities + this.coordinates = attrs.coordinates; // expect an array of [lon, lat] + this.fixes = attrs.fixes; // expect an array of functions for possible fixes +}