docs: describe the system that exists, and remove the importer's remains #147

Merged
tudor merged 2 commits from docs/current-truth-and-legacy-inventory into main 2026-09-15 08:58:54 +00:00
Owner

Why

The README opened on "Primary School Compass — a tool for comparing primary school (KS2) performance in Wandsworth and Merton, built with FastAPI and vanilla JavaScript." Every clause is now false. Coverage is England-wide across KS2, KS4, all-through and post-16; Next.js owns the public UI; school data comes from dbt-built marts.*. Worse, the Quick Start walked a reader into a virtualenv and a CSV import that cannot build the current schema — so following the docs produced an empty database and a wrong mental model at once.

The same drift left behind code that no longer imports.

What changed

Documentation — two reference docs checked against the code

  • docs/ARCHITECTURE.md — request flow, data ownership per layer, backend/frontend module boundaries, and the real publication sequence (Airflow → dbt → Typesense alias swap → /api/admin/reload), including its known non-atomicity.
  • docs/DEVELOPMENT.md — the checks that actually run, plus the container/CI version skew (backend 3.11 vs 3.12, Node 24 vs 22) that makes "just run pytest" misleading.
  • README.md, claude.md, DOCKER_DEPLOY.md, nextjs-app/README.md, nextjs-app/DEPLOYMENT.md trimmed to point at these.
  • MIGRATION_SUMMARY.md keeps its content, gains a banner — it reads like setup instructions and is not.

Env examples drifted the same way: ALLOWED_ORIGINS is a JSON array, not comma-separated; FASTAPI_URL, DATABASE_URL and PAYLOAD_SECRET were undocumented; RATE_LIMIT_BURST / DEFAULT_PAGE_SIZE / MAX_PAGE_SIZE were presented as tuning controls the routes never consult. Each now states how it behaves.

