docs(state): Phase D decisions resolved + vendor recon findings, B3 contract widened

This commit is contained in:
2026-05-17 00:36:13 +10:00
parent 3a7bd749cc
commit cbe9ae5d9c
+52 -8
View File
@@ -18,10 +18,24 @@
| C | C1: `GET /me` endpoint | ✅ DONE | commit `a0bdcaa` + signature fix `1e5b822` — appended to `backend/app/api/mechanic.py`; returns `MeOut` from `require_mechanic` user; `permissions` is `["admin", "mechanic:inspect"]` for admins else `["mechanic:inspect"]`. Test fixtures (`client`, `override_mechanic`, `_fake_user`) in `backend/tests/test_mechanic_endpoints.py` use FastAPI `TestClient` + `app.dependency_overrides` per Option A — no real DB / no async. 6 endpoint tests. | | C | C1: `GET /me` endpoint | ✅ DONE | commit `a0bdcaa` + signature fix `1e5b822` — appended to `backend/app/api/mechanic.py`; returns `MeOut` from `require_mechanic` user; `permissions` is `["admin", "mechanic:inspect"]` for admins else `["mechanic:inspect"]`. Test fixtures (`client`, `override_mechanic`, `_fake_user`) in `backend/tests/test_mechanic_endpoints.py` use FastAPI `TestClient` + `app.dependency_overrides` per Option A — no real DB / no async. 6 endpoint tests. |
| C | C2: `GET /vehicles` list with `q`+`limit` search | ✅ DONE | commit `7be8783` — new `backend/app/api/mechanic_service.py` with `search_vehicles_with_last_inspection` (single aggregated subquery + outerjoin to avoid N+1 for last completed inspection per vehicle); endpoint in `mechanic.py`; new `VehicleListResponse` schema; 8 tests (6 endpoint + 2 service-stmt) with `_FakeSession.execute()` returning canned rows. Search hits plate (`CarV2.number`) + VIN only. | | C | C2: `GET /vehicles` list with `q`+`limit` search | ✅ DONE | commit `7be8783` — new `backend/app/api/mechanic_service.py` with `search_vehicles_with_last_inspection` (single aggregated subquery + outerjoin to avoid N+1 for last completed inspection per vehicle); endpoint in `mechanic.py`; new `VehicleListResponse` schema; 8 tests (6 endpoint + 2 service-stmt) with `_FakeSession.execute()` returning canned rows. Search hits plate (`CarV2.number`) + VIN only. |
| C | C3: `GET /vehicles/{id}` detail + bound /vehicles limit | ✅ DONE | commit `9109383` + style fix `90b8c66` — new `VehicleDetail(VehicleSummary)` + `InspectionSummary(_Base)` schemas; service `get_vehicle_by_id` + `recent_mechanic_inspections`; endpoint raises 404 when missing, `last_inspection_at` derived from first completed inspection in the list; applied `Query(20, ge=1, le=200)` bounds to C2's `/vehicles` limit param. 7 new tests (5 detail + 2 bounds). Service is now FastAPI-free (404 raise lives in endpoint, matching `automations.py` convention). | | C | C3: `GET /vehicles/{id}` detail + bound /vehicles limit | ✅ DONE | commit `9109383` + style fix `90b8c66` — new `VehicleDetail(VehicleSummary)` + `InspectionSummary(_Base)` schemas; service `get_vehicle_by_id` + `recent_mechanic_inspections`; endpoint raises 404 when missing, `last_inspection_at` derived from first completed inspection in the list; applied `Query(20, ge=1, le=200)` bounds to C2's `/vehicles` limit param. 7 new tests (5 detail + 2 bounds). Service is now FastAPI-free (404 raise lives in endpoint, matching `automations.py` convention). |
| D-pre | B3 schemas widened from vendor recon (`a187a92`) | ✅ DONE | After read-only recon of `car_inspections` (14k rows, 2yr): `InspectionType` extended 4→9 (added `initial`, `seizure`, `equipment-change`, `pre-repair`, `post-repair`); `DamageType` fully replaced 7→16 (removed `rust`/`glass`/`paint` which had 0 vendor occurrences; added real-world types from vendor distribution: `damaged`, `chip`, `scuff`, `burn`, `bent`, `puncture`, `bulge`, `not-working`, etc.). `Severity` unchanged (4 values, optional — vendor populates only 3%). 2 new positive-coverage tests. Stale comment in `mechanic_damage_marker.py:54` references old DamageType values — fix in Phase F when touching that model. |
**Branch state:** `feat/mechanic-mvp` at `90b8c66`. Full stack: `be0553e` (A4) → `d2113a0` (B1) → `2739cc8` + `2fb978d` (B3) → `f0d7e83` (B4) → `a0bdcaa` + `1e5b822` (C1) → `7be8783` (C2) → `9109383` + `90b8c66` (C3). All pushed to `origin` (Gitea). **Branch state:** `feat/mechanic-mvp` at `a187a92`. Full stack: `be0553e` (A4) → `d2113a0` (B1) → `2739cc8` + `2fb978d` (B3) → `f0d7e83` (B4) → `a0bdcaa` + `1e5b822` (C1) → `7be8783` (C2) → `9109383` + `90b8c66` (C3)`a187a92` (B3-vendor-align). All pushed to `origin` (Gitea).
**Test count on branch:** **83 mechanic tests pass** (38 models + 15 schemas + 9 deps + 21 endpoints across C1/C2/C3) in ~2s, no real DB needed. **Test count on branch:** **85 mechanic tests pass** (38 models + 17 schemas + 9 deps + 21 endpoints across C1/C2/C3) in ~2s, no real DB needed.
### Vendor data reality (from recon 2026-05-17 — 14,013 inspections, 2 years)
| Metric | Value | Implication |
|---|---|---|
| Inspections with damage | 1,941 / 14,013 = 13.9% | Most inspections have empty marker list — UI optimize for that |
| Median markers per inspection | 2 | Inline carry-over copy is cheap |
| p95 markers | 10 | Confirms no async needed |
| Max markers | 34 | Worst case still ~50 inserts on commit |
| Median photos per inspection | 11 (`all_photo_ids`) | Includes 9 sides + damage photos |
| Max photos per inspection | 30 | Set MinIO presigned batch size accordingly |
| Carry-over in vendor | **None — denormalized snapshot** | Our `prev_inspection_id`+`carried_over_from_id` FK approach is net improvement |
| Vendor inspection status field | **None — write-once when completed** | Our `in_progress`/`completed`/`cancelled` is greenfield, no conflicts |
**Live mechanic API surface (4 routes):** **Live mechanic API surface (4 routes):**
- `GET /api/v1/mechanic/healthz` — liveness - `GET /api/v1/mechanic/healthz` — liveness
@@ -96,14 +110,44 @@ If Phase D/E/F needs a more complex query shape (e.g. `.scalars().one()` / `.fir
- Idempotency / race conditions: two mechanics opening the same vehicle simultaneously - Idempotency / race conditions: two mechanics opening the same vehicle simultaneously
- Real persistence with `_FakeSession`: tests need to verify the right `add()` / `flush()` / `refresh()` calls happened. The current `_FakeSession` only mocks read operations — needs extension for write paths (`add(obj)`, `flush()`, `refresh(obj)` to seed an `id`). - Real persistence with `_FakeSession`: tests need to verify the right `add()` / `flush()` / `refresh()` calls happened. The current `_FakeSession` only mocks read operations — needs extension for write paths (`add(obj)`, `flush()`, `refresh(obj)` to seed an `id`).
**Recommendation for the next session:** before dispatching the D1 implementer, decide: ### Phase D architecture decisions (RESOLVED 2026-05-17)
1. Whether the carry-over copy happens inside the create endpoint (simpler, atomic) or as a separate background step (e.g. Celery).
2. How `_FakeSession.add/flush/refresh` should behave for write-path tests (auto-assign id? track inserted objects?).
3. Whether Phase D is the right place to introduce a tiny real-DB conftest after all (re-evaluate Option C from earlier).
These three decisions are NOT trivial and warrant a brainstorming pass at the start of the next session before any implementer is dispatched. 1. **Carry-over: inline в POST /inspections** (atomic in the same transaction). Until carry-over copy completes, the new inspection isn't "ready" for the UI — async would create race conditions and complicate testing without a meaningful payoff for ~10-20 markers per copy.
Read the D1 task definition in the plan before deciding. Also check what other relevant Pydantic schemas exist that we built in B3 (`InspectionCreate`, `InspectionPatch`, `MarkerCreate`, `MarkerPatch`). 2. **`_FakeSession` write-path extension.** Add the following to the existing `_FakeSession` in `tests/test_mechanic_endpoints.py`:
- `add(obj)` — appends to `self.added` list; auto-assigns `obj.id` if not set, popping from `_next_ids` queue if provided, else incrementing `_next_id_counter` (starts at 1000 to avoid collision with test-seeded ids in the 1-99 range)
- `async flush()` — no-op + `self.flush_count += 1` (ids already assigned in `add`)
- `async commit()` — no-op + `self.commit_count += 1`
- `async refresh(obj)` — no-op + `self.refresh_count += 1`
- **`rows_per_call: Optional[list[Sequence]]`** kwarg — if provided, each `execute()` call pops one item from this queue and returns it as the result rows. This handles multi-query service flows (e.g. `start_inspection` does `select(prev_inspection)` then `select(prev_markers)`).
- Existing `execute(stmt)` and `get(model, pk)` keep their behavior.
- All extensions backward-compatible with current 83 tests.
3. **Test infrastructure: stay on Option A, do NOT add real DB.** SQL specifics (selectinload, cascade, JSONB) will be validated in Phase J e2e. aiosqlite ≠ Postgres (JSONB mismatch), testcontainers is too heavy for 3 endpoints, two-paradigm test suite is more pain than gain. Extend the mock instead (see point 2).
### Phase D path overrides (apply to D1, D2, D3)
Same flat-file pattern as Phase B-C; map plan's stale paths:
| Plan says | Use instead |
|---|---|
| `backend/app/mechanic/router.py` | `backend/app/api/mechanic.py` (append) |
| `backend/app/mechanic/service.py` | `backend/app/api/mechanic_service.py` (append) |
| `backend/app/mechanic/schemas.py` | `backend/app/api/mechanic_schemas.py` (already has all needed shapes from B3) |
| `backend/tests/test_mechanic/test_inspections.py` | `backend/tests/test_mechanic_endpoints.py` (append) |
| `Inspection` model | `MechanicInspection` (`app/models/mechanic_inspection`) |
| `DamageMarker` model | `MechanicDamageMarker` (`app/models/mechanic_damage_marker`) |
| `InspectionPhoto` model | `MechanicInspectionPhoto` (`app/models/mechanic_inspection_photo`) |
| `service.get_vehicle_or_404` (plan) | `service.get_vehicle_by_id` + endpoint-level `None`-check + `HTTPException(404)` raise (C3 fix) |
| `from fastapi import HTTPException` inline in service | At endpoint level only; service stays FastAPI-free |
| 404 raise inside endpoint via inline `from fastapi import HTTPException` | Use top-of-file import (already there from C3) |
Pydantic schemas (already shipped in B3):
- `InspectionCreate` (vehicle_id, type, driver_id) — request body for POST
- `InspectionPatch` (status, notes, finished_at) — request body for PATCH
- `InspectionDetail` (full response with photos + markers list) — response model
Phase B3 schemas use `model_config = ConfigDict(from_attributes=True)` via `_Base`, so SQLAlchemy ORM instances serialize automatically with `response_model=InspectionDetail`.
## How to resume in a new session ## How to resume in a new session