From 6463a32dfcec47e41f1211db00f12a945aa90578 Mon Sep 17 00:00:00 2001 From: Marcel Date: Sun, 5 Apr 2026 11:43:35 +0200 Subject: [PATCH] =?UTF-8?q?fix:=20address=20PR=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20security,=20architecture,=20dead=20code?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes from PR #178 review: Migration fixes: - V18/V19: fix FK references from app_users to users (correct table name) - V18: change annotation_id FK from ON DELETE CASCADE to ON DELETE RESTRICT (block is aggregate root, cascade flows from block, not annotation) Backend fixes: - TranscriptionService.deleteBlock(): remove userId param, delete block first then annotation directly via repository (no ownership check — block owns annotation) - TranscriptionService.sanitizeText(): remove flawed regex HTML stripping, textarea content is plain text by design — just enforce max length - TranscriptionBlockController.requireUserId(): throw DomainException.unauthorized() instead of silently returning null on auth failure - CreateTranscriptionBlockDTO: add @Min/@Positive validation on coordinates - Add @Slf4j logging to TranscriptionService for create/delete operations Frontend fixes: - Delete DocumentBottomPanel.svelte entirely (issue #175 requirement) - Remove redundant mode exclusivity $effect (handled at toggle call sites) - Remove dead handleCommentClick + onCommentClick prop (comments are future work) - Remove quote hint UI (depends on comment feature) Co-Authored-By: Claude Sonnet 4.6 --- .../TranscriptionBlockController.java | 27 ++- .../dto/CreateTranscriptionBlockDTO.java | 7 + .../service/TranscriptionService.java | 25 ++- .../V18__add_transcription_blocks.sql | 6 +- .../V19__add_transcription_block_versions.sql | 2 +- .../lib/components/DocumentBottomPanel.svelte | 193 ------------------ .../lib/components/TranscriptionBlock.svelte | 19 +- .../components/TranscriptionEditView.svelte | 5 - .../src/routes/documents/[id]/+page.svelte | 7 - 9 files changed, 41 insertions(+), 250 deletions(-) delete mode 100644 frontend/src/lib/components/DocumentBottomPanel.svelte diff --git a/backend/src/main/java/org/raddatz/familienarchiv/controller/TranscriptionBlockController.java b/backend/src/main/java/org/raddatz/familienarchiv/controller/TranscriptionBlockController.java index a7af973d..ed622826 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/controller/TranscriptionBlockController.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/controller/TranscriptionBlockController.java @@ -5,6 +5,7 @@ import lombok.extern.slf4j.Slf4j; import org.raddatz.familienarchiv.dto.CreateTranscriptionBlockDTO; import org.raddatz.familienarchiv.dto.ReorderTranscriptionBlocksDTO; import org.raddatz.familienarchiv.dto.UpdateTranscriptionBlockDTO; +import org.raddatz.familienarchiv.exception.DomainException; import org.raddatz.familienarchiv.model.AppUser; import org.raddatz.familienarchiv.model.TranscriptionBlock; import org.raddatz.familienarchiv.model.TranscriptionBlockVersion; @@ -47,7 +48,7 @@ public class TranscriptionBlockController { @PathVariable UUID documentId, @RequestBody CreateTranscriptionBlockDTO dto, Authentication authentication) { - UUID userId = resolveUserId(authentication); + UUID userId = requireUserId(authentication); return transcriptionService.createBlock(documentId, dto, userId); } @@ -58,7 +59,7 @@ public class TranscriptionBlockController { @PathVariable UUID blockId, @RequestBody UpdateTranscriptionBlockDTO dto, Authentication authentication) { - UUID userId = resolveUserId(authentication); + UUID userId = requireUserId(authentication); return transcriptionService.updateBlock(documentId, blockId, dto, userId); } @@ -67,10 +68,8 @@ public class TranscriptionBlockController { @RequirePermission(Permission.WRITE_ALL) public void deleteBlock( @PathVariable UUID documentId, - @PathVariable UUID blockId, - Authentication authentication) { - UUID userId = resolveUserId(authentication); - transcriptionService.deleteBlock(documentId, blockId, userId); + @PathVariable UUID blockId) { + transcriptionService.deleteBlock(documentId, blockId); } @PutMapping("/reorder") @@ -89,14 +88,14 @@ public class TranscriptionBlockController { return transcriptionService.getBlockHistory(documentId, blockId); } - private UUID resolveUserId(Authentication authentication) { - if (authentication == null || !authentication.isAuthenticated()) return null; - try { - AppUser user = userService.findByUsername(authentication.getName()); - return user != null ? user.getId() : null; - } catch (Exception e) { - log.warn("Could not resolve user for transcription: {}", e.getMessage()); - return null; + private UUID requireUserId(Authentication authentication) { + if (authentication == null || !authentication.isAuthenticated()) { + throw DomainException.unauthorized("Authentication required"); } + AppUser user = userService.findByUsername(authentication.getName()); + if (user == null) { + throw DomainException.unauthorized("User not found"); + } + return user.getId(); } } diff --git a/backend/src/main/java/org/raddatz/familienarchiv/dto/CreateTranscriptionBlockDTO.java b/backend/src/main/java/org/raddatz/familienarchiv/dto/CreateTranscriptionBlockDTO.java index e1d932c0..90f46359 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/dto/CreateTranscriptionBlockDTO.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/dto/CreateTranscriptionBlockDTO.java @@ -1,5 +1,7 @@ package org.raddatz.familienarchiv.dto; +import jakarta.validation.constraints.Min; +import jakarta.validation.constraints.Positive; import lombok.AllArgsConstructor; import lombok.Data; import lombok.NoArgsConstructor; @@ -8,10 +10,15 @@ import lombok.NoArgsConstructor; @NoArgsConstructor @AllArgsConstructor public class CreateTranscriptionBlockDTO { + @Min(0) private int pageNumber; + @Min(0) private double x; + @Min(0) private double y; + @Positive private double width; + @Positive private double height; private String text; private String label; diff --git a/backend/src/main/java/org/raddatz/familienarchiv/service/TranscriptionService.java b/backend/src/main/java/org/raddatz/familienarchiv/service/TranscriptionService.java index 37440cc2..caf8604c 100644 --- a/backend/src/main/java/org/raddatz/familienarchiv/service/TranscriptionService.java +++ b/backend/src/main/java/org/raddatz/familienarchiv/service/TranscriptionService.java @@ -12,6 +12,7 @@ import org.raddatz.familienarchiv.model.Document; import org.raddatz.familienarchiv.model.DocumentAnnotation; import org.raddatz.familienarchiv.model.TranscriptionBlock; import org.raddatz.familienarchiv.model.TranscriptionBlockVersion; +import org.raddatz.familienarchiv.repository.AnnotationRepository; import org.raddatz.familienarchiv.repository.TranscriptionBlockRepository; import org.raddatz.familienarchiv.repository.TranscriptionBlockVersionRepository; import org.springframework.stereotype.Service; @@ -30,6 +31,7 @@ public class TranscriptionService { private final TranscriptionBlockRepository blockRepository; private final TranscriptionBlockVersionRepository versionRepository; + private final AnnotationRepository annotationRepository; private final AnnotationService annotationService; private final DocumentService documentService; @@ -69,6 +71,7 @@ public class TranscriptionService { TranscriptionBlock saved = blockRepository.save(block); saveVersion(saved, userId); + log.info("Created transcription block {} for document {}", saved.getId(), documentId); return saved; } @@ -90,11 +93,17 @@ public class TranscriptionService { } @Transactional - public void deleteBlock(UUID documentId, UUID blockId, UUID userId) { + public void deleteBlock(UUID documentId, UUID blockId) { TranscriptionBlock block = getBlock(documentId, blockId); - // CASCADE deletes annotation, versions, and comments via DB constraints + UUID annotationId = block.getAnnotationId(); + + // Block is the aggregate root — delete block first (cascades to versions + comments), + // then delete the dependent annotation directly (no ownership check needed) blockRepository.delete(block); - annotationService.deleteAnnotation(documentId, block.getAnnotationId(), userId); + blockRepository.flush(); + annotationRepository.deleteById(annotationId); + log.info("Deleted transcription block {} and annotation {} for document {}", + blockId, annotationId, documentId); } @Transactional @@ -122,13 +131,11 @@ public class TranscriptionService { versionRepository.save(version); } - private String sanitizeText(String text) { + String sanitizeText(String text) { if (text == null) return ""; - // Strip any HTML tags — textarea content should be plain text only - String cleaned = text.replaceAll("<[^>]*>", ""); - if (cleaned.length() > MAX_TEXT_LENGTH) { - cleaned = cleaned.substring(0, MAX_TEXT_LENGTH); + if (text.length() > MAX_TEXT_LENGTH) { + text = text.substring(0, MAX_TEXT_LENGTH); } - return cleaned; + return text; } } diff --git a/backend/src/main/resources/db/migration/V18__add_transcription_blocks.sql b/backend/src/main/resources/db/migration/V18__add_transcription_blocks.sql index 03e5aaf8..524a2649 100644 --- a/backend/src/main/resources/db/migration/V18__add_transcription_blocks.sql +++ b/backend/src/main/resources/db/migration/V18__add_transcription_blocks.sql @@ -1,13 +1,13 @@ CREATE TABLE transcription_blocks ( id UUID PRIMARY KEY DEFAULT gen_random_uuid(), - annotation_id UUID NOT NULL REFERENCES document_annotations(id) ON DELETE CASCADE, + annotation_id UUID NOT NULL REFERENCES document_annotations(id) ON DELETE RESTRICT, document_id UUID NOT NULL REFERENCES documents(id) ON DELETE CASCADE, text TEXT NOT NULL DEFAULT '', label VARCHAR(200), sort_order INTEGER NOT NULL DEFAULT 0, version INTEGER NOT NULL DEFAULT 0, - created_by UUID REFERENCES app_users(id) ON DELETE SET NULL, - updated_by UUID REFERENCES app_users(id) ON DELETE SET NULL, + created_by UUID REFERENCES users(id) ON DELETE SET NULL, + updated_by UUID REFERENCES users(id) ON DELETE SET NULL, created_at TIMESTAMP NOT NULL DEFAULT now(), updated_at TIMESTAMP NOT NULL DEFAULT now() ); diff --git a/backend/src/main/resources/db/migration/V19__add_transcription_block_versions.sql b/backend/src/main/resources/db/migration/V19__add_transcription_block_versions.sql index 54df152c..2c03ab2c 100644 --- a/backend/src/main/resources/db/migration/V19__add_transcription_block_versions.sql +++ b/backend/src/main/resources/db/migration/V19__add_transcription_block_versions.sql @@ -2,7 +2,7 @@ CREATE TABLE transcription_block_versions ( id UUID PRIMARY KEY DEFAULT gen_random_uuid(), block_id UUID NOT NULL REFERENCES transcription_blocks(id) ON DELETE CASCADE, text TEXT NOT NULL, - changed_by UUID REFERENCES app_users(id) ON DELETE SET NULL, + changed_by UUID REFERENCES users(id) ON DELETE SET NULL, changed_at TIMESTAMP NOT NULL DEFAULT now() ); diff --git a/frontend/src/lib/components/DocumentBottomPanel.svelte b/frontend/src/lib/components/DocumentBottomPanel.svelte deleted file mode 100644 index 209bf76e..00000000 --- a/frontend/src/lib/components/DocumentBottomPanel.svelte +++ /dev/null @@ -1,193 +0,0 @@ - - -
- - - - -
- -
- {#each tabs as tab (tab.id)} - - {/each} -
- - {#if open} - - {/if} -
- - - {#if open} -
- {#if activeTab === 'metadata'} - - {:else if activeTab === 'transcription'} - - {:else if activeTab === 'discussion'} - - {:else if activeTab === 'history'} - - {/if} -
- {/if} -
diff --git a/frontend/src/lib/components/TranscriptionBlock.svelte b/frontend/src/lib/components/TranscriptionBlock.svelte index 6dfbe548..7f66eb6e 100644 --- a/frontend/src/lib/components/TranscriptionBlock.svelte +++ b/frontend/src/lib/components/TranscriptionBlock.svelte @@ -12,7 +12,6 @@ type Props = { saveState: SaveState; onTextChange: (text: string) => void; onFocus: () => void; - onCommentClick: () => void; onDeleteClick: () => void; onRetry: () => void; }; @@ -26,7 +25,6 @@ let { saveState, onTextChange, onFocus, - onCommentClick, onDeleteClick, onRetry }: Props = $props(); @@ -91,22 +89,7 @@ function handleDelete() { > -
-
- - {#if active} - - {m.transcription_block_quote_hint()} - - {/if} -
- +
{#if saveState === 'saving'} diff --git a/frontend/src/lib/components/TranscriptionEditView.svelte b/frontend/src/lib/components/TranscriptionEditView.svelte index 34067c61..77a2f742 100644 --- a/frontend/src/lib/components/TranscriptionEditView.svelte +++ b/frontend/src/lib/components/TranscriptionEditView.svelte @@ -119,10 +119,6 @@ function handleDelete(blockId: string) { onDeleteBlock(blockId); } -function handleCommentClick() { - // Placeholder for future comment functionality -} - $effect(() => { function onBeforeUnload() { flushAllPending(); @@ -153,7 +149,6 @@ $effect(() => { saveState={getSaveState(block.id)} onTextChange={(text) => handleTextChange(block.id, text)} onFocus={() => handleFocus(block.id)} - onCommentClick={handleCommentClick} onDeleteClick={() => handleDelete(block.id)} onRetry={() => handleRetry(block.id)} /> diff --git a/frontend/src/routes/documents/[id]/+page.svelte b/frontend/src/routes/documents/[id]/+page.svelte index 543c7178..c95dc4de 100644 --- a/frontend/src/routes/documents/[id]/+page.svelte +++ b/frontend/src/routes/documents/[id]/+page.svelte @@ -63,13 +63,6 @@ let transcribeMode = $state(false); let activeAnnotationId = $state(null); let activeAnnotationPage = $state(null); -// Mode exclusivity: entering one mode exits the other -$effect(() => { - if (annotateMode && transcribeMode) { - transcribeMode = false; - } -}); - // ── Transcription blocks ───────────────────────────────────────────────────── type TranscriptionBlockData = {