From d0290837ca17c661a3d83ad6efce71cf6707fc1b Mon Sep 17 00:00:00 2001 From: AndresL230 <190146319+AndresL230@users.noreply.github.com> Date: Tue, 28 Jul 2026 09:39:17 -0700 Subject: [PATCH] fix(graph): dedupe subject-root synthesis per distinct course, not per enrollment MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit GET /api/graph/{user_id} returned the subject-root node (subject_root__) and its ~5 hub-spoke edges duplicated for any user with two offerings of the same abstract course, because subject-root synthesis in graph_service.get_graph iterated enrollments instead of distinct abstract course ids. Fixes #355. - backend/services/graph_service.py: track seen_course_ids and skip synthesizing a second subject_root/hub-spoke set for a course already processed, so the API never returns two nodes with the same id. - backend/tests/test_graph_service.py: TDD regression test — a user with TWO offerings of the same abstract course now gets exactly one subject_root__ node and one hub spoke per concept node (failed before the fix: 3 node ids incl. one dup, now 2 unique). - frontend/e2e/graph.spec.ts: un-fixme the #355 acceptance test (promotion 1 of 3) and refresh the header/pre-test/companion comments that described the bug as still open. No assertions relaxed. Co-Authored-By: Claude Fable 5 --- backend/services/graph_service.py | 11 ++++++- backend/tests/test_graph_service.py | 34 +++++++++++++++++++ frontend/e2e/graph.spec.ts | 51 ++++++++++++++++------------- 3 files changed, 72 insertions(+), 24 deletions(-) diff --git a/backend/services/graph_service.py b/backend/services/graph_service.py index 684be90b..e1b36019 100644 --- a/backend/services/graph_service.py +++ b/backend/services/graph_service.py @@ -227,12 +227,21 @@ def get_graph(user_id: str) -> dict: if cid and cid in course_color_map: n["course_color"] = course_color_map[cid] - # Build subject root hubs from enrolled courses + # Build subject root hubs from enrolled courses — one root per DISTINCT + # abstract course, not per enrollment. A user can hold two offerings of the + # same abstract course (e.g. re-taking it, or two sections); the graph stays + # keyed on the abstract course_id, so without this dedup the synthesized + # subject_root node (and its hub-spoke edges) would be duplicated once per + # extra enrollment (#355). subject_nodes = [] subject_edges = [] + seen_course_ids: set[str] = set() for enrollment in enrolled_courses: course_id = enrollment["course_id"] + if course_id in seen_course_ids: + continue + seen_course_ids.add(course_id) course = enrollment.get("courses", {}) if isinstance(enrollment.get("courses"), dict) else {} course_code = course.get("course_code", "") course_name = course.get("course_name", "") diff --git a/backend/tests/test_graph_service.py b/backend/tests/test_graph_service.py index c00df99e..5379a2e9 100644 --- a/backend/tests/test_graph_service.py +++ b/backend/tests/test_graph_service.py @@ -261,6 +261,40 @@ def test_learning_velocity_computed_from_event_rows(self): assert len(node["mastery_events"]) == 2 assert result["stats"]["avg_learning_velocity"] > 0 + def test_dedupes_subject_root_for_two_offerings_of_same_course(self): + """#355: a user enrolled in TWO offerings of the same abstract course + (e.g. re-taking it, or two sections) must get exactly ONE subject-root + node — synthesis dedupes by abstract course_id, not per-enrollment.""" + nodes = [ + {"id": "n1", "concept_name": "Loops", "mastery_tier": "learning", + "mastery_score": 0.5, "subject": "CS101", "course_id": "c1", + "times_studied": 2, "user_id": "u1"}, + ] + enrollments = [ + _enrollment_row("c1", "CS101", "Intro CS", offering_id="off-1"), + _enrollment_row("c1", "CS101", "Intro CS", offering_id="off-2"), + ] + factory = _mock_table({ + "users": [{"streak_count": 0}], + "enrollments": enrollments, + "graph_nodes": nodes, + "graph_edges": [], + }) + with patch("services.graph_service.table", side_effect=factory): + result = get_graph("u1") + + node_ids = [n["id"] for n in result["nodes"]] + assert len(node_ids) == len(set(node_ids)), f"duplicate node ids: {node_ids}" + + roots = [n for n in result["nodes"] if n.get("is_subject_root")] + assert len(roots) == 1 + assert roots[0]["id"] == "subject_root__c1" + + # No surplus hub-spoke edges either: exactly one subject_edge per + # concept node under the course, not one per enrollment. + subject_edges = [e for e in result["edges"] if e["id"].startswith("subject_edge__")] + assert len(subject_edges) == len(nodes) + def test_velocity_zero_when_no_events(self): nodes = [ {"id": "n1", "concept_name": "Loops", "mastery_tier": "learning", diff --git a/frontend/e2e/graph.spec.ts b/frontend/e2e/graph.spec.ts index e353f5d6..c737cdd0 100644 --- a/frontend/e2e/graph.spec.ts +++ b/frontend/e2e/graph.spec.ts @@ -29,18 +29,18 @@ * collisions structurally irrelevant. Label TEXT is still asserted per node * (it is part of the rendered data), just keyed by id. * - * KNOWN BUG #355: /api/graph duplicates the subject-root hub when a user is - * enrolled in two offerings of the SAME abstract course (subject-root - * synthesis iterates enrollments, not distinct courses). The rich seed - * intentionally has rich-user-active in both the F25 and S26 offerings of - * rich-course-cs101, so the exact-count test below trips on the duplicated - * `subject_root__rich-course-cs101` hub (+1 node, +5 spokes) — verified live: - * PROBE355 nodes_total=18 nodes_unique=17 + * FIXED BUG #355: /api/graph used to duplicate the subject-root hub when a + * user was enrolled in two offerings of the SAME abstract course + * (subject-root synthesis iterated enrollments, not distinct courses). The + * rich seed intentionally has rich-user-active in both the F25 and S26 + * offerings of rich-course-cs101, so the exact-count test below is the + * regression coverage for it — it used to trip on the duplicated + * `subject_root__rich-course-cs101` hub (+1 node, +5 spokes) — verified live + * before the fix: PROBE355 nodes_total=18 nodes_unique=17 * dup_node_ids=["subject_root__rich-course-cs101"] edges_total=25 - * edges_unique=20 dup_edges=5. That is this journey catching precisely the - * defect class it exists for — the correct assertion is kept and the test is - * marked fixme(#355) rather than relaxed. The companion tests assert - * everything #355 does not corrupt. + * edges_unique=20 dup_edges=5. `graph_service.get_graph` now dedupes subject + * roots by distinct abstract `course_id` (backend/services/graph_service.py), + * so this journey runs as a normal (non-fixme) exact-count test. */ import type { Locator, Page } from "@playwright/test"; @@ -174,11 +174,12 @@ function byNodeId(loc: Locator, id: string) { return loc.and(loc.page().locator(`[data-node-id=${JSON.stringify(id)}]`)); } -// Marked fixme, NOT relaxed: red today solely because of open bug #355 (see -// header). The duplicated CS subject root makes the UI render one node and -// five hub spokes more than a correct payload would. Un-fixme when #355 -// lands — the assertions below are the acceptance test for it. -test.fixme("renders exactly one node per DB graph node plus one subject root per enrolled course, and one edge per DB edge plus one hub spoke per course node", async ({ page }) => { +// Acceptance test for #355 (now fixed): before the fix, the duplicated CS +// subject root made the UI render one node and five hub spokes more than a +// correct payload would, so this ran as test.fixme. Promoted to a normal +// test now that graph_service.get_graph dedupes subject roots by distinct +// abstract course_id. +test("renders exactly one node per DB graph node plus one subject root per enrolled course, and one edge per DB edge plus one hub spoke per course node", async ({ page }) => { const g = await graphExpectations(); expect(g.nodes.length).toBeGreaterThan(0); // journey guard: seeded graph present @@ -195,7 +196,8 @@ test.fixme("renders exactly one node per DB graph node plus one subject root per await expect(svgEdges).toHaveCount(g.expectedEdgeCount); // Each subject-root hub renders exactly once per distinct enrolled course - // (the precise duplication #355 causes: these read 2 for CS101 today). + // (this is the precise regression coverage for #355: before the fix, this + // read 2 for CS101 for a user enrolled in two offerings of it). for (const c of g.courses) { await expect(byNodeId(items, rootId(c.course_id))).toHaveCount(1); await expect(byNodeId(svgNodes, rootId(c.course_id))).toHaveCount(1); @@ -211,9 +213,10 @@ test("renders every DB concept node exactly once, classified by its DB mastery s // Every DB concept node renders exactly once — in the SVG and in the a11y // list — and shows its own concept name. Keyed by node id, so this holds - // even if two courses share a concept name. (Concept rows are what #355 - // does NOT duplicate, so exact counts hold here; hub multiplicity lives in - // the fixme test above.) + // even if two courses share a concept name. (Concept rows were never + // affected by #355 — only the synthesized subject-root hub was — so exact + // counts hold here; the hub's own exact multiplicity is covered by the + // dedicated test above.) for (const n of g.nodes) { const item = byNodeId(items, n.id); await expect(item).toHaveCount(1); @@ -222,7 +225,9 @@ test("renders every DB concept node exactly once, classified by its DB mastery s } // Every enrolled course's subject-root hub is present with its course - // label (≥1 — exact multiplicity is the #355-blocked assertion above). + // label (≥1 here; the dedicated test above asserts the exact multiplicity + // — this test stays a presence check so it doesn't duplicate that + // coverage). for (const c of g.courses) { const hub = byNodeId(items, rootId(c.course_id)).first(); await expect(hub).toBeVisible(); @@ -230,8 +235,8 @@ test("renders every DB concept node exactly once, classified by its DB mastery s } // Edge floor: at least every DB edge + hub spoke is drawn (exact equality - // is #355-blocked — the duplicate hub adds surplus spokes, but a MISSING - // edge must still fail here). + // is asserted by the dedicated test above; this stays a floor check so a + // MISSING edge still fails here even independent of hub-count coverage). expect(await svgEdges.count()).toBeGreaterThanOrEqual(g.expectedEdgeCount); // Mastery classification at the render layer: the 2D graph encodes the