feat(timeline): derive person life-events (Geburt/Tod/Heirat) — issue #776 #825

Merged

marcel merged 6 commits from worktree-feat-issue-776-derived-person-life-events into main 2026-06-13 15:13:29 +02:00

Conversation 7 Commits 6 Files Changed 10 +659 -8

marcel commented 2026-06-13 14:40:54 +02:00

Owner

Copy Link

Summary

Implements the backend assembly layer for derived person life-events on the family timeline (issue #776, Zeitstrahl milestone #14). Births, deaths, and marriages are computed on-read from curated Person + PersonRelationship data — never stored.

• ADR-043 written in Proposed status (pre-condition gate for this issue)
• DerivedEventType enum (BIRTH / DEATH / MARRIAGE) added to timeline package
• TimelineEntryDTO record created — unified DTO for derived + curated events; id: String to accommodate synthetic prefixed ids (birth:, death:, marriage:)
• RelationshipService.findAllSpouseEdges() — new method wrapping the existing JOIN FETCH query for SPOUSE_OF edges; no N+1 (REQ-011)
• TimelineEventService.assembleDerivedEvents() — public @Transactional(readOnly=true) orchestrator; calls buildBirthEvents(), buildDeathEvents(), buildMarriageEvents() (all private)
• Synthetic ids are structurally non-UUID; write-rejection for prefixed ids is enforced in issue #5
• 16 Mockito unit tests in DerivedEventsAssemblyTest cover all 16 REQ-NNN requirements

Traceability

All REQ-001–REQ-016 implemented, tested, and marked Done in .specify/rtm.md.

Notes

• TimelineEntryDTO and DerivedEventType were planned for issues 774/775 but not created there — created fresh here
• generate:api deferred to issue #5 (no controller endpoint exposes TimelineEntryDTO yet)
• PersonRelationshipRepository.findAllByRelationTypeIn already JOIN FETCHes both person sides — no new repository query needed

Test plan

• DerivedEventsAssemblyTest — 16/16 green (./mvnw test -Dtest=DerivedEventsAssemblyTest)
• Backend builds cleanly (./mvnw clean package -DskipTests)
• Log audit: assembleDerivedEvents() only logs result.size() + persons.size() — no PII
• REQ-015: @Transactional(readOnly=true) visible on assembleDerivedEvents() in TimelineEventService

🤖 Generated with Claude Code

## Summary Implements the backend assembly layer for derived person life-events on the family timeline (issue #776, Zeitstrahl milestone #14). Births, deaths, and marriages are computed on-read from curated `Person` + `PersonRelationship` data — never stored. - **ADR-043** written in Proposed status (pre-condition gate for this issue) - **`DerivedEventType`** enum (`BIRTH` / `DEATH` / `MARRIAGE`) added to timeline package - **`TimelineEntryDTO`** record created — unified DTO for derived + curated events; `id: String` to accommodate synthetic prefixed ids (`birth:`, `death:`, `marriage:`) - **`RelationshipService.findAllSpouseEdges()`** — new method wrapping the existing JOIN FETCH query for `SPOUSE_OF` edges; no N+1 (REQ-011) - **`TimelineEventService.assembleDerivedEvents()`** — public `@Transactional(readOnly=true)` orchestrator; calls `buildBirthEvents()`, `buildDeathEvents()`, `buildMarriageEvents()` (all private) - Synthetic ids are structurally non-UUID; write-rejection for prefixed ids is enforced in issue #5 - 16 Mockito unit tests in `DerivedEventsAssemblyTest` cover all 16 REQ-NNN requirements ## Traceability All REQ-001–REQ-016 implemented, tested, and marked Done in `.specify/rtm.md`. ## Notes - `TimelineEntryDTO` and `DerivedEventType` were planned for issues 774/775 but not created there — created fresh here - `generate:api` deferred to issue #5 (no controller endpoint exposes `TimelineEntryDTO` yet) - `PersonRelationshipRepository.findAllByRelationTypeIn` already JOIN FETCHes both person sides — no new repository query needed ## Test plan - [ ] `DerivedEventsAssemblyTest` — 16/16 green (`./mvnw test -Dtest=DerivedEventsAssemblyTest`) - [ ] Backend builds cleanly (`./mvnw clean package -DskipTests`) - [ ] Log audit: `assembleDerivedEvents()` only logs `result.size()` + `persons.size()` — no PII - [ ] REQ-015: `@Transactional(readOnly=true)` visible on `assembleDerivedEvents()` in `TimelineEventService` 🤖 Generated with [Claude Code](https://claude.com/claude-code)

marcel added 5 commits 2026-06-13 14:40:55 +02:00

docs(adr): add ADR-043 — derived person life-events on-read strategy (Proposed) ... 663ffad49b

Covers three pre-implementation decisions for issue #776:
1. On-read assembly, never persisted (no migration)
2. Synthetic prefixed String ids (birth:/death:/marriage:)
3. assembleDerivedEvents() as the public cross-issue contract on TimelineService

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

feat(timeline): add DerivedEventType enum and TimelineEntryDTO record ... 4245b821b9

DerivedEventType: BIRTH / DEATH / MARRIAGE discriminator for derived events.
TimelineEntryDTO: unified String-id DTO for both curated and derived events;
id is String (not UUID) to accommodate synthetic prefixed ids (birth:/death:/marriage:).

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

feat(relationship): add findAllSpouseEdges() for timeline assembly ... 7810ca7dd7

Returns all SPOUSE_OF edges with JOIN FETCH on both person sides,
preventing N+1 in TimelineService.assembleDerivedEvents() (REQ-011).
Reuses existing findAllByRelationTypeIn query which already JOIN FETCHes.

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

feat(timeline): implement assembleDerivedEvents() with TDD (REQ-001–REQ-016) ... c66d83cfc6

Adds RelationshipService dependency to TimelineEventService and implements:
- assembleDerivedEvents() — public @Transactional(readOnly=true) orchestrator
- buildBirthEvents() — Person.birthDate → BIRTH events with precision pass-through
- buildDeathEvents() — Person.deathDate → DEATH events with precision pass-through
- buildMarriageEvents() — SPOUSE_OF edges → MARRIAGE events, dedup on row id

Synthetic prefixed ids (birth:/death:/marriage:) are structurally non-UUID.
Null fromYear marriages are emitted with eventDate=null + precision=UNKNOWN (REQ-006).
Non-family-member persons excluded from birth/death; SPOUSE_OF edges always emit (REQ-013).

All 16 tests in DerivedEventsAssemblyTest pass.

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

docs(timeline): update RTM and CLAUDE.md for issue #776 ... 033001559d

RTM: add REQ-001–REQ-016 rows with Done status, implementation files, and test IDs.
CLAUDE.md: expand timeline package entry with TimelineEntryDTO, DerivedEventType,
and assembleDerivedEvents(); add TimelineEntryDTO to domain model table.

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

marcel commented 2026-06-13 14:42:45 +02:00

Author

Owner

Copy Link

Requirements Engineer — PR Code Review

Verdict: ✅ Approved

Traceability table — checking every REQ-001–REQ-016 against the PR diff:

REQ-ID	Requirement Summary	Implemented?	Tested?	RTM Status	Notes
REQ-001	family-member Person + birthDate → 1 BIRTH event, precision pass-through	✅ buildBirthEvents()	✅ should_emit_one_geburt_for_person_with_birthdate, should_pass_birth_precision_through_unchanged	Done	Precision asserted as equality (DAY == DAY)
REQ-002	family-member Person + deathDate → 1 DEATH event	✅ buildDeathEvents()	✅ should_emit_one_tod_for_person_with_deathdate, should_emit_one_tod_and_zero_geburt_for_person_with_deathdate_only	Done
REQ-003	null birthDate → 0 Geburt events	✅ filter p.getBirthDate() != null	✅ should_emit_zero_tod_when_person_has_birthdate_but_no_deathdate	Done	Test name says "tod" but actually tests REQ-003 (zero DEATH count for birth-only person); see minor note below
REQ-004	null deathDate → 0 Tod events; no dates → 0 events	✅ filter p.getDeathDate() != null	✅ should_emit_no_events_for_person_with_neither_date	Done
REQ-005	SPOUSE_OF + fromYear → MARRIAGE, eventDate={fromYear}-01-01, YEAR	✅ buildMarriageEvents()	✅ should_emit_one_heirat_for_spouse_edge_with_fromYear	Done	Precision asserted exactly
REQ-006	SPOUSE_OF + null fromYear → MARRIAGE emitted, eventDate=null, UNKNOWN	✅	✅ should_emit_unknown_precision_heirat_when_fromYear_is_null	Done
REQ-007	exactly one MARRIAGE per SPOUSE_OF row	✅ dedup on Set<UUID> seen	✅ should_emit_exactly_one_heirat_when_both_spouses_in_scope, should_emit_two_heirat_for_person_married_to_two_partners	Done
REQ-008	synthetic prefixed ids, never UUID	✅ birth:, death:, marriage: prefixes	✅ should_mint_prefixed_synthetic_ids_never_uuid	Done	UUID.fromString() throw assertion present
REQ-009	every derived event: derived=true, type=PERSONAL, non-null derivedType, non-UUID id	✅ all fields set in builders	✅ structural invariants in every test	Done
REQ-010	non-null non-blank primaryPersonName; Heirat also non-null non-blank relatedPersonName	✅ getDisplayName()	✅ should_emit_heirat_with_displayname_for_both_spouses	Done
REQ-011	exactly 1 call to findAllFamilyMembers() + 1 to findAllSpouseEdges()	✅ one call each in assembleDerivedEvents()	✅ by test structure (no per-person stubs)	Done
REQ-012	familyMember=false → excluded from Geburt/Tod	✅ via findAllFamilyMembers() delegation	✅ should_exclude_non_family_member_persons_from_derived_events	Done
REQ-013	SPOUSE_OF with one non-family spouse → 1 MARRIAGE emitted	✅ marriage list from edges, not re-filtered	✅ should_emit_heirat_when_one_spouse_is_not_family_member	Done
REQ-014	empty family-member list → empty result, no error	✅	✅ should_emit_zero_events_when_no_family_members	Done
REQ-015	@Transactional(readOnly=true) on assembleDerivedEvents()	✅ annotation present in diff	Code-review check (per spec)	Done
REQ-016	no PII in logs	✅ only result.size() + persons.size() logged	Log-audit (per spec)	Done

Minor observation (not blocking): The test should_emit_zero_tod_when_person_has_birthdate_but_no_deathdate — the name says "tod" but the assertion counts DerivedEventType.DEATH events, which correctly tests REQ-003 (zero DEATH for birth-only person). The name is slightly confusing ("zero tod when has birthdate" reads backwards — it's testing REQ-003: person with birth but no death → 0 DEATH events). Not a blocker, but a rename to should_emit_zero_death_events_for_person_with_birthdate_but_no_deathdate would aid RTM traceability.

RTM: All 16 rows present in .specify/rtm.md with Issue #776, Feature derive-person-life-events, Status Done. Implementation files and test names match the code. PASS.

Scope creep check: TimelineEntryDTO and DerivedEventType are new types — the spec explicitly allows this (see Resolved Decision #7 and the PR note). No out-of-scope behavior introduced. PASS.

Verdict: APPROVE — all REQ-001–REQ-016 implemented, tested, and traced.

### Requirements Engineer — PR Code Review **Verdict: ✅ Approved** Traceability table — checking every REQ-001–REQ-016 against the PR diff: | REQ-ID | Requirement Summary | Implemented? | Tested? | RTM Status | Notes | |--------|---------------------|-------------|---------|------------|-------| | REQ-001 | family-member Person + birthDate → 1 BIRTH event, precision pass-through | ✅ `buildBirthEvents()` | ✅ `should_emit_one_geburt_for_person_with_birthdate`, `should_pass_birth_precision_through_unchanged` | Done | Precision asserted as equality (DAY == DAY) | | REQ-002 | family-member Person + deathDate → 1 DEATH event | ✅ `buildDeathEvents()` | ✅ `should_emit_one_tod_for_person_with_deathdate`, `should_emit_one_tod_and_zero_geburt_for_person_with_deathdate_only` | Done | | | REQ-003 | null birthDate → 0 Geburt events | ✅ filter `p.getBirthDate() != null` | ✅ `should_emit_zero_tod_when_person_has_birthdate_but_no_deathdate` | Done | Test name says "tod" but actually tests REQ-003 (zero DEATH count for birth-only person); see minor note below | | REQ-004 | null deathDate → 0 Tod events; no dates → 0 events | ✅ filter `p.getDeathDate() != null` | ✅ `should_emit_no_events_for_person_with_neither_date` | Done | | | REQ-005 | SPOUSE_OF + fromYear → MARRIAGE, eventDate={fromYear}-01-01, YEAR | ✅ `buildMarriageEvents()` | ✅ `should_emit_one_heirat_for_spouse_edge_with_fromYear` | Done | Precision asserted exactly | | REQ-006 | SPOUSE_OF + null fromYear → MARRIAGE emitted, eventDate=null, UNKNOWN | ✅ | ✅ `should_emit_unknown_precision_heirat_when_fromYear_is_null` | Done | | | REQ-007 | exactly one MARRIAGE per SPOUSE_OF row | ✅ dedup on `Set<UUID> seen` | ✅ `should_emit_exactly_one_heirat_when_both_spouses_in_scope`, `should_emit_two_heirat_for_person_married_to_two_partners` | Done | | | REQ-008 | synthetic prefixed ids, never UUID | ✅ `birth:`, `death:`, `marriage:` prefixes | ✅ `should_mint_prefixed_synthetic_ids_never_uuid` | Done | `UUID.fromString()` throw assertion present | | REQ-009 | every derived event: derived=true, type=PERSONAL, non-null derivedType, non-UUID id | ✅ all fields set in builders | ✅ structural invariants in every test | Done | | | REQ-010 | non-null non-blank primaryPersonName; Heirat also non-null non-blank relatedPersonName | ✅ `getDisplayName()` | ✅ `should_emit_heirat_with_displayname_for_both_spouses` | Done | | | REQ-011 | exactly 1 call to findAllFamilyMembers() + 1 to findAllSpouseEdges() | ✅ one call each in `assembleDerivedEvents()` | ✅ by test structure (no per-person stubs) | Done | | | REQ-012 | familyMember=false → excluded from Geburt/Tod | ✅ via `findAllFamilyMembers()` delegation | ✅ `should_exclude_non_family_member_persons_from_derived_events` | Done | | | REQ-013 | SPOUSE_OF with one non-family spouse → 1 MARRIAGE emitted | ✅ marriage list from edges, not re-filtered | ✅ `should_emit_heirat_when_one_spouse_is_not_family_member` | Done | | | REQ-014 | empty family-member list → empty result, no error | ✅ | ✅ `should_emit_zero_events_when_no_family_members` | Done | | | REQ-015 | `@Transactional(readOnly=true)` on `assembleDerivedEvents()` | ✅ annotation present in diff | Code-review check (per spec) | Done | | | REQ-016 | no PII in logs | ✅ only `result.size()` + `persons.size()` logged | Log-audit (per spec) | Done | | **Minor observation (not blocking):** The test `should_emit_zero_tod_when_person_has_birthdate_but_no_deathdate` — the name says "tod" but the assertion counts `DerivedEventType.DEATH` events, which correctly tests REQ-003 (zero DEATH for birth-only person). The name is slightly confusing ("zero tod when has birthdate" reads backwards — it's testing REQ-003: person with birth but no death → 0 DEATH events). Not a blocker, but a rename to `should_emit_zero_death_events_for_person_with_birthdate_but_no_deathdate` would aid RTM traceability. **RTM:** All 16 rows present in `.specify/rtm.md` with Issue `#776`, Feature `derive-person-life-events`, Status `Done`. Implementation files and test names match the code. PASS. **Scope creep check:** `TimelineEntryDTO` and `DerivedEventType` are new types — the spec explicitly allows this (see Resolved Decision #7 and the PR note). No out-of-scope behavior introduced. PASS. Verdict: **APPROVE** — all REQ-001–REQ-016 implemented, tested, and traced.

marcel commented 2026-06-13 14:43:14 +02:00

Author

Owner

Copy Link

Developer (Felix Brandt) — PR Code Review

Verdict: ⚠️ Approved with concerns

Checklist:

#	Item	Status	Note
1	Layering rule — no cross-domain repository injection	PASS	TimelineEventService injects RelationshipService (a service), not PersonRelationshipRepository. Constitution §1.3 respected.
2	@Schema(requiredMode = REQUIRED) on always-populated fields	PASS	TimelineEntryDTO: id, type, precision, derived all marked REQUIRED. eventDate, derivedType, primaryPersonName, relatedPersonName nullable/optional — correctly left unmarked.
3	npm run generate:api	CONCERN	PR notes explicitly defer generate:api to issue #5: "no controller endpoint exposes TimelineEntryDTO yet." The spec task says "Commit the generated TypeScript types as part of this PR." The spec is clear this is a breaking OpenAPI change. However, since TimelineEntryDTO is not yet reachable from any controller endpoint (it's an internal service contract only), Springdoc won't include it in the OpenAPI spec until #5 wires the controller — so regenerating now would produce no diff. Deferral is acceptable given this reasoning, but issue #5 must regenerate before merge.
4	Lombok/Spring patterns	PASS	@Service, @RequiredArgsConstructor, @Slf4j all present on TimelineEventService. RelationshipService.findAllSpouseEdges() is a plain public method, no annotation needed.
5	@Transactional annotation discipline	PASS	assembleDerivedEvents() correctly uses @Transactional(readOnly = true). The three build* private helpers are rightly unannotated. No @Transactional on the class.
6	Function size and single responsibility	PASS	assembleDerivedEvents() is ~10 lines orchestrating three helpers. Each build* method is ~15 lines doing one job. Under the 20-line guideline.
7	Guard clauses and null safety	PASS	Birth/death filtering via stream .filter(p -> p.getBirthDate() != null) is clean. Marriage loop uses seen.add() idiom correctly. No nested conditionals.
8	Lazy-collection entities crossing controller boundary	PASS	No raw entities returned — assembleDerivedEvents() returns List<TimelineEntryDTO>. ADR-036 respected.
9	No cross-domain repository injection	PASS	Confirmed: TimelineEventService imports RelationshipService, not PersonRelationshipRepository. Constitution §1.3 satisfied.
10	Names reveal intent	PASS	buildBirthEvents, buildDeathEvents, buildMarriageEvents, assembleDerivedEvents — all self-documenting.
11	Dead code / backwards-compat shims	PASS	No shims added. Constitution §3.4 satisfied.
12	Records as DTOs	PASS	TimelineEntryDTO as a Java record is correct for an immutable DTO — modern Java 21 feature, aligns with architect.md guidance.

Concern — TimelineEntryDTO missing @Schema(requiredMode = REQUIRED) on primaryPersonName:

• primaryPersonName: String is REQUIRED for all derived events (REQ-009 + REQ-010). In the DTO it is left as nullable (no @Schema(REQUIRED)).
• This is architecturally correct for the unified DTO (curated events don't have primaryPersonName), but it means the OpenAPI spec will mark it as optional even though derived events always populate it.
• The spec explicitly states this field must be non-null for derived events. The @Schema omission is acceptable since the DTO serves two use cases, but issue #5/#6 should document this asymmetry in the Javadoc or add a discriminated-union pattern note.
• Not a blocker — the field is nullable in the record declaration, which is correct for the unified-DTO design. Raising as a suggestion for the Javadoc.

Minor: RelationshipService.findAllSpouseEdges() — no @Transactional(readOnly=true) annotation. The existing getRelationships(), getInferredRelationships(), etc. are also not annotated (correct per project style — reads don't need explicit transactions). Consistent with existing pattern. PASS.

Blocker: None.

Verdict: APPROVE — clean layering, correct Lombok/Spring patterns, @Schema applied correctly for the unified-DTO design. One suggestion on Javadoc for the asymmetric nullability of primaryPersonName in the unified DTO.

### Developer (Felix Brandt) — PR Code Review **Verdict: ⚠️ Approved with concerns** **Checklist:** | # | Item | Status | Note | |---|------|--------|------| | 1 | Layering rule — no cross-domain repository injection | PASS | `TimelineEventService` injects `RelationshipService` (a service), not `PersonRelationshipRepository`. Constitution §1.3 respected. | | 2 | `@Schema(requiredMode = REQUIRED)` on always-populated fields | PASS | `TimelineEntryDTO`: `id`, `type`, `precision`, `derived` all marked REQUIRED. `eventDate`, `derivedType`, `primaryPersonName`, `relatedPersonName` nullable/optional — correctly left unmarked. | | 3 | `npm run generate:api` | CONCERN | PR notes explicitly defer `generate:api` to issue #5: "no controller endpoint exposes `TimelineEntryDTO` yet." The spec task says "Commit the generated TypeScript types as part of this PR." The spec is clear this is a breaking OpenAPI change. However, since `TimelineEntryDTO` is not yet reachable from any controller endpoint (it's an internal service contract only), Springdoc won't include it in the OpenAPI spec until #5 wires the controller — so regenerating now would produce no diff. Deferral is acceptable **given this reasoning**, but issue #5 must regenerate before merge. | | 4 | Lombok/Spring patterns | PASS | `@Service`, `@RequiredArgsConstructor`, `@Slf4j` all present on `TimelineEventService`. `RelationshipService.findAllSpouseEdges()` is a plain public method, no annotation needed. | | 5 | `@Transactional` annotation discipline | PASS | `assembleDerivedEvents()` correctly uses `@Transactional(readOnly = true)`. The three `build*` private helpers are rightly unannotated. No `@Transactional` on the class. | | 6 | Function size and single responsibility | PASS | `assembleDerivedEvents()` is ~10 lines orchestrating three helpers. Each `build*` method is ~15 lines doing one job. Under the 20-line guideline. | | 7 | Guard clauses and null safety | PASS | Birth/death filtering via stream `.filter(p -> p.getBirthDate() != null)` is clean. Marriage loop uses `seen.add()` idiom correctly. No nested conditionals. | | 8 | Lazy-collection entities crossing controller boundary | PASS | No raw entities returned — `assembleDerivedEvents()` returns `List<TimelineEntryDTO>`. ADR-036 respected. | | 9 | No cross-domain repository injection | PASS | Confirmed: `TimelineEventService` imports `RelationshipService`, not `PersonRelationshipRepository`. Constitution §1.3 satisfied. | | 10 | Names reveal intent | PASS | `buildBirthEvents`, `buildDeathEvents`, `buildMarriageEvents`, `assembleDerivedEvents` — all self-documenting. | | 11 | Dead code / backwards-compat shims | PASS | No shims added. Constitution §3.4 satisfied. | | 12 | Records as DTOs | PASS | `TimelineEntryDTO` as a Java record is correct for an immutable DTO — modern Java 21 feature, aligns with `architect.md` guidance. | **Concern — `TimelineEntryDTO` missing `@Schema(requiredMode = REQUIRED)` on `primaryPersonName`:** - `primaryPersonName: String` is REQUIRED for all derived events (REQ-009 + REQ-010). In the DTO it is left as nullable (no `@Schema(REQUIRED)`). - This is architecturally correct for the unified DTO (curated events don't have `primaryPersonName`), but it means the OpenAPI spec will mark it as optional even though derived events always populate it. - The spec explicitly states this field must be non-null for derived events. The `@Schema` omission is acceptable since the DTO serves two use cases, but issue #5/#6 should document this asymmetry in the Javadoc or add a discriminated-union pattern note. - **Not a blocker** — the field is nullable in the record declaration, which is correct for the unified-DTO design. Raising as a suggestion for the Javadoc. **Minor: `RelationshipService.findAllSpouseEdges()` — no `@Transactional(readOnly=true)` annotation.** The existing `getRelationships()`, `getInferredRelationships()`, etc. are also not annotated (correct per project style — reads don't need explicit transactions). Consistent with existing pattern. PASS. **Blocker: None.** Verdict: **APPROVE** — clean layering, correct Lombok/Spring patterns, `@Schema` applied correctly for the unified-DTO design. One suggestion on Javadoc for the asymmetric nullability of `primaryPersonName` in the unified DTO.

marcel commented 2026-06-13 14:43:50 +02:00

Author

Owner

Copy Link

Tester (Sara Holt) — PR Code Review

Verdict: ✅ Approved

Checklist:

#	Item	Status	Note
1	Red/Green TDD evidence	PASS	16 tests cover all 16 REQ-NNN. Test structure matches the spec's TDD order. No evidence of tests written after implementation was green (pure Mockito unit tests — the service compiles only once the implementation exists, but the spec pre-defines all test names confirming the intention).
2	Test framework choice: correct level	PASS	@ExtendWith(MockitoExtension.class) — no Spring context, no Testcontainers. Correct for pure service logic. DB behavior (JOIN FETCH, unique constraint) is tested by trusting the existing repository query annotation and the spec's documented assumption test.
3	Descriptive test names	PASS	All names read as sentences: should_emit_one_geburt_for_person_with_birthdate, should_mint_prefixed_synthetic_ids_never_uuid, etc. Fail messages in CI will be self-explanatory.
4	Factory helpers	PASS	makePerson(), makePersonWithDeath(), makePersonWithBoth(), makeNonFamilyPerson(), makeSpouseEdge() — all present. Each test body is 5-15 lines with intent-revealing setup.
5	One logical assertion per test	MOSTLY PASS	Most tests assert one behavior. should_emit_one_geburt_for_person_with_birthdate asserts 5 fields on one event — this is the "structural invariants" pattern mandated by the spec (REQ-009: assert derived/type/derivedType on every test). Acceptable.
6	Strong assertions (not just non-null)	PASS	Precision asserted as equality (DatePrecision.DAY, DatePrecision.YEAR, DatePrecision.UNKNOWN), not just non-null. eventDate asserted as exact LocalDate.of(...). UUID.fromString() throw asserted.
7	Edge cases covered	PASS	Null dates, empty list, self-edge (assumption test), non-family-member exclusion, mixed-membership Heirat, two marriages.
8	REQ-012 test quality concern	CONCERN	should_exclude_non_family_member_persons_from_derived_events mocks findAllFamilyMembers() returning an empty list and asserts result is empty. This is correct — but it doesn't actually prove the familyMember=false filter works, because the service delegates filtering to PersonService.findAllFamilyMembers(). The test is testing the delegation contract, not the filter itself. The filter lives in PersonService (not in scope for this PR). The test name should clarify it's testing the delegation, e.g. should_emit_no_events_when_findAllFamilyMembers_returns_empty. This is a suggestion, not a blocker — the spec explicitly stated REQ-012 is "verified by delegation to PersonService.findAllFamilyMembers()".
9	REQ-011 verification	PASS	No test stubs personService.getById() per-person — only batch-fetch mocks are registered. Mockito's strict stubbing would fail any unexpected call.
10	Assumption/documentation test quality	PASS	self_spouse_edge_invariant_is_enforced_by_db_constraint has a clear comment explaining it's a documentation test, not a behavioral assertion. Comment explains the DB constraint is authoritative.
11	Missing edge cases?	MINOR GAP	No test for a Person where getDisplayName() returns an empty string or null. The spec says Person.getDisplayName() is "confirmed null-safe in existing code" — this is an accepted assumption. The spec also says to "add an assertion test that display names are non-null AND non-blank" — this IS present in should_emit_heirat_with_displayname_for_both_spouses for Heirat, but not explicitly for Geburt/Tod events in a dedicated test. It is implicitly covered by asserting event.primaryPersonName() equality to anna.getDisplayName() in should_emit_one_geburt_for_person_with_birthdate. Acceptable.
12	No @SpringBootTest misuse	PASS	Pure Mockito. Correct level.

One concern worth noting for issue #5: When the GET /api/timeline controller endpoint is wired in #5, it will need a @WebMvcTest slice test proving that READ_ALL is enforced before assembleDerivedEvents() is called. This PR deliberately does not add that test (no controller yet). Flag for #5 review.

Blocker: None.

Verdict: APPROVE — 16 well-named tests with strong assertions and correct factory helpers. Minor suggestion to clarify REQ-012 test name. The test suite would catch any regression in the assembly logic.

### Tester (Sara Holt) — PR Code Review **Verdict: ✅ Approved** **Checklist:** | # | Item | Status | Note | |---|------|--------|------| | 1 | Red/Green TDD evidence | PASS | 16 tests cover all 16 REQ-NNN. Test structure matches the spec's TDD order. No evidence of tests written after implementation was green (pure Mockito unit tests — the service compiles only once the implementation exists, but the spec pre-defines all test names confirming the intention). | | 2 | Test framework choice: correct level | PASS | `@ExtendWith(MockitoExtension.class)` — no Spring context, no Testcontainers. Correct for pure service logic. DB behavior (JOIN FETCH, unique constraint) is tested by trusting the existing repository query annotation and the spec's documented assumption test. | | 3 | Descriptive test names | PASS | All names read as sentences: `should_emit_one_geburt_for_person_with_birthdate`, `should_mint_prefixed_synthetic_ids_never_uuid`, etc. Fail messages in CI will be self-explanatory. | | 4 | Factory helpers | PASS | `makePerson()`, `makePersonWithDeath()`, `makePersonWithBoth()`, `makeNonFamilyPerson()`, `makeSpouseEdge()` — all present. Each test body is 5-15 lines with intent-revealing setup. | | 5 | One logical assertion per test | MOSTLY PASS | Most tests assert one behavior. `should_emit_one_geburt_for_person_with_birthdate` asserts 5 fields on one event — this is the "structural invariants" pattern mandated by the spec (REQ-009: assert derived/type/derivedType on every test). Acceptable. | | 6 | Strong assertions (not just non-null) | PASS | Precision asserted as equality (`DatePrecision.DAY`, `DatePrecision.YEAR`, `DatePrecision.UNKNOWN`), not just non-null. `eventDate` asserted as exact `LocalDate.of(...)`. `UUID.fromString()` throw asserted. | | 7 | Edge cases covered | PASS | Null dates, empty list, self-edge (assumption test), non-family-member exclusion, mixed-membership Heirat, two marriages. | | 8 | REQ-012 test quality concern | CONCERN | `should_exclude_non_family_member_persons_from_derived_events` mocks `findAllFamilyMembers()` returning an empty list and asserts result is empty. This is correct — but it doesn't actually prove the `familyMember=false` filter works, because the service delegates filtering to `PersonService.findAllFamilyMembers()`. The test is testing the delegation contract, not the filter itself. The filter lives in `PersonService` (not in scope for this PR). The test name should clarify it's testing the delegation, e.g. `should_emit_no_events_when_findAllFamilyMembers_returns_empty`. This is a suggestion, not a blocker — the spec explicitly stated REQ-012 is "verified by delegation to `PersonService.findAllFamilyMembers()`". | | 9 | REQ-011 verification | PASS | No test stubs `personService.getById()` per-person — only batch-fetch mocks are registered. Mockito's strict stubbing would fail any unexpected call. | | 10 | Assumption/documentation test quality | PASS | `self_spouse_edge_invariant_is_enforced_by_db_constraint` has a clear comment explaining it's a documentation test, not a behavioral assertion. Comment explains the DB constraint is authoritative. | | 11 | Missing edge cases? | MINOR GAP | No test for a `Person` where `getDisplayName()` returns an empty string or null. The spec says `Person.getDisplayName()` is "confirmed null-safe in existing code" — this is an accepted assumption. The spec also says to "add an assertion test that display names are non-null AND non-blank" — this IS present in `should_emit_heirat_with_displayname_for_both_spouses` for Heirat, but not explicitly for Geburt/Tod events in a dedicated test. It is implicitly covered by asserting `event.primaryPersonName()` equality to `anna.getDisplayName()` in `should_emit_one_geburt_for_person_with_birthdate`. Acceptable. | | 12 | No `@SpringBootTest` misuse | PASS | Pure Mockito. Correct level. | **One concern worth noting for issue #5:** When the `GET /api/timeline` controller endpoint is wired in #5, it will need a `@WebMvcTest` slice test proving that `READ_ALL` is enforced before `assembleDerivedEvents()` is called. This PR deliberately does not add that test (no controller yet). Flag for #5 review. **Blocker: None.** Verdict: **APPROVE** — 16 well-named tests with strong assertions and correct factory helpers. Minor suggestion to clarify REQ-012 test name. The test suite would catch any regression in the assembly logic.

marcel commented 2026-06-13 14:44:18 +02:00

Author

Owner

Copy Link

Security (Nora "NullX" Steiner) — PR Code Review

Verdict: ✅ Approved

This PR is a read-only backend assembly layer with no HTTP endpoints, no file handling, and no new auth boundaries. The STRIDE analysis in the issue spec is accurate. Reviewing against constitution §2 rules and the security checklist:

#	Item	Status	Note
1	All POST/PUT/PATCH/DELETE endpoints carry @RequirePermission	N/A	No new endpoints in this PR. No mutating endpoint introduced. Constitution §2.1 not applicable here. The READ_ALL gate is deferred to #5 — spec explicitly documents this.
2	Least-privilege permission correctly chosen	PASS	N/A for this PR. Issue spec correctly identifies READ_ALL as the right permission for #5's endpoint.
3	Audit fields set from session principal, not request body	N/A	No new entities, no new @Entity, no createdBy/updatedBy to set.
4	IDOR surfaces addressed	PASS	No IDOR surface: assembleDerivedEvents() returns aggregated data from the full family-member set, not a per-user filtered resource. No resource-by-id lookup that could be IDORed.
5	Untrusted text via default escaping, no {@html}	N/A	Backend-only PR. primaryPersonName/relatedPersonName from getDisplayName() are stored DB values, not rendered in this PR. Frontend rendering is #6/#7.
6	File upload: content-type, size, magic-byte validation	N/A	No file uploads.
7	No entity internals (email, hash) in responses	PASS	assembleDerivedEvents() returns List<TimelineEntryDTO> — only id, type, precision, derived, derivedType, primaryPersonName, relatedPersonName. No email, no password hash, no session data.
8	Concurrency conflicts surface as 409, not raw 500	N/A	No write path. No optimistic locking needed.
9	PII in logs	PASS	Confirmed: only log.debug("Assembled {} derived events for {} persons", result.size(), persons.size()). No person names, no dates, no UUIDs in log statements. Constitution §2.7 + REQ-016 satisfied.
10	Secrets from env vars only	PASS	No new secrets introduced.
11	New runtime dependency	N/A	No new dependencies.

Security observation — assembleDerivedEvents() is unauthenticated at the service layer (by design):

• The method has no auth check inside it. The Javadoc correctly states: "Callers outside the #5 endpoint must independently enforce READ_ALL authorization before invoking this method."
• This is the correct pattern for a service method in this architecture (auth is enforced at the controller layer via @RequirePermission). ADR-043 Decision 3 documents this obligation.
• Risk: If a future endpoint or batch job calls assembleDerivedEvents() without the READ_ALL guard, person names (PII) would be exposed. The Javadoc warning is the mitigation here. Constitution §2.8 requires an Unwanted-behavior requirement for the unauthenticated case — this is deferred to #5 as designed.
• Recommendation (not blocker): When #5 lands, add a @WebMvcTest test: GET /api/timeline as unauthenticated → 401, as READ_ALL-only → 200. This PR's tests cannot cover this because there is no controller yet.

CWE check:

• CWE-639 (IDOR): N/A
• CWE-200 (Information Disclosure): Low risk, mitigated by Javadoc + #5 gate
• CWE-532 (Sensitive Information in Logs): CLEAR — only counts in logs

Blocker: None.

Verdict: APPROVE — no mutating endpoints, no PII in logs, no entity internals in responses. Auth enforcement correctly deferred to #5 with documented obligation in Javadoc and ADR-043.

### Security (Nora "NullX" Steiner) — PR Code Review **Verdict: ✅ Approved** This PR is a read-only backend assembly layer with no HTTP endpoints, no file handling, and no new auth boundaries. The STRIDE analysis in the issue spec is accurate. Reviewing against constitution §2 rules and the security checklist: | # | Item | Status | Note | |---|------|--------|------| | 1 | All POST/PUT/PATCH/DELETE endpoints carry `@RequirePermission` | N/A | **No new endpoints in this PR.** No mutating endpoint introduced. Constitution §2.1 not applicable here. The `READ_ALL` gate is deferred to #5 — spec explicitly documents this. | | 2 | Least-privilege permission correctly chosen | PASS | N/A for this PR. Issue spec correctly identifies `READ_ALL` as the right permission for #5's endpoint. | | 3 | Audit fields set from session principal, not request body | N/A | No new entities, no new `@Entity`, no `createdBy`/`updatedBy` to set. | | 4 | IDOR surfaces addressed | PASS | No IDOR surface: `assembleDerivedEvents()` returns aggregated data from the full family-member set, not a per-user filtered resource. No resource-by-id lookup that could be IDORed. | | 5 | Untrusted text via default escaping, no `{@html}` | N/A | Backend-only PR. `primaryPersonName`/`relatedPersonName` from `getDisplayName()` are stored DB values, not rendered in this PR. Frontend rendering is #6/#7. | | 6 | File upload: content-type, size, magic-byte validation | N/A | No file uploads. | | 7 | No entity internals (email, hash) in responses | PASS | `assembleDerivedEvents()` returns `List<TimelineEntryDTO>` — only `id`, `type`, `precision`, `derived`, `derivedType`, `primaryPersonName`, `relatedPersonName`. No email, no password hash, no session data. | | 8 | Concurrency conflicts surface as 409, not raw 500 | N/A | No write path. No optimistic locking needed. | | 9 | PII in logs | **PASS** | Confirmed: only `log.debug("Assembled {} derived events for {} persons", result.size(), persons.size())`. No person names, no dates, no UUIDs in log statements. Constitution §2.7 + REQ-016 satisfied. | | 10 | Secrets from env vars only | PASS | No new secrets introduced. | | 11 | New runtime dependency | N/A | No new dependencies. | **Security observation — `assembleDerivedEvents()` is unauthenticated at the service layer (by design):** - The method has no auth check inside it. The Javadoc correctly states: "Callers outside the #5 endpoint must independently enforce READ_ALL authorization before invoking this method." - This is the correct pattern for a service method in this architecture (auth is enforced at the controller layer via `@RequirePermission`). ADR-043 Decision 3 documents this obligation. - **Risk:** If a future endpoint or batch job calls `assembleDerivedEvents()` without the `READ_ALL` guard, person names (PII) would be exposed. The Javadoc warning is the mitigation here. Constitution §2.8 requires an Unwanted-behavior requirement for the unauthenticated case — this is deferred to #5 as designed. - **Recommendation (not blocker):** When #5 lands, add a `@WebMvcTest` test: `GET /api/timeline` as unauthenticated → 401, as READ_ALL-only → 200. This PR's tests cannot cover this because there is no controller yet. **CWE check:** - CWE-639 (IDOR): N/A - CWE-200 (Information Disclosure): Low risk, mitigated by Javadoc + #5 gate - CWE-532 (Sensitive Information in Logs): CLEAR — only counts in logs **Blocker: None.** Verdict: **APPROVE** — no mutating endpoints, no PII in logs, no entity internals in responses. Auth enforcement correctly deferred to #5 with documented obligation in Javadoc and ADR-043.

marcel commented 2026-06-13 14:44:35 +02:00

Author

Owner

Copy Link

DevOps (Tobias Wendt) — PR Code Review

Verdict: ✅ Approved

#	Item	Status	Note
1	Flyway migration rollback strategy	N/A	No Flyway migration in this PR. Derived events are computed on read, never persisted. ADR-043 Decision 1 explicitly states: "No schema changes. No Flyway migration." PASS.
2	Flyway migration number verified on disk	N/A	No migration.
3	New env vars documented in .env.example	N/A	No new env vars.
4	CI steps avoid upload-artifact@v4+	N/A	No CI workflow changes in this PR.
5	New CI guard is self-testing	N/A	No new CI guards.
6	Management port / app port separation intact	PASS	No port changes.
7	Dependencies pinned, npm audit green	PASS	No new dependencies added.
8	No new external services or sidecars	PASS	No Docker Compose changes.
9	No new observability channels	PASS	Logging uses existing SLF4J @Slf4j with parameterized log.debug(...). No new log pipeline changes.
10	PII-free, structured logging	PASS	Only result.size() and persons.size() — no names, no dates. Consistent with existing log pipeline.
11	Backwards-compatible across rolling deploy	PASS	No schema changes. Service method addition is additive and non-breaking.
12	No secrets committed	PASS	No credentials anywhere in the diff.

ADR-043 number verified: The ADR is written to docs/adr/043-derived-person-events.md. The spec required verifying the number 043 against disk before use. The existing ADRs in this project run through 042-sdd-adoption.md (referenced in constitution). ADR-043 is the correct next free number. PASS.

One operational note for future: ADR-043 notes that caching should be considered if findAllFamilyMembers() exceeds ~500 rows. At MVP scale this is fine. When/if the timeline endpoint (#5) is called frequently, a short TTL cache on assembleDerivedEvents() or on the two backing queries may be warranted. File a follow-up issue at that point.

Blocker: None.

Verdict: APPROVE — no infrastructure changes, no migration, no new dependencies, no new env vars, PII-free logging. Clean DevOps footprint.

### DevOps (Tobias Wendt) — PR Code Review **Verdict: ✅ Approved** | # | Item | Status | Note | |---|------|--------|------| | 1 | Flyway migration rollback strategy | N/A | **No Flyway migration in this PR.** Derived events are computed on read, never persisted. ADR-043 Decision 1 explicitly states: "No schema changes. No Flyway migration." PASS. | | 2 | Flyway migration number verified on disk | N/A | No migration. | | 3 | New env vars documented in `.env.example` | N/A | No new env vars. | | 4 | CI steps avoid `upload-artifact@v4+` | N/A | No CI workflow changes in this PR. | | 5 | New CI guard is self-testing | N/A | No new CI guards. | | 6 | Management port / app port separation intact | PASS | No port changes. | | 7 | Dependencies pinned, `npm audit` green | PASS | No new dependencies added. | | 8 | No new external services or sidecars | PASS | No Docker Compose changes. | | 9 | No new observability channels | PASS | Logging uses existing SLF4J `@Slf4j` with parameterized `log.debug(...)`. No new log pipeline changes. | | 10 | PII-free, structured logging | PASS | Only `result.size()` and `persons.size()` — no names, no dates. Consistent with existing log pipeline. | | 11 | Backwards-compatible across rolling deploy | PASS | No schema changes. Service method addition is additive and non-breaking. | | 12 | No secrets committed | PASS | No credentials anywhere in the diff. | **ADR-043 number verified:** The ADR is written to `docs/adr/043-derived-person-events.md`. The spec required verifying the number `043` against disk before use. The existing ADRs in this project run through `042-sdd-adoption.md` (referenced in constitution). ADR-043 is the correct next free number. PASS. **One operational note for future:** ADR-043 notes that caching should be considered if `findAllFamilyMembers()` exceeds ~500 rows. At MVP scale this is fine. When/if the timeline endpoint (#5) is called frequently, a short TTL cache on `assembleDerivedEvents()` or on the two backing queries may be warranted. File a follow-up issue at that point. **Blocker: None.** Verdict: **APPROVE** — no infrastructure changes, no migration, no new dependencies, no new env vars, PII-free logging. Clean DevOps footprint.

marcel commented 2026-06-13 14:44:58 +02:00

Author

Owner

Copy Link

UI/UX (Leonie Voss) — PR Code Review

Verdict: ✅ Approved

This is a backend-only PR. No Svelte components, no i18n message files, no routes. My review is scoped to the two UI/UX concerns that touch this PR:

#	Item	Status	Note
1	Hard-coded German/English literals in backend output	PASS	TimelineEntryDTO carries raw data fields (primaryPersonName, relatedPersonName, derivedType, precision, eventDate) — no pre-composed labels. The backend does NOT return "Geburt von Anna Müller" — it returns the discriminator (DerivedEventType.BIRTH) and the name separately. Frontend (#6/#7) owns label composition via Paraglide i18n. Correct.
2	i18n key convention locked correctly	PASS	Issue #776 locks the key convention: `timeline.derived.{birth
3	Null-date marriage handling deferred properly	PASS	The spec explicitly calls out: "The null-date marriage case (precision=UNKNOWN, eventDate=null) needs a companion display key (e.g. timeline.derived.marriage_no_date) or 'Ohne Datum' bucket label — spec this in #6, do not discover it during implementation." This PR correctly does NOT attempt to resolve this display question. The UNKNOWN precision is surfaced (per REQ-006) so #6/#7 will have the data to implement the null-date bucket.
4	Separate name fields (not pre-concatenated) for Heirat	PASS	primaryPersonName and relatedPersonName are separate fields on TimelineEntryDTO. The frontend (#6/#7) owns the " & " separator and localized formatting. This gives the UI full control over locale-appropriate name ordering and punctuation.
5	derivedType enum values match the i18n key convention	PASS	DerivedEventType.BIRTH/DEATH/MARRIAGE → frontend maps to timeline.derived.birth/death/marriage keys. Simple, consistent, no mismatch.

Observation: The relatedPersonName field is nullable (null for Geburt/Tod, populated for Heirat). The frontend template in #6/#7 should handle the null case explicitly — {relatedPersonName ?? ''} or conditional rendering. This is a reminder for the #6/#7 PR review, not an issue with this PR.

Blocker: None.

Verdict: APPROVE — backend-only PR. i18n key convention locked cleanly, null-date marriage deferred correctly to #6, name fields kept separate for frontend control. Everything UI/UX cares about for this layer is in the right shape.

### UI/UX (Leonie Voss) — PR Code Review **Verdict: ✅ Approved** This is a backend-only PR. No Svelte components, no i18n message files, no routes. My review is scoped to the two UI/UX concerns that touch this PR: | # | Item | Status | Note | |---|------|--------|------| | 1 | Hard-coded German/English literals in backend output | PASS | `TimelineEntryDTO` carries raw data fields (`primaryPersonName`, `relatedPersonName`, `derivedType`, `precision`, `eventDate`) — no pre-composed labels. The backend does NOT return "Geburt von Anna Müller" — it returns the discriminator (`DerivedEventType.BIRTH`) and the name separately. Frontend (#6/#7) owns label composition via Paraglide i18n. Correct. | | 2 | i18n key convention locked correctly | PASS | Issue #776 locks the key convention: `timeline.derived.{birth|death|marriage}` with slots `{name}` for birth/death and `{nameA}` + `{nameB}` for marriage. This convention is referenced in ADR-043 and the PR description. The message file entries are created in #6 as designed. Key convention is non-breaking and implementable. | | 3 | Null-date marriage handling deferred properly | PASS | The spec explicitly calls out: "The null-date marriage case (`precision=UNKNOWN`, `eventDate=null`) needs a companion display key (e.g. `timeline.derived.marriage_no_date`) or 'Ohne Datum' bucket label — spec this in #6, do not discover it during implementation." This PR correctly does NOT attempt to resolve this display question. The `UNKNOWN` precision is surfaced (per REQ-006) so #6/#7 will have the data to implement the null-date bucket. | | 4 | Separate name fields (not pre-concatenated) for Heirat | PASS | `primaryPersonName` and `relatedPersonName` are separate fields on `TimelineEntryDTO`. The frontend (#6/#7) owns the " & " separator and localized formatting. This gives the UI full control over locale-appropriate name ordering and punctuation. | | 5 | `derivedType` enum values match the i18n key convention | PASS | `DerivedEventType.BIRTH`/`DEATH`/`MARRIAGE` → frontend maps to `timeline.derived.birth`/`death`/`marriage` keys. Simple, consistent, no mismatch. | **Observation:** The `relatedPersonName` field is nullable (null for Geburt/Tod, populated for Heirat). The frontend template in #6/#7 should handle the null case explicitly — `{relatedPersonName ?? ''}` or conditional rendering. This is a reminder for the #6/#7 PR review, not an issue with this PR. **Blocker: None.** Verdict: **APPROVE** — backend-only PR. i18n key convention locked cleanly, null-date marriage deferred correctly to #6, name fields kept separate for frontend control. Everything UI/UX cares about for this layer is in the right shape.

marcel commented 2026-06-13 14:45:32 +02:00

Author

Owner

Copy Link

Architect (Markus Keller) — PR Code Review

Verdict: ⚠️ Approved with concerns

#	Item	Status	Note
1	Package-by-domain structure respected	PASS	DerivedEventType, TimelineEntryDTO live in org.raddatz.familienarchiv.timeline. RelationshipService.findAllSpouseEdges() stays in org.raddatz.familienarchiv.person.relationship. No code smeared across layer packages.
2	Layering rule honoured — no cross-domain repository injection	PASS	TimelineEventService injects RelationshipService (its service interface), not PersonRelationshipRepository. Constitution §1.3 explicitly satisfied. The diff shows PersonRelationship entity is used as a return type — this is the DTO-like role of the entity within the relationship sub-domain, which is acceptable since it crosses via RelationshipService.
3	assembleDerivedEvents() contract correct	PASS	Public method on TimelineEventService, returns List<TimelineEntryDTO>, annotated @Transactional(readOnly=true). Javadoc documents auth obligation for callers. Three private builder methods are correctly private.
4	ADR-043 covers the right decisions	PASS	Decision 1 (on-read vs persisted), Decision 2 (synthetic String id format), Decision 3 (public service contract + domain boundary). All three irreversible decisions documented. Alternatives table present for each. Status: Proposed (correct — not yet Accepted, as it should be ratified after implementation review).
5	ADR number correct	PASS	ADR-043 is the next free number (ADR-042 is the SDD adoption ADR per the constitution header). Verified.
6	No existing Accepted ADR contradicted	PASS	ADR-036 (view-not-entity): respected — TimelineEntryDTO record, not raw entity. ADR-039 (Person dates as LocalDate + DatePrecision): used correctly. ADR-040 (timeline domain): extended correctly.
7	docs/architecture/c4/l3-backend-*.puml updated	CONCERN — requires investigation	The developer persona checklist requires updating the C4 L3 backend diagram when "a new Spring Boot service method is added to an existing domain." TimelineEventService gained assembleDerivedEvents() as a public method and now has a new dependency on RelationshipService. The PR updates CLAUDE.md (done, verified in diff) and the RTM (done). The C4 L3 diagram update is not visible in the diff.
8	docs/architecture/db/ diagrams	N/A	No schema changes. No new entity or relationship added. No diagram update needed.
9	docs/GLOSSARY.md updated	CONCERN	Issue #776 Tasks list: "Add entries for DerivedEventType, derivedType, assembleDerivedEvents, and 'derived event' as a concept." This is NOT visible in the PR diff. The issue spec explicitly listed this as a task.
10	Domain boundaries — blast radius bounded	PASS	Changes touch timeline/ (new types + new methods on existing service) and person/relationship/ (one new public method). No other domain packages touched. Blast radius is minimal and justified.
11	No premature abstraction	PASS	Three private helpers + one public orchestrator — textbook KISS decomposition. No abstract base class, no generic assembly framework.
12	N+1 risk documented	PASS	ADR-043 Decision 3 and the inline comment in buildMarriageEvents() both document the JOIN FETCH guarantee.

Concern 1 — GLOSSARY.md not updated (potential blocker per architecture checklist):
Per the architect's review table: "New domain concept or term → docs/GLOSSARY.md." The diff does not include GLOSSARY.md updates. The spec's task list includes: "Add entries for DerivedEventType, derivedType, assembleDerivedEvents, and 'derived event' as a concept." This is a blocker per the architecture review rule ("a doc omission is a blocker, not a concern") — unless docs/GLOSSARY.md was already updated in a prior commit not visible in the diff, or the file doesn't exist yet.

Concern 2 — C4 L3 backend diagram not updated:
TimelineEventService now has a new public method and a new injected dependency (RelationshipService). The matching docs/architecture/c4/l3-backend-*.puml diagram for the timeline domain should show this new method and the new service-to-service dependency arrow. If this diagram file exists and was not updated, it's a blocker per the architect's checklist.

Action required: Verify whether docs/GLOSSARY.md and docs/architecture/c4/l3-backend-*.puml exist. If they do and are missing the new terms/arrows, those updates must be added before merge. If the GLOSSARY doesn't exist yet (possible for a newer project), document that decision.

Blocker: Conditional — if docs/GLOSSARY.md exists and docs/architecture/c4/l3-backend-*.puml exists, both must be updated.

Verdict: APPROVE WITH CONCERNS — core architectural decisions are sound, domain boundaries clean, ADR-043 thorough. Pending verification of GLOSSARY.md and C4 L3 diagram updates.

### Architect (Markus Keller) — PR Code Review **Verdict: ⚠️ Approved with concerns** | # | Item | Status | Note | |---|------|--------|------| | 1 | Package-by-domain structure respected | PASS | `DerivedEventType`, `TimelineEntryDTO` live in `org.raddatz.familienarchiv.timeline`. `RelationshipService.findAllSpouseEdges()` stays in `org.raddatz.familienarchiv.person.relationship`. No code smeared across layer packages. | | 2 | Layering rule honoured — no cross-domain repository injection | PASS | `TimelineEventService` injects `RelationshipService` (its service interface), not `PersonRelationshipRepository`. Constitution §1.3 explicitly satisfied. The diff shows `PersonRelationship` entity is used as a return type — this is the DTO-like role of the entity within the relationship sub-domain, which is acceptable since it crosses via `RelationshipService`. | | 3 | `assembleDerivedEvents()` contract correct | PASS | Public method on `TimelineEventService`, returns `List<TimelineEntryDTO>`, annotated `@Transactional(readOnly=true)`. Javadoc documents auth obligation for callers. Three private builder methods are correctly private. | | 4 | ADR-043 covers the right decisions | PASS | Decision 1 (on-read vs persisted), Decision 2 (synthetic String id format), Decision 3 (public service contract + domain boundary). All three irreversible decisions documented. Alternatives table present for each. Status: Proposed (correct — not yet Accepted, as it should be ratified after implementation review). | | 5 | ADR number correct | PASS | ADR-043 is the next free number (ADR-042 is the SDD adoption ADR per the constitution header). Verified. | | 6 | No existing Accepted ADR contradicted | PASS | ADR-036 (view-not-entity): respected — `TimelineEntryDTO` record, not raw entity. ADR-039 (Person dates as LocalDate + DatePrecision): used correctly. ADR-040 (timeline domain): extended correctly. | | 7 | `docs/architecture/c4/l3-backend-*.puml` updated | **CONCERN — requires investigation** | The developer persona checklist requires updating the C4 L3 backend diagram when "a new Spring Boot service method is added to an existing domain." `TimelineEventService` gained `assembleDerivedEvents()` as a public method and now has a new dependency on `RelationshipService`. The PR updates `CLAUDE.md` (done, verified in diff) and the RTM (done). The C4 L3 diagram update is **not visible in the diff**. | | 8 | `docs/architecture/db/` diagrams | N/A | No schema changes. No new entity or relationship added. No diagram update needed. | | 9 | `docs/GLOSSARY.md` updated | **CONCERN** | Issue #776 Tasks list: "Add entries for `DerivedEventType`, `derivedType`, `assembleDerivedEvents`, and 'derived event' as a concept." This is NOT visible in the PR diff. The issue spec explicitly listed this as a task. | | 10 | Domain boundaries — blast radius bounded | PASS | Changes touch `timeline/` (new types + new methods on existing service) and `person/relationship/` (one new public method). No other domain packages touched. Blast radius is minimal and justified. | | 11 | No premature abstraction | PASS | Three private helpers + one public orchestrator — textbook KISS decomposition. No abstract base class, no generic assembly framework. | | 12 | N+1 risk documented | PASS | ADR-043 Decision 3 and the inline comment in `buildMarriageEvents()` both document the JOIN FETCH guarantee. | **Concern 1 — GLOSSARY.md not updated (potential blocker per architecture checklist):** Per the architect's review table: "New domain concept or term → `docs/GLOSSARY.md`." The diff does not include GLOSSARY.md updates. The spec's task list includes: "Add entries for `DerivedEventType`, `derivedType`, `assembleDerivedEvents`, and 'derived event' as a concept." This is a **blocker per the architecture review rule** ("a doc omission is a blocker, not a concern") — unless `docs/GLOSSARY.md` was already updated in a prior commit not visible in the diff, or the file doesn't exist yet. **Concern 2 — C4 L3 backend diagram not updated:** `TimelineEventService` now has a new public method and a new injected dependency (`RelationshipService`). The matching `docs/architecture/c4/l3-backend-*.puml` diagram for the timeline domain should show this new method and the new service-to-service dependency arrow. If this diagram file exists and was not updated, it's a blocker per the architect's checklist. **Action required:** Verify whether `docs/GLOSSARY.md` and `docs/architecture/c4/l3-backend-*.puml` exist. If they do and are missing the new terms/arrows, those updates must be added before merge. If the GLOSSARY doesn't exist yet (possible for a newer project), document that decision. **Blocker: Conditional** — if `docs/GLOSSARY.md` exists and `docs/architecture/c4/l3-backend-*.puml` exists, both must be updated. Verdict: **APPROVE WITH CONCERNS** — core architectural decisions are sound, domain boundaries clean, ADR-043 thorough. Pending verification of GLOSSARY.md and C4 L3 diagram updates.

marcel added 1 commit 2026-06-13 14:54:14 +02:00

docs(timeline): add derived-event glossary entries and update C4 diagram ... 6b593a7bc6

Add GLOSSARY.md entries for derived event, DerivedEventType, derivedType,
and assembleDerivedEvents() to cover the vocabulary introduced by #776.
Update l3-backend-timeline.puml: remove stale "planned, #775" labels,
add Rel from TimelineEventService to personDomain for assembleDerivedEvents
batch-fetch calls, document the on-read strategy in the component notes.

Refs #776
Co-Authored-By: claude-sonnet-4-6 <noreply@anthropic.com>

marcel merged commit 6b593a7bc6 into main 2026-06-13 15:13:29 +02:00

marcel deleted branch worktree-feat-issue-776-derived-person-life-events 2026-06-13 15:13:29 +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 renovate_bot marcel

No Assignees

1 Participants

Notifications

Subscribe

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#825