data: enforce user_places app contract in the database (#170)

[cekuu35]

Closes #170

[vercel]

@cekuu35 is attempting to deploy a commit to the studentsuite Team on Vercel.

A member of the Team first needs to authorize it.

[yakew7]

Hey @cekuu35! Will check your pr and merge it if it is good meanwhile could you pls star the repo since it helps the project grow and also helps others find this project

[shauryagangrade]

Review: enforce user_places app contract (#170)

Nice, focused hardening migration. The constraint list matches PLACE_TYPES exactly (src/lib/types.ts:1), the header's rationale for the explicit WITH CHECK is accurate (Postgres reuses USING as the check against the NEW row when WITH CHECK is absent), and re-creating the policies makes that invariant explicit rather than incidental. Checks are green.

Two things worth addressing before merge:

1. Existing rows can make ADD CONSTRAINT fail (blocking)

ALTER TABLE ... ADD CONSTRAINT ... CHECK validates every existing row under an ACCESS EXCLUSIVE lock. This repo has already had data-shape churn on type (imp_locations → other_places rename per CHANGELOG, plus bulk CEEB import artifacts like ALL-CAPS cities). If any production row still carries a retired type or an out-of-range coord, this migration will fail mid-deploy and block everything behind it.

Suggested options:

• Precede it with a normalizing UPDATE scrubbing/retiring unknown types (or run one manually first), or
• Use the two-step pattern so validation never blocks the transaction:

  alter table user_places add constraint user_places_type_check check (...) not valid;
  alter table user_places validate constraint user_places_type_check;

  (still fails if bad rows exist, but the failure surfaces as a clean validation step instead of a locked table)

If user_places is known-empty/tiny in prod, shipping as-is is defensible — but say so in the header.

2. Type-list drift risk (nit, non-blocking)

The 6-type enum now lives in 4+ places: src/lib/types.ts, data/places.schema.json, .github/ISSUE_TEMPLATE/add-place.yml, and now this SQL. When type #7 lands someone will need a follow-up migration. Worth a line in data/CONTRIBUTING.md: "adding a place type also requires a DB migration for user_places.type".

Minor nits, feel free to ignore:

• Could add check (city = lower(city)) to match the slug convention documented in PRODUCT.md — though the app doesn't validate it either, so consistency argues both ways.
• If supabase db push is used against a DB where 20260701_create_user_places.sql was hand-applied via the SQL editor (per that file's header), confirm the base migration is recorded in schema_migrations, otherwise push will replay both files.

Otherwise LGTM once the existing-rows concern is answered.

[cekuu35]

Thanks @shauryagangrade for the sharp review — both points addressed in f845512:

1. Existing rows / blocking: adopted the two-step pattern for all five constraints (ADD CONSTRAINT ... NOT VALID + separate VALIDATE CONSTRAINT). On top of that, unknown type values are normalized to 'other_places' first — it's the bucket the app already uses for anything unmapped (and the historical landing spot of the imp_locations rename). Coordinates are not silently rewritten; a surviving out-of-range row now surfaces as a clean validation failure on a specific constraint instead of a locked-table timeout.

2. Type-list drift: added an "Adding a new place type" section to data/CONTRIBUTING.md listing all four places the enum lives, including the DB migration.

On the nits: skipping the city = lower(city) check for exactly the consistency reason you mention — happy to revisit if the app ever starts enforcing slugs. And good catch on the hand-applied base migration: if anyone's in that state, supabase migration repair 20260701_create_user_places.sql records it before db push.

@yakew7 starred o/

[yakew7]

@cekuu35 thanks for the star appreciate it! @shauryagangrade will look into merging the pr and if all the edits r fine we will be merging it! thanks for the contributions

[shauryagangrade]

Re-review: f845512

Both substantive points are properly addressed:

1. Non-blocking rollout ✅ — The normalizing UPDATE (unknown types → other_places) runs before any constraint exists, and all five constraints ship as NOT VALID + separate VALIDATE. Correct semantics: ADD CONSTRAINT ... NOT VALID only touches the catalog briefly, VALIDATE takes SHARE UPDATE EXCLUSIVE so DML keeps flowing, and surviving bad rows now surface as a clean validation failure naming the exact constraint instead of a locked-table deploy hang. Ordering and lock behavior check out.

2. Type-drift checklist ✅ — Covers all four homes of the enum (types.ts, places.schema.json, issue template, DB migration) with a sensible note that labels/colors derive from PLACE_TYPES.

CI green on this head (Vercel "fail" is the same pre-existing deploy-auth issue, unrelated).

Two consciously-skipped nits from round one (city = lower(city), schema_migrations operational note) were marked feel-free-to-ignore — agreeing they're fine to leave out; skipping them is defensible since the app doesn't validate either.

One last non-blocking nit on the docs wording: data/CONTRIBUTING.md pins the migration by filename (20260822_harden_user_places.sql). When type #7 lands, that pointer goes stale — consider phrasing it generically ("a new DB migration extending the CHECK constraint") in a follow-up. Not worth another round trip.

Approving. Nice responsive turnaround.