Merge pull request #1675 from Freika/fix/unified-imports

Fix import detection

CHANGELOG.md

@@ -4,6 +4,16 @@ 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] -
+
+## Changed
+
+- If user already have import with the same name, it will be appended with timestamp during the import process.
+
+## Fixed
+
+- Some types of imports were not being detected correctly and were failing to import.
+
 # [0.30.10] - 2025-08-22
 
 ## Added

app/controllers/imports_controller.rb

@@ -102,27 +102,25 @@ class ImportsController < ApplicationController
 
     blob = ActiveStorage::Blob.find_signed(signed_id)
 
-    import = current_user.imports.build(name: blob.filename.to_s)
+    import_name = generate_unique_import_name(blob.filename.to_s)
+    import = current_user.imports.build(name: import_name)
     import.file.attach(blob)
-    import.source = detect_import_source(import.file) if import.source.blank?
 
     import.save!
 
     import
   end
 
-  def detect_import_source(file_attachment)
-    temp_file_path = Imports::SecureFileDownloader.new(file_attachment).download_to_temp_file
+  def generate_unique_import_name(original_name)
+    return original_name unless current_user.imports.exists?(name: original_name)
 
-    Imports::SourceDetector.new_from_file_header(temp_file_path).detect_source
-  rescue StandardError => e
-    Rails.logger.warn "Failed to auto-detect import source for #{file_attachment.filename}: #{e.message}"
-    nil
-  ensure
-    # Cleanup temp file
-    if temp_file_path && File.exist?(temp_file_path)
-      File.unlink(temp_file_path)
-    end
+    # Extract filename and extension
+    basename = File.basename(original_name, File.extname(original_name))
+    extension = File.extname(original_name)
+    
+    # Add current datetime
+    timestamp = Time.current.strftime('%Y%m%d_%H%M%S')
+    "#{basename}_#{timestamp}#{extension}"
   end
 
   def validate_points_limit

app/controllers/settings/background_jobs_controller.rb

@@ -6,7 +6,7 @@ class Settings::BackgroundJobsController < ApplicationController
     %w[start_immich_import start_photoprism_import].include?(params[:job_name])
   }
 
-  def index;end
+  def index; end
 
   def create
     EnqueueBackgroundJob.perform_later(params[:job_name], current_user.id)

app/models/import.rb

@@ -21,7 +21,7 @@ class Import < ApplicationRecord
     google_semantic_history: 0, owntracks: 1, google_records: 2,
     google_phone_takeout: 3, gpx: 4, immich_api: 5, geojson: 6, photoprism_api: 7,
     user_data_archive: 8
-  }
+  }, allow_nil: true
 
   def process!
     if user_data_archive?

app/services/imports/create.rb

@@ -16,7 +16,13 @@ class Imports::Create
 
     temp_file_path = Imports::SecureFileDownloader.new(import.file).download_to_temp_file
 
-    source = import.source.presence || detect_source_from_file(temp_file_path)
+    source = if import.source.nil? || should_detect_source?
+               detect_source_from_file(temp_file_path)
+             else
+               import.source
+             end
+
+    import.update!(source: source)
     importer(source).new(import, user.id, temp_file_path).call
 
     schedule_stats_creating(user.id)
@@ -90,8 +96,14 @@ class Imports::Create
     ).call
   end
 
+  def should_detect_source?
+    # Don't override API-based sources that can't be reliably detected
+    !%w[immich_api photoprism_api].include?(import.source)
+  end
+
   def detect_source_from_file(temp_file_path)
     detector = Imports::SourceDetector.new_from_file_header(temp_file_path)
+
     detector.detect_source!
   end
 

app/services/imports/source_detector.rb

@@ -62,10 +62,10 @@ class Imports::SourceDetector
 
   def self.new_from_file_header(file_path)
     filename = File.basename(file_path)
-    
+
     # For detection, read only first 2KB to optimize performance
     header_content = File.open(file_path, 'rb') { |f| f.read(2048) }
