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
<status>" 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 <noreply@anthropic.com>
This commit is contained in:
NuklearRabbit
co-authored by
Claude Sonnet 5
Claude Sonnet 5
parent
18344bc8b7
commit
6deb95524d
@@ -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` → `<dd>{String(issue.evidence.summary ?? "")}</dd>` — 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.
|
||||
@@ -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.<code>` | 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.
|
||||
@@ -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`.
|
||||
Reference in New Issue
Block a user