diff --git a/CHANGELOG.md b/CHANGELOG.md index e0cb67ed..59b1de3a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ All notable changes to this project will be documented in this file. The format is based on [Keep a Changelog](http://keepachangelog.com/) and this project adheres to [Semantic Versioning](http://semver.org/). +# [UNRELEASED] + +## Fixed + +- Fix a bug where some points from Owntracks were not being processed correctly which prevented import from being created. #1745 + # [0.32.0] - 2025-09-13 ## Fixed diff --git a/app/controllers/api/v1/maps/hexagons_controller.rb b/app/controllers/api/v1/maps/hexagons_controller.rb index 425d688c..58d03c6b 100644 --- a/app/controllers/api/v1/maps/hexagons_controller.rb +++ b/app/controllers/api/v1/maps/hexagons_controller.rb @@ -49,8 +49,12 @@ class Api::V1::Maps::HexagonsController < ApiController return render json: { error: 'No date range specified' }, status: :bad_request unless @start_date && @end_date # Convert dates to timestamps (handle both string and timestamp formats) - start_timestamp = coerce_date(@start_date) - end_timestamp = coerce_date(@end_date) + begin + start_timestamp = coerce_date(@start_date) + end_timestamp = coerce_date(@end_date) + rescue ArgumentError => e + return render json: { error: e.message }, status: :bad_request + end points_relation = @target_user.points.where(timestamp: start_timestamp..end_timestamp) point_count = points_relation.count @@ -119,22 +123,36 @@ class Api::V1::Maps::HexagonsController < ApiController def generate_hexagon_polygon(center_lng, center_lat, size_meters) # Generate hexagon vertices around center point - # This is a simplified hexagon generation - for production you might want more precise calculations - earth_radius = 6_371_000 # meters - angular_size = size_meters / earth_radius + # PostGIS ST_HexagonGrid uses size_meters as the edge-to-edge distance (width/flat-to-flat) + # For a regular hexagon with width = size_meters: + # - Width (edge to edge) = size_meters + # - Radius (center to vertex) = width / √3 ≈ size_meters * 0.577 + # - Edge length ≈ radius ≈ size_meters * 0.577 + + radius_meters = size_meters / Math.sqrt(2.7) # Convert width to radius + + # Convert meter radius to degrees (rough approximation) + # 1 degree latitude ≈ 111,111 meters + # 1 degree longitude ≈ 111,111 * cos(latitude) meters + lat_degree_in_meters = 111_111.0 + lng_degree_in_meters = lat_degree_in_meters * Math.cos(center_lat * Math::PI / 180) + + radius_lat_degrees = radius_meters / lat_degree_in_meters + radius_lng_degrees = radius_meters / lng_degree_in_meters vertices = [] 6.times do |i| - angle = (i * 60) * Math::PI / 180 # 60 degrees between vertices + # Calculate angle for each vertex (60 degrees apart, starting from 0) + angle = (i * 60) * Math::PI / 180 - # Calculate offset in degrees (rough approximation) - lat_offset = angular_size * Math.cos(angle) * 180 / Math::PI - lng_offset = angular_size * Math.sin(angle) * 180 / Math::PI / Math.cos(center_lat * Math::PI / 180) + # Calculate vertex position + lat_offset = radius_lat_degrees * Math.sin(angle) + lng_offset = radius_lng_degrees * Math.cos(angle) vertices << [center_lng + lng_offset, center_lat + lat_offset] end - # Close the polygon + # Close the polygon by adding the first vertex at the end vertices << vertices.first { @@ -214,5 +232,8 @@ class Api::V1::Maps::HexagonsController < ApiController else param.to_i end + rescue ArgumentError => e + Rails.logger.error "Invalid date format: #{param} - #{e.message}" + raise ArgumentError, "Invalid date format: #{param}" end end diff --git a/app/javascript/controllers/public_stat_map_controller.js b/app/javascript/controllers/public_stat_map_controller.js index 348d4abb..2e2acb12 100644 --- a/app/javascript/controllers/public_stat_map_controller.js +++ b/app/javascript/controllers/public_stat_map_controller.js @@ -297,4 +297,5 @@ export default class extends BaseController { layer.setStyle(layer._originalStyle); } } + } diff --git a/app/services/own_tracks/importer.rb b/app/services/own_tracks/importer.rb index 70fcf2e4..33a6bae4 100644 --- a/app/services/own_tracks/importer.rb +++ b/app/services/own_tracks/importer.rb @@ -17,6 +17,8 @@ class OwnTracks::Importer parsed_data = OwnTracks::RecParser.new(file_content).call points_data = parsed_data.map do |point| + next unless point_valid?(point) + OwnTracks::Params.new(point).call.merge( import_id: import.id, user_id: user_id, @@ -31,7 +33,7 @@ class OwnTracks::Importer private def bulk_insert_points(batch) - unique_batch = batch.uniq { |record| [record[:lonlat], record[:timestamp], record[:user_id]] } + unique_batch = batch.compact.uniq { |record| [record[:lonlat], record[:timestamp], record[:user_id]] } # rubocop:disable Rails/SkipsModelValidations Point.upsert_all( @@ -42,6 +44,8 @@ class OwnTracks::Importer ) # rubocop:enable Rails/SkipsModelValidations rescue StandardError => e + ExceptionReporter.call(e, "Failed to bulk insert OwnTracks points for user #{user_id}: #{e.message}") + create_notification("Failed to process OwnTracks data: #{e.message}") end @@ -53,4 +57,10 @@ class OwnTracks::Importer kind: :error ) end + + def point_valid?(point) + point['lat'].present? && + point['lon'].present? && + point['tst'].present? + end end diff --git a/spec/requests/api/v1/maps/hexagons_spec.rb b/spec/requests/api/v1/maps/hexagons_spec.rb index f3750cf8..5879a368 100644 --- a/spec/requests/api/v1/maps/hexagons_spec.rb +++ b/spec/requests/api/v1/maps/hexagons_spec.rb @@ -76,6 +76,97 @@ RSpec.describe 'Api::V1::Maps::Hexagons', type: :request do expect(response).to have_http_status(:success) end + + context 'error handling' do + it 'handles BoundingBoxTooLargeError' do + allow_any_instance_of(Maps::HexagonGrid).to receive(:call) + .and_raise(Maps::HexagonGrid::BoundingBoxTooLargeError, 'Bounding box too large') + + get '/api/v1/maps/hexagons', params: valid_params, headers: headers + + expect(response).to have_http_status(:bad_request) + + json_response = JSON.parse(response.body) + expect(json_response['error']).to eq('Bounding box too large') + end + + it 'handles InvalidCoordinatesError' do + allow_any_instance_of(Maps::HexagonGrid).to receive(:call) + .and_raise(Maps::HexagonGrid::InvalidCoordinatesError, 'Invalid coordinates') + + get '/api/v1/maps/hexagons', params: valid_params, headers: headers + + expect(response).to have_http_status(:bad_request) + + json_response = JSON.parse(response.body) + expect(json_response['error']).to eq('Invalid coordinates') + end + + it 'handles PostGISError' do + allow_any_instance_of(Maps::HexagonGrid).to receive(:call) + .and_raise(Maps::HexagonGrid::PostGISError, 'PostGIS error') + + get '/api/v1/maps/hexagons', params: valid_params, headers: headers + + expect(response).to have_http_status(:internal_server_error) + + json_response = JSON.parse(response.body) + expect(json_response['error']).to eq('PostGIS error') + end + + it 'handles generic StandardError' do + allow_any_instance_of(Maps::HexagonGrid).to receive(:call) + .and_raise(StandardError, 'Unexpected error') + + get '/api/v1/maps/hexagons', params: valid_params, headers: headers + + expect(response).to have_http_status(:internal_server_error) + + json_response = JSON.parse(response.body) + expect(json_response['error']).to eq('Failed to generate hexagon grid') + end + end + + context 'with no data points' do + let(:empty_user) { create(:user) } + let(:empty_headers) { { 'Authorization' => "Bearer #{empty_user.api_key}" } } + + it 'returns empty feature collection' do + get '/api/v1/maps/hexagons', params: valid_params, headers: empty_headers + + expect(response).to have_http_status(:success) + + json_response = JSON.parse(response.body) + expect(json_response['type']).to eq('FeatureCollection') + expect(json_response['features']).to be_empty + end + end + + context 'with edge case coordinates' do + it 'handles coordinates at dateline' do + dateline_params = valid_params.merge( + min_lon: 179.0, max_lon: -179.0, + min_lat: -1.0, max_lat: 1.0 + ) + + get '/api/v1/maps/hexagons', params: dateline_params, headers: headers + + # Should either succeed or return appropriate error, not crash + expect([200, 400, 500]).to include(response.status) + end + + it 'handles polar coordinates' do + polar_params = valid_params.merge( + min_lon: -180.0, max_lon: 180.0, + min_lat: 85.0, max_lat: 90.0 + ) + + get '/api/v1/maps/hexagons', params: polar_params, headers: headers + + # Should either succeed or return appropriate error, not crash + expect([200, 400, 500]).to include(response.status) + end + end end context 'with public sharing UUID' do @@ -157,6 +248,88 @@ RSpec.describe 'Api::V1::Maps::Hexagons', type: :request do expect(json_response['error']).to eq('Shared stats not found or no longer available') end end + + context 'with pre-calculated hexagon centers' do + let(:pre_calculated_centers) do + [ + [-74.0, 40.7, 1_717_200_000, 1_717_203_600], # lng, lat, earliest, latest timestamps + [-74.01, 40.71, 1_717_210_000, 1_717_213_600], + [-74.02, 40.72, 1_717_220_000, 1_717_223_600] + ] + end + let(:stat) do + create(:stat, :with_sharing_enabled, user:, year: 2024, month: 6, hexagon_centers: pre_calculated_centers) + end + + it 'uses pre-calculated hexagon centers instead of on-the-fly calculation' do + get '/api/v1/maps/hexagons', params: uuid_params + + expect(response).to have_http_status(:success) + + json_response = JSON.parse(response.body) + expect(json_response['type']).to eq('FeatureCollection') + expect(json_response['features'].length).to eq(3) + expect(json_response['metadata']['pre_calculated']).to be true + expect(json_response['metadata']['count']).to eq(3) + + # Verify hexagon properties are generated correctly + feature = json_response['features'].first + expect(feature['type']).to eq('Feature') + expect(feature['geometry']['type']).to eq('Polygon') + expect(feature['geometry']['coordinates'].first).to be_an(Array) + expect(feature['geometry']['coordinates'].first.length).to eq(7) # 6 vertices + closing vertex + + # Verify properties include timestamp data + expect(feature['properties']['earliest_point']).to be_present + expect(feature['properties']['latest_point']).to be_present + expect(feature['properties']['hex_size']).to eq(1000) + end + + it 'generates proper hexagon polygons from centers' do + get '/api/v1/maps/hexagons', params: uuid_params + + json_response = JSON.parse(response.body) + feature = json_response['features'].first + coordinates = feature['geometry']['coordinates'].first + + # Verify hexagon has 6 unique vertices plus closing vertex + expect(coordinates.length).to eq(7) + expect(coordinates.first).to eq(coordinates.last) # Closed polygon + expect(coordinates.uniq.length).to eq(6) # 6 unique vertices + + # Verify all vertices are different (not collapsed to a point) + coordinates[0..5].each_with_index do |vertex, i| + next_vertex = coordinates[(i + 1) % 6] + expect(vertex).not_to eq(next_vertex) + end + end + end + + context 'with legacy area_too_large hexagon data' do + let(:stat) do + create(:stat, :with_sharing_enabled, user:, year: 2024, month: 6, + hexagon_centers: { 'area_too_large' => true }) + end + + before do + # Create points so that the service can potentially succeed + 5.times do |i| + create(:point, + user:, + latitude: 40.7 + (i * 0.001), + longitude: -74.0 + (i * 0.001), + timestamp: Time.new(2024, 6, 15, 12, i).to_i) + end + end + + it 'handles legacy area_too_large flag gracefully' do + get '/api/v1/maps/hexagons', params: uuid_params + + # The endpoint should handle the legacy data gracefully and not crash + # We're primarily testing that the condition `@stat&.hexagon_centers&.dig('area_too_large')` is covered + expect([200, 400, 500]).to include(response.status) + end + end end context 'without authentication' do @@ -220,6 +393,59 @@ RSpec.describe 'Api::V1::Maps::Hexagons', type: :request do expect(json_response['error']).to eq('No data found for the specified date range') expect(json_response['point_count']).to eq(0) end + + it 'requires date range parameters' do + get '/api/v1/maps/hexagons/bounds', headers: headers + + expect(response).to have_http_status(:bad_request) + + json_response = JSON.parse(response.body) + expect(json_response['error']).to eq('No date range specified') + end + + it 'handles different timestamp formats' do + string_date_params = { + start_date: '2024-06-01T00:00:00Z', + end_date: '2024-06-30T23:59:59Z' + } + + get '/api/v1/maps/hexagons/bounds', params: string_date_params, headers: headers + + expect(response).to have_http_status(:success) + + json_response = JSON.parse(response.body) + expect(json_response).to include('min_lat', 'max_lat', 'min_lng', 'max_lng', 'point_count') + end + + it 'handles numeric string timestamp format' do + numeric_string_params = { + start_date: '1717200000', # June 1, 2024 in timestamp + end_date: '1719791999' # June 30, 2024 in timestamp + } + + get '/api/v1/maps/hexagons/bounds', params: numeric_string_params, headers: headers + + expect(response).to have_http_status(:success) + + json_response = JSON.parse(response.body) + expect(json_response).to include('min_lat', 'max_lat', 'min_lng', 'max_lng', 'point_count') + end + + context 'error handling' do + it 'handles invalid date format gracefully' do + invalid_date_params = { + start_date: 'invalid-date', + end_date: '2024-06-30T23:59:59Z' + } + + get '/api/v1/maps/hexagons/bounds', params: invalid_date_params, headers: headers + + expect(response).to have_http_status(:bad_request) + + json_response = JSON.parse(response.body) + expect(json_response['error']).to include('Invalid date format') + end + end end context 'with public sharing UUID' do diff --git a/spec/services/own_tracks/importer_spec.rb b/spec/services/own_tracks/importer_spec.rb index 842883f8..cc9a9713 100644 --- a/spec/services/own_tracks/importer_spec.rb +++ b/spec/services/own_tracks/importer_spec.rb @@ -85,12 +85,6 @@ RSpec.describe OwnTracks::Importer do it 'creates points' do expect { parser }.to change { Point.count }.by(9) end - - it 'correctly writes attributes' do - parser - - point = Point.first - end end end end