Fix deletion of imports on error

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!

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)

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