From f492a69fbbf7cb56c60b69e14cf8bf14c0617a76 Mon Sep 17 00:00:00 2001
From: Eugene Burmakin <iamfrey@gmail.com>
Date: Mon, 28 Jul 2025 18:53:19 +0200
Subject: [PATCH 1/3] Add country_name to points and fix some bugs.

---
 CHANGELOG.md                                  |   6 +
 app/controllers/imports_controller.rb         |  16 +-
 app/controllers/notifications_controller.rb   |   2 +-
 .../backfill_country_name_job.rb              |  34 +++++
 app/models/point.rb                           |  12 +-
 app/models/user.rb                            |  23 ++-
 app/policies/import_policy.rb                 |  45 ++++++
 app/services/countries_and_cities.rb          |   4 +-
 .../reverse_geocoding/points/fetch_data.rb    |   1 +
 app/services/stats/calculate_month.rb         |   2 +-
 ...250728191359_add_country_name_to_points.rb |  11 ++
 db/schema.rb                                  |   4 +-
 spec/factories/imports.rb                     |   2 +-
 spec/factories/points.rb                      |   5 +-
 .../files/geojson/export_same_points.json     |   2 +-
 spec/policies/import_policy_spec.rb           | 140 ++++++++++++++++++
 spec/requests/imports_spec.rb                 |  79 ++++++++++
 17 files changed, 367 insertions(+), 21 deletions(-)
 create mode 100644 app/jobs/data_migrations/backfill_country_name_job.rb
 create mode 100644 app/policies/import_policy.rb
 create mode 100644 db/migrate/20250728191359_add_country_name_to_points.rb
 create mode 100644 spec/policies/import_policy_spec.rb

