Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 20 additions & 0 deletions CONTEXT.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,20 @@
# CONTEXT — appointments-api domain terms

A short glossary of the terms tickets and ADRs use. Add a term when a ticket introduces one.

- **Appointment** — a booked slot for a patient with a clinician at a clinic. Statuses:
`REQUESTED`, `CONFIRMED`, `COMPLETED`, `CANCELLED`, `NO_SHOW`.
- **Cancel** — `POST /api/v1/appointments/{id}/cancel`. The only way to reach `CANCELLED`. Needs a
current `If-Match` ETag (ADR 0008) and respects the clinic's cancellation cutoff.
- **Cancellation cutoff** — hours before `starts_at` after which a patient or clinician may no longer
cancel; a clinic admin or platform admin still may.
- **Refused cancel** — a cancel request that ends in an error response (404, 403, 428, 412, 409)
and changes nothing.
- **Audit log** — the append-only `audit_log` table and its admin-only read endpoint
`GET /api/v1/audit-log` (ADR 0015). Rows are never edited.
- **Audit entry** — one row: `actor_id`, `action`, `entity_type`, `entity_id`, `before`, `after`,
`created_at`.
- **Audit action** — the `AuditAction` value naming what happened, e.g. `appointment.cancelled`
(ADR 0016).
- **Audit writer** — the `AuditWriter` Protocol the audit service records through; implemented by
`AuditLogRepository` (ADR 0016).
22 changes: 11 additions & 11 deletions docs/KNOWN_GAPS.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,19 +2,19 @@

Honest list of what is deliberately incomplete, and why. Updated as milestones land.

## Audit log has no write instrumentation
## Audit log has no write instrumentation (except appointment cancellation)

The `audit_log` table exists (migration 0001) and now has a read API — the admin-only,
The `audit_log` table exists (migration 0001) and has a read API — the admin-only,
keyset-paginated, filterable `GET /api/v1/audit-log` added for the web audit-log page (milestone 13,
ADR 0015). But **nothing writes to it yet**: no service records an entry on appointment
create/transition/cancel, device provisioning, or excursion changes. So the endpoint returns an empty
list in production; the web app seeds rows only in its MSW mock so the page is demonstrable and
tested.

**To close:** add an audit-writer (a small service invoked from the domain state changes, ideally via
the same transaction as the change, mirroring the webhook-outbox direction) that records
`actor_id/action/entity_type/entity_id/before/after`. Deferred so milestone 13 stays a web milestone;
the read side is contract-driven and ready for the writer.
ADR 0015). Since AURORA-2, **appointment cancellation is recorded** (ADR 0016): every successful
`POST /api/v1/appointments/{id}/cancel` writes one `appointment.cancelled` row in the same
transaction as the cancel, through `services/audit.py`. **The other actions still write nothing**:
appointment create, status transitions (confirm, complete, no-show), device provisioning, and
excursion changes. So the endpoint lists only cancellations in production.

**To close:** call `services.audit.record` from each remaining domain state change, in the same
transaction as the change, adding an `AuditAction` member per action (the writer, the
`AuditLogRepository.add_entry` implementation, and the read side are in place).

## CLINIC_ADMIN is not clinic-scoped yet

Expand Down
39 changes: 39 additions & 0 deletions docs/adr/0016-audit-log-write-on-appointment-cancel.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,39 @@
# ADR 0016 — Audit-log write on appointment cancel

Status: accepted (AURORA-2)

## Context

`GET /api/v1/audit-log` exists (ADR 0015), but nothing writes to the `audit_log` table, so the endpoint
returns an empty list against the real API (`docs/KNOWN_GAPS.md`). AURORA-2 adds the first write
path: one entry per successful appointment cancellation. Other actions (create, transition, devices,
excursions) come later and should reuse the same writer.

## Decision

- **A small audit service behind a Protocol.** `services/audit.py` exposes
`record(writer, *, actor_id, action, entity_type, entity_id, before, after)` over an `AuditWriter`
`typing.Protocol`. It has no FastAPI imports, so it is unit-tested with a fake writer and no I/O.
`AuditLogRepository.add_entry(...)` implements `AuditWriter`.
- **A typed action vocabulary.** A new `AuditAction` StrEnum in `enums.py`, starting with
`APPOINTMENT_CANCELLED = "appointment.cancelled"`. It is separate from `WebhookEventType`: the
webhook set is a public contract, and audit actions will grow past it.
- **Same transaction as the cancel.** `cancel_appointment` flushes the cancel, then records the
audit entry, then publishes the webhook. `add_entry` flushes, so an insert failure raises inside
the handler: the request session rolls the cancel back (500) and no webhook is published.
- **Row content.** `actor_id` is the authenticated user, `entity_type` is `"appointment"`,
`entity_id` is the appointment UUID as a string, `before` is `{"status": <old status>}` and `after`
is `{"status": "CANCELLED"}`. The cancellation reason is not copied into the audit row.
- **Refused cancels write nothing.** Every refusal (404, 403, 428, 412, 409) is raised before the
write. A lost-update race fails at the cancel's flush (`StaleDataError` → 412), before the write.

## Consequences

