From 9bffc97ab910c91a61c77d9e277c01c5513e8790 Mon Sep 17 00:00:00 2001 From: Anthony Giacalone Date: Fri, 14 Aug 2026 11:24:47 -0700 Subject: [PATCH] fix(recon): resolve Classroom 50 roster columns in discover_repos A Classroom 50 roster.csv and a legacy GitHub Classroom roster collide on the `github_id` column: C50 stores the immutable NUMERIC id there and puts the login in `username`, while legacy rosters store the LOGIN in `github_id`. Feeding a C50 roster to `lectern recon` therefore built repo names like `-1548364` instead of `-agiacalone`, dropped every row whose numeric id was unresolved, and left the student name blank (C50 splits the name across first_name/last_name). Identifier precedence is now github_username -> username -> github_id, so behavior is unchanged whenever `username` is absent and the explicit github_username override still outranks both. Student name falls back to first_name + last_name. Verified against the live c50-throwaway-tst0 roster: 11/11 rows resolve and agiacalone's repo name matches the real repo on GitHub. Full suite 411 passed / 1 skipped. The three C50 tests were confirmed RED against the unpatched module before landing. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01P2mBwr8n3r97t4g9AUV8j5 --- lectern/recon_discover.py | 17 ++++++++++- tests/test_recon_discover.py | 58 ++++++++++++++++++++++++++++++++++++ 2 files changed, 74 insertions(+), 1 deletion(-) diff --git a/lectern/recon_discover.py b/lectern/recon_discover.py index 11a87bd..b33c277 100644 --- a/lectern/recon_discover.py +++ b/lectern/recon_discover.py @@ -12,10 +12,25 @@ def discover_repos(roster_csv: Path, *, repo_prefix: str) -> list[RepoRef]: refs: list[RepoRef] = [] with Path(roster_csv).open(newline="") as f: for row in csv.DictReader(f): - gid = (row.get("github_username") or row.get("github_id") or "").strip() + # Identifier precedence. `username` must outrank `github_id` because + # a Classroom 50 roster.csv uses BOTH columns with different meanings + # than a legacy GitHub Classroom roster does: `username` is the login + # and `github_id` is the immutable NUMERIC id. Legacy rosters put the + # login in `github_id` (see examples/.../.cohort-spec.json), so that + # stays in the chain as the last resort and old behavior is unchanged + # whenever `username` is absent. Reading `github_id` first would build + # repo names like `-1548364` instead of `-agiacalone`. + gid = (row.get("github_username") or row.get("username") + or row.get("github_id") or "").strip() if not gid: continue name = (row.get("student_name") or row.get("canonical_name") or row.get("name") or "").strip() + if not name: + # Classroom 50 splits the name across two columns. + name = " ".join(p for p in ( + (row.get("first_name") or "").strip(), + (row.get("last_name") or "").strip(), + ) if p) refs.append(RepoRef(github_id=gid, student=name, repo=f"{repo_prefix}{gid}")) return refs diff --git a/tests/test_recon_discover.py b/tests/test_recon_discover.py index 3299bf5..14bd55a 100644 --- a/tests/test_recon_discover.py +++ b/tests/test_recon_discover.py @@ -20,3 +20,61 @@ def test_discover_uses_canonical_name(tmp_path): refs = discover_repos(csv_path, repo_prefix="pre-") assert refs[0].student == "Barbara Gordon" assert refs[0].repo == "pre-lucfox" + + +# --- Classroom 50 roster shape ------------------------------------------- +# A C50 roster.csv carries BOTH `username` (the login) and `github_id` (the +# immutable NUMERIC id), and splits the name across first_name/last_name. +# A legacy GitHub Classroom roster put the LOGIN in `github_id`, so the two +# formats collide on that column name and precedence is what separates them. + +C50_HEADER = "username,first_name,last_name,email,section,github_id,role\n" + + +def test_discover_c50_roster_prefers_username_over_numeric_github_id(tmp_path): + """Reading github_id first would name the repo `-1548364`.""" + csv_path = tmp_path / "roster.csv" + csv_path.write_text(C50_HEADER + + "agiacalone,Anthony,Giacalone,,section-1,1548364,student\n") + refs = discover_repos(csv_path, repo_prefix="c50-tst0-lab-01-") + assert len(refs) == 1 + assert refs[0].github_id == "agiacalone" + assert refs[0].repo == "c50-tst0-lab-01-agiacalone" + + +def test_discover_c50_roster_composes_name_from_first_last(tmp_path): + csv_path = tmp_path / "roster.csv" + csv_path.write_text(C50_HEADER + + "bruce-wayne,Bruce,Wayne,,section-1,,student\n") + refs = discover_repos(csv_path, repo_prefix="pre-") + assert refs[0].student == "Bruce Wayne" + + +def test_discover_c50_roster_keeps_rows_with_unresolved_github_id(tmp_path): + """A blank numeric github_id must not drop the student from the population.""" + csv_path = tmp_path / "roster.csv" + csv_path.write_text(C50_HEADER + + "bruce-wayne,Bruce,Wayne,,section-1,,student\n" + "dick-grayson,Dick,Grayson,,section-1,,student\n") + refs = discover_repos(csv_path, repo_prefix="pre-") + assert [r.repo for r in refs] == ["pre-bruce-wayne", "pre-dick-grayson"] + + +def test_discover_legacy_github_id_login_still_wins_when_no_username(tmp_path): + """Back-compat: legacy rosters store the LOGIN in github_id and have no + `username` column, so behavior there must be unchanged.""" + csv_path = tmp_path / "roster.csv" + csv_path.write_text("github_id,name\nbruce-wayne,Bruce Wayne\n") + refs = discover_repos(csv_path, repo_prefix="cecs-378-su26-01-lab-03-") + assert refs[0].github_id == "bruce-wayne" + assert refs[0].repo == "cecs-378-su26-01-lab-03-bruce-wayne" + assert refs[0].student == "Bruce Wayne" + + +def test_discover_explicit_github_username_outranks_c50_username(tmp_path): + """`github_username` stays top of the chain as the explicit override.""" + csv_path = tmp_path / "roster.csv" + csv_path.write_text("username,github_username,first_name,last_name\n" + "campus-sso-id,reallogin,Barbara,Gordon\n") + refs = discover_repos(csv_path, repo_prefix="pre-") + assert refs[0].repo == "pre-reallogin"