Dead code removed — backend/migration.py, backend/version.py, scripts/migrate_csv_to_db.py, scripts/geocode_schools.py. The first three import School, SchoolResult, init_db, set_db_schema_version: names that no longer exist. These were not dormant fallbacks, they were files that fail on the first line. Plus three symbols with no caller: haversine_distance (superseded by the inline NumPy path in search), fetcher (an SWR helper for a dependency this project doesn't install), and kmToMiles. calculateDistance stays — CutoffMapPanel uses it.

Two comments justified Payload's separate schema by pointing at migrate_csv_to_db.py --drop. The reason outlives the script, so it's reworded rather than deleted — the constraint keeps its justification.

docs/LEGACY_CODE.md records the removals for history lookup, and — the more useful half — what was deliberately not removed: unused UI components awaiting a design decision, manual data utilities whose operators a repo search cannot see, and fallbacks that look obsolete but are load-bearing (data_loader.py's older-mart branches, the generated GIAS dictionary copies, the legacy-named dbt models that annual DAG selectors explicitly include). A zero-import count is evidence, not a verdict.

Verification

Check Result
backend/tests 190 passed
nextjs-app Jest 429 passed, 51 suites
tsc --noEmit clean
Reference search for every removed name docs only
Relative markdown links across touched docs all resolve

No user-facing behaviour changes, so no E2E journey updates. No database, pipeline or deployment action taken.

🤖 Generated with Claude Code

https://claude.ai/code/session_016y2J6bs8gbuSJbH18w7Tan

## Why The README opened on *"Primary School Compass — a tool for comparing primary school (KS2) performance in Wandsworth and Merton, built with FastAPI and vanilla JavaScript."* Every clause is now false. Coverage is England-wide across KS2, KS4, all-through and post-16; Next.js owns the public UI; school data comes from dbt-built `marts.*`. Worse, the Quick Start walked a reader into a virtualenv and a CSV import that **cannot build the current schema** — so following the docs produced an empty database and a wrong mental model at once. The same drift left behind code that no longer imports. ## What changed **Documentation — two reference docs checked against the code** - `docs/ARCHITECTURE.md` — request flow, data ownership per layer, backend/frontend module boundaries, and the real publication sequence (Airflow → dbt → Typesense alias swap → `/api/admin/reload`), including its known non-atomicity. - `docs/DEVELOPMENT.md` — the checks that actually run, plus the container/CI version skew (backend 3.11 vs 3.12, Node 24 vs 22) that makes "just run pytest" misleading. - `README.md`, `claude.md`, `DOCKER_DEPLOY.md`, `nextjs-app/README.md`, `nextjs-app/DEPLOYMENT.md` trimmed to point at these. - `MIGRATION_SUMMARY.md` keeps its content, gains a banner — it reads like setup instructions and is not. **Env examples** drifted the same way: `ALLOWED_ORIGINS` is a JSON array, not comma-separated; `FASTAPI_URL`, `DATABASE_URL` and `PAYLOAD_SECRET` were undocumented; `RATE_LIMIT_BURST` / `DEFAULT_PAGE_SIZE` / `MAX_PAGE_SIZE` were presented as tuning controls the routes never consult. Each now states how it behaves. **Dead code removed** — `backend/migration.py`, `backend/version.py`, `scripts/migrate_csv_to_db.py`, `scripts/geocode_schools.py`. The first three import `School`, `SchoolResult`, `init_db`, `set_db_schema_version`: names that no longer exist. These were not dormant fallbacks, they were files that fail on the first line. Plus three symbols with no caller: `haversine_distance` (superseded by the inline NumPy path in search), `fetcher` (an SWR helper for a dependency this project doesn't install), and `kmToMiles`. `calculateDistance` stays — `CutoffMapPanel` uses it. Two comments justified Payload's separate schema by pointing at `migrate_csv_to_db.py --drop`. The reason outlives the script, so it's reworded rather than deleted — the constraint keeps its justification. **`docs/LEGACY_CODE.md`** records the removals for history lookup, and — the more useful half — what was deliberately **not** removed: unused UI components awaiting a design decision, manual data utilities whose operators a repo search cannot see, and fallbacks that look obsolete but are load-bearing (`data_loader.py`'s older-mart branches, the generated GIAS dictionary copies, the `legacy`-named dbt models that annual DAG selectors explicitly include). A zero-import count is evidence, not a verdict. ## Verification | Check | Result | |---|---| | `backend/tests` | 190 passed | | `nextjs-app` Jest | 429 passed, 51 suites | | `tsc --noEmit` | clean | | Reference search for every removed name | docs only | | Relative markdown links across touched docs | all resolve | No user-facing behaviour changes, so no E2E journey updates. No database, pipeline or deployment action taken. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_016y2J6bs8gbuSJbH18w7Tan
tudor added 2 commits 2026-09-14 22:01:33 +00:00
The README still opened on "Primary School Compass", a KS2 tool for Wandsworth
and Merton served by FastAPI and vanilla JavaScript with Chart.js. Every layer
of that sentence is now wrong: coverage is England-wide across KS2, KS4,
all-through and post-16, Next.js owns the public UI, and school data comes from
dbt-built `marts.*` rather than CSVs loaded at startup. The setup instructions
walked a reader into a virtualenv and a CSV import that cannot build the current
schema, so following the docs produced an empty database and a wrong mental
model at the same time.

Replace the narrative docs with two reference documents that were checked
against the code: docs/ARCHITECTURE.md for request flow, data ownership, the
backend/frontend module boundaries and the real publication sequence, and
docs/DEVELOPMENT.md for the checks that actually run, including the container
and CI version skew that makes "just run pytest" misleading.

The env examples drifted the same way. ALLOWED_ORIGINS is a JSON array, not a
comma-separated list; the frontend needs FASTAPI_URL, DATABASE_URL and
PAYLOAD_SECRET, none of which were documented; and RATE_LIMIT_BURST,
DEFAULT_PAGE_SIZE and MAX_PAGE_SIZE were presented as tuning controls the routes
do not consult. Each is now stated as it behaves.

MIGRATION_SUMMARY.md keeps its content but gains a banner, because it reads like
setup instructions and is not.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016y2J6bs8gbuSJbH18w7Tan
chore: remove the code the legacy CSV importer left behind
PR Checks / Frontend Typecheck + Tests (pull_request) Successful in 1m11s
PR Checks / Backend Smoke (pull_request) Successful in 9s
PR Checks / Build Backend (no push) (pull_request) Successful in 18s
PR Checks / Build Frontend (no push) (pull_request) Successful in 1m18s
PR Checks / Build Pipeline (no push) (pull_request) Successful in 11s
PR Checks / AI Code Review (Claude) (pull_request) Successful in 1m2s
1d8858fbda
`backend/migration.py` and `scripts/migrate_csv_to_db.py` import `School`,
`SchoolResult`, `init_db` and `set_db_schema_version` — names that no longer
exist. `scripts/geocode_schools.py` imports the same removed ORM model. None of
the three can be imported against the current backend, so they were not dormant
utilities anyone could fall back on; they were files that would fail on the
first line. `backend/version.py` existed only to hand `SCHEMA_VERSION` to that
importer, and the FastAPI lifespan performs no version-triggered import.

Three symbols go with them, each confirmed to have no caller: the unvectorised
`haversine_distance`, superseded by the inline NumPy calculation in search;
`fetcher`, an SWR helper for a dependency this project does not install; and
`kmToMiles`. `calculateDistance` stays — CutoffMapPanel uses it.

Two comments pointed at `migrate_csv_to_db.py --drop` to explain why Payload
owns its own schema. The reason survives the script: blog content must stay
clear of the school marts and Airflow's metadata. Reworded rather than deleted,
so the constraint keeps its justification.

docs/LEGACY_CODE.md records what was removed and where to find it in history. It
also records what was deliberately *not* removed, which is the more useful half:
unused UI components awaiting a design decision, manual data utilities whose
operators a repository search cannot see, and fallbacks that look obsolete but
are load-bearing — `data_loader.py`'s older-mart branches, the generated GIAS
dictionary copies, and the `legacy`-named dbt models that annual DAG selectors
explicitly include. A zero-import count is evidence, not a verdict.

The scripts that fetch DfE CSVs are marked historical and kept, pending
confirmation that nobody runs them by hand.

Checked: 190 backend tests, 429 frontend tests, `tsc --noEmit` clean.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_016y2J6bs8gbuSJbH18w7Tan

🤖 AI Code Review (Claude Code)

This PR is a large documentation consolidation and dead-code removal: it rewrites README/claude.md/DEPLOYMENT docs into a smaller set of maintained docs (ARCHITECTURE.md, DEVELOPMENT.md, LEGACY_CODE.md), and deletes the legacy CSV importer (backend/migration.py, backend/version.py, scripts/migrate_csv_to_db.py, scripts/geocode_schools.py) plus a few confirmed-unused functions (haversine_distance, fetcher, kmToMiles). I verified the removed symbols have no remaining callers anywhere in the repo (backend, tests, scripts, .gitea workflows, Dockerfile/compose), that the Dockerfile CMD never invoked the deleted migration scripts, and that the new GLOBAL_RATE_LIMIT_PER_MINUTE env var maps to an actually-used settings field, while the removed DEFAULT_PAGE_SIZE/MAX_PAGE_SIZE/RATE_LIMIT_BURST are indeed dead settings as the docs claim. No correctness, security, or deploy/CI issues found.

✅ No issues found.

## 🤖 AI Code Review (Claude Code) This PR is a large documentation consolidation and dead-code removal: it rewrites README/claude.md/DEPLOYMENT docs into a smaller set of maintained docs (ARCHITECTURE.md, DEVELOPMENT.md, LEGACY_CODE.md), and deletes the legacy CSV importer (backend/migration.py, backend/version.py, scripts/migrate_csv_to_db.py, scripts/geocode_schools.py) plus a few confirmed-unused functions (haversine_distance, fetcher, kmToMiles). I verified the removed symbols have no remaining callers anywhere in the repo (backend, tests, scripts, .gitea workflows, Dockerfile/compose), that the Dockerfile CMD never invoked the deleted migration scripts, and that the new GLOBAL_RATE_LIMIT_PER_MINUTE env var maps to an actually-used settings field, while the removed DEFAULT_PAGE_SIZE/MAX_PAGE_SIZE/RATE_LIMIT_BURST are indeed dead settings as the docs claim. No correctness, security, or deploy/CI issues found. ✅ No issues found.
tudor merged commit dc156058fe into main 2026-09-15 08:58:54 +00:00
Sign in to join this conversation.
No Reviewers
No labels
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: tudor/school_compare#147