feat(persons): audit + complete person-merge flow for all data domains #368

New Issue

marcel · 2026-04-28T23:50:33+02:00

marcel commented

2026-04-28 23:50:33 +02:00

Context

The person merge flow (PUT /api/persons/{id}/merge, person detail page) was built before several newer data domains existed. This issue audits every domain that holds person references and specifies what the merge must do in each one, so a full pass can close the gaps before the feature is considered production-safe.

Triggered by: PR-A #366 introduces transcription_block_mentioned_persons (a sidecar table + block.text inline references) that the current merge almost certainly does not handle.

Data domains that hold person references

Domain	Column / table	Current merge handles?	Required action on merge
Documents — sender	`documents.sender_id`	✅ (assumed)	Reassign to target
Documents — receivers	`document_receivers (document_id, person_id)`	✅ (assumed)	Reassign; deduplicate if target is already a receiver on the same document
Person aliases	`person_aliases.person_id` (if table exists)	❓ verify	Move all source aliases to target
Family relationships	`person_relationships (person1_id, person2_id)`	❓ verify	Reassign both sides; drop any source↔target relationship (can't be related to yourself)
@mention sidecar	`transcription_block_mentioned_persons.person_id`	❌ new in #366	Reassign + deduplicate; rewrite `display_name` to target's display name; rewrite `@SourceName` in `block.text` (see below)
AppUser link	`app_users` → `Person` association (if any)	❓ verify	Block merge if source Person is linked to a live AppUser account, or require explicit account transfer

The @mention gap (introduced by PR-A #366)

What must happen

When source person S is merged into target person T:

Sidecar reassignment — every row in transcription_block_mentioned_persons where person_id = S.id must become person_id = T.id with display_name = T.getDisplayName().
Deduplication — if a block already mentions T, reassigning S's sidecar row would create two rows with the same (block_id, T.id) — a violation of the UNIQUE constraint added in V57. These must be deduplicated: keep the T row (update its display_name if needed), delete the S row.
block.text rewrite — if S.getDisplayName() ≠ T.getDisplayName(), every occurrence of @SDisplayName in block.text must be replaced with @TDisplayName. Use the same boundary-anchored regex already in PersonMentionPropagationListener.rewriteBlockText — do not duplicate the logic.

Recommended approach

Introduce a MergePersonMentionsStep (or reuse the propagation listener) called from within the merge transaction:

// pseudocode — implementation detail for the developer
1. blocks = findByPersonIdWithMentionsFetched(source.id)
2. for each block:
   a. if block already has a sidecar entry for target.id → remove source sidecar entry
   b. else → update source sidecar entry: set person_id = target.id, display_name = target.getDisplayName()
   c. if source.getDisplayName() ≠ target.getDisplayName(): rewriteBlockText(block, @source, @target)
3. blockRepository.saveAllAndFlush(blocks)

Document deduplication edge case

A document can list the same person as a receiver only once. If document D has both source S and target T as receivers:

After merge, D should have T as receiver exactly once.
Constraint: remove S from document_receivers for D; T's entry stays.

Same logic applies if source is the sender and target is also a receiver (or vice versa) — no action needed there since sender and receivers are independent.

Family relationship edge case

If a relationship row has person1_id = S and person2_id = T (or vice versa), that row represents a relationship between the two persons being merged. After merge they are the same person — the row must be deleted, not reassigned.

All other relationship rows involving S are reassigned to T. Check for duplicate relationship types after reassignment (e.g. two "Vater" relationships for T+same third person) and deduplicate.

AppUser link guard

If the source Person is linked to an AppUser account, silently deleting the Person would leave the account without a profile. The merge endpoint must:

Check whether source.id is referenced by any app_users row.
If yes: return 400 / DomainException(PERSON_HAS_USER_ACCOUNT) with a clear error message.
The UI should surface this as: "Diese Person ist mit einem Benutzerkonto verknüpft und kann nicht gelöscht werden. Bitte übertrage das Konto zuerst."

Atomicity requirement

The entire merge — document reassignment, alias merge, relationship reassignment, sidecar rewrite, text rewrite, source deletion — must execute in a single @Transactional boundary. If any step fails, the whole operation rolls back. No partial merges.

Acceptance criteria

Given two persons S (source) and T (target) exist with overlapping documents, aliases, relationships, and @mentions
When a WRITE_ALL user calls PUT /api/persons/{sourceId}/merge?targetId={targetId}
Then:
  - All documents that listed S as sender now list T as sender
  - All documents that listed S as receiver now list T as receiver, with no duplicate receiver rows
  - All aliases of S are added to T (no duplicates)
  - All family relationships of S are transferred to T; the S↔T relationship (if it existed) is deleted
  - All sidecar @mention entries for S are transferred to T; blocks mentioning both S and T before the merge have exactly one sidecar entry for T after the merge
  - block.text occurrences of @S.displayName are rewritten to @T.displayName where displayName differs
  - S is deleted
  - The operation is atomic: if any step fails, no changes are persisted

Given the source Person is linked to an AppUser account
When a WRITE_ALL user calls PUT /api/persons/{sourceId}/merge?targetId={targetId}
Then the response is 400 with ErrorCode PERSON_HAS_USER_ACCOUNT
And the source Person is not deleted

Given sourceId == targetId
When a WRITE_ALL user calls PUT /api/persons/{sourceId}/merge?targetId={targetId}
Then the response is 400 with ErrorCode INVALID_INPUT

Open questions

OQ	Question	Why it matters
OQ-1	Does the current merge already handle `person_aliases`?	If missing, aliases are silently lost on merge
OQ-2	Does the current merge already handle `person_relationships`?	Relationships disappear without trace
OQ-3	Is any Person linked to an AppUser in the current data model?	Determines whether the AppUser guard is needed now or later
OQ-4	Should the merge produce an audit log entry?	Useful for debugging accidental merges
OQ-5	Should the UI show a preview of what will be merged before confirming?	Nielsen H5 (error prevention) — a merge is irreversible

PR-A #366 — introduces the sidecar table and the V57 unique constraint that make this gap detectable
Issue #362 — @mention feature that created the sidecar table
PersonMentionPropagationListener — contains rewriteBlockText that must be reused, not duplicated

## Context The person merge flow (`PUT /api/persons/{id}/merge`, person detail page) was built before several newer data domains existed. This issue audits every domain that holds person references and specifies what the merge must do in each one, so a full pass can close the gaps before the feature is considered production-safe. Triggered by: PR-A #366 introduces `transcription_block_mentioned_persons` (a sidecar table + `block.text` inline references) that the current merge almost certainly does not handle. --- ## Data domains that hold person references | Domain | Column / table | Current merge handles? | Required action on merge | |---|---|---|---| | Documents — sender | `documents.sender_id` | ✅ (assumed) | Reassign to target | | Documents — receivers | `document_receivers (document_id, person_id)` | ✅ (assumed) | Reassign; **deduplicate** if target is already a receiver on the same document | | Person aliases | `person_aliases.person_id` (if table exists) | ❓ verify | Move all source aliases to target | | Family relationships | `person_relationships (person1_id, person2_id)` | ❓ verify | Reassign both sides; **drop** any source↔target relationship (can't be related to yourself) | | @mention sidecar | `transcription_block_mentioned_persons.person_id` | ❌ new in #366 | Reassign + deduplicate; rewrite `display_name` to target's display name; rewrite `@SourceName` in `block.text` (see below) | | AppUser link | `app_users` → `Person` association (if any) | ❓ verify | **Block merge** if source Person is linked to a live AppUser account, or require explicit account transfer | --- ## The @mention gap (introduced by PR-A #366) ### What must happen When source person S is merged into target person T: 1. **Sidecar reassignment** — every row in `transcription_block_mentioned_persons` where `person_id = S.id` must become `person_id = T.id` with `display_name = T.getDisplayName()`. 2. **Deduplication** — if a block already mentions T, reassigning S's sidecar row would create two rows with the same `(block_id, T.id)` — a violation of the `UNIQUE` constraint added in V57. These must be deduplicated: keep the T row (update its `display_name` if needed), delete the S row. 3. **block.text rewrite** — if `S.getDisplayName() ≠ T.getDisplayName()`, every occurrence of `@SDisplayName` in `block.text` must be replaced with `@TDisplayName`. Use the same boundary-anchored regex already in `PersonMentionPropagationListener.rewriteBlockText` — do not duplicate the logic. ### Recommended approach Introduce a `MergePersonMentionsStep` (or reuse the propagation listener) called from within the merge transaction: ``` // pseudocode — implementation detail for the developer 1. blocks = findByPersonIdWithMentionsFetched(source.id) 2. for each block: a. if block already has a sidecar entry for target.id → remove source sidecar entry b. else → update source sidecar entry: set person_id = target.id, display_name = target.getDisplayName() c. if source.getDisplayName() ≠ target.getDisplayName(): rewriteBlockText(block, @source, @target) 3. blockRepository.saveAllAndFlush(blocks) ``` --- ## Document deduplication edge case A document can list the same person as a receiver only once. If document D has both source S and target T as receivers: - After merge, D should have T as receiver exactly once. - Constraint: remove S from `document_receivers` for D; T's entry stays. Same logic applies if source is the sender and target is also a receiver (or vice versa) — no action needed there since sender and receivers are independent. --- ## Family relationship edge case If a relationship row has `person1_id = S` and `person2_id = T` (or vice versa), that row represents a relationship between the two persons being merged. After merge they are the same person — the row must be deleted, not reassigned. All other relationship rows involving S are reassigned to T. Check for duplicate relationship types after reassignment (e.g. two "Vater" relationships for T+same third person) and deduplicate. --- ## AppUser link guard If the source Person is linked to an AppUser account, silently deleting the Person would leave the account without a profile. The merge endpoint must: - Check whether `source.id` is referenced by any `app_users` row. - If yes: return `400 / DomainException(PERSON_HAS_USER_ACCOUNT)` with a clear error message. - The UI should surface this as: *"Diese Person ist mit einem Benutzerkonto verknüpft und kann nicht gelöscht werden. Bitte übertrage das Konto zuerst."* --- ## Atomicity requirement **The entire merge — document reassignment, alias merge, relationship reassignment, sidecar rewrite, text rewrite, source deletion — must execute in a single `@Transactional` boundary.** If any step fails, the whole operation rolls back. No partial merges. --- ## Acceptance criteria ``` Given two persons S (source) and T (target) exist with overlapping documents, aliases, relationships, and @mentions When a WRITE_ALL user calls PUT /api/persons/{sourceId}/merge?targetId={targetId} Then: - All documents that listed S as sender now list T as sender - All documents that listed S as receiver now list T as receiver, with no duplicate receiver rows - All aliases of S are added to T (no duplicates) - All family relationships of S are transferred to T; the S↔T relationship (if it existed) is deleted - All sidecar @mention entries for S are transferred to T; blocks mentioning both S and T before the merge have exactly one sidecar entry for T after the merge - block.text occurrences of @S.displayName are rewritten to @T.displayName where displayName differs - S is deleted - The operation is atomic: if any step fails, no changes are persisted ``` ``` Given the source Person is linked to an AppUser account When a WRITE_ALL user calls PUT /api/persons/{sourceId}/merge?targetId={targetId} Then the response is 400 with ErrorCode PERSON_HAS_USER_ACCOUNT And the source Person is not deleted ``` ``` Given sourceId == targetId When a WRITE_ALL user calls PUT /api/persons/{sourceId}/merge?targetId={targetId} Then the response is 400 with ErrorCode INVALID_INPUT ``` --- ## Open questions | OQ | Question | Why it matters | |---|---|---| | OQ-1 | Does the current merge already handle `person_aliases`? | If missing, aliases are silently lost on merge | | OQ-2 | Does the current merge already handle `person_relationships`? | Relationships disappear without trace | | OQ-3 | Is any Person linked to an AppUser in the current data model? | Determines whether the AppUser guard is needed now or later | | OQ-4 | Should the merge produce an audit log entry? | Useful for debugging accidental merges | | OQ-5 | Should the UI show a preview of what will be merged before confirming? | Nielsen H5 (error prevention) — a merge is irreversible | --- ## Related - PR-A #366 — introduces the sidecar table and the V57 unique constraint that make this gap detectable - Issue #362 — @mention feature that created the sidecar table - `PersonMentionPropagationListener` — contains `rewriteBlockText` that must be reused, not duplicated

marcel added this to the Persons Redesign v2 milestone 2026-04-28 23:50:33 +02:00

marcel added the P1-high feature person labels 2026-04-28 23:50:38 +02:00

marcel commented

2026-04-28 23:51:06 +02:00

OQ-3 resolved — product decision:

Historical Persons (the archive subjects — Auguste Raddatz, Hans Müller, etc.) and AppUsers (the family members who operate the archive) are strictly separate domains. A Person record never has an associated AppUser account.

Consequence: The AppUser link guard in the merge flow is not needed. Remove it from the implementation scope. The merge endpoint can proceed directly to document/alias/relationship/sidecar reassignment without a pre-flight account check.

**OQ-3 resolved — product decision:** Historical Persons (the archive subjects — Auguste Raddatz, Hans Müller, etc.) and AppUsers (the family members who operate the archive) are strictly separate domains. A Person record never has an associated AppUser account. **Consequence:** The AppUser link guard in the merge flow is not needed. Remove it from the implementation scope. The merge endpoint can proceed directly to document/alias/relationship/sidecar reassignment without a pre-flight account check.

marcel commented

2026-04-28 23:52:22 +02:00

Code audit — existing merge scope confirmed

Read PersonService.mergePersons (line 187). Current implementation:

personRepository.reassignSender(sourceId, targetId);
personRepository.insertMissingReceiverReference(sourceId, targetId);
personRepository.deleteReceiverReferences(sourceId);
personRepository.deleteById(sourceId);

What it already handles correctly:

✅ Source == target guard
✅ Documents: sender reassignment
✅ Documents: receiver deduplication (insert missing → delete source)
✅ Source person deleted

What it does NOT handle (and the feature still works today only because these are post-#366):

❌ @mention sidecar — the actual gap

transcription_block_mentioned_persons has no FK on person_id (V56, by design). When deleteById(sourceId) fires:

The sidecar rows with person_id = sourceId are not cascade-deleted — they become orphaned
block.text still contains @S.displayName — never rewritten to @T.displayName
PR-B's hover card will try to resolve person_id = sourceId and get a 404 for every merged person's mention

Fix needed in mergePersons, before deleteById:

Load blocks via findByPersonIdWithMentionsFetched(sourceId)
For each block: if it already has a sidecar entry for targetId → delete source's sidecar row; else → update sidecar row to person_id = targetId, display_name = target.getDisplayName()
If source.getDisplayName() ≠ target.getDisplayName() → call rewriteBlockText (already in PersonMentionPropagationListener) — do not duplicate the regex
blockRepository.saveAllAndFlush(blocks)

This runs inside the existing @Transactional boundary so it rolls back with everything else.

Out of scope for this issue (existing behaviour, not a regression):

Aliases: deleted with source person (no transfer) — pre-existing limitation, separate issue if desired
Family relationships: cascade-deleted with source person — pre-existing limitation, separate issue if desired

**Code audit — existing merge scope confirmed** Read `PersonService.mergePersons` (line 187). Current implementation: ```java personRepository.reassignSender(sourceId, targetId); personRepository.insertMissingReceiverReference(sourceId, targetId); personRepository.deleteReceiverReferences(sourceId); personRepository.deleteById(sourceId); ``` **What it already handles correctly:** - ✅ Source == target guard - ✅ Documents: sender reassignment - ✅ Documents: receiver deduplication (insert missing → delete source) - ✅ Source person deleted **What it does NOT handle (and the feature still works today only because these are post-#366):** ### ❌ @mention sidecar — the actual gap `transcription_block_mentioned_persons` has **no FK on `person_id`** (V56, by design). When `deleteById(sourceId)` fires: - The sidecar rows with `person_id = sourceId` are **not cascade-deleted** — they become orphaned - `block.text` still contains `@S.displayName` — never rewritten to `@T.displayName` - PR-B's hover card will try to resolve `person_id = sourceId` and get a 404 for every merged person's mention **Fix needed in `mergePersons`, before `deleteById`:** 1. Load blocks via `findByPersonIdWithMentionsFetched(sourceId)` 2. For each block: if it already has a sidecar entry for `targetId` → delete source's sidecar row; else → update sidecar row to `person_id = targetId`, `display_name = target.getDisplayName()` 3. If `source.getDisplayName() ≠ target.getDisplayName()` → call `rewriteBlockText` (already in `PersonMentionPropagationListener`) — do not duplicate the regex 4. `blockRepository.saveAllAndFlush(blocks)` This runs inside the existing `@Transactional` boundary so it rolls back with everything else. **Out of scope for this issue (existing behaviour, not a regression):** - Aliases: deleted with source person (no transfer) — pre-existing limitation, separate issue if desired - Family relationships: cascade-deleted with source person — pre-existing limitation, separate issue if desired

marcel commented

2026-05-07 17:38:15 +02:00

🏛️ Markus Keller (@mkeller) — Application Architect

Observations

The existing mergePersons runs inside @Transactional — the atomicity requirement is already met structurally. The new mention-sidecar step must stay inside this same transactional boundary; it will, as long as it is called before deleteById.
PersonRelationshipRepository has no merge-aware query. The migration V54 uses ON DELETE CASCADE on both person_id and related_person_id — this means all relationships of the source person are silently cascade-deleted when deleteById(sourceId) fires. No transfer happens at all. This is a data-loss gap not explicitly called out in the issue's "code audit" comment (which punted aliases and relationships to a future issue), but the cascade behavior makes the gap worse than a "pre-existing limitation" — it is an invisible silent drop, not a documented non-transfer.
The PersonService currently only injects PersonRepository and PersonNameAliasRepository. Adding the sidecar step will require injecting TranscriptionBlockRepository from the document.transcription package. This is a cross-domain dependency: person domain calling document domain's repository directly, which violates the layering rule (services call other domain's service, not repository). The correct path is: inject TranscriptionService (or introduce a TranscriptionMentionService callable method) — or, if the mention logic is cohesive enough, move it into a new PersonMentionMergeService that lives in the transcription package and is called from PersonService.
rewriteBlockText is referenced in the issue but does not appear in the codebase — PersonMentionPropagationListener does not exist yet (it is introduced by PR-A #366 which is still open). The implementation of this issue should not proceed until the listener method is merged and available, or should include it as part of this issue's scope.
The mergePersons method throws ResponseStatusException for the source==target guard, but throws DomainException for not-found. This is inconsistent with the project's error-handling convention (services should use DomainException everywhere; ResponseStatusException is acceptable only in controllers). The issue spec's acceptance criteria includes INVALID_INPUT as the ErrorCode for this case — fixing the inconsistency is part of the spec.
The merge endpoint is declared as @PostMapping("/{id}/merge") in the controller and called as POST from the frontend. The issue title and original code audit say PUT /api/persons/{id}/merge. These must align — pick one HTTP verb and keep it consistent across controller, frontend, and any API test files.

Recommendations

Move the cross-domain sidecar-rewrite logic into a method on TranscriptionService (e.g., transferMentions(UUID sourcePersonId, UUID targetPersonId)) and inject TranscriptionService into PersonService. This keeps the domain boundary clean and the sidecar logic co-located with the other mention code.
Add a reassignRelationships(UUID sourceId, UUID targetId) native-SQL helper to PersonRelationshipRepository that: (1) updates rows where person_id = source to target, (2) updates rows where related_person_id = source to target, (3) deletes any row where both sides resolve to the same person after reassignment. All three in one @Modifying @Transactional method. This must execute before deleteById — do not rely on the cascade, as the cascade silently drops all edges rather than reassigning them.
Fix the ResponseStatusException in mergePersons — replace with DomainException.badRequest(ErrorCode.INVALID_INPUT, "Source and target must differ") so the frontend can map it to an i18n string. This requires adding INVALID_INPUT to ErrorCode.java, mirroring it in errors.ts, and adding i18n keys — the issue spec already calls this out.
Agree on one HTTP verb for the merge endpoint and update the controller, frontend action, and any API test files to match. POST is acceptable for a non-idempotent destructive operation.
Block this issue on PR-A #366 merging first. The rewriteBlockText utility must be in the codebase before this issue can reuse it.

Open Decisions

Relationship transfer scope: The issue acknowledges that aliases and relationships are "pre-existing limitations, separate issue if desired" (comment #5484). However, the cascade-delete on V54's FKs means relationships disappear silently rather than being preserved. The decision is whether to include relationship reassignment in this issue or explicitly accept silent deletion as the current production behavior. The issue spec's acceptance criteria explicitly requires relationship transfer — this should be confirmed as in-scope before implementation begins.

## 🏛️ Markus Keller (@mkeller) — Application Architect ### Observations - The existing `mergePersons` runs inside `@Transactional` — the atomicity requirement is already met structurally. The new mention-sidecar step must stay inside this same transactional boundary; it will, as long as it is called before `deleteById`. - `PersonRelationshipRepository` has no merge-aware query. The migration V54 uses `ON DELETE CASCADE` on both `person_id` and `related_person_id` — this means all relationships of the source person are **silently cascade-deleted** when `deleteById(sourceId)` fires. No transfer happens at all. This is a data-loss gap not explicitly called out in the issue's "code audit" comment (which punted aliases and relationships to a future issue), but the cascade behavior makes the gap worse than a "pre-existing limitation" — it is an invisible silent drop, not a documented non-transfer. - The `PersonService` currently only injects `PersonRepository` and `PersonNameAliasRepository`. Adding the sidecar step will require injecting `TranscriptionBlockRepository` from the `document.transcription` package. This is a cross-domain dependency: `person` domain calling `document` domain's repository directly, which violates the layering rule (services call other domain's **service**, not repository). The correct path is: inject `TranscriptionService` (or introduce a `TranscriptionMentionService` callable method) — or, if the mention logic is cohesive enough, move it into a new `PersonMentionMergeService` that lives in the transcription package and is called from `PersonService`. - `rewriteBlockText` is referenced in the issue but does not appear in the codebase — `PersonMentionPropagationListener` does not exist yet (it is introduced by PR-A #366 which is still open). The implementation of this issue should not proceed until the listener method is merged and available, or should include it as part of this issue's scope. - The `mergePersons` method throws `ResponseStatusException` for the source==target guard, but throws `DomainException` for not-found. This is inconsistent with the project's error-handling convention (services should use `DomainException` everywhere; `ResponseStatusException` is acceptable only in controllers). The issue spec's acceptance criteria includes `INVALID_INPUT` as the ErrorCode for this case — fixing the inconsistency is part of the spec. - The merge endpoint is declared as `@PostMapping("/{id}/merge")` in the controller and called as `POST` from the frontend. The issue title and original code audit say `PUT /api/persons/{id}/merge`. These must align — pick one HTTP verb and keep it consistent across controller, frontend, and any API test files. ### Recommendations - Move the cross-domain sidecar-rewrite logic into a method on `TranscriptionService` (e.g., `transferMentions(UUID sourcePersonId, UUID targetPersonId)`) and inject `TranscriptionService` into `PersonService`. This keeps the domain boundary clean and the sidecar logic co-located with the other mention code. - Add a `reassignRelationships(UUID sourceId, UUID targetId)` native-SQL helper to `PersonRelationshipRepository` that: (1) updates rows where `person_id = source` to `target`, (2) updates rows where `related_person_id = source` to `target`, (3) deletes any row where both sides resolve to the same person after reassignment. All three in one `@Modifying @Transactional` method. This must execute **before** `deleteById` — do not rely on the cascade, as the cascade silently drops all edges rather than reassigning them. - Fix the `ResponseStatusException` in `mergePersons` — replace with `DomainException.badRequest(ErrorCode.INVALID_INPUT, "Source and target must differ")` so the frontend can map it to an i18n string. This requires adding `INVALID_INPUT` to `ErrorCode.java`, mirroring it in `errors.ts`, and adding i18n keys — the issue spec already calls this out. - Agree on one HTTP verb for the merge endpoint and update the controller, frontend action, and any API test files to match. `POST` is acceptable for a non-idempotent destructive operation. - Block this issue on PR-A #366 merging first. The `rewriteBlockText` utility must be in the codebase before this issue can reuse it. ### Open Decisions - **Relationship transfer scope**: The issue acknowledges that aliases and relationships are "pre-existing limitations, separate issue if desired" (comment #5484). However, the cascade-delete on V54's FKs means relationships disappear silently rather than being preserved. The decision is whether to include relationship reassignment in this issue or explicitly accept silent deletion as the current production behavior. The issue spec's acceptance criteria explicitly requires relationship transfer — this should be confirmed as in-scope before implementation begins.

marcel commented

2026-05-07 17:38:38 +02:00

👨‍💻 Felix Brandt (@felixbrandt) — Fullstack Developer

Observations

mergePersons does three things: validates, reassigns documents, and deletes. The new sidecar step will make it four. Each step should be extracted into a named private method so the orchestrator reads as a sequence of intent-revealing calls. The current 8-line method is at the growth limit.
The sourceId == targetId guard throws ResponseStatusException while the not-found guards throw DomainException — two different exception types in the same method. The convention is DomainException from services. Fix this as part of the issue.
mergePersons lacks @Transactional — wait, looking again at line 174, it does have @Transactional. Good. The new steps will inherit the boundary automatically.
PersonMention is an @Embeddable collected via @ElementCollection(fetch = FetchType.EAGER). The findByPersonIdWithMentionsFetched query in TranscriptionBlockRepository already exists and returns blocks with mentions pre-joined. The implementation can use this query directly.
The deduplication logic described in the issue is: if the block already has a mention for targetId, remove the source's PersonMention from the list; otherwise update the source entry's personId and displayName. Since mentionedPersons is a List<PersonMention> (not a Set), the comparison must use personId field equality, not object identity.
PersonMention has no equals/hashCode beyond Lombok @Data defaults (field equality). personId and displayName are both included in equality. When searching for an existing target mention, compare by personId field only — a match on personId with a different displayName is still a duplicate that should be merged, not kept.
The frontend merge action in +page.server.ts hardcodes a German error string: 'Bitte eine Zielperson auswählen.' — this should be an i18n key via m.person_merge_select_target() or similar. Not introduced by this issue but worth noting.
After a successful merge, the frontend redirects to /persons/${targetPersonId}. The target person page should now show the transferred mentions and aliases — no frontend change needed for the redirect, but the person detail page must already render mentions correctly (depends on PR-B).

Recommendations

Extract the sidecar step into private void transferMentions(Person source, Person target) in PersonService (or delegate to TranscriptionService). The orchestrating mergePersons then reads:

transferMentions(source, target);
personRepository.reassignSender(sourceId, targetId);
personRepository.insertMissingReceiverReference(sourceId, targetId);
personRepository.deleteReceiverReferences(sourceId);
personRepository.deleteById(sourceId);

Five intent-revealing lines, no inline logic.

For the mention deduplication: use removeIf on the list to drop the source entry when a target entry already exists, or update in-place with a replaceAll-style loop. Keep it under 10 lines; extract a helper if it grows.
Replace the ResponseStatusException in the source==target guard with DomainException.badRequest(ErrorCode.INVALID_INPUT, "Source and target must not be the same") before this PR merges — the issue spec requires it and the test for it already asserts on ResponseStatusException, so the test needs updating too.
Add @Transactional explicitly to mergePersons (it has it already) and add a comment above the sidecar step: // Must execute before deleteById — sidecar has no FK, orphans on delete so future readers understand the ordering constraint.
Write the TDD cycle: red test for sidecar-not-transferred (assert 404 on hover card after merge), then green with the implementation. The existing unit test mergePersons_reassignsDocumentsAndDeletesSource should be extended with a mock expectation for the new findByPersonIdWithMentionsFetched call and the subsequent save.

## 👨‍💻 Felix Brandt (@felixbrandt) — Fullstack Developer ### Observations - `mergePersons` does three things: validates, reassigns documents, and deletes. The new sidecar step will make it four. Each step should be extracted into a named private method so the orchestrator reads as a sequence of intent-revealing calls. The current 8-line method is at the growth limit. - The `sourceId == targetId` guard throws `ResponseStatusException` while the not-found guards throw `DomainException` — two different exception types in the same method. The convention is `DomainException` from services. Fix this as part of the issue. - `mergePersons` lacks `@Transactional` — wait, looking again at line 174, it **does** have `@Transactional`. Good. The new steps will inherit the boundary automatically. - `PersonMention` is an `@Embeddable` collected via `@ElementCollection(fetch = FetchType.EAGER)`. The `findByPersonIdWithMentionsFetched` query in `TranscriptionBlockRepository` already exists and returns blocks with mentions pre-joined. The implementation can use this query directly. - The deduplication logic described in the issue is: if the block already has a mention for `targetId`, remove the source's `PersonMention` from the list; otherwise update the source entry's `personId` and `displayName`. Since `mentionedPersons` is a `List<PersonMention>` (not a `Set`), the comparison must use `personId` field equality, not object identity. - `PersonMention` has no `equals`/`hashCode` beyond Lombok `@Data` defaults (field equality). `personId` and `displayName` are both included in equality. When searching for an existing target mention, compare by `personId` field only — a match on `personId` with a different `displayName` is still a duplicate that should be merged, not kept. - The frontend merge action in `+page.server.ts` hardcodes a German error string: `'Bitte eine Zielperson auswählen.'` — this should be an i18n key via `m.person_merge_select_target()` or similar. Not introduced by this issue but worth noting. - After a successful merge, the frontend redirects to `/persons/${targetPersonId}`. The target person page should now show the transferred mentions and aliases — no frontend change needed for the redirect, but the person detail page must already render mentions correctly (depends on PR-B). ### Recommendations - Extract the sidecar step into `private void transferMentions(Person source, Person target)` in `PersonService` (or delegate to `TranscriptionService`). The orchestrating `mergePersons` then reads: ```java transferMentions(source, target); personRepository.reassignSender(sourceId, targetId); personRepository.insertMissingReceiverReference(sourceId, targetId); personRepository.deleteReceiverReferences(sourceId); personRepository.deleteById(sourceId); ``` Five intent-revealing lines, no inline logic. - For the mention deduplication: use `removeIf` on the list to drop the source entry when a target entry already exists, or update in-place with a `replaceAll`-style loop. Keep it under 10 lines; extract a helper if it grows. - Replace the `ResponseStatusException` in the source==target guard with `DomainException.badRequest(ErrorCode.INVALID_INPUT, "Source and target must not be the same")` before this PR merges — the issue spec requires it and the test for it already asserts on `ResponseStatusException`, so the test needs updating too. - Add `@Transactional` explicitly to `mergePersons` (it has it already) and add a comment above the sidecar step: `// Must execute before deleteById — sidecar has no FK, orphans on delete` so future readers understand the ordering constraint. - Write the TDD cycle: red test for sidecar-not-transferred (assert 404 on hover card after merge), then green with the implementation. The existing unit test `mergePersons_reassignsDocumentsAndDeletesSource` should be extended with a mock expectation for the new `findByPersonIdWithMentionsFetched` call and the subsequent save.

marcel commented

2026-05-07 17:39:00 +02:00

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

No authorization gap: The merge endpoint correctly carries @RequirePermission(Permission.WRITE_ALL) in the controller. This is verified in the code — no finding here.
UUID.fromString without try-catch: The controller calls UUID.fromString(targetIdStr) directly. If targetIdStr is not a valid UUID (e.g., a path-traversal string or garbage input), this throws an unchecked IllegalArgumentException that bypasses the GlobalExceptionHandler's structured error mapping. The user gets a 500 instead of a 400, and the stack trace may leak in logs. Low severity, but the fix is a one-liner.
Source==target guard uses ResponseStatusException: No security impact, but inconsistent with the structured error convention. DomainException carries an ErrorCode that the frontend maps to i18n — ResponseStatusException may leak the German error string "Quelle und Ziel dürfen nicht identisch sein" directly to the API consumer.
Sidecar has no FK on person_id (by design, per V56 comment): This was a deliberate architectural choice to allow soft degradation when a Person is deleted. The merge step must explicitly clean up orphaned sidecar rows — it cannot rely on any cascade. The issue spec calls this out correctly. Verify in the implementation that the cleanup happens inside the transaction boundary so no window exists where sidecar rows reference a deleted person UUID.
Block text rewrite is a string replacement, not eval: The rewriteBlockText logic described is boundary-anchored regex replacement on stored text. No injection risk from the replacement itself. However, confirm that the regex pattern used in rewriteBlockText uses Pattern.quote() or equivalent to prevent a person's display name containing regex metacharacters (e.g., Auguste (Müller)) from becoming an injection into the regex engine. Severity: Medium if unmitigated.
Audit log gap: The issue mentions OQ-4 (should merges be audit-logged?) as open. A merge is a destructive, irreversible operation performed by a WRITE_ALL user — it should appear in the audit log. Without it, there is no way to reconstruct who merged Auguste Alt into Auguste Neu and when. The existing AuditService in the audit/ package is the right hook.

Recommendations

Wrap UUID.fromString(targetIdStr) in the controller in a try-catch, returning a 400:

UUID targetId;
try { targetId = UUID.fromString(targetIdStr); }
catch (IllegalArgumentException e) {
    throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "targetPersonId is not a valid UUID");
}

Verify that rewriteBlockText (from PR-A #366) uses Pattern.quote(displayName) when constructing the regex. If it does not, add it — a person name like Anna (Braun) would corrupt the match without quoting.
Add an audit log entry inside mergePersons after all steps succeed but before the method returns: auditService.log(MERGE_PERSON, sourceId, "merged into " + targetId). This requires injecting AuditService — already a pattern in other services.
On the question of OQ-4: log the merge. The cost is one service call; the benefit is complete audit trail for an irreversible operation.

Open Decisions

Regex safety in rewriteBlockText: Whether Pattern.quote() is already applied depends on the implementation in PR-A #366, which is not yet merged. The implementer should verify this and add it if absent — it cannot be assumed safe.

## 🔒 Nora "NullX" Steiner — Application Security Engineer ### Observations - **No authorization gap**: The merge endpoint correctly carries `@RequirePermission(Permission.WRITE_ALL)` in the controller. This is verified in the code — no finding here. - **UUID.fromString without try-catch**: The controller calls `UUID.fromString(targetIdStr)` directly. If `targetIdStr` is not a valid UUID (e.g., a path-traversal string or garbage input), this throws an unchecked `IllegalArgumentException` that bypasses the `GlobalExceptionHandler`'s structured error mapping. The user gets a 500 instead of a 400, and the stack trace may leak in logs. Low severity, but the fix is a one-liner. - **Source==target guard uses `ResponseStatusException`**: No security impact, but inconsistent with the structured error convention. `DomainException` carries an `ErrorCode` that the frontend maps to i18n — `ResponseStatusException` may leak the German error string `"Quelle und Ziel dürfen nicht identisch sein"` directly to the API consumer. - **Sidecar has no FK on `person_id`** (by design, per V56 comment): This was a deliberate architectural choice to allow soft degradation when a Person is deleted. The merge step must explicitly clean up orphaned sidecar rows — it cannot rely on any cascade. The issue spec calls this out correctly. Verify in the implementation that the cleanup happens inside the transaction boundary so no window exists where sidecar rows reference a deleted person UUID. - **Block text rewrite is a string replacement, not eval**: The `rewriteBlockText` logic described is boundary-anchored regex replacement on stored text. No injection risk from the replacement itself. However, confirm that the regex pattern used in `rewriteBlockText` uses `Pattern.quote()` or equivalent to prevent a person's display name containing regex metacharacters (e.g., `Auguste (Müller)`) from becoming an injection into the regex engine. Severity: Medium if unmitigated. - **Audit log gap**: The issue mentions OQ-4 (should merges be audit-logged?) as open. A merge is a destructive, irreversible operation performed by a `WRITE_ALL` user — it should appear in the audit log. Without it, there is no way to reconstruct who merged `Auguste Alt` into `Auguste Neu` and when. The existing `AuditService` in the `audit/` package is the right hook. ### Recommendations - Wrap `UUID.fromString(targetIdStr)` in the controller in a try-catch, returning a 400: ```java UUID targetId; try { targetId = UUID.fromString(targetIdStr); } catch (IllegalArgumentException e) { throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "targetPersonId is not a valid UUID"); } ``` - Verify that `rewriteBlockText` (from PR-A #366) uses `Pattern.quote(displayName)` when constructing the regex. If it does not, add it — a person name like `Anna (Braun)` would corrupt the match without quoting. - Add an audit log entry inside `mergePersons` after all steps succeed but before the method returns: `auditService.log(MERGE_PERSON, sourceId, "merged into " + targetId)`. This requires injecting `AuditService` — already a pattern in other services. - On the question of OQ-4: log the merge. The cost is one service call; the benefit is complete audit trail for an irreversible operation. ### Open Decisions - **Regex safety in `rewriteBlockText`**: Whether `Pattern.quote()` is already applied depends on the implementation in PR-A #366, which is not yet merged. The implementer should verify this and add it if absent — it cannot be assumed safe.

marcel commented

2026-05-07 17:39:21 +02:00

🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist

Observations

The existing unit tests in PersonServiceTest cover: source==target guard, source not found, target not found, and the happy-path document reassignment. They use Mockito + @ExtendWith(MockitoExtension.class) — correct layer. However, none of the new behaviors have test scaffolding yet.
The happy-path test mergePersons_reassignsDocumentsAndDeletesSource will need to be extended (or a new test added) to mock TranscriptionBlockRepository.findByPersonIdWithMentionsFetched and verify the sidecar update path. Currently the PersonService does not inject TranscriptionBlockRepository, so any new mock must be added once the dependency is wired.
The deduplication branch (block already mentions target) has zero test coverage. This is the riskiest code path — a missed deduplication creates a duplicate (block_id, person_id) row that violates the UNIQUE constraint from V57, causing a 500 at runtime. This path must have a unit test before the PR merges.
The block-text rewrite branch (source.getDisplayName() != target.getDisplayName()) also has no test coverage. The regex behavior (boundary anchoring, case sensitivity, partial match avoidance) cannot be verified without a test.
Integration tests are needed to cover the full transactional boundary. The current PersonServiceTest is pure Mockito — there is no @SpringBootTest or Testcontainers integration test for mergePersons. A partial-failure scenario (e.g., unique constraint violation mid-merge) cannot be verified without a real PostgreSQL instance.
The relationship cascade-delete behavior (V54 ON DELETE CASCADE) means the existing integration test suite may pass even though relationships are silently dropped — no assertion exists that checks relationship counts before and after merge.
The frontend merge flow has no E2E Playwright test. A merge is the most destructive user-facing operation in the person domain — it should have at least one happy-path E2E test confirming the redirect to the target person and one error-path test confirming the error message is displayed.

Recommendations

Write these unit tests before any production code changes (TDD red phase):
1. mergePersons_transfersMentions_whenSourceHasMentions — verifies findByPersonIdWithMentionsFetched is called and blocks are saved with updated personId.
2. mergePersons_deduplicatesMentions_whenTargetAlreadyMentionedInBlock — verifies the source sidecar row is removed, not doubled.
3. mergePersons_rewritesBlockText_whenDisplayNamesDiffer — verifies block text is rewritten.
4. mergePersons_doesNotRewriteBlockText_whenDisplayNamesAreEqual — verifies no unnecessary text modification.
Add an integration test class PersonMergeIntegrationTest using Testcontainers (postgres:16-alpine) covering:
- Full merge with sender, receiver, alias, relationship, and sidecar mention.
- Deduplication: document with source as receiver where target is already a receiver — assert exactly one receiver row for target after merge.
- Constraint enforcement: attempt to insert a duplicate sidecar row via a mocked mid-merge failure — assert rollback.
Add a Playwright E2E test in frontend/e2e/ for the merge flow: navigate to a person's edit page, pick a merge target, confirm, assert redirect to target person's detail page.
Specific test name for the constraint-violation rollback scenario: merge_rollsBack_whenUniqueConstraintViolatedMidTransaction — this documents the atomicity requirement that cannot be verified at the Mockito layer.

Open Decisions

Scope of integration test: Should PersonMergeIntegrationTest be a @SpringBootTest (full context) or a @DataJpaTest (JPA layer only)? Given that mergePersons now crosses domain boundaries (person + transcription), a full @SpringBootTest is simpler to wire correctly, though slower. The trade-off is CI time vs. setup complexity.

## 🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist ### Observations - The existing unit tests in `PersonServiceTest` cover: source==target guard, source not found, target not found, and the happy-path document reassignment. They use Mockito + `@ExtendWith(MockitoExtension.class)` — correct layer. However, none of the new behaviors have test scaffolding yet. - The happy-path test `mergePersons_reassignsDocumentsAndDeletesSource` will need to be extended (or a new test added) to mock `TranscriptionBlockRepository.findByPersonIdWithMentionsFetched` and verify the sidecar update path. Currently the `PersonService` does not inject `TranscriptionBlockRepository`, so any new mock must be added once the dependency is wired. - The deduplication branch (block already mentions target) has zero test coverage. This is the riskiest code path — a missed deduplication creates a duplicate `(block_id, person_id)` row that violates the `UNIQUE` constraint from V57, causing a 500 at runtime. This path **must** have a unit test before the PR merges. - The block-text rewrite branch (`source.getDisplayName() != target.getDisplayName()`) also has no test coverage. The regex behavior (boundary anchoring, case sensitivity, partial match avoidance) cannot be verified without a test. - Integration tests are needed to cover the full transactional boundary. The current `PersonServiceTest` is pure Mockito — there is no `@SpringBootTest` or Testcontainers integration test for `mergePersons`. A partial-failure scenario (e.g., unique constraint violation mid-merge) cannot be verified without a real PostgreSQL instance. - The relationship cascade-delete behavior (V54 `ON DELETE CASCADE`) means the existing integration test suite may pass even though relationships are silently dropped — no assertion exists that checks relationship counts before and after merge. - The frontend merge flow has no E2E Playwright test. A merge is the most destructive user-facing operation in the person domain — it should have at least one happy-path E2E test confirming the redirect to the target person and one error-path test confirming the error message is displayed. ### Recommendations - Write these unit tests before any production code changes (TDD red phase): 1. `mergePersons_transfersMentions_whenSourceHasMentions` — verifies `findByPersonIdWithMentionsFetched` is called and blocks are saved with updated `personId`. 2. `mergePersons_deduplicatesMentions_whenTargetAlreadyMentionedInBlock` — verifies the source sidecar row is removed, not doubled. 3. `mergePersons_rewritesBlockText_whenDisplayNamesDiffer` — verifies block text is rewritten. 4. `mergePersons_doesNotRewriteBlockText_whenDisplayNamesAreEqual` — verifies no unnecessary text modification. - Add an integration test class `PersonMergeIntegrationTest` using Testcontainers (`postgres:16-alpine`) covering: - Full merge with sender, receiver, alias, relationship, and sidecar mention. - Deduplication: document with source as receiver where target is already a receiver — assert exactly one receiver row for target after merge. - Constraint enforcement: attempt to insert a duplicate sidecar row via a mocked mid-merge failure — assert rollback. - Add a Playwright E2E test in `frontend/e2e/` for the merge flow: navigate to a person's edit page, pick a merge target, confirm, assert redirect to target person's detail page. - Specific test name for the constraint-violation rollback scenario: `merge_rollsBack_whenUniqueConstraintViolatedMidTransaction` — this documents the atomicity requirement that cannot be verified at the Mockito layer. ### Open Decisions - **Scope of integration test**: Should `PersonMergeIntegrationTest` be a `@SpringBootTest` (full context) or a `@DataJpaTest` (JPA layer only)? Given that `mergePersons` now crosses domain boundaries (person + transcription), a full `@SpringBootTest` is simpler to wire correctly, though slower. The trade-off is CI time vs. setup complexity.

marcel commented

2026-05-07 17:39:43 +02:00

📋 Elicit — Requirements Engineer

Observations

The issue is well-specified and above average for a Gitea feature issue. The acceptance criteria use Given-When-Then structure, the data domain table is complete, and the OQ register is explicit. The code audit comment (#5484) sharpens the scope significantly and resolves OQ-3.
OQ-1 and OQ-2 are marked open but answered by the code: OQ-1 (does merge handle person_aliases?) — Person.nameAliases has CascadeType.ALL, orphanRemoval = true. When deleteById(sourceId) fires, JPA cascades deletion to all aliases. Aliases are not transferred, they are deleted. OQ-2 (does merge handle person_relationships?) — V54 uses ON DELETE CASCADE on both FK columns. Relationships are also deleted, not transferred. Both OQs should be closed with these answers in the issue, since an implementer reading only the issue body would not find them.
The acceptance criteria state "All family relationships of S are transferred to T" and "All aliases of S are added to T" — but comment #5484 explicitly out-scopes both as "pre-existing limitations." This is a contradiction between the issue body's AC and the audit comment. The two sources need to be reconciled before implementation starts, otherwise the implementer must guess which to follow.
The "block.text rewrite" requirement depends on PersonMentionPropagationListener.rewriteBlockText, which is introduced by PR-A #366 (not yet merged). The issue does not explicitly block on #366. This is a hidden dependency that will cause a compilation failure if the implementer starts work before #366 lands.
The UI warning text in PersonMergePanel.svelte only mentions that the source person will be deleted. It does not mention that @mentions in transcription blocks will be rewritten, or that any person with this name in existing transcription text will change. For a user who typed @Auguste Alt six months ago, seeing that text silently change to @Auguste Neu could be alarming. The UI should communicate what the merge affects.
NFR gap: the issue specifies atomicity but not performance. If a source person has hundreds of @mentions across thousands of blocks, the findByPersonIdWithMentionsFetched + saveAllAndFlush loop runs inside a single transaction. No size guard or async fallback is specified. For the current archive scale this is acceptable, but it is worth documenting as a known constraint.

Recommendations

Close OQ-1 and OQ-2 in the issue with the answers found in code: aliases are cascade-deleted (not transferred), relationships are cascade-deleted (not transferred). Explicitly state whether the acceptance criteria for these two items are in-scope or deferred, so no ambiguity remains for the implementer.
Add a Blocks on: #366 link in the issue so the dependency is visible in Gitea's UI.
Add a sentence to the person_merge_warning i18n key (currently used in PersonMergePanel.svelte) describing that @mentions in transcription blocks will be updated: e.g., "Alle @-Erwähnungen von [Quelle] in Transkriptionen werden auf [Ziel] umgeschrieben."
Document the scale constraint explicitly in the issue: "This implementation is optimized for the expected archive size (< 10,000 blocks total). If block count exceeds 50,000, consider batching the sidecar update."

Open Decisions

Alias transfer: The issue body requires alias transfer in AC; comment #5484 out-scopes it. One of these must be authoritative. Recommend keeping alias transfer in-scope for this issue, since CascadeType.ALL means the current code actively deletes them — a user who merges two persons loses the source's aliases without warning, which is a data-loss regression, not a missing feature.

## 📋 Elicit — Requirements Engineer ### Observations - The issue is well-specified and above average for a Gitea feature issue. The acceptance criteria use Given-When-Then structure, the data domain table is complete, and the OQ register is explicit. The code audit comment (#5484) sharpens the scope significantly and resolves OQ-3. - **OQ-1 and OQ-2 are marked open but answered by the code**: OQ-1 (does merge handle `person_aliases`?) — `Person.nameAliases` has `CascadeType.ALL, orphanRemoval = true`. When `deleteById(sourceId)` fires, JPA cascades deletion to all aliases. Aliases are **not transferred, they are deleted**. OQ-2 (does merge handle `person_relationships`?) — V54 uses `ON DELETE CASCADE` on both FK columns. Relationships are **also deleted, not transferred**. Both OQs should be closed with these answers in the issue, since an implementer reading only the issue body would not find them. - The acceptance criteria state "All family relationships of S are transferred to T" and "All aliases of S are added to T" — but comment #5484 explicitly out-scopes both as "pre-existing limitations." This is a contradiction between the issue body's AC and the audit comment. The two sources need to be reconciled before implementation starts, otherwise the implementer must guess which to follow. - The "block.text rewrite" requirement depends on `PersonMentionPropagationListener.rewriteBlockText`, which is introduced by PR-A #366 (not yet merged). The issue does not explicitly block on #366. This is a hidden dependency that will cause a compilation failure if the implementer starts work before #366 lands. - The UI warning text in `PersonMergePanel.svelte` only mentions that the source person will be deleted. It does not mention that @mentions in transcription blocks will be rewritten, or that any person with this name in existing transcription text will change. For a user who typed `@Auguste Alt` six months ago, seeing that text silently change to `@Auguste Neu` could be alarming. The UI should communicate what the merge affects. - NFR gap: the issue specifies atomicity but not performance. If a source person has hundreds of @mentions across thousands of blocks, the `findByPersonIdWithMentionsFetched` + `saveAllAndFlush` loop runs inside a single transaction. No size guard or async fallback is specified. For the current archive scale this is acceptable, but it is worth documenting as a known constraint. ### Recommendations - Close OQ-1 and OQ-2 in the issue with the answers found in code: aliases are cascade-deleted (not transferred), relationships are cascade-deleted (not transferred). Explicitly state whether the acceptance criteria for these two items are in-scope or deferred, so no ambiguity remains for the implementer. - Add a `Blocks on: #366` link in the issue so the dependency is visible in Gitea's UI. - Add a sentence to the `person_merge_warning` i18n key (currently used in `PersonMergePanel.svelte`) describing that @mentions in transcription blocks will be updated: e.g., "Alle @-Erwähnungen von [Quelle] in Transkriptionen werden auf [Ziel] umgeschrieben." - Document the scale constraint explicitly in the issue: "This implementation is optimized for the expected archive size (< 10,000 blocks total). If block count exceeds 50,000, consider batching the sidecar update." ### Open Decisions - **Alias transfer**: The issue body requires alias transfer in AC; comment #5484 out-scopes it. One of these must be authoritative. Recommend keeping alias transfer in-scope for this issue, since `CascadeType.ALL` means the current code actively deletes them — a user who merges two persons loses the source's aliases without warning, which is a data-loss regression, not a missing feature.

marcel commented

2026-05-07 17:40:04 +02:00

🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead

Observations

PersonMergePanel.svelte follows the card pattern correctly (rounded-sm border border-line bg-surface p-6 shadow-sm) with the danger zone styled in border-red-200 bg-surface — appropriately signaling a destructive section without using raw hex values.
The two-step confirmation (first click reveals confirm/cancel, second click submits) is good error-prevention practice (Nielsen H5). The confirm button is clearly labeled and the source name is displayed in the warning text.
The warning text (person_merge_warning + person_merge_will_be_deleted) currently says the source person will be deleted. It does not mention @mention rewrites, alias deletion, or relationship removal. For the senior audience (60+), a merge is irreversible — the warning must enumerate what will be lost, not just that the person record disappears.
The PersonTypeahead used as the merge target selector has no visible label in the panel — it passes label={m.person_merge_target_label()} as a prop, so whether the label renders depends on PersonTypeahead's internal implementation. Verify that the label is associated with the input (<label for="..."> or wrapping pattern) for screen-reader accessibility.
The confirm button (text-white bg-red-600) and cancel button (text-ink-2 border-line) are visually distinct and keyboard-focusable. Touch target: the px-4 py-2 buttons are approximately 36px tall — below the 44px WCAG 2.2 minimum for a senior audience. The primary Merge button should use min-h-[44px].
Error messages (form?.mergeError) are displayed in text-sm text-red-600. Red text on white at this size needs a contrast check — text-red-600 (#DC2626) on white is approximately 5.9:1, which passes WCAG AA for normal text. No finding here.
The mergeTargetId hidden input uses bind:value which is Svelte 5 compatible in this context. However, if PersonTypeahead calls onchange with an empty string when cleared, mergeTargetId becomes '' and the disabled={!mergeTargetId} guard correctly prevents submission. Verify that clearing the typeahead resets showMergeConfirm = false — the current code does reset it (showMergeConfirm = false in the onchange handler). Good.

Recommendations

Expand the merge warning text to enumerate consequences: mention that @mentions in transcription texts will be rewritten to the target person's name, and that the source person cannot be recovered. Example i18n key content: "Die Person [Quelle] wird gelöscht. Alle Dokumente, Beziehungen und @-Erwähnungen werden auf [Ziel] übertragen. Diese Aktion kann nicht rückgängig gemacht werden."
Increase the Merge confirm button to min-h-[44px]: change class="rounded bg-red-600 px-4 py-2 ..." to class="rounded bg-red-600 min-h-[44px] px-4 py-2 ...". This is a WCAG 2.2 SC 2.5.8 requirement for the target audience.
Verify PersonTypeahead renders a proper <label> element associated to its <input> — run axe-playwright on the person edit page to confirm no label-missing violation.
Consider adding a summary of what will be merged in the confirmation step, similar to GitHub's repository deletion confirmation that lists what will be deleted. Even a simple bulleted list of "documents will be reassigned", "aliases will be removed", "@mentions will be rewritten" reduces surprise for users who encounter this for the first time.

## 🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead ### Observations - `PersonMergePanel.svelte` follows the card pattern correctly (`rounded-sm border border-line bg-surface p-6 shadow-sm`) with the danger zone styled in `border-red-200 bg-surface` — appropriately signaling a destructive section without using raw hex values. - The two-step confirmation (first click reveals confirm/cancel, second click submits) is good error-prevention practice (Nielsen H5). The confirm button is clearly labeled and the source name is displayed in the warning text. - The warning text (`person_merge_warning` + `person_merge_will_be_deleted`) currently says the source person will be deleted. It does not mention @mention rewrites, alias deletion, or relationship removal. For the senior audience (60+), a merge is irreversible — the warning must enumerate what will be lost, not just that the person record disappears. - The `PersonTypeahead` used as the merge target selector has no visible label in the panel — it passes `label={m.person_merge_target_label()}` as a prop, so whether the label renders depends on `PersonTypeahead`'s internal implementation. Verify that the label is associated with the input (`<label for="...">` or wrapping pattern) for screen-reader accessibility. - The confirm button (`text-white bg-red-600`) and cancel button (`text-ink-2 border-line`) are visually distinct and keyboard-focusable. Touch target: the `px-4 py-2` buttons are approximately 36px tall — below the 44px WCAG 2.2 minimum for a senior audience. The primary Merge button should use `min-h-[44px]`. - Error messages (`form?.mergeError`) are displayed in `text-sm text-red-600`. Red text on white at this size needs a contrast check — `text-red-600` (#DC2626) on white is approximately 5.9:1, which passes WCAG AA for normal text. No finding here. - The `mergeTargetId` hidden input uses `bind:value` which is Svelte 5 compatible in this context. However, if `PersonTypeahead` calls `onchange` with an empty string when cleared, `mergeTargetId` becomes `''` and the `disabled={!mergeTargetId}` guard correctly prevents submission. Verify that clearing the typeahead resets `showMergeConfirm = false` — the current code does reset it (`showMergeConfirm = false` in the `onchange` handler). Good. ### Recommendations - Expand the merge warning text to enumerate consequences: mention that @mentions in transcription texts will be rewritten to the target person's name, and that the source person cannot be recovered. Example i18n key content: "Die Person **[Quelle]** wird gelöscht. Alle Dokumente, Beziehungen und @-Erwähnungen werden auf **[Ziel]** übertragen. Diese Aktion kann nicht rückgängig gemacht werden." - Increase the Merge confirm button to `min-h-[44px]`: change `class="rounded bg-red-600 px-4 py-2 ..."` to `class="rounded bg-red-600 min-h-[44px] px-4 py-2 ..."`. This is a WCAG 2.2 SC 2.5.8 requirement for the target audience. - Verify `PersonTypeahead` renders a proper `<label>` element associated to its `<input>` — run `axe-playwright` on the person edit page to confirm no label-missing violation. - Consider adding a summary of what will be merged in the confirmation step, similar to GitHub's repository deletion confirmation that lists what will be deleted. Even a simple bulleted list of "documents will be reassigned", "aliases will be removed", "@mentions will be rewritten" reduces surprise for users who encounter this for the first time.

marcel commented

2026-05-07 17:40:18 +02:00

⚙️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

Observations

This issue is a pure application-logic change — no new Docker services, no schema-breaking migration, no new infrastructure dependencies. The DevOps surface is minimal.
The new step calls TranscriptionBlockRepository.findByPersonIdWithMentionsFetched and saveAllAndFlush inside the merge transaction. Both are standard JPA operations against the existing PostgreSQL instance — no new queries to tune at the infrastructure layer.
The UNIQUE constraint in V57 (uq_tbmp_block_person) is already applied. The application-layer deduplication described in the issue is the correct approach; there is no need for a new migration.
@ElementCollection(fetch = FetchType.EAGER) on mentionedPersons means every TranscriptionBlock load fetches the sidecar rows. For a merge touching many blocks, this will issue N+1 JPA queries unless the existing findByPersonIdWithMentionsFetched (which uses JOIN FETCH) is used — and it is, per the issue spec. No N+1 concern if the implementation follows the spec.
The existing pg_isready healthcheck and Flyway migration test will catch any schema-level regressions in CI. No additional infrastructure gate is needed.
One operational concern: if the merge operation is called on a person with a very large number of @mentions (hypothetically thousands of blocks), the @Transactional boundary holds an open DB connection for the full duration of the saveAllAndFlush. For the current archive scale this is not a problem, but it is worth noting in the code comment.

Recommendations

No Compose file changes required for this issue.
Confirm in the CI integration test that the Flyway migration sequence is unbroken after this PR — specifically that V57 (unique constraint) is applied before any test attempts a merge. The @SpringBootTest with Testcontainers will verify this automatically since Flyway runs all migrations in order.
Add a comment inside mergePersons noting the operational scale constraint: // Scale note: linear in blocks-mentioning-source. Acceptable for archive scale (~10k blocks); revisit if block count exceeds 100k.
No new Docker services, Caddyfile rules, or CI workflow changes are needed for this feature.

## ⚙️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer ### Observations - This issue is a pure application-logic change — no new Docker services, no schema-breaking migration, no new infrastructure dependencies. The DevOps surface is minimal. - The new step calls `TranscriptionBlockRepository.findByPersonIdWithMentionsFetched` and `saveAllAndFlush` inside the merge transaction. Both are standard JPA operations against the existing PostgreSQL instance — no new queries to tune at the infrastructure layer. - The `UNIQUE` constraint in V57 (`uq_tbmp_block_person`) is already applied. The application-layer deduplication described in the issue is the correct approach; there is no need for a new migration. - `@ElementCollection(fetch = FetchType.EAGER)` on `mentionedPersons` means every `TranscriptionBlock` load fetches the sidecar rows. For a merge touching many blocks, this will issue N+1 JPA queries unless the existing `findByPersonIdWithMentionsFetched` (which uses `JOIN FETCH`) is used — and it is, per the issue spec. No N+1 concern if the implementation follows the spec. - The existing `pg_isready` healthcheck and Flyway migration test will catch any schema-level regressions in CI. No additional infrastructure gate is needed. - One operational concern: if the merge operation is called on a person with a very large number of @mentions (hypothetically thousands of blocks), the `@Transactional` boundary holds an open DB connection for the full duration of the `saveAllAndFlush`. For the current archive scale this is not a problem, but it is worth noting in the code comment. ### Recommendations - No Compose file changes required for this issue. - Confirm in the CI integration test that the Flyway migration sequence is unbroken after this PR — specifically that V57 (unique constraint) is applied before any test attempts a merge. The `@SpringBootTest` with Testcontainers will verify this automatically since Flyway runs all migrations in order. - Add a comment inside `mergePersons` noting the operational scale constraint: `// Scale note: linear in blocks-mentioning-source. Acceptable for archive scale (~10k blocks); revisit if block count exceeds 100k.` - No new Docker services, Caddyfile rules, or CI workflow changes are needed for this feature.

marcel commented

2026-05-07 17:40:43 +02:00

Decision Queue — Consolidated Open Items

The following decisions were raised across the persona reviews. All require a human call before implementation begins.

Theme 1: Scope of alias and relationship transfer

Raised by: Markus (architect), Elicit (requirements)

The issue body's acceptance criteria explicitly requires alias transfer ("All aliases of S are added to T") and relationship transfer ("All family relationships of S are transferred to T"). Comment #5484 explicitly out-scopes both as "pre-existing limitations."

Code confirms: aliases are cascade-deleted via CascadeType.ALL, orphanRemoval = true on Person.nameAliases. Relationships are cascade-deleted via ON DELETE CASCADE in V54. Both are silent deletions — the user gets no indication that this data is lost.

Decision needed: Are alias transfer and relationship transfer in-scope for this issue, or explicitly deferred? If deferred, the acceptance criteria in the issue body must be updated to remove those items, and a separate issue should be opened to track the data-loss behavior.

Theme 2: Blocking dependency on PR-A #366

Raised by: Markus (architect), Elicit (requirements)

rewriteBlockText (referenced in the issue's recommended approach) does not exist in the current codebase. It is introduced by PR-A #366 (PersonMentionPropagationListener), which is still open.

Decision needed: Should this issue formally block on #366 merging first? Or should this issue include the rewriteBlockText implementation as part of its own scope? Either is acceptable, but the issue should state the choice explicitly.

Theme 3: Regex safety in `rewriteBlockText`

Raised by: Nora (security)

The block-text rewrite uses a boundary-anchored regex where the pattern is built from a person's display name. If the display name contains regex metacharacters (e.g., Auguste (Müller)), the regex engine will misinterpret them. This needs Pattern.quote() applied to the display name before constructing the pattern.

Decision needed: Confirm that PR-A #366's rewriteBlockText implementation uses Pattern.quote(). If it does not, add it as part of this issue. This is a correctness bug, not a hypothetical — the archive contains names with parentheses and hyphens.

Theme 4: Integration test scope

Raised by: Sara (QA)

mergePersons crosses the person and transcription domain boundaries. A @DataJpaTest slice cannot wire both repositories easily; a @SpringBootTest with Testcontainers is simpler to set up but slower (~15s vs ~2s).

Decision needed: Use @SpringBootTest for PersonMergeIntegrationTest (simpler wiring) or @DataJpaTest (faster, more focused). Given the cross-domain nature of the merge, @SpringBootTest is the recommended choice — confirm before the test is written.

Theme 5: Audit logging for merge operations

Raised by: Nora (security)

A merge is irreversible and destructive. Without an audit log entry, there is no record of who merged whom and when. The AuditService already exists in the audit/ package.

Decision needed: Should mergePersons emit an audit log entry? Recommendation from all personas: yes. Confirm so the implementer can wire AuditService without a separate PR.

## Decision Queue — Consolidated Open Items The following decisions were raised across the persona reviews. All require a human call before implementation begins. --- ### Theme 1: Scope of alias and relationship transfer **Raised by**: Markus (architect), Elicit (requirements) The issue body's acceptance criteria explicitly requires alias transfer ("All aliases of S are added to T") and relationship transfer ("All family relationships of S are transferred to T"). Comment #5484 explicitly out-scopes both as "pre-existing limitations." Code confirms: aliases are cascade-deleted via `CascadeType.ALL, orphanRemoval = true` on `Person.nameAliases`. Relationships are cascade-deleted via `ON DELETE CASCADE` in V54. Both are silent deletions — the user gets no indication that this data is lost. **Decision needed**: Are alias transfer and relationship transfer in-scope for this issue, or explicitly deferred? If deferred, the acceptance criteria in the issue body must be updated to remove those items, and a separate issue should be opened to track the data-loss behavior. --- ### Theme 2: Blocking dependency on PR-A #366 **Raised by**: Markus (architect), Elicit (requirements) `rewriteBlockText` (referenced in the issue's recommended approach) does not exist in the current codebase. It is introduced by PR-A #366 (`PersonMentionPropagationListener`), which is still open. **Decision needed**: Should this issue formally block on #366 merging first? Or should this issue include the `rewriteBlockText` implementation as part of its own scope? Either is acceptable, but the issue should state the choice explicitly. --- ### Theme 3: Regex safety in `rewriteBlockText` **Raised by**: Nora (security) The block-text rewrite uses a boundary-anchored regex where the pattern is built from a person's display name. If the display name contains regex metacharacters (e.g., `Auguste (Müller)`), the regex engine will misinterpret them. This needs `Pattern.quote()` applied to the display name before constructing the pattern. **Decision needed**: Confirm that PR-A #366's `rewriteBlockText` implementation uses `Pattern.quote()`. If it does not, add it as part of this issue. This is a correctness bug, not a hypothetical — the archive contains names with parentheses and hyphens. --- ### Theme 4: Integration test scope **Raised by**: Sara (QA) `mergePersons` crosses the person and transcription domain boundaries. A `@DataJpaTest` slice cannot wire both repositories easily; a `@SpringBootTest` with Testcontainers is simpler to set up but slower (~15s vs ~2s). **Decision needed**: Use `@SpringBootTest` for `PersonMergeIntegrationTest` (simpler wiring) or `@DataJpaTest` (faster, more focused). Given the cross-domain nature of the merge, `@SpringBootTest` is the recommended choice — confirm before the test is written. --- ### Theme 5: Audit logging for merge operations **Raised by**: Nora (security) A merge is irreversible and destructive. Without an audit log entry, there is no record of who merged whom and when. The `AuditService` already exists in the `audit/` package. **Decision needed**: Should `mergePersons` emit an audit log entry? Recommendation from all personas: yes. Confirm so the implementer can wire `AuditService` without a separate PR.

Sign in to join this conversation.

1 Participants

Notifications

Due Date

No due date set.

Dependencies

No dependencies set.

Reference: marcel/familienarchiv#368

feat(persons): audit + complete person-merge flow for all data domains #368

Context

Data domains that hold person references

The @mention gap (introduced by PR-A #366)

What must happen

Recommended approach

Document deduplication edge case

Family relationship edge case

AppUser link guard

Atomicity requirement

Acceptance criteria

Open questions

Related

❌ @mention sidecar — the actual gap

🏛️ Markus Keller (@mkeller) — Application Architect

Observations

Recommendations

Open Decisions

👨‍💻 Felix Brandt (@felixbrandt) — Fullstack Developer

Observations

Recommendations

🔒 Nora "NullX" Steiner — Application Security Engineer

Observations

Recommendations

Open Decisions

🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist

Observations

Recommendations

Open Decisions

📋 Elicit — Requirements Engineer

Observations

Recommendations

Open Decisions

🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead

Observations

Recommendations

⚙️ Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer

Observations

Recommendations

Decision Queue — Consolidated Open Items

Theme 1: Scope of alias and relationship transfer

Theme 2: Blocking dependency on PR-A #366

Theme 3: Regex safety in rewriteBlockText

Theme 4: Integration test scope

Theme 5: Audit logging for merge operations

Theme 3: Regex safety in `rewriteBlockText`