From fd8f8cedd70cf71cb09a50490253b1dc01a6ca65 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Fri, 29 Aug 2025 17:24:09 +0200 Subject: [PATCH] Refactor code a bit --- app/jobs/tracks/create_job.rb | 5 +- app/jobs/tracks/daily_generation_job.rb | 13 ++-- app/jobs/tracks/parallel_generator_job.rb | 2 +- app/jobs/tracks/time_chunk_processor_job.rb | 1 + app/services/tracks/boundary_detector.rb | 1 - app/services/tracks/generator.rb | 20 ++--- app/services/tracks/segmentation.rb | 15 ++-- app/services/tracks/session_manager.rb | 4 +- spec/jobs/tracks/create_job_spec.rb | 5 +- spec/jobs/tracks/daily_generation_job_spec.rb | 30 +++----- .../services/tracks/boundary_detector_spec.rb | 6 +- .../tracks/parallel_generator_spec.rb | 34 ++++----- spec/services/tracks/session_manager_spec.rb | 73 +++++++++---------- .../services/users/export_data/points_spec.rb | 4 + 14 files changed, 95 insertions(+), 118 deletions(-) diff --git a/app/jobs/tracks/create_job.rb b/app/jobs/tracks/create_job.rb index 537c2f39..491ffca5 100644 --- a/app/jobs/tracks/create_job.rb +++ b/app/jobs/tracks/create_job.rb @@ -8,6 +8,9 @@ class Tracks::CreateJob < ApplicationJob Tracks::Generator.new(user, start_at:, end_at:, mode:).call rescue StandardError => e - ExceptionReporter.call(e, 'Failed to create tracks for user') + ExceptionReporter.call( + e, + "Failed to create tracks for user #{user_id} (mode: #{mode}, start_at: #{start_at.inspect}, end_at: #{end_at.inspect})" + ) end end diff --git a/app/jobs/tracks/daily_generation_job.rb b/app/jobs/tracks/daily_generation_job.rb index 275d0cfa..84d0f2d8 100644 --- a/app/jobs/tracks/daily_generation_job.rb +++ b/app/jobs/tracks/daily_generation_job.rb @@ -7,7 +7,7 @@ class Tracks::DailyGenerationJob < ApplicationJob def perform Rails.logger.info "Starting daily track generation for users with recent activity" - + users_with_recent_activity.find_each do |user| process_user_tracks(user) end @@ -18,11 +18,10 @@ class Tracks::DailyGenerationJob < ApplicationJob private def users_with_recent_activity - # Find users who have created points in the last 2 days - # This gives buffer to handle cross-day tracks - User.joins(:points) - .where(points: { created_at: 2.days.ago..Time.current }) - .distinct + # Users with points in last 2 days (buffer for cross-day tracks), via subquery + + user_ids = Point.where(created_at: 2.days.ago..Time.current).select(:user_id).distinct + User.where(id: user_ids) end def process_user_tracks(user) @@ -43,4 +42,4 @@ class Tracks::DailyGenerationJob < ApplicationJob Rails.logger.error "Failed to enqueue daily track generation for user #{user.id}: #{e.message}" ExceptionReporter.call(e, "Daily track generation failed for user #{user.id}") end -end \ No newline at end of file +end diff --git a/app/jobs/tracks/parallel_generator_job.rb b/app/jobs/tracks/parallel_generator_job.rb index 14ffb592..cc22afed 100644 --- a/app/jobs/tracks/parallel_generator_job.rb +++ b/app/jobs/tracks/parallel_generator_job.rb @@ -8,7 +8,7 @@ class Tracks::ParallelGeneratorJob < ApplicationJob def perform(user_id, start_at: nil, end_at: nil, mode: :bulk, chunk_size: 1.day) user = User.find(user_id) - session = Tracks::ParallelGenerator.new( + Tracks::ParallelGenerator.new( user, start_at: start_at, end_at: end_at, diff --git a/app/jobs/tracks/time_chunk_processor_job.rb b/app/jobs/tracks/time_chunk_processor_job.rb index d78923ca..15952fca 100644 --- a/app/jobs/tracks/time_chunk_processor_job.rb +++ b/app/jobs/tracks/time_chunk_processor_job.rb @@ -58,6 +58,7 @@ class Tracks::TimeChunkProcessorJob < ApplicationJob def load_chunk_points user.points + .without_raw_data .where(timestamp: chunk_data[:buffer_start_timestamp]..chunk_data[:buffer_end_timestamp]) .order(:timestamp) end diff --git a/app/services/tracks/boundary_detector.rb b/app/services/tracks/boundary_detector.rb index 6f88f4a8..b6a7fa61 100644 --- a/app/services/tracks/boundary_detector.rb +++ b/app/services/tracks/boundary_detector.rb @@ -38,7 +38,6 @@ class Tracks::BoundaryDetector return [] if recent_tracks.empty? # Group tracks that might be connected - boundary_groups = [] potential_groups = [] recent_tracks.each do |track| diff --git a/app/services/tracks/generator.rb b/app/services/tracks/generator.rb index 0510a4e5..113f10b6 100644 --- a/app/services/tracks/generator.rb +++ b/app/services/tracks/generator.rb @@ -70,16 +70,6 @@ class Tracks::Generator end end - def load_points - case mode - when :bulk then load_bulk_points - when :incremental then load_incremental_points - when :daily then load_daily_points - else - raise ArgumentError, "Tracks::Generator: Unknown mode: #{mode}" - end - end - def load_bulk_points scope = user.points.order(:timestamp) scope = scope.where(timestamp: timestamp_range) if time_range_defined? @@ -154,8 +144,7 @@ class Tracks::Generator case mode when :bulk then clean_bulk_tracks when :daily then clean_daily_tracks - else - raise ArgumentError, "Tracks::Generator: Unknown mode: #{mode}" + else unknown_mode! end end @@ -179,8 +168,7 @@ class Tracks::Generator when :bulk then bulk_timestamp_range when :daily then daily_timestamp_range when :incremental then incremental_timestamp_range - else - raise ArgumentError, "Tracks::Generator: Unknown mode: #{mode}" + else unknown_mode! end end @@ -212,4 +200,8 @@ class Tracks::Generator def time_threshold_minutes @time_threshold_minutes ||= user.safe_settings.minutes_between_routes.to_i end + + def unknown_mode! + raise ArgumentError, "Tracks::Generator: Unknown mode: #{mode}" + end end diff --git a/app/services/tracks/segmentation.rb b/app/services/tracks/segmentation.rb index 3dd8e853..388ebede 100644 --- a/app/services/tracks/segmentation.rb +++ b/app/services/tracks/segmentation.rb @@ -142,18 +142,13 @@ module Tracks::Segmentation # In-memory distance calculation using Geocoder (no SQL dependency) def calculate_km_distance_between_points_geocoder(point1, point2) - begin - distance = point1.distance_to_geocoder(point2, :km) + distance = point1.distance_to_geocoder(point2, :km) - # Validate result - if !distance.finite? || distance < 0 - return 0 - end + return 0 unless distance.finite? && distance >= 0 - distance - rescue StandardError => e - 0 - end + distance + rescue StandardError => _e + 0 end def should_finalize_segment?(segment_points, grace_period_minutes = 5) diff --git a/app/services/tracks/session_manager.rb b/app/services/tracks/session_manager.rb index 9a0280de..cf5e6815 100644 --- a/app/services/tracks/session_manager.rb +++ b/app/services/tracks/session_manager.rb @@ -44,7 +44,7 @@ class Tracks::SessionManager def get_session_data data = Rails.cache.read(cache_key) return nil unless data - + # Rails.cache already deserializes the data, no need for JSON parsing data end @@ -149,4 +149,4 @@ class Tracks::SessionManager def cache_key "#{CACHE_KEY_PREFIX}:user:#{user_id}:session:#{session_id}" end -end \ No newline at end of file +end diff --git a/spec/jobs/tracks/create_job_spec.rb b/spec/jobs/tracks/create_job_spec.rb index b23fea8d..693da3ec 100644 --- a/spec/jobs/tracks/create_job_spec.rb +++ b/spec/jobs/tracks/create_job_spec.rb @@ -14,7 +14,7 @@ RSpec.describe Tracks::CreateJob, type: :job do allow(generator_instance).to receive(:call).and_return(2) end - it 'calls the generator and creates a notification' do + it 'calls the generator' do described_class.new.perform(user.id) expect(Tracks::Generator).to have_received(:new).with( @@ -75,10 +75,9 @@ RSpec.describe Tracks::CreateJob, type: :job do before do allow(User).to receive(:find).with(999).and_raise(ActiveRecord::RecordNotFound) allow(ExceptionReporter).to receive(:call) - allow(Notifications::Create).to receive(:new).and_return(instance_double(Notifications::Create, call: nil)) end - it 'handles the error gracefully and creates error notification' do + it 'handles the error gracefully' do expect { described_class.new.perform(999) }.not_to raise_error expect(ExceptionReporter).to have_received(:call) diff --git a/spec/jobs/tracks/daily_generation_job_spec.rb b/spec/jobs/tracks/daily_generation_job_spec.rb index 8d28eb20..2b0bb990 100644 --- a/spec/jobs/tracks/daily_generation_job_spec.rb +++ b/spec/jobs/tracks/daily_generation_job_spec.rb @@ -8,7 +8,7 @@ RSpec.describe Tracks::DailyGenerationJob, type: :job do before do # Clear any existing jobs ActiveJob::Base.queue_adapter.enqueued_jobs.clear - + # Mock the incremental processing callback to avoid interference allow_any_instance_of(Point).to receive(:trigger_incremental_track_generation) end @@ -28,10 +28,10 @@ RSpec.describe Tracks::DailyGenerationJob, type: :job do before do # User1 - has points created yesterday (should be processed) create(:point, user: user1, created_at: 1.day.ago, timestamp: 1.day.ago.to_i) - + # User2 - has points created 1.5 days ago (should be processed) create(:point, user: user2, created_at: 1.5.days.ago, timestamp: 1.5.days.ago.to_i) - + # User3 - has points created 3 days ago (should NOT be processed) create(:point, user: user3, created_at: 3.days.ago, timestamp: 3.days.ago.to_i) end @@ -54,7 +54,7 @@ RSpec.describe Tracks::DailyGenerationJob, type: :job do args = enqueued_job['arguments'] user_id = args[0] options = args[1] - + expect([user1.id, user2.id]).to include(user_id) expect(options['mode']['value']).to eq('daily') # ActiveJob serializes symbols expect(options['chunk_size']['value']).to eq(6.hours.to_i) # ActiveJob serializes durations @@ -76,7 +76,7 @@ RSpec.describe Tracks::DailyGenerationJob, type: :job do it 'logs the process' do allow(Rails.logger).to receive(:info) - + expect(Rails.logger).to receive(:info).with("Starting daily track generation for users with recent activity") expect(Rails.logger).to receive(:info).with("Completed daily track generation") @@ -91,14 +91,12 @@ RSpec.describe Tracks::DailyGenerationJob, type: :job do end it 'does not enqueue any parallel generation jobs' do - expect { - job.perform - }.not_to have_enqueued_job(Tracks::ParallelGeneratorJob) + expect { job.perform }.not_to have_enqueued_job(Tracks::ParallelGeneratorJob) end it 'still logs start and completion' do allow(Rails.logger).to receive(:info) - + expect(Rails.logger).to receive(:info).with("Starting daily track generation for users with recent activity") expect(Rails.logger).to receive(:info).with("Completed daily track generation") @@ -109,7 +107,7 @@ RSpec.describe Tracks::DailyGenerationJob, type: :job do context 'when user processing fails' do before do create(:point, user: user1, created_at: 1.day.ago, timestamp: 1.day.ago.to_i) - + # Mock Tracks::ParallelGeneratorJob to raise an error allow(Tracks::ParallelGeneratorJob).to receive(:perform_later).and_raise(StandardError.new("Job failed")) allow(Rails.logger).to receive(:info) @@ -119,20 +117,16 @@ RSpec.describe Tracks::DailyGenerationJob, type: :job do expect(Rails.logger).to receive(:error).with("Failed to enqueue daily track generation for user #{user1.id}: Job failed") expect(ExceptionReporter).to receive(:call).with(instance_of(StandardError), "Daily track generation failed for user #{user1.id}") - expect { - job.perform - }.not_to raise_error + expect { job.perform }.not_to raise_error end end context 'with users having no points' do it 'does not process users without any points' do # user1, user2, user3 exist but have no points - - expect { - job.perform - }.not_to have_enqueued_job(Tracks::ParallelGeneratorJob) + + expect { job.perform }.not_to have_enqueued_job(Tracks::ParallelGeneratorJob) end end end -end \ No newline at end of file +end diff --git a/spec/services/tracks/boundary_detector_spec.rb b/spec/services/tracks/boundary_detector_spec.rb index 7a02b205..7938bd27 100644 --- a/spec/services/tracks/boundary_detector_spec.rb +++ b/spec/services/tracks/boundary_detector_spec.rb @@ -30,7 +30,7 @@ RSpec.describe Tracks::BoundaryDetector do expect(detector.resolve_cross_chunk_tracks).to eq(0) end - it 'does not log boundary operations when no candidates found' do + it 'returns 0 without boundary side effects when no candidates found' do # This test may log other things, but should not log boundary-related messages result = detector.resolve_cross_chunk_tracks expect(result).to eq(0) @@ -107,7 +107,7 @@ RSpec.describe Tracks::BoundaryDetector do allow(detector).to receive(:create_track_from_points).and_return(nil) end - it 'returns 0 and logs warning' do + it 'returns 0' do expect(detector.resolve_cross_chunk_tracks).to eq(0) end @@ -291,7 +291,7 @@ RSpec.describe Tracks::BoundaryDetector do it 'sorts points by timestamp' do # Create points out of order - point_early = create(:point, user: user, track: track2, timestamp: 3.hours.ago.to_i) + create(:point, user: user, track: track2, timestamp: 3.hours.ago.to_i) captured_points = nil allow(detector).to receive(:create_track_from_points) do |points, _distance| diff --git a/spec/services/tracks/parallel_generator_spec.rb b/spec/services/tracks/parallel_generator_spec.rb index 26d89802..f1adf9d1 100644 --- a/spec/services/tracks/parallel_generator_spec.rb +++ b/spec/services/tracks/parallel_generator_spec.rb @@ -80,15 +80,13 @@ RSpec.describe Tracks::ParallelGenerator do end it 'enqueues time chunk processor jobs' do - expect { - generator.call - }.to have_enqueued_job(Tracks::TimeChunkProcessorJob).at_least(:once) + expect { generator.call }.to \ + have_enqueued_job(Tracks::TimeChunkProcessorJob).at_least(:once) end it 'enqueues boundary resolver job with delay' do - expect { - generator.call - }.to have_enqueued_job(Tracks::BoundaryResolverJob).at(be >= 5.minutes.from_now) + expect { generator.call }.to \ + have_enqueued_job(Tracks::BoundaryResolverJob).at(be >= 5.minutes.from_now) end it 'logs the operation' do @@ -108,9 +106,7 @@ RSpec.describe Tracks::ParallelGenerator do end it 'does not enqueue any jobs' do - expect { - generator.call - }.not_to have_enqueued_job + expect { generator.call }.not_to have_enqueued_job end end @@ -191,17 +187,17 @@ RSpec.describe Tracks::ParallelGenerator do create(:point, user: user, timestamp: (10 - i).days.ago.to_i) end - expect { + expect do generator.call - }.to have_enqueued_job(Tracks::BoundaryResolverJob) + end.to have_enqueued_job(Tracks::BoundaryResolverJob) .with(user.id, kind_of(String)) end it 'ensures minimum delay for boundary resolver' do # Even with few chunks, should have minimum delay - expect { + expect do generator.call - }.to have_enqueued_job(Tracks::BoundaryResolverJob) + end.to have_enqueued_job(Tracks::BoundaryResolverJob) .at(be >= 5.minutes.from_now) end end @@ -216,9 +212,9 @@ RSpec.describe Tracks::ParallelGenerator do it 'raises error for unknown mode in clean_existing_tracks' do generator.instance_variable_set(:@mode, :unknown) - expect { + expect do generator.send(:clean_existing_tracks) - }.to raise_error(ArgumentError, 'Unknown mode: unknown') + end.to raise_error(ArgumentError, 'Unknown mode: unknown') end end @@ -311,16 +307,16 @@ RSpec.describe Tracks::ParallelGenerator do end it 'uses minimum delay for small chunk counts' do - expect { + expect do generator.send(:enqueue_boundary_resolver, session_id, 1) - }.to have_enqueued_job(Tracks::BoundaryResolverJob) + end.to have_enqueued_job(Tracks::BoundaryResolverJob) .at(be >= 5.minutes.from_now) end it 'scales delay with chunk count' do - expect { + expect do generator.send(:enqueue_boundary_resolver, session_id, 20) - }.to have_enqueued_job(Tracks::BoundaryResolverJob) + end.to have_enqueued_job(Tracks::BoundaryResolverJob) .at(be >= 10.minutes.from_now) end end diff --git a/spec/services/tracks/session_manager_spec.rb b/spec/services/tracks/session_manager_spec.rb index 61f5a1df..b0a72b5e 100644 --- a/spec/services/tracks/session_manager_spec.rb +++ b/spec/services/tracks/session_manager_spec.rb @@ -28,10 +28,10 @@ RSpec.describe Tracks::SessionManager do it 'creates a new session with default values' do result = manager.create_session(metadata) - + expect(result).to eq(manager) expect(manager.session_exists?).to be true - + session_data = manager.get_session_data expect(session_data['status']).to eq('pending') expect(session_data['total_chunks']).to eq(0) @@ -45,7 +45,7 @@ RSpec.describe Tracks::SessionManager do it 'sets TTL on the cache entry' do manager.create_session(metadata) - + # Check that the key exists and will expire expect(Rails.cache.exist?(manager.send(:cache_key))).to be true end @@ -59,7 +59,7 @@ RSpec.describe Tracks::SessionManager do it 'returns session data when session exists' do metadata = { test: 'data' } manager.create_session(metadata) - + data = manager.get_session_data expect(data).to be_a(Hash) expect(data['metadata']).to eq(metadata.deep_stringify_keys) @@ -85,9 +85,9 @@ RSpec.describe Tracks::SessionManager do it 'updates existing session data' do updates = { status: 'processing', total_chunks: 5 } result = manager.update_session(updates) - + expect(result).to be true - + data = manager.get_session_data expect(data['status']).to eq('processing') expect(data['total_chunks']).to eq(5) @@ -96,7 +96,7 @@ RSpec.describe Tracks::SessionManager do it 'returns false when session does not exist' do manager.cleanup_session result = manager.update_session({ status: 'processing' }) - + expect(result).to be false end @@ -104,9 +104,9 @@ RSpec.describe Tracks::SessionManager do original_metadata = { mode: 'bulk' } manager.cleanup_session manager.create_session(original_metadata) - + manager.update_session({ status: 'processing' }) - + data = manager.get_session_data expect(data['metadata']).to eq(original_metadata.stringify_keys) expect(data['status']).to eq('processing') @@ -120,9 +120,9 @@ RSpec.describe Tracks::SessionManager do it 'marks session as processing with total chunks' do result = manager.mark_started(10) - + expect(result).to be true - + data = manager.get_session_data expect(data['status']).to eq('processing') expect(data['total_chunks']).to eq(10) @@ -137,11 +137,9 @@ RSpec.describe Tracks::SessionManager do end it 'increments completed chunks counter' do - expect { + expect do manager.increment_completed_chunks - }.to change { - manager.get_session_data['completed_chunks'] - }.from(0).to(1) + end.to change { manager.get_session_data['completed_chunks'] }.from(0).to(1) end it 'returns false when session does not exist' do @@ -156,23 +154,20 @@ RSpec.describe Tracks::SessionManager do end it 'increments tracks created counter by 1 by default' do - expect { + expect do manager.increment_tracks_created - }.to change { - manager.get_session_data['tracks_created'] - }.from(0).to(1) + end.to change { manager.get_session_data['tracks_created'] }.from(0).to(1) end it 'increments tracks created counter by specified amount' do - expect { + expect do manager.increment_tracks_created(5) - }.to change { - manager.get_session_data['tracks_created'] - }.from(0).to(5) + end.to change { manager.get_session_data['tracks_created'] }.from(0).to(5) end it 'returns false when session does not exist' do manager.cleanup_session + expect(manager.increment_tracks_created).to be false end end @@ -184,9 +179,9 @@ RSpec.describe Tracks::SessionManager do it 'marks session as completed with timestamp' do result = manager.mark_completed - + expect(result).to be true - + data = manager.get_session_data expect(data['status']).to eq('completed') expect(data['completed_at']).to be_present @@ -200,11 +195,11 @@ RSpec.describe Tracks::SessionManager do it 'marks session as failed with error message and timestamp' do error_message = 'Something went wrong' - + result = manager.mark_failed(error_message) - + expect(result).to be true - + data = manager.get_session_data expect(data['status']).to eq('failed') expect(data['error_message']).to eq(error_message) @@ -256,14 +251,14 @@ RSpec.describe Tracks::SessionManager do it 'calculates correct percentage' do manager.mark_started(4) 2.times { manager.increment_completed_chunks } - + expect(manager.progress_percentage).to eq(50.0) end it 'rounds to 2 decimal places' do manager.mark_started(3) manager.increment_completed_chunks - + expect(manager.progress_percentage).to eq(33.33) end end @@ -275,9 +270,9 @@ RSpec.describe Tracks::SessionManager do it 'removes session from cache' do expect(manager.session_exists?).to be true - + manager.cleanup_session - + expect(manager.session_exists?).to be false end end @@ -287,11 +282,11 @@ RSpec.describe Tracks::SessionManager do it 'creates and returns a session manager' do result = described_class.create_for_user(user_id, metadata) - + expect(result).to be_a(described_class) expect(result.user_id).to eq(user_id) expect(result.session_exists?).to be true - + data = result.get_session_data expect(data['metadata']).to eq(metadata.deep_stringify_keys) end @@ -305,9 +300,9 @@ RSpec.describe Tracks::SessionManager do it 'returns session manager when session exists' do manager.create_session - + result = described_class.find_session(user_id, session_id) - + expect(result).to be_a(described_class) expect(result.user_id).to eq(user_id) expect(result.session_id).to eq(session_id) @@ -324,16 +319,16 @@ RSpec.describe Tracks::SessionManager do it 'uses user-scoped cache keys' do expected_key = "track_generation:user:#{user_id}:session:#{session_id}" actual_key = manager.send(:cache_key) - + expect(actual_key).to eq(expected_key) end it 'prevents cross-user session access' do manager.create_session other_manager = described_class.new(999, session_id) - + expect(manager.session_exists?).to be true expect(other_manager.session_exists?).to be false end end -end \ No newline at end of file +end diff --git a/spec/services/users/export_data/points_spec.rb b/spec/services/users/export_data/points_spec.rb index b2fa0a52..4e761bda 100644 --- a/spec/services/users/export_data/points_spec.rb +++ b/spec/services/users/export_data/points_spec.rb @@ -3,6 +3,10 @@ require 'rails_helper' RSpec.describe Users::ExportData::Points, type: :service do + before do + allow_any_instance_of(Point).to receive(:trigger_incremental_track_generation) + end + let(:user) { create(:user) } let(:service) { described_class.new(user) }