marcel/familienarchiv
1
0
Fork 0
Code Issues 119 Pull Requests 2 Actions Packages Projects Releases Wiki Activity

refactor: preparatory infrastructure for PersonNameParser enhancements (#209-#212) #213

New Issue
Closed
opened 2026-04-07 18:50:13 +02:00 by marcel · 8 comments
marcel commented 2026-04-07 18:50:13 +02:00
Owner
Copy Link

Context

Issues #209, #210, #211, and #212 all modify PersonNameParser.split() and the Person entity. Each adds a pipeline step and a field. Doing them independently causes migration collisions, repeated SplitName record changes, and frontend churn.

This issue lays the shared foundation so #209-#212 become clean, additive feature issues.

Scope

Part 1 - Redesign SplitName record

Current (PersonNameParser.java:26):

public record SplitName(String firstName, String lastName) {}

Target:

public record SplitName(
    String title,       // #212: "Dr.", "Tante", "Frau" — null when absent
    String firstName,   // nullable after #212
    String lastName,
    String maidenName,  // #209: extracted from geb pattern — null when absent
    String annotation   // #210: raw parenthesized content — null when absent
) {}

Call sites to update (1 production, 13 test):

  • PersonService.findOrCreateByAlias() (line 64) — only consumer
  • PersonNameParserTest.java — 13 test methods calling split()

All existing call sites pass null for the new fields initially. Each feature issue populates its field.

Part 2 - Extract split() pipeline into named methods

Current split() is a monolithic method. Extract each normalization step into a named method and compose them as a pipeline with explicit ordering:

public static SplitName split(String rawName) {
    if (rawName == null || rawName.isBlank()) return new SplitName(null, "?", "?", null, null);

    MaidenNameResult maiden = stripMaidenName(rawName);        // #209
    String cleaned = maiden.cleaned();

    cleaned = normalizeDotCompressed(cleaned);                  // #190

    AnnotationResult paren = stripAnnotation(cleaned);          // #210
    cleaned = paren.cleaned();

    TitleResult title = stripTitle(cleaned);                    // #212
    cleaned = title.cleaned();

    NameParts parts = splitByKnownLastNameOrFallback(cleaned);  // existing logic

    return new SplitName(
        title.title(), parts.firstName(), parts.lastName(),
        maiden.maidenName(), paren.annotation()
    );
}

Initially each extracted method is a pass-through (returns input unchanged). Each feature issue fills in its logic.

Part 3 - Person entity changes

Add fields:

@Column(name = "title")
private String title;

@Enumerated(EnumType.STRING)
@Column(name = "person_type", nullable = false)
@Builder.Default
private PersonType personType = PersonType.PERSON;

Make firstName nullable:

@Column(nullable = true)   // was nullable = false
private String firstName;   // remove @Schema(requiredMode = REQUIRED)

Part 4 - PersonType enum

public enum PersonType {
    PERSON,
    INSTITUTION,
    GROUP,
    UNKNOWN
}

Part 5 - PersonNameAliasType additions

Add to existing enum:

MAIDEN_NAME  // #209

Part 6 - Migration

-- Add title column
ALTER TABLE persons ADD COLUMN title VARCHAR(50);

-- Add person_type column with default
ALTER TABLE persons ADD COLUMN person_type VARCHAR(20) NOT NULL DEFAULT 'PERSON';
ALTER TABLE persons ADD CONSTRAINT chk_person_type
    CHECK (person_type IN ('PERSON', 'INSTITUTION', 'GROUP', 'UNKNOWN'));

-- Make first_name nullable
ALTER TABLE persons ALTER COLUMN first_name DROP NOT NULL;

Single migration file. Version number must be coordinated (next after current max).

Part 7 - Person.getDisplayName() — single source of truth

@Transient
public String getDisplayName() {
    StringBuilder sb = new StringBuilder();
    if (title != null) sb.append(title).append(" ");
    if (firstName != null) sb.append(firstName).append(" ");
    sb.append(lastName);
    return sb.toString().trim();
}

Exposed via @Schema so it appears in the generated API types.

Part 8 - Frontend: replace all name concatenation with displayName

17+ files currently concatenate firstName + " " + lastName directly. After adding displayName to the API response, all of these become person.displayName:

Components (display):

File Current pattern Change
PersonChip.svelte ${person.firstName} ${person.lastName} ${person.displayName}
PersonCard.svelte {person.firstName} {person.lastName} {person.displayName}
PersonMultiSelect.svelte {person.lastName}, {person.firstName} {person.displayName}
PersonTypeahead.svelte ${person.firstName} ${person.lastName} ${person.displayName}
DocumentMetadataDrawer.svelte ${person.firstName} ${person.lastName} ${person.displayName}
DocumentList.svelte {doc.sender.firstName} {doc.sender.lastName} {doc.sender.displayName}
CorrespondentSuggestionsDropdown.svelte ${person.firstName.charAt(0)}${person.lastName.charAt(0)} initials helper
ConversationTimeline.svelte (both) ${r.firstName} ${r.lastName} ${r.displayName}
MentionEditor.svelte ${user.firstName} ${user.lastName} ${user.displayName}
OverflowPillButton.svelte type definition update type
PersonChipRow.svelte type definition update type
DashboardRecentDocuments.svelte type definition update type
DocumentTopBar.svelte type definition update type
persons/+page.svelte initials from firstName/lastName initials helper
persons/[id]/+page.svelte ${receiver.firstName} ${receiver.lastName} ${receiver.displayName}
users/[id]/+page.svelte conditional check use displayName

Utilities:

File Current pattern Change
personFormat.ts abbreviateName() / abbreviateCompact() update to use displayName or handle null firstName
mention.ts ${user.firstName} ${user.lastName} use displayName

Server files:

File Current pattern Change
briefwechsel/+page.server.ts ${p.firstName} ${p.lastName} ${p.displayName}
conversations/+page.server.ts ${p.firstName} ${p.lastName} ${p.displayName}
+page.server.ts (root) ${senderObj.firstName} ${senderObj.lastName} ${senderObj.displayName}
documents/new/+page.server.ts ${data.firstName} ${data.lastName} use displayName

Initials helper — extract a utility function:

function getInitials(person: { displayName: string; firstName?: string; lastName: string }): string {
    if (person.firstName) return person.firstName[0] + person.lastName[0];
    return person.lastName.substring(0, 2).toUpperCase();
}

Part 9 - Regenerate API types

After backend changes, run npm run generate:api to get the new displayName, title, personType fields and nullable firstName into the TypeScript types.

Part 10 - i18n keys

Add translation keys for:

  • PersonType values: PERSON, INSTITUTION, GROUP, UNKNOWN
  • PersonNameAliasType.MAIDEN_NAME: de "Geburtsname", en "Maiden name", es "Apellido de soltera"

What this does NOT include

  • GEB pattern widening (#209)
  • Parenthesis stripping logic (#210)
  • PersonType classification heuristics (#211)
  • Title stripping logic (#212)
  • Von-column multi-person splitting (raised in #211, separate issue)

These become clean, additive changes after this foundation lands.

Depends on

  • #190 (// separator + dot-compressed) — must be merged first (provides dot-normalization step)

Unblocks

Design decisions

  • displayName is computed, not stored — @Transient getter on the entity, exposed via OpenAPI. No database column. Single source of truth.
  • Nullable firstName — honest representation of unknown data. No more "?" placeholders for new records. Existing "?" records can be cleaned up in a data migration later.
  • Pipeline as named methods — each step is independently testable. Ordering is explicit in the split() method body.
  • Non-persons use lastName for full name, firstName = null — no new displayName column needed. getDisplayName() handles this naturally.
## Context Issues #209, #210, #211, and #212 all modify `PersonNameParser.split()` and the `Person` entity. Each adds a pipeline step and a field. Doing them independently causes migration collisions, repeated `SplitName` record changes, and frontend churn. This issue lays the shared foundation so #209-#212 become clean, additive feature issues. ## Scope ### Part 1 - Redesign `SplitName` record Current (`PersonNameParser.java:26`): ```java public record SplitName(String firstName, String lastName) {} ``` Target: ```java public record SplitName( String title, // #212: "Dr.", "Tante", "Frau" — null when absent String firstName, // nullable after #212 String lastName, String maidenName, // #209: extracted from geb pattern — null when absent String annotation // #210: raw parenthesized content — null when absent ) {} ``` **Call sites to update (1 production, 13 test):** - `PersonService.findOrCreateByAlias()` (line 64) — only consumer - `PersonNameParserTest.java` — 13 test methods calling `split()` All existing call sites pass `null` for the new fields initially. Each feature issue populates its field. ### Part 2 - Extract `split()` pipeline into named methods Current `split()` is a monolithic method. Extract each normalization step into a named method and compose them as a pipeline with explicit ordering: ```java public static SplitName split(String rawName) { if (rawName == null || rawName.isBlank()) return new SplitName(null, "?", "?", null, null); MaidenNameResult maiden = stripMaidenName(rawName); // #209 String cleaned = maiden.cleaned(); cleaned = normalizeDotCompressed(cleaned); // #190 AnnotationResult paren = stripAnnotation(cleaned); // #210 cleaned = paren.cleaned(); TitleResult title = stripTitle(cleaned); // #212 cleaned = title.cleaned(); NameParts parts = splitByKnownLastNameOrFallback(cleaned); // existing logic return new SplitName( title.title(), parts.firstName(), parts.lastName(), maiden.maidenName(), paren.annotation() ); } ``` Initially each extracted method is a pass-through (returns input unchanged). Each feature issue fills in its logic. ### Part 3 - `Person` entity changes **Add fields:** ```java @Column(name = "title") private String title; @Enumerated(EnumType.STRING) @Column(name = "person_type", nullable = false) @Builder.Default private PersonType personType = PersonType.PERSON; ``` **Make `firstName` nullable:** ```java @Column(nullable = true) // was nullable = false private String firstName; // remove @Schema(requiredMode = REQUIRED) ``` ### Part 4 - `PersonType` enum ```java public enum PersonType { PERSON, INSTITUTION, GROUP, UNKNOWN } ``` ### Part 5 - `PersonNameAliasType` additions Add to existing enum: ```java MAIDEN_NAME // #209 ``` ### Part 6 - Migration ```sql -- Add title column ALTER TABLE persons ADD COLUMN title VARCHAR(50); -- Add person_type column with default ALTER TABLE persons ADD COLUMN person_type VARCHAR(20) NOT NULL DEFAULT 'PERSON'; ALTER TABLE persons ADD CONSTRAINT chk_person_type CHECK (person_type IN ('PERSON', 'INSTITUTION', 'GROUP', 'UNKNOWN')); -- Make first_name nullable ALTER TABLE persons ALTER COLUMN first_name DROP NOT NULL; ``` Single migration file. Version number must be coordinated (next after current max). ### Part 7 - `Person.getDisplayName()` — single source of truth ```java @Transient public String getDisplayName() { StringBuilder sb = new StringBuilder(); if (title != null) sb.append(title).append(" "); if (firstName != null) sb.append(firstName).append(" "); sb.append(lastName); return sb.toString().trim(); } ``` Exposed via `@Schema` so it appears in the generated API types. ### Part 8 - Frontend: replace all name concatenation with `displayName` **17+ files** currently concatenate `firstName + " " + lastName` directly. After adding `displayName` to the API response, all of these become `person.displayName`: **Components (display):** | File | Current pattern | Change | |---|---|---| | `PersonChip.svelte` | `${person.firstName} ${person.lastName}` | `${person.displayName}` | | `PersonCard.svelte` | `{person.firstName} {person.lastName}` | `{person.displayName}` | | `PersonMultiSelect.svelte` | `{person.lastName}, {person.firstName}` | `{person.displayName}` | | `PersonTypeahead.svelte` | `${person.firstName} ${person.lastName}` | `${person.displayName}` | | `DocumentMetadataDrawer.svelte` | `${person.firstName} ${person.lastName}` | `${person.displayName}` | | `DocumentList.svelte` | `{doc.sender.firstName} {doc.sender.lastName}` | `{doc.sender.displayName}` | | `CorrespondentSuggestionsDropdown.svelte` | `${person.firstName.charAt(0)}${person.lastName.charAt(0)}` | initials helper | | `ConversationTimeline.svelte` (both) | `${r.firstName} ${r.lastName}` | `${r.displayName}` | | `MentionEditor.svelte` | `${user.firstName} ${user.lastName}` | `${user.displayName}` | | `OverflowPillButton.svelte` | type definition | update type | | `PersonChipRow.svelte` | type definition | update type | | `DashboardRecentDocuments.svelte` | type definition | update type | | `DocumentTopBar.svelte` | type definition | update type | | `persons/+page.svelte` | initials from firstName/lastName | initials helper | | `persons/[id]/+page.svelte` | `${receiver.firstName} ${receiver.lastName}` | `${receiver.displayName}` | | `users/[id]/+page.svelte` | conditional check | use displayName | **Utilities:** | File | Current pattern | Change | |---|---|---| | `personFormat.ts` | `abbreviateName()` / `abbreviateCompact()` | update to use displayName or handle null firstName | | `mention.ts` | `${user.firstName} ${user.lastName}` | use displayName | **Server files:** | File | Current pattern | Change | |---|---|---| | `briefwechsel/+page.server.ts` | `${p.firstName} ${p.lastName}` | `${p.displayName}` | | `conversations/+page.server.ts` | `${p.firstName} ${p.lastName}` | `${p.displayName}` | | `+page.server.ts` (root) | `${senderObj.firstName} ${senderObj.lastName}` | `${senderObj.displayName}` | | `documents/new/+page.server.ts` | `${data.firstName} ${data.lastName}` | use displayName | **Initials helper** — extract a utility function: ```typescript function getInitials(person: { displayName: string; firstName?: string; lastName: string }): string { if (person.firstName) return person.firstName[0] + person.lastName[0]; return person.lastName.substring(0, 2).toUpperCase(); } ``` ### Part 9 - Regenerate API types After backend changes, run `npm run generate:api` to get the new `displayName`, `title`, `personType` fields and nullable `firstName` into the TypeScript types. ### Part 10 - i18n keys Add translation keys for: - `PersonType` values: PERSON, INSTITUTION, GROUP, UNKNOWN - `PersonNameAliasType.MAIDEN_NAME`: de "Geburtsname", en "Maiden name", es "Apellido de soltera" ## What this does NOT include - GEB pattern widening (#209) - Parenthesis stripping logic (#210) - PersonType classification heuristics (#211) - Title stripping logic (#212) - Von-column multi-person splitting (raised in #211, separate issue) These become clean, additive changes after this foundation lands. ## Depends on - #190 (// separator + dot-compressed) — must be merged first (provides dot-normalization step) ## Unblocks - #209, #210, #211, #212 — all become additive after this ## Design decisions - **`displayName` is computed, not stored** — `@Transient` getter on the entity, exposed via OpenAPI. No database column. Single source of truth. - **Nullable `firstName`** — honest representation of unknown data. No more "?" placeholders for new records. Existing "?" records can be cleaned up in a data migration later. - **Pipeline as named methods** — each step is independently testable. Ordering is explicit in the `split()` method body. - **Non-persons use `lastName` for full name, `firstName = null`** — no new `displayName` column needed. `getDisplayName()` handles this naturally.
marcel added the personrefactor labels 2026-04-07 18:50:21 +02:00
marcel commented 2026-04-07 21:08:30 +02:00
Author
Owner
Copy Link

👨‍💻 Felix Brandt — Senior Fullstack Developer

Questions & Observations

  • Part 2 pipeline: pass-through methods need tests too. The issue says each extracted method is initially a pass-through. But the extraction itself is a refactor — existing tests must still pass after extracting stripMaidenName(), normalizeDotCompressed(), stripAnnotation(), stripTitle(), and splitByKnownLastNameOrFallback(). The refactor phase needs a "run all 35 existing tests, confirm green" step before any new behavior is added. This is the safety net.
  • SplitName constructor change touches 13 test call sites. Every test that calls split() and asserts on the result needs updating — not the assertion, but the expected record shape. With 5 fields, asserting result.firstName() and result.lastName() still works, but tests should also assert result.title() == null, result.maidenName() == null, etc. to document the contract that pass-through methods produce nulls.
  • The getInitials() helper — the proposed signature takes { displayName: string; firstName?: string; lastName: string }. But for non-persons (#211), lastName holds the full institution name. getInitials("Arthur Collignon GmbH") producing "AR" (first two chars) is awkward. Should non-persons show initials at all, or a type icon instead?
  • 17+ files is a lot of churn in one commit. The frontend displayName migration (Part 8) touches every component that displays a person name. This is the riskiest part of the issue — one missed file and "null Molly" shows up. Consider splitting Part 8 into its own commit with a grep -r "firstName.*lastName\|lastName.*firstName" frontend/src/ verification step.

Suggestions

  • TDD sequence: (1) Extract pipeline methods as pass-throughs, run existing 35 tests green. (2) Add SplitName fields, update test assertions. (3) Backend entity + migration. (4) Regenerate API types. (5) Frontend displayName migration. Each step is a commit.
  • For the frontend migration, use the generated TypeScript types as the guide — after regeneration, tsc will flag every place that accesses firstName on a type where it's now optional. Let the compiler find the misses.
  • The getInitials() helper should check personType (once available) and return a type icon placeholder for non-persons rather than trying to extract initials from an institution name.
## 👨‍💻 Felix Brandt — Senior Fullstack Developer ### Questions & Observations - **Part 2 pipeline: pass-through methods need tests too.** The issue says each extracted method is initially a pass-through. But the extraction itself is a refactor — existing tests must still pass after extracting `stripMaidenName()`, `normalizeDotCompressed()`, `stripAnnotation()`, `stripTitle()`, and `splitByKnownLastNameOrFallback()`. The refactor phase needs a "run all 35 existing tests, confirm green" step before any new behavior is added. This is the safety net. - **`SplitName` constructor change touches 13 test call sites.** Every test that calls `split()` and asserts on the result needs updating — not the assertion, but the expected record shape. With 5 fields, asserting `result.firstName()` and `result.lastName()` still works, but tests should also assert `result.title() == null`, `result.maidenName() == null`, etc. to document the contract that pass-through methods produce nulls. - **The `getInitials()` helper** — the proposed signature takes `{ displayName: string; firstName?: string; lastName: string }`. But for non-persons (#211), `lastName` holds the full institution name. `getInitials("Arthur Collignon GmbH")` producing "AR" (first two chars) is awkward. Should non-persons show initials at all, or a type icon instead? - **17+ files is a lot of churn in one commit.** The frontend `displayName` migration (Part 8) touches every component that displays a person name. This is the riskiest part of the issue — one missed file and "null Molly" shows up. Consider splitting Part 8 into its own commit with a `grep -r "firstName.*lastName\|lastName.*firstName" frontend/src/` verification step. ### Suggestions - TDD sequence: (1) Extract pipeline methods as pass-throughs, run existing 35 tests green. (2) Add `SplitName` fields, update test assertions. (3) Backend entity + migration. (4) Regenerate API types. (5) Frontend `displayName` migration. Each step is a commit. - For the frontend migration, use the generated TypeScript types as the guide — after regeneration, `tsc` will flag every place that accesses `firstName` on a type where it's now optional. Let the compiler find the misses. - The `getInitials()` helper should check `personType` (once available) and return a type icon placeholder for non-persons rather than trying to extract initials from an institution name.
marcel commented 2026-04-07 21:08:46 +02:00
Author
Owner
Copy Link

🏗️ Markus Keller — Application Architect

Questions & Observations

  • @Transient getDisplayName() and OpenAPI exposure. A @Transient getter is not automatically included in the OpenAPI spec by SpringDoc — it needs @Schema annotation to appear. Confirm the field is annotated with @Schema(accessMode = Schema.AccessMode.READ_ONLY) so it appears in responses but is not accepted in request bodies. Without this, the frontend TypeScript types won't include displayName.
  • SKIP value in PersonType enum. The #211 discussion decided to add SKIP to the enum, but the issue body lists only PERSON, INSTITUTION, GROUP, UNKNOWN. The migration's CHECK constraint must include SKIP if it's in the Java enum, even though it's never persisted — otherwise JPA could theoretically attempt an INSERT with SKIP and hit a constraint violation. Alternatively, if SKIP is truly never persisted, enforce that in the service layer and leave it out of the CHECK constraint. Either way, the enum and constraint must be aligned.
  • PersonMultiSelect currently shows lastName, firstName. Part 8 changes this to person.displayName. But displayName assembles as [title] [firstName] lastName — forward order. This changes the display format from "Cram, Clara" to "Clara Cram". Is that intentional? Some users may prefer the reversed format for alphabetical scanning. If so, consider a getDisplayNameReversed() or let the component format it locally.
  • The intermediate records (MaidenNameResult, AnnotationResult, TitleResult, NameParts) — are these inner records of PersonNameParser, or top-level classes? As internal pipeline types that no caller sees, they should be package-private inner records. Keep them out of the public API.

Suggestions

  • Add SKIP to the issue body's Part 4 enum definition so implementers don't miss it.
  • The intermediate pipeline records should be private inner records of PersonNameParser. They are implementation details, not API.
  • For the PersonMultiSelect display format change, decide upfront: does displayName replace ALL display patterns (including reversed), or do some components need their own formatting? If the answer is "displayName for everything," update the issue. If some components need reversed order, displayName is insufficient and those components need to keep accessing firstName/lastName directly.
## 🏗️ Markus Keller — Application Architect ### Questions & Observations - **`@Transient getDisplayName()` and OpenAPI exposure.** A `@Transient` getter is not automatically included in the OpenAPI spec by SpringDoc — it needs `@Schema` annotation to appear. Confirm the field is annotated with `@Schema(accessMode = Schema.AccessMode.READ_ONLY)` so it appears in responses but is not accepted in request bodies. Without this, the frontend TypeScript types won't include `displayName`. - **`SKIP` value in `PersonType` enum.** The #211 discussion decided to add `SKIP` to the enum, but the issue body lists only `PERSON, INSTITUTION, GROUP, UNKNOWN`. The migration's CHECK constraint must include `SKIP` if it's in the Java enum, even though it's never persisted — otherwise JPA could theoretically attempt an INSERT with `SKIP` and hit a constraint violation. Alternatively, if `SKIP` is truly never persisted, enforce that in the service layer and leave it out of the CHECK constraint. Either way, the enum and constraint must be aligned. - **`PersonMultiSelect` currently shows `lastName, firstName`.** Part 8 changes this to `person.displayName`. But `displayName` assembles as `[title] [firstName] lastName` — forward order. This changes the display format from "Cram, Clara" to "Clara Cram". Is that intentional? Some users may prefer the reversed format for alphabetical scanning. If so, consider a `getDisplayNameReversed()` or let the component format it locally. - **The intermediate records (`MaidenNameResult`, `AnnotationResult`, `TitleResult`, `NameParts`)** — are these inner records of `PersonNameParser`, or top-level classes? As internal pipeline types that no caller sees, they should be package-private inner records. Keep them out of the public API. ### Suggestions - Add `SKIP` to the issue body's Part 4 enum definition so implementers don't miss it. - The intermediate pipeline records should be `private` inner records of `PersonNameParser`. They are implementation details, not API. - For the `PersonMultiSelect` display format change, decide upfront: does `displayName` replace ALL display patterns (including reversed), or do some components need their own formatting? If the answer is "displayName for everything," update the issue. If some components need reversed order, `displayName` is insufficient and those components need to keep accessing `firstName`/`lastName` directly.
marcel commented 2026-04-07 21:08:59 +02:00
Author
Owner
Copy Link

🧪 Sara Holt — QA Engineer

Questions & Observations

  • This is a pure refactor — zero behavior change. The acceptance criterion is simple: all existing tests pass, no user-visible change in the UI. But the scope is wide (backend entity, migration, 17+ frontend files, API types). The risk isn't in any one change — it's in the combination. A missed file, a stale type, or a null-safety gap produces "null" or "undefined" in the UI.
  • No new tests are scoped in this issue. The pass-through methods don't add behavior, so no new tests are needed for them. But the frontend displayName migration introduces a new display contract — every component that previously showed firstName + " " + lastName now shows displayName. Component tests should verify: (a) displayName with title, firstName, and lastName all present, (b) displayName with null firstName, (c) displayName with null title. These three cases exercise the getDisplayName() logic end-to-end.
  • The getInitials() helper is new behavior. It needs unit tests: firstName present (two initials), firstName null (first two chars of lastName), empty/short lastName edge cases.
  • Migration test. The migration drops NOT NULL on first_name and adds two columns. A Testcontainers integration test should verify: (a) an existing person with non-null firstName is unaffected, (b) a new person can be inserted with null firstName, (c) a person can be inserted with person_type = 'INSTITUTION', (d) inserting person_type = 'INVALID' fails the CHECK constraint.

Suggestions

  • Add a npm run check step after the frontend migration to catch any TypeScript errors from nullable firstName or missing displayName references. This is the fastest way to find missed files.
  • Consider a Playwright smoke test: navigate to the person list, verify no "null" or "undefined" strings appear in any person name display. This catches frontend regressions that TypeScript can't (e.g., template interpolation of optional fields).
## 🧪 Sara Holt — QA Engineer ### Questions & Observations - **This is a pure refactor — zero behavior change.** The acceptance criterion is simple: all existing tests pass, no user-visible change in the UI. But the scope is wide (backend entity, migration, 17+ frontend files, API types). The risk isn't in any one change — it's in the combination. A missed file, a stale type, or a null-safety gap produces "null" or "undefined" in the UI. - **No new tests are scoped in this issue.** The pass-through methods don't add behavior, so no new tests are needed *for them*. But the frontend `displayName` migration introduces a new display contract — every component that previously showed `firstName + " " + lastName` now shows `displayName`. Component tests should verify: (a) `displayName` with title, firstName, and lastName all present, (b) `displayName` with null firstName, (c) `displayName` with null title. These three cases exercise the `getDisplayName()` logic end-to-end. - **The `getInitials()` helper is new behavior.** It needs unit tests: firstName present (two initials), firstName null (first two chars of lastName), empty/short lastName edge cases. - **Migration test.** The migration drops NOT NULL on `first_name` and adds two columns. A Testcontainers integration test should verify: (a) an existing person with non-null firstName is unaffected, (b) a new person can be inserted with null firstName, (c) a person can be inserted with `person_type = 'INSTITUTION'`, (d) inserting `person_type = 'INVALID'` fails the CHECK constraint. ### Suggestions - Add a `npm run check` step after the frontend migration to catch any TypeScript errors from nullable `firstName` or missing `displayName` references. This is the fastest way to find missed files. - Consider a Playwright smoke test: navigate to the person list, verify no "null" or "undefined" strings appear in any person name display. This catches frontend regressions that TypeScript can't (e.g., template interpolation of optional fields).
marcel commented 2026-04-07 21:09:13 +02:00
Author
Owner
Copy Link

🔒 Nora "NullX" Steiner — Security Engineer

Questions & Observations

  • displayName as a computed field in the API response. Since it's @Transient, it's read-only. But verify that Jackson/SpringDoc does not accept displayName in request bodies (e.g., PUT/PATCH on a Person). If it does, the value should be silently ignored — never let a client override a computed field. The @Schema(accessMode = READ_ONLY) annotation handles this.
  • title field accepts freeform strings. The migration defines VARCHAR(50) with no CHECK constraint on content. When #212 implements the edit form, input validation must be applied (max length, no HTML/script content). For this refactor issue, the field is only populated programmatically (from the parser), so no immediate risk. But flag it for #212.
  • personType CHECK constraint alignment with the Java enum. If the Java enum includes SKIP but the CHECK constraint doesn't, and a bug in the service layer accidentally persists a Person with type SKIP, the constraint will reject it — which is the correct behavior (defense in depth). This is actually good: the CHECK constraint acts as a safety net for the service-layer invariant. Leave SKIP out of the CHECK constraint intentionally.
  • Nullable firstName and search queries. Any JPQL query with WHERE p.firstName LIKE :query will now exclude persons with null firstName from search results. Verify that DocumentSpecifications and any person search code handles null firstName with OR p.firstName IS NULL where appropriate.

Suggestions

  • No security blockers. The main note: when the edit form for title is added in #212, apply @Size(max = 50) and @Pattern validation to reject HTML/script content. Not needed in this issue since title is only set by the parser.
  • Intentionally omit SKIP from the DB CHECK constraint. The constraint enforcing only persistable values is the right design — it catches bugs rather than accommodating them.
## 🔒 Nora "NullX" Steiner — Security Engineer ### Questions & Observations - **`displayName` as a computed field in the API response.** Since it's `@Transient`, it's read-only. But verify that Jackson/SpringDoc does not accept `displayName` in request bodies (e.g., PUT/PATCH on a Person). If it does, the value should be silently ignored — never let a client override a computed field. The `@Schema(accessMode = READ_ONLY)` annotation handles this. - **`title` field accepts freeform strings.** The migration defines `VARCHAR(50)` with no CHECK constraint on content. When #212 implements the edit form, input validation must be applied (max length, no HTML/script content). For this refactor issue, the field is only populated programmatically (from the parser), so no immediate risk. But flag it for #212. - **`personType` CHECK constraint alignment with the Java enum.** If the Java enum includes `SKIP` but the CHECK constraint doesn't, and a bug in the service layer accidentally persists a Person with type `SKIP`, the constraint will reject it — which is the correct behavior (defense in depth). This is actually good: the CHECK constraint acts as a safety net for the service-layer invariant. Leave `SKIP` out of the CHECK constraint intentionally. - **Nullable `firstName` and search queries.** Any JPQL query with `WHERE p.firstName LIKE :query` will now exclude persons with null firstName from search results. Verify that `DocumentSpecifications` and any person search code handles null firstName with `OR p.firstName IS NULL` where appropriate. ### Suggestions - No security blockers. The main note: when the edit form for `title` is added in #212, apply `@Size(max = 50)` and `@Pattern` validation to reject HTML/script content. Not needed in this issue since `title` is only set by the parser. - Intentionally omit `SKIP` from the DB CHECK constraint. The constraint enforcing only persistable values is the right design — it catches bugs rather than accommodating them.
marcel commented 2026-04-07 21:09:28 +02:00
Author
Owner
Copy Link

🎨 Leonie Voss — UI/UX Design Lead

Questions & Observations

  • displayName replaces all name concatenation — but not all display formats are the same. The issue maps every firstName + " " + lastName to displayName. But some components currently show reversed format: PersonMultiSelect shows {person.lastName}, {person.firstName}. Switching to displayName changes this to forward order. Is that an intentional UX change? Reversed format is common in list contexts for alphabetical scanning. If some components need reversed order, they can't rely on displayName alone.
  • Initials handling for non-persons. The getInitials() helper falls back to lastName.substring(0, 2) when firstName is null. For "Gesellschafter des Verlages", initials would be "GE" — meaningless. Non-person entries should show a type icon (building icon, people icon) instead of initials. The helper needs a personType parameter or the component needs to branch on type.
  • Avatar circles with one vs two initials. Currently avatars show two-letter initials (first + last). With null firstName, some will show two letters from lastName. This creates visual inconsistency — "CM" (Clara Muller) next to "MO" (Molly, from lastName). Consider: single-letter initial (first char of displayName) when firstName is null, to signal "this person has incomplete data."
  • The title field is not displayed yet in this issue. Part 8 replaces concatenation with displayName, which includes the title via getDisplayName(). So "Tante Molly" will display correctly. But the title is not visually distinguished — it renders in the same weight/size as the name. The #212 discussion with Leonie should define the visual treatment (muted style for title prefix).

Suggestions

  • Decide now: does displayName replace the reversed format in PersonMultiSelect and PersonTypeahead, or do those components keep their own formatting? If forward order everywhere, document it as an intentional change.
  • The getInitials() helper should accept personType and return a type-specific placeholder for INSTITUTION/GROUP/UNKNOWN instead of extracting initials from an institution name.
  • For this refactor, displayName renders title in the same style as the name. That's fine as a starting point. The visual distinction (muted title) can come in #212.
## 🎨 Leonie Voss — UI/UX Design Lead ### Questions & Observations - **`displayName` replaces all name concatenation — but not all display formats are the same.** The issue maps every `firstName + " " + lastName` to `displayName`. But some components currently show reversed format: `PersonMultiSelect` shows `{person.lastName}, {person.firstName}`. Switching to `displayName` changes this to forward order. Is that an intentional UX change? Reversed format is common in list contexts for alphabetical scanning. If some components need reversed order, they can't rely on `displayName` alone. - **Initials handling for non-persons.** The `getInitials()` helper falls back to `lastName.substring(0, 2)` when firstName is null. For "Gesellschafter des Verlages", initials would be "GE" — meaningless. Non-person entries should show a type icon (building icon, people icon) instead of initials. The helper needs a `personType` parameter or the component needs to branch on type. - **Avatar circles with one vs two initials.** Currently avatars show two-letter initials (first + last). With null firstName, some will show two letters from lastName. This creates visual inconsistency — "CM" (Clara Muller) next to "MO" (Molly, from lastName). Consider: single-letter initial (first char of displayName) when firstName is null, to signal "this person has incomplete data." - **The `title` field is not displayed yet in this issue.** Part 8 replaces concatenation with `displayName`, which includes the title via `getDisplayName()`. So "Tante Molly" will display correctly. But the title is not visually distinguished — it renders in the same weight/size as the name. The #212 discussion with Leonie should define the visual treatment (muted style for title prefix). ### Suggestions - Decide now: does `displayName` replace the reversed format in `PersonMultiSelect` and `PersonTypeahead`, or do those components keep their own formatting? If forward order everywhere, document it as an intentional change. - The `getInitials()` helper should accept `personType` and return a type-specific placeholder for INSTITUTION/GROUP/UNKNOWN instead of extracting initials from an institution name. - For this refactor, `displayName` renders title in the same style as the name. That's fine as a starting point. The visual distinction (muted title) can come in #212.
marcel commented 2026-04-07 21:09:37 +02:00
Author
Owner
Copy Link

🛠️ Tobias Wendt — DevOps Engineer

Questions & Observations

  • Migration version number. The issue says "next after current max" but doesn't claim a specific number. With #190 merged, what's the current highest version? If it's V21 (from #181's V21__add_person_name_aliases.sql), this would be V22. Claim it now in the issue to prevent collisions.
  • Single migration file for 3 DDL changes. Adding a column, adding another column with CHECK, and dropping NOT NULL are three operations. All are safe, fast, and non-blocking on PostgreSQL. A single migration file is appropriate — no need to split. Good.
  • npm run generate:api requires a running backend. Part 9 says regenerate API types after backend changes. The backend must be running with --spring.profiles.active=dev for the OpenAPI spec to be accessible. Ensure the implementation instructions include: (1) apply migration, (2) start backend with dev profile, (3) run npm run generate:api, (4) commit the regenerated types. If the implementer forgets step 2-3, the frontend types will be stale.
  • No Docker/CI changes needed. Confirmed: no new services, no new ports, no Dockerfile changes. The migration runs automatically on startup via Flyway. Clean from an infrastructure perspective.

Suggestions

  • Claim migration version V22 in the issue body to prevent collisions with parallel work.
  • Consider adding a CI check that verifies generated API types are up to date (npm run generate:api && git diff --exit-code). This would catch the "forgot to regenerate" class of bugs permanently. Not blocking for this issue, but a good improvement to add alongside it.
## 🛠️ Tobias Wendt — DevOps Engineer ### Questions & Observations - **Migration version number.** The issue says "next after current max" but doesn't claim a specific number. With #190 merged, what's the current highest version? If it's V21 (from #181's `V21__add_person_name_aliases.sql`), this would be V22. Claim it now in the issue to prevent collisions. - **Single migration file for 3 DDL changes.** Adding a column, adding another column with CHECK, and dropping NOT NULL are three operations. All are safe, fast, and non-blocking on PostgreSQL. A single migration file is appropriate — no need to split. Good. - **`npm run generate:api` requires a running backend.** Part 9 says regenerate API types after backend changes. The backend must be running with `--spring.profiles.active=dev` for the OpenAPI spec to be accessible. Ensure the implementation instructions include: (1) apply migration, (2) start backend with dev profile, (3) run `npm run generate:api`, (4) commit the regenerated types. If the implementer forgets step 2-3, the frontend types will be stale. - **No Docker/CI changes needed.** Confirmed: no new services, no new ports, no Dockerfile changes. The migration runs automatically on startup via Flyway. Clean from an infrastructure perspective. ### Suggestions - Claim migration version V22 in the issue body to prevent collisions with parallel work. - Consider adding a CI check that verifies generated API types are up to date (`npm run generate:api && git diff --exit-code`). This would catch the "forgot to regenerate" class of bugs permanently. Not blocking for this issue, but a good improvement to add alongside it.
marcel commented 2026-04-08 11:18:36 +02:00
Author
Owner
Copy Link

🏗️ Markus Keller — Application Architect (Discussion Summary)

Interactive discussion with Marcel covering 5 open items from the review comments. All resolved.

Resolved Items

  • SKIP in Java enum vs DB CHECK constraint — SKIP is added to the PersonType Java enum (5 values: PERSON, INSTITUTION, GROUP, UNKNOWN, SKIP). Intentionally omitted from the DB CHECK constraint (4 persistable values). The constraint acts as defense-in-depth — if a bug tries to persist a SKIP record, the DB rejects it. Update Part 4 to show 5-value enum, add a comment in Part 6 noting the intentional omission.

  • Reversed display format — Decision: forward order everywhere. displayName replaces ALL display formats, including the reversed "Cram, Clara" in PersonMultiSelect and PersonTypeahead. This is an intentional UX change. Rationale: the person list is sorted by lastName anyway (alphabetical scanning works from sort order), and keeping reversed format means each component reinvents null-firstName/title handling — exactly the duplication displayName eliminates.

  • getInitials() for non-persons — Ship the simple helper now (persons only, no personType check). Extend with a non-person branch in #211 when classification actually creates non-person records. No speculative code in this refactor.

  • @Schema annotation for getDisplayName() — Must be annotated @Schema(accessMode = Schema.AccessMode.READ_ONLY, requiredMode = Schema.RequiredMode.REQUIRED). Without this, SpringDoc won't include displayName in the OpenAPI spec and the frontend TypeScript types won't have the field. Explicitly noted in Part 7.

  • Nullable firstName impact on search — Add an explicit acceptance criterion: "Verify all person search queries (DocumentSpecifications, person search, any JPQL touching firstName) handle null firstName correctly." The displayName field is @Transient and cannot be used in JPQL — search must continue using firstName/lastName/title individually. This is an implementation checklist item, not a design change.

Issue Body Updates Needed

  1. Part 4: Add SKIP to the enum definition
  2. Part 6: Add comment noting SKIP intentionally omitted from CHECK constraint
  3. Part 7: Add @Schema(accessMode = READ_ONLY, requiredMode = REQUIRED) annotation
  4. Part 8: Note that reversed format in PersonMultiSelect/PersonTypeahead is intentionally replaced with forward-order displayName
  5. Add acceptance criterion: verify all search queries handle null firstName

Overall this issue is well-scoped and architecturally sound. The 10-part structure is clear and the dependency chain is correctly ordered.

## 🏗️ Markus Keller — Application Architect (Discussion Summary) Interactive discussion with Marcel covering 5 open items from the review comments. All resolved. ### Resolved Items - **SKIP in Java enum vs DB CHECK constraint** — `SKIP` is added to the `PersonType` Java enum (5 values: PERSON, INSTITUTION, GROUP, UNKNOWN, SKIP). Intentionally omitted from the DB CHECK constraint (4 persistable values). The constraint acts as defense-in-depth — if a bug tries to persist a SKIP record, the DB rejects it. Update Part 4 to show 5-value enum, add a comment in Part 6 noting the intentional omission. - **Reversed display format** — Decision: **forward order everywhere**. `displayName` replaces ALL display formats, including the reversed "Cram, Clara" in `PersonMultiSelect` and `PersonTypeahead`. This is an intentional UX change. Rationale: the person list is sorted by `lastName` anyway (alphabetical scanning works from sort order), and keeping reversed format means each component reinvents null-firstName/title handling — exactly the duplication `displayName` eliminates. - **`getInitials()` for non-persons** — Ship the simple helper now (persons only, no `personType` check). Extend with a non-person branch in #211 when classification actually creates non-person records. No speculative code in this refactor. - **`@Schema` annotation for `getDisplayName()`** — Must be annotated `@Schema(accessMode = Schema.AccessMode.READ_ONLY, requiredMode = Schema.RequiredMode.REQUIRED)`. Without this, SpringDoc won't include `displayName` in the OpenAPI spec and the frontend TypeScript types won't have the field. Explicitly noted in Part 7. - **Nullable `firstName` impact on search** — Add an explicit acceptance criterion: "Verify all person search queries (DocumentSpecifications, person search, any JPQL touching firstName) handle null firstName correctly." The `displayName` field is `@Transient` and cannot be used in JPQL — search must continue using `firstName`/`lastName`/`title` individually. This is an implementation checklist item, not a design change. ### Issue Body Updates Needed 1. Part 4: Add `SKIP` to the enum definition 2. Part 6: Add comment noting `SKIP` intentionally omitted from CHECK constraint 3. Part 7: Add `@Schema(accessMode = READ_ONLY, requiredMode = REQUIRED)` annotation 4. Part 8: Note that reversed format in PersonMultiSelect/PersonTypeahead is intentionally replaced with forward-order `displayName` 5. Add acceptance criterion: verify all search queries handle null firstName Overall this issue is well-scoped and architecturally sound. The 10-part structure is clear and the dependency chain is correctly ordered.
marcel commented 2026-04-08 12:48:23 +02:00
Author
Owner
Copy Link

Implementation Complete

All 10 parts of this issue have been implemented on branch feat/issues-209-213-person-parser-enhancements.

Commits

Commit Description
1e1921e Expand SplitName record to 5 fields (title, firstName, lastName, maidenName, annotation)
dea1635 Extract split() pipeline into named pass-through methods
8101ddb Add PersonType enum (PERSON, INSTITUTION, GROUP, UNKNOWN, SKIP) and MAIDEN_NAME alias type
9f14648 Add title, personType, displayName to Person entity; make firstName nullable
92f1a11 V22 migration: add title, person_type columns, drop NOT NULL on first_name
de2cc67 Fix all search queries (6 locations) to handle null firstName via COALESCE
0ce803c Regenerate API types
f11d8a3 Replace all name concatenation with displayName across 32 files
9caef1e Add i18n keys for PersonType values and MAIDEN_NAME
1aabd98 Update all frontend test mock data for new Person shape

Key decisions applied

  • SKIP in Java enum, intentionally omitted from DB CHECK constraint (defense-in-depth)
  • @Schema(accessMode = READ_ONLY, requiredMode = REQUIRED) on getDisplayName()
  • Forward-order displayName everywhere (including PersonMultiSelect dropdown)
  • Pipeline methods are pass-throughs until feature issues fill them in
  • PersonSummaryDTO gets displayName via default method (computed from title/firstName/lastName)
  • Simple getInitials() without personType check (deferred to #211)

Test results

  • Backend: 702 tests passing (2 new for null firstName search)
  • Frontend: 646 passing, 4 pre-existing failures (unrelated: CommentThread, TranscriptionBlock, TranscriptionEditView, Conversations page)
## Implementation Complete All 10 parts of this issue have been implemented on branch `feat/issues-209-213-person-parser-enhancements`. ### Commits | Commit | Description | |--------|-------------| | `1e1921e` | Expand SplitName record to 5 fields (title, firstName, lastName, maidenName, annotation) | | `dea1635` | Extract split() pipeline into named pass-through methods | | `8101ddb` | Add PersonType enum (PERSON, INSTITUTION, GROUP, UNKNOWN, SKIP) and MAIDEN_NAME alias type | | `9f14648` | Add title, personType, displayName to Person entity; make firstName nullable | | `92f1a11` | V22 migration: add title, person_type columns, drop NOT NULL on first_name | | `de2cc67` | Fix all search queries (6 locations) to handle null firstName via COALESCE | | `0ce803c` | Regenerate API types | | `f11d8a3` | Replace all name concatenation with displayName across 32 files | | `9caef1e` | Add i18n keys for PersonType values and MAIDEN_NAME | | `1aabd98` | Update all frontend test mock data for new Person shape | ### Key decisions applied - `SKIP` in Java enum, intentionally omitted from DB CHECK constraint (defense-in-depth) - `@Schema(accessMode = READ_ONLY, requiredMode = REQUIRED)` on `getDisplayName()` - Forward-order `displayName` everywhere (including PersonMultiSelect dropdown) - Pipeline methods are pass-throughs until feature issues fill them in - PersonSummaryDTO gets `displayName` via default method (computed from title/firstName/lastName) - Simple `getInitials()` without personType check (deferred to #211) ### Test results - Backend: 702 tests passing (2 new for null firstName search) - Frontend: 646 passing, 4 pre-existing failures (unrelated: CommentThread, TranscriptionBlock, TranscriptionEditView, Conversations page)
Sign in to join this conversation.
No Branch/Tag Specified
Branches Tags
Labels
Clear labels
P0-critical
Blocks a core user journey, causes data loss, or is a security/accessibility barrier. Work on this before P1+.
 P1-high
Significant friction on a main user journey; workaround exists but is non-obvious. Next up after P0.
 P2-medium
Noticeable improvement; doesn't block anything; schedule opportunistically.
 P3-later
Cosmetic or parking-lot work; fine to stay open indefinitely.
 audit
Read-only audit / assessment work; produces a report rather than changing code
 bug
Something isn't working
 cleanup
Removal of dead code, vague comments, naming violations, and scratch artifacts
 collaboration
We want to extend the family archive to have more collaboration tools
 conversation
descoped
We will do that later
 devops
documentation
README, ARCHITECTURE, GLOSSARY, CONTRIBUTING, per-domain docs
 epic
Marker for epic-level issues that group multiple child issues
 feature
file-upload
legibility
Codebase Legibility Refactor — preparing the codebase for human evaluation and bus-factor reduction
 notification
person
phase-1: security
Security hygiene — must be done first
 phase-2: container-images
Production-ready multi-stage Docker images
 phase-3: prod-compose
Production compose overlay + Caddy reverse proxy
 phase-4: spring-prod-profile
Spring Boot production configuration profile
 phase-5: backups
Database and object storage backup strategy
 phase-6: deployment-docs
.env.example, DEPLOYMENT.md, runbook
 phase-7: monitoring
Prometheus, Loki, Grafana, Alertmanager
 refactor
Code restructuring without behaviour change
 security
test
ui
UI/UX and accessibility issues
 user
No Label person refactor
Milestone
No items
No Milestone
Projects
Clear projects
No project
Assignees
Clear assignees
Jens marcel
No Assignees
1 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: marcel/familienarchiv#213
Delete Branch "%!s()"

Deleting a branch is permanent. Although the deleted branch may continue to exist for a short time before it actually gets removed, it CANNOT be undone in most cases. Continue?