From f492a69fbbf7cb56c60b69e14cf8bf14c0617a76 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Mon, 28 Jul 2025 18:53:19 +0200 Subject: [PATCH] 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