diff --git a/PROJECT_STATE.md b/PROJECT_STATE.md index 229c655..f1f5d3e 100644 --- a/PROJECT_STATE.md +++ b/PROJECT_STATE.md @@ -2,7 +2,7 @@ ## Current milestone -M2 — complete. Starting M3 next. +M3 — complete. Starting M4 next. ## Locked decisions @@ -65,10 +65,23 @@ M2 — complete. Starting M3 next. - `docker compose up -d --build` (all services) then `docker compose exec api python -m app.cli seed --reset`, then a full browser run of the S1 demo scenario against `BK-DEMO-RETURN`/`MO-024`: submitted 53000 km (below canonical 54820) → result panel showed `INSP-0076`, `resulting_vehicle_status: maintenance` (correctly derived, since canonical 54820 ≥ `next_service_km` 40000), `DQ-RET-0076` created, automation event queued with a real UUID, "no upcoming booking" risk; vehicle detail page confirmed odometer unchanged at 54,820 km and a "Needs attention" badge. - Both real defects listed above (form disappearing before showing its result; `event_id` reading as `None`) were **found via the browser run, not by pytest** — the test suite asserted on API response shape/values, not on what the UI actually rendered after a status transition. Worth remembering for M3+: UI state-after-mutation bugs need a browser check, not just API tests. +### M3 — Data Quality Workbench +- `app/services/data_quality.py`: `run_scan()` implements all five rules and is called automatically at the end of `seed_loader.reset_and_seed()` (after `db.commit()` of the base seed), plus exposed as `POST /api/v1/data-quality/scan` (Operations Manager only). Idempotency is simplified from the doc's literal `(rule_type, entity_type, entity_id, evidence fingerprint)` to just `(rule_type, entity_type, entity_id)` while an issue is open — see rationale below. +- Router `app/api/routers/data_quality.py`: `GET /issues` (filters status/rule_type/severity), `GET /issues/{ref}` (adds `entity_snapshot`/`related_snapshots` for the UI), `POST /issues/{ref}/defer`, `/reject`, `/merge-customers` (Operations Manager only — enforced via `require_operations_manager`), `POST /scan`. +- **Real bug found and fixed during this milestone, before any browser check**: the first cut of DQ-03 (odometer regression) compared every historical *returned* booking's `end_odometer_km` against the vehicle's *current* `odometer_km`. Since the seed generator assigns `vehicle.odometer_km` independently of booking history (see `seed/generate_seed.py`), this is true for nearly every historical booking by construction (odometer is monotonically increasing over time, so all-but-the-latest reading is "below current") — it produced 51 false-positive issues out of 50 vehicles on first run. Fixed twice: first attempt (compare only the single most-recent booking against canonical) still produced the same problem because canonical itself is disconnected from booking history in this dataset; the working fix compares each vehicle's *own returned-booking sequence* against itself (each booking's end reading vs. the immediately preceding one, chronologically) — a self-consistency check that doesn't depend on the unrelated `vehicle.odometer_km` field at all. Final deterministic seed+scan totals: 15 CSV-seeded + 11 scan-discovered = **26** open/resolved `data_quality_issues` (breakdown: 14 vehicle_status_conflict, 5 missing_required_field, 3 possible_duplicate_customer, 3 odometer_regression, 1 booking_overlap). `tests/test_seed.py`'s exact-count assertion was updated from 15 to 26 accordingly — if the scan logic changes again, update that count. +- Idempotency simplification rationale: the doc's fingerprint-based key would make the scan blind to issues it structurally can't compute a matching fingerprint for against the CSV-seeded rows (which don't carry a fingerprint field), producing duplicate issues for the same real-world problem (e.g. a second `MO-016` overlap issue next to the seeded `DQ-DEMO-OVERLAP`). Using `(rule_type, entity_type, entity_id)` alone while open is a stricter, safe simplification: it can never falsely suppress an issue for a *different* entity, and per-entity there's realistically only one meaningful open issue of a given rule type at a time for this PoC's scope. +- Merge UI intentionally does **not** use `window.confirm()` — a native dialog blocks further automation/testing and isn't screen-reader-distinguishable from page content the same way a rendered `role="alertdialog"` panel is. Built an inline two-step confirm instead (`ReturnForm`-style pattern reused). +- `AttentionItem` gained an `issue_ref` field (dashboard now links attention items straight to `/data-quality/{issue_ref}` instead of only to vehicles); dashboard attention list capped at 8 items (was unbounded, would have shown up to 26 with the richer scan). +- Commands run and verified from this checkout: + - `docker compose run --rm api pytest -q` — **35 passed** (new `tests/test_data_quality.py`: all five rule types present, scan idempotent on rerun, scan requires Operations Manager, S2/S4 issue-detail snapshots correct, defer→reject-on-closed 409, merge requires Operations Manager, merge rejects an unrelated survivor ref, full S2 merge scenario asserting rewiring + audit + replay-is-409). + - `docker compose run --rm api ruff check .` — All checks passed. + - `npm run build` — clean (had to fix two `possibly 'null'` TS errors from a closure-narrowing limitation — TS doesn't narrow `const` captured-by-closure across nested function boundaries when the value comes from an index/property expression; fixed by re-binding to explicitly-typed local consts right after the guard). + - Full browser run: Data Quality list (26 open issues, filterable) → `DQ-DEMO-DUPLICATE` two-column compare (CUS-0012 vs CUS-0178, per-field diff highlighting only where they differ) → merge with inline confirm → issue flips to `resolved` → confirmed `customer_merged` audit event with correct actor/entity/correlation → `DQ-DEMO-OVERLAP` (non-duplicate type) renders evidence JSON + defer/reject, no dead compare UI shown for a rule type it doesn't apply to. + ## Known blockers None. External service credentials may be absent; use the documented demo/degraded providers. ## Exact next action -Start M3 (Data Quality Workbench): read `docs/07-data-quality.md`. Implement the five rule scanners (possible_duplicate_customer with weighted scoring, missing_required_field, odometer_regression, booking_overlap, vehicle_status_conflict) as an explicit scan service (idempotent by `(rule_type, entity_type, entity_id, evidence fingerprint)` while open, per the doc's lifecycle section), issue defer/reject/merge-customers endpoints, a transactional customer-merge (rewires bookings, tombstones the loser, audits before/after), the Data Quality nav item + Workbench UI (two-column duplicate comparison, evidence/proposed-resolution for other types), and wire "open quality issues" into the dashboard attention section it already reads from. Seed data already has 15 `data_quality_issues` including the named `DQ-DEMO-*` scenarios (S2 duplicate CUS-0012/CUS-0178, S4 overlap MO-016) to scan/resolve against — remember M2's return workflow also creates ad-hoc `odometer_regression` issues (`DQ-RET-*`) directly, so the scan service's idempotent-fingerprint rule must not double-report those. +Start M4 (n8n automation): read `docs/11-n8n-integration.md`, `n8n/README.md`. Implement the outbox dispatcher (poll pending `outbox_events`, claim with `FOR UPDATE SKIP LOCKED`, POST to n8n's webhook with timeout + exponential backoff + small max-attempt cap, never hold a DB transaction open during the HTTP call), correct/import `n8n/mobilityops-return-processing.json` (credential + the callback route — M2's `vehicle.returned.v1` outbox payload shape is already `contracts/events.schema.json`-compliant, so the workflow should be able to consume it as-is), a narrow MobilityOps callback endpoint the workflow calls back into, an Automation nav item + UI (pending/succeeded/failed list, manual retry button, matches `docs/06-ui-ux.md`'s "Recent automation" concept already on the dashboard), and manual-retry wiring for `POST /api/v1/workflows/{event_id}/retry` per `docs/05-api-contract.md`. The seeded `workflow_runs.csv` already includes one deterministic `failed` row (`event_id 00000000-...-0020`, "Synthetic connection timeout to n8n") for the S5 demo scenario — dashboard/Automation UI must surface it and allow retry. Remember the compose `n8n` service is already up (`http://localhost:5678`, basic auth `admin`/`change-me` from `.env.example`) but the workflow itself hasn't been imported/activated yet this session. diff --git a/backend/app/api/routers/dashboard.py b/backend/app/api/routers/dashboard.py index 8f6b03c..6153e64 100644 --- a/backend/app/api/routers/dashboard.py +++ b/backend/app/api/routers/dashboard.py @@ -86,9 +86,11 @@ def get_dashboard( detail=issue.evidence_json.get("summary", ""), link_type=link_type, link_ref=link_ref, + issue_ref=issue.public_ref, ) ) attention_items.sort(key=lambda item: _SEVERITY_ORDER.get(item.severity, 3)) + attention_items = attention_items[:8] today = _today() bookings = db.scalars(select(Booking)).all() diff --git a/backend/app/api/routers/data_quality.py b/backend/app/api/routers/data_quality.py new file mode 100644 index 0000000..f4a9b69 --- /dev/null +++ b/backend/app/api/routers/data_quality.py @@ -0,0 +1,146 @@ +from __future__ import annotations + +from fastapi import APIRouter, Depends, HTTPException, Query +from sqlalchemy import select +from sqlalchemy.orm import Session + +from app.api.deps import get_current_user, get_db, require_operations_manager +from app.models.customer import Customer +from app.models.data_quality import DataQualityIssue +from app.models.vehicle import Vehicle +from app.schemas import ( + CurrentUser, + DataQualityIssueDetailOut, + DataQualityIssueOut, + MergeCustomersRequest, + MergeCustomersResult, + ScanResultOut, +) +from app.services.data_quality import defer_issue, merge_customers, reject_issue, run_scan + +router = APIRouter(prefix="/api/v1/data-quality", tags=["data-quality"]) + + +def _to_out(issue: DataQualityIssue) -> DataQualityIssueOut: + return DataQualityIssueOut( + public_ref=issue.public_ref, + rule_type=issue.rule_type, + entity_type=issue.entity_type, + entity_ref=issue.evidence_json.get("entity_ref", ""), + severity=issue.severity, + status=issue.status, + evidence=issue.evidence_json, + detected_at=issue.detected_at, + resolved_at=issue.resolved_at, + ) + + +@router.get("/issues", response_model=list[DataQualityIssueOut]) +def list_issues( + status: str | None = Query(default=None), + rule_type: str | None = Query(default=None), + severity: str | None = Query(default=None), + db: Session = Depends(get_db), + _user: CurrentUser = Depends(get_current_user), +) -> list[DataQualityIssueOut]: + stmt = select(DataQualityIssue).order_by(DataQualityIssue.detected_at.desc()) + if status: + stmt = stmt.where(DataQualityIssue.status == status) + if rule_type: + stmt = stmt.where(DataQualityIssue.rule_type == rule_type) + if severity: + stmt = stmt.where(DataQualityIssue.severity == severity) + issues = db.scalars(stmt).all() + return [_to_out(i) for i in issues] + + +def _snapshot(entity_type: str, ref: str, db: Session) -> dict | None: + if entity_type == "customer": + obj = db.scalar(select(Customer).where(Customer.public_ref == ref)) + if obj is None: + return None + return { + "public_ref": obj.public_ref, + "first_name": obj.first_name, + "last_name": obj.last_name, + "email": obj.email, + "phone": obj.phone, + "postal_code": obj.postal_code, + "city": obj.city, + } + obj = db.scalar(select(Vehicle).where(Vehicle.public_ref == ref)) + if obj is None: + return None + return { + "public_ref": obj.public_ref, + "make": obj.make, + "model": obj.model, + "location": obj.location, + "operational_status": obj.operational_status, + "odometer_km": obj.odometer_km, + } + + +@router.get("/issues/{public_ref}", response_model=DataQualityIssueDetailOut) +def get_issue( + public_ref: str, + db: Session = Depends(get_db), + _user: CurrentUser = Depends(get_current_user), +) -> DataQualityIssueDetailOut: + issue = db.scalar(select(DataQualityIssue).where(DataQualityIssue.public_ref == public_ref)) + if issue is None: + raise HTTPException(status_code=404, detail="Data quality issue not found") + base = _to_out(issue) + related_refs = issue.evidence_json.get("related_refs", []) + related_entity_type = ( + "customer" if issue.rule_type == "possible_duplicate_customer" else "vehicle" + ) + return DataQualityIssueDetailOut( + **base.model_dump(), + entity_snapshot=_snapshot(issue.entity_type, base.entity_ref, db), + related_snapshots=[ + snap + for ref in related_refs + if (snap := _snapshot(related_entity_type, ref, db)) is not None + ], + ) + + +@router.post("/issues/{public_ref}/defer", response_model=DataQualityIssueOut) +def defer( + public_ref: str, + db: Session = Depends(get_db), + user: CurrentUser = Depends(get_current_user), +) -> DataQualityIssueOut: + issue = defer_issue(db, public_ref, user) + return _to_out(issue) + + +@router.post("/issues/{public_ref}/reject", response_model=DataQualityIssueOut) +def reject( + public_ref: str, + db: Session = Depends(get_db), + user: CurrentUser = Depends(get_current_user), +) -> DataQualityIssueOut: + issue = reject_issue(db, public_ref, user) + return _to_out(issue) + + +@router.post("/issues/{public_ref}/merge-customers", response_model=MergeCustomersResult) +def merge( + public_ref: str, + body: MergeCustomersRequest, + db: Session = Depends(get_db), + user: CurrentUser = Depends(require_operations_manager), +) -> MergeCustomersResult: + result = merge_customers(db, public_ref, body.survivor_ref, body.field_overrides, user) + return MergeCustomersResult(**result) + + +@router.post("/scan", response_model=ScanResultOut) +def scan( + db: Session = Depends(get_db), + _user: CurrentUser = Depends(require_operations_manager), +) -> ScanResultOut: + result = run_scan(db) + return ScanResultOut(created=result.created) diff --git a/backend/app/main.py b/backend/app/main.py index 729708a..973ddc8 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -4,7 +4,7 @@ from fastapi import FastAPI, HTTPException, Request from fastapi.middleware.cors import CORSMiddleware from fastapi.responses import JSONResponse -from app.api.routers import audit, bookings, dashboard, demo, vehicles +from app.api.routers import audit, bookings, dashboard, data_quality, demo, vehicles from app.core.config import get_settings from app.core.errors import AppError, error_body @@ -61,3 +61,4 @@ app.include_router(dashboard.router) app.include_router(vehicles.router) app.include_router(bookings.router) app.include_router(audit.router) +app.include_router(data_quality.router) diff --git a/backend/app/schemas.py b/backend/app/schemas.py index d756aea..5a252cf 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -106,6 +106,27 @@ class DataQualityIssueOut(BaseModel): resolved_at: datetime | None = None +class DataQualityIssueDetailOut(DataQualityIssueOut): + entity_snapshot: dict[str, Any] | None = None + related_snapshots: list[dict[str, Any]] = Field(default_factory=list) + + +class MergeCustomersRequest(BaseModel): + survivor_ref: str + field_overrides: dict[str, str] | None = None + + +class MergeCustomersResult(BaseModel): + issue_ref: str + survivor_ref: str + loser_ref: str + rewired_bookings: int + + +class ScanResultOut(BaseModel): + created: dict[str, int] + + class VehicleDetailOut(VehicleOut): bookings: list[BookingSummaryOut] = Field(default_factory=list) inspections: list[InspectionOut] = Field(default_factory=list) @@ -130,6 +151,7 @@ class AttentionItem(BaseModel): detail: str link_type: Literal["vehicle", "booking", "customer"] link_ref: str + issue_ref: str | None = None class TodayItem(BaseModel): diff --git a/backend/app/seed_loader.py b/backend/app/seed_loader.py index df9a266..c30f2b1 100644 --- a/backend/app/seed_loader.py +++ b/backend/app/seed_loader.py @@ -260,7 +260,11 @@ def load_seed(db: Session) -> SeedResult: def reset_and_seed(db: Session) -> SeedResult: + from app.services.data_quality import run_scan + clear_all(db) result = load_seed(db) db.commit() + scan = run_scan(db) + result.counts["data_quality_issues"] += sum(scan.created.values()) return result diff --git a/backend/app/services/data_quality.py b/backend/app/services/data_quality.py new file mode 100644 index 0000000..24272fa --- /dev/null +++ b/backend/app/services/data_quality.py @@ -0,0 +1,426 @@ +from __future__ import annotations + +import uuid +from dataclasses import dataclass, field +from datetime import UTC, datetime +from difflib import SequenceMatcher + +from sqlalchemy import select +from sqlalchemy.orm import Session + +from app.core.errors import AppError +from app.models.booking import Booking +from app.models.customer import Customer +from app.models.data_quality import DataQualityIssue +from app.models.vehicle import Vehicle +from app.schemas import CurrentUser +from app.services.audit import record_audit_event + +REQUIRED_CUSTOMER_FIELDS = ("first_name", "last_name") +REQUIRED_VEHICLE_FIELDS = ("registration_number", "make", "model", "location") +DUPLICATE_THRESHOLD = 70 + + +@dataclass +class ScanResult: + created: dict[str, int] = field(default_factory=dict) + + def bump(self, rule_type: str) -> None: + self.created[rule_type] = self.created.get(rule_type, 0) + 1 + + +def _normalize(value: str | None) -> str: + return (value or "").strip().lower() + + +def _has_open_issue(db: Session, rule_type: str, entity_type: str, entity_id: uuid.UUID) -> bool: + return ( + db.scalar( + select(DataQualityIssue.id).where( + DataQualityIssue.rule_type == rule_type, + DataQualityIssue.entity_type == entity_type, + DataQualityIssue.entity_id == entity_id, + DataQualityIssue.status == "open", + ) + ) + is not None + ) + + +def _next_public_ref(db: Session, prefix: str) -> str: + existing = db.execute(select(DataQualityIssue.public_ref)).scalars().all() + numbers = [ + int(ref.rsplit("-", 1)[-1]) + for ref in existing + if ref.startswith(f"{prefix}-") and ref.rsplit("-", 1)[-1].isdigit() + ] + next_number = (max(numbers) + 1) if numbers else 1 + return f"{prefix}-{next_number:04d}" + + +def _open_issue( + db: Session, + scan: ScanResult, + *, + rule_type: str, + entity_type: str, + entity_id: uuid.UUID, + severity: str, + summary: str, + entity_ref: str, + related_refs: list[str], +) -> None: + if _has_open_issue(db, rule_type, entity_type, entity_id): + return + now = datetime.now(UTC) + issue = DataQualityIssue( + public_ref=_next_public_ref(db, "DQ-SCAN"), + rule_type=rule_type, + entity_type=entity_type, + entity_id=entity_id, + severity=severity, + status="open", + evidence_json={ + "summary": summary, + "entity_ref": entity_ref, + "related_refs": related_refs, + }, + proposed_action_json={}, + detected_at=now, + ) + db.add(issue) + db.flush() + scan.bump(rule_type) + + +def _scan_duplicate_customers(db: Session, scan: ScanResult) -> None: + customers = db.scalars( + select(Customer).where(Customer.merged_into_customer_id.is_(None)) + ).all() + customers.sort(key=lambda c: c.public_ref) + + for i, a in enumerate(customers): + for b in customers[i + 1 :]: + score = 0 + signals = [] + if _normalize(a.email) and _normalize(a.email) == _normalize(b.email): + score += 60 + signals.append("exact email") + if _normalize(a.phone) and _normalize(a.phone) == _normalize(b.phone): + score += 50 + signals.append("exact phone") + if _normalize(a.postal_code) and _normalize(a.postal_code) == _normalize(b.postal_code): + score += 10 + signals.append("exact postal code") + name_a = f"{_normalize(a.first_name)} {_normalize(a.last_name)}" + name_b = f"{_normalize(b.first_name)} {_normalize(b.last_name)}" + ratio = SequenceMatcher(None, name_a, name_b).ratio() + if ratio >= 0.5: + score += round(ratio * 30) + signals.append("similar name") + + if score >= DUPLICATE_THRESHOLD: + _open_issue( + db, + scan, + rule_type="possible_duplicate_customer", + entity_type="customer", + entity_id=a.id, + severity="high", + summary="; ".join(signals) + f" (score {score})", + entity_ref=a.public_ref, + related_refs=[b.public_ref], + ) + + +def _scan_missing_required_fields(db: Session, scan: ScanResult) -> None: + for customer in db.scalars( + select(Customer).where(Customer.merged_into_customer_id.is_(None)) + ).all(): + missing = [f for f in REQUIRED_CUSTOMER_FIELDS if not getattr(customer, f)] + if not customer.email and not customer.phone: + missing.append("email_or_phone") + if missing: + _open_issue( + db, + scan, + rule_type="missing_required_field", + entity_type="customer", + entity_id=customer.id, + severity="low", + summary=f"Missing: {', '.join(missing)}", + entity_ref=customer.public_ref, + related_refs=[], + ) + + for vehicle in db.scalars(select(Vehicle).where(Vehicle.active.is_(True))).all(): + missing = [f for f in REQUIRED_VEHICLE_FIELDS if not getattr(vehicle, f)] + if missing: + _open_issue( + db, + scan, + rule_type="missing_required_field", + entity_type="vehicle", + entity_id=vehicle.id, + severity="low", + summary=f"Missing: {', '.join(missing)}", + entity_ref=vehicle.public_ref, + related_refs=[], + ) + + +def _scan_booking_overlaps(db: Session, scan: ScanResult) -> None: + vehicles = db.scalars(select(Vehicle)).all() + bookings_by_vehicle: dict[uuid.UUID, list[Booking]] = {} + for booking in db.scalars( + select(Booking).where(Booking.status.in_(["reserved", "active"])) + ).all(): + bookings_by_vehicle.setdefault(booking.vehicle_id, []).append(booking) + + vehicle_by_id = {v.id: v for v in vehicles} + for vehicle_id, bookings in bookings_by_vehicle.items(): + bookings.sort(key=lambda b: b.starts_at) + for i, first in enumerate(bookings): + for second in bookings[i + 1 :]: + if second.starts_at < first.ends_at and first.starts_at < second.ends_at: + vehicle = vehicle_by_id[vehicle_id] + _open_issue( + db, + scan, + rule_type="booking_overlap", + entity_type="vehicle", + entity_id=vehicle_id, + severity="high", + summary=f"Overlapping bookings {first.public_ref} and {second.public_ref}", + entity_ref=vehicle.public_ref, + related_refs=[first.public_ref, second.public_ref], + ) + + +def _scan_vehicle_status_conflicts(db: Session, scan: ScanResult) -> None: + vehicles = db.scalars(select(Vehicle)).all() + active_by_vehicle: dict[uuid.UUID, list[Booking]] = {} + for booking in db.scalars(select(Booking).where(Booking.status == "active")).all(): + active_by_vehicle.setdefault(booking.vehicle_id, []).append(booking) + + open_high_by_vehicle = { + row[0] + for row in db.execute( + select(DataQualityIssue.entity_id).where( + DataQualityIssue.entity_type == "vehicle", + DataQualityIssue.status == "open", + DataQualityIssue.severity == "high", + ) + ).all() + } + + for vehicle in vehicles: + has_active_booking = vehicle.id in active_by_vehicle + reason = None + if vehicle.operational_status == "available" and has_active_booking: + reason = "marked available while an active booking exists" + elif vehicle.operational_status == "rented" and not has_active_booking: + reason = "marked rented without an active booking" + elif vehicle.operational_status == "available" and vehicle.id in open_high_by_vehicle: + reason = "marked available while a high-severity quality issue is open" + elif vehicle.operational_status == "maintenance" and has_active_booking: + reason = "marked maintenance while an active booking exists" + + if reason: + _open_issue( + db, + scan, + rule_type="vehicle_status_conflict", + entity_type="vehicle", + entity_id=vehicle.id, + severity="high", + summary=f"Vehicle {reason}", + entity_ref=vehicle.public_ref, + related_refs=[], + ) + + +def _scan_odometer_regressions(db: Session, scan: ScanResult) -> None: + # The seed dataset's vehicle.odometer_km is generated independently of booking + # history, so comparing every historical booking against it produces near-universal + # false positives. Instead check the booking sequence's own internal consistency: + # each vehicle's completed bookings should show a non-decreasing odometer reading. + vehicles = {v.id: v for v in db.scalars(select(Vehicle)).all()} + bookings_by_vehicle: dict[uuid.UUID, list[Booking]] = {} + for booking in db.scalars( + select(Booking).where( + Booking.status == "returned", Booking.end_odometer_km.is_not(None) + ) + ).all(): + bookings_by_vehicle.setdefault(booking.vehicle_id, []).append(booking) + + for vehicle_id, bookings in bookings_by_vehicle.items(): + bookings.sort(key=lambda b: b.ends_at) + for earlier, later in zip(bookings, bookings[1:], strict=False): + if later.end_odometer_km < earlier.end_odometer_km: + vehicle = vehicles[vehicle_id] + _open_issue( + db, + scan, + rule_type="odometer_regression", + entity_type="vehicle", + entity_id=vehicle_id, + severity="medium", + summary=( + f"Booking {later.public_ref} recorded {later.end_odometer_km} km, " + f"below the {earlier.end_odometer_km} km recorded by earlier " + f"booking {earlier.public_ref}." + ), + entity_ref=vehicle.public_ref, + related_refs=[earlier.public_ref, later.public_ref], + ) + break + + +def run_scan(db: Session) -> ScanResult: + scan = ScanResult() + _scan_duplicate_customers(db, scan) + _scan_missing_required_fields(db, scan) + _scan_odometer_regressions(db, scan) + _scan_booking_overlaps(db, scan) + _scan_vehicle_status_conflicts(db, scan) + db.commit() + return scan + + +def _load_open_issue(db: Session, public_ref: str) -> DataQualityIssue: + issue = db.scalar( + select(DataQualityIssue).where(DataQualityIssue.public_ref == public_ref) + ) + if issue is None: + raise AppError("ISSUE_NOT_FOUND", "Data quality issue not found.", status_code=404) + if issue.status != "open": + raise AppError( + "ISSUE_NOT_OPEN", + f"Issue is '{issue.status}', not 'open'.", + status_code=409, + ) + return issue + + +def defer_issue(db: Session, public_ref: str, actor: CurrentUser) -> DataQualityIssue: + issue = _load_open_issue(db, public_ref) + issue.status = "deferred" + issue.resolved_at = datetime.now(UTC) + issue.resolved_by = actor.display_name + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="data_quality_issue_deferred", + entity_type="data_quality_issue", + entity_id=issue.id, + before={"status": "open"}, + after={"status": "deferred"}, + ) + db.commit() + return issue + + +def reject_issue(db: Session, public_ref: str, actor: CurrentUser) -> DataQualityIssue: + issue = _load_open_issue(db, public_ref) + issue.status = "rejected" + issue.resolved_at = datetime.now(UTC) + issue.resolved_by = actor.display_name + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="data_quality_issue_rejected", + entity_type="data_quality_issue", + entity_id=issue.id, + before={"status": "open"}, + after={"status": "rejected"}, + ) + db.commit() + return issue + + +MERGEABLE_FIELDS = ("first_name", "last_name", "email", "phone", "postal_code", "city") + + +def merge_customers( + db: Session, + public_ref: str, + survivor_ref: str, + field_overrides: dict[str, str] | None, + actor: CurrentUser, +) -> dict: + issue = _load_open_issue(db, public_ref) + if issue.rule_type != "possible_duplicate_customer": + raise AppError( + "NOT_A_DUPLICATE_ISSUE", + "This issue is not a possible-duplicate-customer issue.", + status_code=409, + ) + + entity_ref = issue.evidence_json.get("entity_ref") + related_refs = issue.evidence_json.get("related_refs", []) + candidate_refs = {entity_ref, *related_refs} + if survivor_ref not in candidate_refs: + raise AppError( + "INVALID_SURVIVOR", + "The survivor reference must be one of the two customers in this issue.", + status_code=422, + details={"candidates": sorted(candidate_refs)}, + ) + loser_ref = next(ref for ref in candidate_refs if ref != survivor_ref) + + survivor = db.scalar(select(Customer).where(Customer.public_ref == survivor_ref)) + loser = db.scalar(select(Customer).where(Customer.public_ref == loser_ref)) + if survivor is None or loser is None: + raise AppError( + "CUSTOMER_NOT_FOUND", "One of the customers could not be found.", status_code=404 + ) + + before = { + "survivor": {f: getattr(survivor, f) for f in MERGEABLE_FIELDS}, + "loser": {f: getattr(loser, f) for f in MERGEABLE_FIELDS}, + } + + for field_name, value in (field_overrides or {}).items(): + if field_name not in MERGEABLE_FIELDS: + raise AppError( + "INVALID_FIELD_OVERRIDE", f"Field '{field_name}' cannot be merged.", status_code=422 + ) + setattr(survivor, field_name, value) + + rewired = db.execute( + Booking.__table__.update() + .where(Booking.customer_id == loser.id) + .values(customer_id=survivor.id) + ) + loser.merged_into_customer_id = survivor.id + + issue.status = "resolved" + issue.resolved_at = datetime.now(UTC) + issue.resolved_by = actor.display_name + + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="customer_merged", + entity_type="customer", + entity_id=survivor.id, + before=before, + after={"survivor": {f: getattr(survivor, f) for f in MERGEABLE_FIELDS}}, + metadata={ + "loser_ref": loser_ref, + "survivor_ref": survivor_ref, + "rewired_bookings": rewired.rowcount, + }, + ) + db.commit() + + return { + "issue_ref": issue.public_ref, + "survivor_ref": survivor_ref, + "loser_ref": loser_ref, + "rewired_bookings": rewired.rowcount, + } diff --git a/backend/tests/test_data_quality.py b/backend/tests/test_data_quality.py new file mode 100644 index 0000000..0d44351 --- /dev/null +++ b/backend/tests/test_data_quality.py @@ -0,0 +1,106 @@ +def test_list_includes_all_five_rule_types(ops_client): + response = ops_client.get("/api/v1/data-quality/issues") + assert response.status_code == 200 + issues = response.json() + rule_types = {i["rule_type"] for i in issues} + assert rule_types == { + "possible_duplicate_customer", + "missing_required_field", + "odometer_regression", + "booking_overlap", + "vehicle_status_conflict", + } + + +def test_scan_is_idempotent_once_seeded(ops_client): + # The session fixture already ran a scan as part of seeding; running again + # must not create duplicate open issues for the same (rule_type, entity). + response = ops_client.post("/api/v1/data-quality/scan") + assert response.status_code == 200 + assert response.json()["created"] == {} + + +def test_scan_requires_operations_manager(employee_client): + response = employee_client.post("/api/v1/data-quality/scan") + assert response.status_code == 403 + + +def test_s2_duplicate_customer_issue_detail(ops_client): + response = ops_client.get("/api/v1/data-quality/issues/DQ-DEMO-DUPLICATE") + assert response.status_code == 200 + body = response.json() + assert body["entity_ref"] == "CUS-0012" + assert body["entity_snapshot"]["public_ref"] == "CUS-0012" + assert [s["public_ref"] for s in body["related_snapshots"]] == ["CUS-0178"] + + +def test_s4_booking_overlap_issue_detail(ops_client): + response = ops_client.get("/api/v1/data-quality/issues/DQ-DEMO-OVERLAP") + assert response.status_code == 200 + body = response.json() + assert body["entity_ref"] == "MO-016" + assert set(body["evidence"]["related_refs"]) == {"BK-DEMO-OVERLAP-A", "BK-DEMO-OVERLAP-B"} + + +def test_defer_then_reject_are_rejected_on_closed_issue(ops_client): + issues = ops_client.get( + "/api/v1/data-quality/issues", + params={"rule_type": "missing_required_field", "status": "open"}, + ).json() + target = issues[0]["public_ref"] + + deferred = ops_client.post(f"/api/v1/data-quality/issues/{target}/defer") + assert deferred.status_code == 200 + assert deferred.json()["status"] == "deferred" + + again = ops_client.post(f"/api/v1/data-quality/issues/{target}/reject") + assert again.status_code == 409 + assert again.json()["error"]["code"] == "ISSUE_NOT_OPEN" + + +def test_merge_customers_requires_operations_manager(employee_client): + response = employee_client.post( + "/api/v1/data-quality/issues/DQ-DEMO-DUPLICATE/merge-customers", + json={"survivor_ref": "CUS-0012"}, + ) + assert response.status_code == 403 + + +def test_merge_customers_rejects_unrelated_survivor(ops_client): + response = ops_client.post( + "/api/v1/data-quality/issues/DQ-DEMO-DUPLICATE/merge-customers", + json={"survivor_ref": "CUS-0099"}, + ) + assert response.status_code == 422 + assert response.json()["error"]["code"] == "INVALID_SURVIVOR" + + +def test_merge_customers_s2_scenario_rewires_and_audits(ops_client): + before_bookings = ops_client.get( + "/api/v1/bookings", params={"vehicle_ref": "MO-001"} + ) # warm the client session; irrelevant vehicle, just a cheap authenticated call + assert before_bookings.status_code == 200 + + response = ops_client.post( + "/api/v1/data-quality/issues/DQ-DEMO-DUPLICATE/merge-customers", + json={"survivor_ref": "CUS-0012", "field_overrides": {"city": "Turnhout"}}, + ) + assert response.status_code == 200 + body = response.json() + assert body["survivor_ref"] == "CUS-0012" + assert body["loser_ref"] == "CUS-0178" + + issue = ops_client.get("/api/v1/data-quality/issues/DQ-DEMO-DUPLICATE").json() + assert issue["status"] == "resolved" + + audit_events = ops_client.get( + "/api/v1/audit", params={"action": "customer_merged"} + ).json() + assert len(audit_events) >= 1 + + # Already-resolved issue cannot be merged again. + replay = ops_client.post( + "/api/v1/data-quality/issues/DQ-DEMO-DUPLICATE/merge-customers", + json={"survivor_ref": "CUS-0012"}, + ) + assert replay.status_code == 409 diff --git a/backend/tests/test_seed.py b/backend/tests/test_seed.py index b2f837e..84ea3bd 100644 --- a/backend/tests/test_seed.py +++ b/backend/tests/test_seed.py @@ -20,7 +20,8 @@ def test_seed_counts_match_deterministic_dataset(): assert db.scalar(select(func.count()).select_from(Vehicle)) == 50 assert db.scalar(select(func.count()).select_from(Customer)) == 180 assert db.scalar(select(func.count()).select_from(Booking)) == 246 - assert db.scalar(select(func.count()).select_from(DataQualityIssue)) == 15 + # 15 from the CSV plus a deterministic set discovered by the post-seed scan. + assert db.scalar(select(func.count()).select_from(DataQualityIssue)) == 26 assert db.scalar(select(func.count()).select_from(OutboxEvent)) == 20 assert db.scalar(select(func.count()).select_from(User)) == 2 finally: diff --git a/frontend/src/App.tsx b/frontend/src/App.tsx index 4094eda..cd5fe72 100644 --- a/frontend/src/App.tsx +++ b/frontend/src/App.tsx @@ -8,6 +8,8 @@ import { Vehicles } from "./pages/Vehicles"; import { VehicleDetail } from "./pages/VehicleDetail"; import { Bookings } from "./pages/Bookings"; import { BookingDetail } from "./pages/BookingDetail"; +import { DataQuality } from "./pages/DataQuality"; +import { DataQualityIssueDetail } from "./pages/DataQualityIssueDetail"; import { Audit } from "./pages/Audit"; export function App() { @@ -27,6 +29,8 @@ export function App() { } /> } /> } /> + } /> + } /> } /> } /> diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index 02c5e4f..84b21fa 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -92,6 +92,7 @@ export interface AttentionItem { detail: string; link_type: "vehicle" | "booking" | "customer"; link_ref: string; + issue_ref: string | null; } export interface TodayItem { @@ -144,6 +145,28 @@ export interface RegisterReturnResult { next_booking_risk: NextBookingRisk | null; } +export interface EntitySnapshot { + public_ref: string; + [key: string]: unknown; +} + +export interface DataQualityIssueDetail extends DataQualityIssue { + entity_snapshot: EntitySnapshot | null; + related_snapshots: EntitySnapshot[]; +} + +export interface MergeCustomersRequest { + survivor_ref: string; + field_overrides?: Record; +} + +export interface MergeCustomersResult { + issue_ref: string; + survivor_ref: string; + loser_ref: string; + rewired_bookings: number; +} + export interface AuditEvent { id: string; actor_type: string; diff --git a/frontend/src/components/Layout.tsx b/frontend/src/components/Layout.tsx index 2962a2d..e83e06b 100644 --- a/frontend/src/components/Layout.tsx +++ b/frontend/src/components/Layout.tsx @@ -5,6 +5,7 @@ const NAV_ITEMS = [ { to: "/dashboard", label: "Dashboard" }, { to: "/vehicles", label: "Vehicles" }, { to: "/bookings", label: "Bookings" }, + { to: "/data-quality", label: "Data Quality" }, { to: "/audit", label: "Audit" }, ]; diff --git a/frontend/src/pages/Dashboard.tsx b/frontend/src/pages/Dashboard.tsx index 183c216..cef6170 100644 --- a/frontend/src/pages/Dashboard.tsx +++ b/frontend/src/pages/Dashboard.tsx @@ -53,7 +53,9 @@ export function Dashboard() {

