From d518603719145c99c36552b3582d0e7fb4a54125 Mon Sep 17 00:00:00 2001 From: Eugene Burmakin Date: Wed, 2 Jul 2025 20:22:40 +0200 Subject: [PATCH] Update importing process --- .../controllers/imports_controller.js | 5 + app/services/imports/broadcaster.rb | 16 ++- app/services/imports/create.rb | 9 +- app/services/users/import_data.rb | 41 ++++++- app/services/users/import_data/places.rb | 29 +++-- app/services/users/import_data/trips.rb | 30 +++-- app/services/users/import_data/visits.rb | 65 +++++++++-- app/views/devise/registrations/edit.html.erb | 5 +- app/views/imports/index.html.erb | 2 +- db/schema.rb | 16 --- .../users/export_import_integration_spec.rb | 106 +++++++++++++----- .../services/users/import_data/places_spec.rb | 42 +++++-- 12 files changed, 276 insertions(+), 90 deletions(-) diff --git a/app/javascript/controllers/imports_controller.js b/app/javascript/controllers/imports_controller.js index d39455a0..4a864074 100644 --- a/app/javascript/controllers/imports_controller.js +++ b/app/javascript/controllers/imports_controller.js @@ -31,6 +31,11 @@ export default class extends BaseController { if (pointsCell) { pointsCell.textContent = new Intl.NumberFormat().format(data.import.points_count); } + + const statusCell = row.querySelector('[data-status-display]'); + if (statusCell && data.import.status) { + statusCell.textContent = data.import.status; + } } } } diff --git a/app/services/imports/broadcaster.rb b/app/services/imports/broadcaster.rb index 1c7f54bb..ead96546 100644 --- a/app/services/imports/broadcaster.rb +++ b/app/services/imports/broadcaster.rb @@ -8,7 +8,21 @@ module Imports::Broadcaster action: 'update', import: { id: import.id, - points_count: index + points_count: index, + status: import.status + } + } + ) + end + + def broadcast_status_update + ImportsChannel.broadcast_to( + import.user, + { + action: 'status_update', + import: { + id: import.id, + status: import.status } } ) diff --git a/app/services/imports/create.rb b/app/services/imports/create.rb index b2056663..d86fe337 100644 --- a/app/services/imports/create.rb +++ b/app/services/imports/create.rb @@ -1,6 +1,8 @@ # frozen_string_literal: true class Imports::Create + include Imports::Broadcaster + attr_reader :user, :import def initialize(user, import) @@ -10,6 +12,7 @@ class Imports::Create def call import.update!(status: :processing) + broadcast_status_update importer(import.source).new(import, user.id).call @@ -18,10 +21,14 @@ class Imports::Create update_import_points_count(import) rescue StandardError => e import.update!(status: :failed) + broadcast_status_update create_import_failed_notification(import, user, e) ensure - import.update!(status: :completed) if import.processing? + if import.processing? + import.update!(status: :completed) + broadcast_status_update + end end private diff --git a/app/services/users/import_data.rb b/app/services/users/import_data.rb index 820b37ce..664c27cc 100644 --- a/app/services/users/import_data.rb +++ b/app/services/users/import_data.rb @@ -103,15 +103,28 @@ class Users::ImportData import_settings(data['settings']) if data['settings'] import_areas(data['areas']) if data['areas'] - import_places(data['places']) if data['places'] + + # Import places first to ensure they're available for visits + places_imported = import_places(data['places']) if data['places'] + Rails.logger.info "Places import phase completed: #{places_imported} places imported" + import_imports(data['imports']) if data['imports'] import_exports(data['exports']) if data['exports'] import_trips(data['trips']) if data['trips'] import_stats(data['stats']) if data['stats'] import_notifications(data['notifications']) if data['notifications'] - import_visits(data['visits']) if data['visits'] + + # Import visits after places to ensure proper place resolution + visits_imported = import_visits(data['visits']) if data['visits'] + Rails.logger.info "Visits import phase completed: #{visits_imported} visits imported" + import_points(data['points']) if data['points'] + # Final validation check + if data['counts'] + validate_import_completeness(data['counts']) + end + Rails.logger.info "Data import completed. Stats: #{@import_stats}" end @@ -131,6 +144,7 @@ class Users::ImportData Rails.logger.debug "Importing #{places_data&.size || 0} places" places_created = Users::ImportData::Places.new(user, places_data).call @import_stats[:places_created] = places_created + places_created end def import_imports(imports_data) @@ -169,6 +183,7 @@ class Users::ImportData Rails.logger.debug "Importing #{visits_data&.size || 0} visits" visits_created = Users::ImportData::Visits.new(user, visits_data).call @import_stats[:visits_created] = visits_created + visits_created end def import_points(points_data) @@ -221,4 +236,26 @@ class Users::ImportData kind: :error ).call end + + def validate_import_completeness(expected_counts) + Rails.logger.info "Validating import completeness..." + + discrepancies = [] + + expected_counts.each do |entity, expected_count| + actual_count = @import_stats[:"#{entity}_created"] || 0 + + if actual_count < expected_count + discrepancy = "#{entity}: expected #{expected_count}, got #{actual_count} (#{expected_count - actual_count} missing)" + discrepancies << discrepancy + Rails.logger.warn "Import discrepancy - #{discrepancy}" + end + end + + if discrepancies.any? + Rails.logger.warn "Import completed with discrepancies: #{discrepancies.join(', ')}" + else + Rails.logger.info "Import validation successful - all entities imported correctly" + end + end end diff --git a/app/services/users/import_data/places.rb b/app/services/users/import_data/places.rb index 370c9119..6d4ed023 100644 --- a/app/services/users/import_data/places.rb +++ b/app/services/users/import_data/places.rb @@ -16,7 +16,7 @@ class Users::ImportData::Places places_data.each do |place_data| next unless place_data.is_a?(Hash) - place = find_or_create_place(place_data) + place = find_or_create_place_for_import(place_data) places_created += 1 if place&.respond_to?(:previously_new_record?) && place.previously_new_record? end @@ -28,7 +28,7 @@ class Users::ImportData::Places attr_reader :user, :places_data - def find_or_create_place(place_data) + def find_or_create_place_for_import(place_data) name = place_data['name'] latitude = place_data['latitude']&.to_f longitude = place_data['longitude']&.to_f @@ -38,33 +38,42 @@ class Users::ImportData::Places return nil end - existing_place = Place.find_by(name: name) + Rails.logger.debug "Processing place for import: #{name} at (#{latitude}, #{longitude})" - unless existing_place - existing_place = Place.where(latitude: latitude, longitude: longitude).first - end + # During import, we prioritize data integrity for the importing user + # First try exact match (name + coordinates) + existing_place = Place.where( + name: name, + latitude: latitude, + longitude: longitude + ).first if existing_place - Rails.logger.debug "Place already exists: #{name}" + Rails.logger.debug "Found exact place match: #{name} at (#{latitude}, #{longitude}) -> existing place ID #{existing_place.id}" existing_place.define_singleton_method(:previously_new_record?) { false } return existing_place end + Rails.logger.debug "No exact match found for #{name} at (#{latitude}, #{longitude}). Creating new place." + + # If no exact match, create a new place to ensure data integrity + # This prevents data loss during import even if similar places exist place_attributes = place_data.except('created_at', 'updated_at', 'latitude', 'longitude') place_attributes['lonlat'] = "POINT(#{longitude} #{latitude})" place_attributes['latitude'] = latitude place_attributes['longitude'] = longitude place_attributes.delete('user') + Rails.logger.debug "Creating place with attributes: #{place_attributes.inspect}" + begin place = Place.create!(place_attributes) place.define_singleton_method(:previously_new_record?) { true } - Rails.logger.debug "Created place: #{place.name}" + Rails.logger.debug "Created place during import: #{place.name} (ID: #{place.id})" place rescue ActiveRecord::RecordInvalid => e - ExceptionReporter.call(e, 'Failed to create place') - + Rails.logger.error "Failed to create place: #{place_data.inspect}, error: #{e.message}" nil end end diff --git a/app/services/users/import_data/trips.rb b/app/services/users/import_data/trips.rb index d695479f..72b6a5c4 100644 --- a/app/services/users/import_data/trips.rb +++ b/app/services/users/import_data/trips.rb @@ -135,9 +135,9 @@ class Users::ImportData::Trips def valid_trip_data?(trip_data) return false unless trip_data.is_a?(Hash) - validate_trip_name(trip_data) - validate_trip_started_at(trip_data) - validate_trip_ended_at(trip_data) + return false unless validate_trip_name(trip_data) + return false unless validate_trip_started_at(trip_data) + return false unless validate_trip_ended_at(trip_data) true rescue StandardError => e @@ -147,23 +147,29 @@ class Users::ImportData::Trips def validate_trip_name(trip_data) - unless trip_data['name'].present? - Rails.logger.error 'Failed to create trip: Validation failed: Name can\'t be blank' - return false + if trip_data['name'].present? + true + else + Rails.logger.debug 'Trip validation failed: Name can\'t be blank' + false end end def validate_trip_started_at(trip_data) - unless trip_data['started_at'].present? - Rails.logger.error 'Failed to create trip: Validation failed: Started at can\'t be blank' - return false + if trip_data['started_at'].present? + true + else + Rails.logger.debug 'Trip validation failed: Started at can\'t be blank' + false end end def validate_trip_ended_at(trip_data) - unless trip_data['ended_at'].present? - Rails.logger.error 'Failed to create trip: Validation failed: Ended at can\'t be blank' - return false + if trip_data['ended_at'].present? + true + else + Rails.logger.debug 'Trip validation failed: Ended at can\'t be blank' + false end end end diff --git a/app/services/users/import_data/visits.rb b/app/services/users/import_data/visits.rb index bb256fec..e5508175 100644 --- a/app/services/users/import_data/visits.rb +++ b/app/services/users/import_data/visits.rb @@ -28,7 +28,12 @@ class Users::ImportData::Visits visits_created += 1 Rails.logger.debug "Created visit: #{visit_record.name}" rescue ActiveRecord::RecordInvalid => e - ExceptionReporter.call(e, 'Failed to create visit') + Rails.logger.error "Failed to create visit: #{visit_data.inspect}, error: #{e.message}" + ExceptionReporter.call(e, 'Failed to create visit during import') + next + rescue StandardError => e + Rails.logger.error "Unexpected error creating visit: #{visit_data.inspect}, error: #{e.message}" + ExceptionReporter.call(e, 'Unexpected error during visit import') next end end @@ -58,29 +63,67 @@ class Users::ImportData::Visits attributes = visit_data.except('place_reference') if visit_data['place_reference'] - place = find_referenced_place(visit_data['place_reference']) + place = find_or_create_referenced_place(visit_data['place_reference']) attributes[:place] = place if place end attributes end - def find_referenced_place(place_reference) + def find_or_create_referenced_place(place_reference) return nil unless place_reference.is_a?(Hash) name = place_reference['name'] - latitude = place_reference['latitude'].to_f - longitude = place_reference['longitude'].to_f + latitude = place_reference['latitude']&.to_f + longitude = place_reference['longitude']&.to_f - place = Place.find_by(name: name) || - Place.where("latitude = ? AND longitude = ?", latitude, longitude).first + return nil unless name.present? && latitude.present? && longitude.present? + + Rails.logger.debug "Looking for place reference: #{name} at (#{latitude}, #{longitude})" + + # First try exact match (name + coordinates) + place = Place.where( + name: name, + latitude: latitude, + longitude: longitude + ).first if place - Rails.logger.debug "Found referenced place: #{name}" - else - Rails.logger.warn "Referenced place not found: #{name} (#{latitude}, #{longitude})" + Rails.logger.debug "Found exact place match for visit: #{name} -> existing place ID #{place.id}" + return place end - place + # Try coordinate-only match with close proximity + place = Place.where( + "latitude BETWEEN ? AND ? AND longitude BETWEEN ? AND ?", + latitude - 0.0001, latitude + 0.0001, + longitude - 0.0001, longitude + 0.0001 + ).first + + if place + Rails.logger.debug "Found nearby place match for visit: #{name} -> #{place.name} (ID: #{place.id})" + return place + end + + # If no match found, create the place to ensure visit import succeeds + # This handles cases where places weren't imported in the places phase + Rails.logger.info "Creating missing place during visit import: #{name} at (#{latitude}, #{longitude})" + + begin + place = Place.create!( + name: name, + latitude: latitude, + longitude: longitude, + lonlat: "POINT(#{longitude} #{latitude})", + source: place_reference['source'] || 'manual' + ) + + Rails.logger.debug "Created missing place for visit: #{place.name} (ID: #{place.id})" + place + rescue ActiveRecord::RecordInvalid => e + Rails.logger.error "Failed to create missing place: #{place_reference.inspect}, error: #{e.message}" + ExceptionReporter.call(e, 'Failed to create missing place during visit import') + nil + end end end diff --git a/app/views/devise/registrations/edit.html.erb b/app/views/devise/registrations/edit.html.erb index 25e742a3..5fb84f95 100644 --- a/app/views/devise/registrations/edit.html.erb +++ b/app/views/devise/registrations/edit.html.erb @@ -63,7 +63,10 @@

