Skip to content

feat: Add Banners Table and API - #337

Open
camielvs wants to merge 1 commit into
TangleML:masterfrom
camielvs:cvs/banners
Open

feat: Add Banners Table and API#337
camielvs wants to merge 1 commit into
TangleML:masterfrom
camielvs:cvs/banners

Conversation

@camielvs

@camielvs camielvs commented Aug 18, 2026

Copy link
Copy Markdown
Collaborator

What this adds

A generic, site-wide announcement banner: a dedicated banner table, a public read endpoint for the frontend, and admin CRUD endpoints for managing banners. API only — there is no admin UI in this phase.

Design objective

Give operators a way to show a short, timed message to every user of the app (planned maintenance, degraded functionality, a link to a status page) without a deploy, and give the frontend a single cheap endpoint it can poll to render whatever is currently live. The banner content is deliberately generic — title, body, severity variant, optional link — with no assumptions about who is producing it or why.

API

Method Path Auth Purpose
GET /api/banners/active any user Banners that are live right now. Returns only display fields, sends Cache-Control: no-store.
GET /api/admin/banners admin All banners, newest first. ?include_deleted=true also returns soft-deleted ones.
POST /api/admin/banners admin Create.
GET /api/admin/banners/{id} admin Read one, including admin-only fields.
PATCH /api/admin/banners/{id} admin Partial update.
DELETE /api/admin/banners/{id} admin Soft delete.

A banner is active when deleted_at IS NULL AND is_enabled = true AND (starts_at IS NULL OR starts_at <= now) AND (ends_at IS NULL OR ends_at > now). Active banners are sorted by starts_at descending with un-scheduled banners last, then created_at descending.

You can exercise all of this from the /docs route.

image

Key decisions

A dedicated banner table instead of UserSettings. UserSettings is a per-user key/value JSON blob keyed by user_id; a banner is a global object with its own lifecycle, so it fits neither the key nor the shape. Storing banners there would mean either duplicating a banner into every user's settings row or inventing a magic pseudo-user to hold the global ones, and in both cases the scheduling window and the enabled/deleted flags would live inside opaque JSON — not queryable, not indexable, not constrainable. A table gives us a real WHERE clause for the active lookup, real indexes, and per-row audit columns.

Soft delete only. DELETE sets deleted_at and never removes the row, so a banner that was shown to users stays auditable and an accidental delete is recoverable. deleted_at IS NULL is part of both the active query and the default admin list; ?include_deleted=true opts back in. Delete is idempotent — deleting an already-deleted banner leaves the original timestamp alone.

Two response shapes rather than one. /api/banners/active returns 11 display fields; the admin endpoints add is_enabled, created_by, updated_by and deleted_at. Keeping them as separate response types (BannerResponse / AdminBannerResponse) means the public endpoint cannot leak operator identities by accident. This is also why the banner routes don't use the router's default_config: that config strips null fields, and the frontend wants url: null present rather than absent.

Reusing errors.ApiValidationError for validation failures. Invalid input (bad URL, over-length title/body, url_text without a url, ends_at <= starts_at) raises the existing ApiValidationError, which the existing handler maps to 422. No new error type and no new exception handler were added. An unknown variant is rejected by FastAPI request validation, so it also returns 422 — one status code for all bad input.

variant as a str enum column. BannerVariant (info / warning / success / error) is mapped onto the column with values_callable, matching how ContainerExecutionStatus is handled, so the DB stores "warning" rather than "WARNING" and the valid set shows up in the OpenAPI schema instead of living in a hand-written validator.

Client datetimes are normalized to UTC on the way in. The DB stores naive UTC (see UtcDateTime), which means a starts_at of 2026-01-01T12:00:00+02:00 would otherwise be stored as if 12:00 were UTC. It is converted to 10:00Z before being stored, and cross-field comparison normalizes both sides so a request-supplied aware datetime can be compared against a DB-loaded one.

Two indexes, matching the two access paths. (is_enabled, deleted_at, starts_at, ends_at) serves the active lookup that the frontend hits on every page load; created_at DESC serves the admin list.

Partial update semantics. PATCH follows the existing convention in this codebase (PublishedComponentService.update): a field that is null/absent is left unchanged. The trade-off is that PATCH cannot currently clear a nullable field back to null — {"url": null} is a no-op, not a clear. Worth revisiting if that turns out to matter, but it would need a departure from the convention (a sentinel or exclude_unset).

Schema migration

Purely additive: a new table plus its two indexes, created by the existing metadata.create_all. No existing table, column or index is touched, so no migrate_db step is needed. Verified by building a DB with the code at master, inserting a row, then running create_db_engine_and_migrate_db with this branch: banner and its indexes appear, every other table is unchanged, and the pre-existing row survives.

