From 8d48f774bb33756d9cfd24ed8503b68498c4e787 Mon Sep 17 00:00:00 2001 From: ang3l12 Date: Tue, 28 Jul 2026 11:15:16 -0600 Subject: [PATCH] Use NCR business number in URLs (/ncrs/NCR-2026-0021) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Detail routes, queue navigation, and notification email links now use the business identifier instead of the database id — friendlier for end users quoting NCR numbers. The API resolves both forms (case-insensitive number, or legacy numeric id) so existing bookmarks and email links keep working. Adds regression tests incl. a guard that /ncrs/export.csv isn't shadowed by the path parameter. Co-Authored-By: Claude Fable 5 --- backend/app/routers/ncrs.py | 75 ++++++++++++++------------ backend/app/services/notifications.py | 2 +- backend/tests/test_ncr_ref.py | 44 +++++++++++++++ frontend/src/api/hooks.ts | 14 +++-- frontend/src/components/QueueTable.tsx | 4 +- frontend/src/pages/NcrDetailPage.tsx | 5 +- frontend/src/pages/NewNcrPage.tsx | 2 +- 7 files changed, 102 insertions(+), 44 deletions(-) create mode 100644 backend/tests/test_ncr_ref.py diff --git a/backend/app/routers/ncrs.py b/backend/app/routers/ncrs.py index b05c09b..caad419 100644 --- a/backend/app/routers/ncrs.py +++ b/backend/app/routers/ncrs.py @@ -71,8 +71,17 @@ _STAGE_ORDER = [ # ── helpers ────────────────────────────────────────────────────────────────── -async def _get_ncr(db: AsyncSession, ncr_id: int) -> Ncr: - ncr = await db.get(Ncr, ncr_id) +async def _get_ncr(db: AsyncSession, ref: str | int) -> Ncr: + """Resolve an NCR by its business identifier (e.g. NCR-2026-0021, + case-insensitive) or — for backward compatibility with older links — + by numeric database id.""" + ref = str(ref).strip() + if ref.isdigit(): + ncr = await db.get(Ncr, int(ref)) + else: + ncr = ( + await db.execute(select(Ncr).where(Ncr.ncr_number == ref.upper())) + ).scalar_one_or_none() if ncr is None: raise HTTPException(status_code=404, detail="NCR not found.") return ncr @@ -453,27 +462,27 @@ async def export_ncrs_csv( ) -@router.get("/ncrs/{ncr_id}", response_model=NcrDetailOut) +@router.get("/ncrs/{ncr_ref}", response_model=NcrDetailOut) async def get_ncr( - ncr_id: int, + ncr_ref: str, current: CurrentUser = Depends(get_current_user), db: AsyncSession = Depends(get_db), ) -> NcrDetailOut: - ncr = await _get_ncr(db, ncr_id) + ncr = await _get_ncr(db, ncr_ref) return _detail(ncr, current) # ── stage actions ──────────────────────────────────────────────────────────── -@router.post("/ncrs/{ncr_id}/initial-disposition", response_model=NcrMutationOut) +@router.post("/ncrs/{ncr_ref}/initial-disposition", response_model=NcrMutationOut) async def initial_disposition( - ncr_id: int, + ncr_ref: str, payload: InitialDispositionIn, current: CurrentUser = Depends(require_roles(Role.DISPOSITION_AUTHORITY)), db: AsyncSession = Depends(get_db), ) -> NcrMutationOut: """Stage 2 — Initial Disposition, performed on a New Request. Routes to Secondary Disposition (when secondary review is needed) or Operations.""" - ncr = await _get_ncr(db, ncr_id) + ncr = await _get_ncr(db, ncr_ref) _ensure_stage(ncr, Stage.NEW_REQUEST) assignees: list[User] = [] @@ -536,16 +545,16 @@ async def initial_disposition( return NcrMutationOut(ncr=_detail(ncr, current), warnings=warnings) -@router.post("/ncrs/{ncr_id}/secondary-disposition", response_model=NcrMutationOut) +@router.post("/ncrs/{ncr_ref}/secondary-disposition", response_model=NcrMutationOut) async def secondary_disposition( - ncr_id: int, + ncr_ref: str, payload: SecondaryDispositionIn, current: CurrentUser = Depends(get_current_user), db: AsyncSession = Depends(get_db), ) -> NcrMutationOut: """Stage 3 — Secondary Disposition. Only the assigned secondary authorities (or an Admin) may update or release to Operations.""" - ncr = await _get_ncr(db, ncr_id) + ncr = await _get_ncr(db, ncr_ref) _ensure_stage(ncr, Stage.SECONDARY_DISPOSITION) if not (current.is_admin or _is_secondary_assignee(ncr, current)): raise HTTPException( @@ -577,14 +586,14 @@ async def secondary_disposition( return NcrMutationOut(ncr=_detail(ncr, current), warnings=warnings) -@router.post("/ncrs/{ncr_id}/operations-complete", response_model=NcrMutationOut) +@router.post("/ncrs/{ncr_ref}/operations-complete", response_model=NcrMutationOut) async def operations_complete( - ncr_id: int, + ncr_ref: str, current: CurrentUser = Depends(require_roles(Role.OPERATIONS)), db: AsyncSession = Depends(get_db), ) -> NcrMutationOut: """Stage 4 — Operations marks rework complete; NCR moves to QC Inspection.""" - ncr = await _get_ncr(db, ncr_id) + ncr = await _get_ncr(db, ncr_ref) _ensure_stage(ncr, Stage.OPERATIONS) apply_field_updates( @@ -612,16 +621,16 @@ async def operations_complete( return NcrMutationOut(ncr=_detail(ncr, current), warnings=warnings) -@router.post("/ncrs/{ncr_id}/inspection", response_model=NcrMutationOut) +@router.post("/ncrs/{ncr_ref}/inspection", response_model=NcrMutationOut) async def inspection( - ncr_id: int, + ncr_ref: str, payload: InspectionIn, current: CurrentUser = Depends(require_roles(Role.QC_INSPECTOR)), db: AsyncSession = Depends(get_db), ) -> NcrMutationOut: """Stage 5 — QC Inspection. QC can save repeatedly; checking QC Closed advances the NCR to Costing.""" - ncr = await _get_ncr(db, ncr_id) + ncr = await _get_ncr(db, ncr_ref) _ensure_stage(ncr, Stage.QC_INSPECTION) updates = payload.model_dump(exclude_unset=True, exclude={"qc_closed"}) @@ -649,15 +658,15 @@ async def inspection( return NcrMutationOut(ncr=_detail(ncr, current), warnings=warnings) -@router.post("/ncrs/{ncr_id}/costing", response_model=NcrMutationOut) +@router.post("/ncrs/{ncr_ref}/costing", response_model=NcrMutationOut) async def costing( - ncr_id: int, + ncr_ref: str, payload: CostingIn, current: CurrentUser = Depends(require_roles(Role.COSTING)), db: AsyncSession = Depends(get_db), ) -> NcrMutationOut: """Stage 6 — Costing. Saving costs completes the workflow and closes the NCR.""" - ncr = await _get_ncr(db, ncr_id) + ncr = await _get_ncr(db, ncr_ref) _ensure_stage(ncr, Stage.COSTING) now = utcnow() @@ -691,16 +700,16 @@ async def costing( return NcrMutationOut(ncr=_detail(ncr, current), warnings=warnings) -@router.post("/ncrs/{ncr_id}/reopen", response_model=NcrMutationOut) +@router.post("/ncrs/{ncr_ref}/reopen", response_model=NcrMutationOut) async def reopen( - ncr_id: int, + ncr_ref: str, payload: ReopenIn, current: CurrentUser = Depends(require_roles(Role.ADMIN)), db: AsyncSession = Depends(get_db), ) -> NcrMutationOut: """Admin-only: reopen a closed NCR into a chosen prior stage. The reason is required and recorded in the audit trail and transition history.""" - ncr = await _get_ncr(db, ncr_id) + ncr = await _get_ncr(db, ncr_ref) if ncr.stage != Stage.CLOSED.value: raise HTTPException(status_code=409, detail="Only closed NCRs can be reopened.") if payload.to_stage == Stage.SECONDARY_DISPOSITION and not ncr.secondary_assignee_rows: @@ -756,16 +765,16 @@ def _do_transition( # ── attachments ────────────────────────────────────────────────────────────── -@router.post("/ncrs/{ncr_id}/attachments", response_model=list[AttachmentOut], status_code=201) +@router.post("/ncrs/{ncr_ref}/attachments", response_model=list[AttachmentOut], status_code=201) async def upload_attachments( - ncr_id: int, + ncr_ref: str, files: list[UploadFile], current: CurrentUser = Depends(get_current_user), db: AsyncSession = Depends(get_db), ) -> list[AttachmentOut]: """Photo/file attachments (multiple per request; camera capture on tablets posts here too). Blocked once the NCR is closed.""" - ncr = await _get_ncr(db, ncr_id) + ncr = await _get_ncr(db, ncr_ref) if ncr.stage == Stage.CLOSED.value: raise HTTPException( status_code=409, detail="This NCR is closed; attachments are locked." @@ -818,21 +827,21 @@ async def download_attachment( # ── audit history ──────────────────────────────────────────────────────────── -@router.get("/ncrs/{ncr_id}/audit", response_model=AuditListOut) +@router.get("/ncrs/{ncr_ref}/audit", response_model=AuditListOut) async def ncr_audit( - ncr_id: int, + ncr_ref: str, current: CurrentUser = Depends(require_roles(Role.QC_INSPECTOR)), db: AsyncSession = Depends(get_db), ) -> AuditListOut: """Audit History tab — Admin and QC roles.""" from app.models import AuditLog - await _get_ncr(db, ncr_id) + ncr = await _get_ncr(db, ncr_ref) rows = ( ( await db.execute( select(AuditLog) - .where(AuditLog.ncr_id == ncr_id) + .where(AuditLog.ncr_id == ncr.id) .order_by(AuditLog.created_at.desc(), AuditLog.id.desc()) ) ) @@ -845,9 +854,9 @@ async def ncr_audit( # ── printable PDF ──────────────────────────────────────────────────────────── -@router.get("/ncrs/{ncr_id}/pdf") +@router.get("/ncrs/{ncr_ref}/pdf") async def ncr_pdf( - ncr_id: int, + ncr_ref: str, current: CurrentUser = Depends(get_current_user), db: AsyncSession = Depends(get_db), ) -> Response: @@ -855,7 +864,7 @@ async def ncr_pdf( travelers and audits.""" from app.services.pdf import render_ncr_pdf - ncr = await _get_ncr(db, ncr_id) + ncr = await _get_ncr(db, ncr_ref) pdf_bytes = await render_ncr_pdf(ncr) return Response( content=pdf_bytes, diff --git a/backend/app/services/notifications.py b/backend/app/services/notifications.py index 42378d6..a1c24c0 100644 --- a/backend/app/services/notifications.py +++ b/backend/app/services/notifications.py @@ -85,7 +85,7 @@ async def _recipients(db: AsyncSession, ncr: Ncr, event: NotifyEvent) -> list[st def _build_body(ncr: Ncr, event: NotifyEvent, summary: str) -> str: e = html.escape - link = f"{get_settings().app_base_url}/ncrs/{ncr.id}" + link = f"{get_settings().app_base_url}/ncrs/{ncr.ncr_number}" rows = [ ("NCR Number", ncr.ncr_number), ("Job Number", ncr.job_number), diff --git a/backend/tests/test_ncr_ref.py b/backend/tests/test_ncr_ref.py new file mode 100644 index 0000000..92b3388 --- /dev/null +++ b/backend/tests/test_ncr_ref.py @@ -0,0 +1,44 @@ +"""NCR lookup by business identifier (URL paths) with legacy-id fallback.""" +from .util import create_ncr, do_initial_disposition, hdr + + +async def test_lookup_by_business_number(client, team): + ncr = await create_ncr(client, team) + number = ncr["ncr_number"] + + r = await client.get(f"/api/ncrs/{number}", headers=hdr(team["requester"])) + assert r.status_code == 200 + assert r.json()["id"] == ncr["id"] + + # case-insensitive + r = await client.get(f"/api/ncrs/{number.lower()}", headers=hdr(team["requester"])) + assert r.status_code == 200 + + +async def test_stage_actions_work_by_business_number(client, team): + ncr = await create_ncr(client, team) + r = await do_initial_disposition(client, team, ncr["ncr_number"]) + assert r.status_code == 200, r.text + assert r.json()["ncr"]["stage"] == "operations" + + +async def test_legacy_numeric_id_still_resolves(client, team): + """Old email links used /ncrs/; they must keep working.""" + ncr = await create_ncr(client, team) + r = await client.get(f"/api/ncrs/{ncr['id']}", headers=hdr(team["requester"])) + assert r.status_code == 200 + assert r.json()["ncr_number"] == ncr["ncr_number"] + + +async def test_unknown_refs_are_404(client, team): + for ref in ("NCR-1999-9999", "999999", "not-a-number-at-all"): + r = await client.get(f"/api/ncrs/{ref}", headers=hdr(team["requester"])) + assert r.status_code == 404, ref + + +async def test_csv_export_route_not_shadowed(client, team): + """/ncrs/export.csv must match the literal route, not the {ncr_ref} param.""" + await create_ncr(client, team) + r = await client.get("/api/ncrs/export.csv?queue=all", headers=hdr(team["requester"])) + assert r.status_code == 200 + assert r.headers["content-type"].startswith("text/csv") diff --git a/frontend/src/api/hooks.ts b/frontend/src/api/hooks.ts index d471f12..fb91c0f 100644 --- a/frontend/src/api/hooks.ts +++ b/frontend/src/api/hooks.ts @@ -54,11 +54,13 @@ export function useQueue(queue: string, filters: QueueFilters, page: number, pag }); } -export function useNcr(id: number | undefined) { +/** `ref` is the NCR business number (NCR-2026-0021) or, for older links, + * the numeric database id — the API resolves both. */ +export function useNcr(ref: string | undefined) { return useQuery({ - queryKey: ["ncr", id], - queryFn: () => api(`/api/ncrs/${id}`), - enabled: id !== undefined, + queryKey: ["ncr", ref], + queryFn: () => api(`/api/ncrs/${encodeURIComponent(ref!)}`), + enabled: !!ref, }); } @@ -97,7 +99,9 @@ export function useNcrMutation( return useMutation({ mutationFn, onSuccess: (data) => { - qc.setQueryData(["ncr", data.ncr.id], data.ncr); + // The detail page may be keyed by business number or legacy id. + qc.setQueryData(["ncr", data.ncr.ncr_number], data.ncr); + qc.setQueryData(["ncr", String(data.ncr.id)], data.ncr); qc.invalidateQueries({ queryKey: ["ncrs"] }); qc.invalidateQueries({ queryKey: ["ncr-audit", data.ncr.id] }); if (data.warnings.length && onWarnings) onWarnings(data.warnings); diff --git a/frontend/src/components/QueueTable.tsx b/frontend/src/components/QueueTable.tsx index cd71f0c..bb4b072 100644 --- a/frontend/src/components/QueueTable.tsx +++ b/frontend/src/components/QueueTable.tsx @@ -72,7 +72,7 @@ export function QueueTable({ items, total, page, pageSize, onPageChange, loading {items.map((n) => ( - navigate(`/ncrs/${n.id}`)}> + navigate(`/ncrs/${n.ncr_number}`)}> navigate(`/ncrs/${n.id}`)} + onClick={() => navigate(`/ncrs/${n.ncr_number}`)} > {n.ncr_number} {n.job_number} diff --git a/frontend/src/pages/NcrDetailPage.tsx b/frontend/src/pages/NcrDetailPage.tsx index 773e241..a5d0c22 100644 --- a/frontend/src/pages/NcrDetailPage.tsx +++ b/frontend/src/pages/NcrDetailPage.tsx @@ -325,9 +325,10 @@ function DetailBody({ ncr }: { ncr: NcrDetail }) { } export function NcrDetailPage() { + // URL carries the NCR business number (e.g. /ncrs/NCR-2026-0021); numeric + // ids from older links still resolve server-side. const { id } = useParams(); - const ncrId = Number(id); - const ncrQuery = useNcr(Number.isFinite(ncrId) ? ncrId : undefined); + const ncrQuery = useNcr(id); const me = useMe(); const { toast } = useToast(); const [tab, setTab] = useState(0); diff --git a/frontend/src/pages/NewNcrPage.tsx b/frontend/src/pages/NewNcrPage.tsx index 38bc3b6..0c299c8 100644 --- a/frontend/src/pages/NewNcrPage.tsx +++ b/frontend/src/pages/NewNcrPage.tsx @@ -98,7 +98,7 @@ export function NewNcrPage() { -