From 9785021f44d960210b8055ba4724090280780269 Mon Sep 17 00:00:00 2001 From: Quincy Morgan Date: Fri, 1 Feb 2019 15:19:28 -0500 Subject: [PATCH] Rename simpleMultipolygon functions to oldMultipolygon Return old_multipolygon validation issues for the relations as well as the outer ways Remove console logging Don't run missing_tag validation for relations with old_multipolygon issues --- modules/actions/split.js | 4 +-- modules/core/validator.js | 34 ++++++++++---------- modules/osm/index.js | 5 +-- modules/osm/multipolygon.js | 39 +++++++++++++++++++++-- modules/svg/areas.js | 4 +-- modules/svg/lines.js | 4 +-- modules/validations/old_multipolygon.js | 27 ++++++++++++---- test/spec/osm/multipolygon.js | 42 ++++++++++++------------- 8 files changed, 106 insertions(+), 53 deletions(-) diff --git a/modules/actions/split.js b/modules/actions/split.js index 54c41f380..ad19e2457 100644 --- a/modules/actions/split.js +++ b/modules/actions/split.js @@ -7,7 +7,7 @@ import { actionAddMember } from './add_member'; import { geoSphericalDistance } from '../geo'; import { - osmIsSimpleMultipolygonOuterMember, + osmIsOldMultipolygonOuterMember, osmRelation, osmWay } from '../osm'; @@ -93,7 +93,7 @@ export function actionSplit(nodeId, newWayIds) { var nodesA; var nodesB; var isArea = wayA.isArea(); - var isOuter = osmIsSimpleMultipolygonOuterMember(wayA, graph); + var isOuter = osmIsOldMultipolygonOuterMember(wayA, graph); if (wayA.isClosed()) { var nodes = wayA.nodes.slice(0, -1); diff --git a/modules/core/validator.js b/modules/core/validator.js index b1bc9611b..98f74306d 100644 --- a/modules/core/validator.js +++ b/modules/core/validator.js @@ -1,6 +1,5 @@ import { dispatch as d3_dispatch } from 'd3-dispatch'; -import _isObject from 'lodash-es/isObject'; import _isFunction from 'lodash-es/isFunction'; import _map from 'lodash-es/map'; import _filter from 'lodash-es/filter'; @@ -21,9 +20,6 @@ export function coreValidator(context) { var validations = _filter(Validations, _isFunction).reduce(function(obj, validation) { var func = validation(); - if (!func.type) { - console.log('Validation rule not found for: ' + validation); - } obj[func.type] = func; return obj; }, {}); @@ -84,12 +80,23 @@ export function coreValidator(context) { var ranValidations = new Set([]); // runs validation and appends resulting issues, returning true if validation passed function runValidation(type) { - var fn = validations[type]; - var typeIssues = fn(entity, context); - issues = issues.concat(typeIssues); - ranValidations.add(type); - return typeIssues.length === 0; + if (!ranValidations.has(type)) { + var fn = validations[type]; + var typeIssues = fn(entity, context); + issues = issues.concat(typeIssues); + ranValidations.add(type); + return typeIssues.length === 0; + } + return true; } + + if (entity.type === 'relation') { + if (!runValidation('old_multipolygon')) { + // don't flag missing tags if they are on the outer way + ranValidations.add('missing_tag'); + } + } + // other validations require feature to be tagged if (!runValidation('missing_tag')) return issues; if (entity.type === 'way') { @@ -103,10 +110,8 @@ export function coreValidator(context) { runValidation('tag_suggests_area'); } // run all validations not yet run manually - entityValidationIds.forEach(function(ruleId) { - if (!ranValidations.has(ruleId)) { - runValidation(ruleId); - } + entityValidationIds.forEach(function(id) { + runValidation(id); }); return issues; } @@ -184,9 +189,6 @@ export function validationIssue(attrs) { return id; }; - if (!_isObject(attrs)) console.log('Input attrs is not an object'); - if (!attrs.message) console.log('attrs.message is empty'); - this.type = attrs.type; this.severity = attrs.severity; this.message = attrs.message; diff --git a/modules/osm/index.js b/modules/osm/index.js index f8d7addc1..3a9783b1e 100644 --- a/modules/osm/index.js +++ b/modules/osm/index.js @@ -17,8 +17,9 @@ export { } from './lanes'; export { - osmIsSimpleMultipolygonOuterMember, - osmSimpleMultipolygonOuterMember, + osmOldMultipolygonOuterMemberOfRelation, + osmIsOldMultipolygonOuterMember, + osmOldMultipolygonOuterMember, osmJoinWays } from './multipolygon'; diff --git a/modules/osm/multipolygon.js b/modules/osm/multipolygon.js index 966e0b774..38bb37353 100644 --- a/modules/osm/multipolygon.js +++ b/modules/osm/multipolygon.js @@ -3,9 +3,44 @@ import { osmIsInterestingTag } from './tags'; import { osmWay } from './way'; +// "Old" multipolyons, previously known as "simple" multipolygons, are as follows: +// +// 1. Relation tagged with `type=multipolygon` and no interesting tags. +// 2. One and only one member with the `outer` role. Must be a way with interesting tags. +// 3. No members without a role. +// +// Old multipolygons are no longer recommended but are still rendered as areas by iD. + +export function osmOldMultipolygonOuterMemberOfRelation(entity, graph) { + if (entity.type !== 'relation' || + !entity.isMultipolygon() + || Object.keys(entity.tags).filter(osmIsInterestingTag).length > 1) { + return false; + } + + var outerMember; + for (var memberIndex in entity.members) { + var member = entity.members[memberIndex]; + if (!member.role) return false; + if (member.role === 'outer') { + if (outerMember) return false; + if (member.type !== 'way') return false; + if (!graph.hasEntity(member.id)) return false; + + outerMember = graph.entity(member.id); + + if (Object.keys(outerMember.tags).filter(osmIsInterestingTag).length === 0) { + return false; + } + } + } + + return outerMember; +} + // For fixing up rendering of multipolygons with tags on the outer member. // https://github.com/openstreetmap/iD/issues/613 -export function osmIsSimpleMultipolygonOuterMember(entity, graph) { +export function osmIsOldMultipolygonOuterMember(entity, graph) { if (entity.type !== 'way' || Object.keys(entity.tags).filter(osmIsInterestingTag).length === 0) return false; @@ -30,7 +65,7 @@ export function osmIsSimpleMultipolygonOuterMember(entity, graph) { } -export function osmSimpleMultipolygonOuterMember(entity, graph) { +export function osmOldMultipolygonOuterMember(entity, graph) { if (entity.type !== 'way') return false; diff --git a/modules/svg/areas.js b/modules/svg/areas.js index 8a0ef97e1..1cfbae252 100644 --- a/modules/svg/areas.js +++ b/modules/svg/areas.js @@ -3,7 +3,7 @@ import _values from 'lodash-es/values'; import { bisector as d3_bisector } from 'd3-array'; -import { osmEntity, osmIsSimpleMultipolygonOuterMember } from '../osm'; +import { osmEntity, osmIsOldMultipolygonOuterMember } from '../osm'; import { svgPath, svgSegmentWay, svgTagClasses } from './index'; @@ -200,7 +200,7 @@ export function svgAreas(projection, context) { var entity = entities[i]; if (entity.geometry(graph) !== 'area') continue; - multipolygon = osmIsSimpleMultipolygonOuterMember(entity, graph); + multipolygon = osmIsOldMultipolygonOuterMember(entity, graph); if (multipolygon) { areas[multipolygon.id] = { entity: multipolygon.mergeTags(entity.tags), diff --git a/modules/svg/lines.js b/modules/svg/lines.js index 6300f23ca..7834f276b 100644 --- a/modules/svg/lines.js +++ b/modules/svg/lines.js @@ -14,7 +14,7 @@ import { svgTagClasses } from './index'; -import { osmEntity, osmSimpleMultipolygonOuterMember } from '../osm'; +import { osmEntity, osmOldMultipolygonOuterMember } from '../osm'; import { utilDetect } from '../util/detect'; @@ -191,7 +191,7 @@ export function svgLines(projection, context) { for (var i = 0; i < entities.length; i++) { var entity = entities[i]; - var outer = osmSimpleMultipolygonOuterMember(entity, graph); + var outer = osmOldMultipolygonOuterMember(entity, graph); if (outer) { ways.push(entity.mergeTags(outer.tags)); oldMultiPolygonOuters[outer.id] = true; diff --git a/modules/validations/old_multipolygon.js b/modules/validations/old_multipolygon.js index 510afa6f3..0ae3d9512 100644 --- a/modules/validations/old_multipolygon.js +++ b/modules/validations/old_multipolygon.js @@ -1,5 +1,8 @@ import { t } from '../util/locale'; -import { osmIsSimpleMultipolygonOuterMember } from '../osm'; +import { + osmIsOldMultipolygonOuterMember, + osmOldMultipolygonOuterMemberOfRelation +} from '../osm'; import { utilDisplayLabel } from '../util'; import { validationIssue, @@ -12,17 +15,29 @@ import { export function validationOldMultipolygon() { var validation = function(entity, context) { + var issues = []; var graph = context.graph(); - var mistaggedMultipolygon = osmIsSimpleMultipolygonOuterMember(entity, graph); - if (mistaggedMultipolygon) { - var multipolygonLabel = utilDisplayLabel(mistaggedMultipolygon, context); + + var multipolygon, outerWay; + if (entity.type === 'relation') { + outerWay = osmOldMultipolygonOuterMemberOfRelation(entity, graph); + multipolygon = entity; + } else if (entity.type === 'way') { + multipolygon = osmIsOldMultipolygonOuterMember(entity, graph); + outerWay = entity; + } else { + return issues; + } + + if (multipolygon && outerWay) { + var multipolygonLabel = utilDisplayLabel(multipolygon, context); issues.push(new validationIssue({ type: 'old_multipolygon', severity: 'warning', - message: t('issues.old_multipolygon.message', {multipolygon: multipolygonLabel}), + message: t('issues.old_multipolygon.message', { multipolygon: multipolygonLabel }), tooltip: t('issues.old_multipolygon.tip'), - entities: [entity, mistaggedMultipolygon], + entities: [outerWay, multipolygon], fixes: [ new validationIssueFix({ title: t('issues.fix.move_tags.title'), diff --git a/test/spec/osm/multipolygon.js b/test/spec/osm/multipolygon.js index b5f4b78c2..646356954 100644 --- a/test/spec/osm/multipolygon.js +++ b/test/spec/osm/multipolygon.js @@ -1,11 +1,11 @@ -describe('iD.osmIsSimpleMultipolygonOuterMember', function() { +describe('iD.osmIsOldMultipolygonOuterMember', function() { it('returns the parent relation of a simple multipolygon outer', function() { var outer = iD.osmWay({tags: {'natural':'wood'}}); var relation = iD.osmRelation( {tags: {type: 'multipolygon'}, members: [{id: outer.id, role: 'outer'}]} ); var graph = iD.coreGraph([outer, relation]); - expect(iD.osmIsSimpleMultipolygonOuterMember(outer, graph)).to.equal(relation); + expect(iD.osmIsOldMultipolygonOuterMember(outer, graph)).to.equal(relation); }); it('returns the parent relation of a simple multipolygon outer, assuming role outer if unspecified', function() { @@ -14,7 +14,7 @@ describe('iD.osmIsSimpleMultipolygonOuterMember', function() { {tags: {type: 'multipolygon'}, members: [{id: outer.id}]} ); var graph = iD.coreGraph([outer, relation]); - expect(iD.osmIsSimpleMultipolygonOuterMember(outer, graph)).to.equal(relation); + expect(iD.osmIsOldMultipolygonOuterMember(outer, graph)).to.equal(relation); }); it('returns false if entity is not a way', function() { @@ -23,7 +23,7 @@ describe('iD.osmIsSimpleMultipolygonOuterMember', function() { {tags: {type: 'multipolygon'}, members: [{id: outer.id, role: 'outer'}]} ); var graph = iD.coreGraph([outer, relation]); - expect(iD.osmIsSimpleMultipolygonOuterMember(outer, graph)).to.be.false; + expect(iD.osmIsOldMultipolygonOuterMember(outer, graph)).to.be.false; }); it('returns false if entity does not have interesting tags', function() { @@ -32,13 +32,13 @@ describe('iD.osmIsSimpleMultipolygonOuterMember', function() { {tags: {type: 'multipolygon'}, members: [{id: outer.id, role: 'outer'}]} ); var graph = iD.coreGraph([outer, relation]); - expect(iD.osmIsSimpleMultipolygonOuterMember(outer, graph)).to.be.false; + expect(iD.osmIsOldMultipolygonOuterMember(outer, graph)).to.be.false; }); it('returns false if entity does not have a parent relation', function() { var outer = iD.osmWay({tags: {'natural':'wood'}}); var graph = iD.coreGraph([outer]); - expect(iD.osmIsSimpleMultipolygonOuterMember(outer, graph)).to.be.false; + expect(iD.osmIsOldMultipolygonOuterMember(outer, graph)).to.be.false; }); it('returns false if the parent is not a multipolygon', function() { @@ -47,7 +47,7 @@ describe('iD.osmIsSimpleMultipolygonOuterMember', function() { {tags: {type: 'route'}, members: [{id: outer.id, role: 'outer'}]} ); var graph = iD.coreGraph([outer, relation]); - expect(iD.osmIsSimpleMultipolygonOuterMember(outer, graph)).to.be.false; + expect(iD.osmIsOldMultipolygonOuterMember(outer, graph)).to.be.false; }); it('returns false if the parent has interesting tags', function() { @@ -56,7 +56,7 @@ describe('iD.osmIsSimpleMultipolygonOuterMember', function() { {tags: {natural: 'wood', type: 'multipolygon'}, members: [{id: outer.id, role: 'outer'}]} ); var graph = iD.coreGraph([outer, relation]); - expect(iD.osmIsSimpleMultipolygonOuterMember(outer, graph)).to.be.false; + expect(iD.osmIsOldMultipolygonOuterMember(outer, graph)).to.be.false; }); it('returns the parent relation of a simple multipolygon outer, ignoring uninteresting parent tags', function() { @@ -65,7 +65,7 @@ describe('iD.osmIsSimpleMultipolygonOuterMember', function() { {tags: {'tiger:reviewed':'no', type: 'multipolygon'}, members: [{id: outer.id, role: 'outer'}]} ); var graph = iD.coreGraph([outer, relation]); - expect(iD.osmIsSimpleMultipolygonOuterMember(outer, graph)).to.equal(relation); + expect(iD.osmIsOldMultipolygonOuterMember(outer, graph)).to.equal(relation); }); it('returns false if the parent has multiple outer ways', function() { @@ -75,8 +75,8 @@ describe('iD.osmIsSimpleMultipolygonOuterMember', function() { {tags: {type: 'multipolygon'}, members: [{id: outer1.id, role: 'outer'}, {id: outer2.id, role: 'outer'}]} ); var graph = iD.coreGraph([outer1, outer2, relation]); - expect(iD.osmIsSimpleMultipolygonOuterMember(outer1, graph)).to.be.false; - expect(iD.osmIsSimpleMultipolygonOuterMember(outer2, graph)).to.be.false; + expect(iD.osmIsOldMultipolygonOuterMember(outer1, graph)).to.be.false; + expect(iD.osmIsOldMultipolygonOuterMember(outer2, graph)).to.be.false; }); it('returns false if the parent has multiple outer ways, assuming role outer if unspecified', function() { @@ -86,8 +86,8 @@ describe('iD.osmIsSimpleMultipolygonOuterMember', function() { {tags: {type: 'multipolygon'}, members: [{id: outer1.id}, {id: outer2.id}]} ); var graph = iD.coreGraph([outer1, outer2, relation]); - expect(iD.osmIsSimpleMultipolygonOuterMember(outer1, graph)).to.be.false; - expect(iD.osmIsSimpleMultipolygonOuterMember(outer2, graph)).to.be.false; + expect(iD.osmIsOldMultipolygonOuterMember(outer1, graph)).to.be.false; + expect(iD.osmIsOldMultipolygonOuterMember(outer2, graph)).to.be.false; }); it('returns false if the entity is not an outer', function() { @@ -96,12 +96,12 @@ describe('iD.osmIsSimpleMultipolygonOuterMember', function() { {tags: {type: 'multipolygon'}, members: [{id: inner.id, role: 'inner'}]} ); var graph = iD.coreGraph([inner, relation]); - expect(iD.osmIsSimpleMultipolygonOuterMember(inner, graph)).to.be.false; + expect(iD.osmIsOldMultipolygonOuterMember(inner, graph)).to.be.false; }); }); -describe('iD.osmSimpleMultipolygonOuterMember', function() { +describe('iD.osmOldMultipolygonOuterMember', function() { it('returns the outer member of a simple multipolygon', function() { var inner = iD.osmWay(); var outer = iD.osmWay({tags: {'natural':'wood'}}); @@ -111,8 +111,8 @@ describe('iD.osmSimpleMultipolygonOuterMember', function() { }); var graph = iD.coreGraph([inner, outer, relation]); - expect(iD.osmSimpleMultipolygonOuterMember(inner, graph)).to.equal(outer); - expect(iD.osmSimpleMultipolygonOuterMember(outer, graph)).to.equal(outer); + expect(iD.osmOldMultipolygonOuterMember(inner, graph)).to.equal(outer); + expect(iD.osmOldMultipolygonOuterMember(outer, graph)).to.equal(outer); }); it('returns falsy for a complex multipolygon', function() { @@ -126,9 +126,9 @@ describe('iD.osmSimpleMultipolygonOuterMember', function() { }); var graph = iD.coreGraph([inner, outer1, outer2, relation]); - expect(iD.osmSimpleMultipolygonOuterMember(inner, graph)).not.to.be.ok; - expect(iD.osmSimpleMultipolygonOuterMember(outer1, graph)).not.to.be.ok; - expect(iD.osmSimpleMultipolygonOuterMember(outer2, graph)).not.to.be.ok; + expect(iD.osmOldMultipolygonOuterMember(inner, graph)).not.to.be.ok; + expect(iD.osmOldMultipolygonOuterMember(outer1, graph)).not.to.be.ok; + expect(iD.osmOldMultipolygonOuterMember(outer2, graph)).not.to.be.ok; }); it('handles incomplete relations', function() { @@ -139,7 +139,7 @@ describe('iD.osmSimpleMultipolygonOuterMember', function() { }); var graph = iD.coreGraph([way, relation]); - expect(iD.osmSimpleMultipolygonOuterMember(way, graph)).not.to.be.ok; + expect(iD.osmOldMultipolygonOuterMember(way, graph)).not.to.be.ok; }); });