Merge pull request #474 from Freika/fix/reverse-geocoded-points

Introduce reverse_geocoded_at to points

CHANGELOG.md

@@ -39,6 +39,12 @@ The `GET /api/v1/trips/:id/photos` endpoint now returns a different structure of
 - A `orientation` field in the Api::PhotoSerializer, hence the `GET /api/v1/photos` endpoint now includes the orientation of the photo. Valid values are `portrait` and `landscape`.
 - Examples for the `type`, `orientation` and `source` fields in the `GET /api/v1/photos` endpoint in the Swagger UI.
 - `DISABLE_TELEMETRY` env var to disable telemetry. More on telemetry: https://dawarich.app/docs/tutorials/telemetry
+- `reverse_geocoded_at` column added to the `points` table.
+
+### Changed
+
+- On the Stats page, the "Reverse geocoding" section is now showing the number of points that were reverse geocoded based on `reverse_geocoded_at` column, value of which is based on the time when the point was reverse geocoded. If no geodata for the point is available, `reverse_geocoded_at` will be set anyway. Number of points that were reverse geocoded but no geodata is available for them is shown below the "Reverse geocoded" number.
+
 
 # 0.19.3 - 2024-12-06
 

app/controllers/stats_controller.rb

@@ -5,6 +5,9 @@ class StatsController < ApplicationController
 
   def index
     @stats = current_user.stats.group_by(&:year).sort.reverse
+    @points_total = current_user.tracked_points.count
+    @points_reverse_geocoded = current_user.total_reverse_geocoded_points
+    @points_reverse_geocoded_without_data = current_user.total_reverse_geocoded_points_without_data
   end
 
   def show

app/controllers/visits_controller.rb

@@ -11,6 +11,7 @@ class VisitsController < ApplicationController
     visits = current_user
              .visits
              .where(status:)
+             .includes(%i[suggested_places area])
              .order(started_at: order_by)
              .group_by { |visit| visit.started_at.to_date }
              .map { |k, v| { date: k, visits: v } }

app/models/point.rb

@@ -17,8 +17,8 @@ class Point < ApplicationRecord
   }, suffix: true
   enum :connection, { mobile: 0, wifi: 1, offline: 2, unknown: 4 }, suffix: true
 
-  scope :reverse_geocoded, -> { where.not(geodata: {}) }
-  scope :not_reverse_geocoded, -> { where(geodata: {}) }
+  scope :reverse_geocoded, -> { where.not(reverse_geocoded_at: nil) }
+  scope :not_reverse_geocoded, -> { where(reverse_geocoded_at: nil) }
   scope :visited, -> { where.not(visit_id: nil) }
   scope :not_visited, -> { where(visit_id: nil) }
 
@@ -39,6 +39,10 @@ class Point < ApplicationRecord
     ReverseGeocodingJob.perform_later(self.class.to_s, id)
   end
 
+  def reverse_geocoded?
+    reverse_geocoded_at.present?
+  end
+
   private
 
   def broadcast_coordinates

app/models/user.rb

@@ -50,8 +50,12 @@ class User < ApplicationRecord
     cities_visited.size
   end
 
-  def total_reverse_geocoded
-    tracked_points.select(:id).where.not(geodata: {}).count
+  def total_reverse_geocoded_points
+    tracked_points.where.not(reverse_geocoded_at: nil).count
+  end
+
+  def total_reverse_geocoded_points_without_data
+    tracked_points.where(geodata: {}).count
   end
 
   def immich_integration_configured?

app/services/reverse_geocoding/points/fetch_data.rb

@@ -10,17 +10,16 @@ class ReverseGeocoding::Points::FetchData
   end
 
   def call
-    return if reverse_geocoded?
+    return if point.reverse_geocoded?
 
     response = Geocoder.search([point.latitude, point.longitude]).first
     return if response.blank? || response.data['error'].present?
 
-    point.update!(city: response.city, country: response.country, geodata: response.data)
-  end
-
-  private
-
-  def reverse_geocoded?
-    point.geodata.present?
+    point.update!(
+      city: response.city,
+      country: response.country,
+      geodata: response.data,
+      reverse_geocoded_at: Time.current
+    )
   end
 end

app/views/stats/_reverse_geocoding_stats.html.erb

