[ENG-373] Persisted review flags on oc_recordings, with retroactive backfill - #134
Open
henokteixeira wants to merge 6 commits into
Open
[ENG-373] Persisted review flags on oc_recordings, with retroactive backfill#134henokteixeira wants to merge 6 commits into
henokteixeira wants to merge 6 commits into
Conversation
A recording with no classification, a throwaway description or no storyteller was
invisible: nothing could list or count it. ENG-354 stopped the bleeding on write and
deliberately grandfathered every existing row, so the rule that marks them has to live
server-side and be readable.
One JSON column holding {code, origin} objects rather than bare strings, so a
user-raised flag can join the same list later without a schema change. JSON rather than
ARRAY because the suite runs on SQLite and production on Postgres. none_as_null makes a
stray None a loud NOT NULL violation instead of the JSON string "null", which would
insert cleanly and then break response validation for the whole listing.
The rule ignores subcategory on purpose: the Dart predicate it replaces does too, and a
server that disagreed would fight the app over the same recording.
A persisted flag that nothing invalidates is worse than no flag. The issue named two write paths; a search found four. persist_split_segments builds child recordings directly, outside create_recording, so segments would otherwise be born with an empty flag list that is simply wrong. delete_storyteller is the one no hook could have caught: the FK is ON DELETE SET NULL, so the column changed inside Postgres with no ORM object loaded, leaving orphaned recordings reporting a storyteller they no longer have. The NULL is now applied in Python first so the loss is observable. The documented contract is unchanged. update_recording recomputes after the generic setattr loop, so it sees final state and covers any field later added to RecordingUpdate.
Both GET endpoints already validate from ORM attributes, so the router needs no change.
The server default is what lets existing rows read back as an empty list rather than NULL without a table rewrite, but an empty list reads as "reviewed and clean" when in fact nothing has ever looked at the row. Hence the backfill. Schema change and backfill are one revision on purpose. Split in two, the CI gate's downgrade -1 would only reach the backfill, whose downgrade is necessarily a no-op, and the column drop would stop being exercised at all. env.py wraps the run in a single transaction, so there was never an observable window between them. Written against the lesson of 20260518_0002: preconditions return an empty report rather than raising, and the row counts are logged, so an operator can tell a table with nothing to do apart from a backfill that never ran. The rule is inlined rather than imported from the service. Reaching into app.services pulls in nearly three hundred modules and builds a database engine at import time, which would let an unrelated service's import error block alembic upgrade head against the database six production apps share. Only the description rule is imported, as ENG-354 requires — a second copy of that would drift from the Dart client.
Mutation-tested rather than assumed. The shared vector alone cannot detect a count over code points instead of grapheme clusters, because every row sits at exactly twenty clusters and more than twenty code points, so a naive count only over-counts and never flags; the just-under-threshold cases are what make that requirement real. Membership negation is likewise satisfied by losing the flag list entirely, so the clearing tests assert exact sets. The backfill's callable is imported from the revision by path: the suite builds its schema from Base.metadata and never runs Alembic.
It named eight service packages of twenty-three, and had already drifted long before this change. Naming them all would rot again within weeks.
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
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.
Summary
Closes ENG-373.
tripod-apihad no notion of "this recording needs review", so recordings with no classification, a throwaway description or no storyteller sat in the API and nothing could list or count them. ENG-354 (#129) made a sufficient description mandatory but deliberately only on write, grandfathering every existing row — this marks what is already on the floor. The Flutter half is ENG-374, which will render these flags and delete its own local derivation.sa.JSONcolumn onoc_recordingsholding{code, origin}objects. Objects rather than bare strings so a user-raised flag can join the same list later without a schema change; onlysystemexists today. JSON rather thanARRAYbecause the suite runs on SQLite and production on Postgres.none_as_null=Trueturns a strayNoneinto a loud NOT NULL violation instead of the JSON string"null", which would insert cleanly into a NOT NULL column and then fail response validation for an entire project listing.recompute_review_flagsis called from every write path. The issue named two; a search found four.persist_split_segmentsbuilds child recordings outsidecreate_recording. Anddelete_storytelleris the one no Python hook could have caught:storyteller_idis an FK withON DELETE SET NULL, so deleting a storyteller nulled the column inside Postgres with no ORM object loaded, leaving orphaned recordings reporting a storyteller they no longer had. The NULL is now applied in Python first. Its documented contract is unchanged.missing_classificationdeliberately ignores subcategory, matching the exact Dart predicate ENG-374 deletes. A server that disagreed would fight the app over the same recording. The description condition imports ENG-354's rule rather than reimplementing it.downgrade -1would only ever reach the backfill — whosedowngrade()is necessarily a no-op — and the column drop would stop being exercised.env.pywraps the run in a single transaction, so there was never an observable window between them.app.servicespulls in ~291 modules and constructs a database engine at import time, which would let an unrelated service's import error blockalembic upgrade headagainst the database six production apps share. Onlyapp/utils/description_rule.pyis imported (5app.*modules, no engine); the three conditions are inlined, which is the correct semantics for a frozen historical artifact.Not in this PR, and why
No flag filter on
list_recordings.sa.JSONrenders as Postgresjson, notjsonb, so@>andjsonb_path_existsare unavailable, and SQLite'sjson_eachhas no portable equivalent — a filter would need dialect branching, reintroducing exactly the divergence that made JSON the right choice over ARRAY. Filtering in Python after the query breaks pagination, sinceoffset/limitare already applied. And it would not deliver what ENG-374 §4 actually needs:list_recordingsreturns an untotalled list capped at 50, so the client would still be counting a truncated page, which that issue explicitly forbids. This needs a decision (normalised child table, aggregate endpoint, or defer) and is flagged for @henokteixeira rather than guessed at.update_recording_fields(app/inngest/helpers.py) has no recompute. It is a genericsetattrhelper shared by three Inngest jobs; no current caller passes a flag-bearing field, and it opens its own session so nothing could test a change there. Deliberate gap, not an oversight.Test plan
uv run pytest tests— 1022 passed, 2 skipped (baseline onmainis 992 passed, 2 skipped).uv run ruff check .,uv run ruff format --check .,uv run mypy app— all clean.20260731_0001, andupgrade head→downgrade -1→upgrade headon Postgres 16.postgres:16container with seeded legacy rows, which the CI gate cannot cover because its database is empty: the backfill loggedscanned 2 recording(s), updated 2, a second run reportedupdated 0, andinformation_schemaconfirmed the column is genuinely dropped bydowngrade -1and the rows survive the round trip.Reviewer's attention
UNCLASSIFIED_GENRE_IDmust stay in step withalembic/versions/20260415_0001_seed_unclassified_oc_taxonomy.py. If the sentinel id ever changes,missing_classificationstops firing and the failure looks like "everything is fine" — the worst shape of failure for a review-flag feature. There is no automated tie, because the test database is built fromBase.metadataand never runs migrations.updated_at(it writes through a lightweightsa.table()with noonupdate), whiledelete_storytellernow does. If the Flutter client ever delta-syncs onupdated_at, backfilled flags would not reach it. Noupdated_sincefilter exists today, so this is not live.