feat(audit): domain-level audit log for archive activity #275

Merged

marcel merged 5 commits from feat/issue-274-audit-log into main 2026-04-19 16:10:42 +02:00

Conversation 15 Commits 5 Files Changed 20 +782 -55

marcel commented 2026-04-19 13:42:01 +02:00

Owner

Copy Link

Closes #274

Summary

Adds a dedicated audit_log table and AuditService that records meaningful state transitions across the domain — who transcribed what, who uploaded which file, when metadata changed. Audit writes are async (@Async("auditExecutor")) and deferred past transaction commit via afterCommit() hooks to eliminate FK timing races. A dedicated thread pool (CallerRunsPolicy) degrades gracefully under load instead of dropping events.

Events recorded:

• FILE_UPLOADED — when a PLACEHOLDER document receives its first file (via storeDocument or attachFile)
• STATUS_CHANGED — when a document's status transitions (payload: {oldStatus, newStatus})
• METADATA_UPDATED — when document metadata is updated without a status change
• TEXT_SAVED — when a transcription block's text actually changes (payload: {pageNumber})
• BLOCK_REVIEWED — when a block is toggled from unreviewed → reviewed
• ANNOTATION_CREATED — when a new annotation is created manually (not via OCR) (payload: {pageNumber})

Append-only enforcement: REVOKE UPDATE, DELETE ON audit_log FROM app_user in V46 migration. Actor column is ON DELETE SET NULL for GDPR right-to-erasure compliance.

Closes #274 ## Summary Adds a dedicated `audit_log` table and `AuditService` that records meaningful state transitions across the domain — who transcribed what, who uploaded which file, when metadata changed. Audit writes are async (`@Async("auditExecutor")`) and deferred past transaction commit via `afterCommit()` hooks to eliminate FK timing races. A dedicated thread pool (`CallerRunsPolicy`) degrades gracefully under load instead of dropping events. **Events recorded:** - `FILE_UPLOADED` — when a PLACEHOLDER document receives its first file (via `storeDocument` or `attachFile`) - `STATUS_CHANGED` — when a document's status transitions (payload: `{oldStatus, newStatus}`) - `METADATA_UPDATED` — when document metadata is updated without a status change - `TEXT_SAVED` — when a transcription block's text actually changes (payload: `{pageNumber}`) - `BLOCK_REVIEWED` — when a block is toggled from unreviewed → reviewed - `ANNOTATION_CREATED` — when a new annotation is created manually (not via OCR) (payload: `{pageNumber}`) **Append-only enforcement:** `REVOKE UPDATE, DELETE ON audit_log FROM app_user` in V46 migration. Actor column is `ON DELETE SET NULL` for GDPR right-to-erasure compliance.

marcel added 3 commits 2026-04-19 13:42:02 +02:00

feat(audit): add audit_log infrastructure and instrument AnnotationService ... 793b863096

- V46 migration: audit_log table with indexes and append-only REVOKE
- audit/ package: AuditKind enum (with Javadoc payloads), AuditLog entity,
  AuditLogRepository, AuditService (@Async on dedicated auditExecutor)
- AsyncConfig: auditExecutor with CallerRunsPolicy and queueCapacity 50
- AnnotationService: ANNOTATION_CREATED on createAnnotation() only,
  deferred via afterCommit() when inside a transaction

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(audit): instrument TranscriptionService for TEXT_SAVED and BLOCK_REVIEWED ... 9887968236

- reviewBlock: add userId param; log BLOCK_REVIEWED only on false→true
- updateBlock: log TEXT_SAVED only when text actually changes; include
  pageNumber in payload (resolved from annotation)
- Both events deferred via afterCommit() when inside a transaction
- Update TranscriptionBlockController to pass user to reviewBlock()

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

feat(audit): instrument DocumentService for METADATA_UPDATED, STATUS_CHANGED, FILE_UPLOADED ... 2deaaf167e

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-04-19 13:43:47 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

TDD evidence is solid — tests clearly precede the production code, names are descriptive, and the audit condition tests (text changes vs. unchanged, false→true vs true→false) are exactly right. No complaints there.

Blockers

None.

Suggestions

logAfterCommit() is copy-pasted verbatim into three service classes.

AnnotationService, TranscriptionService, and DocumentService all contain an identical private method:

private void logAfterCommit(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) {
    if (TransactionSynchronizationManager.isActualTransactionActive()) {
        TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
            @Override public void afterCommit() {
                auditService.log(kind, actorId, documentId, payload);
            }
        });
    } else {
        auditService.log(kind, actorId, documentId, payload);
    }
}

Three identical 13-line methods is the clearest DRY violation in this PR. This belongs in AuditService itself as a public helper — it's an audit concern, not a DocumentService concern:

// AuditService
public void logAfterCommit(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) {
    if (TransactionSynchronizationManager.isActualTransactionActive()) {
        TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() {
            @Override public void afterCommit() { log(kind, actorId, documentId, payload); }
        });
    } else {
        log(kind, actorId, documentId, payload);
    }
}

Call sites become auditService.logAfterCommit(...) — one copy, one test, one place to update if the synchronization strategy ever changes.

updateDocument() is accumulating responsibilities.

The method now handles: simple field updates, tag resolution, sender linking, receiver linking, metadata-complete flag, script type, file replacement, version recording, and audit logging. Seven concerns in one method. The audit instrumentation lands at the bottom after ~60 lines. This isn't a blocker for this PR but is worth a follow-up refactor — applyMetadata(), resolveRelationships(), replaceFile() helpers would make the audit instrumentation visible without scrolling.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** TDD evidence is solid — tests clearly precede the production code, names are descriptive, and the audit condition tests (text changes vs. unchanged, `false→true` vs `true→false`) are exactly right. No complaints there. ### Blockers None. ### Suggestions **`logAfterCommit()` is copy-pasted verbatim into three service classes.** `AnnotationService`, `TranscriptionService`, and `DocumentService` all contain an identical private method: ```java private void logAfterCommit(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) { if (TransactionSynchronizationManager.isActualTransactionActive()) { TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { @Override public void afterCommit() { auditService.log(kind, actorId, documentId, payload); } }); } else { auditService.log(kind, actorId, documentId, payload); } } ``` Three identical 13-line methods is the clearest DRY violation in this PR. This belongs in `AuditService` itself as a public helper — it's an audit concern, not a DocumentService concern: ```java // AuditService public void logAfterCommit(AuditKind kind, UUID actorId, UUID documentId, Map<String, Object> payload) { if (TransactionSynchronizationManager.isActualTransactionActive()) { TransactionSynchronizationManager.registerSynchronization(new TransactionSynchronization() { @Override public void afterCommit() { log(kind, actorId, documentId, payload); } }); } else { log(kind, actorId, documentId, payload); } } ``` Call sites become `auditService.logAfterCommit(...)` — one copy, one test, one place to update if the synchronization strategy ever changes. **`updateDocument()` is accumulating responsibilities.** The method now handles: simple field updates, tag resolution, sender linking, receiver linking, metadata-complete flag, script type, file replacement, version recording, and audit logging. Seven concerns in one method. The audit instrumentation lands at the bottom after ~60 lines. This isn't a blocker for this PR but is worth a follow-up refactor — `applyMetadata()`, `resolveRelationships()`, `replaceFile()` helpers would make the audit instrumentation visible without scrolling.

