Fix unsolvable validator error for presets with locationSet

re: #1626
re: openstreetmap/iD#10459

Author: bhousel

modules/core/PresetSystem.js

@@ -73,10 +73,14 @@ export class PresetSystem extends AbstractSystem {
       }
     }
 
-    const assets = this.context.systems.assets;
-    const urlhash = this.context.systems.urlhash;
+    const context = this.context;
+    const assets = context.systems.assets;
+    const locations = context.systems.locations;
+    const urlhash = context.systems.urlhash;
+
     const prerequisites = Promise.all([
       assets.initAsync(),
+      locations.initAsync(),
       urlhash.initAsync()
     ]);
 
@@ -143,7 +147,7 @@ export class PresetSystem extends AbstractSystem {
   merge(src) {
     let newLocationSets = [];
     const context = this.context;
-    const locationSystem = context.systems.locations;
+    const locations = context.systems.locations;
 
     // Merge Fields
     if (src.fields) {
@@ -246,12 +250,12 @@ if (c.icon) c.icon = c.icon.replace(/^iD-/, 'rapid-');
 
     // Merge Custom Features
     if (src.featureCollection && Array.isArray(src.featureCollection.features)) {
-      locationSystem.mergeCustomGeoJSON(src.featureCollection);
+      locations.mergeCustomGeoJSON(src.featureCollection);
     }
 
     // Resolve all locationSet features.
     if (newLocationSets.length) {
-      locationSystem.mergeLocationSets(newLocationSets);
+      locations.mergeLocationSets(newLocationSets);
     }
 
     return this;
@@ -294,29 +298,36 @@ if (c.icon) c.icon = c.icon.replace(/^iD-/, 'rapid-');
    */
   matchTags(tags, geometry, loc) {
     const keyIndex = this._geometryIndex[geometry];
+    const context = this.context;
+    const locations = context.systems.locations;
+
+    // If we care about location, gather the locationSets allowed at this location
+    const validHere = Array.isArray(loc) ? locations.locationSetsAt(loc) : null;
+
     let bestScore = -1;
-    let bestMatch;
+    let bestMatch = null;
     let matchCandidates = [];
 
     for (let k in tags) {
-      let indexMatches = [];
-
-      let valueIndex = keyIndex[k];
+      const valueIndex = keyIndex[k];
       if (!valueIndex) continue;
 
-      let keyValueMatches = valueIndex[tags[k]];
+      const indexMatches = [];
+      const keyValueMatches = valueIndex[tags[k]];
       if (keyValueMatches) indexMatches.push(...keyValueMatches);
-      let keyStarMatches = valueIndex['*'];
+      const keyStarMatches = valueIndex['*'];
       if (keyStarMatches) indexMatches.push(...keyStarMatches);
 
       if (indexMatches.length === 0) continue;
 
-      for (let i = 0; i < indexMatches.length; i++) {
-        const candidate = indexMatches[i];
+      for (const candidate of indexMatches) {
         const score = candidate.matchScore(tags);
         if (score === -1) continue;
 
-        matchCandidates.push({score, candidate});
+        // Exclude candidate if it is scoped to a location not valid here
+        if (validHere && candidate.locationSetID && !validHere[candidate.locationSetID]) continue;
+
+        matchCandidates.push({ score, candidate });
 
         if (score > bestScore) {
           bestScore = score;
@@ -325,22 +336,6 @@ if (c.icon) c.icon = c.icon.replace(/^iD-/, 'rapid-');
       }
     }
 
-    const locationSystem = this.context.systems.locations;
-    if (bestMatch && bestMatch.locationSetID && bestMatch.locationSetID !== '+[Q2]' && Array.isArray(loc)) {
-      const validHere = locationSystem.locationSetsAt(loc);
-      if (!validHere[bestMatch.locationSetID]) {
-        matchCandidates.sort((a, b) => (a.score < b.score) ? 1 : -1);
-        for (let i = 0; i < matchCandidates.length; i++){
-          const candidateScore = matchCandidates[i];
-          if (!candidateScore.candidate.locationSetID || validHere[candidateScore.candidate.locationSetID]) {
-            bestMatch = candidateScore.candidate;
-            bestScore = candidateScore.score;
-            break;
-          }
-        }
-      }
-    }
-
     // If any part of an address is present, allow fallback to "Address" preset - iD#4353
     if (!bestMatch || bestMatch.isFallback()) {
       for (let k in tags) {
@@ -526,8 +521,8 @@ if (c.icon) c.icon = c.icon.replace(/^iD-/, 'rapid-');
     // If a location was provided, filter results to only those valid here.
     let arr = [...results.values()];
     if (Array.isArray(loc)) {
-      const locationSystem = this.context.systems.locations;
-      const validHere = locationSystem.locationSetsAt(loc);
+      const locations = this.context.systems.locations;
+      const validHere = locations.locationSetsAt(loc);
       arr = arr.filter(item => !item.locationSetID || validHere[item.locationSetID]);
     }
 

modules/validations/mismatched_geometry.js

@@ -236,6 +236,7 @@ export function validationMismatchedGeometry(context) {
         if (entity.type === 'node' && entity.isOnAddressLine(graph)) return null;
 
         var sourceGeom = entity.geometry(graph);
+        var loc = entity.extent(graph).center();
 
         var targetGeoms = entity.type === 'way' ? ['point', 'vertex'] : ['line', 'area'];
 
@@ -244,7 +245,7 @@ export function validationMismatchedGeometry(context) {
         var asSource = presets.match(entity, graph);
 
         var targetGeom = targetGeoms.find(nodeGeom => {
-            var asTarget = presets.matchTags(entity.tags, nodeGeom);
+            var asTarget = presets.matchTags(entity.tags, nodeGeom, loc);
             // sometimes there are two presets with the same tags for different geometries
             if (!asSource || !asTarget || asSource === asTarget || deepEqual(asSource.tags, asTarget.tags)) return false;
 

test/browser/validations/ambiguous_crossing_tags.js

@@ -16,9 +16,10 @@ describe('validationAmbiguousCrossingTags', () => {
     constructor() {
       this.services = {};
       this.systems = {
-        editor:   new MockEditSystem(),
-        l10n:     new MockLocalizationSystem(),
-        presets:  new Rapid.PresetSystem(this)
+        editor:     new MockEditSystem(this),
+        l10n:       new MockLocalizationSystem(this),
+        locations:  new Rapid.LocationSystem(this),
+        presets:    new Rapid.PresetSystem(this)
       };
     }
   }

test/browser/validations/mismatched_geometry.js

@@ -8,10 +8,6 @@ describe('validationMismatchedGeometry', () => {
     t(id)                 { return id; }
   }
 
-  class MockLocationSystem {
-    constructor() {}
-  }
-
   class MockStorageSystem {
     constructor() { }
     getItem() { return ''; }
@@ -30,7 +26,7 @@ describe('validationMismatchedGeometry', () => {
       this.systems = {
         assets:     new Rapid.AssetSystem(this),
         l10n:       new MockLocalizationSystem(),
-        locations:  new MockLocationSystem(),
+        locations:  new Rapid.LocationSystem(this),
         presets:    new Rapid.PresetSystem(this),
         storage:    new MockStorageSystem(),
         urlhash:    new MockUrlSystem()
@@ -45,11 +41,15 @@ describe('validationMismatchedGeometry', () => {
     graph = new Rapid.Graph();     // reset
     _savedAreaKeys = Rapid.osmAreaKeys;
 
-    // For the tests that need a building preset
     const testPresets = {
       building: {
         tags: { building: '*' },
         geometry: ['area']
+      },
+      desert_library: {
+        tags: { amenity: 'library' },
+        geometry: ['point'],
+        locationSet: { include: ['Q620634'] }
       }
     };
     context.systems.assets._cache.tagging_preset_presets = testPresets;
@@ -171,6 +171,16 @@ describe('validationMismatchedGeometry', () => {
     });
   });
 
+  it(`does not flag open way if the preset location doesn't match the entity location` , () => {
+    // In this test case, there is an area preset for `amenity=library` but we won't match it because of the location
+    const presets = context.systems.presets;
+    return presets.initAsync().then(() => {
+      createOpenWay({ amenity: 'library' });
+      const issues = validate();
+      expect(issues).to.have.lengthOf(0);
+    });
+  });
+
   it('flags open way with both area and line tags', () => {
     createOpenWay({ area: 'yes', barrier: 'fence' });
     const issues = validate();

test/browser/validations/outdated_tags.js

@@ -6,17 +6,13 @@ describe('validationOutdatedTags', () => {
     t(id)                 { return id; }
   }
 
-  class MockLocationSystem {
-    constructor() {}
-  }
-
   class MockContext {
     constructor() {
       this.services = {};
       this.systems = {
         assets:     new Rapid.AssetSystem(this),
-        l10n:       new MockLocalizationSystem(),
-        locations:  new MockLocationSystem(),
+        l10n:       new MockLocalizationSystem(this),
+        locations:  new Rapid.LocationSystem(this),
         presets:    new Rapid.PresetSystem(this)
       };
     }