From fa8065890464924e07bcc14781213214857bf624 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Thu, 8 May 2025 17:28:06 +0200 Subject: [PATCH 01/41] Fix GeoJSON import speed/velocity --- CHANGELOG.md | 6 ++++++ app/services/geojson/params.rb | 4 +++- spec/fixtures/files/geojson/export.json | 20 ++++++++++---------- spec/services/geojson/params_spec.rb | 4 ++-- 4 files changed, 21 insertions(+), 13 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9945b0c..fca70ead 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +# 0.26.1 - UNRELEASED + +## Fixed + +- Importing GeoJSON files now saves velocity if it was stored in either `velocity` or `speed` property. + # 0.26.0 - 2025-05-08 ⚠️ This release includes a breaking change. ⚠️ diff --git a/app/services/geojson/params.rb b/app/services/geojson/params.rb index 8ae4fb41..00b9907c 100644 --- a/app/services/geojson/params.rb +++ b/app/services/geojson/params.rb @@ -95,7 +95,9 @@ class Geojson::Params end def speed(feature) - feature.dig(:properties, :speed).to_f.round(1) + value = feature.dig(:properties, :speed) || feature.dig(:properties, :velocity) + + value.to_f.round(1) end def accuracy(feature) diff --git a/spec/fixtures/files/geojson/export.json b/spec/fixtures/files/geojson/export.json index 8b916ab1..ea1e3fa9 100644 --- a/spec/fixtures/files/geojson/export.json +++ b/spec/fixtures/files/geojson/export.json @@ -12,7 +12,7 @@ "topic": "MyString", "altitude": 1, "longitude": "0.1", - "velocity": "MyString", + "velocity": 1.5, "trigger": "background_event", "bssid": "MyString", "ssid": "MyString", @@ -41,7 +41,7 @@ "topic": "MyString", "altitude": 1, "longitude": "0.2", - "velocity": "MyString", + "velocity": 1.5, "trigger": "background_event", "bssid": "MyString", "ssid": "MyString", @@ -70,7 +70,7 @@ "topic": "MyString", "altitude": 1, "longitude": "0.3", - "velocity": "MyString", + "velocity": 1.5, "trigger": "background_event", "bssid": "MyString", "ssid": "MyString", @@ -99,7 +99,7 @@ "topic": "MyString", "altitude": 1, "longitude": "0.4", - "velocity": "MyString", + "velocity": 1.5, "trigger": "background_event", "bssid": "MyString", "ssid": "MyString", @@ -128,7 +128,7 @@ "topic": "MyString", "altitude": 1, "longitude": "0.5", - "velocity": "MyString", + "velocity": 1.5, "trigger": "background_event", "bssid": "MyString", "ssid": "MyString", @@ -157,7 +157,7 @@ "topic": "MyString", "altitude": 1, "longitude": "0.6", - "velocity": "MyString", + "velocity": 1.5, "trigger": "background_event", "bssid": "MyString", "ssid": "MyString", @@ -186,7 +186,7 @@ "topic": "MyString", "altitude": 1, "longitude": "0.7", - "velocity": "MyString", + "velocity": 1.5, "trigger": "background_event", "bssid": "MyString", "ssid": "MyString", @@ -215,7 +215,7 @@ "topic": "MyString", "altitude": 1, "longitude": "0.8", - "velocity": "MyString", + "velocity": 1.5, "trigger": "background_event", "bssid": "MyString", "ssid": "MyString", @@ -244,7 +244,7 @@ "topic": "MyString", "altitude": 1, "longitude": "0.9", - "velocity": "MyString", + "velocity": 1.5, "trigger": "background_event", "bssid": "MyString", "ssid": "MyString", @@ -273,7 +273,7 @@ "topic": "MyString", "altitude": 1, "longitude": "1.0", - "velocity": "MyString", + "velocity": 1.5, "trigger": "background_event", "bssid": "MyString", "ssid": "MyString", diff --git a/spec/services/geojson/params_spec.rb b/spec/services/geojson/params_spec.rb index 817cd85f..345ddbe3 100644 --- a/spec/services/geojson/params_spec.rb +++ b/spec/services/geojson/params_spec.rb @@ -23,7 +23,7 @@ RSpec.describe Geojson::Params do battery: nil, timestamp: Time.zone.at(1_609_459_201), altitude: 1, - velocity: 0, + velocity: 1.5, tracker_id: nil, ssid: nil, accuracy: 1, @@ -45,7 +45,7 @@ RSpec.describe Geojson::Params do 'topic' => 'MyString', 'altitude' => 1, 'longitude' => '0.1', - 'velocity' => 'MyString', + 'velocity' => 1.5, 'trigger' => 'background_event', 'bssid' => 'MyString', 'ssid' => 'MyString', From 2f88a7189eebe96459cf1c72d620e24a65d83342 Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 12 May 2025 14:12:03 +0000 Subject: [PATCH 02/41] Bump chartkick from 5.1.4 to 5.1.5 Bumps [chartkick](https://github.com/ankane/chartkick) from 5.1.4 to 5.1.5. - [Changelog](https://github.com/ankane/chartkick/blob/master/CHANGELOG.md) - [Commits](https://github.com/ankane/chartkick/compare/v5.1.4...v5.1.5) --- updated-dependencies: - dependency-name: chartkick dependency-version: 5.1.5 dependency-type: direct:production update-type: version-update:semver-patch ... Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 03ceb8ef..366f4da6 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -105,7 +105,7 @@ GEM racc builder (3.3.0) byebug (12.0.0) - chartkick (5.1.4) + chartkick (5.1.5) coderay (1.1.3) concurrent-ruby (1.3.5) connection_pool (2.5.3) From 022bcf23845c46d7ebef05822349c0c367f4239c Mon Sep 17 00:00:00 2001 From: "dependabot[bot]" <49699333+dependabot[bot]@users.noreply.github.com> Date: Mon, 12 May 2025 14:12:14 +0000 Subject: [PATCH 03/41] Bump shoulda-matchers from 6.4.0 to 6.5.0 Bumps [shoulda-matchers](https://github.com/thoughtbot/shoulda-matchers) from 6.4.0 to 6.5.0. - [Release notes](https://github.com/thoughtbot/shoulda-matchers/releases) - [Changelog](https://github.com/thoughtbot/shoulda-matchers/blob/main/CHANGELOG.md) - [Commits](https://github.com/thoughtbot/shoulda-matchers/compare/v6.4.0...v6.5.0) --- updated-dependencies: - dependency-name: shoulda-matchers dependency-version: 6.5.0 dependency-type: direct:development update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] --- Gemfile.lock | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/Gemfile.lock b/Gemfile.lock index 03ceb8ef..75df1197 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -395,7 +395,7 @@ GEM sentry-ruby (5.23.0) bigdecimal concurrent-ruby (~> 1.0, >= 1.0.2) - shoulda-matchers (6.4.0) + shoulda-matchers (6.5.0) activesupport (>= 5.2.0) sidekiq (7.3.9) base64 From 52aefa109e2ac8a7b0479b0ce940f05f9bd60e52 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 12 May 2025 21:41:55 +0200 Subject: [PATCH 04/41] Skip points without lonlat and timestamp from Owntracks --- CHANGELOG.md | 8 + Gemfile | 1 + Gemfile.lock | 2 + app/jobs/owntracks/point_creating_job.rb | 1 + app/services/points/create.rb | 5 +- app/views/home/index.html.erb | 3 +- app/views/imports/index.html.erb | 2 +- config/initializers/sentry.rb | 1 + spec/requests/home_spec.rb | 2 +- spec/services/points/create_spec.rb | 179 +++++++++++++++++++++-- 10 files changed, 184 insertions(+), 20 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index f9945b0c..accfb7fb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,14 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +# 0.26.1 - 2025-05-12 + +## Fixed + +- Fixed a bug with an attempt to write points with same lonlat and timestamp from iOS app. #1170 + + + # 0.26.0 - 2025-05-08 ⚠️ This release includes a breaking change. ⚠️ diff --git a/Gemfile b/Gemfile index 1b6b8ece..e651c4df 100644 --- a/Gemfile +++ b/Gemfile @@ -34,6 +34,7 @@ gem 'rswag-api' gem 'rswag-ui' gem 'sentry-ruby' gem 'sentry-rails' +gem 'stackprof' gem 'sidekiq' gem 'sidekiq-cron' gem 'sidekiq-limit_fetch' diff --git a/Gemfile.lock b/Gemfile.lock index 03ceb8ef..9cb1dde4 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -423,6 +423,7 @@ GEM actionpack (>= 6.1) activesupport (>= 6.1) sprockets (>= 3.0.0) + stackprof (0.2.27) stimulus-rails (1.3.4) railties (>= 6.0.0) stringio (3.1.7) @@ -525,6 +526,7 @@ DEPENDENCIES sidekiq-limit_fetch simplecov sprockets-rails + stackprof stimulus-rails strong_migrations super_diff diff --git a/app/jobs/owntracks/point_creating_job.rb b/app/jobs/owntracks/point_creating_job.rb index 9dfcc83e..947ba6ec 100644 --- a/app/jobs/owntracks/point_creating_job.rb +++ b/app/jobs/owntracks/point_creating_job.rb @@ -8,6 +8,7 @@ class Owntracks::PointCreatingJob < ApplicationJob def perform(point_params, user_id) parsed_params = OwnTracks::Params.new(point_params).call + return if parsed_params[:timestamp].nil? || parsed_params[:lonlat].nil? return if point_exists?(parsed_params, user_id) Point.create!(parsed_params.merge(user_id:)) diff --git a/app/services/points/create.rb b/app/services/points/create.rb index 24b5b275..c373fc20 100644 --- a/app/services/points/create.rb +++ b/app/services/points/create.rb @@ -11,9 +11,12 @@ class Points::Create def call data = Points::Params.new(params, user.id).call + # Deduplicate points based on unique constraint + deduplicated_data = data.uniq { |point| [point[:lonlat], point[:timestamp], point[:user_id]] } + created_points = [] - data.each_slice(1000) do |location_batch| + deduplicated_data.each_slice(1000) do |location_batch| # rubocop:disable Rails/SkipsModelValidations result = Point.upsert_all( location_batch, diff --git a/app/views/home/index.html.erb b/app/views/home/index.html.erb index 9b223f40..f290fc01 100644 --- a/app/views/home/index.html.erb +++ b/app/views/home/index.html.erb @@ -8,7 +8,8 @@

The only location history tracker you'll ever need.

- <%#= link_to 'Sign up', new_user_registration_path, class: "rounded-lg py-3 px-5 my-3 bg-blue-600 text-white block font-medium" %> + <%= link_to 'Sign up', new_user_registration_path, class: "rounded-lg py-3 px-5 my-3 bg-blue-600 text-white block font-medium" %> +
or
<%= link_to 'Sign in', new_user_session_path, class: "rounded-lg py-3 px-5 bg-neutral text-neutral-content block font-medium" %> diff --git a/app/views/imports/index.html.erb b/app/views/imports/index.html.erb index 54a7eb79..0d2f480e 100644 --- a/app/views/imports/index.html.erb +++ b/app/views/imports/index.html.erb @@ -19,7 +19,7 @@
<% if @imports.empty? %> -
+

Hello there!

