diff --git a/app/controllers/api/v1/places_controller.rb b/app/controllers/api/v1/places_controller.rb index 65322cf2..5675ea7f 100644 --- a/app/controllers/api/v1/places_controller.rb +++ b/app/controllers/api/v1/places_controller.rb @@ -18,10 +18,11 @@ module Api end def create - @place = current_api_user.places.build(place_params) + @place = current_api_user.places.build(place_params.except(:tag_ids)) if @place.save add_tags if tag_ids.present? + @place.reload # Reload to get updated associations render json: serialize_place(@place), status: :created else render json: { errors: @place.errors.full_messages }, status: :unprocessable_entity @@ -65,11 +66,13 @@ module Api end def place_params - params.require(:place).permit(:name, :latitude, :longitude, :source, :note) + params.require(:place).permit(:name, :latitude, :longitude, :source, :note, tag_ids: []) end def tag_ids - params.dig(:place, :tag_ids) || [] + # tag_ids can be in params[:place][:tag_ids] or params[:tag_ids] + ids = params.dig(:place, :tag_ids) || params[:tag_ids] + Array(ids).compact end def add_tags diff --git a/db/migrate/20251116134506_add_user_id_to_places.rb b/db/migrate/20251116184506_add_user_id_to_places.rb similarity index 100% rename from db/migrate/20251116134506_add_user_id_to_places.rb rename to db/migrate/20251116184506_add_user_id_to_places.rb diff --git a/db/migrate/20251116134514_create_tags.rb b/db/migrate/20251116184514_create_tags.rb similarity index 100% rename from db/migrate/20251116134514_create_tags.rb rename to db/migrate/20251116184514_create_tags.rb diff --git a/db/migrate/20251116134520_create_taggings.rb b/db/migrate/20251116184520_create_taggings.rb similarity index 100% rename from db/migrate/20251116134520_create_taggings.rb rename to db/migrate/20251116184520_create_taggings.rb diff --git a/spec/factories/places.rb b/spec/factories/places.rb index 19263b8e..303f1f48 100644 --- a/spec/factories/places.rb +++ b/spec/factories/places.rb @@ -41,6 +41,26 @@ FactoryBot.define do end end + # Trait for setting coordinates from lonlat geometry + # This is forward-compatible for when latitude/longitude are deprecated + trait :from_lonlat do + transient do + lonlat_wkt { nil } + end + + after(:build) do |place, evaluator| + if evaluator.lonlat_wkt + # Parse WKT to extract coordinates + # Format: "POINT(longitude latitude)" or "SRID=4326;POINT(longitude latitude)" + coords = evaluator.lonlat_wkt.match(/POINT\(([^ ]+) ([^ ]+)\)/) + if coords + place.longitude = coords[1].to_f + place.latitude = coords[2].to_f + end + end + end + end + # Special trait for testing with nil lonlat trait :without_lonlat do # Skip validation to create an invalid record for testing diff --git a/spec/jobs/data_migrations/migrate_places_lonlat_job_spec.rb b/spec/jobs/data_migrations/migrate_places_lonlat_job_spec.rb index 5e31b377..ea0074c2 100644 --- a/spec/jobs/data_migrations/migrate_places_lonlat_job_spec.rb +++ b/spec/jobs/data_migrations/migrate_places_lonlat_job_spec.rb @@ -7,15 +7,22 @@ RSpec.describe DataMigrations::MigratePlacesLonlatJob, type: :job do let(:user) { create(:user) } context 'when places exist for the user' do - let!(:place1) { create(:place, :without_lonlat, longitude: 10.0, latitude: 20.0) } - let!(:place2) { create(:place, :without_lonlat, longitude: -73.935242, latitude: 40.730610) } - let!(:other_place) { create(:place, :without_lonlat, longitude: 15.0, latitude: 25.0) } + let!(:place1) { create(:place, user: user, longitude: 10.0, latitude: 20.0) } + let!(:place2) { create(:place, user: user, longitude: -73.935242, latitude: 40.730610) } + let!(:other_place) { create(:place, longitude: 15.0, latitude: 25.0) } # Create visits to associate places with users let!(:visit1) { create(:visit, user: user, place: place1) } let!(:visit2) { create(:visit, user: user, place: place2) } let!(:other_visit) { create(:visit, place: other_place) } # associated with a different user + # Simulate old data by clearing lonlat after creation (to test migration) + before do + place1.update_column(:lonlat, nil) + place2.update_column(:lonlat, nil) + other_place.update_column(:lonlat, nil) + end + it 'updates lonlat field for all places belonging to the user' do # Force a reload to ensure we have the initial state place1.reload diff --git a/spec/services/reverse_geocoding/places/fetch_data_spec.rb b/spec/services/reverse_geocoding/places/fetch_data_spec.rb index 12e7497d..c95562f2 100644 --- a/spec/services/reverse_geocoding/places/fetch_data_spec.rb +++ b/spec/services/reverse_geocoding/places/fetch_data_spec.rb @@ -591,7 +591,7 @@ RSpec.describe ReverseGeocoding::Places::FetchData do end context 'when lonlat is already present on existing place' do - let!(:existing_place) { create(:place, :with_geodata, lonlat: 'POINT(10.0 50.0)') } + let!(:existing_place) { create(:place, :with_geodata, latitude: 50.0, longitude: 10.0) } let(:existing_data) do double( data: { @@ -600,7 +600,9 @@ RSpec.describe ReverseGeocoding::Places::FetchData do 'osm_id' => existing_place.geodata.dig('properties', 'osm_id'), 'name' => 'Updated Name' } - } + }, + latitude: 55.0, + longitude: 15.0 ) end @@ -608,11 +610,14 @@ RSpec.describe ReverseGeocoding::Places::FetchData do allow(Geocoder).to receive(:search).and_return([mock_geocoded_place, existing_data]) end - it 'does not override existing lonlat' do + it 'does not override existing coordinates when updating geodata' do service.call existing_place.reload + # lonlat is auto-generated from latitude/longitude, so it should remain based on original coordinates expect(existing_place.lonlat.to_s).to eq('POINT (10.0 50.0)') + expect(existing_place.latitude).to eq(50.0) + expect(existing_place.longitude).to eq(10.0) end end end diff --git a/spec/services/users/export_data/places_spec.rb b/spec/services/users/export_data/places_spec.rb index a940db16..31abcf83 100644 --- a/spec/services/users/export_data/places_spec.rb +++ b/spec/services/users/export_data/places_spec.rb @@ -16,8 +16,8 @@ RSpec.describe Users::ExportData::Places, type: :service do end context 'when user has places' do - let!(:place1) { create(:place, name: 'Home', longitude: -74.0059, latitude: 40.7128) } - let!(:place2) { create(:place, name: 'Office', longitude: -73.9851, latitude: 40.7589) } + let!(:place1) { create(:place, user: user, name: 'Home', longitude: -74.0059, latitude: 40.7128) } + let!(:place2) { create(:place, user: user, name: 'Office', longitude: -73.9851, latitude: 40.7589) } let!(:visit1) { create(:visit, user: user, place: place1) } let!(:visit2) { create(:visit, user: user, place: place2) } diff --git a/spec/services/users/export_import_integration_spec.rb b/spec/services/users/export_import_integration_spec.rb index 2be18ee7..ead92687 100644 --- a/spec/services/users/export_import_integration_spec.rb +++ b/spec/services/users/export_import_integration_spec.rb @@ -128,9 +128,9 @@ RSpec.describe 'Users Export-Import Integration', type: :service do original_user = create(:user, email: 'original@example.com') # Create places with different characteristics - home_place = create(:place, name: 'Home', latitude: 40.7128, longitude: -74.0060) - office_place = create(:place, name: 'Office', latitude: 40.7589, longitude: -73.9851) - gym_place = create(:place, name: 'Gym', latitude: 40.7505, longitude: -73.9934) + home_place = create(:place, user: original_user, name: 'Home', latitude: 40.7128, longitude: -74.0060) + office_place = create(:place, user: original_user, name: 'Office', latitude: 40.7589, longitude: -73.9851) + gym_place = create(:place, user: original_user, name: 'Gym', latitude: 40.7505, longitude: -73.9934) # Create visits associated with those places create(:visit, user: original_user, place: home_place, name: 'Home Visit') diff --git a/spec/services/visits/creator_spec.rb b/spec/services/visits/creator_spec.rb index dad0bef0..fd048d1b 100644 --- a/spec/services/visits/creator_spec.rb +++ b/spec/services/visits/creator_spec.rb @@ -25,7 +25,7 @@ RSpec.describe Visits::Creator do end context 'when a confirmed visit already exists at the same location' do - let(:place) { create(:place, lonlat: 'POINT(-74.0060 40.7128)', name: 'Existing Place') } + let(:place) { create(:place, user: user, latitude: 40.7128, longitude: -74.0060, name: 'Existing Place') } let!(:existing_visit) do create( :visit, @@ -61,7 +61,7 @@ RSpec.describe Visits::Creator do end context 'when a confirmed visit exists but at a different location' do - let(:different_place) { create(:place, lonlat: 'POINT(-73.9000 41.0000)', name: 'Different Place') } + let(:different_place) { create(:place, user: user, latitude: 41.0000, longitude: -73.9000, name: 'Different Place') } let!(:existing_visit) do create( :visit, diff --git a/spec/support/map_layer_helpers.rb b/spec/support/map_layer_helpers.rb deleted file mode 100644 index b29a8957..00000000 --- a/spec/support/map_layer_helpers.rb +++ /dev/null @@ -1,70 +0,0 @@ -# frozen_string_literal: true - -module MapLayerHelpers - OVERLAY_LAYERS = [ - 'Points', - 'Routes', - 'Fog of War', - 'Heatmap', - 'Scratch map', - 'Areas', - 'Photos', - 'Suggested Visits', - 'Confirmed Visits' - ].freeze - - def test_layer_toggle(layer_name) - within('.leaflet-control-layers-expanded') do - if page.has_content?(layer_name) - # Find the label that contains the layer name, then find its associated checkbox - layer_label = find('label', text: layer_name) - layer_checkbox = layer_label.find('input[type="checkbox"]', visible: false) - - # Get initial state - initial_checked = layer_checkbox.checked? - - # Toggle the layer by clicking the label (more reliable) - layer_label.click - sleep 0.5 # Small delay for layer toggle - - # Verify state changed - expect(layer_checkbox.checked?).not_to eq(initial_checked) - - # Toggle back - layer_label.click - sleep 0.5 # Small delay for layer toggle - - # Verify state returned to original - expect(layer_checkbox.checked?).to eq(initial_checked) - end - end - end - - def test_base_layer_switching - within('.leaflet-control-layers-expanded') do - # Check that we have base layer options (radio buttons) - expect(page).to have_css('input[type="radio"]') - - # Verify OpenStreetMap is available - expect(page).to have_content('OpenStreetMap') - - # Test clicking different radio buttons if available - radio_buttons = all('input[type="radio"]', visible: false) - expect(radio_buttons.length).to be >= 1 - - # Click the first radio button to test layer switching - if radio_buttons.length > 1 - radio_buttons[1].click - sleep 1 - - # Click back to the first one - radio_buttons[0].click - sleep 1 - end - end - end -end - -RSpec.configure do |config| - config.include MapLayerHelpers, type: :system -end diff --git a/spec/support/point_helpers.rb b/spec/support/point_helpers.rb deleted file mode 100644 index 3e6b45c7..00000000 --- a/spec/support/point_helpers.rb +++ /dev/null @@ -1,20 +0,0 @@ -# frozen_string_literal: true - -module PointHelpers - # Creates a list of points spaced ~100m apart northwards - def create_points_around(user:, count:, base_lat: 20.0, base_lon: 10.0, timestamp: nil, **attrs) - Array.new(count) do |i| - create( - :point, - user: user, - timestamp: (timestamp.respond_to?(:call) ? timestamp.call(i) : timestamp) || (Time.current - i.minutes).to_i, - lonlat: "POINT(#{base_lon} #{base_lat + i * 0.0009})", - **attrs - ) - end - end -end - -RSpec.configure do |config| - config.include PointHelpers -end diff --git a/spec/support/polyline_popup_helpers.rb b/spec/support/polyline_popup_helpers.rb deleted file mode 100644 index 47716f1f..00000000 --- a/spec/support/polyline_popup_helpers.rb +++ /dev/null @@ -1,150 +0,0 @@ -# frozen_string_literal: true - -module PolylinePopupHelpers - def trigger_polyline_hover_and_get_popup - # Wait for polylines to be fully loaded - expect(page).to have_css('.leaflet-overlay-pane', wait: 10) - sleep 2 # Allow time for polylines to render - - # Try multiple approaches to trigger polyline hover - popup_content = try_canvas_hover || try_polyline_click || try_map_interaction - - popup_content - end - - def verify_popup_content_structure(popup_content, distance_unit) - return false unless popup_content - - # Check for required fields in popup - required_fields = [ - 'Start:', - 'End:', - 'Duration:', - 'Total Distance:', - 'Current Speed:' - ] - - # Check that all required fields are present - fields_present = required_fields.all? { |field| popup_content.include?(field) } - - # Check distance unit in Total Distance field - distance_unit_present = popup_content.include?(distance_unit == 'km' ? 'km' : 'mi') - - # Check speed unit in Current Speed field (should match distance unit) - speed_unit_present = if distance_unit == 'mi' - popup_content.include?('mph') - else - popup_content.include?('km/h') - end - - fields_present && distance_unit_present && speed_unit_present - end - - def extract_popup_data(popup_content) - return {} unless popup_content - - data = {} - - # Extract start time - if match = popup_content.match(/Start:<\/strong>\s*([^<]+)/) - data[:start] = match[1].strip - end - - # Extract end time - if match = popup_content.match(/End:<\/strong>\s*([^<]+)/) - data[:end] = match[1].strip - end - - # Extract duration - if match = popup_content.match(/Duration:<\/strong>\s*([^<]+)/) - data[:duration] = match[1].strip - end - - # Extract total distance - if match = popup_content.match(/Total Distance:<\/strong>\s*([^<]+)/) - data[:total_distance] = match[1].strip - end - - # Extract current speed - if match = popup_content.match(/Current Speed:<\/strong>\s*([^<]+)/) - data[:current_speed] = match[1].strip - end - - data - end - - private - - def try_canvas_hover - page.evaluate_script(<<~JS) - return new Promise((resolve) => { - const polylinesPane = document.querySelector('.leaflet-polylinesPane-pane'); - if (polylinesPane) { - const canvas = polylinesPane.querySelector('canvas'); - if (canvas) { - const rect = canvas.getBoundingClientRect(); - const centerX = rect.left + rect.width / 2; - const centerY = rect.top + rect.height / 2; - - const event = new MouseEvent('mouseover', { - bubbles: true, - cancelable: true, - clientX: centerX, - clientY: centerY - }); - - canvas.dispatchEvent(event); - - setTimeout(() => { - const popup = document.querySelector('.leaflet-popup-content'); - resolve(popup ? popup.innerHTML : null); - }, 1000); - } else { - resolve(null); - } - } else { - resolve(null); - } - }); - JS - rescue => e - Rails.logger.debug "Canvas hover failed: #{e.message}" - nil - end - - def try_polyline_click - # Try to find and click on polyline elements directly - if page.has_css?('path[stroke]', wait: 2) - polyline = first('path[stroke]') - polyline.click if polyline - sleep 1 - - if page.has_css?('.leaflet-popup-content') - return find('.leaflet-popup-content').native.inner_html - end - end - nil - rescue => e - Rails.logger.debug "Polyline click failed: #{e.message}" - nil - end - - def try_map_interaction - # As a fallback, click in the center of the map - map_element = find('.leaflet-container') - map_element.click - sleep 1 - - if page.has_css?('.leaflet-popup-content', wait: 2) - return find('.leaflet-popup-content').native.inner_html - end - nil - rescue => e - Rails.logger.debug "Map interaction failed: #{e.message}" - nil - end -end - -RSpec.configure do |config| - config.include PolylinePopupHelpers, type: :system -end diff --git a/spec/support/shared_examples/map_examples.rb b/spec/support/shared_examples/map_examples.rb deleted file mode 100644 index b99fd00a..00000000 --- a/spec/support/shared_examples/map_examples.rb +++ /dev/null @@ -1,132 +0,0 @@ -# frozen_string_literal: true - -RSpec.shared_context 'authenticated map user' do - before do - sign_in_and_visit_map(user) - end -end - -RSpec.shared_examples 'map basic functionality' do - it 'displays the leaflet map with basic elements' do - expect(page).to have_css('#map') - expect(page).to have_css('.leaflet-map-pane') - expect(page).to have_css('.leaflet-tile-pane') - end - - it 'loads map data and displays route information' do - expect(page).to have_css('.leaflet-overlay-pane', wait: 10) - expect(page).to have_css('[data-maps-target="container"]') - end -end - -RSpec.shared_examples 'map controls' do - it 'has zoom controls' do - expect(page).to have_css('.leaflet-control-zoom') - expect(page).to have_css('.leaflet-control-zoom-in') - expect(page).to have_css('.leaflet-control-zoom-out') - end - - it 'has layer control' do - expect(page).to have_css('.leaflet-control-layers', wait: 10) - end - - it 'has scale control' do - expect(page).to have_css('.leaflet-control-scale') - expect(page).to have_css('.leaflet-control-scale-line') - end - - it 'has stats control' do - expect(page).to have_css('.leaflet-control-stats', wait: 10) - end - - it 'has attribution control' do - expect(page).to have_css('.leaflet-control-attribution') - end -end - -RSpec.shared_examples 'expandable layer control' do - let(:layer_control) { find('.leaflet-control-layers') } - - def expand_layer_control - if page.has_css?('.leaflet-control-layers-toggle', visible: true) - find('.leaflet-control-layers-toggle').click - else - layer_control.click - end - expect(page).to have_css('.leaflet-control-layers-expanded', wait: 5) - end - - def collapse_layer_control - if page.has_css?('.leaflet-control-layers-toggle', visible: true) - find('.leaflet-control-layers-toggle').click - else - find('.leaflet-container').click - end - sleep 1 - expect(page).not_to have_css('.leaflet-control-layers-expanded') - end -end - -RSpec.shared_examples 'polyline popup content' do |distance_unit| - it "displays correct popup content with #{distance_unit} units" do - # Wait for polylines to load - expect(page).to have_css('.leaflet-overlay-pane', wait: 10) - sleep 2 # Allow polylines to fully render - - # Find and hover over a polyline to trigger popup - # We need to use JavaScript to trigger the mouseover event on polylines - popup_content = page.evaluate_script(<<~JS) - // Find the first polyline group and trigger mouseover - const polylinesPane = document.querySelector('.leaflet-polylinesPane-pane'); - if (polylinesPane) { - const canvas = polylinesPane.querySelector('canvas'); - if (canvas) { - // Create a mouseover event at the center of the canvas - const rect = canvas.getBoundingClientRect(); - const centerX = rect.left + rect.width / 2; - const centerY = rect.top + rect.height / 2; - - const event = new MouseEvent('mouseover', { - bubbles: true, - cancelable: true, - clientX: centerX, - clientY: centerY - }); - - canvas.dispatchEvent(event); - - // Wait a moment for popup to appear - setTimeout(() => { - const popup = document.querySelector('.leaflet-popup-content'); - return popup ? popup.innerHTML : null; - }, 500); - } - } - return null; - JS - - # Alternative approach: try to click on the map area where polylines should be - if popup_content.nil? - # Click in the center of the map to potentially trigger polyline interaction - map_element = find('.leaflet-container') - map_element.click - sleep 1 - - # Try to find any popup that might have appeared - if page.has_css?('.leaflet-popup-content', wait: 2) - popup_content = find('.leaflet-popup-content').text - end - end - - # If we still don't have popup content, let's verify the polylines exist and are interactive - expect(page).to have_css('.leaflet-overlay-pane') - - # Check that the map has the expected data attributes for distance unit - map_element = find('#map') - expect(map_element['data-user_settings']).to include("maps") - - # Verify the user settings contain the expected distance unit - user_settings = JSON.parse(map_element['data-user_settings']) - expect(user_settings.dig('maps', 'distance_unit')).to eq(distance_unit) - end -end diff --git a/spec/support/system_helpers.rb b/spec/support/system_helpers.rb deleted file mode 100644 index 9418e8b6..00000000 --- a/spec/support/system_helpers.rb +++ /dev/null @@ -1,24 +0,0 @@ -# frozen_string_literal: true - -module SystemHelpers - include Rails.application.routes.url_helpers - - def sign_in_user(user, password = 'password123') - visit '/users/sign_in' - expect(page).to have_field('Email', wait: 10) - fill_in 'Email', with: user.email - fill_in 'Password', with: password - click_button 'Log in' - end - - def sign_in_and_visit_map(user, password = 'password123') - sign_in_user(user, password) - expect(page).to have_current_path('/map') - expect(page).to have_css('.leaflet-container', wait: 10) - end -end - -RSpec.configure do |config| - config.include SystemHelpers, type: :system - config.include Rails.application.routes.url_helpers, type: :system -end