diff --git a/app/controllers/users/digests_controller.rb b/app/controllers/users/digests_controller.rb index 5016b81f..abbaa883 100644 --- a/app/controllers/users/digests_controller.rb +++ b/app/controllers/users/digests_controller.rb @@ -6,7 +6,7 @@ class Users::DigestsController < ApplicationController before_action :authenticate_user! before_action :authenticate_active_user!, only: [:create] - before_action :set_digest, only: [:show] + before_action :set_digest, only: %i[show destroy] def index @digests = current_user.digests.yearly.order(year: :desc) @@ -30,6 +30,12 @@ class Users::DigestsController < ApplicationController end end + def destroy + year = @digest.year + @digest.destroy! + redirect_to users_digests_path, notice: "Year-end digest for #{year} has been deleted", status: :see_other + end + private def set_digest diff --git a/app/models/users/digest.rb b/app/models/users/digest.rb index 843aa115..b4d94549 100644 --- a/app/models/users/digest.rb +++ b/app/models/users/digest.rb @@ -132,6 +132,11 @@ class Users::Digest < ApplicationRecord (all_time_stats['total_distance'] || 0).to_i end + def untracked_days + days_in_year = Date.leap?(year) ? 366 : 365 + [days_in_year - total_tracked_days, 0].max.round(1) + end + def distance_km distance.to_f / 1000 end @@ -151,4 +156,12 @@ class Users::Digest < ApplicationRecord def generate_sharing_uuid self.sharing_uuid ||= SecureRandom.uuid end + + def total_tracked_days + (total_tracked_minutes / 1440.0).round(1) + end + + def total_tracked_minutes + top_countries_by_time.sum { |country| country['minutes'].to_i } + end end diff --git a/app/services/users/digests/calculate_year.rb b/app/services/users/digests/calculate_year.rb index 857effc9..217b51e7 100644 --- a/app/services/users/digests/calculate_year.rb +++ b/app/services/users/digests/calculate_year.rb @@ -3,6 +3,8 @@ module Users module Digests class CalculateYear + MAX_COUNTRY_GAP_SECONDS = 60 * 60 # 60 minutes + def initialize(user_id, year) @user = ::User.find(user_id) @year = year.to_i @@ -50,7 +52,7 @@ module Users next unless toponym.is_a?(Hash) country = toponym['country'] - next unless country.present? + next if country.blank? if toponym['cities'].is_a?(Array) toponym['cities'].each do |city| @@ -95,29 +97,32 @@ module Users end def calculate_country_time_spent - country_days = build_country_days_map + country_minutes = calculate_actual_country_minutes - # Convert days to minutes (days * 24 * 60) and return top 10 - country_days - .transform_values { |days| days.size * 24 * 60 } + country_minutes .sort_by { |_, minutes| -minutes } .first(10) .map { |name, minutes| { 'name' => name, 'minutes' => minutes } } end - def build_country_days_map - year_points = fetch_year_points_with_country - country_days = Hash.new { |h, k| h[k] = Set.new } + def calculate_actual_country_minutes + points = fetch_year_points_with_country_ordered + country_minutes = Hash.new(0) - year_points.each do |point| - date = Time.zone.at(point.timestamp).to_date - country_days[point.country_name].add(date) + points.each_cons(2) do |point_a, point_b| + next if point_a.country_name != point_b.country_name + + gap_seconds = point_b.timestamp - point_a.timestamp + next if gap_seconds > MAX_COUNTRY_GAP_SECONDS + next if gap_seconds <= 0 + + country_minutes[point_a.country_name] += (gap_seconds / 60) end - country_days + country_minutes end - def fetch_year_points_with_country + def fetch_year_points_with_country_ordered start_of_year = Time.zone.local(year, 1, 1, 0, 0, 0) end_of_year = start_of_year.end_of_year @@ -126,6 +131,7 @@ module Users .where('timestamp >= ? AND timestamp <= ?', start_of_year.to_i, end_of_year.to_i) .where.not(country_name: [nil, '']) .select(:country_name, :timestamp) + .order(timestamp: :asc) end def calculate_city_time_spent diff --git a/app/views/users/digests/show.html.erb b/app/views/users/digests/show.html.erb index e9ef7ed5..d3d28600 100644 --- a/app/views/users/digests/show.html.erb +++ b/app/views/users/digests/show.html.erb @@ -142,6 +142,19 @@ <%= format_time_spent(country['minutes']) %> <% end %> + + <% if @digest.untracked_days > 0 %> +
+
+ ? + No tracking data +
+ <%= pluralize(@digest.untracked_days.round, 'day') %> +
+

+ <%= icon 'lightbulb' %> Track more in <%= @digest.year + 1 %> to see a fuller picture of your travels! +