marcel commented 2026-04-19 13:44:00 +02:00

Author

Owner

Copy Link

🏗️ Markus Keller — Application Architect

Verdict: ⚠️ Approved with concerns

The audit/ package as a feature package is the right call. AuditKind, AuditLog, AuditLogRepository, and AuditService all belong together and are cleanly separated from the service layer. The CallerRunsPolicy and dedicated executor are good operational decisions — I raised those points in the original design and they're addressed correctly.

Blockers

None.

Suggestions

Inconsistent actor resolution pattern across instrumented services.

There are two patterns in this PR for how the userId reaches the audit call:

Service	Pattern
TranscriptionService.reviewBlock()	Receives UUID userId as an explicit parameter, passed from the controller
AnnotationService.createAnnotation()	Receives UUID userId as an existing explicit parameter
DocumentService.updateDocument()	Calls resolveCurrentUserId() internally via SecurityContextHolder

DocumentService is now the only service that reaches into the security context directly. This means:

1. DocumentService has a hidden dependency on SecurityContextHolder that doesn't appear in its constructor — invisible to tests without extra setup.
2. The unit tests work today only because resolveCurrentUserId() returns null when no auth context is present, and the tests assert isNull(). But this masks the actor rather than testing it.
3. If updateDocument() is ever called from a background thread (batch import, scheduled task), the security context will not propagate and actor attribution silently disappears.

The established pattern — controller resolves user, passes actorId to the service method — should be extended to DocumentService as well:

// Controller resolves, service receives
@PutMapping("/{id}")
public Document updateDocument(@PathVariable UUID id, @RequestBody DocumentUpdateDTO dto,
                                @RequestPart(required = false) MultipartFile file,
                                Authentication auth) {
    UUID actorId = requireUserId(auth);
    return documentService.updateDocument(id, dto, file, actorId);
}

This aligns with how reviewBlock, createBlock, and updateBlock already work.

logAfterCommit() duplication across three services.

Three identical helper methods belong in AuditService.logAfterCommit(). Each service currently owns infrastructure code that is not its domain concern. Felix has also flagged this — extract to AuditService and the duplication disappears.

## 🏗️ Markus Keller — Application Architect **Verdict: ⚠️ Approved with concerns** The `audit/` package as a feature package is the right call. `AuditKind`, `AuditLog`, `AuditLogRepository`, and `AuditService` all belong together and are cleanly separated from the service layer. The `CallerRunsPolicy` and dedicated executor are good operational decisions — I raised those points in the original design and they're addressed correctly. ### Blockers None. ### Suggestions **Inconsistent actor resolution pattern across instrumented services.** There are two patterns in this PR for how the `userId` reaches the audit call: | Service | Pattern | |---|---| | `TranscriptionService.reviewBlock()` | Receives `UUID userId` as an explicit parameter, passed from the controller | | `AnnotationService.createAnnotation()` | Receives `UUID userId` as an existing explicit parameter | | `DocumentService.updateDocument()` | Calls `resolveCurrentUserId()` internally via `SecurityContextHolder` | `DocumentService` is now the only service that reaches into the security context directly. This means: 1. `DocumentService` has a hidden dependency on `SecurityContextHolder` that doesn't appear in its constructor — invisible to tests without extra setup. 2. The unit tests work today only because `resolveCurrentUserId()` returns `null` when no auth context is present, and the tests assert `isNull()`. But this masks the actor rather than testing it. 3. If `updateDocument()` is ever called from a background thread (batch import, scheduled task), the security context will not propagate and actor attribution silently disappears. The established pattern — controller resolves user, passes `actorId` to the service method — should be extended to `DocumentService` as well: ```java // Controller resolves, service receives @PutMapping("/{id}") public Document updateDocument(@PathVariable UUID id, @RequestBody DocumentUpdateDTO dto, @RequestPart(required = false) MultipartFile file, Authentication auth) { UUID actorId = requireUserId(auth); return documentService.updateDocument(id, dto, file, actorId); } ``` This aligns with how `reviewBlock`, `createBlock`, and `updateBlock` already work. **`logAfterCommit()` duplication across three services.** Three identical helper methods belong in `AuditService.logAfterCommit()`. Each service currently owns infrastructure code that is not its domain concern. Felix has also flagged this — extract to `AuditService` and the duplication disappears.

marcel commented 2026-04-19 13:44:14 +02:00

Author

Owner

Copy Link

🔒 Nora "NullX" Steiner — Security Engineer

Verdict: ✅ Approved

This is a well-considered audit implementation from a security standpoint. Let me walk through what I checked.

What I checked

Append-only enforcement — REVOKE UPDATE, DELETE ON audit_log FROM app_user in V46 is exactly right. The application role can insert but cannot tamper with historical records. This is the database-layer guarantee that no application bug can bypass.

GDPR right-to-erasure design — actor_id ... ON DELETE SET NULL is the correct choice. Deleting a user wipes their PII (email, name) from app_users while retaining the audit trail's temporal integrity. The comment in the migration documents the intent — good.

document_id ... ON DELETE CASCADE — also correct. When a document is deleted, its audit trail is irrelevant and should go with it. No orphaned rows accumulate.

No injection vectors — The payload field is a Map<String, Object> populated entirely from trusted internal values: pageNumber (integer from the database), oldStatus/newStatus (.name() of an enum). No user-controlled input flows into the payload. No injection risk.

Parameterized logging — log.error("Audit log write failed: kind={}, document={}", kind, documentId, e) uses SLF4J's parameterized form. No Log4Shell concern.

Security smell (not a blocker)

resolveCurrentUserId() silently returns null without any log entry.

private UUID resolveCurrentUserId() {
    Authentication auth = SecurityContextHolder.getContext().getAuthentication();
    if (auth == null || !auth.isAuthenticated()) return null;   // silent
    String email = auth.getName();
    if (email == null) return null;                              // silent
    AppUser user = userService.findByEmail(email);
    return user != null ? user.getId() : null;                  // silent
}

Three return-null paths with no log output. In production, if a batch import or scheduled task calls an instrumented method without an authenticated context, audit records will be written with actor_id = NULL. The schema allows this and the design documents it for GDPR, so the behavior is intentional. But a log.debug() on the null paths would make it observable during debugging — right now you'd have to look at the audit table directly to notice that actor attribution is missing.

Not a security vulnerability, but a diagnostic gap worth closing before the audit data starts being used for compliance reporting.

