fix(geschichte): delete note-less journey items before document delete (#805) #806

Merged

marcel merged 8 commits from feat/issue-805-journey-note-less-cascade into feat/issue-795-story-documents 2026-06-11 19:36:06 +02:00

Conversation 8 Commits 8 Files Changed 12 +466 -9

marcel commented 2026-06-11 17:56:44 +02:00

Owner

Copy Link

Summary

• Fixes the P1 constraint 500: DocumentService.deleteDocument now publishes a DocumentDeletingEvent before deleteById; JourneyItemDocumentDeleteListener handles it synchronously and deletes all note-less items (note IS NULL OR note = '') for that document, so ON DELETE SET NULL only fires on note-carrying rows that satisfy chk_journey_item_not_empty
• Plain @EventListener is load-bearing (not AFTER_COMMIT, not @Async) — see ADR-038 for rationale
• No schema change, no migration
• Adds DOCUMENT_DELETED to AuditKind; documents this as the first custom domain event in the codebase

⚠️ Merge order

This PR must merge before #795 (PR #804). Merging #795 first means the first document delete after deploy hits the 500.

Test plan

• JourneyItemDocumentDeleteTest — 8 new integration tests (real Postgres/Testcontainers) covering AC-1 through AC-7 + idempotency; all pass
• JourneyItemIntegrationTest — 15 existing tests still pass; 2 updated to route deletes through DocumentService
• JourneyItemServiceTest + JourneyItemConstraintsTest — 52 tests pass
• ./mvnw clean package -DskipTests — build clean
• Post-deploy smoke action: actively trigger a document delete that would have 500'd pre-fix; confirm no chk_journey_item_not_empty errors in GlitchTip / Loki

🤖 Generated with Claude Code

## Summary - Fixes the P1 constraint 500: `DocumentService.deleteDocument` now publishes a `DocumentDeletingEvent` before `deleteById`; `JourneyItemDocumentDeleteListener` handles it synchronously and deletes all note-less items (`note IS NULL OR note = ''`) for that document, so `ON DELETE SET NULL` only fires on note-carrying rows that satisfy `chk_journey_item_not_empty` - Plain `@EventListener` is load-bearing (not `AFTER_COMMIT`, not `@Async`) — see ADR-038 for rationale - No schema change, no migration - Adds `DOCUMENT_DELETED` to `AuditKind`; documents this as the first custom domain event in the codebase ## ⚠️ Merge order **This PR must merge before #795 (PR #804).** Merging #795 first means the first document delete after deploy hits the 500. ## Test plan - [x] `JourneyItemDocumentDeleteTest` — 8 new integration tests (real Postgres/Testcontainers) covering AC-1 through AC-7 + idempotency; all pass - [x] `JourneyItemIntegrationTest` — 15 existing tests still pass; 2 updated to route deletes through `DocumentService` - [x] `JourneyItemServiceTest` + `JourneyItemConstraintsTest` — 52 tests pass - [x] `./mvnw clean package -DskipTests` — build clean - [ ] **Post-deploy smoke action**: actively trigger a document delete that would have 500'd pre-fix; confirm no `chk_journey_item_not_empty` errors in GlitchTip / Loki 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

marcel added 2 commits 2026-06-11 17:56:46 +02:00

fix(geschichte): delete note-less journey items before document delete to prevent chk constraint 500 (#805) ... 44869d64f7

Publishes DocumentDeletingEvent from DocumentService.deleteDocument before
deleteById; JourneyItemDocumentDeleteListener handles it synchronously so
note-less items are gone before ON DELETE SET NULL fires on note-carrying rows.
Plain @EventListener chosen over AFTER_COMMIT (fires too late) and @Async
(breaks rollback atomicity) — see ADR-038. Adds DOCUMENT_DELETED to AuditKind.

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

docs(adr): ADR-038 — domain event drives note-less journey-item cleanup on document delete (#805) ... eefc67bd81

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

marcel referenced this pull request 2026-06-11 17:57:08 +02:00

bug: deleting a document linked via a note-less journey_item 500s at DB constraint #805

marcel commented 2026-06-11 18:13:00 +02:00

Author

Owner

Copy Link

🏛️ Markus Keller — Application Architect

Verdict: ⚠️ Approved with concerns

This is how a cross-domain fix should be done. The event direction (document → journey, publisher knows nothing about consumers) is exactly right, and ADR-038 is one of the better ADRs in docs/adr/ — it documents the load-bearing listener-phase choice, names the rejected alternatives with concrete reasons, and explicitly states the boundary rule ("documents must not depend on journey"). The decision to bypass JourneyItemService and call the repository directly is justified here: the listener lives inside geschichte.journeyitem, so it accesses its own domain's repository — no boundary is crossed. The audit-suppression rationale (N journeys would produce N audit rows for one user action) is sound.

Pushing the cleanup into the application layer instead of a DB trigger is the right call for this team — trigger logic invisible to code review is a maintenance trap, and the ADR says so.

Blockers

1. Doc currency: docs/architecture/c4/l3-backend-3g-supporting.puml does not reflect the new component. The diagram currently shows only GeschichteController/GeschichteService — the journeyitem sub-domain and now JourneyItemDocumentDeleteListener + the DocumentDeletingEvent flow are absent. Per our review table, a new component in an existing backend domain requires the matching l3 diagram update. Since this PR targets the #795 feature branch, I accept resolving this on the feature branch before PR #804 merges to main — but it must not reach main undrawn. This is the first event-driven edge in the architecture; it is precisely the kind of invisible coupling the diagrams exist to make visible. A dashed Rel(documentSvc, journeyListener, "DocumentDeletingEvent", "in-process event") edge is enough.

Suggestions

2. Consider @Modifying(clearAutomatically = true, flushAutomatically = true) on deleteNoteLessByDocumentId (JourneyItemRepository.java:56). Hibernate's AUTO flush mode will flush pending changes whose query space overlaps journey_items before the bulk delete, so this is not a bug today — but flushAutomatically = true makes the contract explicit instead of relying on flush-mode behavior, and it costs nothing. You already did the equivalent favor for readers with clearAutomatically.

3. ADR-038's "first custom domain event" consequence is worth a sentence in CLAUDE.md eventually — the next developer who hits a Spring 7 constructor cycle should find the pattern without archaeology. Not required for this PR; the ADR is discoverable.

The merge-order constraint (this PR before #804) is structurally enforced by targeting feat/issue-795-story-documents — good. No schema change, no migration, no new infrastructure. Fix the diagram and this is clean.

## 🏛️ Markus Keller — Application Architect **Verdict: ⚠️ Approved with concerns** This is how a cross-domain fix should be done. The event direction (`document → journey`, publisher knows nothing about consumers) is exactly right, and ADR-038 is one of the better ADRs in `docs/adr/` — it documents the load-bearing listener-phase choice, names the rejected alternatives with concrete reasons, and explicitly states the boundary rule ("documents must not depend on journey"). The decision to bypass `JourneyItemService` and call the repository directly is justified here: the listener lives **inside** `geschichte.journeyitem`, so it accesses its own domain's repository — no boundary is crossed. The audit-suppression rationale (N journeys would produce N audit rows for one user action) is sound. Pushing the cleanup into the application layer instead of a DB trigger is the right call for this team — trigger logic invisible to code review is a maintenance trap, and the ADR says so. ### Blockers 1. **Doc currency: `docs/architecture/c4/l3-backend-3g-supporting.puml` does not reflect the new component.** The diagram currently shows only `GeschichteController`/`GeschichteService` — the `journeyitem` sub-domain and now `JourneyItemDocumentDeleteListener` + the `DocumentDeletingEvent` flow are absent. Per our review table, a new component in an existing backend domain requires the matching l3 diagram update. Since this PR targets the #795 feature branch, I accept resolving this **on the feature branch before PR #804 merges to main** — but it must not reach main undrawn. This is the first event-driven edge in the architecture; it is precisely the kind of invisible coupling the diagrams exist to make visible. A dashed `Rel(documentSvc, journeyListener, "DocumentDeletingEvent", "in-process event")` edge is enough. ### Suggestions 2. **Consider `@Modifying(clearAutomatically = true, flushAutomatically = true)`** on `deleteNoteLessByDocumentId` (`JourneyItemRepository.java:56`). Hibernate's AUTO flush mode will flush pending changes whose query space overlaps `journey_items` before the bulk delete, so this is not a bug today — but `flushAutomatically = true` makes the contract explicit instead of relying on flush-mode behavior, and it costs nothing. You already did the equivalent favor for readers with `clearAutomatically`. 3. **ADR-038's "first custom domain event" consequence is worth a sentence in `CLAUDE.md` eventually** — the next developer who hits a Spring 7 constructor cycle should find the pattern without archaeology. Not required for this PR; the ADR is discoverable. The merge-order constraint (this PR before #804) is structurally enforced by targeting `feat/issue-795-story-documents` — good. No schema change, no migration, no new infrastructure. Fix the diagram and this is clean.

marcel commented 2026-06-11 18:13:12 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Verdict: ⚠️ Approved with concerns

TDD evidence is strong: eight AC-mapped integration tests with names that read as sentences (deleting_document_preserves_note_carrying_item_as_placeholder), the rollback guard via @MockitoSpyBean + doThrow is a clean way to prove AC-5, and using raw JdbcTemplate inserts for the note = '' state the service layer can never produce (AC-6) is exactly the right technique. The repository javadoc explaining why explicit JPQL is needed (the transient getDocumentId() derived-query trap) is a "why" comment done right.

Blockers

1. Fully-qualified type as a field declaration — DocumentService.java:83:

private final org.springframework.context.ApplicationEventPublisher eventPublisher;

No other field in this class is declared this way. Add the import:

import org.springframework.context.ApplicationEventPublisher;
...
private final ApplicationEventPublisher eventPublisher;

2. AuditKind javadoc contradicts the call site — AuditKind.java:57 documents DOCUMENT_DELETED as Payload: {"documentId": "uuid"}, but DocumentService.deleteDocument calls logAfterCommit(AuditKind.DOCUMENT_DELETED, null, id, null) — the id goes into the documentId column and the payload is null. Those javadocs are the contract consumers of the audit log read. Either fix the javadoc (Payload: none — documentId column carries the id) or pass the documented payload. The javadoc convention elsewhere in the enum is accurate (e.g. LOGIN_RATE_LIMITED); this entry breaks it.

Concerns

3. deleteDocument(UUID id) is the only audited write path without an actorId parameter. storeDocument, updateDocument, applyBulkEditToDocument, attachFile all take UUID actorId and pass it to the audit call; the new DOCUMENT_DELETED row gets actorId = null. Nora will speak to the severity — from the code-consistency side alone, threading actorId from the controller matches the established pattern and is a small change.

Suggestions

4. log.warn in the listener is defensible (data is being removed as a side effect), and including the count + document id gives the log line everything an operator needs. No change requested — just noting I checked it deliberately.

Fix the import and the javadoc and I'm happy.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer **Verdict: ⚠️ Approved with concerns** TDD evidence is strong: eight AC-mapped integration tests with names that read as sentences (`deleting_document_preserves_note_carrying_item_as_placeholder`), the rollback guard via `@MockitoSpyBean` + `doThrow` is a clean way to prove AC-5, and using raw `JdbcTemplate` inserts for the `note = ''` state the service layer can never produce (AC-6) is exactly the right technique. The repository javadoc explaining *why* explicit JPQL is needed (the transient `getDocumentId()` derived-query trap) is a "why" comment done right. ### Blockers 1. **Fully-qualified type as a field declaration** — `DocumentService.java:83`: ```java private final org.springframework.context.ApplicationEventPublisher eventPublisher; ``` No other field in this class is declared this way. Add the import: ```java import org.springframework.context.ApplicationEventPublisher; ... private final ApplicationEventPublisher eventPublisher; ``` 2. **`AuditKind` javadoc contradicts the call site** — `AuditKind.java:57` documents `DOCUMENT_DELETED` as `Payload: {"documentId": "uuid"}`, but `DocumentService.deleteDocument` calls `logAfterCommit(AuditKind.DOCUMENT_DELETED, null, id, null)` — the id goes into the `documentId` **column** and the payload is `null`. Those javadocs are the contract consumers of the audit log read. Either fix the javadoc (`Payload: none — documentId column carries the id`) or pass the documented payload. The javadoc convention elsewhere in the enum is accurate (e.g. `LOGIN_RATE_LIMITED`); this entry breaks it. ### Concerns 3. **`deleteDocument(UUID id)` is the only audited write path without an `actorId` parameter.** `storeDocument`, `updateDocument`, `applyBulkEditToDocument`, `attachFile` all take `UUID actorId` and pass it to the audit call; the new `DOCUMENT_DELETED` row gets `actorId = null`. Nora will speak to the severity — from the code-consistency side alone, threading `actorId` from the controller matches the established pattern and is a small change. ### Suggestions 4. `log.warn` in the listener is defensible (data is being removed as a side effect), and including the count + document id gives the log line everything an operator needs. No change requested — just noting I checked it deliberately. Fix the import and the javadoc and I'm happy.

marcel commented 2026-06-11 18:13:27 +02:00

Author

Owner

Copy Link

🔐 Nora "NullX" Steiner — Application Security Engineer

Verdict: 🚫 Changes requested

The fix itself is structurally sound from a security standpoint: the JPQL is fully parameterized (:documentId named param — no injection surface), the event payload carries only a UUID (no sensitive data crossing the in-process boundary), logging is SLF4J-parameterized with system-generated values only, and no endpoint, permission, or SecurityConfig surface changes. The synchronous in-transaction listener also fails closed: if the cleanup throws, the whole delete rolls back — no partial state.

Blockers

1. DOCUMENT_DELETED is written with actorId = null — an audit record for a destructive action that cannot answer "who did it." (DocumentService.java:1107: auditService.logAfterCommit(AuditKind.DOCUMENT_DELETED, null, id, null).)

This is the inverse of the codebase's own standard: USER_DELETED, FILE_UPLOADED, STATUS_CHANGED, COMMENT_ADDED, ANNOTATION_CREATED all record the actor. Document deletion is arguably the most accountability-sensitive operation in a family archive — it destroys an irreplaceable historical record and, with this PR, silently removes journey items as collateral. An audit trail that can show that a document was deleted but not by whom fails its core purpose during incident reconstruction ("who deleted Opa's 1943 letter?").

The fix follows the existing pattern — thread the actor through from the controller like every other write path:

@Transactional
public void deleteDocument(UUID id, UUID actorId) {
    ...
    auditService.logAfterCommit(AuditKind.DOCUMENT_DELETED, actorId, id, null);
}

And add the regression assertion in JourneyItemDocumentDeleteTest (AC-7 currently uses any() for the actor slot — it would not catch this):

verify(auditService).logAfterCommit(eq(AuditKind.DOCUMENT_DELETED), eq(writer.getId()), eq(doc.getId()), any());

CWE-778 (Insufficient Logging) — not exploitable, but it degrades the control that every other mutation in this system upholds.

Suggestions

2. AuditKind.java:57 javadoc claims Payload: {"documentId": "uuid"} but the payload is null. Audit javadocs are the schema your future forensics queries are written against — an inaccurate one causes someone to query payload->>'documentId' and conclude no documents were ever deleted. Fix the doc or send the payload (Felix flagged the same).

3. Consider including the cascade count in the audit payload ({"journeyItemsRemoved": n}). The listener already computes it; persisting it would make the audit record self-contained instead of requiring Loki correlation. Optional.

One small change, big accountability win. Everything else in this PR I'd hold up as the reference for how to do constraint-collision fixes safely.

## 🔐 Nora "NullX" Steiner — Application Security Engineer **Verdict: 🚫 Changes requested** The fix itself is structurally sound from a security standpoint: the JPQL is fully parameterized (`:documentId` named param — no injection surface), the event payload carries only a UUID (no sensitive data crossing the in-process boundary), logging is SLF4J-parameterized with system-generated values only, and no endpoint, permission, or `SecurityConfig` surface changes. The synchronous in-transaction listener also fails closed: if the cleanup throws, the whole delete rolls back — no partial state. ### Blockers 1. **`DOCUMENT_DELETED` is written with `actorId = null` — an audit record for a destructive action that cannot answer "who did it."** (`DocumentService.java:1107`: `auditService.logAfterCommit(AuditKind.DOCUMENT_DELETED, null, id, null)`.) This is the inverse of the codebase's own standard: `USER_DELETED`, `FILE_UPLOADED`, `STATUS_CHANGED`, `COMMENT_ADDED`, `ANNOTATION_CREATED` all record the actor. Document deletion is arguably the **most** accountability-sensitive operation in a family archive — it destroys an irreplaceable historical record and, with this PR, silently removes journey items as collateral. An audit trail that can show *that* a document was deleted but not *by whom* fails its core purpose during incident reconstruction ("who deleted Opa's 1943 letter?"). The fix follows the existing pattern — thread the actor through from the controller like every other write path: ```java @Transactional public void deleteDocument(UUID id, UUID actorId) { ... auditService.logAfterCommit(AuditKind.DOCUMENT_DELETED, actorId, id, null); } ``` And add the regression assertion in `JourneyItemDocumentDeleteTest` (AC-7 currently uses `any()` for the actor slot — it would not catch this): ```java verify(auditService).logAfterCommit(eq(AuditKind.DOCUMENT_DELETED), eq(writer.getId()), eq(doc.getId()), any()); ``` CWE-778 (Insufficient Logging) — not exploitable, but it degrades the control that every other mutation in this system upholds. ### Suggestions 2. **`AuditKind.java:57` javadoc claims `Payload: {"documentId": "uuid"}` but the payload is `null`.** Audit javadocs are the schema your future forensics queries are written against — an inaccurate one causes someone to query `payload->>'documentId'` and conclude no documents were ever deleted. Fix the doc or send the payload (Felix flagged the same). 3. Consider including the cascade count in the audit payload (`{"journeyItemsRemoved": n}`). The listener already computes it; persisting it would make the audit record self-contained instead of requiring Loki correlation. Optional. One small change, big accountability win. Everything else in this PR I'd hold up as the reference for how to do constraint-collision fixes safely.

marcel commented 2026-06-11 18:13:43 +02:00

Author

Owner

Copy Link

🧪 Sara Holt — QA Engineer & Test Strategist

Verdict: ⚠️ Approved with concerns

This is a test suite I'd point people at. Real PostgreSQL via Testcontainers (the chk_journey_item_not_empty CHECK constraint is the entire subject of this bug — H2 would have been worthless), every AC has a named test that reads as a sentence, and three techniques deserve explicit praise:

• AC-5 rollback proof via @MockitoSpyBean DocumentRepository + doThrow on deleteById — this is the test that pins the @EventListener (not @Async) choice. If anyone ever "optimizes" the listener to async, this test fails. The load-bearing design decision has a load-bearing test.
• AC-6 raw JdbcTemplate inserts for note = '' — testing a state the service layer can't produce by going under it. Correct layer choice.
• AC-7's positive assertion (DOCUMENT_DELETED emitted) alongside the negative one (JOURNEY_ITEM_REMOVED absent) — the ADR even explains why: absence-only assertions pass vacuously when auditing regresses entirely. Someone thought about vacuous truth. Thank you.

The em.clear() discipline between arrange/act/assert phases is consistent, and updating the two existing JourneyItemIntegrationTest tests to route through DocumentService (rather than deleting them) preserves the placeholder-UX coverage under the new delete path.

Concerns

1. AC-7's actor slot is any() — verify(auditService).logAfterCommit(eq(AuditKind.DOCUMENT_DELETED), any(), eq(doc.getId()), any()). Nora has requested actorId be threaded through; when that lands, tighten this to eq(writer.getId()). As written, the test cannot catch an actor regression — and it didn't (the actor is currently null).

2. Every checkbox in the PR test plan is unchecked. The description asserts "all pass" in prose while the boxes say nothing ran. Tick them (or let CI speak). I don't gate on prose.

3. Cleanup is manual deleteAll() in @AfterEach rather than @Transactional rollback — consistent with the existing JourneyItemIntegrationTest pattern, and necessary here since the test exercises real commits across the event boundary, so I accept it. Note the deletion order (journeyItemRepository → docRepo → geschichteRepository) is FK-load-bearing; a comment would save the next person a constraint violation.

Test-strategy note

Coverage at the right layer: 8 integration tests for a constraint-interaction bug, zero new E2E. Correct — this bug lives between Hibernate, Spring events, and Postgres constraints; no browser needed. The post-deploy smoke action in the test plan (trigger a previously-500ing delete, watch GlitchTip/Loki) covers the production-config gap that Testcontainers can't.

Tighten the AC-7 verify once the actor fix lands and this is a model PR from the QA side.

## 🧪 Sara Holt — QA Engineer & Test Strategist **Verdict: ⚠️ Approved with concerns** This is a test suite I'd point people at. Real PostgreSQL via Testcontainers (the `chk_journey_item_not_empty` CHECK constraint is the entire subject of this bug — H2 would have been worthless), every AC has a named test that reads as a sentence, and three techniques deserve explicit praise: - **AC-5 rollback proof** via `@MockitoSpyBean DocumentRepository` + `doThrow` on `deleteById` — this is the test that pins the `@EventListener` (not `@Async`) choice. If anyone ever "optimizes" the listener to async, this test fails. The load-bearing design decision has a load-bearing test. - **AC-6 raw `JdbcTemplate` inserts** for `note = ''` — testing a state the service layer can't produce by going under it. Correct layer choice. - **AC-7's positive assertion** (`DOCUMENT_DELETED` emitted) alongside the negative one (`JOURNEY_ITEM_REMOVED` absent) — the ADR even explains why: absence-only assertions pass vacuously when auditing regresses entirely. Someone thought about vacuous truth. Thank you. The `em.clear()` discipline between arrange/act/assert phases is consistent, and updating the two existing `JourneyItemIntegrationTest` tests to route through `DocumentService` (rather than deleting them) preserves the placeholder-UX coverage under the new delete path. ### Concerns 1. **AC-7's actor slot is `any()`** — `verify(auditService).logAfterCommit(eq(AuditKind.DOCUMENT_DELETED), any(), eq(doc.getId()), any())`. Nora has requested `actorId` be threaded through; when that lands, tighten this to `eq(writer.getId())`. As written, the test cannot catch an actor regression — and it didn't (the actor is currently `null`). 2. **Every checkbox in the PR test plan is unchecked.** The description asserts "all pass" in prose while the boxes say nothing ran. Tick them (or let CI speak). I don't gate on prose. 3. **Cleanup is manual `deleteAll()` in `@AfterEach`** rather than `@Transactional` rollback — consistent with the existing `JourneyItemIntegrationTest` pattern, and necessary here since the test exercises real commits across the event boundary, so I accept it. Note the deletion order (`journeyItemRepository` → `docRepo` → `geschichteRepository`) is FK-load-bearing; a comment would save the next person a constraint violation. ### Test-strategy note Coverage at the right layer: 8 integration tests for a constraint-interaction bug, zero new E2E. Correct — this bug lives between Hibernate, Spring events, and Postgres constraints; no browser needed. The post-deploy smoke action in the test plan (trigger a previously-500ing delete, watch GlitchTip/Loki) covers the production-config gap that Testcontainers can't. Tighten the AC-7 verify once the actor fix lands and this is a model PR from the QA side.

marcel commented 2026-06-11 18:13:54 +02:00

Author

Owner

Copy Link

🛠️ Tobias Wendt — DevOps & Platform Engineer

Verdict: ✅ Approved

What I checked: no Compose changes, no Dockerfile changes, no CI workflow changes, no new env vars, no Flyway migration, no new ports or services. The fix is pure application code — exactly the kind of "no schema change, no migration" P1 fix that deploys with zero infrastructure ceremony and rolls back by redeploying the previous JAR. Nothing to size, nothing to firewall.

Things done right from the operations seat:

• Merge ordering is structurally enforced, not process-enforced. Targeting feat/issue-795-story-documents instead of main means PR #804 cannot ship without this fix — the "⚠️ Merge order" warning in the description is satisfied by the branch topology itself, not by someone remembering to click merge in the right order. That's the correct way to encode a deploy-order constraint. One caveat: if #804's branch gets rebased or this PR lingers, re-verify the base before merging.
• The log.warn("Cascade-deleted {} note-less journey item(s) for document {}", ...) line is an operable signal. It gives me a Loki query (|= "Cascade-deleted") to quantify how often this path fires in production — which directly answers whether the pre-fix 500 was hurting real users. Structured, parameterized, counts included.
• The post-deploy smoke action in the test plan is the right instinct — actively triggering the previously-500ing delete and checking GlitchTip/Loki for chk_journey_item_not_empty beats "I checked the logs and it looks fine."

Suggestions

1. After this lands on prod (via #804), consider a one-time GlitchTip check that the pre-existing chk_journey_item_not_empty error group stops accruing events, then resolve the group — otherwise the stale group muddies the error-rate signal.
2. The test plan checkboxes are all unticked — tick them once CI is green so the PR record reflects reality (Sara flagged this too).

No infrastructure concerns. LGTM.

## 🛠️ Tobias Wendt — DevOps & Platform Engineer **Verdict: ✅ Approved** What I checked: no Compose changes, no Dockerfile changes, no CI workflow changes, no new env vars, no Flyway migration, no new ports or services. The fix is pure application code — exactly the kind of "no schema change, no migration" P1 fix that deploys with zero infrastructure ceremony and rolls back by redeploying the previous JAR. Nothing to size, nothing to firewall. Things done right from the operations seat: - **Merge ordering is structurally enforced, not process-enforced.** Targeting `feat/issue-795-story-documents` instead of `main` means PR #804 *cannot* ship without this fix — the "⚠️ Merge order" warning in the description is satisfied by the branch topology itself, not by someone remembering to click merge in the right order. That's the correct way to encode a deploy-order constraint. One caveat: if #804's branch gets rebased or this PR lingers, re-verify the base before merging. - **The `log.warn("Cascade-deleted {} note-less journey item(s) for document {}", ...)` line is an operable signal.** It gives me a Loki query (`|= "Cascade-deleted"`) to quantify how often this path fires in production — which directly answers whether the pre-fix 500 was hurting real users. Structured, parameterized, counts included. - **The post-deploy smoke action in the test plan is the right instinct** — actively triggering the previously-500ing delete and checking GlitchTip/Loki for `chk_journey_item_not_empty` beats "I checked the logs and it looks fine." ### Suggestions 1. After this lands on prod (via #804), consider a one-time GlitchTip check that the pre-existing `chk_journey_item_not_empty` error group stops accruing events, then resolve the group — otherwise the stale group muddies the error-rate signal. 2. The test plan checkboxes are all unticked — tick them once CI is green so the PR record reflects reality (Sara flagged this too). No infrastructure concerns. LGTM.

marcel commented 2026-06-11 18:14:10 +02:00

Author

Owner

Copy Link

📋 Elicit — Requirements Engineer & Business Analyst

Verdict: ⚠️ Approved with concerns

Traceability on this PR is exemplary: AC-1 through AC-7 each map to a named test, the headline test (deleting_document_linked_via_note_less_item_deletes_item_not_500) states the user-visible outcome, and ADR-038 records why alternatives (RESTRICT, relaxing the CHECK, DB trigger) were rejected in terms of user-facing behavior ("breaks the note-carrying placeholder UX: ... would 409 instead of preserving the item"). That is requirements thinking embedded in an architecture document — exactly where it belongs.

I am not reviewing implementation. My scope is whether the specified behavior is the right behavior and what was left unasked. Two open questions:

OQ-1 — Silent collateral deletion: should the curator be told?

Why it matters: With this fix, deleting a document silently removes note-less journey items from any Lesereise that references it. The curator who built that journey gets no warning, no notification, no trace in the journey's history — the item simply vanishes. The behavior is correct (the alternative was a 500), but the requirement "what does the curator experience when their journey loses an item?" was never elicited.
Blocks: nothing in this PR — the P1 must ship.
Recommendation: open a follow-up issue: "When deleting a document, show affected Lesereisen (count of items to be removed / converted to placeholders) in the delete confirmation." The listener already computes the count; the UI just never sees it. Given the product frame — family members curating journeys for relatives — discovering a hole in your journey weeks later erodes trust in the archive.

OQ-2 — Whitespace-only notes survive as placeholders: intended, or accidental precision?

Why it matters: AC-6 specifies note = '' is cascaded but note = ' ' is preserved as a "note-carrying placeholder" — a placeholder whose note renders as visually nothing. The ADR documents this boundary honestly, but documents it as a consequence of the predicate, not as an elicited decision. A reader of a journey sees an empty placeholder card and learns nothing.
Blocks: nothing — service-layer normalizeNote means this state only arises from legacy/raw-SQL data.
Recommendation: record the decision explicitly (one line in the follow-up issue or the ADR): either "whitespace notes are treated as content by design" or "a future data-hygiene pass will null-trim them." Don't let it remain an accident that behaves like a decision.

Minor

• The AuditKind javadoc/payload mismatch (flagged by Felix and Nora) is also a documentation requirement defect: the javadoc is the spec for audit consumers. Same fix serves both.

Ship the P1; park OQ-1 as a Should for a future release, OQ-2 as a one-line decision record.

## 📋 Elicit — Requirements Engineer & Business Analyst **Verdict: ⚠️ Approved with concerns** Traceability on this PR is exemplary: AC-1 through AC-7 each map to a named test, the headline test (`deleting_document_linked_via_note_less_item_deletes_item_not_500`) states the user-visible outcome, and ADR-038 records why alternatives (RESTRICT, relaxing the CHECK, DB trigger) were rejected in terms of *user-facing behavior* ("breaks the note-carrying placeholder UX: ... would 409 instead of preserving the item"). That is requirements thinking embedded in an architecture document — exactly where it belongs. I am not reviewing implementation. My scope is whether the specified behavior is the *right* behavior and what was left unasked. Two open questions: ### OQ-1 — Silent collateral deletion: should the curator be told? **Why it matters:** With this fix, deleting a document silently removes note-less journey items from any Lesereise that references it. The curator who built that journey gets no warning, no notification, no trace in the journey's history — the item simply vanishes. The behavior is *correct* (the alternative was a 500), but the requirement "what does the curator experience when their journey loses an item?" was never elicited. **Blocks:** nothing in this PR — the P1 must ship. **Recommendation:** open a follow-up issue: *"When deleting a document, show affected Lesereisen (count of items to be removed / converted to placeholders) in the delete confirmation."* The listener already computes the count; the UI just never sees it. Given the product frame — family members curating journeys for relatives — discovering a hole in your journey weeks later erodes trust in the archive. ### OQ-2 — Whitespace-only notes survive as placeholders: intended, or accidental precision? **Why it matters:** AC-6 specifies `note = ''` is cascaded but `note = ' '` is preserved as a "note-carrying placeholder" — a placeholder whose note renders as visually nothing. The ADR documents this boundary honestly, but documents it as a consequence of the predicate, not as an elicited decision. A reader of a journey sees an empty placeholder card and learns nothing. **Blocks:** nothing — service-layer `normalizeNote` means this state only arises from legacy/raw-SQL data. **Recommendation:** record the decision explicitly (one line in the follow-up issue or the ADR): either "whitespace notes are treated as content by design" or "a future data-hygiene pass will null-trim them." Don't let it remain an accident that behaves like a decision. ### Minor - The `AuditKind` javadoc/payload mismatch (flagged by Felix and Nora) is also a *documentation requirement* defect: the javadoc is the spec for audit consumers. Same fix serves both. Ship the P1; park OQ-1 as a Should for a future release, OQ-2 as a one-line decision record.

marcel commented 2026-06-11 18:14:19 +02:00

Author

Owner

Copy Link

🎨 Leonie Voss — UX Design Lead & Accessibility Advocate

Verdict: ✅ Approved

This PR contains no frontend changes — no Svelte components, no routes, no styling, no i18n keys — so there is nothing for me to gate on. What I checked and confirmed:

• The user-facing effect of this fix is the removal of a raw 500. Before: a family member deletes a document that happens to sit in someone's Lesereise and gets an unstyled server error that bypasses GlobalExceptionHandler — for our 60+ transcriber audience, that reads as "I broke the archive." After: the delete just works. That alone is a meaningful UX improvement, delivered entirely in the backend.
• The note-carrying placeholder behavior is preserved (AC-2, and the two updated JourneyItemIntegrationTest cases) — a curator's written context survives the document's deletion, which respects the user's authored content. Right priority.
• No new error states reach the UI, so no new ErrorCode → getErrorMessage() → messages/{de,en,es}.json chain is needed. Verified: the diff adds no ErrorCode.

One forward-looking note, seconding Elicit's OQ-1: journey items now disappear silently when a document is deleted. The curator gets no signal — not at delete time, not when next viewing their journey. When the follow-up lands, the delete confirmation should name the affected journeys in plain language ("Dieses Dokument ist Teil von 2 Lesereisen. Einträge ohne Notiz werden entfernt.") — serif body text, minimum 16px, and a real confirmation button at 44px, not a browser confirm(). Happy to spec it when that issue is opened.

Nothing to fix here. LGTM.

## 🎨 Leonie Voss — UX Design Lead & Accessibility Advocate **Verdict: ✅ Approved** This PR contains no frontend changes — no Svelte components, no routes, no styling, no i18n keys — so there is nothing for me to gate on. What I checked and confirmed: - **The user-facing effect of this fix is the removal of a raw 500.** Before: a family member deletes a document that happens to sit in someone's Lesereise and gets an unstyled server error that bypasses `GlobalExceptionHandler` — for our 60+ transcriber audience, that reads as "I broke the archive." After: the delete just works. That alone is a meaningful UX improvement, delivered entirely in the backend. - **The note-carrying placeholder behavior is preserved** (AC-2, and the two updated `JourneyItemIntegrationTest` cases) — a curator's written context survives the document's deletion, which respects the user's authored content. Right priority. - **No new error states reach the UI**, so no new `ErrorCode` → `getErrorMessage()` → `messages/{de,en,es}.json` chain is needed. Verified: the diff adds no ErrorCode. One forward-looking note, seconding Elicit's OQ-1: journey items now disappear *silently* when a document is deleted. The curator gets no signal — not at delete time, not when next viewing their journey. When the follow-up lands, the delete confirmation should name the affected journeys in plain language ("Dieses Dokument ist Teil von 2 Lesereisen. Einträge ohne Notiz werden entfernt.") — serif body text, minimum 16px, and a real confirmation button at 44px, not a browser `confirm()`. Happy to spec it when that issue is opened. Nothing to fix here. LGTM.

marcel referenced this issue from a commit 2026-06-11 19:24:51 +02:00

fix(document): record actorId on DOCUMENT_DELETED audit (#805)

marcel added 6 commits 2026-06-11 19:24:51 +02:00

fix(document): record actorId on DOCUMENT_DELETED audit (#805) ... c2b98e6e84

deleteDocument now threads the acting user through to the audit log so
DOCUMENT_DELETED records who deleted the document, matching every other
audited write path (storeDocument, updateDocument, applyBulkEditToDocument,
attachFile). Tightens the AC-7 assertion from any() to the concrete actor.

Also adds the missing ApplicationEventPublisher @Mock to DocumentServiceTest,
which the publishEvent call (added with the cascade fix) left null.

Addresses @nora (CWE-778, blocker), @felix and @sara review on PR #806.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

refactor(document): import ApplicationEventPublisher instead of FQN field (#805) ... e9b8920b3b

Replaces the fully-qualified org.springframework.context.ApplicationEventPublisher
field declaration with an import + simple type name, consistent with every
other field in DocumentService.

Addresses @felix review on PR #806.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

docs(audit): correct DOCUMENT_DELETED payload javadoc to none (#805) ... 84c5ff5b3f

The call site passes a null payload and carries the id in the documentId
column (matching FILE_UPLOADED), so the javadoc claiming Payload:
{"documentId": "uuid"} misdescribed the audit schema. Audit javadocs are the
contract forensic queries are written against.

Addresses @felix, @nora and @elicit review on PR #806.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

refactor(geschichte): make flush-before-delete explicit on note-less cleanup (#805) ... 00baf0d881

Adds flushAutomatically = true to @Modifying on deleteNoteLessByDocumentId so
the flush-before-bulk-delete contract is explicit instead of relying on
Hibernate AUTO flush-mode behaviour.

Addresses @markus review on PR #806.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

docs(architecture): draw JourneyItemDocumentDeleteListener event flow (#805) ... 6230deaa96

Adds the JourneyItemDocumentDeleteListener component and the
DocumentDeletingEvent edge (documentSvc -> listener) to the supporting-domains
l3 diagram. This is the first event-driven edge in the backend; the diagram
must make that in-process coupling visible per the doc-currency rule.

Addresses @markus review (blocker) on PR #806.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

test(geschichte): document FK-load-bearing cleanup order (#805) ... f65296172f

Adds a comment on the @AfterEach deletion sequence explaining that journey_items
must be removed before their referenced documents and geschichten.

Addresses @sara review on PR #806.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

marcel referenced this issue from a commit 2026-06-11 19:24:52 +02:00

refactor(document): import ApplicationEventPublisher instead of FQN field (#805)

marcel referenced this issue from a commit 2026-06-11 19:24:52 +02:00

docs(audit): correct DOCUMENT_DELETED payload javadoc to none (#805)

marcel referenced this issue from a commit 2026-06-11 19:24:52 +02:00

refactor(geschichte): make flush-before-delete explicit on note-less cleanup (#805)

marcel referenced this issue from a commit 2026-06-11 19:24:52 +02:00

docs(architecture): draw JourneyItemDocumentDeleteListener event flow (#805)

marcel referenced this issue from a commit 2026-06-11 19:24:52 +02:00

test(geschichte): document FK-load-bearing cleanup order (#805)

marcel commented 2026-06-11 19:26:53 +02:00

Author

Owner

Copy Link

👨‍💻 Felix Brandt — Review concerns addressed

Pushed six atomic commits on feat/issue-805-journey-note-less-cascade (eefc67bd..f6529617). Mapping each to the concern it resolves:

Blockers

Concern	Resolution	Commit
🔐 @nora (CWE-778) — DOCUMENT_DELETED written with actorId = null	deleteDocument(UUID id, UUID actorId) threads the actor from the controller via requireUserId(authentication), matching every other audited write path; AC-7 verify tightened from any() to eq(writer.getId())	c2b98e6e
👨‍💻 @felix — fully-qualified ApplicationEventPublisher field	Replaced with an import + simple type name	e9b8920b
👨‍💻 @felix / 🔐 @nora / 📋 @elicit — AuditKind.DOCUMENT_DELETED javadoc contradicts the call site	Corrected to Payload: none — the deleted document's id is carried in the documentId column (matches the FILE_UPLOADED convention)	84c5ff5b
🏛️ @markus — l3-backend-3g-supporting.puml missing the new component	Added JourneyItemDocumentDeleteListener + the DocumentDeletingEvent edge (documentSvc → listener) and the listener→DB write	6230deaa

Suggestions / concerns

Concern	Resolution	Commit
🏛️ @markus — make flush explicit	@Modifying(clearAutomatically = true, flushAutomatically = true)	00baf0d8
🧪 @sara — FK-load-bearing cleanup order undocumented	Comment added on the @AfterEach deletion sequence	f6529617
🧪 @sara / 🛠️ @tobias — test-plan checkboxes unticked	Ticked the four verified boxes in the description (post-deploy smoke action left unchecked)	—

Extra fix found while addressing the actor concern

DocumentServiceTest.deleteDocument_deletesById_whenExists was already red on the head branch: the eventPublisher.publishEvent call added with the cascade fix left eventPublisher null under @InjectMocks (no @Mock for it). The reviewers couldn't have caught it — the PR test plan never listed DocumentServiceTest. Added the missing @Mock ApplicationEventPublisher in c2b98e6e.

Verification (all green locally, real Postgres via Testcontainers)

• JourneyItemDocumentDeleteTest — 8/8
• JourneyItemIntegrationTest — 15/15
• JourneyItemServiceTest + JourneyItemConstraintsTest — 52/52
• DocumentServiceTest — 187/187 · DocumentControllerTest — 105/105
• ./mvnw clean package -DskipTests — build clean (261 main + 140 test sources compile)

Deliberately not done in this PR

• 📋 @elicit OQ-1 (delete-confirmation UI showing affected Lesereisen) and OQ-2 (whitespace-note decision record) — follow-up scope; the P1 ships first.
• 🔐 @nora suggestion #3 / 📋 @elicit OQ-2 — cascade count in the audit payload: the count lives in the listener, not DocumentService, so surfacing it would mean plumbing it back through the event; payload stays none. Left as a deliberate decision.
• 🏛️ @markus suggestion #3 (CLAUDE.md sentence on the event pattern) — marked "not required; ADR is discoverable".
• 🛠️ @tobias suggestion #1 (resolve the stale GlitchTip error group) — operational, post-deploy.

🤖 Generated with Claude Code

## 👨‍💻 Felix Brandt — Review concerns addressed Pushed six atomic commits on `feat/issue-805-journey-note-less-cascade` (`eefc67bd..f6529617`). Mapping each to the concern it resolves: ### Blockers | Concern | Resolution | Commit | |---------|-----------|--------| | 🔐 @nora (CWE-778) — `DOCUMENT_DELETED` written with `actorId = null` | `deleteDocument(UUID id, UUID actorId)` threads the actor from the controller via `requireUserId(authentication)`, matching every other audited write path; AC-7 verify tightened from `any()` to `eq(writer.getId())` | `c2b98e6e` | | 👨‍💻 @felix — fully-qualified `ApplicationEventPublisher` field | Replaced with an `import` + simple type name | `e9b8920b` | | 👨‍💻 @felix / 🔐 @nora / 📋 @elicit — `AuditKind.DOCUMENT_DELETED` javadoc contradicts the call site | Corrected to `Payload: none — the deleted document's id is carried in the documentId column` (matches the `FILE_UPLOADED` convention) | `84c5ff5b` | | 🏛️ @markus — `l3-backend-3g-supporting.puml` missing the new component | Added `JourneyItemDocumentDeleteListener` + the `DocumentDeletingEvent` edge (`documentSvc → listener`) and the listener→DB write | `6230deaa` | ### Suggestions / concerns | Concern | Resolution | Commit | |---------|-----------|--------| | 🏛️ @markus — make flush explicit | `@Modifying(clearAutomatically = true, flushAutomatically = true)` | `00baf0d8` | | 🧪 @sara — FK-load-bearing cleanup order undocumented | Comment added on the `@AfterEach` deletion sequence | `f6529617` | | 🧪 @sara / 🛠️ @tobias — test-plan checkboxes unticked | Ticked the four verified boxes in the description (post-deploy smoke action left unchecked) | — | ### Extra fix found while addressing the actor concern `DocumentServiceTest.deleteDocument_deletesById_whenExists` was **already red** on the head branch: the `eventPublisher.publishEvent` call added with the cascade fix left `eventPublisher` null under `@InjectMocks` (no `@Mock` for it). The reviewers couldn't have caught it — the PR test plan never listed `DocumentServiceTest`. Added the missing `@Mock ApplicationEventPublisher` in `c2b98e6e`. ### Verification (all green locally, real Postgres via Testcontainers) - `JourneyItemDocumentDeleteTest` — 8/8 - `JourneyItemIntegrationTest` — 15/15 - `JourneyItemServiceTest` + `JourneyItemConstraintsTest` — 52/52 - `DocumentServiceTest` — 187/187 · `DocumentControllerTest` — 105/105 - `./mvnw clean package -DskipTests` — build clean (261 main + 140 test sources compile) ### Deliberately not done in this PR - **📋 @elicit OQ-1** (delete-confirmation UI showing affected Lesereisen) and **OQ-2** (whitespace-note decision record) — follow-up scope; the P1 ships first. - **🔐 @nora suggestion #3 / 📋 @elicit OQ-2** — cascade count in the audit payload: the count lives in the listener, not `DocumentService`, so surfacing it would mean plumbing it back through the event; payload stays `none`. Left as a deliberate decision. - **🏛️ @markus suggestion #3** (CLAUDE.md sentence on the event pattern) — marked "not required; ADR is discoverable". - **🛠️ @tobias suggestion #1** (resolve the stale GlitchTip error group) — operational, post-deploy. 🤖 Generated with [Claude Code](https://claude.ai/claude-code)

marcel merged commit f65296172f into feat/issue-795-story-documents 2026-06-11 19:36:06 +02:00

marcel deleted branch feat/issue-805-journey-note-less-cascade 2026-06-11 19:36:07 +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

needs-discussion

Has an open decision or design question that must be resolved before implementation can start.

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#806