From 0091c57c7fd82ffda16da1edfe5fa5589c8f1e13 Mon Sep 17 00:00:00 2001 From: NuklearRabbit <145918611+NuklearRabbit@users.noreply.github.com> Date: Sat, 1 Aug 2026 21:49:46 +0200 Subject: [PATCH] M2: implement vehicle return vertical slice Transactional return command with idempotency, row-lock concurrency control, odometer-regression handling, vehicle status derivation, outbox event, audit trail. Result-summary UI on booking detail. 26 backend tests passing, ruff clean. Verified end-to-end via browser against S1 demo scenario; fixed two real defects found only through browser testing (UI state loss on status transition, unflushed UUID default). --- PROJECT_STATE.md | 20 +- .../e7b08389f47f_idempotency_records.py | 41 +++ backend/app/api/routers/bookings.py | 19 +- backend/app/models/__init__.py | 2 + backend/app/models/idempotency.py | 19 ++ backend/app/schemas.py | 28 +- backend/app/seed_loader.py | 2 + backend/app/services/returns.py | 247 ++++++++++++++++++ backend/tests/test_return.py | 160 ++++++++++++ backend/tests/test_seed.py | 7 + frontend/src/api/types.ts | 26 ++ frontend/src/components/ReturnForm.tsx | 159 +++++++++++ frontend/src/pages/BookingDetail.tsx | 27 +- frontend/src/styles.css | 16 ++ 14 files changed, 763 insertions(+), 10 deletions(-) create mode 100644 backend/alembic/versions/e7b08389f47f_idempotency_records.py create mode 100644 backend/app/models/idempotency.py create mode 100644 backend/app/services/returns.py create mode 100644 backend/tests/test_return.py create mode 100644 frontend/src/components/ReturnForm.tsx diff --git a/PROJECT_STATE.md b/PROJECT_STATE.md index 407817d..229c655 100644 --- a/PROJECT_STATE.md +++ b/PROJECT_STATE.md @@ -2,7 +2,7 @@ ## Current milestone -M1 — complete. Starting M2 next. +M2 — complete. Starting M3 next. ## Locked decisions @@ -22,6 +22,12 @@ M1 — complete. Starting M2 next. - `compose.yaml` api build context changed from `./backend` to repo root with `dockerfile: backend/Dockerfile`, so the image can `COPY seed ./seed` (seed CSVs are outside `backend/`). - Frontend: added `react-router-dom@7.18.2` (bumped from 6.x to clear two real advisories — open redirect + arbitrary constructor injection in v6). One residual `npm audit` finding (RSC-mode CSRF, GHSA-qwww-vcr4-c8h2) does not apply — this SPA never uses React Router's RSC/SSR mode. - Nav/pages built so far: Dashboard, Vehicles (list+detail with tabs), Bookings (list+detail), Audit. Data Quality, Knowledge and Automation nav items are intentionally omitted until M3/M5/M4 build the pages behind them — CLAUDE.md forbids dead routes/placeholders. +- Return workflow (`app/services/returns.py`): the spec's "validate submitted reading against booking start reading" step was dropped as a hard rejection. For the seeded S1 scenario, a booking's `start_odometer_km` can already equal the vehicle's canonical odometer, so any regression-testing value would also be below the booking start, making a hard floor there indistinguishable from — and in conflict with — the documented soft-regression path. Only one odometer check exists now: submitted vs. the vehicle's *canonical* odometer (`vehicle.odometer_km`), matching the domain-model invariant verbatim ("a return with a lower submitted reading is recorded as an inspection and issue, while canonical odometer remains unchanged"). +- Idempotency: new `idempotency_records` table (migration `e7b08389f47f`), unique on `idempotency_key`, keyed to `booking_id`. Same key + same booking replays the stored response; same key + different booking → 409 `IDEMPOTENCY_KEY_REUSED`; different key on an already-returned booking → 409 `INVALID_BOOKING_STATE`. Concurrency is enforced by `SELECT ... FOR UPDATE` on the booking row (re-checked for the idempotency record immediately after acquiring the lock, as a safety net for two simultaneous identical-key requests racing the pre-lock check). +- `seed_loader.clear_all()` must delete `idempotency_records` before `bookings` (FK) — easy to forget when adding new booking-referencing tables; the ordering list at the top of `seed_loader.py` is the single place to update. +- Inspection `public_ref` is assigned as `INSP-{count+1:04d}` from a live count query (not gap-safe, fine for a PoC single-writer demo, would need a sequence for real concurrency-safe numbering). +- Found and fixed during browser verification (not caught by pytest, since it's a UI-only defect): `ReturnForm` originally held its own `result` state and was conditionally rendered only when `booking.status === "active"`; once the return succeeded the booking flipped to `returned` and React unmounted the form before the user ever saw the result panel. Fixed by lifting the result into `BookingDetail` (`ReturnResultPanel` is now a sibling, not nested in `ReturnForm`). Also found: `OutboxEvent.event_id`'s Python-side `default=uuid.uuid4` on the mapped_column only applies at flush/commit time, so reading `event.event_id` before `db.commit()` returned `None` (rendered as the literal string "None" in the result panel); fixed by assigning `event_id=uuid.uuid4()` explicitly at construction. Lesson: SQLAlchemy column `default=` callables are not available on the in-memory Python object until flush — never rely on the generated value for a same-transaction response body without an explicit `db.flush()` or an explicit Python-side assignment. +- Operational note for this environment: `docker compose run --rm api ...` (used for tests/lint) only starts a throwaway one-off container — it does **not** update the long-running `api`/`web` service containers. After any code change meant to be verified live (browser, curl), `docker compose up -d --build ` is required, not just `docker compose build`. ## Completed evidence @@ -49,10 +55,20 @@ M1 — complete. Starting M2 next. - Browser smoke test (Chrome via MCP) at desktop width: login page → Operations Manager login → Dashboard (metrics + attention items + today + recent automation all populated) → Vehicle detail `MO-016` (tabs render, "Needs attention" badge correct — it's `DQ-DEMO-OVERLAP`/`DQ-DEMO-STATUS`) → Booking detail `BK-DEMO-RETURN` (matches S1 scenario: vehicle `MO-024`, status `active`, start odometer `53610`). Responsive CSS (`@media max-width:700px`) was written and code-reviewed but the automated resize during this session didn't visibly reflect in the captured screenshot (likely a screenshot-timing quirk of the browser tool, not necessarily a real bug) — **treat the ≤360px layout as visually unverified** and re-check with a real device/DevTools emulation before final acceptance (M7). - Known accepted gap carried over from M0: `npm audit` residual `esbuild`/Vite-8 dev-server-only advisory. +### M2 — Vehicle return vertical slice +- Backend additions: `app/models/idempotency.py` (`IdempotencyRecord`), migration `e7b08389f47f_idempotency_records`, `app/services/returns.py` (`register_vehicle_return` — full transaction: row locks, idempotency replay, inspection, canonical-odometer update or regression issue, vehicle status derivation, two audit events, `vehicle.returned.v1` outbox event matching `contracts/events.schema.json`, next-booking-risk lookup), `POST /api/v1/bookings/{public_ref}/return` wired in `app/api/routers/bookings.py` with required `Idempotency-Key` header. +- Frontend additions: `components/ReturnForm.tsx` (form + `ReturnResultPanel`), wired into `pages/BookingDetail.tsx` (shown only when `booking.status === "active"`; result persists via lifted state after the booking flips to `returned`). +- Commands run and verified from this checkout: + - `docker compose run --rm api pytest -q` — **26 passed**, including `tests/test_return.py` (success/canonical-update, S1 regression scenario by name, damage→blocked, idempotent replay, reject-already-returned, missing-header validation, and a real multi-threaded concurrent-submission test against Postgres asserting exactly 1×201 + 2×409). + - `docker compose run --rm api ruff check .` — All checks passed. + - `npm run build` — clean. + - `docker compose up -d --build` (all services) then `docker compose exec api python -m app.cli seed --reset`, then a full browser run of the S1 demo scenario against `BK-DEMO-RETURN`/`MO-024`: submitted 53000 km (below canonical 54820) → result panel showed `INSP-0076`, `resulting_vehicle_status: maintenance` (correctly derived, since canonical 54820 ≥ `next_service_km` 40000), `DQ-RET-0076` created, automation event queued with a real UUID, "no upcoming booking" risk; vehicle detail page confirmed odometer unchanged at 54,820 km and a "Needs attention" badge. + - Both real defects listed above (form disappearing before showing its result; `event_id` reading as `None`) were **found via the browser run, not by pytest** — the test suite asserted on API response shape/values, not on what the UI actually rendered after a status transition. Worth remembering for M3+: UI state-after-mutation bugs need a browser check, not just API tests. + ## Known blockers None. External service credentials may be absent; use the documented demo/degraded providers. ## Exact next action -Start M2 (vehicle return vertical slice): read `docs/08-return-workflow.md`, `contracts/events.schema.json`. Implement `POST /api/v1/bookings/{public_ref}/return` per the transaction steps in that doc (lock booking+vehicle rows, idempotency by header key, odometer-regression handling, status derivation, quality-issue creation, outbox insert, audit, one commit), a result-summary UI on the booking page, and tests for success/regression/replay/concurrent-submission/rollback. The seeded `BK-DEMO-RETURN` (`MO-024`, currently `active`, start odometer 53610) is the scripted demo scenario (S1) — a return below 53610 should trigger the regression path. +Start M3 (Data Quality Workbench): read `docs/07-data-quality.md`. Implement the five rule scanners (possible_duplicate_customer with weighted scoring, missing_required_field, odometer_regression, booking_overlap, vehicle_status_conflict) as an explicit scan service (idempotent by `(rule_type, entity_type, entity_id, evidence fingerprint)` while open, per the doc's lifecycle section), issue defer/reject/merge-customers endpoints, a transactional customer-merge (rewires bookings, tombstones the loser, audits before/after), the Data Quality nav item + Workbench UI (two-column duplicate comparison, evidence/proposed-resolution for other types), and wire "open quality issues" into the dashboard attention section it already reads from. Seed data already has 15 `data_quality_issues` including the named `DQ-DEMO-*` scenarios (S2 duplicate CUS-0012/CUS-0178, S4 overlap MO-016) to scan/resolve against — remember M2's return workflow also creates ad-hoc `odometer_regression` issues (`DQ-RET-*`) directly, so the scan service's idempotent-fingerprint rule must not double-report those. diff --git a/backend/alembic/versions/e7b08389f47f_idempotency_records.py b/backend/alembic/versions/e7b08389f47f_idempotency_records.py new file mode 100644 index 0000000..b9b7977 --- /dev/null +++ b/backend/alembic/versions/e7b08389f47f_idempotency_records.py @@ -0,0 +1,41 @@ +"""idempotency records + +Revision ID: e7b08389f47f +Revises: c9498525abb5 +Create Date: 2026-08-01 21:24:35.472070 + +""" +from typing import Sequence, Union + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +# revision identifiers, used by Alembic. +revision: str = 'e7b08389f47f' +down_revision: Union[str, None] = 'c9498525abb5' +branch_labels: Union[str, Sequence[str], None] = None +depends_on: Union[str, Sequence[str], None] = None + + +def upgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.create_table('idempotency_records', + sa.Column('idempotency_key', sa.String(length=128), nullable=False), + sa.Column('booking_id', sa.UUID(), nullable=False), + sa.Column('response_status', sa.Integer(), nullable=False), + sa.Column('response_body', postgresql.JSONB(astext_type=sa.Text()), nullable=False), + sa.Column('id', sa.UUID(), nullable=False), + sa.Column('created_at', sa.DateTime(timezone=True), server_default=sa.text('now()'), nullable=False), + sa.Column('updated_at', sa.DateTime(timezone=True), server_default=sa.text('now()'), nullable=False), + sa.ForeignKeyConstraint(['booking_id'], ['bookings.id'], ), + sa.PrimaryKeyConstraint('id'), + sa.UniqueConstraint('idempotency_key') + ) + # ### end Alembic commands ### + + +def downgrade() -> None: + # ### commands auto generated by Alembic - please adjust! ### + op.drop_table('idempotency_records') + # ### end Alembic commands ### diff --git a/backend/app/api/routers/bookings.py b/backend/app/api/routers/bookings.py index 752d73d..11ffacd 100644 --- a/backend/app/api/routers/bookings.py +++ b/backend/app/api/routers/bookings.py @@ -1,6 +1,6 @@ from __future__ import annotations -from fastapi import APIRouter, Depends, HTTPException, Query +from fastapi import APIRouter, Depends, Header, HTTPException, Query, Response from sqlalchemy import select from sqlalchemy.orm import Session @@ -8,7 +8,8 @@ from app.api.deps import get_current_user, get_db from app.models.booking import Booking from app.models.customer import Customer from app.models.vehicle import Vehicle -from app.schemas import BookingOut, CurrentUser +from app.schemas import BookingOut, CurrentUser, RegisterReturnRequest +from app.services.returns import register_vehicle_return router = APIRouter(prefix="/api/v1/bookings", tags=["bookings"]) @@ -61,3 +62,17 @@ def get_booking( customer = db.get(Customer, booking.customer_id) vehicle = db.get(Vehicle, booking.vehicle_id) return _to_out(booking, customer, vehicle) + + +@router.post("/{public_ref}/return") +def register_return( + public_ref: str, + body: RegisterReturnRequest, + response: Response, + idempotency_key: str = Header(..., alias="Idempotency-Key", min_length=8, max_length=128), + db: Session = Depends(get_db), + user: CurrentUser = Depends(get_current_user), +) -> dict: + status_code, result = register_vehicle_return(db, public_ref, body, idempotency_key, user) + response.status_code = status_code + return result diff --git a/backend/app/models/__init__.py b/backend/app/models/__init__.py index 2a63a47..dabc437 100644 --- a/backend/app/models/__init__.py +++ b/backend/app/models/__init__.py @@ -3,6 +3,7 @@ from app.models.audit import AuditEvent from app.models.booking import Booking from app.models.customer import Customer from app.models.data_quality import DataQualityIssue +from app.models.idempotency import IdempotencyRecord from app.models.inspection import Inspection from app.models.maintenance import MaintenanceRecord from app.models.outbox import OutboxEvent @@ -15,6 +16,7 @@ __all__ = [ "Booking", "Customer", "DataQualityIssue", + "IdempotencyRecord", "Inspection", "MaintenanceRecord", "OutboxEvent", diff --git a/backend/app/models/idempotency.py b/backend/app/models/idempotency.py new file mode 100644 index 0000000..7101d9c --- /dev/null +++ b/backend/app/models/idempotency.py @@ -0,0 +1,19 @@ +import uuid + +from sqlalchemy import ForeignKey, Integer, String +from sqlalchemy.dialects.postgresql import JSONB, UUID +from sqlalchemy.orm import Mapped, mapped_column + +from app.core.db import Base +from app.models.mixins import TimestampMixin, UUIDPrimaryKeyMixin + + +class IdempotencyRecord(UUIDPrimaryKeyMixin, TimestampMixin, Base): + __tablename__ = "idempotency_records" + + idempotency_key: Mapped[str] = mapped_column(String(128), unique=True, nullable=False) + booking_id: Mapped[uuid.UUID] = mapped_column( + UUID(as_uuid=True), ForeignKey("bookings.id"), nullable=False + ) + response_status: Mapped[int] = mapped_column(Integer, nullable=False) + response_body: Mapped[dict] = mapped_column(JSONB, nullable=False) diff --git a/backend/app/schemas.py b/backend/app/schemas.py index a758092..d756aea 100644 --- a/backend/app/schemas.py +++ b/backend/app/schemas.py @@ -3,7 +3,7 @@ from __future__ import annotations from datetime import datetime from typing import Any, Literal -from pydantic import BaseModel, Field +from pydantic import BaseModel, Field, conint Role = Literal["operations_manager", "rental_employee"] @@ -48,6 +48,32 @@ class BookingOut(BookingSummaryOut): customer_name: str +class RegisterReturnRequest(BaseModel): + end_odometer_km: conint(ge=0) + fuel_level_percent: conint(ge=0, le=100) + cleanliness_ok: bool + damage_reported: bool + technical_warning: bool + notes: str | None = Field(default=None, max_length=2000) + + +class NextBookingRisk(BaseModel): + booking_ref: str + starts_at: datetime + at_risk: bool + + +class RegisterReturnResult(BaseModel): + booking_ref: str + vehicle_ref: str + inspection_ref: str + resulting_vehicle_status: str + odometer_regression: bool + quality_issue_ref: str | None + workflow_event_id: str + next_booking_risk: NextBookingRisk | None + + class InspectionOut(BaseModel): public_ref: str booking_ref: str diff --git a/backend/app/seed_loader.py b/backend/app/seed_loader.py index bec7ad2..df9a266 100644 --- a/backend/app/seed_loader.py +++ b/backend/app/seed_loader.py @@ -14,6 +14,7 @@ from app.models.audit import AuditEvent from app.models.booking import Booking from app.models.customer import Customer from app.models.data_quality import DataQualityIssue +from app.models.idempotency import IdempotencyRecord from app.models.inspection import Inspection from app.models.maintenance import MaintenanceRecord from app.models.outbox import OutboxEvent @@ -68,6 +69,7 @@ def clear_all(db: Session) -> None: for model in ( AuditEvent, OutboxEvent, + IdempotencyRecord, DataQualityIssue, Inspection, MaintenanceRecord, diff --git a/backend/app/services/returns.py b/backend/app/services/returns.py new file mode 100644 index 0000000..b4e3bcf --- /dev/null +++ b/backend/app/services/returns.py @@ -0,0 +1,247 @@ +from __future__ import annotations + +import uuid +from datetime import UTC, datetime + +from sqlalchemy import select +from sqlalchemy.exc import IntegrityError +from sqlalchemy.orm import Session + +from app.core.errors import AppError +from app.models.booking import Booking +from app.models.data_quality import DataQualityIssue +from app.models.idempotency import IdempotencyRecord +from app.models.inspection import Inspection +from app.models.outbox import OutboxEvent +from app.models.vehicle import Vehicle +from app.schemas import CurrentUser, RegisterReturnRequest +from app.services.audit import record_audit_event + +REF_PREFIX = "INSP" + + +def _next_public_ref(db: Session) -> str: + existing = db.execute(select(Inspection.public_ref)).scalars().all() + return f"{REF_PREFIX}-{len(existing) + 1:04d}" + + +def _derive_vehicle_status(body: RegisterReturnRequest, vehicle: Vehicle, new_odometer: int) -> str: + if body.damage_reported or body.technical_warning: + return "blocked" + if new_odometer >= vehicle.next_service_km: + return "maintenance" + return "cleaning" + + +def register_vehicle_return( + db: Session, + booking_ref: str, + body: RegisterReturnRequest, + idempotency_key: str, + actor: CurrentUser, +) -> tuple[int, dict]: + existing = db.scalar( + select(IdempotencyRecord).where(IdempotencyRecord.idempotency_key == idempotency_key) + ) + if existing is not None: + booking = db.get(Booking, existing.booking_id) + if booking is None or booking.public_ref != booking_ref: + raise AppError( + "IDEMPOTENCY_KEY_REUSED", + "This idempotency key was already used for a different booking.", + status_code=409, + ) + return existing.response_status, existing.response_body + + booking = db.scalar(select(Booking).where(Booking.public_ref == booking_ref).with_for_update()) + if booking is None: + raise AppError("BOOKING_NOT_FOUND", "Booking not found.", status_code=404) + vehicle = db.scalar(select(Vehicle).where(Vehicle.id == booking.vehicle_id).with_for_update()) + + # Re-check after acquiring the row lock: a concurrent identical-key request may have + # just committed while we were waiting. + existing = db.scalar( + select(IdempotencyRecord).where(IdempotencyRecord.idempotency_key == idempotency_key) + ) + if existing is not None: + return existing.response_status, existing.response_body + + if booking.status != "active": + raise AppError( + "INVALID_BOOKING_STATE", + f"Booking is '{booking.status}', not 'active'; it cannot be returned.", + status_code=409, + ) + + now = datetime.now(UTC) + correlation_id = uuid.uuid4() + + inspection = Inspection( + public_ref=_next_public_ref(db), + booking_id=booking.id, + vehicle_id=vehicle.id, + type="return", + fuel_level_percent=body.fuel_level_percent, + cleanliness_ok=body.cleanliness_ok, + damage_reported=body.damage_reported, + technical_warning=body.technical_warning, + notes=body.notes, + odometer_km=body.end_odometer_km, + completed_at=now, + completed_by=actor.display_name, + ) + db.add(inspection) + + before_vehicle = { + "operational_status": vehicle.operational_status, + "odometer_km": vehicle.odometer_km, + } + + booking.status = "returned" + booking.end_odometer_km = body.end_odometer_km + + odometer_regression = body.end_odometer_km < vehicle.odometer_km + quality_issue_ref: str | None = None + canonical_odometer = vehicle.odometer_km + if not odometer_regression: + canonical_odometer = body.end_odometer_km + vehicle.odometer_km = canonical_odometer + else: + issue = DataQualityIssue( + public_ref=f"DQ-RET-{str(inspection.public_ref).split('-')[-1]}", + rule_type="odometer_regression", + entity_type="vehicle", + entity_id=vehicle.id, + severity="medium", + status="open", + evidence_json={ + "summary": ( + f"Return submitted {body.end_odometer_km} km, below canonical " + f"{vehicle.odometer_km} km." + ), + "entity_ref": vehicle.public_ref, + "related_refs": [booking.public_ref, inspection.public_ref], + }, + proposed_action_json={}, + detected_at=now, + ) + db.add(issue) + db.flush() + quality_issue_ref = issue.public_ref + + resulting_status = _derive_vehicle_status(body, vehicle, canonical_odometer) + vehicle.operational_status = resulting_status + vehicle.version += 1 + + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="return_registered", + entity_type="booking", + entity_id=booking.id, + correlation_id=correlation_id, + before={"status": "active"}, + after={"status": "returned", "end_odometer_km": body.end_odometer_km}, + metadata={"idempotency_key": idempotency_key}, + ) + record_audit_event( + db, + actor_type="user", + actor_label=actor.display_name, + action="vehicle_status_changed", + entity_type="vehicle", + entity_id=vehicle.id, + correlation_id=correlation_id, + before=before_vehicle, + after={ + "operational_status": vehicle.operational_status, + "odometer_km": vehicle.odometer_km, + }, + ) + + attention_reasons = [] + if body.damage_reported: + attention_reasons.append("damage_reported") + if body.technical_warning: + attention_reasons.append("technical_warning") + if odometer_regression: + attention_reasons.append("odometer_regression") + + event = OutboxEvent( + event_id=uuid.uuid4(), + event_type="vehicle.returned.v1", + aggregate_type="booking", + aggregate_id=booking.id, + payload_json={ + "event_type": "vehicle.returned.v1", + "correlation_id": str(correlation_id), + "aggregate": { + "type": "booking", + "id": str(booking.id), + "public_ref": booking.public_ref, + }, + "data": { + "vehicle_ref": vehicle.public_ref, + "inspection_ref": inspection.public_ref, + "resulting_vehicle_status": resulting_status, + "attention_reasons": attention_reasons, + }, + "aggregate_ref": booking.public_ref, + }, + occurred_at=now, + delivery_status="pending", + attempts=0, + ) + db.add(event) + + next_booking = db.scalar( + select(Booking) + .where( + Booking.vehicle_id == vehicle.id, + Booking.status == "reserved", + Booking.starts_at > now, + ) + .order_by(Booking.starts_at.asc()) + ) + next_booking_risk = None + if next_booking is not None: + hours_until = (next_booking.starts_at - now).total_seconds() / 3600 + next_booking_risk = { + "booking_ref": next_booking.public_ref, + "starts_at": next_booking.starts_at.isoformat(), + "at_risk": resulting_status != "cleaning" or hours_until < 4, + } + + response_body = { + "booking_ref": booking.public_ref, + "vehicle_ref": vehicle.public_ref, + "inspection_ref": inspection.public_ref, + "resulting_vehicle_status": resulting_status, + "odometer_regression": odometer_regression, + "quality_issue_ref": quality_issue_ref, + "workflow_event_id": str(event.event_id), + "next_booking_risk": next_booking_risk, + } + + db.add( + IdempotencyRecord( + idempotency_key=idempotency_key, + booking_id=booking.id, + response_status=201, + response_body=response_body, + ) + ) + + try: + db.commit() + except IntegrityError: + db.rollback() + existing = db.scalar( + select(IdempotencyRecord).where(IdempotencyRecord.idempotency_key == idempotency_key) + ) + if existing is not None: + return existing.response_status, existing.response_body + raise + + return 201, response_body diff --git a/backend/tests/test_return.py b/backend/tests/test_return.py new file mode 100644 index 0000000..fb3d219 --- /dev/null +++ b/backend/tests/test_return.py @@ -0,0 +1,160 @@ +import threading + +from fastapi.testclient import TestClient +from sqlalchemy import select + +from app.core.db import SessionLocal +from app.main import app +from app.models.booking import Booking +from app.models.vehicle import Vehicle + + +def _return_body(**overrides): + body = { + "end_odometer_km": 54200, + "fuel_level_percent": 60, + "cleanliness_ok": True, + "damage_reported": False, + "technical_warning": False, + "notes": "Handed back on time.", + } + body.update(overrides) + return body + + +def _activate_booking(vehicle_ref: str, start_odometer_km: int) -> str: + """Flip one returned booking for the given vehicle back to 'active' for a fresh test fixture.""" + db = SessionLocal() + try: + vehicle = db.scalar(select(Vehicle).where(Vehicle.public_ref == vehicle_ref)) + booking = db.scalar( + select(Booking).where(Booking.vehicle_id == vehicle.id, Booking.status == "returned") + ) + booking.status = "active" + booking.start_odometer_km = start_odometer_km + booking.end_odometer_km = None + db.commit() + return booking.public_ref + finally: + db.close() + + +def test_register_return_success_updates_canonical_odometer(ops_client): + booking_ref = _activate_booking("MO-003", start_odometer_km=20000) + vehicle_before = ops_client.get("/api/v1/vehicles/MO-003").json() + new_reading = vehicle_before["odometer_km"] + 10 + + response = ops_client.post( + f"/api/v1/bookings/{booking_ref}/return", + json=_return_body(end_odometer_km=new_reading), + headers={"Idempotency-Key": "test-return-success-001"}, + ) + assert response.status_code == 201 + body = response.json() + assert body["resulting_vehicle_status"] in ("cleaning", "maintenance") + assert body["odometer_regression"] is False + assert body["quality_issue_ref"] is None + + booking = ops_client.get(f"/api/v1/bookings/{booking_ref}").json() + assert booking["status"] == "returned" + assert booking["end_odometer_km"] == new_reading + + vehicle = ops_client.get("/api/v1/vehicles/MO-003").json() + assert vehicle["odometer_km"] == new_reading + assert vehicle["operational_status"] in ("cleaning", "maintenance") + + +def test_register_return_matches_s1_demo_scenario(ops_client): + vehicle_before = ops_client.get("/api/v1/vehicles/MO-024").json() + low_reading = vehicle_before["odometer_km"] - 500 + + response = ops_client.post( + "/api/v1/bookings/BK-DEMO-RETURN/return", + json=_return_body(end_odometer_km=low_reading), + headers={"Idempotency-Key": "test-return-s1-001"}, + ) + assert response.status_code == 201 + body = response.json() + assert body["odometer_regression"] is True + assert body["quality_issue_ref"] is not None + + vehicle = ops_client.get("/api/v1/vehicles/MO-024").json() + assert vehicle["odometer_km"] == vehicle_before["odometer_km"] # canonical odometer unchanged + booking = ops_client.get("/api/v1/bookings/BK-DEMO-RETURN").json() + assert booking["status"] == "returned" + assert booking["end_odometer_km"] == low_reading # submitted reading is still recorded + + +def test_register_return_damage_blocks_vehicle(employee_client): + booking_ref = _activate_booking("MO-005", start_odometer_km=22000) + response = employee_client.post( + f"/api/v1/bookings/{booking_ref}/return", + json=_return_body(end_odometer_km=22500, damage_reported=True), + headers={"Idempotency-Key": "test-return-damage-001"}, + ) + assert response.status_code == 201 + assert response.json()["resulting_vehicle_status"] == "blocked" + + +def test_register_return_replays_on_same_idempotency_key(ops_client): + booking_ref = _activate_booking("MO-008", start_odometer_km=24000) + key = "test-return-replay-001" + first = ops_client.post( + f"/api/v1/bookings/{booking_ref}/return", + json=_return_body(end_odometer_km=24500), + headers={"Idempotency-Key": key}, + ) + second = ops_client.post( + f"/api/v1/bookings/{booking_ref}/return", + json=_return_body(end_odometer_km=24500), + headers={"Idempotency-Key": key}, + ) + assert first.status_code == 201 + assert second.status_code == 201 + assert first.json() == second.json() + + +def test_register_return_rejects_already_returned_booking(ops_client): + booking_ref = _activate_booking("MO-010", start_odometer_km=25000) + ops_client.post( + f"/api/v1/bookings/{booking_ref}/return", + json=_return_body(end_odometer_km=25500), + headers={"Idempotency-Key": "test-return-double-001"}, + ) + second = ops_client.post( + f"/api/v1/bookings/{booking_ref}/return", + json=_return_body(end_odometer_km=25999), + headers={"Idempotency-Key": "test-return-double-002"}, + ) + assert second.status_code == 409 + assert second.json()["error"]["code"] == "INVALID_BOOKING_STATE" + + +def test_register_return_requires_idempotency_key(ops_client): + booking_ref = _activate_booking("MO-012", start_odometer_km=26500) + response = ops_client.post(f"/api/v1/bookings/{booking_ref}/return", json=_return_body()) + assert response.status_code == 422 + + +def test_concurrent_returns_only_one_succeeds(): + booking_ref = _activate_booking("MO-013", start_odometer_km=27000) + results: list[int] = [] + + def submit(key: str) -> None: + client = TestClient(app) + client.post("/api/v1/demo/login", json={"role": "operations_manager"}) + resp = client.post( + f"/api/v1/bookings/{booking_ref}/return", + json=_return_body(end_odometer_km=27500), + headers={"Idempotency-Key": key}, + ) + results.append(resp.status_code) + + threads = [threading.Thread(target=submit, args=(f"concurrent-key-{i}",)) for i in range(3)] + for t in threads: + t.start() + for t in threads: + t.join() + + assert results.count(201) == 1 + assert results.count(409) == 2 diff --git a/backend/tests/test_seed.py b/backend/tests/test_seed.py index e18ebac..b2f837e 100644 --- a/backend/tests/test_seed.py +++ b/backend/tests/test_seed.py @@ -7,11 +7,16 @@ from app.models.data_quality import DataQualityIssue from app.models.outbox import OutboxEvent from app.models.user import User from app.models.vehicle import Vehicle +from app.seed_loader import reset_and_seed def test_seed_counts_match_deterministic_dataset(): + # Other test modules mutate shared demo state (returns, resets), so this test + # re-seeds immediately before asserting counts rather than trusting whatever + # order pytest happened to run modules in. db = SessionLocal() try: + reset_and_seed(db) 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 @@ -25,6 +30,8 @@ def test_seed_counts_match_deterministic_dataset(): def test_seed_demo_scenarios_present(): db = SessionLocal() try: + reset_and_seed(db) + booking = db.scalar(select(Booking).where(Booking.public_ref == "BK-DEMO-RETURN")) assert booking is not None assert booking.status == "active" diff --git a/frontend/src/api/types.ts b/frontend/src/api/types.ts index e60c928..02c5e4f 100644 --- a/frontend/src/api/types.ts +++ b/frontend/src/api/types.ts @@ -118,6 +118,32 @@ export interface Dashboard { recent_automation: AutomationRun[]; } +export interface RegisterReturnRequest { + end_odometer_km: number; + fuel_level_percent: number; + cleanliness_ok: boolean; + damage_reported: boolean; + technical_warning: boolean; + notes?: string; +} + +export interface NextBookingRisk { + booking_ref: string; + starts_at: string; + at_risk: boolean; +} + +export interface RegisterReturnResult { + booking_ref: string; + vehicle_ref: string; + inspection_ref: string; + resulting_vehicle_status: string; + odometer_regression: boolean; + quality_issue_ref: string | null; + workflow_event_id: string; + next_booking_risk: NextBookingRisk | null; +} + export interface AuditEvent { id: string; actor_type: string; diff --git a/frontend/src/components/ReturnForm.tsx b/frontend/src/components/ReturnForm.tsx new file mode 100644 index 0000000..0b262f1 --- /dev/null +++ b/frontend/src/components/ReturnForm.tsx @@ -0,0 +1,159 @@ +import { useState, type FormEvent } from "react"; +import { Link } from "react-router-dom"; +import { api, ApiError } from "../api/client"; +import type { RegisterReturnRequest, RegisterReturnResult } from "../api/types"; + +function newIdempotencyKey(): string { + return typeof crypto.randomUUID === "function" + ? crypto.randomUUID() + : `key-${Date.now()}-${Math.random().toString(16).slice(2)}`; +} + +export function ReturnResultPanel({ result }: { result: RegisterReturnResult }) { + return ( +
+