Testing

tests/test_banners_api.py adds 28 tests against a real TestClient app over an in-memory SQLite DB, covering: empty active list and the no-store header; create and read-back through the admin endpoints; the active window (enabled/in-window included, disabled/future/expired excluded); the public response exposing exactly the 11 display fields; partial update including trimming, updated_at advancing, and untouched fields staying put; soft delete removing the banner from both the active list and the default admin list while the row remains fetchable; 403 for a non-admin on every admin route while the public read still works; 422 for every invalid-input case; 404 for unknown ids; active sort order; and UTC conversion of offset datetimes.

Also verified by hand against a running server: create → active → disable → active empty → delete → row still present in SQLite.

@camielvs
camielvs requested a review from a team August 18, 2026 00:24
@camielvs
camielvs requested a review from Ark-kun as a code owner August 18, 2026 00:24
@camielvs
camielvs marked this pull request as draft August 18, 2026 00:25
@camielvs
camielvs marked this pull request as ready for review August 18, 2026 00:56

@morgan-wowk morgan-wowk left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Agent-assisted review (posted by Morgan) — lenses: correctness, security, performance/ops, UX, breaking-change/migration, and cross-dialect (SQLite-local / MySQL-prod) portability.

Overall this is a strong, well-tested PR that follows existing conventions (ApiValidationError→422, PublishedComponentService-style PATCH, values_callable enum, admin/readonly deps, separate public vs admin response shapes). None of the items below are hard blockers; the two most worth resolving before merge are the soft-delete restore gap and the MySQL DATETIME precision question. Inline comments follow, and a standalone comment with ERD + sequence/architecture diagrams is posted below as a reference. On the table-vs-settings thread: a dedicated table is the right call for this feature (real indexed active-window query, per-row audit, multiple concurrent banners); the generic-config concern is better addressed by a dedicated global-config table if/when feature-flags etc. actually arrive, rather than forcing banners into user-settings now.

)
title: orm.Mapped[str]
# `str` is mapped to VARCHAR(255) by default, which is too short for the body.
body: orm.Mapped[str] = orm.mapped_column(sql.Text())

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Security (plain-text contract): title/body/url_text are length-checked but not sanitized, and they're rendered to every user — so the XSS blast radius is site-wide if the tangle-ui consumer ever renders body as HTML/markdown-with-raw-HTML. url is correctly restricted to absolute http(s) (blocks javascript:), so just do the same discipline for text: document these as plain text here, and make sure the frontend PR renders them as text (or sanitizes).

# `mapped_column()` (despite having no arguments) is needed so that the columns
# can be referenced in `__table_args__` below.
created_at: orm.Mapped[datetime.datetime] = orm.mapped_column()
updated_at: orm.Mapped[datetime.datetime] = orm.mapped_column()

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Cross-dialect (SQLite tests vs MySQL prod): UtcDateTime is DateTime(timezone=True) with no fractional-seconds arg, which maps to MySQL DATETIME at second precision by default. SQLite keeps microseconds, so test_patch_updates_fields_and_updated_at's strict updated_at > created_at and the created_at DESC list ordering pass locally, but on MySQL two writes in the same second get equal timestamps → the 'updated_at advances' invariant can fail and the list tiebreak becomes non-deterministic. Please confirm prod columns are DATETIME(6) (or add a stable tiebreak). Standing rule: new DB interactions should be portable across dialects, not SQLite-only.

),
# Used by the admin banner list.
sql.Index(
"ix_banner_created_at_desc",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Cross-dialect nit: descending indexes (created_at.desc()) are only honored on MySQL 8.0+; older MySQL parses-and-ignores the direction (results still correct, just an ascending index). Quick confirm of the prod MySQL major version. (The NULLS LAST workaround in list_active is the right portable pattern — nice.)

response: fastapi.Response,
) -> api_server_sql.ListBannersResponse:
# A banner can start or expire at any moment, so responses must not be cached.
response.headers["Cache-Control"] = "no-store"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Performance/ops: this is the incident-time hot path and hits the DB on every page load with no-store. The irony is it's most needed exactly when traffic + DB stress spike. no-store on the client is fine, but consider a short server-side/ETag cache (~5–10s) so banner rendering doesn't add DB load when the DB is already struggling — which is the P000 stability scenario this feature exists for.



@dataclasses.dataclass(kw_only=True)
class UpdateBannerRequest:

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 UX/API semantics: with the null-means-unchanged convention, {"url": null} / {"ends_at": null} are no-ops, so an admin can never clear a link or drop an end-time back to indefinite — both plausible banner edits. Since this is net-new (no existing consumer to protect), it's the right moment to support clearing via exclude_unset/a sentinel rather than inheriting the limitation. You flagged this in the PR body; just calling it out as worth doing now.

