From 6e227a214a3d101146770decfa6a19dfe6e2eaea Mon Sep 17 00:00:00 2001 From: NuklearRabbit <145918611+NuklearRabbit@users.noreply.github.com> Date: Sun, 2 Aug 2026 06:16:07 +0200 Subject: [PATCH] feat(quality): complete bounded resolution flows and typed snapshots Two real gaps here: related-entity snapshots were typed by inferring from the issue's rule_type (get_issue always resolved related refs as "customer" for duplicates and "vehicle" for everything else), so a booking_overlap issue's related bookings silently failed to resolve; and defer/reject were the only resolution actions for 4 of 5 rule types, leaving missing_required_field, odometer_regression, booking_overlap and vehicle_status_conflict with no real path beyond a generic reject. Type related entities from their own public-reference prefix (CUS-/MO-/ BK-/INSP-) instead of the issue's rule_type, and add typed snapshots for booking and inspection. Add one bounded resolution endpoint per remaining rule type: provide-fields (re-runs the missing-field check, resolves only once nothing required is missing), resolve-odometer-regression (retain canonical or correct the reading -- never silently lowers canonical mileage), resolve-overlap (blocks one of the two bookings, re-verifies no overlap remains), apply-recommended-status (one authoritative recommendation function shared with re-validation). Manual scan now takes an actor and audits data_quality_scan_run. Reintroduced evidence after a non-open decision links the new issue back to the prior one (evidence.reopened_from / previous_decision) instead of looking like a fresh, undecided problem. --- backend/app/api/routers/data_quality.py | 162 +++++++-- backend/app/schemas.py | 22 ++ backend/app/services/data_quality.py | 426 +++++++++++++++++++++++- backend/tests/test_data_quality.py | 262 +++++++++++++++ 4 files changed, 843 insertions(+), 29 deletions(-) diff --git a/backend/app/api/routers/data_quality.py b/backend/app/api/routers/data_quality.py index 308551f..e8bd1e7 100644 --- a/backend/app/api/routers/data_quality.py +++ b/backend/app/api/routers/data_quality.py @@ -5,18 +5,33 @@ from sqlalchemy import select from sqlalchemy.orm import Session from app.api.deps import get_db, require_operations_manager +from app.models.booking import Booking from app.models.customer import Customer from app.models.data_quality import DataQualityIssue +from app.models.inspection import Inspection from app.models.vehicle import Vehicle from app.schemas import ( + ApplyRecommendedStatusResult, CurrentUser, DataQualityIssueDetailOut, DataQualityIssueOut, MergeCustomersRequest, MergeCustomersResult, + ProvideFieldsRequest, + ResolveOdometerRegressionRequest, + ResolveOverlapRequest, ScanResultOut, ) -from app.services.data_quality import defer_issue, merge_customers, reject_issue, run_scan +from app.services.data_quality import ( + apply_recommended_status, + defer_issue, + merge_customers, + provide_missing_fields, + reject_issue, + resolve_booking_overlap, + resolve_odometer_regression, + run_scan, +) router = APIRouter(prefix="/api/v1/data-quality", tags=["data-quality"]) @@ -54,12 +69,33 @@ def list_issues( return [_to_out(i) for i in issues] +# Every public reference in this system carries its entity type in its own prefix +# (CUS-/MO-/BK-/INSP-/DQ-). Related-entity typing is resolved from the reference itself, +# not guessed from the issue's rule_type -- a booking_overlap issue's related refs are +# bookings, not vehicles, and an inline odometer_regression issue's related refs mix a +# booking and an inspection ref in the same list. +_PREFIX_TO_TYPE = { + "CUS-": "customer", + "MO-": "vehicle", + "BK-": "booking", + "INSP-": "inspection", +} + + +def _entity_type_for_ref(ref: str) -> str | None: + for prefix, entity_type in _PREFIX_TO_TYPE.items(): + if ref.startswith(prefix): + return entity_type + return None + + def _snapshot(entity_type: str, ref: str, db: Session) -> dict | None: if entity_type == "customer": customer = db.scalar(select(Customer).where(Customer.public_ref == ref)) if customer is None: return None return { + "entity_type": "customer", "public_ref": customer.public_ref, "first_name": customer.first_name, "last_name": customer.last_name, @@ -68,17 +104,49 @@ def _snapshot(entity_type: str, ref: str, db: Session) -> dict | None: "postal_code": customer.postal_code, "city": customer.city, } - vehicle = db.scalar(select(Vehicle).where(Vehicle.public_ref == ref)) - if vehicle is None: - return None - return { - "public_ref": vehicle.public_ref, - "make": vehicle.make, - "model": vehicle.model, - "location": vehicle.location, - "operational_status": vehicle.operational_status, - "odometer_km": vehicle.odometer_km, - } + if entity_type == "vehicle": + vehicle = db.scalar(select(Vehicle).where(Vehicle.public_ref == ref)) + if vehicle is None: + return None + return { + "entity_type": "vehicle", + "public_ref": vehicle.public_ref, + "make": vehicle.make, + "model": vehicle.model, + "location": vehicle.location, + "operational_status": vehicle.operational_status, + "odometer_km": vehicle.odometer_km, + } + if entity_type == "booking": + booking = db.scalar(select(Booking).where(Booking.public_ref == ref)) + if booking is None: + return None + vehicle = db.get(Vehicle, booking.vehicle_id) + customer = db.get(Customer, booking.customer_id) + return { + "entity_type": "booking", + "public_ref": booking.public_ref, + "status": booking.status, + "starts_at": booking.starts_at.isoformat(), + "ends_at": booking.ends_at.isoformat(), + "vehicle_ref": vehicle.public_ref if vehicle else None, + "customer_ref": customer.public_ref if customer else None, + "end_odometer_km": booking.end_odometer_km, + } + if entity_type == "inspection": + inspection = db.scalar(select(Inspection).where(Inspection.public_ref == ref)) + if inspection is None: + return None + booking = db.get(Booking, inspection.booking_id) + return { + "entity_type": "inspection", + "public_ref": inspection.public_ref, + "type": inspection.type, + "odometer_km": inspection.odometer_km, + "completed_at": inspection.completed_at.isoformat(), + "booking_ref": booking.public_ref if booking else None, + } + return None @router.get("/issues/{public_ref}", response_model=DataQualityIssueDetailOut) @@ -92,17 +160,18 @@ def get_issue( 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" - ) + related_snapshots = [] + for ref in related_refs: + entity_type = _entity_type_for_ref(ref) + if entity_type is None: + continue + snap = _snapshot(entity_type, ref, db) + if snap is not None: + related_snapshots.append(snap) 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 - ], + related_snapshots=related_snapshots, ) @@ -137,10 +206,59 @@ def merge( return MergeCustomersResult(**result) +@router.post("/issues/{public_ref}/provide-fields", response_model=DataQualityIssueOut) +def provide_fields( + public_ref: str, + body: ProvideFieldsRequest, + db: Session = Depends(get_db), + user: CurrentUser = Depends(require_operations_manager), +) -> DataQualityIssueOut: + issue = provide_missing_fields(db, public_ref, body.fields, user) + return _to_out(issue) + + +@router.post( + "/issues/{public_ref}/resolve-odometer-regression", response_model=DataQualityIssueOut +) +def resolve_odometer( + public_ref: str, + body: ResolveOdometerRegressionRequest, + db: Session = Depends(get_db), + user: CurrentUser = Depends(require_operations_manager), +) -> DataQualityIssueOut: + issue = resolve_odometer_regression(db, public_ref, body, user) + return _to_out(issue) + + +@router.post("/issues/{public_ref}/resolve-overlap", response_model=DataQualityIssueOut) +def resolve_overlap( + public_ref: str, + body: ResolveOverlapRequest, + db: Session = Depends(get_db), + user: CurrentUser = Depends(require_operations_manager), +) -> DataQualityIssueOut: + issue = resolve_booking_overlap(db, public_ref, body.booking_ref, body.note, user) + return _to_out(issue) + + +@router.post( + "/issues/{public_ref}/apply-recommended-status", response_model=ApplyRecommendedStatusResult +) +def apply_status( + public_ref: str, + db: Session = Depends(get_db), + user: CurrentUser = Depends(require_operations_manager), +) -> ApplyRecommendedStatusResult: + issue, applied_status, reason = apply_recommended_status(db, public_ref, user) + return ApplyRecommendedStatusResult( + issue=_to_out(issue), applied_status=applied_status, reason=reason + ) + + @router.post("/scan", response_model=ScanResultOut) def scan( db: Session = Depends(get_db), - _user: CurrentUser = Depends(require_operations_manager), + user: CurrentUser = Depends(require_operations_manager), ) -> ScanResultOut: - result = run_scan(db) + result = run_scan(db, actor=user) return ScanResultOut(created=result.created) diff --git a/backend/app/schemas.py b/backend/app/schemas.py index 3f4204a..e348c56 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -141,6 +141,28 @@ class ScanResultOut(BaseModel): created: dict[str, int] +class ProvideFieldsRequest(BaseModel): + fields: dict[str, str] + + +class ResolveOdometerRegressionRequest(BaseModel): + decision: Literal["retain_canonical", "correct_reading"] + booking_ref: str | None = None + corrected_odometer_km: Annotated[int, Field(ge=0)] | None = None + note: str | None = Field(default=None, max_length=500) + + +class ResolveOverlapRequest(BaseModel): + booking_ref: str + note: str | None = Field(default=None, max_length=500) + + +class ApplyRecommendedStatusResult(BaseModel): + issue: DataQualityIssueOut + applied_status: str + reason: str + + class VehicleDetailOut(VehicleOut): bookings: list[BookingSummaryOut] = Field(default_factory=list) inspections: list[InspectionOut] = Field(default_factory=list) diff --git a/backend/app/services/data_quality.py b/backend/app/services/data_quality.py index 0ac42a2..e1f1e25 100644 --- a/backend/app/services/data_quality.py +++ b/backend/app/services/data_quality.py @@ -13,7 +13,7 @@ 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.schemas import CurrentUser, ResolveOdometerRegressionRequest from app.services.audit import record_audit_event REQUIRED_CUSTOMER_FIELDS = ("first_name", "last_name") @@ -73,6 +73,29 @@ def _open_issue( if _has_open_issue(db, rule_type, entity_type, entity_id): return now = datetime.now(UTC) + + # Reintroduced evidence creates a new issue rather than silently reopening the old + # one, but it stays linked to whatever decision was made last time so an operator + # doesn't re-litigate from a blank slate. + previous = db.scalar( + select(DataQualityIssue) + .where( + DataQualityIssue.rule_type == rule_type, + DataQualityIssue.entity_type == entity_type, + DataQualityIssue.entity_id == entity_id, + DataQualityIssue.status != "open", + ) + .order_by(DataQualityIssue.detected_at.desc()) + ) + evidence: dict = { + "summary": summary, + "entity_ref": entity_ref, + "related_refs": related_refs, + } + if previous is not None: + evidence["reopened_from"] = previous.public_ref + evidence["previous_decision"] = previous.status + issue = DataQualityIssue( public_ref=_next_public_ref(db, "DQ-SCAN"), rule_type=rule_type, @@ -80,11 +103,7 @@ def _open_issue( entity_id=entity_id, severity=severity, status="open", - evidence_json={ - "summary": summary, - "entity_ref": entity_ref, - "related_refs": related_refs, - }, + evidence_json=evidence, proposed_action_json={}, detected_at=now, ) @@ -280,13 +299,22 @@ def _scan_odometer_regressions(db: Session, scan: ScanResult) -> None: break -def run_scan(db: Session) -> ScanResult: +def run_scan(db: Session, *, actor: CurrentUser | None = None) -> 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) + if actor is not None: + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="data_quality_scan_run", + entity_type="system", + metadata={"created": scan.created}, + ) db.commit() return scan @@ -344,6 +372,390 @@ def reject_issue(db: Session, public_ref: str, actor: CurrentUser) -> DataQualit return issue +def provide_missing_fields( + db: Session, public_ref: str, fields: dict[str, str], actor: CurrentUser +) -> DataQualityIssue: + issue = _load_open_issue(db, public_ref) + if issue.rule_type != "missing_required_field": + raise AppError( + "NOT_A_MISSING_FIELD_ISSUE", + "This issue is not a missing-required-field issue.", + status_code=409, + ) + + entity: Customer | Vehicle | None + if issue.entity_type == "customer": + entity = db.get(Customer, issue.entity_id) + allowed = {*REQUIRED_CUSTOMER_FIELDS, "email", "phone"} + elif issue.entity_type == "vehicle": + entity = db.get(Vehicle, issue.entity_id) + allowed = set(REQUIRED_VEHICLE_FIELDS) + else: + raise AppError( + "UNSUPPORTED_ENTITY", + f"Cannot provide fields for entity type '{issue.entity_type}'.", + status_code=409, + ) + if entity is None: + raise AppError( + "ENTITY_NOT_FOUND", "The underlying record could not be found.", status_code=404 + ) + + invalid = set(fields) - allowed + if invalid: + raise AppError( + "INVALID_FIELD", + f"Fields not permitted here: {', '.join(sorted(invalid))}.", + status_code=422, + ) + if not fields: + raise AppError( + "NO_FIELDS_PROVIDED", "At least one field must be provided.", status_code=422 + ) + + before = {f: getattr(entity, f) for f in allowed} + for field_name, value in fields.items(): + if not value.strip(): + raise AppError("EMPTY_VALUE", f"Field '{field_name}' cannot be blank.", status_code=422) + setattr(entity, field_name, value.strip()) + after = {f: getattr(entity, f) for f in allowed} + + correlation_id = uuid.uuid4() + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="data_quality_fields_provided", + entity_type=issue.entity_type, + entity_id=entity.id, + correlation_id=correlation_id, + before=before, + after=after, + metadata={"issue_ref": issue.public_ref}, + ) + + if isinstance(entity, Customer): + missing = [f for f in REQUIRED_CUSTOMER_FIELDS if not getattr(entity, f)] + if not entity.email and not entity.phone: + missing.append("email_or_phone") + else: + missing = [f for f in REQUIRED_VEHICLE_FIELDS if not getattr(entity, f)] + + if not missing: + 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="data_quality_issue_resolved", + entity_type="data_quality_issue", + entity_id=issue.id, + correlation_id=correlation_id, + before={"status": "open"}, + after={"status": "resolved"}, + ) + else: + issue.evidence_json = {**issue.evidence_json, "summary": f"Missing: {', '.join(missing)}"} + + db.commit() + return issue + + +def resolve_odometer_regression( + db: Session, public_ref: str, body: ResolveOdometerRegressionRequest, actor: CurrentUser +) -> DataQualityIssue: + issue = _load_open_issue(db, public_ref) + if issue.rule_type != "odometer_regression": + raise AppError( + "NOT_AN_ODOMETER_ISSUE", + "This issue is not an odometer_regression issue.", + status_code=409, + ) + vehicle = db.scalar(select(Vehicle).where(Vehicle.id == issue.entity_id).with_for_update()) + if vehicle is None: + raise AppError( + "VEHICLE_NOT_FOUND", "The vehicle for this issue was not found.", status_code=404 + ) + + correlation_id = uuid.uuid4() + + if body.decision == "retain_canonical": + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="data_quality_odometer_retained", + entity_type="vehicle", + entity_id=vehicle.id, + correlation_id=correlation_id, + metadata={"issue_ref": issue.public_ref, "canonical_odometer_km": vehicle.odometer_km}, + ) + else: + related_refs = issue.evidence_json.get("related_refs", []) + if body.booking_ref not in related_refs: + raise AppError( + "INVALID_BOOKING_REFERENCE", + "booking_ref must be one of this issue's related bookings.", + status_code=422, + ) + if body.corrected_odometer_km is None: + raise AppError( + "CORRECTED_VALUE_REQUIRED", + "corrected_odometer_km is required when correcting a reading.", + status_code=422, + ) + # Never silently lower the canonical odometer: a correction must be at or above + # the current canonical value, otherwise it would just create a new regression. + if body.corrected_odometer_km < vehicle.odometer_km: + raise AppError( + "CORRECTION_BELOW_CANONICAL", + ( + f"Corrected value {body.corrected_odometer_km} km is still below the " + f"canonical {vehicle.odometer_km} km; it would not resolve the regression." + ), + status_code=422, + ) + booking = db.scalar( + select(Booking).where(Booking.public_ref == body.booking_ref).with_for_update() + ) + if booking is None: + raise AppError( + "BOOKING_NOT_FOUND", "The booking to correct was not found.", status_code=404 + ) + + before = { + "booking_end_odometer_km": booking.end_odometer_km, + "vehicle_odometer_km": vehicle.odometer_km, + } + booking.end_odometer_km = body.corrected_odometer_km + vehicle.odometer_km = body.corrected_odometer_km + vehicle.version += 1 + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="data_quality_odometer_corrected", + entity_type="vehicle", + entity_id=vehicle.id, + correlation_id=correlation_id, + before=before, + after={ + "booking_end_odometer_km": booking.end_odometer_km, + "vehicle_odometer_km": vehicle.odometer_km, + }, + metadata={"issue_ref": issue.public_ref, "booking_ref": booking.public_ref}, + ) + + 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="data_quality_issue_resolved", + entity_type="data_quality_issue", + entity_id=issue.id, + correlation_id=correlation_id, + before={"status": "open"}, + after={"status": "resolved"}, + metadata={"decision": body.decision, "note": body.note}, + ) + db.commit() + return issue + + +def resolve_booking_overlap( + db: Session, public_ref: str, booking_ref: str, note: str | None, actor: CurrentUser +) -> DataQualityIssue: + issue = _load_open_issue(db, public_ref) + if issue.rule_type != "booking_overlap": + raise AppError( + "NOT_AN_OVERLAP_ISSUE", "This issue is not a booking_overlap issue.", status_code=409 + ) + related_refs = issue.evidence_json.get("related_refs", []) + if booking_ref not in related_refs: + raise AppError( + "INVALID_BOOKING_REFERENCE", + "booking_ref must be one of this issue's overlapping bookings.", + status_code=422, + ) + booking = db.scalar(select(Booking).where(Booking.public_ref == booking_ref).with_for_update()) + if booking is None: + raise AppError("BOOKING_NOT_FOUND", "The booking to block was not found.", status_code=404) + if booking.status not in ("reserved", "active"): + raise AppError( + "BOOKING_NOT_ACTIVE", + f"Booking is '{booking.status}'; only a reserved or active booking can be blocked.", + status_code=409, + ) + + before = {"status": booking.status} + booking.status = "blocked" + + # Verify the minimal safe resolution actually removed the conflict: no two + # reserved/active bookings for this vehicle should still overlap. The session has + # autoflush disabled, so exclude the just-blocked booking by id rather than relying + # on the in-memory status change being visible to this query. + remaining = db.scalars( + select(Booking).where( + Booking.vehicle_id == booking.vehicle_id, + Booking.status.in_(["reserved", "active"]), + Booking.public_ref.in_(related_refs), + Booking.id != booking.id, + ) + ).all() + for i, first in enumerate(remaining): + for second in remaining[i + 1 :]: + if second.starts_at < first.ends_at and first.starts_at < second.ends_at: + raise AppError( + "OVERLAP_STILL_PRESENT", + "Blocking this booking did not remove the overlap; another commitment remains.", + status_code=409, + ) + + correlation_id = uuid.uuid4() + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="data_quality_booking_blocked", + entity_type="booking", + entity_id=booking.id, + correlation_id=correlation_id, + before=before, + after={"status": booking.status}, + metadata={"issue_ref": issue.public_ref, "note": note}, + ) + + 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="data_quality_issue_resolved", + entity_type="data_quality_issue", + entity_id=issue.id, + correlation_id=correlation_id, + before={"status": "open"}, + after={"status": "resolved"}, + ) + db.commit() + return issue + + +def _recommend_vehicle_status( + operational_status: str, has_active_booking: bool, has_open_high_issue: bool +) -> tuple[str, str] | None: + """The single authoritative recommendation function for vehicle_status_conflict, + mirroring the exact conditions `_scan_vehicle_status_conflicts` flags.""" + if operational_status == "available" and has_active_booking: + return "rented", "An active booking exists; the vehicle should be marked rented." + if operational_status == "rented" and not has_active_booking: + return "available", "No active booking exists; the vehicle should be marked available." + if operational_status == "available" and has_open_high_issue: + return "blocked", "A high-severity quality issue is open; the vehicle should be blocked." + if operational_status == "maintenance" and has_active_booking: + return ( + "rented", + "An active booking exists despite the maintenance status; it should be rented.", + ) + return None + + +def apply_recommended_status( + db: Session, public_ref: str, actor: CurrentUser +) -> tuple[DataQualityIssue, str, str]: + issue = _load_open_issue(db, public_ref) + if issue.rule_type != "vehicle_status_conflict": + raise AppError( + "NOT_A_STATUS_CONFLICT_ISSUE", + "This issue is not a vehicle_status_conflict issue.", + status_code=409, + ) + vehicle = db.scalar(select(Vehicle).where(Vehicle.id == issue.entity_id).with_for_update()) + if vehicle is None: + raise AppError( + "VEHICLE_NOT_FOUND", "The vehicle for this issue was not found.", status_code=404 + ) + + has_active_booking = ( + db.scalar( + select(Booking.id).where(Booking.vehicle_id == vehicle.id, Booking.status == "active") + ) + is not None + ) + has_open_high_issue = ( + db.scalar( + select(DataQualityIssue.id).where( + DataQualityIssue.entity_type == "vehicle", + DataQualityIssue.entity_id == vehicle.id, + DataQualityIssue.status == "open", + DataQualityIssue.severity == "high", + DataQualityIssue.id != issue.id, + ) + ) + is not None + ) + recommendation = _recommend_vehicle_status( + vehicle.operational_status, has_active_booking, has_open_high_issue + ) + if recommendation is None: + raise AppError( + "NO_CONFLICT_DETECTED", + "The current vehicle state no longer conflicts; nothing to apply.", + status_code=409, + ) + new_status, reason = recommendation + + before = {"operational_status": vehicle.operational_status} + vehicle.operational_status = new_status + vehicle.version += 1 + + # Re-validate: the same recommendation function must find no further conflict. + if _recommend_vehicle_status(new_status, has_active_booking, has_open_high_issue) is not None: + raise AppError( + "CONFLICT_STILL_PRESENT", + "Applying the recommended status did not resolve the conflict.", + status_code=409, + ) + + correlation_id = uuid.uuid4() + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="data_quality_status_applied", + entity_type="vehicle", + entity_id=vehicle.id, + correlation_id=correlation_id, + before=before, + after={"operational_status": vehicle.operational_status}, + metadata={"issue_ref": issue.public_ref, "reason": reason}, + ) + + 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="data_quality_issue_resolved", + entity_type="data_quality_issue", + entity_id=issue.id, + correlation_id=correlation_id, + before={"status": "open"}, + after={"status": "resolved"}, + ) + db.commit() + return issue, new_status, reason + + MERGEABLE_FIELDS = ("first_name", "last_name", "email", "phone", "postal_code", "city") diff --git a/backend/tests/test_data_quality.py b/backend/tests/test_data_quality.py index 6628e10..c995b35 100644 --- a/backend/tests/test_data_quality.py +++ b/backend/tests/test_data_quality.py @@ -1,3 +1,26 @@ +from sqlalchemy import select + +from app.core.db import SessionLocal +from app.models.booking import Booking +from app.models.vehicle import Vehicle + + +def _activate_booking(vehicle_ref: str, start_odometer_km: int) -> str: + db = SessionLocal() + try: + vehicle = db.scalar(select(Vehicle).where(Vehicle.public_ref == vehicle_ref)) + booking = db.scalar( + select(Booking).where(Booking.vehicle_id == vehicle.id, Booking.status == "returned") + ) + booking.status = "active" + booking.start_odometer_km = start_odometer_km + booking.end_odometer_km = None + db.commit() + return booking.public_ref + finally: + db.close() + + def test_list_includes_all_five_rule_types(ops_client): response = ops_client.get("/api/v1/data-quality/issues") assert response.status_code == 200 @@ -124,3 +147,242 @@ def test_merge_customers_s2_scenario_rewires_and_audits(ops_client): json={"survivor_ref": "CUS-0012"}, ) assert replay.status_code == 409 + + +def test_overlap_related_snapshots_are_typed_as_bookings_not_vehicles(ops_client): + body = ops_client.get("/api/v1/data-quality/issues/DQ-DEMO-OVERLAP").json() + assert len(body["related_snapshots"]) == 2 + for snap in body["related_snapshots"]: + assert snap["entity_type"] == "booking" + assert snap["public_ref"] in {"BK-DEMO-OVERLAP-A", "BK-DEMO-OVERLAP-B"} + assert "starts_at" in snap and "ends_at" in snap + + +def _first_open(ops_client, rule_type: str) -> dict: + issues = ops_client.get( + "/api/v1/data-quality/issues", params={"rule_type": rule_type, "status": "open"} + ).json() + assert issues, f"expected at least one open {rule_type} issue" + return issues[0] + + +def test_provide_fields_requires_operations_manager(employee_client): + response = employee_client.post( + "/api/v1/data-quality/issues/DQ-DEMO-ATTENTION/provide-fields", + json={"fields": {"registration_number": "TST-001"}}, + ) + assert response.status_code == 403 + + +def test_provide_fields_rejects_wrong_rule_type(ops_client): + response = ops_client.post( + "/api/v1/data-quality/issues/DQ-DEMO-OVERLAP/provide-fields", + json={"fields": {"make": "Test"}}, + ) + assert response.status_code == 409 + assert response.json()["error"]["code"] == "NOT_A_MISSING_FIELD_ISSUE" + + +def test_provide_fields_rejects_disallowed_field(ops_client): + target = _first_open(ops_client, "missing_required_field") + detail = ops_client.get(f"/api/v1/data-quality/issues/{target['public_ref']}").json() + disallowed = "city" if detail["entity_type"] == "customer" else "next_service_km" + response = ops_client.post( + f"/api/v1/data-quality/issues/{target['public_ref']}/provide-fields", + json={"fields": {disallowed: "anything"}}, + ) + assert response.status_code == 422 + assert response.json()["error"]["code"] == "INVALID_FIELD" + + +def test_provide_fields_resolves_a_vehicle_missing_field_issue(ops_client): + issues = ops_client.get( + "/api/v1/data-quality/issues", + params={"rule_type": "missing_required_field", "status": "open"}, + ).json() + target = next(i for i in issues if i["entity_type"] == "vehicle") + + response = ops_client.post( + f"/api/v1/data-quality/issues/{target['public_ref']}/provide-fields", + json={ + "fields": { + "registration_number": "TST-999", + "make": "TestMake", + "model": "TestModel", + "location": "Depot", + } + }, + ) + assert response.status_code == 200 + assert response.json()["status"] == "resolved" + + vehicle = ops_client.get(f"/api/v1/vehicles/{target['entity_ref']}").json() + assert vehicle["registration_number"] == "TST-999" + + +def test_resolve_overlap_requires_operations_manager(employee_client): + response = employee_client.post( + "/api/v1/data-quality/issues/DQ-DEMO-OVERLAP/resolve-overlap", + json={"booking_ref": "BK-DEMO-OVERLAP-A"}, + ) + assert response.status_code == 403 + + +def test_resolve_overlap_rejects_unrelated_booking(ops_client): + response = ops_client.post( + "/api/v1/data-quality/issues/DQ-DEMO-OVERLAP/resolve-overlap", + json={"booking_ref": "BK-DEMO-RETURN"}, + ) + assert response.status_code == 422 + assert response.json()["error"]["code"] == "INVALID_BOOKING_REFERENCE" + + +def test_resolve_overlap_blocks_one_booking_and_resolves(ops_client): + response = ops_client.post( + "/api/v1/data-quality/issues/DQ-DEMO-OVERLAP/resolve-overlap", + json={"booking_ref": "BK-DEMO-OVERLAP-A", "note": "Blocked the later commitment."}, + ) + assert response.status_code == 200 + assert response.json()["status"] == "resolved" + + booking = ops_client.get("/api/v1/bookings/BK-DEMO-OVERLAP-A").json() + assert booking["status"] == "blocked" + + +def test_apply_recommended_status_requires_operations_manager(employee_client): + response = employee_client.post( + "/api/v1/data-quality/issues/DQ-DEMO-STATUS/apply-recommended-status" + ) + assert response.status_code == 403 + + +def test_apply_recommended_status_resolves_conflict(ops_client): + target = _first_open(ops_client, "vehicle_status_conflict") + response = ops_client.post( + f"/api/v1/data-quality/issues/{target['public_ref']}/apply-recommended-status" + ) + assert response.status_code == 200 + body = response.json() + assert body["issue"]["status"] == "resolved" + assert body["applied_status"] + assert body["reason"] + + vehicle = ops_client.get(f"/api/v1/vehicles/{target['entity_ref']}").json() + assert vehicle["operational_status"] == body["applied_status"] + + +def test_resolve_odometer_regression_requires_operations_manager(employee_client): + response = employee_client.post( + "/api/v1/data-quality/issues/DQ-0007/resolve-odometer-regression", + json={"decision": "retain_canonical"}, + ) + assert response.status_code == 403 + + +def test_resolve_odometer_regression_correction_below_canonical_is_rejected_then_retained( + ops_client, +): + target = _first_open(ops_client, "odometer_regression") + vehicle_before = ops_client.get(f"/api/v1/vehicles/{target['entity_ref']}").json() + + too_low = ops_client.post( + f"/api/v1/data-quality/issues/{target['public_ref']}/resolve-odometer-regression", + json={ + "decision": "correct_reading", + "booking_ref": "BK-DEMO-RETURN", + "corrected_odometer_km": max(vehicle_before["odometer_km"] - 100, 0), + }, + ) + assert too_low.status_code == 422 + assert too_low.json()["error"]["code"] in ( + "CORRECTION_BELOW_CANONICAL", + "INVALID_BOOKING_REFERENCE", + ) + + # The rejected attempt must not have resolved or mutated anything. + still_open = ops_client.get(f"/api/v1/data-quality/issues/{target['public_ref']}").json() + assert still_open["status"] == "open" + + retained = ops_client.post( + f"/api/v1/data-quality/issues/{target['public_ref']}/resolve-odometer-regression", + json={"decision": "retain_canonical", "note": "Submitted reading treated as erroneous."}, + ) + assert retained.status_code == 200 + assert retained.json()["status"] == "resolved" + + vehicle_after = ops_client.get(f"/api/v1/vehicles/{target['entity_ref']}").json() + assert vehicle_after["odometer_km"] == vehicle_before["odometer_km"] + + +def test_resolve_odometer_regression_correct_reading_updates_canonical(ops_client): + # The seeded odometer_regression issues carry no related booking (CSV-only rows). + # Create a fresh one with a real related booking via a live regression return, so + # the "correct_reading" path has an actual booking_ref to target. + booking_ref = _activate_booking("MO-018", start_odometer_km=12000) + vehicle_before = ops_client.get("/api/v1/vehicles/MO-018").json() + low_reading = vehicle_before["odometer_km"] - 200 + returned = ops_client.post( + f"/api/v1/bookings/{booking_ref}/return", + json={ + "end_odometer_km": low_reading, + "fuel_level_percent": 50, + "cleanliness_ok": True, + "damage_reported": False, + "technical_warning": False, + }, + headers={"Idempotency-Key": "test-dq-odometer-correct-001"}, + ) + assert returned.status_code == 201 + issue_ref = returned.json()["quality_issue_ref"] + assert issue_ref is not None + + corrected = vehicle_before["odometer_km"] + 500 + response = ops_client.post( + f"/api/v1/data-quality/issues/{issue_ref}/resolve-odometer-regression", + json={ + "decision": "correct_reading", + "booking_ref": booking_ref, + "corrected_odometer_km": corrected, + }, + ) + assert response.status_code == 200 + assert response.json()["status"] == "resolved" + + vehicle = ops_client.get("/api/v1/vehicles/MO-018").json() + assert vehicle["odometer_km"] == corrected + booking = ops_client.get(f"/api/v1/bookings/{booking_ref}").json() + assert booking["end_odometer_km"] == corrected + + +def test_manual_scan_records_audit_event(ops_client): + scan = ops_client.post("/api/v1/data-quality/scan") + assert scan.status_code == 200 + + events = ops_client.get( + "/api/v1/audit", params={"action": "data_quality_scan_run"} + ).json() + assert len(events) >= 1 + assert "created" in events[0]["metadata"] + + +def test_rejected_issue_recurrence_links_to_prior_decision(ops_client): + # Reject an open vehicle_status_conflict issue without changing the vehicle, so the + # next scan re-detects the same unresolved condition -- it must not silently vanish + # or reopen the old row, but the new issue should stay linked to the rejection. + target = _first_open(ops_client, "vehicle_status_conflict") + rejected = ops_client.post(f"/api/v1/data-quality/issues/{target['public_ref']}/reject") + assert rejected.status_code == 200 + + rescan = ops_client.post("/api/v1/data-quality/scan") + assert rescan.status_code == 200 + assert rescan.json()["created"].get("vehicle_status_conflict", 0) >= 1 + + reopened = ops_client.get( + "/api/v1/data-quality/issues", + params={"rule_type": "vehicle_status_conflict", "status": "open"}, + ).json() + match = next( + (i for i in reopened if i["evidence"].get("reopened_from") == target["public_ref"]), None + ) + assert match is not None, "expected a new issue linked back to the rejected one" + assert match["evidence"]["previous_decision"] == "rejected"