marcel/familienarchiv
1
0
Fork 0
Code Issues 114 Pull Requests 3 Actions Packages Projects Releases Wiki Activity

fix(import): address PR review concerns
Some checks failed
CI / Unit & Component Tests (pull_request) Successful in 3m3s
Details
CI / OCR Service Tests (pull_request) Successful in 20s
Details
CI / Backend Unit Tests (pull_request) Failing after 3m2s
Details
CI / fail2ban Regex (pull_request) Successful in 39s
Details
CI / Semgrep Security Scan (pull_request) Successful in 19s
Details
CI / Compose Bucket Idempotency (pull_request) Successful in 1m1s
Details

Browse Source 
- remove duplicate List import in AdminControllerTest
- derive skipped() from skippedFiles.size() — drop redundant int field
- use machine codes for SkippedFile.reason (INVALID_PDF_SIGNATURE, FILE_READ_ERROR)
- map reason codes to i18n strings in ImportStatusCard (de/en/es)
- replace raw amber Tailwind classes with warning semantic token
- fix <summary> accessibility: replace list-none with rotating chevron SVG
- replace <p> with <span> inside <summary> (phrasing content rule)
- extract setupOneValidOneFakeImport() helper — remove 3x copy-paste
- add lenient mock to short-file test for defensive coverage
- add IOException path test for isPdfMagicBytes

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Marcel
2026-05-18 14:45:21 +02:00
parent 22589e4729
commit 50441f558f
8 changed files with 94 additions and 49 deletions
Download Patch File Download Diff File Expand all files Collapse all files

24
backend/src/main/java/org/raddatz/familienarchiv/importing/MassImportService.java
View File

