diff --git a/.gitignore b/.gitignore index c2157ee6..df17244f 100644 --- a/.gitignore +++ b/.gitignore @@ -39,3 +39,5 @@ cypress.env.json next-env.d.ts backlog/** + +docs/adr/** diff --git a/docs/adr/0001-bulk-upload-state-machine.md b/docs/adr/0001-bulk-upload-state-machine.md deleted file mode 100644 index 741e4a36..00000000 --- a/docs/adr/0001-bulk-upload-state-machine.md +++ /dev/null @@ -1,20 +0,0 @@ -# BulkUpload state machine (v1) - -The BulkUpload lifecycle is: - -``` -ready_for_processing → mapping_complete → processing → combining → awaiting_review - → complete | needs_review | failed -``` - -Two writers: Next.js writes the user-driven transitions (`mapping_complete`, `processing`, `complete`, `needs_review`); the FastAPI `bulk_address2uprn_combiner` worker writes `combining` and `awaiting_review` directly to the DB during its run. Any aggregate enforcing the state machine on the Next.js side must treat those two as observed-not-owned. - -Three deliberate "not yet" decisions are baked into this version, each likely to be re-suggested without a record: - -1. **`needs_review` is terminal — no recovery flow.** A finalised upload with missing or duplicate UPRNs ends up here, and that's it. The imported Properties show up in the property table; a proper review/recovery UI is planned but out of scope. Without this note, every future architecture pass will surface "needs_review has no exit" as a bug. - -2. **Re-mapping is rejected after `mapping_complete`.** PATCHing `columnMapping` once Address matching has been triggered returns 409. We considered (B) reset-and-rerun (clear the combiner output, return to `mapping_complete`, force the user to re-trigger) but rejected it for now: it requires cleaning up abandoned Tasks/SubTasks and re-charging a FastAPI run. (A) is the smallest correct thing — the DB column never drifts from what produced the combined output. Revisit when re-run-from-review lands. - -3. **`failed` exists in the schema but is not yet written by any route.** Synchronous trigger failures (route can't reach FastAPI) are surfaced as React Query toasts on 5xx; the BulkUpload stays in its prior status and the user retries. In-flight failures (Combiner crashes silently) currently leave uploads stuck in `processing` — this is known-incomplete, waiting on FastAPI-side callback work to report failure. The status is in the schema now so the seam is ready when that work is scheduled. - -These are the only "no" decisions in v1; everything else (concurrency guards on stage triggers, persisting `failed`, etc.) is intended to be added. diff --git a/docs/adr/0002-bulk-upload-browser-driven-orchestration.md b/docs/adr/0002-bulk-upload-browser-driven-orchestration.md deleted file mode 100644 index f27a5e07..00000000 --- a/docs/adr/0002-bulk-upload-browser-driven-orchestration.md +++ /dev/null @@ -1,17 +0,0 @@ -# BulkUpload pipeline stays browser-driven for now - -The BulkUpload pipeline chains three stages — address matching, combiner trigger, finalise — and the chain is currently driven by the **frontend polling** in `OnboardingProgress.tsx`. The browser fires `POST /combine` when the Task looks done, then `POST /finalize` when the upload reaches `awaiting_review`. Close the tab → upload gets stuck. - -A server-driven alternative (FastAPI callback into a Next.js webhook, or a sweeper route) would be more robust, but the entire bulk-upload flow is scheduled for redesign. This code path is being kept as **internal tooling for the Domna tech team** to onboard portfolios while the new flow is built — not for end-user use. - -So the cleanup invests only in things that survive the redesign: -- A real state machine on the BulkUpload aggregate (so the tech team can debug from the row alone). -- Deduplicated FastAPI trigger logic. -- Concurrency guards on stage triggers. - -It deliberately does **not** invest in: -- Server-side stage orchestration. -- Recovery from `failed` / stuck uploads. -- A real `awaiting_review` review UI. - -When the redesign lands, browser-driven chaining and the auto-finalise behaviour go with it. Future readers asking "why didn't they fix this" — that's why. diff --git a/docs/adr/0003-task-creation-inside-bulk-upload.md b/docs/adr/0003-task-creation-inside-bulk-upload.md deleted file mode 100644 index 2915346a..00000000 --- a/docs/adr/0003-task-creation-inside-bulk-upload.md +++ /dev/null @@ -1,12 +0,0 @@ -# Task creation lives in BulkUpload, not behind a generic Task endpoint - -A `Task` is the orchestration handle for a FastAPI pipeline run; per `CONTEXT.md`, **a BulkUpload has at most one Task**. Today BulkUpload is the only feature that creates Tasks. - -We're collapsing the previous two-step client seam — `POST /api/tasks` to create a Task and SubTask, then `POST /bulk-uploads/[uploadId]/start-address-matching` with the resulting IDs — into a single route. Task + SubTask creation moves inside `triggerAddressMatching` in `src/lib/bulkUpload/server.ts`. The generic `POST /api/tasks` endpoint is deleted; the `GET /api/tasks` listing (admin Tasks UI) stays. - -The trade-off was between: - -- **(rejected) Keep `POST /api/tasks` for future generic use.** No other feature creates Tasks today. By the deletion test, the generic endpoint wasn't earning its keep — every line was overhead carried for hypothetical callers, while the only real caller had to perform a leaky two-call dance to satisfy a contract that didn't reflect the domain. -- **(chosen) Collapse Task creation into the BulkUpload module.** The seam matches the domain: BulkUpload owns its Task's lifecycle, including the moment of creation. One route handles the full transition into `processing`. The client surface (`useStartAddressMatching`) is one mutation, not a chain. - -A future reader will see `/api/tasks` exposing GET-only and reasonably wonder where Tasks are created. The answer is: inside whichever feature owns the Task. Today that's BulkUpload only. If a second feature ever needs to create Tasks, re-extract a generic creator at that point — extracting from two real consumers is straightforward, whereas keeping a speculative endpoint live without consumers leaves a stale-by-default seam that drifts from how Tasks are actually created. diff --git a/docs/adr/0004-bulk-upload-explicit-stage-buttons.md b/docs/adr/0004-bulk-upload-explicit-stage-buttons.md deleted file mode 100644 index de7a98c4..00000000 --- a/docs/adr/0004-bulk-upload-explicit-stage-buttons.md +++ /dev/null @@ -1,19 +0,0 @@ -# Bulk-upload pipeline advances via explicit Run Combiner / Finalise buttons - -[ADR-0002](./0002-bulk-upload-browser-driven-orchestration.md) described the bulk-upload pipeline as **browser-driven and auto-firing**: a polling loop in `OnboardingProgress.tsx` watched the Task summary and fired `POST /combine` when the Task looked done, then `POST /finalize` when the upload reached `awaiting_review`, with mutable `combineFired` / `finalizeFired` / `refreshed` flags gating the side effects. - -This decision keeps the pipeline browser-driven (per ADR-0002) but replaces the auto-fire behaviour with **explicit buttons** the Domna tech team clicks to advance each stage. The state machine in [ADR-0001](./0001-bulk-upload-state-machine.md) is unchanged. - -Concretely: - -- The progress screen polls a single `GET /bulk-uploads/[uploadId]/progress` snapshot (Task summary + BulkUpload row in one query). -- "Run Combiner" appears when the Task reaches terminal-non-failed and the Combiner hasn't been triggered yet. -- "Finalise" appears when the BulkUpload reaches `awaiting_review`. -- Polling stops once the BulkUpload reaches a terminal status; on `useFinalize` success the caller runs `router.refresh()`. - -The trade-off was between: - -- **(rejected) Keep auto-fire and translate the orchestration to react-query with `useEffect`s.** Functional, but the auto-fire UX hides where the pipeline gets stuck — which is the failure mode the Domna tech team needs to debug. CLAUDE.md's "avoid `useEffect`" rule also gets stretched: three effects, one per side-effecting transition, each watching derived eligibility flags. -- **(chosen) Explicit buttons.** The "fire once" mutable flags collapse into react-query mutation state (`isIdle` / `isPending` / `isSuccess`) — pure derivations from the cache, no `useEffect`. Each stage is observable, retryable, and reflects the current BulkUpload status directly. Tab close still leaves uploads stuck (browser-driven progression, per ADR-0002), but "stuck" is now legible: the team sees which button is pending. - -When the bulk-upload redesign lands (per ADR-0002, "out of scope"), this entire screen and its buttons go away in favour of server-driven progression. Until then, the buttons are the right level of investment: they survive the redesign as discoverable artifacts of the prior flow, and they make the current flow tractable for the team using it. diff --git a/docs/adr/0005-retire-needs-review-status.md b/docs/adr/0005-retire-needs-review-status.md deleted file mode 100644 index f12ebb67..00000000 --- a/docs/adr/0005-retire-needs-review-status.md +++ /dev/null @@ -1,14 +0,0 @@ -# Retire the `needs_review` BulkUpload status - -[ADR-0001](./0001-bulk-upload-state-machine.md) preserved `needs_review` as a terminal status (deliberate decision #1): a finalised upload with missing or duplicate UPRNs ended up there, distinct from `complete`, even though no recovery flow existed. The reasoning was that the status flag still carried a UI signal — "Imported with issues" copy on the upload detail page — that told the team manual cleanup was needed in the property table. - -This decision retires `needs_review` entirely. Finalise now always sets `complete`, regardless of whether any rows were missing a UPRN or shared one with another row. The status enum, the `markFinalized` signature, and the per-status UI copy are all simplified accordingly. - -The terminal set is now `complete` and `failed` only. The state machine in ADR-0001 otherwise stands; only the post-Finalise branching changes. - -The trade-off was between: - -- **(rejected) Keep `needs_review` for the UI distinction.** The flag was the only consumer of the missing/duplicate-UPRN counts during Finalise, and the counts themselves were already dead in the response payload. Keeping the flag meant maintaining a state-machine branch and a STATUS_CONFIG entry for a signal that — per ADR-0001 — has no follow-up action. Future readers would continue to surface "needs_review has no exit" as friction. -- **(chosen) Always `complete`.** One terminal success state. The Finalise route loses the missing/duplicate-UPRN booleans entirely; rows are inserted with whatever UPRN they have (or none), and the team handles cleanup in the property table directly. If a recovery flow ever lands, it can re-introduce a more meaningful intermediate status at that point — driven by the actual recovery UX rather than a defensive flag set today. - -Legacy `bulk_address_uploads` rows with `status = 'needs_review'` (if any exist) will fall through `STATUS_CONFIG` lookup and render as the default `ready_for_processing` card. This is acceptable because (a) the flow is internal Domna tooling per ADR-0002, (b) any such rows pre-date this change and represent uploads the team has already triaged manually, and (c) the records still link to imported properties via the BulkUpload row's relationships. No migration is run.