Merge pull request 'Update OSM data when opening saved places' (#32) from feature/update_place_data into master

Reviewed-on: #32

app/routes/place.js

@@ -101,6 +101,23 @@ export default class PlaceRoute extends Route {
     return null;
   }
 
+  setupController(controller, model) {
+    super.setupController(controller, model);
+    this.checkUpdates(model);
+  }
+
+  async checkUpdates(place) {
+    // Only check for updates if it's a saved place (has ID) and is an OSM object
+    if (place && place.id && place.osmId && place.osmType) {
+      const updatedPlace = await this.storage.refreshPlace(place);
+      if (updatedPlace) {
+        // If an update occurred, refresh the map UI selection without moving the camera
+        // This ensures the sidebar shows the new data
+        this.mapUi.selectPlace(updatedPlace, { preventZoom: true });
+      }
+    }
+  }
+
   serialize(model) {
     // If the model is a saved bookmark, use its ID
     if (model.id) {

app/services/storage.js

@@ -1,4 +1,4 @@
-import Service from '@ember/service';
+import Service, { service } from '@ember/service';
 import RemoteStorage from 'remotestoragejs';
 import Places from '@remotestorage/module-places';
 import Widget from 'remotestorage-widget';
@@ -7,8 +7,10 @@ import { getGeohashPrefixesInBbox } from '../utils/geohash-coverage';
 import { action } from '@ember/object';
 import { debounceTask } from 'ember-lifeline';
 import Geohash from 'latlon-geohash';
+import { getLocalizedName } from '../utils/osm';
 
 export default class StorageService extends Service {
+  @service osm;
   rs;
   widget;
   @tracked placesInView = [];
@@ -366,6 +368,82 @@ export default class StorageService extends Service {
     }
   }
 
+  async refreshPlace(place) {
+    if (!place || !place.id || !place.osmId || !place.osmType) {
+      return null;
+    }
+
+    try {
+      console.debug(`Checking for updates for ${place.title} (${place.osmId})`);
+      const freshData = await this.osm.fetchOsmObject(
+        place.osmId,
+        place.osmType
+      );
+
+      if (!freshData) {
+        console.warn('Could not fetch fresh data for', place.osmId);
+        return null;
+      }
+
+      // Check for changes
+      let hasChanges = false;
+      const changes = {};
+
+      // 1. Check Coordinates (allow tiny drift < ~1m)
+      const latDiff = Math.abs(place.lat - freshData.lat);
+      const lonDiff = Math.abs(place.lon - freshData.lon);
+      if (latDiff > 0.00001 || lonDiff > 0.00001) {
+        hasChanges = true;
+        changes.lat = freshData.lat;
+        changes.lon = freshData.lon;
+      }
+
+      // 2. Check Tags
+      const oldTags = place.osmTags || {};
+      const newTags = freshData.osmTags || {};
+      const allKeys = new Set([
+        ...Object.keys(oldTags),
+        ...Object.keys(newTags),
+      ]);
+
+      for (const key of allKeys) {
+        if (oldTags[key] !== newTags[key]) {
+          hasChanges = true;
+          changes.osmTags = newTags;
+          break;
+        }
+      }
+
+      if (!hasChanges) {
+        console.debug('No changes detected for', place.title);
+        return null;
+      }
+
+      console.debug('Changes detected:', changes);
+
+      // 3. Prepare Update
+      const updatedPlace = {
+        ...place,
+        ...changes,
+      };
+
+      // If the current title matches the old localized name, update it to the
+      // new localized name. If the user renamed it (custom title), keep it.
+      const oldDefaultName = getLocalizedName(oldTags);
+      const newDefaultName = getLocalizedName(newTags);
+
+      if (place.title === oldDefaultName && oldDefaultName !== newDefaultName) {
+        updatedPlace.title = newDefaultName;
+      }
+
+      // 4. Save
+      return await this.updatePlace(updatedPlace);
+    } catch (e) {
+      console.error('Failed to refresh place:', e);
+      return null;
+    }
+  }
+
   @action
   connect() {
     this.isWidgetOpen = true;

tests/unit/routes/place-test.js

@@ -125,4 +125,60 @@ module('Unit | Route | place', function (hooks) {
 
     assert.notOk(fetchCalled, 'fetchOsmObject should NOT be called for nodes');
   });
+
+  test('setupController triggers checkUpdates', async function (assert) {
+    let route = this.owner.lookup('route:place');
+
+    // Stub Storage Service
+    let refreshPlaceCalled = false;
+    class StorageStub extends Service {
+      async refreshPlace(place) {
+        refreshPlaceCalled = true;
+        assert.strictEqual(place.id, '123', 'Passed correct place to storage');
+        return {
+          ...place,
+          title: 'Updated Title',
+        };
+      }
+    }
+
+    // Stub MapUi Service
+    let selectPlaceCalled = false;
+    class MapUiStub extends Service {
+      selectPlace(place, options) {
+        selectPlaceCalled = true;
+        assert.strictEqual(
+          place.title,
+          'Updated Title',
+          'Selected updated place'
+        );
+        assert.ok(options.preventZoom, 'Prevented zoom on update');
+      }
+      stopSearch() {}
+    }
+
+    this.owner.register('service:storage', StorageStub);
+    this.owner.register('service:map-ui', MapUiStub);
+
+    let model = {
+      id: '123',
+      osmId: '456',
+      osmType: 'node',
+      title: 'Original Title',
+    };
+
+    let controller = {};
+
+    // Trigger setupController
+    route.setupController(controller, model);
+
+    // checkUpdates is async and not awaited in setupController, so we need to wait a tick
+    await new Promise((resolve) => setTimeout(resolve, 10));
+
+    assert.ok(refreshPlaceCalled, 'refreshPlace should be called');
+    assert.ok(
+      selectPlaceCalled,
+      'mapUi.selectPlace should be called with update'
+    );
+  });
 });

tests/unit/services/storage-test.js

@@ -0,0 +1,202 @@
+import { module, test } from 'qunit';
+import { setupTest } from 'marco/tests/helpers';
+import Service from '@ember/service';
+
+module('Unit | Service | storage', function (hooks) {
+  setupTest(hooks);
+
+  test('refreshPlace skips invalid places', async function (assert) {
+    let service = this.owner.lookup('service:storage');
+    let result = await service.refreshPlace({});
+    assert.strictEqual(result, null);
+  });
+
+  test('refreshPlace detects coordinate drift', async function (assert) {
+    let service = this.owner.lookup('service:storage');
+
+    // Stub OSM Service
+    class OsmStub extends Service {
+      async fetchOsmObject(id, type) {
+        return {
+          osmId: id,
+          osmType: type,
+          lat: 52.5201, // Changed significantly from 52.5200
+          lon: 13.405,
+          osmTags: { name: 'Foo' },
+        };
+      }
+    }
+    this.owner.register('service:osm', OsmStub);
+
+    // Mock storage update
+    let updatePlaceCalled = false;
+    service.updatePlace = async (place) => {
+      updatePlaceCalled = true;
+      return place;
+    };
+
+    let place = {
+      id: '123',
+      osmId: '456',
+      osmType: 'node',
+      lat: 52.52,
+      lon: 13.405,
+      osmTags: { name: 'Foo' },
+      title: 'Foo',
+    };
+
+    let result = await service.refreshPlace(place);
+
+    assert.ok(updatePlaceCalled, 'updatePlace should be called');
+    assert.strictEqual(result.lat, 52.5201, 'Latitude updated');
+  });
+
+  test('refreshPlace ignores tiny coordinate drift', async function (assert) {
+    let service = this.owner.lookup('service:storage');
+
+    class OsmStub extends Service {
+      async fetchOsmObject(id, type) {
+        return {
+          osmId: id,
+          osmType: type,
+          lat: 52.5200005, // Tiny change (< 0.00001)
+          lon: 13.405,
+          osmTags: { name: 'Foo' },
+        };
+      }
+    }
+    this.owner.register('service:osm', OsmStub);
+
+    let updatePlaceCalled = false;
+    service.updatePlace = async () => {
+      updatePlaceCalled = true;
+    };
+
+    let place = {
+      id: '123',
+      osmId: '456',
+      osmType: 'node',
+      lat: 52.52,
+      lon: 13.405,
+      osmTags: { name: 'Foo' },
+      title: 'Foo',
+    };
+
+    await service.refreshPlace(place);
+
+    assert.notOk(updatePlaceCalled, 'updatePlace should NOT be called');
+  });
+
+  test('refreshPlace detects tag changes', async function (assert) {
+    let service = this.owner.lookup('service:storage');
+
+    class OsmStub extends Service {
+      async fetchOsmObject(id, type) {
+        return {
+          osmId: id,
+          osmType: type,
+          lat: 52.52,
+          lon: 13.405,
+          osmTags: { name: 'Bar' }, // Changed name
+        };
+      }
+    }
+    this.owner.register('service:osm', OsmStub);
+
+    let updatePlaceCalled = false;
+    service.updatePlace = async (place) => {
+      updatePlaceCalled = true;
+      return place;
+    };
+
+    let place = {
+      id: '123',
+      osmId: '456',
+      osmType: 'node',
+      lat: 52.52,
+      lon: 13.405,
+      osmTags: { name: 'Foo' },
+      title: 'Foo',
+    };
+
+    let result = await service.refreshPlace(place);
+
+    assert.ok(updatePlaceCalled, 'updatePlace should be called');
+    assert.strictEqual(result.osmTags.name, 'Bar', 'Tags updated');
+  });
+
+  test('refreshPlace updates title if it was default', async function (assert) {
+    let service = this.owner.lookup('service:storage');
+
+    class OsmStub extends Service {
+      async fetchOsmObject(id, type) {
+        return {
+          osmId: id,
+          osmType: type,
+          lat: 52.52,
+          lon: 13.405,
+          osmTags: { name: 'New Name' },
+        };
+      }
+    }
+    this.owner.register('service:osm', OsmStub);
+
+    service.updatePlace = async (place) => place;
+
+    let place = {
+      id: '123',
+      osmId: '456',
+      osmType: 'node',
+      lat: 52.52,
+      lon: 13.405,
+      osmTags: { name: 'Old Name' },
+      title: 'Old Name', // Matches default
+    };
+
+    let result = await service.refreshPlace(place);
+
+    assert.strictEqual(result.title, 'New Name', 'Title should update');
+  });
+
+  test('refreshPlace preserves custom title', async function (assert) {
+    let service = this.owner.lookup('service:storage');
+
+    class OsmStub extends Service {
+      async fetchOsmObject(id, type) {
+        return {
+          osmId: id,
+          osmType: type,
+          lat: 52.52,
+          lon: 13.405,
+          osmTags: { name: 'New Name' },
+        };
+      }
+    }
+    this.owner.register('service:osm', OsmStub);
+
+    service.updatePlace = async (place) => place;
+
+    let place = {
+      id: '123',
+      osmId: '456',
+      osmType: 'node',
+      lat: 52.52,
+      lon: 13.405,
+      osmTags: { name: 'Old Name' },
+      title: 'My Custom Place', // User renamed it
+    };
+
+    let result = await service.refreshPlace(place);
+
+    assert.strictEqual(
+      result.title,
+      'My Custom Place',
+      'Title should NOT update'
+    );
+    assert.strictEqual(
+      result.osmTags.name,
+      'New Name',
+      'Tags should still update'
+    );
+  });
+});