diff --git a/config/initializers/sentry.rb b/config/initializers/sentry.rb index 40954c8f..4aa80f03 100644 --- a/config/initializers/sentry.rb +++ b/config/initializers/sentry.rb @@ -6,4 +6,5 @@ Sentry.init do |config| config.breadcrumbs_logger = [:active_support_logger] config.dsn = SENTRY_DSN config.traces_sample_rate = 1.0 + config.profiles_sample_rate = 1.0 end diff --git a/spec/requests/home_spec.rb b/spec/requests/home_spec.rb index 7a276c88..102ecafe 100644 --- a/spec/requests/home_spec.rb +++ b/spec/requests/home_spec.rb @@ -9,7 +9,7 @@ RSpec.describe 'Homes', type: :request do .to_return(status: 200, body: '[{"name": "1.0.0"}]', headers: {}) end - it 'returns http success' do + xit 'returns http success' do get '/' expect(response).to have_http_status(:success) diff --git a/spec/services/points/create_spec.rb b/spec/services/points/create_spec.rb index 12f383eb..3c1fa796 100644 --- a/spec/services/points/create_spec.rb +++ b/spec/services/points/create_spec.rb @@ -23,15 +23,15 @@ RSpec.describe Points::Create do lonlat: 'POINT(-0.1278 51.5074)', timestamp: timestamp, user_id: user.id, - created_at: anything, - updated_at: anything + created_at: Time.current, + updated_at: Time.current }, { lonlat: 'POINT(-74.006 40.7128)', timestamp: timestamp + 1.hour, user_id: user.id, - created_at: anything, - updated_at: anything + created_at: Time.current, + updated_at: Time.current } ] end @@ -43,20 +43,167 @@ RSpec.describe Points::Create do ] end - it 'processes the points and upserts them to the database' do - expect(Points::Params).to receive(:new).with(point_params, user.id).and_return(params_service) - expect(params_service).to receive(:call).and_return(processed_data) - expect(Point).to receive(:upsert_all) - .with( - processed_data, - unique_by: %i[lonlat timestamp user_id], - returning: Arel.sql('id, timestamp, ST_X(lonlat::geometry) AS longitude, ST_Y(lonlat::geometry) AS latitude') - ) - .and_return(upsert_result) + describe 'basic point creation' do + before do + allow(Points::Params).to receive(:new).with(point_params, user.id).and_return(params_service) + allow(params_service).to receive(:call).and_return(processed_data) + end - result = described_class.new(user, point_params).call + it 'initializes the params service with correct arguments' do + expect(Points::Params).to receive(:new).with(point_params, user.id) + described_class.new(user, point_params).call + end - expect(result).to eq(upsert_result) + it 'calls the params service' do + expect(params_service).to receive(:call) + described_class.new(user, point_params).call + end + + it 'upserts the processed data' do + expect(Point).to receive(:upsert_all) + .with( + processed_data, + unique_by: %i[lonlat timestamp user_id], + returning: Arel.sql( + 'id, timestamp, ST_X(lonlat::geometry) AS longitude, ST_Y(lonlat::geometry) AS latitude' + ) + ) + .and_return(upsert_result) + + described_class.new(user, point_params).call + end + + it 'returns the upsert result' do + allow(Point).to receive(:upsert_all).and_return(upsert_result) + result = described_class.new(user, point_params).call + expect(result).to eq(upsert_result) + end + end + + context 'with duplicate points' do + let(:duplicate_point_params) do + { + locations: [ + { lat: 51.5074, lon: -0.1278, timestamp: timestamp.iso8601 }, + { lat: 51.5074, lon: -0.1278, timestamp: timestamp.iso8601 }, # Duplicate + { lat: 40.7128, lon: -74.0060, timestamp: (timestamp + 1.hour).iso8601 } + ] + } + end + + let(:duplicate_processed_data) do + current_time = Time.current + [ + { + lonlat: 'POINT(-0.1278 51.5074)', + timestamp: timestamp, + user_id: user.id, + created_at: current_time, + updated_at: current_time + }, + { + lonlat: 'POINT(-0.1278 51.5074)', # Duplicate + timestamp: timestamp, + user_id: user.id, + created_at: current_time, + updated_at: current_time + }, + { + lonlat: 'POINT(-74.006 40.7128)', + timestamp: timestamp + 1.hour, + user_id: user.id, + created_at: current_time, + updated_at: current_time + } + ] + end + + let(:deduplicated_upsert_result) do + [ + Point.new(id: 1, lonlat: 'POINT(-0.1278 51.5074)', timestamp: timestamp), + Point.new(id: 2, lonlat: 'POINT(-74.006 40.7128)', timestamp: timestamp + 1.hour) + ] + end + + before do + allow_any_instance_of(Points::Params).to receive(:call).and_return(duplicate_processed_data) + end + + describe 'deduplication behavior' do + it 'reduces the number of points to unique combinations' do + expect(Point).to receive(:upsert_all) do |data, _options| + expect(data.size).to eq(2) + deduplicated_upsert_result + end + + described_class.new(user, duplicate_point_params).call + end + + it 'preserves the correct lonlat values' do + expect(Point).to receive(:upsert_all) do |data, _options| + expect(data.map { |d| d[:lonlat] }).to match_array(['POINT(-0.1278 51.5074)', 'POINT(-74.006 40.7128)']) + deduplicated_upsert_result + end + + described_class.new(user, duplicate_point_params).call + end + + it 'preserves the correct timestamps' do + expect(Point).to receive(:upsert_all) do |data, _options| + expect(data.map { |d| d[:timestamp] }).to match_array([timestamp, timestamp + 1.hour]) + deduplicated_upsert_result + end + + described_class.new(user, duplicate_point_params).call + end + + it 'maintains the correct user_id for all points' do + expect(Point).to receive(:upsert_all) do |data, _options| + expect(data.map { |d| d[:user_id] }).to all(eq(user.id)) + deduplicated_upsert_result + end + + described_class.new(user, duplicate_point_params).call + end + + it 'uses the correct unique constraint' do + expect(Point).to receive(:upsert_all) do |_data, options| + expect(options[:unique_by]).to eq(%i[lonlat timestamp user_id]) + deduplicated_upsert_result + end + + described_class.new(user, duplicate_point_params).call + end + + it 'uses the correct returning clause' do + expect(Point).to receive(:upsert_all) do |_data, options| + expect(options[:returning]).to eq( + Arel.sql('id, timestamp, ST_X(lonlat::geometry) AS longitude, ST_Y(lonlat::geometry) AS latitude') + ) + deduplicated_upsert_result + end + + described_class.new(user, duplicate_point_params).call + end + end + + describe 'database interaction' do + it 'creates only unique points' do + expect do + described_class.new(user, duplicate_point_params).call + end.to change(Point, :count).by(2) + end + + it 'creates points with correct coordinates' do + described_class.new(user, duplicate_point_params).call + points = Point.order(:timestamp).last(2) + + expect(points[0].lonlat.x).to be_within(0.0001).of(-0.1278) + expect(points[0].lonlat.y).to be_within(0.0001).of(51.5074) + expect(points[1].lonlat.x).to be_within(0.0001).of(-74.006) + expect(points[1].lonlat.y).to be_within(0.0001).of(40.7128) + end + end end context 'with large datasets' do From cf82be5b8d1fa02d7db701ce98822a7411661486 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 12 May 2025 21:54:25 +0200 Subject: [PATCH 05/41] Update version --- .app_version | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/.app_version b/.app_version index 4e8f395f..30f6cf8d 100644 --- a/.app_version +++ b/.app_version @@ -1 +1 @@ -0.26.0 +0.26.1 From ed7b6d6d2475a7bf4f226bc0f0bfd3a7f57fb826 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 12 May 2025 22:33:47 +0200 Subject: [PATCH 06/41] Add a `STORE_GEODATA` environment variable to control whether to store geodata in the database. --- CHANGELOG.md | 12 ++++++++++++ app/models/place.rb | 6 ------ app/models/point.rb | 2 +- app/services/jobs/create.rb | 4 +--- config/initializers/01_constants.rb | 1 + config/initializers/03_dawarich_settings.rb | 4 ++++ spec/models/place_spec.rb | 9 --------- spec/models/point_spec.rb | 15 ++++++++++++++- spec/services/jobs/create_spec.rb | 5 ++++- 9 files changed, 37 insertions(+), 21 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 5983d2ab..b150176a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,18 @@ and this project adheres to [Semantic Versioning](http://semver.org/). # 0.26.1 - 2025-05-12 +Geodata on demand + +- [x] Introduce a `STORE_GEODATA` environment variable to control whether to store geodata in the database. +- [ ] When `STORE_GEODATA` is disabled, each feature that uses geodata will now make a direct request to the geocoding service to calculate required data. +- [ ] When `STORE_GEODATA` is disabled, points are not being reverse geocoded on creation. +- [ ] 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 + +- Reverse geocoding is now working as on-demand job instead of storing the result in the database. + ## Fixed - Fixed a bug with an attempt to write points with same lonlat and timestamp from iOS app. #1170 diff --git a/app/models/place.rb b/app/models/place.rb index 836358c8..99f57756 100644 --- a/app/models/place.rb +++ b/app/models/place.rb @@ -22,12 +22,6 @@ class Place < ApplicationRecord lonlat.y end - def async_reverse_geocode - return unless DawarichSettings.reverse_geocoding_enabled? - - ReverseGeocodingJob.perform_later(self.class.to_s, id) - end - def reverse_geocoded? geodata.present? end diff --git a/app/models/point.rb b/app/models/point.rb index 38970d91..68ed47c9 100644 --- a/app/models/point.rb +++ b/app/models/point.rb @@ -28,7 +28,7 @@ class Point < ApplicationRecord scope :visited, -> { where.not(visit_id: nil) } scope :not_visited, -> { where(visit_id: nil) } - after_create :async_reverse_geocode + after_create :async_reverse_geocode, if: -> { DawarichSettings.store_geodata? } after_create_commit :broadcast_coordinates def self.without_raw_data diff --git a/app/services/jobs/create.rb b/app/services/jobs/create.rb index bbbcb15c..0d46b60f 100644 --- a/app/services/jobs/create.rb +++ b/app/services/jobs/create.rb @@ -21,8 +21,6 @@ class Jobs::Create raise InvalidJobName, 'Invalid job name' end - points.find_each(batch_size: 1_000) do |point| - point.async_reverse_geocode - end + points.find_each(&:async_reverse_geocode) end end diff --git a/config/initializers/01_constants.rb b/config/initializers/01_constants.rb index 6a475aa2..1f3203da 100644 --- a/config/initializers/01_constants.rb +++ b/config/initializers/01_constants.rb @@ -17,6 +17,7 @@ NOMINATIM_API_KEY = ENV.fetch('NOMINATIM_API_KEY', nil) NOMINATIM_API_USE_HTTPS = ENV.fetch('NOMINATIM_API_USE_HTTPS', 'true') == 'true' GEOAPIFY_API_KEY = ENV.fetch('GEOAPIFY_API_KEY', nil) +STORE_GEODATA = ENV.fetch('STORE_GEODATA', 'false') == 'true' # /Reverse geocoding settings SENTRY_DSN = ENV.fetch('SENTRY_DSN', nil) diff --git a/config/initializers/03_dawarich_settings.rb b/config/initializers/03_dawarich_settings.rb index 96ee30ee..19f7394e 100644 --- a/config/initializers/03_dawarich_settings.rb +++ b/config/initializers/03_dawarich_settings.rb @@ -32,5 +32,9 @@ class DawarichSettings def nominatim_enabled? @nominatim_enabled ||= NOMINATIM_API_HOST.present? end + + def store_geodata? + @store_geodata ||= STORE_GEODATA + end end end diff --git a/spec/models/place_spec.rb b/spec/models/place_spec.rb index 6787c3f9..cf6852c4 100644 --- a/spec/models/place_spec.rb +++ b/spec/models/place_spec.rb @@ -21,15 +21,6 @@ RSpec.describe Place, type: :model do describe 'methods' do let(:place) { create(:place, :with_geodata) } - describe '#async_reverse_geocode' do - before { allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) } - before { allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) } - - it 'updates address' do - expect { place.async_reverse_geocode }.to have_enqueued_job(ReverseGeocodingJob).with('Place', place.id) - end - end - describe '#osm_id' do it 'returns the osm_id' do expect(place.osm_id).to eq(5_762_449_774) diff --git a/spec/models/point_spec.rb b/spec/models/point_spec.rb index 7f5f03e9..92f36f04 100644 --- a/spec/models/point_spec.rb +++ b/spec/models/point_spec.rb @@ -45,7 +45,10 @@ RSpec.describe Point, type: :model do describe '#async_reverse_geocode' do let(:point) { build(:point) } - before { allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) } + before do + allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) + allow(DawarichSettings).to receive(:store_geodata?).and_return(true) + end it 'enqueues ReverseGeocodeJob with correct arguments' do point.save @@ -61,6 +64,16 @@ RSpec.describe Point, type: :model do expect { point.async_reverse_geocode }.to have_enqueued_job(ReverseGeocodingJob) end end + + context 'when reverse geocoding is disabled' do + before do + allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(false) + end + + it 'does not enqueue ReverseGeocodeJob' do + expect { point.save }.not_to have_enqueued_job(ReverseGeocodingJob) + end + end end describe '#lon' do diff --git a/spec/services/jobs/create_spec.rb b/spec/services/jobs/create_spec.rb index 84988ff3..ad28f498 100644 --- a/spec/services/jobs/create_spec.rb +++ b/spec/services/jobs/create_spec.rb @@ -4,7 +4,10 @@ require 'rails_helper' RSpec.describe Jobs::Create do describe '#call' do - before { allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) } + before do + allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) + allow(DawarichSettings).to receive(:store_geodata?).and_return(true) + end context 'when job_name is start_reverse_geocoding' do let(:user) { create(:user) } From aa521dba9b903b891c0287e7e18883a03dcc6d75 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 12 May 2025 22:49:30 +0200 Subject: [PATCH 07/41] Extract place name suggester --- app/models/place.rb | 4 - app/models/visit.rb | 4 - .../reverse_geocoding/places/fetch_data.rb | 4 - app/services/visits/detector.rb | 46 +----- app/services/visits/place_name_suggester.rb | 70 ++++++++ .../stats/_reverse_geocoding_stats.html.erb | 22 +-- .../visits/place_name_suggester_spec.rb | 153 ++++++++++++++++++ 7 files changed, 238 insertions(+), 65 deletions(-) create mode 100644 app/services/visits/place_name_suggester.rb create mode 100644 spec/services/visits/place_name_suggester_spec.rb diff --git a/app/models/place.rb b/app/models/place.rb index 99f57756..d7cde7f6 100644 --- a/app/models/place.rb +++ b/app/models/place.rb @@ -22,10 +22,6 @@ class Place < ApplicationRecord lonlat.y end - def reverse_geocoded? - geodata.present? - end - def osm_id geodata['properties']['osm_id'] end diff --git a/app/models/visit.rb b/app/models/visit.rb index 2ca3faf8..4e452a11 100644 --- a/app/models/visit.rb +++ b/app/models/visit.rb @@ -12,10 +12,6 @@ class Visit < ApplicationRecord enum :status, { suggested: 0, confirmed: 1, declined: 2 } - def reverse_geocoded? - place.geodata.present? - end - def coordinates points.pluck(:latitude, :longitude).map { [_1[0].to_f, _1[1].to_f] } end diff --git a/app/services/reverse_geocoding/places/fetch_data.rb b/app/services/reverse_geocoding/places/fetch_data.rb index 1390c29a..733ddde7 100644 --- a/app/services/reverse_geocoding/places/fetch_data.rb +++ b/app/services/reverse_geocoding/places/fetch_data.rb @@ -57,10 +57,6 @@ class ReverseGeocoding::Places::FetchData new_place.save! end - def reverse_geocoded? - place.geodata.present? - end - def find_place(place_data) found_place = Place.where( "geodata->'properties'->>'osm_id' = ?", place_data['properties']['osm_id'].to_s diff --git a/app/services/visits/detector.rb b/app/services/visits/detector.rb index 13a2d64b..eced74ba 100644 --- a/app/services/visits/detector.rb +++ b/app/services/visits/detector.rb @@ -7,10 +7,11 @@ module Visits MAXIMUM_VISIT_GAP = 30.minutes MINIMUM_POINTS_FOR_VISIT = 2 - attr_reader :points + attr_reader :points, :place_name_suggester def initialize(points) @points = points + @place_name_suggester = PlaceNameSuggester end def detect_potential_visits @@ -111,48 +112,7 @@ module Visits end def suggest_place_name(points) - # Get points with geodata - geocoded_points = points.select { |p| p.geodata.present? && !p.geodata.empty? } - return nil if geocoded_points.empty? - - # Extract all features from points' geodata - features = geocoded_points.flat_map do |point| - next [] unless point.geodata['features'].is_a?(Array) - - point.geodata['features'] - end.compact - - return nil if features.empty? - - # Group features by type and count occurrences - feature_counts = features.group_by { |f| f.dig('properties', 'type') } - .transform_values(&:size) - - # Find the most common feature type - most_common_type = feature_counts.max_by { |_, count| count }&.first - return nil unless most_common_type - - # Get all features of the most common type - common_features = features.select { |f| f.dig('properties', 'type') == most_common_type } - - # Group these features by name and get the most common one - name_counts = common_features.group_by { |f| f.dig('properties', 'name') } - .transform_values(&:size) - most_common_name = name_counts.max_by { |_, count| count }&.first - - return if most_common_name.blank? - - # If we have a name, try to get additional context - feature = common_features.find { |f| f.dig('properties', 'name') == most_common_name } - properties = feature['properties'] - - # Build a more descriptive name if possible - [ - most_common_name, - properties['street'], - properties['city'], - properties['state'] - ].compact.uniq.join(', ') + place_name_suggester.new(points).call end end end diff --git a/app/services/visits/place_name_suggester.rb b/app/services/visits/place_name_suggester.rb new file mode 100644 index 00000000..d86e5573 --- /dev/null +++ b/app/services/visits/place_name_suggester.rb @@ -0,0 +1,70 @@ +# frozen_string_literal: true + +module Visits + # Suggests names for places based on geodata from tracked points + class PlaceNameSuggester + def initialize(points) + @points = points + end + + def call + geocoded_points = extract_geocoded_points(points) + return nil if geocoded_points.empty? + + features = extract_features(geocoded_points) + return nil if features.empty? + + most_common_type = find_most_common_feature_type(features) + return nil unless most_common_type + + most_common_name = find_most_common_name(features, most_common_type) + return nil if most_common_name.blank? + + build_descriptive_name(features, most_common_type, most_common_name) + end + + private + + attr_reader :points + + def extract_geocoded_points(points) + points.select { |p| p.geodata.present? && !p.geodata.empty? } + end + + def extract_features(geocoded_points) + geocoded_points.flat_map do |point| + next [] unless point.geodata['features'].is_a?(Array) + + point.geodata['features'] + end.compact + end + + def find_most_common_feature_type(features) + feature_counts = features.group_by { |f| f.dig('properties', 'type') } + .transform_values(&:size) + feature_counts.max_by { |_, count| count }&.first + end + + def find_most_common_name(features, feature_type) + common_features = features.select { |f| f.dig('properties', 'type') == feature_type } + name_counts = common_features.group_by { |f| f.dig('properties', 'name') } + .transform_values(&:size) + name_counts.max_by { |_, count| count }&.first + end + + def build_descriptive_name(features, feature_type, name) + feature = features.find do |f| + f.dig('properties', 'type') == feature_type && + f.dig('properties', 'name') == name + end + + properties = feature['properties'] + [ + name, + properties['street'], + properties['city'], + properties['state'] + ].compact.uniq.join(', ') + end + end +end diff --git a/app/views/stats/_reverse_geocoding_stats.html.erb b/app/views/stats/_reverse_geocoding_stats.html.erb index d7fb145e..11d3768f 100644 --- a/app/views/stats/_reverse_geocoding_stats.html.erb +++ b/app/views/stats/_reverse_geocoding_stats.html.erb @@ -1,14 +1,16 @@ -
-
- <%= number_with_delimiter @points_reverse_geocoded %> +<% if DawarichSettings.store_geodata? %> +
+
+ <%= number_with_delimiter @points_reverse_geocoded %> +
+
Reverse geocoded points
+
+ + <%= number_with_delimiter @points_reverse_geocoded_without_data %> points without data + +
-
Reverse geocoded points
-
- - <%= number_with_delimiter @points_reverse_geocoded_without_data %> points without data - -
-
+<% end %>
diff --git a/spec/services/visits/place_name_suggester_spec.rb b/spec/services/visits/place_name_suggester_spec.rb new file mode 100644 index 00000000..8b326d8a --- /dev/null +++ b/spec/services/visits/place_name_suggester_spec.rb @@ -0,0 +1,153 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Visits::PlaceNameSuggester do + subject(:suggester) { described_class.new(points) } + + describe '#call' do + context 'when no points have geodata' do + let(:points) do + [ + double('Point', geodata: nil), + double('Point', geodata: {}) + ] + end + + it 'returns nil' do + expect(suggester.call).to be_nil + end + end + + context 'when points have geodata but no features' do + let(:points) do + [ + double('Point', geodata: { 'features' => [] }) + ] + end + + it 'returns nil' do + expect(suggester.call).to be_nil + end + end + + 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' } } + ] + }) + ] + 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 + + 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' } } + ] + }) + ] + end + + it 'returns the most common name' do + expect(suggester.call).to eq('Central Park') + end + end + + 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' + } + } + ] + }) + ] + end + + it 'returns a descriptive name with all components' do + expect(suggester.call).to eq('Starbucks, 123 Main St, San Francisco, CA') + end + end + + 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 + } + } + ] + }) + ] + end + + it 'returns a name with available components' do + expect(suggester.call).to eq('Starbucks, San Francisco') + end + end + + context 'when points have geodata with non-array features' do + let(:points) do + [ + double('Point', geodata: { 'features' => 'not an array' }) + ] + end + + it 'returns nil' do + expect(suggester.call).to be_nil + end + end + + context 'when most common name is blank' do + let(:points) do + [ + double('Point', geodata: { + 'features' => [ + { 'properties' => { 'type' => 'road', 'name' => '' } } + ] + }) + ] + end + + it 'returns nil' do + expect(suggester.call).to be_nil + end + end + end +end From 857f1da9429da61e7d44f2a25312b3e528f12406 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 12 May 2025 23:36:46 +0200 Subject: [PATCH 08/41] Unify name builder usage --- app/serializers/api/place_serializer.rb | 20 +-- app/services/visits/detector.rb | 8 +- app/services/visits/names/builder.rb | 68 ++++++++++ app/services/visits/names/fetcher.rb | 56 ++++++++ app/services/visits/names/suggester.rb | 59 ++++++++ app/services/visits/place_finder.rb | 38 +++--- app/services/visits/place_name_suggester.rb | 70 ---------- spec/services/visits/names/builder_spec.rb | 126 ++++++++++++++++++ .../suggester_spec.rb} | 2 +- spec/services/visits/place_finder_spec.rb | 2 +- 10 files changed, 349 insertions(+), 100 deletions(-) create mode 100644 app/services/visits/names/builder.rb create mode 100644 app/services/visits/names/fetcher.rb create mode 100644 app/services/visits/names/suggester.rb delete mode 100644 app/services/visits/place_name_suggester.rb create mode 100644 spec/services/visits/names/builder_spec.rb rename spec/services/visits/{place_name_suggester_spec.rb => names/suggester_spec.rb} (98%) diff --git a/app/serializers/api/place_serializer.rb b/app/serializers/api/place_serializer.rb index 2857379a..787d5815 100644 --- a/app/serializers/api/place_serializer.rb +++ b/app/serializers/api/place_serializer.rb @@ -7,15 +7,17 @@ class Api::PlaceSerializer def call { - id: place.id, - name: place.name, - longitude: place.lon, - latitude: place.lat, - city: place.city, - country: place.country, - source: place.source, - geodata: place.geodata, - reverse_geocoded_at: place.reverse_geocoded_at + id: place.id, + name: place.name, + longitude: place.lon, + latitude: place.lat, + city: place.city, + country: place.country, + source: place.source, + geodata: place.geodata, + reverse_geocoded_at: place.reverse_geocoded_at, + created_at: place.created_at, + updated_at: place.updated_at } end diff --git a/app/services/visits/detector.rb b/app/services/visits/detector.rb index eced74ba..2a75855a 100644 --- a/app/services/visits/detector.rb +++ b/app/services/visits/detector.rb @@ -11,7 +11,7 @@ module Visits def initialize(points) @points = points - @place_name_suggester = PlaceNameSuggester + @place_name_suggester = Visits::Names::Suggester end def detect_potential_visits @@ -90,7 +90,7 @@ module Visits center_lat: center[0], center_lon: center[1], radius: calculate_visit_radius(points, center), - suggested_name: suggest_place_name(points) + suggested_name: suggest_place_name(points) || fetch_place_name(center) ) end @@ -114,5 +114,9 @@ module Visits def suggest_place_name(points) place_name_suggester.new(points).call end + + def fetch_place_name(center) + Visits::Names::Fetcher.new(center).call + end end end diff --git a/app/services/visits/names/builder.rb b/app/services/visits/names/builder.rb new file mode 100644 index 00000000..c3b6bf93 --- /dev/null +++ b/app/services/visits/names/builder.rb @@ -0,0 +1,68 @@ +# frozen_string_literal: true + +module Visits + module Names + # Builds descriptive names for places from geodata features + class Builder + def self.build_from_properties(properties) + return nil if properties.blank? + + name_components = [ + properties['name'], + properties['street'], + properties['housenumber'], + properties['city'], + properties['state'] + ].compact.reject(&:empty?).uniq + + name_components.any? ? name_components.join(', ') : nil + end + + def initialize(features, feature_type, name) + @features = features + @feature_type = feature_type + @name = name + end + + def call + return nil if features.blank? || feature_type.blank? || name.blank? + return nil unless feature + + [ + name, + properties['street'], + properties['city'], + properties['state'] + ].compact.uniq.join(', ') + end + + private + + attr_reader :features, :feature_type, :name + + def feature + @feature ||= find_feature + end + + def find_feature + features.find do |f| + f.dig('properties', 'type') == feature_type && + f.dig('properties', 'name') == name + end || find_feature_by_osm_value + end + + def find_feature_by_osm_value + features.find do |f| + f.dig('properties', 'osm_value') == feature_type && + f.dig('properties', 'name') == name + end + end + + def properties + return {} unless feature && feature['properties'].is_a?(Hash) + + feature['properties'] + end + end + end +end diff --git a/app/services/visits/names/fetcher.rb b/app/services/visits/names/fetcher.rb new file mode 100644 index 00000000..6a3ed157 --- /dev/null +++ b/app/services/visits/names/fetcher.rb @@ -0,0 +1,56 @@ +# frozen_string_literal: true + +module Visits + module Names + # Fetches names for places from reverse geocoding API + class Fetcher + def initialize(center) + @center = center + end + + def call + return nil if geocoder_results.blank? + + build_place_name + end + + private + + attr_reader :center + + def geocoder_results + @geocoder_results ||= Geocoder.search( + center, limit: 10, distance_sort: true, radius: 1, units: ::DISTANCE_UNIT + ) + end + + def build_place_name + return nil if geocoder_results.first&.data.blank? + + properties = geocoder_results.first.data['properties'] + return nil unless properties.present? + + # First try the direct properties approach + name = Visits::Names::Builder.build_from_properties(properties) + return name if name.present? + + # Fall back to the instance-based approach + return nil unless properties['name'] && properties['osm_value'] + + Visits::Names::Builder.new( + features, + properties['osm_value'], + properties['name'] + ).call + end + + def features + geocoder_results.map do |result| + { + 'properties' => result.data['properties'] + } + end.compact + end + end + end +end diff --git a/app/services/visits/names/suggester.rb b/app/services/visits/names/suggester.rb new file mode 100644 index 00000000..f3c7a945 --- /dev/null +++ b/app/services/visits/names/suggester.rb @@ -0,0 +1,59 @@ +# frozen_string_literal: true + +module Visits + module Names + # Suggests names for places based on geodata from tracked points + class Suggester + def initialize(points) + @points = points + end + + def call + geocoded_points = extract_geocoded_points(points) + return nil if geocoded_points.empty? + + features = extract_features(geocoded_points) + return nil if features.empty? + + most_common_type = find_most_common_feature_type(features) + return nil unless most_common_type + + most_common_name = find_most_common_name(features, most_common_type) + return nil if most_common_name.blank? + + Visits::Names::Builder.new( + features, most_common_type, most_common_name + ).call + end + + private + + attr_reader :points + + def extract_geocoded_points(points) + points.select { |p| p.geodata.present? && !p.geodata.empty? } + end + + def extract_features(geocoded_points) + geocoded_points.flat_map do |point| + next [] unless point.geodata['features'].is_a?(Array) + + point.geodata['features'] + end.compact + end + + def find_most_common_feature_type(features) + feature_counts = features.group_by { |f| f.dig('properties', 'type') } + .transform_values(&:size) + feature_counts.max_by { |_, count| count }&.first + end + + def find_most_common_name(features, feature_type) + common_features = features.select { |f| f.dig('properties', 'type') == feature_type } + name_counts = common_features.group_by { |f| f.dig('properties', 'name') } + .transform_values(&:size) + name_counts.max_by { |_, count| count }&.first + end + end + end +end diff --git a/app/services/visits/place_finder.rb b/app/services/visits/place_finder.rb index 72a35e72..86f0a547 100644 --- a/app/services/visits/place_finder.rb +++ b/app/services/visits/place_finder.rb @@ -51,7 +51,7 @@ module Visits return existing_by_location if existing_by_location # Then try by name if available - return nil unless name.present? + return nil if name.blank? Place.where(name: name) .near([lat, lon], SEARCH_RADIUS, :m) @@ -64,16 +64,13 @@ module Visits lon = visit_data[:center_lon] # Get places from points' geodata - places_from_points = extract_places_from_points(visit_data[:points], lat, lon) - - # Get places from external API - places_from_api = fetch_places_from_api(lat, lon) + places_from_points = extract_places_from_points(visit_data[:points]) # Combine and deduplicate by name combined_places = [] # Add API places first (usually better quality) - places_from_api.each do |api_place| + reverse_geocoded_places(lat, lon).each do |api_place| combined_places << api_place unless place_name_exists?(combined_places, api_place.name) end @@ -86,7 +83,7 @@ module Visits end # Step 3: Extract places from points - def extract_places_from_points(points, center_lat, center_lon) + def extract_places_from_points(points) return [] if points.blank? # Filter points with geodata @@ -101,7 +98,7 @@ module Visits places << place if place end - places.uniq { |place| place.name } + places.uniq(&:name) end # Step 4: Create place from point @@ -141,7 +138,7 @@ module Visits end # Step 5: Fetch places from API - def fetch_places_from_api(lat, lon) + def reverse_geocoded_places(lat, lon) # Get broader search results from Geocoder geocoder_results = Geocoder.search([lat, lon], units: :km, limit: 20, distance_sort: true) return [] if geocoder_results.blank? @@ -228,15 +225,22 @@ module Visits # Helper methods def build_place_name(properties) - name_components = [ - properties['name'], - properties['street'], - properties['housenumber'], - properties['postcode'], - properties['city'] - ].compact.reject(&:empty?).uniq + # First try building with our name builder + built_name = Visits::Names::Builder.build_from_properties(properties) + return built_name if built_name.present? - name_components.any? ? name_components.join(', ') : Place::DEFAULT_NAME + # Try using the instance-based approach as a fallback + features = [{ 'properties' => properties }] + feature_type = properties['type'] || properties['osm_value'] + name = properties['name'] + + if feature_type.present? && name.present? + built_name = Visits::Names::Builder.new(features, feature_type, name).call + return built_name if built_name.present? + end + + # Fallback to the default name if all else fails + Place::DEFAULT_NAME end def place_name_exists?(places, name) diff --git a/app/services/visits/place_name_suggester.rb b/app/services/visits/place_name_suggester.rb deleted file mode 100644 index d86e5573..00000000 --- a/app/services/visits/place_name_suggester.rb +++ /dev/null @@ -1,70 +0,0 @@ -# frozen_string_literal: true - -module Visits - # Suggests names for places based on geodata from tracked points - class PlaceNameSuggester - def initialize(points) - @points = points - end - - def call - geocoded_points = extract_geocoded_points(points) - return nil if geocoded_points.empty? - - features = extract_features(geocoded_points) - return nil if features.empty? - - most_common_type = find_most_common_feature_type(features) - return nil unless most_common_type - - most_common_name = find_most_common_name(features, most_common_type) - return nil if most_common_name.blank? - - build_descriptive_name(features, most_common_type, most_common_name) - end - - private - - attr_reader :points - - def extract_geocoded_points(points) - points.select { |p| p.geodata.present? && !p.geodata.empty? } - end - - def extract_features(geocoded_points) - geocoded_points.flat_map do |point| - next [] unless point.geodata['features'].is_a?(Array) - - point.geodata['features'] - end.compact - end - - def find_most_common_feature_type(features) - feature_counts = features.group_by { |f| f.dig('properties', 'type') } - .transform_values(&:size) - feature_counts.max_by { |_, count| count }&.first - end - - def find_most_common_name(features, feature_type) - common_features = features.select { |f| f.dig('properties', 'type') == feature_type } - name_counts = common_features.group_by { |f| f.dig('properties', 'name') } - .transform_values(&:size) - name_counts.max_by { |_, count| count }&.first - end - - def build_descriptive_name(features, feature_type, name) - feature = features.find do |f| - f.dig('properties', 'type') == feature_type && - f.dig('properties', 'name') == name - end - - properties = feature['properties'] - [ - name, - properties['street'], - properties['city'], - properties['state'] - ].compact.uniq.join(', ') - end - end -end diff --git a/spec/services/visits/names/builder_spec.rb b/spec/services/visits/names/builder_spec.rb new file mode 100644 index 00000000..47d2586c --- /dev/null +++ b/spec/services/visits/names/builder_spec.rb @@ -0,0 +1,126 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Visits::Names::Builder do + describe '.build_from_properties' do + it 'builds a name from all available properties' do + properties = { + 'name' => 'Coffee Shop', + 'street' => 'Main St', + 'housenumber' => '123', + 'city' => 'New York', + 'state' => 'NY' + } + + result = described_class.build_from_properties(properties) + expect(result).to eq('Coffee Shop, Main St, 123, New York, NY') + end + + it 'handles missing properties' do + properties = { + 'name' => 'Coffee Shop', + 'city' => 'New York', + 'state' => 'NY' + } + + result = described_class.build_from_properties(properties) + expect(result).to eq('Coffee Shop, New York, NY') + end + + it 'deduplicates components' do + properties = { + 'name' => 'New York Cafe', + 'city' => 'New York', + 'state' => 'NY' + } + + result = described_class.build_from_properties(properties) + expect(result).to eq('New York Cafe, New York, NY') + end + + it 'returns nil for empty properties' do + result = described_class.build_from_properties({}) + expect(result).to be_nil + end + + it 'returns nil for nil properties' do + result = described_class.build_from_properties(nil) + expect(result).to be_nil + end + end + + describe '#call' do + subject { described_class.new(features, feature_type, name).call } + + let(:feature_type) { 'amenity' } + let(:name) { 'Coffee Shop' } + let(:features) do + [ + { + 'properties' => { + 'type' => 'amenity', + 'name' => 'Coffee Shop', + 'street' => '123 Main St', + 'city' => 'San Francisco', + 'state' => 'CA' + } + }, + { + 'properties' => { + 'type' => 'park', + 'name' => 'Central Park', + 'city' => 'New York', + 'state' => 'NY' + } + } + ] + end + + it 'returns a descriptive name with all available components' do + expect(subject).to eq('Coffee Shop, 123 Main St, San Francisco, CA') + end + + context 'when feature uses osm_value instead of type' do + let(:features) do + [ + { + 'properties' => { + 'osm_value' => 'amenity', + 'name' => 'Coffee Shop', + 'street' => '123 Main St', + 'city' => 'San Francisco', + 'state' => 'CA' + } + } + ] + end + + it 'finds the feature using osm_value' do + expect(subject).to eq('Coffee Shop, 123 Main St, San Francisco, CA') + end + end + + context 'when no matching feature is found' do + let(:name) { 'Non-existent Shop' } + + it 'returns nil' do + expect(subject).to be_nil + end + end + + context 'with empty inputs' do + it 'returns nil for empty features' do + expect(described_class.new([], feature_type, name).call).to be_nil + end + + it 'returns nil for blank feature_type' do + expect(described_class.new(features, '', name).call).to be_nil + end + + it 'returns nil for blank name' do + expect(described_class.new(features, feature_type, '').call).to be_nil + end + end + end +end diff --git a/spec/services/visits/place_name_suggester_spec.rb b/spec/services/visits/names/suggester_spec.rb similarity index 98% rename from spec/services/visits/place_name_suggester_spec.rb rename to spec/services/visits/names/suggester_spec.rb index 8b326d8a..dff5ff91 100644 --- a/spec/services/visits/place_name_suggester_spec.rb +++ b/spec/services/visits/names/suggester_spec.rb @@ -2,7 +2,7 @@ require 'rails_helper' -RSpec.describe Visits::PlaceNameSuggester do +RSpec.describe Visits::Names::Suggester do subject(:suggester) { described_class.new(points) } describe '#call' do diff --git a/spec/services/visits/place_finder_spec.rb b/spec/services/visits/place_finder_spec.rb index 493ceec5..b924ffae 100644 --- a/spec/services/visits/place_finder_spec.rb +++ b/spec/services/visits/place_finder_spec.rb @@ -75,7 +75,7 @@ RSpec.describe Visits::PlaceFinder do before do allow(Geocoder).to receive(:search).and_return([]) - allow(subject).to receive(:fetch_places_from_api).and_return([]) + allow(subject).to receive(:reverse_geocoded_places).and_return([]) end it 'extracts and creates places from point geodata' do From 79f2522f54d452b45fdd0cb32cf846ee886c59c0 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Tue, 13 May 2025 19:43:02 +0200 Subject: [PATCH 09/41] Fetch countries for a trip via geocoding service --- CHANGELOG.md | 5 + Gemfile | 2 + Gemfile.lock | 6 + app/jobs/trips/create_path_job.rb | 2 +- app/models/trip.rb | 15 +- .../reverse_geocoding/places/fetch_data.rb | 32 +- app/services/trips/countries.rb | 128 +++++ app/services/visits/names/fetcher.rb | 7 +- app/views/home/index.html.erb | 6 +- ...13164521_add_visited_countries_to_trips.rb | 7 + db/schema.rb | 448 +++++++++--------- spec/models/trip_spec.rb | 24 + spec/rails_helper.rb | 1 + spec/services/trips/countries_spec.rb | 108 +++++ spec/support/geocoder_stubs.rb | 30 ++ 15 files changed, 579 insertions(+), 242 deletions(-) create mode 100644 app/services/trips/countries.rb create mode 100644 db/migrate/20250513164521_add_visited_countries_to_trips.rb create mode 100644 spec/services/trips/countries_spec.rb create mode 100644 spec/support/geocoder_stubs.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index b150176a..cdb8c5c3 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -11,7 +11,12 @@ Geodata on demand - [x] Introduce a `STORE_GEODATA` environment variable to control whether to store geodata in the database. - [ ] When `STORE_GEODATA` is disabled, each feature that uses geodata will now make a direct request to the geocoding service to calculate required data. + 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. - [ ] 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. diff --git a/Gemfile b/Gemfile index e651c4df..1759ef3f 100644 --- a/Gemfile +++ b/Gemfile @@ -30,6 +30,8 @@ gem 'rails', '~> 8.0' gem 'rexml' gem 'rgeo' gem 'rgeo-activerecord' +gem 'rgeo-geojson' +gem 'parallel' gem 'rswag-api' gem 'rswag-ui' gem 'sentry-ruby' diff --git a/Gemfile.lock b/Gemfile.lock index 1128ba27..284b3dba 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -219,6 +219,7 @@ GEM mini_portile2 (2.8.8) minitest (5.25.5) msgpack (1.7.3) + multi_json (1.15.0) multi_xml (0.7.1) bigdecimal (~> 3.1) net-imap (0.5.8) @@ -339,6 +340,9 @@ GEM rgeo-activerecord (8.0.0) activerecord (>= 7.0) rgeo (>= 3.0) + rgeo-geojson (2.2.0) + multi_json (~> 1.15) + rgeo (>= 1.0.0) rspec-core (3.13.3) rspec-support (~> 3.13.0) rspec-expectations (3.13.3) @@ -502,6 +506,7 @@ DEPENDENCIES kaminari lograge oj + parallel pg prometheus_exporter pry-byebug @@ -513,6 +518,7 @@ DEPENDENCIES rexml rgeo rgeo-activerecord + rgeo-geojson rspec-rails rswag-api rswag-specs diff --git a/app/jobs/trips/create_path_job.rb b/app/jobs/trips/create_path_job.rb index d64a39ec..5c73dc90 100644 --- a/app/jobs/trips/create_path_job.rb +++ b/app/jobs/trips/create_path_job.rb @@ -6,7 +6,7 @@ class Trips::CreatePathJob < ApplicationJob def perform(trip_id) trip = Trip.find(trip_id) - trip.calculate_path_and_distance + trip.calculate_trip_data trip.save! end diff --git a/app/models/trip.rb b/app/models/trip.rb index 098feb82..caab72ab 100644 --- a/app/models/trip.rb +++ b/app/models/trip.rb @@ -7,11 +7,12 @@ class Trip < ApplicationRecord validates :name, :started_at, :ended_at, presence: true - before_save :calculate_path_and_distance + before_save :calculate_trip_data - def calculate_path_and_distance + def calculate_trip_data calculate_path calculate_distance + calculate_countries end def points @@ -19,7 +20,9 @@ class Trip < ApplicationRecord end def countries - points.pluck(:country).uniq.compact + return points.pluck(:country).uniq.compact if DawarichSettings.store_geodata? + + visited_countries end def photo_previews @@ -56,4 +59,10 @@ class Trip < ApplicationRecord self.distance = distance.round end + + def calculate_countries + countries = Trips::Countries.new(self).call + + self.visited_countries = countries + end end diff --git a/app/services/reverse_geocoding/places/fetch_data.rb b/app/services/reverse_geocoding/places/fetch_data.rb index 733ddde7..72195437 100644 --- a/app/services/reverse_geocoding/places/fetch_data.rb +++ b/app/services/reverse_geocoding/places/fetch_data.rb @@ -17,10 +17,19 @@ class ReverseGeocoding::Places::FetchData return end - first_place = reverse_geocoded_places.shift + places = reverse_geocoded_places + first_place = places.shift update_place(first_place) - reverse_geocoded_places.each { |reverse_geocoded_place| fetch_and_create_place(reverse_geocoded_place) } + # Extract all osm_ids for preloading + osm_ids = places.map { |place| place.data['properties']['osm_id'].to_s } + + # Preload all existing places with these osm_ids in a single query + existing_places = Place.where("geodata->'properties'->>'osm_id' IN (?)", osm_ids) + .index_by { |p| p.geodata.dig('properties', 'osm_id').to_s } + + # Process with preloaded data + places.each { |reverse_geocoded_place| fetch_and_create_place(reverse_geocoded_place, existing_places) } end private @@ -41,9 +50,9 @@ class ReverseGeocoding::Places::FetchData ) end - def fetch_and_create_place(reverse_geocoded_place) + def fetch_and_create_place(reverse_geocoded_place, existing_places = nil) data = reverse_geocoded_place.data - new_place = find_place(data) + new_place = find_place(data, existing_places) new_place.name = place_name(data) new_place.city = data['properties']['city'] @@ -57,12 +66,17 @@ class ReverseGeocoding::Places::FetchData new_place.save! end - def find_place(place_data) - found_place = Place.where( - "geodata->'properties'->>'osm_id' = ?", place_data['properties']['osm_id'].to_s - ).first + def find_place(place_data, existing_places = nil) + osm_id = place_data['properties']['osm_id'].to_s - return found_place if found_place.present? + # Use the preloaded data if available + if existing_places + return existing_places[osm_id] if existing_places[osm_id].present? + else + # Fall back to individual query if no preloaded data + found_place = Place.where("geodata->'properties'->>'osm_id' = ?", osm_id).first + return found_place if found_place.present? + end Place.find_or_initialize_by( lonlat: "POINT(#{place_data['geometry']['coordinates'][0].to_f.round(5)} #{place_data['geometry']['coordinates'][1].to_f.round(5)})", diff --git a/app/services/trips/countries.rb b/app/services/trips/countries.rb new file mode 100644 index 00000000..2ed18cbb --- /dev/null +++ b/app/services/trips/countries.rb @@ -0,0 +1,128 @@ +# 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') + + def initialize(trip, batch_count = 2) + @trip = trip + @batch_count = batch_count + @factory = RGeo::Geographic.spherical_factory + @file = File.read(FILE_PATH) + @countries_features = + RGeo::GeoJSON.decode(@file, json_parser: :json, geo_factory: @factory) + end + + def call + all_points = @trip.points.to_a + total_points = all_points.size + + # Return empty hash if no points + return {} if total_points.zero? + + batches = split_into_batches(all_points, @batch_count) + threads_results = process_batches_in_threads(batches, total_points) + country_counts = merge_thread_results(threads_results) + + log_results(country_counts, total_points) + country_counts.sort_by { |_country, count| -count }.to_h + end + + private + + def split_into_batches(points, batch_count) + batch_count = [batch_count, 1].max # Ensure batch_count is at least 1 + batch_size = (points.size / batch_count.to_f).ceil + points.each_slice(batch_size).to_a + end + + def process_batches_in_threads(batches, total_points) + threads_results = [] + threads = [] + + batches.each_with_index do |batch, batch_index| + start_index = batch_index * batch.size + 1 + threads << Thread.new do + threads_results << process_batch(batch, start_index, total_points) + end + end + + threads.each(&:join) + threads_results + end + + def merge_thread_results(threads_results) + country_counts = {} + + threads_results.each do |result| + result.each do |country, count| + country_counts[country] ||= 0 + country_counts[country] += count + end + end + + country_counts + end + + def log_results(country_counts, total_points) + total_counted = country_counts.values.sum + Rails.logger.info("Processed #{total_points} points and found #{country_counts.size} countries") + Rails.logger.info("Points counted: #{total_counted} out of #{total_points}") + end + + def process_batch(points, start_index, total_points) + country_counts = {} + + points.each_with_index do |point, idx| + current_index = start_index + idx + country_code = geocode_point(point, current_index, total_points) + next unless country_code + + country_counts[country_code] ||= 0 + country_counts[country_code] += 1 + end + + country_counts + end + + def geocode_point(point, current_index, total_points) + lonlat = point.lonlat + return nil unless lonlat + + latitude = lonlat.y + longitude = lonlat.x + + log_processing_point(current_index, total_points, latitude, longitude) + country_code = fetch_country_code(latitude, longitude) + log_found_country(country_code, latitude, longitude) if country_code + + country_code + end + + def log_processing_point(current_index, total_points, latitude, longitude) + thread_id = Thread.current.object_id + Rails.logger.info( + "Thread #{thread_id}: Processing point #{current_index} of #{total_points}: lat=#{latitude}, lon=#{longitude}" + ) + end + + def log_found_country(country_code, latitude, longitude) + thread_id = Thread.current.object_id + Rails.logger.info("Thread #{thread_id}: Found country: #{country_code} for point at #{latitude}, #{longitude}") + end + + def fetch_country_code(latitude, longitude) + results = Geocoder.search([latitude, longitude], limit: 1) + return nil unless results.any? + + result = results.first + result.data['properties']['countrycode'] + rescue StandardError => e + Rails.logger.error("Error geocoding point: #{e.message}") + nil + end +end diff --git a/app/services/visits/names/fetcher.rb b/app/services/visits/names/fetcher.rb index 6a3ed157..7e970551 100644 --- a/app/services/visits/names/fetcher.rb +++ b/app/services/visits/names/fetcher.rb @@ -27,8 +27,7 @@ module Visits def build_place_name return nil if geocoder_results.first&.data.blank? - properties = geocoder_results.first.data['properties'] - return nil unless properties.present? + return nil if properties.blank? # First try the direct properties approach name = Visits::Names::Builder.build_from_properties(properties) @@ -51,6 +50,10 @@ module Visits } end.compact end + + def properties + @properties ||= geocoder_results.first.data['properties'] + end end end end diff --git a/app/views/home/index.html.erb b/app/views/home/index.html.erb index f290fc01..259b784d 100644 --- a/app/views/home/index.html.erb +++ b/app/views/home/index.html.erb @@ -8,8 +8,10 @@