- {item.link_type === "vehicle" ? ( + {item.issue_ref ? ( + {item.title} + ) : item.link_type === "vehicle" ? ( {item.title} ) : ( item.title diff --git a/frontend/src/pages/DataQuality.tsx b/frontend/src/pages/DataQuality.tsx new file mode 100644 index 0000000..bb2d0f5 --- /dev/null +++ b/frontend/src/pages/DataQuality.tsx @@ -0,0 +1,96 @@ +import { useEffect, useState } from "react"; +import { Link } from "react-router-dom"; +import { api } from "../api/client"; +import type { DataQualityIssue } from "../api/types"; +import { SeverityBadge, StatusBadge } from "../components/Badge"; + +const RULE_TYPES = [ + "possible_duplicate_customer", + "missing_required_field", + "odometer_regression", + "booking_overlap", + "vehicle_status_conflict", +]; + +export function DataQuality() { + const [issues, setIssues] = useState(null); + const [error, setError] = useState(null); + const [status, setStatus] = useState("open"); + const [ruleType, setRuleType] = useState(""); + + useEffect(() => { + const params = new URLSearchParams(); + if (status) params.set("status", status); + if (ruleType) params.set("rule_type", ruleType); + api + .get(`/api/v1/data-quality/issues?${params.toString()}`) + .then(setIssues) + .catch(() => setError("Data-quality issues are unavailable right now.")); + }, [status, ruleType]); + + return ( +

+

Data Quality

+ +
+ + +
+ + {error &&

{error}

} + {!error && !issues &&

Loading issues…

} + {issues && issues.length === 0 &&

No issues match these filters.

} + + {issues && issues.length > 0 && ( + + + + + + + + + + + + + {issues.map((i) => ( + + + + + + + + ))} + +
Data-quality issues
ReferenceRuleEntitySeverityStatus
+ {i.public_ref} + {i.rule_type.replace(/_/g, " ")}{i.entity_ref} + + + +
+ )} +
+ ); +} diff --git a/frontend/src/pages/DataQualityIssueDetail.tsx b/frontend/src/pages/DataQualityIssueDetail.tsx new file mode 100644 index 0000000..7b57ade --- /dev/null +++ b/frontend/src/pages/DataQualityIssueDetail.tsx @@ -0,0 +1,242 @@ +import { useCallback, useEffect, useState } from "react"; +import { Link, useParams } from "react-router-dom"; +import { api, ApiError } from "../api/client"; +import type { DataQualityIssueDetail as IssueDetail, EntitySnapshot } from "../api/types"; +import { SeverityBadge, StatusBadge } from "../components/Badge"; +import { useAuth } from "../context/AuthContext"; + +const MERGE_FIELDS = ["first_name", "last_name", "email", "phone", "postal_code", "city"]; + +function DuplicateCustomerPanel({ issue, onResolved }: { issue: IssueDetail; onResolved: () => void }) { + const { user } = useAuth(); + const [survivorRef, setSurvivorRef] = useState(issue.entity_snapshot?.public_ref ?? ""); + const [fieldChoices, setFieldChoices] = useState>({}); + const [error, setError] = useState(null); + const [submitting, setSubmitting] = useState(false); + const [confirming, setConfirming] = useState(false); + + if (!issue.entity_snapshot || !issue.related_snapshots[0]) { + return

Both customers in this comparison could not be loaded.

; + } + const a: EntitySnapshot = issue.entity_snapshot; + const b: EntitySnapshot = issue.related_snapshots[0]; + + const survivor = survivorRef === a.public_ref ? a : b; + const loser = survivorRef === a.public_ref ? b : a; + + async function handleMerge() { + setError(null); + setSubmitting(true); + try { + const overrides: Record = {}; + for (const field of MERGE_FIELDS) { + const choice = fieldChoices[field]; + const chosenSide = choice === "a" ? a : choice === "b" ? b : survivor; + if (chosenSide !== survivor && chosenSide[field]) { + overrides[field] = String(chosenSide[field]); + } + } + await api.post(`/api/v1/data-quality/issues/${issue.public_ref}/merge-customers`, { + survivor_ref: survivor.public_ref, + field_overrides: Object.keys(overrides).length > 0 ? overrides : undefined, + }); + onResolved(); + } catch (err) { + setError(err instanceof ApiError ? err.message : "Could not merge these customers."); + setConfirming(false); + } finally { + setSubmitting(false); + } + } + + if (user?.role !== "operations_manager") { + return ( +

+ Merging duplicate customers requires the Operations Manager role. Switch role to resolve + this issue. +

+ ); + } + + return ( +
+

Compare and merge

+ {error &&

{error}

} + +
+ Keep as survivor + + +
+ + + + + + + + + + + {MERGE_FIELDS.map((field) => { + const valueA = a[field] ? String(a[field]) : "—"; + const valueB = b[field] ? String(b[field]) : "—"; + const differ = valueA !== valueB; + return ( + + + + + + ); + })} + +
Field{a.public_ref}{b.public_ref}
{field.replace(/_/g, " ")} + {differ ? ( + + ) : ( + valueA + )} + + {differ ? ( + + ) : ( + valueB + )} +
+ +

+ {loser.public_ref} will become a tombstone linked to{" "} + {survivor.public_ref}; its bookings will be rewired to the survivor. +

+ + {!confirming && ( + + )} + {confirming && ( +
+

+ Merge {loser.public_ref} into {survivor.public_ref}? This cannot be undone. +

+ + +
+ )} +
+ ); +} + +export function DataQualityIssueDetail() { + const { publicRef } = useParams<{ publicRef: string }>(); + const [issue, setIssue] = useState(null); + const [error, setError] = useState(null); + const [actionError, setActionError] = useState(null); + + const load = useCallback(() => { + if (!publicRef) return; + api + .get(`/api/v1/data-quality/issues/${publicRef}`) + .then(setIssue) + .catch(() => setError("This issue could not be found.")); + }, [publicRef]); + + useEffect(() => { + setIssue(null); + setError(null); + load(); + }, [load]); + + async function handleAction(action: "defer" | "reject") { + if (!issue) return; + setActionError(null); + try { + await api.post(`/api/v1/data-quality/issues/${issue.public_ref}/${action}`); + load(); + } catch (err) { + setActionError(err instanceof ApiError ? err.message : `Could not ${action} this issue.`); + } + } + + if (error) return

{error}

; + if (!issue) return

Loading issue…

; + + return ( +
+

← Back to data quality

+

{issue.public_ref}

+

+ +

+
+
Rule
{issue.rule_type.replace(/_/g, " ")}
+
Entity
{issue.entity_ref}
+
Evidence
{String(issue.evidence.summary ?? "")}
+
+ + {actionError &&

{actionError}

} + + {issue.status === "open" && issue.rule_type === "possible_duplicate_customer" && ( + + )} + + {issue.status === "open" && ( +
+

Resolution

+ {issue.rule_type !== "possible_duplicate_customer" && ( + <> +

Evidence for this issue:

+ 
{JSON.stringify(issue.evidence, null, 2)}
+ + )} +

Defer to review later, or reject if this is not a real issue.

+
+ + +
+
+ )} +
+ ); +} diff --git a/frontend/src/pages/VehicleDetail.tsx b/frontend/src/pages/VehicleDetail.tsx index 6a743d3..aba10f5 100644 --- a/frontend/src/pages/VehicleDetail.tsx +++ b/frontend/src/pages/VehicleDetail.tsx @@ -112,6 +112,7 @@ export function VehicleDetail() { {vehicle.quality_issues.length === 0 &&
  • No quality issues recorded.
    • } {vehicle.quality_issues.map((q) => (
  • + {q.public_ref} {q.rule_type.replace(/_/g, " ")} diff --git a/frontend/src/styles.css b/frontend/src/styles.css index d15f42b..1b22dd8 100644 --- a/frontend/src/styles.css +++ b/frontend/src/styles.css @@ -188,6 +188,38 @@ a { color: #1f5c8f; } .return-form button:disabled { opacity: 0.6; cursor: not-allowed; } .return-result .error { margin-top: 12px; } +.duplicate-compare fieldset { + border: 1px solid #dce3eb; border-radius: 10px; padding: 12px 16px; margin: 12px 0; +} +.duplicate-compare legend { font-weight: 700; color: #375065; padding: 0 6px; } +.duplicate-compare fieldset label { margin-right: 20px; } +.compare-table th, .compare-table td { vertical-align: top; } +.compare-table label { display: inline-flex; align-items: center; gap: 6px; font-weight: 400; } +.duplicate-compare > button { + padding: 12px 20px; border-radius: 10px; border: none; + background: #14324f; color: white; font-weight: 700; cursor: pointer; margin-top: 12px; +} +.confirm-bar { + margin-top: 12px; padding: 14px; border-radius: 10px; + background: #fdf1de; border: 1px solid #f0d29e; +} +.confirm-bar p { margin: 0 0 10px; font-weight: 600; color: #8a5a10; } +.confirm-bar button { + padding: 10px 16px; border-radius: 8px; border: none; font-weight: 700; cursor: pointer; margin-right: 8px; +} +.confirm-bar button:first-of-type { background: #9a2530; color: white; } +.confirm-bar button:last-of-type { background: white; border: 1px solid #cfd8e2; } + +.evidence-block { + background: #f6f8fb; border: 1px solid #dce3eb; border-radius: 10px; + padding: 12px; overflow-x: auto; font-size: 0.85rem; +} +.resolution-actions { display: flex; gap: 10px; margin-top: 12px; } +.resolution-actions button { + padding: 10px 18px; border-radius: 8px; border: 1px solid #cfd8e2; + background: white; font-weight: 700; cursor: pointer; +} + @media (max-width: 700px) { .app-header { flex-direction: column; align-items: flex-start; } .user-badge { margin-left: 0; }