diff --git a/CHANGELOG.md b/CHANGELOG.md
index c6e18936..5adffd3c 100644
--- a/CHANGELOG.md
+++ b/CHANGELOG.md
@@ -11,10 +11,16 @@ and this project adheres to [Semantic Versioning](http://semver.org/).
 - Put all jobs in their own queues.
 - Visits page should load faster now.
 - Reverse geocoding jobs now make less database queries.
+- Country name is now being backfilled for all points. #1562
+- Stats are now reflecting countries and cities. #1562
 
 ## Added
 - Points now support discharging and connected_not_charging battery statuses. #768
 
+## Fixed
+
+- Fixed a bug where import or notification could have been deleted by a different user.
+
 
 # [0.30.5] - 2025-07-26
 
diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb
index d5ad4489..61a9f690 100644
--- a/app/controllers/imports_controller.rb
+++ b/app/controllers/imports_controller.rb
@@ -4,13 +4,15 @@ class ImportsController < ApplicationController
   include ActiveStorage::SetCurrent
 
   before_action :authenticate_user!
-  before_action :authenticate_active_user!, only: %i[new create]
   before_action :set_import, only: %i[show edit update destroy]
+  before_action :authorize_import, only: %i[show edit update destroy]
   before_action :validate_points_limit, only: %i[new create]
+
+  after_action :verify_authorized, except: %i[index]
+  after_action :verify_policy_scoped, only: %i[index]
+
   def index
-    @imports =
-      current_user
-      .imports
+    @imports = policy_scope(Import)
       .select(:id, :name, :source, :created_at, :processed, :status)
       .order(created_at: :desc)
       .page(params[:page])
@@ -22,6 +24,8 @@ class ImportsController < ApplicationController
 
   def new
     @import = Import.new
+
+    authorize @import
   end
 
   def update
@@ -82,6 +86,10 @@ class ImportsController < ApplicationController
     @import = Import.find(params[:id])
   end
 
+  def authorize_import
+    authorize @import
+  end
+
   def import_params
     params.require(:import).permit(:name, :source, files: [])
   end
diff --git a/app/controllers/notifications_controller.rb b/app/controllers/notifications_controller.rb
index 0516063c..22e944fa 100644
--- a/app/controllers/notifications_controller.rb
+++ b/app/controllers/notifications_controller.rb
@@ -32,6 +32,6 @@ class NotificationsController < ApplicationController
   private
 
   def set_notification
-    @notification = Notification.find(params[:id])
+    @notification = current_user.notifications.find(params[:id])
   end
 end
diff --git a/app/jobs/data_migrations/backfill_country_name_job.rb b/app/jobs/data_migrations/backfill_country_name_job.rb
new file mode 100644
index 00000000..7526dc95
--- /dev/null
+++ b/app/jobs/data_migrations/backfill_country_name_job.rb
@@ -0,0 +1,34 @@
+# frozen_string_literal: true
+
+class DataMigrations::BackfillCountryNameJob < ApplicationJob
+  queue_as :data_migrations
+
+  def perform(batch_size: 1000)
+    Rails.logger.info('Starting country_name backfill job')
+
+    total_count = Point.where(country_name: nil).count
+    processed_count = 0
+
+    Point.where(country_name: nil).find_in_batches(batch_size: batch_size) do |points|
+      points.each do |point|
+        country_name = determine_country_name(point)
+        point.update_column(:country_name, country_name) if country_name.present?
+        processed_count += 1
+      end
+
+      Rails.logger.info("Backfilled country_name for #{processed_count}/#{total_count} points")
+    end
+
+    Rails.logger.info("Completed country_name backfill job. Processed #{processed_count} points")
+  end
+
+  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
+  end
+end
diff --git a/app/models/point.rb b/app/models/point.rb
index 68ebc827..6fd311e9 100644
--- a/app/models/point.rb
+++ b/app/models/point.rb
@@ -66,6 +66,11 @@ class Point < ApplicationRecord
     Country.containing_point(lon, lat)
   end
 
+  def country_name
+    # Use the new country_name column first, then fallback to association, then legacy column
+    read_attribute(:country_name) || self.country&.name || read_attribute(:country) || ''
+  end
+
   private
 
   # rubocop:disable Metrics/MethodLength Metrics/AbcSize
@@ -93,13 +98,6 @@ class Point < ApplicationRecord
     save! if changed?
   end
 
-  def country_name
-    # We have a country column in the database,
-    # but we also have a country_id column.
-    # TODO: rename country column to country_name
-    self.country&.name || read_attribute(:country) || ''
-  end
-
   def recalculate_track
     track.recalculate_path_and_distance!
   end
diff --git a/app/models/user.rb b/app/models/user.rb
index 2107c876..614bbd8c 100644
--- a/app/models/user.rb
+++ b/app/models/user.rb
@@ -33,10 +33,30 @@ class User < ApplicationRecord
   end
 
   def countries_visited
-    stats.pluck(:toponyms).flatten.map { _1['country'] }.uniq.compact
+    # 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
   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)
@@ -50,7 +70,6 @@ class User < ApplicationRecord
   end
 
   def total_distance
-    # Distance is stored in meters, convert to user's preferred unit for display
     total_distance_meters = stats.sum(:distance)
     Stat.convert_distance(total_distance_meters, safe_settings.distance_unit)
   end
diff --git a/app/policies/import_policy.rb b/app/policies/import_policy.rb
new file mode 100644
index 00000000..77d5c8e0
--- /dev/null
+++ b/app/policies/import_policy.rb
@@ -0,0 +1,45 @@
+# frozen_string_literal: true
+
+class ImportPolicy < ApplicationPolicy
+  # Allow users to view the imports index
+  def index?
+    user.present?
+  end
+
+  # Users can only show their own imports
+  def show?
+    user.present? && record.user == user
+  end
+
+  # Users can create new imports if they are active
+  def new?
+    create?
+  end
+
+  def create?
+    user.present? && user.active?
+  end
+
+  # Users can only edit their own imports
+  def edit?
+    update?
+  end
+
+  def update?
+    user.present? && record.user == user
+  end
+
+  # Users can only destroy their own imports
+  def destroy?
+    user.present? && record.user == user
+  end
+
+  class Scope < ApplicationPolicy::Scope
+    def resolve
+      return scope.none unless user.present?
+
+      # Users can only see their own imports
+      scope.where(user: user)
+    end
+  end
+end
\ No newline at end of file
diff --git a/app/services/countries_and_cities.rb b/app/services/countries_and_cities.rb
index f0eb77c7..ac4b2451 100644
--- a/app/services/countries_and_cities.rb
+++ b/app/services/countries_and_cities.rb
@@ -10,8 +10,8 @@ class CountriesAndCities
 
   def call
     points