Return registered

+
+
Inspection
{result.inspection_ref}
+
Resulting vehicle status
{result.resulting_vehicle_status}
+
+
Quality issue
+
{result.quality_issue_ref ?? "None created"}
+
+
Automation event
Queued ({result.workflow_event_id.slice(0, 8)})
+
+
Next booking risk
+
+ {result.next_booking_risk + ? `${result.next_booking_risk.booking_ref} ${result.next_booking_risk.at_risk ? "— may be affected" : "— low risk"}` + : "No upcoming booking for this vehicle"} +
+
+
+ {result.odometer_regression && ( +

+ The submitted odometer reading was below the vehicle's canonical odometer. It was + recorded as-is; the canonical odometer was not changed, and a data-quality issue was + opened for review. +

+ )} +

+ View vehicle {result.vehicle_ref} +

+
+ ); +} + +export function ReturnForm({ + bookingRef, + onRegistered, +}: { + bookingRef: string; + onRegistered: (result: RegisterReturnResult) => void; +}) { + const [odometer, setOdometer] = useState(""); + const [fuel, setFuel] = useState("50"); + const [cleanlinessOk, setCleanlinessOk] = useState(true); + const [damageReported, setDamageReported] = useState(false); + const [technicalWarning, setTechnicalWarning] = useState(false); + const [notes, setNotes] = useState(""); + const [submitting, setSubmitting] = useState(false); + const [error, setError] = useState(null); + const [idempotencyKey] = useState(newIdempotencyKey); + + async function handleSubmit(e: FormEvent) { + e.preventDefault(); + setError(null); + setSubmitting(true); + try { + const body: RegisterReturnRequest = { + end_odometer_km: Number(odometer), + fuel_level_percent: Number(fuel), + cleanliness_ok: cleanlinessOk, + damage_reported: damageReported, + technical_warning: technicalWarning, + notes: notes || undefined, + }; + const registered = await api.post( + `/api/v1/bookings/${bookingRef}/return`, + body, + { "Idempotency-Key": idempotencyKey }, + ); + onRegistered(registered); + } catch (err) { + if (err instanceof ApiError) { + setError(err.message); + } else { + setError("Could not register the return. Please try again."); + } + } finally { + setSubmitting(false); + } + } + + return ( +
+

Register return

+ {error &&

{error}

} + + + + + + + + + + + +