diff --git a/CHANGELOG.md b/CHANGELOG.md index 6afdc81c..ec16e47e 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -9,7 +9,8 @@ TODO: - Specs for app/services/visits/merge_service.rb and rename it probably - Remove Stimulus controllers for visits on the Visits page - Revert changes to Visits page - +- Decide on how to suggest visits for the past +- Should visits be disabled for non-reverse-geocoded instances? # 0.25.0 - 2025-03-08 diff --git a/app/assets/stylesheets/application.css b/app/assets/stylesheets/application.css index d6a6ecf3..e7913676 100644 --- a/app/assets/stylesheets/application.css +++ b/app/assets/stylesheets/application.css @@ -14,17 +14,6 @@ *= require_self */ -/* Leaflet map container styles */ -[data-controller="visits-map"] { - height: 100%; - width: 100%; -} - -[data-visits-map-target="container"] { - height: 100%; - width: 100%; -} - /* Loading spinner animation */ @keyframes spinner { to { diff --git a/app/javascript/maps/visits.js b/app/javascript/maps/visits.js index 4256cdb3..5e3f4d8c 100644 --- a/app/javascript/maps/visits.js +++ b/app/javascript/maps/visits.js @@ -101,7 +101,7 @@ export class VisitsManager { const SelectionControl = L.Control.extend({ onAdd: (map) => { const button = L.DomUtil.create('button', 'leaflet-bar leaflet-control leaflet-control-custom'); - button.innerHTML = ''; + button.innerHTML = '📌'; button.title = 'Select Area'; button.id = 'selection-tool-button'; button.style.width = '48px'; diff --git a/app/jobs/data_migrations/migrate_places_lonlat_job.rb b/app/jobs/data_migrations/migrate_places_lonlat_job.rb index e4ba1e33..b71f0f55 100644 --- a/app/jobs/data_migrations/migrate_places_lonlat_job.rb +++ b/app/jobs/data_migrations/migrate_places_lonlat_job.rb @@ -6,8 +6,24 @@ class DataMigrations::MigratePlacesLonlatJob < ApplicationJob def perform(user_id) user = User.find(user_id) - # rubocop:disable Rails/SkipsModelValidations - user.places.update_all('lonlat = ST_SetSRID(ST_MakePoint(longitude, latitude), 4326)') - # rubocop:enable Rails/SkipsModelValidations + # Find all places with nil lonlat + places_to_update = user.places.where(lonlat: nil) + + # For each place, set the lonlat value based on longitude and latitude + places_to_update.find_each do |place| + next if place.longitude.nil? || place.latitude.nil? + + # Set the lonlat to a PostGIS point with the proper SRID + # rubocop:disable Rails/SkipsModelValidations + place.update_column(:lonlat, "SRID=4326;POINT(#{place.longitude} #{place.latitude})") + # rubocop:enable Rails/SkipsModelValidations + end + + # Double check if there are any remaining places without lonlat + remaining = user.places.where(lonlat: nil) + return unless remaining.exists? + + # Log an error for these places + Rails.logger.error("Places with ID #{remaining.pluck(:id).join(', ')} for user #{user.id} could not be updated with lonlat values") end end diff --git a/app/services/visits/smart_detect.rb b/app/services/visits/smart_detect.rb index 04864dee..64d66440 100644 --- a/app/services/visits/smart_detect.rb +++ b/app/services/visits/smart_detect.rb @@ -22,8 +22,8 @@ module Visits return [] if points.empty? potential_visits = Visits::Detector.new(points).detect_potential_visits - merged_visits = Visits::Merger.new(points).merge_visits(potential_visits) - grouped_visits = group_nearby_visits(merged_visits).flatten + merged_visits = Visits::Merger.new(points).merge_visits(potential_visits) + grouped_visits = group_nearby_visits(merged_visits).flatten Visits::Creator.new(user).create_visits(grouped_visits) end diff --git a/app/services/visits/time_chunks.rb b/app/services/visits/time_chunks.rb index d7fc2738..5c7470da 100644 --- a/app/services/visits/time_chunks.rb +++ b/app/services/visits/time_chunks.rb @@ -11,16 +11,8 @@ module Visits def call # If the start date is in the future or equal to the end date, # handle as a special case extending to the end of the start's year - if start_at >= end_at - year_end = start_at.end_of_year - return [start_at...year_end] - end - - # Special case for dates within the same year - if start_at.year == end_at.year - year_end = start_at.end_of_year - return [start_at...year_end] - end + # or if the start and end are in the same year, return the year chunk + return [start_at..start_at.end_of_year] if start_in_future? || same_year? # First chunk: from start_at to end of that year first_end = start_at.end_of_year @@ -43,5 +35,13 @@ module Visits private attr_reader :start_at, :end_at, :time_chunks + + def start_in_future? + start_at >= end_at + end + + def same_year? + start_at.year == end_at.year + end end end diff --git a/app/views/layouts/application.html.erb b/app/views/layouts/application.html.erb index 2cc1d82e..f41baeda 100644 --- a/app/views/layouts/application.html.erb +++ b/app/views/layouts/application.html.erb @@ -10,7 +10,6 @@ - <%= stylesheet_link_tag "tailwind", "inter-font", "data-turbo-track": "reload" %> <%= stylesheet_link_tag "application", "data-turbo-track": "reload" %> diff --git a/app/views/visits/_visit.html.erb b/app/views/visits/_visit.html.erb index be72f2ea..c5de1ce7 100644 --- a/app/views/visits/_visit.html.erb +++ b/app/views/visits/_visit.html.erb @@ -1,8 +1,4 @@ -
+
<%= render 'visits/name', visit: visit %> diff --git a/app/views/visits/index.html.erb b/app/views/visits/index.html.erb index 793649c9..3fd03b64 100644 --- a/app/views/visits/index.html.erb +++ b/app/views/visits/index.html.erb @@ -1,103 +1,89 @@ <% content_for :title, "Visits" %> -
- <%# Top navigation tabs %> -
-
- <%= link_to 'Visits', visits_path(status: :confirmed), role: 'tab', class: "tab font-bold text-xl #{active_visit_places_tab?('visits')}" %> - <%= link_to 'Places', places_path, role: 'tab', class: "tab font-bold text-xl #{active_visit_places_tab?('places')}" %> +
+
+ <%= link_to 'Visits', visits_path(status: :confirmed), role: 'tab', class: "tab font-bold text-xl #{active_visit_places_tab?('visits')}" %> + <%= link_to 'Places', places_path, role: 'tab', class: "tab font-bold text-xl #{active_visit_places_tab?('places')}" %> +
+ +
+
+ <%= link_to 'Confirmed', visits_path(status: :confirmed), role: 'tab', + class: "tab #{active_tab?(visits_path(status: :confirmed))}" %> + <%= link_to visits_path(status: :suggested), role: 'tab', + class: "tab #{active_tab?(visits_path(status: :suggested))}" do %> + Suggested + <% if @suggested_visits_count.positive? %> + <%= @suggested_visits_count %> + <% end %> + <% end %> + <%= link_to 'Declined', visits_path(status: :declined), role: 'tab', + class: "tab #{active_tab?(visits_path(status: :declined))}" %> +
+
+ Order by: + <%= link_to 'Newest', visits_path(order_by: :desc, status: params[:status]), class: 'btn btn-xs btn-primary mx-1' %> + <%= link_to 'Oldest', visits_path(order_by: :asc, status: params[:status]), class: 'btn btn-xs btn-primary mx-1' %>
- <%# Main content area with map and side panel %> -
- <%# Map container %> -
-
+ + + <% if @visits.empty? %> +
+
+
+

