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.
This commit is contained in:
@@ -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)
|
||||
|
||||
@@ -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)
|
||||
|
||||
@@ -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")
|
||||
|
||||
|
||||
|
||||
@@ -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"
|
||||
|
||||
Reference in New Issue
Block a user