feat: Add Banners Table and API - #337
Conversation
morgan-wowk
left a comment
There was a problem hiding this comment.
🤖 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()) |
There was a problem hiding this comment.
🤖 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() |
There was a problem hiding this comment.
🤖 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", |
There was a problem hiding this comment.
🤖 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" |
There was a problem hiding this comment.
🤖 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: |
There was a problem hiding this comment.
🤖 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 |
There was a problem hiding this comment.
🤖 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'.
|
🤖 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"
}
Indexes: 2. Architecture / request flowflowchart 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
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)
4. Admin write lifecyclestateDiagram-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
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); |
|
I will review the architecture, independent of the code, in the next 1-2 days. Thank you for waiting! |
What this adds
A generic, site-wide announcement banner: a dedicated
bannertable, 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
GET/api/banners/activeCache-Control: no-store.GET/api/admin/banners?include_deleted=truealso returns soft-deleted ones.POST/api/admin/bannersGET/api/admin/banners/{id}PATCH/api/admin/banners/{id}DELETE/api/admin/banners/{id}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 bystarts_atdescending with un-scheduled banners last, thencreated_atdescending.You can exercise all of this from the
/docsroute.Key decisions
A dedicated
bannertable instead ofUserSettings.UserSettingsis a per-user key/value JSON blob keyed byuser_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 realWHEREclause for the active lookup, real indexes, and per-row audit columns.Soft delete only.
DELETEsetsdeleted_atand never removes the row, so a banner that was shown to users stays auditable and an accidental delete is recoverable.deleted_at IS NULLis part of both the active query and the default admin list;?include_deleted=trueopts back in. Delete is idempotent — deleting an already-deleted banner leaves the original timestamp alone.Two response shapes rather than one.
/api/banners/activereturns 11 display fields; the admin endpoints addis_enabled,created_by,updated_byanddeleted_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'sdefault_config: that config strips null fields, and the frontend wantsurl: nullpresent rather than absent.Reusing
errors.ApiValidationErrorfor validation failures. Invalid input (bad URL, over-length title/body,url_textwithout aurl,ends_at <= starts_at) raises the existingApiValidationError, which the existing handler maps to422. No new error type and no new exception handler were added. An unknownvariantis rejected by FastAPI request validation, so it also returns422— one status code for all bad input.variantas astrenum column.BannerVariant(info/warning/success/error) is mapped onto the column withvalues_callable, matching howContainerExecutionStatusis 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 astarts_atof2026-01-01T12:00:00+02:00would otherwise be stored as if12:00were UTC. It is converted to10:00Zbefore 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 DESCserves the admin list.Partial update semantics.
PATCHfollows the existing convention in this codebase (PublishedComponentService.update): a field that isnull/absent is left unchanged. The trade-off is thatPATCHcannot 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 orexclude_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 nomigrate_dbstep is needed. Verified by building a DB with the code atmaster, inserting a row, then runningcreate_db_engine_and_migrate_dbwith this branch:bannerand its indexes appear, every other table is unchanged, and the pre-existing row survives.Testing
tests/test_banners_api.pyadds 28 tests against a realTestClientapp over an in-memory SQLite DB, covering: empty active list and theno-storeheader; 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_atadvancing, 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;403for a non-admin on every admin route while the public read still works;422for every invalid-input case;404for 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.