From 7ece33b7b66fe6b8338a5a6ea8efd66f64a5d551 Mon Sep 17 00:00:00 2001 From: Jun-te Kim Date: Wed, 6 May 2026 16:05:24 +0000 Subject: [PATCH] removed finalise code as its needed only in the final new process --- CONTEXT.md | 7 ++-- docs/adr/0005-retire-needs-review-status.md | 14 +++++++ .../bulk-uploads/[uploadId]/finalize/route.ts | 39 +++---------------- .../bulk-upload/[uploadId]/page.tsx | 10 +---- src/lib/bulkUpload/client.ts | 11 +----- src/lib/bulkUpload/server.ts | 12 ++---- src/lib/bulkUpload/types.ts | 2 - 7 files changed, 29 insertions(+), 66 deletions(-) create mode 100644 docs/adr/0005-retire-needs-review-status.md diff --git a/CONTEXT.md b/CONTEXT.md index 652d44c..4a95696 100644 --- a/CONTEXT.md +++ b/CONTEXT.md @@ -41,12 +41,11 @@ ready_for_processing → processing (Address matching triggered; Next.js writes) → combining (Combiner stage running; FastAPI writes directly) → awaiting_review (Combiner output in S3; FastAPI writes directly) - → complete (Finalise succeeded, no row issues; Next.js writes) - → needs_review (Finalise succeeded, missing or duplicate UPRNs; Next.js writes) + → complete (Finalise succeeded; Next.js writes) → failed (FastAPI reports in-flight failure — schema only, not yet wired) ``` -`complete`, `needs_review`, and `failed` are terminal. +`complete` and `failed` are terminal. Re-mapping (PATCHing `columnMapping`) is legal only in `ready_for_processing` and `mapping_complete`. Any later state rejects with 409. @@ -66,7 +65,7 @@ See [ADR-0001](./docs/adr/0001-bulk-upload-state-machine.md) for the deliberate > **Domain expert:** "The BulkUpload sits in `awaiting_review`. The frontend polls and shows a 'review and confirm' button. Nothing's been written to **Properties** yet." > > **Dev:** "And if **Finalise** runs and 30% of rows have no **UPRN**?" -> **Domain expert:** "Those still get imported as **Properties** — just without a UPRN — and the BulkUpload moves to `needs_review`. The `complete` state is only for clean runs." +> **Domain expert:** "Those still get imported as **Properties** — just without a UPRN — and the BulkUpload moves to `complete`. Manual cleanup happens later in the property table." ## Flagged ambiguities diff --git a/docs/adr/0005-retire-needs-review-status.md b/docs/adr/0005-retire-needs-review-status.md new file mode 100644 index 0000000..f12ebb6 --- /dev/null +++ b/docs/adr/0005-retire-needs-review-status.md @@ -0,0 +1,14 @@ +# 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. diff --git a/src/app/api/portfolio/[portfolioId]/bulk-uploads/[uploadId]/finalize/route.ts b/src/app/api/portfolio/[portfolioId]/bulk-uploads/[uploadId]/finalize/route.ts index b006e92..b6fd7ec 100644 --- a/src/app/api/portfolio/[portfolioId]/bulk-uploads/[uploadId]/finalize/route.ts +++ b/src/app/api/portfolio/[portfolioId]/bulk-uploads/[uploadId]/finalize/route.ts @@ -74,10 +74,7 @@ export async function POST( case "not_found": return NextResponse.json({ error: "Not found" }, { status: 404 }); case "already_finalized": - return NextResponse.json( - { alreadyComplete: true, status: guarded.status }, - { status: 200 } - ); + return new NextResponse(null, { status: 200 }); case "wrong_state": return NextResponse.json( { error: `Upload not ready to finalize (state: ${guarded.current})` }, @@ -111,24 +108,12 @@ export async function POST( const portfolioIdBig = BigInt(upload.portfolioId); - const uprnCounts = new Map(); - for (const r of rawRows) { - const v = normalize(r[UPRN_COL]); - if (isMissing(v)) continue; - uprnCounts.set(v, (uprnCounts.get(v) ?? 0) + 1); - } - - let missingUprnCount = 0; - let duplicateUprnCount = 0; - const values = rawRows.map((raw) => { const userInputtedAddress = ADDRESS_COLS.map((c) => normalize(raw[c])).filter(Boolean).join(", ") || null; const userInputtedPostcode = normalize(raw[POSTCODE_COL]) || null; const uprn = parseUprn(raw[UPRN_COL]); - if (uprn === null) missingUprnCount++; - else if ((uprnCounts.get(uprn.toString()) ?? 0) >= 2) duplicateUprnCount++; const matchedAddressRaw = normalize(raw[MATCHED_ADDRESS_COL]); const matchedAddress = isMissing(matchedAddressRaw) ? null : matchedAddressRaw; @@ -152,36 +137,24 @@ export async function POST( }; }); - let inserted = 0; try { if (values.length > 0) { - const result = await db + await db .insert(property) .values(values) .onConflictDoNothing({ target: [property.portfolioId, property.uprn], where: sql`${property.uprn} IS NOT NULL`, - }) - .returning({ id: property.id }); - inserted = result.length; + }); } - const needsReview = missingUprnCount > 0 || duplicateUprnCount > 0; - await markFinalized(uploadId, { needsReview }); - const nextStatus = needsReview ? "needs_review" : "complete"; + await markFinalized(uploadId); revalidatePath("/portfolio/[slug]", "layout"); - return NextResponse.json( - { inserted, missingUprnCount, duplicateUprnCount, status: nextStatus }, - { status: 200 } - ); + return new NextResponse(null, { status: 200 }); } catch (err) { console.error("Failed to finalize bulk upload:", err); - const detail = err instanceof Error ? err.message : String(err); - return NextResponse.json( - { error: "Failed to import properties", detail }, - { status: 500 } - ); + return NextResponse.json({ error: "Failed to import properties" }, { status: 500 }); } } diff --git a/src/app/portfolio/[slug]/(portfolio)/bulk-upload/[uploadId]/page.tsx b/src/app/portfolio/[slug]/(portfolio)/bulk-upload/[uploadId]/page.tsx index f8f1a35..5509be2 100644 --- a/src/app/portfolio/[slug]/(portfolio)/bulk-upload/[uploadId]/page.tsx +++ b/src/app/portfolio/[slug]/(portfolio)/bulk-upload/[uploadId]/page.tsx @@ -77,14 +77,6 @@ const STATUS_CONFIG = { body: "All addresses have been imported into your portfolio.", cta: false, }, - needs_review: { - icon: ExclamationCircleIcon, - iconBg: "bg-amber-50", - iconColor: "text-amber-500", - title: "Imported with issues", - body: "Some addresses didn't match a UPRN or matched the same UPRN as another row. Open the properties list to fix them manually.", - cta: false, - }, failed: { icon: ExclamationCircleIcon, iconBg: "bg-red-50", @@ -188,7 +180,7 @@ export default async function BulkUploadDetailPage(props: { /> )} - {(statusKey === "needs_review" || statusKey === "complete") && ( + {statusKey === "complete" && ( ({ + return useMutation({ mutationFn: async () => { const res = await fetch( `/api/portfolio/${portfolioId}/bulk-uploads/${uploadId}/finalize`, { method: "POST" }, ); if (!res.ok) throw await parseError(res, "Finalize failed."); - return res.json(); }, onSuccess: () => { queryClient.invalidateQueries({ queryKey: bulkUploadKeys.progress(uploadId) }); diff --git a/src/lib/bulkUpload/server.ts b/src/lib/bulkUpload/server.ts index a4ce2d5..a5785c3 100644 --- a/src/lib/bulkUpload/server.ts +++ b/src/lib/bulkUpload/server.ts @@ -250,7 +250,7 @@ export async function requestCombineRetrigger(args: { export type LoadForFinalizeOutcome = | { kind: "ready"; upload: BulkUpload } - | { kind: "already_finalized"; status: "complete" | "needs_review" } + | { kind: "already_finalized" } | { kind: "not_found" } | { kind: "not_yet_combined" } | { kind: "wrong_state"; current: string }; @@ -258,20 +258,16 @@ export type LoadForFinalizeOutcome = export async function loadForFinalize(uploadId: string): Promise { const upload = await loadById(uploadId); if (!upload) return { kind: "not_found" }; - if (upload.status === "complete" || upload.status === "needs_review") - return { kind: "already_finalized", status: upload.status }; + if (upload.status === "complete") return { kind: "already_finalized" }; if (upload.status !== "awaiting_review") return { kind: "wrong_state", current: upload.status }; if (!upload.combinedOutputS3Uri) return { kind: "not_yet_combined" }; return { kind: "ready", upload }; } -export async function markFinalized( - uploadId: string, - args: { needsReview: boolean }, -): Promise { +export async function markFinalized(uploadId: string): Promise { await db .update(bulkAddressUploads) - .set({ status: args.needsReview ? "needs_review" : "complete" }) + .set({ status: "complete" }) .where(eq(bulkAddressUploads.id, uploadId)); } diff --git a/src/lib/bulkUpload/types.ts b/src/lib/bulkUpload/types.ts index 1559f52..c2b1cb6 100644 --- a/src/lib/bulkUpload/types.ts +++ b/src/lib/bulkUpload/types.ts @@ -7,7 +7,6 @@ export const BULK_UPLOAD_STATUSES = [ "combining", "awaiting_review", "complete", - "needs_review", "failed", ] as const; @@ -35,7 +34,6 @@ export type ProgressView = { const TERMINAL_UPLOAD_STATUSES: ReadonlySet = new Set([ "complete", - "needs_review", "failed", ]);