Rework calculation of cities visited

app/controllers/export_controller.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 class ExportController < ApplicationController
   before_action :authenticate_user!
 
@@ -6,12 +8,17 @@ class ExportController < ApplicationController
   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 @@ class Stat < ApplicationRecord
 
     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,23 +27,21 @@ class CountriesAndCities
       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] < CITY_VISIT_THRESHOLD }
     end
   end
 
@@ -51,7 +49,9 @@ class CountriesAndCities
     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 @@ class CreateStats
           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" 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" 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/initializers/00_constants.rb

@@ -1,5 +1,6 @@
 # frozen_string_literal: true
 
 MINIMUM_POINTS_IN_CITY = ENV.fetch('MINIMUM_POINTS_IN_CITY', 5).to_i
+CITY_VISIT_THRESHOLD = ENV.fetch('MINIMUM_POINTS_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

spec/models/stat_spec.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'rails_helper'
 
 RSpec.describe Stat, type: :model do
@@ -13,19 +15,31 @@ RSpec.describe Stat, type: :model do
     describe '.year_cities_and_countries' do
       subject { described_class.year_cities_and_countries(year) }
 
+      let(:timestamp) { DateTime.new(year, 1, 1, 0, 0, 0) }
+
       before do
-        stub_const('MINIMUM_POINTS_IN_CITY', 1)
+        stub_const('CITY_VISIT_THRESHOLD', 60)
       end
 
       context 'when there are points' do
         let!(:points) do
-          create_list(:point, 3, city: 'City', country: 'Country', timestamp: DateTime.new(year, 1))
-          create_list(:point, 2, city: 'Some City', country: 'Another country', timestamp: DateTime.new(year, 2))
+          [
+            create(:point, city: 'Berlin', country: 'Germany', timestamp:),
+            create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 10.minutes),
+            create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 20.minutes),
+            create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 30.minutes),
+            create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 40.minutes),
+            create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 50.minutes),
+            create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 60.minutes),
+            create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 70.minutes),
+            create(:point, city: 'Brugges', country: 'Belgium', timestamp: timestamp + 80.minutes),
+            create(:point, city: 'Brugges', country: 'Belgium', timestamp: timestamp + 90.minutes)
+          ]
         end
 
-
         it 'returns countries and cities' do
-          expect(subject).to eq(countries: 2, cities: 2)
+          # User spent only 20 minutes in Brugges, so it should not be included
+          expect(subject).to eq(countries: 2, cities: 1)
         end
       end
 
@@ -50,8 +64,8 @@ RSpec.describe Stat, type: :model do
         let(:expected_years) { (year..Time.current.year).to_a.reverse }
 
         before do
-          create(:stat, year: 2021, user: user)
-          create(:stat, year: 2020, user: user)
+          create(:stat, year: 2021, user:)
+          create(:stat, year: 2020, user:)
         end
 
         it 'returns years' do
@@ -64,7 +78,7 @@ RSpec.describe Stat, type: :model do
       subject { stat.distance_by_day }
 
       let(:user) { create(:user) }
-      let(:stat) { create(:stat, year: year, month: 1, user: user) }
+      let(:stat) { create(:stat, year:, month: 1, user:) }
       let(:expected_distance) do
         # 31 day of January
         (1..31).map { |day| [day, 0] }
@@ -93,7 +107,7 @@ RSpec.describe Stat, type: :model do
     describe '#timespan' do
       subject { stat.send(:timespan) }
 
-      let(:stat) { build(:stat, year: year, month: 1) }
+      let(:stat) { build(:stat, year:, month: 1) }
       let(:expected_timespan) { DateTime.new(year, 1).beginning_of_month..DateTime.new(year, 1).end_of_month }
 
       it 'returns timespan' do
@@ -111,8 +125,8 @@ RSpec.describe Stat, type: :model do
 
       context 'when there are stats' do
         let!(:stats) do
-          create(:stat, year: year, month: 1, distance: 100, user: user)
-          create(:stat, year: year, month: 2, distance: 200, user: user)
+          create(:stat, year:, month: 1, distance: 100, user:)
+          create(:stat, year:, month: 2, distance: 200, user:)
         end
 
         before do

spec/models/user_spec.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'rails_helper'
 
 RSpec.describe User, type: :model do