+ <% end %> @@ -214,6 +227,12 @@ + <%= button_to users_digest_path(year: @digest.year), + method: :delete, + class: 'btn btn-outline btn-error', + data: { turbo_confirm: "Are you sure you want to delete the #{@digest.year} digest? This cannot be undone." } do %> + <%= icon 'trash-2' %> Delete + <% end %> diff --git a/app/views/users/digests_mailer/year_end_digest.html.erb b/app/views/users/digests_mailer/year_end_digest.html.erb index 66a6f4d5..582c53db 100644 --- a/app/views/users/digests_mailer/year_end_digest.html.erb +++ b/app/views/users/digests_mailer/year_end_digest.html.erb @@ -250,13 +250,24 @@
Where You Spent the Most Time
+ <% if @digest.untracked_days > 0 %> +

+ 💡 Track more in <%= @digest.year + 1 %> to see a fuller picture of your travels! +

+ <% end %>
<% end %> diff --git a/config/routes.rb b/config/routes.rb index ff40c143..64ec52ba 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -101,8 +101,8 @@ Rails.application.routes.draw do # User digests routes (yearly/monthly digest reports) scope module: 'users' do - resources :digests, only: %i[index create], param: :year, as: :users_digests - get 'digests/:year', to: 'digests#show', as: :users_digest, constraints: { year: /\d{4}/ } + resources :digests, only: %i[index create show destroy], param: :year, as: :users_digests, + constraints: { year: /\d{4}/ } end get 'shared/digest/:uuid', to: 'shared/digests#show', as: :shared_users_digest patch 'digests/:year/sharing', diff --git a/db/schema.rb b/db/schema.rb index b472e90b..d7baaeb4 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -522,6 +522,7 @@ ActiveRecord::Schema[8.0].define(version: 2026_01_03_114630) do add_foreign_key "notifications", "users" add_foreign_key "place_visits", "places" add_foreign_key "place_visits", "visits" + add_foreign_key "points", "points_raw_data_archives", column: "raw_data_archive_id", name: "fk_rails_points_raw_data_archives", on_delete: :nullify, validate: false add_foreign_key "points", "points_raw_data_archives", column: "raw_data_archive_id", on_delete: :nullify add_foreign_key "points", "users" add_foreign_key "points", "visits" diff --git a/spec/requests/users/digests_spec.rb b/spec/requests/users/digests_spec.rb index 56e65ec0..485b0fd7 100644 --- a/spec/requests/users/digests_spec.rb +++ b/spec/requests/users/digests_spec.rb @@ -27,6 +27,14 @@ RSpec.describe '/digests', type: :request do expect(response.status).to eq(302) end end + + describe 'DELETE /destroy' do + it 'redirects to the sign in page' do + delete users_digest_url(year: 2024) + + expect(response).to redirect_to(new_user_session_path) + end + end end context 'when user is signed in' do @@ -137,5 +145,40 @@ RSpec.describe '/digests', type: :request do end end end + + describe 'DELETE /destroy' do + let!(:digest) { create(:users_digest, user:, year: 2024) } + + it 'deletes the digest' do + expect do + delete users_digest_url(year: 2024) + end.to change(Users::Digest, :count).by(-1) + end + + it 'redirects with success notice' do + delete users_digest_url(year: 2024) + + expect(response).to redirect_to(users_digests_path) + expect(flash[:notice]).to eq('Year-end digest for 2024 has been deleted') + end + + it 'returns not found for non-existent digest' do + delete users_digest_url(year: 2020) + + expect(response).to redirect_to(users_digests_path) + expect(flash[:alert]).to eq('Digest not found') + end + + it 'cannot delete another user digest' do + other_user = create(:user) + other_digest = create(:users_digest, user: other_user, year: 2023) + + delete users_digest_url(year: 2023) + + expect(response).to redirect_to(users_digests_path) + expect(flash[:alert]).to eq('Digest not found') + expect(other_digest.reload).to be_present + end + end end end diff --git a/spec/services/points/raw_data/verifier_spec.rb b/spec/services/points/raw_data/verifier_spec.rb index 5611748a..6f035230 100644 --- a/spec/services/points/raw_data/verifier_spec.rb +++ b/spec/services/points/raw_data/verifier_spec.rb @@ -61,16 +61,18 @@ RSpec.describe Points::RawData::Verifier do end.not_to change { archive.reload.verified_at } end - it 'detects deleted points' do + it 'still verifies successfully when points are deleted from database' do # Force archive creation first archive_id = archive.id # Then delete one point from database points.first.destroy + # Verification should still succeed - deleted points are acceptable + # (users should be able to delete their data without failing archive verification) expect do verifier.verify_specific_archive(archive_id) - end.not_to change { archive.reload.verified_at } + end.to change { archive.reload.verified_at }.from(nil) end it 'detects raw_data mismatch between archive and database' do diff --git a/spec/services/users/digests/calculate_year_spec.rb b/spec/services/users/digests/calculate_year_spec.rb index cef3ff98..8769b785 100644 --- a/spec/services/users/digests/calculate_year_spec.rb +++ b/spec/services/users/digests/calculate_year_spec.rb @@ -76,22 +76,28 @@ RSpec.describe Users::Digests::CalculateYear do expect(calculate_digest.monthly_distances['3']).to eq('0') # Missing month end - it 'calculates time spent by location' do - # Create points to enable country time calculation based on unique days - jan_1 = Time.zone.local(2024, 1, 1, 10, 0, 0).to_i - jan_2 = Time.zone.local(2024, 1, 2, 10, 0, 0).to_i - feb_1 = Time.zone.local(2024, 2, 1, 10, 0, 0).to_i + it 'calculates time spent by location using actual minutes between consecutive points' do + # Create points with specific gaps to test actual minute calculation + jan_1_10am = Time.zone.local(2024, 1, 1, 10, 0, 0).to_i + jan_1_11am = Time.zone.local(2024, 1, 1, 11, 0, 0).to_i # 60 min later + jan_1_12pm = Time.zone.local(2024, 1, 1, 12, 0, 0).to_i # 60 min later + feb_1_10am = Time.zone.local(2024, 2, 1, 10, 0, 0).to_i - create(:point, user: user, timestamp: jan_1, country_name: 'Germany', city: 'Berlin') - create(:point, user: user, timestamp: jan_2, country_name: 'Germany', city: 'Munich') - create(:point, user: user, timestamp: feb_1, country_name: 'France', city: 'Paris') + create(:point, user: user, timestamp: jan_1_10am, country_name: 'Germany', city: 'Berlin') + create(:point, user: user, timestamp: jan_1_11am, country_name: 'Germany', city: 'Berlin') + create(:point, user: user, timestamp: jan_1_12pm, country_name: 'Germany', city: 'Munich') + create(:point, user: user, timestamp: feb_1_10am, country_name: 'France', city: 'Paris') countries = calculate_digest.time_spent_by_location['countries'] cities = calculate_digest.time_spent_by_location['cities'] - # Countries: based on unique days (2 days in Germany, 1 day in France) + # Germany: 60 min (10am->11am) + 60 min (11am->12pm) = 120 minutes germany_country = countries.find { |c| c['name'] == 'Germany' } - expect(germany_country['minutes']).to eq(2 * 24 * 60) # 2 days = 2880 minutes + expect(germany_country['minutes']).to eq(120) + + # France: only 1 point, so 0 minutes (no consecutive pair) + france_country = countries.find { |c| c['name'] == 'France' } + expect(france_country).to be_nil # No time counted for single point # Cities: based on stayed_for from monthly stats (sum across months) expect(cities.first['name']).to eq('Berlin') @@ -103,35 +109,37 @@ RSpec.describe Users::Digests::CalculateYear do end context 'when user visits same country across multiple months' do - it 'does not double-count days' do - # Create a user who was in Germany for 10 days in March and 10 days in July - # If we summed the stayed_for values from cities, we might get inflated numbers - # The fix counts unique days to prevent exceeding 365 days per year + it 'calculates actual minutes from consecutive point pairs' do + # Create hourly points across multiple days in March and July mar_start = Time.zone.local(2024, 3, 1, 10, 0, 0).to_i jul_start = Time.zone.local(2024, 7, 1, 10, 0, 0).to_i - # Create 10 days of points in March - 10.times do |i| - timestamp = mar_start + (i * 24 * 60 * 60) - create(:point, user: user, timestamp: timestamp, country_name: 'Germany', city: 'Berlin') + # Create 3 days of hourly points in March (3 points per day = 2 gaps of 60 min each) + 3.times do |day| + 3.times do |hour| + timestamp = mar_start + (day * 24 * 60 * 60) + (hour * 60 * 60) + create(:point, user: user, timestamp: timestamp, country_name: 'Germany', city: 'Berlin') + end end - # Create 10 days of points in July - 10.times do |i| - timestamp = jul_start + (i * 24 * 60 * 60) - create(:point, user: user, timestamp: timestamp, country_name: 'Germany', city: 'Munich') + # Create 3 days of hourly points in July + 3.times do |day| + 3.times do |hour| + timestamp = jul_start + (day * 24 * 60 * 60) + (hour * 60 * 60) + create(:point, user: user, timestamp: timestamp, country_name: 'Germany', city: 'Munich') + end end # Create the monthly stats (simulating what would be created by the stats calculation) create(:stat, user: user, year: 2024, month: 3, distance: 10_000, toponyms: [ { 'country' => 'Germany', 'cities' => [ - { 'city' => 'Berlin', 'stayed_for' => 14_400 } # 10 days in minutes + { 'city' => 'Berlin', 'stayed_for' => 14_400 } ] } ]) create(:stat, user: user, year: 2024, month: 7, distance: 15_000, toponyms: [ { 'country' => 'Germany', 'cities' => [ - { 'city' => 'Munich', 'stayed_for' => 14_400 } # 10 days in minutes + { 'city' => 'Munich', 'stayed_for' => 14_400 } ] } ]) @@ -139,13 +147,85 @@ RSpec.describe Users::Digests::CalculateYear do countries = digest.time_spent_by_location['countries'] germany = countries.find { |c| c['name'] == 'Germany' } - # Should be 20 days total (10 unique days in Mar + 10 unique days in Jul) - expected_minutes = 20 * 24 * 60 # 28,800 minutes - expect(germany['minutes']).to eq(expected_minutes) + # Each day: 2 gaps of 60 minutes = 120 minutes + # 6 days total (3 in March + 3 in July) = 720 minutes + # But gaps between days are > 60 min threshold, so not counted + expect(germany['minutes']).to eq(6 * 2 * 60) - # Verify this is less than 365 days (the bug would cause inflated numbers) - total_days = germany['minutes'] / (24 * 60) - expect(total_days).to be <= 365 + # Total should be much less than 365 days + total_hours = germany['minutes'] / 60.0 + expect(total_hours).to eq(12) # 12 hours of tracked time + end + end + + context 'when there are large gaps between points' do + it 'does not count time during gaps exceeding 60 minute threshold' do + point_1 = Time.zone.local(2024, 1, 1, 10, 0, 0).to_i + point_2 = Time.zone.local(2024, 1, 1, 12, 0, 0).to_i # 2 hours later (> 1 hour threshold) + point_3 = Time.zone.local(2024, 1, 1, 13, 0, 0).to_i # 1 hour after point_2 + + create(:point, user: user, timestamp: point_1, country_name: 'Germany') + create(:point, user: user, timestamp: point_2, country_name: 'Germany') + create(:point, user: user, timestamp: point_3, country_name: 'Germany') + + digest = calculate_digest + germany = digest.time_spent_by_location['countries'].find { |c| c['name'] == 'Germany' } + + # Only point_2 -> point_3 gap (60 min) should be counted + # point_1 -> point_2 gap (120 min) exceeds threshold + expect(germany['minutes']).to eq(60) + end + end + + context 'when transitioning between countries' do + it 'does not count transition time' do + point_1 = Time.zone.local(2024, 1, 1, 10, 0, 0).to_i + point_2 = Time.zone.local(2024, 1, 1, 10, 30, 0).to_i # In Germany + point_3 = Time.zone.local(2024, 1, 1, 11, 0, 0).to_i # Now in France + point_4 = Time.zone.local(2024, 1, 1, 11, 30, 0).to_i # Still in France + + create(:point, user: user, timestamp: point_1, country_name: 'Germany') + create(:point, user: user, timestamp: point_2, country_name: 'Germany') + create(:point, user: user, timestamp: point_3, country_name: 'France') + create(:point, user: user, timestamp: point_4, country_name: 'France') + + digest = calculate_digest + countries = digest.time_spent_by_location['countries'] + + germany = countries.find { |c| c['name'] == 'Germany' } + france = countries.find { |c| c['name'] == 'France' } + + expect(germany['minutes']).to eq(30) # point_1 -> point_2 + expect(france['minutes']).to eq(30) # point_3 -> point_4 + # Transition time (point_2 -> point_3) is NOT counted + end + end + + context 'when visiting multiple countries on same day' do + it 'does not exceed the actual time in the day' do + # This tests the fix for the original bug: border crossing should not count double + jan_1_8am = Time.zone.local(2024, 1, 1, 8, 0, 0).to_i + jan_1_9am = Time.zone.local(2024, 1, 1, 9, 0, 0).to_i + jan_1_10am = Time.zone.local(2024, 1, 1, 10, 0, 0).to_i # Border crossing + jan_1_11am = Time.zone.local(2024, 1, 1, 11, 0, 0).to_i + + create(:point, user: user, timestamp: jan_1_8am, country_name: 'France') + create(:point, user: user, timestamp: jan_1_9am, country_name: 'France') + create(:point, user: user, timestamp: jan_1_10am, country_name: 'Germany') + create(:point, user: user, timestamp: jan_1_11am, country_name: 'Germany') + + digest = calculate_digest + countries = digest.time_spent_by_location['countries'] + + france = countries.find { |c| c['name'] == 'France' } + germany = countries.find { |c| c['name'] == 'Germany' } + + # France: 60 min (8am->9am) + # Germany: 60 min (10am->11am) + # Total: 120 min (2 hours) - NOT 2 days (2880 min) as the bug would have caused + expect(france['minutes']).to eq(60) + expect(germany['minutes']).to eq(60) + expect(france['minutes'] + germany['minutes']).to eq(120) end end