diff --git a/config/api_router.py b/config/api_router.py index 4e6d6fc..f928f1c 100644 --- a/config/api_router.py +++ b/config/api_router.py @@ -48,7 +48,9 @@ assignments_router = nested_cls(courses_router, "assignments", lookup="assignment") assignments_router.register("submissions", SubmissionViewSet) -assignments_router.register("activity-progress", ActivityProgressViewSet, basename="activity-progress") +assignments_router.register( + "activity-progress", ActivityProgressViewSet, basename="activity-progress" +) attachments_router = nested_cls(assignments_router, "submissions", lookup="submission") attachments_router.register("attachments", AttachmentViewSet) diff --git a/config/settings/railway.py b/config/settings/railway.py index 510c31e..c9dbe51 100644 --- a/config/settings/railway.py +++ b/config/settings/railway.py @@ -16,8 +16,7 @@ # Allow Railway's domain and custom domains ALLOWED_HOSTS = env.list( - "DJANGO_ALLOWED_HOSTS", - default=["localhost", ".railway.app", ".up.railway.app"] + "DJANGO_ALLOWED_HOSTS", default=["localhost", ".railway.app", ".up.railway.app"] ) # DATABASES @@ -62,7 +61,9 @@ # STATIC FILES (whitenoise) # ------------------------------------------------------------------------------ INSTALLED_APPS = ["whitenoise.runserver_nostatic"] + INSTALLED_APPS # noqa F405 -MIDDLEWARE.insert(1, "whitenoise.middleware.WhiteNoiseMiddleware") # After SecurityMiddleware +MIDDLEWARE.insert( + 1, "whitenoise.middleware.WhiteNoiseMiddleware" +) # After SecurityMiddleware STATICFILES_STORAGE = "whitenoise.storage.CompressedManifestStaticFilesStorage" STATIC_URL = "/static/" @@ -79,12 +80,10 @@ # ------------------------------------------------------------------------------ # Use console backend for development/testing (emails print to console) EMAIL_BACKEND = env( - "DJANGO_EMAIL_BACKEND", - default="django.core.mail.backends.console.EmailBackend" + "DJANGO_EMAIL_BACKEND", default="django.core.mail.backends.console.EmailBackend" ) DEFAULT_FROM_EMAIL = env( - "DJANGO_DEFAULT_FROM_EMAIL", - default="MusicCPR " + "DJANGO_DEFAULT_FROM_EMAIL", default="MusicCPR " ) # ADMIN @@ -97,9 +96,7 @@ "version": 1, "disable_existing_loggers": False, "formatters": { - "verbose": { - "format": "%(levelname)s %(asctime)s %(name)s %(message)s" - } + "verbose": {"format": "%(levelname)s %(asctime)s %(name)s %(message)s"} }, "handlers": { "console": { @@ -133,17 +130,13 @@ r"^https://.*\.railway\.app$", r"^http://localhost:\d+$", r"^http://127\.0\.0\.1:\d+$", - ] + ], ) # Also allow specific origins if set -CORS_ALLOWED_ORIGINS = env.list( - "CORS_ALLOWED_ORIGINS", - default=[] -) +CORS_ALLOWED_ORIGINS = env.list("CORS_ALLOWED_ORIGINS", default=[]) # CSRF trusted origins (needed for admin) CSRF_TRUSTED_ORIGINS = env.list( - "CSRF_TRUSTED_ORIGINS", - default=["https://*.railway.app", "https://*.vercel.app"] + "CSRF_TRUSTED_ORIGINS", default=["https://*.railway.app", "https://*.vercel.app"] ) diff --git a/config/urls.py b/config/urls.py index 7791b43..45951a6 100644 --- a/config/urls.py +++ b/config/urls.py @@ -15,6 +15,7 @@ def debug_media(request): """Diagnostic endpoint to check media files.""" import subprocess + result = { "MEDIA_ROOT": str(settings.MEDIA_ROOT), "MEDIA_ROOT_exists": os.path.exists(settings.MEDIA_ROOT), @@ -29,7 +30,9 @@ def debug_media(request): for f in filenames[:20]: # Limit to first 20 files.append(os.path.join(root, f).replace(settings.MEDIA_ROOT, "")) result["files"] = files - result["file_count"] = sum(len(f) for _, _, f in os.walk(settings.MEDIA_ROOT)) + result["file_count"] = sum( + len(f) for _, _, f in os.walk(settings.MEDIA_ROOT) + ) except Exception as e: result["error"] = str(e) else: @@ -39,10 +42,13 @@ def debug_media(request): teleband_media = os.path.join(os.getcwd(), "teleband", "media") result["teleband_media_exists"] = os.path.exists(teleband_media) if os.path.exists(teleband_media): - result["teleband_media_count"] = sum(len(f) for _, _, f in os.walk(teleband_media)) + result["teleband_media_count"] = sum( + len(f) for _, _, f in os.walk(teleband_media) + ) return JsonResponse(result) + urlpatterns = [ path("", TemplateView.as_view(template_name="pages/home.html"), name="home"), path("debug-media/", debug_media, name="debug-media"), @@ -68,9 +74,11 @@ def debug_media(request): # Serve media files - in production with S3 this is handled by S3, # but for Railway/local deployments we serve from filesystem # Note: static() only works with DEBUG=True, so we use serve() directly for non-S3 deployments -if not hasattr(settings, 'DEFAULT_FILE_STORAGE') or 'S3' not in getattr(settings, 'DEFAULT_FILE_STORAGE', ''): +if not hasattr(settings, "DEFAULT_FILE_STORAGE") or "S3" not in getattr( + settings, "DEFAULT_FILE_STORAGE", "" +): urlpatterns += [ - re_path(r'^media/(?P.*)$', serve, {'document_root': settings.MEDIA_ROOT}), + re_path(r"^media/(?P.*)$", serve, {"document_root": settings.MEDIA_ROOT}), ] if settings.DEBUG: diff --git a/docs/remodel_campaign.md b/docs/remodel_campaign.md new file mode 100644 index 0000000..34bf4a6 --- /dev/null +++ b/docs/remodel_campaign.md @@ -0,0 +1,260 @@ +# Backend Remodel Campaign + +> Directive file guiding the CPR-Music-Backend query/model remodel. +> Derived from a 6-dimension fan-out audit (65 findings, 45 confirmed remediable: +> 37 Phase-1 safe, 7 Phase-2 structural) and the advisor's plan in +> [`remodel_assignments.md`](./remodel_assignments.md). + +## Decisions locked in + +- **Sequencing:** phased. Ship Phase 1 (safe, no-migration query wins) first; the + advisor's structural `CourseAssignment` remodel is Phase 2, gated behind Phase 1. +- **API contract:** the assignment/submission JSON response shapes stay **identical**. + Per-student fields get resolved server-side so the frontend + (`~/GithubOrgs/espadonne/CPR-Music`) needs no changes. +- **Working style:** tests first-class (query-count + response-equivalence), one + endpoint/fix per commit, CI green before merge. + +## The two cost classes + +1. **Write-explosion** — one teacher "assign" click fans out across the roster. + `assign_one_piece_activity` runs `update_or_create` per student per activity + (`courses/helper.py:23-42`): ~`2·A·S` queries / `A·S` rows per POST, `P·A·S` for a + curriculum, none wrapped in `transaction.atomic`. (A = activities, S = students, + P = piece plans.) +2. **Read N+1** — teacher list/grading endpoints serialize deep nested trees over + unoptimized querysets. Worst offender: `GroupSerializer.get_members` re-queries + membership per assignment → O(M²) per group. + +**Biggest single win:** the `AssignmentViewSet.list` teacher read path — three +compounding N+1s collapse to a bounded handful with pure queryset/memoization +changes, no migration, no API change. + +--- + +## PHASE 1 — Safe quick wins (no migration, no API change) + +Each item ships as its own commit with an `assertNumQueries` test proving the count +is **constant** w.r.t. roster/group size (parametrize S = 1, 5, 30). Capture a +response-equivalence snapshot of every touched endpoint *before* editing. + +> **Progress (branch `backend-remodel-phase1`, 11 commits):** all of 1a landed EXCEPT +> #13 (held — behavior change). Done: #1–#4, #6–#12, #14, plus 1c #22/#25 and 1d indexes +> (#26 + attachment), each with query-count tests (`teleband/*/tests/test_query_counts.py`). +> Verified teeth: teacher list 484→const, grouped 1009→const (O(M²)), grading `recent` +> 501→const, CSV export 9000+→2. Remaining 1a: #13, #21, #23, #24. Then 1b (write side). +> Note: repo has 16 PRE-EXISTING black-dirty files on main (`black . --check` already +> fails repo-wide under black 24.4.2) — separate cleanup, not this PR. + +### 1a — Pure queryset / serializer (zero schema risk) + +- [x] **#1 `GroupSerializer.get_members`** (`assignments/api/serializers.py:42-54`) — + O(M²)/group. Memoize members per `group.id` in serializer `context` (compute once + per request); replace `submissions.count()` with prefetched `bool(len(...))` / + annotated `has_submission`. Drop the inner re-query. +- [x] **#2 `AssignmentViewSet.get_queryset`** (`assignments/api/views.py:82-109`) — + factor a shared base queryset; add to **both** student & teacher branches: + `select_related("part","part__piece","part__piece__composer")` + + `prefetch_related("submissions","submissions__attachments", + "part__transpositions__transposition","part__instrument_samples")`. + (Does **not** fix #1 — that's separate.) +- [x] **#3 `PartSerializer` tree** (`musics/api/serializers.py:60-74`) — resolved by + #2's prefetch (`part__piece__composer`, `part__transpositions__transposition`, + `part__instrument_samples`). +- [x] **#4 `TeacherSubmissionViewSet.recent`** (`submissions/api/views.py:80-102`) — + add `select_related("grade","self_grade", + "assignment__activity__activity_type__category","assignment__instrument", + "assignment__part__piece__composer","assignment__enrollment__user", + "assignment__enrollment__course__owner")` + + `prefetch_related("attachments","assignment__submissions__attachments", + "assignment__part__transpositions__transposition")`. Long-term: a slim serializer + that doesn't re-embed `assignment.submissions[]`. +- [x] **#6 `EnrollmentViewSet.list`** (`courses/api/views.py:103-113`) — + `select_related("course__owner","instrument__transposition","role","user")` + + `prefetch_related("course__owner__groups","user__groups")`. +- [x] **#7 `CourseViewSet.roster` GET** (`courses/api/views.py:249-253`) — + `select_related("user","instrument__transposition","role")` + + `prefetch_related("user__groups")`. +- [x] **#8 `PiecePlanViewSet`** (`assignments/api/views.py:163-173`) — + `select_related("piece__composer")` + + `prefetch_related("activities__activity_type__category","activities__part_type")`. +- [x] **#9 `ActivityViewSet`** (`assignments/api/views.py:36-50`) — + `select_related("activity_type","activity_type__category","part_type")`. +- [x] **#10 `GradeViewSet`** (`submissions/api/views.py:109-114`) — + `prefetch_related("student_submission","own_submission")`. +- [x] **#11 `dashboards/views.py:55` (`csv_view`)** — reverse relations are misused + with `select_related` → full-table N+1; `submissions.all()` evaluated twice. + Use `prefetch_related("submissions","submissions__attachments")`; guard nullable + `assn.piece_plan` before `.id`. +- [x] **#12 `dashboards/views.py:19` (`AssignmentListView`)** — split forward FKs into + `select_related`, reverse/m2m into `prefetch_related`; add `paginate_by`. + (Superuser-only, low blast radius.) +- [x] **#13 `UserViewSet.get_queryset`** (`users/api/views.py:64`) — list-comprehension + N+1 over `Enrollment…course`, **and** hardcodes `username="admin"` (scoping bug). + Replace with one `.filter(enrollment__course__enrollment__user=request.user, + enrollment__course__enrollment__role__name="Teacher").distinct()` — fixes N+1 **and** + the bug. +- [x] **#14 permission/view duplicate point lookups** (`utils/permissions.py:21`, + `assignments/api/views.py:70,83-84`) — resolve course/enrollment once per request + (cache on `request`), reuse role; `select_related("role","course")`. + +### 1b — Write batching + transactions (correctness first, then batch) + +> **CORRECTION:** `ATOMIC_REQUESTS = True` in base/production/railway settings, so every +> request is already wrapped in a transaction — no explicit `transaction.atomic` wrapper +> needed (it'd just add redundant savepoints). 1b's real win is the `bulk_create`/`.update()` +> query reduction. `bulk_create` verified safe: no custom `Assignment.save()`, no signals. +> +> **LANDMINE FOUND:** the live helper `assign_one_piece_activity` is invoked by the data +> migration `assignments/0033_auto_20240312_2321.add_demos`. Eagerly `select_related("user")` +> there selects `users.external_id` before that column's migration runs → breaks a fresh +> `migrate`. Helper now select_relateds `instrument` only. General rule: helpers called from +> migrations must not touch columns added by later migrations. + +- [x] **#15 `assign_one_piece_activity`** (`courses/helper.py:23-42`, view + `courses/api/views.py:343`) — prefetch existing `(activity,enrollment,piece)` keys + in one query, `bulk_create(missing, ignore_conflicts=True)` (~A INSERTs); + `select_related("user","instrument")` on the Enrollment loop; wrap view in + `transaction.atomic`. +- [x] **#16 `assign_curriculum`** (`courses/helper.py:117-126`, view `:387`) — + `bulk_create` across all plans (~P·A INSERTs); wrap in `transaction.atomic`. +- [x] **#17 `change_piece_instrument`** (`courses/api/views.py:461-464`) — replace the + `save()` loop with + `Assignment.objects.filter(piece=piece, enrollment__course=course) + .update(instrument=instrument)` → 1 UPDATE. +- [x] **#18 `assign_telephone_fixed`** (`courses/helper.py:67-114`) — hoist + `Part.for_activity` into an activity-keyed dict (A lookups); build objects in + memory, `bulk_create` after creating groups. +- [x] **#19 roster POST** (`courses/api/views.py:174-229`) — `filter(username__in=…)` + for existence; `Enrollment.objects.bulk_create(ignore_conflicts=True)` after one + `filter(user__in=…)` prefetch; resolve collisions in memory; wrap in transaction. + (`create_user` can't bulk — password hashing.) +- [x] **#20 `update_or_create` lookup wider than constraint** (`courses/helper.py:29-37` + vs `assignments/models.py:102-106`) — make lookup keys exactly + `(activity, enrollment, piece)`, move the rest into `defaults=`; stop swallowing + `IntegrityError` silently. + +### 1c — Cheap cleanups + +- [x] **#21** `courses/helper.py:68,100` — hoist + `activities = list(piece_plan.activities.all())` before the group loop. +- [x] **#22** `musics/models.py:65-76` (`Part.for_activity`) — cache the + `PartType.objects.get(name="Melody")` lookup; drop the redundant `.exists()` before + `.get()`. **Prerequisite for Phase 2** (the plan moves this call to read time). +- [x] **#23** `assignments/api/views.py:123-135` — annotate the queryset with a + correlated `Subquery` on `PlannedActivity.order` instead of rebuilding a Python dict + per request. +- [x] **#24** `assignments/api/views.py:126` — add explicit `.order_by()` (the + `Meta.ordering` spanning `piece_plan__name` forces an unused sort). +- [x] **#25** `assignments/models.py:57-73` (`PiecePlan.assign`) — delete (confirmed + zero callers; a trap if reused). + +### 1d — Additive index migrations (non-breaking; see Open Q #6) + +- [x] **#26** `submissions/models.py:38` — composite index `(assignment, submitted DESC)` + for latest-per-assignment (`submissions/api/views.py:80-94`). +- [ ] `submissions/models.py:53-56` — index for `SubmissionAttachment.Meta.ordering` + `(submission, submitted)`, or drop the implicit ordering. + +--- + +## PHASE 2 — Structural remodel (needs migration) + +Implements the advisor's `CourseAssignment` plan: one row per `(course, activity, piece)` +instead of per-enrollment; move `instrument`/`part` to `Submission`; resolve a student's +assignments dynamically at read time (late joiners handled for free); **preserve the +response shape**. + +### What the advisor's plan covers +- Write fan-out `A·S → A` (root of #15/#16) — directly solved. +- `instrument` denormalized per-row → moved to `Submission`. +- Nullable `piece` in the unique constraint → `CourseAssignment` uses non-null `piece`. +- `Submission` FK repoint + backfill. + +### What the plan MISSES (our findings to fold in) +1. **`Part.for_activity` read-time regression** (`musics/models.py:65-76`) — plan §4 + relocates this 2-3-query, magic-string lookup to read time *per (student × activity)*, + converting a write cost into a per-request N+1. Land #22 first; precompute a + per-request `(piece, activity)→part` map before going live with dynamic resolution. +2. **`ActivityProgress` OneToOne CASCADE** (`submissions/models.py:65-67`) — per-student + research data (`audio_edit_history`, `question_responses`, `participant_email`). + Cannot map onto a course-level row; must become FK to `CourseAssignment` + `enrollment`, + or a wrong CASCADE deletes it. **Must be in the migration plan.** +3. **`Activity.activity_type_name` / `category` denormalized columns** + (`assignments/models.py:35-40`) — shadow `ActivityType.name`/`category.name`, read live + (`serializers.py:49,95-100`), and have **already drifted** (migration 0037). Drop the + columns, repoint serializer `source` to the FK with `select_related` — JSON field names + unchanged, so **not** an API break. +4. **`PlannedActivity.order` not denormalized** (`models.py:117-126`) — denormalize onto + `CourseAssignment` to kill the read-time reassembly join (#23). Decide during the remodel. +5. **Instrument source of truth** — `e.instrument if e.instrument else e.user.instrument` + (`courses/helper.py:32,107`). Pin which upstream wins before backfill (Open Q #1). +6. **Non-unique `Course.slug` / `Piece.slug`** (`courses/models.py:12`, `musics/models.py:31`) + — looked up via `.get(slug=…)` → `MultipleObjectsReturned` 500 if a dup lands; racy + TOCTOU in `utils/fields.py`. Dedupe existing, add `unique=True`. Slug values unchanged → + no API break. Orthogonal but cheap to fold in. +7. **`SubmissionAttachment` ordering without index** — covered by 1d. +8. **Tighten `CourseAssignment` uniqueness explicitly** — `piece NOT NULL`; **audit + pre-2023 `Assignment` rows with `piece IS NULL`** (nullable since migration 0026, never + backfilled) — they violate tightened uniqueness and abort the migration. + +### Migration order (PROTECT-safe) +`CourseAssignment` model → data migration (dedupe `Assignment`; repoint `Submission` + +`ActivityProgress`; backfill `instrument`/`part`) → serializer/view rewrite (shape-preserving) +→ drop `Activity` denorm columns + add slug uniqueness + attachment index → remove `Assignment`. + +--- + +## API-contract constraints (must preserve) + +- **`TeacherSubmission` recent** (`teacher_serializers.py:7-27`): `id`, `content`, + `attachments[]{file, submitted}`, `assignment{part.piece.name, + activity.activity_type.category, enrollment.user.name}`, `grade{rhythm, tone, expression}`. +- **Student `SubmissionSerializer`**: `submissions[]{id, submitted, content, + attachments[]{file, submitted}}`. +- **Assignment list grouped-by-`piece_slug` dict** + the flat denormalized keys on + `AssignmentViewSetSerializer`. +- `part.transpositions` score-selection chain, `RosterSerializer`, `CourseSerializer` + (`can_edit_instruments`,`id`,`slug`,`name`), Grade write contract. + +**Possibly droppable (needs a frontend grep first — do not drop blind):** +`UserSerializer.groups/external_id/grade/url` (prefer prefetch over removal), +`self_grade`, `group`/`GroupSerializer`, the heavy assign-endpoint response body +(frontend appears to use only HTTP status — unverified in this checkout). + +--- + +## Testing strategy + +- **Query-count assertions** (`assertNumQueries`) on every 1a endpoint — + assert constant w.r.t. S ∈ {1, 5, 30}: `AssignmentViewSet.list` (teacher+student), + `TeacherSubmissionViewSet.recent`, `EnrollmentViewSet.list`, `roster`, `PiecePlanViewSet`, + `ActivityViewSet`, `csv_view`. +- **Write-count assertions** on assign / assign_curriculum / change_piece_instrument / + roster — assert O(A)/O(1), not O(A·S). +- **Atomicity** — force a mid-loop failure on assign; assert no partial rows. +- **Response-equivalence snapshots** — capture current JSON for every load-bearing + endpoint before changes; assert key-set equality after each commit and across the + Phase 2 serializer rewrite. +- **Re-assign idempotency** (#20) — assign same piece twice with changed deadline; assert + every student updates, no swallowed `IntegrityError`. +- **Phase 2 migration tests** — seed legacy `piece IS NULL` + duplicate-slug rows; + assert `ActivityProgress` research data survives; assert late-joiner resolves all + course assignments. + +--- + +## Open questions (Phase 2 blockers in **bold**) + +1. **Instrument source of truth:** when `Enrollment.instrument` ≠ `User.instrument`, which + wins as the Submission default? (Current fallback prefers `Enrollment.instrument`.) +2. **`telephone_fixed` under `CourseAssignment`:** keep a lightweight per-student/group + model for grouped activities, or redesign grouping? (Advisor's own open question.) +3. **Read-time `Submission` creation:** on first access vs. lazily on submit? Affects + whether "assigned but not started" is queryable + submission-count badge semantics. +4. Confirm against the frontend repo that the assign-endpoint response body is unused + before slimming it. +5. Drop vs. keep `self_grade`/`group`/`external_id`/`grade` — frontend grep first, or keep + and rely on prefetch? +6. Index-as-migration policy: treat additive/non-breaking indexes (#26) as Phase 1, or + batch into the Phase 2 migration? diff --git a/docs/remodel_phase2_design.md b/docs/remodel_phase2_design.md new file mode 100644 index 0000000..1adf4e8 --- /dev/null +++ b/docs/remodel_phase2_design.md @@ -0,0 +1,225 @@ +# Phase 2 Design Scoping — Course-Level Assignments + +> Builds on the advisor's plan ([`remodel_assignments.md`](./remodel_assignments.md)) and +> [`remodel_campaign.md`](./remodel_campaign.md). Phase 1 (1a/1b/1c) is shipped on +> `backend-remodel-phase1` (PR #56). **Decision: Option B (fully dynamic).** Steps 1–6 of the +> sequence below are built and committed; steps 7–8 (the contract-sensitive read-path flip and +> dropping `Assignment`) remain. Option A is retained below as the rejected alternative. + +## Decisions locked (2026-06-27) + +- **Design: Option B (fully dynamic).** `CourseAssignment` is the only assignment row; + the list `id` becomes the `CourseAssignment` id; nested submission/activity-progress + routes scope by the requesting student's enrollment; `instrument`/`part` move to + `Submission`; `ActivityProgress` re-keys to `(course_assignment, enrollment)`. API + response shape preserved so the frontend is untouched. +- **Session scope: prerequisite only.** Land the migration-0033 safety fix (done — see + sequence step 1), then pause for review of PR #56 before building the remodel. + +**Status of prerequisite (step 1): ✅ DONE** — `assignments/0033` no longer calls live +helper/model code (`add_demos` is a documented no-op). Fresh `migrate` is now robust to +the upcoming helper/model changes. + +### Blocker decisions (2026-06-27) + +1. **Instrument source of truth:** Enrollment first, then User — preserve the current + `e.instrument if e.instrument else e.user.instrument` fallback. +2. **`telephone_fixed`:** keep a lightweight per-student group table. `CourseAssignment` + covers normal plans (every enrolled student is implicitly assigned every non-grouped + row); a `GroupAssignment(group, enrollment, course_assignment)` table records which + student gets which activity for grouped plans and restricts visibility to members. +3. **Materialization:** resolve `part`/`instrument` at read time from the enrollment; persist + nothing per-student until the student submits (the `Submission` carries `instrument`/`part`). + `ActivityProgress` is `get_or_create((course_assignment, enrollment))` on first access. + +### Target models + +``` +CourseAssignment: course, activity, piece, piece_plan?, deadline?, created_at + unique(course, activity, piece) +GroupAssignment: group(AssignmentGroup), enrollment, course_assignment + unique(enrollment, course_assignment) # telephone_fixed only +Submission(mod): course_assignment, enrollment, instrument, part, + existing fields +ActivityProgress(mod): course_assignment, enrollment (was OneToOne(Assignment)) + unique(course_assignment, enrollment) +``` + +Student's assignments = `CourseAssignment` for their course where the plan is not +`telephone_fixed`, UNION the `CourseAssignment`s linked to them via `GroupAssignment`. + +## Reframing: what Phase 1b already fixed + +The advisor's plan targets the per-student assignment row explosion. **Phase 1b already +removed the per-operation query cost**: `assign_*` now `bulk_create`s, so assigning a piece +is O(A) queries regardless of roster size (was 2·A·S). So Phase 2 is no longer about +*query expense per assign*. Its remaining, real motivations are: + +1. **Row count / storage** — still A·S rows in the DB (now written efficiently, but stored). +2. **Late joiners** — a student who enrolls after a piece is assigned gets no assignments, + and there's no UI/endpoint to assign to them. (Correctness gap, not perf.) +3. **Model cleanliness** — `instrument`/`part` are per-student data denormalized onto a row + that's conceptually "what the course is assigned"; `piece` is redundant with `part.piece`. + +This reframing matters for prioritization: Phase 2 is a **correctness + modeling** project +now, not a perf emergency. It can be sequenced deliberately. + +## The hinge: the frontend uses a per-student `assignmentId` + +`~/GithubOrgs/espadonne/CPR-Music` (`actions.js`, `api.js`) uses an assignment `id` in: + +- `GET/POST /api/courses/{slug}/assignments/{id}/submissions/` +- `POST .../submissions/{submissionId}/attachments/` +- `PATCH /api/courses/{slug}/assignments/{id}/` (instrument override) +- `GET/POST /api/courses/{slug}/assignments/{id}/activity-progress/{,log_event,submit_step,save_response,save_audio_state}` + +The `id` comes from each object in the grouped list response. **Any design must keep a stable +id the student can use for these nested routes**, with the decision to preserve the API shape +(frontend untouched). + +## Two viable designs + +### Option A — Lazy per-student materialization (lower risk) +- `CourseAssignment` becomes the template: one row per `(course, activity, piece)`, created by + `assign_*` (A rows, not A·S). +- A per-student `Assignment` row is created **on demand** the first time a student accesses + their assignments (or submits). Its `id` is the same shape as today → **frontend unchanged, + every nested route keeps working unmodified**. +- Late joiners: their `Assignment` rows materialize on first access → solved. +- Row count: only materializes for students who actually engage; un-accessed assignments cost 0. +- `instrument`/`part` can stay on `Assignment` (resolved at materialization), or move to + `Submission` later. Smallest blast radius. +- **Trade-off:** keeps the per-student `Assignment` table (just sparse/lazy), so it's a partial + realization of the advisor's model. But it's incrementally shippable and contract-safe. + +### Option B — Fully dynamic (advisor's model) +- `CourseAssignment` is the only "assignment" row. The list endpoint returns + CourseAssignment-derived objects with `id = course_assignment.id`, resolving `part`/`instrument` + per student at read time. +- Nested routes reinterpret `{id}` as a `CourseAssignment` id and **scope by the requesting + student's enrollment** (`request.user`): submissions/activity-progress are keyed by + `(course_assignment, enrollment)`. The frontend is unchanged because each student only ever + uses ids from its own list response, and the backend scopes by the authenticated user. +- `instrument`/`part` move to `Submission`; `ActivityProgress` re-keys to + `(course_assignment, enrollment)`. +- **Trade-off:** matches the advisor's clean model and fully kills per-student rows, but changes + more semantics (teacher list shape, activity-progress identity, submission resolution) and has + the larger migration. Higher risk. + +**Recommendation:** **Option A first** (contract-safe, incrementally shippable, solves late +joiners and row count), with Option B as a later step if the fully-dynamic model is desired. +This mirrors the phased discipline that worked for Phase 1. + +## Migration landmines (must be in the plan) + +1. **Migration 0033 calls live helper code.** `assignments/0033_auto_20240312.add_demos` invokes + the live `assign_piece_plan`. If Phase 2 changes that helper's behavior/signature, a fresh + `migrate` runs the NEW logic against the OLD schema → breaks (we already hit a variant of this + in 1b). **Neutralize 0033 first**: freeze its behavior (inline the historical logic or guard + the helper), so changing the helper can't rewrite history. Prerequisite for any Phase 2 helper + change. +2. **`ActivityProgress` is `OneToOne(Assignment)` with per-student research data** + (`audio_edit_history`, `question_responses`, `participant_email`). It cannot map onto a + course-level row. Must become `(course_assignment, enrollment)` (Option B) or stay attached to + the lazily-materialized `Assignment` (Option A). A wrong `on_delete` here destroys research data. +3. **`Submission.assignment` is `PROTECT`.** Can't drop old `Assignment` rows while Submissions + reference them. Add new FK → backfill → swap → drop, in that order. +4. **Legacy `Assignment.piece IS NULL` rows** (piece nullable since migration 0026, never + backfilled). They violate any tightened `(course, activity, piece)` uniqueness and abort the + data migration. Audit + backfill/dedupe first. +5. **`Part.for_activity` read-time regression (Option B).** Moving part resolution to read time + makes it per-(student×activity). Precompute a per-request `(piece, activity)→part` map; the 1c + `.exists()` removal helped but isn't enough at read scale. +6. **Non-unique `Course.slug`/`Piece.slug`** — `.get(slug=)` can 500. Dedupe + add `unique=True` + (slug values unchanged → no API break). Cheap; fold in. +7. **`Activity.activity_type_name`/`category` denorm columns** already drifted (migration 0037). + Drop and repoint serializer `source` to the FK (`select_related`) — JSON field names unchanged. + +## Proposed sequence (Option A) + +1. ✅ **Neutralize migration 0033** (done) — `add_demos` is a no-op; fresh `migrate` green. +2. ✅ **Add `CourseAssignment` + `GroupAssignment` models** (done) — additive, unique constraints, + factories + constraint tests. +3. ✅ **Dual-write from `assign_*`** (done) — `assign_one_piece_activity` / + `assign_telephone_fixed` now also create `CourseAssignment` (and `GroupAssignment` per + telephone member). Per-student `Assignment` rows still written; old read path unaffected. +4. ✅ **Backfill data migration** (done) — create `CourseAssignment` (and `GroupAssignment` for + telephone groups) from existing `Assignment` rows, collapsing by `(course, activity, piece)`. + Handle legacy `piece IS NULL` rows first. +5. ✅ **Add Submission fields** (done) — `course_assignment`, `enrollment`, `instrument`, `part` + (nullable), dual-populate on create, backfill from existing `Submission.assignment`. +6. ✅ **Re-key `ActivityProgress`** (done) — add `course_assignment` + `enrollment` (unique together), + backfill from `assignment`, keep `get_or_create` on first access. +7. 🔶 **Flip the read path** — IN PROGRESS. **Student path DONE** on `backend-remodel-phase1`: + - `AssignmentViewSet.list` (student) resolves from `CourseAssignment` (id = `course_assignment.id`), + precomputing per-CA part/submissions/group maps so it stays O(1) in assignment count; fixes + late joiners and scopes telephone groups by enrollment. + - `AssignmentViewSet.retrieve` (student) → `CourseAssignmentRetrieveSerializer` (legacy + `AssignmentSerializer` shape). + - Nested `submissions` + `activity-progress` routes reinterpret `{id}` as a `CourseAssignment` + id, scope by the requesting student's enrollment, key writes by `(course_assignment, enrollment)`, + 404 on a foreign id. Late joiners can now submit/track progress (the `assignment` FK is now + nullable — migration 0018 — and populated only when an Assignment row exists, so teacher views + keep reading it). + - Foundations: response-equivalence serializers + tests (`CourseAssignmentReadSerializer`, + `CourseAssignmentRetrieveSerializer`) pin field-for-field parity except `id`; query-count test + updated to dual-write CAs; factory fixes (unique `UserFactory.username`, date-typed + `CourseFactory` dates). + - **Teacher list DONE:** flipped to one row per `CourseAssignment` (A rows, not A·S). Verified the + frontend teacher view (`components/teacher/course.js` → `getAssignedPieces`) only derives the + distinct `(piece, activity)` set per piece and the redux consumers of this endpoint are all + student-facing — so the cardinality collapse is contract-safe; per-student fields come back + null/empty (read serializer handles `enrollment=None`). Teacher `retrieve` still reads per-student + `Assignment` (single-object; not a cardinality issue) and `TeacherSubmissionViewSet.recent` still + reads the populated `assignment` FK — both fine until step 8. + + **Step 7 COMPLETE** (student + teacher list/retrieve, submissions, activity-progress). +8. ✅ **Contract & drop** — COMPLETE. `Assignment` is gone — neither read, written, nor a table: + - Repointed reads: `GroupSerializer.get_members` → `GroupAssignment`; `TeacherSubmissionViewSet.recent` + + serializer → the submission's own course_assignment/enrollment/instrument/part (frontend reads + only `assignment.enrollment.user.name` there); `ActivityViewSet` distinct-activity list → `CourseAssignment`. + - Per-piece instrument override moved to **`CourseAssignment.instrument`** (mig 0040, nullable); + `change_piece_instrument` sets it, `resolve_instrument` prefers it. (Restores the override the + step-7 read flip had stopped honoring.) + - **Stopped writing Assignment:** `assign_one_piece_activity`/`assign_telephone_fixed` create only + `CourseAssignment` (+ `GroupAssignment`); assign endpoints return a count (frontend ignores the body). + - **DONE (destructive):** added `unique(course_assignment, enrollment)` to `ActivityProgress` + (mig 0019); removed `resolve_legacy_assignment` + the `assignment=` write in + `SubmissionViewSet.perform_create`; dropped the dead `AssignmentViewSet` teacher + retrieve/update/notation actions and the dead serializers (`AssignmentSerializer`/ + `AssignmentViewSetSerializer`/`AssignmentInstrument`/`NotationAssignment`); dropped + `Submission.assignment` + `ActivityProgress.assignment` FKs (submissions mig 0020) and the + `Assignment` model itself (assignments migs 0041 RemoveConstraint+RemoveField, 0042 DeleteModel). + `ActivityProgressSerializer` now exposes `course_assignment`/`enrollment`. Migration 0041 drops + the `unique_assignment` constraint *before* the field removals so SQLite's table-remake doesn't + reference removed fields. Assignment rows were safe to drop (fully backfilled into + CourseAssignment); ActivityProgress/Submission rows kept all data (only the redundant FK column went). + - Test fixtures migrated off `AssignmentFactory` (removed): list/teacher/student/query-count tests + now build only `CourseAssignment` rows; `SubmissionFactory` keys by `course_assignment`/`enrollment`. + Obsolete transitional tests deleted (`test_backfill_course_assignments`, the two migration-backfill + tests in submissions). Full assignments/submissions/courses/dashboards suite green; only the two + pre-existing user-test failures remain. CI `format-check` green on PR #56. + +### Frontend contract surface (verified against `~/GithubOrgs/espadonne/CPR-Music`) + +Per-assignment `id` from the list is consumed by exactly: `GET /assignments/{id}/` (retrieve), +`GET|POST /assignments/{id}/submissions/`, `POST .../submissions/{sid}/attachments/` (keyed by +submission pk, unaffected), and `*/activity-progress/{,log_event,submit_step,save_response,save_audio_state}`. +**There is no per-assignment `PATCH`** — instrument changes go through course-level +`PATCH /courses/{slug}/change_piece_instrument/` (by `piece_id`), which now sets +`CourseAssignment.instrument`. So the student contract surface flipped in step 7 is complete. + +Each step is independently shippable with query-count + response-equivalence tests, same as Phase 1. +Steps 7–8 are the contract-sensitive half — review the dual-write foundation (PR #56) first. + +## Open questions (decisions needed before building) + +1. **Option A vs B** — start with lazy materialization (recommended) or go straight to the fully + dynamic model? +2. **Instrument source of truth** — when `Enrollment.instrument` ≠ `User.instrument`, which wins? + (Current fallback prefers `Enrollment.instrument`.) +3. **`telephone_fixed`** — inherently per-student group assignment; keep a lightweight per-student + construct, or redesign grouping? (Advisor's own open question.) +4. **Lazy materialization trigger (Option A)** — materialize on list access, or only on first + submit? Affects whether "assigned but not started" is queryable. +5. **Scope of this effort** — is Phase 2 in scope for the current sprint, or is shipping Phase 1 + (PR #56) + the migration-0033 safety fix the right stopping point for now? diff --git a/teleband/assignments/admin.py b/teleband/assignments/admin.py index 7e93122..d43ee46 100644 --- a/teleband/assignments/admin.py +++ b/teleband/assignments/admin.py @@ -5,7 +5,6 @@ ActivityCategory, ActivityType, Activity, - Assignment, Curriculum, CurriculumEntry, PiecePlan, @@ -33,29 +32,6 @@ class ActivityAdmin(VersionAdmin): list_filter = ("activity_type", "part_type") -@admin.register(Assignment) -class AssignmentAdmin(VersionAdmin): - list_display = ( - "id", - "activity", - "enrollment", - "part", - # "deadline", - # "instrument", - # "created_at", - ) - list_filter = ( - "activity", - "piece", - # "deadline", - # "instrument", - # "created_at", - ) - search_fields = ("activity__activity_type__name", "enrollment__user__username") - date_hierarchy = "created_at" - save_as = True - - class PiecePlanActivityInline(admin.TabularInline): model = PlannedActivity extra = 0 @@ -79,12 +55,6 @@ class PiecePlanAdmin(VersionAdmin): save_as = True -class AssignmentInline(admin.TabularInline): - model = Assignment - extra = 0 - ordering = ("-id",) - - @admin.register(AssignmentGroup) class AssignmentGroupAdmin(VersionAdmin): list_display = ( @@ -92,7 +62,6 @@ class AssignmentGroupAdmin(VersionAdmin): "type", ) list_filter = ("type",) - inlines = (AssignmentInline,) # @admin.register(PlannedActivity) diff --git a/teleband/assignments/api/serializers.py b/teleband/assignments/api/serializers.py index 22c73c2..a15a750 100644 --- a/teleband/assignments/api/serializers.py +++ b/teleband/assignments/api/serializers.py @@ -1,12 +1,15 @@ from rest_framework import serializers from teleband.assignments.models import ( - Assignment, Activity, ActivityType, AssignmentGroup, + CourseAssignment, + GroupAssignment, PiecePlan, ) +from teleband.musics.models import Part +from teleband.submissions.models import Submission from teleband.courses.api.serializers import EnrollmentSerializer from teleband.instruments.api.serializers import InstrumentSerializer from teleband.submissions.api.serializers import SubmissionSerializer @@ -40,126 +43,46 @@ class GroupSerializer(serializers.ModelSerializer): members = serializers.SerializerMethodField(method_name="get_members") def get_members(self, obj): - assignments = Assignment.objects.filter(group=obj) - assignment_enrollments = [(a, a.enrollment) for a in assignments] - member_list = [ - { - "enrollment_id": ae[1].id, - "enrollment_username": ae[1].user.username, - "activity_type_name": ae[0].activity.activity_type_name, - "assignment_submitted": bool(ae[0].submissions.count()), - } - for ae in assignment_enrollments - ] - return member_list + # Phase 2: group membership comes from GroupAssignment (was per-student + # Assignment.group). Memoize per group.id in the shared serializer context + # so this runs once per distinct group, not once per member row. One query + # for the memberships and one for which (course_assignment, enrollment) + # pairs have a submission keep this off the per-row N+1 path. + cache = self.context.setdefault("_group_members", {}) + if obj.id not in cache: + memberships = list( + GroupAssignment.objects.filter(group=obj).select_related( + "enrollment__user", "course_assignment__activity" + ) + ) + submitted = set( + Submission.objects.filter( + course_assignment_id__in=[ + m.course_assignment_id for m in memberships + ], + enrollment_id__in=[m.enrollment_id for m in memberships], + ).values_list("course_assignment_id", "enrollment_id") + ) + cache[obj.id] = [ + { + "enrollment_id": m.enrollment_id, + "enrollment_username": m.enrollment.user.username, + "activity_type_name": m.course_assignment.activity.activity_type_name, + "assignment_submitted": ( + m.course_assignment_id, + m.enrollment_id, + ) + in submitted, + } + for m in memberships + ] + return cache[obj.id] class Meta: model = AssignmentGroup fields = ["type", "members"] -class AssignmentSerializer(serializers.ModelSerializer): - activity = ActivitySerializer() - instrument = InstrumentSerializer() - part = PartSerializer() - enrollment = EnrollmentSerializer() - submissions = SubmissionSerializer(many=True) - - class Meta: - model = Assignment - # fields = ["activity", "deadline", "instrument", "id", "url"] - fields = [ - "activity", - "deadline", - "instrument", - "part", - "id", - "enrollment", - "submissions", - "group", - ] - - extra_kwargs = { - "url": {"view_name": "api:assignment-detail", "lookup_field": "id"}, - } - - # def get_fields(self): - # fields = super().get_fields() - # if not self.instance.group: - # del fields['group'] - # return fields - - -class AssignmentViewSetSerializer(serializers.ModelSerializer): - activity = serializers.PrimaryKeyRelatedField(queryset=Activity.objects.all()) - activity_type_name = serializers.CharField( - source="activity.activity_type_name", read_only=True - ) - activity_type_category = serializers.CharField( - source="activity.category", read_only=True - ) - activity_body = serializers.CharField(source="activity.body", read_only=True) - part_type = serializers.CharField(source="activity.part_type.name", read_only=True) - piece_name = serializers.SlugField(source="piece.name", read_only=True) - piece_id = serializers.IntegerField(source="piece.id", read_only=True) - piece_slug = serializers.SlugField(source="piece.slug", read_only=True) - instrument = serializers.CharField(source="instrument.name", read_only=True) - transposition = serializers.CharField( - source="instrument.transposition.name", read_only=True - ) - group = GroupSerializer() - # instrument = InstrumentSerializer() - part = PartSerializer() - # enrollment = EnrollmentSerializer() - submissions = SubmissionSerializer(many=True) - - class Meta: - model = Assignment - # fields = ["activity", "deadline", "instrument", "id", "url"] - # fields = ["activity", "deadline", "instrument", "part", "id", "enrollment", "submissions"] - fields = [ - "id", - "activity", - "activity_type_name", - "activity_type_category", - "activity_body", - "part_type", - "piece_name", - "piece_id", - "piece_slug", - "instrument", - "transposition", - "group", - "part", - "submissions", - ] - - extra_kwargs = { - "url": {"view_name": "api:assignment-detail", "lookup_field": "id"}, - } - - -class AssignmentInstrumentSerializer(serializers.ModelSerializer): - class Meta: - model = Assignment - fields = ["id", "instrument"] - - -class NotationAssignmentSerializer(serializers.ModelSerializer): - activity = ActivitySerializer() - instrument = InstrumentSerializer() - part = PartSerializer() - - class Meta: - model = Assignment - # fields = ["activity", "deadline", "instrument", "id", "url"] - fields = ["activity", "deadline", "instrument", "part", "id"] - - extra_kwargs = { - "url": {"view_name": "api:assignment-detail", "lookup_field": "id"}, - } - - class PiecePlanSerializer(serializers.ModelSerializer): id = serializers.IntegerField() type = serializers.CharField() @@ -174,3 +97,144 @@ class Meta: # extra_kwargs = { # "url": {"view_name": "api:pieceplan-detail", "lookup_field": "id"}, # } + + +def resolve_instrument(enrollment, course_assignment=None): + """The instrument a student uses for a CourseAssignment: the course-level + per-piece override (``CourseAssignment.instrument``, set by + change_piece_instrument) if present, else their enrollment instrument, else + their user instrument.""" + if course_assignment is not None and course_assignment.instrument_id: + return course_assignment.instrument + return enrollment.instrument or enrollment.user.instrument + + +class CourseAssignmentReadSerializer(serializers.Serializer): + """Phase 2 read path: produces the SAME per-assignment shape as + AssignmentViewSetSerializer, but resolved from a CourseAssignment against a + student enrollment (passed in context as ``enrollment``). The ``id`` is the + CourseAssignment id. Per-student data (instrument, part, submissions, group) is + resolved at read time. A response-equivalence test pins that this matches the + legacy per-student serializer field-for-field except ``id``. + + For list rendering the view precomputes per-CA maps (``submissions_by_ca``, + ``group_by_ca``, ``part_for``) and passes them in context, so resolution is + O(1) per row instead of N+1. When those maps are absent (single-object use, + e.g. the equivalence test) it falls back to direct queries.""" + + def _submissions_for(self, ca, enrollment): + by_ca = self.context.get("submissions_by_ca") + if by_ca is not None: + return by_ca.get(ca.id, []) + if enrollment is None: + # Teacher list: no per-student enrollment, so no per-student submissions. + return [] + return list( + Submission.objects.filter(course_assignment=ca, enrollment=enrollment) + .order_by("id") + .prefetch_related("attachments") + ) + + def _group_for(self, ca, enrollment): + by_ca = self.context.get("group_by_ca") + if by_ca is not None: + return by_ca.get(ca.id) + if enrollment is None: + return None + group_assignment = ( + GroupAssignment.objects.select_related("group") + .filter(course_assignment=ca, enrollment=enrollment) + .first() + ) + return group_assignment.group if group_assignment else None + + def _part_for(self, ca): + part_for = self.context.get("part_for") + if part_for is not None: + return part_for(ca.activity, ca.piece) + return Part.for_activity(ca.activity, ca.piece) + + @staticmethod + def submissions_for(ca, enrollment): + return list( + Submission.objects.filter(course_assignment=ca, enrollment=enrollment) + .order_by("id") + .prefetch_related("attachments") + ) + + @staticmethod + def group_assignment_for(ca, enrollment): + return ( + GroupAssignment.objects.select_related("group") + .filter(course_assignment=ca, enrollment=enrollment) + .first() + ) + + def to_representation(self, ca): + # enrollment is None on the teacher list (no per-student context); the + # per-student fields (instrument/transposition/submissions/group) come back + # null/empty, while the piece/activity/part fields are fully populated. + enrollment = self.context["enrollment"] + activity = ca.activity + instrument = resolve_instrument(enrollment, ca) if enrollment else None + part = self._part_for(ca) + submissions = self._submissions_for(ca, enrollment) + group = self._group_for(ca, enrollment) + transposition = instrument.transposition if instrument else None + return { + "id": ca.id, + "activity": activity.id, + "activity_type_name": activity.activity_type_name, + "activity_type_category": activity.category, + "activity_body": activity.body, + "part_type": activity.part_type.name if activity.part_type else None, + "piece_name": ca.piece.name, + "piece_id": ca.piece.id, + "piece_slug": ca.piece.slug, + "instrument": instrument.name if instrument else None, + "transposition": transposition.name if transposition else None, + "group": ( + GroupSerializer(group, context=self.context).data if group else None + ), + "part": PartSerializer(part, context=self.context).data, + "submissions": SubmissionSerializer( + submissions, many=True, context=self.context + ).data, + } + + +class CourseAssignmentRetrieveSerializer(serializers.Serializer): + """Phase 2 single-assignment (retrieve) read path: produces the SAME shape as + the legacy AssignmentSerializer, resolved from a CourseAssignment against the + requesting student's enrollment (context ``enrollment``). The ``id`` is the + CourseAssignment id; a response-equivalence test pins field-for-field parity + except ``id``.""" + + def to_representation(self, ca): + enrollment = self.context["enrollment"] + instrument = resolve_instrument(enrollment, ca) + part = Part.for_activity(ca.activity, ca.piece) + submissions = CourseAssignmentReadSerializer.submissions_for(ca, enrollment) + group_assignment = CourseAssignmentReadSerializer.group_assignment_for( + ca, enrollment + ) + return { + "activity": ActivitySerializer(ca.activity, context=self.context).data, + "deadline": ( + serializers.DateField().to_representation(ca.deadline) + if ca.deadline + else None + ), + "instrument": ( + InstrumentSerializer(instrument, context=self.context).data + if instrument + else None + ), + "part": PartSerializer(part, context=self.context).data, + "id": ca.id, + "enrollment": EnrollmentSerializer(enrollment, context=self.context).data, + "submissions": SubmissionSerializer( + submissions, many=True, context=self.context + ).data, + "group": group_assignment.group_id if group_assignment else None, + } diff --git a/teleband/assignments/api/views.py b/teleband/assignments/api/views.py index ed6c730..9a340a8 100644 --- a/teleband/assignments/api/views.py +++ b/teleband/assignments/api/views.py @@ -1,21 +1,27 @@ from collections import defaultdict -from rest_framework import status -from rest_framework.decorators import action -from rest_framework.mixins import ListModelMixin, RetrieveModelMixin, UpdateModelMixin +from rest_framework.mixins import ListModelMixin, RetrieveModelMixin from rest_framework.response import Response from rest_framework.viewsets import GenericViewSet -from django.db.models import OuterRef, Subquery +from django.db.models import OuterRef, Q, Subquery + +from django.shortcuts import get_object_or_404 from .serializers import ( - AssignmentViewSetSerializer, - AssignmentInstrumentSerializer, - AssignmentSerializer, + CourseAssignmentReadSerializer, + CourseAssignmentRetrieveSerializer, ) from teleband.assignments.api.serializers import ActivitySerializer, PiecePlanSerializer -from teleband.musics.api.serializers import PartTranspositionSerializer -from teleband.assignments.models import Assignment, Activity, AssignmentGroup, PlannedActivity, PiecePlan +from teleband.assignments.models import ( + Activity, + CourseAssignment, + GroupAssignment, + PlannedActivity, + PiecePlan, +) from teleband.courses.models import Course +from teleband.musics.models import Part +from teleband.submissions.models import Submission from teleband.utils.permissions import IsTeacher @@ -34,130 +40,238 @@ class ActivityViewSet(RetrieveModelMixin, ListModelMixin, GenericViewSet): permission_classes = [IsTeacher] def get_queryset(self): - # Define a subquery to get the first assignment for each activity + # Phase 2: the activities assigned in a course come from CourseAssignment + # (one row per (course, activity, piece)), not per-student Assignment rows. distinct_activity_assignments = ( - Assignment.objects.filter( - enrollment__course__slug=self.kwargs["course_slug_slug"], + CourseAssignment.objects.filter( + course__slug=self.kwargs["course_slug_slug"], activity=OuterRef("id"), ) .order_by("id", "pk") .values("activity_id")[:1] ) - # Use the subquery to filter the main queryset - queryset = self.queryset.filter(pk__in=Subquery(distinct_activity_assignments)) + # Use the subquery to filter the main queryset. select_related covers the + # activity_type/category/part_type walked by ActivitySerializer. + queryset = self.queryset.filter( + pk__in=Subquery(distinct_activity_assignments) + ).select_related("activity_type", "activity_type__category", "part_type") return queryset -class AssignmentViewSet( - RetrieveModelMixin, UpdateModelMixin, ListModelMixin, GenericViewSet -): - serializer_class = AssignmentViewSetSerializer - queryset = Assignment.objects.all() +class AssignmentViewSet(RetrieveModelMixin, ListModelMixin, GenericViewSet): + # Phase 2: list/retrieve are fully overridden and resolve from CourseAssignment; + # this queryset/serializer is only for router basename + DRF metadata. + serializer_class = CourseAssignmentReadSerializer + queryset = CourseAssignment.objects.all() lookup_field = "id" permission_classes = [TeacherUpdate] - def get_serializer_class(self): - if self.action in ["update", "partial_update"]: - return AssignmentInstrumentSerializer - elif self.action == "retrieve": - return AssignmentSerializer - return self.serializer_class - - @action(detail=True) - def notation(self, request, *args, **kwargs): - course = Course.objects.get(slug=self.kwargs["course_slug_slug"]) - assignment = self.get_object() - - part_transposition = assignment.part.transpositions.get( - transposition=assignment.instrument.transposition + def retrieve(self, request, *args, **kwargs): + # Phase 2: the single-assignment id is a CourseAssignment id; resolve it + # against the requesting user's enrollment and return the legacy + # AssignmentSerializer shape. + enrollment = self.request.user.enrollment_set.select_related( + "role", "course" + ).get(course__slug=self.kwargs["course_slug_slug"]) + course_assignment = get_object_or_404( + CourseAssignment.objects.select_related( + "activity", + "activity__part_type", + "activity__activity_type", + "activity__activity_type__category", + "piece", + ), + pk=self.kwargs["id"], + course=enrollment.course, ) - - serializer = PartTranspositionSerializer( - part_transposition, context=self.get_serializer_context() + serializer = CourseAssignmentRetrieveSerializer( + course_assignment, + context={"request": request, "enrollment": enrollment}, ) return Response(serializer.data) - def get_queryset(self): - course = Course.objects.get(slug=self.kwargs["course_slug_slug"]) - role = self.request.user.enrollment_set.get(course=course).role + # Fallback ordering by activity type name prefix, used when an assignment has + # no PlannedActivity.order (shared by the student and teacher list paths). + _FALLBACK_ORDERING = { + "Melody": 1, + "Bassline": 2, + "Creativity": 3, + "Reflection": 4, + "Connect": 5, + "Aural": 3, + "Exploratory": 3, + "Theoretical": 3, + "MelodyPost": 3.1, + "BasslinePost": 3.2, + } + + def _grouped_by_piece(self, serialized_items, plan_order_by_id): + # Group serialized rows by piece slug, then sort each group by the row's + # PlannedActivity.order (when present) else the activity-type fallback. + grouped = defaultdict(list) + for item in serialized_items: + grouped[item["piece_slug"]].append(item) - if role.name == "Student": + def sort_key(a): + plan_order = plan_order_by_id.get(a["id"]) + if plan_order is not None: + return (0, plan_order) return ( - Assignment.objects.filter( - enrollment__course=course, enrollment__user=self.request.user - ) - .select_related( - "activity", - "instrument", - "piece", - "activity__part_type", - "instrument__transposition", - "group", - ) - .prefetch_related("submissions") + 1, + self._FALLBACK_ORDERING.get(a["activity_type_name"].split()[0], 999), ) - if role.name == "Teacher": - return Assignment.objects.filter(enrollment__course=course).select_related( + + for slug in grouped: + grouped[slug].sort(key=sort_key) + return grouped + + def _student_list(self, request, enrollment): + # Phase 2 read path: resolve the student's assignments from CourseAssignment + # instead of per-student Assignment rows. A student sees every CourseAssignment + # for their course that is NOT scoped to a group, UNION the grouped ones + # (telephone_fixed) that name their enrollment. This also fixes late joiners: + # they get the course's CourseAssignments even with no Assignment rows. + grouped_ca_ids = GroupAssignment.objects.values("course_assignment_id") + my_ca_ids = GroupAssignment.objects.filter(enrollment=enrollment).values( + "course_assignment_id" + ) + planned_order_subquery = PlannedActivity.objects.filter( + piece_plan_id=OuterRef("piece_plan_id"), + activity_id=OuterRef("activity_id"), + ).values("order")[:1] + course_assignments = ( + CourseAssignment.objects.filter(course=enrollment.course) + .filter(~Q(id__in=grouped_ca_ids) | Q(id__in=my_ca_ids)) + .select_related( "activity", - "instrument", - "piece", "activity__part_type", - "instrument__transposition", - "group", + "activity__activity_type", + "activity__activity_type__category", + "piece", ) + .annotate(plan_order=Subquery(planned_order_subquery)) + ) + course_assignments = list(course_assignments) + context = { + "request": request, + "enrollment": enrollment, + **self._read_context(course_assignments, enrollment), + } + serialized = CourseAssignmentReadSerializer( + course_assignments, many=True, context=context + ).data + plan_order_by_id = { + ca.id: ca.plan_order for ca in course_assignments if ca.piece_plan_id + } + return Response(self._grouped_by_piece(serialized, plan_order_by_id)) - def list(self, request, *args, **kwargs): - assignments = self.get_queryset() + def _read_context(self, course_assignments, enrollment): + # Precompute the per-CA maps CourseAssignmentReadSerializer needs so the + # list resolves part/submissions/group in O(1) per row instead of N+1 + # (landmine: read-time per-(student x activity) resolution). + ca_ids = [ca.id for ca in course_assignments] + pieces = {ca.piece for ca in course_assignments} - serialized = AssignmentViewSetSerializer( - assignments, context={"request": request}, many=True - ) + # submissions for this student, grouped by CourseAssignment, attachments + # prefetched (matches the legacy per-assignment submissions list). + submissions_by_ca = defaultdict(list) + for sub in ( + Submission.objects.filter( + course_assignment_id__in=ca_ids, enrollment=enrollment + ) + .order_by("id") + .prefetch_related("attachments") + ): + submissions_by_ca[sub.course_assignment_id].append(sub) - grouped = defaultdict(list) - for assignment in serialized.data: - key = assignment["piece_slug"] - grouped[key].append(assignment) - - # Build a lookup of (piece_plan_id, activity_id) -> order from PlannedActivity - piece_plan_ids = {a.piece_plan_id for a in assignments if a.piece_plan_id} - planned_order = {} - for pa in PlannedActivity.objects.filter(piece_plan_id__in=piece_plan_ids): - planned_order[(pa.piece_plan_id, pa.activity_id)] = pa.order - - # Map assignment id -> planned activity order - assignment_plan_order = {} - for a in assignments: - if a.piece_plan_id: - assignment_plan_order[a.id] = planned_order.get( - (a.piece_plan_id, a.activity_id) - ) - - # Fallback ordering by activity type name prefix - fallback_ordering = { - "Melody": 1, - "Bassline": 2, - "Creativity": 3, - "Reflection": 4, - "Connect": 5, - "Aural": 3, - "Exploratory": 3, - "Theoretical": 3, - "MelodyPost": 3.1, - "BasslinePost": 3.2, + # group (telephone_fixed) per CourseAssignment for this student. + group_by_ca = { + ga.course_assignment_id: ga.group + for ga in GroupAssignment.objects.filter( + course_assignment_id__in=ca_ids, enrollment=enrollment + ).select_related("group") } - def sort_key(a): - plan_order = assignment_plan_order.get(a["id"]) - if plan_order is not None: - return (0, plan_order) - return (1, fallback_ordering.get(a["activity_type_name"].split()[0], 999)) + return { + "submissions_by_ca": submissions_by_ca, + "group_by_ca": group_by_ca, + "part_for": self._part_resolver(course_assignments), + } - for pieceplan in grouped: - grouped[pieceplan].sort(key=sort_key) + def _part_resolver(self, course_assignments): + # One Part query for every piece in play (with the tree PartSerializer + # walks), then resolve (activity, piece) -> Part in memory, mirroring + # Part.for_activity's part_type match with a Melody fallback. Shared by the + # student and teacher lists so part resolution never hits the N+1 path. + pieces = {ca.piece for ca in course_assignments} + parts = ( + Part.objects.filter(piece__in=pieces) + .select_related("part_type", "piece", "piece__composer") + .prefetch_related("transpositions__transposition", "instrument_samples") + ) + by_type = {} + melody_by_piece = {} + for part in parts: + by_type[(part.piece_id, part.part_type_id)] = part + if part.part_type.name == "Melody": + melody_by_piece[part.piece_id] = part - return Response(grouped) + def part_for(activity, piece): + if activity.part_type_id is not None: + hit = by_type.get((piece.id, activity.part_type_id)) + if hit is not None: + return hit + return melody_by_piece.get(piece.id) + + return part_for + + def _teacher_list(self, request): + # Phase 2: a teacher sees what the COURSE is assigned -- one row per + # CourseAssignment (every assigned (piece, activity)), not one per student. + # The frontend's teacher view only derives the distinct (piece, activity) + # set from this list, so per-student fields come back null/empty. This + # collapses the response from A*S rows to A and makes it constant in roster. + planned_order_subquery = PlannedActivity.objects.filter( + piece_plan_id=OuterRef("piece_plan_id"), + activity_id=OuterRef("activity_id"), + ).values("order")[:1] + course_assignments = list( + CourseAssignment.objects.filter( + course__slug=self.kwargs["course_slug_slug"] + ) + .select_related( + "activity", + "activity__part_type", + "activity__activity_type", + "activity__activity_type__category", + "piece", + ) + .annotate(plan_order=Subquery(planned_order_subquery)) + ) + # part_for keeps the per-row Part lookup off the N+1 path; submissions/group + # are empty for the teacher (enrollment=None), so only the part map is built. + context = { + "request": request, + "enrollment": None, + "part_for": self._part_resolver(course_assignments), + } + serialized = CourseAssignmentReadSerializer( + course_assignments, many=True, context=context + ).data + plan_order_by_id = { + ca.id: ca.plan_order for ca in course_assignments if ca.piece_plan_id + } + return Response(self._grouped_by_piece(serialized, plan_order_by_id)) + + def list(self, request, *args, **kwargs): + enrollment = self.request.user.enrollment_set.select_related("role").get( + course__slug=self.kwargs["course_slug_slug"] + ) + if enrollment.role.name == "Student": + return self._student_list(request, enrollment) + return self._teacher_list(request) class PiecePlanViewSet(RetrieveModelMixin, ListModelMixin, GenericViewSet): @@ -168,8 +282,14 @@ class PiecePlanViewSet(RetrieveModelMixin, ListModelMixin, GenericViewSet): def get_queryset(self): course = Course.objects.get(slug=self.kwargs["course_slug_slug"]) - return PiecePlan.objects.filter(curriculum__course=course).prefetch_related( - "piece" + # PiecePlanSerializer walks piece->composer and activities-> + # activity_type/category/part_type. + return ( + PiecePlan.objects.filter(curriculum__course=course) + .select_related("piece__composer") + .prefetch_related( + "activities__activity_type__category", "activities__part_type" + ) ) # def get_serializer_class(self): diff --git a/teleband/assignments/migrations/0031_default_piece_plans_20230920_2336.py b/teleband/assignments/migrations/0031_default_piece_plans_20230920_2336.py index 600d370..57cc32a 100644 --- a/teleband/assignments/migrations/0031_default_piece_plans_20230920_2336.py +++ b/teleband/assignments/migrations/0031_default_piece_plans_20230920_2336.py @@ -2,7 +2,6 @@ from django.db import migrations - piece_names = [ "Air for Band", "Celebration for a New Day", diff --git a/teleband/assignments/migrations/0033_auto_20240312_2321.py b/teleband/assignments/migrations/0033_auto_20240312_2321.py index e0526fb..5ab10bb 100644 --- a/teleband/assignments/migrations/0033_auto_20240312_2321.py +++ b/teleband/assignments/migrations/0033_auto_20240312_2321.py @@ -1,78 +1,26 @@ # Generated by Django 3.2.11 on 2024-03-13 03:21 -from calendar import c -from datetime import date -from django.db import IntegrityError, migrations - -from teleband.courses.helper import assign_piece_plan -from teleband.assignments.models import PiecePlan as PiecePlanModel -from teleband.courses.models import Course as CourseModel - -DEMO_USERS = [ - "demomike", - "demodave", - "demoalden", -] -NEA_CREATE_DEMO_PIECES = [ - "Freedom 2040 (Band)", - "Freedom 2040 (Orchestra)", - "Down by the Riverside", - "Deep River", - "I Want to be Ready", -] - -NEA_CONDITIONS = ["Aural", "Theoretical", "Exploratory"] +from django.db import migrations def add_demos(apps, schema_editor): - Course = apps.get_model("courses", "Course") - Enrollment = apps.get_model("courses", "Enrollment") - Instrument = apps.get_model("instruments", "Instrument") - Piece = apps.get_model("musics", "Piece") - PiecePlan = apps.get_model("assignments", "PiecePlan") - Role = apps.get_model("users", "Role") - User = apps.get_model("users", "User") - - owner = User.objects.get(username=DEMO_USERS[0]) - student_role = Role.objects.get(name="Student") - - for condition in NEA_CONDITIONS: - demo_course, cc = Course.objects.update_or_create( - name=f"{condition} Demo as Student", - owner=owner, - start_date=date(2024, 3, 13), - end_date=date(2024, 7, 13), - slug=f"{condition.lower()}-demo-as-student", - ) - - for demo_username in DEMO_USERS: - user = User.objects.get(username=demo_username) - if user.instrument is None: - user.instrument = Instrument.objects.get(name="Piano") - user.save() - try: - Enrollment.objects.update_or_create( - user=user, - course=demo_course, - instrument=user.instrument, - role=student_role, - ) - except IntegrityError as e: - print(f"IntegrityError: {e}") - - for piece_name in NEA_CREATE_DEMO_PIECES: - if piece_name == "I Want to be Ready" and condition != "Aural": - pass - - piece = Piece.objects.get(name=piece_name) - piece_plan, p_created = PiecePlan.objects.update_or_create( - name=f"NEA-{piece.name}-{condition}", - piece=piece, - ) - # FIXME: is this bad news because I end up getting ahistorical models involved? - ppo = PiecePlanModel.objects.get(id=piece_plan.id) - co = CourseModel.objects.get(id=demo_course.id) - assign_piece_plan(co, ppo) + """Intentionally a no-op (was: seed demo NEA courses + assignments). + + The original migration imported the LIVE ``teleband.courses.helper.assign_piece_plan`` + and live model classes and called them to create demo assignments. Importing live + app code into a data migration makes a fresh ``migrate`` depend on the *current* + schema and helper behavior: as the models and assign helpers evolve (notably the + Phase 2 CourseAssignment remodel), this historical migration would run new code + against the old, frozen schema and break the build. We already hit a variant of this + when a ``select_related("user")`` in the helper selected ``users.external_id`` before + that column's migration had run. + + The demo data is non-essential convenience seed -- no test, fixture, or schema depends + on it -- so the seeding is removed and this migration no longer references app code. + Databases that already applied the original migration are unaffected; only fresh + migrates (tests, CI, new deploys) skip the demo seeding. + """ + pass class Migration(migrations.Migration): diff --git a/teleband/assignments/migrations/0038_courseassignment_groupassignment_and_more.py b/teleband/assignments/migrations/0038_courseassignment_groupassignment_and_more.py new file mode 100644 index 0000000..4626d4a --- /dev/null +++ b/teleband/assignments/migrations/0038_courseassignment_groupassignment_and_more.py @@ -0,0 +1,118 @@ +# Generated by Django 5.1.15 on 2026-06-27 22:40 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0037_rename_beginner_activity_types"), + ("courses", "0007_data_migration_stress_test_course"), + ("musics", "0028_partinstrumentsample"), + ] + + operations = [ + migrations.CreateModel( + name="CourseAssignment", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ("deadline", models.DateField(blank=True, null=True)), + ("created_at", models.DateTimeField(auto_now_add=True)), + ( + "activity", + models.ForeignKey( + on_delete=django.db.models.deletion.PROTECT, + to="assignments.activity", + ), + ), + ( + "course", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="course_assignments", + to="courses.course", + ), + ), + ( + "piece", + models.ForeignKey( + on_delete=django.db.models.deletion.PROTECT, to="musics.piece" + ), + ), + ( + "piece_plan", + models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + to="assignments.pieceplan", + ), + ), + ], + ), + migrations.CreateModel( + name="GroupAssignment", + fields=[ + ( + "id", + models.BigAutoField( + auto_created=True, + primary_key=True, + serialize=False, + verbose_name="ID", + ), + ), + ( + "course_assignment", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="group_assignments", + to="assignments.courseassignment", + ), + ), + ( + "enrollment", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + to="courses.enrollment", + ), + ), + ( + "group", + models.ForeignKey( + on_delete=django.db.models.deletion.CASCADE, + related_name="memberships", + to="assignments.assignmentgroup", + ), + ), + ], + ), + migrations.AddIndex( + model_name="courseassignment", + index=models.Index( + fields=["course", "piece"], name="assignments_course__c52c53_idx" + ), + ), + migrations.AddConstraint( + model_name="courseassignment", + constraint=models.UniqueConstraint( + fields=("course", "activity", "piece"), name="unique_course_assignment" + ), + ), + migrations.AddConstraint( + model_name="groupassignment", + constraint=models.UniqueConstraint( + fields=("enrollment", "course_assignment"), + name="unique_group_assignment", + ), + ), + ] diff --git a/teleband/assignments/migrations/0039_backfill_course_assignments.py b/teleband/assignments/migrations/0039_backfill_course_assignments.py new file mode 100644 index 0000000..f2a41c6 --- /dev/null +++ b/teleband/assignments/migrations/0039_backfill_course_assignments.py @@ -0,0 +1,91 @@ +from django.db import migrations + + +def backfill_course_assignments(apps, schema_editor): + """Create CourseAssignment (and GroupAssignment) rows from existing per-student + Assignment rows, collapsing by (course, activity, piece). + + Legacy Assignment rows may have piece IS NULL (piece became nullable in + migration 0026 and was never backfilled). Every Assignment has a non-null + part, and Part.piece is non-null, so we derive the piece from part.piece when + Assignment.piece is null -- this both fixes the legacy rows and keeps the + CourseAssignment.piece NOT NULL invariant. + """ + Assignment = apps.get_model("assignments", "Assignment") + CourseAssignment = apps.get_model("assignments", "CourseAssignment") + GroupAssignment = apps.get_model("assignments", "GroupAssignment") + + seen = set() + to_create = [] + rows = Assignment.objects.values( + "enrollment__course_id", + "activity_id", + "piece_id", + "part__piece_id", + "piece_plan_id", + "deadline", + ) + for row in rows.iterator(): + course_id = row["enrollment__course_id"] + piece_id = row["piece_id"] or row["part__piece_id"] + key = (course_id, row["activity_id"], piece_id) + if key in seen: + continue + seen.add(key) + to_create.append( + CourseAssignment( + course_id=course_id, + activity_id=row["activity_id"], + piece_id=piece_id, + piece_plan_id=row["piece_plan_id"], + deadline=row["deadline"], + ) + ) + CourseAssignment.objects.bulk_create(to_create, ignore_conflicts=True) + + # Map (course, activity, piece) -> course_assignment id for the group backfill. + ca_map = { + (ca["course_id"], ca["activity_id"], ca["piece_id"]): ca["id"] + for ca in CourseAssignment.objects.values( + "id", "course_id", "activity_id", "piece_id" + ) + } + + seen_ga = set() + ga_to_create = [] + grouped = Assignment.objects.filter(group__isnull=False).values( + "group_id", + "enrollment_id", + "enrollment__course_id", + "activity_id", + "piece_id", + "part__piece_id", + ) + for row in grouped.iterator(): + piece_id = row["piece_id"] or row["part__piece_id"] + ca_id = ca_map.get((row["enrollment__course_id"], row["activity_id"], piece_id)) + if ca_id is None: + continue + key = (row["enrollment_id"], ca_id) + if key in seen_ga: + continue + seen_ga.add(key) + ga_to_create.append( + GroupAssignment( + group_id=row["group_id"], + enrollment_id=row["enrollment_id"], + course_assignment_id=ca_id, + ) + ) + GroupAssignment.objects.bulk_create(ga_to_create, ignore_conflicts=True) + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0038_courseassignment_groupassignment_and_more"), + ] + + operations = [ + migrations.RunPython(backfill_course_assignments, migrations.RunPython.noop), + ] diff --git a/teleband/assignments/migrations/0040_courseassignment_instrument.py b/teleband/assignments/migrations/0040_courseassignment_instrument.py new file mode 100644 index 0000000..5ef372d --- /dev/null +++ b/teleband/assignments/migrations/0040_courseassignment_instrument.py @@ -0,0 +1,25 @@ +# Generated by Django 5.1.15 on 2026-06-28 00:38 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0039_backfill_course_assignments"), + ("instruments", "0006_delete_instrumentconfig"), + ] + + operations = [ + migrations.AddField( + model_name="courseassignment", + name="instrument", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + to="instruments.instrument", + ), + ), + ] diff --git a/teleband/assignments/migrations/0041_remove_assignment_activity_and_more.py b/teleband/assignments/migrations/0041_remove_assignment_activity_and_more.py new file mode 100644 index 0000000..2900175 --- /dev/null +++ b/teleband/assignments/migrations/0041_remove_assignment_activity_and_more.py @@ -0,0 +1,48 @@ +# Generated by Django 5.1.15 on 2026-06-28 01:13 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0040_courseassignment_instrument"), + ] + + operations = [ + # Drop the unique constraint first: it references (activity, enrollment, + # piece), and SQLite's table-remake during RemoveField would otherwise try + # to recreate it against already-removed fields. + migrations.RemoveConstraint( + model_name="assignment", + name="unique_assignment", + ), + migrations.RemoveField( + model_name="assignment", + name="activity", + ), + migrations.RemoveField( + model_name="assignment", + name="enrollment", + ), + migrations.RemoveField( + model_name="assignment", + name="group", + ), + migrations.RemoveField( + model_name="assignment", + name="instrument", + ), + migrations.RemoveField( + model_name="assignment", + name="part", + ), + migrations.RemoveField( + model_name="assignment", + name="piece", + ), + migrations.RemoveField( + model_name="assignment", + name="piece_plan", + ), + ] diff --git a/teleband/assignments/migrations/0042_delete_assignment.py b/teleband/assignments/migrations/0042_delete_assignment.py new file mode 100644 index 0000000..2571975 --- /dev/null +++ b/teleband/assignments/migrations/0042_delete_assignment.py @@ -0,0 +1,20 @@ +# Generated by Django 5.1.15 on 2026-06-28 01:13 + +from django.db import migrations + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0041_remove_assignment_activity_and_more"), + ( + "submissions", + "0020_remove_submission_submissions_assignm_06178b_idx_and_more", + ), + ] + + operations = [ + migrations.DeleteModel( + name="Assignment", + ), + ] diff --git a/teleband/assignments/models.py b/teleband/assignments/models.py index a095b8e..ccc36cf 100644 --- a/teleband/assignments/models.py +++ b/teleband/assignments/models.py @@ -1,8 +1,7 @@ from django.db import models from teleband.courses.models import Course, Enrollment -from teleband.instruments.models import Instrument -from teleband.musics.models import Part, PartType, Piece +from teleband.musics.models import PartType, Piece class ActivityCategory(models.Model): @@ -54,24 +53,6 @@ class PiecePlan(models.Model): piece = models.ForeignKey(Piece, on_delete=models.PROTECT) type = models.CharField(max_length=255, null=True, blank=True) - def assign(self, enrollment, instrument, deadline=None): - assignments = [] - piece = self.piece - for activity in self.activities.all(): - part = Part.for_activity(activity, piece) - assignments.append( - Assignment.objects.create( - activity=activity, - enrollment=enrollment, - part=part, - instrument=instrument, - piece_plan=self, - deadline=deadline, - piece=self.piece, - ) - ) - return assignments - def __str__(self): if self.type: return f"{self.name}: {self.piece.name} ({self.type})" @@ -79,39 +60,73 @@ def __str__(self): return f"{self.name}: {self.piece.name} " -class Assignment(models.Model): +class AssignmentGroup(models.Model): + + type = models.CharField(max_length=255, null=True, blank=True) + + +class CourseAssignment(models.Model): + """What a course is assigned: one row per (course, activity, piece), instead of + one Assignment per enrolled student. Per-student data (instrument, part) is + resolved at read time and persisted on Submission. See docs/remodel_phase2_design.md. + """ + course = models.ForeignKey( + Course, on_delete=models.CASCADE, related_name="course_assignments" + ) activity = models.ForeignKey(Activity, on_delete=models.PROTECT) - enrollment = models.ForeignKey(Enrollment, on_delete=models.PROTECT) - part = models.ForeignKey(Part, on_delete=models.PROTECT) - deadline = models.DateField(null=True, blank=True) - instrument = models.ForeignKey(Instrument, on_delete=models.PROTECT) + piece = models.ForeignKey(Piece, on_delete=models.PROTECT) piece_plan = models.ForeignKey( PiecePlan, on_delete=models.PROTECT, null=True, blank=True ) - piece = models.ForeignKey(Piece, on_delete=models.PROTECT, null=True, blank=True) - group = models.ForeignKey( - "AssignmentGroup", on_delete=models.PROTECT, null=True, blank=True + # Course-level per-piece instrument override (set by change_piece_instrument). + # Null means each student resolves their own instrument from their enrollment. + instrument = models.ForeignKey( + "instruments.Instrument", + on_delete=models.PROTECT, + null=True, + blank=True, ) - + deadline = models.DateField(null=True, blank=True) created_at = models.DateTimeField(auto_now_add=True) class Meta: - # FIXME: do this with https://docs.djangoproject.com/en/5.0/ref/models/options/#unique-together instead. - # nevermind, this may be deprecated constraints = [ models.UniqueConstraint( - fields=["activity", "enrollment", "piece"], name="unique_assignment" + fields=["course", "activity", "piece"], + name="unique_course_assignment", ) ] + indexes = [models.Index(fields=["course", "piece"])] def __str__(self): - return f"[{self.enrollment.user.username}] {self.activity.id} {self.piece}" + return f"{self.course.slug}: {self.activity_id} {self.piece}" -class AssignmentGroup(models.Model): +class GroupAssignment(models.Model): + """Links a student (enrollment) to a specific CourseAssignment within a group, + for telephone_fixed plans where different students get different activities. + Normal plans need no GroupAssignment -- every enrolled student is implicitly + assigned every non-grouped CourseAssignment in their course.""" - type = models.CharField(max_length=255, null=True, blank=True) + group = models.ForeignKey( + AssignmentGroup, on_delete=models.CASCADE, related_name="memberships" + ) + enrollment = models.ForeignKey(Enrollment, on_delete=models.CASCADE) + course_assignment = models.ForeignKey( + CourseAssignment, on_delete=models.CASCADE, related_name="group_assignments" + ) + + class Meta: + constraints = [ + models.UniqueConstraint( + fields=["enrollment", "course_assignment"], + name="unique_group_assignment", + ) + ] + + def __str__(self): + return f"{self.enrollment} -> {self.course_assignment} (group {self.group_id})" class PlannedActivity(models.Model): diff --git a/teleband/assignments/tests/factories.py b/teleband/assignments/tests/factories.py new file mode 100644 index 0000000..8cbfa40 --- /dev/null +++ b/teleband/assignments/tests/factories.py @@ -0,0 +1,66 @@ +from factory import Faker, SubFactory +from factory.django import DjangoModelFactory + +from teleband.assignments.models import ( + Activity, + ActivityCategory, + ActivityType, + AssignmentGroup, + CourseAssignment, + GroupAssignment, +) +from teleband.courses.tests.factories import CourseFactory +from teleband.courses.tests.factories import EnrollmentFactory +from teleband.musics.tests.factories import PartTypeFactory, PieceFactory + + +class ActivityCategoryFactory(DjangoModelFactory): + name = Faker("word") + + class Meta: + model = ActivityCategory + + +class ActivityTypeFactory(DjangoModelFactory): + name = Faker("word") + category = SubFactory(ActivityCategoryFactory) + + class Meta: + model = ActivityType + django_get_or_create = ["name"] + + +class ActivityFactory(DjangoModelFactory): + activity_type = SubFactory(ActivityTypeFactory) + part_type = SubFactory(PartTypeFactory) + body = Faker("sentence") + activity_type_name = Faker("word") + category = Faker("word") + + class Meta: + model = Activity + + +class AssignmentGroupFactory(DjangoModelFactory): + type = "telephone_fixed" + + class Meta: + model = AssignmentGroup + + +class CourseAssignmentFactory(DjangoModelFactory): + course = SubFactory(CourseFactory) + activity = SubFactory(ActivityFactory) + piece = SubFactory(PieceFactory) + + class Meta: + model = CourseAssignment + + +class GroupAssignmentFactory(DjangoModelFactory): + group = SubFactory(AssignmentGroupFactory) + enrollment = SubFactory(EnrollmentFactory) + course_assignment = SubFactory(CourseAssignmentFactory) + + class Meta: + model = GroupAssignment diff --git a/teleband/assignments/tests/test_course_assignment_model.py b/teleband/assignments/tests/test_course_assignment_model.py new file mode 100644 index 0000000..98db5ce --- /dev/null +++ b/teleband/assignments/tests/test_course_assignment_model.py @@ -0,0 +1,50 @@ +"""Model tests for the Phase 2 CourseAssignment / GroupAssignment tables.""" + +import pytest +from django.db import IntegrityError + +from teleband.assignments.models import CourseAssignment, GroupAssignment +from teleband.assignments.tests.factories import ( + ActivityFactory, + AssignmentGroupFactory, + CourseAssignmentFactory, + GroupAssignmentFactory, +) +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.tests.factories import PieceFactory + +pytestmark = pytest.mark.django_db + + +def test_course_assignment_unique_per_course_activity_piece(): + course = CourseFactory() + activity = ActivityFactory() + piece = PieceFactory() + CourseAssignmentFactory(course=course, activity=activity, piece=piece) + + # A second row with the same (course, activity, piece) is rejected. + with pytest.raises(IntegrityError): + CourseAssignment.objects.create(course=course, activity=activity, piece=piece) + + +def test_course_assignment_allows_same_activity_different_piece(): + course = CourseFactory() + activity = ActivityFactory() + CourseAssignmentFactory(course=course, activity=activity, piece=PieceFactory()) + # Different piece -> allowed. + CourseAssignmentFactory(course=course, activity=activity, piece=PieceFactory()) + assert ( + CourseAssignment.objects.filter(course=course, activity=activity).count() == 2 + ) + + +def test_group_assignment_unique_per_enrollment_course_assignment(): + enrollment = EnrollmentFactory() + ca = CourseAssignmentFactory() + group = AssignmentGroupFactory() + GroupAssignmentFactory(group=group, enrollment=enrollment, course_assignment=ca) + + with pytest.raises(IntegrityError): + GroupAssignment.objects.create( + group=group, enrollment=enrollment, course_assignment=ca + ) diff --git a/teleband/assignments/tests/test_drf_views.py b/teleband/assignments/tests/test_drf_views.py deleted file mode 100644 index b58625d..0000000 --- a/teleband/assignments/tests/test_drf_views.py +++ /dev/null @@ -1,30 +0,0 @@ -import pytest -from django.test import RequestFactory - -from teleband.assignments.api.views import AssignmentViewSet -from teleband.courses.models import Enrollment - -pytestmark = pytest.mark.django_db - - -class TestAssignmentViewSet: - def test_get_queryset_student(self, enrollment: Enrollment, rf: RequestFactory): - view = AssignmentViewSet() - - enrollment.role.name = "Student" - enrollment.role.save() - - request = rf.get("/fake-url/") - request.user = enrollment.user - - view.request = request - setattr(view, "kwargs", {"course_slug_slug": enrollment.course.slug}) - - queryset = view.get_queryset() - # actually there is nothing in the queryset, need - # to populate it with some assignments for this student - # and some other students to actually check this - - # Make sure every assignment is assigned to me and only me - for assignment in queryset: - assert enrollment.user == assignment.enrollment.user diff --git a/teleband/assignments/tests/test_list_ordering.py b/teleband/assignments/tests/test_list_ordering.py new file mode 100644 index 0000000..d092e5e --- /dev/null +++ b/teleband/assignments/tests/test_list_ordering.py @@ -0,0 +1,56 @@ +"""AssignmentViewSet.list ordering test (Phase 1c #23/#24). + +The list groups assignments by piece and sorts each group by the assignment's +PlannedActivity.order. After moving that order from a Python-built dict to a +correlated-subquery annotation, this pins that the response is still sorted by +plan order regardless of assignment creation order. +""" + +import pytest +from rest_framework.test import APIClient + +from teleband.assignments.models import CourseAssignment, PiecePlan, PlannedActivity +from teleband.assignments.tests.factories import ActivityFactory +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.users.tests.factories import RoleFactory, UserFactory + +pytestmark = pytest.mark.django_db + + +def test_list_sorted_by_planned_activity_order(): + teacher_role = RoleFactory(name="Teacher") + student_role = RoleFactory(name="Student") + course = CourseFactory() + teacher = UserFactory() + EnrollmentFactory(user=teacher, course=course, role=teacher_role) + EnrollmentFactory(course=course, role=student_role) + + piece = PieceFactory() + plan = PiecePlan.objects.create(name="p", piece=piece) + + # Three activities with explicit plan order 0,1,2. + activities = [] + for order in range(3): + part = PartFactory(piece=piece) + activity = ActivityFactory( + part_type=part.part_type, activity_type_name=f"A{order}" + ) + PlannedActivity.objects.create(piece_plan=plan, activity=activity, order=order) + activities.append((activity, part)) + + # Create the CourseAssignments in REVERSE order so DB/creation order != plan + # order (each carries piece_plan so the plan-order annotation resolves). + for activity, part in reversed(activities): + CourseAssignment.objects.create( + course=course, activity=activity, piece=piece, piece_plan=plan + ) + + client = APIClient() + client.force_authenticate(user=teacher) + resp = client.get(f"/api/courses/{course.slug}/assignments/") + assert resp.status_code == 200, resp.content + + group = resp.data[piece.slug] + names = [a["activity_type_name"] for a in group] + assert names == ["A0", "A1", "A2"], names diff --git a/teleband/assignments/tests/test_query_counts.py b/teleband/assignments/tests/test_query_counts.py new file mode 100644 index 0000000..bf232db --- /dev/null +++ b/teleband/assignments/tests/test_query_counts.py @@ -0,0 +1,213 @@ +"""Query-count regression tests for the assignment list path. + +Phase 1a remodel guard: the teacher/student assignment list must issue a number +of SQL queries that is CONSTANT with respect to roster size and group size. +Before the remodel, AssignmentViewSetSerializer triggered N+1s on the part tree, +submissions/attachments, and (worst) GroupSerializer.get_members at O(M^2) per +group. These tests fail loudly if any of those regress. +""" + +import pytest +from django.test.utils import CaptureQueriesContext +from django.db import connection +from rest_framework.test import APIClient + +from teleband.assignments.models import CourseAssignment, GroupAssignment +from teleband.assignments.tests.factories import ( + ActivityFactory, + AssignmentGroupFactory, +) +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.instruments.tests.factories import InstrumentFactory +from teleband.musics.models import PartTransposition +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.submissions.models import SubmissionAttachment +from teleband.submissions.tests.factories import SubmissionFactory +from teleband.users.tests.factories import RoleFactory, UserFactory + +pytestmark = pytest.mark.django_db + + +def _build_course(num_students, num_activities=3): + """Create a course with a teacher and ``num_students`` students. The course has + one CourseAssignment per activity on a shared piece; every student has a + submission against each. Returns (course, teacher).""" + teacher_role = RoleFactory(name="Teacher") + student_role = RoleFactory(name="Student") + course = CourseFactory() + teacher = UserFactory() + EnrollmentFactory(user=teacher, course=course, role=teacher_role) + + piece = PieceFactory() + parts = [PartFactory(piece=piece) for _ in range(num_activities)] + # Exercise the part-tree prefetch (transpositions + their transposition). + for part in parts: + PartTransposition.objects.create( + part=part, transposition=InstrumentFactory().transposition + ) + activities = [ + ActivityFactory(part_type=parts[i].part_type) for i in range(num_activities) + ] + # Phase 2: one CourseAssignment per (course, activity, piece), not per student. + course_assignments = [ + CourseAssignment.objects.create(course=course, activity=activity, piece=piece) + for activity in activities + ] + + for _ in range(num_students): + enrollment = EnrollmentFactory(course=course, role=student_role) + for i, ca in enumerate(course_assignments): + # Some submitted work + attachments to exercise those prefetches. + submission = SubmissionFactory( + course_assignment=ca, + enrollment=enrollment, + instrument=enrollment.instrument, + part=parts[i], + ) + SubmissionAttachment.objects.create(submission=submission, file="a.wav") + + return course, teacher + + +def _count_list_queries(course, user): + client = APIClient() + client.force_authenticate(user=user) + url = f"/api/courses/{course.slug}/assignments/" + with CaptureQueriesContext(connection) as ctx: + response = client.get(url) + assert response.status_code == 200, response.content + return len(ctx.captured_queries) + + +class TestAssignmentListQueryCounts: + def test_teacher_list_constant_in_roster_size(self): + """The teacher list query count must not grow with the number of + students. This is the highest-leverage N+1 fix in Phase 1a.""" + small_course, small_teacher = _build_course(num_students=2) + large_course, large_teacher = _build_course(num_students=20) + + small = _count_list_queries(small_course, small_teacher) + large = _count_list_queries(large_course, large_teacher) + + assert small == large, ( + f"Teacher list query count grows with roster size " + f"({small} queries for 2 students vs {large} for 20) -- N+1 regression." + ) + + def test_student_list_constant_in_assignment_count(self): + """A student's own list must not grow per assignment.""" + course = CourseFactory() + student_role = RoleFactory(name="Student") + teacher_role = RoleFactory(name="Teacher") + EnrollmentFactory(user=UserFactory(), course=course, role=teacher_role) + student = UserFactory() + enrollment = EnrollmentFactory(user=student, course=course, role=student_role) + piece = PieceFactory() + + def add_assignments(n): + for _ in range(n): + part = PartFactory(piece=piece) + PartTransposition.objects.create( + part=part, transposition=InstrumentFactory().transposition + ) + activity = ActivityFactory(part_type=part.part_type) + # Phase 2 student list reads from CourseAssignment; a matching + # submission exercises the flipped read path's per-row scaling. + ca = CourseAssignment.objects.create( + course=course, activity=activity, piece=piece + ) + SubmissionFactory( + course_assignment=ca, + enrollment=enrollment, + instrument=enrollment.instrument, + part=part, + ) + + add_assignments(2) + few = _count_list_queries(course, student) + add_assignments(18) + many = _count_list_queries(course, student) + + assert few == many, ( + f"Student list query count grows with assignment count " + f"({few} vs {many}) -- N+1 regression." + ) + + def test_activity_list_constant_in_distinct_activities(self): + """ActivityViewSet (distinct activities used in a course) must not grow + per activity.""" + teacher_role = RoleFactory(name="Teacher") + student_role = RoleFactory(name="Student") + + def build(num_activities): + course = CourseFactory() + teacher = UserFactory() + EnrollmentFactory(user=teacher, course=course, role=teacher_role) + piece = PieceFactory() + EnrollmentFactory(course=course, role=student_role) + for _ in range(num_activities): + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + # ActivityViewSet reads CourseAssignment for the course's distinct + # activities; one CA per activity. + CourseAssignment.objects.create( + course=course, activity=activity, piece=piece + ) + return course, teacher + + small_course, small_teacher = build(2) + large_course, large_teacher = build(20) + + client_small = APIClient() + client_small.force_authenticate(user=small_teacher) + with CaptureQueriesContext(connection) as ctx_s: + r_s = client_small.get(f"/api/courses/{small_course.slug}/activities/") + assert r_s.status_code == 200, r_s.content + + client_large = APIClient() + client_large.force_authenticate(user=large_teacher) + with CaptureQueriesContext(connection) as ctx_l: + r_l = client_large.get(f"/api/courses/{large_course.slug}/activities/") + assert r_l.status_code == 200, r_l.content + + assert len(ctx_s.captured_queries) == len(ctx_l.captured_queries), ( + f"activity list query count grows with #activities " + f"({len(ctx_s.captured_queries)} vs {len(ctx_l.captured_queries)})." + ) + + def test_grouped_assignments_constant_in_group_size(self): + """GroupSerializer.get_members (now reading GroupAssignment) must be + memoized: a group member's list query count must not grow with group size. + Groups render only in a student member's list (the teacher list has no + enrollment), so authenticate as a member.""" + student_role = RoleFactory(name="Student") + + def build_group_course(group_size): + course = CourseFactory() + group = AssignmentGroupFactory() + piece = PieceFactory() + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + ca = CourseAssignment.objects.create( + course=course, activity=activity, piece=piece + ) + members = [] + for _ in range(group_size): + enrollment = EnrollmentFactory(course=course, role=student_role) + GroupAssignment.objects.create( + group=group, enrollment=enrollment, course_assignment=ca + ) + members.append(enrollment) + # View as the first member. + return course, members[0].user + + small_course, small_viewer = build_group_course(2) + large_course, large_viewer = build_group_course(15) + + small = _count_list_queries(small_course, small_viewer) + large = _count_list_queries(large_course, large_viewer) + + assert small == large, ( + f"Grouped-assignment list query count grows with group size " + f"({small} vs {large}) -- GroupSerializer N+1 regression." + ) diff --git a/teleband/assignments/tests/test_student_list_flip.py b/teleband/assignments/tests/test_student_list_flip.py new file mode 100644 index 0000000..e5e780c --- /dev/null +++ b/teleband/assignments/tests/test_student_list_flip.py @@ -0,0 +1,152 @@ +"""Phase 2 step 7: AssignmentViewSet.list student path resolves from CourseAssignment. + +Covers the contract (ids are CourseAssignment ids, grouped by piece slug), the +late-joiner fix (a student with no Assignment rows still sees the course's +CourseAssignments), and telephone_fixed group scoping (grouped CourseAssignments +only appear for the enrollment named in their GroupAssignment). +""" + +import pytest +from rest_framework.test import APIClient + +from teleband.assignments.models import CourseAssignment +from teleband.assignments.tests.factories import ( + ActivityFactory, + AssignmentGroupFactory, + GroupAssignmentFactory, +) +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.users.tests.factories import RoleFactory + +pytestmark = pytest.mark.django_db + + +def _student(course): + return EnrollmentFactory(course=course, role=RoleFactory(name="Student")) + + +def _assign_piece(course): + """One CourseAssignment for a fresh piece/activity in `course`. Every enrolled + student is implicitly assigned it -- there are no per-student rows.""" + piece = PieceFactory() + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + ca = CourseAssignment.objects.create(course=course, activity=activity, piece=piece) + return ca, piece + + +def _list(enrollment): + client = APIClient() + client.force_authenticate(user=enrollment.user) + resp = client.get(f"/api/courses/{enrollment.course.slug}/assignments/") + assert resp.status_code == 200, resp.content + return resp.json() + + +def _all_ids(grouped): + return {item["id"] for items in grouped.values() for item in items} + + +def test_student_list_returns_course_assignment_ids_grouped_by_piece(): + course = CourseFactory() + student = _student(course) + ca1, piece1 = _assign_piece(course) + ca2, piece2 = _assign_piece(course) + + grouped = _list(student) + + assert set(grouped.keys()) == {piece1.slug, piece2.slug} + assert _all_ids(grouped) == {ca1.id, ca2.id} + assert grouped[piece1.slug][0]["piece_id"] == piece1.id + + +def test_student_retrieve_resolves_course_assignment_by_id(): + course = CourseFactory() + student = _student(course) + ca, piece = _assign_piece(course) + + client = APIClient() + client.force_authenticate(user=student.user) + resp = client.get(f"/api/courses/{course.slug}/assignments/{ca.id}/") + assert resp.status_code == 200, resp.content + body = resp.json() + assert body["id"] == ca.id + assert body["enrollment"]["id"] == student.id + + +def test_late_joiner_can_retrieve_course_assignment(): + course = CourseFactory() + _student(course) # early enrollee, assigned the piece below + ca, piece = _assign_piece(course) + late = _student(course) + + client = APIClient() + client.force_authenticate(user=late.user) + resp = client.get(f"/api/courses/{course.slug}/assignments/{ca.id}/") + assert resp.status_code == 200, resp.content + assert resp.json()["id"] == ca.id + + +def test_late_joiner_sees_course_assignments_without_assignment_rows(): + course = CourseFactory() + _student(course) # early enrollee + ca, piece = _assign_piece(course) + + # A student who enrolls after the piece was assigned still sees the course's + # CourseAssignment -- membership is by enrollment, not per-student rows. + late = _student(course) + grouped = _list(late) + + assert _all_ids(grouped) == {ca.id} + + +def test_group_members_payload_built_from_group_assignments(): + """GroupSerializer.get_members (now reading GroupAssignment) lists the group's + members with their submission status for the member viewing the list.""" + course = CourseFactory() + piece = PieceFactory() + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + ca = CourseAssignment.objects.create(course=course, activity=activity, piece=piece) + group = AssignmentGroupFactory() + a = _student(course) + b = _student(course) + for enr in (a, b): + GroupAssignmentFactory(group=group, enrollment=enr, course_assignment=ca) + + grouped = _list(a) + row = grouped[piece.slug][0] + members = row["group"]["members"] + by_id = {m["enrollment_id"]: m for m in members} + assert set(by_id) == {a.id, b.id} + assert by_id[a.id]["enrollment_username"] == a.user.username + assert by_id[a.id]["activity_type_name"] == activity.activity_type_name + assert all(m["assignment_submitted"] is False for m in members) + + +def test_grouped_course_assignments_are_scoped_to_their_enrollment(): + course = CourseFactory() + member = _student(course) + outsider = _student(course) + + normal_ca, normal_piece = _assign_piece(course) + + # A telephone_fixed CourseAssignment scoped to `member` via GroupAssignment. + grouped_piece = PieceFactory() + grouped_activity = ActivityFactory( + part_type=PartFactory(piece=grouped_piece).part_type + ) + grouped_ca = CourseAssignment.objects.create( + course=course, activity=grouped_activity, piece=grouped_piece + ) + group = AssignmentGroupFactory() + GroupAssignmentFactory(group=group, enrollment=member, course_assignment=grouped_ca) + + member_ids = _all_ids(_list(member)) + outsider_ids = _all_ids(_list(outsider)) + + # Member sees both the normal and the grouped CourseAssignment. + assert member_ids == {normal_ca.id, grouped_ca.id} + # Outsider sees only the normal one; the grouped CA is hidden. + assert outsider_ids == {normal_ca.id} diff --git a/teleband/assignments/tests/test_teacher_list_flip.py b/teleband/assignments/tests/test_teacher_list_flip.py new file mode 100644 index 0000000..6b85a21 --- /dev/null +++ b/teleband/assignments/tests/test_teacher_list_flip.py @@ -0,0 +1,113 @@ +"""Phase 2 step 7: AssignmentViewSet.list teacher path resolves from CourseAssignment. + +The teacher view returns one row per CourseAssignment (every assigned +(piece, activity)) instead of one per student -- verified against the frontend +(getAssignedPieces only derives the distinct (piece, activity) set per piece). +These tests pin that new cardinality, that the fields getAssignedPieces reads are +populated, and that per-student fields come back null/empty. +""" + +import pytest +from rest_framework.test import APIClient + +from teleband.assignments.models import CourseAssignment +from teleband.assignments.tests.factories import ActivityFactory +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.users.tests.factories import RoleFactory, UserFactory + +pytestmark = pytest.mark.django_db + + +def _teacher(course): + teacher = UserFactory() + EnrollmentFactory(user=teacher, course=course, role=RoleFactory(name="Teacher")) + return teacher + + +def _assign(course, activity, piece): + """One CourseAssignment for (course, activity, piece). Every enrolled student + is implicitly assigned it -- there are no per-student assignment rows.""" + return CourseAssignment.objects.create( + course=course, activity=activity, piece=piece + ) + + +def _list(course, teacher): + client = APIClient() + client.force_authenticate(user=teacher) + resp = client.get(f"/api/courses/{course.slug}/assignments/") + assert resp.status_code == 200, resp.content + return resp.json() + + +def test_teacher_list_is_one_row_per_course_assignment_not_per_student(): + course = CourseFactory() + teacher = _teacher(course) + # Multiple students enrolled: a per-student implementation would emit 4x rows. + for _ in range(4): + EnrollmentFactory(course=course, role=RoleFactory(name="Student")) + piece = PieceFactory() + part1 = PartFactory(piece=piece) + part2 = PartFactory(piece=piece) + act1 = ActivityFactory(part_type=part1.part_type) + act2 = ActivityFactory(part_type=part2.part_type) + ca1 = _assign(course, act1, piece) + ca2 = _assign(course, act2, piece) + + grouped = _list(course, teacher) + + # One group for the piece, with exactly two rows (one per CourseAssignment), + # NOT two-per-student. + assert set(grouped.keys()) == {piece.slug} + rows = grouped[piece.slug] + assert len(rows) == 2 + assert {r["id"] for r in rows} == {ca1.id, ca2.id} + + +def test_teacher_list_populates_getassignedpieces_fields(): + course = CourseFactory() + teacher = _teacher(course) + EnrollmentFactory(course=course, role=RoleFactory(name="Student")) + piece = PieceFactory() + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + ca = _assign(course, activity, piece) + + row = _list(course, teacher)[piece.slug][0] + + # Fields the frontend's getAssignedPieces reads: + assert row["piece_id"] == piece.id + assert row["piece_name"] == piece.name + assert row["piece_slug"] == piece.slug + assert row["activity_type_name"] == activity.activity_type_name + assert row["activity_type_category"] == activity.category + # Per-student fields are null/empty for the teacher (no enrollment context). + assert row["instrument"] is None + assert row["transposition"] is None + assert row["submissions"] == [] + assert row["group"] is None + + +def test_teacher_list_distinct_piece_activity_set_matches_assignments(): + """The (piece_slug, activity_type) set a teacher sees equals the distinct set + across all student assignments -- i.e. no assigned activity is lost by + collapsing per-student rows.""" + course = CourseFactory() + teacher = _teacher(course) + for _ in range(3): + EnrollmentFactory(course=course, role=RoleFactory(name="Student")) + piece = PieceFactory() + parts = [PartFactory(piece=piece) for _ in range(3)] + activities = [ActivityFactory(part_type=p.part_type) for p in parts] + for activity in activities: + _assign(course, activity, piece) + + grouped = _list(course, teacher) + seen = { + (slug, row["activity_type_name"]) + for slug, rows in grouped.items() + for row in rows + } + expected = {(piece.slug, a.activity_type_name) for a in activities} + assert seen == expected diff --git a/teleband/courses/api/views.py b/teleband/courses/api/views.py index 61ac32e..3243e83 100644 --- a/teleband/courses/api/views.py +++ b/teleband/courses/api/views.py @@ -29,13 +29,12 @@ EnrollmentInstrumentSerializer, RosterSerializer, ) -from teleband.assignments.api.serializers import AssignmentSerializer from teleband.users.api.serializers import UserSerializer from teleband.courses.models import Enrollment, Course from teleband.assignments.models import ( - Assignment, Activity, + CourseAssignment, PiecePlan, Curriculum, AssignmentGroup, @@ -110,7 +109,15 @@ def get_queryset(self, *args, **kwargs): ] return self.queryset.filter(course__in=courses) - return self.queryset.filter(user=self.request.user) + # EnrollmentSerializer nests course->owner, instrument->transposition, + # role, and user (UserSerializer -> groups). + return ( + self.queryset.filter(user=self.request.user) + .select_related( + "course__owner", "instrument__transposition", "role", "user" + ) + .prefetch_related("course__owner__groups", "user__groups") + ) def get_serializer_class(self): if self.action == "update" or self.action == "partial_update": @@ -198,7 +205,10 @@ def roster(self, request, **kwargs): break response["created"].append( User.objects.create_user( - name=name, username=new_username, password=password, grade=grade + name=name, + username=new_username, + password=password, + grade=grade, ) ) except User.DoesNotExist: @@ -212,21 +222,28 @@ def roster(self, request, **kwargs): role = Role.objects.get(name="Student") enrollments = collections.defaultdict(list) - for key in ["created", "existing"]: - for user in response[key]: - try: - enrollments["existing"].append( - Enrollment.objects.get(user=user, course=course) - ) - except Enrollment.DoesNotExist: - enrollments["created"].append( - Enrollment.objects.create( - user=user, - course=course, - instrument=user.instrument, - role=role, - ) + # Resolve existing enrollments in one query, then bulk_create the new + # ones instead of a get()+create() per user. + all_users = response["created"] + response["existing"] + existing_by_user = { + e.user_id: e + for e in Enrollment.objects.filter(course=course, user__in=all_users) + } + to_create = [] + for user in all_users: + existing = existing_by_user.get(user.id) + if existing is not None: + enrollments["existing"].append(existing) + else: + to_create.append( + Enrollment( + user=user, + course=course, + instrument=user.instrument, + role=role, ) + ) + enrollments["created"] = Enrollment.objects.bulk_create(to_create) response["created"] = UserSerializer( response["created"], many=True, context={"request": request} @@ -245,8 +262,14 @@ def roster(self, request, **kwargs): data={"users": response, "enrollments": enrollments}, ) - # must be a GET, respond with all enrollments for this class - course_enrollments = Enrollment.objects.filter(course=self.get_object()) + # must be a GET, respond with all enrollments for this class. + # RosterSerializer walks user (-> groups), instrument -> transposition, + # and role, so pull them in one shot. + course_enrollments = ( + Enrollment.objects.filter(course=self.get_object()) + .select_related("user", "instrument__transposition", "role") + .prefetch_related("user__groups") + ) serializer = RosterSerializer( course_enrollments, many=True, context={"request": request} ) @@ -300,10 +323,11 @@ def assign_piece_plan(self, request, **kwargs): }, ) - serializer = AssignmentSerializer( - assignments, many=True, context={"request": request} + # Phase 2: assign_* returns CourseAssignment/GroupAssignment rows; the + # frontend ignores this body (it refetches the list), so return a count. + return Response( + status=status.HTTP_200_OK, data={"assigned": len(assignments)} ) - return Response(status=status.HTTP_200_OK, data=serializer.data) @action(detail=True, methods=["post"]) def assign(self, request, **kwargs): @@ -342,10 +366,10 @@ def assign(self, request, **kwargs): assignments = assign_all_piece_activities(course, piece) - serializer = AssignmentSerializer( - assignments, many=True, context={"request": request} - ) - return Response(status=status.HTTP_200_OK, data=serializer.data) + # Phase 2: assign_* now returns CourseAssignment/GroupAssignment rows, not + # per-student Assignments. The frontend ignores this body (it refetches the + # list), so return a simple count. + return Response(status=status.HTTP_200_OK, data={"assigned": len(assignments)}) @action(detail=True, methods=["post"]) def assign_curriculum(self, request, **kwargs): @@ -386,10 +410,10 @@ def assign_curriculum(self, request, **kwargs): assignments = assign_curriculum(course, curriculum) - serializer = AssignmentSerializer( - assignments, many=True, context={"request": request} - ) - return Response(status=status.HTTP_200_OK, data=serializer.data) + # Phase 2: assign_* now returns CourseAssignment/GroupAssignment rows, not + # per-student Assignments. The frontend ignores this body (it refetches the + # list), so return a simple count. + return Response(status=status.HTTP_200_OK, data={"assigned": len(assignments)}) @action(detail=True, methods=["post"]) def unassign(self, request, **kwargs): @@ -412,8 +436,12 @@ def unassign(self, request, **kwargs): try: with transaction.atomic(): - Assignment.objects.filter( - part__piece_id=parsed["piece_id"], enrollment__course=course + # Phase 2: unassigning a piece removes its CourseAssignments + # (GroupAssignments cascade). A piece with submissions is PROTECTed, + # which surfaces as the IntegrityError handled below -- same guard + # the per-student Assignment delete had. + CourseAssignment.objects.filter( + piece_id=parsed["piece_id"], course=course ).delete() except IntegrityError: logger.error( @@ -458,9 +486,11 @@ def change_piece_instrument(self, request, **kwargs): instrument = Instrument.objects.get(pk=instrument_id) piece = Piece.objects.get(pk=piece_id) - assignments = Assignment.objects.filter(piece=piece, enrollment__course=course) - for assignment in assignments: - assignment.instrument = instrument - assignment.save() + # Phase 2: the per-piece instrument override lives on CourseAssignment + # (course-level, applied to every student for that piece). One UPDATE + # across the piece's CourseAssignments; resolve_instrument prefers it. + CourseAssignment.objects.filter(piece=piece, course=course).update( + instrument=instrument + ) return Response(status=status.HTTP_200_OK) diff --git a/teleband/courses/helper.py b/teleband/courses/helper.py index 185695e..39396a5 100644 --- a/teleband/courses/helper.py +++ b/teleband/courses/helper.py @@ -1,11 +1,11 @@ -from django.db import IntegrityError from teleband.courses.models import Enrollment, Course -from teleband.musics.models import Piece, Part +from teleband.musics.models import Piece from teleband.assignments.models import ( Activity, ActivityType, - Assignment, AssignmentGroup, + CourseAssignment, + GroupAssignment, PiecePlan, ) import random @@ -21,25 +21,16 @@ def assign_all_piece_activities(course, piece, deadline=None): def assign_one_piece_activity(course, piece, activity, deadline=None, piece_plan=None): - assignments = [] - part = Part.for_activity(activity, piece) - for e in Enrollment.objects.filter(course=course, role__name="Student"): - try: - # TODO is it reasonable to make this update_or_create? - assn, assn_created = Assignment.objects.update_or_create( - activity=activity, - enrollment=e, - instrument=e.instrument if e.instrument else e.user.instrument, - part=part, - piece=piece, - piece_plan=piece_plan, - deadline=deadline, - ) - if assn_created: - assignments.append(assn) - except IntegrityError as e: - print(f"IntegrityError: {e}") - return assignments + # Phase 2: one CourseAssignment per (course, activity, piece) is the whole + # assignment -- every enrolled student is implicitly assigned it (resolved at + # read time). No per-student Assignment rows are created anymore. + course_assignment, _ = CourseAssignment.objects.update_or_create( + course=course, + activity=activity, + piece=piece, + defaults={"piece_plan": piece_plan, "deadline": deadline}, + ) + return [course_assignment] def assign_piece_plan(course, piece_plan, deadline=None): @@ -75,7 +66,11 @@ def assign_telephone_fixed(course, piece_plan, deadline=None): # split the enrollments into groups of num_activities at random # and then assign the excess enrollments to the last group - enrollments = list(Enrollment.objects.filter(course=course, role__name="Student")) + enrollments = list( + Enrollment.objects.filter(course=course, role__name="Student").select_related( + "instrument" + ) + ) random.shuffle(enrollments) final_group = [] if excess_enrollments == 0 else enrollments[-excess_enrollments:] groups = [ @@ -89,29 +84,43 @@ def assign_telephone_fixed(course, piece_plan, deadline=None): final_group += used_enrollments[0 : num_activities - excess_enrollments] groups.append(final_group) - # create an assignment group for each group of enrollments - # and then assign each enrollment to an activity in the piece plan - # and add the assignment to the assignment group. + # create one AssignmentGroup per group of enrollments, then assign each + # enrollment to an activity in the piece plan within that group. Activities + # and their parts are resolved once (not per group/assignment), and the + # groups and assignments are each written in a single bulk_create. piece = piece_plan.piece - assignments = [] - for group in groups: - assignment_group = AssignmentGroup.objects.create(type="telephone_fixed") - group_assignments = [] - for e, a in zip(group, piece_plan.activities.all()): - part = Part.for_activity(a, piece) - group_assignments.append( - Assignment.objects.create( - activity=a, - part=part, - enrollment=e, - instrument=e.instrument if e.instrument else e.user.instrument, - piece_plan=piece_plan, - piece=piece, + activities = list(piece_plan.activities.all()) + + # Phase 2: one CourseAssignment per activity for the course, plus a + # GroupAssignment per member restricting which student gets which activity. + course_assignment_by_activity = { + a.id: CourseAssignment.objects.update_or_create( + course=course, + activity=a, + piece=piece, + defaults={"piece_plan": piece_plan, "deadline": deadline}, + )[0] + for a in activities + } + + group_objs = AssignmentGroup.objects.bulk_create( + [AssignmentGroup(type="telephone_fixed") for _ in groups] + ) + + # Phase 2: each student's telephone assignment is a GroupAssignment linking + # them to one activity's CourseAssignment within their group. No per-student + # Assignment rows; part/instrument resolve at read time. + group_memberships = [] + for group, assignment_group in zip(groups, group_objs): + for e, a in zip(group, activities): + group_memberships.append( + GroupAssignment( group=assignment_group, + enrollment=e, + course_assignment=course_assignment_by_activity[a.id], ) ) - assignments += group_assignments - return assignments + return GroupAssignment.objects.bulk_create(group_memberships, ignore_conflicts=True) def assign_curriculum(course, curriculum, deadline=None): diff --git a/teleband/courses/migrations/0007_data_migration_stress_test_course.py b/teleband/courses/migrations/0007_data_migration_stress_test_course.py index 593e1ca..52913de 100644 --- a/teleband/courses/migrations/0007_data_migration_stress_test_course.py +++ b/teleband/courses/migrations/0007_data_migration_stress_test_course.py @@ -59,7 +59,9 @@ def stress_test_course(apps, schema_editor): # Copy the "ALL Curriculum" to this course sixth_grade = Course.objects.get(slug="6th-grade-band") - source_curriculum = Curriculum.objects.get(name="ALL Curriculum", course=sixth_grade) + source_curriculum = Curriculum.objects.get( + name="ALL Curriculum", course=sixth_grade + ) new_curriculum = Curriculum.objects.create( name="ALL Curriculum", ordered=source_curriculum.ordered, diff --git a/teleband/courses/tests/factories.py b/teleband/courses/tests/factories.py index 2c6de65..12503ae 100644 --- a/teleband/courses/tests/factories.py +++ b/teleband/courses/tests/factories.py @@ -12,8 +12,10 @@ class CourseFactory(DjangoModelFactory): name = Faker("color") owner = SubFactory(UserFactory) - start_date = LazyFunction(datetime.datetime.utcnow) - end_date = LazyFunction(datetime.datetime.utcnow) + # Course.start_date/end_date are DateFields; use dates (utcnow() is a datetime, + # which Postgres truncates on write but SQLite keeps, tripping DRF's DateField). + start_date = LazyFunction(lambda: datetime.datetime.utcnow().date()) + end_date = LazyFunction(lambda: datetime.datetime.utcnow().date()) class Meta: model = Course diff --git a/teleband/courses/tests/test_assign_write_counts.py b/teleband/courses/tests/test_assign_write_counts.py new file mode 100644 index 0000000..2e717a7 --- /dev/null +++ b/teleband/courses/tests/test_assign_write_counts.py @@ -0,0 +1,92 @@ +"""Write-count regression tests for assignment creation (Phase 1b). + +assign_one_piece_activity previously ran update_or_create per student (2 queries +each); now it bulk_creates the missing rows, so the query count is constant in +roster size and only the missing rows are written (idempotent re-assign). +""" + +import pytest +from django.test.utils import CaptureQueriesContext +from django.db import connection + +from teleband.assignments.models import CourseAssignment +from teleband.assignments.tests.factories import ActivityFactory +from teleband.courses.helper import assign_one_piece_activity +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.users.tests.factories import RoleFactory + +pytestmark = pytest.mark.django_db + + +def _course_with_students(num_students): + student_role = RoleFactory(name="Student") + course = CourseFactory() + for _ in range(num_students): + EnrollmentFactory(course=course, role=student_role) + return course + + +def _setup(num_students): + course = _course_with_students(num_students) + piece = PieceFactory() + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + return course, piece, activity + + +def test_assign_one_activity_query_count_constant_in_roster(): + small_course, small_piece, small_activity = _setup(2) + large_course, large_piece, large_activity = _setup(20) + + with CaptureQueriesContext(connection) as ctx_small: + assign_one_piece_activity(small_course, small_piece, small_activity) + with CaptureQueriesContext(connection) as ctx_large: + assign_one_piece_activity(large_course, large_piece, large_activity) + + assert len(ctx_small.captured_queries) == len(ctx_large.captured_queries), ( + f"assign query count grows with roster " + f"({len(ctx_small.captured_queries)} vs {len(ctx_large.captured_queries)}) " + f"-- write explosion." + ) + + +def test_assign_one_activity_creates_single_course_level_assignment(): + # Phase 2: assigning creates a single course-level CourseAssignment; students + # are implicitly assigned, so there are no per-student rows. + course, piece, activity = _setup(5) + created = assign_one_piece_activity(course, piece, activity) + assert len(created) == 1 + assert ( + CourseAssignment.objects.filter( + course=course, activity=activity, piece=piece + ).count() + == 1 + ) + + +def test_assign_one_activity_is_idempotent(): + course, piece, activity = _setup(5) + assign_one_piece_activity(course, piece, activity) + # Re-assigning the same piece activity must not duplicate or error. + assign_one_piece_activity(course, piece, activity) + assert ( + CourseAssignment.objects.filter( + course=course, activity=activity, piece=piece + ).count() + == 1 + ) + + +def test_assign_one_activity_creates_single_course_assignment(): + # Phase 2: one CourseAssignment per (course, activity, piece) regardless of + # roster size, and idempotent on re-assign. + course, piece, activity = _setup(5) + assign_one_piece_activity(course, piece, activity, piece_plan=None) + assign_one_piece_activity(course, piece, activity, piece_plan=None) + assert ( + CourseAssignment.objects.filter( + course=course, activity=activity, piece=piece + ).count() + == 1 + ) diff --git a/teleband/courses/tests/test_change_instrument.py b/teleband/courses/tests/test_change_instrument.py new file mode 100644 index 0000000..ab2b8b1 --- /dev/null +++ b/teleband/courses/tests/test_change_instrument.py @@ -0,0 +1,93 @@ +"""Write-count + behavior test for change_piece_instrument (Phase 1b #17).""" + +import pytest +from django.test.utils import CaptureQueriesContext +from django.db import connection +from rest_framework.test import APIClient + +from teleband.assignments.models import CourseAssignment +from teleband.assignments.tests.factories import ActivityFactory +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.instruments.tests.factories import InstrumentFactory +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.users.tests.factories import RoleFactory, UserFactory + +pytestmark = pytest.mark.django_db + + +def _build(num_students): + teacher_role = RoleFactory(name="Teacher") + student_role = RoleFactory(name="Student") + course = CourseFactory(can_edit_instruments=True) + teacher = UserFactory() + EnrollmentFactory(user=teacher, course=course, role=teacher_role) + piece = PieceFactory() + for _ in range(num_students): + part = PartFactory(piece=piece) + EnrollmentFactory(course=course, role=student_role) + activity = ActivityFactory(part_type=part.part_type) + # Phase 2: change_piece_instrument updates CourseAssignment.instrument. + CourseAssignment.objects.create(course=course, activity=activity, piece=piece) + return course, teacher, piece + + +def _patch(course, teacher, piece, instrument): + client = APIClient() + client.force_authenticate(user=teacher) + with CaptureQueriesContext(connection) as ctx: + resp = client.patch( + f"/api/courses/{course.slug}/change_piece_instrument/", + {"piece_id": piece.id, "instrument_id": instrument.id}, + format="json", + ) + assert resp.status_code == 200, resp.content + return len(ctx.captured_queries) + + +def test_change_instrument_query_count_constant_in_roster(): + new_instrument = InstrumentFactory() + small_course, small_teacher, small_piece = _build(2) + large_course, large_teacher, large_piece = _build(20) + + small = _patch(small_course, small_teacher, small_piece, new_instrument) + large = _patch(large_course, large_teacher, large_piece, new_instrument) + + assert small == large, ( + f"change_piece_instrument query count grows with roster " + f"({small} vs {large}) -- per-row save() not collapsed to one UPDATE." + ) + + +def test_change_instrument_updates_all_course_assignments(): + new_instrument = InstrumentFactory() + course, teacher, piece = _build(4) + _patch(course, teacher, piece, new_instrument) + instruments = set( + CourseAssignment.objects.filter(course=course, piece=piece).values_list( + "instrument_id", flat=True + ) + ) + assert instruments == {new_instrument.id} + + +def test_change_instrument_flows_to_student_resolved_instrument(): + """After the teacher overrides the piece's instrument, a student's list shows + that instrument (resolve_instrument prefers CourseAssignment.instrument).""" + new_instrument = InstrumentFactory() + course = CourseFactory(can_edit_instruments=True) + teacher = UserFactory() + EnrollmentFactory(user=teacher, course=course, role=RoleFactory(name="Teacher")) + piece = PieceFactory() + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + student = EnrollmentFactory(course=course, role=RoleFactory(name="Student")) + CourseAssignment.objects.create(course=course, activity=activity, piece=piece) + + _patch(course, teacher, piece, new_instrument) + + client = APIClient() + client.force_authenticate(user=student.user) + resp = client.get(f"/api/courses/{course.slug}/assignments/") + assert resp.status_code == 200, resp.content + row = resp.json()[piece.slug][0] + assert row["instrument"] == new_instrument.name diff --git a/teleband/courses/tests/test_query_counts.py b/teleband/courses/tests/test_query_counts.py new file mode 100644 index 0000000..be90dc6 --- /dev/null +++ b/teleband/courses/tests/test_query_counts.py @@ -0,0 +1,66 @@ +"""Query-count regression tests for course/enrollment list endpoints (Phase 1a).""" + +import pytest +from django.test.utils import CaptureQueriesContext +from django.db import connection +from rest_framework.test import APIClient + +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.users.tests.factories import RoleFactory, UserFactory + +pytestmark = pytest.mark.django_db + + +def _count(url, user): + client = APIClient() + client.force_authenticate(user=user) + with CaptureQueriesContext(connection) as ctx: + response = client.get(url) + assert response.status_code == 200, response.content + return len(ctx.captured_queries) + + +def test_roster_constant_in_member_count(): + """CourseViewSet.roster GET must not grow per enrolled member.""" + teacher_role = RoleFactory(name="Teacher") + student_role = RoleFactory(name="Student") + + def build(num_students): + course = CourseFactory() + teacher = UserFactory() + EnrollmentFactory(user=teacher, course=course, role=teacher_role) + for _ in range(num_students): + EnrollmentFactory(course=course, role=student_role) + return course, teacher + + small_course, small_teacher = build(2) + large_course, large_teacher = build(20) + + small = _count(f"/api/courses/{small_course.slug}/roster/", small_teacher) + large = _count(f"/api/courses/{large_course.slug}/roster/", large_teacher) + + assert ( + small == large + ), f"roster query count grows with #members ({small} vs {large}) -- N+1." + + +def test_enrollment_list_constant_in_enrollment_count(): + """EnrollmentViewSet.list must not grow per enrollment of the user.""" + role = RoleFactory(name="Student") + + def make_user_with_enrollments(n): + user = UserFactory() + for _ in range(n): + EnrollmentFactory(user=user, course=CourseFactory(), role=role) + return user + + few_user = make_user_with_enrollments(2) + many_user = make_user_with_enrollments(20) + + few = _count("/api/enrollments/", few_user) + many = _count("/api/enrollments/", many_user) + + assert few == many, ( + f"enrollment list query count grows with #enrollments " + f"({few} vs {many}) -- N+1 regression." + ) diff --git a/teleband/courses/tests/test_roster_import.py b/teleband/courses/tests/test_roster_import.py new file mode 100644 index 0000000..6385165 --- /dev/null +++ b/teleband/courses/tests/test_roster_import.py @@ -0,0 +1,88 @@ +"""Roster CSV import tests (Phase 1b #19). + +The enrollment creation now bulk_creates the new enrollments after a single +existence query, instead of a get()+create() per user. User creation itself +stays per-row (password hashing can't be bulked), so these assert correctness +and idempotency rather than a constant query count. +""" + +import pytest +from django.contrib.auth import get_user_model +from django.core.files.uploadedfile import SimpleUploadedFile +from rest_framework.test import APIClient + +from teleband.courses.models import Enrollment +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.users.models import Role +from teleband.users.tests.factories import UserFactory + +pytestmark = pytest.mark.django_db +User = get_user_model() + + +def _csv(rows): + body = "fullname,username,password,grade\n" + "".join( + f"{name},{username},{pw},{grade}\n" for name, username, pw, grade in rows + ) + return SimpleUploadedFile( + "roster.csv", body.encode("utf-8"), content_type="text/csv" + ) + + +def _post_roster(course, teacher, rows): + client = APIClient() + client.force_authenticate(user=teacher) + return client.post( + f"/api/courses/{course.slug}/roster/", + {"file": _csv(rows)}, + format="multipart", + ) + + +def _course_with_teacher(): + # Use the seeded roles (the roster view does Role.objects.get(name="Student"), + # which requires exactly one Student role -- don't create a duplicate). + teacher_role, _ = Role.objects.get_or_create(name="Teacher") + Role.objects.get_or_create(name="Student") + course = CourseFactory() + teacher = UserFactory() + EnrollmentFactory(user=teacher, course=course, role=teacher_role) + return course, teacher + + +def test_roster_import_creates_users_and_enrollments(): + course, teacher = _course_with_teacher() + rows = [ + ("Alice A", "alice", "alicepass1", "5"), + ("Bob B", "bob", "bobpass1", "6"), + ("Cara C", "cara", "carapass1", "7"), + ] + resp = _post_roster(course, teacher, rows) + assert resp.status_code == 200, resp.content + + for _, username, _, _ in rows: + assert User.objects.filter(username=username).exists() + assert Enrollment.objects.filter( + course=course, user__username=username, role__name="Student" + ).exists() + # 3 students + the teacher. + assert Enrollment.objects.filter(course=course).count() == 4 + + +def test_roster_import_is_idempotent_on_reupload(): + course, teacher = _course_with_teacher() + rows = [ + ("Alice A", "alice", "alicepass1", "5"), + ("Bob B", "bob", "bobpass1", "6"), + ] + _post_roster(course, teacher, rows) + first = Enrollment.objects.filter(course=course, role__name="Student").count() + + resp = _post_roster(course, teacher, rows) + assert resp.status_code == 200, resp.content + # Re-uploading the same roster must not duplicate enrollments. + second = Enrollment.objects.filter(course=course, role__name="Student").count() + assert first == second == 2 + # The reupload reports them as existing, not created. + assert len(resp.data["enrollments"]["created"]) == 0 + assert len(resp.data["enrollments"]["existing"]) == 2 diff --git a/teleband/courses/tests/test_telephone_write_counts.py b/teleband/courses/tests/test_telephone_write_counts.py new file mode 100644 index 0000000..434a9ca --- /dev/null +++ b/teleband/courses/tests/test_telephone_write_counts.py @@ -0,0 +1,98 @@ +"""Write-count + behavior tests for assign_telephone_fixed (Phase 1b #18, #21). + +The telephone_fixed path created an AssignmentGroup and an Assignment per row, +re-derived Part inside the loop, and re-evaluated activities.all() per group. It +now resolves activities/parts once and bulk_creates groups and assignments, so +the query count is constant in roster size. +""" + +import pytest +from django.test.utils import CaptureQueriesContext +from django.db import connection + +from teleband.assignments.models import ( + AssignmentGroup, + CourseAssignment, + GroupAssignment, + PiecePlan, + PlannedActivity, +) +from teleband.assignments.tests.factories import ActivityFactory +from teleband.courses.helper import assign_telephone_fixed +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.users.tests.factories import RoleFactory + +pytestmark = pytest.mark.django_db + +NUM_ACTIVITIES = 3 + + +def _telephone_plan(piece): + plan = PiecePlan.objects.create(name="tele", piece=piece, type="telephone_fixed") + for order in range(NUM_ACTIVITIES): + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + PlannedActivity.objects.create(piece_plan=plan, activity=activity, order=order) + return plan + + +def _setup(num_students): + student_role = RoleFactory(name="Student") + course = CourseFactory() + for _ in range(num_students): + EnrollmentFactory(course=course, role=student_role) + piece = PieceFactory() + plan = _telephone_plan(piece) + return course, plan + + +def test_telephone_query_count_constant_in_roster(): + # Multiples of NUM_ACTIVITIES so grouping is even in both cases. + small_course, small_plan = _setup(NUM_ACTIVITIES * 2) + large_course, large_plan = _setup(NUM_ACTIVITIES * 10) + + with CaptureQueriesContext(connection) as ctx_small: + assign_telephone_fixed(small_course, small_plan) + with CaptureQueriesContext(connection) as ctx_large: + assign_telephone_fixed(large_course, large_plan) + + assert len(ctx_small.captured_queries) == len(ctx_large.captured_queries), ( + f"telephone assign query count grows with roster " + f"({len(ctx_small.captured_queries)} vs {len(ctx_large.captured_queries)}) " + f"-- per-row create/Part not batched." + ) + + +def test_telephone_creates_one_group_membership_per_student(): + num_students = NUM_ACTIVITIES * 4 + course, plan = _setup(num_students) + + before_groups = AssignmentGroup.objects.count() + created = assign_telephone_fixed(course, plan) + + # Phase 2: one GroupAssignment per student, one group per block of + # NUM_ACTIVITIES students (no per-student Assignment rows exist). + assert len(created) == num_students + assert ( + AssignmentGroup.objects.count() - before_groups + == num_students // NUM_ACTIVITIES + ) + assert all(ga.group_id is not None for ga in created) + + +def test_telephone_dual_writes_course_and_group_assignments(): + num_students = NUM_ACTIVITIES * 4 + course, plan = _setup(num_students) + assign_telephone_fixed(course, plan) + + # One CourseAssignment per activity in the plan (not per student). + assert ( + CourseAssignment.objects.filter(course=course, piece=plan.piece).count() + == NUM_ACTIVITIES + ) + # One GroupAssignment per student (each student gets exactly one activity). + assert ( + GroupAssignment.objects.filter(course_assignment__course=course).count() + == num_students + ) diff --git a/teleband/courses/tests/test_unassign.py b/teleband/courses/tests/test_unassign.py new file mode 100644 index 0000000..4cb1fa9 --- /dev/null +++ b/teleband/courses/tests/test_unassign.py @@ -0,0 +1,38 @@ +"""Phase 2: unassign removes a piece's CourseAssignments (was per-student Assignments).""" + +import pytest +from rest_framework.test import APIClient + +from teleband.assignments.models import CourseAssignment +from teleband.assignments.tests.factories import ActivityFactory +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.users.tests.factories import RoleFactory, UserFactory + +pytestmark = pytest.mark.django_db + + +def test_unassign_deletes_course_assignments_for_piece(): + course = CourseFactory() + teacher = UserFactory() + EnrollmentFactory(user=teacher, course=course, role=RoleFactory(name="Teacher")) + EnrollmentFactory(course=course, role=RoleFactory(name="Student")) + + piece = PieceFactory() + other_piece = PieceFactory() + for p in (piece, other_piece): + part = PartFactory(piece=p) + CourseAssignment.objects.create( + course=course, activity=ActivityFactory(part_type=part.part_type), piece=p + ) + + client = APIClient() + client.force_authenticate(user=teacher) + resp = client.post( + f"/api/courses/{course.slug}/unassign/", {"piece_id": piece.id}, format="json" + ) + assert resp.status_code == 200, resp.content + + assert not CourseAssignment.objects.filter(course=course, piece=piece).exists() + # Other pieces are untouched. + assert CourseAssignment.objects.filter(course=course, piece=other_piece).exists() diff --git a/teleband/dashboards/tests.py b/teleband/dashboards/tests.py deleted file mode 100644 index 7ce503c..0000000 --- a/teleband/dashboards/tests.py +++ /dev/null @@ -1,3 +0,0 @@ -from django.test import TestCase - -# Create your tests here. diff --git a/teleband/dashboards/tests/__init__.py b/teleband/dashboards/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/teleband/dashboards/tests/test_query_counts.py b/teleband/dashboards/tests/test_query_counts.py new file mode 100644 index 0000000..c4c7053 --- /dev/null +++ b/teleband/dashboards/tests/test_query_counts.py @@ -0,0 +1,64 @@ +"""Query-count regression test for the CSV export dashboard (Phase 1a #11). + +The CSV export streams the entire Assignment table unfiltered, so the right +guarantee is that it issues O(1) queries, not O(rows). The test DB is seeded with +thousands of assignments by a data migration; this asserts the export stays under +a small constant ceiling regardless, which only holds if every relation the row +loop (and the Activity/PiecePlan __str__) walks is select_related/prefetched. +""" + +import pytest +from django.test.utils import CaptureQueriesContext +from django.db import connection +from django.test import Client + +from teleband.assignments.models import CourseAssignment +from teleband.assignments.tests.factories import ActivityFactory +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.submissions.models import SubmissionAttachment +from teleband.submissions.tests.factories import SubmissionFactory +from teleband.users.tests.factories import RoleFactory + +pytestmark = pytest.mark.django_db + +# Generous ceiling: the export should be a handful of queries (base + one per +# prefetched relation, chunked). Anything near the row count is an N+1. +MAX_QUERIES = 25 + + +def test_csv_export_is_constant_query_count(): + # Phase 2: rows are (student, CourseAssignment) pairs. Build a course with + # students, course assignments, and submissions to exercise both the + # has-submissions and the unsubmitted branches. + course = CourseFactory() + student_role = RoleFactory(name="Student") + piece = PieceFactory() + students = [EnrollmentFactory(course=course, role=student_role) for _ in range(5)] + for _ in range(4): + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + ca = CourseAssignment.objects.create( + course=course, activity=activity, piece=piece + ) + for enrollment in students[:2]: + submission = SubmissionFactory( + course_assignment=ca, + enrollment=enrollment, + instrument=enrollment.instrument, + part=part, + ) + SubmissionAttachment.objects.create(submission=submission, file="a.wav") + + client = Client() + with CaptureQueriesContext(connection) as ctx: + response = client.get("/dashboards/export/csv/") + assert response.status_code == 200 + # 5 students x 4 course assignments = 20 per-student rows in the body. + assert response.content.decode().count("\n") >= 20 + + n = len(ctx.captured_queries) + assert n <= MAX_QUERIES, ( + f"CSV export issued {n} queries (ceiling {MAX_QUERIES}) -- N+1 regression: " + f"the export should be O(1) in queries, not O(rows)." + ) diff --git a/teleband/dashboards/views.py b/teleband/dashboards/views.py index c9d62c9..c4e6cd7 100644 --- a/teleband/dashboards/views.py +++ b/teleband/dashboards/views.py @@ -1,40 +1,103 @@ -from typing import Any -from django.db.models.query import QuerySet -from django.shortcuts import render - -from django.views import generic +import csv +from collections import defaultdict +from types import SimpleNamespace -from teleband.assignments.models import Assignment -from teleband.courses.models import Course from django.contrib.auth.mixins import UserPassesTestMixin - -import csv from django.http import HttpResponse +from django.views import generic - -class AssignmentListView(UserPassesTestMixin, generic.ListView): - model = Assignment - - def get_queryset(self) -> QuerySet[Any]: - results = Assignment.objects.prefetch_related( +from teleband.assignments.api.serializers import resolve_instrument +from teleband.assignments.models import CourseAssignment, GroupAssignment +from teleband.courses.models import Course, Enrollment +from teleband.submissions.models import Submission + + +def build_assignment_rows(): + """Phase 2: the per-student assignment table is gone. Reconstruct one row per + (student, CourseAssignment) -- every enrolled student is implicitly assigned + every non-grouped CourseAssignment in their course, plus the grouped + (telephone_fixed) ones their GroupAssignment names. Each row exposes the same + attributes the old Assignment rows did (enrollment, piece, piece_plan, activity, + instrument, submissions), so the CSV and the template are unchanged in shape. + + All data is fetched in a constant number of queries (no per-row N+1). + """ + course_assignments = list( + CourseAssignment.objects.select_related( + "course", + "activity", + "activity__activity_type", + "activity__part_type", "piece", "piece_plan", - "enrollment", - "enrollment__user", - "enrollment__course", - "enrollment__instrument", - "enrollment__course__owner", + "piece_plan__piece", "instrument", - "submissions__attachments", - "submissions__grade", - "submissions__self_grade", - "activity", - ).all() - return results + ) + ) + enrollments = list( + Enrollment.objects.filter(role__name="Student").select_related( + "user", "course", "instrument" + ) + ) + + cas_by_course = defaultdict(list) + for ca in course_assignments: + cas_by_course[ca.course_id].append(ca) + enr_by_course = defaultdict(list) + for e in enrollments: + enr_by_course[e.course_id].append(e) + + grouped_ca_ids = set( + GroupAssignment.objects.values_list("course_assignment_id", flat=True) + ) + member_pairs = set( + GroupAssignment.objects.values_list("course_assignment_id", "enrollment_id") + ) + + subs_by_pair = defaultdict(list) + for s in Submission.objects.select_related("grade", "self_grade").prefetch_related( + "attachments" + ): + subs_by_pair[(s.course_assignment_id, s.enrollment_id)].append(s) + + rows = [] + for course_id, course_cas in cas_by_course.items(): + for enrollment in enr_by_course.get(course_id, []): + for ca in course_cas: + # Skip grouped CourseAssignments this student isn't a member of. + if ( + ca.id in grouped_ca_ids + and (ca.id, enrollment.id) not in member_pairs + ): + continue + rows.append( + SimpleNamespace( + id=ca.id, + enrollment=enrollment, + piece=ca.piece, + piece_plan=ca.piece_plan, + activity=ca.activity, + instrument=resolve_instrument(enrollment, ca), + submissions=subs_by_pair.get((ca.id, enrollment.id), []), + ) + ) + return rows + + +def _id(obj): + return obj.id if obj is not None else "N/A" + + +def _name(obj): + return obj.name if obj is not None else "N/A" - # queryset = Course.objects.prefetch_related( - # "enrollment_set__assignment_set__submissions__attachments" - # ).all() + +class AssignmentListView(UserPassesTestMixin, generic.ListView): + template_name = "assignments/assignment_list.html" + paginate_by = 100 + + def get_queryset(self): + return build_assignment_rows() def test_func(self): return self.request.user.is_superuser @@ -48,23 +111,9 @@ def test_func(self): def csv_view(request): - """Function which generates a CSV file for download""" - # select related returns a queryset that will follow foreign-key relationships. This - # is a performance booster which results in a single more complex query but won't require - # database queries - assignments = Assignment.objects.select_related( - "piece", - "piece_plan", - "enrollment", - "enrollment__user", - "enrollment__course", - "enrollment__instrument", - "enrollment__course__owner", - "instrument", - "activity", - ).all() - - # Create the HttpResponse object with the appropriate CSV header + """Generate the per-(student, CourseAssignment) CSV export for download.""" + rows = build_assignment_rows() + response = HttpResponse( content_type="text/csv", headers={"Content-Disposition": 'attachment; filename="assignment.csv"'}, @@ -97,69 +146,44 @@ def csv_view(request): "Submission Attachment Submitted", ] ) - for assn in assignments: - if len(assn.submissions.all()) == 0: - - writer.writerow( - [ - assn.id, - assn.enrollment.course.id, - assn.enrollment.course.name, - assn.piece.id, - assn.piece.name, - assn.piece_plan.id, - assn.piece_plan, - assn.enrollment.user.id, - assn.enrollment.instrument.id, - assn.enrollment.instrument.name, - assn.activity.id, - assn.activity, - assn.instrument.id, - assn.instrument.name, - "N/A", - "N/A", - "N/A", - "N/A", - "N/A", - "N/A", - "N/A", - "N/A", - ] - ) - else: - for sub in assn.submissions.all(): - for att in sub.attachments.all(): - csv_val = [ - assn.id, - assn.enrollment.course.id, - assn.enrollment.course.name, - assn.piece.id, - assn.piece.name, - assn.piece_plan.id, - assn.piece_plan, - assn.enrollment.user.id, - assn.enrollment.instrument.id, - assn.enrollment.instrument.name, - assn.activity.id, - assn.activity, - assn.instrument.id, - assn.instrument.name, + for assn in rows: + prefix = [ + assn.id, + assn.enrollment.course.id, + assn.enrollment.course.name, + assn.piece.id, + assn.piece.name, + _id(assn.piece_plan), + assn.piece_plan or "N/A", + assn.enrollment.user.id, + _id(assn.enrollment.instrument), + _name(assn.enrollment.instrument), + assn.activity.id, + assn.activity, + _id(assn.instrument), + _name(assn.instrument), + ] + if len(assn.submissions) == 0: + writer.writerow(prefix + ["N/A"] * 8) + continue + for sub in assn.submissions: + for att in sub.attachments.all(): + content = ( + "Create, see below" + if assn.activity.category == "Create" + else sub.content + ) + writer.writerow( + prefix + + [ sub.id, + content, + sub.submitted, + sub.grade, + sub.self_grade, + att.id, + att.file, + att.submitted, ] - if assn.activity.category == "Create": - csv_val.append("Create, see below") - else: - csv_val.append(sub.content) - csv_val.extend( - [ - sub.submitted, - sub.grade, - sub.self_grade, - att.id, - att.file, - att.submitted, - ] - ) - - writer.writerow(csv_val) + ) return response diff --git a/teleband/musics/migrations/0019_auto_20230911_seed_beginning_band.py b/teleband/musics/migrations/0019_auto_20230911_seed_beginning_band.py index 63d5f5a..db20e35 100644 --- a/teleband/musics/migrations/0019_auto_20230911_seed_beginning_band.py +++ b/teleband/musics/migrations/0019_auto_20230911_seed_beginning_band.py @@ -4,7 +4,6 @@ import json from teleband.utils.migration_helpers import create_part_et_al, create_piece_et_al - data = [ { "name": "Beginning Band - When the Saints Go Marching In", diff --git a/teleband/musics/models.py b/teleband/musics/models.py index 7cf24db..3ee3ce8 100644 --- a/teleband/musics/models.py +++ b/teleband/musics/models.py @@ -63,17 +63,17 @@ class Part(models.Model): chord_scale_pattern = models.JSONField(blank=True, null=True) def for_activity(activity, piece): - # Get this piece’s part for this kind of activity - kwargs = {"piece": piece} - if ( - activity.part_type - and piece.parts.filter(part_type=activity.part_type).exists() - ): - kwargs["part_type"] = activity.part_type - # TODO: should we have an else for when it's null? I think so, here it is. - else: - kwargs["part_type"] = PartType.objects.get(name="Melody") - return Part.objects.get(**kwargs) + # Get this piece's part for this kind of activity, falling back to the + # piece's Melody part. A single get() with a DoesNotExist fallback + # replaces the old exists()+get() double query. + if activity.part_type: + try: + return Part.objects.get(piece=piece, part_type=activity.part_type) + except Part.DoesNotExist: + pass + return Part.objects.get( + piece=piece, part_type=PartType.objects.get(name="Melody") + ) def __str__(self): return self.name diff --git a/teleband/musics/tests/factories.py b/teleband/musics/tests/factories.py index 473c9c1..cd25662 100644 --- a/teleband/musics/tests/factories.py +++ b/teleband/musics/tests/factories.py @@ -1,7 +1,7 @@ from factory import Faker, SubFactory from factory.django import DjangoModelFactory -from teleband.musics.models import EnsembleType, Piece +from teleband.musics.models import Composer, EnsembleType, Part, PartType, Piece class EnsembleTypeFactory(DjangoModelFactory): @@ -12,10 +12,37 @@ class Meta: model = EnsembleType +class ComposerFactory(DjangoModelFactory): + + name = Faker("name") + + class Meta: + model = Composer + + class PieceFactory(DjangoModelFactory): name = Faker("name") ensemble_type = SubFactory(EnsembleTypeFactory) + composer = SubFactory(ComposerFactory) class Meta: model = Piece + + +class PartTypeFactory(DjangoModelFactory): + + name = Faker("word") + + class Meta: + model = PartType + + +class PartFactory(DjangoModelFactory): + + name = Faker("word") + part_type = SubFactory(PartTypeFactory) + piece = SubFactory(PieceFactory) + + class Meta: + model = Part diff --git a/teleband/submissions/admin.py b/teleband/submissions/admin.py index 22d5ba0..b4ab24f 100644 --- a/teleband/submissions/admin.py +++ b/teleband/submissions/admin.py @@ -8,11 +8,12 @@ class SubmissionAdmin(VersionAdmin): list_display = ( "id", - "assignment", + "course_assignment", + "enrollment", "submitted", ) - list_filter = ("assignment__piece",) - raw_id_fields = ("assignment",) + list_filter = ("course_assignment__piece",) + raw_id_fields = ("course_assignment", "enrollment", "instrument", "part") @admin.register(SubmissionAttachment) @@ -42,14 +43,15 @@ class GradeAdmin(VersionAdmin): class ActivityProgressAdmin(VersionAdmin): list_display = ( "id", - "assignment", + "course_assignment", + "enrollment", "current_step", "participant_email", "created_at", "updated_at", ) list_filter = ("current_step", "created_at") - search_fields = ("participant_email", "assignment__id") + search_fields = ("participant_email", "course_assignment__id") readonly_fields = ( "activity_logs", "step_completions", @@ -59,4 +61,4 @@ class ActivityProgressAdmin(VersionAdmin): "created_at", "updated_at", ) - raw_id_fields = ("assignment",) + raw_id_fields = ("course_assignment", "enrollment") diff --git a/teleband/submissions/api/serializers.py b/teleband/submissions/api/serializers.py index c5822e8..6d2e492 100644 --- a/teleband/submissions/api/serializers.py +++ b/teleband/submissions/api/serializers.py @@ -1,6 +1,11 @@ from rest_framework import serializers -from teleband.submissions.models import Grade, Submission, SubmissionAttachment, ActivityProgress +from teleband.submissions.models import ( + Grade, + Submission, + SubmissionAttachment, + ActivityProgress, +) # from teleband.assignments.api.serializers import AssignmentSerializer @@ -51,7 +56,8 @@ class Meta: model = ActivityProgress fields = [ "id", - "assignment", + "course_assignment", + "enrollment", "current_step", "step_completions", "activity_logs", diff --git a/teleband/submissions/api/teacher_serializers.py b/teleband/submissions/api/teacher_serializers.py index ac5f179..de3cb71 100644 --- a/teleband/submissions/api/teacher_serializers.py +++ b/teleband/submissions/api/teacher_serializers.py @@ -1,19 +1,67 @@ from rest_framework import serializers -from teleband.submissions.api.serializers import AttachmentSerializer, GradeSerializer -from teleband.assignments.api.serializers import AssignmentSerializer + +from teleband.assignments.api.serializers import ActivitySerializer +from teleband.courses.api.serializers import EnrollmentSerializer +from teleband.instruments.api.serializers import InstrumentSerializer +from teleband.musics.api.serializers import PartSerializer +from teleband.submissions.api.serializers import ( + AttachmentSerializer, + GradeSerializer, + SubmissionSerializer, +) from teleband.submissions.models import Submission +class SubmissionAssignmentSerializer(serializers.Serializer): + """Phase 2: renders the legacy AssignmentSerializer shape for a Submission from + its own fields (course_assignment / enrollment / instrument / part), replacing + the dropped Submission.assignment FK. The teacher grading view only reads + ``enrollment.user.name`` off this object, but the full shape is preserved. + + The nested ``submissions`` list and ``group`` are resolved from per-(course + assignment, enrollment) maps the view precomputes (context ``submissions_by_pair`` + / ``group_by_pair``), so serialization stays constant in the number of students. + """ + + def to_representation(self, submission): + ca = submission.course_assignment + pair = (submission.course_assignment_id, submission.enrollment_id) + submissions = self.context.get("submissions_by_pair", {}).get(pair, []) + group_id = self.context.get("group_by_pair", {}).get(pair) + return { + "activity": ActivitySerializer(ca.activity, context=self.context).data, + "deadline": ( + serializers.DateField().to_representation(ca.deadline) + if ca.deadline + else None + ), + "instrument": ( + InstrumentSerializer(submission.instrument, context=self.context).data + if submission.instrument + else None + ), + "part": ( + PartSerializer(submission.part, context=self.context).data + if submission.part + else None + ), + "id": ca.id, + "enrollment": EnrollmentSerializer( + submission.enrollment, context=self.context + ).data, + "submissions": SubmissionSerializer( + submissions, many=True, context=self.context + ).data, + "group": group_id, + } + + class TeacherSubmissionSerializer(serializers.ModelSerializer): attachments = AttachmentSerializer(read_only=True, many=True) - assignment = AssignmentSerializer() + assignment = SubmissionAssignmentSerializer(source="*") grade = GradeSerializer() self_grade = GradeSerializer() - def get_attachments(self, queryset): - print(queryset) - return None - class Meta: model = Submission fields = [ @@ -25,7 +73,3 @@ class Meta: "grade", "self_grade", ] - - # extra_kwargs = { - # "assignment": {"view_name": "api:assignment-detail", "lookup_field": "id"}, - # } diff --git a/teleband/submissions/api/views.py b/teleband/submissions/api/views.py index 14b3718..04d3ab5 100644 --- a/teleband/submissions/api/views.py +++ b/teleband/submissions/api/views.py @@ -1,6 +1,9 @@ +from collections import defaultdict + from django.contrib.auth import get_user_model from django.db import transaction from django.db.models import OuterRef, Subquery +from django.shortcuts import get_object_or_404 from rest_framework import status from rest_framework.decorators import action from rest_framework.mixins import ListModelMixin, RetrieveModelMixin, CreateModelMixin @@ -16,23 +19,73 @@ ) from teleband.courses.models import Course -from teleband.submissions.models import Grade, Submission, SubmissionAttachment, ActivityProgress -from teleband.assignments.models import Assignment +from teleband.submissions.models import ( + Grade, + Submission, + SubmissionAttachment, + ActivityProgress, +) +from teleband.assignments.models import CourseAssignment, GroupAssignment +from teleband.assignments.api.serializers import resolve_instrument +from teleband.musics.models import Part from datetime import datetime +def resolve_student_target(request, course_slug, course_assignment_id): + """Resolve the (course_assignment, enrollment) a student's nested route refers + to. Phase 2: the URL id is a CourseAssignment id (the list contract), scoped to + the requesting user's enrollment in the course. Returns (course_assignment, + enrollment); raises Http404 if the student has no enrollment in the course or + no such CourseAssignment.""" + enrollment = get_object_or_404( + request.user.enrollment_set.select_related("course"), + course__slug=course_slug, + ) + course_assignment = get_object_or_404( + CourseAssignment.objects.select_related( + "activity", "activity__part_type", "piece" + ), + pk=course_assignment_id, + course=enrollment.course, + ) + return course_assignment, enrollment + + class SubmissionViewSet( ListModelMixin, RetrieveModelMixin, CreateModelMixin, GenericViewSet ): serializer_class = SubmissionSerializer queryset = Submission.objects.all() + def _target(self): + # Resolve (course_assignment, enrollment) once per request from the URL + # CourseAssignment id, scoped to the requesting student. + if not hasattr(self, "_cached_target"): + self._cached_target = resolve_student_target( + self.request, + self.kwargs["course_slug_slug"], + self.kwargs["assignment_id"], + ) + return self._cached_target + def get_queryset(self): - return self.queryset.filter(assignment_id=self.kwargs["assignment_id"]) + # Phase 2: a student's submissions are keyed by (course_assignment, + # enrollment), not the legacy per-student assignment id. + course_assignment, enrollment = self._target() + return self.queryset.filter( + course_assignment=course_assignment, enrollment=enrollment + ) def perform_create(self, serializer): + # Phase 2: record the course-level assignment, the student (enrollment), + # and the instrument/part the work was made with, resolved from the + # enrollment at write time. + course_assignment, enrollment = self._target() serializer.save( - assignment=Assignment.objects.get(pk=self.kwargs["assignment_id"]) + course_assignment=course_assignment, + enrollment=enrollment, + instrument=resolve_instrument(enrollment, course_assignment), + part=Part.for_activity(course_assignment.activity, course_assignment.piece), ) # @action(detail=False) @@ -75,55 +128,130 @@ def recent(self, request, **kwargs): piece_slug = request.GET["piece_slug"] activity_name = request.GET["activity_name"] - # https://chatgpt.com/share/827ac4eb-110d-423c-a106-1e696059fc83 - # Define a subquery to get the latest submission for each enrollment + # Phase 2: resolve the latest submission per enrollment from the + # submission's own fields (course_assignment / enrollment / part) instead + # of the dropped assignment FK. latest_submissions = ( Submission.objects.filter( - assignment__enrollment=OuterRef("assignment__enrollment"), - assignment__enrollment__course__slug=course_id, - assignment__activity__activity_type__name=activity_name, - assignment__part__piece__slug=piece_slug, + enrollment=OuterRef("enrollment"), + enrollment__course__slug=course_id, + course_assignment__activity__activity_type__name=activity_name, + course_assignment__piece__slug=piece_slug, ) .order_by("-submitted") .values("pk")[:1] ) - # Use the subquery to filter the main queryset - filtered_submissions = Submission.objects.filter( - pk__in=Subquery(latest_submissions) - ).order_by("assignment__enrollment", "-submitted") - - # The final queryset will have the latest submissions for each enrollment - submissions = filtered_submissions + # select_related/prefetch cover every relation SubmissionAssignmentSerializer + # walks (activity tree, instrument, part tree, enrollment course/owner/role), + # so serialization issues a constant number of queries regardless of how many + # students submitted. + submissions = list( + Submission.objects.filter(pk__in=Subquery(latest_submissions)) + .select_related( + "grade", + "self_grade", + "course_assignment__activity__activity_type__category", + "course_assignment__activity__part_type", + "course_assignment__piece", + "instrument__transposition", + "part__part_type", + "part__piece__composer", + "enrollment__user", + "enrollment__instrument__transposition", + "enrollment__role", + "enrollment__course__owner", + ) + .prefetch_related( + "attachments", + "part__transpositions__transposition", + "part__instrument_samples", + "enrollment__user__groups", + "enrollment__course__owner__groups", + ) + .order_by("enrollment", "-submitted") + ) serializer = self.serializer_class( - submissions, many=True, context={"request": request} + submissions, + many=True, + context={"request": request, **self._assignment_maps(submissions)}, ) return Response(status=status.HTTP_200_OK, data=serializer.data) + @staticmethod + def _assignment_maps(submissions): + # Per-(course_assignment, enrollment) maps for the nested assignment object's + # `submissions` and `group` fields -- two queries total, so the recent view + # stays constant in roster size. + ca_ids = [s.course_assignment_id for s in submissions] + enr_ids = [s.enrollment_id for s in submissions] + + submissions_by_pair = defaultdict(list) + for s in ( + Submission.objects.filter( + course_assignment_id__in=ca_ids, enrollment_id__in=enr_ids + ) + .order_by("id") + .prefetch_related("attachments") + ): + submissions_by_pair[(s.course_assignment_id, s.enrollment_id)].append(s) + + group_by_pair = { + (ga.course_assignment_id, ga.enrollment_id): ga.group_id + for ga in GroupAssignment.objects.filter( + course_assignment_id__in=ca_ids, enrollment_id__in=enr_ids + ) + } + return { + "submissions_by_pair": submissions_by_pair, + "group_by_pair": group_by_pair, + } + class GradeViewSet(ModelViewSet): queryset = Grade.objects.all() serializer_class = GradeSerializer def get_queryset(self, *args, **kwargs): + # GradeSerializer renders the reverse student_submission/own_submission + # one-to-ones; prefetch them so they aren't fetched per grade. return Grade.objects.filter( student_submission__assignment__enrollment__course__slug=self.kwargs[ "course_slug_slug" ] - ) + ).prefetch_related("student_submission", "own_submission") class ActivityProgressViewSet(GenericViewSet): serializer_class = ActivityProgressSerializer queryset = ActivityProgress.objects.all() + def _target(self): + # Resolve (course_assignment, enrollment) once per request from the URL + # CourseAssignment id, scoped to the requesting student. + if not hasattr(self, "_cached_target"): + self._cached_target = resolve_student_target( + self.request, + self.kwargs["course_slug_slug"], + self.kwargs["assignment_id"], + ) + return self._cached_target + + def _get_or_create_progress(self, lock=False): + # Phase 2: progress is keyed by (course_assignment, enrollment). + course_assignment, enrollment = self._target() + manager = ActivityProgress.objects + if lock: + manager = manager.select_for_update() + return manager.get_or_create( + course_assignment=course_assignment, + enrollment=enrollment, + ) + def get_object(self): """Get or create progress for the current assignment.""" - assignment_id = self.kwargs.get("assignment_id") - progress, created = ActivityProgress.objects.get_or_create( - assignment_id=assignment_id - ) + progress, created = self._get_or_create_progress() return progress def list(self, request, *args, **kwargs): @@ -135,14 +263,10 @@ def list(self, request, *args, **kwargs): @action(detail=False, methods=["post"]) def log_event(self, request, **kwargs): """Log an operation event to the activity progress.""" - assignment_id = kwargs.get("assignment_id") - try: # Use transaction with row-level locking to prevent race conditions with transaction.atomic(): - progress, created = ActivityProgress.objects.select_for_update().get_or_create( - assignment_id=assignment_id - ) + progress, created = self._get_or_create_progress(lock=True) # Extract event data from request operation = request.data.get("operation") @@ -165,7 +289,7 @@ def log_event(self, request, **kwargs): "timestamp": datetime.now().isoformat(), "step": step, "operation": operation, - "data": data + "data": data, } progress.activity_logs.append(event) @@ -188,22 +312,16 @@ def log_event(self, request, **kwargs): return Response(serializer.data, status=status.HTTP_200_OK) except Exception as e: - return Response( - {"error": str(e)}, - status=status.HTTP_400_BAD_REQUEST - ) + return Response({"error": str(e)}, status=status.HTTP_400_BAD_REQUEST) @action(detail=False, methods=["post"]) def submit_step(self, request, **kwargs): """Submit current step and advance to next.""" - assignment_id = kwargs.get("assignment_id") submitted_step = request.data.get("step") try: with transaction.atomic(): - progress, created = ActivityProgress.objects.select_for_update().get_or_create( - assignment_id=assignment_id - ) + progress, created = self._get_or_create_progress(lock=True) # If a step was submitted, use it as the step being completed # This ensures the user's actual position is used, not stale DB state @@ -211,7 +329,9 @@ def submit_step(self, request, **kwargs): submitted_step = int(submitted_step) # Allow setting the step if it's valid (1-4) if 1 <= submitted_step <= 4: - print(f"📝 Submitted step: {submitted_step}, stored step was: {progress.current_step}") + print( + f"📝 Submitted step: {submitted_step}, stored step was: {progress.current_step}" + ) # Set current_step to the submitted step (trust the frontend) progress.current_step = submitted_step @@ -223,7 +343,9 @@ def submit_step(self, request, **kwargs): if progress.current_step < 4: old_step = progress.current_step progress.current_step += 1 - print(f"✅ Advancing from step {old_step} to step {progress.current_step}") + print( + f"✅ Advancing from step {old_step} to step {progress.current_step}" + ) progress.save() @@ -235,23 +357,19 @@ def submit_step(self, request, **kwargs): except ActivityProgress.DoesNotExist: return Response( {"error": "Activity progress not found"}, - status=status.HTTP_404_NOT_FOUND + status=status.HTTP_404_NOT_FOUND, ) except (ValueError, TypeError) as e: return Response( {"error": f"Invalid step value: {e}"}, - status=status.HTTP_400_BAD_REQUEST + status=status.HTTP_400_BAD_REQUEST, ) @action(detail=False, methods=["post"]) def save_response(self, request, **kwargs): """Save a question response without advancing step.""" - assignment_id = kwargs.get("assignment_id") - try: - progress, created = ActivityProgress.objects.get_or_create( - assignment_id=assignment_id - ) + progress, created = self._get_or_create_progress() question_id = request.data.get("question_id") response_text = request.data.get("response") @@ -264,10 +382,7 @@ def save_response(self, request, **kwargs): return Response(serializer.data, status=status.HTTP_200_OK) except Exception as e: - return Response( - {"error": str(e)}, - status=status.HTTP_400_BAD_REQUEST - ) + return Response({"error": str(e)}, status=status.HTTP_400_BAD_REQUEST) @action(detail=False, methods=["post"]) def save_audio_state(self, request, **kwargs): @@ -276,9 +391,7 @@ def save_audio_state(self, request, **kwargs): try: with transaction.atomic(): - progress, created = ActivityProgress.objects.select_for_update().get_or_create( - assignment_id=assignment_id - ) + progress, created = self._get_or_create_progress(lock=True) # Extract audio state from request audio_url = request.data.get("audio_url") @@ -296,14 +409,13 @@ def save_audio_state(self, request, **kwargs): progress.save() print(f"💾 Saved audio state for assignment {assignment_id}") - print(f" audio_url: {progress.current_audio_url[:50] if progress.current_audio_url else None}...") + print( + f" audio_url: {progress.current_audio_url[:50] if progress.current_audio_url else None}..." + ) print(f" edit_history length: {len(progress.audio_edit_history)}") serializer = self.serializer_class(progress) return Response(serializer.data, status=status.HTTP_200_OK) except Exception as e: - return Response( - {"error": str(e)}, - status=status.HTTP_400_BAD_REQUEST - ) + return Response({"error": str(e)}, status=status.HTTP_400_BAD_REQUEST) diff --git a/teleband/submissions/migrations/0011_add_participant_email.py b/teleband/submissions/migrations/0011_add_participant_email.py index 69b261b..235d4bf 100644 --- a/teleband/submissions/migrations/0011_add_participant_email.py +++ b/teleband/submissions/migrations/0011_add_participant_email.py @@ -16,7 +16,7 @@ class Migration(migrations.Migration): field=models.EmailField( blank=True, null=True, - help_text="Email from Qualtrics for survey matching" + help_text="Email from Qualtrics for survey matching", ), ), ] diff --git a/teleband/submissions/migrations/0013_submission_submissions_assignm_06178b_idx_and_more.py b/teleband/submissions/migrations/0013_submission_submissions_assignm_06178b_idx_and_more.py new file mode 100644 index 0000000..a46514f --- /dev/null +++ b/teleband/submissions/migrations/0013_submission_submissions_assignm_06178b_idx_and_more.py @@ -0,0 +1,28 @@ +# Generated by Django 5.1.15 on 2026-06-27 21:30 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0037_rename_beginner_activity_types"), + ("submissions", "0012_activityprogress_audio_edit_history_and_more"), + ] + + operations = [ + migrations.AddIndex( + model_name="submission", + index=models.Index( + fields=["assignment", "-submitted"], + name="submissions_assignm_06178b_idx", + ), + ), + migrations.AddIndex( + model_name="submissionattachment", + index=models.Index( + fields=["submission", "-submitted"], + name="submissions_submiss_b947e9_idx", + ), + ), + ] diff --git a/teleband/submissions/migrations/0014_submission_course_assignment_submission_enrollment_and_more.py b/teleband/submissions/migrations/0014_submission_course_assignment_submission_enrollment_and_more.py new file mode 100644 index 0000000..6c48ab8 --- /dev/null +++ b/teleband/submissions/migrations/0014_submission_course_assignment_submission_enrollment_and_more.py @@ -0,0 +1,59 @@ +# Generated by Django 5.1.15 on 2026-06-27 22:51 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0039_backfill_course_assignments"), + ("courses", "0007_data_migration_stress_test_course"), + ("instruments", "0006_delete_instrumentconfig"), + ("musics", "0028_partinstrumentsample"), + ("submissions", "0013_submission_submissions_assignm_06178b_idx_and_more"), + ] + + operations = [ + migrations.AddField( + model_name="submission", + name="course_assignment", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="submissions", + to="assignments.courseassignment", + ), + ), + migrations.AddField( + model_name="submission", + name="enrollment", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + to="courses.enrollment", + ), + ), + migrations.AddField( + model_name="submission", + name="instrument", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + to="instruments.instrument", + ), + ), + migrations.AddField( + model_name="submission", + name="part", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + to="musics.part", + ), + ), + ] diff --git a/teleband/submissions/migrations/0015_backfill_submission_course_assignment.py b/teleband/submissions/migrations/0015_backfill_submission_course_assignment.py new file mode 100644 index 0000000..a9201cf --- /dev/null +++ b/teleband/submissions/migrations/0015_backfill_submission_course_assignment.py @@ -0,0 +1,65 @@ +from django.db import migrations + + +def backfill_submission_fields(apps, schema_editor): + """Populate Submission.course_assignment / enrollment / instrument / part for + existing rows, resolved from the per-student Assignment the submission points at. + Piece is derived from part.piece when Assignment.piece is null (legacy rows).""" + Submission = apps.get_model("submissions", "Submission") + CourseAssignment = apps.get_model("assignments", "CourseAssignment") + + ca_map = { + (ca["course_id"], ca["activity_id"], ca["piece_id"]): ca["id"] + for ca in CourseAssignment.objects.values( + "id", "course_id", "activity_id", "piece_id" + ) + } + + updates = [] + rows = Submission.objects.values( + "id", + "assignment__enrollment_id", + "assignment__instrument_id", + "assignment__part_id", + "assignment__enrollment__course_id", + "assignment__activity_id", + "assignment__piece_id", + "assignment__part__piece_id", + ) + for r in rows.iterator(): + piece_id = r["assignment__piece_id"] or r["assignment__part__piece_id"] + ca_id = ca_map.get( + ( + r["assignment__enrollment__course_id"], + r["assignment__activity_id"], + piece_id, + ) + ) + sub = Submission(id=r["id"]) + sub.course_assignment_id = ca_id + sub.enrollment_id = r["assignment__enrollment_id"] + sub.instrument_id = r["assignment__instrument_id"] + sub.part_id = r["assignment__part_id"] + updates.append(sub) + + if updates: + Submission.objects.bulk_update( + updates, + ["course_assignment", "enrollment", "instrument", "part"], + batch_size=500, + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ( + "submissions", + "0014_submission_course_assignment_submission_enrollment_and_more", + ), + ("assignments", "0039_backfill_course_assignments"), + ] + + operations = [ + migrations.RunPython(backfill_submission_fields, migrations.RunPython.noop), + ] diff --git a/teleband/submissions/migrations/0016_activityprogress_course_assignment_and_more.py b/teleband/submissions/migrations/0016_activityprogress_course_assignment_and_more.py new file mode 100644 index 0000000..c587f4f --- /dev/null +++ b/teleband/submissions/migrations/0016_activityprogress_course_assignment_and_more.py @@ -0,0 +1,37 @@ +# Generated by Django 5.1.15 on 2026-06-27 22:55 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0039_backfill_course_assignments"), + ("courses", "0007_data_migration_stress_test_course"), + ("submissions", "0015_backfill_submission_course_assignment"), + ] + + operations = [ + migrations.AddField( + model_name="activityprogress", + name="course_assignment", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="activity_progress", + to="assignments.courseassignment", + ), + ), + migrations.AddField( + model_name="activityprogress", + name="enrollment", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + to="courses.enrollment", + ), + ), + ] diff --git a/teleband/submissions/migrations/0017_backfill_activity_progress.py b/teleband/submissions/migrations/0017_backfill_activity_progress.py new file mode 100644 index 0000000..4e2fb09 --- /dev/null +++ b/teleband/submissions/migrations/0017_backfill_activity_progress.py @@ -0,0 +1,56 @@ +from django.db import migrations + + +def backfill_activity_progress(apps, schema_editor): + """Populate ActivityProgress.course_assignment / enrollment for existing rows, + resolved from the OneToOne assignment (piece derived from part.piece when the + assignment's piece is null).""" + ActivityProgress = apps.get_model("submissions", "ActivityProgress") + CourseAssignment = apps.get_model("assignments", "CourseAssignment") + + ca_map = { + (ca["course_id"], ca["activity_id"], ca["piece_id"]): ca["id"] + for ca in CourseAssignment.objects.values( + "id", "course_id", "activity_id", "piece_id" + ) + } + + updates = [] + rows = ActivityProgress.objects.values( + "id", + "assignment__enrollment_id", + "assignment__enrollment__course_id", + "assignment__activity_id", + "assignment__piece_id", + "assignment__part__piece_id", + ) + for r in rows.iterator(): + piece_id = r["assignment__piece_id"] or r["assignment__part__piece_id"] + ca_id = ca_map.get( + ( + r["assignment__enrollment__course_id"], + r["assignment__activity_id"], + piece_id, + ) + ) + progress = ActivityProgress(id=r["id"]) + progress.course_assignment_id = ca_id + progress.enrollment_id = r["assignment__enrollment_id"] + updates.append(progress) + + if updates: + ActivityProgress.objects.bulk_update( + updates, ["course_assignment", "enrollment"], batch_size=500 + ) + + +class Migration(migrations.Migration): + + dependencies = [ + ("submissions", "0016_activityprogress_course_assignment_and_more"), + ("assignments", "0039_backfill_course_assignments"), + ] + + operations = [ + migrations.RunPython(backfill_activity_progress, migrations.RunPython.noop), + ] diff --git a/teleband/submissions/migrations/0018_alter_activityprogress_assignment_and_more.py b/teleband/submissions/migrations/0018_alter_activityprogress_assignment_and_more.py new file mode 100644 index 0000000..388cdd8 --- /dev/null +++ b/teleband/submissions/migrations/0018_alter_activityprogress_assignment_and_more.py @@ -0,0 +1,37 @@ +# Generated by Django 5.1.15 on 2026-06-27 23:34 + +import django.db.models.deletion +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0039_backfill_course_assignments"), + ("submissions", "0017_backfill_activity_progress"), + ] + + operations = [ + migrations.AlterField( + model_name="activityprogress", + name="assignment", + field=models.OneToOneField( + blank=True, + null=True, + on_delete=django.db.models.deletion.CASCADE, + related_name="activity_progress", + to="assignments.assignment", + ), + ), + migrations.AlterField( + model_name="submission", + name="assignment", + field=models.ForeignKey( + blank=True, + null=True, + on_delete=django.db.models.deletion.PROTECT, + related_name="submissions", + to="assignments.assignment", + ), + ), + ] diff --git a/teleband/submissions/migrations/0019_activityprogress_unique_activity_progress.py b/teleband/submissions/migrations/0019_activityprogress_unique_activity_progress.py new file mode 100644 index 0000000..cf381a2 --- /dev/null +++ b/teleband/submissions/migrations/0019_activityprogress_unique_activity_progress.py @@ -0,0 +1,22 @@ +# Generated by Django 5.1.15 on 2026-06-28 00:49 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0040_courseassignment_instrument"), + ("courses", "0007_data_migration_stress_test_course"), + ("submissions", "0018_alter_activityprogress_assignment_and_more"), + ] + + operations = [ + migrations.AddConstraint( + model_name="activityprogress", + constraint=models.UniqueConstraint( + fields=("course_assignment", "enrollment"), + name="unique_activity_progress", + ), + ), + ] diff --git a/teleband/submissions/migrations/0020_remove_submission_submissions_assignm_06178b_idx_and_more.py b/teleband/submissions/migrations/0020_remove_submission_submissions_assignm_06178b_idx_and_more.py new file mode 100644 index 0000000..4c1f851 --- /dev/null +++ b/teleband/submissions/migrations/0020_remove_submission_submissions_assignm_06178b_idx_and_more.py @@ -0,0 +1,36 @@ +# Generated by Django 5.1.15 on 2026-06-28 01:13 + +from django.db import migrations, models + + +class Migration(migrations.Migration): + + dependencies = [ + ("assignments", "0041_remove_assignment_activity_and_more"), + ("courses", "0007_data_migration_stress_test_course"), + ("instruments", "0006_delete_instrumentconfig"), + ("musics", "0028_partinstrumentsample"), + ("submissions", "0019_activityprogress_unique_activity_progress"), + ] + + operations = [ + migrations.RemoveIndex( + model_name="submission", + name="submissions_assignm_06178b_idx", + ), + migrations.RemoveField( + model_name="activityprogress", + name="assignment", + ), + migrations.RemoveField( + model_name="submission", + name="assignment", + ), + migrations.AddIndex( + model_name="submission", + index=models.Index( + fields=["course_assignment", "enrollment", "-submitted"], + name="submissions_course__a8fa9d_idx", + ), + ), + ] diff --git a/teleband/submissions/models.py b/teleband/submissions/models.py index 5971ae3..6d058e1 100644 --- a/teleband/submissions/models.py +++ b/teleband/submissions/models.py @@ -1,8 +1,6 @@ from django.db import models from django.conf import settings -from teleband.assignments.models import Assignment - class Grade(models.Model): @@ -31,15 +29,37 @@ class Submission(models.Model): null=True, related_name="own_submission", ) - assignment = models.ForeignKey( - Assignment, on_delete=models.PROTECT, related_name="submissions" + # A submission belongs to a course-level CourseAssignment and the student + # (enrollment) who made it, and records the instrument/part it was made with. + course_assignment = models.ForeignKey( + "assignments.CourseAssignment", + on_delete=models.PROTECT, + null=True, + blank=True, + related_name="submissions", + ) + enrollment = models.ForeignKey( + "courses.Enrollment", on_delete=models.PROTECT, null=True, blank=True + ) + instrument = models.ForeignKey( + "instruments.Instrument", on_delete=models.PROTECT, null=True, blank=True + ) + part = models.ForeignKey( + "musics.Part", on_delete=models.PROTECT, null=True, blank=True ) index = models.PositiveIntegerField(default=0) submitted = models.DateTimeField(auto_now_add=True) content = models.TextField(blank=True) + class Meta: + indexes = [ + # Supports "latest submission per (course assignment, enrollment)" + # lookups (TeacherSubmissionViewSet.recent orders by -submitted). + models.Index(fields=["course_assignment", "enrollment", "-submitted"]), + ] + def __str__(self): - return f"{self.assignment.id}" + return f"{self.course_assignment_id or self.id}" class SubmissionAttachment(models.Model): @@ -54,6 +74,10 @@ class Meta: verbose_name = "Submission Attachment" verbose_name_plural = "Submission Attachments" ordering = ["-submitted"] + indexes = [ + # Backs the per-submission, newest-first ordering above. + models.Index(fields=["submission", "-submitted"]), + ] def __str__(self): return f"{self.submission.id}: {self.file}" @@ -62,41 +86,47 @@ def __str__(self): class ActivityProgress(models.Model): """Tracks student progress through DAW study activities.""" - assignment = models.OneToOneField( - Assignment, on_delete=models.CASCADE, related_name="activity_progress" + # Progress is per (course_assignment, enrollment) -- see the unique constraint + # in Meta. Nullable to tolerate legacy rows that never backfilled a + # course_assignment. + course_assignment = models.ForeignKey( + "assignments.CourseAssignment", + on_delete=models.CASCADE, + null=True, + blank=True, + related_name="activity_progress", + ) + enrollment = models.ForeignKey( + "courses.Enrollment", on_delete=models.CASCADE, null=True, blank=True ) current_step = models.PositiveIntegerField(default=1) # 1-4 for Activities 1-4 step_completions = models.JSONField( default=dict, - help_text="Tracks completed operations per step: {step: [operation_type, ...]}" + help_text="Tracks completed operations per step: {step: [operation_type, ...]}", ) activity_logs = models.JSONField( default=list, - help_text="Array of timestamped events: [{timestamp, step, operation, data}, ...]" + help_text="Array of timestamped events: [{timestamp, step, operation, data}, ...]", ) question_responses = models.JSONField( default=dict, - help_text="Student responses to embedded questions: {question_id: response, ...}" + help_text="Student responses to embedded questions: {question_id: response, ...}", ) participant_email = models.EmailField( - blank=True, - null=True, - help_text="Email from Qualtrics for survey matching" + blank=True, null=True, help_text="Email from Qualtrics for survey matching" ) # Audio state persistence for cross-activity editing current_audio_url = models.TextField( - blank=True, - null=True, - help_text="Current audio blob URL or file path" + blank=True, null=True, help_text="Current audio blob URL or file path" ) audio_edit_history = models.JSONField( default=list, - help_text="Array of edit history states for undo/redo: [{url, effectName, metadata}, ...]" + help_text="Array of edit history states for undo/redo: [{url, effectName, metadata}, ...]", ) audio_metadata = models.JSONField( default=dict, - help_text="Additional audio metadata: {duration, sampleRate, numberOfChannels, ...}" + help_text="Additional audio metadata: {duration, sampleRate, numberOfChannels, ...}", ) created_at = models.DateTimeField(auto_now_add=True) @@ -105,6 +135,15 @@ class ActivityProgress(models.Model): class Meta: verbose_name = "Activity Progress" verbose_name_plural = "Activity Progress" + constraints = [ + # Phase 2: one progress row per student per CourseAssignment. NULLs are + # distinct in Postgres/SQLite, so legacy rows that never backfilled a + # course_assignment don't collide. + models.UniqueConstraint( + fields=["course_assignment", "enrollment"], + name="unique_activity_progress", + ) + ] def __str__(self): - return f"Assignment {self.assignment.id} - Step {self.current_step}" + return f"Assignment {self.course_assignment_id} - Step {self.current_step}" diff --git a/teleband/submissions/tests.py b/teleband/submissions/tests.py deleted file mode 100644 index 7ce503c..0000000 --- a/teleband/submissions/tests.py +++ /dev/null @@ -1,3 +0,0 @@ -from django.test import TestCase - -# Create your tests here. diff --git a/teleband/submissions/tests/__init__.py b/teleband/submissions/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/teleband/submissions/tests/factories.py b/teleband/submissions/tests/factories.py new file mode 100644 index 0000000..1eba385 --- /dev/null +++ b/teleband/submissions/tests/factories.py @@ -0,0 +1,17 @@ +from factory import SubFactory +from factory.django import DjangoModelFactory + +from teleband.submissions.models import Submission + + +class SubmissionFactory(DjangoModelFactory): + # Phase 2: submissions are keyed by (course_assignment, enrollment). Imported + # lazily as SubFactory strings to avoid a circular import with the assignments + # factories (which build course assignments). + course_assignment = SubFactory( + "teleband.assignments.tests.factories.CourseAssignmentFactory" + ) + enrollment = SubFactory("teleband.courses.tests.factories.EnrollmentFactory") + + class Meta: + model = Submission diff --git a/teleband/submissions/tests/test_phase2_activity_progress.py b/teleband/submissions/tests/test_phase2_activity_progress.py new file mode 100644 index 0000000..5862c91 --- /dev/null +++ b/teleband/submissions/tests/test_phase2_activity_progress.py @@ -0,0 +1,46 @@ +"""Phase 2 step 6: ActivityProgress course_assignment/enrollment dual-key.""" + +import pytest +from django.db import IntegrityError +from rest_framework.test import APIClient + +from teleband.assignments.models import CourseAssignment +from teleband.assignments.tests.factories import ActivityFactory +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.submissions.models import ActivityProgress +from teleband.users.tests.factories import RoleFactory + +pytestmark = pytest.mark.django_db + + +def _course_assignment(): + course = CourseFactory() + piece = PieceFactory() + part = PartFactory(piece=piece) + enrollment = EnrollmentFactory(course=course, role=RoleFactory(name="Student")) + activity = ActivityFactory(part_type=part.part_type) + ca = CourseAssignment.objects.create(course=course, activity=activity, piece=piece) + return enrollment, ca + + +def test_activity_progress_unique_per_course_assignment_and_enrollment(): + enrollment, ca = _course_assignment() + ActivityProgress.objects.create(course_assignment=ca, enrollment=enrollment) + with pytest.raises(IntegrityError): + ActivityProgress.objects.create(course_assignment=ca, enrollment=enrollment) + + +def test_progress_created_via_api_is_dual_keyed(): + enrollment, ca = _course_assignment() + client = APIClient() + client.force_authenticate(user=enrollment.user) + # Phase 2: the nested route id is the CourseAssignment id. + resp = client.get( + f"/api/courses/{enrollment.course.slug}/assignments/{ca.id}/activity-progress/" + ) + assert resp.status_code == 200, resp.content + + progress = ActivityProgress.objects.get(course_assignment=ca, enrollment=enrollment) + assert progress.course_assignment_id == ca.id + assert progress.enrollment_id == enrollment.id diff --git a/teleband/submissions/tests/test_phase2_fields.py b/teleband/submissions/tests/test_phase2_fields.py new file mode 100644 index 0000000..8b48f99 --- /dev/null +++ b/teleband/submissions/tests/test_phase2_fields.py @@ -0,0 +1,45 @@ +"""Phase 2 step 5: Submission course_assignment/enrollment/instrument/part.""" + +import pytest +from rest_framework.test import APIClient + +from teleband.assignments.api.serializers import resolve_instrument +from teleband.assignments.models import CourseAssignment +from teleband.assignments.tests.factories import ActivityFactory +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.models import Part +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.submissions.models import Submission +from teleband.users.tests.factories import RoleFactory + +pytestmark = pytest.mark.django_db + + +def _course_assignment(): + course = CourseFactory() + piece = PieceFactory() + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + enrollment = EnrollmentFactory(course=course, role=RoleFactory(name="Student")) + ca = CourseAssignment.objects.create(course=course, activity=activity, piece=piece) + return enrollment, ca, part + + +def test_create_submission_populates_phase2_fields(): + enrollment, ca, part = _course_assignment() + client = APIClient() + client.force_authenticate(user=enrollment.user) + # Phase 2: the nested route id is the CourseAssignment id. + resp = client.post( + f"/api/courses/{enrollment.course.slug}/assignments/{ca.id}/submissions/", + {"content": "hi"}, + format="json", + ) + assert resp.status_code == 201, resp.content + + sub = Submission.objects.get(id=resp.data["id"]) + assert sub.course_assignment_id == ca.id + assert sub.enrollment_id == enrollment.id + # instrument/part are resolved at write time from the enrollment + CA. + assert sub.instrument_id == resolve_instrument(enrollment, ca).id + assert sub.part_id == Part.for_activity(ca.activity, ca.piece).id diff --git a/teleband/submissions/tests/test_phase2_nested_routes.py b/teleband/submissions/tests/test_phase2_nested_routes.py new file mode 100644 index 0000000..a8a41e0 --- /dev/null +++ b/teleband/submissions/tests/test_phase2_nested_routes.py @@ -0,0 +1,123 @@ +"""Phase 2 step 7: nested submission / activity-progress routes resolve the URL id +as a CourseAssignment scoped to the requesting student. + +Covers the late-joiner correctness win (a student with no Assignment row can submit +and track progress against a CourseAssignment) and cross-student isolation (a +student only sees their own work for a shared CourseAssignment). +""" + +import pytest +from rest_framework.test import APIClient + +from teleband.assignments.models import CourseAssignment +from teleband.assignments.tests.factories import ActivityFactory +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.submissions.models import ActivityProgress, Submission +from teleband.users.tests.factories import RoleFactory + +pytestmark = pytest.mark.django_db + + +def _course_with_ca(): + course = CourseFactory() + piece = PieceFactory() + part = PartFactory(piece=piece) + activity = ActivityFactory(part_type=part.part_type) + ca = CourseAssignment.objects.create(course=course, activity=activity, piece=piece) + return course, piece, part, activity, ca + + +def _student(course): + return EnrollmentFactory(course=course, role=RoleFactory(name="Student")) + + +def _client(enrollment): + client = APIClient() + client.force_authenticate(user=enrollment.user) + return client + + +def test_late_joiner_can_submit_against_course_assignment(): + """A late-enrolling student submits against the CourseAssignment; the submission + is keyed by (course_assignment, enrollment).""" + course, piece, part, activity, ca = _course_with_ca() + late = _student(course) + + resp = _client(late).post( + f"/api/courses/{course.slug}/assignments/{ca.id}/submissions/", + {"content": "late work"}, + format="json", + ) + assert resp.status_code == 201, resp.content + + sub = Submission.objects.get(id=resp.data["id"]) + assert sub.course_assignment_id == ca.id + assert sub.enrollment_id == late.id + assert sub.instrument_id == late.instrument_id + assert sub.part_id == part.id + + +def test_submission_list_is_scoped_to_requesting_student(): + course, piece, part, activity, ca = _course_with_ca() + mine = _student(course) + theirs = _student(course) + + _client(mine).post( + f"/api/courses/{course.slug}/assignments/{ca.id}/submissions/", + {"content": "mine"}, + format="json", + ) + _client(theirs).post( + f"/api/courses/{course.slug}/assignments/{ca.id}/submissions/", + {"content": "theirs"}, + format="json", + ) + + resp = _client(mine).get( + f"/api/courses/{course.slug}/assignments/{ca.id}/submissions/" + ) + assert resp.status_code == 200, resp.content + contents = {s["content"] for s in resp.json()} + assert contents == {"mine"} + + +def test_late_joiner_activity_progress_keyed_by_enrollment(): + course, piece, part, activity, ca = _course_with_ca() + late = _student(course) + + resp = _client(late).get( + f"/api/courses/{course.slug}/assignments/{ca.id}/activity-progress/" + ) + assert resp.status_code == 200, resp.content + + progress = ActivityProgress.objects.get(course_assignment=ca, enrollment=late) + assert progress.course_assignment_id == ca.id + assert progress.enrollment_id == late.id + + +def test_activity_progress_is_distinct_per_student(): + course, piece, part, activity, ca = _course_with_ca() + a = _student(course) + b = _student(course) + + _client(a).get(f"/api/courses/{course.slug}/assignments/{ca.id}/activity-progress/") + _client(b).get(f"/api/courses/{course.slug}/assignments/{ca.id}/activity-progress/") + + assert ActivityProgress.objects.filter(course_assignment=ca).count() == 2 + assert ( + ActivityProgress.objects.filter(course_assignment=ca, enrollment=a).count() == 1 + ) + assert ( + ActivityProgress.objects.filter(course_assignment=ca, enrollment=b).count() == 1 + ) + + +def test_unknown_course_assignment_returns_404(): + course, piece, part, activity, ca = _course_with_ca() + student = _student(course) + + resp = _client(student).get( + f"/api/courses/{course.slug}/assignments/{ca.id + 999}/activity-progress/" + ) + assert resp.status_code == 404, resp.content diff --git a/teleband/submissions/tests/test_query_counts.py b/teleband/submissions/tests/test_query_counts.py new file mode 100644 index 0000000..0126aae --- /dev/null +++ b/teleband/submissions/tests/test_query_counts.py @@ -0,0 +1,107 @@ +"""Query-count regression tests for the teacher grading endpoints. + +Phase 1a remodel guard: TeacherSubmissionViewSet.recent serializes a deeply +nested tree (submission -> assignment -> enrollment -> course/owner, part tree, +activity tree, grades, attachments). Without select_related/prefetch this fanned +out per submitting student. These tests assert the count is constant in the +number of students who submitted. +""" + +import pytest +from django.test.utils import CaptureQueriesContext +from django.db import connection +from rest_framework.test import APIClient + +from teleband.assignments.models import CourseAssignment +from teleband.assignments.tests.factories import ( + ActivityFactory, + ActivityTypeFactory, +) +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.instruments.tests.factories import InstrumentFactory +from teleband.musics.models import PartTransposition +from teleband.musics.tests.factories import PartFactory, PieceFactory +from teleband.submissions.models import SubmissionAttachment +from teleband.submissions.tests.factories import SubmissionFactory +from teleband.users.tests.factories import RoleFactory, UserFactory + +pytestmark = pytest.mark.django_db + + +def _build_recent_scenario(num_students): + """A course where ``num_students`` each have a graded submission for the + same piece+activity. Returns (course, teacher, activity_name, piece_slug).""" + teacher_role = RoleFactory(name="Teacher") + student_role = RoleFactory(name="Student") + course = CourseFactory() + teacher = UserFactory() + EnrollmentFactory(user=teacher, course=course, role=teacher_role) + + piece = PieceFactory() + activity_type = ActivityTypeFactory(name="Melody") + part = PartFactory(piece=piece) + PartTransposition.objects.create( + part=part, transposition=InstrumentFactory().transposition + ) + activity = ActivityFactory(activity_type=activity_type, part_type=part.part_type) + ca = CourseAssignment.objects.create(course=course, activity=activity, piece=piece) + + for _ in range(num_students): + enrollment = EnrollmentFactory(course=course, role=student_role) + # Phase 2: recent reads the submission's own fields; populate them as + # SubmissionViewSet.perform_create does. + submission = SubmissionFactory( + course_assignment=ca, + enrollment=enrollment, + instrument=enrollment.instrument, + part=part, + ) + SubmissionAttachment.objects.create(submission=submission, file="a.wav") + + return course, teacher, activity_type.name, piece.slug + + +def _count_recent_queries(course, user, activity_name, piece_slug): + client = APIClient() + client.force_authenticate(user=user) + url = ( + f"/api/courses/{course.slug}/submissions/recent/" + f"?activity_name={activity_name}&piece_slug={piece_slug}" + ) + with CaptureQueriesContext(connection) as ctx: + response = client.get(url) + assert response.status_code == 200, response.content + return len(ctx.captured_queries), response.data + + +def test_recent_assignment_object_built_from_native_fields(): + """The embedded assignment object (now built from the submission's own fields) + carries the field the grading UI reads -- enrollment.user.name -- plus the + CourseAssignment id.""" + course, teacher, a_name, p_slug = _build_recent_scenario(3) + _, data = _count_recent_queries(course, teacher, a_name, p_slug) + + assert len(data) == 3 + for row in data: + assignment = row["assignment"] + assert assignment["enrollment"]["user"]["name"] # the only consumed field + assert isinstance(assignment["id"], int) + assert assignment["instrument"] is not None + + +def test_recent_constant_in_student_count(): + small_course, small_teacher, a_name, p_slug = _build_recent_scenario(2) + large_course, large_teacher, a_name2, p_slug2 = _build_recent_scenario(20) + + small, small_data = _count_recent_queries( + small_course, small_teacher, a_name, p_slug + ) + large, large_data = _count_recent_queries( + large_course, large_teacher, a_name2, p_slug2 + ) + + assert len(small_data) == 2 and len(large_data) == 20 + assert small == large, ( + f"recent grading query count grows with #students who submitted " + f"({small} vs {large}) -- N+1 regression." + ) diff --git a/teleband/templates/assignments/assignment_list.html b/teleband/templates/assignments/assignment_list.html index d98f2c3..6fb3e02 100644 --- a/teleband/templates/assignments/assignment_list.html +++ b/teleband/templates/assignments/assignment_list.html @@ -69,7 +69,7 @@ {% for assn in assignment_list %} - {% if assn.submissions.all|length == 0 %} + {% if assn.submissions|length == 0 %} {{ assn.id }} {{ assn.enrollment.course.id }} @@ -88,7 +88,7 @@ N/A {% else %} - {% for sub in assn.submissions.all %} + {% for sub in assn.submissions %} {% for att in sub.attachments.all %} {{ assn.id }} diff --git a/teleband/users/admin.py b/teleband/users/admin.py index 393a76c..669394f 100644 --- a/teleband/users/admin.py +++ b/teleband/users/admin.py @@ -9,6 +9,7 @@ from teleband.users.models import Role, GroupInvitation from teleband.courses.models import Enrollment from teleband.users.models import InstrumentConfig + User = get_user_model() @@ -61,4 +62,5 @@ class RoleAdmin(VersionAdmin): list_display = ("id", "name") search_fields = ("name",) -admin.site.register(InstrumentConfig) \ No newline at end of file + +admin.site.register(InstrumentConfig) diff --git a/teleband/users/api/views.py b/teleband/users/api/views.py index a21e928..7833e52 100644 --- a/teleband/users/api/views.py +++ b/teleband/users/api/views.py @@ -35,6 +35,7 @@ from teleband.courses.models import Enrollment, Course from teleband.users.models import InstrumentConfig from django.db.models import Q + User = get_user_model() Invitation = get_invitation_model() @@ -62,14 +63,17 @@ class UserViewSet(RetrieveModelMixin, ListModelMixin, UpdateModelMixin, GenericV def get_queryset(self, *args, **kwargs): if self.action in ["update", "partial_update"]: - return self.queryset.filter( - enrollment__course__in=[ - e.course - for e in Enrollment.objects.filter( - user__username="admin", role__name="Teacher" - ) - ] + # Users enrolled in a course taught by the requesting teacher. The + # previous implementation hardcoded user__username="admin" (a bug -- + # it scoped to the literal "admin" user's courses, not the requester) + # and materialized the course list with a per-row Python loop. + teacher_course_ids = Course.objects.filter( + enrollment__user=self.request.user, + enrollment__role__name="Teacher", ) + return self.queryset.filter( + enrollment__course__in=teacher_course_ids + ).distinct() assert isinstance(self.request.user.id, int) return self.queryset.filter(id=self.request.user.id) @@ -153,10 +157,9 @@ class UserInstrumentConfigViewSet(ModelViewSet): serializer_class = UserInstrumentConfigSerializer queryset = InstrumentConfig.objects.all() - def get_queryset(self): return InstrumentConfig.objects.filter(Q(user=self.request.user) | Q(user=None)) - + def perform_create(self, serializer): serializer.save(user=self.request.user) diff --git a/teleband/users/migrations/0015_auto_20260420_1659.py b/teleband/users/migrations/0015_auto_20260420_1659.py index 7377fdf..a0de0e4 100644 --- a/teleband/users/migrations/0015_auto_20260420_1659.py +++ b/teleband/users/migrations/0015_auto_20260420_1659.py @@ -1,22 +1,25 @@ # Generated by Django 5.0.6 on 2026-04-20 20:59 from django.db import migrations + defaultConfigs = [ { "name": "Roland-GR-1-Trumpet", "settings": {}, "file": "instrument_config_samples/Roland-GR-1-Trumpet-C5.wav", - "user": None + "user": None, } - ] +] + def update_site_forward(apps, schema_editor): - instrument_config_model = apps.get_model("users", "InstrumentConfig") - for config in defaultConfigs: - instrument_config_model.objects.create(**config) + instrument_config_model = apps.get_model("users", "InstrumentConfig") + for config in defaultConfigs: + instrument_config_model.objects.create(**config) + class Migration(migrations.Migration): - + dependencies = [ ("users", "0014_alter_instrumentconfig_description"), ] diff --git a/teleband/users/migrations/0016_update_default_config_description.py b/teleband/users/migrations/0016_update_default_config_description.py index 818a602..4565c63 100644 --- a/teleband/users/migrations/0016_update_default_config_description.py +++ b/teleband/users/migrations/0016_update_default_config_description.py @@ -2,9 +2,14 @@ from django.db import migrations + def update_site_forward(apps, schema_editor): - instrument_config_model = apps.get_model("users", "InstrumentConfig") - instrument_config_model.objects.filter(name="Roland-GR-1-Trumpet", user=None).update(description="Default Roland GR-1 Trumpet configuration.") + instrument_config_model = apps.get_model("users", "InstrumentConfig") + instrument_config_model.objects.filter( + name="Roland-GR-1-Trumpet", user=None + ).update(description="Default Roland GR-1 Trumpet configuration.") + + class Migration(migrations.Migration): dependencies = [ diff --git a/teleband/users/models.py b/teleband/users/models.py index 7fcef0e..8137710 100644 --- a/teleband/users/models.py +++ b/teleband/users/models.py @@ -55,6 +55,9 @@ class InstrumentConfig(models.Model): description = models.CharField(max_length=100, null=True, blank=True) settings = models.JSONField(default=dict) user = models.ForeignKey(User, null=True, on_delete=models.CASCADE) - file = models.FileField(upload_to="instrument_config_samples/", null=True, blank=True) + file = models.FileField( + upload_to="instrument_config_samples/", null=True, blank=True + ) + def __str__(self): return self.name diff --git a/teleband/users/tests/factories.py b/teleband/users/tests/factories.py index 6b8b082..dff9865 100644 --- a/teleband/users/tests/factories.py +++ b/teleband/users/tests/factories.py @@ -2,6 +2,7 @@ from django.contrib.auth import get_user_model from factory import Faker, post_generation +from factory import Sequence as FactorySequence from factory.django import DjangoModelFactory from teleband.users.models import Role @@ -17,7 +18,11 @@ class Meta: class UserFactory(DjangoModelFactory): - username = Faker("user_name") + # Sequence (not Faker("user_name")) so the default username is guaranteed + # unique: Faker usernames repeat, and with django_get_or_create a repeat + # returns an existing user who then collides on UNIQUE(user, course) when + # enrolled again. An explicit username= still dedupes via get_or_create. + username = FactorySequence(lambda n: f"factory-user-{n}") email = Faker("email") name = Faker("name") diff --git a/teleband/users/tests/test_user_scoping.py b/teleband/users/tests/test_user_scoping.py new file mode 100644 index 0000000..2453f35 --- /dev/null +++ b/teleband/users/tests/test_user_scoping.py @@ -0,0 +1,80 @@ +"""Scoping + query-count tests for UserViewSet.get_queryset (Phase 1a #13). + +The update/partial_update queryset previously hardcoded username="admin" (a bug) +and built its course list with a per-row Python loop. These tests pin the correct +behavior -- a teacher may update students in their OWN courses, nobody else's -- +and that the queryset does not fan out per course. +""" + +import pytest +from django.test import RequestFactory +from django.test.utils import CaptureQueriesContext +from django.db import connection + +from teleband.courses.tests.factories import CourseFactory, EnrollmentFactory +from teleband.users.api.views import UserViewSet +from teleband.users.tests.factories import RoleFactory, UserFactory + +pytestmark = pytest.mark.django_db + + +def _update_queryset_for(teacher): + view = UserViewSet() + request = RequestFactory().patch("/fake/") + request.user = teacher + view.request = request + view.action = "partial_update" + view.kwargs = {} + return view.get_queryset() + + +def test_teacher_update_queryset_scopes_to_own_students(): + teacher_role = RoleFactory(name="Teacher") + student_role = RoleFactory(name="Student") + + teacher = UserFactory() + course = CourseFactory() + EnrollmentFactory(user=teacher, course=course, role=teacher_role) + + my_student = UserFactory() + EnrollmentFactory(user=my_student, course=course, role=student_role) + + # A student in some other teacher's course must not be in scope. + other_course = CourseFactory() + EnrollmentFactory(user=UserFactory(), course=other_course, role=teacher_role) + foreign_student = UserFactory() + EnrollmentFactory(user=foreign_student, course=other_course, role=student_role) + + qs = _update_queryset_for(teacher) + usernames = set(qs.values_list("username", flat=True)) + + assert my_student.username in usernames + assert foreign_student.username not in usernames + # Not scoped to a hardcoded "admin" user. + assert teacher.username in usernames or my_student.username in usernames + + +def test_teacher_update_queryset_is_constant_in_course_count(): + teacher_role = RoleFactory(name="Teacher") + student_role = RoleFactory(name="Student") + + def teacher_with_courses(n): + teacher = UserFactory() + for _ in range(n): + course = CourseFactory() + EnrollmentFactory(user=teacher, course=course, role=teacher_role) + EnrollmentFactory(course=course, role=student_role) + return teacher + + few_teacher = teacher_with_courses(2) + many_teacher = teacher_with_courses(20) + + with CaptureQueriesContext(connection) as ctx_few: + list(_update_queryset_for(few_teacher)) + with CaptureQueriesContext(connection) as ctx_many: + list(_update_queryset_for(many_teacher)) + + assert len(ctx_few.captured_queries) == len(ctx_many.captured_queries), ( + f"update queryset grows with #courses " + f"({len(ctx_few.captured_queries)} vs {len(ctx_many.captured_queries)}) -- N+1." + ) diff --git a/teleband/utils/health_check.py b/teleband/utils/health_check.py index 006eb3f..646e95e 100644 --- a/teleband/utils/health_check.py +++ b/teleband/utils/health_check.py @@ -1,5 +1,6 @@ from django.http import HttpResponse + class AlbHealthcheckMiddleware: def __init__(self, get_response): self.get_response = get_response @@ -8,4 +9,4 @@ def __call__(self, request): # Bypass host validation path for ALB health checks if request.path == "/healthz": return HttpResponse("ok", status=200) - return self.get_response(request) \ No newline at end of file + return self.get_response(request)