raise errors.ItemNotFoundError(f"Banner with {id=} does not exist.")
if banner_row.deleted_at is None:
current_time = _get_current_time()
banner_row.deleted_at = current_time

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🤖 Correctness: the 'accidental delete is recoverable' rationale isn't actually true through the API — nothing clears deleted_at, and update() doesn't touch it (it will happily edit a soft-deleted banner while it stays deleted). So a mis-delete is only recoverable via direct DB access. Consider a restore/un-delete path, or soften the claim to 'auditable' rather than 'recoverable'.

@morgan-wowk

Copy link
Copy Markdown
Collaborator

🤖 Agent-generated reference (posted by Morgan) — visual overview of the banners feature in this PR, for the team to return to. Four diagrams: data model (ERD), request/architecture flow, the active-banners read path, and the admin write lifecycle.

1. Data model (ERD)

erDiagram
    BANNER {
        int id PK
        string title "≤120 chars, plain text"
        string body "TEXT, ≤2000 chars, plain text"
        enum variant "info | warning | success | error"
        string url "nullable, absolute http/https, ≤2048"
        string url_text "nullable, ≤80 chars, plain text"
        bool is_enabled "admin on/off toggle"
        datetime starts_at "nullable, UTC — window start"
        datetime ends_at "nullable, UTC — window end"
        datetime created_at "UTC, server-set"
        datetime updated_at "UTC, server-set on write"
        datetime deleted_at "nullable, UTC — soft delete"
    }
Loading

Indexes: ix_banner_is_enabled_deleted_at_starts_at_ends_at (composite, drives the active-window query) and ix_banner_created_at_desc (admin list ordering). A row is active when is_enabled AND deleted_at IS NULL AND (starts_at IS NULL OR starts_at <= now) AND (ends_at IS NULL OR ends_at >= now).

2. Architecture / request flow

flowchart LR
    subgraph Clients
        U[Any user / app shell]
        A[Admin UI]
    end
    subgraph API["api_router.py"]
        P["GET /api/banners/active<br/>(public, no-store)"]
        AR["/api/admin/banners<br/>GET · POST · GET id · PATCH id · DELETE id"]
    end
    subgraph Deps["FastAPI dependencies"]
        RO[require admin]
        NW[check_not_readonly]
    end
    S["BannersApiService_Sql<br/>(api_server_sql.py)"]
    DB[("banner table<br/>SQLite local / MySQL prod")]

    U --> P --> S
    A --> AR
    AR --> RO
    AR -->|writes only| NW
    RO --> S
    NW --> S
    S --> DB
Loading

3. Active-banners read path (the hot path)

sequenceDiagram
    participant C as Client (app shell)
    participant R as GET /api/banners/active
    participant S as BannersApiService_Sql.list_active
    participant DB as banner table

    C->>R: page load
    R->>R: set Cache-Control: no-store
    R->>S: list_active(now=UTC)
    S->>DB: SELECT where is_enabled AND deleted_at IS NULL<br/>AND (starts_at IS NULL OR <= now)<br/>AND (ends_at IS NULL OR >= now)
    Note over S,DB: ORDER BY (starts_at IS NULL), starts_at<br/>portable NULLS-LAST emulation (no MySQL NULLS LAST)
    DB-->>S: active rows
    S-->>R: public banner list (no admin/audit fields)
    R-->>C: 200 (uncached → 1 DB hit per load)
Loading

4. Admin write lifecycle

stateDiagram-v2
    [*] --> Draft: POST (create)
    Draft --> Draft: PATCH (null field = unchanged)
    Draft --> Enabled: is_enabled = true
    Enabled --> Draft: is_enabled = false
    Enabled --> SoftDeleted: DELETE (sets deleted_at)
    Draft --> SoftDeleted: DELETE (sets deleted_at)
    SoftDeleted --> SoftDeleted: DELETE (idempotent)
    note right of SoftDeleted
        No API restore path today:
        deleted_at is never cleared,
        update() still edits deleted rows.
        Recovery = direct DB access.
    end note
Loading

Notes captured during review (see inline comments for detail): the public read path is uncached and DB-backed on every load (worth a short server-side/ETag cache for incident-time load); updated_at/created_at strict ordering relies on sub-second precision that MySQL DATETIME drops by default; PATCH's null-means-unchanged means nullable fields (url, ends_at) can't be cleared; and text fields are plain-text-by-contract (frontend must render as text, not HTML).

@morgan-wowk

Copy link
Copy Markdown
Collaborator

I will review the architecture, independent of the code, in the next 1-2 days. Thank you for waiting!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants