Chase fournier/grade migration - #10
Open
Chase-Fournier wants to merge 22 commits into
Open
Conversation
Phase 0 and the foundation of Phase 1 from CHANGES_.md. Until now the schema was applied by hand-running grades/schema.sql in the Supabase SQL editor. That works for one idempotent file and stops working for a change that has to add a column, backfill it, and rewrite the views reading it, in that order, once, in production. db/migrate.sh applies numbered files over a direct Postgres connection (PostgREST cannot do DDL) and records each in `schema_migrations` with a checksum, so editing an already-applied migration fails loudly instead of silently doing nothing. The migrations here cover instructor identity: `normalize_name`/`slugify`, `instructor_aliases`, `instructor_match_queue`, `section_instructors`, `grades.instructor_id`, the `testudo` provenance tier, the instructor rollup views, and RLS. Two things worth flagging for review: * `normalize_name` strips everything non-alphanumeric after unaccenting rather than listing punctuation to remove. That leaves no character class to keep in sync across three languages and no dependence on the database locale. A name in a script unaccent cannot map normalizes to null and goes to the match queue rather than being half-mangled. * Both functions carry an explicit `set search_path`. Supabase installs extensions into the `extensions` schema, so a bare `unaccent` is not visible to a function called during a write with a different path -- and `name_norm` is a generated column, so that call happens on write. tests/fixtures/names.json is the shared contract between the SQL, Python, and (later) TypeScript implementations. db/tests/name_parity.sql is generated from it so there is no second copy to drift. Nothing here has been run against a database: there are no credentials in this working tree. The SQL is validated against PostgreSQL 17's own grammar via libpg_query, and the 54 Python fixtures pass. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phases 1-3 from CHANGES_.md, the scraper half. The matching algorithm deliberately lives in SQL and nowhere else (link_instructor, migration 0007). The nightly Testudo scrape and the one-off grade backfill both call it, and the one thing that must not happen is the two disagreeing: a name resolved one way by one and another way by the other produces a duplicate instructor whose grade history is split across two professor pages. Python owns the workflow around it -- deduplicating names before they reach the database, rebuilding section_instructors, reporting queue depth -- not the matching. The two callers differ in exactly one argument. `reconcile_instructors` passes create_if_missing=true, because somebody teaching a section this term is a real person and making a human confirm each one every August would make the scrape unusable. `backfill_instructor_ids.py` passes false: a sixteen-year-old registrar spelling with nothing similar in the database is precisely where a human should look before a professor page appears at a permanent URL. Run `backfill_instructor_ids.py --dry-run` first. The match rate it prints decides how much manual triage the rest of the migration carries, and it is the one number nobody can estimate in advance. snapshot_planetterp.py is unrepeatable and should run early. It also captures num_reviews, which the old scraper discarded -- the rating blend needs it so a 4.9 from three students does not outrank a 4.6 from sixty. It updates existing rows only and never inserts: adding ~13k PlanetTerp records would reintroduce the identity problem this migration removes. `ingest-term` applies Testudo attribution before resolution (it changes which names there are), resolution before the upsert (instructor_id is written with the row), and the matview refresh last (it reads what the upsert wrote). Untested against a live database -- no credentials here. grades/ requires Python 3.10+, which predates this change, so its CLI could not be exercised on this machine either. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Phase 9 from CHANGES_.md. Only safe once the snapshot in scripts/snapshot_planetterp.py has run and its archive is stored somewhere durable -- PlanetTerp is no longer updated and this data cannot be re-fetched if it goes offline. instructors.py and its nightly workflow are deleted. Instructors now originate from Testudo section scrapes, resolved through link_instructor. db.py gains a NEVER_TRUNCATE guard. upload_data exists because Testudo data is a snapshot that goes stale, so each run replaces it wholesale; applying that to `instructors` once reviews exist -- which FK to it with `on delete cascade` -- would silently destroy every review in the database. The `instructors` branch was removed rather than never having existed, and the obvious way to add a table to the nightly scrape is to copy the line above it, so this is a guard and not a comment. Also replaces `.neq(col, 0)`, which compared a text column to an integer and worked only by accident of PostgREST coercion. ci.py moves from fixed row-count floors to two kinds of check. Snapshot tables keep absolute floors. Accumulating tables (instructors, grades) are compared against the previous run instead, because they no longer get rebuilt nightly and a shrinking instructor table is now a data-loss signal that a floor of 13,000 would never catch. Two new checks cover failures that are silent by construction: * instructor_match_queue depth. A Testudo markup change that breaks name parsing reduces no row count -- it quietly fills the queue while professor pages start showing no grade data. * matview freshness. A matview that was never refreshed after an ingest has no symptom at all: the site serves last term's numbers and looks entirely healthy. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Migrations 0008 and 0009, plus tests for the ingest-term Testudo attribution that could not be exercised by running the command. 0008 puts three invariants in the schema rather than in application code, because application code is where this eventually goes wrong: * No review is readable before approval. Public reads go through `public_reviews`, a view over approved rows that does not select the identity columns at all, and the anon role has no grant on `reviews`. * Raw email addresses are never stored, only a peppered SHA-256. The pepper lives in Secret Manager: a university address space is small and highly guessable, so an unpeppered digest is reversible by enumeration. * Every moderation decision is recorded, automated or human, with the actor and the pinned model id. That is the audit trail and the dataset for deciding whether the classifier can be trusted to act alone. `email_outbox` is the answer to the mail provider's daily cap. Hitting it defers a send rather than letting a submission through unverified, so a cap or an outage delays delivery instead of dropping the check that backs the UMD-affiliation claim and the per-email dedupe. 0009 implements the rating model with the Bayesian shrinkage you asked for. Both sources plus five notional reviews at the global mean, so a three-review 4.9 no longer outranks a sixty-review 4.6 in a sort once the PlanetTerp weight has decayed away. Constants live in a one-row table rather than in the function, which is what makes `rating_sensitivity_sweep` possible: the same model at 2, 4 and 8-year half-lives, so "does this parameter matter?" is a query rather than an argument. Ratings are recomputed on a schedule, never by a trigger -- the decay makes them time-dependent, so they change on days when no review does. All nine migrations parse against PostgreSQL 17's grammar. Unexecuted: still no credentials in this tree. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Copies the public schema and its data between projects, so the migration can be rehearsed on a test project before it touches production. That rehearsal answers the one question nobody can estimate in advance: clone, apply the migrations, run backfill_instructor_ids.py --dry-run, and the match rate it prints is how much manual triage the real migration will cost. It also captures active_instructors, whose definition currently exists only inside the production database. The script drops and recreates the target's public schema, so it refuses to run without the target project ref typed out by hand, refuses when source and target are the same project, and checks that before spending time on a dump rather than after. --dry-run inspects both ends and writes nothing. Only the public schema is copied. Supabase's auth, storage, realtime and vault schemas are platform-managed and copying them breaks the target. Ownership and grants are dropped on the way through -- the two projects have different role OIDs, and migration 0006 re-applies the grants that matter. Restore errors are deliberately not fatal. A Supabase target has platform-managed objects a restore will collide with, and stopping at the first one leaves a half-restored database that is worse than a complete one with warnings. The row-count comparison at the end is what decides whether it actually worked. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
clone_project.sh needs the Postgres password for both projects and a route to port 5432. clone_via_api.py does the same job over the Supabase Management API, which authenticates with a personal access token instead. Tokens are per-account, so cloning between two accounts is just two tokens. It is effectively a small pg_dump written against the system catalogs: enum types, tables with identity and generated columns, constraints, indexes, views, materialized views, functions, RLS policies, grants, and row data. Tables are created without foreign keys, data is loaded, then the keys are added -- which sidesteps having to derive a valid insertion order between tables that reference each other. Rows move as JSON through jsonb_populate_recordset rather than as generated INSERT tuples. That maps by column name and handles every type the schema uses, instead of requiring this script to quote each value correctly by type, which is where a hand-rolled copier goes wrong. Two details worth knowing: * urllib's default User-Agent is blocked by Cloudflare in front of the Management API, returning an opaque 403 that looks like an auth failure. Any ordinary UA works. * Identity sequences are resynced after loading. Rows arrive with their original ids while the sequence stays at 1, so the next insert on the target would collide -- later, and only there, which is a bad way to find out. Honest about its limits: triggers, non-standard extensions, composite and domain types, and partitioned tables are not copied. None exist in this schema today, and the script reports them rather than skipping silently. Tested without a database. The Project class is replaced with one that records SQL instead of sending it, and all 14 generated statements are parsed against PostgreSQL's grammar via libpg_query, alongside unit tests for column DDL, dollar-quote collision, policy reconstruction, generated columns being excluded from inserts, and both refusal guards. 10 tests pass. pglast is optional -- the grammar checks skip without it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The .gitignore fix is a real bug I introduced. The original file ended `.env` with no trailing newline, so appending to it with `cat >>` fused the lines into `.env__pycache__/` -- a pattern matching nothing. From that commit until now, `.env` was not ignored anywhere in this repository, which means a `git add -A` would have committed credentials. Rewritten properly, with `.env.*` covered too and `.env.example` explicitly allowed. clone_via_api.py now reads db/.env itself, so the credentials do not have to be exported into the shell. Parsed with a few lines rather than python-dotenv so the script still runs with nothing installed, and real environment variables win over the file so a one-off override still works. Accepts the `export KEY=value` form, since that is what copying the lines out of the README gives you. db/.env.example documents the four values and says plainly what the tokens are: full Management API access to every project on their account. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Testudo emits the waitlist and holdfile counts as two spans of the same
`waitlist-count` class inside one wrapper, distinguished only by the
`seats-info-label` in front of each. `parse_section` read them by
position, which breaks on both ends of that markup:
- A section that doesn't run a waitlist still gets the wrapper, but it
contains only the help link and no counts at all. Indexing [0] into
an empty list raised `IndexError`, and because chunks are parsed in a
ThreadPoolExecutor the exception surfaced out of `executor.map` and
aborted the entire term. This is what has been failing main; 185 of
2083 sections sampled from 202608 have this shape.
- A section can carry a holdfile and no waitlist, where the single
span present is the *holdfile*. Position read it as the waitlist, so
those sections have been silently uploading a holdfile count in the
waitlist field - 110 of the same 2083 sampled sections.
Key the counts off their labels instead, defaulting waitlist to 0 when
Testudo reports none, and falling back to document order if the labels
ever disappear. Holdfile now comes back as an int rather than the raw
span text, matching the `number | null` the site already expects.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Grade Migration