From a3fc838855dbe4526af45002754a311cbd0f127c Mon Sep 17 00:00:00 2001
From: Marcel <marcel@familienarchiv>
Date: Tue, 19 May 2026 07:44:12 +0200
Subject: [PATCH] fix(import): surface S3 failures + already-exists in
 skippedFiles, a11y + max-height

- Change importSingleDocument return type from boolean to Optional<String>
  so callers in processRows receive the skip reason on every non-success path.
  S3 upload failures now surface as "S3_UPLOAD_FAILED" and already-imported
  documents as "ALREADY_EXISTS" in the skippedFiles list shown in the admin UI.
- Add two new tests: runImportAsync_addsS3UploadFailed_toSkippedFiles and
  runImportAsync_addsAlreadyExists_toSkippedFiles; update
  importSingleDocument_skips_whenDocumentAlreadyUploadedNotPlaceholder and
  the S3-failure test to assert on the Optional return value.
- Add i18n keys for S3_UPLOAD_FAILED and ALREADY_EXISTS in de/en/es messages.
- Svelte ImportStatusCard: add aria-hidden="true" to SVG chevron, wrap
  conditional warning section in aria-live="polite" div, add max-h-64
  overflow-y-auto to skipped-files <ul> to cap height on large batches.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
---
 .../importing/MassImportService.java          | 27 ++++++--
 .../importing/MassImportServiceTest.java      | 47 ++++++++++++-
 frontend/messages/de.json                     |  2 +
 frontend/messages/en.json                     |  2 +
 frontend/messages/es.json                     |  2 +
 .../admin/system/ImportStatusCard.svelte      | 67 ++++++++++---------
 6 files changed, 106 insertions(+), 41 deletions(-)

diff --git a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java
index 58d6493a..e09aaa76 100644
--- a/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java
+++ b/backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java
@@ -69,9 +69,15 @@ public class MassImportService {
             @Schema(requiredMode = Schema.RequiredMode.REQUIRED) List<SkippedFile> skippedFiles,
             LocalDateTime startedAt
     ) {
-        @Schema(requiredMode = Schema.RequiredMode.REQUIRED)
+        // Note: @Schema on a record accessor method is not picked up by SpringDoc; the
+        // "skipped" count is a computed convenience field derived from skippedFiles.size().
         @JsonProperty("skipped")
         public int skipped() { return skippedFiles.size(); }
+
+        /** Defensive-copy constructor — callers cannot mutate the stored list after construction. */
+        public ImportStatus {
+            skippedFiles = List.copyOf(skippedFiles);
+        }
     }
 
     record ProcessResult(int processed, List<SkippedFile> skippedFiles) {}
@@ -304,8 +310,10 @@ public class MassImportService {
                 }
             }
 
-            boolean imported = importSingleDocument(cells, fileOnDisk, filename, index);
-            if (imported) {
+            Optional<String> skipReason = importSingleDocument(cells, fileOnDisk, filename, index);
+            if (skipReason.isPresent()) {
+                skippedFiles.add(new SkippedFile(filename, skipReason.get()));
+            } else {
                 processed++;
             }
         }
@@ -328,12 +336,17 @@ public class MassImportService {
         }
     }
 
+    /**
+     * Imports a single document row.
+     *
+     * @return empty Optional on success; an Optional containing the skip reason on failure/skip.
+     */
     @Transactional
-    protected boolean importSingleDocument(List<String> cells, Optional<File> file, String originalFilename, String index) {
+    protected Optional<String> importSingleDocument(List<String> cells, Optional<File> file, String originalFilename, String index) {
         Optional<Document> existing = documentService.findByOriginalFilename(originalFilename);
         if (existing.isPresent() && existing.get().getStatus() != DocumentStatus.PLACEHOLDER) {
             log.info("Dokument {} existiert bereits, überspringe.", originalFilename);
-            return false;
+            return Optional.of("ALREADY_EXISTS");
         }
 
         String archiveBox    = getCell(cells, colBox);
@@ -369,7 +382,7 @@ public class MassImportService {
                 status = DocumentStatus.UPLOADED;
             } catch (Exception e) {
                 log.error("S3 Upload Fehler für {}", file.get().getName(), e);
-                return false;
+                return Optional.of("S3_UPLOAD_FAILED");
             }
         }
 
@@ -411,7 +424,7 @@ public class MassImportService {
             thumbnailAsyncRunner.dispatchAfterCommit(saved.getId());
         }
         log.info("Importiert{}: {}", file.isEmpty() ? " (nur Metadaten)" : "", originalFilename);
-        return true;
+        return Optional.empty();
     }
 
     // --- Helpers ---