-    
+
     new(header_content, filename, file_path)
   end
 
@@ -103,7 +103,7 @@ class Imports::SourceDetector
 
     # Must have .gpx extension AND contain GPX XML structure
     return false unless filename.downcase.end_with?('.gpx')
-    
+
     # Check content for GPX structure
     content_to_check = if file_path && File.exist?(file_path)
                         # Read first 1KB for GPX detection
@@ -111,7 +111,7 @@ class Imports::SourceDetector
                       else
                         file_content
                       end
-    
+
     content_to_check.strip.start_with?('<?xml') && content_to_check.include?('<gpx')
   end
 
@@ -120,7 +120,7 @@ class Imports::SourceDetector
 
     # Check for .rec extension first (fastest check)
     return true if filename.downcase.end_with?('.rec')
-    
+
     # Check for specific OwnTracks line format in content
     content_to_check = if file_path && File.exist?(file_path)
                         # For OwnTracks, read first few lines only
@@ -128,7 +128,7 @@ class Imports::SourceDetector
                       else
                         file_content
                       end
-    
+
     content_to_check.lines.any? { |line| line.include?('"_type":"location"') }
   end
 

app/views/imports/index.html.erb

@@ -36,7 +36,7 @@
         </div>
       </div>
       <div class="overflow-x-auto">
-        <table class="table overflow-x-auto">
+        <table class="table table-zebra overflow-x-auto">
           <thead>
             <tr>
               <th>Name</th>
@@ -55,7 +55,8 @@
             <% @imports.each do |import| %>
               <tr data-import-id="<%= import.id %>"
                   id="import-<%= import.id %>"
-                  data-points-total="<%= import.processed %>">
+                  data-points-total="<%= import.processed %>"
+                  class="hover">
                 <td>
                   <%= link_to import.name, import, class: 'underline hover:no-underline' %>
                   (<%= import.source %>)
@@ -72,9 +73,9 @@
                 <td><%= human_datetime(import.created_at) %></td>
                 <td class="whitespace-nowrap">
                   <% if import.file.present? %>
-                    <%= link_to 'Download', rails_blob_path(import.file, disposition: 'attachment'), class: "px-4 py-2 bg-blue-500 text-white rounded-md", download: import.name %>
+                    <%= link_to 'Download', rails_blob_path(import.file, disposition: 'attachment'), class: "btn btn-outline btn-sm btn-info", download: import.name %>
                   <% end %>
-                  <%= link_to 'Delete', import, data: { confirm: "Are you sure?", turbo_confirm: "Are you sure?", turbo_method: :delete }, method: :delete, class: "px-4 py-2 bg-red-500 text-white rounded-md" %>
+                  <%= link_to 'Delete', import, data: { confirm: "Are you sure?", turbo_confirm: "Are you sure?", turbo_method: :delete }, method: :delete, class: "btn btn-outline btn-sm btn-error" %>
                 </td>
               </tr>
             <% end %>

db/migrate/20250823125940_remove_default_from_imports_source.rb

@@ -0,0 +1,5 @@
+class RemoveDefaultFromImportsSource < ActiveRecord::Migration[8.0]
+  def change
+    change_column_default :imports, :source, from: 0, to: nil
+  end
+end

db/schema.rb

@@ -10,7 +10,7 @@
 #
 # It's strongly recommended that you check this file into your version control system.
 
-ActiveRecord::Schema[8.0].define(version: 2025_07_28_191359) do
+ActiveRecord::Schema[8.0].define(version: 2025_08_23_125940) do
   # These are extensions that must be enabled in order to support this database
   enable_extension "pg_catalog.plpgsql"
   enable_extension "postgis"
@@ -99,7 +99,7 @@ ActiveRecord::Schema[8.0].define(version: 2025_07_28_191359) do
   create_table "imports", force: :cascade do |t|
     t.string "name", null: false
     t.bigint "user_id", null: false