@@ -1,8 +1,13 @@
 <div class="stat text-center">
   <div class="stat-value text-secondary">
-    <%= number_with_delimiter current_user.total_reverse_geocoded %>
+    <%= number_with_delimiter @points_reverse_geocoded %>
   </div>
   <div class="stat-title">Reverse geocoded points</div>
+  <div class="stat-title">
+    <span class="tooltip underline decoration-dotted" data-tip="Points that were reverse geocoded but had no data">
+      <%= number_with_delimiter @points_reverse_geocoded_without_data %> points without data
+    </span>
+  </div>
 </div>
 
 <div class="stat text-center">

app/views/stats/index.html.erb

@@ -11,7 +11,7 @@
 
     <div class="stat text-center">
       <div class="stat-value text-success">
-        <%= number_with_delimiter current_user.tracked_points.count %>
+        <%= number_with_delimiter @points_total %>
       </div>
       <div class="stat-title">Geopoints tracked</div>
     </div>

config/environments/test.rb

@@ -1,4 +1,4 @@
-require "active_support/core_ext/integer/time"
+require 'active_support/core_ext/integer/time'
 
 # The test environment is used exclusively to run your application's
 # test suite. You never need to work with it otherwise. Remember that
@@ -15,12 +15,12 @@ Rails.application.configure do
   # this is usually not necessary, and can slow down your test suite. However, it's
   # recommended that you enable it in continuous integration systems to ensure eager
   # loading is working properly before deploying your code.
-  config.eager_load = ENV["CI"].present?
+  config.eager_load = ENV['CI'].present?
 
   # Configure public file server for tests with Cache-Control for performance.
   config.public_file_server.enabled = true
   config.public_file_server.headers = {
-    "Cache-Control" => "public, max-age=#{1.hour.to_i}"
+    'Cache-Control' => "public, max-age=#{1.hour.to_i}"
   }
 
   # Show full error reports and disable caching.

db/data/20241202125248_set_reverse_geocoded_at_for_points.rb

@@ -0,0 +1,13 @@
+# frozen_string_literal: true
+
+class SetReverseGeocodedAtForPoints < ActiveRecord::Migration[7.2]
+  def up
+    # rubocop:disable Rails/SkipsModelValidations
+    Point.where.not(geodata: {}).update_all(reverse_geocoded_at: Time.current)
+    # rubocop:enable Rails/SkipsModelValidations
+  end
+
+  def down
+    raise ActiveRecord::IrreversibleMigration
+  end
+end

db/migrate/20241202114820_add_reverse_geocoded_at_to_points.rb

@@ -0,0 +1,11 @@
+# frozen_string_literal: true
+
+class AddReverseGeocodedAtToPoints < ActiveRecord::Migration[7.2]
+  disable_ddl_transaction!
+
+  def change
+    add_column :points, :reverse_geocoded_at, :datetime
+
+    add_index :points, :reverse_geocoded_at, algorithm: :concurrently
+  end
+end

spec/factories/points.rb

@@ -54,11 +54,12 @@ FactoryBot.define do
           }
         }
       end
+    end
 
-      trait :reverse_geocoded do
-        country { FFaker::Address.country }
-        city { FFaker::Address.city }
-      end
+    trait :reverse_geocoded do
+      country { FFaker::Address.country }
+      city { FFaker::Address.city }
+      reverse_geocoded_at { Time.current }
     end
   end
 end

spec/models/point_spec.rb

@@ -16,7 +16,7 @@ RSpec.describe Point, type: :model do
 
   describe 'scopes' do
     describe '.reverse_geocoded' do
-      let(:point) { create(:point, :with_geodata) }
+      let(:point) { create(:point, :reverse_geocoded) }
       let(:point_without_address) { create(:point, city: nil, country: nil) }
 
       it 'returns points with reverse geocoded address' do

spec/models/user_spec.rb

@@ -92,19 +92,28 @@ RSpec.describe User, type: :model do
       end
     end
 
-    describe '#total_reverse_geocoded' do
-      subject { user.total_reverse_geocoded }
+    describe '#total_reverse_geocoded_points' do
+      subject { user.total_reverse_geocoded_points }
 