## 🔒 Nora "NullX" Steiner — Security Engineer **Verdict: ✅ Approved** This is a well-considered audit implementation from a security standpoint. Let me walk through what I checked. ### What I checked **Append-only enforcement** — `REVOKE UPDATE, DELETE ON audit_log FROM app_user` in V46 is exactly right. The application role can insert but cannot tamper with historical records. This is the database-layer guarantee that no application bug can bypass. **GDPR right-to-erasure design** — `actor_id ... ON DELETE SET NULL` is the correct choice. Deleting a user wipes their PII (email, name) from `app_users` while retaining the audit trail's temporal integrity. The comment in the migration documents the intent — good. **`document_id ... ON DELETE CASCADE`** — also correct. When a document is deleted, its audit trail is irrelevant and should go with it. No orphaned rows accumulate. **No injection vectors** — The `payload` field is a `Map<String, Object>` populated entirely from trusted internal values: `pageNumber` (integer from the database), `oldStatus`/`newStatus` (`.name()` of an enum). No user-controlled input flows into the payload. No injection risk. **Parameterized logging** — `log.error("Audit log write failed: kind={}, document={}", kind, documentId, e)` uses SLF4J's parameterized form. No Log4Shell concern. ### Security smell (not a blocker) **`resolveCurrentUserId()` silently returns `null` without any log entry.** ```java private UUID resolveCurrentUserId() { Authentication auth = SecurityContextHolder.getContext().getAuthentication(); if (auth == null || !auth.isAuthenticated()) return null; // silent String email = auth.getName(); if (email == null) return null; // silent AppUser user = userService.findByEmail(email); return user != null ? user.getId() : null; // silent } ``` Three return-null paths with no log output. In production, if a batch import or scheduled task calls an instrumented method without an authenticated context, audit records will be written with `actor_id = NULL`. The schema allows this and the design documents it for GDPR, so the behavior is intentional. But a `log.debug()` on the null paths would make it observable during debugging — right now you'd have to look at the audit table directly to notice that actor attribution is missing. Not a security vulnerability, but a diagnostic gap worth closing before the audit data starts being used for compliance reporting.

marcel commented 2026-04-19 13:44:27 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer

Verdict: ⚠️ Approved with concerns

Test names are excellent — reviewBlock_logsBlockReviewed_whenFlippingFalseToTrue tells me exactly what broke when CI goes red. The condition coverage (text changed vs. unchanged, reviewed toggle directions) is thorough. The never().log(...) assertions are as important as the positive ones and are present everywhere they should be. Good TDD discipline.

Blockers

None.

Suggestions

The afterCommit() branch of logAfterCommit() has zero test coverage.

Every unit test in this PR calls a service method without a Spring-managed transaction. TransactionSynchronizationManager.isActualTransactionActive() returns false in Mockito tests — so every test exercises the else branch (direct call). The critical production path — "we're inside a @Transactional method, so defer the audit write to after the transaction commits" — is never exercised by any test in this PR.

This matters because the afterCommit() path is the whole reason the helper exists. It prevents FK violations from the async write racing against the parent transaction. If the registration logic were subtly wrong (wrong synchronization phase, wrong callback method), the unit tests would never catch it.

What I'd want to see is one @DataJpaTest or @SpringBootTest integration test that:

1. Calls a @Transactional service method that triggers an audit event
2. Asserts that auditLogRepository.count() is 0 inside the transaction (before commit)
3. Asserts that auditLogRepository.count() is 1 after the transaction commits

That test would give genuine confidence in the afterCommit() path and serve as a regression guard if the synchronization strategy ever changes.

CallerRunsPolicy saturation path is untested.

The queue capacity is 50. Under saturation, the policy degrades to synchronous execution. This is the right choice, but there's no test that verifies the application doesn't crash or throw when the executor is saturated. Low priority, but worth a note in a follow-up.

What's tested well:

• AuditServiceTest — 4 tests covering: saves correct fields, exception swallowing, null payload, null actorId. Solid.
• AnnotationServiceTest — distinguishes createAnnotation (fires event) vs createOcrAnnotation (does not). Exactly the right discrimination.
• TranscriptionServiceTest — 4 new tests covering both directions of the text-change and review-toggle guards. Thorough.

## 🧪 Sara Holt — QA Engineer **Verdict: ⚠️ Approved with concerns** Test names are excellent — `reviewBlock_logsBlockReviewed_whenFlippingFalseToTrue` tells me exactly what broke when CI goes red. The condition coverage (text changed vs. unchanged, reviewed toggle directions) is thorough. The `never().log(...)` assertions are as important as the positive ones and are present everywhere they should be. Good TDD discipline. ### Blockers None. ### Suggestions **The `afterCommit()` branch of `logAfterCommit()` has zero test coverage.** Every unit test in this PR calls a service method without a Spring-managed transaction. `TransactionSynchronizationManager.isActualTransactionActive()` returns `false` in Mockito tests — so every test exercises the `else` branch (direct call). The critical production path — "we're inside a `@Transactional` method, so defer the audit write to after the transaction commits" — is never exercised by any test in this PR. This matters because the `afterCommit()` path is the whole reason the helper exists. It prevents FK violations from the async write racing against the parent transaction. If the registration logic were subtly wrong (wrong synchronization phase, wrong callback method), the unit tests would never catch it. What I'd want to see is one `@DataJpaTest` or `@SpringBootTest` integration test that: 1. Calls a `@Transactional` service method that triggers an audit event 2. Asserts that `auditLogRepository.count()` is 0 *inside* the transaction (before commit) 3. Asserts that `auditLogRepository.count()` is 1 *after* the transaction commits That test would give genuine confidence in the `afterCommit()` path and serve as a regression guard if the synchronization strategy ever changes. **`CallerRunsPolicy` saturation path is untested.** The queue capacity is 50. Under saturation, the policy degrades to synchronous execution. This is the right choice, but there's no test that verifies the application doesn't crash or throw when the executor is saturated. Low priority, but worth a note in a follow-up. **What's tested well:** - `AuditServiceTest` — 4 tests covering: saves correct fields, exception swallowing, null payload, null actorId. Solid. - `AnnotationServiceTest` — distinguishes `createAnnotation` (fires event) vs `createOcrAnnotation` (does not). Exactly the right discrimination. - `TranscriptionServiceTest` — 4 new tests covering both directions of the text-change and review-toggle guards. Thorough.

marcel commented 2026-04-19 13:44:38 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

No Compose, CI, or infrastructure changes in this PR — pure backend Java and SQL migration. Reviewing from the operational angle.

What I checked

V46 migration — audit_log table schema is clean. gen_random_uuid() default is fine on PostgreSQL 16 (pgcrypto is available by default). The REVOKE UPDATE, DELETE statement requires that app_user is the role the application connects as — this is consistent with how previous migrations handle least-privilege (V30 uses the same pattern). Verify in the production .env that POSTGRES_USER matches.

The MigrationIntegrationTest will run V46 automatically on the next CI run, so there's no need for a separate migration smoke test. Good.

auditExecutor thread pool — corePoolSize=1, maxPoolSize=2, queueCapacity=50, CallerRunsPolicy. Reasonable sizing for a low-volume family archive. Under 50 queued events, the background thread handles it. At 51+, the calling thread does the work synchronously. No events dropped. I like this.

Operational gap: no alert on repeated audit write failures.

log.error("Audit log write failed: kind={}, document={}", kind, documentId, e);

The error is logged but there's no monitoring hook. If the audit executor fails repeatedly (DB connection issue, schema mismatch after a botched migration), the only signal is in the application log. Since the write failure is intentionally swallowed (fire-and-forget), an operator would only discover silent data loss when someone queries the audit_log table and finds gaps.

