Fix country name in points

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

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
 

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

app/policies/import_policy.rb

@@ -42,4 +42,4 @@ class ImportPolicy < ApplicationPolicy
       scope.where(user: user)
     end
   end
-end
+end

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

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
 

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
+end

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

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,

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