feat(registreren): oprichter krijgt school_role='owner' (eigenaar-SSOT) - #36
Conversation
De registratie-oprichter is de eigenaar van de nieuwe school en krijgt daarom 'owner' i.p.v. 'admin' (eigenaar-SSOT, ribbaPro-migratie 20260724160000). Bewust géén stille fallback naar 'admin' als de CHECK-constraint 'owner' nog niet kent: registratie faalt dan hard met volledige cleanup en een diagnostische logregel (23514) die naar de ontbrekende migratie wijst. Test hernoemd naar register-school-owner-role.test.mjs; nieuw regressietestpad pint het fail-closed-gedrag vast (één owner-poging, geen fallback-insert, school + auth-user opgeruimd, geen vervolg-writes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
📝 WalkthroughWalkthroughSchool registration now creates the founding instructor with ChangesOwner role registration
Estimated code review effort: 2 (Simple) | ~10 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@app/api/register-school/route.ts`:
- Around line 323-333: Handle and persist the `{ error }` results from cleanup
operations in the school- and instructor-failure paths, including `deleteUser`
and the school deletion near the instructor insert failure. Before reporting
rollback as complete, detect cleanup failures, log or propagate them durably,
and reconcile any orphaned auth user or school record.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 6a837522-8cb4-4309-b114-f9dd4d376b0b
📒 Files selected for processing (2)
app/api/register-school/route.tstests/register-school-owner-role.test.mjs
| if (instructorError || !instructor) { | ||
| console.error('Instructor insert error:', instructorError); | ||
| if (instructorError?.code === '23514') { | ||
| // check_violation — vrijwel zeker instructors_school_role_check die | ||
| // 'owner' nog niet kent: ribbaPro-migratie 20260724160000 ontbreekt. | ||
| console.error( | ||
| 'Instructor insert geweigerd door CHECK-constraint. Is ribbaPro-migratie 20260724160000 (school_role owner) toegepast?', | ||
| instructorError, | ||
| ); | ||
| } else { | ||
| console.error('Instructor insert error:', instructorError); | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)route\.ts$|migrations|supabase|schema|supabase' | head -200
echo
echo "== target file outline/size =="
wc -l app/api/register-school/route.ts 2>/dev/null || true
ast-grep outline app/api/register-school/route.ts --view expanded 2>/dev/null | sed -n '1,220p' || true
echo
echo "== cleanup/delete related code =="
rg -n "supabase\.|from \('`@supabase`|\.from\(|\.delete\(|\.delete\b|auth|owners?|school|instructor|rollback|cleanup|delete|error\)" app/api/register-school/route.ts | sed -n '1,260p'
echo
echo "== target lines 260-390 =="
cat -n app/api/register-school/route.ts | sed -n '260,390p'Repository: PolderLabs/ribba-web
Length of output: 10945
🌐 Web query:
Supabase JavaScript Delete returns error object from database operations documentation
💡 Result:
In the Supabase JavaScript client, the delete method follows the standard pattern for database operations: it returns an object containing { data, error, count, status, statusText } [1][2]. Key points regarding the error object and delete operations include: 1. Standard Error Handling: When a database operation fails, the error field will contain a PostgrestError object [3]. This object typically includes a message, code, details, and a hint [3]. It is recommended to log the entire error object rather than just the message, as the hint field often contains the actionable fix provided by Postgres [3]. 2. RLS and Deletion Behavior: A common source of confusion is that a delete operation may appear "successful" (resulting in a null error) even when no rows are deleted [4][2]. This is standard behavior in PostgreSQL: if a Row Level Security (RLS) policy prevents a user from modifying or even "seeing" a row to delete it, the database does not necessarily throw an error [4]. Instead, it may simply return a successful status (e.g., 204 No Content) because the command completed without encountering a technical failure [4][5]. 3. Verifying Deletions: Because delete operations do not return rows by default, they do not provide immediate confirmation that a specific row was removed [1][6]. If you need to confirm that a row was deleted, you can chain the.select method to your delete call [1][6]. If the returned data array is empty, it indicates that no rows were deleted [4]. Additionally, you can check the status and statusText fields returned by the operation to inspect the HTTP response [2]. If you are encountering issues where you suspect a deletion is being blocked by RLS, ensure that your policy allows both the DELETE action and the necessary SELECT permissions (as PostgreSQL often needs to "see" the row to delete it) [4].
Citations:
- 1: https://supabase.com/docs/reference/javascript/delete
- 2: Supabase JS library delete does not give any response supabase/postgrest-js#535
- 3: https://supabase.com/docs/guides/api/handling-errors-in-supabase-js
- 4: error is always null if delete is not successful due to RLS policy supabase/supabase-js#902
- 5: Delete RLS violations return 204 not 403 supabase/postgrest-js#409
- 6: https://supabase.com/docs/reference/javascript/v1/delete
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== target lines 390-471 =="
cat -n app/api/register-school/route.ts | sed -n '390,471p'
echo
echo "== read-only check: error-destructuring and delete result usage =="
python3 - <<'PY'
from pathlib import Path
p = Path("app/api/register-school/route.ts")
text = p.read_text()
for name, marker in [
("school cleanup block", "schoolError || !school"),
("instructor cleanup block", "instructorError || !instructor"),
("catch cleanup block", "if (authUserId)"),
]:
idx = text.find(marker)
start = max(0, text.rfind("\n", 0, idx) - 200)
end = text.find("\n", idx, idx + 800)
print(f"[{marker}]")
for i, line in enumerate(text[start:end].splitlines(), 1):
if "delete" in line or "error:" in line or "if (" in line:
print(f"{i:4}: {line}")
print()
# Extract standalone await delete/deleteUser lines outside matching destructured {error} contexts.
import re
lines = text.splitlines()
for i, line in enumerate(lines, 1):
stripped = line.strip()
if not stripped.startswith("await ") or ("delete" not in stripped and ".deleteUser" not in stripped):
continue
lookback = text.rfind("\n", 0, text.find(line, text.rfind("\n", 0, i * 80)))
pre = text[tex t.rfind("\n", 0, lookback) : i * 80] if isinstance(text, str) else "" # avoid typos?
PYRepository: PolderLabs/ribba-web
Length of output: 5188
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("app/api/register-school/route.ts")
lines = p.read_text().splitlines()
delete_lines = []
for i, line in enumerate(lines, start=1):
if "await" in line and ("deleteUser" in line or ".delete(" in line):
snippet = line.strip()
before_close = line[:line.find("await")].rstrip()
after_close = line[line.find("await") + 5:].strip()
if not before_close and after_close.startswith("const ") and "{ error }" in after_close:
status = "ERROR_DISCARDED"
elif not before_close and snip.
PYRepository: PolderLabs/ribba-web
Length of output: 285
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
p = Path("app/api/register-school/route.ts")
text = p.read_text()
lines = p.read_text().splitlines()
cleanup_block = text[
text.find("if (schoolError || !school)") : text.find("if (instructorError || !instructor)")
] + "\n" + text[
text.find("if (instructorError || !instructor)") : text.find("return NextResponse.json({ success: true })")
]
print("cleanup_blocks_found:")
for query in ["deleteUser", ".delete(", "{ error }", "return NextResponse.json"]:
print(f" {query}: {cleanup_block.count(query)}")
await_delete_user = [i for i,l in enumerate(lines,1) if "deleteUser" in l and l.strip().startswith("await")]
await_delete_db = [i for i,l in enumerate(lines,1) if ".delete(" in l and l.strip().startswith("await")]
print("await deleteUser line numbers:", await_delete_user)
print("await db delete line numbers:", await_delete_db)
for i in await_delete_user + await_delete_db:
print(f"{i}: {lines[i-1].strip()}")
# Show context around relevant blocks and any try/catch that wraps them.
targets = []
for start, end in [(294,302),(334,341),(458,464)]:
targets.append((start, end))
for start, end in targets:
print(f"\nlines {start}-{end}:")
for idx in range(start, min(end+1, len(lines)+1)):
print(f"{idx}: {lines[idx-1]}")
PYRepository: PolderLabs/ribba-web
Length of output: 1549
Handle cleanup operations that return errors.
Supabase returns deletion failures in { error }, but the cleanup results in the school- and instructor-failure paths are discarded. A failed deleteUser can leave orphaned auth data; a failed school delete at line 335 can leave a school with no corresponding instructor. Check the returned errors durably and reconcile before reporting the rollback as complete.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@app/api/register-school/route.ts` around lines 323 - 333, Handle and persist
the `{ error }` results from cleanup operations in the school- and
instructor-failure paths, including `deleteUser` and the school deletion near
the instructor insert failure. Before reporting rollback as complete, detect
cleanup failures, log or propagate them durably, and reconcile any orphaned auth
user or school record.
1 participant
Wat
De registratie-oprichter in
app/api/register-school/route.tswordt aangemaakt metschool_role: 'owner'in plaats van'admin', conform het eigenaar-SSOT-rollenmodel (ribbaPro PR #291, ontwerpdocs/design/owner-role-ssot-ontwerp-2026-07-24.md).Volgorde is bindend:
20260724160000(migratie A: CHECK-constraintinstructors_school_role_checkkent'owner', partial unique index, guard G1) applied op productie — apart GO in het ribbaPro-traject.Wordt deze PR vóór stap 1 gedeployed, dan weigert de CHECK-constraint
'owner'en faalt elke schoolregistratie (zie fail-closed-gedrag hieronder). Dat is een bewuste keuze, geen bug.Fail-closed, geen stille fallback
Overwogen: bij CHECK-weigering terugvallen op
'admin'. Afgewezen — een stille fallback maakt een school zonder eigenaar aan en maskeert precies de deploy-volgorde-fout die we willen zien. In lijn met de bestaande regel in dit bestand ("nooit stil terugvallen op een default") en het fail-closed-principe van het owner-ontwerp:23514(check_violation) wijst expliciet naar de ontbrekende migratie.Tests
tests/register-school-admin-role.test.mjs→ hernoemd naarregister-school-owner-role.test.mjs; asserteert nu'owner'.Inventaris ribba-web (rol-aannames)
Repo-brede grep op
school_role,is_school_adminen rol-literals: register-school (route + test) is de enige plek in ribba-web dieschool_roleschrijft of leest. Geenis_school_admin-aanroepen, geen'owner'/'employee'-vergelijkingen elders; overigerole-treffers zijn marketplace-chatrollen (rijschool/leerling), auth-metadata en HTML-attributen.🤖 Generated with Claude Code
Summary by CodeRabbit
New Features
Bug Fixes