Add country_name to points and fix some bugs.

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
 

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

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

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

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

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

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

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

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

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

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

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"

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

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

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

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