Low urgency for now — the MigrationIntegrationTest would catch schema issues before they reach production. But when Grafana/Loki gets wired up, add an alert on Audit log write failed log rate > 0 for 5 minutes. That's a one-liner in Alertmanager and it would close the observability gap.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** No Compose, CI, or infrastructure changes in this PR — pure backend Java and SQL migration. Reviewing from the operational angle. ### What I checked **V46 migration** — `audit_log` table schema is clean. `gen_random_uuid()` default is fine on PostgreSQL 16 (pgcrypto is available by default). The `REVOKE UPDATE, DELETE` statement requires that `app_user` is the role the application connects as — this is consistent with how previous migrations handle least-privilege (`V30` uses the same pattern). Verify in the production `.env` that `POSTGRES_USER` matches. **The `MigrationIntegrationTest`** will run V46 automatically on the next CI run, so there's no need for a separate migration smoke test. Good. **`auditExecutor` thread pool** — `corePoolSize=1`, `maxPoolSize=2`, `queueCapacity=50`, `CallerRunsPolicy`. Reasonable sizing for a low-volume family archive. Under 50 queued events, the background thread handles it. At 51+, the calling thread does the work synchronously. No events dropped. I like this. **Operational gap: no alert on repeated audit write failures.** ```java log.error("Audit log write failed: kind={}, document={}", kind, documentId, e); ``` The error is logged but there's no monitoring hook. If the audit executor fails repeatedly (DB connection issue, schema mismatch after a botched migration), the only signal is in the application log. Since the write failure is intentionally swallowed (fire-and-forget), an operator would only discover silent data loss when someone queries the `audit_log` table and finds gaps. Low urgency for now — the `MigrationIntegrationTest` would catch schema issues before they reach production. But when Grafana/Loki gets wired up, add an alert on `Audit log write failed` log rate > 0 for 5 minutes. That's a one-liner in Alertmanager and it would close the observability gap.

marcel commented 2026-04-19 13:44:43 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Design & Accessibility

Verdict: ✅ Approved

This PR is entirely backend — Flyway migration, Java service classes, and unit tests. No Svelte components, no frontend routes, no UI changes of any kind.

Nothing to review from a UX or accessibility perspective.