Unhappy? <%= link_to "Cancel my account", registration_path(resource_name), data: { confirm: "Are you sure?", turbo_confirm: "Are you sure?", turbo_method: :delete }, method: :delete, class: 'btn' %>

- <%= link_to "Export my data", export_settings_users_path, class: 'btn btn-primary' %> + <%= link_to "Export my data", export_settings_users_path, class: 'btn btn-primary', data: { + turbo_confirm: "Are you sure you want to export your data?", + turbo_method: :get + } %>

diff --git a/app/views/imports/index.html.erb b/app/views/imports/index.html.erb index d0dd7680..f2b6467b 100644 --- a/app/views/imports/index.html.erb +++ b/app/views/imports/index.html.erb @@ -68,7 +68,7 @@ <%= number_with_delimiter import.processed %> - <%= import.status %> + <%= import.status %> <%= human_datetime(import.created_at) %> <% if import.file.present? %> diff --git a/db/schema.rb b/db/schema.rb index 2c81e6bb..4db0f831 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -77,9 +77,6 @@ ActiveRecord::Schema[8.0].define(version: 2025_06_27_184017) do t.index ["name"], name: "index_countries_on_name" end - create_table "data_migrations", primary_key: "version", id: :string, force: :cascade do |t| - end - create_table "exports", force: :cascade do |t| t.string "name", null: false t.string "url" @@ -232,18 +229,6 @@ ActiveRecord::Schema[8.0].define(version: 2025_06_27_184017) do t.index ["user_id"], name: "index_trips_on_user_id" end - create_table "user_data_imports", force: :cascade do |t| - t.bigint "user_id", null: false - t.string "status", default: "pending", null: false - t.string "archive_file_name" - t.text "error_message" - t.datetime "created_at", null: false - t.datetime "updated_at", null: false - t.index ["status"], name: "index_user_data_imports_on_status" - t.index ["user_id", "created_at"], name: "index_user_data_imports_on_user_id_and_created_at" - t.index ["user_id"], name: "index_user_data_imports_on_user_id" - end - create_table "users", force: :cascade do |t| t.string "email", default: "", null: false t.string "encrypted_password", default: "", null: false @@ -296,7 +281,6 @@ ActiveRecord::Schema[8.0].define(version: 2025_06_27_184017) do add_foreign_key "points", "visits" add_foreign_key "stats", "users" add_foreign_key "trips", "users" - add_foreign_key "user_data_imports", "users" add_foreign_key "visits", "areas" add_foreign_key "visits", "places" add_foreign_key "visits", "users" diff --git a/spec/services/users/export_import_integration_spec.rb b/spec/services/users/export_import_integration_spec.rb index 67603101..ed959048 100644 --- a/spec/services/users/export_import_integration_spec.rb +++ b/spec/services/users/export_import_integration_spec.rb @@ -31,8 +31,6 @@ RSpec.describe 'Users Export-Import Integration', type: :service do original_counts = calculate_user_entity_counts(original_user) - debug_export_data(temp_archive_path) - original_log_level = Rails.logger.level Rails.logger.level = Logger::DEBUG @@ -129,36 +127,94 @@ RSpec.describe 'Users Export-Import Integration', type: :service do end end - private + describe 'places and visits import integrity' do + it 'imports all places and visits without losses due to global deduplication' do + # Create a user with specific places and visits + original_user = create(:user, email: 'original@example.com') - def debug_export_data(archive_path) - require 'zip' + # 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) - puts "\n=== DEBUGGING EXPORT DATA ===" + # Create visits associated with those places + create(:visit, user: original_user, place: home_place, name: 'Home Visit') + create(:visit, user: original_user, place: office_place, name: 'Work Visit') + create(:visit, user: original_user, place: gym_place, name: 'Workout') - data = nil - Zip::File.open(archive_path) do |zip_file| - data_entry = zip_file.find { |entry| entry.name == 'data.json' } - if data_entry - json_content = data_entry.get_input_stream.read - data = JSON.parse(json_content) + # Create a visit without a place + create(:visit, user: original_user, place: nil, name: 'Unknown Location') - puts "Export counts: #{data['counts'].inspect}" - puts "Points in export: #{data['points']&.size || 0}" - puts "Places in export: #{data['places']&.size || 0}" - puts "First point sample: #{data['points']&.first&.slice('timestamp', 'longitude', 'latitude', 'import_reference', 'country_info', 'visit_reference')}" - puts "First place sample: #{data['places']&.first&.slice('name', 'latitude', 'longitude', 'source')}" - puts "Imports in export: #{data['imports']&.size || 0}" - puts "Countries referenced: #{data['points']&.map { |p| p['country_info']&.dig('name') }&.compact&.uniq || []}" - else - puts "No data.json found in export!" - end + # Calculate counts properly - places are accessed through visits + original_places_count = original_user.places.distinct.count + original_visits_count = original_user.visits.count + + # Export the data + export_service = Users::ExportData.new(original_user) + export_record = export_service.export + + # Download and save to a temporary file for processing + archive_content = export_record.file.download + temp_export_file = Tempfile.new(['test_export', '.zip']) + temp_export_file.binmode + temp_export_file.write(archive_content) + temp_export_file.close + + # SIMULATE FRESH DATABASE: Remove the original places to simulate database migration + # This simulates the scenario where we're importing into a different database + place_ids_to_remove = [home_place.id, office_place.id, gym_place.id] + Place.where(id: place_ids_to_remove).destroy_all + + # Create another user on a "different database" scenario + import_user = create(:user, email: 'import@example.com') + + # Create some existing global places that might conflict + # These should NOT prevent import of the user's places + create(:place, name: 'Home', latitude: 40.8000, longitude: -74.1000) # Different coordinates + create(:place, name: 'Coffee Shop', latitude: 40.7589, longitude: -73.9851) # Same coordinates, different name + + # Simulate import into "new database" + temp_import_file = Tempfile.new(['test_import', '.zip']) + temp_import_file.binmode + temp_import_file.write(archive_content) + temp_import_file.close + + # Import the data + import_service = Users::ImportData.new(import_user, temp_import_file.path) + import_stats = import_service.import + + # Verify all entities were imported correctly + expect(import_stats[:places_created]).to eq(original_places_count), + "Expected #{original_places_count} places to be created, got #{import_stats[:places_created]}" + expect(import_stats[:visits_created]).to eq(original_visits_count), + "Expected #{original_visits_count} visits to be created, got #{import_stats[:visits_created]}" + + # Verify the imported user has access to all their data + imported_places_count = import_user.places.distinct.count + imported_visits_count = import_user.visits.count + + expect(imported_places_count).to eq(original_places_count), + "Expected user to have access to #{original_places_count} places, got #{imported_places_count}" + expect(imported_visits_count).to eq(original_visits_count), + "Expected user to have #{original_visits_count} visits, got #{imported_visits_count}" + + # Verify specific visits have their place associations + imported_visits = import_user.visits.includes(:place) + visits_with_places = imported_visits.where.not(place: nil) + expect(visits_with_places.count).to eq(3) # Home, Office, Gym + + # Verify place names are preserved + place_names = visits_with_places.map { |v| v.place.name }.sort + expect(place_names).to eq(['Gym', 'Home', 'Office']) + + # Cleanup + temp_export_file.unlink + temp_import_file.unlink end - - puts "=== END DEBUG ===" - data end + private + def create_full_user_dataset(user) user.update!(settings: { 'distance_unit' => 'km', diff --git a/spec/services/users/import_data/places_spec.rb b/spec/services/users/import_data/places_spec.rb index f00f09a8..bcb5e7da 100644 --- a/spec/services/users/import_data/places_spec.rb +++ b/spec/services/users/import_data/places_spec.rb @@ -71,19 +71,41 @@ RSpec.describe Users::ImportData::Places, type: :service do context 'with duplicate places (same name)' do before do - # Create an existing place with same name + # Create an existing place with same name but different coordinates + create(:place, name: 'Home', + latitude: 41.0000, longitude: -75.0000, + lonlat: 'POINT(-75.0000 41.0000)') + end + + it 'creates the place since coordinates are different' do + expect { service.call }.to change { Place.count }.by(2) + end + + it 'creates both places with different coordinates' do + service.call + home_places = Place.where(name: 'Home') + expect(home_places.count).to eq(2) + + imported_home = home_places.find_by(latitude: 40.7128, longitude: -74.0060) + expect(imported_home).to be_present + end + end + + context 'with exact duplicate places (same name and coordinates)' do + before do + # Create an existing place with exact same name and coordinates create(:place, name: 'Home', latitude: 40.7128, longitude: -74.0060, lonlat: 'POINT(-74.0060 40.7128)') end - it 'skips duplicate places' do + it 'skips exact duplicate places' do expect { service.call }.to change { Place.count }.by(1) end - it 'logs when skipping duplicates' do + it 'logs when finding exact duplicates' do allow(Rails.logger).to receive(:debug) # Allow any debug logs - expect(Rails.logger).to receive(:debug).with("Place already exists: Home") + expect(Rails.logger).to receive(:debug).with(/Found exact place match: Home at \(40\.7128, -74\.006\) -> existing place ID \d+/) service.call end @@ -102,15 +124,15 @@ RSpec.describe Users::ImportData::Places, type: :service do lonlat: 'POINT(-74.0060 40.7128)') end - it 'skips duplicate places by coordinates' do - expect { service.call }.to change { Place.count }.by(1) + it 'creates the place since name is different' do + expect { service.call }.to change { Place.count }.by(2) end - it 'logs when skipping duplicates' do - allow(Rails.logger).to receive(:debug) # Allow any debug logs - expect(Rails.logger).to receive(:debug).with("Place already exists: Home") - + it 'creates both places with different names' do service.call + places_at_location = Place.where(latitude: 40.7128, longitude: -74.0060) + expect(places_at_location.count).to eq(2) + expect(places_at_location.pluck(:name)).to contain_exactly('Home', 'Different Name') end end