From 556af7fd020274fbb89fe09e5589806cab8a9dd6 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Tue, 13 May 2025 20:21:18 +0200 Subject: [PATCH] Replace stubs with Geocoder search --- CHANGELOG.md | 9 +- app/models/place.rb | 8 +- app/services/trips/countries.rb | 8 +- lib/tasks/points.rake | 4 +- spec/services/trips/countries_spec.rb | 48 ++----- spec/services/visits/names/suggester_spec.rb | 124 +++++++++++-------- 6 files changed, 97 insertions(+), 104 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index cdb8c5c3..c0e6fbf4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,10 +14,10 @@ Geodata on demand Geodata is being used: - [ ] Fetching places geodata - [x] Fetching countries for a trip - - [ ] Suggesting place name for a visit -- [ ] When `STORE_GEODATA` is disabled, points are not being reverse geocoded on creation. -- [ ] When `STORE_GEODATA` is disabled, countries for a trip are being pulled from the geocoding service. -- [ ] When `STORE_GEODATA` is enabled, points are being reverse geocoded upon creation and stored in the database. + - [x] Suggesting place name for a visit +- [x] When `STORE_GEODATA` is disabled, points are not being reverse geocoded on creation. +- [x] When `STORE_GEODATA` is disabled, countries for a trip are being pulled from the geocoding service. +- [x] When `STORE_GEODATA` is enabled, points are being reverse geocoded upon creation and stored in the database. - [ ] Each feature that uses geodata will check if an entity (point, place, etc.) has geodata stored in the database and use it if available. If not, it will make a direct request to the geocoding service to calculate required data. ## Changed @@ -28,6 +28,7 @@ Geodata on demand - Fixed a bug with an attempt to write points with same lonlat and timestamp from iOS app. #1170 - Importing GeoJSON files now saves velocity if it was stored in either `velocity` or `speed` property. +- `rake points:migrate_to_lonlat` should work properly now. #1083 #1161 # 0.26.0 - 2025-05-08 diff --git a/app/models/place.rb b/app/models/place.rb index d7cde7f6..96f6a874 100644 --- a/app/models/place.rb +++ b/app/models/place.rb @@ -23,18 +23,18 @@ class Place < ApplicationRecord end def osm_id - geodata['properties']['osm_id'] + geodata.dig('properties', 'osm_id') end def osm_key - geodata['properties']['osm_key'] + geodata.dig('properties', 'osm_key') end def osm_value - geodata['properties']['osm_value'] + geodata.dig('properties', 'osm_value') end def osm_type - geodata['properties']['osm_type'] + geodata.dig('properties', 'osm_type') end end diff --git a/app/services/trips/countries.rb b/app/services/trips/countries.rb index 2ed18cbb..fd9c3e7b 100644 --- a/app/services/trips/countries.rb +++ b/app/services/trips/countries.rb @@ -1,10 +1,5 @@ # frozen_string_literal: true -require 'rgeo/geo_json' -require 'rgeo' -require 'json' -require 'geocoder' - class Trips::Countries FILE_PATH = Rails.root.join('lib/assets/countries.json') @@ -123,6 +118,9 @@ class Trips::Countries result.data['properties']['countrycode'] rescue StandardError => e Rails.logger.error("Error geocoding point: #{e.message}") + + ExceptionReporter.call(e) + nil end end diff --git a/lib/tasks/points.rake b/lib/tasks/points.rake index 7e3a1993..7580688d 100644 --- a/lib/tasks/points.rake +++ b/lib/tasks/points.rake @@ -5,9 +5,7 @@ namespace :points do task migrate_to_lonlat: :environment do puts 'Updating points to use lonlat...' - points = - Point.where(longitude: nil, latitude: nil) - .select(:id, :longitude, :latitude, :raw_data, :user_id, :timestamp) + points = Point.where(longitude: nil, latitude: nil).without_raw_data points.find_each do |point| Points::RawDataLonlatExtractor.new(point).call diff --git a/spec/services/trips/countries_spec.rb b/spec/services/trips/countries_spec.rb index 107e98fc..d2e4638a 100644 --- a/spec/services/trips/countries_spec.rb +++ b/spec/services/trips/countries_spec.rb @@ -29,32 +29,18 @@ RSpec.describe Trips::Countries do allow(File).to receive(:read).with(Trips::Countries::FILE_PATH).and_return(geo_json_content) # Explicitly stub all Geocoder calls with specific coordinates - stub_request(:get, 'https://photon.dawarich.app/reverse?lang=en&lat=50.0&limit=1&lon=10.0') - .to_return( - status: 200, - body: { - type: 'FeatureCollection', - features: [{ type: 'Feature', properties: { countrycode: 'DE' } }] - }.to_json - ) - - stub_request(:get, 'https://photon.dawarich.app/reverse?lang=en&lat=60.0&limit=1&lon=20.0') - .to_return( - status: 200, - body: { - type: 'FeatureCollection', - features: [{ type: 'Feature', properties: { countrycode: 'SE' } }] - }.to_json - ) - - stub_request(:get, 'https://photon.dawarich.app/reverse?lang=en&lat=70.0&limit=1&lon=30.0') - .to_return( - status: 200, - body: { - type: 'FeatureCollection', - features: [{ type: 'Feature', properties: { countrycode: 'FI' } }] - }.to_json - ) + allow(Geocoder).to receive(:search).and_return( + [double(data: { 'properties' => { 'countrycode' => 'DE' } })] + ) + allow(Geocoder).to receive(:search).with([50.0, 10.0], limit: 1).and_return( + [double(data: { 'properties' => { 'countrycode' => 'DE' } })] + ) + allow(Geocoder).to receive(:search).with([60.0, 20.0], limit: 1).and_return( + [double(data: { 'properties' => { 'countrycode' => 'SE' } })] + ) + allow(Geocoder).to receive(:search).with([70.0, 30.0], limit: 1).and_return( + [double(data: { 'properties' => { 'countrycode' => 'FI' } })] + ) allow(Rails.logger).to receive(:info) allow(Rails.logger).to receive(:error) @@ -89,16 +75,6 @@ RSpec.describe Trips::Countries do allow(Thread).to receive(:new).and_yield allow(points).to receive(:to_a).and_return([point1, point1, point2, point3, point4]) - # Make sure we have a stub for the duplicated point - stub_request(:get, 'https://photon.dawarich.app/reverse?lang=en&lat=50.0&limit=1&lon=10.0') - .to_return( - status: 200, - body: { - type: 'FeatureCollection', - features: [{ type: 'Feature', properties: { countrycode: 'DE' } }] - }.to_json - ) - result = described_class.new(trip).call expect(result.keys.first).to eq('DE') diff --git a/spec/services/visits/names/suggester_spec.rb b/spec/services/visits/names/suggester_spec.rb index dff5ff91..d9361342 100644 --- a/spec/services/visits/names/suggester_spec.rb +++ b/spec/services/visits/names/suggester_spec.rb @@ -34,17 +34,19 @@ RSpec.describe Visits::Names::Suggester do context 'when features exist but with different types' do let(:points) do [ - double('Point', geodata: { - 'features' => [ - { 'properties' => { 'type' => 'cafe', 'name' => 'Coffee Shop' } }, - { 'properties' => { 'type' => 'restaurant', 'name' => 'Pizza Place' } } - ] - }) + double( + 'Point', + geodata: { + 'features' => [ + { 'properties' => { 'type' => 'cafe', 'name' => 'Coffee Shop' } }, + { 'properties' => { 'type' => 'restaurant', 'name' => 'Pizza Place' } } + ] + } + ) ] end it 'returns the name of the most common type' do - # Since both types appear once, it will pick the first one alphabetically in practice expect(suggester.call).to eq('Coffee Shop') end end @@ -52,21 +54,30 @@ RSpec.describe Visits::Names::Suggester do context 'when features have a common type but different names' do let(:points) do [ - double('Point', geodata: { - 'features' => [ - { 'properties' => { 'type' => 'park', 'name' => 'Central Park' } } - ] - }), - double('Point', geodata: { - 'features' => [ - { 'properties' => { 'type' => 'park', 'name' => 'City Park' } } - ] - }), - double('Point', geodata: { - 'features' => [ - { 'properties' => { 'type' => 'park', 'name' => 'Central Park' } } - ] - }) + double( + 'Point', + geodata: { + 'features' => [ + { 'properties' => { 'type' => 'park', 'name' => 'Central Park' } } + ] + } + ), + double( + 'Point', + geodata: { + 'features' => [ + { 'properties' => { 'type' => 'park', 'name' => 'City Park' } } + ] + } + ), + double( + 'Point', + geodata: { + 'features' => [ + { 'properties' => { 'type' => 'park', 'name' => 'Central Park' } } + ] + } + ) ] end @@ -78,19 +89,22 @@ RSpec.describe Visits::Names::Suggester do context 'when a complete place can be built' do let(:points) do [ - double('Point', geodata: { - 'features' => [ - { - 'properties' => { - 'type' => 'cafe', - 'name' => 'Starbucks', - 'street' => '123 Main St', - 'city' => 'San Francisco', - 'state' => 'CA' - } - } - ] - }) + double( + 'Point', + geodata: { + 'features' => [ + { + 'properties' => { + 'type' => 'cafe', + 'name' => 'Starbucks', + 'street' => '123 Main St', + 'city' => 'San Francisco', + 'state' => 'CA' + } + } + ] + } + ) ] end @@ -102,18 +116,21 @@ RSpec.describe Visits::Names::Suggester do context 'when only partial place details are available' do let(:points) do [ - double('Point', geodata: { - 'features' => [ - { - 'properties' => { - 'type' => 'cafe', - 'name' => 'Starbucks', - 'city' => 'San Francisco' - # No street or state - } - } - ] - }) + double( + 'Point', + geodata: { + 'features' => [ + { + 'properties' => { + 'type' => 'cafe', + 'name' => 'Starbucks', + 'city' => 'San Francisco' + # No street or state + } + } + ] + } + ) ] end @@ -137,11 +154,14 @@ RSpec.describe Visits::Names::Suggester do context 'when most common name is blank' do let(:points) do [ - double('Point', geodata: { - 'features' => [ - { 'properties' => { 'type' => 'road', 'name' => '' } } - ] - }) + double( + 'Point', + geodata: { + 'features' => [ + { 'properties' => { 'type' => 'road', 'name' => '' } } + ] + } + ) ] end