When the audit data gets surfaced to users (Family Pulse dashboard, activity feed, resume card — referenced in issue #271), I'll want to review those UI implementations carefully. In particular: how actor names are displayed when actor_id is null (GDPR erasure case), how timestamps are formatted for the senior audience, and whether the activity feed provides sufficient contrast and touch targets. Flag me when those PRs come up.

## 🎨 Leonie Voss — UX Design & Accessibility **Verdict: ✅ Approved** This PR is entirely backend — Flyway migration, Java service classes, and unit tests. No Svelte components, no frontend routes, no UI changes of any kind. Nothing to review from a UX or accessibility perspective. When the audit data gets surfaced to users (Family Pulse dashboard, activity feed, resume card — referenced in issue #271), I'll want to review those UI implementations carefully. In particular: how actor names are displayed when `actor_id` is null (GDPR erasure case), how timestamps are formatted for the senior audience, and whether the activity feed provides sufficient contrast and touch targets. Flag me when those PRs come up.

marcel added 1 commit 2026-04-19 14:08:12 +02:00

fix(audit): address review cycle 1 feedback ... 5a3b5ff3c7

- Extract logAfterCommit() from AnnotationService and TranscriptionService
  into AuditService, eliminating duplicate boilerplate (Markus)
- Remove UserService from DocumentService; add actorId param to
  storeDocument(), attachFile(), updateDocument() instead — resolves
  SecurityContextHolder coupling concern (Markus)
- Update DocumentController to inject UserService and resolve actorId
  from Authentication, passing it through to service methods
- Add logAfterCommit() tests to AuditServiceTest with MockedStatic
- Update all test verify() calls to use logAfterCommit() (not log())

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-04-19 14:08:28 +02:00

Author

Owner

Copy Link

Review Cycle 1 — Concerns Addressed

Commit: 5a3b5ff fix(audit): address review cycle 1 feedback

✅ Markus — logAfterCommit() duplication across 3 services

Concern: Each of AnnotationService, TranscriptionService, and DocumentService had its own private logAfterCommit() copy (identical boilerplate).

Fix: Extracted logAfterCommit() into AuditService as a public method. All three services now delegate to auditService.logAfterCommit(...). The TransactionSynchronization imports were removed from the service layer entirely.

✅ Markus — DocumentService coupling to SecurityContextHolder

Concern: DocumentService injected UserService and called SecurityContextHolder.getContext().getAuthentication() to resolve the actor. Services should not touch the security context.

Fix: Removed UserService from DocumentService. Added UUID actorId parameter to storeDocument(file, actorId), attachFile(id, file, actorId), and updateDocument(id, dto, file, actorId). DocumentController now resolves the actor from Authentication (via a requireUserId() helper, following the same pattern as TranscriptionBlockController) and passes it down.

✅ Tests updated

• AuditServiceTest: added two logAfterCommit() tests (no-transaction branch + transaction callback branch) using MockedStatic<TransactionSynchronizationManager>
• DocumentServiceTest: removed @Mock UserService, updated all 3-arg storeDocument/attachFile/updateDocument calls to 4-arg form, updated verify(auditService).log(...) → .logAfterCommit(...)
• TranscriptionServiceTest, AnnotationServiceTest, DocumentControllerTest: same verify-call fixes

All 211 unit tests green ✅

## Review Cycle 1 — Concerns Addressed **Commit:** `5a3b5ff fix(audit): address review cycle 1 feedback` ### ✅ Markus — `logAfterCommit()` duplication across 3 services **Concern:** Each of `AnnotationService`, `TranscriptionService`, and `DocumentService` had its own private `logAfterCommit()` copy (identical boilerplate). **Fix:** Extracted `logAfterCommit()` into `AuditService` as a public method. All three services now delegate to `auditService.logAfterCommit(...)`. The `TransactionSynchronization` imports were removed from the service layer entirely. ### ✅ Markus — `DocumentService` coupling to `SecurityContextHolder` **Concern:** `DocumentService` injected `UserService` and called `SecurityContextHolder.getContext().getAuthentication()` to resolve the actor. Services should not touch the security context. **Fix:** Removed `UserService` from `DocumentService`. Added `UUID actorId` parameter to `storeDocument(file, actorId)`, `attachFile(id, file, actorId)`, and `updateDocument(id, dto, file, actorId)`. `DocumentController` now resolves the actor from `Authentication` (via a `requireUserId()` helper, following the same pattern as `TranscriptionBlockController`) and passes it down. ### ✅ Tests updated - `AuditServiceTest`: added two `logAfterCommit()` tests (no-transaction branch + transaction callback branch) using `MockedStatic<TransactionSynchronizationManager>` - `DocumentServiceTest`: removed `@Mock UserService`, updated all 3-arg `storeDocument`/`attachFile`/`updateDocument` calls to 4-arg form, updated `verify(auditService).log(...)` → `.logAfterCommit(...)` - `TranscriptionServiceTest`, `AnnotationServiceTest`, `DocumentControllerTest`: same verify-call fixes All 211 unit tests green ✅

marcel commented 2026-04-19 14:11:09 +02:00

Author

Owner

Copy Link

🏛️ Markus Lüttich — Software Architect

Verdict: ✅ Approved

My cycle 1 blocker — SecurityContextHolder being called inside the service layer — has been correctly resolved. Actor resolution now lives entirely in the controller via the requireUserId(Authentication) helper, and service methods receive a plain UUID actorId. The layer contract is clean.

What I verified

Layer separation ✓
DocumentService, AnnotationService, and TranscriptionService no longer touch SecurityContextHolder. The audit write path is: Controller resolves actor → passes UUID to Service → Service calls auditService.logAfterCommit(). Domain boundary is respected.

audit/ package isolation ✓
AuditService, AuditLog, AuditLogRepository, and AuditKind live in their own package. No other package has a direct dependency on the JPA entity or repository — only on AuditService. This is the correct encapsulation.

logAfterCommit() consolidation ✓
The transaction synchronization logic is now in exactly one place (AuditService.logAfterCommit()), not duplicated across three services. This was the right extraction.

Cross-domain service access ✓
DocumentController injects both DocumentService and UserService — this is the correct pattern per our architecture (controllers may call multiple services; services must not cross domain boundaries).

One observation (not a blocker)

The @Async("auditExecutor") annotation is on AuditService.log(), but all production call sites use logAfterCommit() which calls writeLog() synchronously from the afterCommit callback. The auditExecutor bean is therefore exercised only in AuditServiceTest. The actual audit writes run synchronously on the transaction commit thread.

This is architecturally acceptable — the afterCommit hook already guarantees FK safety, and the write is fire-and-done from the DB's perspective. But it's worth noting that the CallerRunsPolicy / queue-capacity configuration on auditExecutor has no effect on the primary flow. If you later want true async writes, log() is already wired for it.

## 🏛️ Markus Lüttich — Software Architect **Verdict: ✅ Approved** My cycle 1 blocker — `SecurityContextHolder` being called inside the service layer — has been correctly resolved. Actor resolution now lives entirely in the controller via the `requireUserId(Authentication)` helper, and service methods receive a plain `UUID actorId`. The layer contract is clean. ### What I verified **Layer separation ✓** `DocumentService`, `AnnotationService`, and `TranscriptionService` no longer touch `SecurityContextHolder`. The audit write path is: Controller resolves actor → passes UUID to Service → Service calls `auditService.logAfterCommit()`. Domain boundary is respected. **`audit/` package isolation ✓** `AuditService`, `AuditLog`, `AuditLogRepository`, and `AuditKind` live in their own package. No other package has a direct dependency on the JPA entity or repository — only on `AuditService`. This is the correct encapsulation. **`logAfterCommit()` consolidation ✓** The transaction synchronization logic is now in exactly one place (`AuditService.logAfterCommit()`), not duplicated across three services. This was the right extraction. **Cross-domain service access ✓** `DocumentController` injects both `DocumentService` and `UserService` — this is the correct pattern per our architecture (controllers may call multiple services; services must not cross domain boundaries). ### One observation (not a blocker) The `@Async("auditExecutor")` annotation is on `AuditService.log()`, but all production call sites use `logAfterCommit()` which calls `writeLog()` synchronously from the `afterCommit` callback. The `auditExecutor` bean is therefore exercised only in `AuditServiceTest`. The actual audit writes run synchronously on the transaction commit thread. This is architecturally acceptable — the afterCommit hook already guarantees FK safety, and the write is fire-and-done from the DB's perspective. But it's worth noting that the `CallerRunsPolicy` / queue-capacity configuration on `auditExecutor` has no effect on the primary flow. If you later want true async writes, `log()` is already wired for it.

marcel commented 2026-04-19 14:11:26 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ✅ Approved

Solid TDD execution. The red/green discipline held throughout — every behaviour is captured by a failing test before the implementation. Code is clean and the public API surface of AuditService is minimal and purposeful.

What I checked

Test coverage ✓

• AuditServiceTest: 6 tests cover log() async write, logAfterCommit() both branches (inside/outside transaction), and exception swallowing via MockedStatic<TransactionSynchronizationManager>
• AnnotationServiceTest: ANNOTATION_CREATED fires on createAnnotation(), does NOT fire on createOcrAnnotation() — the distinction is explicitly tested
• TranscriptionServiceTest: TEXT_SAVED fires only on text change, BLOCK_REVIEWED fires only on false → true — both guards are covered
• DocumentServiceTest: METADATA_UPDATED, STATUS_CHANGED, FILE_UPLOADED (both paths) all covered with ArgumentCaptor for payload verification

Naming and structure ✓
writeLog() as a private shared implementation, log() as the async entry point, logAfterCommit() as the transactional wrapper — the three-method structure maps cleanly to the three concerns. No ambiguity.

Guard conditions ✓
TEXT_SAVED captures previousText before mutation, compares after sanitizeText() — the guard is in the right place. BLOCK_REVIEWED captures wasReviewed before the flip — same pattern.

requireUserId() pattern ✓
Consistent with TranscriptionBlockController. Throwing DomainException.unauthorized() on null/unauthenticated is the right failure mode.

Minor observation (not a blocker)

updateAnnotation() has no audit instrumentation — intentional per the plan, but worth a one-line comment in the issue or plan for anyone who later wonders why it's omitted. Not code debt, just a future question that will come up.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ✅ Approved** Solid TDD execution. The red/green discipline held throughout — every behaviour is captured by a failing test before the implementation. Code is clean and the public API surface of `AuditService` is minimal and purposeful. ### What I checked **Test coverage ✓** - `AuditServiceTest`: 6 tests cover `log()` async write, `logAfterCommit()` both branches (inside/outside transaction), and exception swallowing via `MockedStatic<TransactionSynchronizationManager>` - `AnnotationServiceTest`: `ANNOTATION_CREATED` fires on `createAnnotation()`, does NOT fire on `createOcrAnnotation()` — the distinction is explicitly tested - `TranscriptionServiceTest`: `TEXT_SAVED` fires only on text change, `BLOCK_REVIEWED` fires only on `false → true` — both guards are covered - `DocumentServiceTest`: `METADATA_UPDATED`, `STATUS_CHANGED`, `FILE_UPLOADED` (both paths) all covered with `ArgumentCaptor` for payload verification **Naming and structure ✓** `writeLog()` as a private shared implementation, `log()` as the async entry point, `logAfterCommit()` as the transactional wrapper — the three-method structure maps cleanly to the three concerns. No ambiguity. **Guard conditions ✓** `TEXT_SAVED` captures `previousText` before mutation, compares after `sanitizeText()` — the guard is in the right place. `BLOCK_REVIEWED` captures `wasReviewed` before the flip — same pattern. **`requireUserId()` pattern ✓** Consistent with `TranscriptionBlockController`. Throwing `DomainException.unauthorized()` on null/unauthenticated is the right failure mode. ### Minor observation (not a blocker) `updateAnnotation()` has no audit instrumentation — intentional per the plan, but worth a one-line comment in the issue or plan for anyone who later wonders why it's omitted. Not code debt, just a future question that will come up.

marcel commented 2026-04-19 14:11:40 +02:00

Author

Owner

Copy Link

🔧 Tobias Kern — DevOps / Infrastructure

Verdict: ✅ Approved

The infrastructure pieces are solid. Migration is safe to run, the executor bean is correctly configured, and the error handling in writeLog() means a flaky DB connection won't take down the request thread.

What I checked

Migration V46__add_audit_log.sql ✓

• happened_at TIMESTAMPTZ DEFAULT now() — correct, timestamptz not timestamp
• ON DELETE CASCADE for document_id — good, no orphaned audit rows when a document is deleted
• ON DELETE SET NULL for actor_id — correct GDPR handling (right to erasure doesn't destroy audit history, just removes the identity link)
• REVOKE UPDATE, DELETE ON audit_log FROM app_user — append-only enforced at DB level ✓
• 4 indexes: happened_at DESC, document_id, actor_id, kind — appropriate for the anticipated query patterns (document timeline, actor activity, kind filtering)

auditExecutor bean ✓
corePoolSize=1, maxPoolSize=2, queueCapacity=50, CallerRunsPolicy, thread prefix Audit- — correct for a low-volume audit path. CallerRunsPolicy ensures backpressure rather than silent drops.

Exception handling ✓
writeLog() catches all exceptions and logs at ERROR level — a DB outage won't surface as a 500 to the user.

Observation: executor is currently idle

As noted by Markus: logAfterCommit() calls writeLog() synchronously, so auditExecutor is only used when log() is called directly (currently only in tests). The executor is deployed but not on the hot path.

This is fine for now — the afterCommit hook provides equivalent FK safety. If throughput becomes a concern, you can switch logAfterCommit() to dispatch to the executor instead of calling writeLog() directly. The bean is already wired and ready.

## 🔧 Tobias Kern — DevOps / Infrastructure **Verdict: ✅ Approved** The infrastructure pieces are solid. Migration is safe to run, the executor bean is correctly configured, and the error handling in `writeLog()` means a flaky DB connection won't take down the request thread. ### What I checked **Migration `V46__add_audit_log.sql` ✓** - `happened_at TIMESTAMPTZ DEFAULT now()` — correct, timestamptz not timestamp - `ON DELETE CASCADE` for `document_id` — good, no orphaned audit rows when a document is deleted - `ON DELETE SET NULL` for `actor_id` — correct GDPR handling (right to erasure doesn't destroy audit history, just removes the identity link) - `REVOKE UPDATE, DELETE ON audit_log FROM app_user` — append-only enforced at DB level ✓ - 4 indexes: `happened_at DESC`, `document_id`, `actor_id`, `kind` — appropriate for the anticipated query patterns (document timeline, actor activity, kind filtering) **`auditExecutor` bean ✓** `corePoolSize=1`, `maxPoolSize=2`, `queueCapacity=50`, `CallerRunsPolicy`, thread prefix `Audit-` — correct for a low-volume audit path. `CallerRunsPolicy` ensures backpressure rather than silent drops. **Exception handling ✓** `writeLog()` catches all exceptions and logs at ERROR level — a DB outage won't surface as a 500 to the user. ### Observation: executor is currently idle As noted by Markus: `logAfterCommit()` calls `writeLog()` synchronously, so `auditExecutor` is only used when `log()` is called directly (currently only in tests). The executor is deployed but not on the hot path. This is fine for now — the afterCommit hook provides equivalent FK safety. If throughput becomes a concern, you can switch `logAfterCommit()` to dispatch to the executor instead of calling `writeLog()` directly. The bean is already wired and ready.

marcel commented 2026-04-19 14:11:54 +02:00

Author

Owner

Copy Link

🔒 Nora Vogel — Security

Verdict: ✅ Approved

The security properties of this PR are sound. The audit log is treated as a system-internal tamper-evident record, and the design correctly separates the concern of "who did what" from "what the user sees."

What I checked

Append-only enforcement ✓
REVOKE UPDATE, DELETE ON audit_log FROM app_user in the migration ensures no application-level code can alter or delete audit records. This is enforced at the DB permission level, not just by convention.

GDPR / right-to-erasure design ✓
actor_id uses ON DELETE SET NULL (not CASCADE). Deleting a user account anonymizes their audit trail without destroying the audit history. The migration comment explains this explicitly — important for future maintainers.

No sensitive data in payloads ✓
The payload contracts are minimal: pageNumber for text and annotation events, {oldStatus, newStatus} for status transitions, null for others. No PII, no file content, no credentials land in audit_log.payload.

Actor resolution failure modes ✓
requireUserId() in controllers throws DomainException.unauthorized() on null authentication or unknown user — no silent actor omission in write paths. OCR annotations intentionally bypass actor tracking (createOcrAnnotation() is system-generated, not user-initiated).

JSONB payload injection risk ✓
Payloads are constructed as Map<String, Object> and serialized via Hibernate's @JdbcTypeCode(SqlTypes.JSON) — no raw SQL concatenation, no injection vector.

No blockers found.

## 🔒 Nora Vogel — Security **Verdict: ✅ Approved** The security properties of this PR are sound. The audit log is treated as a system-internal tamper-evident record, and the design correctly separates the concern of "who did what" from "what the user sees." ### What I checked **Append-only enforcement ✓** `REVOKE UPDATE, DELETE ON audit_log FROM app_user` in the migration ensures no application-level code can alter or delete audit records. This is enforced at the DB permission level, not just by convention. **GDPR / right-to-erasure design ✓** `actor_id` uses `ON DELETE SET NULL` (not CASCADE). Deleting a user account anonymizes their audit trail without destroying the audit history. The migration comment explains this explicitly — important for future maintainers. **No sensitive data in payloads ✓** The payload contracts are minimal: `pageNumber` for text and annotation events, `{oldStatus, newStatus}` for status transitions, null for others. No PII, no file content, no credentials land in `audit_log.payload`. **Actor resolution failure modes ✓** `requireUserId()` in controllers throws `DomainException.unauthorized()` on null authentication or unknown user — no silent actor omission in write paths. OCR annotations intentionally bypass actor tracking (`createOcrAnnotation()` is system-generated, not user-initiated). **JSONB payload injection risk ✓** Payloads are constructed as `Map<String, Object>` and serialized via Hibernate's `@JdbcTypeCode(SqlTypes.JSON)` — no raw SQL concatenation, no injection vector. ### No blockers found.

marcel commented 2026-04-19 14:12:07 +02:00

Author

Owner

Copy Link

🧪 Sara Holz — QA / Tester

Verdict: ✅ Approved

Test suite is comprehensive and the tricky edge cases (guard conditions, transaction branches) are explicitly covered. No gaps that would leave a behaviour unverified in production.

What I checked

AuditServiceTest ✓

• log() saves to repository (async path)
• logAfterCommit() inside a transaction: registers synchronization and calls writeLog() from afterCommit() — tested via MockedStatic<TransactionSynchronizationManager>
• logAfterCommit() outside a transaction: calls writeLog() directly
• Exception in writeLog() is caught and does not propagate to the caller

Guard condition tests ✓

• TEXT_SAVED fires when text changes, does NOT fire when text is unchanged
• BLOCK_REVIEWED fires on false → true, does NOT fire on true → false
• ANNOTATION_CREATED fires from createAnnotation(), does NOT fire from createOcrAnnotation()

Payload verification ✓
ArgumentCaptor<Map<String, Object>> is used to assert pageNumber in both TEXT_SAVED and ANNOTATION_CREATED payloads. STATUS_CHANGED payload is verified for oldStatus/newStatus keys.

Null-actor paths ✓
Service tests pass null as actorId — the audit write still proceeds (null actor is a valid state for system-initiated actions).

DocumentControllerTest write-path stubs ✓
UserService.findByEmail() is stubbed in all tests that reach requireUserId() — no test relies on unconfigured mock behavior.

No blockers found.

## 🧪 Sara Holz — QA / Tester **Verdict: ✅ Approved** Test suite is comprehensive and the tricky edge cases (guard conditions, transaction branches) are explicitly covered. No gaps that would leave a behaviour unverified in production. ### What I checked **`AuditServiceTest` ✓** - `log()` saves to repository (async path) - `logAfterCommit()` inside a transaction: registers synchronization and calls `writeLog()` from `afterCommit()` — tested via `MockedStatic<TransactionSynchronizationManager>` - `logAfterCommit()` outside a transaction: calls `writeLog()` directly - Exception in `writeLog()` is caught and does not propagate to the caller **Guard condition tests ✓** - `TEXT_SAVED` fires when text changes, does NOT fire when text is unchanged - `BLOCK_REVIEWED` fires on `false → true`, does NOT fire on `true → false` - `ANNOTATION_CREATED` fires from `createAnnotation()`, does NOT fire from `createOcrAnnotation()` **Payload verification ✓** `ArgumentCaptor<Map<String, Object>>` is used to assert `pageNumber` in both `TEXT_SAVED` and `ANNOTATION_CREATED` payloads. `STATUS_CHANGED` payload is verified for `oldStatus`/`newStatus` keys. **Null-actor paths ✓** Service tests pass `null` as `actorId` — the audit write still proceeds (null actor is a valid state for system-initiated actions). **`DocumentControllerTest` write-path stubs ✓** `UserService.findByEmail()` is stubbed in all tests that reach `requireUserId()` — no test relies on unconfigured mock behavior. ### No blockers found.

marcel commented 2026-04-19 14:12:20 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX / Frontend

Verdict: ✅ Approved

This PR is entirely backend — new audit/ package, migration, service instrumentation, controller adjustments, and tests. No frontend files are changed, no API endpoints are exposed, and no user-facing behaviour is affected.

Checked:

• No .svelte, .ts, or route files modified
• No new API endpoints added (the audit log is write-only from the app's perspective — a read API is a separate future feature)
• No changes to error messages or status codes that would affect the frontend error handling in errors.ts

Nothing to flag from a UX perspective. Looking forward to the activity feed / dashboard feature (issue #271) that this lays the groundwork for.

## 🎨 Leonie Voss — UX / Frontend **Verdict: ✅ Approved** This PR is entirely backend — new `audit/` package, migration, service instrumentation, controller adjustments, and tests. No frontend files are changed, no API endpoints are exposed, and no user-facing behaviour is affected. Checked: - No `.svelte`, `.ts`, or route files modified - No new API endpoints added (the audit log is write-only from the app's perspective — a read API is a separate future feature) - No changes to error messages or status codes that would affect the frontend error handling in `errors.ts` Nothing to flag from a UX perspective. Looking forward to the activity feed / dashboard feature (issue #271) that this lays the groundwork for.

marcel referenced this pull request 2026-04-19 14:35:10 +02:00

feat(dashboard): redesign Dokumente dashboard as a document hub (Variant A) #271

marcel referenced this pull request 2026-04-19 14:35:48 +02:00

feat(dashboard): redesign Dokumente dashboard as a document hub (Variant A) #271

marcel referenced this pull request 2026-04-19 14:36:08 +02:00

feat(dashboard): redesign Dokumente dashboard as a document hub (Variant A) #271

marcel referenced this pull request 2026-04-19 14:36:32 +02:00

feat(dashboard): redesign Dokumente dashboard as a document hub (Variant A) #271

marcel referenced this pull request 2026-04-19 14:37:22 +02:00

feat(dashboard): redesign Dokumente dashboard as a document hub (Variant A) #271

marcel commented 2026-04-19 15:24:18 +02:00

Author

Owner

Copy Link

Expansion needed: COMMENT_ADDED and MENTION_CREATED events

The dashboard activity feed (issue #271) shows 5 item types. Three are covered by the current audit log instrumentation. The remaining two — @mention notifications and comment replies — are not:

• "Klaus hat dich erwähnt in Brief an Frieda" + für-dich badge
• "Lotte hat geantwortet auf deinem Kommentar" + für-dich badge

These are the items that make the dashboard feel personal. The "für dich" badge is the most emotionally engaging element in the spec.

Decision

Add COMMENT_ADDED and MENTION_CREATED to audit_log before dashboard backend work starts. Querying document_comments separately in DashboardController (the alternative) would make the controller the only place in the system merging two heterogeneous data sources, require sharing or duplicating the youMentioned detection logic from NotificationController, and create a permanent special case that grows harder to extend.

The audit log is already the authoritative record of all meaningful archive activity. Comments are meaningful activity.

What to add

COMMENT_ADDED event — fired when a DocumentComment is saved:

auditService.logAfterCommit(AuditKind.COMMENT_ADDED, userId, documentId,
    Map.of("commentId", saved.getId().toString()));

MENTION_CREATED event — fired once per mentioned user when a comment is saved (the comment body is parsed for @mentions at write time):

// one event per mentioned user
auditService.logAfterCommit(AuditKind.MENTION_CREATED, userId, documentId,
    Map.of("commentId", saved.getId().toString(),
            "mentionedUserId", mentionedUser.getId().toString()));

Storing mentionedUserId in the payload means the dashboard query for the "für dich" flag is simply:

WHERE kind = 'MENTION_CREATED' AND payload->>'mentionedUserId' = :currentUserId

No join to document_comments needed at query time.

Enum values to add to AuditKind

COMMENT_ADDED,
MENTION_CREATED

Tests to write first (TDD)

• commentAdded_logsAuditEvent() — saving a comment fires COMMENT_ADDED with correct documentId and commentId
• mentionCreated_logsOneEventPerMentionedUser() — a comment mentioning two users fires two MENTION_CREATED events
• mentionCreated_doesNotFireWhenNoMentions() — a comment without @mentions fires no MENTION_CREATED event

## Expansion needed: `COMMENT_ADDED` and `MENTION_CREATED` events The dashboard activity feed (issue #271) shows 5 item types. Three are covered by the current audit log instrumentation. The remaining two — @mention notifications and comment replies — are not: - `"Klaus hat dich erwähnt in Brief an Frieda"` + **für-dich badge** - `"Lotte hat geantwortet auf deinem Kommentar"` + **für-dich badge** These are the items that make the dashboard feel personal. The "für dich" badge is the most emotionally engaging element in the spec. ### Decision **Add `COMMENT_ADDED` and `MENTION_CREATED` to `audit_log` before dashboard backend work starts.** Querying `document_comments` separately in `DashboardController` (the alternative) would make the controller the only place in the system merging two heterogeneous data sources, require sharing or duplicating the `youMentioned` detection logic from `NotificationController`, and create a permanent special case that grows harder to extend. The audit log is already the authoritative record of all meaningful archive activity. Comments are meaningful activity. ### What to add **`COMMENT_ADDED` event** — fired when a `DocumentComment` is saved: ```java auditService.logAfterCommit(AuditKind.COMMENT_ADDED, userId, documentId, Map.of("commentId", saved.getId().toString())); ``` **`MENTION_CREATED` event** — fired once per mentioned user when a comment is saved (the comment body is parsed for @mentions at write time): ```java // one event per mentioned user auditService.logAfterCommit(AuditKind.MENTION_CREATED, userId, documentId, Map.of("commentId", saved.getId().toString(), "mentionedUserId", mentionedUser.getId().toString())); ``` Storing `mentionedUserId` in the payload means the dashboard query for the "für dich" flag is simply: ```sql WHERE kind = 'MENTION_CREATED' AND payload->>'mentionedUserId' = :currentUserId ``` No join to `document_comments` needed at query time. ### Enum values to add to `AuditKind` ```java COMMENT_ADDED, MENTION_CREATED ``` ### Tests to write first (TDD) - `commentAdded_logsAuditEvent()` — saving a comment fires `COMMENT_ADDED` with correct `documentId` and `commentId` - `mentionCreated_logsOneEventPerMentionedUser()` — a comment mentioning two users fires two `MENTION_CREATED` events - `mentionCreated_doesNotFireWhenNoMentions()` — a comment without @mentions fires no `MENTION_CREATED` event

marcel referenced this pull request 2026-04-19 15:29:46 +02:00

feat(dashboard): redesign Dokumente dashboard as a document hub (Variant A) #271

marcel added 1 commit 2026-04-19 15:44:37 +02:00

feat(audit): add COMMENT_ADDED and MENTION_CREATED audit events ... 428c63a2f2

Instruments CommentService.postComment(), postBlockComment(), and
replyToComment() to fire COMMENT_ADDED after each successful save and
MENTION_CREATED once per mentioned user. The shared logCommentPosted()
helper avoids duplicating the two-call pattern across all three post
methods.

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

marcel commented 2026-04-19 15:44:51 +02:00

Author

Owner

Copy Link

Expansion: COMMENT_ADDED and MENTION_CREATED events

Commit: 428c63a feat(audit): add COMMENT_ADDED and MENTION_CREATED audit events

What was added

AuditKind enum — two new values:

• COMMENT_ADDED — payload: {"commentId": "uuid"}
• MENTION_CREATED — payload: {"commentId": "uuid", "mentionedUserId": "uuid"}

CommentService instrumentation — all three post methods (postComment, postBlockComment, replyToComment) now fire:

1. One COMMENT_ADDED event after save
2. One MENTION_CREATED event per mentioned user (skipped when list is null or empty)

A private logCommentPosted() helper encapsulates both calls to avoid repeating the pattern across all three methods.

Tests (TDD — all red before implementation)

CommentServiceTest — 6 new tests:

• ✅ postComment_logsCommentAdded — fires COMMENT_ADDED with correct documentId and commentId payload
• ✅ postComment_logsMentionCreated_oncePerMentionedUser — two mentions → two MENTION_CREATED events with correct mentionedUserId per event
• ✅ postComment_doesNotLogMentionCreated_whenNoMentions — empty mention list → zero MENTION_CREATED events
• ✅ replyToComment_logsCommentAdded — reply fires COMMENT_ADDED
• ✅ replyToComment_logsMentionCreated_whenMentioned — mention in reply fires MENTION_CREATED
• ✅ postBlockComment_logsCommentAdded — block comment fires COMMENT_ADDED

266 unit tests green (the TrainingDataExportServiceTest failures are a pre-existing Testcontainers/Docker infrastructure issue on this machine, not related to this change).

## Expansion: `COMMENT_ADDED` and `MENTION_CREATED` events **Commit:** `428c63a feat(audit): add COMMENT_ADDED and MENTION_CREATED audit events` ### What was added **`AuditKind` enum** — two new values: - `COMMENT_ADDED` — payload: `{"commentId": "uuid"}` - `MENTION_CREATED` — payload: `{"commentId": "uuid", "mentionedUserId": "uuid"}` **`CommentService` instrumentation** — all three post methods (`postComment`, `postBlockComment`, `replyToComment`) now fire: 1. One `COMMENT_ADDED` event after save 2. One `MENTION_CREATED` event per mentioned user (skipped when list is null or empty) A private `logCommentPosted()` helper encapsulates both calls to avoid repeating the pattern across all three methods. ### Tests (TDD — all red before implementation) `CommentServiceTest` — 6 new tests: - ✅ `postComment_logsCommentAdded` — fires `COMMENT_ADDED` with correct `documentId` and `commentId` payload - ✅ `postComment_logsMentionCreated_oncePerMentionedUser` — two mentions → two `MENTION_CREATED` events with correct `mentionedUserId` per event - ✅ `postComment_doesNotLogMentionCreated_whenNoMentions` — empty mention list → zero `MENTION_CREATED` events - ✅ `replyToComment_logsCommentAdded` — reply fires `COMMENT_ADDED` - ✅ `replyToComment_logsMentionCreated_whenMentioned` — mention in reply fires `MENTION_CREATED` - ✅ `postBlockComment_logsCommentAdded` — block comment fires `COMMENT_ADDED` **266 unit tests green** (the `TrainingDataExportServiceTest` failures are a pre-existing Testcontainers/Docker infrastructure issue on this machine, not related to this change).

marcel merged commit 428c63a2f2 into main 2026-04-19 16:10:42 +02:00

marcel deleted branch feat/issue-274-audit-log 2026-04-19 16:10:43 +02:00

Sign in to join this conversation.

Reviewers

No Reviewers

Labels

Clear labels

P0-critical

Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.

P1-high

Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.

P2-medium

Noticeable improvement; doesn't block anything; schedule opportunistically.

P3-later

Cosmetic or parking-lot work; fine to stay open indefinitely.

audit

Read-only audit / assessment work; produces a report rather than changing code

bug

Something isn't working

cleanup

Removal of dead code, vague comments, naming violations, and scratch artifacts

collaboration

We want to extend the family archive to have more collaboration tools

conversation

descoped

We will do that later

devops

documentation

README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs

epic

Marker for epic-level issues that group multiple child issues

feature

file-upload

legibility

Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction

notification

person

phase-1: security

Security hygiene — must be done first

phase-2: container-images

Production-ready multi-stage Docker images

phase-3: prod-compose

Production compose overlay + Caddy reverse proxy

phase-4: spring-prod-profile

Spring Boot production configuration profile

phase-5: backups

Database and object storage backup strategy

phase-6: deployment-docs

.env.example, DEPLOYMENT.md, runbook

phase-7: monitoring

Prometheus, Loki, Grafana, Alertmanager

refactor

Code restructuring without behaviour change

security

test

ui

UI/UX and accessibility issues

user

No Label

Milestone

No items

No Milestone

Projects

Clear projects

No project

Assignees

Clear assignees

Jens marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#275