From 630c813f0bc8fa2eb3649fe859bb56e99f81c3c5 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sat, 17 May 2025 20:10:03 +0200 Subject: [PATCH] Fix visits overlapping issue --- CHANGELOG.md | 2 + app/services/visits/creator.rb | 41 +++++++++++++++- app/views/visits/_buttons.html.erb | 8 +++- spec/services/visits/creator_spec.rb | 70 ++++++++++++++++++++++++++++ 4 files changed, 118 insertions(+), 3 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index fa8ff21b..6b14a9b5 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -40,6 +40,7 @@ If you're running your own Photon instance, you can safely set `STORE_GEODATA` t - Reverse geocoding is now working as on-demand job instead of storing the result in the database. - Stats cards now show the last update time. #733 +- Visit card now shows buttons to confirm or decline a visit only if it's not confirmed or declined yet. ## Fixed @@ -47,6 +48,7 @@ If you're running your own Photon instance, you can safely set `STORE_GEODATA` t - Importing GeoJSON files now saves velocity if it was stored in either `velocity` or `speed` property. - `rake points:migrate_to_lonlat` should work properly now. #1083 #1161 - PostGIS extension is now being enabled only if it's not already enabled. #1186 +- Fixed a bug where visits were returning into Suggested state after being confirmed or declined. #848 # 0.26.0 - 2025-05-08 diff --git a/app/services/visits/creator.rb b/app/services/visits/creator.rb index c7e672d6..fb600894 100644 --- a/app/services/visits/creator.rb +++ b/app/services/visits/creator.rb @@ -11,6 +11,10 @@ module Visits def create_visits(visits) visits.map do |visit_data| + # Check for existing confirmed visits at this location + existing_confirmed = find_existing_confirmed_visit(visit_data) + next existing_confirmed if existing_confirmed + # Variables to store data outside the transaction visit_instance = nil place_data = nil @@ -46,11 +50,46 @@ module Visits end visit_instance - end + end.compact end private + # Find if there's already a confirmed visit at this location within a similar time + def find_existing_confirmed_visit(visit_data) + # Define time window to look for existing visits (slightly wider than the visit) + start_time = Time.zone.at(visit_data[:start_time]) - 1.hour + end_time = Time.zone.at(visit_data[:end_time]) + 1.hour + + # Look for confirmed visits with a similar location + user.visits + .confirmed + .where('(started_at BETWEEN ? AND ?) OR (ended_at BETWEEN ? AND ?)', + start_time, end_time, start_time, end_time) + .find_each do |visit| + # Skip if the visit doesn't have place or area coordinates + next unless visit.place || visit.area + + # Get coordinates to compare + visit_lat = visit.place&.lat || visit.area&.latitude + visit_lon = visit.place&.lon || visit.area&.longitude + + next unless visit_lat && visit_lon + + # Calculate distance between centers + distance = Geocoder::Calculations.distance_between( + [visit_data[:center_lat], visit_data[:center_lon]], + [visit_lat, visit_lon], + units: :km + ) + + # If this confirmed visit is within 100 meters of the new suggestion + return visit if distance <= 0.1 + end + + nil + end + # Create place_visits records directly to avoid deadlocks def associate_suggested_places(visit, suggested_places) existing_place_ids = visit.place_visits.pluck(:place_id) diff --git a/app/views/visits/_buttons.html.erb b/app/views/visits/_buttons.html.erb index e258e450..404e70e6 100644 --- a/app/views/visits/_buttons.html.erb +++ b/app/views/visits/_buttons.html.erb @@ -1,2 +1,6 @@ -<%= link_to 'Confirm', visit_path(visit, 'visit[status]': :confirmed), method: :patch, data: { turbo_method: :patch }, class: 'btn btn-xs btn-success' %> -<%= link_to 'Decline', visit_path(visit, 'visit[status]': :declined), method: :patch, data: { turbo_method: :patch }, class: 'btn btn-xs btn-error mx-1' %> +<% if !visit.confirmed? %> + <%= link_to 'Confirm', visit_path(visit, 'visit[status]': :confirmed), method: :patch, data: { turbo_method: :patch }, class: 'btn btn-xs btn-success' %> +<% end %> +<% if !visit.declined? %> + <%= link_to 'Decline', visit_path(visit, 'visit[status]': :declined), method: :patch, data: { turbo_method: :patch }, class: 'btn btn-xs btn-error mx-1' %> +<% end %> diff --git a/spec/services/visits/creator_spec.rb b/spec/services/visits/creator_spec.rb index 110fe192..dad0bef0 100644 --- a/spec/services/visits/creator_spec.rb +++ b/spec/services/visits/creator_spec.rb @@ -24,6 +24,76 @@ RSpec.describe Visits::Creator do } end + context 'when a confirmed visit already exists at the same location' do + let(:place) { create(:place, lonlat: 'POINT(-74.0060 40.7128)', name: 'Existing Place') } + let!(:existing_visit) do + create( + :visit, + user: user, + place: place, + status: :confirmed, + started_at: 1.5.hours.ago, + ended_at: 45.minutes.ago, + duration: 45 + ) + end + + it 'returns the existing confirmed visit instead of creating a duplicate suggested visit' do + visits = subject.create_visits([visit_data]) + + expect(visits.size).to eq(1) + expect(visits.first).to eq(existing_visit) + expect(visits.first.status).to eq('confirmed') + + # Verify no new visits were created + expect(Visit.count).to eq(1) + end + + it 'does not change points associations' do + original_visit_id = point1.visit_id + + subject.create_visits([visit_data]) + + # Points should remain unassociated + expect(point1.reload.visit_id).to eq(original_visit_id) + expect(point2.reload.visit_id).to eq(nil) + end + end + + context 'when a confirmed visit exists but at a different location' do + let(:different_place) { create(:place, lonlat: 'POINT(-73.9000 41.0000)', name: 'Different Place') } + let!(:existing_visit) do + create( + :visit, + user: user, + place: different_place, + status: :confirmed, + started_at: 1.5.hours.ago, + ended_at: 45.minutes.ago, + duration: 45 + ) + end + let(:place) { create(:place, lonlat: 'POINT(-74.0060 40.7128)', name: 'New Place') } + let(:place_finder) { instance_double(Visits::PlaceFinder) } + + before do + allow(Visits::PlaceFinder).to receive(:new).with(user).and_return(place_finder) + allow(place_finder).to receive(:find_or_create_place).and_return({ main_place: place, suggested_places: [] }) + end + + it 'creates a new suggested visit' do + visits = subject.create_visits([visit_data]) + + expect(visits.size).to eq(1) + expect(visits.first).not_to eq(existing_visit) + expect(visits.first.place).to eq(place) + expect(visits.first.status).to eq('suggested') + + # Should now have two visits + expect(Visit.count).to eq(2) + end + end + context 'when matching an area' do let!(:area) { create(:area, user: user, latitude: 40.7128, longitude: -74.0060, radius: 100) }