Added a link to the admin user's menu for the admin dashboard #542

Merged

johnsturgeon merged 3 commits from add-an-admin-only-link-to-the-nav-535 into main

2026-09-16 17:51:10 +02:00

johnsturgeon commented

2026-09-16 16:48:30 +02:00

Owner

Just stubbed out the dashboard page for now, added the route, and the 'auth'

Also moved awards_for_week to be next the the other 'for_week' methods

Created a common property for is_admin

Just stubbed out the dashboard page for now, added the route, and the 'auth' Also moved awards_for_week to be next the the other 'for_week' methods Created a common property for is_admin

johnsturgeon added 1 commit

2026-09-16 16:48:30 +02:00

Added a link to the admin user's menu for the admin dashboard

Claude Code Review / claude-review (pull_request) Successful in 1m50s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 44s

Details

Tests / pytest (pull_request) Successful in 25s

Details

f93c82e900

Just stubbed out the dashboard page for now, added the route, and the 'auth'

Also moved awards_for_week to be next the the other 'for_week' methods

Created a common property for is_admin

johnsturgeon commented

2026-09-16 16:50:21 +02:00

Author

Owner

Summary

This is a small, well-scoped change: it introduces a Player.is_admin property, routes the admin check (template + require_admin) through it, caches Config.get_config(), stubs a /v3/admin route with a nav link, and relocates awards_for_week. The refactor is clean and behavior-preserving; the main gaps are test coverage for the new admin path and a couple of minor consistency/layering notes.

Worth fixing

No test for the new /v3/admin route or require_admin on the v3 side (app/routers/v3.py:222, app/models/player.py:156). The codebase otherwise tests admin gating carefully (tests/test_admin_player_routes.py, tests/test_admin_email_routes.py stand up a signed-in admin against the v2 routes), so the new route is a coverage gap by local convention. A minimal test that a non-admin gets 403 and the admin gets 200 on /v3/admin, plus a direct is_admin true/false case in tests/test_player_model.py, would match the existing bar. is_admin is only exercised indirectly through the unchanged v2 admin routes today.

Nits

Config.get_config() is now permanently memoized (app/config/config.py:77-79). This is the stated intent ("never read .env after startup"), and it also makes every module's config the same instance instead of independent copies — a net consistency win. Worth being aware that any future test wanting to re-read the environment mid-process can no longer do so via get_config() (existing tests correctly mutate the shared instance through monkeypatch.setattr, e.g. tests/test_mailer.py:47-49, which still works). The @classmethod over @cache ordering is correct.
is_admin is a case-sensitive email comparison (app/models/player.py:158) while Player.by_email deliberately compares case-insensitively and notes emails can differ only by case (app/models/player.py:186-199). This is not a regression — the old template and require_admin both used exact == — but if an admin ever signs up with differently-cased mail than TGFP_ADMIN_EMAIL, the gate silently fails closed. Consider self.email.lower() == Config.get_config().TGFP_ADMIN_EMAIL.lower() for consistency.
app/models/player.py now imports app.config at module top level, making player.py the only model with a direct config dependency. Because get_config() is cached (and already warmed at import in a dozen places), the is_admin attribute access won't do blocking work per request, so it doesn't trip the spirit of the app/models layering guard in tests/test_integrity.py (which only forbids the Tank01 provider). Fine as-is; just flagging the new coupling.
app/templates/v3/admin.html has no trailing newline and a stray double space in the class attribute (text-stone-600 pb-4). Cosmetic.

## Summary This is a small, well-scoped change: it introduces a `Player.is_admin` property, routes the admin check (template + `require_admin`) through it, caches `Config.get_config()`, stubs a `/v3/admin` route with a nav link, and relocates `awards_for_week`. The refactor is clean and behavior-preserving; the main gaps are test coverage for the new admin path and a couple of minor consistency/layering notes. ## Worth fixing - **No test for the new `/v3/admin` route or `require_admin` on the v3 side** (`app/routers/v3.py:222`, `app/models/player.py:156`). The codebase otherwise tests admin gating carefully (`tests/test_admin_player_routes.py`, `tests/test_admin_email_routes.py` stand up a signed-in admin against the v2 routes), so the new route is a coverage gap by local convention. A minimal test that a non-admin gets `403` and the admin gets `200` on `/v3/admin`, plus a direct `is_admin` true/false case in `tests/test_player_model.py`, would match the existing bar. `is_admin` is only exercised indirectly through the unchanged v2 admin routes today. ## Nits - **`Config.get_config()` is now permanently memoized** (`app/config/config.py:77-79`). This is the stated intent ("never read `.env` after startup"), and it also makes every module's `config` the *same* instance instead of independent copies — a net consistency win. Worth being aware that any future test wanting to re-read the environment mid-process can no longer do so via `get_config()` (existing tests correctly mutate the shared instance through `monkeypatch.setattr`, e.g. `tests/test_mailer.py:47-49`, which still works). The `@classmethod` over `@cache` ordering is correct. - **`is_admin` is a case-sensitive email comparison** (`app/models/player.py:158`) while `Player.by_email` deliberately compares case-insensitively and notes emails can differ only by case (`app/models/player.py:186-199`). This is not a regression — the old template and `require_admin` both used exact `==` — but if an admin ever signs up with differently-cased mail than `TGFP_ADMIN_EMAIL`, the gate silently fails closed. Consider `self.email.lower() == Config.get_config().TGFP_ADMIN_EMAIL.lower()` for consistency. - **`app/models/player.py` now imports `app.config` at module top level**, making `player.py` the only model with a direct config dependency. Because `get_config()` is cached (and already warmed at import in a dozen places), the `is_admin` attribute access won't do blocking work per request, so it doesn't trip the spirit of the `app/models` layering guard in `tests/test_integrity.py` (which only forbids the Tank01 provider). Fine as-is; just flagging the new coupling. - **`app/templates/v3/admin.html`** has no trailing newline and a stray double space in the `class` attribute (`text-stone-600 pb-4`). Cosmetic.

johnsturgeon added 1 commit

2026-09-16 16:56:30 +02:00

Caching the config from get_config, this way we never have to read the .env after the app starts up

Claude Code Review / claude-review (pull_request) Successful in 1m36s

Details

Python FastAPI Jinja Linting / build (pull_request) Successful in 31s

Details

Tests / pytest (pull_request) Successful in 16s

Details

e9d36f6e09

johnsturgeon added 1 commit

2026-09-16 17:14:35 +02:00