-      .reject { |point| point.read_attribute(:country).nil? || point.city.nil? }
-      .group_by { |point| point.read_attribute(:country) }
+      .reject { |point| point.country_name.nil? || point.city.nil? }
+      .group_by { |point| point.country_name }
       .transform_values { |country_points| process_country_points(country_points) }
       .map { |country, cities| CountryData.new(country: country, cities: cities) }
   end
diff --git a/app/services/reverse_geocoding/points/fetch_data.rb b/app/services/reverse_geocoding/points/fetch_data.rb
index e2ee56ef..7101d1ff 100644
--- a/app/services/reverse_geocoding/points/fetch_data.rb
+++ b/app/services/reverse_geocoding/points/fetch_data.rb
@@ -27,6 +27,7 @@ class ReverseGeocoding::Points::FetchData
 
     point.update!(
       city: response.city,
+      country_name: response.country,
       country_id: country_record&.id,
       geodata: response.data,
       reverse_geocoded_at: Time.current
diff --git a/app/services/stats/calculate_month.rb b/app/services/stats/calculate_month.rb
index 839ea55a..91c2a1d7 100644
--- a/app/services/stats/calculate_month.rb
+++ b/app/services/stats/calculate_month.rb
@@ -63,7 +63,7 @@ class Stats::CalculateMonth
                       .tracked_points
                       .without_raw_data
                       .where(timestamp: start_timestamp..end_timestamp)
-                      .select(:city, :country)
+                      .select(:city, :country_name)
                       .distinct
 
     CountriesAndCities.new(toponym_points).call
diff --git a/db/migrate/20250728191359_add_country_name_to_points.rb b/db/migrate/20250728191359_add_country_name_to_points.rb
new file mode 100644
index 00000000..2a819007
--- /dev/null
+++ b/db/migrate/20250728191359_add_country_name_to_points.rb
@@ -0,0 +1,11 @@
+class AddCountryNameToPoints < ActiveRecord::Migration[8.0]
+  disable_ddl_transaction!
+
+  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/db/schema.rb b/db/schema.rb
index 25efe3c2..feac06e4 100644
--- a/db/schema.rb
+++ b/db/schema.rb
@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.0].define(version: 2025_07_23_164055) do
+ActiveRecord::Schema[8.0].define(version: 2025_07_28_191359) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_catalog.plpgsql"
   enable_extension "postgis"
@@ -186,6 +186,7 @@ ActiveRecord::Schema[8.0].define(version: 2025_07_23_164055) do
     t.geography "lonlat", limit: {srid: 4326, type: "st_point", geographic: true}
     t.bigint "country_id"
     t.bigint "track_id"
+    t.string "country_name"
     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"
@@ -193,6 +194,7 @@ ActiveRecord::Schema[8.0].define(version: 2025_07_23_164055) do
     t.index ["connection"], name: "index_points_on_connection"
     t.index ["country"], name: "index_points_on_country"
     t.index ["country_id"], name: "index_points_on_country_id"
+    t.index ["country_name"], name: "index_points_on_country_name"
     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"
diff --git a/spec/factories/imports.rb b/spec/factories/imports.rb
index 76f6e0ac..51670b6b 100644
--- a/spec/factories/imports.rb
+++ b/spec/factories/imports.rb
@@ -3,7 +3,7 @@
 FactoryBot.define do
   factory :import do
     user
-    name { 'owntracks_export.json' }
+    sequence(:name) { |n| "owntracks_export_#{n}.json" }
     source { Import.sources[:owntracks] }
 
     trait :with_points do