@@ -23,8 +25,8 @@ RSpec.describe User, type: :model do
     xdescribe '#export_data' do
       subject { user.export_data }
 
-      let(:import) { create(:import, user: user) }
-      let(:point) { create(:point, import: import) }
+      let(:import) { create(:import, user:) }
+      let(:point) { create(:point, import:) }
 
       it 'returns json' do
         expect(subject).to include(user.email)
@@ -33,11 +35,33 @@ RSpec.describe User, type: :model do
       end
     end
 
+    describe '#countries_visited' do
+      subject { user.countries_visited }
+
+      let!(:stat1) { create(:stat, user:, toponyms: [{ 'country' => 'Germany' }]) }
+      let!(:stat2) { create(:stat, user:, toponyms: [{ 'country' => 'France' }]) }
+
+      it 'returns array of countries' do
+        expect(subject).to eq(%w[Germany France])
+      end
+    end
+
+    describe '#cities_visited' do
+      subject { user.cities_visited }
+
+      let!(:stat1) { create(:stat, user:, toponyms: [{ 'cities' => [{ 'city' => 'Berlin' }] }]) }
+      let!(:stat2) { create(:stat, user:, toponyms: [{ 'cities' => [{ 'city' => 'Paris' }] }]) }
+
+      it 'returns array of cities' do
+        expect(subject).to eq(%w[Berlin Paris])
+      end
+    end
+
     describe '#total_km' do
       subject { user.total_km }
 
-      let!(:stat_1) { create(:stat, user: user, distance: 10) }
-      let!(:stat_2) { create(:stat, user: user, distance: 20) }
+      let!(:stat1) { create(:stat, user:, distance: 10) }
+      let!(:stat2) { create(:stat, user:, distance: 20) }
 
       it 'returns sum of distances' do
         expect(subject).to eq(30)
@@ -47,7 +71,7 @@ RSpec.describe User, type: :model do
     describe '#total_countries' do
       subject { user.total_countries }
 
-      let!(:stat) { create(:stat, user: user, toponyms: [{ 'country' => 'Country' }]) }
+      let!(:stat) { create(:stat, user:, toponyms: [{ 'country' => 'Country' }]) }
 
       it 'returns number of countries' do
         expect(subject).to eq(1)
@@ -57,7 +81,16 @@ RSpec.describe User, type: :model do
     describe '#total_cities' do
       subject { user.total_cities }
 
-      let!(:stat) { create(:stat, user: user, toponyms: [{ 'city' => 'City' }]) }
+      let!(:stat) do
+        create(
+          :stat,
+          user:,
+          toponyms: [
+            { 'cities' => [], 'country' => nil },
+            { 'cities' => [{ 'city' => 'Berlin', 'points' => 64, 'timestamp' => 1710446806, 'stayed_for' => 8772 }], 'country' => 'Germany' }
+          ]
+        )
+      end
 
       it 'returns number of cities' do
         expect(subject).to eq(1)
@@ -67,8 +100,8 @@ RSpec.describe User, type: :model do
     describe '#total_reverse_geocoded' do
       subject { user.total_reverse_geocoded }
 
-      let(:import) { create(:import, user: user) }
-      let!(:point) { create(:point, country: 'Country', city: 'City', import: import) }
+      let(:import) { create(:import, user:) }
+      let!(:point) { create(:point, country: 'Country', city: 'City', import:) }
 
       it 'returns number of reverse geocoded points' do
         expect(subject).to eq(1)

spec/requests/imports_spec.rb

@@ -1,3 +1,5 @@
+# frozen_string_literal: true
+
 require 'rails_helper'
 
 RSpec.describe 'Imports', type: :request do
@@ -21,7 +23,7 @@ RSpec.describe 'Imports', type: :request do
       end
 
       context 'when user has imports' do
-        let!(:import) { create(:import, user: user) }
+        let!(:import) { create(:import, user:) }
 
         it 'displays imports' do
           get imports_path
@@ -40,15 +42,15 @@ RSpec.describe 'Imports', type: :request do
       before { sign_in user }
 
       it 'queues import job' do
-        expect {
+        expect do
           post imports_path, params: { import: { source: 'owntracks', files: [file] } }
-        }.to have_enqueued_job(ImportJob).on_queue('default').at_least(1).times
+        end.to have_enqueued_job(ImportJob).on_queue('imports').at_least(1).times
       end
 
       it 'creates a new import' do
