diff --git a/app/controllers/api/v1/locations_controller.rb b/app/controllers/api/v1/locations_controller.rb index ac64c34e..1245186c 100644 --- a/app/controllers/api/v1/locations_controller.rb +++ b/app/controllers/api/v1/locations_controller.rb @@ -5,11 +5,12 @@ class Api::V1::LocationsController < ApiController before_action :validate_suggestion_params, only: [:suggestions] def index - if search_query.present? || coordinate_search? + if coordinate_search? search_results = LocationSearch::PointFinder.new(current_api_user, search_params).call + render json: Api::LocationSearchResultSerializer.new(search_results).call else - render json: { error: 'Search query parameter (q) or coordinates (lat, lon) are required' }, status: :bad_request + render json: { error: 'Coordinates (lat, lon) are required' }, status: :bad_request end rescue StandardError => e Rails.logger.error "Location search error: #{e.message}" @@ -48,11 +49,8 @@ class Api::V1::LocationsController < ApiController def search_params { - query: search_query, latitude: params[:lat]&.to_f, longitude: params[:lon]&.to_f, - name: params[:name], - address: params[:address], limit: params[:limit]&.to_i || 50, date_from: parse_date(params[:date_from]), date_to: parse_date(params[:date_to]), diff --git a/app/services/location_search/geocoding_service.rb b/app/services/location_search/geocoding_service.rb index 5aa208af..9bf29c2b 100644 --- a/app/services/location_search/geocoding_service.rb +++ b/app/services/location_search/geocoding_service.rb @@ -78,7 +78,7 @@ module LocationSearch results.each do |result| # Check if there's already a result within 100m duplicate = deduplicated.find do |existing| - distance = calculate_distance( + distance = calculate_distance_in_meters( result[:lat], result[:lon], existing[:lat], existing[:lon] ) @@ -91,22 +91,18 @@ module LocationSearch deduplicated end - def calculate_distance(lat1, lon1, lat2, lon2) - # Haversine formula for distance calculation in meters - rad_per_deg = Math::PI / 180 - rkm = 6_371_000 # Earth radius in meters - - dlat_rad = (lat2 - lat1) * rad_per_deg - dlon_rad = (lon2 - lon1) * rad_per_deg - - lat1_rad = lat1 * rad_per_deg - lat2_rad = lat2 * rad_per_deg - - a = Math.sin(dlat_rad / 2)**2 + Math.cos(lat1_rad) * Math.cos(lat2_rad) * Math.sin(dlon_rad / 2)**2 - c = 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a)) - - rkm * c + def calculate_distance_in_meters(lat1, lon1, lat2, lon2) + # Use Geocoder's distance calculation (same as in Distanceable concern) + distance_km = Geocoder::Calculations.distance_between( + [lat1, lon1], + [lat2, lon2], + units: :km + ) + + # Convert to meters and handle potential nil/invalid results + return 0 unless distance_km.is_a?(Numeric) && distance_km.finite? + + distance_km * 1000 # Convert km to meters end - end end diff --git a/app/services/location_search/point_finder.rb b/app/services/location_search/point_finder.rb index 71092483..d46e07ae 100644 --- a/app/services/location_search/point_finder.rb +++ b/app/services/location_search/point_finder.rb @@ -4,11 +4,8 @@ module LocationSearch class PointFinder def initialize(user, params = {}) @user = user - @query = params[:query] @latitude = params[:latitude] @longitude = params[:longitude] - @name = params[:name] || 'Selected Location' - @address = params[:address] || '' @limit = params[:limit] || 50 @date_from = params[:date_from] @date_to = params[:date_to] @@ -18,8 +15,6 @@ module LocationSearch def call if coordinate_search? return coordinate_based_search - elsif @query.present? - return text_based_search else return empty_result end @@ -32,40 +27,15 @@ module LocationSearch end def coordinate_based_search - Rails.logger.info "LocationSearch: Coordinate-based search at [#{@latitude}, #{@longitude}] for '#{@name}'" - - # Create a single location object with the provided coordinates location = { lat: @latitude, lon: @longitude, - name: @name, - address: @address, type: 'coordinate_search' } find_matching_points([location]) end - def text_based_search - return empty_result if @query.blank? - - geocoded_locations = LocationSearch::GeocodingService.new(@query).search - - # Debug: Log geocoding results - Rails.logger.info "LocationSearch: Geocoding '#{@query}' returned #{geocoded_locations.length} locations" - geocoded_locations.each_with_index do |loc, idx| - Rails.logger.info "LocationSearch: [#{idx}] #{loc[:name]} at [#{loc[:lat]}, #{loc[:lon]}] - #{loc[:address]}" - end - - return empty_result if geocoded_locations.empty? - - find_matching_points(geocoded_locations) - end - - def geocoding_service - @geocoding_service ||= LocationSearch::GeocodingService.new(@query || '') - end - def find_matching_points(geocoded_locations) results = [] @@ -74,7 +44,7 @@ module LocationSearch Rails.logger.info "LocationSearch: Searching for points near #{location[:name]} at [#{location[:lat]}, #{location[:lon]}]" search_radius = @radius_override || determine_search_radius(location[:type]) - + matching_points = spatial_matcher.find_points_near( @user, location[:lat], @@ -113,14 +83,9 @@ module LocationSearch end { - query: @query, locations: results, total_locations: results.length, - search_metadata: { - geocoding_provider: geocoding_service.provider_name, - candidates_found: geocoded_locations.length, - search_time_ms: nil # TODO: implement timing - } + search_metadata: {} } end @@ -160,11 +125,9 @@ module LocationSearch def empty_result { - query: @query, locations: [], total_locations: 0, search_metadata: { - geocoding_provider: nil, candidates_found: 0, search_time_ms: 0 } diff --git a/spec/services/location_search/point_finder_spec.rb b/spec/services/location_search/point_finder_spec.rb index d2d5a992..14750916 100644 --- a/spec/services/location_search/point_finder_spec.rb +++ b/spec/services/location_search/point_finder_spec.rb @@ -5,22 +5,10 @@ require 'rails_helper' RSpec.describe LocationSearch::PointFinder do let(:user) { create(:user) } let(:service) { described_class.new(user, search_params) } - let(:search_params) { { query: 'Kaufland' } } + let(:search_params) { { latitude: 52.5200, longitude: 13.4050 } } describe '#call' do - context 'with valid search query' do - let(:mock_geocoded_locations) do - [ - { - lat: 52.5200, - lon: 13.4050, - name: 'Kaufland Mitte', - address: 'Alexanderplatz 1, Berlin', - type: 'shop' - } - ] - end - + context 'with valid coordinates' do let(:mock_matching_points) do [ { @@ -47,10 +35,6 @@ RSpec.describe LocationSearch::PointFinder do end before do - allow(LocationSearch::GeocodingService).to receive(:new).and_return( - double('GeocodingService', search: mock_geocoded_locations, provider_name: 'Test Provider') - ) - allow_any_instance_of(LocationSearch::SpatialMatcher) .to receive(:find_points_near).and_return(mock_matching_points) @@ -61,75 +45,25 @@ RSpec.describe LocationSearch::PointFinder do it 'returns search results with location data' do result = service.call - expect(result[:query]).to eq('Kaufland') expect(result[:locations]).to be_an(Array) expect(result[:locations].first).to include( - place_name: 'Kaufland Mitte', coordinates: [52.5200, 13.4050], - address: 'Alexanderplatz 1, Berlin', total_visits: 1 ) end - it 'includes search metadata' do - result = service.call - - expect(result[:search_metadata]).to include( - :geocoding_provider, - :candidates_found, - :search_time_ms - ) - expect(result[:search_metadata][:candidates_found]).to eq(1) - end - - it 'calls geocoding service with the query' do - expect(LocationSearch::GeocodingService) - .to receive(:new).with('Kaufland') - .and_return(double('GeocodingService', search: mock_geocoded_locations, provider_name: 'Test Provider')) - - service.call - end - - it 'calls spatial matcher with correct parameters' do + it 'calls spatial matcher with correct coordinates and radius' do expect_any_instance_of(LocationSearch::SpatialMatcher) .to receive(:find_points_near) - .with(user, 52.5200, 13.4050, 75, { date_from: nil, date_to: nil }) + .with(user, 52.5200, 13.4050, 500, { date_from: nil, date_to: nil }) service.call end - it 'determines appropriate search radius for shop type' do - expect_any_instance_of(LocationSearch::SpatialMatcher) - .to receive(:find_points_near) - .with(user, anything, anything, 75, anything) - - service.call - end - - context 'with different place types' do - it 'uses smaller radius for street addresses' do - mock_geocoded_locations[0][:type] = 'street' - - expect_any_instance_of(LocationSearch::SpatialMatcher) - .to receive(:find_points_near) - .with(user, anything, anything, 50, anything) - - service.call - end - - it 'uses larger radius for neighborhoods' do - mock_geocoded_locations[0][:type] = 'neighborhood' - - expect_any_instance_of(LocationSearch::SpatialMatcher) - .to receive(:find_points_near) - .with(user, anything, anything, 300, anything) - - service.call - end - + context 'with custom radius override' do + let(:search_params) { { latitude: 52.5200, longitude: 13.4050, radius_override: 150 } } + it 'uses custom radius when override provided' do - service = described_class.new(user, search_params.merge(radius_override: 150)) - expect_any_instance_of(LocationSearch::SpatialMatcher) .to receive(:find_points_near) .with(user, anything, anything, 150, anything) @@ -141,7 +75,8 @@ RSpec.describe LocationSearch::PointFinder do context 'with date filtering' do let(:search_params) do { - query: 'Kaufland', + latitude: 52.5200, + longitude: 13.4050, date_from: Date.parse('2024-01-01'), date_to: Date.parse('2024-03-31') } @@ -158,29 +93,31 @@ RSpec.describe LocationSearch::PointFinder do service.call end end - end - context 'when no geocoding results found' do - before do - allow(LocationSearch::GeocodingService).to receive(:new).and_return( - double('GeocodingService', search: [], provider_name: 'Test Provider') - ) - end + context 'with limit parameter' do + let(:search_params) { { latitude: 52.5200, longitude: 13.4050, limit: 10 } } + let(:many_visits) { Array.new(15) { |i| { timestamp: i, date: "2024-01-#{i+1}T12:00:00Z" } } } - it 'returns empty result' do - result = service.call + before do + allow_any_instance_of(LocationSearch::SpatialMatcher) + .to receive(:find_points_near).and_return([{}]) + + allow_any_instance_of(LocationSearch::ResultAggregator) + .to receive(:group_points_into_visits).and_return(many_visits) + end - expect(result[:locations]).to be_empty - expect(result[:total_locations]).to eq(0) + it 'limits the number of visits returned' do + result = service.call + + expect(result[:locations].first[:visits].length).to eq(10) + end end end context 'when no matching points found' do + let(:search_params) { { latitude: 52.5200, longitude: 13.4050 } } + before do - allow(LocationSearch::GeocodingService).to receive(:new).and_return( - double('GeocodingService', search: [{ lat: 52.5200, lon: 13.4050, name: 'Test' }], provider_name: 'Test Provider') - ) - allow_any_instance_of(LocationSearch::SpatialMatcher) .to receive(:find_points_near).and_return([]) end @@ -193,11 +130,11 @@ RSpec.describe LocationSearch::PointFinder do end end - context 'with blank query' do - let(:search_params) { { query: '' } } + context 'when coordinates are missing' do + let(:search_params) { {} } it 'returns empty result without calling services' do - expect(LocationSearch::GeocodingService).not_to receive(:new) + expect(LocationSearch::SpatialMatcher).not_to receive(:new) result = service.call @@ -205,26 +142,23 @@ RSpec.describe LocationSearch::PointFinder do end end - context 'with limit parameter' do - let(:search_params) { { query: 'Kaufland', limit: 10 } } - let(:many_visits) { Array.new(15) { |i| { timestamp: i, date: "2024-01-#{i+1}T12:00:00Z" } } } + context 'when only latitude is provided' do + let(:search_params) { { latitude: 52.5200 } } - before do - allow(LocationSearch::GeocodingService).to receive(:new).and_return( - double('GeocodingService', search: [{ lat: 52.5200, lon: 13.4050, name: 'Test' }], provider_name: 'Test Provider') - ) - - allow_any_instance_of(LocationSearch::SpatialMatcher) - .to receive(:find_points_near).and_return([{}]) - - allow_any_instance_of(LocationSearch::ResultAggregator) - .to receive(:group_points_into_visits).and_return(many_visits) - end - - it 'limits the number of visits returned' do + it 'returns empty result' do result = service.call + + expect(result[:locations]).to be_empty + end + end - expect(result[:locations].first[:visits].length).to eq(10) + context 'when only longitude is provided' do + let(:search_params) { { longitude: 13.4050 } } + + it 'returns empty result' do + result = service.call + + expect(result[:locations]).to be_empty end end end