Merge pull request #22 from Freika/fix/city-detection

Rework calculation of cities visited

CHANGELOG.md

@@ -5,6 +5,16 @@ All notable changes to this project will be documented in this file.
 The format is based on [Keep a Changelog](http://keepachangelog.com/)
 and this project adheres to [Semantic Versioning](http://semver.org/).
 
+## [0.2.0] — 2024-05-05
+
+*Breaking changes:*
+
+This release changes how Dawarich handles a city visit threshold. Previously, the `MINIMUM_POINTS_IN_CITY` environment variable was used to determine the minimum *number of points* in a city to consider it as visited. Now, the `MIN_MINUTES_SPENT_IN_CITY` environment variable is used to determine the minimum *minutes* between two points to consider them as visited the same city.
+
+The logic behind this is the following: if you have a lot of points in a city, it doesn't mean you've spent a lot of time there, especially if your OwnTracks app was in "Move" mode. So, it's better to consider the time spent in a city rather than the number of points.
+
+In your docker-compose.yml file, you need to replace the `MINIMUM_POINTS_IN_CITY` environment variable with `MIN_MINUTES_SPENT_IN_CITY`. The default value is `60`, in minutes.
+
 ## [0.1.9] — 2024-04-25
 
 ### Added

README.md

@@ -41,7 +41,7 @@ Copy the contents of the `docker-compose.yml` file to your server and run `docke
 ## Environment variables
 
 ```
-MINIMUM_POINTS_IN_CITY — minimum number of points in a city to consider it as a city visited, eg. `10`
+MIN_MINUTES_SPENT_IN_CITY — minimum minutes between two points to consider them as visited the same city, e.g. `60`
 MAP_CENTER — default map center, e.g. `55.7558,37.6176`
 TIME_ZONE — time zone, e.g. `Europe/Berlin`
 APPLICATION_HOST — host of the application, e.g. `localhost` or `dawarich.example.com`

app/controllers/export_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class ExportController < ApplicationController
   before_action :authenticate_user!
 
@@ -6,12 +8,17 @@ def index
   end
 
   def download
-    first_point_datetime = Time.at(current_user.points.first.timestamp).to_s
-    last_point_datetime = Time.at(current_user.points.last.timestamp).to_s
-    filename = "dawarich-export-#{first_point_datetime}-#{last_point_datetime}.json".gsub(' ', '_')
-
     export = current_user.export_data
 
-    send_data export, filename: filename
+    send_data export, filename:
+  end
+
+  private
+
+  def filename
+    first_point_datetime = Time.zone.at(current_user.points.first.timestamp).to_s
+    last_point_datetime = Time.zone.at(current_user.points.last.timestamp).to_s
+
+    "dawarich-export-#{first_point_datetime}-#{last_point_datetime}.json".gsub(' ', '_')
   end
 end

app/jobs/import_job.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class ImportJob < ApplicationJob
-  queue_as :default
+  queue_as :imports
 
   def perform(user_id, import_id)
     user = User.find(user_id)

app/jobs/reverse_geocoding_job.rb

@@ -1,5 +1,7 @@
+# frozen_string_literal: true
+
 class ReverseGeocodingJob < ApplicationJob
-  queue_as :low
+  queue_as :reverse_geocoding
 
   def perform(point_id)
     return unless REVERSE_GEOCODING_ENABLED

app/jobs/stat_creating_job.rb

@@ -1,7 +1,7 @@
 # frozen_string_literal: true
 
 class StatCreatingJob < ApplicationJob
-  queue_as :default
+  queue_as :stats
 
   def perform(user_ids = nil)
     CreateStats.new(user_ids).call

app/models/stat.rb

@@ -45,7 +45,10 @@ def self.year_cities_and_countries(year)
 
     data = CountriesAndCities.new(points).call
 
-    { countries: data.map { _1[:country] }.uniq.count, cities: data.sum { |country| country[:cities].count } }
+    {
+      countries: data.map { _1[:country] }.uniq.count,
+      cities: data.sum { |country| country[:cities].count }
+    }
   end
 
   def self.years

app/models/user.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class User < ApplicationRecord
   # Include default devise modules. Others available are:
   # :confirmable, :lockable, :timeoutable, :trackable and :omniauthable
@@ -6,24 +8,41 @@ class User < ApplicationRecord
 
   has_many :imports, dependent: :destroy
   has_many :points, through: :imports
-  has_many :stats
+  has_many :stats, dependent: :destroy
 
   after_create :create_api_key
 
   def export_data
-    ::ExportSerializer.new(points, self.email).call
+    ::ExportSerializer.new(points, email).call
+  end
+
+  def countries_visited
+    stats.pluck(:toponyms).flatten.map { _1['country'] }.uniq.compact
+  end
+
+  def cities_visited
+    stats
+      .where.not(toponyms: nil)
+      .pluck(:toponyms)
+      .flatten
+      .reject { |toponym| toponym['cities'].blank? }
+      .pluck('cities')
+      .flatten
+      .pluck('city')
+      .uniq
+      .compact
   end
 
   def total_km
-    Stat.where(user: self).sum(:distance)
+    stats.sum(:distance)
   end
 
   def total_countries
-    Stat.where(user: self).pluck(:toponyms).flatten.map { _1['country'] }.uniq.size
+    countries_visited.size
   end
 
   def total_cities
-    Stat.where(user: self).pluck(:toponyms).flatten.size
+    cities_visited.size
   end
 
   def total_reverse_geocoded

app/services/countries_and_cities.rb

@@ -27,31 +27,31 @@ def map_with_cities(grouped_records)
       grouped_points
         .pluck(:city, :timestamp) # Extract city and timestamp
         .delete_if { _1.first.nil? } # Remove records without city
-        .group_by { |city, _| city }
+        .group_by { |city, _| city } # Group by city
         .transform_values do |cities|
           {
             points: cities.count,
-            timestamp: cities.map(&:last).max # Get the maximum timestamp
+            last_timestamp: cities.map(&:last).max, # Get the maximum timestamp
+            stayed_for: ((cities.map(&:last).max - cities.map(&:last).min).to_i / 60) # Calculate the time stayed in minutes
           }
         end
     end
   end
 
   def filter_cities(mapped_with_cities)
-    # In future, we would want to remove cities where user spent less than
-    # 1 hour per day
-
-    # Remove cities with less than MINIMUM_POINTS_IN_CITY
+    # Remove cities where user stayed for less than 1 hour
     mapped_with_cities.transform_values do |cities|
-      cities.reject { |_, data| data[:points] < MINIMUM_POINTS_IN_CITY }
+      cities.reject { |_, data| data[:stayed_for] < MIN_MINUTES_SPENT_IN_CITY }
     end
   end
 
   def normalize_result(hash)
     hash.map do |country, cities|
       {
         country:,
-        cities: cities.map { |city, data| { city:, points: data[:points], timestamp: data[:timestamp] } }
+        cities: cities.map do |city, data|
+          { city:, points: data[:points], timestamp: data[:last_timestamp], stayed_for: data[:stayed_for]}
+        end
       }
     end
   end

app/services/create_stats.rb

@@ -19,13 +19,11 @@ def call
           points = points(beginning_of_month_timestamp, end_of_month_timestamp)
           next if points.empty?
 
-          stat = Stat.find_or_initialize_by(year: year, month: month, user: user)
+          stat = Stat.find_or_initialize_by(year:, month:, user:)
           stat.distance = distance(points)
           stat.toponyms = toponyms(points)
           stat.daily_distance = stat.distance_by_day
           stat.save
-
-          stat
         end
       end
     end

app/views/export/index.html.erb

@@ -3,7 +3,4 @@
     <h1 class='text-3xl font-bold'>Export Data</h1>
     <%= link_to 'Download JSON', export_download_path, class: 'btn btn-primary my-5' %>
   </div>
-  <div class="mockup-code p-5">
-    <code><%= current_user.export_data %></code>
-  </div>
 </div>

app/views/shared/_right_sidebar.html.erb

@@ -32,12 +32,16 @@
 <% if REVERSE_GEOCODING_ENABLED && @countries_and_cities&.any? %>
   <hr class='my-5'>
   <% @countries_and_cities.each do |country| %>
+    <% next if country[:cities].empty? %>
+
     <h2 class="text-lg font-semibold mt-5">
       <%= country[:country] %> (<%= country[:cities].count %> cities)
     </h2>
     <ul>
       <% country[:cities].each do |city| %>
-        <li><%= city[:city] %> (<%= Time.zone.at(city[:timestamp]).strftime("%d.%m.%Y") %>)</li>
+        <li>
+          <%= city[:city] %> (<%= Time.zone.at(city[:timestamp]).strftime("%d.%m.%Y") %>)
+        </li>
       <% end %>
     </ul>
   <% end %>

app/views/stats/index.html.erb

@@ -23,17 +23,44 @@
       </div>
 
       <div class="stat text-center">
-        <div class="stat-value text-warning">
+        <div class="stat-value text-warning underline hover:no-underline  hover:cursor-pointer" onclick="countries_visited.showModal()">
           <%= number_with_delimiter current_user.total_countries %>
         </div>
         <div class="stat-title">Countries visited</div>
+
+        <dialog id="countries_visited" class="modal">
+          <div class="modal-box">
+            <h3 class="font-bold text-lg">Countries visited</h3>
+            <p class="py-4">
+              <% current_user.countries_visited.each do |country| %>
+                <p><%= country %></p>
+              <% end %>
+            </p>
+          </div>
+          <form method="dialog" class="modal-backdrop">
+            <button>close</button>
+          </form>
+        </dialog>
       </div>
 
       <div class="stat text-center">
-        <div class="stat-value">
+        <div class="stat-value hover:cursor-pointer hover:no-underline underline" onclick="cities_visited.showModal()">
           <%= current_user.total_cities %>
         </div>
         <div class="stat-title">Cities visited</div>
+                <dialog id="cities_visited" class="modal">
+          <div class="modal-box">
+            <h3 class="font-bold text-lg">Countries visited</h3>
+            <p class="py-4">
+              <% current_user.cities_visited.each do |city| %>
+                <p><%= city %></p>
+              <% end %>
+            </p>
+          </div>
+          <form method="dialog" class="modal-backdrop">
+            <button>close</button>
+          </form>
+        </dialog>
       </div>
     <% end %>
   </div>

config/application.rb

@@ -1,6 +1,8 @@
-require_relative "boot"
+# frozen_string_literal: true
 
-require "rails/all"
+require_relative 'boot'
+
+require 'rails/all'
 
 # Require the gems listed in Gemfile, including any gems
 # you've limited to :test, :development, or :production.
@@ -14,7 +16,7 @@ class Application < Rails::Application
     # Please, add to the `ignore` list any other `lib` subdirectories that do
     # not contain `.rb` files, or that should not be reloaded or eager loaded.
     # Common ones are `templates`, `generators`, or `middleware`, for example.
-    config.autoload_lib(ignore: %w(assets tasks))
+    config.autoload_lib(ignore: %w[assets tasks])
 
     # Configuration for the application, engines, and railties goes here.
     #

config/initializers/00_constants.rb

@@ -1,5 +1,5 @@
 # frozen_string_literal: true
 
-MINIMUM_POINTS_IN_CITY = ENV.fetch('MINIMUM_POINTS_IN_CITY', 5).to_i
+MIN_MINUTES_SPENT_IN_CITY = ENV.fetch('MIN_MINUTES_SPENT_IN_CITY', 60).to_i
 MAP_CENTER = ENV.fetch('MAP_CENTER', '[55.7522, 37.6156]')
 REVERSE_GEOCODING_ENABLED = ENV.fetch('REVERSE_GEOCODING_ENABLED', 'true') == 'true'

config/sidekiq.yml

@@ -1,4 +1,5 @@
 :queues:
-  - critical
   - default
-  - low
+  - imports
+  - stats
+  - reverse_geocoding

docker-compose.yml

@@ -21,7 +21,7 @@ services:
       POSTGRES_USER: postgres
       POSTGRES_PASSWORD: password
   dawarich_app:
-    image: freikin/dawarich:0.1.6.1
+    image: freikin/dawarich:latest
     container_name: dawarich_app
     volumes:
       - gem_cache:/usr/local/bundle/gems
@@ -40,13 +40,13 @@ services:
       DATABASE_USERNAME: postgres
       DATABASE_PASSWORD: password
       DATABASE_NAME: dawarich_development
-      MINIMUM_POINTS_IN_CITY: 5
+      MIN_MINUTES_SPENT_IN_CITY: 60
       APPLICATION_HOST: localhost
     depends_on:
       - dawarich_db
       - dawarich_redis
   dawarich_sidekiq:
-    image: freikin/dawarich:0.1.6.1
+    image: freikin/dawarich:latest
     container_name: dawarich_sidekiq
     volumes:
       - gem_cache:/usr/local/bundle/gems