-        expect {
+        expect do
           post imports_path, params: { import: { source: 'owntracks', files: [file] } }
-        }.to change(user.imports, :count).by(1)
+        end.to change(user.imports, :count).by(1)
 
         expect(response).to redirect_to(imports_path)
       end

spec/services/countries_and_cities_spec.rb

@@ -1,46 +1,76 @@
+# frozen_string_literal: true
+
 require 'rails_helper'
 
 RSpec.describe CountriesAndCities do
   describe '#call' do
     subject(:countries_and_cities) { described_class.new(points).call }
 
+    # we have 5 points in the same city and country within 1 hour,
+    # 5 points in the differnt city within 10 minutes
+    # and we expect to get one country with one city which has 5 points
+
+    let(:timestamp) { DateTime.new(2021, 1, 1, 0, 0, 0) }
+
     let(:points) do
       [
-        create(:point, latitude: 0, longitude: 0, city: 'City', country: 'Country'),
-        create(:point, latitude: 1, longitude: 1, city: 'City', country: 'Country'),
-        create(:point, latitude: 2, longitude: 2, city: 'City', country: 'Country'),
-        create(:point, latitude: 2, longitude: 2, city: 'Another city', country: 'Some Country'),
-        create(:point, latitude: 2, longitude: 6, city: 'Another city', country: 'Some Country')
+        create(:point, city: 'Berlin', country: 'Germany', timestamp:),
+        create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 10.minutes),
+        create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 20.minutes),
+        create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 30.minutes),
+        create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 40.minutes),
+        create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 50.minutes),
+        create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 60.minutes),
+        create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 70.minutes),
+        create(:point, city: 'Brugges', country: 'Belgium', timestamp: timestamp + 80.minutes),
+        create(:point, city: 'Brugges', country: 'Belgium', timestamp: timestamp + 90.minutes)
       ]
     end
 
-    context 'when MINIMUM_POINTS_IN_CITY is 1' do
+    context 'when CITY_VISIT_THRESHOLD is 60 (in minutes)' do
       before do
-        stub_const('CountriesAndCities::MINIMUM_POINTS_IN_CITY', 1)
+        stub_const('CITY_VISIT_THRESHOLD', 60)
       end
 
-      it 'returns countries and cities' do
-        expect(countries_and_cities).to eq(
+      context 'when user stayed in the city for more than 1 hour' do
+        it 'returns countries and cities' do
+          expect(countries_and_cities).to eq(
+            [
+              {
+                cities: [{ city: 'Berlin', points: 8, timestamp: 1609463400, stayed_for: 70 }],
+                country: 'Germany'
+              },
+              {
+                cities: [], country: 'Belgium'
+              }
+            ]
+          )
+        end
+      end
+
+      context 'when user stayed in the city for less than 1 hour' do
+        let(:points) do
           [
-            { cities: [{city: "City", points: 3, timestamp: 1}], country: "Country" },
-            { cities: [{city: "Another city", points: 2, timestamp: 1}], country: "Some Country" }
+            create(:point, city: 'Berlin', country: 'Germany', timestamp:),
+            create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 10.minutes),
+            create(:point, city: 'Berlin', country: 'Germany', timestamp: timestamp + 20.minutes),
+            create(:point, city: 'Brugges', country: 'Belgium', timestamp: timestamp + 80.minutes),
+            create(:point, city: 'Brugges', country: 'Belgium', timestamp: timestamp + 90.minutes)
           ]
-        )
-      end
-    end
+        end
 
-    context 'when MINIMUM_POINTS_IN_CITY is 3' do
-      before do
-        stub_const('CountriesAndCities::MINIMUM_POINTS_IN_CITY', 3)
-      end
-
-      it 'returns countries and cities' do
-        expect(countries_and_cities).to eq(
-          [
-            { cities: [{city: "City", points: 3, timestamp: 1}], country: "Country" },
-            { cities: [], country: "Some Country" }
-          ]
-        )
+        it 'returns countries and cities' do
+          expect(countries_and_cities).to eq(
+            [
+              {
+                cities: [], country: 'Germany'
+              },
+              {
+                cities: [], country: 'Belgium'
+              }
+            ]
+          )
+        end
       end
     end
   end