fix(import): surface S3 failures + already-exists in skippedFiles, a11y + max-height
All checks were successful
CI / Unit & Component Tests (pull_request) Successful in 3m21s
CI / OCR Service Tests (pull_request) Successful in 21s
CI / Backend Unit Tests (pull_request) Successful in 3m6s
CI / fail2ban Regex (pull_request) Successful in 41s
CI / Semgrep Security Scan (pull_request) Successful in 19s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m1s
All checks were successful
CI / Unit & Component Tests (pull_request) Successful in 3m21s
CI / OCR Service Tests (pull_request) Successful in 21s
CI / Backend Unit Tests (pull_request) Successful in 3m6s
CI / fail2ban Regex (pull_request) Successful in 41s
CI / Semgrep Security Scan (pull_request) Successful in 19s
CI / Compose Bucket Idempotency (pull_request) Successful in 1m1s
- 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>
This commit is contained in:
@@ -69,9 +69,15 @@ public class MassImportService {
|
|||||||
@Schema(requiredMode = Schema.RequiredMode.REQUIRED) List<SkippedFile> skippedFiles,
|
@Schema(requiredMode = Schema.RequiredMode.REQUIRED) List<SkippedFile> skippedFiles,
|
||||||
LocalDateTime startedAt
|
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")
|
@JsonProperty("skipped")
|
||||||
public int skipped() { return skippedFiles.size(); }
|
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) {}
|
record ProcessResult(int processed, List<SkippedFile> skippedFiles) {}
|
||||||
@@ -304,8 +310,10 @@ public class MassImportService {
|
|||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
boolean imported = importSingleDocument(cells, fileOnDisk, filename, index);
|
Optional<String> skipReason = importSingleDocument(cells, fileOnDisk, filename, index);
|
||||||
if (imported) {
|
if (skipReason.isPresent()) {
|
||||||
|
skippedFiles.add(new SkippedFile(filename, skipReason.get()));
|
||||||
|
} else {
|
||||||
processed++;
|
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
|
@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);
|
Optional<Document> existing = documentService.findByOriginalFilename(originalFilename);
|
||||||
if (existing.isPresent() && existing.get().getStatus() != DocumentStatus.PLACEHOLDER) {
|
if (existing.isPresent() && existing.get().getStatus() != DocumentStatus.PLACEHOLDER) {
|
||||||
log.info("Dokument {} existiert bereits, überspringe.", originalFilename);
|
log.info("Dokument {} existiert bereits, überspringe.", originalFilename);
|
||||||
return false;
|
return Optional.of("ALREADY_EXISTS");
|
||||||
}
|
}
|
||||||
|
|
||||||
String archiveBox = getCell(cells, colBox);
|
String archiveBox = getCell(cells, colBox);
|
||||||
@@ -369,7 +382,7 @@ public class MassImportService {
|
|||||||
status = DocumentStatus.UPLOADED;
|
status = DocumentStatus.UPLOADED;
|
||||||
} catch (Exception e) {
|
} catch (Exception e) {
|
||||||
log.error("S3 Upload Fehler für {}", file.get().getName(), 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());
|
thumbnailAsyncRunner.dispatchAfterCommit(saved.getId());
|
||||||
}
|
}
|
||||||
log.info("Importiert{}: {}", file.isEmpty() ? " (nur Metadaten)" : "", originalFilename);
|
log.info("Importiert{}: {}", file.isEmpty() ? " (nur Metadaten)" : "", originalFilename);
|
||||||
return true;
|
return Optional.empty();
|
||||||
}
|
}
|
||||||
|
|
||||||
// --- Helpers ---
|
// --- Helpers ---
|
||||||
|
|||||||
@@ -154,9 +154,49 @@ class MassImportServiceTest {
|
|||||||
.build();
|
.build();
|
||||||
when(documentService.findByOriginalFilename("doc001.pdf")).thenReturn(Optional.of(existing));
|
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());
|
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) ───────────
|
// ─── importSingleDocument — create new document (metadata only) ───────────
|
||||||
@@ -208,7 +248,7 @@ class MassImportServiceTest {
|
|||||||
}
|
}
|
||||||
|
|
||||||
@Test
|
@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");
|
Path tempFile = tempDir.resolve("fail.pdf");
|
||||||
Files.write(tempFile, "data".getBytes());
|
Files.write(tempFile, "data".getBytes());
|
||||||
|
|
||||||
@@ -216,10 +256,11 @@ class MassImportServiceTest {
|
|||||||
doThrow(new RuntimeException("S3 error"))
|
doThrow(new RuntimeException("S3 error"))
|
||||||
.when(s3Client).putObject(any(PutObjectRequest.class), any(RequestBody.class));
|
.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");
|
minimalCells("fail.pdf"), Optional.of(tempFile.toFile()), "fail.pdf", "fail");
|
||||||
|
|
||||||
verify(documentService, never()).save(any());
|
verify(documentService, never()).save(any());
|
||||||
|
assertThat(result).isPresent().contains("S3_UPLOAD_FAILED");
|
||||||
}
|
}
|
||||||
|
|
||||||
// ─── importSingleDocument — sender handling ───────────────────────────────
|
// ─── importSingleDocument — sender handling ───────────────────────────────
|
||||||
|
|||||||
@@ -353,6 +353,8 @@
|
|||||||
"admin_system_import_skipped_label": "übersprungen",
|
"admin_system_import_skipped_label": "übersprungen",
|
||||||
"import_reason_invalid_pdf_signature": "Keine gültige PDF-Signatur",
|
"import_reason_invalid_pdf_signature": "Keine gültige PDF-Signatur",
|
||||||
"import_reason_file_read_error": "Fehler beim Lesen der Datei",
|
"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_status_failed": "Import fehlgeschlagen",
|
||||||
"admin_system_import_failed_no_spreadsheet": "Keine Tabellendatei gefunden.",
|
"admin_system_import_failed_no_spreadsheet": "Keine Tabellendatei gefunden.",
|
||||||
"admin_system_import_failed_internal": "Interner Fehler beim Import.",
|
"admin_system_import_failed_internal": "Interner Fehler beim Import.",
|
||||||
|
|||||||
@@ -353,6 +353,8 @@
|
|||||||
"admin_system_import_skipped_label": "skipped",
|
"admin_system_import_skipped_label": "skipped",
|
||||||
"import_reason_invalid_pdf_signature": "Invalid PDF signature",
|
"import_reason_invalid_pdf_signature": "Invalid PDF signature",
|
||||||
"import_reason_file_read_error": "File read error",
|
"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_status_failed": "Import failed",
|
||||||
"admin_system_import_failed_no_spreadsheet": "No spreadsheet file found.",
|
"admin_system_import_failed_no_spreadsheet": "No spreadsheet file found.",
|
||||||
"admin_system_import_failed_internal": "Import failed due to an internal error.",
|
"admin_system_import_failed_internal": "Import failed due to an internal error.",
|
||||||
|
|||||||
@@ -353,6 +353,8 @@
|
|||||||
"admin_system_import_skipped_label": "omitidos",
|
"admin_system_import_skipped_label": "omitidos",
|
||||||
"import_reason_invalid_pdf_signature": "Firma PDF no válida",
|
"import_reason_invalid_pdf_signature": "Firma PDF no válida",
|
||||||
"import_reason_file_read_error": "Error al leer el archivo",
|
"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_status_failed": "Importación fallida",
|
||||||
"admin_system_import_failed_no_spreadsheet": "No se encontró ninguna hoja de cálculo.",
|
"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.",
|
"admin_system_import_failed_internal": "Error interno durante la importación.",
|
||||||
|
|||||||
@@ -19,6 +19,8 @@ const failureMessage = $derived(
|
|||||||
function reasonLabel(code: string): string {
|
function reasonLabel(code: string): string {
|
||||||
if (code === 'INVALID_PDF_SIGNATURE') return m.import_reason_invalid_pdf_signature();
|
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 === '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;
|
return code;
|
||||||
}
|
}
|
||||||
</script>
|
</script>
|
||||||
@@ -54,38 +56,41 @@ function reasonLabel(code: string): string {
|
|||||||
</p>
|
</p>
|
||||||
<p class="mt-1 text-xs text-green-800">{m.admin_system_import_status_done()}</p>
|
<p class="mt-1 text-xs text-green-800">{m.admin_system_import_status_done()}</p>
|
||||||
</div>
|
</div>
|
||||||
{#if importStatus.skipped > 0}
|
<div aria-live="polite">
|
||||||
<details class="mb-4 rounded-sm border border-warning/40 bg-warning/10 p-4 text-amber-900">
|
{#if importStatus.skipped > 0}
|
||||||
<summary class="flex cursor-pointer list-none items-center gap-2">
|
<details class="mb-4 rounded-sm border border-warning/40 bg-warning/10 p-4 text-amber-900">
|
||||||
<svg
|
<summary class="flex cursor-pointer list-none items-center gap-2">
|
||||||
class="details-chevron h-4 w-4 shrink-0 motion-safe:transition-transform"
|
<svg
|
||||||
viewBox="0 0 16 16"
|
aria-hidden="true"
|
||||||
fill="none"
|
class="details-chevron h-4 w-4 shrink-0 motion-safe:transition-transform"
|
||||||
stroke="currentColor"
|
viewBox="0 0 16 16"
|
||||||
stroke-width="2"
|
fill="none"
|
||||||
stroke-linecap="round"
|
stroke="currentColor"
|
||||||
stroke-linejoin="round"
|
stroke-width="2"
|
||||||
>
|
stroke-linecap="round"
|
||||||
<path d="M6 4l4 4-4 4" />
|
stroke-linejoin="round"
|
||||||
</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">
|
<path d="M6 4l4 4-4 4" />
|
||||||
{m.admin_system_import_skipped_label()}
|
</svg>
|
||||||
</span>
|
<div>
|
||||||
</div>
|
<span data-testid="skipped-count" class="block text-base font-bold"
|
||||||
</summary>
|
>{importStatus.skipped}</span
|
||||||
<ul class="mt-3 space-y-1">
|
>
|
||||||
{#each importStatus.skippedFiles as skipped (skipped.filename)}
|
<span class="block font-sans text-xs font-bold tracking-widest uppercase">
|
||||||
<li class="font-mono text-sm text-ink-2">
|
{m.admin_system_import_skipped_label()}
|
||||||
{skipped.filename} — {reasonLabel(skipped.reason)}
|
</span>
|
||||||
</li>
|
</div>
|
||||||
{/each}
|
</summary>
|
||||||
</ul>
|
<ul class="mt-3 max-h-64 space-y-1 overflow-y-auto">
|
||||||
</details>
|
{#each importStatus.skippedFiles as skipped (skipped.filename)}
|
||||||
{/if}
|
<li class="font-mono text-sm text-ink-2">
|
||||||
|
{skipped.filename} — {reasonLabel(skipped.reason)}
|
||||||
|
</li>
|
||||||
|
{/each}
|
||||||
|
</ul>
|
||||||
|
</details>
|
||||||
|
{/if}
|
||||||
|
</div>
|
||||||
<button
|
<button
|
||||||
data-import-trigger
|
data-import-trigger
|
||||||
onclick={ontrigger}
|
onclick={ontrigger}
|
||||||
|
|||||||
Reference in New Issue
Block a user