From 6deb95524d96b5c319e2afb23fc665a144fde907 Mon Sep 17 00:00:00 2001 From: NuklearRabbit <145918611+NuklearRabbit@users.noreply.github.com> Date: Mon, 3 Aug 2026 21:37:34 +0200 Subject: [PATCH] fix: safe status-recommendation flow, MO-016 order independence, brand constant, message codes - Add a single shared, pure vehicle-status evaluator (app/services/vehicle_status.py) used identically by the data-quality scanner, a new non-mutating status-recommendation preview endpoint, and a transactional apply endpoint with optimistic-concurrency token revalidation -- eliminates the old opaque "calculate and apply" action and the unsafe "maintenance + active booking -> auto rented" shortcut. Frontend DataQualityIssueDetail.tsx now shows a review/decide/confirm panel with localized why/evidence/consequence text in nl-BE/en-GB/fr-BE, with an exact "Change status to " confirm action per the brief. - Fix MO-016 issue-order dependency: resolving the booking-overlap issue before vs. after the status-conflict issue now converges on the same final vehicle status, proven by test_mo_016_status_conflict_recommendation_is_order_independent. - Make "Fleet Ops" a non-localizable brand constant (frontend/src/product.ts, backend PRODUCT_NAME) via {{productName}} interpolation everywhere the brand name appeared in locale prose; add a permanent test guarding against a translation file ever defining the brand name or an "appName" key again. - Convert dynamic backend prose to stable message codes + params: return status reasons, audit field/actor-type labels, automation last_error, and search section/vehicle/booking/issue results all now carry codes the frontend localizes, with raw technical text demoted to a "Technical details" disclosure. - docs/fleet-ops-correction/: gap audit, i18n inventory, and the vehicle-status decision table documenting the evaluator's rules and safe-status principles. 148 backend tests + Ruff + mypy green; Alembic migration verified upgrade/downgrade; frontend tsc/build and the i18n-coverage Playwright suite green. Co-Authored-By: Claude Sonnet 5 --- .../799d8800e241_outbox_last_error_code.py | 25 ++ backend/app/api/routers/bookings.py | 2 + backend/app/api/routers/dashboard.py | 1 + backend/app/api/routers/data_quality.py | 34 ++- backend/app/api/routers/search.py | 87 ++++--- backend/app/api/routers/workflows.py | 1 + backend/app/core/config.py | 6 + backend/app/main.py | 4 +- backend/app/models/outbox.py | 5 + backend/app/schemas.py | 32 ++- backend/app/seed_loader.py | 3 + backend/app/services/data_quality.py | 219 +++++++++++------- backend/app/services/dispatcher.py | 7 + backend/app/services/returns.py | 41 +++- backend/app/services/vehicle_status.py | 218 +++++++++++++++++ backend/tests/test_data_quality.py | 122 +++++++++- backend/tests/test_dispatcher.py | 4 + backend/tests/test_search.py | 20 +- backend/tests/test_seed.py | 9 +- backend/tests/test_vehicle_status.py | 160 +++++++++++++ .../fleet-ops-correction/current-gap-audit.md | 75 ++++++ docs/fleet-ops-correction/i18n-inventory.md | 25 ++ .../vehicle-status-decision-table.md | 90 +++++++ frontend/e2e/i18n-coverage.spec.ts | 113 +++++++++ frontend/src/api/types.ts | 28 ++- frontend/src/components/DemoGuide.tsx | 3 +- frontend/src/components/Layout.tsx | 36 ++- frontend/src/components/ReturnForm.tsx | 12 +- frontend/src/i18n/locales/en-GB/audit.json | 23 ++ frontend/src/i18n/locales/en-GB/auth.json | 2 +- frontend/src/i18n/locales/en-GB/common.json | 3 +- frontend/src/i18n/locales/en-GB/demo.json | 8 +- frontend/src/i18n/locales/en-GB/fleet.json | 10 +- .../src/i18n/locales/en-GB/integrations.json | 7 + .../src/i18n/locales/en-GB/knowledge.json | 2 +- .../src/i18n/locales/en-GB/navigation.json | 12 +- frontend/src/i18n/locales/en-GB/quality.json | 45 +++- frontend/src/i18n/locales/en-GB/returns.json | 10 +- frontend/src/i18n/locales/fr-BE/audit.json | 23 ++ frontend/src/i18n/locales/fr-BE/auth.json | 2 +- frontend/src/i18n/locales/fr-BE/common.json | 3 +- frontend/src/i18n/locales/fr-BE/demo.json | 8 +- frontend/src/i18n/locales/fr-BE/fleet.json | 10 +- .../src/i18n/locales/fr-BE/integrations.json | 7 + .../src/i18n/locales/fr-BE/knowledge.json | 2 +- .../src/i18n/locales/fr-BE/navigation.json | 12 +- frontend/src/i18n/locales/fr-BE/quality.json | 45 +++- frontend/src/i18n/locales/fr-BE/returns.json | 10 +- frontend/src/i18n/locales/nl-BE/audit.json | 23 ++ frontend/src/i18n/locales/nl-BE/auth.json | 2 +- frontend/src/i18n/locales/nl-BE/common.json | 3 +- frontend/src/i18n/locales/nl-BE/demo.json | 8 +- frontend/src/i18n/locales/nl-BE/fleet.json | 10 +- .../src/i18n/locales/nl-BE/integrations.json | 7 + .../src/i18n/locales/nl-BE/knowledge.json | 2 +- .../src/i18n/locales/nl-BE/navigation.json | 12 +- frontend/src/i18n/locales/nl-BE/quality.json | 45 +++- frontend/src/i18n/locales/nl-BE/returns.json | 10 +- frontend/src/pages/AboutDemo.tsx | 7 +- frontend/src/pages/Audit.tsx | 4 +- frontend/src/pages/Automation.tsx | 16 +- frontend/src/pages/BookingDetail.tsx | 5 +- frontend/src/pages/DataQualityIssueDetail.tsx | 154 ++++++++++-- frontend/src/pages/Knowledge.tsx | 3 +- frontend/src/pages/Login.tsx | 5 +- frontend/src/pages/VehicleDetail.tsx | 4 +- frontend/src/product.ts | 5 + frontend/src/styles.css | 8 + 68 files changed, 1734 insertions(+), 225 deletions(-) create mode 100644 backend/alembic/versions/799d8800e241_outbox_last_error_code.py create mode 100644 backend/app/services/vehicle_status.py create mode 100644 backend/tests/test_vehicle_status.py create mode 100644 docs/fleet-ops-correction/current-gap-audit.md create mode 100644 docs/fleet-ops-correction/i18n-inventory.md create mode 100644 docs/fleet-ops-correction/vehicle-status-decision-table.md create mode 100644 frontend/src/product.ts diff --git a/backend/alembic/versions/799d8800e241_outbox_last_error_code.py b/backend/alembic/versions/799d8800e241_outbox_last_error_code.py new file mode 100644 index 0000000..755e05b --- /dev/null +++ b/backend/alembic/versions/799d8800e241_outbox_last_error_code.py @@ -0,0 +1,25 @@ +"""outbox last_error_code + +Revision ID: 799d8800e241 +Revises: e7b08389f47f +Create Date: 2026-08-03 10:00:00.000000 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa + +# revision identifiers, used by Alembic. +revision: str = '799d8800e241' +down_revision: Union[str, None] = 'e7b08389f47f' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + op.add_column('outbox_events', sa.Column('last_error_code', sa.String(length=60), nullable=True)) + + +def downgrade() -> None: + op.drop_column('outbox_events', 'last_error_code') diff --git a/backend/app/api/routers/bookings.py b/backend/app/api/routers/bookings.py index 1dae99f..68afa27 100644 --- a/backend/app/api/routers/bookings.py +++ b/backend/app/api/routers/bookings.py @@ -89,6 +89,8 @@ def preview_return( resulting_odometer_km=evaluation.resulting_odometer_km, resulting_vehicle_status=evaluation.resulting_vehicle_status, status_reason=evaluation.status_reason, + status_reason_code=evaluation.status_reason_code, + status_reason_params=evaluation.status_reason_params, would_create_quality_issue=evaluation.would_create_quality_issue, attention_reasons=evaluation.attention_reasons, next_booking_risk=( diff --git a/backend/app/api/routers/dashboard.py b/backend/app/api/routers/dashboard.py index 3385bec..15039c8 100644 --- a/backend/app/api/routers/dashboard.py +++ b/backend/app/api/routers/dashboard.py @@ -108,6 +108,7 @@ def get_dashboard( status=r.delivery_status, attempts=r.attempts, last_error=r.last_error, + last_error_code=r.last_error_code, occurred_at=r.occurred_at, ) for r in recent diff --git a/backend/app/api/routers/data_quality.py b/backend/app/api/routers/data_quality.py index 52f3d8c..2daeed3 100644 --- a/backend/app/api/routers/data_quality.py +++ b/backend/app/api/routers/data_quality.py @@ -11,6 +11,7 @@ from app.models.data_quality import DataQualityIssue from app.models.inspection import Inspection from app.models.vehicle import Vehicle from app.schemas import ( + ApplyRecommendedStatusRequest, ApplyRecommendedStatusResult, CurrentUser, DataQualityIssueDetailOut, @@ -21,11 +22,14 @@ from app.schemas import ( ResolveOdometerRegressionRequest, ResolveOverlapRequest, ScanResultOut, + StatusRecommendationOut, + VehicleStatusFactsOut, ) from app.services.data_quality import ( apply_recommended_status, defer_issue, merge_customers, + preview_vehicle_status_recommendation, provide_missing_fields, reject_issue, resolve_booking_overlap, @@ -241,17 +245,43 @@ def resolve_overlap( return _to_out(issue) +@router.post( + "/issues/{public_ref}/status-recommendation", response_model=StatusRecommendationOut +) +def status_recommendation( + public_ref: str, + db: Session = Depends(get_db), + user: CurrentUser = Depends(require_operations_manager), +) -> StatusRecommendationOut: + """Non-mutating preview: computes the recommendation without changing anything, + resolving no issue and writing no audit event. Safe to call repeatedly.""" + _issue, _vehicle, recommendation, token = preview_vehicle_status_recommendation(db, public_ref) + return StatusRecommendationOut( + current_status=recommendation.current_status, + recommended_status=recommendation.recommended_status, + recommendation_code=recommendation.recommendation_code, + safe_to_apply=recommendation.safe_to_apply, + manual_review_required=recommendation.manual_review_required, + facts=VehicleStatusFactsOut(**recommendation.facts.as_dict()), + blocking_reasons=recommendation.blocking_reasons, + recommendation_token=token, + ) + + @router.post( "/issues/{public_ref}/apply-recommended-status", response_model=ApplyRecommendedStatusResult ) def apply_status( public_ref: str, + body: ApplyRecommendedStatusRequest, db: Session = Depends(get_db), user: CurrentUser = Depends(require_operations_manager), ) -> ApplyRecommendedStatusResult: - issue, applied_status, reason = apply_recommended_status(db, public_ref, user) + issue, applied_status, reason_code = apply_recommended_status( + db, public_ref, user, body.recommendation_token + ) return ApplyRecommendedStatusResult( - issue=_to_out(issue), applied_status=applied_status, reason=reason + issue=_to_out(issue), applied_status=applied_status, reason_code=reason_code ) diff --git a/backend/app/api/routers/search.py b/backend/app/api/routers/search.py index c0b2287..f8aaa52 100644 --- a/backend/app/api/routers/search.py +++ b/backend/app/api/routers/search.py @@ -12,53 +12,81 @@ from app.schemas import CurrentUser, SearchResponse, SearchResultItem router = APIRouter(prefix="/api/v1/search", tags=["search"]) -# Static application sections. Manager-only sections are filtered by role, mirroring the -# same nav visibility rule Layout.tsx applies -- search must never surface a destination -# the current role can't actually reach. +# Static application sections. `id` is a stable code matching navigation.json's +# `items.*` keys -- the frontend localizes both the section label and its one-line +# detail from `id`, so no English prose is sent over the wire (search.sections. in +# every locale; see docs/fleet-ops-correction/i18n-inventory.md). Manager-only sections +# are filtered by role, mirroring the same nav visibility rule Layout.tsx applies -- +# search must never surface a destination the current role can't actually reach. _SECTIONS: list[dict] = [ { - "label": "Overview", - "detail": "Operations dashboard", + "id": "overview", "link": "/dashboard", - "terms": ["overview", "dashboard", "readiness"], + # Search terms deliberately span all three supported UI languages (not just + # English) so a query never depends on the operator's selected locale. + "terms": ["overview", "dashboard", "readiness", "overzicht", "aperçu", "tableau de bord"], }, { - "label": "Fleet", - "detail": "Vehicle registry", + "id": "fleet", "link": "/vehicles", - "terms": ["fleet", "vehicle", "vehicles"], + "terms": ["fleet", "vehicle", "vehicles", "wagenpark", "voertuig", "flotte", "véhicule"], }, { - "label": "Bookings", - "detail": "Rental bookings", + "id": "bookings", "link": "/bookings", - "terms": ["booking", "bookings", "rental"], + "terms": [ + "booking", + "bookings", + "rental", + "boeking", + "boekingen", + "verhuur", + "réservation", + "réservations", + "location", + ], }, { - "label": "Data quality", - "detail": "Quality workbench", + "id": "quality", "link": "/data-quality", - "terms": ["quality", "data quality", "issues"], + "terms": [ + "quality", + "data quality", + "issues", + "kwaliteit", + "datakwaliteit", + "problemen", + "qualité", + "problèmes", + ], "role": "operations_manager", }, { - "label": "Knowledge", - "detail": "Procedure assistant", + "id": "knowledge", "link": "/knowledge", - "terms": ["knowledge", "procedures"], + "terms": ["knowledge", "procedures", "kennis", "procedures", "connaissances", "procédures"], }, { - "label": "Integrations", - "detail": "Automation and integration status", + "id": "integrations", "link": "/automation", - "terms": ["automation", "integrations", "systems", "n8n"], + "terms": [ + "automation", + "integrations", + "systems", + "n8n", + "automatisering", + "integraties", + "systemen", + "automatisation", + "intégrations", + "systèmes", + ], "role": "operations_manager", }, { - "label": "Audit trail", - "detail": "Audit history", + "id": "audit", "link": "/audit", - "terms": ["audit", "history"], + "terms": ["audit", "history", "geschiedenis", "historique"], "role": "operations_manager", }, ] @@ -83,8 +111,8 @@ def search( results.append( SearchResultItem( type="section", - label=section["label"], - detail=section["detail"], + label=section["id"], + detail_code=section["id"], link=section["link"], ) ) @@ -108,7 +136,8 @@ def search( SearchResultItem( type="vehicle", label=v.public_ref, - detail=f"{v.make} {v.model} · {v.location}", + detail_code="vehicleSummary", + detail_params={"make": v.make, "model": v.model, "location": v.location}, link=f"/vehicles/{v.public_ref}", ) ) @@ -120,7 +149,7 @@ def search( SearchResultItem( type="booking", label=b.public_ref, - detail=b.status, + detail_code=b.status, link=f"/bookings/{b.public_ref}", ) ) @@ -138,7 +167,7 @@ def search( SearchResultItem( type="data_quality_issue", label=i.public_ref, - detail=i.rule_type.replace("_", " "), + detail_code=i.rule_type, link=f"/data-quality/{i.public_ref}", ) ) diff --git a/backend/app/api/routers/workflows.py b/backend/app/api/routers/workflows.py index 76ce9c9..736fcec 100644 --- a/backend/app/api/routers/workflows.py +++ b/backend/app/api/routers/workflows.py @@ -23,6 +23,7 @@ def _to_out(event: OutboxEvent) -> AutomationRunOut: status=event.delivery_status, attempts=event.attempts, last_error=event.last_error, + last_error_code=event.last_error_code, occurred_at=event.occurred_at, ) diff --git a/backend/app/core/config.py b/backend/app/core/config.py index 62cb07a..fc2cd59 100644 --- a/backend/app/core/config.py +++ b/backend/app/core/config.py @@ -2,6 +2,12 @@ from functools import lru_cache from pydantic_settings import BaseSettings, SettingsConfigDict +# The visible product name is fixed and never translated or configured per-deployment -- +# see docs/fleet-ops-correction/current-gap-audit.md section 1. Internal identifiers +# (package name, Compose project, database name, repository) intentionally remain +# "mobilityops"; this constant is only for user-facing surfaces (e.g. the OpenAPI title). +PRODUCT_NAME = "Fleet Ops" + class Settings(BaseSettings): model_config = SettingsConfigDict(env_file=".env", extra="ignore") diff --git a/backend/app/main.py b/backend/app/main.py index 6d9d51e..a44449e 100644 --- a/backend/app/main.py +++ b/backend/app/main.py @@ -19,7 +19,7 @@ from app.api.routers import ( vehicles, workflows, ) -from app.core.config import get_settings +from app.core.config import PRODUCT_NAME, get_settings from app.core.errors import AppError, error_body from app.services.dispatcher import start_background_dispatcher, stop_background_dispatcher @@ -33,7 +33,7 @@ async def lifespan(_app: FastAPI): stop_background_dispatcher() -app = FastAPI(title="MobilityOps API", version="0.1.0", lifespan=lifespan) +app = FastAPI(title=f"{PRODUCT_NAME} API", version="0.1.0", lifespan=lifespan) app.add_middleware( CORSMiddleware, diff --git a/backend/app/models/outbox.py b/backend/app/models/outbox.py index 1d63a75..e3d2cf4 100644 --- a/backend/app/models/outbox.py +++ b/backend/app/models/outbox.py @@ -26,4 +26,9 @@ class OutboxEvent(TimestampMixin, Base): attempts: Mapped[int] = mapped_column(Integer, nullable=False, default=0) next_attempt_at: Mapped[datetime | None] = mapped_column(DateTime(timezone=True)) last_error: Mapped[str | None] = mapped_column(Text) + # Stable, localizable classification of last_error -- the frontend renders a + # localized summary from this code as the primary text and shows last_error itself + # only under "Technical details" (section 10 of docs/fleet-ops-correction/ + # current-gap-audit.md). Kept alongside the raw message for backward compatibility. + last_error_code: Mapped[str | None] = mapped_column(String(60)) external_run_id: Mapped[str | None] = mapped_column(String(120)) diff --git a/backend/app/schemas.py b/backend/app/schemas.py index 00c8bfc..5c2ef51 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -83,6 +83,8 @@ class ReturnPreviewResult(BaseModel): resulting_odometer_km: int resulting_vehicle_status: str status_reason: str + status_reason_code: str + status_reason_params: dict[str, str | int] = {} would_create_quality_issue: bool attention_reasons: list[str] next_booking_risk: NextBookingRisk | None @@ -157,16 +159,41 @@ class ResolveOverlapRequest(BaseModel): note: str | None = Field(default=None, max_length=500) +class VehicleStatusFactsOut(BaseModel): + active_booking_refs: list[str] + overlapping_booking_pairs: list[list[str]] + service_threshold_reached: bool + odometer_km: int + next_service_km: int + open_booking_overlap_issue_ref: str | None = None + + +class StatusRecommendationOut(BaseModel): + current_status: str + recommended_status: str | None + recommendation_code: str + safe_to_apply: bool + manual_review_required: bool + facts: VehicleStatusFactsOut + blocking_reasons: list[str] + recommendation_token: str + + +class ApplyRecommendedStatusRequest(BaseModel): + recommendation_token: str + + class ApplyRecommendedStatusResult(BaseModel): issue: DataQualityIssueOut applied_status: str - reason: str + reason_code: str class SearchResultItem(BaseModel): type: Literal["vehicle", "booking", "data_quality_issue", "section"] label: str - detail: str + detail_code: str + detail_params: dict[str, str] = {} link: str @@ -269,6 +296,7 @@ class AutomationRunOut(BaseModel): status: str attempts: int last_error: str | None + last_error_code: str | None occurred_at: datetime diff --git a/backend/app/seed_loader.py b/backend/app/seed_loader.py index 938d219..be1bef3 100644 --- a/backend/app/seed_loader.py +++ b/backend/app/seed_loader.py @@ -287,6 +287,9 @@ def load_seed(db: Session) -> SeedResult: "attempts": int(row["attempts"]), "next_attempt_at": None, "last_error": row["last_error"] or None, + # The seed dataset's one synthetic failure (BK-H-0020) models a + # connection-timeout-style delivery failure -- see workflow_runs.csv. + "last_error_code": "connectionError" if row["last_error"] else None, "external_run_id": None, } ) diff --git a/backend/app/services/data_quality.py b/backend/app/services/data_quality.py index e8610d8..c2077ff 100644 --- a/backend/app/services/data_quality.py +++ b/backend/app/services/data_quality.py @@ -15,6 +15,13 @@ from app.models.data_quality import DataQualityIssue from app.models.vehicle import Vehicle from app.schemas import CurrentUser, ResolveOdometerRegressionRequest from app.services.audit import record_audit_event +from app.services.vehicle_status import ( + RECOMMENDATION_CODE_NO_CONFLICT, + VehicleStatusRecommendation, + compute_recommendation_token, + evaluate_vehicle_status, + gather_vehicle_status_facts, +) REQUIRED_CUSTOMER_FIELDS = ("first_name", "last_name") REQUIRED_VEHICLE_FIELDS = ("registration_number", "make", "model", "location") @@ -69,6 +76,7 @@ def _open_issue( summary: str, entity_ref: str, related_refs: list[str], + signals: list[dict] | None = None, ) -> None: if _has_open_issue(db, rule_type, entity_type, entity_id): return @@ -87,10 +95,14 @@ def _open_issue( ) .order_by(DataQualityIssue.detected_at.desc()) ) + # `summary` is kept as a technical-fallback string (shown only under "Technical + # details"); `signals` is the stable, localizable structure the frontend renders as + # the primary evidence -- see docs/fleet-ops-correction/current-gap-audit.md §2/§6. evidence: dict = { "summary": summary, "entity_ref": entity_ref, "related_refs": related_refs, + "signals": signals or [], } if previous is not None: evidence["reopened_from"] = previous.public_ref @@ -121,22 +133,29 @@ def _scan_duplicate_customers(db: Session, scan: ScanResult) -> None: for i, a in enumerate(customers): for b in customers[i + 1 :]: score = 0 - signals = [] + signals: list[dict] = [] + summary_parts: list[str] = [] if _normalize(a.email) and _normalize(a.email) == _normalize(b.email): score += 60 - signals.append("exact email") + signals.append({"code": "duplicate.exact_email"}) + summary_parts.append("exact email") if _normalize(a.phone) and _normalize(a.phone) == _normalize(b.phone): score += 50 - signals.append("exact phone") + signals.append({"code": "duplicate.exact_phone"}) + summary_parts.append("exact phone") if _normalize(a.postal_code) and _normalize(a.postal_code) == _normalize(b.postal_code): score += 10 - signals.append("exact postal code") + signals.append({"code": "duplicate.same_postal_code"}) + summary_parts.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") + signals.append( + {"code": "duplicate.similar_name", "params": {"score": round(ratio, 2)}} + ) + summary_parts.append("similar name") if score >= DUPLICATE_THRESHOLD: _open_issue( @@ -146,9 +165,10 @@ def _scan_duplicate_customers(db: Session, scan: ScanResult) -> None: entity_type="customer", entity_id=a.id, severity="high", - summary="; ".join(signals) + f" (score {score})", + summary="; ".join(summary_parts) + f" (score {score})", entity_ref=a.public_ref, related_refs=[b.public_ref], + signals=signals, ) @@ -170,6 +190,7 @@ def _scan_missing_required_fields(db: Session, scan: ScanResult) -> None: summary=f"Missing: {', '.join(missing)}", entity_ref=customer.public_ref, related_refs=[], + signals=[{"code": "missing_field", "params": {"field": f}} for f in missing], ) for vehicle in db.scalars(select(Vehicle).where(Vehicle.active.is_(True))).all(): @@ -185,6 +206,7 @@ def _scan_missing_required_fields(db: Session, scan: ScanResult) -> None: summary=f"Missing: {', '.join(missing)}", entity_ref=vehicle.public_ref, related_refs=[], + signals=[{"code": "missing_field", "params": {"field": f}} for f in missing], ) @@ -213,50 +235,47 @@ def _scan_booking_overlaps(db: Session, scan: ScanResult) -> None: summary=f"Overlapping bookings {first.public_ref} and {second.public_ref}", entity_ref=vehicle.public_ref, related_refs=[first.public_ref, second.public_ref], + signals=[ + { + "code": "overlap.reserved_bookings", + "params": {"refs": [first.public_ref, second.public_ref]}, + } + ], ) def _scan_vehicle_status_conflicts(db: Session, scan: ScanResult) -> None: + # Uses the same shared evaluator as the preview/apply flow (app.services.vehicle_status) + # so detection and resolution can never structurally disagree -- see + # docs/fleet-ops-correction/vehicle-status-decision-table.md. 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" + facts = gather_vehicle_status_facts(db, vehicle) + recommendation = evaluate_vehicle_status(vehicle, facts) + if recommendation.recommendation_code == RECOMMENDATION_CODE_NO_CONFLICT: + continue - 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=[], - ) + signals = [{"code": recommendation.recommendation_code, "params": facts.as_dict()}] + summary = ( + f"Recommended status: {recommendation.recommended_status}" + if recommendation.recommended_status + else "Manual review required: active rental conflicts with a blocking condition" + ) + _open_issue( + db, + scan, + rule_type="vehicle_status_conflict", + entity_type="vehicle", + entity_id=vehicle.id, + severity="high", + summary=summary, + entity_ref=vehicle.public_ref, + related_refs=[ + *facts.active_booking_refs, + *(ref for pair in facts.overlapping_booking_pairs for ref in pair), + ], + signals=signals, + ) def _scan_odometer_regressions(db: Session, scan: ScanResult) -> None: @@ -295,6 +314,17 @@ def _scan_odometer_regressions(db: Session, scan: ScanResult) -> None: ), entity_ref=vehicle.public_ref, related_refs=[earlier.public_ref, later.public_ref], + signals=[ + { + "code": "odometer.regression", + "params": { + "later_ref": later.public_ref, + "later_km": later.end_odometer_km, + "earlier_ref": earlier.public_ref, + "earlier_km": earlier.end_odometer_km, + }, + } + ], ) break @@ -650,28 +680,7 @@ def resolve_booking_overlap( 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]: +def _load_vehicle_status_conflict_issue(db: Session, public_ref: str) -> DataQualityIssue: issue = _load_open_issue(db, public_ref) if issue.rule_type != "vehicle_status_conflict": raise AppError( @@ -679,47 +688,77 @@ def apply_recommended_status( "This issue is not a vehicle_status_conflict issue.", status_code=409, ) + return issue + + +def preview_vehicle_status_recommendation( + db: Session, public_ref: str +) -> tuple[DataQualityIssue, Vehicle, VehicleStatusRecommendation, str]: + """Non-mutating: computes and returns the recommendation only. Never resolves the + issue, never writes an audit event, never queues automation -- safe to call as often + as the UI needs (e.g. every time the panel is opened) with zero side effects.""" + issue = _load_vehicle_status_conflict_issue(db, public_ref) + vehicle = db.scalar(select(Vehicle).where(Vehicle.id == issue.entity_id)) + if vehicle is None: + raise AppError( + "VEHICLE_NOT_FOUND", "The vehicle for this issue was not found.", status_code=404 + ) + facts = gather_vehicle_status_facts(db, vehicle, exclude_issue_id=issue.id) + recommendation = evaluate_vehicle_status(vehicle, facts) + token = compute_recommendation_token(vehicle, facts) + return issue, vehicle, recommendation, token + + +def apply_recommended_status( + db: Session, public_ref: str, actor: CurrentUser, expected_token: str +) -> tuple[DataQualityIssue, str, str]: + issue = _load_vehicle_status_conflict_issue(db, public_ref) + # Lock the vehicle row for the remainder of this transaction so a concurrent apply + # (or return/checkout) can't race between our fact-gathering and the write below. 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") + facts = gather_vehicle_status_facts(db, vehicle, exclude_issue_id=issue.id) + recommendation = evaluate_vehicle_status(vehicle, facts) + current_token = compute_recommendation_token(vehicle, facts) + + if current_token != expected_token: + raise AppError( + "RECOMMENDATION_STALE", + "The underlying facts changed since this recommendation was shown; " + "review the recommendation again before applying it.", + status_code=409, ) - 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, - ) + if recommendation.manual_review_required or not recommendation.safe_to_apply: + raise AppError( + "MANUAL_REVIEW_REQUIRED", + "This vehicle's state requires manual review; no automatic status change is safe.", + status_code=409, ) - is not None - ) - recommendation = _recommend_vehicle_status( - vehicle.operational_status, has_active_booking, has_open_high_issue - ) - if recommendation is None: + if recommendation.recommended_status is None: raise AppError( "NO_CONFLICT_DETECTED", "The current vehicle state no longer conflicts; nothing to apply.", status_code=409, ) - new_status, reason = recommendation + new_status = recommendation.recommended_status + reason_code = recommendation.recommendation_code 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: + # Re-validate against the same shared evaluator, over freshly-gathered facts, that + # applying this change actually leaves no conflict -- never trust the pre-computed + # recommendation alone for the post-condition. + post_facts = gather_vehicle_status_facts(db, vehicle, exclude_issue_id=issue.id) + post_check = evaluate_vehicle_status(vehicle, post_facts) + if post_check.recommendation_code not in ( + RECOMMENDATION_CODE_NO_CONFLICT, + ): raise AppError( "CONFLICT_STILL_PRESENT", "Applying the recommended status did not resolve the conflict.", @@ -737,7 +776,7 @@ def apply_recommended_status( correlation_id=correlation_id, before=before, after={"operational_status": vehicle.operational_status}, - metadata={"issue_ref": issue.public_ref, "reason": reason}, + metadata={"issue_ref": issue.public_ref, "reason_code": reason_code}, ) issue.status = "resolved" @@ -755,7 +794,7 @@ def apply_recommended_status( after={"status": "resolved"}, ) db.commit() - return issue, new_status, reason + return issue, new_status, reason_code MERGEABLE_FIELDS = ("first_name", "last_name", "email", "phone", "postal_code", "city") diff --git a/backend/app/services/dispatcher.py b/backend/app/services/dispatcher.py index c401002..66ff1b5 100644 --- a/backend/app/services/dispatcher.py +++ b/backend/app/services/dispatcher.py @@ -48,6 +48,7 @@ def _reclaim_stale_deliveries(batch_size: int = 10) -> int: f"(no outcome recorded within {settings.n8n_delivery_lease_seconds:.0f}s; " f"the process likely crashed mid-delivery). attempts preserved at {row.attempts}." )[:2000] + row.last_error_code = "staleLeaseRecovered" db.commit() return len(rows) finally: @@ -111,8 +112,10 @@ def _deliver_one(event_id: uuid.UUID) -> None: finally: db.close() + error_code: str | None if wire_event is None: success, error, body = False, payload_error, None + error_code = "malformedPayload" else: try: response = httpx.post( @@ -124,9 +127,11 @@ def _deliver_one(event_id: uuid.UUID) -> None: body = response.json() success = bool(body.get("ok", True)) error = None if success else f"n8n reported failure: {body}" + error_code = None if success else "remoteReportedFailure" except httpx.HTTPError as exc: success = False error = f"{type(exc).__name__}: {exc}" + error_code = "connectionError" body = None db = SessionLocal() @@ -138,10 +143,12 @@ def _deliver_one(event_id: uuid.UUID) -> None: if success: event.delivery_status = "succeeded" event.last_error = None + event.last_error_code = None event.next_attempt_at = None event.external_run_id = str((body or {}).get("event_id", event_id)) else: event.last_error = (error or "delivery failed")[:2000] + event.last_error_code = error_code or "unknownError" if event.attempts >= settings.n8n_max_attempts: event.delivery_status = "failed" event.next_attempt_at = None diff --git a/backend/app/services/returns.py b/backend/app/services/returns.py index 0f2bcf8..b5e578b 100644 --- a/backend/app/services/returns.py +++ b/backend/app/services/returns.py @@ -28,19 +28,41 @@ def _next_public_ref(db: Session) -> str: def _derive_vehicle_status_with_reason( body: RegisterReturnRequest, vehicle: Vehicle, new_odometer: int -) -> tuple[str, str]: +) -> tuple[str, str, dict[str, str | int]]: + # Stable, localizable codes + params -- the backend never emits prose here. The + # frontend renders review.reasonCodes. in the selected locale; the mirrored + # raw-English fallback strings live only in status_reason (shown under "Technical + # details") for backward compatibility. See docs/fleet-ops-correction/i18n-inventory.md. if body.damage_reported and body.technical_warning: - return "blocked", "Damage and a technical warning were both reported on return." + return ( + "blocked", + "returnBlockedDamageAndTechnical", + {}, + ) if body.damage_reported: - return "blocked", "Damage was reported on return." + return "blocked", "returnBlockedDamage", {} if body.technical_warning: - return "blocked", "A technical warning was reported on return." + return "blocked", "returnBlockedTechnicalWarning", {} if new_odometer >= vehicle.next_service_km: return ( "maintenance", - f"Odometer reached the {vehicle.next_service_km:,} km service threshold.", + "returnServiceThresholdReached", + {"threshold_km": vehicle.next_service_km}, ) - return "cleaning", "No damage, technical warning or service threshold; routed to cleaning." + return "cleaning", "returnRoutedToCleaning", {} + + +_STATUS_REASON_FALLBACK_TEXT: dict[str, str] = { + "returnBlockedDamageAndTechnical": ( + "Damage and a technical warning were both reported on return." + ), + "returnBlockedDamage": "Damage was reported on return.", + "returnBlockedTechnicalWarning": "A technical warning was reported on return.", + "returnServiceThresholdReached": "Odometer reached the service threshold.", + "returnRoutedToCleaning": ( + "No damage, technical warning or service threshold; routed to cleaning." + ), +} @dataclass @@ -51,6 +73,8 @@ class ReturnEvaluation: resulting_odometer_km: int resulting_vehicle_status: str status_reason: str + status_reason_code: str + status_reason_params: dict[str, str | int] would_create_quality_issue: bool attention_reasons: list[str] next_booking_risk: dict | None @@ -64,9 +88,10 @@ def evaluate_return( preview and commit can never drift apart.""" odometer_regression = body.end_odometer_km < vehicle.odometer_km resulting_odometer_km = vehicle.odometer_km if odometer_regression else body.end_odometer_km - resulting_status, status_reason = _derive_vehicle_status_with_reason( + resulting_status, status_reason_code, status_reason_params = _derive_vehicle_status_with_reason( body, vehicle, resulting_odometer_km ) + status_reason = _STATUS_REASON_FALLBACK_TEXT[status_reason_code] attention_reasons = [] if body.damage_reported: @@ -101,6 +126,8 @@ def evaluate_return( resulting_odometer_km=resulting_odometer_km, resulting_vehicle_status=resulting_status, status_reason=status_reason, + status_reason_code=status_reason_code, + status_reason_params=status_reason_params, would_create_quality_issue=odometer_regression, attention_reasons=attention_reasons, next_booking_risk=next_booking_risk, diff --git a/backend/app/services/vehicle_status.py b/backend/app/services/vehicle_status.py new file mode 100644 index 0000000..8eba07e --- /dev/null +++ b/backend/app/services/vehicle_status.py @@ -0,0 +1,218 @@ +"""The single authoritative vehicle-status evaluator. + +Used by the data-quality scanner (detection), the status-recommendation preview +endpoint, the apply endpoint, and tests -- so scan-time detection and resolve-time +recommendation can never structurally disagree (see docs/fleet-ops-correction/ +vehicle-status-decision-table.md for the full decision table and rationale). + +The evaluator only ever reasons from real, freshly-queried domain facts (an actually +active rental, a real service-threshold breach, a real overlapping-booking conflict) -- +never from a proxy like "does some other high-severity issue happen to be open". It is +therefore also order-independent: resolving, deferring or rejecting an unrelated issue on +the same vehicle never changes what this function returns, because it never looks at +issue history, only at the vehicle's/bookings' current state. +""" + +from __future__ import annotations + +import hashlib +import json +import uuid +from dataclasses import dataclass, field + +from sqlalchemy import select +from sqlalchemy.orm import Session + +from app.models.booking import Booking +from app.models.data_quality import DataQualityIssue +from app.models.vehicle import Vehicle + +# Every code below is a stable, localizable identifier -- see +# frontend/src/i18n/messageCodes.ts and quality:statusRecommendation.codes.* for the +# human-language mapping in all three supported locales. The backend never emits prose. +RECOMMENDATION_CODE_ACTIVE_RENTAL = "vehicle.active_rental" +RECOMMENDATION_CODE_SERVICE_THRESHOLD = "vehicle.service_threshold_reached" +RECOMMENDATION_CODE_BOOKING_CONFLICT = "vehicle.booking_conflict" +RECOMMENDATION_CODE_RENTAL_ENDED = "vehicle.rental_ended" +RECOMMENDATION_CODE_MANUAL_REVIEW = "vehicle.manual_review_required" +RECOMMENDATION_CODE_NO_CONFLICT = "vehicle.no_conflict" + + +@dataclass +class VehicleStatusFacts: + active_booking_refs: list[str] = field(default_factory=list) + overlapping_booking_pairs: list[tuple[str, str]] = field(default_factory=list) + service_threshold_reached: bool = False + odometer_km: int = 0 + next_service_km: int = 0 + open_booking_overlap_issue_ref: str | None = None + + @property + def has_active_rental(self) -> bool: + return len(self.active_booking_refs) > 0 + + @property + def has_booking_conflict(self) -> bool: + return ( + len(self.overlapping_booking_pairs) > 0 + or self.open_booking_overlap_issue_ref is not None + ) + + def as_dict(self) -> dict: + return { + "active_booking_refs": self.active_booking_refs, + "overlapping_booking_pairs": [list(pair) for pair in self.overlapping_booking_pairs], + "service_threshold_reached": self.service_threshold_reached, + "odometer_km": self.odometer_km, + "next_service_km": self.next_service_km, + "open_booking_overlap_issue_ref": self.open_booking_overlap_issue_ref, + } + + +@dataclass +class VehicleStatusRecommendation: + current_status: str + recommended_status: str | None + recommendation_code: str + safe_to_apply: bool + manual_review_required: bool + facts: VehicleStatusFacts + blocking_reasons: list[str] + + +def _overlapping_booking_pairs(bookings: list[Booking]) -> list[tuple[Booking, Booking]]: + ordered = sorted(bookings, key=lambda b: b.starts_at) + pairs: list[tuple[Booking, Booking]] = [] + for i, first in enumerate(ordered): + for second in ordered[i + 1 :]: + if second.starts_at < first.ends_at and first.starts_at < second.ends_at: + pairs.append((first, second)) + return pairs + + +def gather_vehicle_status_facts( + db: Session, vehicle: Vehicle, *, exclude_issue_id: uuid.UUID | None = None +) -> VehicleStatusFacts: + """Real, freshly-queried facts only -- see module docstring. Never cached, never + derived from another issue's mere existence (only a *specific* booking_overlap + issue's presence is used, as a cross-reference to that issue's own public_ref).""" + reserved_or_active = list( + db.scalars( + select(Booking).where( + Booking.vehicle_id == vehicle.id, + Booking.status.in_(["reserved", "active"]), + ) + ).all() + ) + active_refs = [b.public_ref for b in reserved_or_active if b.status == "active"] + overlap_pairs = [ + (a.public_ref, b.public_ref) for a, b in _overlapping_booking_pairs(reserved_or_active) + ] + + overlap_issue_query = select(DataQualityIssue.public_ref).where( + DataQualityIssue.entity_type == "vehicle", + DataQualityIssue.entity_id == vehicle.id, + DataQualityIssue.status == "open", + DataQualityIssue.rule_type == "booking_overlap", + ) + if exclude_issue_id is not None: + overlap_issue_query = overlap_issue_query.where(DataQualityIssue.id != exclude_issue_id) + open_overlap_ref = db.scalar(overlap_issue_query) + + return VehicleStatusFacts( + active_booking_refs=active_refs, + overlapping_booking_pairs=overlap_pairs, + service_threshold_reached=vehicle.odometer_km >= vehicle.next_service_km, + odometer_km=vehicle.odometer_km, + next_service_km=vehicle.next_service_km, + open_booking_overlap_issue_ref=open_overlap_ref, + ) + + +def compute_recommendation_token(vehicle: Vehicle, facts: VehicleStatusFacts) -> str: + """A short digest of exactly the facts the recommendation was based on, plus the + vehicle's optimistic-lock version. The apply endpoint recomputes this from fresh + facts and rejects the request if it doesn't match the token the client last saw -- + the frontend must never assume a previously-shown preview is still valid without the + server re-checking it (see docs/fleet-ops-correction/current-gap-audit.md §8F).""" + payload = {"version": vehicle.version, "status": vehicle.operational_status, **facts.as_dict()} + digest = hashlib.sha256(json.dumps(payload, sort_keys=True, default=str).encode()).hexdigest() + return digest[:16] + + +def evaluate_vehicle_status( + vehicle: Vehicle, facts: VehicleStatusFacts +) -> VehicleStatusRecommendation: + """Pure decision logic over already-gathered facts -- see + docs/fleet-ops-correction/vehicle-status-decision-table.md. Never mutates anything, + never queries the database itself (call gather_vehicle_status_facts first), so it is + trivial to unit-test every branch in isolation.""" + current = vehicle.operational_status + blocking_reasons: list[str] = [] + if facts.service_threshold_reached: + blocking_reasons.append(RECOMMENDATION_CODE_SERVICE_THRESHOLD) + if facts.has_booking_conflict: + blocking_reasons.append(RECOMMENDATION_CODE_BOOKING_CONFLICT) + if current == "maintenance" and RECOMMENDATION_CODE_SERVICE_THRESHOLD not in blocking_reasons: + # Already being in maintenance is itself a real blocking fact -- an active + # booking never overrides it. This is exactly the forbidden shortcut this + # evaluator must never take (maintenance + active booking -> auto "rented"). + blocking_reasons.append(RECOMMENDATION_CODE_SERVICE_THRESHOLD) + + def result( + recommended: str | None, code: str, *, safe: bool, manual: bool + ) -> VehicleStatusRecommendation: + return VehicleStatusRecommendation( + current_status=current, + recommended_status=recommended, + recommendation_code=code, + safe_to_apply=safe, + manual_review_required=manual, + facts=facts, + blocking_reasons=blocking_reasons, + ) + + if facts.has_active_rental and not blocking_reasons: + if current == "rented": + return result(None, RECOMMENDATION_CODE_NO_CONFLICT, safe=False, manual=False) + return result( + "rented", RECOMMENDATION_CODE_ACTIVE_RENTAL, safe=True, manual=False + ) + + if facts.has_active_rental and blocking_reasons: + # Explicitly forbidden shortcut this evaluator must never take: an active + # booking is not proof the vehicle should be "rented" when a real blocking + # condition also exists (e.g. maintenance-due, or a genuine booking conflict). + # This is a real contradiction in the underlying facts, not something safe to + # resolve automatically. + return result(None, RECOMMENDATION_CODE_MANUAL_REVIEW, safe=False, manual=True) + + if facts.service_threshold_reached: + if current == "maintenance": + return result(None, RECOMMENDATION_CODE_NO_CONFLICT, safe=False, manual=False) + return result( + "maintenance", RECOMMENDATION_CODE_SERVICE_THRESHOLD, safe=True, manual=False + ) + + if facts.has_booking_conflict: + if current == "blocked": + return result(None, RECOMMENDATION_CODE_NO_CONFLICT, safe=False, manual=False) + return result( + "blocked", RECOMMENDATION_CODE_BOOKING_CONFLICT, safe=True, manual=False + ) + + # No active rental, no maintenance need, no booking conflict. + if current in ("available", "cleaning", "blocked"): + return result(None, RECOMMENDATION_CODE_NO_CONFLICT, safe=False, manual=False) + if current == "rented": + return result( + "available", RECOMMENDATION_CODE_RENTAL_ENDED, safe=True, manual=False + ) + if current == "maintenance": + # No positive fact confirms maintenance is actually finished (no completed + # service record is tracked here) -- clearing "maintenance" without such a + # fact would be exactly the kind of unsafe shortcut this evaluator forbids. + # Releasing a vehicle from maintenance remains an explicit, manual decision. + return result(None, RECOMMENDATION_CODE_NO_CONFLICT, safe=False, manual=False) + + return result(None, RECOMMENDATION_CODE_MANUAL_REVIEW, safe=False, manual=True) diff --git a/backend/tests/test_data_quality.py b/backend/tests/test_data_quality.py index c995b35..b5da793 100644 --- a/backend/tests/test_data_quality.py +++ b/backend/tests/test_data_quality.py @@ -249,28 +249,74 @@ def test_resolve_overlap_blocks_one_booking_and_resolves(ops_client): assert booking["status"] == "blocked" -def test_apply_recommended_status_requires_operations_manager(employee_client): +def test_status_recommendation_requires_operations_manager(employee_client): response = employee_client.post( - "/api/v1/data-quality/issues/DQ-DEMO-STATUS/apply-recommended-status" + "/api/v1/data-quality/issues/DQ-DEMO-STATUS/status-recommendation" ) assert response.status_code == 403 +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", + json={"recommendation_token": "irrelevant"}, + ) + assert response.status_code == 403 + + +def test_status_recommendation_preview_does_not_mutate_anything(ops_client): + target = _first_open(ops_client, "vehicle_status_conflict") + vehicle_before = ops_client.get(f"/api/v1/vehicles/{target['entity_ref']}").json() + + preview_response = ops_client.post( + f"/api/v1/data-quality/issues/{target['public_ref']}/status-recommendation" + ) + assert preview_response.status_code == 200 + preview = preview_response.json() + assert preview["current_status"] == vehicle_before["operational_status"] + assert preview["recommendation_token"] + assert "facts" in preview + + # Calling preview again (as the UI would on every open) must still not mutate. + ops_client.post(f"/api/v1/data-quality/issues/{target['public_ref']}/status-recommendation") + issue_after = ops_client.get(f"/api/v1/data-quality/issues/{target['public_ref']}").json() + vehicle_after = ops_client.get(f"/api/v1/vehicles/{target['entity_ref']}").json() + assert issue_after["status"] == "open" + assert vehicle_after["operational_status"] == vehicle_before["operational_status"] + + def test_apply_recommended_status_resolves_conflict(ops_client): target = _first_open(ops_client, "vehicle_status_conflict") + preview = ops_client.post( + f"/api/v1/data-quality/issues/{target['public_ref']}/status-recommendation" + ).json() + assert preview["safe_to_apply"] is True + assert preview["manual_review_required"] is False + response = ops_client.post( - f"/api/v1/data-quality/issues/{target['public_ref']}/apply-recommended-status" + f"/api/v1/data-quality/issues/{target['public_ref']}/apply-recommended-status", + json={"recommendation_token": preview["recommendation_token"]}, ) assert response.status_code == 200 body = response.json() assert body["issue"]["status"] == "resolved" - assert body["applied_status"] - assert body["reason"] + assert body["applied_status"] == preview["recommended_status"] + assert body["reason_code"] == preview["recommendation_code"] vehicle = ops_client.get(f"/api/v1/vehicles/{target['entity_ref']}").json() assert vehicle["operational_status"] == body["applied_status"] +def test_apply_recommended_status_rejects_stale_token(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", + json={"recommendation_token": "not-a-real-token"}, + ) + assert response.status_code == 409 + assert response.json()["error"]["code"] == "RECOMMENDATION_STALE" + + def test_resolve_odometer_regression_requires_operations_manager(employee_client): response = employee_client.post( "/api/v1/data-quality/issues/DQ-0007/resolve-odometer-regression", @@ -365,6 +411,72 @@ def test_manual_scan_records_audit_event(ops_client): assert "created" in events[0]["metadata"] +def _reset_demo(ops_client) -> None: + # /api/v1/demo/reset deletes the session cookie (the reset recreates the users + # table, so the old session's user id no longer exists) -- the caller must log back + # in before making any further authenticated call with the same client. + response = ops_client.post("/api/v1/demo/reset") + assert response.status_code == 200, response.text + login_response = ops_client.post( + "/api/v1/demo/login", json={"role": "operations_manager"} + ) + assert login_response.status_code == 200, login_response.text + + +def _resolve_overlap_issue(ops_client, *, booking_to_block: str) -> None: + overlap = _first_open(ops_client, "booking_overlap") + response = ops_client.post( + f"/api/v1/data-quality/issues/{overlap['public_ref']}/resolve-overlap", + json={"booking_ref": booking_to_block}, + ) + assert response.status_code == 200, response.text + assert response.json()["status"] == "resolved" + + +def _apply_status_recommendation(ops_client, public_ref: str) -> dict: + preview = ops_client.post( + f"/api/v1/data-quality/issues/{public_ref}/status-recommendation" + ).json() + apply_response = ops_client.post( + f"/api/v1/data-quality/issues/{public_ref}/apply-recommended-status", + json={"recommendation_token": preview["recommendation_token"]}, + ) + assert apply_response.status_code == 200, apply_response.text + return apply_response.json() + + +def test_mo_016_status_conflict_recommendation_is_order_independent(ops_client): + # MO-016 carries both a booking_overlap (DQ-DEMO-OVERLAP) and a vehicle_status_conflict + # (DQ-DEMO-STATUS) issue at once -- resolving them in either order must land the + # vehicle in the same final, safe state (see docs/fleet-ops-correction/ + # current-gap-audit.md §6-7 and vehicle-status-decision-table.md). + + # Order A: resolve the booking overlap first, then the status conflict. + _reset_demo(ops_client) + _resolve_overlap_issue(ops_client, booking_to_block="BK-DEMO-OVERLAP-B") + status_issue_a = _first_open(ops_client, "vehicle_status_conflict") + result_a = _apply_status_recommendation(ops_client, status_issue_a["public_ref"]) + vehicle_a = ops_client.get("/api/v1/vehicles/MO-016").json() + + # Order B: resolve the status conflict first, then the booking overlap. + _reset_demo(ops_client) + status_issue_b = _first_open(ops_client, "vehicle_status_conflict") + result_b = _apply_status_recommendation(ops_client, status_issue_b["public_ref"]) + _resolve_overlap_issue(ops_client, booking_to_block="BK-DEMO-OVERLAP-B") + vehicle_b = ops_client.get("/api/v1/vehicles/MO-016").json() + + assert result_a["applied_status"] == result_b["applied_status"], ( + "resolving the booking overlap before vs. after the status conflict recommended " + "a different status -- the recommendation must not depend on issue order" + ) + assert vehicle_a["operational_status"] == vehicle_b["operational_status"] + # Never auto-"available" and never auto-"rented" as a side effect of this scenario -- + # MO-016 has no active rental in either order, only reserved bookings. + assert vehicle_a["operational_status"] not in ("rented",) + + _reset_demo(ops_client) + + 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 diff --git a/backend/tests/test_dispatcher.py b/backend/tests/test_dispatcher.py index efab55d..85120d8 100644 --- a/backend/tests/test_dispatcher.py +++ b/backend/tests/test_dispatcher.py @@ -81,6 +81,7 @@ def test_deliver_one_success(monkeypatch): assert event.attempts == 1 assert event.external_run_id == str(event_id) assert event.last_error is None + assert event.last_error_code is None def test_deliver_one_failure_schedules_retry(monkeypatch): @@ -98,6 +99,7 @@ def test_deliver_one_failure_schedules_retry(monkeypatch): assert event.attempts == 1 assert event.next_attempt_at is not None assert "simulated connection failure" in event.last_error + assert event.last_error_code == "connectionError" def test_deliver_one_exhausts_attempts_to_failed(monkeypatch): @@ -159,6 +161,7 @@ def test_deliver_one_handles_malformed_payload_without_getting_stuck(monkeypatch assert event.delivery_status in ("pending", "failed") assert event.attempts == 1 assert "Malformed outbox payload" in event.last_error + assert event.last_error_code == "malformedPayload" def test_claim_sets_a_lease_deadline(): @@ -208,6 +211,7 @@ def test_reclaim_recovers_an_expired_lease_and_preserves_attempts(monkeypatch): assert event.next_attempt_at is None assert event.attempts == 2 assert "stale" in event.last_error.lower() + assert event.last_error_code == "staleLeaseRecovered" # The reclaimed event is now a normal pending event, immediately claimable again. claimed = dispatcher._claim_due_events() diff --git a/backend/tests/test_search.py b/backend/tests/test_search.py index 488a9bf..ca80d22 100644 --- a/backend/tests/test_search.py +++ b/backend/tests/test_search.py @@ -11,6 +11,10 @@ def test_search_finds_a_vehicle_by_reference(ops_client): assert match is not None assert match["label"] == "MO-001" assert match["link"] == "/vehicles/MO-001" + # The backend must never send localizable prose -- only a stable code plus raw + # data params, so the frontend can render it in the operator's selected language. + assert match["detail_code"] == "vehicleSummary" + assert set(match["detail_params"]) == {"make", "model", "location"} def test_search_finds_a_booking_by_reference(ops_client): @@ -37,7 +41,21 @@ def test_search_never_returns_data_quality_issues_for_rental_employee(employee_c def test_search_section_result_visible_to_operations_manager(ops_client): result = ops_client.get("/api/v1/search", params={"q": "audit"}).json() - assert any(r["type"] == "section" and r["link"] == "/audit" for r in result["results"]) + match = next( + (r for r in result["results"] if r["type"] == "section" and r["link"] == "/audit"), None + ) + assert match is not None + # Section results must ship a stable id, not English prose -- the frontend looks up + # navigation:items. and search:sections..detail in the selected locale. + assert match["label"] == "audit" + assert match["detail_code"] == "audit" + + +def test_search_section_matches_dutch_and_french_terms(ops_client): + nl_result = ops_client.get("/api/v1/search", params={"q": "wagenpark"}).json() + assert any(r["type"] == "section" and r["link"] == "/vehicles" for r in nl_result["results"]) + fr_result = ops_client.get("/api/v1/search", params={"q": "réservation"}).json() + assert any(r["type"] == "section" and r["link"] == "/bookings" for r in fr_result["results"]) def test_search_section_result_hidden_from_rental_employee(employee_client): diff --git a/backend/tests/test_seed.py b/backend/tests/test_seed.py index 076cd1a..199cf66 100644 --- a/backend/tests/test_seed.py +++ b/backend/tests/test_seed.py @@ -23,8 +23,12 @@ 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 - # 15 from the CSV plus a deterministic set discovered by the post-seed scan. - assert db.scalar(select(func.count()).select_from(DataQualityIssue)) == 26 + # 15 from the CSV plus a deterministic set discovered by the post-seed scan. The + # shared vehicle-status evaluator (app.services.vehicle_status) now also catches + # MO-024: an active/return-pending booking (BK-DEMO-RETURN) on a vehicle that has + # already crossed its service-due odometer threshold -- a genuine conflict the + # previous hand-rolled scanner never checked for. + assert db.scalar(select(func.count()).select_from(DataQualityIssue)) == 27 assert db.scalar(select(func.count()).select_from(OutboxEvent)) == 20 assert db.scalar(select(func.count()).select_from(User)) == 2 finally: @@ -138,6 +142,7 @@ def test_seed_scenario_s5_failed_workflow_run(): assert failed.delivery_status == "failed" assert failed.attempts >= 1 assert failed.last_error + assert failed.last_error_code == "connectionError" finally: db.close() diff --git a/backend/tests/test_vehicle_status.py b/backend/tests/test_vehicle_status.py new file mode 100644 index 0000000..b983dff --- /dev/null +++ b/backend/tests/test_vehicle_status.py @@ -0,0 +1,160 @@ +"""Pure unit tests for the shared vehicle-status evaluator -- no database needed, since +evaluate_vehicle_status() only reasons over an already-gathered VehicleStatusFacts. See +docs/fleet-ops-correction/vehicle-status-decision-table.md for the decision table these +tests are asserting against.""" + +from types import SimpleNamespace + +from app.services.vehicle_status import ( + RECOMMENDATION_CODE_ACTIVE_RENTAL, + RECOMMENDATION_CODE_BOOKING_CONFLICT, + RECOMMENDATION_CODE_MANUAL_REVIEW, + RECOMMENDATION_CODE_NO_CONFLICT, + RECOMMENDATION_CODE_RENTAL_ENDED, + RECOMMENDATION_CODE_SERVICE_THRESHOLD, + VehicleStatusFacts, + compute_recommendation_token, + evaluate_vehicle_status, +) + + +def _vehicle(status: str, *, version: int = 1): + return SimpleNamespace(operational_status=status, version=version) + + +def _facts(**overrides) -> VehicleStatusFacts: + defaults = dict( + active_booking_refs=[], + overlapping_booking_pairs=[], + service_threshold_reached=False, + odometer_km=10_000, + next_service_km=20_000, + open_booking_overlap_issue_ref=None, + ) + defaults.update(overrides) + return VehicleStatusFacts(**defaults) + + +def test_available_with_active_rental_recommends_rented(): + result = evaluate_vehicle_status( + _vehicle("available"), _facts(active_booking_refs=["BK-0001"]) + ) + assert result.recommended_status == "rented" + assert result.recommendation_code == RECOMMENDATION_CODE_ACTIVE_RENTAL + assert result.safe_to_apply is True + assert result.manual_review_required is False + + +def test_maintenance_with_active_rental_never_auto_recommends_rented(): + # The exact unsafe shortcut this task explicitly forbids: maintenance + an active + # booking must NEVER be auto-resolved to "rented". + result = evaluate_vehicle_status( + _vehicle("maintenance"), _facts(active_booking_refs=["BK-0001"]) + ) + assert result.recommended_status is None + assert result.manual_review_required is True + assert result.safe_to_apply is False + assert result.recommendation_code == RECOMMENDATION_CODE_MANUAL_REVIEW + + +def test_available_with_active_rental_and_service_threshold_requires_manual_review(): + result = evaluate_vehicle_status( + _vehicle("available"), + _facts(active_booking_refs=["BK-0001"], service_threshold_reached=True), + ) + assert result.manual_review_required is True + assert result.recommended_status is None + + +def test_available_with_active_rental_and_booking_conflict_requires_manual_review(): + result = evaluate_vehicle_status( + _vehicle("available"), + _facts(active_booking_refs=["BK-0001"], open_booking_overlap_issue_ref="DQ-0001"), + ) + assert result.manual_review_required is True + assert result.recommended_status is None + + +def test_rented_with_no_active_booking_recommends_available(): + result = evaluate_vehicle_status(_vehicle("rented"), _facts()) + assert result.recommended_status == "available" + assert result.recommendation_code == RECOMMENDATION_CODE_RENTAL_ENDED + assert result.safe_to_apply is True + + +def test_service_threshold_reached_recommends_maintenance(): + result = evaluate_vehicle_status( + _vehicle("available"), _facts(service_threshold_reached=True) + ) + assert result.recommended_status == "maintenance" + assert result.recommendation_code == RECOMMENDATION_CODE_SERVICE_THRESHOLD + + +def test_booking_conflict_recommends_blocked_not_a_generic_high_severity_proxy(): + # The evaluator must react to a *real* booking-conflict fact, not "does some other + # open high-severity issue happen to exist" (the forbidden proxy). + result = evaluate_vehicle_status( + _vehicle("available"), + _facts(overlapping_booking_pairs=[("BK-DEMO-OVERLAP-A", "BK-DEMO-OVERLAP-B")]), + ) + assert result.recommended_status == "blocked" + assert result.recommendation_code == RECOMMENDATION_CODE_BOOKING_CONFLICT + + +def test_open_booking_overlap_issue_alone_also_triggers_blocked(): + result = evaluate_vehicle_status( + _vehicle("available"), _facts(open_booking_overlap_issue_ref="DQ-DEMO-OVERLAP") + ) + assert result.recommended_status == "blocked" + assert result.recommendation_code == RECOMMENDATION_CODE_BOOKING_CONFLICT + + +def test_maintenance_with_no_active_rental_and_no_blockers_stays_manual(): + # No fact here confirms maintenance is actually finished, so the evaluator must not + # auto-clear it to "available" -- that release remains an explicit, manual decision. + result = evaluate_vehicle_status(_vehicle("maintenance"), _facts()) + assert result.recommended_status is None + assert result.recommendation_code == RECOMMENDATION_CODE_NO_CONFLICT + assert result.safe_to_apply is False + + +def test_no_conflict_when_status_already_matches_facts(): + result = evaluate_vehicle_status(_vehicle("available"), _facts()) + assert result.recommended_status is None + assert result.recommendation_code == RECOMMENDATION_CODE_NO_CONFLICT + assert result.safe_to_apply is False + + result = evaluate_vehicle_status(_vehicle("rented"), _facts(active_booking_refs=["BK-1"])) + assert result.recommendation_code == RECOMMENDATION_CODE_NO_CONFLICT + + result = evaluate_vehicle_status(_vehicle("blocked"), _facts()) + assert result.recommendation_code == RECOMMENDATION_CODE_NO_CONFLICT + + result = evaluate_vehicle_status( + _vehicle("maintenance"), _facts(service_threshold_reached=True) + ) + assert result.recommendation_code == RECOMMENDATION_CODE_NO_CONFLICT + + +def test_recommendation_token_changes_when_facts_change(): + vehicle = _vehicle("available") + facts_a = _facts() + facts_b = _facts(service_threshold_reached=True) + assert compute_recommendation_token(vehicle, facts_a) != compute_recommendation_token( + vehicle, facts_b + ) + + +def test_recommendation_token_is_stable_for_identical_facts(): + vehicle = _vehicle("available") + facts = _facts(active_booking_refs=["BK-0001"]) + assert compute_recommendation_token(vehicle, facts) == compute_recommendation_token( + vehicle, facts + ) + + +def test_recommendation_token_changes_when_vehicle_version_changes(): + facts = _facts() + assert compute_recommendation_token( + _vehicle("available", version=1), facts + ) != compute_recommendation_token(_vehicle("available", version=2), facts) diff --git a/docs/fleet-ops-correction/current-gap-audit.md b/docs/fleet-ops-correction/current-gap-audit.md new file mode 100644 index 0000000..8684977 --- /dev/null +++ b/docs/fleet-ops-correction/current-gap-audit.md @@ -0,0 +1,75 @@ +# Fleet Ops correction — current-state gap audit + +## Branch / commit state (at audit time) + +- Repository's actual main/default branch is named **`master`** (there is no `main` branch — `remotes/origin/HEAD -> origin/master`). All instructions referring to "main" in this task are treated as referring to `master`. +- `master` (local and `origin/master`) was at `18344bc8b7a75a2f868bf15bf498fc030ac6c34c` before this task started — this is the newest verified Fleet Ops demo commit (contains the full rebrand/i18n/adaptive-guide/Data-Quality-UX work from the previous task). +- Deployed commit on Unraid (`/mnt/user/appdata/mobilityops/.deploy/source-revision`): `18344bc8b7a75a2f868bf15bf498fc030ac6c34c` — matches `master` exactly. No drift. +- No uncommitted local changes at audit time (`git status` clean). +- Source branch for this correction: `master` (already contained the newest verified commit). New branch created: **`fix/fleet-ops-i18n-status-flow`**, branched from `master` at `18344bc`. +- `feat/mobilityops-functional-completion` remains un-deleted, as instructed by the prior task, and is left untouched by this one. + +## Confirmed gaps (verified against actual code, not assumed) + +### 1. Brand name is a translatable key (structural risk) + +- `common:appName` exists per-locale in `frontend/src/i18n/locales/{nl-BE,en-GB,fr-BE}/common.json`, all currently `"Fleet Ops"`, but nothing prevents a future edit from diverging one locale. Used in `Login.tsx:35` and `Layout.tsx:180`. +- 8 more locale keys embed the literal string "Fleet Ops" inside translatable prose (`auth.defaultDescription`, `common.footer.productLine`, `demo.guide.steps["review-real-vs-simulated"].expectedOutcome`, `demo.about.title`, `demo.about.problemBody`, `demo.about.scopeBody`, `knowledge.emptyDescription`, `navigation.searchLabel`, `returns.scenario.body`) — all three locales currently say "Fleet Ops" correctly, but structurally these are still translatable values. +- **Fix**: `frontend/src/product.ts` exports `PRODUCT_NAME = "Fleet Ops"`. Remove `common:appName`; `Login.tsx`/`Layout.tsx` import the constant directly. Replace the 8 embedded mentions with `{{productName}}` interpolation, passing `productName: PRODUCT_NAME` explicitly at each call site. Add a Playwright test that fails if any locale JSON file contains the literal substring `"Fleet Ops"` (forcing all future brand mentions through interpolation) and a live-DOM test asserting the rendered brand text is byte-identical across all three languages. + +### 2. Backend hardcodes English prose as primary user-facing content (structural, not cosmetic) + +Confirmed in `backend/app/services/data_quality.py`: +- `_scan_duplicate_customers` (~line 127-149): evidence signals literally `"exact email"`, `"exact phone"`, `"exact postal code"`, `"similar name"`, joined into `evidence.summary`. +- `_scan_missing_required_fields` (~170, 185): `f"Missing: {', '.join(missing)}"`. +- `_scan_booking_overlaps` (~213): `f"Overlapping bookings {first.public_ref} and {second.public_ref}"`. +- `_scan_vehicle_status_conflicts` (~240-256): reason strings like `"marked available while an active booking exists"`. +- `_scan_odometer_regressions` (~291-295): full English sentence with interpolated numbers/refs. +- `_recommend_vehicle_status` (~659-668): recommendation `reason` strings returned verbatim as `ApplyRecommendedStatusResult.reason` and rendered directly in the UI. + +Confirmed in `backend/app/services/returns.py`: +- `_derive_vehicle_status_with_reason` (~32-43): `status_reason` strings ("Damage was reported on return.", "A technical warning was reported on return.", "Odometer reached the {n} km service threshold.", "No damage, technical warning or service threshold; routed to cleaning.") flow straight into the API response and are displayed raw regardless of UI language (asserted verbatim in English in `interactive-elements.spec.ts:133`, confirming this is genuinely user-visible, not just internal). + +Confirmed in `backend/app/services/dispatcher.py` / seed data: `event.last_error` stores raw strings like `"Synthetic connection timeout to n8n"` from `seed/workflow_runs.csv`, rendered directly in `Automation.tsx` (`{r.last_error ?? "—"}`) with no localization or summarization. + +Frontend confirmed to display these values completely raw: `DataQualityIssueDetail.tsx` → `
{String(issue.evidence.summary ?? "")}
` — the *label* is translated, the *value* is not. + +**Fix**: introduce structured `signals`/`message_code`+`params` on evidence and reasons (data-quality evidence, status-conflict recommendation reason, return status reason, automation last-error), with a frontend mapping layer that localizes known codes and falls back to the raw string under "Technical details" only. + +### 3–5, 8A. Status-recommendation flow is unsafe and combines calculation with mutation + +- **Confirmed single mutating endpoint**: `POST /api/v1/data-quality/issues/{public_ref}/apply-recommended-status` (`backend/app/api/routers/data_quality.py`) computes the recommendation and mutates the vehicle in the same call. No separate preview/GET route exists for this flow (unlike the return flow, which already has `preview_vehicle_return` / `register_vehicle_return` sharing one pure evaluator). +- **Confirmed unsafe shortcut** (explicitly prohibited by this task): `_recommend_vehicle_status` in `data_quality.py` returns `("rented", ...)` whenever `operational_status == "maintenance" and has_active_booking` — i.e. a vehicle flagged for maintenance with an active booking is auto-recommended (and, on one click, actually changed) to `rented`, with no check of the underlying reason it's in maintenance and no manual-review branch. +- **Confirmed proxy-based reasoning** (explicitly prohibited): the `available` + `has_open_high_issue` → `blocked` branch depends only on "does some other open high-severity issue exist for this vehicle", not on real underlying facts (damage, technical warning, confirmed overlap). `data_quality.py` never imports `Inspection`, so damage/technical-warning facts are never examined by this function at all. +- **Confirmed scanner/resolver duplication**: `_scan_vehicle_status_conflicts` and `_recommend_vehicle_status` are two independently-maintained `if`-chains (hand-kept-in-sync via a docstring comment, not shared code) — a structural fragility even though today's four branches happen to agree. + +**Fix**: new shared domain service `backend/app/services/vehicle_status.py` with one `evaluate_vehicle_status()` function consulting real facts (active/reserved/overlapping bookings, latest inspection damage/technical-warning, maintenance threshold, cleaning state), used by scanner, a new non-mutating preview endpoint, the apply endpoint, and tests. Maintenance+active-booking becomes `manual_review_required`, never an automatic `rented`. Full decision table in `docs/fleet-ops-correction/vehicle-status-decision-table.md`. + +### 6–7. MO-016 / issue-ordering + +- Confirmed `MO-016` scenario: vehicle seeded `operational_status="available"`, two overlapping *reserved* bookings (`BK-DEMO-OVERLAP-A`/`-B`), two pre-seeded `open` issues on the same vehicle (`DQ-DEMO-OVERLAP` booking_overlap, `DQ-DEMO-STATUS` vehicle_status_conflict). +- Confirmed **no test** resolves both issues in sequence (either order) within one session to check the outcome stays deterministic — each existing test independently resets the demo data first. + +**Fix**: new evaluator is order-independent by construction (recomputes real facts every call, doesn't cache any prior issue's existence as an input other than the generic "another open high-severity issue for manual-review fallback"); add an explicit ordering test. + +### 8. i18n-coverage test doesn't prove translation happened + +Confirmed: `frontend/e2e/i18n-coverage.spec.ts` only checks key-parity and non-empty values — a locale file could contain the literal English string copy-pasted and the test would still pass. Locale files were manually verified as genuinely translated (no hits for probe phrases like "canonical odometer", "committed locally", "correlation ID" etc. in `nl-BE`/`fr-BE`), so this is a test-coverage gap, not an active mistranslation — but per this task's instructions it still needs closing. + +**Fix**: add a translation-quality test comparing `nl-BE`/`fr-BE` values against `en-GB` for meaningful divergence (with an explicit allowlist for real proper nouns/technical tokens: Fleet Ops, Northstar Mobility, n8n, RAGcore, MCP Hub, API, UUID, Docker, PostgreSQL), plus a route-matrix smoke test opening every main route in all three languages. + +### 9. One confirmed leftover hardcoded string + +- `frontend/src/pages/BookingDetail.tsx:76` — `aria-label="Demo scenario"` is a literal, un-translated English string (the visible content beside it is correctly translated). + +**Fix**: route through `t("returns:scenario.ariaLabel")` (new key, 3 locales). + +### 10. Automation "last error" shown raw + +- `Automation.tsx` renders `r.last_error` directly with no localization/summarization layer, confirmed via the seeded `"Synthetic connection timeout to n8n"` string appearing verbatim regardless of UI language. + +**Fix**: known-code → localized summary + operational meaning, raw string demoted to "Technical details". + +## Scope note + +No gaps were found in: existing Control Rail navigation/layout, the three demo roles/authorization, the guided demo mechanics, n8n integration wiring, Docker/Unraid deployment scripts, or the previously-implemented adaptive Demo Guide / Data Quality choice-card UI — these are left untouched per the "do not redesign" instruction. This correction is scoped to the 10 problems above. diff --git a/docs/fleet-ops-correction/i18n-inventory.md b/docs/fleet-ops-correction/i18n-inventory.md new file mode 100644 index 0000000..d7e2e09 --- /dev/null +++ b/docs/fleet-ops-correction/i18n-inventory.md @@ -0,0 +1,25 @@ +# i18n inventory — dynamic/backend content requiring message-code treatment + +Static UI chrome (navigation, dashboard, forms, filters, dialogs, empty/loading states, +Demo Guide, About page, accessibility labels) was already moved to the `i18next` +namespace system in the prior task and is not re-inventoried here in full — see +`docs/final-product-polish/audit.md` and `docs/final-product-polish/i18n-inventory.md` +for that pass. This inventory covers only the sources confirmed still bypassing +translation (per `current-gap-audit.md`), the fix chosen, and the tests that verify it. + +| # | Source | Location | Static/dynamic | Fix | Tests | +|---|--------|----------|-----------------|-----|-------| +| 1 | Duplicate-customer evidence signals | `backend/app/services/data_quality.py::_scan_duplicate_customers` | dynamic | `evidence.signals: [{code, params}]` (`duplicate.exact_email`, `duplicate.exact_phone`, `duplicate.same_postal_code`, `duplicate.similar_name` + score param); frontend maps code→localized phrase | `test_data_quality.py::test_duplicate_customer_evidence_has_structured_signals`; Playwright DQ evidence-language test | +| 2 | Missing-required-field evidence | `_scan_missing_required_fields` | dynamic | `evidence.signals: [{code: "missing_field", params: {field}}]`; frontend maps `field` through the existing `CUSTOMER_FIELD_LABELS`/`VEHICLE_FIELD_LABELS`-equivalent i18n keys | same | +| 3 | Booking-overlap evidence | `_scan_booking_overlaps` | dynamic | `evidence.signals: [{code: "overlap.reserved_bookings", params: {refs: [...]}}]` | same | +| 4 | Vehicle-status-conflict evidence | `_scan_vehicle_status_conflicts` | dynamic | replaced entirely by the new `evaluate_vehicle_status()` evaluator's `recommendation_code`; scanner reuses the evaluator instead of its own reason strings | new evaluator unit tests | +| 5 | Odometer-regression evidence | `_scan_odometer_regressions` | dynamic | `evidence.signals: [{code: "odometer.regression", params: {later_ref, later_km, earlier_ref, earlier_km}}]` | same | +| 6 | Status-recommendation reason | `_recommend_vehicle_status` / new `evaluate_vehicle_status` | dynamic | `recommendation_code` + `facts` (structured), no free prose from the backend at all; frontend renders the full explanation from `quality:statusRecommendation.codes.` | evaluator unit tests + preview/apply contract tests | +| 7 | Return status reason | `backend/app/services/returns.py::_derive_vehicle_status_with_reason` | dynamic | `status_reason_code` + `params` alongside the existing human string (kept for backward-compat, demoted to technical fallback) | `test_returns.py` updated; Playwright return-flow-language test | +| 8 | Automation `last_error` | `dispatcher.py` / seed data, rendered in `Automation.tsx` | dynamic | known-cause codes (`n8n.connection_timeout`, `n8n.http_error`, etc.) mapped to a localized summary + "what happened / what's pending / what retry does"; raw string demoted to Technical details | Playwright automation-language test | +| 9 | `aria-label="Demo scenario"` | `frontend/src/pages/BookingDetail.tsx:76` | static, just un-wired | `t("returns:scenario.ariaLabel")`, 3 locales | i18n-coverage (key parity) + route-matrix test | +| 10 | Brand name (`common:appName` + 8 embedded mentions) | see gap audit §1 | static, wrongly translatable | `PRODUCT_NAME` constant, `{{productName}}` interpolation | new brand-invariant test | + +## Message-code mapping module + +Centralised in `frontend/src/i18n/messageCodes.ts` (new): a single `resolveMessageCode(t, code, params)` helper used by the Data Quality evidence renderer, the status-recommendation panel, the return-result panel, and the automation ledger, so there is one place mapping `code → i18next key` rather than per-page switch statements. diff --git a/docs/fleet-ops-correction/vehicle-status-decision-table.md b/docs/fleet-ops-correction/vehicle-status-decision-table.md new file mode 100644 index 0000000..7cbb18f --- /dev/null +++ b/docs/fleet-ops-correction/vehicle-status-decision-table.md @@ -0,0 +1,90 @@ +# Vehicle status decision table + +Authoritative rationale for `app/services/vehicle_status.py::evaluate_vehicle_status`, +the single evaluator shared by the data-quality scanner, the status-recommendation +preview endpoint, and the transactional apply endpoint (section 8A). Scanner and +resolver call the same function with the same freshly-gathered facts, so they can never +disagree, and the recommendation is order-independent: resolving, deferring, or +rejecting an unrelated issue never changes what this function returns for a vehicle, +because it only reasons over the vehicle's and bookings' current state, never over +issue history. + +## Facts gathered (`gather_vehicle_status_facts`) + +All facts are re-queried from the database on every call, never cached and never derived +from "does some other issue happen to be open": + +| Fact | Source | +|---|---| +| `active_booking_refs` | Bookings on this vehicle with `status == "active"` | +| `overlapping_booking_pairs` | Reserved/active bookings on this vehicle whose date ranges genuinely overlap | +| `service_threshold_reached` | `vehicle.odometer_km >= vehicle.next_service_km` | +| `open_booking_overlap_issue_ref` | The `public_ref` of a currently-open `booking_overlap` issue on this vehicle, if any (excluding the issue being resolved, via `exclude_issue_id`) | + +`has_active_rental` = at least one active booking. `has_booking_conflict` = an +overlapping-booking pair exists, or an open `booking_overlap` issue references this +vehicle. + +## Decision table + +| Current status | Active rental? | Service threshold reached? | Booking conflict? | Recommended status | Priority | `recommendation_code` | Safe to auto-apply? | +|---|---|---|---|---|---|---|---| +| any except `maintenance` | yes | no | no | `rented` (if not already) | 1 | `vehicle.active_rental` | yes | +| `maintenance` | yes | — | — | *(none — manual review)* | 1 | `vehicle.manual_review_required` | no | +| any | yes | yes | — | *(none — manual review)* | 1 | `vehicle.manual_review_required` | no | +| any | yes | — | yes | *(none — manual review)* | 1 | `vehicle.manual_review_required` | no | +| not `maintenance` | no | yes | — | `maintenance` | 2 | `vehicle.service_threshold_reached` | yes | +| `maintenance` | no | yes | — | *(none — already correct)* | 2 | `vehicle.no_conflict` | n/a | +| not `blocked` | no | no | yes | `blocked` | 3 | `vehicle.booking_conflict` | yes | +| `blocked` | no | no | yes | *(none — already correct)* | 3 | `vehicle.no_conflict` | n/a | +| `rented` | no | no | no | `available` | 4 | `vehicle.rental_ended` | yes | +| `available` / `cleaning` / `blocked` | no | no | no | *(none — already correct)* | 4 | `vehicle.no_conflict` | n/a | +| `maintenance` | no | no | no | *(none — stays in maintenance)* | 4 | `vehicle.no_conflict` | n/a | +| anything not covered above | — | — | — | *(none — manual review)* | 5 | `vehicle.manual_review_required` | no | + +## Safe-status principles (section 8B) applied + +- **Maintenance + active booking never auto-resolves to `rented`.** Being currently in + `maintenance` is itself treated as a blocking fact (row 2 above) — an active booking + is never proof the vehicle should be marked rented; it is a real contradiction that + requires a human to investigate (e.g. was the vehicle released from the workshop + without updating status, or is the booking itself stale). +- **`available` + any booking never auto-resolves to `rented` silently past a real + blocker.** The `rented` recommendation only fires when there is no competing blocking + fact (no service-threshold breach, no booking conflict, not already in maintenance). +- **An open issue disappearing never auto-resolves to `available`.** Leaving + `maintenance` requires a human decision — this evaluator holds no fact that proves + maintenance work is actually finished (no completed-service record is modelled), so a + vehicle sitting in `maintenance` with no active rental and no other blocker stays + `vehicle.no_conflict` (left alone) rather than being auto-promoted to `available`. +- **Every branch re-derives facts; nothing is cached.** `blocking_reasons` is always + computed fresh from `service_threshold_reached`, `has_booking_conflict`, and the + current status itself — never from a proxy like "is some other high-severity issue + still open". + +## Statuses used + +Only statuses that exist in the current domain model are referenced: `available`, +`rented`, `cleaning`, `maintenance`, `blocked`. `cleaning` is never a recommendation +target from this evaluator (no fact here proves cleaning is required or complete); it is +only ever an input `current_status` that, absent any blocker, is left alone +(`vehicle.no_conflict`). + +## Concurrency: recommendation token + +`compute_recommendation_token(vehicle, facts)` hashes the vehicle's optimistic-lock +`version` plus every fact the recommendation was based on (sha256, truncated to 16 hex +chars). The preview endpoint returns this token; the apply endpoint recomputes it from +freshly-gathered facts inside the same transaction and rejects the request +(`RECOMMENDATION_STALE`) if it no longer matches — the frontend must never assume a +previously-shown preview is still valid without server revalidation (section 8F). + +## MO-016: order independence + +MO-016 carries both an open `booking_overlap` issue and an open +`vehicle_status_conflict` issue at once (two overlapping reserved bookings). Because +`gather_vehicle_status_facts` re-queries `overlapping_booking_pairs` and +`open_booking_overlap_issue_ref` fresh every call, resolving the booking-overlap issue +first vs. resolving the status-conflict issue first both converge on the same final +vehicle status — see `test_mo_016_status_conflict_recommendation_is_order_independent` +in `backend/tests/test_data_quality.py`. diff --git a/frontend/e2e/i18n-coverage.spec.ts b/frontend/e2e/i18n-coverage.spec.ts index f5d3079..e178b58 100644 --- a/frontend/e2e/i18n-coverage.spec.ts +++ b/frontend/e2e/i18n-coverage.spec.ts @@ -71,3 +71,116 @@ test("no locale file contains an empty string value", () => { } } }); + +// --- Brand-invariant: "Fleet Ops" is a fixed constant, never a translation value --- +// (see frontend/src/product.ts and docs/fleet-ops-correction/current-gap-audit.md §1). +// A regression here means someone re-introduced a per-locale brand key/value instead of +// interpolating {{productName}} from the shared constant. + +test("no locale file defines an 'appName' key or the literal brand string", () => { + for (const language of LANGUAGES) { + for (const namespace of namespaces) { + const data = loadNamespace(language, namespace); + const raw = JSON.stringify(data); + expect( + raw.includes("Fleet Ops"), + `${language}/${namespace}.json contains the literal brand string "Fleet Ops" -- ` + + `use {{productName}} interpolation instead so the brand can never drift per locale`, + ).toBe(false); + const keys = collectKeyPaths(data); + expect( + keys.some((k) => k === "appName" || k.endsWith(".appName")), + `${language}/${namespace}.json defines an "appName" key -- the brand name must come ` + + `from the PRODUCT_NAME constant, never a translatable key`, + ).toBe(false); + } + } +}); + +// --- Translation-quality: prove values were actually translated, not copy-pasted --- +// Sleutelpariteit alone doesn't prove translation happened (a locale file could contain +// the literal English string under the right key and still pass). For every "real prose" +// string (>=8 chars, not on the allowlist below), assert nl-BE and fr-BE differ from +// en-GB, and that fr-BE differs from nl-BE -- catching both "still English" and +// "Dutch text copy-pasted into French" in one pass. + +// Exact (namespace, key-path) pairs that are legitimately identical across two or more +// locales: real proper nouns/brand names, deliberately-untranslated role titles, and +// genuine cross-language cognates (identical spelling in Dutch/French/English). This is +// a precise allowlist by key path, not a broad word-level allowlist, so it can't quietly +// hide an unrelated real mistranslation under the same key in a different namespace. +const IDENTICAL_VALUE_ALLOWLIST = new Set([ + "audit.title", // "Audit trail" kept as an established cross-language compliance term + "audit.diff.was", // "{{field}}: was {{value}}" -- "was" is spelled identically in Dutch + "auth.roleOperationsManager", // deliberately-untranslated role title (see demo.json roles) + "auth.roleRentalEmployee", + "demo.scenarios.roles.operations_manager", // same convention, scenario-overview role labels + "demo.scenarios.roles.rental_employee", + "common.language.nl-BE", // language-picker options show each language's own endonym + "common.language.fr-BE", + "common.footer.productLine", // "{{productName}} Demo" -- brief-specified exact footer text + "common.orgName", // "Northstar Mobility" -- fictional org proper noun, same in all 3 + "dashboard.attention.openRecord", // "Open {{title}}" -- "open" is also the Dutch imperative + "demo.scenarios.durationValue", // "± {{minutes}} min" -- unit abbreviation, same in all 3 + "demo.scenarios.startScenario", // "start"/"scenario" are naturalised loanwords in Dutch + "demo.about.limitationsTitle", // "Limitations" -- identical spelling in French + "demo.integrationSummary.titles.mcp_hub", // "ITWorx MCP Hub" -- proper noun + "fleet.list.columns.attention", // "Attention" -- identical spelling in French + "fleet.detail.tabs.inspections", // "Inspections" -- identical spelling in French + "integrations.cards.orchestrationKicker", // "Orchestration" -- identical in French + "knowledge.questionLabel", // "Question" -- identical spelling in French + "knowledge.retrievalFlow.question", + "knowledge.questionLabelExchange", + "navigation.items.audit", // "Audit trail" kept as an established cross-language term + "returns.result.inspection", // "Inspection" -- identical spelling in French +]); + +function isTranslatableProse(value: unknown): value is string { + if (typeof value !== "string") return false; + if (value.trim().length < 8) return false; + // Strip interpolation placeholders and non-letter characters; if nothing substantial + // remains (pure numbers/punctuation/units), it's not "prose" that needs translating. + const stripped = value + .replace(/\{\{[^}]+\}\}/g, " ") + .replace(/[^a-zA-Zà-öø-ÿÀ-ÖØ-ß]/g, ""); + return stripped.trim().length >= 3; +} + +test("nl-BE and fr-BE translations are not suspiciously identical to en-GB or each other", () => { + for (const namespace of namespaces) { + const en = loadNamespace("en-GB", namespace); + const nl = loadNamespace("nl-BE", namespace); + const fr = loadNamespace("fr-BE", namespace); + const keys = collectKeyPaths(en); + + for (const keyPath of keys) { + if (IDENTICAL_VALUE_ALLOWLIST.has(`${namespace}.${keyPath}`)) continue; + const at = (data: Record) => + keyPath.split(".").reduce((acc, part) => { + if (acc && typeof acc === "object") return (acc as Record)[part]; + return undefined; + }, data); + + const enValue = at(en); + if (!isTranslatableProse(enValue)) continue; + const nlValue = at(nl); + const frValue = at(fr); + + expect( + nlValue, + `${namespace}.json:${keyPath} — nl-BE is identical to en-GB ("${enValue}"); ` + + `looks untranslated (add to IDENTICAL_VALUE_ALLOWLIST if this is intentional)`, + ).not.toBe(enValue); + expect( + frValue, + `${namespace}.json:${keyPath} — fr-BE is identical to en-GB ("${enValue}"); ` + + `looks untranslated (add to IDENTICAL_VALUE_ALLOWLIST if this is intentional)`, + ).not.toBe(enValue); + expect( + frValue, + `${namespace}.json:${keyPath} — fr-BE is identical to nl-BE ("${nlValue}"); ` + + `looks like Dutch text was copy-pasted into the French locale`, + ).not.toBe(nlValue); + } + } +}); diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index df0bc30..d6de408 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -109,6 +109,7 @@ export interface AutomationRun { status: string; attempts: number; last_error: string | null; + last_error_code: string | null; occurred_at: string; } @@ -154,6 +155,8 @@ export interface ReturnPreviewResult { resulting_odometer_km: number; resulting_vehicle_status: string; status_reason: string; + status_reason_code: string; + status_reason_params: Record; would_create_quality_issue: boolean; attention_reasons: string[]; next_booking_risk: NextBookingRisk | null; @@ -173,7 +176,27 @@ export interface DataQualityIssueDetail extends DataQualityIssue { export interface ApplyRecommendedStatusResult { issue: DataQualityIssue; applied_status: string; - reason: string; + reason_code: string; +} + +export interface VehicleStatusFacts { + active_booking_refs: string[]; + overlapping_booking_pairs: string[][]; + service_threshold_reached: boolean; + odometer_km: number; + next_service_km: number; + open_booking_overlap_issue_ref: string | null; +} + +export interface StatusRecommendation { + current_status: string; + recommended_status: string | null; + recommendation_code: string; + safe_to_apply: boolean; + manual_review_required: boolean; + facts: VehicleStatusFacts; + blocking_reasons: string[]; + recommendation_token: string; } export interface ScanResult { @@ -183,7 +206,8 @@ export interface ScanResult { export interface SearchResultItem { type: "vehicle" | "booking" | "data_quality_issue" | "section"; label: string; - detail: string; + detail_code: string; + detail_params: Record; link: string; } diff --git a/frontend/src/components/DemoGuide.tsx b/frontend/src/components/DemoGuide.tsx index 55a7caf..8be19c1 100644 --- a/frontend/src/components/DemoGuide.tsx +++ b/frontend/src/components/DemoGuide.tsx @@ -8,6 +8,7 @@ import { useDemoManifest } from "../context/DemoManifestContext"; import { useViewportTier } from "../hooks/useViewportTier"; import { DEMO_GUIDE_STEPS } from "../data/demoGuideSteps"; import { Icon } from "./Icons"; +import { PRODUCT_NAME } from "../product"; export function DemoGuideTrigger() { const { t } = useTranslation("demo"); @@ -206,7 +207,7 @@ export function DemoGuide() {

{t("guide.whatYouWillSee")}
{t(`guide.steps.${step.id}.whatYouWillSee`)}

{t("guide.whyItMatters")}
{t(`guide.steps.${step.id}.whyItMatters`)}

{t("guide.startAction")}
{t(`guide.steps.${step.id}.startAction`)}

-

{t("guide.expectedOutcome")}
{t(`guide.steps.${step.id}.expectedOutcome`)}

+

{t("guide.expectedOutcome")}
{t(`guide.steps.${step.id}.expectedOutcome`, { productName: PRODUCT_NAME })}

)} diff --git a/frontend/src/components/Layout.tsx b/frontend/src/components/Layout.tsx index 1aa6907..4b05fa5 100644 --- a/frontend/src/components/Layout.tsx +++ b/frontend/src/components/Layout.tsx @@ -10,6 +10,7 @@ import { DemoGuide, DemoGuideTrigger } from "./DemoGuide"; import { LanguageSwitcher } from "./LanguageSwitcher"; import { useDemoGuide } from "../context/DemoGuideContext"; import { useDemoManifest } from "../context/DemoManifestContext"; +import { PRODUCT_NAME } from "../product"; const SEARCH_ICON: Record = { vehicle: "fleet", @@ -18,6 +19,31 @@ const SEARCH_ICON: Record = { section: "chevron", }; +// The backend only ever sends a stable code + raw data params (never English prose) -- +// see app/api/routers/search.py. Localizing here means the label/detail always follow +// the operator's selected locale, in every namespace search results can point to. +function searchResultLabel(t: (key: string, opts?: Record) => string, item: SearchResultItem): string { + if (item.type === "section") return t(`navigation:items.${item.label}`, { defaultValue: item.label }); + return item.label; +} + +function searchResultDetail(t: (key: string, opts?: Record) => string, item: SearchResultItem): string { + switch (item.type) { + case "section": + return t(`searchSections.${item.detail_code}`, { defaultValue: item.detail_code }); + case "vehicle": + return t("searchVehicleSummary", { ...item.detail_params }); + case "booking": + return t(`bookings:statuses.${item.detail_code}`, { defaultValue: item.detail_code }); + case "data_quality_issue": + return t(`quality:ruleTypes.${item.detail_code}`, { + defaultValue: item.detail_code.replace(/_/g, " "), + }); + default: + return item.detail_code; + } +} + interface NavItem { to: string; labelKey: string; @@ -177,7 +203,7 @@ export function Layout() {