-      let!(:reverse_geocoded_point) do
-        create(:point, country: 'Country', city: 'City', geodata: { some: 'data' }, user:)
-      end
-      let!(:not_reverse_geocoded_point) { create(:point, country: 'Country', city: 'City', user:) }
+      let!(:reverse_geocoded_point) { create(:point, :reverse_geocoded, user:) }
+      let!(:not_reverse_geocoded_point) { create(:point, user:, reverse_geocoded_at: nil) }
 
       it 'returns number of reverse geocoded points' do
         expect(subject).to eq(1)
       end
     end
 
+    describe '#total_reverse_geocoded_points_without_data' do
+      subject { user.total_reverse_geocoded_points_without_data }
+
+      let!(:reverse_geocoded_point) { create(:point, :reverse_geocoded, :with_geodata, user:) }
+      let!(:reverse_geocoded_point_without_data) { create(:point, :reverse_geocoded, user:, geodata: {}) }
+
+      it 'returns number of reverse geocoded points without data' do
+        expect(subject).to eq(1)
+      end
+    end
+
     describe '#years_tracked' do
       let!(:points) { create_list(:point, 3, user:, timestamp: DateTime.new(2024, 1, 1, 5, 0, 0)) }
 

spec/requests/api/v1/stats_spec.rb

@@ -9,7 +9,9 @@ RSpec.describe 'Api::V1::Stats', type: :request do
     let!(:user) { create(:user) }
     let!(:stats_in_2020) { create_list(:stat, 12, year: 2020, user:) }
     let!(:stats_in_2021) { create_list(:stat, 12, year: 2021, user:) }
-    let!(:points_in_2020) { create_list(:point, 85, :with_geodata, timestamp: Time.zone.local(2020), user:) }
+    let!(:points_in_2020) do
+      create_list(:point, 85, :with_geodata, :reverse_geocoded, timestamp: Time.zone.local(2020), user:)
+    end
     let!(:points_in_2021) { create_list(:point, 95, timestamp: Time.zone.local(2021), user:) }
     let(:expected_json) do
       {

spec/serializers/stats_serializer_spec.rb

@@ -28,8 +28,12 @@ RSpec.describe StatsSerializer do
     context 'when the user has stats' do
       let!(:stats_in_2020) { create_list(:stat, 12, year: 2020, user:) }
       let!(:stats_in_2021) { create_list(:stat, 12, year: 2021, user:) }
-      let!(:points_in_2020) { create_list(:point, 85, :with_geodata, timestamp: Time.zone.local(2020), user:) }
-      let!(:points_in_2021) { create_list(:point, 95, timestamp: Time.zone.local(2021), user:) }
+      let!(:points_in_2020) do
+        create_list(:point, 85, :with_geodata, :reverse_geocoded, timestamp: Time.zone.local(2020), user:)
+      end
+      let!(:points_in_2021) do
+        create_list(:point, 95, timestamp: Time.zone.local(2021), user:)
+      end
       let(:expected_json) do
         {
           "totalDistanceKm": stats_in_2020.map(&:distance).sum + stats_in_2021.map(&:distance).sum,

spec/services/exports/create_spec.rb

@@ -13,8 +13,13 @@ RSpec.describe Exports::Create do
     let(:export_name)     { "#{start_at.to_date}_#{end_at.to_date}" }
     let(:export)          { create(:export, user:, name: export_name, status: :created) }
     let(:export_content)  { Points::GeojsonSerializer.new(points).call }
+    let(:reverse_geocoded_at) { Time.zone.local(2021, 1, 1) }
     let!(:points) do
-      create_list(:point, 10, :with_known_location, user:, timestamp: start_at.to_datetime.to_i)
+      create_list(:point, 10, :with_known_location, user:, timestamp: start_at.to_datetime.to_i, reverse_geocoded_at:)
+    end
+
+    before do
+      allow_any_instance_of(Point).to receive(:reverse_geocoded_at).and_return(reverse_geocoded_at)
     end
 
     it 'writes the data to a file' do

spec/services/reverse_geocoding/points/fetch_data_spec.rb

@@ -32,7 +32,7 @@ RSpec.describe ReverseGeocoding::Points::FetchData do
     end
 
     context 'when point has city and country' do
-      let(:point) { create(:point, :with_geodata) }
+      let(:point) { create(:point, :with_geodata, :reverse_geocoded) }
 
       before do
         allow(Geocoder).to receive(:search).and_return(