Hello there!

+

+ Here you'll find your <%= params[:status] %> visits, but now there are none. Create some areas on your map and pretty soon you'll see visit suggestions on this page! +

+
+
- - <%# Side panel %> -
- <%# Visit filters %> -
-
- <%= link_to 'Confirmed', visits_path(status: :confirmed), role: 'tab', - class: "tab #{active_tab?(visits_path(status: :confirmed))}" %> - <%= link_to visits_path(status: :suggested), role: 'tab', - class: "tab #{active_tab?(visits_path(status: :suggested))}" do %> - Suggested - <% if @suggested_visits_count.positive? %> - <%= @suggested_visits_count %> - <% end %> - <% end %> - <%= link_to 'Declined', visits_path(status: :declined), role: 'tab', - class: "tab #{active_tab?(visits_path(status: :declined))}" %> -
- -
- Order by: - <%= link_to 'Newest', visits_path(order_by: :desc, status: params[:status]), class: 'btn btn-xs btn-primary mx-1' %> - <%= link_to 'Oldest', visits_path(order_by: :asc, status: params[:status]), class: 'btn btn-xs btn-primary mx-1' %> -
-
- - <%# Beta notice %> - - - <%# Visits list %> -
- <% if @visits.empty? %> -
-

No visits found

-

- Here you'll find your <%= params[:status] %> visits, but now there are none. Create some areas on your map and pretty soon you'll see visit suggestions on this page! -

