From 6870be204580f606fe2bdb1933a05f043271cefd Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Tue, 29 Jul 2025 21:17:33 +0200 Subject: [PATCH] Fix country name in points --- .../backfill_country_name_job.rb | 11 ++--- app/models/point.rb | 2 +- app/models/user.rb | 37 +++------------ app/policies/import_policy.rb | 2 +- ...250728191359_add_country_name_to_points.rb | 3 +- spec/models/user_spec.rb | 45 +++++++++++-------- spec/policies/import_policy_spec.rb | 21 ++++++++- spec/requests/api/v1/stats_spec.rb | 1 - spec/requests/imports_spec.rb | 9 ---- spec/services/tracks/track_builder_spec.rb | 2 +- 10 files changed, 60 insertions(+), 73 deletions(-) diff --git a/app/jobs/data_migrations/backfill_country_name_job.rb b/app/jobs/data_migrations/backfill_country_name_job.rb index 7526dc95..22eefe06 100644 --- a/app/jobs/data_migrations/backfill_country_name_job.rb +++ b/app/jobs/data_migrations/backfill_country_name_job.rb @@ -11,8 +11,9 @@ class DataMigrations::BackfillCountryNameJob < ApplicationJob Point.where(country_name: nil).find_in_batches(batch_size: batch_size) do |points| points.each do |point| - country_name = determine_country_name(point) + country_name = country_name(point) point.update_column(:country_name, country_name) if country_name.present? + processed_count += 1 end @@ -24,11 +25,7 @@ class DataMigrations::BackfillCountryNameJob < ApplicationJob private - def determine_country_name(point) - # First try the legacy country column - return point.read_attribute(:country) if point.read_attribute(:country).present? - - # Then try the associated country record - point.country&.name + def country_name(point) + point.read_attribute(:country) || point.country&.name end end diff --git a/app/models/point.rb b/app/models/point.rb index 6fd311e9..ef00e99b 100644 --- a/app/models/point.rb +++ b/app/models/point.rb @@ -67,7 +67,7 @@ class Point < ApplicationRecord end def country_name - # Use the new country_name column first, then fallback to association, then legacy column + # TODO: Remove the country column in the future. read_attribute(:country_name) || self.country&.name || read_attribute(:country) || '' end diff --git a/app/models/user.rb b/app/models/user.rb index 614bbd8c..4c61d98e 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -33,40 +33,15 @@ class User < ApplicationRecord end def countries_visited - # Primary method: query points directly for better performance - countries_from_points = tracked_points.where.not(country_name: [nil, '']).distinct.pluck(:country_name).compact - - # Fallback to stats-based approach if no points with country_name exist - return countries_from_points if countries_from_points.any? - - countries_visited_from_stats + tracked_points + .where.not(country_name: [nil, '']) + .distinct + .pluck(:country_name) + .compact end def cities_visited - # Primary method: query points directly for better performance - cities_from_points = tracked_points.where.not(city: [nil, '']).distinct.pluck(:city).compact - - # Fallback to stats-based approach if no points with city exist - return cities_from_points if cities_from_points.any? - - cities_visited_from_stats - end - - def countries_visited_from_stats - stats.pluck(:toponyms).flatten.map { _1['country'] }.uniq.compact - end - - def cities_visited_from_stats - stats - .where.not(toponyms: nil) - .pluck(:toponyms) - .flatten - .reject { |toponym| toponym['cities'].blank? } - .pluck('cities') - .flatten - .pluck('city') - .uniq - .compact + tracked_points.where.not(city: [nil, '']).distinct.pluck(:city).compact end def total_distance diff --git a/app/policies/import_policy.rb b/app/policies/import_policy.rb index 77d5c8e0..0d1ceddf 100644 --- a/app/policies/import_policy.rb +++ b/app/policies/import_policy.rb @@ -42,4 +42,4 @@ class ImportPolicy < ApplicationPolicy scope.where(user: user) end end -end \ No newline at end of file +end diff --git a/db/migrate/20250728191359_add_country_name_to_points.rb b/db/migrate/20250728191359_add_country_name_to_points.rb index 2a819007..d00dbef3 100644 --- a/db/migrate/20250728191359_add_country_name_to_points.rb +++ b/db/migrate/20250728191359_add_country_name_to_points.rb @@ -4,8 +4,7 @@ class AddCountryNameToPoints < ActiveRecord::Migration[8.0] def change add_column :points, :country_name, :string add_index :points, :country_name, algorithm: :concurrently - - # Backfill existing records with country_name from country column or associated country + DataMigrations::BackfillCountryNameJob.perform_later end end diff --git a/spec/models/user_spec.rb b/spec/models/user_spec.rb index a1c5601f..3caba416 100644 --- a/spec/models/user_spec.rb +++ b/spec/models/user_spec.rb @@ -65,23 +65,36 @@ RSpec.describe User, type: :model do describe '#countries_visited' do subject { user.countries_visited } - let!(:stat1) { create(:stat, user:, toponyms: [{ 'country' => 'Germany' }]) } - let!(:stat2) { create(:stat, user:, toponyms: [{ 'country' => 'France' }]) } + let!(:point1) { create(:point, user:, country_name: 'Germany') } + let!(:point2) { create(:point, user:, country_name: 'France') } + let!(:point3) { create(:point, user:, country_name: nil) } + let!(:point4) { create(:point, user:, country_name: '') } it 'returns array of countries' do expect(subject).to include('Germany', 'France') expect(subject.count).to eq(2) end + + it 'excludes nil and empty country names' do + expect(subject).not_to include(nil, '') + end end describe '#cities_visited' do subject { user.cities_visited } - let!(:stat1) { create(:stat, user:, toponyms: [{ 'cities' => [{ 'city' => 'Berlin' }] }]) } - let!(:stat2) { create(:stat, user:, toponyms: [{ 'cities' => [{ 'city' => 'Paris' }] }]) } + let!(:point1) { create(:point, user:, city: 'Berlin') } + let!(:point2) { create(:point, user:, city: 'Paris') } + let!(:point3) { create(:point, user:, city: nil) } + let!(:point4) { create(:point, user:, city: '') } it 'returns array of cities' do - expect(subject).to eq(%w[Berlin Paris]) + expect(subject).to include('Berlin', 'Paris') + expect(subject.count).to eq(2) + end + + it 'excludes nil and empty city names' do + expect(subject).not_to include(nil, '') end end @@ -99,30 +112,24 @@ RSpec.describe User, type: :model do describe '#total_countries' do subject { user.total_countries } - let!(:stat) { create(:stat, user:, toponyms: [{ 'country' => 'Country' }]) } + let!(:point1) { create(:point, user:, country_name: 'Germany') } + let!(:point2) { create(:point, user:, country_name: 'France') } + let!(:point3) { create(:point, user:, country_name: nil) } it 'returns number of countries' do - expect(subject).to eq(1) + expect(subject).to eq(2) end end describe '#total_cities' do subject { user.total_cities } - let!(:stat) do - create( - :stat, - user:, - toponyms: [ - { 'cities' => [], 'country' => nil }, - { 'cities' => [{ 'city' => 'Berlin', 'points' => 64, 'timestamp' => 1_710_446_806, 'stayed_for' => 8772 }], -'country' => 'Germany' } - ] - ) - end + let!(:point1) { create(:point, user:, city: 'Berlin') } + let!(:point2) { create(:point, user:, city: 'Paris') } + let!(:point3) { create(:point, user:, city: nil) } it 'returns number of cities' do - expect(subject).to eq(1) + expect(subject).to eq(2) end end diff --git a/spec/policies/import_policy_spec.rb b/spec/policies/import_policy_spec.rb index f518f4f6..3bf93351 100644 --- a/spec/policies/import_policy_spec.rb +++ b/spec/policies/import_policy_spec.rb @@ -11,11 +11,13 @@ RSpec.describe ImportPolicy, type: :policy do describe 'index?' do it 'allows authenticated users' do policy = ImportPolicy.new(user, Import) + expect(policy).to permit(:index) end it 'denies unauthenticated users' do policy = ImportPolicy.new(nil, Import) + expect(policy).not_to permit(:index) end end @@ -23,16 +25,19 @@ RSpec.describe ImportPolicy, type: :policy do describe 'show?' do it 'allows users to view their own imports' do policy = ImportPolicy.new(user, import) + expect(policy).to permit(:show) end it 'denies users from viewing other users imports' do policy = ImportPolicy.new(user, other_import) + expect(policy).not_to permit(:show) end it 'denies unauthenticated users' do policy = ImportPolicy.new(nil, import) + expect(policy).not_to permit(:show) end end @@ -43,6 +48,7 @@ RSpec.describe ImportPolicy, type: :policy do it 'allows active users to access new imports form' do policy = ImportPolicy.new(user, Import.new) + expect(policy).to permit(:new) end end @@ -52,12 +58,14 @@ RSpec.describe ImportPolicy, type: :policy do it 'denies inactive users from accessing new imports form' do policy = ImportPolicy.new(user, Import.new) + expect(policy).not_to permit(:new) end end it 'denies unauthenticated users' do policy = ImportPolicy.new(nil, Import.new) + expect(policy).not_to permit(:new) end end @@ -68,6 +76,7 @@ RSpec.describe ImportPolicy, type: :policy do it 'allows active users to create imports' do policy = ImportPolicy.new(user, Import.new) + expect(policy).to permit(:create) end end @@ -77,12 +86,14 @@ RSpec.describe ImportPolicy, type: :policy do it 'denies inactive users from creating imports' do policy = ImportPolicy.new(user, Import.new) + expect(policy).not_to permit(:create) end end it 'denies unauthenticated users' do policy = ImportPolicy.new(nil, Import.new) + expect(policy).not_to permit(:create) end end @@ -90,16 +101,19 @@ RSpec.describe ImportPolicy, type: :policy do describe 'update?' do it 'allows users to update their own imports' do policy = ImportPolicy.new(user, import) + expect(policy).to permit(:update) end it 'denies users from updating other users imports' do policy = ImportPolicy.new(user, other_import) + expect(policy).not_to permit(:update) end it 'denies unauthenticated users' do policy = ImportPolicy.new(nil, import) + expect(policy).not_to permit(:update) end end @@ -107,16 +121,19 @@ RSpec.describe ImportPolicy, type: :policy do describe 'destroy?' do it 'allows users to destroy their own imports' do policy = ImportPolicy.new(user, import) + expect(policy).to permit(:destroy) end it 'denies users from destroying other users imports' do policy = ImportPolicy.new(user, other_import) + expect(policy).not_to permit(:destroy) end it 'denies unauthenticated users' do policy = ImportPolicy.new(nil, import) + expect(policy).not_to permit(:destroy) end end @@ -128,13 +145,15 @@ RSpec.describe ImportPolicy, type: :policy do it 'returns only the users imports' do scope = ImportPolicy::Scope.new(user, Import).resolve + expect(scope).to contain_exactly(user_import1, user_import2) expect(scope).not_to include(other_user_import) end it 'returns no imports for unauthenticated users' do scope = ImportPolicy::Scope.new(nil, Import).resolve + expect(scope).to be_empty end end -end \ No newline at end of file +end diff --git a/spec/requests/api/v1/stats_spec.rb b/spec/requests/api/v1/stats_spec.rb index 2e7eb8ae..314c375e 100644 --- a/spec/requests/api/v1/stats_spec.rb +++ b/spec/requests/api/v1/stats_spec.rb @@ -29,7 +29,6 @@ RSpec.describe 'Api::V1::Stats', type: :request do end before do - # Create all the test data stats_in_2020 stats_in_2021 points_in_2020 diff --git a/spec/requests/imports_spec.rb b/spec/requests/imports_spec.rb index efe733c8..0d1852de 100644 --- a/spec/requests/imports_spec.rb +++ b/spec/requests/imports_spec.rb @@ -32,7 +32,6 @@ RSpec.describe 'Imports', type: :request do end end - # Security test: Users should only see their own imports context 'when other users have imports' do let!(:other_user) { create(:user) } let!(:other_import) { create(:import, user: other_user) } @@ -176,24 +175,17 @@ RSpec.describe 'Imports', type: :request do let(:signed_id2) { generate_signed_id_for_blob(blob2) } it 'deletes any created imports' do - # The first blob should be found correctly allow(ActiveStorage::Blob).to receive(:find_signed).with(signed_id1).and_return(blob1) - # The second blob find will raise an error allow(ActiveStorage::Blob).to receive(:find_signed).with(signed_id2).and_raise(StandardError, 'Test error') - # Allow ExceptionReporter to be called without actually calling it allow(ExceptionReporter).to receive(:call) - # The request should not ultimately create any imports expect do post imports_path, params: { import: { source: 'owntracks', files: [signed_id1, signed_id2] } } end.not_to change(Import, :count) - # Check that we were redirected with an error message expect(response).to have_http_status(422) - # Just check that we have an alert message, not its exact content - # since error handling might transform the message expect(flash[:alert]).not_to be_nil end end @@ -262,7 +254,6 @@ RSpec.describe 'Imports', type: :request do end end - # Helper methods for creating ActiveStorage blobs and signed IDs in tests def create_blob_for_file(file) ActiveStorage::Blob.create_and_upload!( io: file.open, diff --git a/spec/services/tracks/track_builder_spec.rb b/spec/services/tracks/track_builder_spec.rb index 2213f5a2..e37523d1 100644 --- a/spec/services/tracks/track_builder_spec.rb +++ b/spec/services/tracks/track_builder_spec.rb @@ -49,7 +49,7 @@ RSpec.describe Tracks::TrackBuilder do expect(track.start_at).to be_within(1.second).of(Time.zone.at(points.first.timestamp)) expect(track.end_at).to be_within(1.second).of(Time.zone.at(points.last.timestamp)) expect(track.distance).to eq(1500) - expect(track.duration).to eq(90.minutes.to_i) + expect(track.duration).to be_within(3.seconds).of(90.minutes.to_i) expect(track.avg_speed).to be > 0 expect(track.original_path).to be_present end