Timeline: derive person life-events (Geburt/Tod/Heirat) #776

New Issue

marcel · 2026-06-07T19:28:58+02:00

marcel commented

2026-06-07 19:28:58 +02:00

Milestone: Zeitstrahl — Family Timeline
Spec: docs/superpowers/specs/2026-06-07-family-timeline-design.md § "Derived person-events"
Depends on:

#1 — foundational Person date+precision migration (replaces Person.birthYear/deathYear with Person.birthDate/birthDatePrecision + deathDate/deathDatePrecision). Confirmed: Person.java still has private Integer birthYear; private Integer deathYear; — this issue is un-startable until #1 lands.
#2 / #3 — the TimelineEvent entity and the curated-event DTO (TimelineEntryDTO) this issue reuses. The timeline/ package does not exist yet; #2 owns its creation, the package/DB-diagram/CLAUDE.md doc updates, and the DTO shape. This issue must NOT carry that doc work. Note: this issue (#4) adds the derivedType field to TimelineEntryDTO — see Tasks below.

Branch dependency: cut the feature branch for this issue from #1's merged state (or rebase onto it). Until Person.birthDate / Person.birthDatePrecision exist, this issue's code will not compile. The red-phase tests literally cannot compile until #1 is merged — add a comment in the test class stating this so the developer doesn't waste time debugging.

Context

