Uh oh!
There was an error while loading. Please reload this page.
fix(calendar): rewire assignments to the enrollment-keyed schema (dashboard 500) - #283
Merged
Merged
Conversation
The calendar route + calendar_service still query the pre-redesign assignments table (user_id/course_id/courses!left); 0021 re-keyed assignments on enrollment_id, so every call 400s -> 500 and tanks the dashboard. Spec rewires the calendar domain to resolve course -> enrollment (mirroring gradebook.py), keeping the HTTP shapes stable. Decisions: assignments are always course-tied (no migration), writes auto-create the enrollment, full-domain scope. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Rewires sync_to_google and export_to_google onto the enrollment-keyed schema: select/write-back scoped by enrollment_id membership instead of the removed user_id column; drops courses!left embed in favour of _course_meta_cached. Updates test_calendar_export_idor.py and test_calendar_sibling_write_scoping.py to assert the new enrollment_id boundary (same IDOR guarantee, new key). Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…ment schema - routes/documents.py: pass source="syllabus" at both save_assignments_to_db call sites (_save_orchestrator_syllabus and the legacy call_gemini_json path) - tests/test_calendar_routes.py: rewire TestSaveAssignments to include course_id in fixtures and mock enrollment_id_for/user_enrollment_ids; rewire TestGetUpcoming.test_returns_assignments_from_db to the enrollment-keyed row shape (enrollment_id, no user_id/course_id/courses columns); add _tbl helper - tests/test_assignment_notes_encryption.py: supply course_id to test fixtures and mock academics so insert_new_assignments reaches the encryption boundary - tests/test_documents_routes.py: update assert_called_once_with to include source='syllabus' to match the new tagged call Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
📝 WalkthroughWalkthroughAll calendar/assignment backend routes, the calendar service, and the academics service are updated to use ChangesCalendar Assignments Enrollment-keyed Rewire
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
🚥 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 |
Deploying with |
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs | frontend-staging | 8a38cfe | Commit Preview URL Branch Preview URL | Jun 28 2026, 08:38 PM |
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
docs/superpowers/specs/2026-06-28-calendar-assignments-enrollment-rewire-design.md (1)
57-60: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueAdd language identifier to fenced code block.
The fenced code block showing
enrollment_id_forsignature lacks a language label. Addpythonfor syntax highlighting and to satisfy linting.+```python
enrollment_id_for(user_id, course_id, *, create=False) -> str | None🤖 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 `@docs/superpowers/specs/2026-06-28-calendar-assignments-enrollment-rewire-design.md` around lines 57 - 60, The fenced code block for the enrollment_id_for signature is missing a language identifier, which triggers linting. Update the code fence in the design doc to use python for the signature shown near enrollment_id_for so it is properly highlighted and passes the docs check.backend/tests/test_calendar_routes.py (1)
16-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMove the reusable table mock into
tests/conftest.py.
_tblis now duplicated acrossbackend/tests/test_calendar_routes.py,backend/tests/test_calendar_read_enrollment.py, andbackend/tests/test_calendar_scoping_enrollment.py, so any change to the fake table contract has to be kept in sync by hand. As per coding guidelines, "shared fixtures such as mock Supabase and mock Gemini belong intests/conftest.py."🤖 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 `@backend/tests/test_calendar_routes.py` around lines 16 - 21, The reusable table mock helper `_tbl` is duplicated across multiple calendar tests, so move it into `tests/conftest.py` as a shared fixture/helper and update `test_calendar_routes`, `test_calendar_read_enrollment`, and `test_calendar_scoping_enrollment` to import/use the common version. Keep the existing `MagicMock` table contract and preserve the per-verb return-value behavior so all tests share one source of truth.Source: Coding guidelines
🤖 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 `@backend/services/academics.py`:
- Around line 159-181: The enrollment lookup in the helper that uses
user_offering_ids_for_course, current_term, and resolve_offering should not fall
back to an arbitrary historical offering when create=True. Change the selection
logic so it only reuses an existing enrollment if it matches the current term,
and otherwise let the code continue into the creation path; keep the existing
enrollment return path only for the current-term match. This ensures the branch
in backend/services/academics.py provisioned by create=True does not return a
random old enrollment.
In `@backend/tests/test_academics_enrollment_resolver.py`:
- Around line 27-36: The existing test only exercises the single-enrollment
fallback and never hits the current-term preference path. Update
test_existing_enrollment_current_term in the academics enrollment resolver tests
to use multiple course offerings and a non-None current_term so
ac.enrollment_id_for actually has to choose between enrollments. Make the setup
in services.academics.user_offering_ids_for_course and
services.academics.current_term align with the new branch, and assert the
selected enrollment comes from the current term.
In `@backend/tests/test_calendar_sibling_write_scoping.py`:
- Around line 22-77: The tests only verify write filters, but they should also
cover the guarded read path in the calendar routes. Update the assertions in
test_update_scopes_write_by_enrollment_id,
test_delete_scopes_delete_by_enrollment_id, and
test_sync_scopes_writeback_by_enrollment_id to inspect the relevant table.select
call kwargs and confirm the same enrollment_id membership guard is used before
the write/delete. Use the existing routes.calendar.table and
routes.calendar.academics mocks to locate the select/filter setup and assert it
matches the write-scoping behavior.
---
Nitpick comments:
In `@backend/tests/test_calendar_routes.py`:
- Around line 16-21: The reusable table mock helper `_tbl` is duplicated across
multiple calendar tests, so move it into `tests/conftest.py` as a shared
fixture/helper and update `test_calendar_routes`,
`test_calendar_read_enrollment`, and `test_calendar_scoping_enrollment` to
import/use the common version. Keep the existing `MagicMock` table contract and
preserve the per-verb return-value behavior so all tests share one source of
truth.
In
`@docs/superpowers/specs/2026-06-28-calendar-assignments-enrollment-rewire-design.md`:
- Around line 57-60: The fenced code block for the enrollment_id_for signature
is missing a language identifier, which triggers linting. Update the code fence
in the design doc to use python for the signature shown near enrollment_id_for
so it is properly highlighted and passes the docs check.
🪄 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: 22a175b3-0b0b-4e62-bcf5-51940fdcfee8
📒 Files selected for processing (16)
backend/routes/calendar.pybackend/routes/documents.pybackend/services/academics.pybackend/services/calendar_service.pybackend/tests/test_academics_enrollment_resolver.pybackend/tests/test_assignment_notes_encryption.pybackend/tests/test_calendar_export_idor.pybackend/tests/test_calendar_read_enrollment.pybackend/tests/test_calendar_routes.pybackend/tests/test_calendar_scoping_enrollment.pybackend/tests/test_calendar_sibling_write_scoping.pybackend/tests/test_calendar_sync_export_enrollment.pybackend/tests/test_calendar_write_enrollment.pybackend/tests/test_documents_routes.pydocs/superpowers/plans/2026-06-28-calendar-assignments-enrollment-rewire.mddocs/superpowers/specs/2026-06-28-calendar-assignments-enrollment-rewire-design.md
Comment on lines
+159
to
+181
| offering_ids = user_offering_ids_for_course(user_id, course_id) | ||
| if offering_ids: | ||
| chosen = offering_ids[0] | ||
| cur = current_term() | ||
| cur_id = cur["id"] if cur else None | ||
| if cur_id: | ||
| for oid in offering_ids: | ||
| t = term_for_offering(oid) | ||
| if t and t.get("id") == cur_id: | ||
| chosen = oid | ||
| break | ||
| rows = table("enrollments").select( | ||
| "id", | ||
| filters={"user_id": f"eq.{user_id}", "offering_id": f"eq.{chosen}"}, | ||
| limit=1, | ||
| ) | ||
| if rows: | ||
| return rows[0]["id"] | ||
| if not create: | ||
| return None | ||
| offering_id = resolve_offering(course_id, create=True) |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick win
Don't reuse an arbitrary historical enrollment when create=True.
If the user already has past enrollments for the course but none in current_term(), this branch falls back to offering_ids[0] and returns that enrollment instead of reaching the create path. Because user_offering_ids_for_course() does not order its rows, new assignment writes can land on a random old enrollment rather than the current-term enrollment this helper is meant to provision.
Suggested fix
- offering_ids = user_offering_ids_for_course(user_id, course_id)- if offering_ids:- chosen = offering_ids[0]- cur = current_term()- cur_id = cur["id"] if cur else None- if cur_id:- for oid in offering_ids:- t = term_for_offering(oid)- if t and t.get("id") == cur_id:- chosen = oid- break- rows = table("enrollments").select(- "id",- filters={"user_id": f"eq.{user_id}", "offering_id": f"eq.{chosen}"},- limit=1,- )- if rows:- return rows[0]["id"]+ offering_ids = user_offering_ids_for_course(user_id, course_id)+ if offering_ids:+ cur = current_term()+ cur_id = cur["id"] if cur else None+ chosen = None+ if cur_id:+ for oid in offering_ids:+ t = term_for_offering(oid)+ if t and t.get("id") == cur_id:+ chosen = oid+ break+ elif len(offering_ids) == 1 and not create:+ chosen = offering_ids[0]++ if chosen:+ rows = table("enrollments").select(+ "id",+ filters={"user_id": f"eq.{user_id}", "offering_id": f"eq.{chosen}"},+ limit=1,+ )+ if rows:+ return rows[0]["id"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| offering_ids=user_offering_ids_for_course(user_id, course_id) | |
| ifoffering_ids: | |
| chosen=offering_ids[0] | |
| cur=current_term() | |
| cur_id=cur["id"] ifcurelseNone | |
| ifcur_id: | |
| foroidinoffering_ids: | |
| t=term_for_offering(oid) | |
| iftandt.get("id") ==cur_id: | |
| chosen=oid | |
| break | |
| rows=table("enrollments").select( | |
| "id", | |
| filters={"user_id": f"eq.{user_id}", "offering_id": f"eq.{chosen}"}, | |
| limit=1, | |
| ) | |
| ifrows: | |
| returnrows[0]["id"] | |
| ifnotcreate: | |
| returnNone | |
| offering_id=resolve_offering(course_id, create=True) | |
| offering_ids=user_offering_ids_for_course(user_id, course_id) | |
| ifoffering_ids: | |
| cur=current_term() | |
| cur_id=cur["id"] ifcurelseNone | |
| chosen=None | |
| ifcur_id: | |
| foroidinoffering_ids: | |
| t=term_for_offering(oid) | |
| iftandt.get("id") ==cur_id: | |
| chosen=oid | |
| break | |
| eliflen(offering_ids) ==1andnotcreate: | |
| chosen=offering_ids[0] | |
| ifchosen: | |
| rows=table("enrollments").select( | |
| "id", | |
| filters={"user_id": f"eq.{user_id}", "offering_id": f"eq.{chosen}"}, | |
| limit=1, | |
| ) | |
| ifrows: | |
| returnrows[0]["id"] | |
| ifnotcreate: | |
| returnNone | |
| offering_id=resolve_offering(course_id, create=True) |
🤖 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 `@backend/services/academics.py` around lines 159 - 181, The enrollment lookup
in the helper that uses user_offering_ids_for_course, current_term, and
resolve_offering should not fall back to an arbitrary historical offering when
create=True. Change the selection logic so it only reuses an existing enrollment
if it matches the current term, and otherwise let the code continue into the
creation path; keep the existing enrollment return path only for the
current-term match. This ensures the branch in backend/services/academics.py
provisioned by create=True does not return a random old enrollment.
Comment on lines
+27
to
+36
| def test_existing_enrollment_current_term(self): | ||
| # user_offering_ids_for_course -> ["o1"]; term match; enrollment e1 | ||
| tables = { | ||
| "course_offerings": _tbl(select=[{"id": "o1"}]), | ||
| "enrollments": _tbl(select=[{"id": "e1"}]), | ||
| } | ||
| with patch("services.academics.table", side_effect=_dispatch(tables)), \ | ||
| patch("services.academics.user_offering_ids_for_course", return_value=["o1"]), \ | ||
| patch("services.academics.current_term", return_value=None): | ||
| assert ac.enrollment_id_for("user_andres", "CS101") == "e1" |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
This test never reaches the current-term preference branch.
current_term is mocked to None and there is only one offering, so the resolver returns the lone enrollment without evaluating any term match. Please make this a multi-offering case with a real current term so the new "prefer current-term enrollment" logic is actually covered.
🤖 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 `@backend/tests/test_academics_enrollment_resolver.py` around lines 27 - 36,
The existing test only exercises the single-enrollment fallback and never hits
the current-term preference path. Update test_existing_enrollment_current_term
in the academics enrollment resolver tests to use multiple course offerings and
a non-None current_term so ac.enrollment_id_for actually has to choose between
enrollments. Make the setup in services.academics.user_offering_ids_for_course
and services.academics.current_term align with the new branch, and assert the
selected enrollment comes from the current term.
Comment on lines
+22
to
+77
| def test_update_scopes_write_by_enrollment_id(self): | ||
| with patch("routes.calendar.table") as t, \ | ||
| patch("routes.calendar.academics") as ac: | ||
| ac.user_enrollment_ids.return_value = [{"id": ENROLLMENT_ID, "offering_id": "o1"}] | ||
| t.return_value.select.return_value = [{"id": AID}] # owner's row exists | ||
| r = client.patch( | ||
| f"/api/calendar/assignments/{AID}", | ||
| json={"user_id": OWNER, "title": "New title"}, | ||
| ) | ||
| assert r.status_code == 200 | ||
| # The UPDATE filter must include user_id, not just id. | ||
| # The UPDATE filter must scope by enrollment_id, not user_id. | ||
| update_filters = t.return_value.update.call_args.kwargs["filters"] | ||
| assert update_filters.get("user_id") == f"eq.{OWNER}" | ||
| assert "enrollment_id" in update_filters | ||
| assert ENROLLMENT_ID in update_filters["enrollment_id"] | ||
| assert update_filters.get("id") == f"eq.{AID}" | ||
| def test_delete_scopes_delete_by_user_id(self): | ||
| with patch("routes.calendar.table") as t: | ||
| def test_delete_scopes_delete_by_enrollment_id(self): | ||
| with patch("routes.calendar.table") as t, \ | ||
| patch("routes.calendar.academics") as ac: | ||
| ac.user_enrollment_ids.return_value = [{"id": ENROLLMENT_ID, "offering_id": "o1"}] | ||
| t.return_value.select.return_value = [{"id": AID}] | ||
| r = client.delete(f"/api/calendar/assignments/{AID}?user_id={OWNER}") | ||
| assert r.status_code == 200 | ||
| delete_filters = t.return_value.delete.call_args.kwargs["filters"] | ||
| assert delete_filters.get("user_id") == f"eq.{OWNER}" | ||
| assert "enrollment_id" in delete_filters | ||
| assert ENROLLMENT_ID in delete_filters["enrollment_id"] | ||
| assert delete_filters.get("id") == f"eq.{AID}" | ||
| def test_sync_scopes_writeback_by_user_id(self): | ||
| def test_sync_scopes_writeback_by_enrollment_id(self): | ||
| unsynced = [{ | ||
| "id": AID, "title": "HW", "due_date": "2026-03-01", | ||
| "notes": None, "google_event_id": None, "courses": {}, | ||
| "id": AID, "enrollment_id": ENROLLMENT_ID, "title": "HW", | ||
| "due_date": "2026-03-01", "notes": None, "google_event_id": None, | ||
| }] | ||
| with patch("routes.calendar._require_google_creds", return_value=MagicMock()), \ | ||
| patch("routes.calendar.build") as build, \ | ||
| patch("routes.calendar.decrypt_if_present", return_value=""), \ | ||
| patch("routes.calendar.table") as t: | ||
| patch("routes.calendar.table") as t, \ | ||
| patch("routes.calendar.academics") as ac: | ||
| service = MagicMock() | ||
| service.events.return_value.insert.return_value.execute.return_value = {"id": "evt_1"} | ||
| build.return_value = service | ||
| ac.user_enrollment_ids.return_value = [{"id": ENROLLMENT_ID, "offering_id": "o1"}] | ||
| # offering_course_id returns None so _course_meta_cached skips the | ||
| # courses table select (keeps select side_effect list simple). | ||
| ac.offering_course_id.return_value = None | ||
| # select returns the unsynced row on the first call, [] thereafter. | ||
| t.return_value.select.side_effect = [unsynced, []] | ||
| r = client.post("/api/calendar/sync", json={"user_id": OWNER}) | ||
| assert r.status_code == 200 | ||
| update_filters = t.return_value.update.call_args.kwargs["filters"] | ||
| assert update_filters.get("user_id") == f"eq.{OWNER}" | ||
| # Write-back must scope by enrollment_id (not user_id, which no longer | ||
| # exists on the assignments table) — same IDOR guarantee, new key. | ||
| assert "enrollment_id" in update_filters | ||
| assert ENROLLMENT_ID in update_filters["enrollment_id"] |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟡 Minor | ⚡ Quick win
Assert the guarded SELECT is enrollment-scoped too.
These cases only verify the update/delete filters. Because the mocked select always returns the owned row here, the tests still pass if the PATCH/DELETE ownership check regresses to filters={"id": ...} or if SYNC stops filtering the unsynced read by enrollment_id. Please assert the relevant select call kwargs carry the same membership guard.
Suggested assertions
assert r.status_code == 200
+ select_filters = t.return_value.select.call_args.kwargs["filters"]+ assert "enrollment_id" in select_filters+ assert ENROLLMENT_ID in select_filters["enrollment_id"]+ assert select_filters.get("id") == f"eq.{AID}"
# The UPDATE filter must scope by enrollment_id, not user_id.
update_filters = t.return_value.update.call_args.kwargs["filters"]
assert "enrollment_id" in update_filters
assert ENROLLMENT_ID in update_filters["enrollment_id"]
assert r.status_code == 200
+ select_filters = t.return_value.select.call_args.kwargs["filters"]+ assert "enrollment_id" in select_filters+ assert ENROLLMENT_ID in select_filters["enrollment_id"]+ assert select_filters.get("id") == f"eq.{AID}"
delete_filters = t.return_value.delete.call_args.kwargs["filters"]
assert "enrollment_id" in delete_filters
assert ENROLLMENT_ID in delete_filters["enrollment_id"]
assert r.status_code == 200
+ first_select_filters = t.return_value.select.call_args_list[0].kwargs["filters"]+ assert "enrollment_id" in first_select_filters+ assert ENROLLMENT_ID in first_select_filters["enrollment_id"]
update_filters = t.return_value.update.call_args.kwargs["filters"]
assert "enrollment_id" in update_filters
assert ENROLLMENT_ID in update_filters["enrollment_id"]📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| deftest_update_scopes_write_by_enrollment_id(self): | |
| withpatch("routes.calendar.table") ast, \ | |
| patch("routes.calendar.academics") asac: | |
| ac.user_enrollment_ids.return_value= [{"id": ENROLLMENT_ID, "offering_id": "o1"}] | |
| t.return_value.select.return_value= [{"id": AID}] # owner's row exists | |
| r=client.patch( | |
| f"/api/calendar/assignments/{AID}", | |
| json={"user_id": OWNER, "title": "New title"}, | |
| ) | |
| assertr.status_code==200 | |
| # The UPDATE filter must include user_id, not just id. | |
| # The UPDATE filter must scope by enrollment_id, not user_id. | |
| update_filters=t.return_value.update.call_args.kwargs["filters"] | |
| assertupdate_filters.get("user_id") ==f"eq.{OWNER}" | |
| assert"enrollment_id"inupdate_filters | |
| assertENROLLMENT_IDinupdate_filters["enrollment_id"] | |
| assertupdate_filters.get("id") ==f"eq.{AID}" | |
| deftest_delete_scopes_delete_by_user_id(self): | |
| withpatch("routes.calendar.table") ast: | |
| deftest_delete_scopes_delete_by_enrollment_id(self): | |
| withpatch("routes.calendar.table") ast, \ | |
| patch("routes.calendar.academics") asac: | |
| ac.user_enrollment_ids.return_value= [{"id": ENROLLMENT_ID, "offering_id": "o1"}] | |
| t.return_value.select.return_value= [{"id": AID}] | |
| r=client.delete(f"/api/calendar/assignments/{AID}?user_id={OWNER}") | |
| assertr.status_code==200 | |
| delete_filters=t.return_value.delete.call_args.kwargs["filters"] | |
| assertdelete_filters.get("user_id") ==f"eq.{OWNER}" | |
| assert"enrollment_id"indelete_filters | |
| assertENROLLMENT_IDindelete_filters["enrollment_id"] | |
| assertdelete_filters.get("id") ==f"eq.{AID}" | |
| deftest_sync_scopes_writeback_by_user_id(self): | |
| deftest_sync_scopes_writeback_by_enrollment_id(self): | |
| unsynced= [{ | |
| "id": AID, "title": "HW", "due_date": "2026-03-01", | |
| "notes": None, "google_event_id": None, "courses": {}, | |
| "id": AID, "enrollment_id": ENROLLMENT_ID, "title": "HW", | |
| "due_date": "2026-03-01", "notes": None, "google_event_id": None, | |
| }] | |
| withpatch("routes.calendar._require_google_creds", return_value=MagicMock()), \ | |
| patch("routes.calendar.build") asbuild, \ | |
| patch("routes.calendar.decrypt_if_present", return_value=""), \ | |
| patch("routes.calendar.table") ast: | |
| patch("routes.calendar.table") ast, \ | |
| patch("routes.calendar.academics") asac: | |
| service=MagicMock() | |
| service.events.return_value.insert.return_value.execute.return_value= {"id": "evt_1"} | |
| build.return_value=service | |
| ac.user_enrollment_ids.return_value= [{"id": ENROLLMENT_ID, "offering_id": "o1"}] | |
| # offering_course_id returns None so _course_meta_cached skips the | |
| # courses table select (keeps select side_effect list simple). | |
| ac.offering_course_id.return_value=None | |
| # select returns the unsynced row on the first call, [] thereafter. | |
| t.return_value.select.side_effect= [unsynced, []] | |
| r=client.post("/api/calendar/sync", json={"user_id": OWNER}) | |
| assertr.status_code==200 | |
| update_filters=t.return_value.update.call_args.kwargs["filters"] | |
| assertupdate_filters.get("user_id") ==f"eq.{OWNER}" | |
| # Write-back must scope by enrollment_id (not user_id, which no longer | |
| # exists on the assignments table) — same IDOR guarantee, new key. | |
| assert"enrollment_id"inupdate_filters | |
| assertENROLLMENT_IDinupdate_filters["enrollment_id"] | |
| deftest_update_scopes_write_by_enrollment_id(self): | |
| withpatch("routes.calendar.table") ast, \ | |
| patch("routes.calendar.academics") asac: | |
| ac.user_enrollment_ids.return_value= [{"id": ENROLLMENT_ID, "offering_id": "o1"}] | |
| t.return_value.select.return_value= [{"id": AID}] # owner's row exists | |
| r=client.patch( | |
| f"/api/calendar/assignments/{AID}", | |
| json={"user_id": OWNER, "title": "New title"}, | |
| ) | |
| assertr.status_code==200 | |
| select_filters=t.return_value.select.call_args.kwargs["filters"] | |
| assert"enrollment_id"inselect_filters | |
| assertENROLLMENT_IDinselect_filters["enrollment_id"] | |
| assertselect_filters.get("id") ==f"eq.{AID}" | |
| # The UPDATE filter must scope by enrollment_id, not user_id. | |
| update_filters=t.return_value.update.call_args.kwargs["filters"] | |
| assert"enrollment_id"inupdate_filters | |
| assertENROLLMENT_IDinupdate_filters["enrollment_id"] | |
| assertupdate_filters.get("id") ==f"eq.{AID}" | |
| deftest_delete_scopes_delete_by_enrollment_id(self): | |
| withpatch("routes.calendar.table") ast, \ | |
| patch("routes.calendar.academics") asac: | |
| ac.user_enrollment_ids.return_value= [{"id": ENROLLMENT_ID, "offering_id": "o1"}] | |
| t.return_value.select.return_value= [{"id": AID}] | |
| r=client.delete(f"/api/calendar/assignments/{AID}?user_id={OWNER}") | |
| assertr.status_code==200 | |
| select_filters=t.return_value.select.call_args.kwargs["filters"] | |
| assert"enrollment_id"inselect_filters | |
| assertENROLLMENT_IDinselect_filters["enrollment_id"] | |
| assertselect_filters.get("id") ==f"eq.{AID}" | |
| delete_filters=t.return_value.delete.call_args.kwargs["filters"] | |
| assert"enrollment_id"indelete_filters | |
| assertENROLLMENT_IDindelete_filters["enrollment_id"] | |
| assertdelete_filters.get("id") ==f"eq.{AID}" | |
| deftest_sync_scopes_writeback_by_enrollment_id(self): | |
| unsynced= [{ | |
| "id": AID, "enrollment_id": ENROLLMENT_ID, "title": "HW", | |
| "due_date": "2026-03-01", "notes": None, "google_event_id": None, | |
| }] | |
| withpatch("routes.calendar._require_google_creds", return_value=MagicMock()), \ | |
| patch("routes.calendar.build") asbuild, \ | |
| patch("routes.calendar.decrypt_if_present", return_value=""), \ | |
| patch("routes.calendar.table") ast, \ | |
| patch("routes.calendar.academics") asac: | |
| service=MagicMock() | |
| service.events.return_value.insert.return_value.execute.return_value= {"id": "evt_1"} | |
| build.return_value=service | |
| ac.user_enrollment_ids.return_value= [{"id": ENROLLMENT_ID, "offering_id": "o1"}] | |
| # offering_course_id returns None so _course_meta_cached skips the | |
| # courses table select (keeps select side_effect list simple). | |
| ac.offering_course_id.return_value=None | |
| # select returns the unsynced row on the first call, [] thereafter. | |
| t.return_value.select.side_effect= [unsynced, []] | |
| r=client.post("/api/calendar/sync", json={"user_id": OWNER}) | |
| assertr.status_code==200 | |
| first_select_filters=t.return_value.select.call_args_list[0].kwargs["filters"] | |
| assert"enrollment_id"infirst_select_filters | |
| assertENROLLMENT_IDinfirst_select_filters["enrollment_id"] | |
| update_filters=t.return_value.update.call_args.kwargs["filters"] | |
| # Write-back must scope by enrollment_id (not user_id, which no longer | |
| # exists on the assignments table) — same IDOR guarantee, new key. | |
| assert"enrollment_id"inupdate_filters | |
| assertENROLLMENT_IDinupdate_filters["enrollment_id"] |
🤖 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 `@backend/tests/test_calendar_sibling_write_scoping.py` around lines 22 - 77,
The tests only verify write filters, but they should also cover the guarded read
path in the calendar routes. Update the assertions in
test_update_scopes_write_by_enrollment_id,
test_delete_scopes_delete_by_enrollment_id, and
test_sync_scopes_writeback_by_enrollment_id to inspect the relevant table.select
call kwargs and confirm the same enrollment_id membership guard is used before
the write/delete. Use the existing routes.calendar.table and
routes.calendar.academics mocks to locate the select/filter setup and assert it
matches the write-scoping behavior.
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 freeto 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.
What & why
After the DB modular redesign, migration
0021_gradebook.sqldidDROP TABLE assignments CASCADEand recreatedassignmentskeyed onenrollment_id(nouser_id/course_id/coursesrelationship).routes/calendar.py+services/calendar_service.pystill spoke the old schema, so every/api/calendar/*call returned PostgREST 400 → 500, which tanked the staging dashboard (itsPromise.allfails on the one bad endpoint). The migration itself had flagged this rewire as deferred ("See issues filed for the code rewire").Reproduced against the live staging DB for the real user: only
/api/calendar/upcoming500'd; all other dashboard domains were already migrated and healthy.Approach
Mirror the already-migrated
gradebook.pyhelpers (no fragile nested PostgREST embeds). Assignments are always course-tied and key onenrollment_id; a small resolver inservices/academics.pybridges(user, abstract course) → enrollment_id. No schema migration. HTTP request/response shapes are unchanged (frontend untouched).Design spec:
docs/superpowers/specs/2026-06-28-calendar-assignments-enrollment-rewire-design.mdPlan:
docs/superpowers/plans/2026-06-28-calendar-assignments-enrollment-rewire.mdChanges (6 TDD commits)
services/academics.py—enrollment_id_for(user, course_id, *, create=False)+user_enrollment_ids(user).get_upcoming/get_all/suggest_study_blocks) — fetch the user's enrollments →assignments WHERE enrollment_id IN (...), decorate with abstractcourse_id/course_code/course_name; no enrollments →{"assignments": []}(the dashboard unblock)./save,calendar_service.insert_new_assignments, syllabus saves indocuments.py) — resolvecourse_id → enrollment_id(create-if-missing), tagsource(manual/syllabus), dedup across the enrollment set, encrypt notes exactly once.update/delete/sync/export) —enrollment_id IN (caller's enrollments)on both the pre-check and the write (preserves IDOR guarantee [P0] calendar.export_to_google cross-user IDOR leaks decrypted private notes #123).Verification
_read_assignmentsresolves against the enrollment-keyed schema (0 rows, no 400).Behavior notes / follow-ups
/savenow requires acourse_id— an empty one is silently skipped (spec Decision 1: assignments are always course-tied). Frontend must always sendcourse_id. Consider a400instead of a silent drop as a follow-up.process_and_save_syllabus(OCR-pipeline helper, only invoked by an opt-in live-DB test, no mounted route) feeds assignments withoutcourse_idand would save 0 — file a ticket if it's ever wired to a route._read_assignmentsselects unusedsource;sync/exportcalluser_enrollment_idstwice; a couple of unused test helpers.🤖 Generated with Claude Code
Summary by CodeRabbit