diff --git a/CHANGELOG.md b/CHANGELOG.md index b8ccec99..d01a85b4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -17,6 +17,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). - Validate trip start date to be earlier than end date. #2057 - Fog of war radius slider in map v2 settings is now being respected correctly. #2041 - Applying changes in map v2 settings now works correctly. #2041 +- Invalidate stats cache on recalculation and other operations that change stats data. # [0.36.4] - 2025-12-26 diff --git a/app/jobs/points/nightly_reverse_geocoding_job.rb b/app/jobs/points/nightly_reverse_geocoding_job.rb index d536679f..5321f624 100644 --- a/app/jobs/points/nightly_reverse_geocoding_job.rb +++ b/app/jobs/points/nightly_reverse_geocoding_job.rb @@ -6,8 +6,16 @@ class Points::NightlyReverseGeocodingJob < ApplicationJob def perform return unless DawarichSettings.reverse_geocoding_enabled? + processed_user_ids = Set.new + Point.not_reverse_geocoded.find_each(batch_size: 1000) do |point| point.async_reverse_geocode + processed_user_ids.add(point.user_id) + end + + # Invalidate caches for all users who had points reverse geocoded + processed_user_ids.each do |user_id| + Cache::InvalidateUserCaches.new(user_id).call end end end diff --git a/app/services/cache/clean.rb b/app/services/cache/clean.rb index ecbfafed..e555e6a4 100644 --- a/app/services/cache/clean.rb +++ b/app/services/cache/clean.rb @@ -36,8 +36,8 @@ class Cache::Clean def delete_countries_cities_cache User.find_each do |user| - Rails.cache.delete("dawarich/user_#{user.id}_countries") - Rails.cache.delete("dawarich/user_#{user.id}_cities") + Rails.cache.delete("dawarich/user_#{user.id}_countries_visited") + Rails.cache.delete("dawarich/user_#{user.id}_cities_visited") end end end diff --git a/app/services/cache/invalidate_user_caches.rb b/app/services/cache/invalidate_user_caches.rb new file mode 100644 index 00000000..839efdae --- /dev/null +++ b/app/services/cache/invalidate_user_caches.rb @@ -0,0 +1,34 @@ +# frozen_string_literal: true + +class Cache::InvalidateUserCaches + # Invalidates user-specific caches that depend on point data. + # This should be called after: + # - Reverse geocoding operations (updates country/city data) + # - Stats calculations (updates geocoding stats) + # - Bulk point imports/updates + def initialize(user_id) + @user_id = user_id + end + + def call + invalidate_countries_visited + invalidate_cities_visited + invalidate_points_geocoded_stats + end + + def invalidate_countries_visited + Rails.cache.delete("dawarich/user_#{user_id}_countries_visited") + end + + def invalidate_cities_visited + Rails.cache.delete("dawarich/user_#{user_id}_cities_visited") + end + + def invalidate_points_geocoded_stats + Rails.cache.delete("dawarich/user_#{user_id}_points_geocoded_stats") + end + + private + + attr_reader :user_id +end diff --git a/app/services/stats/calculate_month.rb b/app/services/stats/calculate_month.rb index ff02dbbe..7b8a5bb9 100644 --- a/app/services/stats/calculate_month.rb +++ b/app/services/stats/calculate_month.rb @@ -42,6 +42,9 @@ class Stats::CalculateMonth ) stat.save! + + # Invalidate user caches after stats calculation since they may have changed + Cache::InvalidateUserCaches.new(user.id).call end end diff --git a/spec/jobs/imports/destroy_job_spec.rb b/spec/jobs/imports/destroy_job_spec.rb new file mode 100644 index 00000000..3a50c638 --- /dev/null +++ b/spec/jobs/imports/destroy_job_spec.rb @@ -0,0 +1,192 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Imports::DestroyJob, type: :job do + describe '#perform' do + let(:user) { create(:user) } + let(:import) { create(:import, user: user, status: :completed) } + + describe 'queue configuration' do + it 'uses the default queue' do + expect(described_class.queue_name).to eq('default') + end + end + + context 'when import exists' do + before do + # Create some points for the import + create_list(:point, 3, user: user, import: import) + end + + it 'changes import status to deleting and deletes it' do + expect(import).not_to be_deleting + + import_id = import.id + described_class.perform_now(import_id) + + expect(Import.find_by(id: import_id)).to be_nil + end + + it 'calls the Imports::Destroy service' do + destroy_service = instance_double(Imports::Destroy) + allow(Imports::Destroy).to receive(:new).with(user, import).and_return(destroy_service) + allow(destroy_service).to receive(:call) + + described_class.perform_now(import.id) + + expect(Imports::Destroy).to have_received(:new).with(user, import) + expect(destroy_service).to have_received(:call) + end + + it 'broadcasts status update to the user' do + allow(ImportsChannel).to receive(:broadcast_to) + + described_class.perform_now(import.id) + + expect(ImportsChannel).to have_received(:broadcast_to).with( + user, + hash_including( + action: 'status_update', + import: hash_including( + id: import.id, + status: 'deleting' + ) + ) + ).at_least(:once) + end + + it 'broadcasts deletion complete to the user' do + allow(ImportsChannel).to receive(:broadcast_to) + + described_class.perform_now(import.id) + + expect(ImportsChannel).to have_received(:broadcast_to).with( + user, + hash_including( + action: 'delete', + import: hash_including(id: import.id) + ) + ).at_least(:once) + end + + it 'broadcasts both status update and deletion messages' do + allow(ImportsChannel).to receive(:broadcast_to) + + described_class.perform_now(import.id) + + expect(ImportsChannel).to have_received(:broadcast_to).twice + end + + it 'deletes the import and its points' do + import_id = import.id + point_ids = import.points.pluck(:id) + + described_class.perform_now(import_id) + + expect(Import.find_by(id: import_id)).to be_nil + expect(Point.where(id: point_ids)).to be_empty + end + end + + context 'when import does not exist' do + let(:non_existent_id) { 999_999 } + + it 'does not raise an error' do + expect { described_class.perform_now(non_existent_id) }.not_to raise_error + end + + it 'does not call the Imports::Destroy service' do + expect(Imports::Destroy).not_to receive(:new) + + described_class.perform_now(non_existent_id) + end + + it 'does not broadcast any messages' do + expect(ImportsChannel).not_to receive(:broadcast_to) + + described_class.perform_now(non_existent_id) + end + + it 'returns early without logging' do + allow(Rails.logger).to receive(:warn) + + described_class.perform_now(non_existent_id) + + # Early return at line 8 doesn't log, only rescue block does + expect(Rails.logger).not_to have_received(:warn) + end + end + + context 'when import is deleted during job execution' do + it 'handles RecordNotFound gracefully' do + allow(Import).to receive(:find_by).with(id: import.id).and_return(import) + allow(import).to receive(:deleting!).and_raise(ActiveRecord::RecordNotFound) + + expect { described_class.perform_now(import.id) }.not_to raise_error + end + + it 'logs a warning when RecordNotFound is raised' do + allow(Import).to receive(:find_by).with(id: import.id).and_return(import) + allow(import).to receive(:deleting!).and_raise(ActiveRecord::RecordNotFound) + allow(Rails.logger).to receive(:warn) + + described_class.perform_now(import.id) + + expect(Rails.logger).to have_received(:warn).with(/Import #{import.id} not found/) + end + end + + context 'when broadcast fails' do + before do + allow(ImportsChannel).to receive(:broadcast_to).and_raise(StandardError, 'Broadcast error') + end + + it 'allows the error to propagate' do + expect { described_class.perform_now(import.id) }.to raise_error(StandardError, 'Broadcast error') + end + end + + context 'when Imports::Destroy service fails' do + before do + allow_any_instance_of(Imports::Destroy).to receive(:call).and_raise(StandardError, 'Destroy failed') + end + + it 'allows the error to propagate' do + expect { described_class.perform_now(import.id) }.to raise_error(StandardError, 'Destroy failed') + end + + it 'has already set status to deleting before service is called' do + expect do + described_class.perform_now(import.id) rescue StandardError + end.to change { import.reload.status }.to('deleting') + end + end + + context 'with multiple imports for different users' do + let(:user2) { create(:user) } + let(:import2) { create(:import, user: user2, status: :completed) } + + it 'only broadcasts to the correct user' do + expect(ImportsChannel).to receive(:broadcast_to).with(user, anything).twice + expect(ImportsChannel).not_to receive(:broadcast_to).with(user2, anything) + + described_class.perform_now(import.id) + end + end + + context 'job enqueuing' do + it 'can be enqueued' do + expect do + described_class.perform_later(import.id) + end.to have_enqueued_job(described_class).with(import.id) + end + + it 'can be performed later with correct arguments' do + expect do + described_class.perform_later(import.id) + end.to have_enqueued_job(described_class).on_queue('default').with(import.id) + end + end + end +end diff --git a/spec/jobs/points/nightly_reverse_geocoding_job_spec.rb b/spec/jobs/points/nightly_reverse_geocoding_job_spec.rb index 28dbb9a5..0465852f 100644 --- a/spec/jobs/points/nightly_reverse_geocoding_job_spec.rb +++ b/spec/jobs/points/nightly_reverse_geocoding_job_spec.rb @@ -62,18 +62,22 @@ RSpec.describe Points::NightlyReverseGeocodingJob, type: :job do end context 'with points needing reverse geocoding' do + let(:user2) { create(:user) } let!(:point_without_geocoding1) do create(:point, user: user, reverse_geocoded_at: nil) end let!(:point_without_geocoding2) do create(:point, user: user, reverse_geocoded_at: nil) end + let!(:point_without_geocoding3) do + create(:point, user: user2, reverse_geocoded_at: nil) + end let!(:geocoded_point) do create(:point, user: user, reverse_geocoded_at: 1.day.ago) end it 'processes all points that need reverse geocoding' do - expect { described_class.perform_now }.to have_enqueued_job(ReverseGeocodingJob).exactly(2).times + expect { described_class.perform_now }.to have_enqueued_job(ReverseGeocodingJob).exactly(3).times end it 'enqueues jobs with correct parameters' do @@ -82,6 +86,8 @@ RSpec.describe Points::NightlyReverseGeocodingJob, type: :job do .with('Point', point_without_geocoding1.id) .and have_enqueued_job(ReverseGeocodingJob) .with('Point', point_without_geocoding2.id) + .and have_enqueued_job(ReverseGeocodingJob) + .with('Point', point_without_geocoding3.id) end it 'uses find_each with correct batch size' do @@ -93,6 +99,44 @@ RSpec.describe Points::NightlyReverseGeocodingJob, type: :job do expect(relation_mock).to have_received(:find_each).with(batch_size: 1000) end + + it 'invalidates caches for all affected users' do + allow(Cache::InvalidateUserCaches).to receive(:new).and_call_original + + described_class.perform_now + + # Verify that cache invalidation service was instantiated for both users + expect(Cache::InvalidateUserCaches).to have_received(:new).with(user.id) + expect(Cache::InvalidateUserCaches).to have_received(:new).with(user2.id) + end + + it 'invalidates caches for the correct users' do + cache_service1 = instance_double(Cache::InvalidateUserCaches) + cache_service2 = instance_double(Cache::InvalidateUserCaches) + + allow(Cache::InvalidateUserCaches).to receive(:new).with(user.id).and_return(cache_service1) + allow(Cache::InvalidateUserCaches).to receive(:new).with(user2.id).and_return(cache_service2) + allow(cache_service1).to receive(:call) + allow(cache_service2).to receive(:call) + + described_class.perform_now + + expect(cache_service1).to have_received(:call) + expect(cache_service2).to have_received(:call) + end + + it 'does not invalidate caches multiple times for the same user' do + # user has 2 points, but cache should only be invalidated once + cache_service = instance_double(Cache::InvalidateUserCaches) + + allow(Cache::InvalidateUserCaches).to receive(:new).with(user.id).and_return(cache_service) + allow(Cache::InvalidateUserCaches).to receive(:new).with(user2.id).and_return(instance_double(Cache::InvalidateUserCaches, call: nil)) + allow(cache_service).to receive(:call) + + described_class.perform_now + + expect(cache_service).to have_received(:call).once + end end end diff --git a/spec/requests/imports_spec.rb b/spec/requests/imports_spec.rb index 71ed302c..1e40366f 100644 --- a/spec/requests/imports_spec.rb +++ b/spec/requests/imports_spec.rb @@ -223,9 +223,10 @@ RSpec.describe 'Imports', type: :request do it 'deletes the import' do expect do delete import_path(import) - end.to change(user.imports, :count).by(-1) + end.to have_enqueued_job(Imports::DestroyJob).with(import.id) expect(response).to redirect_to(imports_path) + expect(import.reload).to be_deleting end end end diff --git a/spec/services/cache/clean_spec.rb b/spec/services/cache/clean_spec.rb index 1d0ee55c..7be77a20 100644 --- a/spec/services/cache/clean_spec.rb +++ b/spec/services/cache/clean_spec.rb @@ -12,10 +12,10 @@ RSpec.describe Cache::Clean do let(:user_2_years_tracked_key) { "dawarich/user_#{user2.id}_years_tracked" } let(:user_1_points_geocoded_stats_key) { "dawarich/user_#{user1.id}_points_geocoded_stats" } let(:user_2_points_geocoded_stats_key) { "dawarich/user_#{user2.id}_points_geocoded_stats" } - let(:user_1_countries_key) { "dawarich/user_#{user1.id}_countries" } - let(:user_2_countries_key) { "dawarich/user_#{user2.id}_countries" } - let(:user_1_cities_key) { "dawarich/user_#{user1.id}_cities" } - let(:user_2_cities_key) { "dawarich/user_#{user2.id}_cities" } + let(:user_1_countries_key) { "dawarich/user_#{user1.id}_countries_visited" } + let(:user_2_countries_key) { "dawarich/user_#{user2.id}_countries_visited" } + let(:user_1_cities_key) { "dawarich/user_#{user1.id}_cities_visited" } + let(:user_2_cities_key) { "dawarich/user_#{user2.id}_cities_visited" } before do # Set up cache entries that should be cleaned diff --git a/spec/services/cache/invalidate_user_caches_spec.rb b/spec/services/cache/invalidate_user_caches_spec.rb new file mode 100644 index 00000000..27fc5f2c --- /dev/null +++ b/spec/services/cache/invalidate_user_caches_spec.rb @@ -0,0 +1,60 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Cache::InvalidateUserCaches do + let(:user) { create(:user) } + let(:service) { described_class.new(user.id) } + + describe '#call' do + it 'invalidates all user-related caches' do + # Pre-populate the caches + Rails.cache.write("dawarich/user_#{user.id}_countries_visited", ['USA', 'Canada']) + Rails.cache.write("dawarich/user_#{user.id}_cities_visited", ['New York', 'Toronto']) + Rails.cache.write("dawarich/user_#{user.id}_points_geocoded_stats", { geocoded: 100, without_data: 10 }) + + # Verify caches are populated + expect(Rails.cache.read("dawarich/user_#{user.id}_countries_visited")).to eq(['USA', 'Canada']) + expect(Rails.cache.read("dawarich/user_#{user.id}_cities_visited")).to eq(['New York', 'Toronto']) + expect(Rails.cache.read("dawarich/user_#{user.id}_points_geocoded_stats")).to eq({ geocoded: 100, without_data: 10 }) + + # Invalidate caches + service.call + + # Verify caches are cleared + expect(Rails.cache.read("dawarich/user_#{user.id}_countries_visited")).to be_nil + expect(Rails.cache.read("dawarich/user_#{user.id}_cities_visited")).to be_nil + expect(Rails.cache.read("dawarich/user_#{user.id}_points_geocoded_stats")).to be_nil + end + end + + describe '#invalidate_countries_visited' do + it 'deletes the countries_visited cache' do + Rails.cache.write("dawarich/user_#{user.id}_countries_visited", ['USA', 'Canada']) + + service.invalidate_countries_visited + + expect(Rails.cache.read("dawarich/user_#{user.id}_countries_visited")).to be_nil + end + end + + describe '#invalidate_cities_visited' do + it 'deletes the cities_visited cache' do + Rails.cache.write("dawarich/user_#{user.id}_cities_visited", ['New York', 'Toronto']) + + service.invalidate_cities_visited + + expect(Rails.cache.read("dawarich/user_#{user.id}_cities_visited")).to be_nil + end + end + + describe '#invalidate_points_geocoded_stats' do + it 'deletes the points_geocoded_stats cache' do + Rails.cache.write("dawarich/user_#{user.id}_points_geocoded_stats", { geocoded: 100, without_data: 10 }) + + service.invalidate_points_geocoded_stats + + expect(Rails.cache.read("dawarich/user_#{user.id}_points_geocoded_stats")).to be_nil + end + end +end diff --git a/spec/services/stats/calculate_month_spec.rb b/spec/services/stats/calculate_month_spec.rb index 969926f9..6c4b83b1 100644 --- a/spec/services/stats/calculate_month_spec.rb +++ b/spec/services/stats/calculate_month_spec.rb @@ -201,6 +201,29 @@ RSpec.describe Stats::CalculateMonth do end end end + + context 'when invalidating caches' do + it 'invalidates user caches after updating stats' do + # Use existing points from the parent context + cache_service = instance_double(Cache::InvalidateUserCaches) + allow(Cache::InvalidateUserCaches).to receive(:new).with(user.id).and_return(cache_service) + allow(cache_service).to receive(:call) + + calculate_stats + + expect(cache_service).to have_received(:call) + end + + it 'does not invalidate caches when there are no points' do + # Create a new user without points + new_user = create(:user) + service = described_class.new(new_user.id, year, month) + + expect(Cache::InvalidateUserCaches).not_to receive(:new) + + service.call + end + end end end end