-    t.integer "source", default: 0
+    t.integer "source"
     t.datetime "created_at", null: false
     t.datetime "updated_at", null: false
     t.integer "raw_points", default: 0

spec/factories/imports.rb

@@ -4,7 +4,7 @@ FactoryBot.define do
   factory :import do
     user
     sequence(:name) { |n| "owntracks_export_#{n}.json" }
-    source { Import.sources[:owntracks] }
+    # source { Import.sources[:owntracks] }
 
     trait :with_points do
       after(:create) do |import|

spec/fixtures/files/google/phone-takeout_w_3_duplicates.json

@@ -1,5 +1,3 @@
-// This file contains 3 doubles
-
 {
   "semanticSegments": [
     {

spec/services/google_maps/phone_takeout_importer_spec.rb

@@ -14,7 +14,7 @@ RSpec.describe GoogleMaps::PhoneTakeoutImporter do
 
     context 'when file content is an object' do
       # This file contains 3 duplicates
-      let(:file_path) { Rails.root.join('spec/fixtures/files/google/phone-takeout.json') }
+      let(:file_path) { Rails.root.join('spec/fixtures/files/google/phone-takeout_w_3_duplicates.json') }
       let(:file) { Rack::Test::UploadedFile.new(file_path, 'application/json') }
       let(:import) { create(:import, user:, name: 'phone_takeout.json', file:) }
 

spec/services/imports/create_spec.rb

@@ -21,6 +21,12 @@ RSpec.describe Imports::Create do
         expect(import.reload.status).to eq('processing').or eq('completed')
       end
 
+      it 'updates the import source' do
+        service.call
+
+        expect(import.reload.source).to eq('owntracks')
+      end
+
       context 'when import succeeds' do
         it 'sets status to completed' do
           service.call
@@ -63,10 +69,10 @@ RSpec.describe Imports::Create do
 
     context 'when source is google_phone_takeout' do
       let(:import) { create(:import, source: 'google_phone_takeout') }
-      let(:file_path) { Rails.root.join('spec/fixtures/files/google/phone-takeout.json') }
+      let(:file_path) { Rails.root.join('spec/fixtures/files/google/phone-takeout_w_3_duplicates.json') }
 
       before do
-        import.file.attach(io: File.open(file_path), filename: 'phone-takeout.json',
+        import.file.attach(io: File.open(file_path), filename: 'phone-takeout_w_3_duplicates.json',
                            content_type: 'application/json')
       end
 
@@ -193,6 +199,7 @@ RSpec.describe Imports::Create do
       it 'calls the Photos::Importer' do
         expect(Photos::Importer).to \
           receive(:new).with(import, user.id, kind_of(String)).and_return(double(call: true))
+
         service.call
       end
     end

spec/services/imports/source_detector_spec.rb

@@ -24,7 +24,7 @@ RSpec.describe Imports::SourceDetector do
     end
 
     context 'with Google Phone Takeout format' do
-      let(:file_content) { file_fixture('google/phone-takeout.json').read }
+      let(:file_content) { file_fixture('google/phone-takeout_w_3_duplicates.json').read }
 
       it 'detects google_phone_takeout format' do
         expect(detector.detect_source).to eq(:google_phone_takeout)
@@ -131,7 +131,7 @@ RSpec.describe Imports::SourceDetector do
 
       it 'can detect source efficiently from file' do
         detector = described_class.new_from_file_header(fixture_path)
-        
+
         # Verify it can detect correctly using file-based approach
         expect(detector.detect_source).to eq(:google_records)
       end

spec/services/users/import_data/imports_spec.rb

@@ -221,7 +221,7 @@ RSpec.describe Users::ImportData::Imports, type: :service do
         created_imports = user.imports.pluck(:name, :source)
         expect(created_imports).to contain_exactly(
           ['Valid Import', 'owntracks'],
-          ['Missing Source Import', 'google_semantic_history']
+          ['Missing Source Import', nil]
         )
       end