@@ -1,6 +1,7 @@
package org.raddatz.familienarchiv.importing;
import com.fasterxml.jackson.annotation.JsonIgnore;
import com.fasterxml.jackson.annotation.JsonProperty;
import lombok.RequiredArgsConstructor;
import lombok.extern.slf4j.Slf4j;
import org.apache.poi.ss.usermodel.*;
@@ -56,11 +57,14 @@ public class MassImportService {
public record SkippedFile(String filename, String reason) {}
public record ImportStatus(State state, String statusCode, @JsonIgnore String message, int processed, int skipped, List<SkippedFile> skippedFiles, LocalDateTime startedAt) {}
public record ImportStatus(State state, String statusCode, @JsonIgnore String message, int processed, List<SkippedFile> skippedFiles, LocalDateTime startedAt) {
@JsonProperty("skipped")
public int skipped() { return skippedFiles.size(); }
}
record ProcessResult(int processed, int skipped, List<SkippedFile> skippedFiles) {}
record ProcessResult(int processed, List<SkippedFile> skippedFiles) {}
private volatile ImportStatus currentStatus = new ImportStatus(State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, 0, List.of(), null);
private volatile ImportStatus currentStatus = new ImportStatus(State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, List.of(), null);
public ImportStatus getStatus() {
return currentStatus;
@@ -122,22 +126,22 @@ public class MassImportService {
if (currentStatus.state() == State.RUNNING) {
throw DomainException.conflict(ErrorCode.IMPORT_ALREADY_RUNNING, "A mass import is already in progress");
}
currentStatus = new ImportStatus(State.RUNNING, "IMPORT_RUNNING", "Import läuft...", 0, 0, List.of(), LocalDateTime.now());
currentStatus = new ImportStatus(State.RUNNING, "IMPORT_RUNNING", "Import läuft...", 0, List.of(), LocalDateTime.now());
try {
File spreadsheet = findSpreadsheetFile();
log.info("Starte Massenimport aus: {}", spreadsheet.getAbsolutePath());
ProcessResult result = processRows(readSpreadsheet(spreadsheet));
currentStatus = new ImportStatus(State.DONE, "IMPORT_DONE",
"Import abgeschlossen. " + result.processed() + " Dokumente verarbeitet.",
result.processed(), result.skipped(), result.skippedFiles(), currentStatus.startedAt());
result.processed(), result.skippedFiles(), currentStatus.startedAt());
} catch (NoSpreadsheetException e) {
log.error("Massenimport fehlgeschlagen: keine Tabellendatei", e);
currentStatus = new ImportStatus(State.FAILED, "IMPORT_FAILED_NO_SPREADSHEET",
"Fehler: " + e.getMessage(), 0, 0, List.of(), currentStatus.startedAt());
"Fehler: " + e.getMessage(), 0, List.of(), currentStatus.startedAt());
} catch (Exception e) {
log.error("Massenimport fehlgeschlagen", e);
currentStatus = new ImportStatus(State.FAILED, "IMPORT_FAILED_INTERNAL",
"Fehler: " + e.getMessage(), 0, 0, List.of(), currentStatus.startedAt());
"Fehler: " + e.getMessage(), 0, List.of(), currentStatus.startedAt());
}
}
@@ -278,12 +282,12 @@ public class MassImportService {
try {
if (!isPdfMagicBytes(fileOnDisk.get())) {
log.warn("Überspringe {}: Datei beginnt nicht mit %PDF-Signatur", filename);
skippedFiles.add(new SkippedFile(filename, "Keine gültige PDF-Signatur"));
skippedFiles.add(new SkippedFile(filename, "INVALID_PDF_SIGNATURE"));
continue;
}
} catch (IOException e) {
log.error("Fehler beim Prüfen der Magic-Bytes für {}", filename, e);
skippedFiles.add(new SkippedFile(filename, "Fehler beim Lesen der Datei"));
skippedFiles.add(new SkippedFile(filename, "FILE_READ_ERROR"));
continue;
}
}
@@ -293,7 +297,7 @@ public class MassImportService {
processed++;
}
}
return new ProcessResult(processed, skippedFiles.size(), skippedFiles);
return new ProcessResult(processed, skippedFiles);
}
private boolean isPdfMagicBytes(File file) throws IOException {

59
backend/src/test/java/org/raddatz/familienarchiv/importing/MassImportServiceTest.java
View File

@@ -40,6 +40,7 @@ import java.util.zip.ZipOutputStream;
import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
import static org.junit.jupiter.api.Assumptions.assumeTrue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.*;
@@ -135,7 +136,7 @@ class MassImportServiceTest {
@Test
void runImportAsync_throwsConflict_whenAlreadyRunning() {
MassImportService.ImportStatus running = new MassImportService.ImportStatus(
MassImportService.State.RUNNING, "IMPORT_RUNNING", "Running...", 0, 0, List.of(), LocalDateTime.now());
MassImportService.State.RUNNING, "IMPORT_RUNNING", "Running...", 0, List.of(), LocalDateTime.now());
ReflectionTestUtils.setField(service, "currentStatus", running);
assertThatThrownBy(() -> service.runImportAsync())
@@ -529,14 +530,7 @@ class MassImportServiceTest {
@Test
void runImportAsync_uploadsValidPdf_andSkipsFakeOne(@TempDir Path tempDir) throws Exception {
byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF-
Files.write(tempDir.resolve("real.pdf"), pdfHeader);
Files.writeString(tempDir.resolve("fake.pdf"), "not a pdf");
buildMinimalImportXlsx(tempDir, "real.pdf", "fake.pdf");
ReflectionTestUtils.setField(service, "importDir", tempDir.toString());
when(documentService.findByOriginalFilename(any())).thenReturn(Optional.empty());
when(documentService.save(any())).thenAnswer(inv -> inv.getArgument(0));
setupOneValidOneFakeImport(tempDir);
service.runImportAsync();
@@ -545,14 +539,7 @@ class MassImportServiceTest {
@Test
void runImportAsync_setsSkippedCount_toOne_whenOneFakeFile(@TempDir Path tempDir) throws Exception {
byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF-
Files.write(tempDir.resolve("real.pdf"), pdfHeader);
Files.writeString(tempDir.resolve("fake.pdf"), "not a pdf");
buildMinimalImportXlsx(tempDir, "real.pdf", "fake.pdf");
ReflectionTestUtils.setField(service, "importDir", tempDir.toString());
when(documentService.findByOriginalFilename(any())).thenReturn(Optional.empty());
when(documentService.save(any())).thenAnswer(inv -> inv.getArgument(0));
setupOneValidOneFakeImport(tempDir);
service.runImportAsync();
@@ -561,14 +548,7 @@ class MassImportServiceTest {
@Test
void runImportAsync_includesRejectedFilename_inSkippedFiles(@TempDir Path tempDir) throws Exception {
byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF-
Files.write(tempDir.resolve("real.pdf"), pdfHeader);
Files.writeString(tempDir.resolve("fake.pdf"), "not a pdf");
buildMinimalImportXlsx(tempDir, "real.pdf", "fake.pdf");
ReflectionTestUtils.setField(service, "importDir", tempDir.toString());
when(documentService.findByOriginalFilename(any())).thenReturn(Optional.empty());
when(documentService.save(any())).thenAnswer(inv -> inv.getArgument(0));
setupOneValidOneFakeImport(tempDir);
service.runImportAsync();
@@ -582,12 +562,31 @@ class MassImportServiceTest {
Files.write(tempDir.resolve("tiny.pdf"), new byte[]{0x25, 0x50, 0x44}); // only 3 bytes
buildMinimalImportXlsx(tempDir, "tiny.pdf");
ReflectionTestUtils.setField(service, "importDir", tempDir.toString());
lenient().when(documentService.findByOriginalFilename(any())).thenReturn(Optional.empty());
service.runImportAsync();
assertThat(service.getStatus().skipped()).isEqualTo(1);
}
@Test
void runImportAsync_skipsFile_whenMagicBytesCheckThrowsIOException(@TempDir Path tempDir) throws Exception {
Files.writeString(tempDir.resolve("unreadable.pdf"), "some content");
File unreadable = tempDir.resolve("unreadable.pdf").toFile();
buildMinimalImportXlsx(tempDir, "unreadable.pdf");
ReflectionTestUtils.setField(service, "importDir", tempDir.toString());
assumeTrue(unreadable.setReadable(false), "Requires non-root file permissions");
try {
service.runImportAsync();
assertThat(service.getStatus().skipped()).isEqualTo(1);
assertThat(service.getStatus().skippedFiles())
.extracting(MassImportService.SkippedFile::reason)
.containsExactly("FILE_READ_ERROR");
} finally {
unreadable.setReadable(true);
}
}
// ─── readOds — XXE security regression ───────────────────────────────────
// Security regression — do not remove.
@@ -685,6 +684,16 @@ class MassImportServiceTest {
return destination.toFile();
}
private void setupOneValidOneFakeImport(Path tempDir) throws Exception {
byte[] pdfHeader = {0x25, 0x50, 0x44, 0x46, 0x2D}; // %PDF-
Files.write(tempDir.resolve("real.pdf"), pdfHeader);
Files.writeString(tempDir.resolve("fake.pdf"), "not a pdf");
buildMinimalImportXlsx(tempDir, "real.pdf", "fake.pdf");
ReflectionTestUtils.setField(service, "importDir", tempDir.toString());
when(documentService.findByOriginalFilename(any())).thenReturn(Optional.empty());
when(documentService.save(any())).thenAnswer(inv -> inv.getArgument(0));
}
private void buildMinimalImportXlsx(Path dir, String... filenames) throws Exception {
Path xlsx = dir.resolve("import.xlsx");
try (XSSFWorkbook wb = new XSSFWorkbook()) {

5
backend/src/test/java/org/raddatz/familienarchiv/user/AdminControllerTest.java
View File

@@ -19,7 +19,6 @@ import org.springframework.test.web.servlet.MockMvc;
import java.time.LocalDateTime;
import java.util.List;
import java.util.List;
import static org.mockito.ArgumentMatchers.anyList;
import static org.mockito.Mockito.verify;
@@ -47,7 +46,7 @@ class AdminControllerTest {
@WithMockUser(authorities = "ADMIN")
void importStatus_returns200_withStatusCode_whenAdmin() throws Exception {
MassImportService.ImportStatus status = new MassImportService.ImportStatus(
MassImportService.State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, 0, List.of(), null);
MassImportService.State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, List.of(), null);
when(massImportService.getStatus()).thenReturn(status);
mockMvc.perform(get("/api/admin/import-status"))
@@ -61,7 +60,7 @@ class AdminControllerTest {
@WithMockUser(authorities = "ADMIN")
void importStatus_messageField_notPresentInApiResponse() throws Exception {
MassImportService.ImportStatus status = new MassImportService.ImportStatus(
MassImportService.State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, 0, List.of(), null);
MassImportService.State.IDLE, "IMPORT_IDLE", "Kein Import gestartet.", 0, List.of(), null);
when(massImportService.getStatus()).thenReturn(status);
mockMvc.perform(get("/api/admin/import-status"))

2
frontend/messages/de.json
View File

@@ -351,6 +351,8 @@
"admin_system_import_status_done": "Import abgeschlossen",
"admin_system_import_status_done_label": "Dokumente verarbeitet",
"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",
"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.",

2
frontend/messages/en.json
View File

@@ -351,6 +351,8 @@
"admin_system_import_status_done": "Import complete",
"admin_system_import_status_done_label": "Documents processed",
"admin_system_import_skipped_label": "skipped",
"import_reason_invalid_pdf_signature": "Invalid PDF signature",
"import_reason_file_read_error": "File read error",
"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.",

2
frontend/messages/es.json
View File

@@ -351,6 +351,8 @@
"admin_system_import_status_done": "Importación completada",
"admin_system_import_status_done_label": "Documentos procesados",
"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",
"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.",

41
frontend/src/routes/admin/system/ImportStatusCard.svelte
View File

@@ -15,6 +15,12 @@ const failureMessage = $derived(
? m.admin_system_import_failed_no_spreadsheet()
: m.admin_system_import_failed_internal()
);
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();
return code;
}
</script>
<div class="rounded-sm border border-line bg-surface p-6 shadow-sm">
@@ -49,16 +55,31 @@ const failureMessage = $derived(
<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-amber-200 bg-amber-50 p-4 text-amber-700">
<summary class="cursor-pointer list-none">
<p data-testid="skipped-count" class="text-base font-bold">{importStatus.skipped}</p>
<p class="font-sans text-xs font-bold tracking-widest text-amber-800 uppercase">
{m.admin_system_import_skipped_label()}
</p>
<details class="mb-4 rounded-sm border border-warning/40 bg-warning/10 p-4 text-warning">
<summary class="flex cursor-pointer list-none items-center gap-2">
<svg
class="details-chevron h-4 w-4 shrink-0 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
>
<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 { filename, reason } (filename)}
<li class="font-mono text-xs text-ink-2">{filename} — {reason}</li>
<li class="font-mono text-xs text-ink-2">{filename} — {reasonLabel(reason)}</li>
{/each}
</ul>
</details>
@@ -94,3 +115,9 @@ const failureMessage = $derived(
</button>
{/if}
</div>
<style>
details[open] .details-chevron {
transform: rotate(90deg);
}
</style>

8
frontend/src/routes/admin/system/ImportStatusCard.svelte.test.ts
View File

@@ -140,9 +140,9 @@ describe('ImportStatusCard', () => {
processed: 10,
skipped: 3,
skippedFiles: [
{ filename: 'fake.pdf', reason: 'Keine gültige PDF-Signatur' },
{ filename: 'other.pdf', reason: 'Keine gültige PDF-Signatur' },
{ filename: 'tiny.pdf', reason: 'Keine gültige PDF-Signatur' }
{ filename: 'fake.pdf', reason: 'INVALID_PDF_SIGNATURE' },
{ filename: 'other.pdf', reason: 'INVALID_PDF_SIGNATURE' },
{ filename: 'tiny.pdf', reason: 'INVALID_PDF_SIGNATURE' }
]
}),
ontrigger: () => {}
@@ -160,7 +160,7 @@ describe('ImportStatusCard', () => {
statusCode: 'IMPORT_DONE',
processed: 5,
skipped: 1,
skippedFiles: [{ filename: 'fake.pdf', reason: 'Keine gültige PDF-Signatur' }]
skippedFiles: [{ filename: 'fake.pdf', reason: 'INVALID_PDF_SIGNATURE' }]
}),
ontrigger: () => {}
}