Use NCR business number in URLs (/ncrs/NCR-2026-0021)

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 <noreply@anthropic.com>
This commit is contained in:
ang3l12
2026-07-28 11:15:16 -06:00
parent 12383b2225
commit 8d48f774bb
7 changed files with 102 additions and 44 deletions

View File

@@ -71,8 +71,17 @@ _STAGE_ORDER = [
# ── helpers ────────────────────────────────────────────────────────────────── # ── helpers ──────────────────────────────────────────────────────────────────
async def _get_ncr(db: AsyncSession, ncr_id: int) -> Ncr: async def _get_ncr(db: AsyncSession, ref: str | int) -> Ncr:
ncr = await db.get(Ncr, ncr_id) """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: if ncr is None:
raise HTTPException(status_code=404, detail="NCR not found.") raise HTTPException(status_code=404, detail="NCR not found.")
return ncr 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( async def get_ncr(
ncr_id: int, ncr_ref: str,
current: CurrentUser = Depends(get_current_user), current: CurrentUser = Depends(get_current_user),
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
) -> NcrDetailOut: ) -> NcrDetailOut:
ncr = await _get_ncr(db, ncr_id) ncr = await _get_ncr(db, ncr_ref)
return _detail(ncr, current) return _detail(ncr, current)
# ── stage actions ──────────────────────────────────────────────────────────── # ── 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( async def initial_disposition(
ncr_id: int, ncr_ref: str,
payload: InitialDispositionIn, payload: InitialDispositionIn,
current: CurrentUser = Depends(require_roles(Role.DISPOSITION_AUTHORITY)), current: CurrentUser = Depends(require_roles(Role.DISPOSITION_AUTHORITY)),
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
) -> NcrMutationOut: ) -> NcrMutationOut:
"""Stage 2 — Initial Disposition, performed on a New Request. Routes to """Stage 2 — Initial Disposition, performed on a New Request. Routes to
Secondary Disposition (when secondary review is needed) or Operations.""" 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) _ensure_stage(ncr, Stage.NEW_REQUEST)
assignees: list[User] = [] assignees: list[User] = []
@@ -536,16 +545,16 @@ async def initial_disposition(
return NcrMutationOut(ncr=_detail(ncr, current), warnings=warnings) 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( async def secondary_disposition(
ncr_id: int, ncr_ref: str,
payload: SecondaryDispositionIn, payload: SecondaryDispositionIn,
current: CurrentUser = Depends(get_current_user), current: CurrentUser = Depends(get_current_user),
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
) -> NcrMutationOut: ) -> NcrMutationOut:
"""Stage 3 — Secondary Disposition. Only the assigned secondary """Stage 3 — Secondary Disposition. Only the assigned secondary
authorities (or an Admin) may update or release to Operations.""" 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) _ensure_stage(ncr, Stage.SECONDARY_DISPOSITION)
if not (current.is_admin or _is_secondary_assignee(ncr, current)): if not (current.is_admin or _is_secondary_assignee(ncr, current)):
raise HTTPException( raise HTTPException(
@@ -577,14 +586,14 @@ async def secondary_disposition(
return NcrMutationOut(ncr=_detail(ncr, current), warnings=warnings) 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( async def operations_complete(
ncr_id: int, ncr_ref: str,
current: CurrentUser = Depends(require_roles(Role.OPERATIONS)), current: CurrentUser = Depends(require_roles(Role.OPERATIONS)),
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
) -> NcrMutationOut: ) -> NcrMutationOut:
"""Stage 4 — Operations marks rework complete; NCR moves to QC Inspection.""" """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) _ensure_stage(ncr, Stage.OPERATIONS)
apply_field_updates( apply_field_updates(
@@ -612,16 +621,16 @@ async def operations_complete(
return NcrMutationOut(ncr=_detail(ncr, current), warnings=warnings) 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( async def inspection(
ncr_id: int, ncr_ref: str,
payload: InspectionIn, payload: InspectionIn,
current: CurrentUser = Depends(require_roles(Role.QC_INSPECTOR)), current: CurrentUser = Depends(require_roles(Role.QC_INSPECTOR)),
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
) -> NcrMutationOut: ) -> NcrMutationOut:
"""Stage 5 — QC Inspection. QC can save repeatedly; checking QC Closed """Stage 5 — QC Inspection. QC can save repeatedly; checking QC Closed
advances the NCR to Costing.""" 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) _ensure_stage(ncr, Stage.QC_INSPECTION)
updates = payload.model_dump(exclude_unset=True, exclude={"qc_closed"}) 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) 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( async def costing(
ncr_id: int, ncr_ref: str,
payload: CostingIn, payload: CostingIn,
current: CurrentUser = Depends(require_roles(Role.COSTING)), current: CurrentUser = Depends(require_roles(Role.COSTING)),
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
) -> NcrMutationOut: ) -> NcrMutationOut:
"""Stage 6 — Costing. Saving costs completes the workflow and closes the NCR.""" """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) _ensure_stage(ncr, Stage.COSTING)
now = utcnow() now = utcnow()
@@ -691,16 +700,16 @@ async def costing(
return NcrMutationOut(ncr=_detail(ncr, current), warnings=warnings) 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( async def reopen(
ncr_id: int, ncr_ref: str,
payload: ReopenIn, payload: ReopenIn,
current: CurrentUser = Depends(require_roles(Role.ADMIN)), current: CurrentUser = Depends(require_roles(Role.ADMIN)),
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
) -> NcrMutationOut: ) -> NcrMutationOut:
"""Admin-only: reopen a closed NCR into a chosen prior stage. The reason """Admin-only: reopen a closed NCR into a chosen prior stage. The reason
is required and recorded in the audit trail and transition history.""" 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: if ncr.stage != Stage.CLOSED.value:
raise HTTPException(status_code=409, detail="Only closed NCRs can be reopened.") 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: if payload.to_stage == Stage.SECONDARY_DISPOSITION and not ncr.secondary_assignee_rows:
@@ -756,16 +765,16 @@ def _do_transition(
# ── attachments ────────────────────────────────────────────────────────────── # ── 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( async def upload_attachments(
ncr_id: int, ncr_ref: str,
files: list[UploadFile], files: list[UploadFile],
current: CurrentUser = Depends(get_current_user), current: CurrentUser = Depends(get_current_user),
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
) -> list[AttachmentOut]: ) -> list[AttachmentOut]:
"""Photo/file attachments (multiple per request; camera capture on """Photo/file attachments (multiple per request; camera capture on
tablets posts here too). Blocked once the NCR is closed.""" 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: if ncr.stage == Stage.CLOSED.value:
raise HTTPException( raise HTTPException(
status_code=409, detail="This NCR is closed; attachments are locked." status_code=409, detail="This NCR is closed; attachments are locked."
@@ -818,21 +827,21 @@ async def download_attachment(
# ── audit history ──────────────────────────────────────────────────────────── # ── audit history ────────────────────────────────────────────────────────────
@router.get("/ncrs/{ncr_id}/audit", response_model=AuditListOut) @router.get("/ncrs/{ncr_ref}/audit", response_model=AuditListOut)
async def ncr_audit( async def ncr_audit(
ncr_id: int, ncr_ref: str,
current: CurrentUser = Depends(require_roles(Role.QC_INSPECTOR)), current: CurrentUser = Depends(require_roles(Role.QC_INSPECTOR)),
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
) -> AuditListOut: ) -> AuditListOut:
"""Audit History tab — Admin and QC roles.""" """Audit History tab — Admin and QC roles."""
from app.models import AuditLog from app.models import AuditLog
await _get_ncr(db, ncr_id) ncr = await _get_ncr(db, ncr_ref)
rows = ( rows = (
( (
await db.execute( await db.execute(
select(AuditLog) select(AuditLog)
.where(AuditLog.ncr_id == ncr_id) .where(AuditLog.ncr_id == ncr.id)
.order_by(AuditLog.created_at.desc(), AuditLog.id.desc()) .order_by(AuditLog.created_at.desc(), AuditLog.id.desc())
) )
) )
@@ -845,9 +854,9 @@ async def ncr_audit(
# ── printable PDF ──────────────────────────────────────────────────────────── # ── printable PDF ────────────────────────────────────────────────────────────
@router.get("/ncrs/{ncr_id}/pdf") @router.get("/ncrs/{ncr_ref}/pdf")
async def ncr_pdf( async def ncr_pdf(
ncr_id: int, ncr_ref: str,
current: CurrentUser = Depends(get_current_user), current: CurrentUser = Depends(get_current_user),
db: AsyncSession = Depends(get_db), db: AsyncSession = Depends(get_db),
) -> Response: ) -> Response:
@@ -855,7 +864,7 @@ async def ncr_pdf(
travelers and audits.""" travelers and audits."""
from app.services.pdf import render_ncr_pdf 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) pdf_bytes = await render_ncr_pdf(ncr)
return Response( return Response(
content=pdf_bytes, content=pdf_bytes,

View File

@@ -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: def _build_body(ncr: Ncr, event: NotifyEvent, summary: str) -> str:
e = html.escape 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 = [ rows = [
("NCR Number", ncr.ncr_number), ("NCR Number", ncr.ncr_number),
("Job Number", ncr.job_number), ("Job Number", ncr.job_number),

View File

@@ -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/<database id>; 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")

View File

@@ -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({ return useQuery({
queryKey: ["ncr", id], queryKey: ["ncr", ref],
queryFn: () => api<NcrDetail>(`/api/ncrs/${id}`), queryFn: () => api<NcrDetail>(`/api/ncrs/${encodeURIComponent(ref!)}`),
enabled: id !== undefined, enabled: !!ref,
}); });
} }
@@ -97,7 +99,9 @@ export function useNcrMutation<TVars>(
return useMutation({ return useMutation({
mutationFn, mutationFn,
onSuccess: (data) => { 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: ["ncrs"] });
qc.invalidateQueries({ queryKey: ["ncr-audit", data.ncr.id] }); qc.invalidateQueries({ queryKey: ["ncr-audit", data.ncr.id] });
if (data.warnings.length && onWarnings) onWarnings(data.warnings); if (data.warnings.length && onWarnings) onWarnings(data.warnings);

View File

@@ -72,7 +72,7 @@ export function QueueTable({ items, total, page, pageSize, onPageChange, loading
<Stack spacing={1}> <Stack spacing={1}>
{items.map((n) => ( {items.map((n) => (
<Card key={n.id} variant="outlined"> <Card key={n.id} variant="outlined">
<CardActionArea onClick={() => navigate(`/ncrs/${n.id}`)}> <CardActionArea onClick={() => navigate(`/ncrs/${n.ncr_number}`)}>
<CardContent sx={{ py: 1.5 }}> <CardContent sx={{ py: 1.5 }}>
<Stack <Stack
direction="row" direction="row"
@@ -121,7 +121,7 @@ export function QueueTable({ items, total, page, pageSize, onPageChange, loading
key={n.id} key={n.id}
hover hover
sx={{ cursor: "pointer" }} sx={{ cursor: "pointer" }}
onClick={() => navigate(`/ncrs/${n.id}`)} onClick={() => navigate(`/ncrs/${n.ncr_number}`)}
> >
<TableCell sx={{ fontWeight: 700 }}>{n.ncr_number}</TableCell> <TableCell sx={{ fontWeight: 700 }}>{n.ncr_number}</TableCell>
<TableCell>{n.job_number}</TableCell> <TableCell>{n.job_number}</TableCell>

View File

@@ -325,9 +325,10 @@ function DetailBody({ ncr }: { ncr: NcrDetail }) {
} }
export function NcrDetailPage() { 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 { id } = useParams();
const ncrId = Number(id); const ncrQuery = useNcr(id);
const ncrQuery = useNcr(Number.isFinite(ncrId) ? ncrId : undefined);
const me = useMe(); const me = useMe();
const { toast } = useToast(); const { toast } = useToast();
const [tab, setTab] = useState(0); const [tab, setTab] = useState(0);

View File

@@ -98,7 +98,7 @@ export function NewNcrPage() {
</Box> </Box>
<Stack direction="row" spacing={1} justifyContent="center" sx={{ mt: 3 }}> <Stack direction="row" spacing={1} justifyContent="center" sx={{ mt: 3 }}>
<Button variant="contained" onClick={() => navigate(`/ncrs/${created.id}`)}> <Button variant="contained" onClick={() => navigate(`/ncrs/${created.ncr_number}`)}>
View NCR View NCR
</Button> </Button>
<Button <Button