-
- <% else %> -
- <% @visits.each do |visit| %> -
-
- <%# Visit name %> -
- <%= render 'visits/name', visit: visit %> -
- <%= visit.status %> -
-
- - <%# Visit time and duration %> -
-
<%= "#{visit.started_at.strftime('%H:%M')} - #{visit.ended_at.strftime('%H:%M')}" %>
-
Duration: <%= (visit.duration / 60.0).round(1) %> hours
-
- - <%# Action buttons %> -
- <%= render 'visits/buttons', visit: visit %> - -
-
-
- <%= render 'visits/modal', visit: visit %> - <% end %> -
- <% end %> -
- - <%# Pagination %> -
+ <% else %> +
+
<%= paginate @visits %>
-
+ +
    + <% @visits.each do |visit| %> +
  • +
    + + + +
    +
    + +
    +
    + <%= render partial: 'visit', locals: { visit: visit } %> +
    +
    +
    • + <% end %> +
+ <% end %>
diff --git a/spec/factories/places.rb b/spec/factories/places.rb index e94182d4..e9c86f96 100644 --- a/spec/factories/places.rb +++ b/spec/factories/places.rb @@ -5,7 +5,7 @@ FactoryBot.define do name { 'MyString' } latitude { 54.2905245 } longitude { 13.0948638 } - lonlat { "POINT(#{longitude} #{latitude})" } + lonlat { "SRID=4326;POINT(#{longitude} #{latitude})" } trait :with_geodata do geodata do @@ -39,5 +39,14 @@ FactoryBot.define do } end end + + # Special trait for testing with nil lonlat + trait :without_lonlat do + # Skip validation to create an invalid record for testing + to_create { |instance| instance.save(validate: false) } + after(:build) do |place| + place.lonlat = nil + end + end end end diff --git a/spec/jobs/data_migrations/migrate_places_lonlat_job_spec.rb b/spec/jobs/data_migrations/migrate_places_lonlat_job_spec.rb index 8e2e1bed..994ad142 100644 --- a/spec/jobs/data_migrations/migrate_places_lonlat_job_spec.rb +++ b/spec/jobs/data_migrations/migrate_places_lonlat_job_spec.rb @@ -1,5 +1,82 @@ +# frozen_string_literal: true + require 'rails_helper' RSpec.describe DataMigrations::MigratePlacesLonlatJob, type: :job do - pending "add some examples to (or delete) #{__FILE__}" + describe '#perform' do + let(:user) { create(:user) } + + context 'when places exist for the user' do + let!(:place1) { create(:place, :without_lonlat, longitude: 10.0, latitude: 20.0) } + let!(:place2) { create(:place, :without_lonlat, longitude: -73.935242, latitude: 40.730610) } + let!(:other_place) { create(:place, :without_lonlat, longitude: 15.0, latitude: 25.0) } + + # Create visits to associate places with users + let!(:visit1) { create(:visit, user: user, place: place1) } + let!(:visit2) { create(:visit, user: user, place: place2) } + let!(:other_visit) { create(:visit, place: other_place) } # associated with a different user + + it 'updates lonlat field for all places belonging to the user' do + # Force a reload to ensure we have the initial state + place1.reload + place2.reload + + # Both places should have nil lonlat initially + expect(place1.lonlat).to be_nil + expect(place2.lonlat).to be_nil + + # Run the job + described_class.perform_now(user.id) + + # Reload to get updated state + place1.reload + place2.reload + other_place.reload + + # Check that lonlat is now set correctly + expect(place1.lonlat).not_to be_nil + expect(place2.lonlat).not_to be_nil + + # The other user's place should still have nil lonlat + expect(other_place.lonlat).to be_nil + + # Verify the coordinates + expect(place1.lonlat.x).to eq(10.0) # longitude + expect(place1.lonlat.y).to eq(20.0) # latitude + + expect(place2.lonlat.x).to eq(-73.935242) # longitude + expect(place2.lonlat.y).to eq(40.730610) # latitude + end + + it 'sets the correct SRID (4326) on the geometry' do + described_class.perform_now(user.id) + place1.reload + + # SRID should be 4326 (WGS84) + expect(place1.lonlat.srid).to eq(4326) + end + end + + context 'when no places exist for the user' do + it 'completes successfully without errors' do + expect do + described_class.perform_now(user.id) + end.not_to raise_error + end + end + + context 'when user does not exist' do + it 'raises ActiveRecord::RecordNotFound' do + expect do + described_class.perform_now(-1) + end.to raise_error(ActiveRecord::RecordNotFound) + end + end + end + + describe 'queue' do + it 'uses the default queue' do + expect(described_class.queue_name).to eq('default') + end + end end diff --git a/spec/jobs/visit_suggesting_job_spec.rb b/spec/jobs/visit_suggesting_job_spec.rb index a04cb9d1..5ee0e289 100644 --- a/spec/jobs/visit_suggesting_job_spec.rb +++ b/spec/jobs/visit_suggesting_job_spec.rb @@ -72,20 +72,19 @@ RSpec.describe VisitSuggestingJob, type: :job do end context 'with string dates' do - it 'handles string date parameters correctly' do + let(:string_start) { start_at.to_s } + let(:string_end) { end_at.to_s } + let(:parsed_start) { start_at.to_datetime } + let(:parsed_end) { end_at.to_datetime } + + before do allow(Visits::Suggest).to receive(:new).and_call_original allow_any_instance_of(Visits::Suggest).to receive(:call) - - string_start = start_at.to_s - string_end = end_at.to_s - - # We'll mock the Time.zone.parse method to return predictable values - parsed_start = start_at.to_datetime - parsed_end = end_at.to_datetime - allow(Time.zone).to receive(:parse).with(string_start).and_return(parsed_start) allow(Time.zone).to receive(:parse).with(string_end).and_return(parsed_end) + end + it 'handles string date parameters correctly' do # At minimum we expect one call to Suggest expect(Visits::Suggest).to receive(:new).at_least(:once).and_call_original @@ -100,6 +99,7 @@ RSpec.describe VisitSuggestingJob, type: :job do context 'when user is inactive' do before do user.update(status: :inactive) + allow(Visits::Suggest).to receive(:new).and_call_original allow_any_instance_of(Visits::Suggest).to receive(:call) end @@ -107,6 +107,7 @@ RSpec.describe VisitSuggestingJob, type: :job do it 'still processes the job for the specified user' do # The job doesn't check for user active status, it just processes whatever user is passed expect(Visits::Suggest).to receive(:new).at_least(:once).and_call_original + subject end end diff --git a/spec/serializers/api/place_serializer_spec.rb b/spec/serializers/api/place_serializer_spec.rb index d3f8648e..d1703575 100644 --- a/spec/serializers/api/place_serializer_spec.rb +++ b/spec/serializers/api/place_serializer_spec.rb @@ -5,53 +5,46 @@ require 'rails_helper' RSpec.describe Api::PlaceSerializer do describe '#call' do let(:place) do - instance_double( - Place, - id: 123, + create( + :place, + :with_geodata, name: 'Central Park', - lon: -73.9665, - lat: 40.7812, + longitude: -73.9665, + latitude: 40.7812, + lonlat: 'SRID=4326;POINT(-73.9665 40.7812)', city: 'New York', country: 'United States', - source: 'osm', - geodata: { 'amenity' => 'park', 'leisure' => 'park' }, - reverse_geocoded_at: Time.zone.parse('2023-01-15T12:00:00Z') + source: 'photon', + geodata: { 'amenity' => 'park', 'leisure' => 'park' }, reverse_geocoded_at: Time.zone.parse('2023-01-15T12:00:00Z') ) end subject(:serializer) { described_class.new(place) } - it 'initializes with a place object' do - expect(serializer.instance_variable_get(:@place)).to eq(place) - end - it 'serializes a place into a hash with all attributes' do result = serializer.call expect(result).to be_a(Hash) - expect(result[:id]).to eq(123) + expect(result[:id]).to eq(place.id) expect(result[:name]).to eq('Central Park') expect(result[:longitude]).to eq(-73.9665) expect(result[:latitude]).to eq(40.7812) expect(result[:city]).to eq('New York') expect(result[:country]).to eq('United States') - expect(result[:source]).to eq('osm') + expect(result[:source]).to eq('photon') expect(result[:geodata]).to eq({ 'amenity' => 'park', 'leisure' => 'park' }) expect(result[:reverse_geocoded_at]).to eq(Time.zone.parse('2023-01-15T12:00:00Z')) end context 'with nil values' do let(:place_with_nils) do - instance_double( - Place, - id: 456, + create( + :place, name: 'Unknown Place', - lon: nil, - lat: nil, city: nil, country: nil, source: nil, - geodata: nil, + geodata: {}, reverse_geocoded_at: nil ) end @@ -61,35 +54,14 @@ RSpec.describe Api::PlaceSerializer do it 'handles nil values correctly' do result = serializer_with_nils.call - expect(result[:id]).to eq(456) + expect(result[:id]).to eq(place_with_nils.id) expect(result[:name]).to eq('Unknown Place') - expect(result[:longitude]).to be_nil - expect(result[:latitude]).to be_nil expect(result[:city]).to be_nil expect(result[:country]).to be_nil expect(result[:source]).to be_nil - expect(result[:geodata]).to be_nil + expect(result[:geodata]).to eq({}) expect(result[:reverse_geocoded_at]).to be_nil end end - - context 'with actual Place model', type: :model do - let(:real_place) { create(:place) } - subject(:real_serializer) { described_class.new(real_place) } - - it 'serializes a real place model correctly' do - result = real_serializer.call - - expect(result[:id]).to eq(real_place.id) - expect(result[:name]).to eq(real_place.name) - expect(result[:longitude]).to eq(real_place.lon) - expect(result[:latitude]).to eq(real_place.lat) - expect(result[:city]).to eq(real_place.city) - expect(result[:country]).to eq(real_place.country) - expect(result[:source]).to eq(real_place.source) - expect(result[:geodata]).to eq(real_place.geodata) - expect(result[:reverse_geocoded_at]).to eq(real_place.reverse_geocoded_at) - end - end end end diff --git a/spec/serializers/api/visit_serializer_spec.rb b/spec/serializers/api/visit_serializer_spec.rb index bf29e304..949c2ca8 100644 --- a/spec/serializers/api/visit_serializer_spec.rb +++ b/spec/serializers/api/visit_serializer_spec.rb @@ -4,145 +4,27 @@ require 'rails_helper' RSpec.describe Api::VisitSerializer do describe '#call' do - let(:place) do - instance_double( - Place, - id: 123, - lat: 40.7812, - lon: -73.9665 - ) - end - - let(:area) do - instance_double( - Area, - id: 456, - latitude: 41.9028, - longitude: -87.6350 - ) - end - - let(:visit) do - instance_double( - Visit, - id: 789, - area_id: area.id, - user_id: 101, - started_at: Time.zone.parse('2023-01-15T10:00:00Z'), - ended_at: Time.zone.parse('2023-01-15T12:00:00Z'), - duration: 120, # 2 hours in minutes - name: 'Central Park Visit', - status: 'confirmed', - place: place, - area: area, - place_id: place.id - ) - end + let(:place) { create(:place) } + let(:area) { create(:area) } + let(:visit) { create(:visit, place: place, area: area) } subject(:serializer) { described_class.new(visit) } - context 'when a visit has both place and area' do - it 'serializes the visit with place coordinates' do - result = serializer.call + it 'serializes a real visit model correctly' do + result = serializer.call - expect(result).to be_a(Hash) - expect(result[:id]).to eq(789) - expect(result[:area_id]).to eq(456) - expect(result[:user_id]).to eq(101) - expect(result[:started_at]).to eq(Time.zone.parse('2023-01-15T10:00:00Z')) - expect(result[:ended_at]).to eq(Time.zone.parse('2023-01-15T12:00:00Z')) - expect(result[:duration]).to eq(120) - expect(result[:name]).to eq('Central Park Visit') - expect(result[:status]).to eq('confirmed') + expect(result[:id]).to eq(visit.id) + expect(result[:area_id]).to eq(visit.area_id) + expect(result[:user_id]).to eq(visit.user_id) + expect(result[:started_at]).to eq(visit.started_at) + expect(result[:ended_at]).to eq(visit.ended_at) + expect(result[:duration]).to eq(visit.duration) + expect(result[:name]).to eq(visit.name) + expect(result[:status]).to eq(visit.status) - # Place should use place coordinates - expect(result[:place][:id]).to eq(123) - expect(result[:place][:latitude]).to eq(40.7812) - expect(result[:place][:longitude]).to eq(-73.9665) - end - end - - context 'when a visit has area but no place' do - let(:visit_without_place) do - instance_double( - Visit, - id: 789, - area_id: area.id, - user_id: 101, - started_at: Time.zone.parse('2023-01-15T10:00:00Z'), - ended_at: Time.zone.parse('2023-01-15T12:00:00Z'), - duration: 120, - name: 'Chicago Visit', - status: 'suggested', - place: nil, - area: area, - place_id: nil - ) - end - - subject(:serializer_without_place) { described_class.new(visit_without_place) } - - it 'falls back to area coordinates' do - result = serializer_without_place.call - - expect(result[:place][:id]).to be_nil - expect(result[:place][:latitude]).to eq(41.9028) - expect(result[:place][:longitude]).to eq(-87.6350) - end - end - - context 'when a visit has neither place nor area' do - let(:visit_without_location) do - instance_double( - Visit, - id: 789, - area_id: nil, - user_id: 101, - started_at: Time.zone.parse('2023-01-15T10:00:00Z'), - ended_at: Time.zone.parse('2023-01-15T12:00:00Z'), - duration: 120, - name: 'Unknown Location Visit', - status: 'declined', - place: nil, - area: nil, - place_id: nil - ) - end - - subject(:serializer_without_location) { described_class.new(visit_without_location) } - - it 'returns nil for location coordinates' do - result = serializer_without_location.call - - expect(result[:place][:id]).to be_nil - expect(result[:place][:latitude]).to be_nil - expect(result[:place][:longitude]).to be_nil - end - end - - context 'with actual Visit model', type: :model do - let(:real_place) { create(:place) } - let(:real_area) { create(:area) } - let(:real_visit) { create(:visit, place: real_place, area: real_area) } - - subject(:real_serializer) { described_class.new(real_visit) } - - it 'serializes a real visit model correctly' do - result = real_serializer.call - - expect(result[:id]).to eq(real_visit.id) - expect(result[:area_id]).to eq(real_visit.area_id) - expect(result[:user_id]).to eq(real_visit.user_id) - expect(result[:started_at]).to eq(real_visit.started_at) - expect(result[:ended_at]).to eq(real_visit.ended_at) - expect(result[:duration]).to eq(real_visit.duration) - expect(result[:name]).to eq(real_visit.name) - expect(result[:status]).to eq(real_visit.status) - - expect(result[:place][:id]).to eq(real_place.id) - expect(result[:place][:latitude]).to eq(real_place.lat) - expect(result[:place][:longitude]).to eq(real_place.lon) - end + expect(result[:place][:id]).to eq(place.id) + expect(result[:place][:latitude]).to eq(place.lat) + expect(result[:place][:longitude]).to eq(place.lon) end end end