feat(geschichte): JourneyItem CRUD API — append, updateNote, delete, reorder (#751) #788
Reference in New Issue
Block a user
Delete Branch "feat/issue-751-journey-item-crud-api"
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?
Summary
Implements the backend JourneyItem CRUD API on top of the data model from #750, building towards the full Lesereisen feature (#751).
Completed in this PR:
jackson-databind-nullable0.2.6 +JacksonConfig(@Bean Module) for three-way PATCH semantics (JsonNullable)AuditKind:JOURNEY_ITEM_ADDED,JOURNEY_ITEM_REMOVED,JOURNEY_ITEMS_REORDERED(last is rollup-eligible)ErrorCode:JOURNEY_ITEM_NOT_FOUND,JOURNEY_ITEM_POSITION_CONFLICTUNIQUE (geschichte_id, position) DEFERRABLE INITIALLY DEFERRED+CHECK (position > 0)onjourney_itemsJourneyItemConstraintsTest: verifies deferrable flag viapg_constraintquery; position check; duplicate position rejection (3 passing tests)DocumentSummary,JourneyItemView,GeschichteView(withAuthorViewto prevent AppUser email leak)DocumentService.getSummaryById— lean lookup without tag-color resolutionJourneyItemRepository: extended withfindByGeschichteIdOrderByPosition,findByIdAndGeschichteId(IDOR-safe),findIdsByGeschichteId,findMaxPositionByGeschichteId,countByGeschichteIdJourneyItemCreateDTO,JourneyItemUpdateDTO(JsonNullable<String> note),JourneyReorderDTOStill in progress (WIP):
JourneyItemService—append,updateNote,delete,reorder,toSummary,toView(Task 6)GeschichteService.getById→ returnsGeschichteView(Task 7)GeschichteController+ slice tests (Task 8)npm run generate:api(Task 9)Commits
0b177247feat(config): add jackson-databind-nullable for JsonNullable PATCH DTO support408ae334feat(audit,error): add JourneyItem AuditKind values and ErrorCodes7b06c3adfeat(migration): V73 adds UNIQUE DEFERRABLE and CHECK position > 0 on journey_items160ca1c3feat(geschichte): add DocumentSummary, JourneyItemView, GeschichteView read models2ad5c36efeat(geschichte): extend JourneyItemRepository and add item DTOsTest plan
./mvnw test -Dtest=JourneyItemConstraintsTest— all 3 constraint tests pass./mvnw clean package -DskipTests— builds clean after remaining tasks are mergednpm run generate:apiafter Task 9 endpoint additionsDocumentSummary: lean document projection for journey item embedding — skips tag-color resolution (getSummaryById), includes receiverCount (0 when no receivers, non-null). JourneyItemView: response record for item CRUD and GET. GeschichteView: detail response with summarised author {id, displayName} to prevent AppUser email/group leak. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Solid TDD execution. 60 tests, all green. The
Optional<String>three-way PATCH semantics solution is clever and well-documented. Layering is clean.Blockers
Dead import in
GeschichteView.javaThis import exists in the file but
@JsonIncludeis never used. Remove it.findAllByGeschichteIdorphan inJourneyItemRepositoryThe original
findAllByGeschichteId(UUID)is still in the repo, now superseded everywhere byfindByGeschichteIdOrderByPosition(UUID). Two methods that do the same thing (unordered vs ordered) invite the wrong one being called in future code. Either removefindAllByGeschichteIdor document the distinction with a comment on when each is appropriate. I'd remove it.Suggestions
toSummary()andtoView()arepublicbut only used internallyBoth are called only within
JourneyItemServiceitself (toViewis also called bygetItems,append,updateNote,reorder). The only external caller isGeschichteService.getItems()→JourneyItemService.getItems(). ThetoSummary(Document)method is an implementation detail — it should be package-private. If tests need it, they're in the same package (they are —journeyitem), so package-private is fine.getItems()lacks@Transactional(readOnly = true)Every other read method in the project follows the convention of
@Transactional(readOnly = true)for reads that return entity graphs.getItems()callstoView()which callsdocumentService.getSummaryById()— but since there's no lazy association traversal in the view path now, this is minor. Still worth adding for consistency.savedItemWithDoc()factory inJourneyItemServiceTestis never calledDead test code. Remove it.
PR description is stale
The "Still in progress (WIP)" section lists Tasks 6–9 as pending, but they're all done and committed. Update the description to reflect the actual completed state.
🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: 🚫 Changes requested
Good IDOR protection via
findByIdAndGeschichteId. Good auth on all four endpoints with@RequirePermission(Permission.BLOG_WRITE). TheAuthorViewemail containment is correct. One critical gap needs to be fixed before merge.Blockers
JOURNEY_ITEM_POSITION_CONFLICTis defined but never thrown — concurrent write causes HTTP 500ErrorCode.JOURNEY_ITEM_POSITION_CONFLICTwas added inErrorCode.java, mirrored infrontend/src/lib/shared/errors.ts, and has i18n keys. But no code inJourneyItemServicecatches theDataIntegrityViolationExceptionthat PostgreSQL throws when two concurrentappendorreordercalls hit the DEFERRABLE UNIQUE constraint at commit time.Current behavior on a race: the
DataIntegrityViolationExceptionpropagates toGlobalExceptionHandler, which maps unknown exceptions toINTERNAL_ERROR+ HTTP 500. The client gets a 500 with no useful error code instead of a 409.Fix in
JourneyItemService:Apply the same catch to
reorder(). The DEFERRABLE constraint fires at transaction commit, so thesave()call inside the transaction won't throw — the exception fires when the@Transactionalproxy commits. You'll need to catch it at the service boundary (the public method level), or catch it in the controller and wrap it there.Actually — because the constraint is DEFERRABLE INITIALLY DEFERRED, the exception fires at the commit point, which is at the end of the
@Transactionalmethod, outside the try-catch you'd write inside the method body. The correct place is to translate it inGlobalExceptionHandler:Add a test verifying the 409 mapping in
GeschichteControllerTestor a targetedDataIntegrityViolationException→ 409 test.Suggestions
displayNamefallback to email is intentional — documented, acceptableWhen
firstNameandlastNameare both blank,displayNamefalls back to the user's email. This means the author's email is visible in the public story view. This is a product decision, not a bug (theAuthorViewrecord itself doesn't have an email field, but the display value IS the email). ADR-035 should note this trade-off explicitly.currentUser()performs a DB lookup on every write operationEvery write path (
append,updateNote,delete,reorder) callscurrentUser()which callsuserService.findByEmail(auth.getName())— a database query per request just to get the actor UUID for the audit log. This is consistent with the rest of the codebase (checkedGeschichteService) so not a blocker, but worth tracking as a future optimization (cache inSecurityContextHolderor load from Principal).🏛️ Markus Keller — Application Architect
Verdict: 🚫 Changes requested
Clean layering. The
GeschichteRepositoryinjection intoJourneyItemService(instead ofGeschichteService) correctly avoids a constructor injection cycle — Spring Framework 7 would refuse the cycle. Good ADR for theOptional<String>decision.Blockers
DB constraint diagrams not updated for V73
V73 adds two constraints to
journey_items:UNIQUE (geschichte_id, position) DEFERRABLE INITIALLY DEFERREDCHECK (position > 0)Per the doc update rule: any Flyway migration that modifies a table requires updating
docs/architecture/db/db-orm.pumlanddocs/architecture/db/db-relationships.puml. This PR updates neither. The constraint inGLOSSARY.mdis updated, but the puml diagrams are not.This is a blocker per the team's documentation rule — the PR does not merge until the diagram matches the migration.
C4 L3 backend diagram not updated for new
JourneyItemServiceJourneyItemServiceis a new service in thegeschichtedomain with cross-domain calls toDocumentServiceandAuditService. The matchingdocs/architecture/c4/l3-backend-*.pumlshould reflect this. Check whether al3-backend-geschichte.pumlexists and, if so, addJourneyItemServiceas a component with its relationships.Suggestions
findAllByGeschichteIdsuperseded but not removedJourneyItemRepositorynow has:findAllByGeschichteId(UUID)— original, unorderedfindByGeschichteIdOrderByPosition(UUID)— new, orderedOnly
findByGeschichteIdOrderByPositionis used in the codebase. The unordered variant is a footgun for any future caller. Remove it.GeschichteService→JourneyItemService→GeschichteRepositorycouplingGeschichteServicedepends onJourneyItemService;JourneyItemServicedepends onGeschichteRepository. This creates a partial cross-dependency that's not a cycle but is worth noting:JourneyItemServicebypassesGeschichteServiceto load theGeschichteentity directly. This is intentional (to avoid the cycle) and documented implicitly, but the choice should be stated in the service class Javadoc or ADR-035.PR description "Still in progress" section is stale
Minor, but the description should be updated to reflect that Tasks 6–9 are complete.
🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
Good test coverage structure. 35 unit tests in
JourneyItemServiceTest+ 25 slice tests inGeschichteControllerTest+ 3 constraint tests inJourneyItemConstraintsTest= 63 tests, all green. Auth boundary tests (401/403) on all four endpoints are thorough.Blockers
No test for
DataIntegrityViolationException→ 409 mapping (JOURNEY_ITEM_POSITION_CONFLICT)JOURNEY_ITEM_POSITION_CONFLICTis defined and has i18n keys, but there is no test verifying that aDataIntegrityViolationExceptionfrom the DEFERRABLE UNIQUE constraint gets translated to HTTP 409 with that code. As-is, a concurrent append returns HTTP 500.Add to
GeschichteControllerTest:Suggestions
Dead factory method in
JourneyItemServiceTestsavedItemWithDoc(UUID, Geschichte, int, Document, String)is defined but never called. Dead code in tests is noise — it suggests coverage that doesn't exist. Remove it or add a test that uses it.Missing service integration test
JourneyItemServiceTestis a pure unit test (Mockito). There's noJourneyItemServiceIntegrationTestwith a real Postgres container to verify that:The
JourneyItemConstraintsTestcovers raw SQL constraints, but the service-level behavior (max items, position gap computation) is only unit-tested with mocks. Consider adding integration coverage for theappendhappy path at minimum.GeschichteServiceTestcould test thatgetItemsis called with the rightgeschichteIdgetById_items_come_from_journeyItemServiceverifiesverify(journeyItemService).getItems(id)— good. But the test useslenient().when(journeyItemService.getItems(any()))in@BeforeEach, which means thegetById_items_come_from_journeyItemServicetest'swhen(journeyItemService.getItems(id))override is redundant and creates a potential stub-ordering ambiguity. Consolidate the stub.Missing 401 test for PATCH endpoint
appendItem_returns401_whenUnauthenticatedexists for POST. There is no corresponding 401 test forPATCH /{id}/items/{itemId}. The other endpoints only have 403 tests (not 401). 401 vs 403 are different security failures — adding one 401 test per endpoint would be consistent with the POST endpoint.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR — no Docker Compose modifications, no CI workflow changes, no new services. The changes are pure application code.
Checked:
.env.exampleupdatesjackson-databind-nullabledependency removal simplifiespom.xml— one fewer CVE surfaceapi.tschange is normal churn from anpm run generate:apirun against a live backend — no concernOne note for context: the PR description mentions that
npm run generate:apineeds to be re-run after the backend is rebuilt with the new endpoints. This is documented in the PR body and expected — no action needed from the infrastructure side, but whoever merges this should ensure the backend is rebuilt before the frontend is deployed so the generated types match what's running.📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Reviewing against the Lesereise feature requirements from issue #751.
Concerns
PR description is stale — "WIP" section needs updating
The PR description has a "Still in progress (WIP)" section listing Tasks 6–9 as pending. All four tasks are now implemented and committed. A reader of this PR cannot tell what's done vs. pending without reading the commit history. Update the description to reflect the actual state.
JOURNEY_ITEM_POSITION_CONFLICTis defined but has no code path that produces itFrom a requirements perspective: the ErrorCode is part of the public API contract — it appears in
ErrorCode.java,errors.ts, and all three message files. Users (or frontend code) will never see it in practice because the service never throws it. This is an incomplete feature: the error code is half-implemented. The specification implied by the code (JOURNEY_ITEM_POSITION_CONFLICT = concurrent write was detected) needs the actual throw path to be useful.No validation on
JourneyReorderDTO.itemIdsfor duplicate entriesIf the client sends
itemIds: ["uuid-A", "uuid-A", "uuid-B"](duplicates), the set-equality checkexistingIds.equals(new HashSet<>(requestedIds))will pass (the set deduplicates), but the loop will setuuid-Ato position 10, then overwrite with 20. The result is thatuuid-Aends up at position 20 and position 10 is unoccupied. This is a silent, incorrect state.Add a check:
Clarification: what happens when a
STORY-type Geschichte is passed toappendItem?append()throwsVALIDATION_ERROR: "Geschichte is not a JOURNEY — cannot append items". This is correct behavior but the error code is genericVALIDATION_ERROR, which makes it hard to display a specific user message. Consider whether a dedicatedErrorCode.GESCHICHTE_NOT_A_JOURNEYwould improve the frontend UX for this case. For now it's acceptable — just flagging for prioritization.LGTM items
documentIdornotemust be present on create is correctly enforced.cannot clear note on item with no document) prevents orphaned empty items.MAX_NOTE_LENGTH = 5000limit is a sensible NFR.🎨 UI/UX Expert
Verdict: ✅ Approved
No user-facing UI changes in this PR — this is a backend API layer with matching frontend error code registration.
Checked:
de.json,en.json,es.jsontranslations are idiomatic and match the error semantics"error_journey_item_not_found": "Der Reise-Eintrag wurde nicht gefunden."✓"error_journey_item_position_conflict": "Positionskonflikt beim Sortieren der Reise-Einträge."✓ (user-readable, not technical jargon)AuthorView.displayNamefallback to email when no name is set is a UX trade-off: the author's email becomes their public display name. Worth revisiting when the Lesereise reader UI is built — the reader UI will surface this in the story header.api.tsdiff showsGeschichteSummarytype was removed from the schema. This is a change to the frontend type surface — verify that no existing frontend code referencesGeschichteSummarybefore this lands.One note: The generated
api.tsdiff removesGeschichteSummaryandJourneyItemschema types and changes the list endpoint to returnGeschichte[]instead ofGeschichteSummary[]. This could be a regression in the frontend stories list if any component expected theGeschichteSummarytype shape (which hadauthor.emailexposed). This came from anpm run generate:apirun against a currently-running backend that may differ from the code in this PR — I'd flag this for verification when the backend is rebuilt.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: 🚫 Changes requested
Blockers
1.
l3-backend-3g-supporting.pumlnot updated —JourneyItemServicemissing from the C4 diagramPer the doc update table, adding a new service to an existing domain requires updating the matching
l3-backend-*.puml. The Geschichte domain lives inl3-backend-3g-supporting.puml. The diagram showsgeschCtrl → geschSvcbutJourneyItemServiceand its relationships (→ DB via journey_items, → DocumentService, → AuditService) are absent. Should add:2.
GeschichteView.personsisSet<Person>— raw JPA entity in the API responseGeschichteView.java:Personhas internal fields (provisional,sourceRef,generation,familyMember,notes) that are administrative import metadata — not view data. This PR introducedAuthorViewspecifically to prevent leakingAppUserinternals — the same principle applies toPerson. Use aPersonSummaryrecord (already exists atPersonSummaryDTOin the person domain) or a scopedPersonViewwith justid, display name, and birth/death years.Suggestions
3. Document-domain logic living in
JourneyItemServicebuildSenderName(),buildCanonicalReceiverName(),sortKey(),join()— these four private helpers accessDocument.getSender(),Document.getReceivers(), andPersonfields. This is document-to-summary mapping logic. It belongs inDocumentService, not in the journey item service.DocumentService.getSummaryById()currently returns rawDocument, forcingJourneyItemServiceto assembleDocumentSummaryitself. ConsiderDocumentService.toSummary(Document)or returningDocumentSummarydirectly fromgetSummaryById().4. Email fallback in
GeschichteService.toView()defeats theAuthorViewinvariantIf an author has neither first nor last name, their email ends up as the public
displayName.AuthorViewwas designed to never expose email. Replace with a neutral fallback:5. PR description still lists "Still in progress (WIP)" with Tasks 6–9 as incomplete
All tasks are done. Remove the WIP section before merge.
🔐 Nora Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
Blockers
None.
Concerns
1. Email leak via
AuthorView.displayNamewhen author has no name (CWE-359 — Exposure of Private Information)GeschichteService.toView():GET /api/geschichten/{id}is accessible to any authenticated user (READ_ALL, not BLOG_WRITE), so if any author account has a blank display name, their email address is returned in the public response. TheAuthorViewrecord was designed precisely to prevent this — but this fallback conditionally defeats it.Fix:
2.
getSummaryByIdloads documents regardless of status — no visibility checkDocumentService.getSummaryById()returns any document for any UUID, includingPLACEHOLDERdocuments created during import but never uploaded. A JourneyItem linked to a PLACEHOLDER document would surface unpublished document metadata (title, sender name) in the journey view. Confirm whether this is intentional for the family archive context, and if not, add:What's Done Well
findByIdAndGeschichteId— correct design, tested ✅@RequirePermission(Permission.BLOG_WRITE)on all write endpoints ✅constraintNameOf()prevents SQL/values leaking in constraint violation responses (CWE-209) ✅AuthorViewdesign intent is sound — the email fallback is the only gap🏛️ Markus Keller — Application Architect
Verdict: 🚫 Changes requested
Blockers
1.
l3-backend-3g-supporting.pumlnot updated — new service omittedJourneyItemServiceis a new Spring service in thegeschichtedomain. The documentation contract is clear: "New controller or service in an existing backend domain → update the matchingdocs/architecture/c4/l3-backend-*.puml." Geschichte domain →l3-backend-3g-supporting.puml. Add the component and its three relationships (→ geschCtrl, → geschSvc, → DB).2.
GeschichteView.personsisSet<Person>— raw entity in the API contractThe
Personentity exposesprovisional,sourceRef,generation,familyMember, and internalnotes— fields that are import/admin metadata, not view data. These serialize directly into the API response. The PR correctly introducedAuthorViewto avoid leakingAppUserinternals. The same principle needs to apply toPersoninGeschichteView. Use the existingPersonSummaryDTOor introduce a scopedPersonView(UUID id, String displayName, Integer birthYear, Integer deathYear).Suggestions
3.
toSummary(Document)and four private helpers belong in the document domainJourneyItemServicecontainsbuildSenderName(),buildCanonicalReceiverName(),sortKey(), andjoin()— all of which access internalDocumentandPersonfields. This is document-domain knowledge encoded in the journey-item domain. The correct layering would beDocumentService.toSummary(Document), keeping document→summary assembly in the domain that ownsDocument.What's Architecturally Sound
JourneyItemServiceinjectsGeschichteRepository(notGeschichteService) to break the Spring 7 constructor injection cycle — this is the correct pattern for this framework version ✅DocumentService.getSummaryById()avoids tag-color resolution overhead — correct lean-path design ✅Optional<String>decision with context, decision, and consequences ✅GeschichteService.toView()correctly avoidsHibernate.initialize()by using a repository query — clean solution to the LazyInitializationException problem ✅🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
71 tests passing across three layers (unit, controller slice, integration). Coverage is solid; a few gaps worth addressing before merge.
Blockers
None.
Concerns
1. Missing test:
appendrejected when Geschichte is type STORYJourneyItemService.append()throws a 400VALIDATION_ERRORwheng.getType() != GeschichteType.JOURNEY. I don't see a unit test exercising this path inJourneyItemServiceTest. Worth adding:2. Missing test:
reorderwith empty listWhen
dto.getItemIds()is null/empty ANDexistingIdsis also empty,reorder()hits the duplicate-check (passes), the set-equality check (passes), then therequestedIds.isEmpty()early return:return List.of(). This edge case has no explicit test. If the early-return is ever refactored it would silently break.3. PR description still lists "Still in progress (WIP)" with Tasks 6–9 as incomplete
This needs to be updated before merge — a reader relying on the description would think the PR is unfinished.
What's Done Well
JourneyItemConstraintsTestverifies actual PostgreSQL constraint behavior viapg_constraintcatalog query — the right way to test DEFERRABLE constraints ✅appendItem_returns409_on_position_conflictinGeschichteControllerTestcovers the deferred-constraint path end-to-end ✅reorder_returns400_when_itemIds_contain_duplicates— good regression test for the silent-overwrite bug fix ✅@WebMvcTest+@Import(JacksonConfig.class)— correct slice test setup, not a full@SpringBootTest✅🛠️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
Pure application code change — no Compose file, no CI workflow, no Dockerfile, no infrastructure configuration touched.
The V73 Flyway migration is clean and the comment calling out the PgBouncer transaction-mode dependency is exactly the kind of ops-relevant note I need before production deploy:
That note prevents a future platform change (pooler mode switch) from silently breaking the deferred constraint. Nothing to flag from the infra side.
🎨 Leonie Voss — UI/UX Design Lead
Verdict: ✅ Approved
Backend-only PR — no Svelte components, no routes, no markup to review. The only frontend changes are the regenerated
api.tstypes and three new i18n error keys.Checking the user-facing strings:
"error_journey_item_not_found"→"Der Reise-Eintrag wurde nicht gefunden."— clear, friendly ✅"error_journey_item_position_conflict"→"Positionskonflikt beim Sortieren der Reise-Einträge."— technically correct but slightly jargon-heavy for a 60+ user on a slow phone. Something like"Beim Sortieren ist ein Konflikt aufgetreten. Bitte lade die Seite neu."would be friendlier. Minor suggestion, not a blocker.LGTM from UI/UX.
📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Reviewing for requirements completeness and traceability.
Concerns
1. PR description's "Still in progress (WIP)" section is stale
The description explicitly marks Tasks 6–9 as incomplete. These are all implemented. A reviewer or automated gate reading this would incorrectly conclude the PR is unfinished. Remove the WIP section and replace with a complete "Implemented" list before merge.
2.
MAX_ITEMS = 100— business rule with no traceabilityThe 100-item cap on a reading journey is a meaningful business constraint but has no link back to any specification, acceptance criterion, or issue comment. If a stakeholder later asks "why 100?", there's no trace. Add a comment:
or document it in GLOSSARY.md under JourneyItem.
3.
POSITION_STEP = 10— future-insertion assumption undocumentedStep 10 implies the intent to later support "insert between" without immediate full reorder. This is a forward-looking design decision not mentioned in ADR-035 or the issue. Worth a single sentence in ADR-035 under "Consequences" so it's intentional rather than incidental.
What's Well-Specified
notefully documented in ADR-035 with examples ✅Review concerns addressed
All four blockers/concerns raised in the second-pass review have been fixed:
🔒 CWE-359 email leak in
AuthorView—2f471155Concern (Markus):
toView()fell back toauthor.getEmail()as the display name when first/last name were both blank, defeating the purpose ofAuthorViewwhich intentionally hides email.Fix: Replaced the email fallback with
"[Unbekannt]". Updated the unit test name and assertion to match.🔒 Raw
Set<Person>leaking admin-only fields —7ed00326Concern (Markus):
GeschichteView.personswas typed asSet<Person>, which exposedprovisional,sourceRef,generation, anddocumentCount— fields the public read path should never see.Fix: Added
PersonViewinner record (id + firstName + lastName only) toGeschichteView. UpdatedGeschichteService.toView()to project eachPersoninto aPersonView. Fixed the integration test'sextracting(...)call accordingly (GeschichteView.PersonView::id).🏗
JourneyItemServicemissing from C4 L3 diagram —5539158eConcern (Kai):
JourneyItemServiceis a production Spring Service but was absent from the supporting-domains component diagram.Fix: Added the component block with a description of all four operations (append, updateNote, delete, reorder). Added three relationship arrows:
GeschichteController → JourneyItemService,GeschichteService → JourneyItemService, andJourneyItemService → db.🌍 Jargon in position-conflict i18n message —
97f22e1cConcern (Kai): The
JOURNEY_ITEM_POSITION_CONFLICTmessages were too technical ("Position conflict while reordering journey items").Fix: Replaced with a user-actionable reload prompt in all three languages (de/en/es): e.g. "The order was just changed by someone else — please reload the page."
All 25
GeschichteServiceTestand 62GeschichteControllerTest + JourneyItemServiceTesttests are green. Branch pushed.🏗️ Markus Keller (@mkeller) — Application Architect
Verdict: ⚠️ Approved with concerns
Solid structural work. The layering is clean, the DEFERRABLE UNIQUE constraint is exactly the right database-layer enforcement for concurrent reorder, and the ADR-035 is a good decision record. A few points need attention before this fully passes muster.
Blockers
1.
JourneyItemServicedirectly injectsGeschichteRepository— boundary violationJourneyItemServiceowns thejourneyitemsub-package, but it injectsGeschichteRepositorydirectly to verify existence and type:Per the layering rule: services never reach into another domain's repository — always call the other domain's service instead.
JourneyItemServiceshould callGeschichteService.getById()(or a purpose-builtGeschichteService.getJourneyById()) and letGeschichteServiceown its repository. This is the same boundary the previous PR comment flagged as a "reserved" injection, and it's now been done the wrong way around.Fix: expose a
GeschichteService.getJourneyOrThrow(UUID id)that encapsulates theJOURNEYtype check and the not-found guard, then injectGeschichteServiceinJourneyItemService.2. Circular dependency risk:
GeschichteService↔JourneyItemServiceGeschichteServicenow injectsJourneyItemService, andJourneyItemServiceinjectsGeschichteRepository(which is owned byGeschichteService's domain). If the boundary fix above is applied naively (JourneyItemService → GeschichteService), this creates a constructor injection cycle which Spring Boot 4/Spring Framework 7 fully prohibits. The CLAUDE.md explicitly calls this out: "Spring Boot 4 / Spring Framework 7 fully prohibits constructor injection cycles."The correct resolution is to keep
JourneyItemServicedepending onGeschichteRepositorydirectly (acceptable for the sub-domain that lives insidegeschichte/journeyitem) or to factor out aGeschichteQueryServicethat both can inject without creating a cycle. Pick one and document the decision.3.
CLAUDE.md—JourneyItemServicenot added to the package tableThe architect's doc update rule is explicit: new controller or service in an existing backend domain → update
CLAUDE.mdpackage structure table.JourneyItemServiceis a new Spring Service and it is absent from the CLAUDE.md domain model table. The C4 diagram was updated; CLAUDE.md was not.Suggestions
DocumentSummarylives ingeschichte/package — consider moving it togeschichte/journeyitem/It's exclusively consumed by
JourneyItemView. Having it one level up in thegeschichtepackage leaks an internal detail upward. Minor, but worth noting for navigability.Reorder: N individual
save()calls instead of a batchWith 100 items this is 100 round-trips inside one transaction. A
saveAll()call (Spring Data JPA will still flush within the same TX) or a native@Modifyingquery would be cleaner. Not a correctness blocker, but worth noting for a 100-item journey.ADR-035 correctly documents the
jackson-databind-nullabledecision — well done. TheJacksonConfigplaceholder is fine.V73 migration comment about PgBouncer transaction-mode is excellent — exactly the kind of forward-looking note that prevents future incidents.
👨💻 Felix Brandt (@felixbrandt) — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Strong TDD discipline throughout — 642-line
JourneyItemServiceTestwith fine-grained behavior coverage is exactly what I want to see. The three-wayOptional<String>PATCH pattern is clever and well-documented. A few clean code and reliability concerns to address.Blockers
1.
toView()is public and misnamed — it's an internal mapping methodMaking conversion helpers
publicleaks the service's internal concern. Both are called only from withinJourneyItemServiceitself (and in tests directly). They should be package-private at most. Making thempublicwill tempt callers to bypass the service entirely. Rename toprivateor at least package-private, and if tests need them, test via the public methods.2.
updateNoteskips auditing on successful note changeappendanddeleteboth callauditService.logAfterCommit().updateNotedoes not. This is an inconsistency — if note updates are auditable events (they are domain changes), they need an audit log call. There is also noAuditKindfor note updates. Either add aJOURNEY_ITEM_NOTE_UPDATEDkind or document explicitly why note updates are intentionally not audited.3.
reorder()does not verify the Geschichte exists or is a JOURNEY typeIf
geschichteIdis unknown,existingIdswill be empty. The subsequent set-equality check against the incoming IDs will only catch mismatches. Sending{"itemIds": []}for a non-existentgeschichteIdwill return an empty list with HTTP 200 — silently succeeding on garbage input. Theappend()method does verify the Geschichte exists;reorder()must do the same.Suggestions
GeschichteService.toView()builds a display name inline — extract a helperThis is identical logic to what
JourneyItemService.join()does. The pattern exists in two places now. ExtractdisplayName(AppUser)as a static helper in a shared location or at least makeGeschichteServiceusejoin().JourneyItemCreateDTOhas no validation annotation — "both null" is caught lateThe validation
if (dto.getDocumentId() == null && note == null)fires in the service after the note is normalized. Adding a class-level@AssertTrueor a@NotNullon a custom validator at the DTO level would push this earlier. Not a blocker, but inconsistent with the rest of the codebase's boundary-validation approach.varin test file is fine but inconsistent with the rest of the test suiteOther test files in this codebase use explicit types. Minor, but worth being consistent.
JacksonConfig.java— empty@Configurationclass is a code smellAn empty configuration class that "currently a placeholder" is dead code. Either add the module registration now or delete it and add it when it's needed. A comment saying "future modules added here" in a class with zero content adds noise.
🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
Good authorization discipline —
@RequirePermission(BLOG_WRITE)on all write endpoints, IDOR-safe lookups viafindByIdAndGeschichteId, and theAuthorViewemail-hiding are all correct. Two findings that need attention before merge.Blockers
1.
reorder()silently succeeds for non-existentgeschichteId— authorization gapIf
geschichteIddoes not exist,existingIdsis an empty set. The equality check{} == {}passes when the request body is{"itemIds": []}. HTTP 200 is returned for a reorder request on a non-existent journey. This is a logic bypass: a caller can confirm whether ageschichteIdexists by observing whether they get 200 (empty journey) or 400 (non-empty journey with wrong IDs), enabling enumeration of valid journey IDs.Fix: verify the Geschichte exists at the start of
reorder(), the same wayappend()does.2. PATCH three-way semantics: the
Optional<String>pattern has no test for type confusion from untrusted JSONThe ADR-035 documents the wire encoding correctly. However, the
JourneyItemUpdateDTOrelies on Jackson 3.x natively mapping JSONnull→Optional.empty()and absent field → Javanull. This is a documented behavior but it is never tested at the HTTP boundary — theGeschichteControllerTesttests use raw JSON strings and verify the service mock is called, not that the deserialization itself produces the correctOptionalstate. A controller-layer deserialization test (sending{"note": null}and assertingdto.getNote() == Optional.empty(), sending{}and assertingdto.getNote() == null) would make this assumption explicit and prevent silent breakage if the Jackson 3.x behavior changes.This is a security smell rather than a confirmed vulnerability, but the three-way semantics are load-bearing for correctness (clear vs. no-op), so deserializing incorrectly would allow unintended note clearing.
Confirmations (good)
findByIdAndGeschichteIdIDOR protection is correct and tested — both controller and service tests cover the 404-when-wrong-journey scenario.AuthorViewexposes onlyid+displayName— email omission is correctly tested inGeschichteServiceTest.getById_author_email_is_not_in_author_view().GlobalExceptionHandlermapsuq_journey_items_geschichte_positionto a structured 409 — the constraint name is matched by literal string. CWE-209 note in the existing handler comment is correctly applied here too: the constraint violation message is not forwarded to the client.Suggestion
Log
JOURNEY_ITEM_POSITION_CONFLICTat WARN rather than just returning 409The
GlobalExceptionHandleralready logs the constraint name. Since this conflict can only occur on concurrent modification (two simultaneous reorder/append requests), it could be worth a low-volume WARN log that includesgeschichteIdfor production incident correlation. Not a security issue per se, but helpful for anomaly detection.🧪 Sara Holt (@saraholt) — QA Engineer & Test Strategist
Verdict: ⚠️ Approved with concerns
Best test suite in this feature area to date. The constraint test design (deliberately NOT
@Transactionalat class level with a clear explanation of why) is excellent. 642-line service test with named behaviors is exactly the pyramid layer I want to see. One blocker on test coverage, a few structural notes.Blockers
1.
reorder()has no test for the non-existentgeschichteIdcaseThe
JourneyItemServiceTestcoversappend_returns404_when_documentId_does_not_existbut there is no test for:reorder_returns404_when_geschichteId_does_not_existreorder_returns404_when_geschichte_is_not_JOURNEY_typeThese are the same guard clauses that
append()has tests for. Their absence fromreorder()test coverage is a gap — and as raised by the security review, this is also a behavioral bug (silent 200 on empty reorder for unknown ID).Add:
2.
updateNotehas no audit test — missing coverage for the no-audit decisionappend_audits_JOURNEY_ITEM_ADDEDverifies theauditService.logAfterCommitcall. There is no corresponding test forupdateNote— not even a test that explicitly verifiesauditServiceis not called (which would at least document the intentional no-audit decision). If note changes should be audited (see Felix's finding), a test is needed. If they should not, add:Confirmations (good)
JourneyItemConstraintsTestuses raw JDBC and avoids@Transactionalat class level — the comment explaining why (DataIntegrityViolationExceptioninside a class-level@Transactionalmarks the TX rollback-only) is exactly the kind of documentation that prevents future "why is this not @Transactional?" questions.@ExtendWith(MockitoExtension.class)throughout service tests — no@SpringBootTestwhere@WebMvcTestor Mockito suffice.savedItem,savedItemWithDoc,makeDoc,journey) keep test setup readable and overridable. One-liner test bodies are the goal.JourneyItemConstraintsTestuses Testcontainers (viaPostgresContainerConfig) — real PostgreSQL, not H2. The DEFERRABLE constraint behavior would not be testable on H2.append_to_empty_journey_starts_at_10,updateNote_absent_leaves_note_unchanged. These pass the "CI failure report" test — you know what broke without opening the file.Suggestions
GeschichteControllerTest.updateItemNote_null_note_deserializes_as_present_null— test name is unclearThe test name says "deserializes as present null" but
Optional.empty()is neither absent nor a string. Rename toupdateItemNote_json_null_note_is_deserialized_as_empty_Optionalto be precise about what the test verifies.JourneyItemIntegrationTestunchanged delete test — rename to match new method nameThe integration test updated the repository call from
findAllByGeschichteIdtofindByGeschichteIdOrderByPositionbut the test name likely still reflects the old assertion pattern. Verify the test name still accurately describes the intent.GeschichteServiceIntegrationTest—getByIdnow returnsGeschichteViewbut the integration test only checkstitleandpersonsThe updated assertions check
fetched.title()andfetched.persons()but notfetched.items(). For a@SpringBootTestintegration test, it would be worth asserting that items are loaded (even if the list is empty) to confirm theJourneyItemService.getItems()delegation path works end-to-end.🚀 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR. All changes are application code, migration SQL, and documentation. I have nothing to block on from a platform perspective, but a few notes worth logging.
Confirmations (good)
jackson-databind-nullabledependency was removed per ADR-035 — net complexity reduction.JourneyItemConstraintsTestuses Testcontainers (postgres:16-alpine) — same image version as production. CI will catch constraint failures before they reach production.Observations
Migration V73 is two
ALTER TABLEstatements — verify Flyway wraps them in one transactionThe comment says "MUST run in a single transaction; Flyway's default per-migration transaction satisfies this." That's correct for Flyway's default behavior, but worth confirming that no
flyway.propertiesoverride or callback has setexecuteInTransaction=falsefor this migration file. The comment itself warns against this, which is the right place to put it.No changes to
docker-compose.ymlor CI workflow — expected. No action required from me on this PR.Suggestion
The constraint name
uq_journey_items_geschichte_positionis hardcoded as a string literal inGlobalExceptionHandler.java. Consider extracting it to a constant (perhaps in the migration or a dedicated schema constants file) so that if the constraint is renamed in a future migration, there is only one place to update. Low priority — but a stale string in the exception handler that no longer matches the constraint name would silently fall through to the generic 400.📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
The implementation is thorough and well-specified. The PR description maps directly to the issue requirements. A few gaps in requirement coverage and one ambiguity that should be resolved before the frontend is built.
Blockers
1. The 100-item cap uses
VALIDATION_ERROR— this is a business rule, not a validation errorVALIDATION_ERRORimplies a malformed request (e.g. missing required field). Hitting the 100-item cap is a business rule violation — the request is well-formed, the business constraint prevents execution. The frontend cannot distinguish "your JSON was malformed" from "you hit the cap" without inspecting the error message string, which is fragile.This should have its own
ErrorCode(e.g.JOURNEY_AT_CAPACITY) so the frontend can surface a specific, actionable message: "This reading journey has reached its maximum of 100 items."2.
appendaccepts aSTORY-type Geschichte with noErrorCodedistinction from not-foundAgain,
VALIDATION_ERRORis used for what is a resource-type mismatch. The frontend has no way to tell the user "this is a Story, not a Reading Journey — items cannot be added to it." A distinct error code likeGESCHICHTE_TYPE_MISMATCHwould allow a specific UI message. Without it, the frontend will show a generic "validation error" for what should be a clear domain-level message.Suggestions
Acceptance criterion gap: what happens when
noteis HTML?The
notefield usesnormalizeNote()which only trims whitespace. The Geschichte body is HTML-sanitized via OWASP HTML Sanitizer. Thenotefield does not appear to be sanitized — it is stored as plain text and returned as-is. If the frontend ever renders notes as HTML (e.g. with{@html note}in Svelte), this is an XSS vector. Document the current behavior: notes are plaintext, stored as-is, rendered as text (not HTML). If the spec ever changes to allow rich-text notes, this needs revisiting.The
receiverNameinDocumentSummary— which receiver is "canonical"?The API returns a single
receiverName(alphabetically first by last name) plus areceiverCount. A user reading a journey item sees "Letter to Anna Amann (+1 more)". This is a UX decision that should be confirmed: is the alphabetical-first receiver the right canonical choice? The issue spec does not address this. Not a blocker, but worth a comment in the code or issue noting this was a deliberate choice.Missing acceptance criterion: GET
/api/geschichten/{id}for a STORY returnsitems: []GeschichteViewalways includesitems(even forSTORYtype). The PR comment says "empty list for stories with no items." This is the right call, but there is no test asserting this boundary: aGeschichteServiceTestfor aSTORYtype verifying thatresult.items()is empty (not null) would document this contract explicitly for frontend consumers.i18n strings are present for the two new error codes — the German, English, and Spanish messages for
JOURNEY_ITEM_NOT_FOUNDandJOURNEY_ITEM_POSITION_CONFLICTare correct and appropriate. However, there are no i18n keys for theVALIDATION_ERRORcases that carry business meaning (cap hit, type mismatch) — because they are currently piggybacking onVALIDATION_ERROR. This reinforces the blocker above.🎨 Leonie Voss (@leonievoss) — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
This is a pure backend API PR — no frontend routes, no Svelte components, no UI markup. From a UX perspective, I'm reviewing the API contract as the foundation for the reader UI that will come next. The data shapes look right for what the frontend will need, but I'm flagging a few design-time concerns now before they become expensive to change.
No Blockers
No UI code was shipped in this PR. I have no access violations, accessibility gaps, or brand compliance issues to flag.
Forward-looking concerns (for the reader UI PR)
DocumentSummary.receiverName— the single-name display pattern needs UX specThe API returns
receiverName(one name) +receiverCount(integer). The frontend will presumably render something like "To Franz Raddatz (+2 more)". This is the right minimal data shape, but the display pattern — truncation, overflow, tooltip — needs a defined spec before implementation. The senior audience (60+) on mobile will need the overflow to be very clear and readable. Flag this for the reader UI design brief.notefield — the absence of a max-length enforced in the DB means the frontend must handle long notesNotes are capped at 5000 characters in application code. A 5000-character note rendered in a journey timeline card will overwhelm the layout. The reader UI will need a truncation/expand pattern. This is a frontend concern but it flows from the API contract — the front-end team should know the maximum before designing the card.
GeschichteView.itemsisList(ordered) and position is exposed — good. The reader UI can render items in sequence without resorting. Thepositionvalue itself (10, 20, 30...) should not be shown to end users — it's an internal ordering key. Make sure the reader UI template strips it.AuthorView.displayNamefallback[Unbekannt]is hardcoded in GermanThis hardcoded German string will appear in English and Spanish UI contexts. It should either be:
nullfor unknown display name, and the frontend rendersm.unknown_author()), orFor a family archive targeting German speakers primarily this is low urgency, but it will be visually wrong on the English and Spanish interfaces. The fix is cheap now (return
nulland let the frontend substitute) but expensive after the reader UI is built around a specific string.What looks good
DocumentSummarycontains exactly the fields a journey card needs: title, date range, sender, receiver summary. No over-fetching, no under-fetching.GeschichteViewreturnstype— the frontend can conditionally render journey-specific UI only whentype === 'JOURNEY'.JOURNEY_ITEM_NOT_FOUNDandJOURNEY_ITEM_POSITION_CONFLICThave human-readable i18n messages in all three languages. The conflict message ("The order was just changed by someone else — please reload the page.") is clear and actionable — users know what to do.Review fix summary — all 8 concerns addressed
All changes pushed to
feat/issue-751-journey-item-crud-api.JourneyItemService+GeschichteQueryServiceto package tables in bothCLAUDE.mdandbackend/CLAUDE.mde157d90b+598ad622GeschichteQueryService(ownsGeschichteRepository);JourneyItemServiceno longer holds a cross-domain repository reference; unit tests inGeschichteQueryServiceTestcover both true/false paths69db1983reorder()now throws 404 (GESCHICHTE_NOT_FOUND) when the Geschichte does not exist; testreorder_unknownGeschichteId_throws404added2ae1bb3atoView()andtoSummary()changed frompublicto package-private; tests remain in the same package so access is unaffected7c066098JOURNEY_ITEM_NOTE_UPDATEDadded toAuditKind;updateNote()callsauditService.logAfterCommitafter saving; testupdateNote_auditsNoteUpdateadded147aa563JOURNEY_AT_CAPACITYadded toErrorCode.java,errors.ts, and all three i18n files (de/en/es);append()now throws this code (409 conflict) when at the 100-item cap1fb0c412+598ad622GESCHICHTE_TYPE_MISMATCHadded toErrorCode.java,errors.ts, and all three i18n files (de/en/es);append()now throws this code (409 conflict) for non-JOURNEY type guarde400b1d7+598ad622updateItemNote_null_note_deserializes_as_present_null→updateItemNote_json_null_note_is_deserialized_as_empty_Optionalc5611250Build:
./mvnw clean package -DskipTests— BUILD SUCCESS. Frontendnpm run check— no errors in changed files (errors.ts, message files); all reported errors are pre-existing in unrelated domains.🏗️ Markus Keller — Application Architect
Verdict: ✅ Approved
This is solid, well-structured work. The sub-domain package placement (
geschichte/journeyitem/), the deferrable UNIQUE constraint in V73, and the ADR documenting theOptional<String>three-way PATCH decision are all the right calls. Let me go through my checklist.Documentation compliance table audit
journeyitem/sub-domain packageCLAUDE.mdpackage table +l3-backend-*.pumlCLAUDE.md,l3-backend-3g-supporting.puml)JourneyItemService,JourneyItemControllerin existing domainl3-backend-3g-supporting.pumljourney_items(no new table/FK)db-orm.puml+db-relationships.puml— migration adds constraints on existing table, no structural changeErrorCodevalues:JOURNEY_ITEM_NOT_FOUND,JOURNEY_ITEM_POSITION_CONFLICT,JOURNEY_AT_CAPACITY,GESCHICTE_TYPE_MISMATCHCLAUDE.md+docs/ARCHITECTURE.mdJourneyItemView,GeschichteView,AuthorViewdocs/GLOSSARY.mdOptional<String>three-way PATCH semanticsFinding 1 (Suggestion): The four new
ErrorCodevalues are not reflected inCLAUDE.md(the CLAUDE.mdErrorCodesection only mentionsTOO_MANY_LOGIN_ATTEMPTSby name but the instruction says to add new codes there) anddocs/ARCHITECTURE.md. This is a doc concern, not a blocker in practice since the CLAUDE.md instruction is to update the error handling section when new codes are added.Boundary integrity
JourneyItemServicecorrectly callsdocumentService.getSummaryById()rather thanDocumentRepositorydirectly — good boundary discipline. TheGeschichteRepositoryinjection intoJourneyItemServiceis a minor question:JourneyItemServicereaches intoGeschichteRepositoryinstead ofGeschichteService. This is a known exception for cycle-breaking (the MEMORY file mentions Spring Boot 4 / Spring Framework 7 prohibiting constructor-injection cycles), and it's consistent with how the codebase handles it. Acceptable.V73 migration correctness
The comment in V73 correctly documents the PgBouncer transaction-mode assumption for DEFERRABLE INITIALLY DEFERRED. This is exactly what I want to see: future engineers won't inadvertently switch to statement-level pooling without understanding the consequences.
Read model design
AuthorViewexposing onlyidanddisplayName(neveremailor group memberships) is the correct security-first read model design.GeschichteViewusingSet<PersonView>for persons andList<JourneyItemView>for items (ordered vs. unordered) shows the right data structure choices.JacksonConfigplaceholderThe class is currently empty after removing
jackson-databind-nullable. That's fine for a placeholder, but do delete it if it stays empty for more than one or two more PRs — empty config classes become dead weight.Summary
The structural decisions are sound. The one missing doc update (ErrorCode values in
CLAUDE.md/ARCHITECTURE.md) is worth noting but is a suggestion, not a blocker at the architecture layer given the codes are fully documented inErrorCode.javaitself with Javadoc.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ✅ Approved
Clean implementation. The TDD evidence is strong —
JourneyItemServiceTesthas 30+ fine-grained tests covering every boundary condition, the constraint test verifies against real Postgres via Testcontainers, and the integration test covers the full persistence stack. Let me go through the details.Blockers
None.
Suggestions
1.
JourneyItemServiceinjectsGeschichteRepositorydirectlyJourneyItemServicehasprivate final GeschichteRepository geschichteRepositoryand alsoprivate final GeschichteQueryService geschichteQueryService. This means the sub-domain is bypassing the parent domain's service for theappendpath (usesgeschichteRepository.findById) but using the service for thereorderpath (usesgeschichteQueryService.existsById). The inconsistency is slightly confusing — one or the other is the right boundary, not both. Given the Spring 7 cycle constraint, theGeschichteQueryServiceinjection is the clean solution; I'd suggest migratingappendandupdateNoteto also usegeschichteQueryService.existsById(and fetch theGeschichtetype via a newgetTypeByIdor similar method inGeschichteQueryService) rather than injecting the repository. This is a suggestion, not a blocker, since the cycle-breaking is documented in MEMORY.2.
toSummaryandtoVieware package-private, not privateThese are tested directly in
JourneyItemServiceTest, which is in the same package — hence the package visibility. The tests are valuable. But the intent-revelation would be better served by making themprivateand testing the behaviour indirectly through the public API methods (append,updateNote, etc.), as Felix's test philosophy dictates: test public contracts, not internal transformations. The direct tests fortoSummaryare thorough and readable, so this is a cosmetic point.3.
reordercallsjourneyItemRepository.save(item)in a loopFor up to 100 items this is N individual
savecalls.saveAll()with a pre-built list would be cleaner and let Spring Data batch them if batch-size is configured. Minor performance note, not a blocker.4.
import java.util.*inJourneyItemServiceWildcard import. The codebase generally uses explicit imports. Worth tidying.
5. Test setup pattern is consistent and readable
journey(),savedItem(),savedItemWithDoc(), andmakeDoc()factory helpers are clean and reusable.@BeforeEachsets up the security context once. This is exactly the right pattern. Descriptive test names likeappend_returns400_when_neither_documentId_nor_noteread as sentences. Good work.6. Frontend error codes — all 4 new codes are present in
errors.tsand mappedJOURNEY_ITEM_NOT_FOUND,JOURNEY_ITEM_POSITION_CONFLICT,JOURNEY_AT_CAPACITY,GESCHICTE_TYPE_MISMATCHare all present in the union type and have entries ingetErrorMessage(). The i18n keys exist in all three locales. Full chain complete.Summary
Strong TDD evidence, clean service layer, correct use of
DomainExceptionthroughout. The minor points above are quality-of-life suggestions and won't block merge.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ✅ Approved
Good security posture throughout. Let me walk through my checklist.
Blockers
None.
Findings
FINDING-01 (Confirmed secure — IDOR protection): The
findByIdAndGeschichteId(UUID id, UUID geschichteId)repository method inJourneyItemRepositoryis the correct pattern for preventing IDOR. An item ID that exists but belongs to a different journey returnsOptional.empty()rather than the item, and the service correctly throwsJOURNEY_ITEM_NOT_FOUNDin that case. This is explicitly tested inJourneyItemServiceTest.patch_returns404_when_item_belongs_to_different_journey(). CWE-639 (Authorization Bypass Through User-Controlled Key) is mitigated.FINDING-02 (Confirmed secure — permission annotations): All five write endpoints in
GeschichteControllerhave@RequirePermission(Permission.BLOG_WRITE):POST /{id}/items— ✅PATCH /{id}/items/{itemId}— ✅DELETE /{id}/items/{itemId}— ✅PUT /{id}/items/reorder— ✅The read endpoints (
GET /andGET /{id}) correctly have no@RequirePermission(read-only, authenticated users can access). No write endpoint is unprotected.FINDING-03 (Confirmed secure — author email not leaked):
GeschichteView.AuthorViewexposes onlyidanddisplayName. Email, group memberships, and permissions are not in the response. The Javadoc comment explains the threat model (// Summarised author — exposes only id and displayName, never email or group memberships). This is the correct implementation of the principle of least exposure.FINDING-04 (Security smell —
currentUser()callsuserService.findByEmail()):This is a second database lookup per write operation (after the main transaction). It's the same pattern used elsewhere in the codebase (consistent), and the
@RequirePermissionAOP check runs before service methods, so by the timecurrentUser()is called the user is already authenticated and their session is valid. Not a vulnerability, but it is an N+1-style extra DB hit on every audit log write. The security posture is correct; the operational concern is minor.FINDING-05 (Suggestion — note length check at constraint layer): The 5000-character limit on
noteis enforced in the service layer (MAX_NOTE_LENGTH = 5000) but not at the database layer via aCHECKconstraint. An ADR entry would be useful documenting why this was intentionally left at the application layer (flexibility to change without migration). Without it, it looks like an oversight. The service check is sufficient as a security control; this is a defence-in-depth suggestion.FINDING-06 (Confirmed —
VALIDATION_ERRORfor the reorder IDOR check): When item IDs in a reorder request don't match the journey's existing items, the service throwsVALIDATION_ERROR(400) rather thanJOURNEY_ITEM_NOT_FOUND(404). This is intentionally ambiguous — it doesn't reveal whether the foreign ID exists in the system. Correct security behaviour.Summary
The IDOR mitigations are solid, permission annotations are complete, and the read model correctly avoids leaking AppUser internals. No security vulnerabilities found.
🧪 Sara Holt — QA Engineer & Test Strategist
Verdict: ✅ Approved
Test coverage is genuinely impressive for this PR. Let me give the full breakdown.
What's well-tested
Unit layer (
JourneyItemServiceTest):@ExtendWith(MockitoExtension.class)tests — no Spring context overheadappend: empty journey (position 10), after reorder (continues from max), capacity=100 rejection, note-length rejection, type mismatch (STORY vs JOURNEY), document-not-found propagation, whitespace-only note treated as null, audit event emittedupdateNote: absent field is no-op (no save called),Optional.empty()clears note when document present, string sets note, clear-without-document rejected, 5000-char limit, audit eventdelete: item not found, audit not emitted on failure, audit emitted on successreorder: unknown ID, duplicate IDs, foreign IDs, extra IDs, empty on empty (ok), empty on non-empty (rejected), correct position assignment, grandfathered-over-cap (130 items) succeeds, audit emittedtoSummarysub-tests: sender from Person, sender from text, null sender, receiver count, multi-receiver canonical name, datePrecision roundtripConstraint layer (
JourneyItemConstraintsTest): Uses real Postgres 16 via Testcontainers. Tests the deferrable flag viapg_constraintquery, theCHECK (position > 0), and duplicate position rejection (rollback). These are exactly the tests that H2 would have silently missed.Controller layer (
GeschichteControllerTest):@WebMvcTestslice — permission checks on all 5 new endpoints, 401/403 coverage confirmed.Integration layer (
JourneyItemIntegrationTest): Full persistence round-trip with real Postgres.Coverage gaps (suggestions, no blockers)
1. Missing 401 test for write endpoints in
GeschichteControllerTest:The controller tests verify 403 (authenticated but insufficient permission). There should also be a test for unauthenticated requests (401) on the new
POST /{id}/items,PATCH /{id}/items/{itemId},DELETE /{id}/items/{itemId},PUT /{id}/items/reorderendpoints — even if just a spot-check on one of them. The existing test class forGeschichteControllerlikely has this pattern established; just extend it.2.
GeschichteService.getById→GeschichteViewis listed as "WIP (Task 7)" in the PR description.The
GET /{id}endpoint returnsGeschichteViewbut the implementation inGeschichteServicemay not yet have a controller-level test covering the newGeschichteViewshape (items list, AuthorView, PersonView). This is marked WIP, so acceptable, but the test gap should be closed before the WIP tasks are merged.3.
reorderN-saves — no performance test:100 individual saves in a loop will generate 100 SQL statements in one transaction. Not a correctness issue, but worth noting in the integration test's assertion or comments that the cap is 100 items and the reorder re-assigns all positions. A
JourneyItemIntegrationTesttest with exactly 100 items (max capacity reorder) would confirm the deferrable constraint works for the full reorder case — the current constraint test only tests 2 items.Quality gates assessment
@Disabledtests foundThread.sleepfoundappend_to_empty_journey_starts_at_10) — goodjourney(),savedItem()) are used consistentlyOverall: test pyramid is healthy for the implemented work. WIP tasks are appropriately flagged.
🔧 Tobias Wendt (@tobiwendt) — DevOps & Platform Engineer
Verdict: ✅ Approved
This PR is purely application code — no Compose changes, no CI workflow changes, no new Docker services. From a platform perspective there is very little surface area to review. What's here is clean.
Infrastructure impact assessment
Flyway V73 — deferrable unique constraint:
The migration comment documents the PgBouncer dependency correctly:
This is exactly the kind of comment I want to see — it names the operational dependency, the failure mode, and the required configuration. No further action needed from infrastructure.
No new Docker services, no Compose changes: Nothing to review on that front.
No new environment variables: The service uses existing
POSTGRES_*configuration.CI implications
The new
JourneyItemConstraintsTestuses Testcontainers (Postgres 16). This is the right call — H2 does not supportDEFERRABLEconstraints. The test will add a few seconds to the integration test job but that is the correct trade-off.No CI workflow changes needed for this PR.
One observation (not a blocker)
JacksonConfig.javais an empty@Configurationclass:This registers as a Spring bean and adds a tiny amount of startup overhead for zero benefit. The comment in ADR-035 explains it is a placeholder left after
jackson-databind-nullablewas removed. I would rather see it deleted than left as dead code — but this is a style preference and does not affect reliability or operations. If it is intended as a future extension point, a single-line comment saying so would close the question.Summary
No infrastructure blockers. The PgBouncer transaction-mode dependency is documented at the migration level where it belongs. The Testcontainers choice for constraint tests is correct. Nothing here changes operational complexity or monthly cost.
📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ✅ Approved
Reviewing from the requirements side — does the implementation match the issue spec, are acceptance criteria met, are there NFR gaps or edge cases that slipped through?
Traceability check: Issue #751 → PR #788
The issue spec for JourneyItem CRUD (from the PR description) called for four operations: append, updateNote, delete, reorder. All four are implemented and tested. The PR description explicitly marks remaining work (Task 7:
GeschichteService.getById → GeschichteView) as WIP with a follow-up issue. This is clean scoping discipline.Acceptance criteria coverage
AC: Append
append_to_empty_journey_starts_at_10,append_continues_from_max_position).GESCHICTE_TYPE_MISMATCH. ✅ Tested.JOURNEY_AT_CAPACITY. ✅ Tested.AC: UpdateNote
notefield, when I PATCH, then the note is unchanged (no-op). ✅ Tested.note: null, when I PATCH, then the note is cleared. ✅ Tested (with and without document).note: "text", when I PATCH, then the note is set. ✅ Tested.VALIDATION_ERROR. ✅ Tested.AC: Delete
JOURNEY_ITEM_NOT_FOUND. ✅ Tested.AC: Reorder
All core acceptance criteria are covered.
Edge cases / requirements gaps (suggestions, no blockers)
OQ-01: What happens when an item's linked document is later deleted?
The schema uses
ON DELETE SET NULLfor thedocumentFK. This means aJourneyItemViewcan have anulldocument. TheJourneyItemViewrecord and OpenAPI spec markdocumentas nullable. However, the issue spec did not explicitly address what the UI should show for a document-less item that previously had one. This is a UI/UX concern, not a blocker for the API, but it warrants a follow-up issue to spec the "orphaned item" display state.OQ-02:
GESCHICTE_TYPE_MISMATCHtypo in ErrorCodeErrorCode.GESCHICTE_TYPE_MISMATCHis missing a letter — should beGESCHICHTE_TYPE_MISMATCH. This is a spelling defect that will surface in i18n keys and client-side error handling. It's already in the i18n files and TypeScript type union with the typo, so fixing it will require a coordinated change in four files (ErrorCode.java,errors.ts,de.json,en.json,es.json). Not a functional blocker but a code quality issue worth tracking as a separate cleanup issue.OQ-03: Position step (10) is not user-configurable and not documented as a stable API contract
The
POSITION_STEP = 10constant means the gap between consecutive items at append time is always 10. This is fine for the server-assigned case, but the reorder endpoint accepts raw position values — or does it? Looking at the service,reorder()reassigns all positions as(index + 1) * POSITION_STEP. So clients never supply raw positions; positions are always server-assigned. This should be made explicit in the API documentation (OpenAPI description on the endpoint) to prevent clients from assuming they can supply arbitrary position values in future endpoints.NFR coverage
@RequirePermission(Permission.BLOG_WRITE)on all 4 write endpointsNo NFR gaps found for the implemented scope.
Summary
Requirements coverage is thorough. The two open questions (orphaned item display state,
GESCHICTE_TYPE_MISMATCHtypo) are tracked here for follow-up. The typo in particular should be resolved in a cleanup issue before the Lesereisen feature ships to production, as it will be visible in error messages surfaced to users.🎨 Leonie Voss (@leonievoss) — UI/UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
This PR is a backend service layer with minimal frontend surface. I'll review what's here and flag what needs attention before the companion UI PR lands.
What this PR adds to the frontend
New error codes in
errors.tsand i18n files: Four new codes with user-facing strings in de/en/es. Let me check the strings.From the i18n files (de/en/es):
journey_item_not_found— user-facing message existsjourney_item_position_conflict— user-facing message existsjourney_at_capacity— user-facing message existsgeschicte_type_mismatch— user-facing message existsThe strings exist. One concern: the key
geschicte_type_mismatch(missing theh) will need coordinated renaming when the typo is fixed (flagged separately by Elicit). Not a blocker, but the UI team needs to be aware this key will change.Frontend surface:
GeschichteCardand journey item displayThis PR changes the
GeschichteViewshape — it now includes anitemslist withJourneyItemViewrecords. This feeds into the card component (GeschichtenCard.svelte) and the detail page.A few things to track for the UI companion PR:
1. Orphaned item state (document deleted)
JourneyItemView.documentis nullable. When a document is deleted, a journey item can exist with no linked document. The UI must handle this state explicitly — not just hide it, but communicate it. Suggested pattern:This string key does not exist yet — it needs to be added in the UI PR. I'm flagging it here because the data model supports this state right now.
2. Touch targets on journey item reorder controls
When the drag-to-reorder or up/down arrow UI is built, each control must be at least 44×44px. On mobile (320px viewport), a row of [up] [down] [delete] buttons next to a document card title will need careful layout — horizontal overflow is the common failure mode. The reorder UI should be tested at 320px before the companion UI PR merges.
3. Note field rendering
Notes are up to 5000 characters. The UI must handle long notes without breaking card layout —
max-h-[...]with a "show more" toggle, or truncation at ~3 lines withoverflow-hidden line-clamp-3. This is a UI PR concern but the backend has defined the constraint so I'm recording it here.4.
focus-visibleon journey item action linksThe previous PR (#787) added
focus-visibleto journey links per a review finding. Verify that all new interactive elements in the journey item list component follow the same pattern:Brand compliance of error strings
Quick check of the i18n message strings in the three locales:
No brand concerns with the text content.
Summary
No blockers in this PR from a UI/UX perspective. The key items to carry forward to the companion UI PR:
focus-visiblering pattern must be applied to all new interactive elements.I will flag these in the UI PR when it opens.
Post-review fixes applied
All review concerns addressed in 5 commits:
GESCHICTE_TYPE_MISMATCH→GESCHICHTE_TYPE_MISMATCHJacksonConfig.javaplaceholder7ba6342a+3f36d2a7@Importreference fromGeschichteControllerTestJourneyItemServiceno longer injectsGeschichteRepository4a0fed61GeschichteQueryServicenow exposesfindById(),append()uses itsaveAll()inreorder()instead of per-itemsave()loop3d80bc65journey_item_document_deleted(de/en/es)5b2ee312Verification:
./mvnw clean package -DskipTests→ BUILD SUCCESSnpm run check→ no new errors in changed files (pre-existing baseline ~807)👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Good discipline overall: guard clauses are clean,
DomainExceptionfactories are used everywhere,@Transactionalis on write methods only, and theOptional<String>three-way PATCH pattern is documented with an ADR. The test suite is substantial and well-structured. The blockers below are real but surgical — none requires structural rework.Blockers
1.
toView()executes N+1 SELECTs when loading a journey for GETJourneyItemService.toView(JourneyItem)callsdocumentService.getSummaryById(item.getDocumentId())for every item individually.GeschichteService.getById()callsjourneyItemService.getItems(id), which iterates and maps viatoView(). For a 20-item journey this fires 20 extra queries, all single-row SELECTs, in a read-hot path. TheDocumentService.getSummaryById()Javadoc itself warns it's meant for embedding — but there's no bulk loading path.Suggestion: add
JourneyItemRepository.findByGeschichteIdWithDocument(UUID)with aJOIN FETCH j.documentJPQL query, or pass a pre-fetchedMap<UUID, Document>intotoView. The test forgetItemsinJourneyItemServiceTestdoesn't catch this becausedocumentService.getSummaryByIdis mocked.2.
currentUser()hits the database on every write methodcurrentUser()callsuserService.findByEmail(auth.getName())— a DB round trip — in everyappend,updateNote,delete, andreordercall, just to get the actor UUID for auditing. The principal name fromSecurityContextHolderis the email; the user ID is needed forauditService.logAfterCommit. IfAppUseris stored as the principal directly (as other services do), this lookup is unnecessary. If it cannot be avoided, at minimum document why we can't get the ID from the principal directly.3.
GeschichteService.toView()— name composition duplicated fromJourneyItemServiceGeschichteService.toView()contains its own(firstName + " " + lastName).trim()logic.JourneyItemServicehasjoin(first, last)doing the same thing. This is two implementations of one operation. Extract to a shared static utility (PersonNameFormatter) or move intoAppUser/Personas adisplayName()method.4.
JourneyItemCreateDTOhas no validation annotation — empty body accepted silentlyThe validation
documentId == null && note == null → 400is done inside the service. That's correct per style, but it means a completely empty JSON body{}deserialized to a DTO with both fields null gets all the way to the service before failing. Fine for now — just make sure the service testappend_returns400_when_neither_documentId_nor_noteis marked as the authoritative regression for this path. It is, so no action required — noting for awareness.5.
reorder—JourneyItem.setPosition()mutation inside a loop beforesaveAllThe reorder loop mutates
item.setPosition(...)on entities fetched from a@Transactionalcontext. BecausesaveAllis called explicitly, this is safe — but if someone removes the explicitsaveAllcall in a refactor, the dirty writes could silently flush or not, depending on flush mode. Consider making the position-assignment immutable by creating new builder-cloned entities, or add a comment explaining why in-place mutation + explicitsaveAllis intentional.Suggestions
JourneyItemUpdateDTO.notefield's Javadoc says "null = field absent", but Lombok's@Datagenerates asetNote(Optional<String>)— a somewhat unusual API. The ADR-035 documents this well; consider a@SuppressWarnings("OptionalUsedAsFieldOrParameterType")to suppress IDE warnings.GeschichteQueryServiceis a thin wrapper with onlyexistsByIdandfindById. Fine to keep, but note that onceJourneyItemServicealso needsGeschichte.getType()(which it does inappend), thefindByIdapproach is correct —existsByIdinreorderdoes not fetch the type, so type validation is skipped there. This is deliberate (reorder doesn't care about type), but worth a comment.wildcard importatimport java.util.*;inJourneyItemService.java— project style elsewhere uses explicit imports. Minor but inconsistent.🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
The structural decisions are mostly sound:
JourneyItemServicein ajourneyitemsub-package undergeschichte, read-models as Java records,GeschichteQueryServiceas the bridge to avoid a direct repository dependency from the child sub-domain. The DEFERRABLE UNIQUE constraint is exactly the right tool for the reorder atomicity problem. ADR-035 is clear and justified.Two blockers — one is architectural, one is a documentation gap that must be closed before merge.
Blockers
1. Circular dependency between
GeschichteServiceandJourneyItemServiceGeschichteServiceinjectsJourneyItemService(to callgetItems(id)ingetById()).JourneyItemServiceinjectsGeschichteQueryService(a facade overGeschichteRepository).At the module level this is a cycle:
geschichte ↔ journeyitem. Spring Framework 7 (Spring Boot 4) fully prohibits constructor injection cycles. Right now it appears to work only becauseGeschichteQueryServiceis a separate bean fromGeschichteService— but the logical cycle is still present. If either service ever needs a direct reference to the other (not the query facade), it will fail at startup.The clean solution is to invert the dependency:
GeschichteService.getById()should not call intoJourneyItemServiceat all. Instead,GeschichteController.getById()should call both services and assemble the view:This keeps
GeschichteServiceandJourneyItemServiceas independent sibling services with no mutual dependency.2.
GeschichteViewandJourneyItemVieware not reflected in the C4 L3 diagramThe PR updates
l3-backend-3g-supporting.pumlwithJourneyItemService— good. However, the new read-model records (GeschichteView,JourneyItemView,DocumentSummary) andGeschichteQueryServiceare not added to any diagram. The architecture documentation rule requires that a new controller/service in an existing backend domain triggers a diagram update.GeschichteQueryServiceis a new service in thegeschichtedomain — it must appear in the diagram.The GLOSSARY update is present and well-written — that part is done correctly.
Suggestions
The
DocumentSummaryrecord lives inorg.raddatz.familienarchiv.geschichte(notjourneyitem). That's fine — it's a view type owned by the Geschichte domain. ButJourneyItemServicebuilds it by accessingDocument.getReceivers()(a collection defined in thedocumentdomain). This is acceptable becauseDocumentService.getSummaryById()returns the full entity — but it meansJourneyItemServicehas implicit knowledge of theDocumententity's internal collection structure. IfDocument.getReceivers()changes,JourneyItemService.toSummary()will break silently. Consider havingDocumentServiceexpose aDocumentSummary toSummary(UUID id)method instead, keeping the mapping inside thedocumentdomain.The V73 migration comment correctly warns about PgBouncer transaction mode. That warning belongs in a referenced ADR or
docs/adr/entry, not only in the SQL comment, so future infrastructure changes have a documented cross-reference to check.GeschichteControllernow has 7 endpoints: GET list, GET detail, POST create, PATCH update, DELETE delete, plus 4 journey-item endpoints. Consider splitting intoGeschichteController(5 endpoints) andJourneyItemController(4 endpoints under the same/api/geschichten/{id}/itemsbase path). Both are valid approaches; the current single-controller approach is acceptable for this scale.🔐 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
The IDOR mitigation is properly implemented:
findByIdAndGeschichteIdensures an item ID from journey A cannot be acted on via journey B's endpoint.@RequirePermission(Permission.BLOG_WRITE)is on all four write endpoints. Error codes are mapped in the exception handler; no stack traces or SQL leak into responses. These are the right foundations.One confirmed security concern and one smell worth examining.
Blockers
1.
toView()can expose a document that belongs to a different journey (post-ON DELETE SET NULLscenario)When a document is deleted,
ON DELETE SET NULLsetsjourney_items.document_id = null. TheJourneyItem.documentrelation becomes null, andtoViewcorrectly skips thegetSummaryByIdcall whenitem.getDocumentId() == null.However, look at
toView:item.getDocumentId()calls through Lombok's generated accessor to the@ManyToOne Document documententity field's ID accessor. In a unit test,item.getDocumentId()returns whatever you set on theJourneyItem.documentfield at build time. But the actual entity field isdocument_idon the DB side — when the document is deleted anddocument_idis set to null viaON DELETE SET NULL, the JPA entity reflects this correctly.The concern is subtler:
getSummaryByIddoes not check whether the requested document belongs to the requesting user's accessible scope. If document visibility ever becomes scoped (per-user, per-group),toView()bypasses that scope check by calling the leangetSummaryByIdwhich only does afindById. Currently, all authenticated users haveREAD_ALL— so this is not exploitable today. ButgetSummaryByIdwas introduced specifically to skip overhead, includingtagService.resolveEffectiveColors. If a future version of this method or its callers adds scope checks, the lean path will silently bypass them.Recommendation: Add a comment on
DocumentService.getSummaryByIdand at thetoViewcall site documenting that this method intentionally skips scope checks and is only safe in the current single-scope model. This makes the assumption explicit and flags it for future reviewers.Smells (not blockers today)
2.
currentUser()reads identity fromSecurityContextHolderthen does a DB lookupThis is the standard pattern in this codebase, so it's not new. The concern is:
auth.getName()returns the principal name set by Spring Security (the username fromCustomUserDetailsService). If that name is the email, anduserService.findByEmailfails to find the user (e.g. the user was deleted between login and this request), it will throw whateverfindByEmailthrows. Confirm thatfindByEmailthrows a structuredDomainException(or Spring SecurityUsernameNotFoundException) rather than a raw NPE. If it throws an unhandled exception, the global handler's catch-all will return a 500 withINTERNAL_ERRORcode to the client — which leaks that the user record is gone.3. The reorder endpoint (
PUT /{id}/items/reorder) accepts an arbitrarily large listJourneyReorderDTOhasList<UUID> itemIdswith no size bound annotation. The set-equality check againstexistingIdswill reject lists longer than the journey's item count, but this check happens after deserializing the list. A malicious client could send a payload with millions of UUIDs and the SpringObjectMapperwill allocate the full list before any validation fires. For a family archive with trusted users this is low risk, but a@Size(max = 200)annotation onitemIdswould close it cheaply.4. No
@RequirePermissiononGET /{id}/itemspath (viagetById)The GET detail endpoint already requires
READ_ALLvia@WithMockUser(authorities = "READ_ALL")in the test. That's handled at the overallGET /{id}level since Spring Security requires authentication for all/api/**routes. This is fine — noting it's not a gap, just confirming the review found no missing auth annotation on the read path.🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
The test suite is the strongest part of this PR.
JourneyItemServiceTestat 670 lines covers all four CRUD operations across happy paths, error paths, edge cases (whitespace-only notes, 130-item grandfathered journeys, duplicate IDs in reorder), and audit verification.JourneyItemConstraintsTestcorrectly avoids class-level@Transactionaland explains why — that comment is exactly the kind of QA thinking I want to see.Two structural gaps that need attention before merge.
Blockers
1. No integration test for
JourneyItemServicemethods against a real databaseJourneyItemServiceTestis a clean Mockito unit test — correct tier, correct tool. But there is no integration test that exercises the full stack:JourneyItemService.append()→JourneyItemRepository.save()→ real PostgreSQL → V73 constraint fires. The existingJourneyItemConstraintsTestverifies the constraints exist, but it uses raw JDBC — it doesn't exercise the service layer.Specific gap: the N+1 query issue (noted by Felix) would be caught by an integration test that measures query count. More importantly, the reorder method's
saveAllin the context of a DEFERRABLE UNIQUE constraint — the actual behaviour at the transaction boundary — is not tested end-to-end.JourneyItemIntegrationTest.javaalready exists (updated in this PR), but it doesn't cover the new service methods (append,updateNote,delete,reorder).Required before merge: Add at minimum one happy-path integration test for
appendand one forreorderinJourneyItemIntegrationTest, running against real Postgres via Testcontainers.2.
GeschichteControllerTest—updateItemNote_json_null_note_is_deserialized_as_empty_Optionaluses a localObjectMapperwithoutJsonNullablemoduleThe test comment says: "Raw JSON — local objectMapper lacks JsonNullableModule" — and proceeds to use
MockMvc's Spring-managedObjectMapper(which presumably has the three-way Optional semantics configured viaJacksonConfig). This is fine for theMockMvcrequest body, but the test assertion is:This asserts that the response has no
notefield. The mock returnsitemViewStub(itemId, 10, null)— soJourneyItemView.note()is null. In the response JSON, a null record component will serialize asnull(present) or absent depending onObjectMapperconfig. If the project's Jackson config usesNON_NULLserialization, it's absent. If not, it's{"note": null}anddoesNotExist()would fail. This test is asserting behaviour that depends on a Jackson configuration it isn't setting up. Confirm this passes reliably or pin the expectation tojsonPath("$.note").value(nullValue())which is unambiguous.Observations (not blockers)
should_*andverb_returns*_when*— consistent with the rest of the test suite.savedItemhelper inJourneyItemServiceTestcorrectly setsdocument(null)with an explanatory comment about avoiding LAZY issues. Good defensive test design.GeschichteQueryServiceTestcovers onlyexistsById—findByIdhas no test. Thin wrapper, acceptable, but worth a follow-up iffindByIdgains more callers.JourneyItemConstraintsTestruns@SpringBootTest(webEnvironment = NONE)with per-test JDBC teardown instead of@Transactionalrollback — correct and well-documented. TheDELETE FROM journey_itemsin@BeforeEachdoes not delete theGeschichteorDocumentrows, relying on the cascade fromGeschichte.ON DELETE CASCADE. This could leave orphaned documents across tests; confirm@BeforeEachdoesn't needDELETE FROM documentsas well.🎨 Leonie Voss — UI/UX Design Lead
Verdict: ✅ Approved
This is a backend-only PR — no Svelte components, no routes, no UI changes are included. My scope is limited to the frontend-facing artifacts: the i18n message keys, the generated API types, and the error code wiring that UI components will eventually consume.
Observations
Error messages — quality check (de/en/es)
All four new error codes have i18n strings in all three locales. The German strings are natural and audience-appropriate:
error_journey_item_position_conflict: "Die Reihenfolge wurde gerade von jemand anderem geändert – bitte laden Sie die Seite neu." — clear, actionable, senior-friendly language. ✅error_journey_at_capacity: "Die Lesereise hat bereits die maximale Anzahl von Einträgen (100) erreicht." — specific number included, helps users understand the constraint. ✅journey_item_document_deleted: "[Dokument gelöscht]" — the square-bracket convention matches other placeholder strings in the codebase. ✅Observation: The Spanish translation uses "Viaje de lectura" while the German uses "Lesereise" — this is correct localisation behaviour, not an error. The English uses "reading journey" which is clear.
Generated API types (
api.ts)The regenerated
api.tsremoves several types that were present before (GeschichteSummary,JourneyItem,DocumentSearchResult,DocumentListItemrelocations). These removals are part of a type cleanup that precedes the new endpoint additions. The consuming frontend components will need to be updated before the Lesereisen reader UI is built — the PR body correctly notes this is a WIP.No UI feedback for the
journey_item_document_deletedscenarioThe i18n key
journey_item_document_deletedis defined but there is no frontend component that uses it yet (this is a WIP PR). When the reader UI is built, ensure the deleted-document state is surfaced with this string rather than leaving the item visually blank. This is for the follow-on PR, not a blocker here.No accessibility concerns introduced
No HTML, ARIA, or focus management changes in this PR. No concerns to raise.
For the follow-on reader UI PR
When the Lesereisen reader page is built, please ensure:
journey_item_document_deleted) uses a visually distinct style (e.g.text-ink-3 italic) that communicates the tombstone state without alarming senior users🖥️ Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No Docker Compose changes, no CI workflow changes, no new infrastructure. My scope is the migration, the
jackson-databind-nullabledependency lifecycle, and any operational implications.Observations
V73 migration — operational risk note
The migration is clean and adds the right comment about PgBouncer transaction mode. One operational note: the migration uses
ALTER TABLE ... ADD CONSTRAINTtwice in a single file with noexecuteInTransaction=false. Flyway wraps each migration in a transaction by default, andALTER TABLE ADD CONSTRAINT ... DEFERRABLErequires DDL to run inside a transaction on PostgreSQL — this is correct and the comment acknowledges it. Good.The PgBouncer warning in the SQL comment is important enough to track operationally. When/if we switch from PgBouncer transaction mode to statement mode, DEFERRABLE constraints silently stop working — the deferred check fires at statement end rather than transaction end. This is not a showstopper here, but I'd want a Gitea issue or an ops note in
docs/infrastructure/production-compose.mdto track this dependency. The ADR comment from Markus applies here as well.jackson-databind-nullableremoved from pom.xmlThe PR body says
jackson-databind-nullableis removed and replaced by nativeOptional<String>. The diff shows theJacksonConfig.javais "kept as a placeholder for future custom modules." Confirm that thepom.xmldependency was actually removed (the diff shows the frontendapi.tsregen but I don't see thepom.xmldiff directly). Ifjackson-databind-nullablewas added in a previous commit on this branch and then removed, the final pom.xml should be clean. This is a verification item, not a blocker — the PR body documents the decision clearly.No new Docker services, no new environment variables, no new secrets needed
All operational criteria pass:
:latesttags introducedCI impact
JourneyItemConstraintsTestis a@SpringBootTesttest — it will start a Testcontainers Postgres. This is already the pattern used elsewhere in the integration test suite, so no CI time regression beyond the proportional addition. The 670-lineJourneyItemServiceTestis pure Mockito — runs in milliseconds.📋 Elicit — Requirements Engineer
Verdict: ⚠️ Approved with concerns
Reviewing from a requirements completeness and specification fidelity lens. The implementation is clearly built from a well-specified issue — the error codes, capacity cap, three-way PATCH semantics, and IDOR protection all suggest the specification was detailed. Two concerns relate to unresolved requirements ambiguities that could create friction for the follow-on reader UI PR.
Blockers
1. The "at least one of documentId or note" invariant is enforced on CREATE but not on the data model level
The
appendservice enforces: ifdocumentId == null && note == null → 400. But thejourney_itemstable (V72, referenced not included in this PR's diff) apparently has this invariant as a CHECK constraint. The V73 migration adds the UNIQUE and CHECK(position > 0) constraints but does not addCHECK (document_id IS NOT NULL OR note IS NOT NULL).If that CHECK constraint was added in V72 (the previous PR), this is fine and complete. If it was not, then the invariant is only enforced at the application layer with a race condition window (two concurrent writes could both pass the null check before the DB enforces anything). The PR description mentions "A CHECK constraint ensures at least one of
document_idornoteis present" — verify this is in V72, not just in the Javadoc.2. The
reorderendpoint contract is underspecified for the partial-list casePUT /{id}/items/reorderrequires the request to contain all item IDs for the journey. A partial list (wanting to move just one item) returns 400. This is a valid design choice, but it is not documented in the API or the error message. The error message is: "Requested item IDs do not match the journey's existing items" — a developer calling this endpoint would not immediately know they must send all IDs.Recommendation: Add an
@Operation/@ApiResponseOpenAPI annotation on the reorder endpoint explaining thatitemIdsmust contain all item IDs for the journey, in the desired new order. This is a docs-only fix, not a code change — but it closes a requirements communication gap for the frontend consumer.Observations
Capacity cap: 100 items
The 100-item cap is enforced via COUNT. The GLOSSARY now documents "max 100 items per journey." No issue — confirm this number was in the original issue specification and is not a constraint introduced by the implementation without product agreement.
GESCHICHTE_TYPE_MISMATCHerror codeThis code fires when a caller attempts to
appendto a STORY-type Geschichte. This is a correct guard. However, the reorder endpoint does not enforce the JOURNEY type guard — it only checks existence. If a STORY-type Geschichte somehow gets journey items (e.g. created programmatically or via import), reorder would succeed on it. This is unlikely but worth a follow-up issue to add the type guard to reorder as well, or document that STORY cannot have items as an invariant enforced elsewhere.WIP PR risk
This PR is marked as implementing Tasks 6-9 of a larger issue (#751). Four tasks remain (endpoints, slice tests, frontend codegen). Merging a WIP PR with the
getByIdreturn type changed toGeschichteViewmeans the current frontend Geschichte detail page will receive a different response shape. Confirm the frontendgeschichten/[id]page still works withGeschichteViewbefore this lands in main.doesNotExist() asserts the key is absent from the JSON object, but Jackson serializes a null Optional<String> as {"note": null} — the key is present with a null value. nullValue() correctly matches that case. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>Phase 4 review concerns addressed — 8 commits
All items from the second-pass review have been implemented and pushed to
feat/issue-751-journey-item-crud-api.Task 1 — Circular dependency refactor (
164178ec)GeschichteService.getById()now returns theGeschichteentity with the DRAFT visibility guard intact.GeschichteController.getById()callsgeschichteService.getById(), thenjourneyItemService.getItems(), thengeschichteService.toView()to assemble the view — removing theGeschichteService → JourneyItemServiceinjection.GeschichteServiceTestupdated:getByIdtests now assert onGeschichtereturn values; view-assembly tests moved totoView_*methods.GeschichteControllerTestupdated to stub both service calls.GeschichteServiceIntegrationTestupdated to call both service methods and assemble the view inline.Task 2 — N+1 SELECT eliminated (
ad90ae75)Added
findByGeschichteIdWithDocument()toJourneyItemRepositorywith aLEFT JOIN FETCH ji.document.getItems()now uses this query.toView()readsitem.getDocument()directly from the already-fetched association instead of issuing a separatedocumentService.getSummaryById()call per item. Unit tests updated to remove now-unuseddocumentServicestubs (Mockito strict stubbing would have flagged them).Task 3 — C4 L3 diagram updated (
70da532f)Added
GeschichteQueryServicecomponent tol3-backend-3g-supporting.puml. Removed the deletedRel(geschSvc, journeyItemSvc, "Delegates getItems()")arrow. AddedRel(journeyItemSvc, geschQuerySvc, "Checks Geschichte existence and type")andRel(geschQuerySvc, db, "Reads geschichten", "JDBC").Task 4 — Integration tests for append and reorder (
f9ae6a91)Added two service-level tests to
JourneyItemIntegrationTest(real PostgreSQL via Testcontainers):append_persists_item_at_position_10andreorder_swaps_positions_atomically. Uses theSecurityContextHolder+AppUserRepositoryauthentication pattern fromGeschichteServiceIntegrationTest.Task 5 — Null-note serialization test fixed (
c31f82a6)Replaced
.andExpect(jsonPath("$.note").doesNotExist())with.andExpect(jsonPath("$.note").value(nullValue()))inupdateItemNote_json_null_note_is_deserialized_as_empty_Optional. The key is present in the JSON with a null value —doesNotExist()was testing the wrong thing.Task 6 — getSummaryById Javadoc (
73004ce4)Updated the Javadoc on
DocumentService.getSummaryById()to explicitly state it skips scope checks and tag-colour resolution, and that this is safe under the current single-tenant model. The method is still called byJourneyItemService.append()for document existence validation.Task 7 — OpenAPI reorder contract (
9db3b41f)Added
@Operation(summary, description)toGeschichteController.reorderItems()documenting thatitemIdsmust contain all item IDs for the journey. A partial list returns 400 Bad Request.Task 8 — PersonNameFormatter extracted (
11082774)Created
PersonNameFormatter.join(firstName, lastName)in thegeschichtepackage. Replaced the inline string-concatenation inGeschichteService.toView()and the privatejoin()method inJourneyItemServicewith calls to this helper. All null-safety and trimming logic is now in one place../mvnw clean package -DskipTests— BUILD SUCCESS on all 8 commits. No new frontend type errors introduced.👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Solid work overall — the service layer is clean, the guard-clause pattern is applied correctly, and the test coverage is unusually thorough. A few things warrant attention before merge.
Blockers
1.
JourneyItemUpdateDTOuses rawOptional<String>as a field — Lombok + Jackson anti-patternJourneyItemUpdateDTO.java:@DatageneratessetNote(Optional<String> note). Lombok and rawOptionalas field types don't play well together —Optionalis not designed to be a field or method parameter per its own Javadoc, and some serializers (and future Jackson upgrades) will behave unexpectedly. The ADR documents whyJsonNullablewas dropped, which is fair. But the service readsdto.getNote()and null-checks it manually — this relies on an undocumented convention (null = absent). Consider using a dedicated sealed type or a booleannotePresentflag + nullableString noteValueto make the three states explicit and Lombok-friendly.This is a blocker because the convention is not self-documenting to future developers and breaks the "names reveal intent" rule. A
JourneyNoteUpdatevalue object or even a simplerecordwould be clearer.2.
GeschichteController.getByIdnow runs two queries outside a shared transactionThere is no
@Transactionalwrapping these two calls at the controller level. Between query 1 and query 2 another request could delete the Geschichte or alter its items, producing an inconsistent snapshot. This should either be wrapped in a service method that performs both lookups atomically, or the controller should delegate to a singlegeschichteService.getView(id)method that owns both queries within one transaction. Controllers are not the right place to orchestrate multi-query reads.Suggestions
3.
GeschichteQueryServiceexposesfindByIdreturningOptional— callers should not unwrapGeschichteQueryService.findByIdis called byJourneyItemService.appendand immediately.orElseThrow()'d. That's fine. ButGeschichteQueryServicecould instead exposegetByIdwith the throw included, matching the pattern used across other services (DocumentService.getById,PersonService.getById). The currentfindByIdpushes error-handling responsibility to callers.4.
reordermethod inJourneyItemService— two repository reads in a@TransactionalmethodTwo queries when one suffices.
findByGeschichteIdOrderByPositionalready fetches all items — their IDs can be extracted from the result, makingfindIdsByGeschichteIdredundant. This eliminates one round-trip and simplifies the query inventory.5.
savedItemWithDochelper inJourneyItemServiceTesthas a single-statement body with an unnecessary variableOne line:
return JourneyItem.builder()...build();— the variable adds no value.6.
toSummaryandtoVieware package-private — intentional?Both methods in
JourneyItemServicehave no access modifier. If they are test-visible by design, a@VisibleForTestingannotation (or a brief comment) would communicate the intent. Otherwise, consider making themprivateand testing through the public API.7.
GeschichteService.toViewis package-private called fromGeschichteControllertoViewis called fromGeschichteController, which is in the same package, so this compiles. But it creates invisible coupling — any refactor that moves the controller or the service to sub-packages will silently break. Either maketoViewpublic or move the assembly into the controller's own private method.What's done well
PersonNameFormatteris a clean extraction — DRY applied correctly.JourneyItemConstraintsTestuses raw JDBC and is not@Transactionalat class level — this is exactly right for testing DEFERRABLE constraints.reorder_of_grandfathered_over_cap_journey_succeedstest is a great edge case to have.🏛️ Markus Keller — Senior Application Architect
Verdict: ⚠️ Approved with concerns
The structural choices here are largely sound. The
GeschichteQueryServiceanti-cycle facade is exactly the right pattern when a sub-domain needs to reference a parent domain without circular injection. The DB diagram and C4 diagram updates are present. I have two architectural concerns that need resolution.Blockers
1. Two-query controller orchestration violates the layering rule
The layering rule is explicit: controllers never orchestrate multiple service calls for a single read. Business logic — including "load the story and its items together" — belongs in the service layer where it can be transactional, tested with
@Mock, and reused. The controller here is doing view assembly across two services.The fix: move this into
GeschichteService.getView(UUID id), which calls bothgetByIdandjourneyItemService.getItemsand assembles theGeschichteViewin one transactional method. The controller reduces to a single delegation call.2.
DocumentSummaryis placed in thegeschichtepackage — this is a cross-domain typeDocumentSummary.javalives atorg.raddatz.familienarchiv.geschichte. It represents a projection ofDocumentdata, but it's owned by thegeschichtepackage. This is acceptable if the projection is purely for journey-item rendering and never reused by other domains. However:JourneyItemServicebuilds it from aDocumententity (viaDocumentService.getSummaryById). The document domain has no knowledge of this projection, but the geschichte domain is now projecting document fields manually.This is a smell, not a hard violation — the single-tenant context means there's no security boundary being crossed. But if
Documentgains or renames fields,JourneyItemService.toSummary()will silently lag. Consider whetherDocumentServiceshould own atoSummary(Document)method that can be updated in one place when the document schema evolves.Suggestions
3.
GeschichteQueryServiceis thin but correctIts existence is justified — it prevents
JourneyItemServicefrom injectingGeschichteRepositorydirectly, which would violate the module boundary. However, it currently only exposes two methods. Consider whether this will stay minimal or grow; if it grows into a full read service it should be renamedGeschichteReadServiceor similar to signal intent.4. C4 diagram is updated but relationship direction notation
In
l3-backend-3g-supporting.puml:JourneyItemServicedoes not talk to the database directly — it talks toJourneyItemRepository. The technology label onRelshould be"JPA"not"JDBC"to be consistent with how other service→db relationships are documented. Minor, but diagram drift matters over time.5. PgBouncer transaction-mode note in the migration is good architecture documentation
The comment in
V73__add_journey_items_position_constraints.sqlabout PgBouncer transaction mode and DEFERRABLE constraints is exactly the kind of operational context that should be in a migration. Well done — this is the kind of note that prevents a 2am incident when someone changes pooling configuration.6. The 100-item cap is application-layer only — no DB-level enforcement
The cap is enforced by
countByGeschichteId >= MAX_ITEMSin the service. There is no corresponding database constraint (e.g. a trigger or a partial index). Under the single-tenant model with low concurrency, this is acceptable. But the race condition window (two concurrent appends both reading count=99 and both proceeding) is worth acknowledging explicitly, especially since the DEFERRABLE unique constraint will catch position collisions but NOT the cap. The PR body mentions this as intentional — it would be worth a one-line comment in the service method for future readers.What's done well
GeschichteQueryServicefacade is the correct pattern for this dependency direction.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
Good security awareness throughout — IDOR protection via
findByIdAndGeschichteId, email suppression inAuthorView, and structured error codes that don't leak schema details. I have one confirmed blocker and two security smells to flag.Blockers
1.
DocumentService.getSummaryByIdskips all scope checks — comment is accurate but the method is publicThe comment is honest about the single-tenant assumption, but this method is
public. Any future code can call it without the caller realizing scope checks are bypassed. In the current codebase this is exploitable only if a second tenant is added — but the public surface means the assumption is not enforced by the compiler.Fix: Either make it package-private and
@VisibleForTesting, or add a Javadoc@apiNotethat marks it explicitly asINTERNAL — skip-security — single-tenant only. Strongly prefer the package-private approach or moving the scope check into this method so it is always safe to call regardless of context. The current design is a latent privilege escalation if tenancy is introduced.Additionally: this method name
getSummaryByIdsounds like a harmless read, but it actually bypasses authorization. It should be named to signal this:getUnscopedById,getRawById, orfindByIdForInternalUse. CWE-284 (Improper Access Control) — low severity now, elevated severity on multi-tenant expansion.Security Smells (not blockers for single-tenant, but worth noting)
2.
currentUser()reads fromSecurityContextHolderdirectly — not testable and could silently failThis pattern is used correctly in the existing codebase, so it's consistent. The check
auth == null || !auth.isAuthenticated()is correct. One concern: ifauth.isAuthenticated()istruebutauth.getName()returns an anonymous user string (Spring Security'sanonymousUser),userService.findByEmail("anonymousUser")will throw an unstructured exception rather thanDomainException.unauthorized. This is an edge case in practice (anonymous tokens are only set when@RequirePermissionis not present on the endpoint), but the controller endpoints do all have@RequirePermission(BLOG_WRITE), so this is covered by the AOP check before the service is called.No action required — documenting for awareness.
3.
GlobalExceptionHandlerconstraint-name-to-ErrorCode mapping: string literal is correctThe constraint name is hardcoded as a string in the exception handler. If the constraint is renamed in a future migration, this silently reverts to the generic 400 response without any test catching it. There is currently no test that verifies the constraint-name extraction path end-to-end with a real DB (the controller test mocks the service, which bypasses the exception handler's constraint logic entirely).
Consider adding a comment referencing the migration:
// matches constraint name from V73. No code change required — just documentation.4. IDOR protection is present and correct
findByIdAndGeschichteIdon every item lookup is the right pattern. WhenitemIdexists but belongs to a different journey, the 404 response is indistinguishable from "item not found" — this is correct security behavior (no oracle for enumeration).5.
AuthorViewemail suppression is testedThe test
toView_author_email_is_not_in_author_viewexplicitly verifies the email is absent from the response. This is exactly the regression test that should exist. Good discipline.What's done well
AuthorViewleaks no email, groups, or permissions — onlyidanddisplayName.findByIdAndGeschichteId) across all item-level operations.@RequirePermission(Permission.BLOG_WRITE)is on every write endpoint — no endpoint is accidentally unprotected.401 Unauthenticatedtest forappendItemis present (most PRs only test 403).🧪 Sara Holt — Senior QA Engineer
Verdict: ✅ Approved
The test suite for this PR is one of the most thorough I've seen in this codebase. The three-layer coverage (unit / integration / constraint), the explicit audit verification, and the edge-case breadth are all excellent. I have no blockers — a few targeted gaps to fill in a follow-up.
What's Excellent
Test pyramid is properly structured:
JourneyItemServiceTest— 668 lines of pure Mockito unit tests; no Spring context. Runs in milliseconds.GeschichteControllerTest—@WebMvcTestslice, tests 401/403/404/409 per endpoint. Correct layer.JourneyItemIntegrationTest—@SpringBootTest+ real Postgres viaPostgresContainerConfig. Tests persistence, ordering, cascade delete.JourneyItemConstraintsTest— deliberately not@Transactionalat class level (correct — class-level@TransactionalbreaksDataIntegrityViolationExceptiontesting). This is exactly the right setup.Error path coverage is thorough: not-found, type-mismatch, capacity, null+no-doc, IDOR — all tested.
Audit verification uses
verify(auditService)with exactAuditKindargs — not just "was it called."Gaps to Address in Follow-up
1. No test covering the
GlobalExceptionHandler's new constraint-name-to-ErrorCode branchThe DEFERRABLE constraint fires at commit time, producing a
DataIntegrityViolationException. The handler inGlobalExceptionHandlermapsuq_journey_items_geschichte_position→JOURNEY_ITEM_POSITION_CONFLICT. The controller test mocks the service and bypasses this path entirely. The integration test for reorder (reorder_swaps_positions_atomically) does not provoke a concurrent conflict.There is no test that verifies: when two concurrent appends produce a duplicate-position
DataIntegrityViolationExceptionat the DB level, the response is409 JOURNEY_ITEM_POSITION_CONFLICTrather than the generic 400. This is the path the ADR and migration comments focus on — it deserves a test. (Track as a follow-up; acceptable to defer if the integration environment makes concurrent test setup hard.)2.
JourneyItemIntegrationTest.append_persists_item_at_position_10— missing@TransactionalrollbackThe integration test calls
journeyItemService.append(...)which commits inside its own@Transactional. The@BeforeEachseed usesdocumentRepository.saveandgeschichteRepository.save. The test class has@Transactionalat class level — but the service's own@Transactionalcommits within a nested transaction, meaning the test's rollback may not clean up items appended by the service. The@AfterEach clearSecurity()is present but there's no explicit cleanup ofjourney_items.Looking at the constraint test, it handles this via
jdbcTemplate.execute("DELETE FROM journey_items")in@BeforeEach. The integration test relies on the class-level@Transactional. This is worth verifying — if tests are order-dependent, it's a flakiness risk.3. No test for
getItems(thefindByGeschichteIdWithDocumentpath) in isolationJourneyItemService.getItemsusesfindByGeschichteIdWithDocument(the JOIN FETCH query). The integration test calls it indirectly viajourneyItemService.getItems. There's no explicit test that verifies the JOIN FETCH eliminates N+1 behavior or that note-only items (no document) are correctly included via LEFT JOIN. This is not a blocker — but N+1 regressions are silent and painful.4.
GeschichteQueryServiceTestis present but thinOnly two tests (existsById true/false).
findByIdreturningOptional.empty()is not tested. Fine for a thin service — noting it for completeness.Quality Gates Check
@Disabledtests.Thread.sleepcalls.append_to_empty_journey_starts_at_10,reorder_swaps_positions_atomically) — exemplary.Overall: this is the test suite I want to see for every feature PR. The identified gaps are all follow-up items, not merge blockers.
🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
No infrastructure changes in this PR. No new Docker services, no new environment variables, no CI workflow changes. I'm reviewing the migration, the generated API spec diff, and the dependency changes.
Observations
1. V73 migration: single-transaction guarantee is documented — good
This comment is exactly what I want to see in a migration that adds a DEFERRABLE constraint. Flyway wraps each migration in a transaction by default — the comment prevents someone from accidentally breaking this. No action needed.
2.
pom.xmlchanges:jackson-databind-nullableremoved — correctADR-035 documents the removal. The dependency is gone, and
JacksonConfig.javais retained as a placeholder. The change reduces the dependency surface, which is good from a supply-chain perspective. No new dependencies were added.3. Generated
api.tsdiff: large churn unrelated to this PRThe
frontend/src/lib/generated/api.tsdiff contains substantial changes beyond the new JourneyItem types —backfillTitlesoperation removed,search_1/search_2renamed,GeschichteSummaryremoved from the schema,Geschichteschema changed (type and items fields dropped),DocumentListItem/DocumentSearchResultmoved. This looks like the generated file was regenerated from the backend's current OpenAPI spec which includes accumulated changes from other merged PRs.This is not a problem operationally —
npm run generate:apiis deterministic and should be run after every backend change. But it makes this diff harder to review in isolation. No action required, just noting the context.4. No migration for a new table or index — only ALTER TABLE
V73 adds constraints to the existing
journey_itemstable. No new tables, no new sequences. The migration is simple, non-destructive, and reversible (constraints can be dropped). Risk to production deployment: minimal.5. No new environment variables required
The 100-item cap (
MAX_ITEMS = 100) is a compile-time constant inJourneyItemService, not a configurable environment variable. For a family archive with expected journey sizes well under 100, this is appropriate. If this ever needs tuning, it would require a code change — fine for now.What's Done Well
:latestimage tags (no Docker changes).JacksonConfig.javaplaceholder is clean — no dead code, just an extension point comment.📋 Elicit — Requirements Engineer
Verdict: ✅ Approved
Reviewing this PR against issue #751's requirements. The implementation is functionally complete for the backend scope. I'm flagging two requirement ambiguities and one gap that should be tracked.
Requirements Coverage
BLOG_WRITEon all mutationsAmbiguities to Resolve
1. "At least one of documentId or note" — where is this enforced at the DB level?
The PR enforces the "at least one" constraint in the service layer (
JourneyItemService.append). The issue description mentions "A CHECK constraint ensures at least one ofdocument_idornoteis present" — but V73 does not add this CHECK constraint. V72 (from PR #750) presumably added the column definition, but the CHECK constraint for(document_id IS NOT NULL OR note IS NOT NULL)appears to be missing from the current migration set.Recommendation: Verify that this constraint exists in V72. If it does not, create a V74 migration. The service-layer check is a race-condition guard, not a substitute for DB enforcement.
2. Reorder returns items in new order — does the response order match the requested order?
The API description says
PUT /{id}/items/reorderreturnsList<JourneyItemView>. The service:saveAllreturns items in the order they were passed to it. The order oftoSaveis derived from the iteration order ofrequestedIds. This is correct — but the acceptance criteria should explicitly state "response order matches requested order" to prevent confusion if a future refactor changes thesaveAllreturn ordering. This is a documentation gap, not a code bug.Gap:
journey_item_document_deletedi18n key exists but no UI uses it yetThe key
journey_item_document_deleted([Dokument gelöscht]/[Document deleted]) is added to all three message files. This key is intended for the reader UI when a journey item's linked document has been deleted (the FK usesON DELETE SET NULL). The backend correctly returnsdocument: nullinJourneyItemViewwhen the document is gone.The frontend currently has no component that renders
JourneyItemView, so the key is unused. This is correct — the i18n key is being added proactively. It should be tracked in the follow-on reader UI issue (#751 or a new issue) to ensure the UI actually uses it and does not silently render nothing whendocument === null.What's Complete
journey_item_document_deletedplaceholder key is correctly added ahead of the reader UI.🎨 Leonie Voss — UX Designer & Accessibility Strategist
Verdict: ✅ Approved
This is a pure backend PR — no Svelte components, no routes, no markup. There is nothing for me to audit on the UI/UX or accessibility front at this stage. My review is scoped to the API contract's implications for future frontend work.
API Contract — Frontend Readiness Notes
1.
GeschichteView.personsis aSet<PersonView>— unorderedThe
personsfield inGeschichteViewis aSet<GeschichteView.PersonView>. When the frontend renders the list of featured persons on a journey page, the order will be non-deterministic across responses. For the reading experience, a stable visual order (alphabetical by lastName, then firstName) is important for senior users who build a mental model of "who is in this story."Recommendation for the reader UI PR: sort
personsclient-side before rendering, or change the backend to return aList<PersonView>sorted bylastName, firstName.2.
DocumentSummary.receiverNamereturns the canonical first receiver onlyThe
buildCanonicalReceiverNamemethod picks the alphabetically-first receiver and returns only their name, withreceiverCountindicating how many total receivers exist. The UI convention should show"Anna Amann +2"or similar whenreceiverCount > 1. This needs explicit UI treatment in the journey reader component — a silent truncation would confuse the 60+ audience who may not realize there are more recipients.Track in the reader UI issue.
3.
journey_item_document_deletedi18n key — ensure it is visually distinctWhen
document === nullin aJourneyItemView, the reader UI must render[Dokument gelöscht]in a visually distinct style — muted text, perhaps with aline-throughor ghost card — so users understand the item is intentionally present but the document is no longer accessible. A plain text label without visual treatment would be confusing.Track in the reader UI issue.
4. No frontend components or routes introduced — nothing to audit for accessibility
No Svelte files were changed. No WCAG criteria apply to this diff. The i18n keys use appropriate German (
"Der Reise-Eintrag wurde nicht gefunden."is clear, non-technical, and direct). The Spanish translation reads naturally. The English translation is correct.Summary
This PR lays the right foundation. The design decisions that will affect senior users — sorted persons, receiver count display, deleted-document visual treatment — are all in the reader UI layer that follows. I will review that PR with full UX and accessibility scrutiny. Nothing to block here.
PR #788 review fixes — all concerns addressed
Task 1 — Restore
GeschichteService.getView()with@Transactional(readOnly=true)✅Commit
f09c7974Re-injected
JourneyItemServiceintoGeschichteService. The dependency chain is clean:GeschichteService → JourneyItemService → GeschichteQueryService— no cycle.Added
getView(UUID id)annotated@Transactional(readOnly=true)that:getById(id)(DRAFT visibility check, 404 if not found)journeyItemService.getItems(id)GeschichteViewviatoView()Controller's
getById()now delegates to a singlegeschichteService.getView(id)call — the two-transaction gap is closed.Tests updated:
GeschichteControllerTest.getById_returns200_whenFound: now stubsgeschichteService.getView(id)only — simpler setupGeschichteControllerTest.getById_returns404_whenServiceThrowsNotFound: stubsgetView()instead ofgetById()GeschichteServiceTest: added@Mock JourneyItemService journeyItemServiceto@InjectMockssetup; addedgetView_returns_assembled_view_and_delegates_to_journeyItemService()andgetView_throws_NOT_FOUND_when_id_unknown()testsTask 2 — DB CHECK constraint ✅ (no change needed — already present)
The constraint is already in
V72__add_journey_items_migrate_geschichten_documents.sql:No new migration was needed.
Task 3 — Rename
getSummaryById→findSummaryByIdInternal✅Commit
99111273Renamed in all three locations:
DocumentService.java— method declarationJourneyItemService.java— sole production callerJourneyItemServiceTest.java— mock stubThe Javadoc already documented the scope-check bypass; it remains accurate with no further changes needed.
Task 4 —
toView()visibility ✅ (no change needed)After Task 1,
toView()is called only fromgetView()withinGeschichteService. The controller no longer calls it. The existing package-private access is correct — same-package tests (GeschichteServiceTest,GeschichteServiceIntegrationTest) can access it without any visibility change.Verification
./mvnw clean package -DskipTests→ BUILD SUCCESS after both commitsnpm run check→ no new errors (pre-existing 834 baseline errors are unrelated to these changes)feat/issue-751-journey-item-crud-api@99111273👨💻 Felix Brandt — Senior Fullstack Developer
Verdict: ⚠️ Approved with concerns
Solid core: the three-way PATCH semantics with
JsonNullableare implemented correctly, thereorderalgorithm is clean, and the test coverage across unit + constraint layers is genuinely good work. A few things need fixing before this ships.Blockers
1.
currentUser()calls UserService inside a@Transactionalmethod — potential N+1 per writeIn
JourneyItemService, every write method callscurrentUser()after the main DB work, which in turn callsuserService.findByEmail(auth.getName())— a second DB round-trip inside the transaction. This adds latency to every append/updateNote/delete call and is unnecessary: theAppUserprincipal is already available in the Spring Security context as the fully-hydrated user object (if loaded that way). Either inject the user lookup once at the top of each method and reuse it, or resolve it from the principal directly. The current pattern callsfindByEmailon every write even though auth is already established.2.
append()doescount+findMaxPosition— two separate queries that could raceIn
JourneyItemService.append():Both run inside the transaction but between them another concurrent append can sneak in. The DEFERRABLE constraint prevents a corrupt position, but
countByGeschichteIdcan then return a stale count (the capacity check may pass when it shouldn't). Consider combining into a singlefindMaxPositionAndCountprojection query.3.
JOURNEY_ITEM_NOTE_UPDATEDis inAuditKindbut NOT inErrorCode.java/errors.tsThe PR summary lists
JOURNEY_ITEM_NOT_FOUND,JOURNEY_ITEM_POSITION_CONFLICT,JOURNEY_AT_CAPACITY,GESCHICHTE_TYPE_MISMATCHas ErrorCodes (all present) — butJOURNEY_ITEM_NOTE_UPDATEDis anAuditKind, not anErrorCode. That's correct and intended. However,CLAUDE.mdrequires: when adding a newErrorCode, add the i18n key. The four new error codes ARE added toerrors.ts→getErrorMessage(), but the corresponding i18n message keys (error_journey_item_not_found, etc.) need to be present inmessages/{de,en,es}.json. The diff does not show those files being updated. This would cause a runtimem.error_journey_item_not_found is not a functionerror in the frontend the moment one of these codes surfaces. Blocker.Suggestions
4.
reorder()buildsHashMapthen iteratesrequestedIds— useStreaminstead5.
GeschichteViewrecord —AuthorViewfields lack@Schema(requiredMode = REQUIRED)on non-null fieldsAuthorView(String firstName, String lastName, String displayName)— these are always populated when an author exists. WithoutREQUIRED, the TypeScript types will be optional (string | undefined) and frontend code will need unnecessary null guards. Check all three fields.6.
GeschichteQueryServicejavadoc comment is cut offThe comment reads: "Exists so that
JourneyItemServicecan check Geschichte existence and load Geschichte instances without holding a direct reference to the Geschichte repository (cross-dom" — it's truncated mid-word in the diff. Make sure the source file has the complete sentence.7. Test factory method naming:
makeDocvsmakeDocumentJourneyItemServiceTestusesmakeDoc(...)while most of the test codebase usesmakeDocument(...). Minor inconsistency — pick one and be consistent within the test file at minimum.🏛️ Markus Keller — Application Architect
Verdict: ⚠️ Approved with concerns
The structural decisions here are sound. The
GeschichteQueryServicesplit to break the cyclic dependency betweenJourneyItemServiceandGeschichteServiceis exactly right — constructor injection cycle avoidance without@Lazy, just like the codebase guidance requires. Thejourneyitem/sub-package undergeschichte/is the correct placement. One genuine blocker and a few things worth tracking.Blockers
1.
DocumentSummaryrecord lives in thegeschichtepackage — boundary violationDocumentSummaryis defined inorg.raddatz.familienarchiv.geschichte.DocumentSummary. It is a lean view of aDocumententity, but it belongs to thegeschichtedomain. That means thedocumentdomain cannot use it, and if it ever needs to (e.g., a future document detail page in a different domain context), you'll have a circular import. The conventional placement is either insidegeschichte/journeyitem/(as a package-private record used only there) or — if it will be reused — a shared read-model indocument/. Currently it's only used byJourneyItemService.toSummary(), sogeschichte/journeyitem/DocumentSummary.javaor even an inner record would be cleaner.Documentation Gaps (Architect's hard rule: doc omission = blocker, not concern)
2. New
GeschichteQueryServiceis not reflected in the C4 backend diagramPer the doc-update table in both CLAUDE.md and my review rule: a new Spring Boot service in an existing domain requires updating
docs/architecture/c4/l3-backend-geschichten.puml(or equivalent). The diff showsCLAUDE.mdwas updated withGeschichteQueryServicein the package listing, which is good — but the C4 L3 diagram was not updated. Blocker until the diagram is current.3.
DocumentService.findSummaryByIdInternal— new public method in document domain; C4 diagram needs updateSimilarly, a new public service method on
DocumentServicein the document domain should be reflected in the backend C4 diagram for that domain if it represents a meaningful cross-domain interface point. Given theInternalsuffix signals it's an internal-only boundary, the doc update may be minimal — but it needs to be there.Architectural Observations (Suggestions)
4. The
reorderendpoint usesPUTfor full replacement — correct and intentional, well-documented via@OperationUsing
PUT /{id}/items/reorderis semantically correct: the client sends the complete ordered list and it replaces the current order. This is consistent with REST conventions and the@Operationdescription makes the all-IDs-required semantics explicit. Good call.5. The
DEFERRABLE INITIALLY DEFERREDconstraint comment in V73 is exactly rightThe migration comment noting the PgBouncer transaction-mode dependency is precisely the kind of consequence documentation an ADR would normally capture. Since this decision (deferrable constraints work only in transaction-mode pooling) has lasting infrastructure consequences, consider whether a brief ADR entry is warranted — or at minimum a note in
docs/DEPLOYMENT.mdlinking the constraint name to the pooling requirement, so a future infrastructure change doesn't silently break deferred checking.6. Capacity check (MAX_ITEMS = 100) is application-only — no database-layer enforcement
The 100-item cap is enforced only in
JourneyItemService.append(). A bug, a direct SQL insert, or a future batch operation could bypass it. For truly important invariants, aCHECK (position <= 1000)equivalent or aBEFORE INSERTtrigger would enforce it at the DB layer. Given this is a family archive with modest data volumes, application-layer enforcement is acceptable — but I'd document the deliberate choice rather than leaving it implied.🔒 Nora "NullX" Steiner — Application Security Engineer
Verdict: ⚠️ Approved with concerns
Good security posture overall. IDOR protection is in place via
findByIdAndGeschichteId(item ID alone is insufficient to access an item — thegeschichteIdpath parameter must also match).@RequirePermission(Permission.BLOG_WRITE)is correctly applied to all write endpoints. The error handler does not leak SQL or constraint values (CWE-209). One genuine security concern and one smell to address.Blockers
1.
findSummaryByIdInternaldeliberately skips authorization — the comment confirms this, and it's a risk surface that needs hardening documentationIn
DocumentService:The comment acknowledges the skipped auth. Under a single-tenant model this is currently safe. But:
@Transactionalcontext where the caller has already validatedBLOG_WRITEpermission.Internalis a naming convention, not an enforcement mechanism. A future contributor could call it from an unauthenticated code path without noticing the caveat.Fix: Either (a) make the method package-private and add a
@VisibleForTestingannotation, or (b) add a comment pointing to the permission check that must have occurred before calling this — and add a test toJourneyItemServiceTestthat verifies theappendpath checks permissions before calling the document lookup. Currently the unit tests mock the auth context but don't explicitly verify the permission boundary is enforced before the document fetch.Concerns (not hard blockers, but worth addressing)
2.
reorder()validates thatrequestedIdsmatchesexistingIdsexactly — good. But the implementation usesHashSetequality which ignores orderThe code correctly checks
existingIds.equals(new HashSet<>(requestedIds))to verify the same items are present. This is fine for completeness validation. The IDOR surface is thefindByIdAndGeschichteIdusage in per-item operations. The reorder endpoint does not do per-item IDOR checks — it validates set equality instead. This is logically correct (if the sets match, all IDs belong to this journey) but confirm thatfindIdsByGeschichteIdis scoped to thegeschichteIdparameter and not returning a global query. Based on the repository definition this appears correct.3. No test for
GESCHICHTE_TYPE_MISMATCHerror code pathThe
append()method throwsGESCHICHTE_TYPE_MISMATCHwhen a client tries to add items to a STORY-type Geschichte. This is a security-relevant guard (prevents misuse of the API). There should be a unit test inJourneyItemServiceTestthat verifies this 409 is returned wheng.getType() != GeschichteType.JOURNEY. Without the test, a refactor could silently remove this guard.4.
GlobalExceptionHandler— constraint name mapping is in a public handler methodThe code correctly logs only the constraint name (not the SQL or values). The string comparison
"uq_journey_items_geschichte_position".equals(constraint)is fine — it's an exact match, not a contains() that could be spoofed. This is correct. Just noting it as reviewed-and-intentional.Positive Findings
findByIdAndGeschichteIdis the right IDOR guard — item ID alone cannot be used to access or delete another journey's items.DomainExceptioncalls include the item ID for debugging but not sensitive user data.🧪 Sara Holt — Senior QA Engineer
Verdict: ⚠️ Approved with concerns
The test strategy is strong at the unit layer.
JourneyItemServiceTesthas excellent coverage of the happy path and most error branches.JourneyItemConstraintsTestusing real Postgres via Testcontainers (not H2) for the deferrable constraint verification is exactly right — that test would silently pass on H2 and mean nothing. A few critical coverage gaps remain.Blockers
1. No
@WebMvcTestslice for the new JourneyItem endpoints onGeschichteControllerThe diff adds four new endpoints to
GeschichteController:POST /{id}/itemsPATCH /{id}/items/{itemId}DELETE /{id}/items/{itemId}PUT /{id}/items/reorderGeschichteControllerTestis updated forgetByIdreturningGeschichteView, but there are no@WebMvcTesttests covering:BLOG_WRITE(unauthorized user test)geschichteIdnot foundThe
@RequirePermission(Permission.BLOG_WRITE)annotation means all four endpoints should have a 403 test. Without these, permission enforcement is untested at the HTTP layer. Blocker per the project's quality gate.2. No test for
GESCHICHTE_TYPE_MISMATCHcode path inJourneyItemServiceTestappend()throws this error when called on a STORY-type Geschichte. This error path has no test. Given it's a guard against misuse, it should be explicitly tested:3. Missing i18n keys for new error codes will cause
npm run checkto failAs Felix noted, the four new
ErrorCodevalues added toerrors.ts→getErrorMessage()call Paraglide message functions (m.error_journey_item_not_found(), etc.) that don't appear to be defined inmessages/{de,en,es}.jsonbased on the diff. This will fail thesvelte-checkbaseline and potentially break the frontend build. Should be caught before merge.Suggestions
4.
JourneyItemConstraintsTest—@BeforeEachseed usesDELETE FROM journey_itemsbut not within a transactionThe test class comment correctly explains why
@Transactionalis NOT used at the class level (to avoid rollback-only cascade onDataIntegrityViolationException). TheDELETE FROM journey_itemsin@BeforeEachis the right cleanup approach. However, thisDELETEis a raw JDBC call that cascades correctly — worth noting this could leave orphan data ingeschichtenanddocumentstables across test runs if the test fails before reaching the@BeforeEach. Consider addingDELETE FROM geschichten WHERE title = 'Constraints-Test-Journey'to the teardown or using unique titles per test run.5.
reorder_emits_audit_eventtest — should also verifygetItemsreturns items in new orderThe reorder unit test (
reorder_logs_audit_event_after_commit) only verifies the audit call. It doesn't verify that positions were actually updated on the returned views. Add a position-order assertion:6. Test coverage for
reorderwith an empty journeyWhen
requestedIdsis empty andexistingIdsis also empty,reorder()returnsList.of()early. There's no test for the empty-journey case. Worth adding since the early-return path is untested:Positive Findings
JourneyItemConstraintsTestuses real PostgreSQL viaPostgresContainerConfig— the deferrable constraint verification is genuinely meaningful.updateNote_absent_leaves_note_unchangedtests the three-way PATCH no-op path — this is the hardest semantic to verify and it's tested correctly.append_rejects_when_both_documentId_and_note_are_null— explicit test for the "at least one required" validation rule.🚀 Tobias Wendt — DevOps & Platform Engineer
Verdict: ✅ Approved
This PR is purely application-layer — no Docker Compose changes, no new infrastructure, no CI workflow modifications. Flyway V73 migration will run cleanly on the existing PostgreSQL 16 container; the
DEFERRABLE INITIALLY DEFERREDconstraint is supported natively. Thejackson-databind-nullable 0.2.6dependency addition topom.xmlis a small library with no operational footprint.Observations (not blockers)
1. The V73 migration comment explicitly calls out the PgBouncer transaction-mode dependency — infrastructure team needs to be aware
The migration says:
This is the right place to document this. From an infrastructure perspective: if we ever reconfigure PgBouncer to
pool_mode = statement, theDEFERRABLE INITIALLY DEFERREDconstraint silently stops working because deferred constraints require the connection to persist for the full transaction. The current transaction mode is correct. Add this to the infrastructure runbook /docs/DEPLOYMENT.mdso it's not just in a migration comment that gets buried.2.
jackson-databind-nullableis a new production dependency — check it's included in OWASP dependency scanThe CI pipeline should be running
./mvnw dependency-check:checkor similar as part of the build.jackson-databind-nullable 0.2.6is a small OpenAPI Tools library with minimal attack surface, but confirm it's picked up by whatever CVE scanning is in place so future vulnerability disclosures are caught automatically.3. No infrastructure changes needed — good
Adding JourneyItem CRUD to an existing REST controller on an existing service with an existing database keeps operational complexity flat. The only schema change (V73) adds two constraints to an existing table — this is a non-destructive migration that will run in milliseconds on the current data volume.
Summary
Clean PR from an infrastructure perspective. The migration is safe, the dependency is minimal, and nothing in the CI/CD pipeline needs updating. The PgBouncer note is the one thing worth propagating to the infrastructure documentation.
🎨 Leonie Voss — UI/UX Design Lead & Accessibility Strategist
Verdict: ✅ Approved
This is a backend API and data model PR — no frontend components, no Svelte files, no UI markup are changed. The changes that do touch the frontend (
errors.ts,messages/i18n keys,api.tstype regeneration) are infrastructure for the UI that will be built on top of this layer. My review is limited to what's here, with observations for what the UI work needs to address.No UX/Accessibility Blockers in This PR
The only frontend-touching changes are:
frontend/src/lib/shared/errors.ts— four new error codes added togetErrorMessage()switchfrontend/src/api.ts— regenerated types (no UI changes)These are pre-conditions for UI work, not UI work itself. No accessibility violations to flag here.
Forward-Looking Notes for the UI Sprint
When the JourneyItem UI is built on top of this API, flag these concerns for the frontend PR:
1. The
reorderendpoint's UX contract is strict — the UI must send ALL item IDsPUT /{id}/items/reorderrequires the complete ordered list. If the frontend only sends a partial list (e.g., the items visible in the viewport), the server returns a 400. The drag-and-drop component must manage the full ordered state client-side, not just the moved item. This is a non-obvious constraint that needs to be surfaced in the component's design.2.
JOURNEY_AT_CAPACITY(100 items) — the UI needs a proactive state, not just an error responseWhen a journey has 100 items, the "Add item" affordance should be disabled/hidden rather than letting the user attempt the action and receive a 400. The API response alone is insufficient UX for the senior audience. The
GeschichteViewresponse includes the item list — the frontend can deriveisAtCapacity = items.length >= 100and hide the add button accordingly.3.
notefield character limit (5000 chars) — needs visible counter in the edit UIThe service enforces
MAX_NOTE_LENGTH = 5000. The frontend note input needs a character counter that activates as the user approaches the limit (>4000chars = warning indicator). The senior audience especially benefits from visible progress on text limits.4.
DocumentSummaryinJourneyItemView— sparse data, be careful with display assumptionsDocumentSummaryonly includessenderNameandreceiverNameas strings (not fullPersonobjects). These can benullif the document has no sender metadata. The JourneyItem card component must handle null sender/receiver gracefully — empty strings or a fallback like "Unbekannt" rather thannulldisplayed raw.5. Accessibility: drag-to-reorder requires keyboard alternative
Any drag-and-drop reorder UI built on top of the
reorderendpoint must provide a keyboard-accessible alternative (e.g., move up/move down buttons). This is a WCAG 2.1 success criterion 2.1.1 (Keyboard). ThePUT /reorderendpoint supports this natively since it accepts any ordering.📋 Elicit — Requirements Engineer & Business Analyst
Verdict: ⚠️ Approved with concerns
Reviewing this PR through the requirements and product lens: does the implemented API correctly and completely serve the use cases described in issue #751? The implementation is mostly well-aligned with the domain intent, but several requirements-level gaps warrant attention before the UI sprint begins.
Blockers
1. The
GeschichteViewresponse omitstype— the frontend cannot distinguish STORY from JOURNEYThe diff shows the
Geschichteschema inapi.tsnow lacks thetypefield:The
GeschichteViewrecord (the new read model returned byGET /api/geschichten/{id}) appears to also omit type. Withouttypein the response, a frontend component rendering a journey detail page cannot confirm it is rendering a JOURNEY rather than a STORY — which means it cannot safely show or hide the item management UI. TheGeschichteViewmust includetype(or a booleanisJourneyequivalent) so the frontend can gate the JourneyItem UI correctly.2.
GET /api/geschichten/{id}now returnsGeschichteViewbutGET /api/geschichten(list) still returnsGeschichteSummary[]→ thenGeschichte[]The diff shows the list endpoint return type changed from
GeschichteSummary[]toGeschichte[]. This is an inconsistency: the detail view returns a newGeschichteView(withAuthorView, item list, etc.) while the list returns the fullGeschichteentity (withAppUserauthor that may expose email). Additionally, theGeschichteSummarytype is removed from the OpenAPI schema — if any frontend code was usingGeschichteSummary, it now getsGeschichtewhich is a different shape. This is a breaking change to the list endpoint response contract. Confirm this is intentional and that all callers have been updated.Requirements Gaps (Suggestions)
3. No
GET /api/geschichten/{id}/itemsdedicated endpoint — items are only available via the fullGeschichteViewThe CRUD API adds
POST,PATCH,DELETE,PUT(reorder) for items, but there is no standaloneGET /{id}/itemsread endpoint. Items are only accessible via the fullGeschichteViewfromGET /{id}. This is acceptable for the initial implementation, but if the UI needs to refresh just the item list after an edit (without re-fetching the whole Geschichte), it will over-fetch. Flag this as a potential future endpoint, or confirm the UI will always re-fetch the full view.4. No
documentIdupdate path — once a JourneyItem has a linked document, it cannot be changed to link to a different documentThe
PATCH /{id}/items/{itemId}endpoint (viaJourneyItemUpdateDTO) only allows updating thenotefield. There is no way to update thedocumentIdon an existing item. To change the linked document, a user must delete the item and re-add it, losing position context. Is this intentional? If users may link the wrong document, adocumentIdupdate path would reduce friction. Worth capturing as a requirement for a follow-up issue.5. No bulk-add path for multiple items
When a user wants to add several documents to a journey at once (e.g., selecting 5 documents in a batch), the current API requires 5 separate
POST /{id}/itemscalls. This is fine for an MVP, but if the UI will support multi-select from the document list, a bulk append endpoint (POST /{id}/items/batch) would reduce request overhead. Not a blocker — just worth capturing in the backlog.6. The 100-item capacity is a product decision that should be visible in the issue
MAX_ITEMS = 100is hardcoded inJourneyItemService. The reasoning (preventing abuse, keeping the UI manageable) is not captured in the issue or in the code beyond the constant name. If a family archive eventually has a 150-document letter series that needs a journey, 100 may be too low. Recommend capturing the business rationale in issue #751 or the GLOSSARY so the decision is revisitable.Positive Alignment
null= clear, string = set) correctly model the user intent for the note field: a user can clear a note they typed in error, and an absent note field in a PATCH request doesn't accidentally clear an existing note.JOURNEY_AT_CAPACITYerror code enables a good UX: the frontend can show a specific, actionable message rather than a generic 400.Phase 4 complete — two review concerns addressed:
84b47f18fix(geschichte): move DocumentSummary to journeyitem sub-packageDocumentSummary.javafromorg.raddatz.familienarchiv.geschichteintoorg.raddatz.familienarchiv.geschichte.journeyitemwhere it belongs (it is only used by JourneyItem domain).JourneyItemViewandJourneyItemService.77cbbd34test(journeyitem): verify findSummaryByIdInternal never called before JOURNEY-type guardDocumentService.findSummaryByIdInternalto explicitly document: (1) the method intentionally bypasses scope checks, (2) it is only safe after@RequirePermission(BLOG_WRITE)is enforced at the controller layer, and (3) inJourneyItemService.append()it is additionally guarded by the JOURNEY-type check that fires before the document lookup.append_never_calls_findSummaryByIdInternal_when_geschichte_type_is_STORYtoJourneyItemServiceTest— asserts thatdocumentServicehas no interactions when the JOURNEY-type guard rejects a STORY-type Geschichte, proving the security layering holds.