The only location history tracker you'll ever need.

- <%= link_to 'Sign up', new_user_registration_path, class: "rounded-lg py-3 px-5 my-3 bg-blue-600 text-white block font-medium" %> -
or
+ <% if !DawarichSettings.self_hosted? %> + <%= link_to 'Sign up', new_user_registration_path, class: "rounded-lg py-3 px-5 my-3 bg-blue-600 text-white block font-medium" %> +
or
+ <% end %> <%= link_to 'Sign in', new_user_session_path, class: "rounded-lg py-3 px-5 bg-neutral text-neutral-content block font-medium" %>
diff --git a/db/migrate/20250513164521_add_visited_countries_to_trips.rb b/db/migrate/20250513164521_add_visited_countries_to_trips.rb new file mode 100644 index 00000000..27072d3e --- /dev/null +++ b/db/migrate/20250513164521_add_visited_countries_to_trips.rb @@ -0,0 +1,7 @@ +# frozen_string_literal: true + +class AddVisitedCountriesToTrips < ActiveRecord::Migration[8.0] + def change + add_column :trips, :visited_countries, :jsonb, default: [] + end +end diff --git a/db/schema.rb b/db/schema.rb index 68eac3a8..85f06f62 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -10,264 +10,262 @@ # # It's strongly recommended that you check this file into your version control system. -ActiveRecord::Schema[8.0].define(version: 20_250_404_182_437) do +ActiveRecord::Schema[8.0].define(version: 2025_05_13_164521) do # These are extensions that must be enabled in order to support this database - enable_extension 'pg_catalog.plpgsql' - enable_extension 'postgis' + enable_extension "pg_catalog.plpgsql" + enable_extension "postgis" - create_table 'action_text_rich_texts', force: :cascade do |t| - t.string 'name', null: false - t.text 'body' - t.string 'record_type', null: false - t.bigint 'record_id', null: false - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.index %w[record_type record_id name], name: 'index_action_text_rich_texts_uniqueness', unique: true + create_table "action_text_rich_texts", force: :cascade do |t| + t.string "name", null: false + t.text "body" + t.string "record_type", null: false + t.bigint "record_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["record_type", "record_id", "name"], name: "index_action_text_rich_texts_uniqueness", unique: true end - create_table 'active_storage_attachments', force: :cascade do |t| - t.string 'name', null: false - t.string 'record_type', null: false - t.bigint 'record_id', null: false - t.bigint 'blob_id', null: false - t.datetime 'created_at', null: false - t.index ['blob_id'], name: 'index_active_storage_attachments_on_blob_id' - t.index %w[record_type record_id name blob_id], name: 'index_active_storage_attachments_uniqueness', -unique: true + create_table "active_storage_attachments", force: :cascade do |t| + t.string "name", null: false + t.string "record_type", null: false + t.bigint "record_id", null: false + t.bigint "blob_id", null: false + t.datetime "created_at", null: false + t.index ["blob_id"], name: "index_active_storage_attachments_on_blob_id" + t.index ["record_type", "record_id", "name", "blob_id"], name: "index_active_storage_attachments_uniqueness", unique: true end - create_table 'active_storage_blobs', force: :cascade do |t| - t.string 'key', null: false - t.string 'filename', null: false - t.string 'content_type' - t.text 'metadata' - t.string 'service_name', null: false - t.bigint 'byte_size', null: false - t.string 'checksum' - t.datetime 'created_at', null: false - t.index ['key'], name: 'index_active_storage_blobs_on_key', unique: true + create_table "active_storage_blobs", force: :cascade do |t| + t.string "key", null: false + t.string "filename", null: false + t.string "content_type" + t.text "metadata" + t.string "service_name", null: false + t.bigint "byte_size", null: false + t.string "checksum" + t.datetime "created_at", null: false + t.index ["key"], name: "index_active_storage_blobs_on_key", unique: true end - create_table 'active_storage_variant_records', force: :cascade do |t| - t.bigint 'blob_id', null: false - t.string 'variation_digest', null: false - t.index %w[blob_id variation_digest], name: 'index_active_storage_variant_records_uniqueness', unique: true + create_table "active_storage_variant_records", force: :cascade do |t| + t.bigint "blob_id", null: false + t.string "variation_digest", null: false + t.index ["blob_id", "variation_digest"], name: "index_active_storage_variant_records_uniqueness", unique: true end - create_table 'areas', force: :cascade do |t| - t.string 'name', null: false - t.bigint 'user_id', null: false - t.decimal 'longitude', precision: 10, scale: 6, null: false - t.decimal 'latitude', precision: 10, scale: 6, null: false - t.integer 'radius', null: false - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.index ['user_id'], name: 'index_areas_on_user_id' + create_table "areas", force: :cascade do |t| + t.string "name", null: false + t.bigint "user_id", null: false + t.decimal "longitude", precision: 10, scale: 6, null: false + t.decimal "latitude", precision: 10, scale: 6, null: false + t.integer "radius", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["user_id"], name: "index_areas_on_user_id" end - create_table 'data_migrations', primary_key: 'version', id: :string, force: :cascade do |t| + create_table "data_migrations", primary_key: "version", id: :string, force: :cascade do |t| end - create_table 'exports', force: :cascade do |t| - t.string 'name', null: false - t.string 'url' - t.integer 'status', default: 0, null: false - t.bigint 'user_id', null: false - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.integer 'file_format', default: 0 - t.datetime 'start_at' - t.datetime 'end_at' - t.index ['status'], name: 'index_exports_on_status' - t.index ['user_id'], name: 'index_exports_on_user_id' + create_table "exports", force: :cascade do |t| + t.string "name", null: false + t.string "url" + t.integer "status", default: 0, null: false + t.bigint "user_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.integer "file_format", default: 0 + t.datetime "start_at" + t.datetime "end_at" + t.index ["status"], name: "index_exports_on_status" + t.index ["user_id"], name: "index_exports_on_user_id" end - create_table 'imports', force: :cascade do |t| - t.string 'name', null: false - t.bigint 'user_id', null: false - t.integer 'source', default: 0 - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.integer 'raw_points', default: 0 - t.integer 'doubles', default: 0 - t.integer 'processed', default: 0 - t.jsonb 'raw_data' - t.integer 'points_count', default: 0 - t.index ['source'], name: 'index_imports_on_source' - t.index ['user_id'], name: 'index_imports_on_user_id' + create_table "imports", force: :cascade do |t| + t.string "name", null: false + t.bigint "user_id", null: false + t.integer "source", default: 0 + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.integer "raw_points", default: 0 + t.integer "doubles", default: 0 + t.integer "processed", default: 0 + t.jsonb "raw_data" + t.integer "points_count", default: 0 + t.index ["source"], name: "index_imports_on_source" + t.index ["user_id"], name: "index_imports_on_user_id" end - create_table 'notifications', force: :cascade do |t| - t.string 'title', null: false - t.text 'content', null: false - t.bigint 'user_id', null: false - t.integer 'kind', default: 0, null: false - t.datetime 'read_at' - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.index ['kind'], name: 'index_notifications_on_kind' - t.index ['user_id'], name: 'index_notifications_on_user_id' + create_table "notifications", force: :cascade do |t| + t.string "title", null: false + t.text "content", null: false + t.bigint "user_id", null: false + t.integer "kind", default: 0, null: false + t.datetime "read_at" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["kind"], name: "index_notifications_on_kind" + t.index ["user_id"], name: "index_notifications_on_user_id" end - create_table 'place_visits', force: :cascade do |t| - t.bigint 'place_id', null: false - t.bigint 'visit_id', null: false - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.index ['place_id'], name: 'index_place_visits_on_place_id' - t.index ['visit_id'], name: 'index_place_visits_on_visit_id' + create_table "place_visits", force: :cascade do |t| + t.bigint "place_id", null: false + t.bigint "visit_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.index ["place_id"], name: "index_place_visits_on_place_id" + t.index ["visit_id"], name: "index_place_visits_on_visit_id" end - create_table 'places', force: :cascade do |t| - t.string 'name', null: false - t.decimal 'longitude', precision: 10, scale: 6, null: false - t.decimal 'latitude', precision: 10, scale: 6, null: false - t.string 'city' - t.string 'country' - t.integer 'source', default: 0 - t.jsonb 'geodata', default: {}, null: false - t.datetime 'reverse_geocoded_at' - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.geography 'lonlat', limit: { srid: 4326, type: 'st_point', geographic: true } - t.index ['lonlat'], name: 'index_places_on_lonlat', using: :gist + create_table "places", force: :cascade do |t| + t.string "name", null: false + t.decimal "longitude", precision: 10, scale: 6, null: false + t.decimal "latitude", precision: 10, scale: 6, null: false + t.string "city" + t.string "country" + t.integer "source", default: 0 + t.jsonb "geodata", default: {}, null: false + t.datetime "reverse_geocoded_at" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.geography "lonlat", limit: {srid: 4326, type: "st_point", geographic: true} + t.index ["lonlat"], name: "index_places_on_lonlat", using: :gist end - create_table 'points', force: :cascade do |t| - t.integer 'battery_status' - t.string 'ping' - t.integer 'battery' - t.string 'tracker_id' - t.string 'topic' - t.integer 'altitude' - t.decimal 'longitude', precision: 10, scale: 6 - t.string 'velocity' - t.integer 'trigger' - t.string 'bssid' - t.string 'ssid' - t.integer 'connection' - t.integer 'vertical_accuracy' - t.integer 'accuracy' - t.integer 'timestamp' - t.decimal 'latitude', precision: 10, scale: 6 - t.integer 'mode' - t.text 'inrids', default: [], array: true - t.text 'in_regions', default: [], array: true - t.jsonb 'raw_data', default: {} - t.bigint 'import_id' - t.string 'city' - t.string 'country' - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.bigint 'user_id' - t.jsonb 'geodata', default: {}, null: false - t.bigint 'visit_id' - t.datetime 'reverse_geocoded_at' - t.decimal 'course', precision: 8, scale: 5 - t.decimal 'course_accuracy', precision: 8, scale: 5 - t.string 'external_track_id' - t.geography 'lonlat', limit: { srid: 4326, type: 'st_point', geographic: true } - t.index ['altitude'], name: 'index_points_on_altitude' - t.index ['battery'], name: 'index_points_on_battery' - t.index ['battery_status'], name: 'index_points_on_battery_status' - t.index ['city'], name: 'index_points_on_city' - t.index ['connection'], name: 'index_points_on_connection' - t.index ['country'], name: 'index_points_on_country' - t.index ['external_track_id'], name: 'index_points_on_external_track_id' - t.index ['geodata'], name: 'index_points_on_geodata', using: :gin - t.index ['import_id'], name: 'index_points_on_import_id' - t.index %w[latitude longitude], name: 'index_points_on_latitude_and_longitude' - t.index %w[lonlat timestamp user_id], name: 'index_points_on_lonlat_timestamp_user_id', unique: true - t.index ['lonlat'], name: 'index_points_on_lonlat', using: :gist - t.index ['reverse_geocoded_at'], name: 'index_points_on_reverse_geocoded_at' - t.index ['timestamp'], name: 'index_points_on_timestamp' - t.index ['trigger'], name: 'index_points_on_trigger' - t.index ['user_id'], name: 'index_points_on_user_id' - t.index ['visit_id'], name: 'index_points_on_visit_id' + create_table "points", force: :cascade do |t| + t.integer "battery_status" + t.string "ping" + t.integer "battery" + t.string "tracker_id" + t.string "topic" + t.integer "altitude" + t.decimal "longitude", precision: 10, scale: 6 + t.string "velocity" + t.integer "trigger" + t.string "bssid" + t.string "ssid" + t.integer "connection" + t.integer "vertical_accuracy" + t.integer "accuracy" + t.integer "timestamp" + t.decimal "latitude", precision: 10, scale: 6 + t.integer "mode" + t.text "inrids", default: [], array: true + t.text "in_regions", default: [], array: true + t.jsonb "raw_data", default: {} + t.bigint "import_id" + t.string "city" + t.string "country" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.bigint "user_id" + t.jsonb "geodata", default: {}, null: false + t.bigint "visit_id" + t.datetime "reverse_geocoded_at" + t.decimal "course", precision: 8, scale: 5 + t.decimal "course_accuracy", precision: 8, scale: 5 + t.string "external_track_id" + t.geography "lonlat", limit: {srid: 4326, type: "st_point", geographic: true} + t.index ["altitude"], name: "index_points_on_altitude" + t.index ["battery"], name: "index_points_on_battery" + t.index ["battery_status"], name: "index_points_on_battery_status" + t.index ["city"], name: "index_points_on_city" + t.index ["connection"], name: "index_points_on_connection" + t.index ["country"], name: "index_points_on_country" + t.index ["external_track_id"], name: "index_points_on_external_track_id" + t.index ["geodata"], name: "index_points_on_geodata", using: :gin + t.index ["import_id"], name: "index_points_on_import_id" + t.index ["latitude", "longitude"], name: "index_points_on_latitude_and_longitude" + t.index ["lonlat", "timestamp", "user_id"], name: "index_points_on_lonlat_timestamp_user_id", unique: true + t.index ["lonlat"], name: "index_points_on_lonlat", using: :gist + t.index ["reverse_geocoded_at"], name: "index_points_on_reverse_geocoded_at" + t.index ["timestamp"], name: "index_points_on_timestamp" + t.index ["trigger"], name: "index_points_on_trigger" + t.index ["user_id"], name: "index_points_on_user_id" + t.index ["visit_id"], name: "index_points_on_visit_id" end - create_table 'stats', force: :cascade do |t| - t.integer 'year', null: false - t.integer 'month', null: false - t.integer 'distance', null: false - t.jsonb 'toponyms' - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.bigint 'user_id', null: false - t.jsonb 'daily_distance', default: {} - t.index ['distance'], name: 'index_stats_on_distance' - t.index ['month'], name: 'index_stats_on_month' - t.index ['user_id'], name: 'index_stats_on_user_id' - t.index ['year'], name: 'index_stats_on_year' + create_table "stats", force: :cascade do |t| + t.integer "year", null: false + t.integer "month", null: false + t.integer "distance", null: false + t.jsonb "toponyms" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.bigint "user_id", null: false + t.jsonb "daily_distance", default: {} + t.index ["distance"], name: "index_stats_on_distance" + t.index ["month"], name: "index_stats_on_month" + t.index ["user_id"], name: "index_stats_on_user_id" + t.index ["year"], name: "index_stats_on_year" end - create_table 'trips', force: :cascade do |t| - t.string 'name', null: false - t.datetime 'started_at', null: false - t.datetime 'ended_at', null: false - t.integer 'distance' - t.bigint 'user_id', null: false - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.geometry 'path', limit: { srid: 3857, type: 'line_string' } - t.index ['user_id'], name: 'index_trips_on_user_id' + create_table "trips", force: :cascade do |t| + t.string "name", null: false + t.datetime "started_at", null: false + t.datetime "ended_at", null: false + t.integer "distance" + t.bigint "user_id", null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.geometry "path", limit: {srid: 3857, type: "line_string"} + t.jsonb "visited_countries", default: [] + t.index ["user_id"], name: "index_trips_on_user_id" end - create_table 'users', force: :cascade do |t| - t.string 'email', default: '', null: false - t.string 'encrypted_password', default: '', null: false - t.string 'reset_password_token' - t.datetime 'reset_password_sent_at' - t.datetime 'remember_created_at' - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.string 'api_key', default: '', null: false - t.string 'theme', default: 'dark', null: false - t.jsonb 'settings', - default: { 'fog_of_war_meters' => '100', 'meters_between_routes' => '1000', -'minutes_between_routes' => '60' } - t.boolean 'admin', default: false - t.integer 'sign_in_count', default: 0, null: false - t.datetime 'current_sign_in_at' - t.datetime 'last_sign_in_at' - t.string 'current_sign_in_ip' - t.string 'last_sign_in_ip' - t.integer 'status', default: 0 - t.datetime 'active_until' - t.index ['email'], name: 'index_users_on_email', unique: true - t.index ['reset_password_token'], name: 'index_users_on_reset_password_token', unique: true + create_table "users", force: :cascade do |t| + t.string "email", default: "", null: false + t.string "encrypted_password", default: "", null: false + t.string "reset_password_token" + t.datetime "reset_password_sent_at" + t.datetime "remember_created_at" + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.string "api_key", default: "", null: false + t.string "theme", default: "dark", null: false + t.jsonb "settings", default: {"fog_of_war_meters" => "100", "meters_between_routes" => "1000", "minutes_between_routes" => "60"} + t.boolean "admin", default: false + t.integer "sign_in_count", default: 0, null: false + t.datetime "current_sign_in_at" + t.datetime "last_sign_in_at" + t.string "current_sign_in_ip" + t.string "last_sign_in_ip" + t.integer "status", default: 0 + t.datetime "active_until" + t.index ["email"], name: "index_users_on_email", unique: true + t.index ["reset_password_token"], name: "index_users_on_reset_password_token", unique: true end - add_check_constraint 'users', 'admin IS NOT NULL', name: 'users_admin_null', validate: false + add_check_constraint "users", "admin IS NOT NULL", name: "users_admin_null", validate: false - create_table 'visits', force: :cascade do |t| - t.bigint 'area_id' - t.bigint 'user_id', null: false - t.datetime 'started_at', null: false - t.datetime 'ended_at', null: false - t.integer 'duration', null: false - t.string 'name', null: false - t.integer 'status', default: 0, null: false - t.datetime 'created_at', null: false - t.datetime 'updated_at', null: false - t.bigint 'place_id' - t.index ['area_id'], name: 'index_visits_on_area_id' - t.index ['place_id'], name: 'index_visits_on_place_id' - t.index ['started_at'], name: 'index_visits_on_started_at' - t.index ['user_id'], name: 'index_visits_on_user_id' + create_table "visits", force: :cascade do |t| + t.bigint "area_id" + t.bigint "user_id", null: false + t.datetime "started_at", null: false + t.datetime "ended_at", null: false + t.integer "duration", null: false + t.string "name", null: false + t.integer "status", default: 0, null: false + t.datetime "created_at", null: false + t.datetime "updated_at", null: false + t.bigint "place_id" + t.index ["area_id"], name: "index_visits_on_area_id" + t.index ["place_id"], name: "index_visits_on_place_id" + t.index ["started_at"], name: "index_visits_on_started_at" + t.index ["user_id"], name: "index_visits_on_user_id" end - add_foreign_key 'active_storage_attachments', 'active_storage_blobs', column: 'blob_id' - add_foreign_key 'active_storage_variant_records', 'active_storage_blobs', column: 'blob_id' - add_foreign_key 'areas', 'users' - add_foreign_key 'notifications', 'users' - add_foreign_key 'place_visits', 'places' - add_foreign_key 'place_visits', 'visits' - add_foreign_key 'points', 'users' - add_foreign_key 'points', 'visits' - add_foreign_key 'stats', 'users' - add_foreign_key 'trips', 'users' - add_foreign_key 'visits', 'areas' - add_foreign_key 'visits', 'places' - add_foreign_key 'visits', 'users' + add_foreign_key "active_storage_attachments", "active_storage_blobs", column: "blob_id" + add_foreign_key "active_storage_variant_records", "active_storage_blobs", column: "blob_id" + add_foreign_key "areas", "users" + add_foreign_key "notifications", "users" + add_foreign_key "place_visits", "places" + add_foreign_key "place_visits", "visits" + add_foreign_key "points", "users" + add_foreign_key "points", "visits" + add_foreign_key "stats", "users" + add_foreign_key "trips", "users" + add_foreign_key "visits", "areas" + add_foreign_key "visits", "places" + add_foreign_key "visits", "users" end diff --git a/spec/models/trip_spec.rb b/spec/models/trip_spec.rb index f56daf20..7d17117d 100644 --- a/spec/models/trip_spec.rb +++ b/spec/models/trip_spec.rb @@ -3,6 +3,10 @@ require 'rails_helper' RSpec.describe Trip, type: :model do + before do + allow_any_instance_of(Trips::Countries).to receive(:call).and_return([]) + end + describe 'validations' do it { is_expected.to validate_presence_of(:name) } it { is_expected.to validate_presence_of(:started_at) } @@ -25,6 +29,26 @@ RSpec.describe Trip, type: :model do it 'sets the path' do expect(trip.path).to be_present end + + context 'when DawarichSettings.store_geodata is enabled' do + before do + allow(DawarichSettings).to receive(:store_geodata?).and_return(true) + end + + it 'sets the countries' do + expect(trip.countries).to eq(trip.points.pluck(:country).uniq.compact) + end + end + + context 'when DawarichSettings.store_geodata is disabled' do + it 'sets the visited countries' do + countries_service = instance_double(Trips::Countries, call: []) + expect(Trips::Countries).to receive(:new).with(trip).and_return(countries_service) + expect(countries_service).to receive(:call) + + trip.save + end + end end describe '#countries' do diff --git a/spec/rails_helper.rb b/spec/rails_helper.rb index ed2d0de6..15d0ef7c 100644 --- a/spec/rails_helper.rb +++ b/spec/rails_helper.rb @@ -38,6 +38,7 @@ RSpec.configure do |config| config.before do ActiveJob::Base.queue_adapter = :test + allow(DawarichSettings).to receive(:store_geodata?).and_return(true) end config.after(:suite) do diff --git a/spec/services/trips/countries_spec.rb b/spec/services/trips/countries_spec.rb new file mode 100644 index 00000000..107e98fc --- /dev/null +++ b/spec/services/trips/countries_spec.rb @@ -0,0 +1,108 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Trips::Countries do + let(:trip) { instance_double('Trip') } + let(:point1) { instance_double('Point', lonlat: factory.point(10.0, 50.0)) } + let(:point2) { instance_double('Point', lonlat: factory.point(20.0, 60.0)) } + let(:point3) { instance_double('Point', lonlat: factory.point(30.0, 70.0)) } + let(:point4) { instance_double('Point', lonlat: nil) } + let(:factory) { RGeo::Geographic.spherical_factory } + let(:points) { [point1, point2, point3, point4] } + + let(:geo_json_content) do + { + type: 'FeatureCollection', + features: [ + { + type: 'Feature', + properties: { ADMIN: 'Germany', ISO_A3: 'DEU', ISO_A2: 'DE' }, + geometry: { type: 'MultiPolygon', coordinates: [] } + } + ] + }.to_json + end + + before do + allow(trip).to receive(:points).and_return(points) + 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(Rails.logger).to receive(:info) + allow(Rails.logger).to receive(:error) + end + + describe '#call' do + it 'returns a hash with country counts' do + allow(Thread).to receive(:new).and_yield + + result = described_class.new(trip).call + + expect(result).to be_a(Hash) + expect(result.keys).to match_array(%w[DE SE FI]) + expect(result.values.sum).to eq(3) + end + + it 'handles points without coordinates' do + allow(Thread).to receive(:new).and_yield + + result = described_class.new(trip).call + + expect(result.values.sum).to eq(3) # Should only count the 3 valid points + end + + it 'processes batches in multiple threads' do + expect(Thread).to receive(:new).at_least(:twice).and_yield + + described_class.new(trip).call + end + + it 'sorts countries by count in descending order' 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') + expect(result['DE']).to eq(2) + end + end +end diff --git a/spec/support/geocoder_stubs.rb b/spec/support/geocoder_stubs.rb new file mode 100644 index 00000000..8e34dc2d --- /dev/null +++ b/spec/support/geocoder_stubs.rb @@ -0,0 +1,30 @@ +# frozen_string_literal: true + +# Stub all Geocoder requests in tests +RSpec.configure do |config| + config.before(:each) do + # Create a generic stub for all Geocoder requests + stub_request(:any, %r{photon\.dawarich\.app/reverse}).to_return( + status: 200, + body: { + type: 'FeatureCollection', + features: [ + { + type: 'Feature', + properties: { + name: 'Test Location', + countrycode: 'US', + country: 'United States', + state: 'New York' + }, + geometry: { + coordinates: [-73.9, 40.7], + type: 'Point' + } + } + ] + }.to_json, + headers: { 'Content-Type' => 'application/json' } + ) + end +end From 556af7fd020274fbb89fe09e5589806cab8a9dd6 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Tue, 13 May 2025 20:21:18 +0200 Subject: [PATCH 10/41] 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 From 5fbc1fb884fa80b5fe6362798db6eab696e39e1c Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Tue, 13 May 2025 20:33:04 +0200 Subject: [PATCH 11/41] Make sure geocoder errors are reported --- Gemfile | 1 - Gemfile.lock | 1 - spec/services/trips/countries_spec.rb | 12 +++++++++++ spec/support/geocoder_stubs.rb | 30 ++++++++++----------------- 4 files changed, 23 insertions(+), 21 deletions(-) diff --git a/Gemfile b/Gemfile index 1759ef3f..c8fa08c2 100644 --- a/Gemfile +++ b/Gemfile @@ -31,7 +31,6 @@ gem 'rexml' gem 'rgeo' gem 'rgeo-activerecord' gem 'rgeo-geojson' -gem 'parallel' gem 'rswag-api' gem 'rswag-ui' gem 'sentry-ruby' diff --git a/Gemfile.lock b/Gemfile.lock index 284b3dba..523f8e9f 100644 --- a/Gemfile.lock +++ b/Gemfile.lock @@ -506,7 +506,6 @@ DEPENDENCIES kaminari lograge oj - parallel pg prometheus_exporter pry-byebug diff --git a/spec/services/trips/countries_spec.rb b/spec/services/trips/countries_spec.rb index d2e4638a..6da3829c 100644 --- a/spec/services/trips/countries_spec.rb +++ b/spec/services/trips/countries_spec.rb @@ -80,5 +80,17 @@ RSpec.describe Trips::Countries do expect(result.keys.first).to eq('DE') expect(result['DE']).to eq(2) end + + context 'when an error occurs' do + before do + allow(Geocoder).to receive(:search).and_raise(Geocoder::Error, 'Error') + end + + it 'calls the exception reporter' do + expect(ExceptionReporter).to receive(:call).with(Geocoder::Error).at_least(3).times + + described_class.new(trip).call + end + end end end diff --git a/spec/support/geocoder_stubs.rb b/spec/support/geocoder_stubs.rb index 8e34dc2d..617b263d 100644 --- a/spec/support/geocoder_stubs.rb +++ b/spec/support/geocoder_stubs.rb @@ -4,27 +4,19 @@ RSpec.configure do |config| config.before(:each) do # Create a generic stub for all Geocoder requests - stub_request(:any, %r{photon\.dawarich\.app/reverse}).to_return( - status: 200, - body: { - type: 'FeatureCollection', - features: [ - { - type: 'Feature', - properties: { - name: 'Test Location', - countrycode: 'US', - country: 'United States', - state: 'New York' - }, - geometry: { - coordinates: [-73.9, 40.7], - type: 'Point' + allow(Geocoder).to receive(:search).and_return( + [ + double( + data: { + 'properties' => { + 'countrycode' => 'US', + 'country' => 'United States', + 'state' => 'New York', + 'name' => 'Test Location' } } - ] - }.to_json, - headers: { 'Content-Type' => 'application/json' } + ) + ] ) end end From 5fa4d953f74544d22be8fb5ba0374996bc03ab17 Mon Sep 17 00:00:00 2001 From: MeijiRestored <42336759+MeijiRestored@users.noreply.github.com> Date: Wed, 14 May 2025 18:56:30 +0200 Subject: [PATCH 12/41] Improved fog of war --- app/javascript/maps/fog_of_war.js | 67 +++++++++++++++++++++---------- 1 file changed, 45 insertions(+), 22 deletions(-) diff --git a/app/javascript/maps/fog_of_war.js b/app/javascript/maps/fog_of_war.js index bc3acbb1..f46cb55e 100644 --- a/app/javascript/maps/fog_of_war.js +++ b/app/javascript/maps/fog_of_war.js @@ -33,38 +33,61 @@ export function drawFogCanvas(map, markers, clearFogRadius) { const size = map.getSize(); - // Clear the canvas + // 1) Paint base fog ctx.clearRect(0, 0, size.x, size.y); - - // Keep the light fog for unexplored areas ctx.fillStyle = 'rgba(0, 0, 0, 0.4)'; ctx.fillRect(0, 0, size.x, size.y); - // Set up for "cutting" holes + // 2) Cut out holes ctx.globalCompositeOperation = 'destination-out'; - // Draw clear circles for each point - markers.forEach(point => { - const latLng = L.latLng(point[0], point[1]); - const pixelPoint = map.latLngToContainerPoint(latLng); - const radiusInPixels = metersToPixels(map, clearFogRadius); + // 3) Build & sort points + const thresholdSec = 90; // points will be joined if < 1m30 of time difference + const pts = markers + .map(pt => { + const pixel = map.latLngToContainerPoint(L.latLng(pt[0], pt[1])); + return { pixel, time: parseInt(pt[4], 10) }; + }) + .sort((a, b) => a.time - b.time); - // Make explored areas completely transparent - const gradient = ctx.createRadialGradient( - pixelPoint.x, pixelPoint.y, 0, - pixelPoint.x, pixelPoint.y, radiusInPixels - ); - gradient.addColorStop(0, 'rgba(255, 255, 255, 1)'); // 100% transparent - gradient.addColorStop(0.85, 'rgba(255, 255, 255, 1)'); // Still 100% transparent - gradient.addColorStop(1, 'rgba(255, 255, 255, 0)'); // Fade to fog at edge + const radiusPx = Math.max(metersToPixels(map, clearFogRadius), 2); + console.log(radiusPx); - ctx.fillStyle = gradient; - ctx.beginPath(); - ctx.arc(pixelPoint.x, pixelPoint.y, radiusInPixels, 0, Math.PI * 2); - ctx.fill(); + // 4) Mark which pts are part of a line + const connected = new Array(pts.length).fill(false); + for (let i = 0; i < pts.length - 1; i++) { + if (pts[i + 1].time - pts[i].time <= thresholdSec) { + connected[i] = true; + connected[i + 1] = true; + } + } + + // 5) Draw circles only for “alone” points + pts.forEach((pt, i) => { + if (!connected[i]) { + ctx.fillStyle = 'rgba(255,255,255,1)'; + ctx.beginPath(); + ctx.arc(pt.pixel.x, pt.pixel.y, radiusPx, 0, Math.PI * 2); + ctx.fill(); + } }); - // Reset composite operation + // 6) Draw rounded lines + ctx.lineWidth = radiusPx * 2; + ctx.lineCap = 'round'; + ctx.lineJoin = 'round'; + ctx.strokeStyle = 'rgba(255,255,255,1)'; + + for (let i = 0; i < pts.length - 1; i++) { + if (pts[i + 1].time - pts[i].time <= thresholdSec) { + ctx.beginPath(); + ctx.moveTo(pts[i].pixel.x, pts[i].pixel.y); + ctx.lineTo(pts[i + 1].pixel.x, pts[i + 1].pixel.y); + ctx.stroke(); + } + } + + // 7) Reset composite operation ctx.globalCompositeOperation = 'source-over'; } From e5075d59d3778ea5425cd358e9e3efaf97d53fc8 Mon Sep 17 00:00:00 2001 From: MeijiRestored <42336759+MeijiRestored@users.noreply.github.com> Date: Wed, 14 May 2025 21:04:47 +0200 Subject: [PATCH 13/41] configurable time threshold --- app/controllers/api/v1/settings_controller.rb | 2 +- app/javascript/controllers/maps_controller.js | 26 ++++++++++++------- app/javascript/maps/fog_of_war.js | 7 +++-- app/views/map/_settings_modals.html.erb | 14 ++++++++++ 4 files changed, 35 insertions(+), 14 deletions(-) diff --git a/app/controllers/api/v1/settings_controller.rb b/app/controllers/api/v1/settings_controller.rb index 4237e083..0471f49b 100644 --- a/app/controllers/api/v1/settings_controller.rb +++ b/app/controllers/api/v1/settings_controller.rb @@ -30,7 +30,7 @@ class Api::V1::SettingsController < ApiController :time_threshold_minutes, :merge_threshold_minutes, :route_opacity, :preferred_map_layer, :points_rendering_mode, :live_map_enabled, :immich_url, :immich_api_key, :photoprism_url, :photoprism_api_key, - :speed_colored_routes, :speed_color_scale + :speed_colored_routes, :speed_color_scale, :fog_of_war_threshold ) end end diff --git a/app/javascript/controllers/maps_controller.js b/app/javascript/controllers/maps_controller.js index 5c1eefab..c34c9de0 100644 --- a/app/javascript/controllers/maps_controller.js +++ b/app/javascript/controllers/maps_controller.js @@ -45,6 +45,7 @@ export default class extends BaseController { this.timezone = this.element.dataset.timezone; this.userSettings = JSON.parse(this.element.dataset.user_settings); this.clearFogRadius = parseInt(this.userSettings.fog_of_war_meters) || 50; + this.fogLinethreshold = parseInt(this.userSettings.fog_of_war_threshold) || 90; this.routeOpacity = parseFloat(this.userSettings.route_opacity) || 0.6; this.distanceUnit = this.element.dataset.distance_unit || "km"; this.pointsRenderingMode = this.userSettings.points_rendering_mode || "raw"; @@ -175,13 +176,13 @@ export default class extends BaseController { // Update event handlers this.map.on('moveend', () => { if (document.getElementById('fog')) { - this.updateFog(this.markers, this.clearFogRadius); + this.updateFog(this.markers, this.clearFogRadius, this.fogLinethreshold); } }); this.map.on('zoomend', () => { if (document.getElementById('fog')) { - this.updateFog(this.markers, this.clearFogRadius); + this.updateFog(this.markers, this.clearFogRadius, this.fogLinethreshold); } }); @@ -198,7 +199,7 @@ export default class extends BaseController { if (e.name === 'Fog of War') { fogEnabled = true; document.getElementById('fog').style.display = 'block'; - this.updateFog(this.markers, this.clearFogRadius); + this.updateFog(this.markers, this.clearFogRadius, this.fogLinethreshold); } }); @@ -212,7 +213,7 @@ export default class extends BaseController { // Update fog circles on zoom and move this.map.on('zoomend moveend', () => { if (fogEnabled) { - this.updateFog(this.markers, this.clearFogRadius); + this.updateFog(this.markers, this.clearFogRadius, this.fogLinethreshold); } }); @@ -350,7 +351,7 @@ export default class extends BaseController { // Update fog of war if enabled if (this.map.hasLayer(this.fogOverlay)) { - this.updateFog(this.markers, this.clearFogRadius); + this.updateFog(this.markers, this.clearFogRadius, this.fogLinethreshold); } // Update the last marker @@ -587,7 +588,7 @@ export default class extends BaseController { // Update fog if enabled if (this.map.hasLayer(this.fogOverlay)) { - this.updateFog(this.markers, this.clearFogRadius); + this.updateFog(this.markers, this.clearFogRadius, this.fogLinethreshold); } }) .catch(error => { @@ -623,12 +624,12 @@ export default class extends BaseController { } } - updateFog(markers, clearFogRadius) { + updateFog(markers, clearFogRadius, fogLinethreshold) { const fog = document.getElementById('fog'); if (!fog) { initializeFogCanvas(this.map); } - requestAnimationFrame(() => drawFogCanvas(this.map, markers, clearFogRadius)); + requestAnimationFrame(() => drawFogCanvas(this.map, markers, clearFogRadius, fogLinethreshold)); } initializeDrawControl() { @@ -724,7 +725,7 @@ export default class extends BaseController { // Form HTML div.innerHTML = ` -
+
@@ -738,6 +739,12 @@ export default class extends BaseController {
+ +
+ + +
+
@@ -863,6 +870,7 @@ export default class extends BaseController { settings: { route_opacity: event.target.route_opacity.value, fog_of_war_meters: event.target.fog_of_war_meters.value, + fog_of_war_threshold: event.target.fog_of_war_threshold.value, meters_between_routes: event.target.meters_between_routes.value, minutes_between_routes: event.target.minutes_between_routes.value, time_threshold_minutes: event.target.time_threshold_minutes.value, diff --git a/app/javascript/maps/fog_of_war.js b/app/javascript/maps/fog_of_war.js index f46cb55e..b715c5f0 100644 --- a/app/javascript/maps/fog_of_war.js +++ b/app/javascript/maps/fog_of_war.js @@ -23,7 +23,7 @@ export function initializeFogCanvas(map) { return fog; } -export function drawFogCanvas(map, markers, clearFogRadius) { +export function drawFogCanvas(map, markers, clearFogRadius, fogLinethreshold) { const fog = document.getElementById('fog'); // Return early if fog element doesn't exist or isn't a canvas if (!fog || !(fog instanceof HTMLCanvasElement)) return; @@ -42,7 +42,6 @@ export function drawFogCanvas(map, markers, clearFogRadius) { ctx.globalCompositeOperation = 'destination-out'; // 3) Build & sort points - const thresholdSec = 90; // points will be joined if < 1m30 of time difference const pts = markers .map(pt => { const pixel = map.latLngToContainerPoint(L.latLng(pt[0], pt[1])); @@ -56,7 +55,7 @@ export function drawFogCanvas(map, markers, clearFogRadius) { // 4) Mark which pts are part of a line const connected = new Array(pts.length).fill(false); for (let i = 0; i < pts.length - 1; i++) { - if (pts[i + 1].time - pts[i].time <= thresholdSec) { + if (pts[i + 1].time - pts[i].time <= fogLinethreshold) { connected[i] = true; connected[i + 1] = true; } @@ -79,7 +78,7 @@ export function drawFogCanvas(map, markers, clearFogRadius) { ctx.strokeStyle = 'rgba(255,255,255,1)'; for (let i = 0; i < pts.length - 1; i++) { - if (pts[i + 1].time - pts[i].time <= thresholdSec) { + if (pts[i + 1].time - pts[i].time <= fogLinethreshold) { ctx.beginPath(); ctx.moveTo(pts[i].pixel.x, pts[i].pixel.y); ctx.lineTo(pts[i + 1].pixel.x, pts[i + 1].pixel.y); diff --git a/app/views/map/_settings_modals.html.erb b/app/views/map/_settings_modals.html.erb index 6703f5f2..24d965a5 100644 --- a/app/views/map/_settings_modals.html.erb +++ b/app/views/map/_settings_modals.html.erb @@ -27,6 +27,20 @@
+ + +