- A cancellation cannot commit without its audit row. A broken audit table blocks cancellations;
that is the intended trade.
- No migration (the table exists since 0001) and no contract change: `AuditLogEntryOut.action`
stays `str`, so `contracts/openapi.json` and the web client are unchanged.
- The `KNOWN_GAPS.md` entry narrows to the actions that still have no writer.
- Two concurrent cancels with the same `If-Match` always end as one 200 and one 412 with exactly one
audit row, in any interleaving, so the race is testable with `asyncio.gather` and an outcome-set
assertion.
17 changes: 16 additions & 1 deletion src/appointments_api/api/routers/appointments.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@
from appointments_api.api.conditional import if_match_satisfied, make_etag
from appointments_api.api.deps import (
AppointmentRepoDep,
AuditLogRepoDep,
ClinicianRepoDep,
ClinicRepoDep,
ClockDep,
Expand Down Expand Up @@ -52,8 +53,9 @@
AppointmentTransition,
CancelRequest,
)
from appointments_api.enums import AppointmentStatus, UserRole
from appointments_api.enums import AppointmentStatus, AuditAction, UserRole
from appointments_api.models import Appointment, User
from appointments_api.services import audit
from appointments_api.services.appointments.cancellation import assert_can_cancel
from appointments_api.services.appointments.state_machine import assert_transition
from appointments_api.services.availability import fits_working_hours
Expand Down Expand Up @@ -350,6 +352,7 @@ async def cancel_appointment(
clinicians: ClinicianRepoDep,
clock: ClockDep,
dispatcher: WebhookDispatcherDep,
audit_log: AuditLogRepoDep,
if_match: Annotated[str | None, Header(alias="If-Match")] = None,
) -> Response:
appointment = await appointments.get(appointment_id)
Expand All @@ -368,8 +371,20 @@ async def cancel_appointment(
)
_require_if_match(if_match, appointment.version)
assert_transition(appointment.status, AppointmentStatus.CANCELLED)
previous_status = appointment.status
appointment.status = AppointmentStatus.CANCELLED
appointment.cancellation_reason = body.reason
# Flush first: a lost-update race fails here (412) before anything is audited. The audit row
# then joins the same transaction, so the cancel cannot commit without it (ADR 0016).
await appointments.flush()
await audit.record(
audit_log,
actor_id=current.id,
action=AuditAction.APPOINTMENT_CANCELLED,
entity_type="appointment",
entity_id=str(appointment.id),
before={"status": previous_status.value},
after={"status": AppointmentStatus.CANCELLED.value},
)
await _emit(dispatcher, clock, WebhookEventType.APPOINTMENT_CANCELLED, appointment)
return _appointment_response(appointment)
10 changes: 10 additions & 0 deletions src/appointments_api/enums.py
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,16 @@ class AppointmentStatus(StrEnum):
NO_SHOW = "NO_SHOW"


class AuditAction(StrEnum):
"""What an ``audit_log`` row records (ADR 0016).

Kept apart from ``WebhookEventType``: the webhook set is a public contract, while audit
actions will grow past it (device provisioning, excursions, ...).
"""

APPOINTMENT_CANCELLED = "appointment.cancelled"


class Weekday(IntEnum):
"""Weekday numbering that matches :meth:`datetime.date.weekday` (Monday is 0)."""

Expand Down
34 changes: 30 additions & 4 deletions src/appointments_api/repositories/audit_log.py
Original file line number Diff line number Diff line change
@@ -1,14 +1,16 @@
"""Audit-log repository (read side).
"""Audit-log repository.

The ``audit_log`` table is append-only (see ``models/audit.py``). This repository is the read side:
an admin-facing, keyset-paginated, filterable listing. Filters are optional and combine with AND;
ordering and the cursor come from the shared ``keyset_page`` helper (``(created_at, id)`` DESC).
The ``audit_log`` table is append-only (see ``models/audit.py``). The read side is an admin-facing,
keyset-paginated, filterable listing. Filters are optional and combine with AND; ordering and the
cursor come from the shared ``keyset_page`` helper (``(created_at, id)`` DESC). The write side is
``add_entry``, which implements ``services.audit.AuditWriter`` (ADR 0016).
"""

from __future__ import annotations

import uuid
from collections.abc import Sequence
from typing import Any

from sqlalchemy import select
from sqlalchemy.ext.asyncio import AsyncSession
Expand All @@ -22,6 +24,30 @@ class AuditLogRepository:
def __init__(self, session: AsyncSession) -> None:
self._s = session

async def add_entry(
self,
*,
actor_id: uuid.UUID | None,
action: str,
entity_type: str,
entity_id: str,
before: dict[str, Any] | None,
after: dict[str, Any] | None,
) -> None:
"""Insert one audit row in the caller's transaction and flush it, so an insert failure
raises here and the surrounding request rolls back."""
self._s.add(
AuditLogEntry(
actor_id=actor_id,
action=action,
entity_type=entity_type,
entity_id=entity_id,
before=before,
after=after,
)
)
await self._s.flush()

async def list_entries(
self,
*,
Expand Down
47 changes: 47 additions & 0 deletions src/appointments_api/services/audit.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
"""Audit-log writing (ADR 0016).

``record`` writes one entry through an ``AuditWriter``. The writer is a ``typing.Protocol`` so the
service has no database or FastAPI dependency: in the app it is ``AuditLogRepository`` (which writes
in the request's transaction), in unit tests a fake that keeps entries in memory.
"""

from __future__ import annotations

import uuid
from typing import Any, Protocol

from appointments_api.enums import AuditAction


class AuditWriter(Protocol):
async def add_entry(
self,
*,
actor_id: uuid.UUID | None,
action: str,
entity_type: str,
entity_id: str,
before: dict[str, Any] | None,
after: dict[str, Any] | None,
) -> None: ...


async def record(
writer: AuditWriter,
*,
actor_id: uuid.UUID | None,
action: AuditAction,
entity_type: str,
entity_id: str,
before: dict[str, Any] | None,
after: dict[str, Any] | None,
) -> None:
"""Write one audit entry: ``actor_id`` did ``action`` to ``entity_type``/``entity_id``."""
await writer.add_entry(
actor_id=actor_id,
action=action.value,
entity_type=entity_type,
entity_id=entity_id,
before=before,
after=after,
)
Loading
Loading