diff --git a/spec/factories/points.rb b/spec/factories/points.rb
index 4848250c..acc097e9 100644
--- a/spec/factories/points.rb
+++ b/spec/factories/points.rb
@@ -49,11 +49,13 @@ FactoryBot.define do
         end
         point.update_columns(
           country: evaluator.country,
+          country_name: evaluator.country,
           country_id: country_obj.id
         )
       elsif evaluator.country
         point.update_columns(
           country: evaluator.country.name,
+          country_name: evaluator.country.name,
           country_id: evaluator.country.id
         )
       end
@@ -101,7 +103,8 @@ FactoryBot.define do
             country.iso_a3 = iso_a3
             country.geom = "MULTIPOLYGON (((0 0, 1 0, 1 1, 0 1, 0 0)))"
           end
-          point.write_attribute(:country, country_name)        # Set the string attribute directly
+          point.write_attribute(:country, country_name)        # Set the legacy string attribute
+          point.write_attribute(:country_name, country_name)  # Set the new string attribute
           point.country_id = country_obj.id   # Set the association
         end
       end
diff --git a/spec/fixtures/files/geojson/export_same_points.json b/spec/fixtures/files/geojson/export_same_points.json
index 9a510998..6d1559c3 100644
--- a/spec/fixtures/files/geojson/export_same_points.json
+++ b/spec/fixtures/files/geojson/export_same_points.json
@@ -1 +1 @@
-{"type":"FeatureCollection","features":[{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459200,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459201,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459202,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459203,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459204,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459205,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459206,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459207,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459208,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459209,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null}}]}
+{"type":"FeatureCollection","features":[{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459200,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null,"country_name":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459201,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null,"country_name":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459202,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null,"country_name":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459203,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null,"country_name":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459204,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null,"country_name":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459205,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null,"country_name":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459206,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null,"country_name":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459207,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null,"country_name":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459208,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null,"country_name":null}},{"type":"Feature","geometry":{"type":"Point","coordinates":[37.6173,55.755826]},"properties":{"battery_status":"unplugged","ping":"MyString","battery":1,"tracker_id":"MyString","topic":"MyString","altitude":1,"longitude":"37.6173","velocity":"0","trigger":"background_event","bssid":"MyString","ssid":"MyString","connection":"wifi","vertical_accuracy":1,"accuracy":1,"timestamp":1609459209,"latitude":"55.755826","mode":1,"inrids":[],"in_regions":[],"city":null,"country":null,"geodata":{},"course":null,"course_accuracy":null,"external_track_id":null,"track_id":null,"country_name":null}}]}
diff --git a/spec/policies/import_policy_spec.rb b/spec/policies/import_policy_spec.rb
new file mode 100644
index 00000000..f518f4f6
--- /dev/null
+++ b/spec/policies/import_policy_spec.rb
@@ -0,0 +1,140 @@
+# frozen_string_literal: true
+
+require 'rails_helper'
+
+RSpec.describe ImportPolicy, type: :policy do
+  let(:user) { create(:user) }
+  let(:other_user) { create(:user) }
+  let(:import) { create(:import, user: user) }
+  let(:other_import) { create(:import, user: other_user) }
+
+  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
+
+  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
+
+  describe 'new?' do
+    context 'when user is active' do
+      before { allow(user).to receive(:active?).and_return(true) }
+
+      it 'allows active users to access new imports form' do
+        policy = ImportPolicy.new(user, Import.new)
+        expect(policy).to permit(:new)
+      end
+    end
+
+    context 'when user is not active' do
+      before { allow(user).to receive(:active?).and_return(false) }
+
+      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
+
+  describe 'create?' do
+    context 'when user is active' do
+      before { allow(user).to receive(:active?).and_return(true) }
+
+      it 'allows active users to create imports' do
+        policy = ImportPolicy.new(user, Import.new)
+        expect(policy).to permit(:create)
+      end
+    end
+
+    context 'when user is not active' do
+      before { allow(user).to receive(:active?).and_return(false) }
+
+      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
+
+  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
+
+  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
+
+  describe 'Scope' do
+    let!(:user_import1) { create(:import, user: user) }
+    let!(:user_import2) { create(:import, user: user) }
+    let!(:other_user_import) { create(:import, user: other_user) }
+
+    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
diff --git a/spec/requests/imports_spec.rb b/spec/requests/imports_spec.rb
index 8bb53480..efe733c8 100644
--- a/spec/requests/imports_spec.rb
+++ b/spec/requests/imports_spec.rb
@@ -31,6 +31,85 @@ RSpec.describe 'Imports', type: :request do
           expect(response.body).to include(import.name)
         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) }
