diff --git a/app/controllers/imports_controller.rb b/app/controllers/imports_controller.rb index 66d2d4fb..9cd2765c 100644 --- a/app/controllers/imports_controller.rb +++ b/app/controllers/imports_controller.rb @@ -31,24 +31,26 @@ class ImportsController < ApplicationController end def create - raw_files = params.dig(:import, :files).reject(&:blank?) + files_params = params.dig(:import, :files) + raw_files = Array(files_params).reject(&:blank?) if raw_files.empty? redirect_to new_import_path, alert: 'No files were selected for upload', status: :unprocessable_entity return end - imports = raw_files.map do |item| + created_imports = [] + + raw_files.each do |item| next if item.is_a?(ActionDispatch::Http::UploadedFile) - Rails.logger.debug "Processing signed ID: #{item[0..20]}..." - - create_import_from_signed_id(item) + import = create_import_from_signed_id(item) + created_imports << import if import.present? end - if imports.any? + if created_imports.any? redirect_to imports_url, - notice: "#{imports.size} files are queued to be imported in background", + notice: "#{created_imports.size} files are queued to be imported in background", status: :see_other else redirect_to new_import_path, @@ -56,8 +58,10 @@ class ImportsController < ApplicationController status: :unprocessable_entity end rescue StandardError => e - # Clean up recent imports if there was an error - Import.where(user: current_user).where('created_at > ?', 5.minutes.ago).destroy_all + if created_imports.present? + import_ids = created_imports.map(&:id).compact + Import.where(id: import_ids).destroy_all if import_ids.any? + end Rails.logger.error "Import error: #{e.message}" Rails.logger.error e.backtrace.join("\n") @@ -85,16 +89,13 @@ class ImportsController < ApplicationController def create_import_from_signed_id(signed_id) Rails.logger.debug "Creating import from signed ID: #{signed_id[0..20]}..." - # Find the blob using the signed ID blob = ActiveStorage::Blob.find_signed(signed_id) - # Create the import import = current_user.imports.build( name: blob.filename.to_s, source: params[:import][:source] ) - # Attach the blob to the import import.file.attach(blob) import.save! diff --git a/config/initializers/geocoder.rb b/config/initializers/geocoder.rb index 46cd433d..f15acc33 100644 --- a/config/initializers/geocoder.rb +++ b/config/initializers/geocoder.rb @@ -15,16 +15,14 @@ settings = { if PHOTON_API_HOST.present? settings[:lookup] = :photon settings[:photon] = { use_https: PHOTON_API_USE_HTTPS, host: PHOTON_API_HOST } - settings[:http_headers] = { 'X-Api-Key' => PHOTON_API_KEY } if defined?(PHOTON_API_KEY) + settings[:http_headers] = { 'X-Api-Key' => PHOTON_API_KEY } if PHOTON_API_KEY.present? elsif GEOAPIFY_API_KEY.present? settings[:lookup] = :geoapify settings[:api_key] = GEOAPIFY_API_KEY elsif NOMINATIM_API_HOST.present? settings[:lookup] = :nominatim settings[:nominatim] = { use_https: NOMINATIM_API_USE_HTTPS, host: NOMINATIM_API_HOST } - if NOMINATIM_API_KEY.present? - settings[:api_key] = NOMINATIM_API_KEY - end + settings[:api_key] = NOMINATIM_API_KEY if NOMINATIM_API_KEY.present? end Geocoder.configure(settings) diff --git a/spec/requests/imports_spec.rb b/spec/requests/imports_spec.rb index 7b20f81a..502bcca4 100644 --- a/spec/requests/imports_spec.rb +++ b/spec/requests/imports_spec.rb @@ -87,6 +87,37 @@ RSpec.describe 'Imports', type: :request do expect(response).to redirect_to(imports_path) end end + + context 'when an error occurs during import creation' do + let(:file1) { fixture_file_upload('owntracks/2024-03.rec', 'text/plain') } + let(:file2) { fixture_file_upload('gpx/gpx_track_single_segment.gpx', 'application/gpx+xml') } + let(:blob1) { create_blob_for_file(file1) } + let(:blob2) { create_blob_for_file(file2) } + let(:signed_id1) { generate_signed_id_for_blob(blob1) } + let(:signed_id2) { generate_signed_id_for_blob(blob2) } + + it 'deletes any created imports' do + # The first blob should be found correctly + allow(ActiveStorage::Blob).to receive(:find_signed).with(signed_id1).and_return(blob1) + + # The second blob find will raise an error + allow(ActiveStorage::Blob).to receive(:find_signed).with(signed_id2).and_raise(StandardError, 'Test error') + + # Allow ExceptionReporter to be called without actually calling it + allow(ExceptionReporter).to receive(:call) + + # The request should not ultimately create any imports + expect do + post imports_path, params: { import: { source: 'owntracks', files: [signed_id1, signed_id2] } } + end.not_to change(Import, :count) + + # Check that we were redirected with an error message + expect(response).to have_http_status(422) + # Just check that we have an alert message, not its exact content + # since error handling might transform the message + expect(flash[:alert]).not_to be_nil + end + end end end