Births, deaths, and marriages are surfaced automatically from already-curated Person + relationship data — no manual entry, no auto-extraction from transcriptions. They are computed on read, never stored. This issue delivers the in-process assembly layer only (the GET /api/timeline endpoint, ordering, and bucketing are issue #5). The output of this layer is an unordered set of derived events; #5 owns sort/bucket/year-band ordering.

Scope of derived events: derived events are assembled for family-member persons only — persons with family_member = TRUE (i.e., those returned by PersonService.findAllFamilyMembers()). Correspondents, provisional persons, and historical figures not yet added to the family tree are excluded from this assembly even if they have a birthDate. This matches the "curated family graph" MVP principle.

Scope

Assemble derived events from migrated Person + relationship data, in the same DTO shape as curated events, flagged derived: true, type = PERSONAL:

Source	Derived event	`eventDate`	precision	name source
`Person.birthDate`	Geburt	`Person.birthDate`	`Person.birthDatePrecision` (passed through unchanged)	`Person.getDisplayName()` → `primaryPersonName`
`Person.deathDate`	Tod	`Person.deathDate`	`Person.deathDatePrecision` (passed through unchanged)	`Person.getDisplayName()` → `primaryPersonName`
`SPOUSE_OF` edge, `fromYear` present	Heirat	`{fromYear}-01-01`	`YEAR`	both spouses' `getDisplayName()` → `primaryPersonName` + `relatedPersonName`
`SPOUSE_OF` edge, `fromYear` null	Heirat	`null`	`UNKNOWN`	both spouses' `getDisplayName()` → `primaryPersonName` + `relatedPersonName`

Architecture & layering

TimelineService owns TimelineEventRepository (from #2) and reaches Person/relationship data only through PersonService / RelationshipService — never inject PersonRelationshipRepository or PersonRepository into the timeline domain.
Batch-friendly fetch, no N+1. Assemble from set-returning queries, not a per-person fan-out. Any implementation that calls personService.getById() per person is a defect:
- Persons → PersonService.findAllFamilyMembers() (one call, family members with family_member = TRUE only).
- Spouse edges → RelationshipService.findAllSpouseEdges() (new minimal method — see below; one call returning List<PersonRelationship> with JOIN FETCH on both person sides).
New RelationshipService method: add findAllSpouseEdges() → List<PersonRelationship> wrapping relationshipRepository.findAllByRelationTypeIn(List.of(SPOUSE_OF)) with JOIN FETCH on both person sides. TimelineService calls this; it never touches PersonRelationshipRepository directly. Do NOT call getFamilyNetwork() (that returns a NetworkDTO shaped for Stammbaum, not for timeline assembly).
Assembly decomposition: implement three private methods, each independently testable:
```
private List<TimelineEntryDTO> buildBirthEvents(List<Person> persons) { ... }
private List<TimelineEntryDTO> buildDeathEvents(List<Person> persons) { ... }
private List<TimelineEntryDTO> buildMarriageEvents(List<PersonRelationship> spouseEdges) { ... }
```
A single assembleDerivedEvents() orchestrator calls all three and combines results. Add a Javadoc comment on this method: "Derived events are computed, never persisted, and cannot be mutated via the events API (enforced in #5)."
RelationshipDTO (carries id, relationType, fromYear, both person ids + display names) is the correct wire type returned by existing RelationshipService methods. PersonRelationship entity is used in the new findAllSpouseEdges() — the entity carries the Person references needed for getDisplayName().

`TimelineEntryDTO` contract

The DTO is defined in #2/#3 but this issue (#4) adds derivedType to it. Derived events need a discriminator (BIRTH/DEATH/MARRIAGE) so the frontend can compose localized labels; curated events have a title instead. Adding derivedType to the shared DTO is a breaking OpenAPI change — see Tasks.

Field requirements on TimelineEntryDTO enforced by this issue:

id: String (NOT UUID) — synthetic prefixed ids like birth:{uuid} are non-UUID by construction; if typed UUID they will fail to parse.
derivedType: DerivedEventType (nullable; null for curated events) — enum: BIRTH, DEATH, MARRIAGE.
primaryPersonName: String — display name of the subject (Geburt/Tod) or first spouse (Heirat).
relatedPersonName: String (nullable; null for Geburt/Tod, populated for Heirat) — display name of second spouse. Not pre-concatenated — the frontend (#6/#7) owns the " & " separator and locale-specific formatting. The i18n key timeline.derived.marriage({nameA}, {nameB}) needs two interpolation slots.
derived: boolean, type: EventType (always PERSONAL for derived events).

Person.getDisplayName() is confirmed null-safe in existing code, but add an assertion test that display names are non-null on emitted events.

Synthetic ids

Derived events have no persisted UUID. Mint prefixed synthetic ids:

birth:{personId} (UUID of the Person)
death:{personId} (UUID of the Person)
marriage:{relationshipId} (UUID of the PersonRelationship row)

These are non-UUID by construction and MUST never collide with a real TimelineEvent UUID. The id field on TimelineEntryDTO MUST be String, not UUID.

Symmetric-marriage dedup

SPOUSE_OF is stored once per couple (enforced by the unique_spouse_pair DB index using LEAST/GREATEST — V55). The in-memory dedup on Set<UUID> of relationship ids is a correctness assertion, not the primary defense. Add a comment in the service explaining this: "DB constraint unique_spouse_pair is the authoritative enforcement; in-memory dedup is a defensive assertion."

Logging

Use parameterized SLF4J (@Slf4j): logger.debug("Assembled {} derived events for {} persons", events.size(), persons.size()). Never string concatenation. No PII (person names, dates) in logs at INFO or above.

Resolved decisions

Null-fromYear marriage → emit, do not drop. A SPOUSE_OF edge can be saved with fromYear == null. Resolution: emit the Heirat with eventDate = null and precision = UNKNOWN so it lands in the "Ohne Datum" bucket. Rationale: dropping it hides a real marriage and violates the spec's "no fabricated dates / honest about precision" principle.
Label localization → derivedType discriminator, not a baked German title. The DTO exposes derivedType enum (BIRTH/DEATH/MARRIAGE) plus the person name(s); the frontend (#6/#7) composes the localized, precision-aware label from an i18n key. Rationale: de/en/es product; a server-frozen German string would force German labels on all locales.
DatePrecision stays in document/ for this issue. Do not move it as part of this issue — inherit whatever foundational issue #1 decides. If #1 relocates it to a shared package, this issue simply imports the new path.
Synthetic derived-event ids. Mint prefixed synthetic ids — birth:{personId}, death:{personId}, marriage:{relationshipId}. Non-UUID by construction and MUST never collide with a real TimelineEvent UUID.
Symmetric-marriage dedup key = relationship row id. Dedup on the relationship row id (the unique_spouse_pair DB index is the authoritative enforcement; in-memory Set<UUID> is a defensive assertion).
Derived-event person scope: family members only (family_member = TRUE). PersonService.findAllFamilyMembers() is the correct query. Correspondents and provisional persons not yet in the family tree are excluded. Every acceptance criterion that says "a person" means "a family-member person." Rationale: MVP scope matches the curated family graph principle; broadening to all persons is a future enhancement.
derivedType is added to TimelineEntryDTO by this issue (#4). It was always part of the timeline design but not yet in #2/#3's DTO definition. Adding it here is a breaking OpenAPI change — see Tasks. Rationale: the discriminator is first needed here; delaying it would block localized label rendering in #6/#7.
TimelineEntryDTO.id is typed String, not UUID. Synthetic prefixed ids (birth:{uuid}) cannot parse as UUID — the DTO field must be String. Enforce this when #2 defines the DTO; confirm before implementation starts.
Two separate name fields on TimelineEntryDTO for Heirat. primaryPersonName: String and relatedPersonName: String (nullable) — not pre-concatenated. The frontend owns the " & " separator and i18n formatting.
RelationshipService.findAllSpouseEdges() is the correct service boundary. A new minimal method wrapping findAllByRelationTypeIn(List.of(SPOUSE_OF)) with JOIN FETCH. TimelineService never touches PersonRelationshipRepository or getFamilyNetwork() directly.
Assembly decomposed into three private methods (buildBirthEvents, buildDeathEvents, buildMarriageEvents) + assembleDerivedEvents() orchestrator. Each is independently testable.

Tasks

Pre-condition check: confirm TimelineEntryDTO.id is String (not UUID) in #2's branch before writing any code — if it is UUID, block on that fix first.
Add derivedType: DerivedEventType (enum BIRTH/DEATH/MARRIAGE), primaryPersonName: String, and relatedPersonName: String (nullable) to TimelineEntryDTO in the timeline/ package.
Run npm run generate:api after adding derivedType and the name fields to TimelineEntryDTO — this is a breaking OpenAPI change.
Add RelationshipService.findAllSpouseEdges() → List<PersonRelationship> wrapping relationshipRepository.findAllByRelationTypeIn(List.of(SPOUSE_OF)) with JOIN FETCH on both person sides.
Implement buildBirthEvents(List<Person>), buildDeathEvents(List<Person>), buildMarriageEvents(List<PersonRelationship>) private methods in TimelineService.
Implement assembleDerivedEvents() orchestrator calling all three. Add Javadoc: "Derived events are computed, never persisted, and cannot be mutated via the events API (enforced in #5)."
Geburt from Person.birthDate + birthDatePrecision (1:1, no dedup). Tod from Person.deathDate + deathDatePrecision (1:1, no dedup). Precision passed through unchanged.
Heirat from SPOUSE_OF edges: present fromYear → {fromYear}-01-01 @ YEAR; null fromYear → eventDate = null @ UNKNOWN (emitted, not dropped).
Marriage derived exactly once per SPOUSE_OF edge; dedup on Set<UUID> of relationship row ids; add comment explaining DB constraint is authoritative.
Mint synthetic prefixed ids: birth:{personId}, death:{personId}, marriage:{relationshipId} — never UUIDs.
Pull Person/relationship data via their services only (findAllFamilyMembers() + findAllSpouseEdges()) — no per-person N+1.
@Slf4j with parameterized logging; no PII at INFO+.

Acceptance criteria

All criteria apply to family-member persons (those with family_member = TRUE):

A family-member person with a birthDate yields exactly one Geburt event at the person's birthDatePrecision.
A family-member person with a deathDate but no birthDate yields exactly one Tod and zero Geburt events.
A family-member person with a birthDate but no deathDate yields exactly one Geburt and zero Tod.
A family-member person with neither birthDate nor deathDate yields zero derived events.
A married couple (one SPOUSE_OF edge) yields exactly one Heirat event, even when both spouses are in the queried scope.
A person married to two different partners (two SPOUSE_OF rows) yields two distinct Heirat events.
A SPOUSE_OF edge with null fromYear yields one Heirat with eventDate = null, precision = UNKNOWN (it is surfaced, not suppressed).
A DAY-precision birthDate arrives on the event as DAY (not collapsed to YEAR); Heirat is YEAR (or UNKNOWN when fromYear null).
Every derived event carries derived: true, type = PERSONAL, a non-null derivedType, and a synthetic non-UUID String id — produced without a persisted id and excluded from any update/delete path.
Every derived event has a non-null primaryPersonName; Heirat events also have a non-null relatedPersonName.
A person with family_member = FALSE and a birthDate yields zero derived events (out of scope).

Carried to #5 (read endpoint): the derived-event id namespace is read-only by construction. The events controller in #5 must reject any non-UUID / prefixed id on PUT/DELETE (PUT /api/timeline/events/birth:{...} → 400, never a path that mutates a Person record). The "not editable via the events API" guarantee is enforced and tested in #5, not here. #5 also owns ordering, bucketing, READ_ALL gate, and the PUT /api/timeline/events/birth:{...} → 400 security test.

Tests

Pure unit tests with @ExtendWith(MockitoExtension.class) against mocked PersonService + RelationshipService — no Spring context, no Testcontainers (that cost belongs to #2's migration tests).

Factory helpers:

private Person makePerson(LocalDate birthDate, DatePrecision precision) {
    return Person.builder()
        .id(UUID.randomUUID())
        .firstName("Anna")
        .lastName("Müller")
        .familyMember(true)
        .birthDate(birthDate)
        .birthDatePrecision(precision)
        .build();
}

private PersonRelationship makeSpouseEdge(Person a, Person b, Integer fromYear) {
    return PersonRelationship.builder()
        .id(UUID.randomUUID())
        .person(a)
        .relatedPerson(b)
        .relationType(RelationType.SPOUSE_OF)
        .fromYear(fromYear)
        .build();
}

Note: Person.birthDate/birthDatePrecision/familyMember fields don't exist until #1 merges — the factory helpers (and thus all tests) will not compile until then. Add a class-level comment: // NOTE: tests require #1 (Person date migration) to be merged before this class compiles.

In every event-assertion test also assert structural invariants: assertThat(event.derived()).isTrue(), assertThat(event.type()).isEqualTo(EventType.PERSONAL), assertThat(event.derivedType()).isEqualTo(DerivedEventType.BIRTH) (or DEATH/MARRIAGE as appropriate). These are not separate behaviors — assert them within each test.

For precision tests use: assertThat(event.precision()).isEqualTo(DatePrecision.DAY) (not just non-null).

TDD order:

should_emit_one_geburt_for_person_with_birthdate
should_emit_zero_tod_when_person_has_birthdate_but_no_deathdate
should_emit_no_events_for_person_with_neither_date
should_emit_one_tod_for_person_with_deathdate
should_emit_one_tod_and_zero_geburt_for_person_with_deathdate_only
should_emit_one_heirat_for_spouse_edge_with_fromYear (happy path)
should_emit_unknown_precision_heirat_when_fromYear_is_null (eventDate null, not dropped)
should_emit_exactly_one_heirat_when_both_spouses_in_scope (symmetric dedup, key = relationship row id)
should_emit_two_heirat_for_person_married_to_two_partners
should_pass_birth_precision_through_unchanged (DAY stays DAY — assert equality, not non-null)
should_mint_prefixed_synthetic_ids_never_uuid
should_emit_heirat_with_displayname_for_both_spouses (non-null primaryPersonName + relatedPersonName)
should_assume_no_self_spouse_edge_per_db_constraint (assumption test — DB constraint no_self_rel is the enforcement; this test documents the invariant, not the guard)
should_exclude_non_family_member_persons_from_derived_events (person with family_member = false yields zero events)

**Milestone:** Zeitstrahl — Family Timeline **Spec:** `docs/superpowers/specs/2026-06-07-family-timeline-design.md` § "Derived person-events" **Depends on:** - **#1** — foundational Person date+precision migration (replaces `Person.birthYear`/`deathYear` with `Person.birthDate`/`birthDatePrecision` + `deathDate`/`deathDatePrecision`). _Confirmed: `Person.java` still has `private Integer birthYear; private Integer deathYear;` — this issue is un-startable until #1 lands._ - **#2 / #3** — the `TimelineEvent` entity and the curated-event DTO (`TimelineEntryDTO`) this issue reuses. The `timeline/` package does not exist yet; #2 owns its creation, the package/DB-diagram/`CLAUDE.md` doc updates, and the DTO shape. This issue must NOT carry that doc work. **Note:** this issue (#4) adds the `derivedType` field to `TimelineEntryDTO` — see Tasks below. > **Branch dependency:** cut the feature branch for this issue from #1's merged state (or rebase onto it). Until `Person.birthDate` / `Person.birthDatePrecision` exist, this issue's code will not compile. The red-phase tests literally cannot compile until #1 is merged — add a comment in the test class stating this so the developer doesn't waste time debugging. ## Context Births, deaths, and marriages are surfaced automatically from already-curated Person + relationship data — no manual entry, no auto-extraction from transcriptions. They are **computed on read, never stored**. This issue delivers the **in-process assembly layer only** (the `GET /api/timeline` endpoint, ordering, and bucketing are issue #5). The output of this layer is an **unordered set** of derived events; #5 owns sort/bucket/year-band ordering. **Scope of derived events:** derived events are assembled for **family-member persons only** — persons with `family_member = TRUE` (i.e., those returned by `PersonService.findAllFamilyMembers()`). Correspondents, provisional persons, and historical figures not yet added to the family tree are excluded from this assembly even if they have a `birthDate`. This matches the "curated family graph" MVP principle. ## Scope Assemble derived events from migrated Person + relationship data, in the same DTO shape as curated events, flagged `derived: true`, `type = PERSONAL`: | Source | Derived event | `eventDate` | precision | name source | |---|---|---|---|---| | `Person.birthDate` | Geburt | `Person.birthDate` | `Person.birthDatePrecision` (passed through unchanged) | `Person.getDisplayName()` → `primaryPersonName` | | `Person.deathDate` | Tod | `Person.deathDate` | `Person.deathDatePrecision` (passed through unchanged) | `Person.getDisplayName()` → `primaryPersonName` | | `SPOUSE_OF` edge, `fromYear` present | Heirat | `{fromYear}-01-01` | `YEAR` | both spouses' `getDisplayName()` → `primaryPersonName` + `relatedPersonName` | | `SPOUSE_OF` edge, `fromYear` null | Heirat | `null` | `UNKNOWN` | both spouses' `getDisplayName()` → `primaryPersonName` + `relatedPersonName` | ## Architecture & layering - `TimelineService` owns `TimelineEventRepository` (from #2) and reaches Person/relationship data **only through `PersonService` / `RelationshipService`** — never inject `PersonRelationshipRepository` or `PersonRepository` into the timeline domain. - **Batch-friendly fetch, no N+1.** Assemble from set-returning queries, not a per-person fan-out. Any implementation that calls `personService.getById()` per person is a defect: - Persons → `PersonService.findAllFamilyMembers()` (one call, family members with `family_member = TRUE` only). - Spouse edges → `RelationshipService.findAllSpouseEdges()` (new minimal method — see below; one call returning `List<PersonRelationship>` with JOIN FETCH on both person sides). - **New `RelationshipService` method:** add `findAllSpouseEdges() → List<PersonRelationship>` wrapping `relationshipRepository.findAllByRelationTypeIn(List.of(SPOUSE_OF))` with JOIN FETCH on both person sides. `TimelineService` calls this; it never touches `PersonRelationshipRepository` directly. Do NOT call `getFamilyNetwork()` (that returns a `NetworkDTO` shaped for Stammbaum, not for timeline assembly). - **Assembly decomposition:** implement three private methods, each independently testable: ```java private List<TimelineEntryDTO> buildBirthEvents(List<Person> persons) { ... } private List<TimelineEntryDTO> buildDeathEvents(List<Person> persons) { ... } private List<TimelineEntryDTO> buildMarriageEvents(List<PersonRelationship> spouseEdges) { ... } ``` A single `assembleDerivedEvents()` orchestrator calls all three and combines results. Add a Javadoc comment on this method: "Derived events are computed, never persisted, and cannot be mutated via the events API (enforced in #5)." - **`RelationshipDTO`** (carries `id`, `relationType`, `fromYear`, both person ids + display names) is the correct wire type returned by existing `RelationshipService` methods. `PersonRelationship` entity is used in the new `findAllSpouseEdges()` — the entity carries the `Person` references needed for `getDisplayName()`. ## `TimelineEntryDTO` contract The DTO is defined in #2/#3 but **this issue (#4) adds `derivedType` to it.** Derived events need a discriminator (`BIRTH`/`DEATH`/`MARRIAGE`) so the frontend can compose localized labels; curated events have a `title` instead. Adding `derivedType` to the shared DTO is a breaking OpenAPI change — see Tasks. **Field requirements on `TimelineEntryDTO` enforced by this issue:** - `id: String` (NOT `UUID`) — synthetic prefixed ids like `birth:{uuid}` are non-UUID by construction; if typed `UUID` they will fail to parse. - `derivedType: DerivedEventType` (nullable; null for curated events) — enum: `BIRTH`, `DEATH`, `MARRIAGE`. - `primaryPersonName: String` — display name of the subject (Geburt/Tod) or first spouse (Heirat). - `relatedPersonName: String` (nullable; null for Geburt/Tod, populated for Heirat) — display name of second spouse. **Not pre-concatenated** — the frontend (#6/#7) owns the " & " separator and locale-specific formatting. The i18n key `timeline.derived.marriage({nameA}, {nameB})` needs two interpolation slots. - `derived: boolean`, `type: EventType` (always `PERSONAL` for derived events). `Person.getDisplayName()` is confirmed null-safe in existing code, but add an assertion test that display names are non-null on emitted events. ## Synthetic ids Derived events have no persisted UUID. Mint prefixed synthetic ids: - `birth:{personId}` (UUID of the Person) - `death:{personId}` (UUID of the Person) - `marriage:{relationshipId}` (UUID of the PersonRelationship row) These are non-UUID by construction and MUST never collide with a real `TimelineEvent` UUID. The `id` field on `TimelineEntryDTO` MUST be `String`, not `UUID`. ## Symmetric-marriage dedup `SPOUSE_OF` is stored once per couple (enforced by the `unique_spouse_pair` DB index using `LEAST`/`GREATEST` — V55). The in-memory dedup on `Set<UUID>` of relationship ids is a correctness assertion, not the primary defense. Add a comment in the service explaining this: "DB constraint `unique_spouse_pair` is the authoritative enforcement; in-memory dedup is a defensive assertion." ## Logging Use parameterized SLF4J (`@Slf4j`): `logger.debug("Assembled {} derived events for {} persons", events.size(), persons.size())`. Never string concatenation. No PII (person names, dates) in logs at INFO or above. ## Resolved decisions 1. **Null-`fromYear` marriage → emit, do not drop.** A `SPOUSE_OF` edge can be saved with `fromYear == null`. **Resolution:** emit the Heirat with `eventDate = null` and `precision = UNKNOWN` so it lands in the "Ohne Datum" bucket. Rationale: dropping it hides a real marriage and violates the spec's "no fabricated dates / honest about precision" principle. 2. **Label localization → `derivedType` discriminator, not a baked German `title`.** The DTO exposes `derivedType` enum (`BIRTH`/`DEATH`/`MARRIAGE`) plus the person name(s); the frontend (#6/#7) composes the localized, precision-aware label from an i18n key. Rationale: de/en/es product; a server-frozen German string would force German labels on all locales. 3. **`DatePrecision` stays in `document/` for this issue.** Do not move it as part of this issue — inherit whatever foundational issue #1 decides. If #1 relocates it to a shared package, this issue simply imports the new path. 4. **Synthetic derived-event ids.** Mint prefixed synthetic ids — `birth:{personId}`, `death:{personId}`, `marriage:{relationshipId}`. Non-UUID by construction and MUST never collide with a real `TimelineEvent` UUID. 5. **Symmetric-marriage dedup key = relationship row id.** Dedup on the relationship row id (the `unique_spouse_pair` DB index is the authoritative enforcement; in-memory `Set<UUID>` is a defensive assertion). 6. **Derived-event person scope: family members only (`family_member = TRUE`).** `PersonService.findAllFamilyMembers()` is the correct query. Correspondents and provisional persons not yet in the family tree are excluded. Every acceptance criterion that says "a person" means "a family-member person." Rationale: MVP scope matches the curated family graph principle; broadening to all persons is a future enhancement. 7. **`derivedType` is added to `TimelineEntryDTO` by this issue (#4).** It was always part of the timeline design but not yet in #2/#3's DTO definition. Adding it here is a breaking OpenAPI change — see Tasks. Rationale: the discriminator is first needed here; delaying it would block localized label rendering in #6/#7. 8. **`TimelineEntryDTO.id` is typed `String`, not `UUID`.** Synthetic prefixed ids (`birth:{uuid}`) cannot parse as UUID — the DTO field must be `String`. Enforce this when #2 defines the DTO; confirm before implementation starts. 9. **Two separate name fields on `TimelineEntryDTO` for Heirat.** `primaryPersonName: String` and `relatedPersonName: String` (nullable) — not pre-concatenated. The frontend owns the " & " separator and i18n formatting. 10. **`RelationshipService.findAllSpouseEdges()` is the correct service boundary.** A new minimal method wrapping `findAllByRelationTypeIn(List.of(SPOUSE_OF))` with JOIN FETCH. `TimelineService` never touches `PersonRelationshipRepository` or `getFamilyNetwork()` directly. 11. **Assembly decomposed into three private methods** (`buildBirthEvents`, `buildDeathEvents`, `buildMarriageEvents`) + `assembleDerivedEvents()` orchestrator. Each is independently testable. ## Tasks - [ ] **Pre-condition check:** confirm `TimelineEntryDTO.id` is `String` (not `UUID`) in #2's branch before writing any code — if it is `UUID`, block on that fix first. - [ ] Add `derivedType: DerivedEventType` (enum `BIRTH`/`DEATH`/`MARRIAGE`), `primaryPersonName: String`, and `relatedPersonName: String` (nullable) to `TimelineEntryDTO` in the `timeline/` package. - [ ] Run `npm run generate:api` after adding `derivedType` and the name fields to `TimelineEntryDTO` — this is a breaking OpenAPI change. - [ ] Add `RelationshipService.findAllSpouseEdges() → List<PersonRelationship>` wrapping `relationshipRepository.findAllByRelationTypeIn(List.of(SPOUSE_OF))` with JOIN FETCH on both person sides. - [ ] Implement `buildBirthEvents(List<Person>)`, `buildDeathEvents(List<Person>)`, `buildMarriageEvents(List<PersonRelationship>)` private methods in `TimelineService`. - [ ] Implement `assembleDerivedEvents()` orchestrator calling all three. Add Javadoc: "Derived events are computed, never persisted, and cannot be mutated via the events API (enforced in #5)." - [ ] Geburt from `Person.birthDate` + `birthDatePrecision` (1:1, no dedup). Tod from `Person.deathDate` + `deathDatePrecision` (1:1, no dedup). Precision passed through unchanged. - [ ] Heirat from `SPOUSE_OF` edges: present `fromYear` → `{fromYear}-01-01` @ `YEAR`; null `fromYear` → `eventDate = null` @ `UNKNOWN` (emitted, not dropped). - [ ] Marriage derived exactly once per `SPOUSE_OF` edge; dedup on `Set<UUID>` of relationship row ids; add comment explaining DB constraint is authoritative. - [ ] Mint synthetic prefixed ids: `birth:{personId}`, `death:{personId}`, `marriage:{relationshipId}` — never UUIDs. - [ ] Pull Person/relationship data via their services only (`findAllFamilyMembers()` + `findAllSpouseEdges()`) — no per-person N+1. - [ ] `@Slf4j` with parameterized logging; no PII at INFO+. ## Acceptance criteria All criteria apply to **family-member persons** (those with `family_member = TRUE`): - A family-member person with a `birthDate` yields exactly one Geburt event at the person's `birthDatePrecision`. - A family-member person with a `deathDate` but no `birthDate` yields exactly one Tod and zero Geburt events. - A family-member person with a `birthDate` but no `deathDate` yields exactly one Geburt and zero Tod. - A family-member person with neither `birthDate` nor `deathDate` yields zero derived events. - A married couple (one `SPOUSE_OF` edge) yields **exactly one** Heirat event, even when both spouses are in the queried scope. - A person married to two different partners (two `SPOUSE_OF` rows) yields **two** distinct Heirat events. - A `SPOUSE_OF` edge with null `fromYear` yields one Heirat with `eventDate = null`, `precision = UNKNOWN` (it is surfaced, not suppressed). - A `DAY`-precision `birthDate` arrives on the event as `DAY` (not collapsed to `YEAR`); Heirat is `YEAR` (or `UNKNOWN` when `fromYear` null). - Every derived event carries `derived: true`, `type = PERSONAL`, a non-null `derivedType`, and a synthetic non-UUID `String` id — produced without a persisted id and excluded from any update/delete path. - Every derived event has a non-null `primaryPersonName`; Heirat events also have a non-null `relatedPersonName`. - A person with `family_member = FALSE` and a `birthDate` yields zero derived events (out of scope). > **Carried to #5 (read endpoint):** the derived-event id namespace is read-only by construction. The events controller in #5 must **reject** any non-UUID / prefixed id on `PUT`/`DELETE` (`PUT /api/timeline/events/birth:{...}` → 400, never a path that mutates a Person record). The "not editable via the events API" guarantee is enforced and tested in #5, not here. #5 also owns ordering, bucketing, `READ_ALL` gate, and the `PUT /api/timeline/events/birth:{...} → 400` security test. ## Tests Pure unit tests with `@ExtendWith(MockitoExtension.class)` against mocked `PersonService` + `RelationshipService` — no Spring context, no Testcontainers (that cost belongs to #2's migration tests). **Factory helpers:** ```java private Person makePerson(LocalDate birthDate, DatePrecision precision) { return Person.builder() .id(UUID.randomUUID()) .firstName("Anna") .lastName("Müller") .familyMember(true) .birthDate(birthDate) .birthDatePrecision(precision) .build(); } private PersonRelationship makeSpouseEdge(Person a, Person b, Integer fromYear) { return PersonRelationship.builder() .id(UUID.randomUUID()) .person(a) .relatedPerson(b) .relationType(RelationType.SPOUSE_OF) .fromYear(fromYear) .build(); } ``` Note: `Person.birthDate`/`birthDatePrecision`/`familyMember` fields don't exist until #1 merges — the factory helpers (and thus all tests) will not compile until then. Add a class-level comment: `// NOTE: tests require #1 (Person date migration) to be merged before this class compiles.` In every event-assertion test also assert structural invariants: `assertThat(event.derived()).isTrue()`, `assertThat(event.type()).isEqualTo(EventType.PERSONAL)`, `assertThat(event.derivedType()).isEqualTo(DerivedEventType.BIRTH)` (or DEATH/MARRIAGE as appropriate). These are not separate behaviors — assert them within each test. For precision tests use: `assertThat(event.precision()).isEqualTo(DatePrecision.DAY)` (not just non-null). TDD order: - [ ] `should_emit_one_geburt_for_person_with_birthdate` - [ ] `should_emit_zero_tod_when_person_has_birthdate_but_no_deathdate` - [ ] `should_emit_no_events_for_person_with_neither_date` - [ ] `should_emit_one_tod_for_person_with_deathdate` - [ ] `should_emit_one_tod_and_zero_geburt_for_person_with_deathdate_only` - [ ] `should_emit_one_heirat_for_spouse_edge_with_fromYear` (happy path) - [ ] `should_emit_unknown_precision_heirat_when_fromYear_is_null` (eventDate null, not dropped) - [ ] `should_emit_exactly_one_heirat_when_both_spouses_in_scope` (symmetric dedup, key = relationship row id) - [ ] `should_emit_two_heirat_for_person_married_to_two_partners` - [ ] `should_pass_birth_precision_through_unchanged` (DAY stays DAY — assert equality, not non-null) - [ ] `should_mint_prefixed_synthetic_ids_never_uuid` - [ ] `should_emit_heirat_with_displayname_for_both_spouses` (non-null primaryPersonName + relatedPersonName) - [ ] `should_assume_no_self_spouse_edge_per_db_constraint` (assumption test — DB constraint `no_self_rel` is the enforcement; this test documents the invariant, not the guard) - [ ] `should_exclude_non_family_member_persons_from_derived_events` (person with `family_member = false` yields zero events)

marcel added this to the Zeitstrahl — Family Timeline milestone 2026-06-07 19:28:59 +02:00

marcel added the P2-medium feature person labels 2026-06-07 19:30:00 +02:00

marcel commented

2026-06-07 19:53:13 +02:00

🏗️ Markus Keller — Senior Application Architect

Observations

Domain boundary is correctly drawn. TimelineService → PersonService.findAllFamilyMembers() + RelationshipService.findAllSpouseEdges() respects the layering rule. The prohibition on injecting PersonRelationshipRepository directly is explicit and correct.

findAllByRelationTypeIn already has JOIN FETCH — but it lives in PersonRelationshipRepository directly. The new RelationshipService.findAllSpouseEdges() wraps exactly this method. That means RelationshipService already owns the boundary correctly — the wrapper is merely the service-layer façade. Good. One detail: the existing findAllByRelationTypeIn query currently passes FAMILY_RELATION_TYPES (three types) from getFamilyNetwork(). The new method should pass only List.of(SPOUSE_OF) — not all three family types — to keep the semantic narrow. The issue states this correctly; just confirming it matches the existing repo signature.

ADR is overdue. The issue says "An ADR may be warranted for the new timeline/ domain." By Markus's rules, a new backend domain package with its own entity is a structural decision that requires an ADR — not "may." Next sequential number is ADR-035. The ADR should be written before this issue's code is merged, covering: why derived events are computed on read rather than persisted, the String id type for synthetic ids, and the timeline/ package boundary. This is a blocker by the doc-update table in the persona.

Documentation updates this issue triggers:

CLAUDE.md package table must add timeline/ with its contents.
docs/architecture/c4/l3-backend-*.puml must add TimelineService + the two new methods it calls on person/relationship services.
docs/architecture/db/db-orm.puml is unaffected by this issue (no new entity here; TimelineEvent entity belongs to #2).
docs/GLOSSARY.md needs entries for DerivedEventType, derivedType, assembleDerivedEvents, and "derived event" as a concept.
The issue tasks do not mention any of this. Add a task: "Update CLAUDE.md package table + C4 L3 diagram + GLOSSARY.md before merging."

@Transactional(readOnly = true) on assembleDerivedEvents(). The method calls findAllFamilyMembers() (PersonRepository) and findAllSpouseEdges() (RelationshipRepository) — both load lazy associations on PersonRelationship.person / relatedPerson. If the session closes between the two calls (non-transactional context), calling getDisplayName() on the eagerly-fetched person objects is safe because JOIN FETCH forces eager loading. However, findAllFamilyMembers() uses PersonRepository.findByFamilyMemberTrueOrderByLastNameAscFirstNameAsc() which is a derived query with no JOIN FETCH — the nameAliases collection on Person is FetchType.LAZY (as seen in Person.java). If any code path in the assembly touches person.getNameAliases(), it will throw LazyInitializationException without a transaction. Safe because assembleDerivedEvents only calls getDisplayName() (a @Transient computed from title, firstName, lastName — all eagerly loaded). Still, annotating assembleDerivedEvents() with @Transactional(readOnly = true) is defensive best practice per ADR-022 and should be added to the Tasks.

Precision pass-through is correct architecturally — no conversion in the service layer. The rendering contract lives in the frontend helper (dateLabel.ts), consistent with the "axis is the year" principle.

Recommendations

Require ADR-035 before this issue merges. Topic: derived-event computation strategy (on-read vs. persisted), synthetic String id rationale, timeline/ domain boundary.
Add @Transactional(readOnly = true) to assembleDerivedEvents() as defensive practice per ADR-022, even though the current access paths are safe.
Add a task for doc updates (CLAUDE.md package table, C4 L3 backend diagram, GLOSSARY.md) before the PR is opened.
Confirm the new findAllSpouseEdges() passes only List.of(SPOUSE_OF) — not the full FAMILY_RELATION_TYPES list — so the method semantics stay narrow.

Open Decisions

None. Architecture is clear and well-constrained by the issue.

## 🏗️ Markus Keller — Senior Application Architect ### Observations **Domain boundary is correctly drawn.** `TimelineService` → `PersonService.findAllFamilyMembers()` + `RelationshipService.findAllSpouseEdges()` respects the layering rule. The prohibition on injecting `PersonRelationshipRepository` directly is explicit and correct. **`findAllByRelationTypeIn` already has JOIN FETCH — but it lives in `PersonRelationshipRepository` directly.** The new `RelationshipService.findAllSpouseEdges()` wraps exactly this method. That means `RelationshipService` already owns the boundary correctly — the wrapper is merely the service-layer façade. Good. One detail: the existing `findAllByRelationTypeIn` query currently passes `FAMILY_RELATION_TYPES` (three types) from `getFamilyNetwork()`. The new method should pass only `List.of(SPOUSE_OF)` — not all three family types — to keep the semantic narrow. The issue states this correctly; just confirming it matches the existing repo signature. **ADR is overdue.** The issue says "An ADR may be warranted for the new `timeline/` domain." By Markus's rules, a new backend domain package with its own entity is a structural decision that _requires_ an ADR — not "may." Next sequential number is ADR-035. The ADR should be written before this issue's code is merged, covering: why derived events are computed on read rather than persisted, the `String` id type for synthetic ids, and the `timeline/` package boundary. This is a blocker by the doc-update table in the persona. **Documentation updates this issue triggers:** - `CLAUDE.md` package table must add `timeline/` with its contents. - `docs/architecture/c4/l3-backend-*.puml` must add `TimelineService` + the two new methods it calls on person/relationship services. - `docs/architecture/db/db-orm.puml` is unaffected by this issue (no new entity here; `TimelineEvent` entity belongs to #2). - `docs/GLOSSARY.md` needs entries for `DerivedEventType`, `derivedType`, `assembleDerivedEvents`, and "derived event" as a concept. - The issue tasks do not mention any of this. Add a task: "Update CLAUDE.md package table + C4 L3 diagram + GLOSSARY.md before merging." **`@Transactional(readOnly = true)` on `assembleDerivedEvents()`.** The method calls `findAllFamilyMembers()` (PersonRepository) and `findAllSpouseEdges()` (RelationshipRepository) — both load lazy associations on `PersonRelationship.person` / `relatedPerson`. If the session closes between the two calls (non-transactional context), calling `getDisplayName()` on the eagerly-fetched person objects is safe because `JOIN FETCH` forces eager loading. However, `findAllFamilyMembers()` uses `PersonRepository.findByFamilyMemberTrueOrderByLastNameAscFirstNameAsc()` which is a derived query with no JOIN FETCH — the `nameAliases` collection on `Person` is `FetchType.LAZY` (as seen in `Person.java`). If any code path in the assembly touches `person.getNameAliases()`, it will throw `LazyInitializationException` without a transaction. Safe because `assembleDerivedEvents` only calls `getDisplayName()` (a `@Transient` computed from `title`, `firstName`, `lastName` — all eagerly loaded). Still, annotating `assembleDerivedEvents()` with `@Transactional(readOnly = true)` is defensive best practice per ADR-022 and should be added to the Tasks. **Precision pass-through is correct architecturally** — no conversion in the service layer. The rendering contract lives in the frontend helper (`dateLabel.ts`), consistent with the "axis is the year" principle. ### Recommendations 1. **Require ADR-035 before this issue merges.** Topic: derived-event computation strategy (on-read vs. persisted), synthetic `String` id rationale, `timeline/` domain boundary. 2. **Add `@Transactional(readOnly = true)` to `assembleDerivedEvents()`** as defensive practice per ADR-022, even though the current access paths are safe. 3. **Add a task for doc updates** (CLAUDE.md package table, C4 L3 backend diagram, GLOSSARY.md) before the PR is opened. 4. **Confirm the new `findAllSpouseEdges()` passes only `List.of(SPOUSE_OF)`** — not the full `FAMILY_RELATION_TYPES` list — so the method semantics stay narrow. ### Open Decisions None. Architecture is clear and well-constrained by the issue.

marcel commented

2026-06-07 19:53:38 +02:00

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

The factory helpers in the Tests section will not compile until #1 merges. The issue requires a class-level comment stating this — confirmed and correct. One addition: the makeSpouseEdge() factory should also call .relationType(RelationType.SPOUSE_OF) explicitly (it's shown that way in the issue body, good), and both person and relatedPerson must be non-null for getDisplayName() to work. The factory should create persons via makePerson() to reuse the builder, not construct inline.

The assembleDerivedEvents() orchestrator calls three private methods and combines results — that's exactly one job per function. Clean. However, the orchestrator itself should be package-private (not private) if it's the integration point that #5 will call. The three build* helpers should be private. The current spec says "private methods" for all three builders and the orchestrator — clarify that assembleDerivedEvents() itself is package-private or protected (whatever #5 needs to call it). If #5 wires it through TimelineService as a public method, document that.

Person.getDisplayName() is @Transient and reads only title, firstName, lastName — confirmed null-safe by looking at DisplayNameFormatter.format(). The assertion test should_emit_heirat_with_displayname_for_both_spouses must assert non-null and non-blank (empty string from a null-firstName person is still technically non-null).

The test order covers the critical dedup case well. One gap: there is no test for should_emit_zero_events_for_person_with_familyMember_false_even_when_birthDate_present. This is listed (should_exclude_non_family_member_persons_from_derived_events) — good, but the factory makePerson() sets familyMember(true) by default. A separate factory variant makeNonFamilyPerson() or an override makePerson(LocalDate birthDate, DatePrecision precision, boolean familyMember) should be included to avoid constructing the builder inline in that one test.

should_mint_prefixed_synthetic_ids_never_uuid test — the assertion should verify the format actively: assertThat(event.id()).startsWith("birth:"), not just that it is not a UUID. Use assertThat(UUID.fromString(event.id())).as("id must NOT be parseable as UUID") with an assertThatThrownBy(...) to confirm parse failure. This is the strongest form of the invariant.

Naming: assembleDerivedEvents() is imperative (verb-noun) rather than declarative. Given that it returns a value, a name like derivedEvents() or computeDerivedEvents() would read better as a query. However, the issue locks this name in explicitly — if the convention is "assemble" throughout, keep it consistent. No action needed unless the convention is revisited.

buildMarriageEvents() receives List<PersonRelationship> with both person sides JOIN-FETCHed (confirmed: PersonRelationshipRepository.findAllByRelationTypeIn has JOIN FETCH on both sides). Calling r.getPerson().getDisplayName() inside the method is safe without a transaction. Add an inline comment in the implementation referencing this: // JOIN FETCH in findAllSpouseEdges() guarantees person/relatedPerson are loaded.

Recommendations

Assert both startsWith("birth:") AND UUID parse failure in should_mint_prefixed_synthetic_ids_never_uuid — the two-sided assertion proves both properties.
Add makeNonFamilyPerson(...) factory variant to keep the exclusion test as readable as the happy-path tests.
Clarify assembleDerivedEvents() visibility in the issue tasks — should it be public (called by TimelineService's read endpoint in #5) or remain package-private? The Javadoc comment on it should also document who calls it.
Assert non-blank display names (not just non-null) in should_emit_heirat_with_displayname_for_both_spouses.

Open Decisions

None. TDD order and naming are explicitly spelled out.

## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Observations **The factory helpers in the Tests section will not compile until #1 merges.** The issue requires a class-level comment stating this — confirmed and correct. One addition: the `makeSpouseEdge()` factory should also call `.relationType(RelationType.SPOUSE_OF)` explicitly (it's shown that way in the issue body, good), and both `person` and `relatedPerson` must be non-null for `getDisplayName()` to work. The factory should create persons via `makePerson()` to reuse the builder, not construct inline. **The `assembleDerivedEvents()` orchestrator calls three private methods and combines results — that's exactly one job per function.** Clean. However, the orchestrator itself should be package-private (not `private`) if it's the integration point that #5 will call. The three `build*` helpers should be `private`. The current spec says "private methods" for all three builders and the orchestrator — clarify that `assembleDerivedEvents()` itself is package-private or protected (whatever #5 needs to call it). If #5 wires it through `TimelineService` as a public method, document that. **`Person.getDisplayName()` is `@Transient` and reads only `title`, `firstName`, `lastName` — confirmed null-safe** by looking at `DisplayNameFormatter.format()`. The assertion test `should_emit_heirat_with_displayname_for_both_spouses` must assert non-null _and_ non-blank (empty string from a null-firstName person is still technically non-null). **The test order covers the critical dedup case well.** One gap: there is no test for `should_emit_zero_events_for_person_with_familyMember_false_even_when_birthDate_present`. This is listed (`should_exclude_non_family_member_persons_from_derived_events`) — good, but the factory `makePerson()` sets `familyMember(true)` by default. A separate factory variant `makeNonFamilyPerson()` or an override `makePerson(LocalDate birthDate, DatePrecision precision, boolean familyMember)` should be included to avoid constructing the builder inline in that one test. **`should_mint_prefixed_synthetic_ids_never_uuid` test** — the assertion should verify the format actively: `assertThat(event.id()).startsWith("birth:")`, not just that it is not a UUID. Use `assertThat(UUID.fromString(event.id())).as("id must NOT be parseable as UUID")` with an `assertThatThrownBy(...)` to confirm parse failure. This is the strongest form of the invariant. **Naming: `assembleDerivedEvents()` is imperative (verb-noun) rather than declarative.** Given that it returns a value, a name like `derivedEvents()` or `computeDerivedEvents()` would read better as a query. However, the issue locks this name in explicitly — if the convention is "assemble" throughout, keep it consistent. No action needed unless the convention is revisited. **`buildMarriageEvents()` receives `List<PersonRelationship>` with both person sides JOIN-FETCHed** (confirmed: `PersonRelationshipRepository.findAllByRelationTypeIn` has JOIN FETCH on both sides). Calling `r.getPerson().getDisplayName()` inside the method is safe without a transaction. Add an inline comment in the implementation referencing this: `// JOIN FETCH in findAllSpouseEdges() guarantees person/relatedPerson are loaded`. ### Recommendations 1. **Assert both `startsWith("birth:")` AND UUID parse failure** in `should_mint_prefixed_synthetic_ids_never_uuid` — the two-sided assertion proves both properties. 2. **Add `makeNonFamilyPerson(...)` factory variant** to keep the exclusion test as readable as the happy-path tests. 3. **Clarify `assembleDerivedEvents()` visibility** in the issue tasks — should it be `public` (called by `TimelineService`'s read endpoint in #5) or remain package-private? The Javadoc comment on it should also document who calls it. 4. **Assert non-blank display names** (not just non-null) in `should_emit_heirat_with_displayname_for_both_spouses`. ### Open Decisions None. TDD order and naming are explicitly spelled out.

marcel commented

2026-06-07 19:54:01 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

No direct attack surface on this issue. The assembly layer is read-only, produces no persisted data, and introduces no new endpoints. The attack surface concern lives in #5 (the GET /api/timeline endpoint) and its rejection of synthetic ids on PUT/DELETE. This issue correctly defers that enforcement to #5 and calls it out explicitly in the "Carried to #5" note.

Synthetic id collision risk is addressed at the right layer — but the guarantee needs a test. The issue states: "MUST never collide with a real TimelineEvent UUID." The prefixed format birth:{uuid} is structurally non-UUID, so no UUID-typed field can accidentally hold it. However, if #2 defines TimelineEntryDTO.id as String (not UUID), there is a theoretical risk that someone passes birth:some-real-uuid-here as an id to a write endpoint. The issue correctly pushes the rejection test to #5, but I'd recommend adding a comment to the assembleDerivedEvents() Javadoc: "Ids produced by this method are structurally non-UUID (birth:*, death:*, marriage:*) and MUST be rejected by any write endpoint — enforced and tested in #5." This documents the security contract at the source.

No PII in logs above INFO — explicitly required and correct. The @Slf4j parameterized logging rule logger.debug(...) with events.size() and persons.size() (counts, not names/dates) is the right implementation. Recommend adding to the tasks: explicitly check that neither primaryPersonName nor relatedPersonName appear in any log statement, even at DEBUG. Names are PII in the GDPR sense.

The family_member = TRUE scope filter is a security boundary in addition to a business rule. Persons with family_member = FALSE (correspondents, provisional persons) may be living persons with privacy interests. Excluding them from the timeline is correct — and the test should_exclude_non_family_member_persons_from_derived_events makes this a regression-tested guarantee.

PersonService.findAllFamilyMembers() returns all family members without pagination — this is fine for a family archive (realistically < 500 persons), but a note: if the archive ever grows, a paginated or streamed approach becomes necessary. No action needed now; this is a future-scope observation.

No new ErrorCode is introduced, and no new error handling paths exist in this assembly layer. The "computed on read, never persisted" design means there are no write-path security concerns in this issue.

Resolved decision #8 (TimelineEntryDTO.id is String) is the correct call for security too — using UUID for a synthetic id field would require either a fake-UUID format (collision risk) or a type cast that breaks downstream validation. The String type prevents misuse of synthetic ids in UUID-typed write paths.

Recommendations

Document the security contract in assembleDerivedEvents() Javadoc: ids produced are structurally non-UUID; any write endpoint receiving a prefixed id MUST reject with 400 (enforced in #5).
Add an explicit log-audit task: confirm no log statement at DEBUG or above emits a person's name or date — only aggregate counts.
Confirm in the pre-condition check task (already in Tasks) that TimelineEntryDTO.id is String not UUID before writing — this is both a correctness and a security-contract gate.

Open Decisions

None. Security boundaries are correctly specified and deferred enforcement is clearly attributed to #5.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations **No direct attack surface on this issue.** The assembly layer is read-only, produces no persisted data, and introduces no new endpoints. The attack surface concern lives in #5 (the `GET /api/timeline` endpoint) and its rejection of synthetic ids on `PUT`/`DELETE`. This issue correctly defers that enforcement to #5 and calls it out explicitly in the "Carried to #5" note. **Synthetic id collision risk is addressed at the right layer — but the guarantee needs a test.** The issue states: "MUST never collide with a real `TimelineEvent` UUID." The prefixed format `birth:{uuid}` is structurally non-UUID, so no UUID-typed field can accidentally hold it. However, if #2 defines `TimelineEntryDTO.id` as `String` (not `UUID`), there is a theoretical risk that someone passes `birth:some-real-uuid-here` as an id to a write endpoint. The issue correctly pushes the rejection test to #5, but I'd recommend adding a comment to the `assembleDerivedEvents()` Javadoc: "Ids produced by this method are structurally non-UUID (`birth:*`, `death:*`, `marriage:*`) and MUST be rejected by any write endpoint — enforced and tested in #5." This documents the security contract at the source. **No PII in logs above INFO — explicitly required and correct.** The `@Slf4j` parameterized logging rule `logger.debug(...)` with `events.size()` and `persons.size()` (counts, not names/dates) is the right implementation. Recommend adding to the tasks: explicitly check that neither `primaryPersonName` nor `relatedPersonName` appear in any log statement, even at `DEBUG`. Names are PII in the GDPR sense. **The `family_member = TRUE` scope filter is a security boundary in addition to a business rule.** Persons with `family_member = FALSE` (correspondents, provisional persons) may be living persons with privacy interests. Excluding them from the timeline is correct — and the test `should_exclude_non_family_member_persons_from_derived_events` makes this a regression-tested guarantee. **`PersonService.findAllFamilyMembers()` returns all family members without pagination** — this is fine for a family archive (realistically < 500 persons), but a note: if the archive ever grows, a paginated or streamed approach becomes necessary. No action needed now; this is a future-scope observation. **No new `ErrorCode` is introduced, and no new error handling paths exist** in this assembly layer. The "computed on read, never persisted" design means there are no write-path security concerns in this issue. **Resolved decision #8 (`TimelineEntryDTO.id` is `String`) is the correct call for security too** — using `UUID` for a synthetic id field would require either a fake-UUID format (collision risk) or a type cast that breaks downstream validation. The `String` type prevents misuse of synthetic ids in UUID-typed write paths. ### Recommendations 1. **Document the security contract in `assembleDerivedEvents()` Javadoc**: ids produced are structurally non-UUID; any write endpoint receiving a prefixed id MUST reject with 400 (enforced in #5). 2. **Add an explicit log-audit task**: confirm no log statement at DEBUG or above emits a person's name or date — only aggregate counts. 3. **Confirm in the pre-condition check task** (already in Tasks) that `TimelineEntryDTO.id` is `String` not `UUID` before writing — this is both a correctness and a security-contract gate. ### Open Decisions None. Security boundaries are correctly specified and deferred enforcement is clearly attributed to #5.

marcel commented

2026-06-07 19:54:28 +02:00

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

The TDD order is well-sequenced — it goes from simple (single birth event) to composite (both spouses in scope) to invariant (id format, display names). The ordering ensures each test is independently runnable before the next, which is the right red/green discipline.

Test coverage analysis against the acceptance criteria:

Acceptance criterion	Covered by test	Gap?
birthDate → exactly one Geburt	`should_emit_one_geburt_for_person_with_birthdate`	✅
deathDate only → one Tod, zero Geburt	`should_emit_one_tod_and_zero_geburt_for_person_with_deathdate_only`	✅
birthDate only → one Geburt, zero Tod	`should_emit_zero_tod_when_person_has_birthdate_but_no_deathdate`	✅
neither → zero events	`should_emit_no_events_for_person_with_neither_date`	✅
one SPOUSE_OF → exactly one Heirat	`should_emit_one_heirat_for_spouse_edge_with_fromYear`	✅
both spouses → one Heirat	`should_emit_exactly_one_heirat_when_both_spouses_in_scope`	✅
two SPOUSE_OF rows → two Heirat	`should_emit_two_heirat_for_person_married_to_two_partners`	✅
null fromYear → one Heirat, eventDate=null, UNKNOWN	`should_emit_unknown_precision_heirat_when_fromYear_is_null`	✅
DAY precision preserved	`should_pass_birth_precision_through_unchanged`	✅
Heirat is YEAR precision	NOT explicitly tested	⚠️
synthetic non-UUID id	`should_mint_prefixed_synthetic_ids_never_uuid`	✅
display names non-null	`should_emit_heirat_with_displayname_for_both_spouses`	✅
self-spouse DB constraint	`should_assume_no_self_spouse_edge_per_db_constraint`	✅
family_member=false → zero events	`should_exclude_non_family_member_persons_from_derived_events`	✅

Gap: Heirat precision = YEAR is untested. The acceptance criteria states "A DAY-precision birthDate arrives on the event as DAY... Heirat is YEAR (or UNKNOWN when fromYear null)." The should_pass_birth_precision_through_unchanged test covers birth precision. The Heirat YEAR case is covered by the happy path test (should_emit_one_heirat_for_spouse_edge_with_fromYear) but there's no explicit assertion assertThat(event.precision()).isEqualTo(DatePrecision.YEAR) listed. This should be added to that test's assertion block. Recommend adding an explicit assertion rather than leaving it implicit.

Structural invariants are correctly required within each test — asserting derived: true, type = PERSONAL, derivedType in every event test prevents "assertion tourism" where a dedicated test passes while other tests silently produce wrong invariant values.

The should_assume_no_self_spouse_edge_per_db_constraint test is an assumption test, not a behavior test. It should use Assumptions.assumeThat(...) from AssertJ or a comment explaining this is a documentation test, not a guard. If it uses assertThat, a developer might think the code guards against self-edges (it doesn't — the DB constraint does).

Missing: assembleDerivedEvents() with an empty list of family members should yield zero events — trivial but ensures the orchestrator doesn't NPE on empty input. This is a boundary condition the issue doesn't list.

Test layer placement is correct — pure Mockito unit tests, no Spring context, no Testcontainers. The issue correctly assigns migration tests to #2. CI time impact of this test class: ~50ms.

The @Transactional(readOnly = true) consideration: since these are pure unit tests with mocked services, transaction behavior is irrelevant in the test class. The service-level annotation matters only in integration context (#5's tests).

Recommendations

Add explicit assertThat(event.precision()).isEqualTo(DatePrecision.YEAR) in should_emit_one_heirat_for_spouse_edge_with_fromYear — the Heirat YEAR precision is an acceptance criterion that needs an explicit assertion, not just an implicit one.
Add should_emit_zero_events_when_family_member_list_is_empty as a boundary test for the orchestrator.
Use assertThat comment or restructure should_assume_no_self_spouse_edge_per_db_constraint to make clear it's a documentation test (asserting what the DB guarantees), not a behavior test (asserting what the service guards). Consider renaming to self_spouse_edge_does_not_exist_per_db_constraint_documentation.

Open Decisions

None. Test strategy and pyramid placement are clearly specified.

## 🧪 Sara Holt — QA Engineer & Test Strategist ### Observations **The TDD order is well-sequenced** — it goes from simple (single birth event) to composite (both spouses in scope) to invariant (id format, display names). The ordering ensures each test is independently runnable before the next, which is the right red/green discipline. **Test coverage analysis against the acceptance criteria:** | Acceptance criterion | Covered by test | Gap? | |---|---|---| | birthDate → exactly one Geburt | `should_emit_one_geburt_for_person_with_birthdate` | ✅ | | deathDate only → one Tod, zero Geburt | `should_emit_one_tod_and_zero_geburt_for_person_with_deathdate_only` | ✅ | | birthDate only → one Geburt, zero Tod | `should_emit_zero_tod_when_person_has_birthdate_but_no_deathdate` | ✅ | | neither → zero events | `should_emit_no_events_for_person_with_neither_date` | ✅ | | one SPOUSE_OF → exactly one Heirat | `should_emit_one_heirat_for_spouse_edge_with_fromYear` | ✅ | | both spouses → one Heirat | `should_emit_exactly_one_heirat_when_both_spouses_in_scope` | ✅ | | two SPOUSE_OF rows → two Heirat | `should_emit_two_heirat_for_person_married_to_two_partners` | ✅ | | null fromYear → one Heirat, eventDate=null, UNKNOWN | `should_emit_unknown_precision_heirat_when_fromYear_is_null` | ✅ | | DAY precision preserved | `should_pass_birth_precision_through_unchanged` | ✅ | | Heirat is YEAR precision | NOT explicitly tested | ⚠️ | | synthetic non-UUID id | `should_mint_prefixed_synthetic_ids_never_uuid` | ✅ | | display names non-null | `should_emit_heirat_with_displayname_for_both_spouses` | ✅ | | self-spouse DB constraint | `should_assume_no_self_spouse_edge_per_db_constraint` | ✅ | | family_member=false → zero events | `should_exclude_non_family_member_persons_from_derived_events` | ✅ | **Gap: Heirat precision = YEAR is untested.** The acceptance criteria states "A `DAY`-precision `birthDate` arrives on the event as `DAY`... Heirat is `YEAR` (or `UNKNOWN` when `fromYear` null)." The `should_pass_birth_precision_through_unchanged` test covers birth precision. The Heirat YEAR case is covered by the happy path test (`should_emit_one_heirat_for_spouse_edge_with_fromYear`) but there's no explicit assertion `assertThat(event.precision()).isEqualTo(DatePrecision.YEAR)` listed. This should be added to that test's assertion block. Recommend adding an explicit assertion rather than leaving it implicit. **Structural invariants are correctly required within each test** — asserting `derived: true`, `type = PERSONAL`, `derivedType` in every event test prevents "assertion tourism" where a dedicated test passes while other tests silently produce wrong invariant values. **The `should_assume_no_self_spouse_edge_per_db_constraint` test** is an assumption test, not a behavior test. It should use `Assumptions.assumeThat(...)` from AssertJ or a comment explaining this is a documentation test, not a guard. If it uses `assertThat`, a developer might think the code guards against self-edges (it doesn't — the DB constraint does). **Missing: `assembleDerivedEvents()` with an empty list of family members** should yield zero events — trivial but ensures the orchestrator doesn't NPE on empty input. This is a boundary condition the issue doesn't list. **Test layer placement is correct** — pure Mockito unit tests, no Spring context, no Testcontainers. The issue correctly assigns migration tests to #2. CI time impact of this test class: ~50ms. **The `@Transactional(readOnly = true)` consideration**: since these are pure unit tests with mocked services, transaction behavior is irrelevant in the test class. The service-level annotation matters only in integration context (#5's tests). ### Recommendations 1. **Add explicit `assertThat(event.precision()).isEqualTo(DatePrecision.YEAR)` in `should_emit_one_heirat_for_spouse_edge_with_fromYear`** — the Heirat YEAR precision is an acceptance criterion that needs an explicit assertion, not just an implicit one. 2. **Add `should_emit_zero_events_when_family_member_list_is_empty`** as a boundary test for the orchestrator. 3. **Use `assertThat` comment or restructure `should_assume_no_self_spouse_edge_per_db_constraint`** to make clear it's a documentation test (asserting what the DB guarantees), not a behavior test (asserting what the service guards). Consider renaming to `self_spouse_edge_does_not_exist_per_db_constraint_documentation`. ### Open Decisions None. Test strategy and pyramid placement are clearly specified.

marcel commented

2026-06-07 19:54:51 +02:00

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist

Observations

This is a pure backend issue — it delivers the assembly layer that #6/#7 will consume. There is no UI to review here. I've read the issue to understand what contract the frontend issues (#6, #7, #10) will receive, and I have observations relevant to those downstream issues.

The derivedType discriminator (BIRTH/DEATH/MARRIAGE) with two name fields is the right contract for localized label composition. The frontend receives primaryPersonName + optional relatedPersonName (for Heirat) and composes the label via i18n keys like timeline.derived.marriage({nameA}, {nameB}). This is correct — it allows all three locales (de/en/es) to format names in their natural order. The German "Heirat: Anna Müller & Hans Schmidt" and English "Marriage: Anna Müller & Hans Schmidt" are produced without server involvement.

Precision pass-through to the frontend is the right design. DatePrecision.DAY staying DAY means the dateLabel.ts helper in #6 can produce "28. Juli 1914" (DAY precision birth), "1923" (YEAR precision Heirat), or "Ohne Datum" (UNKNOWN null-date Heirat). The rendering table in the spec maps these correctly. The frontend must handle null eventDate with UNKNOWN precision — the "Ohne Datum" bucket rendering path in YearBand.svelte must handle this gracefully (no crash when eventDate is null).

The relatedPersonName: String (nullable) for Heirat events — when null (for Geburt/Tod), the frontend must not render any separator or "& null" text. EventCard.svelte (#7) should use {#if relatedPersonName} to conditionally render the second name. This is a UX correctness requirement, not just a formatting nicety.

Heirat events with eventDate = null land in the "Ohne Datum" bucket. For the senior audience (60+), this bucket should render with clear visual separation from the year-dated events, and the Heirat label should read "Heirat (Datum unbekannt)" rather than just "Heirat" — otherwise the event looks incomplete or broken. This label formatting is #6's responsibility, but the contract from this issue (null eventDate, UNKNOWN precision) enables it.

No i18n keys are defined in this issue — they are deferred to #6/#7. However, the derivedType enum values (BIRTH/DEATH/MARRIAGE) should be documented now as the exact keys the frontend will key off of, so #6 doesn't invent different names. Recommend adding a comment or note in the issue confirming the i18n key naming pattern: timeline.derived.birth, timeline.derived.death, timeline.derived.marriage — and that these are defined in #6.

No touch target or accessibility concerns at this layer. This issue produces data DTOs, not UI.

Recommendations

Confirm i18n key naming convention now (timeline.derived.birth, .death, .marriage with {name} / {nameA}, {nameB} slots) so #6 and #7 implement consistent keys without coordination overhead.
Note in #7's acceptance criteria (downstream issue): EventCard.svelte must handle relatedPersonName = null gracefully — no separator rendered when absent.
Note in #6/#7: "Ohne Datum" bucket must handle eventDate = null without a JavaScript crash — new Date(null) is not an error but produces epoch, which is wrong. Use conditional rendering, not default date construction.

Open Decisions

None from my angle — the DTO contract is clear and the rendering decisions are correctly deferred to #6/#7.

## 🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist ### Observations This is a pure backend issue — it delivers the assembly layer that #6/#7 will consume. There is no UI to review here. I've read the issue to understand what contract the frontend issues (#6, #7, #10) will receive, and I have observations relevant to those downstream issues. **The `derivedType` discriminator (`BIRTH`/`DEATH`/`MARRIAGE`) with two name fields is the right contract for localized label composition.** The frontend receives `primaryPersonName` + optional `relatedPersonName` (for Heirat) and composes the label via i18n keys like `timeline.derived.marriage({nameA}, {nameB})`. This is correct — it allows all three locales (de/en/es) to format names in their natural order. The German "Heirat: Anna Müller & Hans Schmidt" and English "Marriage: Anna Müller & Hans Schmidt" are produced without server involvement. **Precision pass-through to the frontend is the right design.** `DatePrecision.DAY` staying `DAY` means the `dateLabel.ts` helper in #6 can produce "28. Juli 1914" (DAY precision birth), "1923" (YEAR precision Heirat), or "Ohne Datum" (UNKNOWN null-date Heirat). The rendering table in the spec maps these correctly. The frontend must handle `null` `eventDate` with `UNKNOWN` precision — the "Ohne Datum" bucket rendering path in `YearBand.svelte` must handle this gracefully (no crash when `eventDate` is null). **The `relatedPersonName: String (nullable)` for Heirat events** — when null (for Geburt/Tod), the frontend must not render any separator or "& null" text. `EventCard.svelte` (#7) should use `{#if relatedPersonName}` to conditionally render the second name. This is a UX correctness requirement, not just a formatting nicety. **Heirat events with `eventDate = null` land in the "Ohne Datum" bucket.** For the senior audience (60+), this bucket should render with clear visual separation from the year-dated events, and the Heirat label should read "Heirat (Datum unbekannt)" rather than just "Heirat" — otherwise the event looks incomplete or broken. This label formatting is #6's responsibility, but the contract from this issue (null `eventDate`, `UNKNOWN` precision) enables it. **No i18n keys are defined in this issue** — they are deferred to #6/#7. However, the `derivedType` enum values (`BIRTH`/`DEATH`/`MARRIAGE`) should be documented now as the exact keys the frontend will key off of, so #6 doesn't invent different names. Recommend adding a comment or note in the issue confirming the i18n key naming pattern: `timeline.derived.birth`, `timeline.derived.death`, `timeline.derived.marriage` — and that these are defined in #6. **No touch target or accessibility concerns at this layer.** This issue produces data DTOs, not UI. ### Recommendations 1. **Confirm i18n key naming convention now** (`timeline.derived.birth`, `.death`, `.marriage` with `{name}` / `{nameA}`, `{nameB}` slots) so #6 and #7 implement consistent keys without coordination overhead. 2. **Note in #7's acceptance criteria** (downstream issue): `EventCard.svelte` must handle `relatedPersonName = null` gracefully — no separator rendered when absent. 3. **Note in #6/#7**: "Ohne Datum" bucket must handle `eventDate = null` without a JavaScript crash — `new Date(null)` is not an error but produces epoch, which is wrong. Use conditional rendering, not default date construction. ### Open Decisions None from my angle — the DTO contract is clear and the rendering decisions are correctly deferred to #6/#7.

marcel commented

2026-06-07 19:55:08 +02:00

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

This issue introduces no new infrastructure — no new Docker services, no new environment variables, no new Flyway migrations (those belong to #2). The operational footprint of this issue is zero. From a platform perspective, this is clean.

The N+1 prohibition is the most operationally relevant concern. The issue mandates two bulk fetches (findAllFamilyMembers() + findAllSpouseEdges()). I've verified both paths: PersonRepository.findByFamilyMemberTrueOrderByLastNameAscFirstNameAsc() is a derived query (no explicit @Query) — it will fetch all Person fields eagerly but not nameAliases (which is LAZY). Since assembleDerivedEvents() only calls getDisplayName() (reads title, firstName, lastName — all column fields, no lazy associations), this is safe and produces exactly 2 SQL queries regardless of family size. The findAllByRelationTypeIn query has JOIN FETCH on both person sides — confirmed in the repository. Good.

npm run generate:api after adding derivedType and name fields — this is listed as a task and is operationally correct. The TypeScript types need regeneration before any frontend issue (#6, #7) starts. If the backend is not running when the developer starts #6, they cannot regenerate. Consider adding a note that the dev stack must be up for this step, or that the generated types can be committed as part of this PR's changes.

No new Flyway migration in this issue — confirmed. The timeline/ package and TimelineEvent entity belong to #2. This issue correctly avoids touching the schema.

The @Slf4j + parameterized logging requirement (logger.debug(...) with counts) introduces no operational overhead — DEBUG is typically disabled in production. Confirming this doesn't need a dedicated production log-level check: the existing logging configuration in application.yaml / application-prod.yaml should already suppress DEBUG for the timeline package (it will, by default — Spring Boot sets the root logger to INFO).

CI time impact — pure Mockito unit tests run in milliseconds. No new Testcontainers, no new @SpringBootTest. This issue adds ~50ms to the test suite at most. No CI pipeline changes needed.

One operational edge case to be aware of for #5: when assembleDerivedEvents() is called from the GET /api/timeline endpoint and the Person table has many family members, the response time is bounded by two sequential DB queries. For a family of ~500 persons (realistic ceiling for a family archive), this is still well within the p95 < 500ms SLA. No caching or optimization needed for MVP.

Recommendations

Add a note to the Tasks that the generated TypeScript types (from npm run generate:api) should be committed as part of this PR — not left for #6 to discover they need to regenerate. This prevents a "types are stale" CI failure on the frontend issues.
No infrastructure changes required. No Compose changes, no new environment variables, no CI pipeline modifications.

Open Decisions

None. This issue has zero platform footprint beyond the code it adds.

## 🚀 Tobias Wendt — DevOps & Platform Engineer ### Observations **This issue introduces no new infrastructure** — no new Docker services, no new environment variables, no new Flyway migrations (those belong to #2). The operational footprint of this issue is zero. From a platform perspective, this is clean. **The N+1 prohibition is the most operationally relevant concern.** The issue mandates two bulk fetches (`findAllFamilyMembers()` + `findAllSpouseEdges()`). I've verified both paths: `PersonRepository.findByFamilyMemberTrueOrderByLastNameAscFirstNameAsc()` is a derived query (no explicit `@Query`) — it will fetch all Person fields eagerly but not `nameAliases` (which is `LAZY`). Since `assembleDerivedEvents()` only calls `getDisplayName()` (reads `title`, `firstName`, `lastName` — all column fields, no lazy associations), this is safe and produces exactly 2 SQL queries regardless of family size. The `findAllByRelationTypeIn` query has JOIN FETCH on both person sides — confirmed in the repository. Good. **`npm run generate:api` after adding `derivedType` and name fields** — this is listed as a task and is operationally correct. The TypeScript types need regeneration before any frontend issue (#6, #7) starts. If the backend is not running when the developer starts #6, they cannot regenerate. Consider adding a note that the dev stack must be up for this step, or that the generated types can be committed as part of this PR's changes. **No new Flyway migration in this issue** — confirmed. The `timeline/` package and `TimelineEvent` entity belong to #2. This issue correctly avoids touching the schema. **The `@Slf4j` + parameterized logging requirement** (`logger.debug(...)` with counts) introduces no operational overhead — `DEBUG` is typically disabled in production. Confirming this doesn't need a dedicated production log-level check: the existing logging configuration in `application.yaml` / `application-prod.yaml` should already suppress `DEBUG` for the timeline package (it will, by default — Spring Boot sets the root logger to `INFO`). **CI time impact** — pure Mockito unit tests run in milliseconds. No new Testcontainers, no new `@SpringBootTest`. This issue adds ~50ms to the test suite at most. No CI pipeline changes needed. **One operational edge case to be aware of for #5:** when `assembleDerivedEvents()` is called from the `GET /api/timeline` endpoint and the Person table has many family members, the response time is bounded by two sequential DB queries. For a family of ~500 persons (realistic ceiling for a family archive), this is still well within the p95 < 500ms SLA. No caching or optimization needed for MVP. ### Recommendations 1. **Add a note to the Tasks** that the generated TypeScript types (from `npm run generate:api`) should be committed as part of this PR — not left for #6 to discover they need to regenerate. This prevents a "types are stale" CI failure on the frontend issues. 2. **No infrastructure changes required.** No Compose changes, no new environment variables, no CI pipeline modifications. ### Open Decisions None. This issue has zero platform footprint beyond the code it adds.

marcel commented

2026-06-07 19:55:36 +02:00

📋 Elicit — Requirements Engineer

Observations

The specification is unusually complete for a backend service layer issue. All 11 resolved decisions are documented with rationale. Acceptance criteria map 1:1 to test cases. Dependencies are explicit and the compile-blocking dependency on #1 is called out. This is closer to an IEEE 830 SRS section than a typical feature issue.

One ambiguity in the acceptance criteria: "A married couple (one SPOUSE_OF edge) yields exactly one Heirat event, even when both spouses are in the queried scope." This criterion conflates two distinct behaviors:

(a) One SPOUSE_OF edge → one Heirat (dedup from the edge list)
(b) "even when both spouses are in the queried scope" — implies the scope (family member list) is not the dedup key; the relationship row id is.

The test should_emit_exactly_one_heirat_when_both_spouses_in_scope tests (b). But what happens if findAllFamilyMembers() returns spouse A but findAllSpouseEdges() returns an edge where spouse B has family_member = false? This is an edge case the spec hasn't addressed: is a Heirat emitted when one spouse is a family member and the other is not?

Looking at addRelationship() in RelationshipService: when a SPOUSE_OF edge is added, both endpoints are flagged family_member = true. So in practice, a stored SPOUSE_OF edge implies both spouses are family members. However, it's possible (via direct Person edits or import quirks) to have a SPOUSE_OF edge where one endpoint has family_member = false. The spec is silent on this. Recommendation: add an acceptance criterion — "A SPOUSE_OF edge where one spouse has family_member = false emits [one Heirat / zero Heirat] — clarify the rule."

The "scope of derived events" says family members only — so if the assembly checks family_member only on the Person list (from findAllFamilyMembers()), and the spouse edges include persons not in that list, buildMarriageEvents() would still emit the Heirat with both display names (pulled directly from the edge, not filtered by the family member list). This is a scope ambiguity: is the family-member filter applied to persons (births/deaths) but NOT to marriage participants, or to both?

The Tasks section includes a pre-condition check ("confirm TimelineEntryDTO.id is String") but no equivalent check for derivedType not being present in #2/#3's DTO definition. Since this issue adds derivedType, there may be a coordination gap if #2/#3 define TimelineEntryDTO without derivedType fields — the issue should be implemented on top of #2/#3's branch, not independently.

The branch dependency note is precise and correct. The note "cut the feature branch from #1's merged state" is implementation-ready guidance. The equivalent for #2/#3 dependency is implied but not stated as explicitly: "cut this branch from #3's merged state or rebase onto it."

i18n scope: The issue correctly identifies that derivedType drives the frontend i18n keys. However, the i18n keys themselves (timeline.derived.birth, etc.) are not defined in this issue's tasks — they're deferred to #6. This is fine, but the key naming convention should be locked here so #6 doesn't invent a different pattern (e.g., timeline.event.birth vs timeline.derived.birth). A one-line note in this issue would prevent a cross-issue naming mismatch.

Recommendations

Add an acceptance criterion addressing the mixed-membership Heirat edge case: does a SPOUSE_OF edge where one spouse is family_member = false produce a Heirat event or not? The current addRelationship() logic makes this unlikely in practice, but it's an unstated assumption. My recommendation: emit the Heirat regardless of spouse B's family_member flag (the edge is the source of truth, and the edge's display names come from the entity directly), but document this as a deliberate rule.
Clarify the branch dependency on #2/#3 symmetrically with the #1 dependency: "Cut this branch from #3's merged state; TimelineEntryDTO and DerivedEventType must exist before this issue's code compiles."
Lock the i18n key naming convention in this issue: timeline.derived.{birth|death|marriage} with {name} / {nameA}, {nameB} — so #6 implements what #4 implies without a coordination meeting.

Open Decisions

Mixed-membership Heirat: when one spouse has family_member = false, does buildMarriageEvents() emit a Heirat? The spec is silent. My read of the design intent: emit it (the edge is curated data; filtering marriage partners by family_member would hide real marriages). But this needs explicit confirmation before implementation, since a test must be written for it.

## 📋 Elicit — Requirements Engineer ### Observations **The specification is unusually complete for a backend service layer issue.** All 11 resolved decisions are documented with rationale. Acceptance criteria map 1:1 to test cases. Dependencies are explicit and the compile-blocking dependency on #1 is called out. This is closer to an IEEE 830 SRS section than a typical feature issue. **One ambiguity in the acceptance criteria:** "A married couple (one `SPOUSE_OF` edge) yields **exactly one** Heirat event, even when both spouses are in the queried scope." This criterion conflates two distinct behaviors: - **(a)** One `SPOUSE_OF` edge → one Heirat (dedup from the edge list) - **(b)** "even when both spouses are in the queried scope" — implies the scope (family member list) is not the dedup key; the relationship row id is. The test `should_emit_exactly_one_heirat_when_both_spouses_in_scope` tests (b). But what happens if `findAllFamilyMembers()` returns spouse A but `findAllSpouseEdges()` returns an edge where spouse B has `family_member = false`? This is an edge case the spec hasn't addressed: **is a Heirat emitted when one spouse is a family member and the other is not?** Looking at `addRelationship()` in `RelationshipService`: when a `SPOUSE_OF` edge is added, both endpoints are flagged `family_member = true`. So in practice, a stored `SPOUSE_OF` edge implies both spouses are family members. However, it's possible (via direct Person edits or import quirks) to have a `SPOUSE_OF` edge where one endpoint has `family_member = false`. The spec is silent on this. **Recommendation: add an acceptance criterion** — "A `SPOUSE_OF` edge where one spouse has `family_member = false` emits [one Heirat / zero Heirat] — clarify the rule." **The "scope of derived events" says family members only** — so if the assembly checks `family_member` only on the Person list (from `findAllFamilyMembers()`), and the spouse edges include persons not in that list, `buildMarriageEvents()` would still emit the Heirat with both display names (pulled directly from the edge, not filtered by the family member list). This is a scope ambiguity: is the family-member filter applied to persons (births/deaths) but NOT to marriage participants, or to both? **The Tasks section includes a pre-condition check** ("confirm `TimelineEntryDTO.id` is `String`") but no equivalent check for `derivedType` not being present in #2/#3's DTO definition. Since this issue *adds* `derivedType`, there may be a coordination gap if #2/#3 define `TimelineEntryDTO` without `derivedType` fields — the issue should be implemented on top of #2/#3's branch, not independently. **The branch dependency note is precise and correct.** The note "cut the feature branch from #1's merged state" is implementation-ready guidance. The equivalent for #2/#3 dependency is implied but not stated as explicitly: "cut this branch from #3's merged state or rebase onto it." **i18n scope:** The issue correctly identifies that `derivedType` drives the frontend i18n keys. However, the i18n keys themselves (`timeline.derived.birth`, etc.) are not defined in this issue's tasks — they're deferred to #6. This is fine, but the key naming convention should be locked here so #6 doesn't invent a different pattern (e.g., `timeline.event.birth` vs `timeline.derived.birth`). A one-line note in this issue would prevent a cross-issue naming mismatch. ### Recommendations 1. **Add an acceptance criterion addressing the mixed-membership Heirat edge case**: does a `SPOUSE_OF` edge where one spouse is `family_member = false` produce a Heirat event or not? The current `addRelationship()` logic makes this unlikely in practice, but it's an unstated assumption. My recommendation: emit the Heirat regardless of spouse B's `family_member` flag (the edge is the source of truth, and the edge's display names come from the entity directly), but document this as a deliberate rule. 2. **Clarify the branch dependency on #2/#3** symmetrically with the #1 dependency: "Cut this branch from #3's merged state; `TimelineEntryDTO` and `DerivedEventType` must exist before this issue's code compiles." 3. **Lock the i18n key naming convention in this issue**: `timeline.derived.{birth|death|marriage}` with `{name}` / `{nameA}`, `{nameB}` — so #6 implements what #4 implies without a coordination meeting. ### Open Decisions - **Mixed-membership Heirat**: when one spouse has `family_member = false`, does `buildMarriageEvents()` emit a Heirat? The spec is silent. My read of the design intent: emit it (the edge is curated data; filtering marriage partners by `family_member` would hide real marriages). But this needs explicit confirmation before implementation, since a test must be written for it.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#776

Timeline: derive person life-events (Geburt/Tod/Heirat) #776

Context

Scope

Architecture & layering

TimelineEntryDTO contract

Synthetic ids

Symmetric-marriage dedup

Logging

Resolved decisions

Tasks

Acceptance criteria

Tests

🏗️ Markus Keller — Senior Application Architect

Observations

Recommendations

Open Decisions

👨‍💻 Felix Brandt — Senior Fullstack Developer

Observations

Recommendations

Open Decisions

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

Recommendations

Open Decisions

🧪 Sara Holt — QA Engineer & Test Strategist

Observations

Recommendations

Open Decisions

🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist

Observations

Recommendations

Open Decisions

🚀 Tobias Wendt — DevOps & Platform Engineer

Observations

Recommendations

Open Decisions

📋 Elicit — Requirements Engineer

Observations

Recommendations

Open Decisions

`TimelineEntryDTO` contract