From 4a859fb350e1f76f25e0f07708f88304ce4e9dbe Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Sun, 9 Mar 2025 14:58:30 +0100 Subject: [PATCH] Add bunch of tests and refactor some stuff --- CHANGELOG.md | 16 - .../v1/visits/possible_places_controller.rb | 4 +- app/controllers/api/v1/visits_controller.rb | 41 +-- app/javascript/maps/visits.js | 12 + app/jobs/bulk_visits_suggesting_job.rb | 30 +- app/models/concerns/point_validation.rb | 2 + app/serializers/api/visit_serializer.rb | 52 +-- ...{bulk_update_service.rb => bulk_update.rb} | 6 +- app/services/visits/find_in_time.rb | 31 ++ .../visits/find_within_bounding_box.rb | 29 ++ app/services/visits/finder.rb | 31 ++ app/services/visits/merge_service.rb | 67 ++-- app/services/visits/suggest.rb | 2 +- app/services/visits/time_chunks.rb | 47 +++ spec/factories/places.rb | 38 ++ spec/models/concerns/point_validation_spec.rb | 162 +++++++++ spec/models/place_spec.rb | 41 ++- .../api/v1/visits/possible_places_spec.rb | 54 ++- spec/requests/api/v1/visits_spec.rb | 185 +++++++++- spec/serializers/api/place_serializer_spec.rb | 95 +++++ spec/serializers/api/visit_serializer_spec.rb | 148 ++++++++ spec/services/visits/bulk_update_spec.rb | 154 ++++++++ spec/services/visits/creator_spec.rb | 197 ++++++++++- spec/services/visits/detector_spec.rb | 334 +++++++++++++++--- spec/services/visits/find_in_time_spec.rb | 142 ++++++++ .../visits/find_within_bounding_box_spec.rb | 161 +++++++++ spec/services/visits/finder_spec.rb | 154 ++++++++ spec/services/visits/place_finder_spec.rb | 229 +++++++++++- spec/services/visits/time_chunks_spec.rb | 161 +++++++++ 29 files changed, 2392 insertions(+), 233 deletions(-) rename app/services/visits/{bulk_update_service.rb => bulk_update.rb} (81%) create mode 100644 app/services/visits/find_in_time.rb create mode 100644 app/services/visits/find_within_bounding_box.rb create mode 100644 app/services/visits/finder.rb create mode 100644 app/services/visits/time_chunks.rb create mode 100644 spec/models/concerns/point_validation_spec.rb create mode 100644 spec/serializers/api/place_serializer_spec.rb create mode 100644 spec/serializers/api/visit_serializer_spec.rb create mode 100644 spec/services/visits/bulk_update_spec.rb create mode 100644 spec/services/visits/find_in_time_spec.rb create mode 100644 spec/services/visits/find_within_bounding_box_spec.rb create mode 100644 spec/services/visits/finder_spec.rb create mode 100644 spec/services/visits/time_chunks_spec.rb diff --git a/CHANGELOG.md b/CHANGELOG.md index 3952a841..6afdc81c 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,23 +6,7 @@ and this project adheres to [Semantic Versioning](http://semver.org/). TODO: -- Specs for app/controllers/api/v1/visits/possible_places_controller.rb -- Specs for app/controllers/api/v1/visits_controller.rb -- Move code from app/controllers/api/v1/visits/possible_places_controller.rb to services -- Move code from app/jobs/bulk_visits_suggesting_job.rb to services -- Specs for app/models/concerns/point_validation.rb -- Specs for app/models/place.rb -- Specs for app/serializers/api/place_serializer.rb -- Specs for app/serializers/api/visit_serializer.rb -- ActiveModel::Serializer -- Specs for app/services/visits/bulk_update_service.rb -- Specs for app/services/visits/creator.rb -- Specs for app/services/visits/detector.rb - Specs for app/services/visits/merge_service.rb and rename it probably -- Specs for app/services/visits/merger.rb -- Specs for app/services/visits/place_finder.rb -- Specs for app/services/visits/smart_detect.rb -- Specs for app/services/visits/suggest.rb - Remove Stimulus controllers for visits on the Visits page - Revert changes to Visits page diff --git a/app/controllers/api/v1/visits/possible_places_controller.rb b/app/controllers/api/v1/visits/possible_places_controller.rb index 02f4a1be..add04ef1 100644 --- a/app/controllers/api/v1/visits/possible_places_controller.rb +++ b/app/controllers/api/v1/visits/possible_places_controller.rb @@ -3,7 +3,9 @@ class Api::V1::Visits::PossiblePlacesController < ApiController def index visit = current_api_user.visits.find(params[:id]) - possible_places = visit.suggested_places + possible_places = visit.suggested_places.map do |place| + Api::PlaceSerializer.new(place).call + end render json: possible_places rescue ActiveRecord::RecordNotFound diff --git a/app/controllers/api/v1/visits_controller.rb b/app/controllers/api/v1/visits_controller.rb index 4269230e..9832d6b4 100644 --- a/app/controllers/api/v1/visits_controller.rb +++ b/app/controllers/api/v1/visits_controller.rb @@ -2,44 +2,7 @@ class Api::V1::VisitsController < ApiController def index - # If selection is true, filter by coordinates instead of time - if params[:selection] == 'true' && params[:sw_lat].present? && params[:sw_lng].present? && params[:ne_lat].present? && params[:ne_lng].present? - sw_lat = params[:sw_lat].to_f - sw_lng = params[:sw_lng].to_f - ne_lat = params[:ne_lat].to_f - ne_lng = params[:ne_lng].to_f - - # Create the PostGIS bounding box polygon - bounding_box = "ST_MakeEnvelope(#{sw_lng}, #{sw_lat}, #{ne_lng}, #{ne_lat}, 4326)" - - visits = - Visit - .includes(:place) - .where(user: current_api_user) - .joins(:place) - .where("ST_Contains(#{bounding_box}, ST_SetSRID(places.lonlat::geometry, 4326))") - .order(started_at: :desc) - else - # Regular time-based filtering - start_time = begin - Time.zone.parse(params[:start_at]) - rescue StandardError - Time.zone.now.beginning_of_day - end - end_time = begin - Time.zone.parse(params[:end_at]) - rescue StandardError - Time.zone.now.end_of_day - end - - visits = - Visit - .includes(:place) - .where(user: current_api_user) - .where('started_at >= ? AND ended_at <= ?', start_time, end_time) - .order(started_at: :desc) - end - + visits = Visits::Finder.new(current_api_user, params).call serialized_visits = visits.map do |visit| Api::VisitSerializer.new(visit).call end @@ -81,7 +44,7 @@ class Api::V1::VisitsController < ApiController end def bulk_update - service = Visits::BulkUpdateService.new( + service = Visits::BulkUpdate.new( current_api_user, params[:visit_ids], params[:status] diff --git a/app/javascript/maps/visits.js b/app/javascript/maps/visits.js index 92636c12..4256cdb3 100644 --- a/app/javascript/maps/visits.js +++ b/app/javascript/maps/visits.js @@ -8,6 +8,18 @@ export class VisitsManager { constructor(map, apiKey) { this.map = map; this.apiKey = apiKey; + + // Create custom panes for different visit types + if (!map.getPane('confirmedVisitsPane')) { + map.createPane('confirmedVisitsPane'); + map.getPane('confirmedVisitsPane').style.zIndex = 450; // Above default overlay pane (400) + } + + if (!map.getPane('suggestedVisitsPane')) { + map.createPane('suggestedVisitsPane'); + map.getPane('suggestedVisitsPane').style.zIndex = 430; // Below confirmed visits but above base layers + } + this.visitCircles = L.layerGroup(); this.confirmedVisitCircles = L.layerGroup().addTo(map); // Always visible layer for confirmed visits this.currentPopup = null; diff --git a/app/jobs/bulk_visits_suggesting_job.rb b/app/jobs/bulk_visits_suggesting_job.rb index 0006239a..1d615609 100644 --- a/app/jobs/bulk_visits_suggesting_job.rb +++ b/app/jobs/bulk_visits_suggesting_job.rb @@ -10,38 +10,22 @@ class BulkVisitsSuggestingJob < ApplicationJob start_at = start_at.to_datetime end_at = end_at.to_datetime - time_chunks = time_chunks(start_at:, end_at:) + time_chunks = Visits::TimeChunks.new(start_at:, end_at:).call users.active.find_each do |user| next if user.tracked_points.empty? - time_chunks.each do |time_chunk| - VisitSuggestingJob.perform_later( - user_id: user.id, start_at: time_chunk.first, end_at: time_chunk.last - ) - end + schedule_chunked_jobs(user, time_chunks) end end private - def time_chunks(start_at:, end_at:) - time_chunks = [] - - # First chunk: from start_at to end of that year - first_end = start_at.end_of_year - time_chunks << (start_at...first_end) if start_at < first_end - - # Full-year chunks - current = first_end.beginning_of_year + 1.year # Start from the next full year - while current + 1.year <= end_at.beginning_of_year - time_chunks << (current...current + 1.year) - current += 1.year + def schedule_chunked_jobs(user, time_chunks) + time_chunks.each do |time_chunk| + VisitSuggestingJob.perform_later( + user_id: user.id, start_at: time_chunk.first, end_at: time_chunk.last + ) end - - # Last chunk: from start of the last year to end_at - time_chunks << (current...end_at) if current < end_at - - time_chunks end end diff --git a/app/models/concerns/point_validation.rb b/app/models/concerns/point_validation.rb index 09cda18e..b57d0ba6 100644 --- a/app/models/concerns/point_validation.rb +++ b/app/models/concerns/point_validation.rb @@ -1,3 +1,5 @@ +# frozen_string_literal: true + module PointValidation extend ActiveSupport::Concern diff --git a/app/serializers/api/visit_serializer.rb b/app/serializers/api/visit_serializer.rb index 31ff7cdf..8bf8ff71 100644 --- a/app/serializers/api/visit_serializer.rb +++ b/app/serializers/api/visit_serializer.rb @@ -7,14 +7,14 @@ class Api::VisitSerializer def call { - id: id, - area_id: area_id, - user_id: user_id, - started_at: started_at, - ended_at: ended_at, - duration: duration, - name: name, - status: status, + id: visit.id, + area_id: visit.area_id, + user_id: visit.user_id, + started_at: visit.started_at, + ended_at: visit.ended_at, + duration: visit.duration, + name: visit.name, + status: visit.status, place: { latitude: visit.place&.lat || visit.area&.latitude, longitude: visit.place&.lon || visit.area&.longitude, @@ -26,40 +26,4 @@ class Api::VisitSerializer private attr_reader :visit - - def id - visit.id - end - - def area_id - visit.area_id - end - - def user_id - visit.user_id - end - - def started_at - visit.started_at - end - - def ended_at - visit.ended_at - end - - def duration - visit.duration - end - - def name - visit.name - end - - def status - visit.status - end - - def place_id - visit.place_id - end end diff --git a/app/services/visits/bulk_update_service.rb b/app/services/visits/bulk_update.rb similarity index 81% rename from app/services/visits/bulk_update_service.rb rename to app/services/visits/bulk_update.rb index 71d521a0..a96f566f 100644 --- a/app/services/visits/bulk_update_service.rb +++ b/app/services/visits/bulk_update.rb @@ -1,7 +1,7 @@ # frozen_string_literal: true module Visits - class BulkUpdateService + class BulkUpdate attr_reader :user, :visit_ids, :status, :errors def initialize(user, visit_ids, status) @@ -26,7 +26,7 @@ module Visits return end - return if %w[confirmed declined].include?(status) + return if Visit.statuses.keys.include?(status) errors << 'Invalid status' end @@ -39,7 +39,9 @@ module Visits return false end + # rubocop:disable Rails/SkipsModelValidations updated_count = visits.update_all(status: status) + # rubocop:enable Rails/SkipsModelValidations { count: updated_count, visits: visits } end diff --git a/app/services/visits/find_in_time.rb b/app/services/visits/find_in_time.rb new file mode 100644 index 00000000..e486382a --- /dev/null +++ b/app/services/visits/find_in_time.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Visits + class FindInTime + def initialize(user, params) + @user = user + @start_at = parse_time(params[:start_at]) + @end_at = parse_time(params[:end_at]) + end + + def call + Visit + .includes(:place) + .where(user:) + .where('started_at >= ? AND ended_at <= ?', start_at, end_at) + .order(started_at: :desc) + end + + private + + attr_reader :user, :start_at, :end_at + + def parse_time(time_string) + parsed_time = Time.zone.parse(time_string) + + raise ArgumentError, "Invalid time format: #{time_string}" if parsed_time.nil? + + parsed_time + end + end +end diff --git a/app/services/visits/find_within_bounding_box.rb b/app/services/visits/find_within_bounding_box.rb new file mode 100644 index 00000000..74b72ed7 --- /dev/null +++ b/app/services/visits/find_within_bounding_box.rb @@ -0,0 +1,29 @@ +# frozen_string_literal: true + +module Visits + # Finds visits in a selected area on the map + class FindWithinBoundingBox + def initialize(user, params) + @user = user + @sw_lat = params[:sw_lat].to_f + @sw_lng = params[:sw_lng].to_f + @ne_lat = params[:ne_lat].to_f + @ne_lng = params[:ne_lng].to_f + end + + def call + bounding_box = "ST_MakeEnvelope(#{sw_lng}, #{sw_lat}, #{ne_lng}, #{ne_lat}, 4326)" + + Visit + .includes(:place) + .where(user:) + .joins(:place) + .where("ST_Contains(#{bounding_box}, ST_SetSRID(places.lonlat::geometry, 4326))") + .order(started_at: :desc) + end + + private + + attr_reader :user, :sw_lat, :sw_lng, :ne_lat, :ne_lng + end +end diff --git a/app/services/visits/finder.rb b/app/services/visits/finder.rb new file mode 100644 index 00000000..8ed32593 --- /dev/null +++ b/app/services/visits/finder.rb @@ -0,0 +1,31 @@ +# frozen_string_literal: true + +module Visits + # Finds visits in a selected area on the map + class Finder + def initialize(user, params) + @user = user + @params = params + end + + def call + if area_selected? + Visits::FindWithinBoundingBox.new(user, params).call + else + Visits::FindInTime.new(user, params).call + end + end + + private + + attr_reader :user, :params + + def area_selected? + params[:selection] == 'true' && + params[:sw_lat].present? && + params[:sw_lng].present? && + params[:ne_lat].present? && + params[:ne_lng].present? + end + end +end diff --git a/app/services/visits/merge_service.rb b/app/services/visits/merge_service.rb index a963f58d..01ad9975 100644 --- a/app/services/visits/merge_service.rb +++ b/app/services/visits/merge_service.rb @@ -3,10 +3,11 @@ module Visits # Service to handle merging multiple visits into one class MergeService - attr_reader :visits, :errors + attr_reader :visits, :errors, :base_visit def initialize(visits) @visits = visits + @base_visit = visits.first @errors = [] end @@ -27,36 +28,10 @@ module Visits def merge_visits Visit.transaction do - # Use the first visit as the base for the merged visit - base_visit = visits.first + update_base_visit(base_visit, visits) + reassign_points(base_visit, visits) - # Calculate the new start and end times - earliest_start = visits.min_by(&:started_at).started_at - latest_end = visits.max_by(&:ended_at).ended_at - - # Calculate the total duration (sum of all visit durations) - total_duration = ((latest_end - earliest_start) / 60).round - - # Create a combined name - combined_name = "Combined Visit (#{earliest_start.strftime('%b %d')} - #{latest_end.strftime('%b %d')})" - - # Update the base visit with the new data - base_visit.update!( - started_at: earliest_start, - ended_at: latest_end, - duration: total_duration, - name: combined_name, - status: 'confirmed' # Set status to confirmed for the merged visit - ) - - # Move all points from other visits to the base visit - visits[1..].each do |visit| - # Update points to associate with the base visit - visit.points.update_all(visit_id: base_visit.id) # rubocop:disable Rails/SkipsModelValidations - - # Delete the other visit - visit.destroy! - end + visits.drop(1).each(&:destroy!) base_visit end @@ -65,5 +40,37 @@ module Visits add_error(e.record.errors.full_messages.join(', ')) nil end + + def prepare_base_visit + earliest_start = visits.min_by(&:started_at).started_at + latest_end = visits.max_by(&:ended_at).ended_at + total_duration = ((latest_end - earliest_start) / 60).round + combined_name = "Combined Visit (#{visits.map(&:name).join(', ')})" + + { + earliest_start:, + latest_end:, + total_duration:, + combined_name: + } + end + + def update_base_visit(base_visit) + base_visit_data = prepare_base_visit + + base_visit.update!( + started_at: base_visit_data[:earliest_start], + ended_at: base_visit_data[:latest_end], + duration: base_visit_data[:total_duration], + name: base_visit_data[:combined_name], + status: 'confirmed' + ) + end + + def reassign_points(base_visit, visits) + visits[1..].each do |visit| + visit.points.update_all(visit_id: base_visit.id) # rubocop:disable Rails/SkipsModelValidations + end + end end end diff --git a/app/services/visits/suggest.rb b/app/services/visits/suggest.rb index 292190b3..5ee7881c 100644 --- a/app/services/visits/suggest.rb +++ b/app/services/visits/suggest.rb @@ -14,7 +14,7 @@ class Visits::Suggest def call visits = Visits::SmartDetect.new(user, start_at:, end_at:).call - # create_visits_notification(user) if visits.any? + create_visits_notification(user) if visits.any? return nil unless DawarichSettings.reverse_geocoding_enabled? diff --git a/app/services/visits/time_chunks.rb b/app/services/visits/time_chunks.rb new file mode 100644 index 00000000..d7fc2738 --- /dev/null +++ b/app/services/visits/time_chunks.rb @@ -0,0 +1,47 @@ +# frozen_string_literal: true + +module Visits + class TimeChunks + def initialize(start_at:, end_at:) + @start_at = start_at + @end_at = end_at + @time_chunks = [] + end + + 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 + + # First chunk: from start_at to end of that year + first_end = start_at.end_of_year + time_chunks << (start_at...first_end) + + # Full-year chunks + current = first_end.beginning_of_year + 1.year # Start from the next full year + while current.year < end_at.year + year_end = current.end_of_year + time_chunks << (current...year_end) + current += 1.year + end + + # Last chunk: from start of the last year to end_at + time_chunks << (current...end_at) if current.year == end_at.year + + time_chunks + end + + private + + attr_reader :start_at, :end_at, :time_chunks + end +end diff --git a/spec/factories/places.rb b/spec/factories/places.rb index cdef9618..1eed6bdc 100644 --- a/spec/factories/places.rb +++ b/spec/factories/places.rb @@ -6,5 +6,43 @@ FactoryBot.define do latitude { 1.5 } longitude { 1.5 } lonlat { "POINT(#{longitude} #{latitude})" } + + trait :with_geodata do + geodata do + { + "features": [ + { + "geometry": { + "coordinates": [ + 13.0948638, + 54.2905245 + ], + "type": 'Point' + }, + "type": 'Feature', + "properties": { + "osm_id": 5_762_449_774, + "country": 'Germany', + "city": 'Stralsund', + "countrycode": 'DE', + "postcode": '18439', + "locality": 'Frankensiedlung', + "county": 'Vorpommern-Rügen', + "type": 'house', + "osm_type": 'N', + "osm_key": 'amenity', + "housenumber": '84-85', + "street": 'Greifswalder Chaussee', + "district": 'Franken', + "osm_value": 'restaurant', + "name": 'Braugasthaus Zum Alten Fritz', + "state": 'Mecklenburg-Vorpommern' + } + } + ], + "type": 'FeatureCollection' + } + end + end end end diff --git a/spec/models/concerns/point_validation_spec.rb b/spec/models/concerns/point_validation_spec.rb new file mode 100644 index 00000000..929fed74 --- /dev/null +++ b/spec/models/concerns/point_validation_spec.rb @@ -0,0 +1,162 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe PointValidation do + # Create a test class that includes the concern + let(:test_class) do + Class.new do + include PointValidation + end + end + + let(:validator) { test_class.new } + let(:user) { create(:user) } + + describe '#point_exists?' do + context 'with invalid coordinates' do + it 'returns false for zero coordinates' do + params = { longitude: '0', latitude: '0', timestamp: Time.now.to_i } + expect(validator.point_exists?(params, user.id)).to be false + end + + it 'returns false for longitude outside valid range' do + params = { longitude: '181', latitude: '45', timestamp: Time.now.to_i } + expect(validator.point_exists?(params, user.id)).to be false + + params = { longitude: '-181', latitude: '45', timestamp: Time.now.to_i } + expect(validator.point_exists?(params, user.id)).to be false + end + + it 'returns false for latitude outside valid range' do + params = { longitude: '45', latitude: '91', timestamp: Time.now.to_i } + expect(validator.point_exists?(params, user.id)).to be false + + params = { longitude: '45', latitude: '-91', timestamp: Time.now.to_i } + expect(validator.point_exists?(params, user.id)).to be false + end + end + + context 'with valid coordinates' do + let(:longitude) { 10.0 } + let(:latitude) { 50.0 } + let(:timestamp) { Time.now.to_i } + let(:params) { { longitude: longitude.to_s, latitude: latitude.to_s, timestamp: timestamp } } + + context 'when point does not exist' do + before do + allow(Point).to receive(:where).and_return(double(exists?: false)) + end + + it 'returns false' do + expect(validator.point_exists?(params, user.id)).to be false + end + + it 'queries the database with correct parameters' do + expect(Point).to receive(:where).with( + 'ST_SetSRID(ST_MakePoint(?, ?), 4326) = lonlat AND timestamp = ? AND user_id = ?', + longitude, latitude, timestamp, user.id + ).and_return(double(exists?: false)) + + validator.point_exists?(params, user.id) + end + end + + context 'when point exists' do + before do + allow(Point).to receive(:where).and_return(double(exists?: true)) + end + + it 'returns true' do + expect(validator.point_exists?(params, user.id)).to be true + end + end + end + + context 'with string parameters' do + it 'converts string coordinates to float values' do + params = { longitude: '10.5', latitude: '50.5', timestamp: '1650000000' } + + expect(Point).to receive(:where).with( + 'ST_SetSRID(ST_MakePoint(?, ?), 4326) = lonlat AND timestamp = ? AND user_id = ?', + 10.5, 50.5, 1_650_000_000, user.id + ).and_return(double(exists?: false)) + + validator.point_exists?(params, user.id) + end + end + + context 'with different boundary values' do + it 'accepts maximum valid coordinate values' do + params = { longitude: '180', latitude: '90', timestamp: Time.now.to_i } + + expect(Point).to receive(:where).and_return(double(exists?: false)) + expect(validator.point_exists?(params, user.id)).to be false + end + + it 'accepts minimum valid coordinate values' do + params = { longitude: '-180', latitude: '-90', timestamp: Time.now.to_i } + + expect(Point).to receive(:where).and_return(double(exists?: false)) + expect(validator.point_exists?(params, user.id)).to be false + end + end + + context 'with integration tests', :db do + # These tests require a database with PostGIS support + # Only run them if using real database integration + + let(:existing_timestamp) { 1_650_000_000 } + let(:existing_point_params) do + { + longitude: 10.5, + latitude: 50.5, + timestamp: existing_timestamp, + user_id: user.id + } + end + + before do + # Skip this context if not in integration mode + skip 'Skipping integration tests' unless ENV['RUN_INTEGRATION_TESTS'] + + # Create a point in the database + existing_point = Point.create!( + lonlat: "POINT(#{existing_point_params[:longitude]} #{existing_point_params[:latitude]})", + timestamp: existing_timestamp, + user_id: user.id + ) + end + + it 'returns true when a point with same coordinates and timestamp exists' do + params = { + longitude: existing_point_params[:longitude].to_s, + latitude: existing_point_params[:latitude].to_s, + timestamp: existing_timestamp + } + + expect(validator.point_exists?(params, user.id)).to be true + end + + it 'returns false when a point with different coordinates exists' do + params = { + longitude: (existing_point_params[:longitude] + 0.1).to_s, + latitude: existing_point_params[:latitude].to_s, + timestamp: existing_timestamp + } + + expect(validator.point_exists?(params, user.id)).to be false + end + + it 'returns false when a point with different timestamp exists' do + params = { + longitude: existing_point_params[:longitude].to_s, + latitude: existing_point_params[:latitude].to_s, + timestamp: existing_timestamp + 1 + } + + expect(validator.point_exists?(params, user.id)).to be false + end + end + end +end diff --git a/spec/models/place_spec.rb b/spec/models/place_spec.rb index 3fdf3cac..957aeed4 100644 --- a/spec/models/place_spec.rb +++ b/spec/models/place_spec.rb @@ -19,14 +19,51 @@ RSpec.describe Place, type: :model do end describe 'methods' do - describe '#async_reverse_geocode' do - let(:place) { create(:place) } + let(:place) { create(:place, :with_geodata) } + describe '#async_reverse_geocode' do + before { allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) } before { allow(DawarichSettings).to receive(:reverse_geocoding_enabled?).and_return(true) } it 'updates address' do expect { place.async_reverse_geocode }.to have_enqueued_job(ReverseGeocodingJob).with('Place', place.id) end end + + describe '#osm_id' do + it 'returns the osm_id' do + expect(place.osm_id).to eq(583_204_619) + end + end + + describe '#osm_key' do + it 'returns the osm_key' do + expect(place.osm_key).to eq('tourism') + end + end + + describe '#osm_value' do + it 'returns the osm_value' do + expect(place.osm_value).to eq('restaurant') + end + end + + describe '#osm_type' do + it 'returns the osm_type' do + expect(place.osm_type).to eq('N') + end + end + + describe '#lon' do + it 'returns the longitude' do + expect(place.lon).to eq(13.094891305125158) + end + end + + describe '#lat' do + it 'returns the latitude' do + expect(place.lat).to eq(54.29058712007127) + end + end end end diff --git a/spec/requests/api/v1/visits/possible_places_spec.rb b/spec/requests/api/v1/visits/possible_places_spec.rb index e1d6875d..2640701b 100644 --- a/spec/requests/api/v1/visits/possible_places_spec.rb +++ b/spec/requests/api/v1/visits/possible_places_spec.rb @@ -1,7 +1,55 @@ +# frozen_string_literal: true + require 'rails_helper' -RSpec.describe "Api::V1::Visits::PossiblePlaces", type: :request do - describe "GET /index" do - pending "add some examples (or delete) #{__FILE__}" +RSpec.describe 'Api::V1::Visits::PossiblePlaces', type: :request do + let(:user) { create(:user) } + let(:api_key) { user.api_key } + let(:visit) { create(:visit, user:) } + let(:place) { create(:place) } + let!(:place_visit) { create(:place_visit, visit:, place:) } + let(:other_user) { create(:user) } + let(:other_visit) { create(:visit, user: other_user) } + + describe 'GET /api/v1/visits/:id/possible_places' do + context 'when visit belongs to the user' do + it 'returns a list of suggested places for the visit' do + get "/api/v1/visits/#{visit.id}/possible_places", params: { api_key: } + + expect(response).to have_http_status(:ok) + json_response = JSON.parse(response.body) + expect(json_response).to be_an(Array) + expect(json_response.size).to eq(1) + expect(json_response.first['id']).to eq(place.id) + end + end + + context 'when visit does not exist' do + it 'returns a not found error' do + get '/api/v1/visits/999999/possible_places', headers: { 'Authorization' => "Bearer #{api_key}" } + + expect(response).to have_http_status(:not_found) + json_response = JSON.parse(response.body) + expect(json_response['error']).to eq('Visit not found') + end + end + + context 'when visit does not belong to the user' do + it 'returns a not found error' do + get "/api/v1/visits/#{other_visit.id}/possible_places", params: { api_key: } + + expect(response).to have_http_status(:not_found) + json_response = JSON.parse(response.body) + expect(json_response['error']).to eq('Visit not found') + end + end + + context 'when no api key is provided' do + it 'returns unauthorized error' do + get "/api/v1/visits/#{visit.id}/possible_places" + + expect(response).to have_http_status(:unauthorized) + end + end end end diff --git a/spec/requests/api/v1/visits_spec.rb b/spec/requests/api/v1/visits_spec.rb index 4efb944d..a4b3a877 100644 --- a/spec/requests/api/v1/visits_spec.rb +++ b/spec/requests/api/v1/visits_spec.rb @@ -4,8 +4,66 @@ require 'rails_helper' RSpec.describe 'Api::V1::Visits', type: :request do let(:user) { create(:user) } - let(:api_key) { user.api_key } + let(:place) { create(:place) } + let(:other_user) { create(:user) } + let(:auth_headers) { { 'Authorization' => "Bearer #{api_key}" } } + + describe 'GET /api/v1/visits' do + let!(:visit1) { create(:visit, user: user, place: place, started_at: 2.days.ago, ended_at: 1.day.ago) } + let!(:visit2) { create(:visit, user: user, place: place, started_at: 4.days.ago, ended_at: 3.days.ago) } + let!(:other_user_visit) { create(:visit, user: other_user, place: place) } + + context 'when requesting time-based visits' do + let(:params) do + { + start_at: 5.days.ago.iso8601, + end_at: Time.zone.now.iso8601 + } + end + + it 'returns visits within the specified time range' do + get '/api/v1/visits', params: params, headers: auth_headers + + expect(response).to have_http_status(:ok) + json_response = JSON.parse(response.body) + expect(json_response.size).to eq(2) + expect(json_response.pluck('id')).to include(visit1.id, visit2.id) + end + + it 'does not return visits from other users' do + get '/api/v1/visits', params: params, headers: auth_headers + + json_response = JSON.parse(response.body) + expect(json_response.pluck('id')).not_to include(other_user_visit.id) + end + end + + context 'when requesting area-based visits' do + let(:place_inside) { create(:place, latitude: 50.0, longitude: 14.0) } + let!(:visit_inside) { create(:visit, user: user, place: place_inside) } + + let(:params) do + { + selection: 'true', + sw_lat: '49.0', + sw_lng: '13.0', + ne_lat: '51.0', + ne_lng: '15.0' + } + end + + it 'returns visits within the specified area' do + get '/api/v1/visits', params: params, headers: auth_headers + + expect(response).to have_http_status(:ok) + json_response = JSON.parse(response.body) + expect(json_response.pluck('id')).to include(visit_inside.id) + expect(json_response.pluck('id')).not_to include(visit1.id, visit2.id) + end + end + end + describe 'PUT /api/v1/visits/:id' do let(:visit) { create(:visit, user:) } @@ -27,13 +85,13 @@ RSpec.describe 'Api::V1::Visits', type: :request do context 'with valid parameters' do it 'updates the requested visit' do - put api_v1_visit_url(visit, api_key:), params: valid_attributes + put "/api/v1/visits/#{visit.id}", params: valid_attributes, headers: auth_headers expect(visit.reload.name).to eq('New name') end it 'renders a JSON response with the visit' do - put api_v1_visit_url(visit, api_key:), params: valid_attributes + put "/api/v1/visits/#{visit.id}", params: valid_attributes, headers: auth_headers expect(response).to have_http_status(:ok) end @@ -41,10 +99,129 @@ RSpec.describe 'Api::V1::Visits', type: :request do context 'with invalid parameters' do it 'renders a JSON response with errors for the visit' do - put api_v1_visit_url(visit, api_key:), params: invalid_attributes + put "/api/v1/visits/#{visit.id}", params: invalid_attributes, headers: auth_headers expect(response).to have_http_status(:unprocessable_entity) end end end + + describe 'POST /api/v1/visits/merge' do + let!(:visit1) { create(:visit, user: user, started_at: 2.days.ago, ended_at: 1.day.ago) } + let!(:visit2) { create(:visit, user: user, started_at: 4.days.ago, ended_at: 3.days.ago) } + let!(:other_user_visit) { create(:visit, user: other_user) } + + context 'with valid parameters' do + let(:valid_merge_params) do + { + visit_ids: [visit1.id, visit2.id] + } + end + + it 'merges the specified visits' do + # Mock the service to avoid dealing with complex merging logic in the test + merge_service = instance_double(Visits::MergeService) + merged_visit = create(:visit, user: user) + + expect(Visits::MergeService).to receive(:new).with(kind_of(ActiveRecord::Relation)).and_return(merge_service) + expect(merge_service).to receive(:call).and_return(merged_visit) + + post '/api/v1/visits/merge', params: valid_merge_params, headers: auth_headers + + expect(response).to have_http_status(:ok) + end + end + + context 'with invalid parameters' do + it 'returns an error when fewer than 2 visits are specified' do + post '/api/v1/visits/merge', params: { visit_ids: [visit1.id] }, headers: auth_headers + + expect(response).to have_http_status(:unprocessable_entity) + json_response = JSON.parse(response.body) + expect(json_response['error']).to include('At least 2 visits must be selected') + end + + it 'returns an error when not all visits are found' do + post '/api/v1/visits/merge', params: { visit_ids: [visit1.id, 999_999] }, headers: auth_headers + + expect(response).to have_http_status(:not_found) + json_response = JSON.parse(response.body) + expect(json_response['error']).to include('not found') + end + + it 'returns an error when trying to merge other user visits' do + post '/api/v1/visits/merge', params: { visit_ids: [visit1.id, other_user_visit.id] }, headers: auth_headers + + expect(response).to have_http_status(:not_found) + json_response = JSON.parse(response.body) + expect(json_response['error']).to include('not found') + end + + it 'returns an error when the merge fails' do + merge_service = instance_double(Visits::MergeService) + + expect(Visits::MergeService).to receive(:new).with(kind_of(ActiveRecord::Relation)).and_return(merge_service) + expect(merge_service).to receive(:call).and_return(nil) + expect(merge_service).to receive(:errors).and_return(['Failed to merge visits']) + + post '/api/v1/visits/merge', params: { visit_ids: [visit1.id, visit2.id] }, headers: auth_headers + + expect(response).to have_http_status(:unprocessable_entity) + json_response = JSON.parse(response.body) + expect(json_response['error']).to include('Failed to merge visits') + end + end + end + + describe 'POST /api/v1/visits/bulk_update' do + let!(:visit1) { create(:visit, user: user, status: 'suggested') } + let!(:visit2) { create(:visit, user: user, status: 'suggested') } + let!(:other_user_visit) { create(:visit, user: other_user, status: 'suggested') } + let(:bulk_update_service) { instance_double(Visits::BulkUpdate) } + + context 'with valid parameters' do + let(:valid_update_params) do + { + visit_ids: [visit1.id, visit2.id], + status: 'confirmed' + } + end + + it 'updates the status of specified visits' do + expect(Visits::BulkUpdate).to receive(:new) + .with(user, kind_of(Array), 'confirmed') + .and_return(bulk_update_service) + expect(bulk_update_service).to receive(:call).and_return({ count: 2 }) + + post '/api/v1/visits/bulk_update', params: valid_update_params, headers: auth_headers + + expect(response).to have_http_status(:ok) + json_response = JSON.parse(response.body) + expect(json_response['updated_count']).to eq(2) + end + end + + context 'with invalid parameters' do + let(:invalid_update_params) do + { + visit_ids: [visit1.id, visit2.id], + status: 'invalid_status' + } + end + + it 'returns an error when the update fails' do + expect(Visits::BulkUpdate).to receive(:new) + .with(user, kind_of(Array), 'invalid_status') + .and_return(bulk_update_service) + expect(bulk_update_service).to receive(:call).and_return(nil) + expect(bulk_update_service).to receive(:errors).and_return(['Invalid status']) + + post '/api/v1/visits/bulk_update', params: invalid_update_params, headers: auth_headers + + expect(response).to have_http_status(:unprocessable_entity) + json_response = JSON.parse(response.body) + expect(json_response['error']).to include('Invalid status') + end + end + end end diff --git a/spec/serializers/api/place_serializer_spec.rb b/spec/serializers/api/place_serializer_spec.rb new file mode 100644 index 00000000..d3f8648e --- /dev/null +++ b/spec/serializers/api/place_serializer_spec.rb @@ -0,0 +1,95 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Api::PlaceSerializer do + describe '#call' do + let(:place) do + instance_double( + Place, + id: 123, + name: 'Central Park', + lon: -73.9665, + lat: 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') + ) + 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[: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[: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, + name: 'Unknown Place', + lon: nil, + lat: nil, + city: nil, + country: nil, + source: nil, + geodata: nil, + reverse_geocoded_at: nil + ) + end + + subject(:serializer_with_nils) { described_class.new(place_with_nils) } + + it 'handles nil values correctly' do + result = serializer_with_nils.call + + expect(result[:id]).to eq(456) + 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[: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 new file mode 100644 index 00000000..bf29e304 --- /dev/null +++ b/spec/serializers/api/visit_serializer_spec.rb @@ -0,0 +1,148 @@ +# frozen_string_literal: true + +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 + + 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 + + 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') + + # 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 + end + end +end diff --git a/spec/services/visits/bulk_update_spec.rb b/spec/services/visits/bulk_update_spec.rb new file mode 100644 index 00000000..92f974d9 --- /dev/null +++ b/spec/services/visits/bulk_update_spec.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Visits::BulkUpdate do + let(:user) { create(:user) } + let(:other_user) { create(:user) } + + let!(:visit1) { create(:visit, user: user, status: 'suggested') } + let!(:visit2) { create(:visit, user: user, status: 'suggested') } + let!(:visit3) { create(:visit, user: user, status: 'confirmed') } + let!(:other_user_visit) { create(:visit, user: other_user, status: 'suggested') } + + describe '#call' do + context 'when all parameters are valid' do + let(:visit_ids) { [visit1.id, visit2.id] } + let(:status) { 'confirmed' } + + subject(:service) { described_class.new(user, visit_ids, status) } + + it 'updates the status of all specified visits' do + result = service.call + + expect(result[:count]).to eq(2) + expect(visit1.reload.status).to eq('confirmed') + expect(visit2.reload.status).to eq('confirmed') + expect(visit3.reload.status).to eq('confirmed') # This one wasn't changed + end + + it 'returns a hash with count and visits' do + result = service.call + + expect(result).to be_a(Hash) + expect(result[:count]).to eq(2) + expect(result[:visits]).to include(visit1, visit2) + expect(result[:visits]).not_to include(visit3, other_user_visit) + end + + it 'does not update visits that belong to other users' do + service.call + + expect(other_user_visit.reload.status).to eq('suggested') + end + end + + context 'when changing to declined status' do + let(:visit_ids) { [visit1.id, visit2.id, visit3.id] } + let(:status) { 'declined' } + + subject(:service) { described_class.new(user, visit_ids, status) } + + it 'updates the status to declined' do + result = service.call + + expect(result[:count]).to eq(3) + expect(visit1.reload.status).to eq('declined') + expect(visit2.reload.status).to eq('declined') + expect(visit3.reload.status).to eq('declined') + end + end + + context 'when visit_ids is empty' do + let(:visit_ids) { [] } + let(:status) { 'confirmed' } + + subject(:service) { described_class.new(user, visit_ids, status) } + + it 'returns false' do + expect(service.call).to be(false) + end + + it 'adds an error' do + service.call + expect(service.errors).to include('No visits selected') + end + + it 'does not update any visits' do + service.call + expect(visit1.reload.status).to eq('suggested') + expect(visit2.reload.status).to eq('suggested') + expect(visit3.reload.status).to eq('confirmed') + end + end + + context 'when visit_ids is nil' do + let(:visit_ids) { nil } + let(:status) { 'confirmed' } + + subject(:service) { described_class.new(user, visit_ids, status) } + + it 'returns false' do + expect(service.call).to be(false) + end + + it 'adds an error' do + service.call + expect(service.errors).to include('No visits selected') + end + end + + context 'when status is invalid' do + let(:visit_ids) { [visit1.id, visit2.id] } + let(:status) { 'invalid_status' } + + subject(:service) { described_class.new(user, visit_ids, status) } + + it 'returns false' do + expect(service.call).to be(false) + end + + it 'adds an error' do + service.call + expect(service.errors).to include('Invalid status') + end + + it 'does not update any visits' do + service.call + expect(visit1.reload.status).to eq('suggested') + expect(visit2.reload.status).to eq('suggested') + end + end + + context 'when no matching visits are found' do + let(:visit_ids) { [999_999, 888_888] } + let(:status) { 'confirmed' } + + subject(:service) { described_class.new(user, visit_ids, status) } + + it 'returns false' do + expect(service.call).to be(false) + end + + it 'adds an error' do + service.call + expect(service.errors).to include('No matching visits found') + end + end + + context 'when some visit IDs do not belong to the user' do + let(:visit_ids) { [visit1.id, other_user_visit.id] } + let(:status) { 'confirmed' } + + subject(:service) { described_class.new(user, visit_ids, status) } + + it 'only updates visits that belong to the user' do + result = service.call + + expect(result[:count]).to eq(1) + expect(visit1.reload.status).to eq('confirmed') + expect(other_user_visit.reload.status).to eq('suggested') + end + end + end +end diff --git a/spec/services/visits/creator_spec.rb b/spec/services/visits/creator_spec.rb index 804e834f..110fe192 100644 --- a/spec/services/visits/creator_spec.rb +++ b/spec/services/visits/creator_spec.rb @@ -27,10 +27,6 @@ RSpec.describe Visits::Creator do context 'when matching an area' do let!(:area) { create(:area, user: user, latitude: 40.7128, longitude: -74.0060, radius: 100) } - before do - allow(subject).to receive(:near_area?).and_return(true) - end - it 'creates a visit associated with the area' do visits = subject.create_visits([visit_data]) @@ -48,16 +44,39 @@ RSpec.describe Visits::Creator do expect(point1.reload.visit_id).to eq(visit.id) expect(point2.reload.visit_id).to eq(visit.id) end + + it 'uses area name for visit name' do + area.update(name: 'Custom Area Name') + visits = subject.create_visits([visit_data]) + + expect(visits.first.name).to eq('Custom Area Name') + end + + it 'does not find areas too far from the visit center' do + far_area = create(:area, user: user, latitude: 41.8781, longitude: -87.6298, radius: 100) # Chicago + + visits = subject.create_visits([visit_data]) + + expect(visits.first.area).to eq(area) # Should match the closer area + expect(visits.first.area).not_to eq(far_area) + end end context 'when matching a place' do - let(:place) { create(:place, latitude: 40.7128, longitude: -74.0060) } + let(:place) { create(:place, name: 'Test Place') } + let(:suggested_place1) { create(:place, name: 'Suggested Place 1') } + let(:suggested_place2) { create(:place, name: 'Suggested Place 2') } let(:place_finder) { instance_double(Visits::PlaceFinder) } + let(:place_data) do + { + main_place: place, + suggested_places: [suggested_place1, suggested_place2] + } + end before do - allow(subject).to receive(:near_area?).and_return(false) allow(Visits::PlaceFinder).to receive(:new).with(user).and_return(place_finder) - allow(place_finder).to receive(:find_or_create_place).and_return(place) + allow(place_finder).to receive(:find_or_create_place).and_return(place_data) end it 'creates a visit associated with the place' do @@ -70,6 +89,170 @@ RSpec.describe Visits::Creator do expect(visit.place).to eq(place) expect(visit.name).to eq(place.name) end + + it 'associates suggested places with the visit' do + visits = subject.create_visits([visit_data]) + visit = visits.first + + # Check for place_visits associations + expect(visit.place_visits.count).to eq(2) + expect(visit.place_visits.pluck(:place_id)).to contain_exactly( + suggested_place1.id, + suggested_place2.id + ) + expect(visit.suggested_places).to contain_exactly(suggested_place1, suggested_place2) + end + + it 'does not create duplicate place_visit associations' do + # Create an existing association + visit = create(:visit, user: user, place: place) + create(:place_visit, visit: visit, place: suggested_place1) + + allow(Visit).to receive(:create!).and_return(visit) + + # Only one new association should be created + expect do + subject.create_visits([visit_data]) + end.to change(PlaceVisit, :count).by(1) + expect(visit.place_visits.pluck(:place_id)).to contain_exactly( + suggested_place1.id, + suggested_place2.id + ) + end + end + + context 'when no area or place is found' do + 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(nil) + end + + it 'uses suggested name from visit data' do + visits = subject.create_visits([visit_data]) + + expect(visits.first.area).to be_nil + expect(visits.first.place).to be_nil + expect(visits.first.name).to eq('Test Place') + end + + it 'uses "Unknown Location" when no name is available' do + visit_data_without_name = visit_data.dup + visit_data_without_name[:suggested_name] = nil + + visits = subject.create_visits([visit_data_without_name]) + + expect(visits.first.name).to eq('Unknown Location') + end + end + + context 'when processing multiple visits' do + let(:place1) { create(:place, name: 'Place 1') } + let(:place2) { create(:place, name: 'Place 2') } + let(:place_finder) { instance_double(Visits::PlaceFinder) } + + let(:visit_data2) do + { + start_time: 3.hours.ago.to_i, + end_time: 2.hours.ago.to_i, + duration: 60.minutes.to_i, + center_lat: 41.8781, + center_lon: -87.6298, + radius: 50, + suggested_name: 'Chicago Visit', + points: [create(:point, user: user), create(:point, user: user)] + } + end + + before do + allow(Visits::PlaceFinder).to receive(:new).with(user).and_return(place_finder) + allow(place_finder).to receive(:find_or_create_place) + .with(visit_data).and_return({ main_place: place1, suggested_places: [] }) + allow(place_finder).to receive(:find_or_create_place) + .with(visit_data2).and_return({ main_place: place2, suggested_places: [] }) + end + + it 'creates multiple visits' do + visits = subject.create_visits([visit_data, visit_data2]) + + expect(visits.size).to eq(2) + expect(visits[0].place).to eq(place1) + expect(visits[0].name).to eq('Place 1') + expect(visits[1].place).to eq(place2) + expect(visits[1].name).to eq('Place 2') + end + end + + context 'when transaction fails' do + 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(nil) + allow(Visit).to receive(:create!).and_raise(ActiveRecord::RecordInvalid) + end + + it 'does not update points if visit creation fails' do + expect do + subject.create_visits([visit_data]) + end.to raise_error(ActiveRecord::RecordInvalid) + + # Points should not be associated with any visit + expect(point1.reload.visit_id).to be_nil + expect(point2.reload.visit_id).to be_nil + end + end + end + + describe '#find_matching_area' do + let(:visit_data) do + { + center_lat: 40.7128, + center_lon: -74.0060, + radius: 50 + } + end + + it 'finds areas within radius' do + area_within = create(:area, user: user, latitude: 40.7129, longitude: -74.0061, radius: 100) + area_outside = create(:area, user: user, latitude: 40.7500, longitude: -74.0500, radius: 100) + + result = subject.send(:find_matching_area, visit_data) + expect(result).to eq(area_within) + end + + it 'returns nil when no areas match' do + create(:area, user: user, latitude: 42.0, longitude: -72.0, radius: 100) + + result = subject.send(:find_matching_area, visit_data) + expect(result).to be_nil + end + + it 'only considers user areas' do + area_other_user = create(:area, latitude: 40.7128, longitude: -74.0060, radius: 100) + area_user = create(:area, user: user, latitude: 40.7128, longitude: -74.0060, radius: 100) + + result = subject.send(:find_matching_area, visit_data) + expect(result).to eq(area_user) + end + end + + describe '#near_area?' do + it 'returns true when point is within area radius' do + area = create(:area, latitude: 40.7128, longitude: -74.0060, radius: 100) + center = [40.7129, -74.0061] # Very close to area center + + result = subject.send(:near_area?, center, area) + expect(result).to be true + end + + it 'returns false when point is outside area radius' do + area = create(:area, latitude: 40.7128, longitude: -74.0060, radius: 100) + center = [40.7500, -74.0500] # Further away + + result = subject.send(:near_area?, center, area) + expect(result).to be false end end end diff --git a/spec/services/visits/detector_spec.rb b/spec/services/visits/detector_spec.rb index 106be3df..bb30f657 100644 --- a/spec/services/visits/detector_spec.rb +++ b/spec/services/visits/detector_spec.rb @@ -3,73 +3,321 @@ require 'rails_helper' RSpec.describe Visits::Detector do - let(:user) { create(:user) } + # Constants from the class to make tests more maintainable + let(:minimum_visit_duration) { described_class::MINIMUM_VISIT_DURATION } + let(:maximum_visit_gap) { described_class::MAXIMUM_VISIT_GAP } + let(:minimum_points_for_visit) { described_class::MINIMUM_POINTS_FOR_VISIT } - # The issue is likely with how we're creating the test points - # Let's make them more realistic with proper spacing in time and location + # Base time for tests + let(:base_time) { Time.zone.now } + + # Create points for a typical visit scenario let(:points) do [ - # First visit - 3 points close together - build_stubbed(:point, lonlat: 'POINT(-74.0060 40.7128)', timestamp: 1.hour.ago.to_i), - build_stubbed(:point, lonlat: 'POINT(-74.0061 40.7129)', timestamp: 50.minutes.ago.to_i), - build_stubbed(:point, lonlat: 'POINT(-74.0062 40.7130)', timestamp: 40.minutes.ago.to_i), + # First visit - multiple points close together + build_stubbed(:point, lonlat: 'POINT(-74.0060 40.7128)', timestamp: (base_time - 1.hour).to_i), + build_stubbed(:point, lonlat: 'POINT(-74.0061 40.7129)', timestamp: (base_time - 50.minutes).to_i), + build_stubbed(:point, lonlat: 'POINT(-74.0062 40.7130)', timestamp: (base_time - 40.minutes).to_i), # Gap in time (> MAXIMUM_VISIT_GAP) # Second visit - different location - build_stubbed(:point, lonlat: 'POINT(-74.0500 40.7500)', timestamp: 10.minutes.ago.to_i), - build_stubbed(:point, lonlat: 'POINT(-74.0501 40.7501)', timestamp: 5.minutes.ago.to_i) + build_stubbed(:point, lonlat: 'POINT(-74.0500 40.7500)', timestamp: (base_time - 10.minutes).to_i), + build_stubbed(:point, lonlat: 'POINT(-74.0501 40.7501)', timestamp: (base_time - 5.minutes).to_i) ] end subject { described_class.new(points) } describe '#detect_potential_visits' do - before do - allow(subject).to receive(:valid_visit?).and_return(true) - allow(subject).to receive(:suggest_place_name).and_return('Test Place') + context 'with valid visit data' do + before do + allow(subject).to receive(:suggest_place_name).and_return('Test Place') + end + + it 'identifies separate visits based on time gaps and location changes' do + visits = subject.detect_potential_visits + + expect(visits.size).to eq(2) + expect(visits.first[:points].size).to eq(3) + expect(visits.last[:points].size).to eq(2) + end + + it 'calculates correct visit properties' do + visits = subject.detect_potential_visits + first_visit = visits.first + + # The center should be the average of the first 3 points + expected_lat = (40.7128 + 40.7129 + 40.7130) / 3 + expected_lon = (-74.0060 + -74.0061 + -74.0062) / 3 + + expect(first_visit[:start_time]).to eq((base_time - 1.hour).to_i) + expect(first_visit[:end_time]).to eq((base_time - 40.minutes).to_i) + expect(first_visit[:duration]).to eq(20.minutes.to_i) + expect(first_visit[:center_lat]).to be_within(0.0001).of(expected_lat) + expect(first_visit[:center_lon]).to be_within(0.0001).of(expected_lon) + expect(first_visit[:radius]).to be > 0 + expect(first_visit[:suggested_name]).to eq('Test Place') + end end - it 'identifies potential visits from points' do - visits = subject.detect_potential_visits - - expect(visits.size).to eq(2) - expect(visits.first[:points].size).to eq(3) - expect(visits.last[:points].size).to eq(2) - end - - it 'calculates visit properties correctly' do - visits = subject.detect_potential_visits - first_visit = visits.first - - # The center should be the average of the first 3 points - expected_lat = (40.7128 + 40.7129 + 40.7130) / 3 - expected_lon = (-74.0060 + -74.0061 + -74.0062) / 3 - - expect(first_visit[:duration]).to be_within(60).of(20.minutes.to_i) - expect(first_visit[:center_lat]).to be_within(0.001).of(expected_lat) - expect(first_visit[:center_lon]).to be_within(0.001).of(expected_lon) - expect(first_visit[:radius]).to be > 0 - expect(first_visit[:suggested_name]).to eq('Test Place') - end - - context 'with insufficient points for a visit' do - let(:points) do + context 'with visits that are too short in duration' do + let(:short_duration_points) do [ - build_stubbed(:point, lonlat: 'POINT(-74.0060 40.7128)', timestamp: 1.hour.ago.to_i), - # Large time gap - build_stubbed(:point, lonlat: 'POINT(-74.0061 40.7129)', timestamp: 10.minutes.ago.to_i) + build_stubbed(:point, lonlat: 'POINT(-74.0060 40.7128)', timestamp: (base_time - 1.hour).to_i), + build_stubbed(:point, lonlat: 'POINT(-74.0061 40.7129)', + timestamp: (base_time - 1.hour + 2.minutes).to_i) ] end - before do - allow(subject).to receive(:valid_visit?).and_call_original + subject { described_class.new(short_duration_points) } + + it 'filters out visits that are too short' do + visits = subject.detect_potential_visits + expect(visits).to be_empty + end + end + + context 'with insufficient points for a visit' do + let(:single_point) do + [ + build_stubbed(:point, lonlat: 'POINT(-74.0060 40.7128)', timestamp: (base_time - 1.hour).to_i) + ] end - it 'does not create a visit' do + subject { described_class.new(single_point) } + + it 'does not create a visit with just one point' do + visits = subject.detect_potential_visits + expect(visits).to be_empty + end + end + + context 'with points that create multiple valid visits' do + let(:multi_visit_points) do + [ + # First visit + build_stubbed(:point, lonlat: 'POINT(-74.0060 40.7128)', timestamp: (base_time - 3.hours).to_i), + build_stubbed(:point, lonlat: 'POINT(-74.0061 40.7129)', timestamp: (base_time - 2.5.hours).to_i), + + # Second visit (different location, after a gap) + build_stubbed(:point, lonlat: 'POINT(-73.9800 40.7600)', timestamp: (base_time - 1.5.hours).to_i), + build_stubbed(:point, lonlat: 'POINT(-73.9801 40.7601)', timestamp: (base_time - 1.hour).to_i), + + # Third visit (another location, after another gap) + build_stubbed(:point, lonlat: 'POINT(-74.0500 40.7500)', timestamp: (base_time - 30.minutes).to_i), + build_stubbed(:point, lonlat: 'POINT(-74.0501 40.7501)', timestamp: (base_time - 20.minutes).to_i) + ] + end + + subject { described_class.new(multi_visit_points) } + + before do + allow(subject).to receive(:suggest_place_name).and_return('Test Place') + end + + it 'correctly identifies all valid visits' do + visits = subject.detect_potential_visits + + expect(visits.size).to eq(3) + expect(visits[0][:points].size).to eq(2) + expect(visits[1][:points].size).to eq(2) + expect(visits[2][:points].size).to eq(2) + end + end + + context 'with points having small time gaps but in same area' do + let(:same_area_points) do + [ + build_stubbed(:point, lonlat: 'POINT(-74.0060 40.7128)', timestamp: (base_time - 1.hour).to_i), + # Small gap (less than MAXIMUM_VISIT_GAP) + build_stubbed(:point, lonlat: 'POINT(-74.0061 40.7129)', + timestamp: (base_time - 1.hour + 25.minutes).to_i), + build_stubbed(:point, lonlat: 'POINT(-74.0062 40.7130)', + timestamp: (base_time - 1.hour + 40.minutes).to_i) + ] + end + + subject { described_class.new(same_area_points) } + + before do + allow(subject).to receive(:suggest_place_name).and_return('Test Place') + end + + it 'groups points into a single visit despite small gaps' do + visits = subject.detect_potential_visits + + expect(visits.size).to eq(1) + expect(visits.first[:points].size).to eq(3) + expect(visits.first[:duration]).to eq(40.minutes.to_i) + end + end + + context 'with no points' do + subject { described_class.new([]) } + + it 'returns an empty array' do visits = subject.detect_potential_visits expect(visits).to be_empty end end end + + describe 'private methods' do + describe '#belongs_to_current_visit?' do + let(:current_visit) do + { + start_time: (base_time - 1.hour).to_i, + end_time: (base_time - 50.minutes).to_i, + center_lat: 40.7128, + center_lon: -74.0060, + points: [] + } + end + + it 'returns true for a point with small time gap and close to center' do + point = build_stubbed(:point, lonlat: 'POINT(-74.0062 40.7130)', + timestamp: (base_time - 45.minutes).to_i) + + result = subject.send(:belongs_to_current_visit?, point, current_visit) + expect(result).to be true + end + + it 'returns false for a point with large time gap' do + point = build_stubbed(:point, lonlat: 'POINT(-74.0062 40.7130)', + timestamp: (base_time - 10.minutes).to_i) + + result = subject.send(:belongs_to_current_visit?, point, current_visit) + expect(result).to be false + end + + it 'returns false for a point far from the center' do + point = build_stubbed(:point, lonlat: 'POINT(-74.0500 40.7500)', + timestamp: (base_time - 49.minutes).to_i) + + result = subject.send(:belongs_to_current_visit?, point, current_visit) + expect(result).to be false + end + end + + describe '#calculate_max_radius' do + it 'returns larger radius for longer visits' do + short_radius = subject.send(:calculate_max_radius, 5.minutes.to_i) + long_radius = subject.send(:calculate_max_radius, 1.hour.to_i) + + expect(long_radius).to be > short_radius + end + + it 'has a minimum radius even for very short visits' do + radius = subject.send(:calculate_max_radius, 1.minute.to_i) + expect(radius).to be > 0 + end + + it 'caps the radius at maximum value' do + radius = subject.send(:calculate_max_radius, 24.hours.to_i) + expect(radius).to be <= 0.5 # Cap at 500 meters + end + end + + describe '#calculate_visit_radius' do + let(:center) { [40.7128, -74.0060] } + let(:test_points) do + [ + build_stubbed(:point, lonlat: 'POINT(-74.0060 40.7128)'), # At center + build_stubbed(:point, lonlat: 'POINT(-74.0070 40.7138)'), # ~100m away + build_stubbed(:point, lonlat: 'POINT(-74.0080 40.7148)') # ~200m away + ] + end + + it 'returns the distance to the furthest point as radius' do + radius = subject.send(:calculate_visit_radius, test_points, center) + + # Approximately 200 meters, but with some tolerance + expect(radius).to be_within(50).of(200) + end + + it 'ensures a minimum radius even with close points' do + close_points = [ + build_stubbed(:point, lonlat: 'POINT(-74.0060 40.7128)'), + build_stubbed(:point, lonlat: 'POINT(-74.0061 40.7129)') + ] + + radius = subject.send(:calculate_visit_radius, close_points, center) + expect(radius).to be >= 15 # Minimum 15 meters + end + end + + describe '#suggest_place_name' do + let(:point_with_geodata) do + build_stubbed(:point, + geodata: { + 'features' => [ + { + 'properties' => { + 'type' => 'restaurant', + 'name' => 'Awesome Pizza', + 'street' => 'Main St', + 'city' => 'New York', + 'state' => 'NY' + } + } + ] + }) + end + + let(:point_with_different_geodata) do + build_stubbed(:point, + geodata: { + 'features' => [ + { + 'properties' => { + 'type' => 'park', + 'name' => 'Central Park', + 'city' => 'New York', + 'state' => 'NY' + } + } + ] + }) + end + + let(:point_without_geodata) do + build_stubbed(:point, geodata: nil) + end + + it 'extracts the most common feature name' do + test_points = [point_with_geodata, point_with_geodata] + name = subject.send(:suggest_place_name, test_points) + + expect(name).to eq('Awesome Pizza, Main St, New York, NY') + end + + it 'returns nil for points without geodata' do + test_points = [point_without_geodata, point_without_geodata] + name = subject.send(:suggest_place_name, test_points) + + expect(name).to be_nil + end + + it 'uses the most common feature type across multiple points' do + restaurant_points = Array.new(3) { point_with_geodata } + park_points = Array.new(2) { point_with_different_geodata } + + test_points = restaurant_points + park_points + name = subject.send(:suggest_place_name, test_points) + + expect(name).to eq('Awesome Pizza, Main St, New York, NY') + end + + it 'handles empty or invalid geodata gracefully' do + point_with_empty_features = build_stubbed(:point, geodata: { 'features' => [] }) + point_with_invalid_geodata = build_stubbed(:point, geodata: { 'invalid' => 'data' }) + + test_points = [point_with_empty_features, point_with_invalid_geodata] + name = subject.send(:suggest_place_name, test_points) + + expect(name).to be_nil + end + end + end end diff --git a/spec/services/visits/find_in_time_spec.rb b/spec/services/visits/find_in_time_spec.rb new file mode 100644 index 00000000..cb2c90e5 --- /dev/null +++ b/spec/services/visits/find_in_time_spec.rb @@ -0,0 +1,142 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Visits::FindInTime do + let(:user) { create(:user) } + let(:other_user) { create(:user) } + let(:place) { create(:place) } + + let(:reference_time) { Time.zone.parse('2023-01-15 12:00:00') } + + let!(:visit1) do + create( + :visit, + user: user, + place: place, + started_at: reference_time, + ended_at: reference_time + 1.hour + ) + end + + let!(:visit2) do + create( + :visit, + user: user, + place: place, + started_at: reference_time + 2.hours, + ended_at: reference_time + 3.hours + ) + end + + # Visit outside range (before) + let!(:visit_before) do + create( + :visit, + user: user, + place: place, + started_at: reference_time - 3.hours, + ended_at: reference_time - 2.hours + ) + end + + # Visit outside range (after) + let!(:visit_after) do + create( + :visit, + user: user, + place: place, + started_at: reference_time + 5.hours, + ended_at: reference_time + 6.hours + ) + end + + # Visit for different user within range + let!(:other_user_visit) do + create( + :visit, + user: other_user, + place: place, + started_at: reference_time + 1.hour, + ended_at: reference_time + 2.hours + ) + end + + describe '#call' do + context 'when given a time range' do + let(:params) do + { + start_at: reference_time.to_s, + end_at: (reference_time + 4.hours).to_s + } + end + + subject(:result) { described_class.new(user, params).call } + + it 'returns visits within the time range' do + expect(result).to include(visit1, visit2) + expect(result).not_to include(visit_before, visit_after) + end + + it 'returns visits in descending order by started_at' do + expect(result.to_a).to eq([visit2, visit1]) + end + + it 'does not include visits from other users' do + expect(result).not_to include(other_user_visit) + end + + it 'preloads the place association' do + expect(result.first.association(:place)).to be_loaded + end + end + + context 'with visits at the boundaries of the time range' do + let!(:visit_at_start) do + create( + :visit, + user: user, + place: place, + started_at: reference_time, + ended_at: reference_time + 30.minutes + ) + end + + let!(:visit_at_end) do + create( + :visit, + user: user, + place: place, + started_at: reference_time + 3.hours + 30.minutes, + ended_at: reference_time + 4.hours + ) + end + + let(:params) do + { + start_at: reference_time.to_s, + end_at: (reference_time + 4.hours).to_s + } + end + + subject(:result) { described_class.new(user, params).call } + + it 'includes visits at the boundaries of the time range' do + expect(result).to include(visit_at_start, visit_at_end) + end + end + + context 'when time parameters are invalid' do + let(:params) do + { + start_at: 'invalid-date', + end_at: (reference_time + 4.hours).to_s + } + end + + it 'raises an ArgumentError' do + expect { described_class.new(user, params).call }.to raise_error(ArgumentError) + end + end + end +end diff --git a/spec/services/visits/find_within_bounding_box_spec.rb b/spec/services/visits/find_within_bounding_box_spec.rb new file mode 100644 index 00000000..db507b17 --- /dev/null +++ b/spec/services/visits/find_within_bounding_box_spec.rb @@ -0,0 +1,161 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Visits::FindWithinBoundingBox do + let(:user) { create(:user) } + let(:other_user) { create(:user) } + + # Define a bounding box for testing + # This creates a box around central Paris + let(:sw_lat) { 48.8534 } # Southwest latitude + let(:sw_lng) { 2.3380 } # Southwest longitude + let(:ne_lat) { 48.8667 } # Northeast latitude + let(:ne_lng) { 2.3580 } # Northeast longitude + + # Create places inside the bounding box + let!(:place_inside_1) do + create(:place, latitude: 48.8600, longitude: 2.3500) # Inside the bounding box + end + + let!(:place_inside_2) do + create(:place, latitude: 48.8580, longitude: 2.3450) # Inside the bounding box + end + + # Create places outside the bounding box + let!(:place_outside_1) do + create(:place, latitude: 48.8700, longitude: 2.3600) # North of the bounding box + end + + let!(:place_outside_2) do + create(:place, latitude: 48.8500, longitude: 2.3300) # Southwest of the bounding box + end + + # Create visits for the test user + let!(:visit_inside_1) do + create( + :visit, + user: user, + place: place_inside_1, + started_at: 2.hours.ago, + ended_at: 1.hour.ago + ) + end + + let!(:visit_inside_2) do + create( + :visit, + user: user, + place: place_inside_2, + started_at: 4.hours.ago, + ended_at: 3.hours.ago + ) + end + + let!(:visit_outside_1) do + create( + :visit, + user: user, + place: place_outside_1, + started_at: 6.hours.ago, + ended_at: 5.hours.ago + ) + end + + let!(:visit_outside_2) do + create( + :visit, + user: user, + place: place_outside_2, + started_at: 8.hours.ago, + ended_at: 7.hours.ago + ) + end + + # Create a visit for another user inside the bounding box + let!(:other_user_visit_inside) do + create( + :visit, + user: other_user, + place: place_inside_1, + started_at: 3.hours.ago, + ended_at: 2.hours.ago + ) + end + + describe '#call' do + let(:params) do + { + sw_lat: sw_lat.to_s, + sw_lng: sw_lng.to_s, + ne_lat: ne_lat.to_s, + ne_lng: ne_lng.to_s + } + end + + subject(:result) { described_class.new(user, params).call } + + it 'returns visits within the specified bounding box' do + expect(result).to include(visit_inside_1, visit_inside_2) + expect(result).not_to include(visit_outside_1, visit_outside_2) + end + + it 'returns visits in descending order by started_at' do + expect(result.to_a).to eq([visit_inside_1, visit_inside_2]) + end + + it 'does not include visits from other users' do + expect(result).not_to include(other_user_visit_inside) + end + + it 'preloads the place association' do + expect(result.first.association(:place)).to be_loaded + end + + context 'with an empty bounding box' do + let(:params) do + { + sw_lat: '0', + sw_lng: '0', + ne_lat: '0', + ne_lng: '0' + } + end + + it 'returns an empty collection' do + expect(result).to be_empty + end + end + + context 'with a very large bounding box' do + let(:params) do + { + sw_lat: '-90', + sw_lng: '-180', + ne_lat: '90', + ne_lng: '180' + } + end + + it 'returns all visits for the user' do + expect(result).to include(visit_inside_1, visit_inside_2, visit_outside_1, visit_outside_2) + expect(result).not_to include(other_user_visit_inside) + end + end + + context 'with string coordinates' do + let(:params) do + { + sw_lat: sw_lat.to_s, + sw_lng: sw_lng.to_s, + ne_lat: ne_lat.to_s, + ne_lng: ne_lng.to_s + } + end + + it 'converts strings to floats' do + expect(result).to include(visit_inside_1, visit_inside_2) + end + end + end +end diff --git a/spec/services/visits/finder_spec.rb b/spec/services/visits/finder_spec.rb new file mode 100644 index 00000000..78164747 --- /dev/null +++ b/spec/services/visits/finder_spec.rb @@ -0,0 +1,154 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Visits::Finder do + let(:user) { create(:user) } + + describe '#call' do + context 'when area selection parameters are provided' do + let(:area_params) do + { + selection: 'true', + sw_lat: '48.8534', + sw_lng: '2.3380', + ne_lat: '48.8667', + ne_lng: '2.3580' + } + end + + it 'delegates to FindWithinBoundingBox service' do + bounding_box_finder = instance_double(Visits::FindWithinBoundingBox) + expect(Visits::FindWithinBoundingBox).to receive(:new) + .with(user, area_params) + .and_return(bounding_box_finder) + + expect(bounding_box_finder).to receive(:call) + + described_class.new(user, area_params).call + end + + it 'does not call FindInTime service' do + expect(Visits::FindWithinBoundingBox).to receive(:new).and_call_original + expect(Visits::FindInTime).not_to receive(:new) + + described_class.new(user, area_params).call + end + end + + context 'when time-based parameters are provided' do + let(:time_params) do + { + start_at: Time.zone.now.beginning_of_day.iso8601, + end_at: Time.zone.now.end_of_day.iso8601 + } + end + + it 'delegates to FindInTime service' do + time_finder = instance_double(Visits::FindInTime) + expect(Visits::FindInTime).to receive(:new) + .with(user, time_params) + .and_return(time_finder) + + expect(time_finder).to receive(:call) + + described_class.new(user, time_params).call + end + + it 'does not call FindWithinBoundingBox service' do + expect(Visits::FindInTime).to receive(:new).and_call_original + expect(Visits::FindWithinBoundingBox).not_to receive(:new) + + described_class.new(user, time_params).call + end + end + + context 'when selection is true but coordinates are missing' do + let(:incomplete_params) do + { + selection: 'true', + sw_lat: '48.8534' + # Missing other coordinates + } + end + + it 'falls back to FindInTime service' do + time_finder = instance_double(Visits::FindInTime) + expect(Visits::FindInTime).to receive(:new) + .with(user, incomplete_params) + .and_return(time_finder) + + expect(time_finder).to receive(:call) + + described_class.new(user, incomplete_params).call + end + end + + context 'when both area and time parameters are provided' do + let(:combined_params) do + { + selection: 'true', + sw_lat: '48.8534', + sw_lng: '2.3380', + ne_lat: '48.8667', + ne_lng: '2.3580', + start_at: Time.zone.now.beginning_of_day.iso8601, + end_at: Time.zone.now.end_of_day.iso8601 + } + end + + it 'prioritizes area search over time search' do + bounding_box_finder = instance_double(Visits::FindWithinBoundingBox) + expect(Visits::FindWithinBoundingBox).to receive(:new) + .with(user, combined_params) + .and_return(bounding_box_finder) + + expect(bounding_box_finder).to receive(:call) + expect(Visits::FindInTime).not_to receive(:new) + + described_class.new(user, combined_params).call + end + end + + context 'when selection is not "true"' do + let(:params) do + { + selection: 'false', # explicitly not true + sw_lat: '48.8534', + sw_lng: '2.3380', + ne_lat: '48.8667', + ne_lng: '2.3580' + } + end + + it 'uses FindInTime service' do + expect(Visits::FindInTime).to receive(:new).and_call_original + expect(Visits::FindWithinBoundingBox).not_to receive(:new) + + described_class.new(user, params).call + end + end + + context 'edge cases' do + context 'with empty params' do + let(:empty_params) { {} } + + it 'uses FindInTime service' do + # We need to handle the ArgumentError from FindInTime when params are empty + expect(Visits::FindInTime).to receive(:new).and_raise(ArgumentError) + expect(Visits::FindWithinBoundingBox).not_to receive(:new) + + expect { described_class.new(user, empty_params).call }.to raise_error(ArgumentError) + end + end + + context 'with nil params' do + let(:nil_params) { nil } + + it 'raises an error' do + expect { described_class.new(user, nil_params).call }.to raise_error(NoMethodError) + end + end + end + end +end diff --git a/spec/services/visits/place_finder_spec.rb b/spec/services/visits/place_finder_spec.rb index 863564b5..ba454684 100644 --- a/spec/services/visits/place_finder_spec.rb +++ b/spec/services/visits/place_finder_spec.rb @@ -4,25 +4,89 @@ require 'rails_helper' RSpec.describe Visits::PlaceFinder do let(:user) { create(:user) } + let(:latitude) { 40.7128 } + let(:longitude) { -74.0060 } subject { described_class.new(user) } describe '#find_or_create_place' do let(:visit_data) do { - center_lat: 40.7128, - center_lon: -74.0060, - suggested_name: 'Test Place' + center_lat: latitude, + center_lon: longitude, + suggested_name: 'Test Place', + points: [] } end context 'when an existing place is found' do - let!(:existing_place) { create(:place, latitude: 40.7128, longitude: -74.0060) } + let!(:existing_place) { create(:place, latitude: latitude, longitude: longitude) } - it 'returns the existing place' do - place = subject.find_or_create_place(visit_data) + it 'returns the existing place as main_place' do + result = subject.find_or_create_place(visit_data) - expect(place).to eq(existing_place) + expect(result).to be_a(Hash) + expect(result[:main_place]).to eq(existing_place) + end + + it 'includes suggested places in the result' do + result = subject.find_or_create_place(visit_data) + + expect(result[:suggested_places]).to be_an(Array) + expect(result[:suggested_places]).to include(existing_place) + end + + it 'finds an existing place by name within search radius' do + similar_named_place = create(:place, + name: 'Test Place', + latitude: latitude + 0.0001, + longitude: longitude + 0.0001) + + # Use the name but slightly different coordinates + modified_visit_data = visit_data.merge( + center_lat: latitude + 0.0002, + center_lon: longitude + 0.0002 + ) + + result = subject.find_or_create_place(modified_visit_data) + + expect(result[:main_place]).to eq(similar_named_place) + end + end + + context 'with places from points data' do + let(:point_with_geodata) do + build_stubbed(:point, + latitude: latitude, + longitude: longitude, + geodata: { + 'properties' => { + 'name' => 'POI from Point', + 'city' => 'New York', + 'country' => 'USA' + } + }) + end + + let(:visit_data_with_points) do + visit_data.merge(points: [point_with_geodata]) + end + + before do + # Mock external API calls to isolate point-based place creation + allow(Geocoder).to receive(:search).and_return([]) + allow(subject).to receive(:fetch_places_from_api).and_return([]) + end + + it 'extracts and creates places from point geodata' do + allow(subject).to receive(:create_place_from_point).and_call_original + + expect do + result = subject.find_or_create_place(visit_data_with_points) + expect(result[:main_place].name).to include('POI from Point') + end.to change(Place, :count).by(1) + + expect(subject).to have_received(:create_place_from_point) end end @@ -37,29 +101,52 @@ RSpec.describe Visits::PlaceFinder do 'country' => 'Test Country' } }, - latitude: 40.7128, - longitude: -74.0060 + latitude: latitude, + longitude: longitude + ) + end + + let(:other_geocoder_result) do + double( + data: { + 'properties' => { + 'name' => 'Other Location', + 'street' => 'Other Street', + 'city' => 'Test City', + 'country' => 'Test Country' + } + }, + latitude: latitude + 0.001, + longitude: longitude + 0.001 ) end before do - allow(Geocoder).to receive(:search).and_return([geocoder_result]) - allow(subject).to receive(:process_nearby_organizations) + allow(Geocoder).to receive(:search).and_return([geocoder_result, other_geocoder_result]) end it 'creates a new place with geocoded data' do + # Main place and other place expect do - subject.find_or_create_place(visit_data) - end.to change(Place, :count).by(1) + result = subject.find_or_create_place(visit_data) + expect(result[:main_place].name).to include('Test Location') + end.to change(Place, :count).by(2) + place = Place.find_by(name: 'Test Location, Test Street') - place = Place.last - - expect(place.name).to include('Test Location') expect(place.city).to eq('Test City') expect(place.country).to eq('Test Country') expect(place.source).to eq('photon') end + it 'returns both main place and suggested places' do + result = subject.find_or_create_place(visit_data) + + expect(result[:main_place].name).to include('Test Location') + expect(result[:suggested_places].length).to eq(2) + expect(result[:suggested_places].map(&:name)).to include('Test Location, Test Street', + 'Other Location, Other Street') + end + context 'when geocoding returns no results' do before do allow(Geocoder).to receive(:search).and_return([]) @@ -67,14 +154,120 @@ RSpec.describe Visits::PlaceFinder do it 'creates a place with the suggested name' do expect do - subject.find_or_create_place(visit_data) + result = subject.find_or_create_place(visit_data) + expect(result[:main_place].name).to eq('Test Place') end.to change(Place, :count).by(1) place = Place.last - expect(place.name).to eq('Test Place') expect(place.source).to eq('manual') end + + it 'returns the created place as both main and the only suggested place' do + result = subject.find_or_create_place(visit_data) + + expect(result[:main_place].name).to eq('Test Place') + expect(result[:suggested_places]).to eq([result[:main_place]]) + end + + it 'falls back to default name when suggested name is missing' do + visit_data_without_name = visit_data.merge(suggested_name: nil) + + result = subject.find_or_create_place(visit_data_without_name) + + expect(result[:main_place].name).to eq(Place::DEFAULT_NAME) + end + end + end + + context 'with multiple potential places' do + let!(:place1) { create(:place, name: 'Place 1', latitude: latitude, longitude: longitude) } + let!(:place2) { create(:place, name: 'Place 2', latitude: latitude + 0.0005, longitude: longitude + 0.0005) } + let!(:place3) { create(:place, name: 'Place 3', latitude: latitude + 0.001, longitude: longitude + 0.001) } + + it 'selects the closest place as main_place' do + result = subject.find_or_create_place(visit_data) + + expect(result[:main_place]).to eq(place1) + end + + it 'includes nearby places as suggested_places' do + result = subject.find_or_create_place(visit_data) + + expect(result[:suggested_places]).to include(place1, place2) + # place3 might be outside the search radius depending on the constants defined + end + + it 'deduplicates places by name' do + # Create a duplicate place with the same name + create(:place, name: 'Place 1', latitude: latitude + 0.0002, longitude: longitude + 0.0002) + + result = subject.find_or_create_place(visit_data) + + names = result[:suggested_places].map(&:name) + expect(names.count('Place 1')).to eq(1) + end + end + + context 'with API place creation failures' do + let(:invalid_geocoder_result) do + double( + data: { + 'properties' => { + # Missing required fields + } + }, + latitude: latitude, + longitude: longitude + ) + end + + before do + allow(Geocoder).to receive(:search).and_return([invalid_geocoder_result]) + end + + it 'gracefully handles errors in place creation' do + allow(subject).to receive(:create_place_from_api_result).and_call_original + + result = subject.find_or_create_place(visit_data) + + # Should create the default place + expect(result[:main_place].name).to eq('Test Place') + expect(result[:main_place].source).to eq('manual') + end + end + end + + describe 'private methods' do + context '#build_place_name' do + it 'combines name components correctly' do + properties = { + 'name' => 'Coffee Shop', + 'street' => 'Main St', + 'housenumber' => '123', + 'city' => 'New York' + } + + name = subject.send(:build_place_name, properties) + expect(name).to eq('Coffee Shop, Main St, 123, New York') + end + + it 'removes duplicate components' do + properties = { + 'name' => 'Coffee Shop', + 'street' => 'Coffee Shop', # Duplicate of name + 'city' => 'New York' + } + + name = subject.send(:build_place_name, properties) + expect(name).to eq('Coffee Shop, New York') + end + + it 'returns default name when no components are available' do + properties = { 'other' => 'irrelevant' } + + name = subject.send(:build_place_name, properties) + expect(name).to eq(Place::DEFAULT_NAME) end end end diff --git a/spec/services/visits/time_chunks_spec.rb b/spec/services/visits/time_chunks_spec.rb new file mode 100644 index 00000000..4a5b7f9f --- /dev/null +++ b/spec/services/visits/time_chunks_spec.rb @@ -0,0 +1,161 @@ +# frozen_string_literal: true + +require 'rails_helper' + +RSpec.describe Visits::TimeChunks do + describe '#call' do + context 'with a multi-year span' do + it 'splits time correctly across year boundaries' do + # Span over multiple years + start_at = DateTime.new(2020, 6, 15) + end_at = DateTime.new(2023, 3, 10) + + service = described_class.new(start_at: start_at, end_at: end_at) + chunks = service.call + + # Should have 4 chunks: + # 1. 2020-06-15 to 2021-01-01 + # 2. 2021-01-01 to 2022-01-01 + # 3. 2022-01-01 to 2023-01-01 + # 4. 2023-01-01 to 2023-03-10 + expect(chunks.size).to eq(4) + + # First chunk: partial year (Jun 15 - Jan 1) + expect(chunks[0].begin).to eq(start_at) + expect(chunks[0].end).to eq(DateTime.new(2020, 12, 31).end_of_day) + + # Second chunk: full year 2021 + expect(chunks[1].begin).to eq(DateTime.new(2021, 1, 1).beginning_of_year) + expect(chunks[1].end).to eq(DateTime.new(2021, 12, 31).end_of_year) + + # Third chunk: full year 2022 + expect(chunks[2].begin).to eq(DateTime.new(2022, 1, 1).beginning_of_year) + expect(chunks[2].end).to eq(DateTime.new(2022, 12, 31).end_of_year) + + # Fourth chunk: partial year (Jan 1 - Mar 10, 2023) + expect(chunks[3].begin).to eq(DateTime.new(2023, 1, 1).beginning_of_year) + expect(chunks[3].end).to eq(end_at) + end + end + + context 'with a span within a single year' do + it 'creates a single chunk ending at year end' do + start_at = DateTime.new(2020, 3, 15) + end_at = DateTime.new(2020, 10, 20) + + service = described_class.new(start_at: start_at, end_at: end_at) + chunks = service.call + + expect(chunks.size).to eq(1) + expect(chunks[0].begin).to eq(start_at) + # The implementation appears to extend to the end of the year + expect(chunks[0].end).to eq(DateTime.new(2020, 12, 31).end_of_day) + end + end + + context 'with spans exactly on year boundaries' do + it 'creates one chunk per year ending at next year start' do + start_at = DateTime.new(2020, 1, 1) + end_at = DateTime.new(2022, 12, 31).end_of_day + + service = described_class.new(start_at: start_at, end_at: end_at) + chunks = service.call + + expect(chunks.size).to eq(3) + + # Three full years, each ending at the start of the next year + expect(chunks[0].begin).to eq(DateTime.new(2020, 1, 1).beginning_of_year) + expect(chunks[0].end).to eq(DateTime.new(2020, 12, 31).end_of_year) + + expect(chunks[1].begin).to eq(DateTime.new(2021, 1, 1).beginning_of_year) + expect(chunks[1].end).to eq(DateTime.new(2021, 12, 31).end_of_year) + + expect(chunks[2].begin).to eq(DateTime.new(2022, 1, 1).beginning_of_year) + expect(chunks[2].end).to eq(DateTime.new(2022, 12, 31).end_of_year) + end + end + + context 'with start and end dates in the same day' do + it 'returns a single chunk ending at the end of the year' do + date = DateTime.new(2020, 5, 15) + start_at = date.beginning_of_day + end_at = date.end_of_day + + service = described_class.new(start_at: start_at, end_at: end_at) + chunks = service.call + + expect(chunks.size).to eq(1) + expect(chunks[0].begin).to eq(start_at) + # Implementation extends to end of year + expect(chunks[0].end).to eq(DateTime.new(2020, 12, 31).end_of_day) + end + end + + context 'with a full single year' do + it 'returns a single chunk for the entire year' do + start_at = DateTime.new(2020, 1, 1).beginning_of_day + end_at = DateTime.new(2020, 12, 31).end_of_day + + service = described_class.new(start_at: start_at, end_at: end_at) + chunks = service.call + + expect(chunks.size).to eq(1) + expect(chunks[0].begin).to eq(start_at) + expect(chunks[0].end).to eq(end_at) + end + end + + context 'with dates spanning a decade' do + it 'creates appropriate chunks for each year ending at next year start' do + start_at = DateTime.new(2020, 1, 1) + end_at = DateTime.new(2030, 12, 31) + + service = described_class.new(start_at: start_at, end_at: end_at) + chunks = service.call + + # Should have 11 chunks (2020 through 2030) + expect(chunks.size).to eq(11) + + # Check first and last chunks + expect(chunks.first.begin).to eq(start_at) + expect(chunks.last.end).to eq(end_at) + + # Check that each chunk starts on Jan 1 and ends on next Jan 1 (except last) + (1...chunks.size - 1).each do |i| + year = 2020 + i + expect(chunks[i].begin).to eq(DateTime.new(year, 1, 1).beginning_of_year) + expect(chunks[i].end).to eq(DateTime.new(year, 12, 31).end_of_year) + end + end + end + + context 'with start date after end date' do + it 'still creates a chunk for start date year' do + start_at = DateTime.new(2023, 1, 1) + end_at = DateTime.new(2020, 1, 1) + + service = described_class.new(start_at: start_at, end_at: end_at) + chunks = service.call + + # The implementation creates one chunk for the start date year + expect(chunks.size).to eq(1) + expect(chunks[0].begin).to eq(start_at) + expect(chunks[0].end).to eq(DateTime.new(2023, 12, 31).end_of_day) + end + end + + context 'when start date equals end date' do + it 'returns a single chunk extending to year end' do + date = DateTime.new(2022, 6, 15, 12, 30) + + service = described_class.new(start_at: date, end_at: date) + chunks = service.call + + expect(chunks.size).to eq(1) + expect(chunks[0].begin).to eq(date) + # Implementation extends to end of year + expect(chunks[0].end).to eq(DateTime.new(2022, 12, 31).end_of_day) + end + end + end +end