57 lines
5.7 KiB
Markdown
57 lines
5.7 KiB
Markdown
# Code review — NEO-129 (E7M2-06)
|
|
|
|
**Date:** 2026-06-07
|
|
**Scope:** Branch `NEO-129-quest-progress-completion-reward-summary` vs `main` — commits `2b06eb2` … `78e805e`
|
|
**Base:** `main`
|
|
**Follow-up:** Review suggestions and nits addressed in a subsequent commit on the same branch.
|
|
|
|
## Verdict
|
|
|
|
**Approve with nits** — additive `completionRewardSummary` on completed quest-progress rows, sourced from `IRewardDeliveryStore`, matches the adopted plan and NEO-108 encounter precedent; mapper threading through `QuestAcceptApi` is correct; tests and Bruno cover primary AC paths; risk is low (server-only until NEO-131 client parse).
|
|
|
|
## Summary
|
|
|
|
NEO-129 extends **`GET /game/players/{id}/quest-progress`** with optional **`completionRewardSummary`** on **`completed`** rows: nested **`itemGrants`** + **`skillXpGrants`** mapped from **`RewardDeliveryEvent`** snapshots via **`IRewardDeliveryStore.TryGet`**, omitted for **`not_started`** / **`active`** and when no delivery record exists (kickoff). **`QuestProgressApi.BuildSnapshot`** / **`MapQuestProgressRow`** accept **`playerId`** + **`deliveryStore`**; **`QuestAcceptApi.MapResponse`** uses the same mapper so embedded accept rows stay consistent. DTOs in **`QuestProgressListDtos.cs`** use **`JsonIgnore(WhenWritingNull)`** and preserve commit-time grant order (XML on mappers mirrors NEO-108). Eight AAA integration tests in **`QuestProgressApiTests`** assert omit on default/active, gather-intro salvage **25**, operator-chain item + XP, and idempotent GET replay; Bruno adds gather-intro complete spine plus default GET omit assertion. Docs (plan reconciliation, E7.M2 anchor, alignment register, E7M2-06 backlog AC, **`server/README.md`**) are updated.
|
|
|
|
## Documentation checked
|
|
|
|
| Path | Result |
|
|
|------|--------|
|
|
| `docs/plans/NEO-129-implementation-plan.md` | **Matches** — kickoff nested shape and omit-on-miss adopted; reconciliation accurate; AC checked. |
|
|
| `docs/plans/E7M2-prototype-backlog.md` (E7M2-06) | **Matches** — AC checkboxes complete; client counterpart NEO-131 correctly deferred. |
|
|
| `docs/decomposition/modules/E7_M2_RewardAndUnlockRouter.md` | **Matches** — NEO-129 HTTP read anchor added; separated from NEO-128 wiring note. |
|
|
| `docs/decomposition/modules/documentation_and_implementation_alignment.md` | **Matches** — E7.M2 row notes E7M2-06 HTTP read; row correctly stays **Planned** until NEO-132 capstone. |
|
|
| `docs/decomposition/modules/module_dependency_register.md` | **Matches** — no register change required. |
|
|
| `server/README.md` | **Matches** — quest-progress section documents **`completionRewardSummary`** projection source and gather-intro sample. |
|
|
| Full-stack epic decomposition | **N/A** — E7M2-06 is server HTTP read; client counterpart [NEO-131](https://linear.app/neon-sprawl/issue/NEO-131) explicitly out of scope. |
|
|
|
|
**Register / tracking:** E7.M2 correctly remains **Planned** until capstone NEO-132; alignment update is appropriate.
|
|
|
|
## Blocking issues
|
|
|
|
None.
|
|
|
|
## Suggestions
|
|
|
|
1. ~~**GET-refresh chain completion summary** — `GetQuestProgress_ShouldCompleteChainTerminal_WhenTokenHeldAndInventoryRefreshRunsOnGet` asserts **`completed`** via inventory refresh wiring but does not assert **`CompletionRewardSummary`** (unlike gather and operator-chain complete tests). That path exercises deliver-then-mark; adding item + skill XP asserts would lock summary projection on the GET side-effect completion path.~~ **Done.** `AssertOperatorChainCompletionRewardSummary` on GET-refresh chain terminal test.
|
|
|
|
2. ~~**Bruno idempotent second GET** — The implementation plan Bruno AC calls for a second GET unchanged; **`Get quest progress after gather intro complete.bru`** performs only one GET. Optional follow-up: duplicate GET in pre-request or a second request file to match plan and integration test coverage.~~ **Done.** Pre-request first GET + `bru.setVar`; main GET test asserts `eql` with stored summary.
|
|
|
|
3. ~~**Completed without delivery record** — Kickoff adopts omit-on-miss (softer than NEO-108 fail-fast). No test forces **`status: completed`** with a store miss (e.g. direct **`IPlayerQuestStateStore`** seed without **`IRewardDeliveryStore`** record). Low priority given NEO-128 deliver-then-mark invariant in normal paths; a single defensive test would document the contract.~~ **Done.** `GetQuestProgress_ShouldOmitCompletionRewardSummary_WhenCompletedWithoutDeliveryRecord`.
|
|
|
|
## Nits
|
|
|
|
- ~~Nit: Idempotent replay test (`GetQuestProgress_ShouldReturnSameCompletionRewardSummary_WhenGatherIntroCompletedTwice`) compares only the first **`skillXpGrants`** line, not full **`completionRewardSummary`** object equality (Bruno uses **`eql`** on arrays). Fine for single-line gather intro; operator-chain idempotent replay is untested.~~ **Done.** `AssertCompletionRewardSummariesEqual` helper; added operator-chain idempotent replay test.
|
|
|
|
- ~~Nit: **`QuestAcceptApiTests`** do not assert **`CompletionRewardSummary`** null on active accept embedded rows. Mapper consistency is covered indirectly via shared **`MapQuestProgressRow`**; a one-line assert on existing accept-success tests would mirror GET omit coverage.~~ **Done.** Null on active accept; summary on already-completed deny row.
|
|
|
|
- Nit: Grant-order XML is present on **`MapItemGrants`** / **`MapSkillXpGrants`**; C# operator-chain test asserts fields via **`Single`** / index **0** (order-agnostic for single grants). Acceptable for prototype freeze table; Bruno gather test asserts ordered **`skillXpGrants`** array.
|
|
|
|
## Verification
|
|
|
|
```bash
|
|
cd server && dotnet test NeonSprawl.Server.Tests/NeonSprawl.Server.Tests.csproj --filter "FullyQualifiedName~QuestProgressApiTests"
|
|
```
|
|
|
|
Optional: run Bruno folder `bruno/neon-sprawl-server/quest-progress/` against a running dev server (especially **`Get quest progress after gather intro complete.bru`**).
|