+        let!(:user_import) { create(:import, user: user) }
+
+        it 'only displays current users imports' do
+          get imports_path
+
+          expect(response.body).to include(user_import.name)
+          expect(response.body).not_to include(other_import.name)
+        end
+      end
+    end
+  end
+
+  describe 'GET /imports/:id' do
+    let(:user) { create(:user) }
+    let(:other_user) { create(:user) }
+    let(:import) { create(:import, user: user) }
+    let(:other_import) { create(:import, user: other_user) }
+
+    context 'when user is logged in' do
+      before { sign_in user }
+
+      it 'allows viewing own import' do
+        get import_path(import)
+        expect(response).to have_http_status(200)
+      end
+
+      it 'prevents viewing other users import' do
+        expect {
+          get import_path(other_import)
+        }.to raise_error(Pundit::NotAuthorizedError)
+      end
+    end
+
+    context 'when user is not logged in' do
+      it 'redirects to login' do
+        get import_path(import)
+        expect(response).to redirect_to(new_user_session_path)
+      end
+    end
+  end
+
+  describe 'GET /imports/new' do
+    let(:user) { create(:user) }
+
+    context 'when user is active' do
+      before do
+        allow(user).to receive(:active?).and_return(true)
+        sign_in user
+      end
+
+      it 'allows access to new import form' do
+        get new_import_path
+        expect(response).to have_http_status(200)
+      end
+    end
+
+    context 'when user is inactive' do
+      before do
+        allow(user).to receive(:active?).and_return(false)
+        sign_in user
+      end
+
+      it 'prevents access to new import form' do
+        expect {
+          get new_import_path
+        }.to raise_error(Pundit::NotAuthorizedError)
+      end
+    end
+
+    context 'when user is not logged in' do
+      it 'redirects to login' do
+        get new_import_path
+        expect(response).to redirect_to(new_user_session_path)
+      end
     end
   end
 

From a2b20cfdf12d63acc295d54a6f0a17f82af071c2 Mon Sep 17 00:00:00 2001
From: Eugene Burmakin <iamfrey@gmail.com>
Date: Tue, 29 Jul 2025 20:14:24 +0200
Subject: [PATCH 2/3] Fix tests

---
 app/controllers/imports_controller.rb     |  4 +++
 spec/requests/api/v1/stats_spec.rb        | 36 +++++++++++++++++------
 spec/serializers/point_serializer_spec.rb |  3 +-
 spec/serializers/stats_serializer_spec.rb | 14 +++++++--
 4 files changed, 45 insertions(+), 12 deletions(-)

diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb
index 61a9f690..2d7feef1 100644
--- a/app/controllers/imports_controller.rb
+++ b/app/controllers/imports_controller.rb
@@ -35,6 +35,10 @@ class ImportsController < ApplicationController
   end
 
   def create
+    @import = Import.new
+
+    authorize @import
+
     files_params = params.dig(:import, :files)
     raw_files = Array(files_params).reject(&:blank?)
 
diff --git a/spec/requests/api/v1/stats_spec.rb b/spec/requests/api/v1/stats_spec.rb
index 43e8f142..2e7eb8ae 100644
--- a/spec/requests/api/v1/stats_spec.rb
+++ b/spec/requests/api/v1/stats_spec.rb
@@ -3,22 +3,39 @@
 require 'rails_helper'
 
 RSpec.describe 'Api::V1::Stats', type: :request do
-  let(:user) { create(:user) }
-
   describe 'GET /index' do
