Removed API_BASE_URL and replaced with SITE_BASE_URL #458

Merged

johnsturgeon merged 4 commits from add-site-base-url-to-config-456 into main

2026-09-03 13:00:49 +02:00

johnsturgeon commented

2026-09-03 12:28:43 +02:00

Owner

Resolves #456
Resolves #452

Resolves #456 Resolves #452

johnsturgeon added 1 commit

2026-09-03 12:28:43 +02:00

Removed API_BASE_URL and replaced with SITE_BASE_URL

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

Details

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

Details

Tests / pytest (pull_request) Successful in 22s

Details

aea2798be9

Resolves #456
Resolves #452

forgejo-actions commented

2026-09-03 12:30:07 +02:00

This is a clean, well-scoped refactor: a single hardcoded host (https://tgfp.us) and a now-unused API_BASE_URL are replaced by one SITE_BASE_URL config value threaded through the jobs and mailer, and the config plumbing (config.py, op.env, sample.env, test seeding) is updated consistently. I found no correctness or security problems — the notes below are minor.

Worth fixing

app/jobs/nag_players.py:17 — NagBotException is introduced in this PR but never raised, caught, or imported anywhere (confirmed by grep). It's dead code. Either wire it into nag_the_players (e.g. the first_game is None / no-current-week paths) or drop it.

Nits

app/jobs/nag_players.py:55 — the new if player.discord_id is not None: guard is redundant: _get_late_players already skips anyone without a Discord id (if not player.has_discord_id: continue at line 35), so late_players never contains a player who would emit <@None>. Harmless, just belt-and-suspenders that can mislead a future reader into thinking the upstream filter is unreliable.
Trailing-slash fragility — the URL is assembled by string concatenation in several places (config.SITE_BASE_URL + "/picks" in announce_picks_page.py:18 and nag_players.py:46; f"{url}/rules" in mailer.py:52; f"{url}/static/..." in award_notify_discord.py:23). All of these assume SITE_BASE_URL has no trailing slash. The committed sample.env/op.env values are fine, but a value like https://tgfp.us/ silently yields https://tgfp.us//picks. Not worth abstracting for four call sites, but worth a one-line comment on the config field, or an .rstrip("/") when reading it in config.py.
tests/test_integrity.py:13 — the docstring still cites KeyError: 'API_BASE_URL' as the example of what a missing config raises; that key no longer exists. Update to a current field (or a generic phrasing) so the comment doesn't send someone chasing a removed variable.
Docstring wording regressed from "picks page" to "pick page" in app/jobs/announce_picks_page.py:44 and app/mailer.py:64. Cosmetic, but the original read better.
No test asserts the URLs now come from config.SITE_BASE_URL (the conftest seeds it as the placeholder test-SITE_BASE_URL, so a test could cheaply pin the substitution in the nag/announce/mailer payloads). Given the existing coverage of these jobs, adding one assertion would lock in the behavior this PR is specifically about.

One thing worth calling out as a positive: switching send_welcome_email off request.url_for(...) removes the last reason it needed a Request, which is what lets the admin "resend welcome" path (v2.py:519) call it without a synthetic request — consistent with the comment already in mailer.py:45.

This is a clean, well-scoped refactor: a single hardcoded host (`https://tgfp.us`) and a now-unused `API_BASE_URL` are replaced by one `SITE_BASE_URL` config value threaded through the jobs and mailer, and the config plumbing (`config.py`, `op.env`, `sample.env`, test seeding) is updated consistently. I found no correctness or security problems — the notes below are minor. ## Worth fixing - `app/jobs/nag_players.py:17` — `NagBotException` is introduced in this PR but never raised, caught, or imported anywhere (confirmed by grep). It's dead code. Either wire it into `nag_the_players` (e.g. the `first_game is None` / no-current-week paths) or drop it. ## Nits - `app/jobs/nag_players.py:55` — the new `if player.discord_id is not None:` guard is redundant: `_get_late_players` already skips anyone without a Discord id (`if not player.has_discord_id: continue` at line 35), so `late_players` never contains a player who would emit `<@None>`. Harmless, just belt-and-suspenders that can mislead a future reader into thinking the upstream filter is unreliable. - Trailing-slash fragility — the URL is assembled by string concatenation in several places (`config.SITE_BASE_URL + "/picks"` in `announce_picks_page.py:18` and `nag_players.py:46`; `f"{url}/rules"` in `mailer.py:52`; `f"{url}/static/..."` in `award_notify_discord.py:23`). All of these assume `SITE_BASE_URL` has no trailing slash. The committed `sample.env`/`op.env` values are fine, but a value like `https://tgfp.us/` silently yields `https://tgfp.us//picks`. Not worth abstracting for four call sites, but worth a one-line comment on the config field, or an `.rstrip("/")` when reading it in `config.py`. - `tests/test_integrity.py:13` — the docstring still cites `KeyError: 'API_BASE_URL'` as the example of what a missing config raises; that key no longer exists. Update to a current field (or a generic phrasing) so the comment doesn't send someone chasing a removed variable. - Docstring wording regressed from "picks page" to "pick page" in `app/jobs/announce_picks_page.py:44` and `app/mailer.py:64`. Cosmetic, but the original read better. - No test asserts the URLs now come from `config.SITE_BASE_URL` (the conftest seeds it as the placeholder `test-SITE_BASE_URL`, so a test could cheaply pin the substitution in the nag/announce/mailer payloads). Given the existing coverage of these jobs, adding one assertion would lock in the behavior this PR is specifically about. One thing worth calling out as a positive: switching `send_welcome_email` off `request.url_for(...)` removes the last reason it needed a `Request`, which is what lets the admin "resend welcome" path (`v2.py:519`) call it without a synthetic request — consistent with the comment already in `mailer.py:45`.

johnsturgeon added 1 commit

2026-09-03 12:46:23 +02:00

[skip review] fixed accidental exception addition

Claude Code Review / claude-review (pull_request) Successful in 4s

Details

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

Details

Tests / pytest (pull_request) Successful in 13s

Details

b73faedb5d

johnsturgeon added 1 commit

2026-09-03 12:47:40 +02:00