Conversation
📊 Code Quality Score: 10/100
Was this score accurate? 👍 Yes · 👎 No Scored by GitVelocity · How are scores calculated? |
There was a problem hiding this comment.
Pull request overview
This PR aligns the powered_by app’s Postgres schema across environments by adding a corrective migration for Project JSONField columns that may still be stored as array types in some dev databases. It also introduces a one-time CSV upsert script to import/update “Powered by Sefaria” projects into the powered_by_project table.
Changes:
- Add a migration to convert
powered_by_project.sefaria_tools_used/tagscolumns tojsonbwhen needed. - Add a one-time CSV import/upsert script for
powered_by.models.Project.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.
| File | Description |
|---|---|
| powered_by/scripts/pbs_ingest_script.py | Adds a one-off CSV import/upsert utility for Project records. |
| powered_by/migrations/0002_fix_jsonfield_columns.py | Adds a schema-fix migration to convert legacy array-typed columns to jsonb. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| Usage: | ||
| python powered_by/scripts/import_pbs.py path/to/pbs.csv | ||
| """ |
| try: | ||
| dt = timezone.datetime.fromisoformat(value) | ||
| except ValueError: |
| for line_num, row in enumerate(reader, start=2): | ||
| project_link = (row.get("project_link") or "").strip() | ||
| if not project_link: | ||
| print(f"line {line_num}: skipping, no project_link", file=sys.stderr) | ||
| error_count += 1 | ||
| continue | ||
|
|
| IF EXISTS ( | ||
| SELECT 1 FROM information_schema.columns | ||
| WHERE table_name = 'powered_by_project' | ||
| AND column_name = 'sefaria_tools_used' | ||
| AND data_type != 'jsonb' | ||
| ) THEN | ||
| ALTER TABLE powered_by_project | ||
| ALTER COLUMN sefaria_tools_used TYPE jsonb | ||
| USING to_jsonb(sefaria_tools_used); | ||
| ALTER TABLE powered_by_project | ||
| ALTER COLUMN tags TYPE jsonb | ||
| USING to_jsonb(tags); | ||
| END IF; |
| #!/usr/bin/env python | ||
| """ | ||
| One-time import: upsert Powered by Sefaria projects from a CSV export | ||
| (matching the powered_by.models.Project field names) into the Project table. | ||
| Rows are matched on project_link, so re-running is safe -- existing rows are | ||
| updated in place rather than duplicated. | ||
|
|
| IF EXISTS ( | ||
| SELECT 1 FROM information_schema.columns | ||
| WHERE table_name = 'powered_by_project' | ||
| AND column_name = 'sefaria_tools_used' |
There was a problem hiding this comment.
[claude] The guard tests only sefaria_tools_used, but the THEN branch alters both this column and tags (lines 27-32). If a database has tags still character varying[] while sefaria_tools_used is already jsonb, the whole block is skipped and tags is never corrected.
Since the stated cause is an in-place edit to 0001_initial, the two columns can have drifted independently on any database that applied it mid-flight — so the "no-op on prod" claim is only verified for sefaria_tools_used. Gating each ALTER on its own IF EXISTS makes this correct regardless of how they drifted.
| END IF; | ||
| END $$; | ||
| """, | ||
| reverse_sql=migrations.RunSQL.noop, |
There was a problem hiding this comment.
[claude] noop marks this reversible in Django's bookkeeping, but rolling back to 0001 won't restore the array type. Fine for a corrective migration — worth a one-line comment so nobody reads migrate powered_by 0001 as an undo.
|
|
||
| try: | ||
| defaults = {field_name: (row.get(field_name) or "").strip() for field_name in DIRECT_FIELDS} | ||
| defaults["submission_source"] = SUBMISSION_SOURCE_MAP[(row.get("submission_source") or "").strip().lower()] |
There was a problem hiding this comment.
[claude] An unmapped submission_source raises KeyError here, which except Exception (line 107) catches and continues past — so the entire row is dropped, including its valid fields. The printed message for a bare KeyError is just the missing key, which makes it hard to tell a bad source value from a real failure. .get(..., <default>) plus a warning would keep the row.
| updated in place rather than duplicated. | ||
|
|
||
| Usage: | ||
| python powered_by/scripts/import_pbs.py path/to/pbs.csv |
There was a problem hiding this comment.
[claude] Usage line says import_pbs.py; the file is pbs_ingest_script.py.
Migration
For some reason,
prodandlocalare aligned but dev env has a weird field on the powered_by postgres. This migration fixes the dev env to be aligned with prod and local - and will be trivial when run on prod (makes no changes).Script
Script which ingests PBS projects from a CSV, can be taken out.