-    let!(:user) { create(:user) }
-    let!(:stats_in_2020) { create_list(:stat, 12, year: 2020, user:) }
-    let!(:stats_in_2021) { create_list(:stat, 12, year: 2021, user:) }
-    let!(:points_in_2020) do
+    let(:user) { create(:user) }
+    let(:stats_in_2020) { create_list(:stat, 12, year: 2020, user:) }
+    let(:stats_in_2021) { create_list(:stat, 12, year: 2021, user:) }
+    let(:points_in_2020) do
       (1..85).map do |i|
-        create(:point, :with_geodata, :reverse_geocoded, timestamp: Time.zone.local(2020, 1, 1).to_i + i.hours, user:)
+        create(:point, :with_geodata,
+               timestamp: Time.zone.local(2020, 1, 1).to_i + i.hours,
+               user:,
+               country_name: 'Test Country',
+               city: 'Test City',
+               reverse_geocoded_at: Time.current)
       end
     end
-    let!(:points_in_2021) do
+    let(:points_in_2021) do
       (1..95).map do |i|
-        create(:point, :with_geodata, :reverse_geocoded, timestamp: Time.zone.local(2021, 1, 1).to_i + i.hours, user:)
+        create(:point, :with_geodata,
+               timestamp: Time.zone.local(2021, 1, 1).to_i + i.hours,
+               user:,
+               country_name: 'Test Country',
+               city: 'Test City',
+               reverse_geocoded_at: Time.current)
       end
     end
+
+    before do
+      # Create all the test data
+      stats_in_2020
+      stats_in_2021
+      points_in_2020
+      points_in_2021
+    end
+
     let(:expected_json) do
       {
         totalDistanceKm: (stats_in_2020.map(&:distance).sum + stats_in_2021.map(&:distance).sum) / 1000,
@@ -84,3 +101,4 @@ RSpec.describe 'Api::V1::Stats', type: :request do
     end
   end
 end
+
diff --git a/spec/serializers/point_serializer_spec.rb b/spec/serializers/point_serializer_spec.rb
index e202a761..c07e2a90 100644
--- a/spec/serializers/point_serializer_spec.rb
+++ b/spec/serializers/point_serializer_spec.rb
@@ -34,7 +34,8 @@ RSpec.describe PointSerializer do
         'course' => point.course,
         'course_accuracy' => point.course_accuracy,
         'external_track_id' => point.external_track_id,
-        'track_id' => point.track_id
+        'track_id' => point.track_id,
+        'country_name' => point.read_attribute(:country_name)
       }
     end
 
diff --git a/spec/serializers/stats_serializer_spec.rb b/spec/serializers/stats_serializer_spec.rb
index eef34e59..7198f48f 100644
--- a/spec/serializers/stats_serializer_spec.rb
+++ b/spec/serializers/stats_serializer_spec.rb
@@ -30,12 +30,22 @@ RSpec.describe StatsSerializer do
       let!(:stats_in_2021) { create_list(:stat, 12, year: 2021, user:) }
       let!(:points_in_2020) do
         (1..85).map do |i|
-          create(:point, :with_geodata, :reverse_geocoded, timestamp: Time.zone.local(2020, 1, 1).to_i + i.hours, user:)
+          create(:point, :with_geodata,
+                 timestamp: Time.zone.local(2020, 1, 1).to_i + i.hours,
+                 user:,
+                 country_name: 'Test Country',
+                 city: 'Test City',
+                 reverse_geocoded_at: Time.current)
         end
       end
       let!(:points_in_2021) do
         (1..95).map do |i|
-          create(:point, :with_geodata, :reverse_geocoded, timestamp: Time.zone.local(2021, 1, 1).to_i + i.hours, user:)
+          create(:point, :with_geodata,
+                 timestamp: Time.zone.local(2021, 1, 1).to_i + i.hours,
+                 user:,
+                 country_name: 'Test Country',
+                 city: 'Test City',
+                 reverse_geocoded_at: Time.current)
         end
       end
       let(:expected_json) do

From 6870be204580f606fe2bdb1933a05f043271cefd Mon Sep 17 00:00:00 2001
From: Eugene Burmakin <iamfrey@gmail.com>
Date: Tue, 29 Jul 2025 21:17:33 +0200
Subject: [PATCH 3/3] 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