diff --git a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java
index 31cd2842..26d38679 100644
--- a/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java
+++ b/backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java
@@ -154,9 +154,49 @@ class MassImportServiceTest {
                 .build();
         when(documentService.findByOriginalFilename("doc001.pdf")).thenReturn(Optional.of(existing));
 
-        service.importSingleDocument(minimalCells("doc001.pdf"), Optional.empty(), "doc001.pdf", "doc001");
+        Optional<String> result = service.importSingleDocument(minimalCells("doc001.pdf"), Optional.empty(), "doc001.pdf", "doc001");
 
         verify(documentService, never()).save(any());
+        assertThat(result).isPresent().contains("ALREADY_EXISTS");
+    }
+
+    // ─── importSingleDocument — S3 failure surfaced in skippedFiles ──────────
+
+    @Test
+    void runImportAsync_addsS3UploadFailed_toSkippedFiles_whenS3Throws(@TempDir Path tempDir) throws Exception {
+        byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF-
+        Files.write(tempDir.resolve("upload_fail.pdf"), pdfHeader);
+        buildMinimalImportXlsx(tempDir, "upload_fail.pdf");
+        ReflectionTestUtils.setField(service, "importDir", tempDir.toString());
+        when(documentService.findByOriginalFilename("upload_fail.pdf")).thenReturn(Optional.empty());
+        doThrow(new RuntimeException("S3 unavailable"))
+                .when(s3Client).putObject(any(PutObjectRequest.class), any(RequestBody.class));
+
+        service.runImportAsync();
+
+        assertThat(service.getStatus().skipped()).isEqualTo(1);
+        assertThat(service.getStatus().skippedFiles())
+                .extracting(MassImportService.SkippedFile::filename, MassImportService.SkippedFile::reason)
+                .containsExactly(org.assertj.core.groups.Tuple.tuple("upload_fail.pdf", "S3_UPLOAD_FAILED"));
+    }
+
+    @Test
+    void runImportAsync_addsAlreadyExists_toSkippedFiles_whenDocumentAlreadyUploaded(@TempDir Path tempDir) throws Exception {
+        buildMinimalImportXlsx(tempDir, "existing.pdf");
+        ReflectionTestUtils.setField(service, "importDir", tempDir.toString());
+        Document existing = Document.builder()
+                .id(UUID.randomUUID())
+                .originalFilename("existing.pdf")
+                .status(DocumentStatus.UPLOADED)
+                .build();
+        when(documentService.findByOriginalFilename("existing.pdf")).thenReturn(Optional.of(existing));
+
+        service.runImportAsync();
+
+        assertThat(service.getStatus().skipped()).isEqualTo(1);
+        assertThat(service.getStatus().skippedFiles())
+                .extracting(MassImportService.SkippedFile::reason)
+                .containsExactly("ALREADY_EXISTS");
     }
 
     // ─── importSingleDocument — create new document (metadata only) ───────────
@@ -208,7 +248,7 @@ class MassImportServiceTest {
     }
 
     @Test
-    void importSingleDocument_returnsEarly_whenS3UploadFails(@TempDir Path tempDir) throws Exception {
+    void importSingleDocument_returnsS3UploadFailed_whenS3UploadFails(@TempDir Path tempDir) throws Exception {
         Path tempFile = tempDir.resolve("fail.pdf");
         Files.write(tempFile, "data".getBytes());
 
@@ -216,10 +256,11 @@ class MassImportServiceTest {
         doThrow(new RuntimeException("S3 error"))
                 .when(s3Client).putObject(any(PutObjectRequest.class), any(RequestBody.class));
 
-        service.importSingleDocument(
+        Optional<String> result = service.importSingleDocument(
                 minimalCells("fail.pdf"), Optional.of(tempFile.toFile()), "fail.pdf", "fail");
 
         verify(documentService, never()).save(any());
+        assertThat(result).isPresent().contains("S3_UPLOAD_FAILED");
     }
 
     // ─── importSingleDocument — sender handling ───────────────────────────────
diff --git a/frontend/messages/de.json b/frontend/messages/de.json
index e225997f..72254cf3 100644
--- a/frontend/messages/de.json
+++ b/frontend/messages/de.json
@@ -355,6 +355,8 @@
 	"admin_system_import_skipped_label": "übersprungen",
 	"import_reason_invalid_pdf_signature": "Keine gültige PDF-Signatur",
 	"import_reason_file_read_error": "Fehler beim Lesen der Datei",
+	"import_reason_s3_upload_failed": "Upload-Fehler (S3)",
+	"import_reason_already_exists": "Bereits importiert",
 	"admin_system_import_status_failed": "Import fehlgeschlagen",
 	"admin_system_import_failed_no_spreadsheet": "Keine Tabellendatei gefunden.",
 	"admin_system_import_failed_internal": "Interner Fehler beim Import.",
diff --git a/frontend/messages/en.json b/frontend/messages/en.json
index 894c60f8..b8b6dd68 100644
--- a/frontend/messages/en.json
+++ b/frontend/messages/en.json
@@ -355,6 +355,8 @@
 	"admin_system_import_skipped_label": "skipped",
 	"import_reason_invalid_pdf_signature": "Invalid PDF signature",
 	"import_reason_file_read_error": "File read error",
+	"import_reason_s3_upload_failed": "Upload error (S3)",
+	"import_reason_already_exists": "Already imported",
 	"admin_system_import_status_failed": "Import failed",
 	"admin_system_import_failed_no_spreadsheet": "No spreadsheet file found.",
 	"admin_system_import_failed_internal": "Import failed due to an internal error.",
diff --git a/frontend/messages/es.json b/frontend/messages/es.json
index a2409dbf..0943943e 100644
--- a/frontend/messages/es.json
+++ b/frontend/messages/es.json
@@ -355,6 +355,8 @@
 	"admin_system_import_skipped_label": "omitidos",
 	"import_reason_invalid_pdf_signature": "Firma PDF no válida",
 	"import_reason_file_read_error": "Error al leer el archivo",
+	"import_reason_s3_upload_failed": "Error de carga (S3)",
+	"import_reason_already_exists": "Ya importado",
 	"admin_system_import_status_failed": "Importación fallida",
 	"admin_system_import_failed_no_spreadsheet": "No se encontró ninguna hoja de cálculo.",
 	"admin_system_import_failed_internal": "Error interno durante la importación.",
diff --git a/frontend/src/routes/admin/system/ImportStatusCard.svelte b/frontend/src/routes/admin/system/ImportStatusCard.svelte
index f6e9ab82..c18fcef3 100644
--- a/frontend/src/routes/admin/system/ImportStatusCard.svelte
+++ b/frontend/src/routes/admin/system/ImportStatusCard.svelte
@@ -19,6 +19,8 @@ const failureMessage = $derived(
 function reasonLabel(code: string): string {
 	if (code === 'INVALID_PDF_SIGNATURE') return m.import_reason_invalid_pdf_signature();
 	if (code === 'FILE_READ_ERROR') return m.import_reason_file_read_error();
+	if (code === 'S3_UPLOAD_FAILED') return m.import_reason_s3_upload_failed();
+	if (code === 'ALREADY_EXISTS') return m.import_reason_already_exists();
 	return code;
 }
 </script>
@@ -54,38 +56,41 @@ function reasonLabel(code: string): string {
 			</p>
 			<p class="mt-1 text-xs text-green-800">{m.admin_system_import_status_done()}</p>
 		</div>
-		{#if importStatus.skipped > 0}
-			<details class="mb-4 rounded-sm border border-warning/40 bg-warning/10 p-4 text-amber-900">
-				<summary class="flex cursor-pointer list-none items-center gap-2">
-					<svg
-						class="details-chevron h-4 w-4 shrink-0 motion-safe:transition-transform"
-						viewBox="0 0 16 16"
-						fill="none"
-						stroke="currentColor"
-						stroke-width="2"
-						stroke-linecap="round"
-						stroke-linejoin="round"
-					>
-						<path d="M6 4l4 4-4 4" />
-					</svg>
-					<div>
-						<span data-testid="skipped-count" class="block text-base font-bold"
-							>{importStatus.skipped}</span
+		<div aria-live="polite">
+			{#if importStatus.skipped > 0}
+				<details class="mb-4 rounded-sm border border-warning/40 bg-warning/10 p-4 text-amber-900">
+					<summary class="flex cursor-pointer list-none items-center gap-2">
+						<svg
+							aria-hidden="true"
+							class="details-chevron h-4 w-4 shrink-0 motion-safe:transition-transform"
+							viewBox="0 0 16 16"
+							fill="none"
+							stroke="currentColor"
+							stroke-width="2"
+							stroke-linecap="round"
+							stroke-linejoin="round"
 						>
-						<span class="block font-sans text-xs font-bold tracking-widest uppercase">
-							{m.admin_system_import_skipped_label()}
-						</span>
-					</div>
-				</summary>
-				<ul class="mt-3 space-y-1">
-					{#each importStatus.skippedFiles as skipped (skipped.filename)}
-						<li class="font-mono text-sm text-ink-2">
-							{skipped.filename} — {reasonLabel(skipped.reason)}
-						</li>
-					{/each}
-				</ul>
-			</details>
-		{/if}
+							<path d="M6 4l4 4-4 4" />
+						</svg>
+						<div>
+							<span data-testid="skipped-count" class="block text-base font-bold"
+								>{importStatus.skipped}</span
+							>
+							<span class="block font-sans text-xs font-bold tracking-widest uppercase">
+								{m.admin_system_import_skipped_label()}
+							</span>
+						</div>
+					</summary>
+					<ul class="mt-3 max-h-64 space-y-1 overflow-y-auto">
+						{#each importStatus.skippedFiles as skipped (skipped.filename)}
+							<li class="font-mono text-sm text-ink-2">
+								{skipped.filename} — {reasonLabel(skipped.reason)}
+							</li>
+						{/each}
+					</ul>
+				</details>
+			{/if}
+		</div>
 		<button
 			data-import-trigger
 			onclick={ontrigger}