From ca1edd05de99e7e1d43a4b5124bfbca81aef172a Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Wed, 21 Oct 2026 23:04:12 -0700 Subject: [PATCH 1/2] feat(judging): round locks, judge progress endpoint, bulk assignments, batched member lookup - judging_round_locks collection read uncached on every judge write; POST /score and /draft answer 409 round_locked once an admin locks the round (fail-open on read errors). GET /api/judge/locks/, POST /api/judge/admin/locks//. - GET /api/judge/admin/progress//: judge x team submitted/draft/ missing matrix + summary + lock, computed server-side in 3 queries. - POST /api/judge/admin/assignments/bulk: idempotent cross-product assignment creation with optional replace (prunes pairs outside the panel's set). - get_judge_teams embeds event summary + locks and resolves members with ONE db.get_all instead of a serial get_user_by_id per member per team. - get_teams_batch chunks the __name__ in query by 30 and dedupes ids. Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 8 + api/judging/README.md | 52 +- api/judging/judging_service.py | 449 +++++++++++++++++- api/judging/judging_views.py | 130 ++++- api/judging/tests/test_judging_views_locks.py | 172 +++++++ .../tests/test_round_locks_and_progress.py | 220 +++++++++ .../tests/test_teams_batch_chunking.py | 63 +++ services/teams_service.py | 16 +- 8 files changed, 1095 insertions(+), 15 deletions(-) create mode 100644 api/judging/tests/test_judging_views_locks.py create mode 100644 api/judging/tests/test_round_locks_and_progress.py create mode 100644 api/judging/tests/test_teams_batch_chunking.py diff --git a/CLAUDE.md b/CLAUDE.md index aff0f2e..8df42d5 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -269,6 +269,14 @@ No rubric, scoring, round, or results logic changed. `get_team_details`/`format_ - `update_judge_assignment_details` (`PUT /api/judge/assignments/`) always 400'd "Assignment not found" — it looked assignments up via `fetch_judge_assignments_by_judge_id("")` (an empty judge_id can never match a real assignment) instead of by the assignment's own id. Added `fetch_judge_assignment_by_id` (direct doc-get, `db/firestore.py` + `db/db.py`) and used that instead. - `api/github/github_views.py`'s `/issues` route let a request through with no `org` (the service then 400'd with a 200 status, since the view unconditionally returned `jsonify(...)` with no status code), and logged `len(issues)` where `issues` is the service's response **dict** — that logged the dict's key count, not the issue count. Both fixed while adding `/activity` (see below). +### Judging — round locks, judge progress, bulk assignments (Oct 2026, `api/judging/`) +Built for the "15 judges hitting /judge at once" case; rubric/rounds/results math unchanged. +- **Round locks** live in their own collection `judging_round_locks` (doc id `__`, `{locked, locked_at, locked_by, locked_by_name, updated_at}`), read with ONE uncached doc-get on every judge write — deliberately NOT on the hackathon doc, whose getter is behind a 10-min per-worker cache and would lag a live awards ceremony. `POST /api/judge/score` and `POST /api/judge/draft` answer **409 `{error:"round_locked", round, event_id, message}`** once locked (checked AFTER the assignment check, BEFORE the write; an unknown round is 400 `invalid_round` first). A lock *read* failure fails OPEN (never block a judge over a flaky read). Routes: `GET /api/judge/locks/` (any signed-in user — judges render read-only pages from it; the judge teams payload also embeds `locks`), `POST /api/judge/admin/locks//` `{locked: bool}` (`judge.admin`, bool-validated, Slack-audited, `locked_by_name` best-effort via `get_propel_user_details_by_id`). +- **`GET /api/judge/admin/progress//`** (`judge.admin`) — judge × team matrix computed server-side from 3 queries (assignments, scores incl. drafts, judge applications) + one team batch: per judge `assigned/submitted/draft/missing_team_ids`, `complete`, `last_submitted_at`; per team `assigned/submitted_judge_ids`; `summary` incl. `drafts_total` (drafts whose pair has since been SUBMITTED are not drafts) and `orphan_submitted` (scores from judges who no longer hold an assignment — the Results total can exceed the matrix; surface it, don't hide it); plus the round's `lock`. Replaces the admin UI's one-request-per-(judge,team) score polling. +- **`POST /api/judge/admin/assignments/bulk`** (`judge.admin`) `{event_id, round, panel_id, judge_ids[], team_ids[], room?, demo_times?{team_id: iso}, replace?}` — creates every missing (judge, team) pair for the panel in one request, idempotent on `(panel_id, round, judge, team)`, re-syncs `room`/`demo_time` on kept pairs, and with `replace: true` deletes the panel's pairs outside the cross product (admin removed a finalist/judge and re-saved). **Scores are never touched by assignment changes.** Caps: 100 judges × 200 teams. +- `get_judge_teams` now returns `event {title,start_date,end_date,timezone}` + `locks`, and resolves members through `_batch_users_for_teams` (ONE `db.get_all` for every member of every assigned team) with the old per-user `get_user_by_id` loop kept as the fallback when `users_by_id` is None (tests patch that path). `services/teams_service.py::get_teams_batch` chunks the `__name__ in [...]` query by 30 (Firestore's cap) and dedupes ids. +- Tests: `api/judging/tests/test_round_locks_and_progress.py` (service), `test_judging_views_locks.py` (routes, auth-stubbed Flask client — includes the view-signature-vs-URL-params check), `test_teams_batch_chunking.py`. + ### `GET /api/github/activity?org&repo` — team dashboard "Code activity" card `common/utils/github.py::get_repo_activity(org, repo)` makes **exactly 3** GitHub API calls (rate-limit budget, not completeness — this is a live-ish snapshot): `g.get_repo(f"{org}/{repo}")`, `repo.get_commits().get_page(0)` (first page only, ≤100 commits; a 409 "empty repository" is a valid all-zeros result for a fresh team repo, not an error), `repo.get_pulls(state="open").totalCount`. Contributors are derived from that single commit page (a `Counter` over each commit's author), not a separate stats call. `api/github/github_service.py::get_github_activity` validates `org`/`repo` against `^[A-Za-z0-9_.-]{1,100}$`, caches successes only for 300s (`_ACTIVITY_CACHE`, separate from the existing `_ISSUES_CACHE`), and translates `UnknownObjectException`→404 `repo_not_found`, `RateLimitExceededException`→503 `github_rate_limited`, anything else→502 `github_unavailable`. **Commits by `activity_excluded_logins()` (env `GITHUB_ACTIVITY_EXCLUDED_LOGINS`, default `gregv` — the GITHUB_TOKEN owner who seeds LICENSE + README via `create_github_repo`) are dropped before any counting**, matched on GitHub login or, for unlinked commits, the git author name; otherwise every new team repo showed 2 commits and the dashboard's "Push code" row was done before the team pushed anything. diff --git a/api/judging/README.md b/api/judging/README.md index e0862c9..8f1bdb1 100644 --- a/api/judging/README.md +++ b/api/judging/README.md @@ -401,4 +401,54 @@ Remove a judge panel. 9. **Error Handling**: Check for assignment verification errors 10. **Round Management**: Support multiple judging rounds per event 11. **Panel UI**: Build admin interface for panel and assignment management -12. **Feedback**: Support rich text feedback from judges \ No newline at end of file +12. **Feedback**: Support rich text feedback from judges + +--- + +## Round Locks (Oct 2026) + +Admins can close scoring for a round. While locked, `POST /api/judge/score` and `POST /api/judge/draft` return **409**: + +```json +{"error": "round_locked", "round": "round2", "event_id": "fall-2026", "message": "Scoring for this round is closed. Your score was not changed."} +``` + +### Get locks +**GET** `/api/judge/locks/{event_id}` — any authenticated user. + +```json +{"locks": {"round1": {"locked": false, "locked_at": null, "locked_by_name": null}, "round2": {"locked": true, "locked_at": "2026-10-11T17:40:00-07:00", "locked_by_name": "Greg V"}}} +``` + +`GET /api/judge/teams/{judge_id}/{event_id}` also embeds the same `locks` object and an `event` summary (`title`, `start_date`, `end_date`, `timezone`). + +### Set a lock (Admin) +**POST** `/api/judge/admin/locks/{event_id}/{round_name}` — `judge.admin`. Body `{"locked": true}` (JSON boolean required). Returns `{"success": true, "lock": {...}}`. + +## Judge Progress (Admin) +**GET** `/api/judge/admin/progress/{event_id}/{round_name}` — `judge.admin`. + +One call that answers "who has sent in scores and who hasn't": + +```json +{ + "lock": {"locked": false}, + "judges": [{"judge_id": "…", "name": "Ada", "email": "…", "assigned": 5, "submitted": 3, "complete": false, + "assigned_team_ids": ["…"], "submitted_team_ids": ["…"], "draft_team_ids": ["…"], "missing_team_ids": ["…"], + "last_submitted_at": "…"}], + "teams": [{"team_id": "…", "name": "Alpha", "assigned": 15, "submitted": 12, "assigned_judge_ids": ["…"], "submitted_judge_ids": ["…"]}], + "summary": {"judges_total": 15, "judges_complete": 9, "assignments_total": 75, "submitted_total": 58, "drafts_total": 4, "orphan_submitted": 0} +} +``` + +Incomplete judges sort first. A draft that has since been submitted is not counted as a draft. + +## Bulk Assignments (Admin) +**POST** `/api/judge/admin/assignments/bulk` — `judge.admin`. + +```json +{"event_id": "fall-2026", "round": "round2", "panel_id": "", "judge_ids": ["…"], "team_ids": ["…"], + "room": "Main Room", "demo_times": {"": "2026-10-11T16:40:00-07:00"}, "replace": true} +``` + +Creates every missing (judge, team) pair for the panel in one request (idempotent). `replace: true` also deletes the panel's pairs that are no longer in the cross product. Scores are never modified. Response: `{"success": true, "created": 12, "kept": 63, "updated": 0, "removed": 3, "total": 75}`. diff --git a/api/judging/judging_service.py b/api/judging/judging_service.py index 71217db..bb4b31e 100644 --- a/api/judging/judging_service.py +++ b/api/judging/judging_service.py @@ -153,16 +153,24 @@ def get_judge_teams(judge_id: str, event_id: str) -> Dict: nonprofit_mapping = {id: get_nonprofit_by_id(id) for id in nonprofit_ids} logger.info(f"Fetched {len(nonprofit_mapping)} nonprofits for event {event_id}") + # One batched profile read for every member of every assigned team + # (was one serial get_user_by_id per member per team per judge — + # 15 judges × 10 teams × 4 members at kickoff). + users_by_id = _batch_users_for_teams(teams_data) + for team in teams_data: team_id = team.get('id') nonprofit_name = nonprofit_mapping.get(team.get('selected_nonprofit_id', ''), {}).get('name', '') nonprofit_id = team.get('selected_nonprofit_id', '') - # Get assignment details for demo scheduling + # Round 2 assignments carry the demo slot; prefer that one when + # the same team is also assigned in round 1. assignment = next( - (a for a in assignments if a.team_id == team_id), None) + (a for a in assignments if a.team_id == team_id and a.round == 'round2'), + None) or next((a for a in assignments if a.team_id == team_id), None) - team_data = format_team_for_judge(team, score_lookup, nonprofit_id, nonprofit_name) + team_data = format_team_for_judge(team, score_lookup, nonprofit_id, nonprofit_name, + users_by_id=users_by_id) if team_id in round1_team_ids: round1_teams.append(team_data) @@ -181,7 +189,9 @@ def get_judge_teams(judge_id: str, event_id: str) -> Dict: return { "round1_teams": round1_teams, - "round2_teams": round2_teams + "round2_teams": round2_teams, + "event": _event_summary(event_id), + "locks": get_round_locks(event_id), } except Exception as e: @@ -194,6 +204,23 @@ def get_judge_teams(judge_id: str, event_id: str) -> Dict: "error": "Failed to fetch teams"} +def _event_summary(event_id: str) -> Dict: + """Title/dates/timezone for the judge dashboard masthead (cached getter; + best-effort — never fails the teams payload).""" + try: + h = get_single_hackathon_event(event_id) or {} + return { + "event_id": event_id, + "title": h.get("title", ""), + "start_date": h.get("start_date"), + "end_date": h.get("end_date"), + "timezone": h.get("timezone"), + } + except Exception as e: + warning(logger, "Event summary lookup failed", event_id=event_id, error=str(e)) + return {"event_id": event_id, "title": "", "start_date": None, "end_date": None, "timezone": None} + + def get_team_details(team_id: str) -> Dict: """Get detailed information about a specific team for judging.""" try: @@ -465,8 +492,50 @@ def get_draft_score(judge_id: str, team_id: str, event_id: str, round_name: str) return {"draft": None, "error": "Failed to fetch draft"} -def format_team_for_judge(team: Dict, score_lookup: Dict = None, nonprofit_id: str = None, nonprofit_name: str = None) -> Dict: - """Format team data for judge API response.""" +def _member_ids(team: Dict) -> list: + """users[] may be enriched dicts or legacy id strings.""" + out = [] + for u in team.get('users', []) or []: + uid = u.get("id") if isinstance(u, dict) else u + if uid and isinstance(uid, str): + out.append(uid) + return out + + +def _batch_users_for_teams(teams: list) -> Dict: + """id -> slim member dict for every member across `teams`, via ONE + Firestore get_all. Returns None on any failure so callers fall back to + the per-user lookup (never fail a judge's dashboard over an optimisation).""" + ids = [] + for team in teams or []: + for uid in _member_ids(team): + if uid not in ids: + ids.append(uid) + if not ids: + return {} + try: + db = _locks_db() + refs = [db.collection("users").document(uid) for uid in ids] + out = {} + for snap in db.get_all(refs): + if getattr(snap, "exists", False): + d = snap.to_dict() or {} + out[snap.id] = { + "id": snap.id, + "name": d.get('name', ''), + "email": d.get('email_address', ''), + "profile_image": d.get('profile_image', ''), + } + return out + except Exception as e: + warning(logger, "Batch member lookup failed; falling back to per-user reads", error=str(e)) + return None + + +def format_team_for_judge(team: Dict, score_lookup: Dict = None, nonprofit_id: str = None, + nonprofit_name: str = None, users_by_id: Dict = None) -> Dict: + """Format team data for judge API response. Pass `users_by_id` (from + `_batch_users_for_teams`) to avoid a Firestore read per member.""" if score_lookup is None: score_lookup = {} @@ -475,7 +544,11 @@ def format_team_for_judge(team: Dict, score_lookup: Dict = None, nonprofit_id: s # Resolve team members from user IDs if present members = [] user_ids = team.get('users', []) - if user_ids: + if user_ids and isinstance(users_by_id, dict): + for uid in _member_ids(team): + if uid in users_by_id: + members.append(dict(users_by_id[uid])) + elif user_ids: # Import here to avoid circular imports from common.utils.firebase import get_user_by_id @@ -1016,3 +1089,365 @@ def get_bulk_judge_details(event_id: str) -> Dict: import traceback traceback.print_exc() return {"judges": [], "total_count": 0, "error": "Failed to fetch judge details"} + + +# --------------------------------------------------------------------------- +# Round locks, judge progress, bulk assignments (Oct 2026) +# +# Locks live in their own collection (`judging_round_locks`, doc id +# `__`) and are read with ONE uncached doc-get on every +# score/draft write, so flipping the lock takes effect immediately — the +# hackathon doc is behind a 10-minute cache and would have lagged a live +# awards ceremony. Writes go through `common.utils.firebase.get_db()` (the +# same client the submissions service uses; MockFirestore under +# ENVIRONMENT=test). +# --------------------------------------------------------------------------- + +JUDGING_ROUNDS = ("round1", "round2") +JUDGING_LOCKS_COLLECTION = "judging_round_locks" +BULK_ASSIGN_MAX_JUDGES = 100 +BULK_ASSIGN_MAX_TEAMS = 200 + + +def _locks_db(): + # Lazy import: common.utils.firebase builds Firebase credentials at import + # time; keeping it inside the function keeps judging_service importable in + # unit tests that patch it. + from common.utils.firebase import get_db as _get_db + return _get_db() + + +def _lock_doc_id(event_id: str, round_name: str) -> str: + return f"{event_id}__{round_name}" + + +def _empty_lock(event_id: str, round_name: str) -> Dict: + return { + "event_id": event_id, + "round": round_name, + "locked": False, + "locked_at": None, + "locked_by": None, + "locked_by_name": None, + "updated_at": None, + } + + +def _normalize_lock(raw: Dict, event_id: str, round_name: str) -> Dict: + lock = _empty_lock(event_id, round_name) + if isinstance(raw, dict): + lock["locked"] = bool(raw.get("locked", False)) + for key in ("locked_at", "locked_by", "locked_by_name", "updated_at"): + lock[key] = raw.get(key) + return lock + + +def get_round_lock(event_id: str, round_name: str) -> Dict: + """Current lock state for one round. Missing doc ⇒ unlocked.""" + try: + snap = _locks_db().collection(JUDGING_LOCKS_COLLECTION).document( + _lock_doc_id(event_id, round_name)).get() + raw = snap.to_dict() if getattr(snap, "exists", False) else None + return _normalize_lock(raw, event_id, round_name) + except Exception as e: + # Fail OPEN on a read error: a flaky lock read must never block a judge + # from scoring. Admins see the lock state on their panel and can retry. + warning(logger, "Error reading judging round lock", + event_id=event_id, round_name=round_name, error=str(e)) + return _empty_lock(event_id, round_name) + + +def get_round_locks(event_id: str) -> Dict: + """`{"round1": lock, "round2": lock}` for the judge dashboard + admin.""" + return {r: get_round_lock(event_id, r) for r in JUDGING_ROUNDS} + + +def is_round_locked(event_id: str, round_name: str) -> bool: + return bool(get_round_lock(event_id, round_name).get("locked")) + + +def set_round_lock(event_id: str, round_name: str, locked: bool, + actor_id: str = None, actor_name: str = None) -> Dict: + """Lock or unlock scoring for a round. Idempotent; returns the new lock.""" + if round_name not in JUDGING_ROUNDS: + return {"success": False, "error": "invalid_round"} + if not isinstance(locked, bool): + return {"success": False, "error": "locked_must_be_boolean"} + try: + now_iso = datetime.now().astimezone().isoformat() + doc = { + "event_id": event_id, + "round": round_name, + "locked": locked, + "updated_at": now_iso, + } + if locked: + doc.update({ + "locked_at": now_iso, + "locked_by": actor_id, + "locked_by_name": actor_name, + }) + else: + doc.update({"locked_at": None, "locked_by": None, "locked_by_name": None}) + + _locks_db().collection(JUDGING_LOCKS_COLLECTION).document( + _lock_doc_id(event_id, round_name)).set(doc, merge=True) + + try: + from common.utils.slack import send_slack_audit + send_slack_audit( + action="judging_round_lock", + message=f"{'Locked' if locked else 'Unlocked'} {round_name} scoring for {event_id}", + payload={"event_id": event_id, "round": round_name, "locked": locked, + "by": actor_name or actor_id}, + ) + except Exception as e: # audit is best-effort + warning(logger, "judging lock audit failed", error=str(e)) + + return {"success": True, "lock": _normalize_lock(doc, event_id, round_name)} + except Exception as e: + error(logger, "Error setting judging round lock", + event_id=event_id, round_name=round_name, error=str(e)) + return {"success": False, "error": "Failed to update lock"} + + +def _rows(result) -> list: + """The event-scoped db fetchers return {"data": [...]}; tolerate a bare + list. Only dict rows are returned (keeps pylint's inference honest too).""" + if isinstance(result, dict): + result = result.get("data") or [] + return [row for row in (result or []) if isinstance(row, dict)] + + +def _judge_directory(event_id: str) -> Dict: + """user_id -> {name, email} for every judge application on the event + (one query), so progress rows carry names without N lookups.""" + directory = {} + try: + for judge in _rows(get_volunteer_from_db_by_event(event_id, "judge", admin=True)): + uid = judge.get("user_id") + if not uid: + continue + directory[uid] = { + "name": judge.get("name") or "", + "email": judge.get("email") or "", + "is_selected": bool(judge.get("isSelected")), + } + except TypeError: + # Older signature without the admin kwarg. + for judge in _rows(get_volunteer_from_db_by_event(event_id, "judge")): + uid = judge.get("user_id") + if uid: + directory[uid] = {"name": judge.get("name") or "", "email": judge.get("email") or "", + "is_selected": bool(judge.get("isSelected"))} + except Exception as e: + warning(logger, "Judge directory lookup failed", event_id=event_id, error=str(e)) + return directory + + +def get_judging_progress(event_id: str, round_name: str) -> Dict: + """Judge × team scoring progress for one round, computed server-side in + three queries (assignments, scores, judge applications) + one team batch. + + A judge/team cell is `submitted` when a non-draft score doc exists, + `draft` when only a draft exists, else `missing`. Drafts that have since + been submitted are NOT counted as drafts.""" + if round_name not in JUDGING_ROUNDS: + return {"error": "invalid_round"} + try: + assignments = [a for a in _rows(fetch_judge_assignments_by_event_id(event_id)) + if a.get("round") == round_name and a.get("judge_id") and a.get("team_id")] + scores = [s for s in _rows(fetch_judge_scores_by_event_id(event_id)) + if s.get("round") == round_name and s.get("judge_id") and s.get("team_id")] + + submitted = {} # (judge_id, team_id) -> submitted_at iso|None + drafts = {} # (judge_id, team_id) -> updated_at iso|None + for s in scores: + key = (s["judge_id"], s["team_id"]) + if s.get("is_draft"): + drafts[key] = _iso(s.get("updated_at")) + else: + submitted[key] = _iso(s.get("submitted_at") or s.get("updated_at")) + + judges_by_id = {} + teams_by_id = {} + for a in assignments: + j = judges_by_id.setdefault(a["judge_id"], { + "judge_id": a["judge_id"], "assigned_team_ids": [], + "submitted_team_ids": [], "draft_team_ids": [], "missing_team_ids": [], + "last_submitted_at": None, "panel_ids": set(), + }) + if a["team_id"] not in j["assigned_team_ids"]: + j["assigned_team_ids"].append(a["team_id"]) + if a.get("panel_id"): + j["panel_ids"].add(a["panel_id"]) + t = teams_by_id.setdefault(a["team_id"], { + "team_id": a["team_id"], "assigned_judge_ids": [], "submitted_judge_ids": [], + }) + if a["judge_id"] not in t["assigned_judge_ids"]: + t["assigned_judge_ids"].append(a["judge_id"]) + + for jid, j in judges_by_id.items(): + for tid in j["assigned_team_ids"]: + key = (jid, tid) + if key in submitted: + j["submitted_team_ids"].append(tid) + teams_by_id[tid]["submitted_judge_ids"].append(jid) + ts = submitted[key] + if ts and (j["last_submitted_at"] is None or ts > j["last_submitted_at"]): + j["last_submitted_at"] = ts + elif key in drafts: + j["draft_team_ids"].append(tid) + else: + j["missing_team_ids"].append(tid) + + # Names + directory = _judge_directory(event_id) + team_names = {} + team_ids = list(teams_by_id.keys()) + if team_ids: + try: + for team in get_teams_batch({"team_ids": team_ids}): + team_names[team.get("id")] = team.get("name", "") + except Exception as e: + warning(logger, "Progress team batch failed", event_id=event_id, error=str(e)) + + judges_out = [] + for j in judges_by_id.values(): + info_ = directory.get(j["judge_id"], {}) + assigned_n = len(j["assigned_team_ids"]) + submitted_n = len(j["submitted_team_ids"]) + judges_out.append({ + "judge_id": j["judge_id"], + "name": info_.get("name") or "Unknown judge", + "email": info_.get("email") or "", + "panel_ids": sorted(j["panel_ids"]), + "assigned_team_ids": j["assigned_team_ids"], + "submitted_team_ids": j["submitted_team_ids"], + "draft_team_ids": j["draft_team_ids"], + "missing_team_ids": j["missing_team_ids"], + "assigned": assigned_n, + "submitted": submitted_n, + "complete": assigned_n > 0 and submitted_n >= assigned_n, + "last_submitted_at": j["last_submitted_at"], + }) + judges_out.sort(key=lambda j: (j["complete"], -(j["assigned"] - j["submitted"]), j["name"].lower())) + + teams_out = [] + for t in teams_by_id.values(): + teams_out.append({ + "team_id": t["team_id"], + "name": team_names.get(t["team_id"], ""), + "assigned_judge_ids": t["assigned_judge_ids"], + "submitted_judge_ids": t["submitted_judge_ids"], + "assigned": len(t["assigned_judge_ids"]), + "submitted": len(t["submitted_judge_ids"]), + }) + teams_out.sort(key=lambda t: (t["name"] or "").lower()) + + # Scores from judges who no longer hold an assignment (admin re-paneled) + # still exist in the collection; surface the count so nobody is confused + # by a Results total that is higher than the matrix. + assigned_keys = {(a["judge_id"], a["team_id"]) for a in assignments} + orphan_submitted = len([k for k in submitted if k not in assigned_keys]) + + return { + "event_id": event_id, + "round": round_name, + "lock": get_round_lock(event_id, round_name), + "judges": judges_out, + "teams": teams_out, + "summary": { + "judges_total": len(judges_out), + "judges_complete": len([j for j in judges_out if j["complete"]]), + "assignments_total": len(assigned_keys), + "submitted_total": len([k for k in assigned_keys if k in submitted]), + "drafts_total": len([k for k in assigned_keys if k in drafts and k not in submitted]), + "orphan_submitted": orphan_submitted, + }, + } + except Exception as e: + error(logger, "Error computing judging progress", + event_id=event_id, round_name=round_name, error=str(e)) + import traceback + traceback.print_exc() + return {"error": "Failed to compute judging progress"} + + +def _iso(value): + if value is None: + return None + if hasattr(value, "isoformat"): + try: + return value.isoformat() + except Exception: + return str(value) + return str(value) + + +def bulk_create_judge_assignments(event_id: str, round_name: str, panel_id: str, + judge_ids: list, team_ids: list, room: str = None, + demo_times: Dict = None, replace: bool = False) -> Dict: + """Create every missing (judge, team) assignment for a panel in one call. + + Idempotent: pairs that already exist for (panel_id, event_id, round) are + left alone. With `replace=True` the panel's assignments for this round are + made to EQUAL the cross product — pairs no longer in it are deleted (the + admin removed a judge/finalist and re-saved). Scores are never touched.""" + if round_name not in JUDGING_ROUNDS: + return {"success": False, "error": "invalid_round"} + if not panel_id: + return {"success": False, "error": "panel_id_required"} + judge_ids = [j for j in dict.fromkeys(judge_ids or []) if j] + team_ids = [t for t in dict.fromkeys(team_ids or []) if t] + if len(judge_ids) > BULK_ASSIGN_MAX_JUDGES or len(team_ids) > BULK_ASSIGN_MAX_TEAMS: + return {"success": False, "error": "too_many_items"} + demo_times = demo_times if isinstance(demo_times, dict) else {} + + try: + existing = [a for a in _rows(fetch_judge_assignments_by_event_id(event_id)) + if a.get("round") == round_name and a.get("panel_id") == panel_id] + existing_by_pair = {} + for a in existing: + existing_by_pair.setdefault((a.get("judge_id"), a.get("team_id")), a) + + wanted = {(j, t) for j in judge_ids for t in team_ids} + created, kept, updated = 0, 0, 0 + for (judge_id, team_id) in sorted(wanted): + current = existing_by_pair.get((judge_id, team_id)) + if current: + kept += 1 + # Keep room/demo_time in sync when the admin changed them. + new_room = room if room is not None else current.get("room") + new_demo = demo_times.get(team_id, current.get("demo_time")) + if new_room != current.get("room") or new_demo != current.get("demo_time"): + assignment = JudgeAssignment.deserialize({**current, "room": new_room, "demo_time": new_demo}) + update_judge_assignment(assignment) + updated += 1 + continue + assignment = JudgeAssignment() + assignment.judge_id = judge_id + assignment.event_id = event_id + assignment.team_id = team_id + assignment.round = round_name + assignment.panel_id = panel_id + assignment.room = room + assignment.demo_time = demo_times.get(team_id) + insert_judge_assignment(assignment) + created += 1 + + removed = 0 + if replace: + for pair, a in existing_by_pair.items(): + if pair not in wanted and a.get("id"): + delete_judge_assignment(a["id"]) + removed += 1 + + return {"success": True, "created": created, "kept": kept, "updated": updated, + "removed": removed, "total": len(wanted)} + except Exception as e: + error(logger, "Error bulk-creating judge assignments", + event_id=event_id, round_name=round_name, panel_id=panel_id, error=str(e)) + import traceback + traceback.print_exc() + return {"success": False, "error": "Failed to create assignments"} diff --git a/api/judging/judging_views.py b/api/judging/judging_views.py index 95888fb..4edbd23 100644 --- a/api/judging/judging_views.py +++ b/api/judging/judging_views.py @@ -22,7 +22,13 @@ get_judge_assignments_for_panel, get_judge_event_details, get_bulk_judge_scores, - get_bulk_judge_details + get_bulk_judge_details, + get_round_locks, + is_round_locked, + set_round_lock, + get_judging_progress, + bulk_create_judge_assignments, + JUDGING_ROUNDS, ) logger = get_logger("judging_views") @@ -40,6 +46,29 @@ def get_authenticated_user_id(): return auth_user.user_id +def _round_locked_response(event_id, round_name): + """409 body every judge-side write returns once an admin has locked the + round. The frontend branches on `error == "round_locked"`.""" + return { + "error": "round_locked", + "round": round_name, + "event_id": event_id, + "message": "Scoring for this round is closed. Your score was not changed.", + }, 409 + + +def _actor_display_name(user_id): + """Best-effort human name for lock audit rows; never raises.""" + try: + from services.users_service import get_propel_user_details_by_id + details = get_propel_user_details_by_id(user_id) + if details and len(details) >= 5: + return details[4] or details[0] + except Exception as e: + debug(logger, "actor name lookup failed", error=str(e)) + return None + + @bp.route("/assignments/", methods=["GET"]) @auth.require_user def get_assignments(judge_id): @@ -151,6 +180,14 @@ def submit_score(): judge_id=user_id, team_id=data['team_id']) return {"error": "Forbidden: Judge not assigned to this team"}, 403 + if data['round'] not in JUDGING_ROUNDS: + return {"error": "invalid_round"}, 400 + + if is_round_locked(data['event_id'], data['round']): + info(logger, "Score rejected: round locked", judge_id=user_id, + team_id=data['team_id'], event_id=data['event_id'], round_name=data['round']) + return _round_locked_response(data['event_id'], data['round']) + debug(logger, "Submitting score", judge_id=data['judge_id'], team_id=data['team_id'], event_id=data['event_id']) @@ -244,6 +281,12 @@ def save_draft(): judge_id=user_id, team_id=data['team_id']) return {"error": "Forbidden: Judge not assigned to this team"}, 403 + if data['round'] not in JUDGING_ROUNDS: + return {"error": "invalid_round"}, 400 + + if is_round_locked(data['event_id'], data['round']): + return _round_locked_response(data['event_id'], data['round']) + debug(logger, "Saving draft score", judge_id=data['judge_id'], team_id=data['team_id'], event_id=data['event_id']) @@ -551,3 +594,88 @@ def get_bulk_scores_admin(event_id, round_name): return result, 500 return result + + +# --------------------------------------------------------------------------- +# Round locks + judge progress + bulk assignments (Oct 2026) +# --------------------------------------------------------------------------- + +@bp.route("/locks/", methods=["GET"]) +@auth.require_user +def get_locks_api(event_id): + """Lock state for both rounds. Any signed-in user (judges read it to + render a read-only scoring page); it carries no scores.""" + info(logger, "API called: GET /locks/{event_id}", event_id=event_id) + return {"locks": get_round_locks(event_id)} + + +@bp.route("/admin/locks//", methods=["POST"]) +@auth.require_user +@auth.require_org_member_with_permission("judge.admin", req_to_org_id=getOrgId) +def set_lock_api(event_id, round_name): + """Lock or unlock scoring for a round. Body: {"locked": true|false}.""" + info(logger, "API called: POST /admin/locks/{event_id}/{round_name}", + event_id=event_id, round_name=round_name) + user_id = get_authenticated_user_id() + if not user_id: + return {"error": "Unauthorized"}, 401 + if round_name not in JUDGING_ROUNDS: + return {"error": "invalid_round"}, 400 + data = request.get_json(silent=True) or {} + locked = data.get("locked") + if not isinstance(locked, bool): + return {"error": "locked_must_be_boolean"}, 400 + + result = set_round_lock(event_id, round_name, locked, + actor_id=user_id, actor_name=_actor_display_name(user_id)) + if not result.get("success"): + return result, 400 + return result + + +@bp.route("/admin/progress//", methods=["GET"]) +@auth.require_org_member_with_permission("judge.admin", req_to_org_id=getOrgId) +def get_progress_api(event_id, round_name): + """Who has submitted scores, who is mid-draft, who hasn't started — per + judge and per team — plus the round's lock state. One call.""" + info(logger, "API called: GET /admin/progress/{event_id}/{round_name}", + event_id=event_id, round_name=round_name) + if round_name not in JUDGING_ROUNDS: + return {"error": "invalid_round"}, 400 + result = get_judging_progress(event_id, round_name) + if "error" in result: + return result, 500 + return result + + +@bp.route("/admin/assignments/bulk", methods=["POST"]) +@auth.require_org_member_with_permission("judge.admin", req_to_org_id=getOrgId) +def bulk_assignments_api(): + """Create every (judge, team) assignment for a panel in one request. + Body: {event_id, round, panel_id, judge_ids[], team_ids[], room?, + demo_times?: {team_id: iso}, replace?: bool}.""" + info(logger, "API called: POST /admin/assignments/bulk") + data = request.get_json(silent=True) or {} + for field in ("event_id", "round", "panel_id"): + if not data.get(field): + return {"error": f"Missing required field: {field}"}, 400 + if data["round"] not in JUDGING_ROUNDS: + return {"error": "invalid_round"}, 400 + judge_ids = data.get("judge_ids") or [] + team_ids = data.get("team_ids") or [] + if not isinstance(judge_ids, list) or not isinstance(team_ids, list): + return {"error": "judge_ids and team_ids must be lists"}, 400 + + result = bulk_create_judge_assignments( + event_id=data["event_id"], + round_name=data["round"], + panel_id=data["panel_id"], + judge_ids=judge_ids, + team_ids=team_ids, + room=data.get("room"), + demo_times=data.get("demo_times"), + replace=bool(data.get("replace", False)), + ) + if not result.get("success"): + return result, 400 + return result diff --git a/api/judging/tests/test_judging_views_locks.py b/api/judging/tests/test_judging_views_locks.py new file mode 100644 index 0000000..54fdd13 --- /dev/null +++ b/api/judging/tests/test_judging_views_locks.py @@ -0,0 +1,172 @@ +""" +Route-level tests: a locked round rejects score + draft writes with 409 +`round_locked`, the lock/progress/bulk admin routes validate input, and every +view signature matches its URL params (copied from the submissions views +test — propelauth decorators don't catch that at import time). +""" +import functools +import importlib +import inspect +import os +import sys +import types +from unittest.mock import MagicMock + +os.environ.setdefault("ENVIRONMENT", "test") + +import pytest +from flask import Flask, g +from werkzeug.local import LocalProxy + +VIEWS_MODULE = "api.judging.judging_views" +FAKE_USER = types.SimpleNamespace(user_id="judge-1", email="judge@example.com") + + +def _passthrough_decorator_factory(*_args, **_kwargs): + def decorator(fn): + @functools.wraps(fn) + def wrapper(*args, **kwargs): + g.propelauth_current_user = FAKE_USER + return fn(*args, **kwargs) + return wrapper + return decorator + + +@pytest.fixture +def views(monkeypatch): + stub = types.ModuleType("common.auth") + stub.auth = types.SimpleNamespace( + require_org_member_with_permission=_passthrough_decorator_factory, + require_user=_passthrough_decorator_factory(), + optional_user=_passthrough_decorator_factory(), + ) + stub.auth_user = LocalProxy(lambda: g.propelauth_current_user) + stub.getOrgId = lambda req: req.headers.get("X-Org-Id") + monkeypatch.setitem(sys.modules, "common.auth", stub) + sys.modules.pop(VIEWS_MODULE, None) + module = importlib.import_module(VIEWS_MODULE) + yield module + sys.modules.pop(VIEWS_MODULE, None) + + +@pytest.fixture +def client(views): + app = Flask(__name__) + app.register_blueprint(views.bp) + return app.test_client() + + +HEADERS = {"Authorization": "Bearer test", "X-Org-Id": "org"} +SCORE_BODY = { + "judge_id": "judge-1", "team_id": "team-1", "event_id": "fall-2026", "round": "round1", + "scores": {"scopeImpact": 3}, "feedback": {"general": "x" * 30}, +} + + +def test_every_view_signature_matches_its_url_params(views): + app = Flask(__name__) + app.register_blueprint(views.bp) + mismatches = [] + for rule in app.url_map.iter_rules(): + if rule.endpoint == "static": + continue + view = inspect.unwrap(app.view_functions[rule.endpoint]) + declared = {name for name, p in inspect.signature(view).parameters.items() + if p.kind in (p.POSITIONAL_OR_KEYWORD, p.KEYWORD_ONLY)} + if declared != set(rule.arguments): + mismatches.append((rule.rule, sorted(declared), sorted(rule.arguments))) + assert mismatches == [] + + +def test_locked_round_rejects_score_submit_with_409(views, client, monkeypatch): + submit = MagicMock(return_value={"success": True}) + monkeypatch.setattr(views, "is_judge_assigned_to_team", lambda *_: True) + monkeypatch.setattr(views, "is_round_locked", lambda e, r: True) + monkeypatch.setattr(views, "submit_judge_score", submit) + + res = client.post("/api/judge/score", json=SCORE_BODY, headers=HEADERS) + assert res.status_code == 409 + assert res.get_json()["error"] == "round_locked" + assert res.get_json()["round"] == "round1" + submit.assert_not_called() + + +def test_unlocked_round_submits_score(views, client, monkeypatch): + submit = MagicMock(return_value={"success": True, "score_id": "s1"}) + monkeypatch.setattr(views, "is_judge_assigned_to_team", lambda *_: True) + monkeypatch.setattr(views, "is_round_locked", lambda e, r: False) + monkeypatch.setattr(views, "submit_judge_score", submit) + + res = client.post("/api/judge/score", json=SCORE_BODY, headers=HEADERS) + assert res.status_code == 200 + submit.assert_called_once() + + +def test_locked_round_rejects_draft_save_too(views, client, monkeypatch): + save = MagicMock(return_value={"success": True}) + monkeypatch.setattr(views, "is_judge_assigned_to_team", lambda *_: True) + monkeypatch.setattr(views, "is_round_locked", lambda e, r: True) + monkeypatch.setattr(views, "save_draft_score", save) + + res = client.post("/api/judge/draft", json=SCORE_BODY, headers=HEADERS) + assert res.status_code == 409 + save.assert_not_called() + + +def test_unknown_round_is_400_before_lock_check(views, client, monkeypatch): + monkeypatch.setattr(views, "is_judge_assigned_to_team", lambda *_: True) + called = MagicMock(return_value=False) + monkeypatch.setattr(views, "is_round_locked", called) + res = client.post("/api/judge/score", json={**SCORE_BODY, "round": "finals"}, headers=HEADERS) + assert res.status_code == 400 + called.assert_not_called() + + +def test_get_locks_route(views, client, monkeypatch): + monkeypatch.setattr(views, "get_round_locks", + lambda e: {"round1": {"locked": True}, "round2": {"locked": False}}) + res = client.get("/api/judge/locks/fall-2026", headers=HEADERS) + assert res.status_code == 200 + assert res.get_json()["locks"]["round1"]["locked"] is True + + +def test_set_lock_route_validates_and_delegates(views, client, monkeypatch): + setter = MagicMock(return_value={"success": True, "lock": {"locked": True}}) + monkeypatch.setattr(views, "set_round_lock", setter) + monkeypatch.setattr(views, "_actor_display_name", lambda uid: "Greg") + + assert client.post("/api/judge/admin/locks/fall-2026/round3", json={"locked": True}, + headers=HEADERS).status_code == 400 + assert client.post("/api/judge/admin/locks/fall-2026/round1", json={"locked": "yes"}, + headers=HEADERS).status_code == 400 + setter.assert_not_called() + + res = client.post("/api/judge/admin/locks/fall-2026/round2", json={"locked": True}, headers=HEADERS) + assert res.status_code == 200 + setter.assert_called_once_with("fall-2026", "round2", True, actor_id="judge-1", actor_name="Greg") + + +def test_progress_route(views, client, monkeypatch): + monkeypatch.setattr(views, "get_judging_progress", lambda e, r: {"judges": [], "teams": [], "summary": {}}) + assert client.get("/api/judge/admin/progress/fall-2026/round2", headers=HEADERS).status_code == 200 + assert client.get("/api/judge/admin/progress/fall-2026/nope", headers=HEADERS).status_code == 400 + + +def test_bulk_assignments_route_validates_and_delegates(views, client, monkeypatch): + bulk = MagicMock(return_value={"success": True, "created": 2}) + monkeypatch.setattr(views, "bulk_create_judge_assignments", bulk) + + assert client.post("/api/judge/admin/assignments/bulk", json={"event_id": "e", "round": "round1"}, + headers=HEADERS).status_code == 400 + assert client.post("/api/judge/admin/assignments/bulk", + json={"event_id": "e", "round": "round1", "panel_id": "p", "judge_ids": "j1"}, + headers=HEADERS).status_code == 400 + bulk.assert_not_called() + + body = {"event_id": "e", "round": "round2", "panel_id": "p", "judge_ids": ["j1"], "team_ids": ["t1"], + "room": "Main", "demo_times": {"t1": "2026-10-11T22:00:00-07:00"}, "replace": True} + res = client.post("/api/judge/admin/assignments/bulk", json=body, headers=HEADERS) + assert res.status_code == 200 + bulk.assert_called_once_with(event_id="e", round_name="round2", panel_id="p", judge_ids=["j1"], + team_ids=["t1"], room="Main", demo_times={"t1": "2026-10-11T22:00:00-07:00"}, + replace=True) diff --git a/api/judging/tests/test_round_locks_and_progress.py b/api/judging/tests/test_round_locks_and_progress.py new file mode 100644 index 0000000..64495a2 --- /dev/null +++ b/api/judging/tests/test_round_locks_and_progress.py @@ -0,0 +1,220 @@ +""" +Round locks, judge progress and bulk assignments (Oct 2026). + +Locks are stored in `judging_round_locks` and read uncached on every judge +write; progress is computed server-side from assignments + scores; bulk +assignment creation is idempotent. Service-level tests patch the db layer +exactly like the sibling judging tests do. +""" +import os +from datetime import datetime + +os.environ.setdefault("ENVIRONMENT", "test") + +from unittest.mock import patch + +import pytest + +import api.judging.judging_service as svc + + +# --------------------------------------------------------------------------- +# Tiny in-memory doc store standing in for the locks collection +# --------------------------------------------------------------------------- + +class _Snap: + def __init__(self, doc_id, data): + self.id = doc_id + self.exists = data is not None + self._data = data + + def to_dict(self): + return dict(self._data) if self._data is not None else None + + +class _DocRef: + def __init__(self, store, doc_id): + self._store, self._id = store, doc_id + + def get(self): + return _Snap(self._id, self._store.get(self._id)) + + def set(self, data, merge=False): + if merge and self._id in self._store: + self._store[self._id].update(data) + else: + self._store[self._id] = dict(data) + + +class _Collection: + def __init__(self, store): + self._store = store + + def document(self, doc_id): + return _DocRef(self._store, doc_id) + + +class _Db: + def __init__(self, stores): + self._stores = stores + + def collection(self, name): + return _Collection(self._stores.setdefault(name, {})) + + +@pytest.fixture +def lock_store(monkeypatch): + stores = {} + monkeypatch.setattr(svc, "_locks_db", lambda: _Db(stores)) + monkeypatch.setattr("common.utils.slack.send_slack_audit", lambda **kw: None, raising=False) + return stores + + +# --------------------------------------------------------------------------- +# Locks +# --------------------------------------------------------------------------- + +def test_lock_defaults_to_unlocked(lock_store): + assert svc.is_round_locked("fall-2026", "round1") is False + locks = svc.get_round_locks("fall-2026") + assert set(locks) == {"round1", "round2"} + assert locks["round2"]["locked"] is False + + +def test_set_lock_roundtrip_and_unlock_clears_actor(lock_store): + res = svc.set_round_lock("fall-2026", "round2", True, actor_id="p-1", actor_name="Greg") + assert res["success"] and res["lock"]["locked"] is True + assert res["lock"]["locked_by_name"] == "Greg" + assert svc.is_round_locked("fall-2026", "round2") is True + # round1 untouched + assert svc.is_round_locked("fall-2026", "round1") is False + + res = svc.set_round_lock("fall-2026", "round2", False, actor_id="p-1", actor_name="Greg") + assert res["lock"]["locked"] is False + assert res["lock"]["locked_by"] is None and res["lock"]["locked_at"] is None + + +def test_set_lock_rejects_bad_round_and_non_bool(lock_store): + assert svc.set_round_lock("fall-2026", "round3", True)["success"] is False + assert svc.set_round_lock("fall-2026", "round1", "yes")["success"] is False + + +def test_lock_read_error_fails_open(monkeypatch): + class Boom: + def collection(self, *_): + raise RuntimeError("firestore down") + monkeypatch.setattr(svc, "_locks_db", lambda: Boom()) + assert svc.is_round_locked("fall-2026", "round1") is False + + +# --------------------------------------------------------------------------- +# Progress +# --------------------------------------------------------------------------- + +def _assignment(judge_id, team_id, round_name="round1", panel_id="p1"): + return {"id": f"{judge_id}-{team_id}", "judge_id": judge_id, "team_id": team_id, + "round": round_name, "panel_id": panel_id, "event_id": "fall-2026"} + + +def _score(judge_id, team_id, is_draft, round_name="round1", when="2026-10-10T20:00:00+00:00"): + return {"judge_id": judge_id, "team_id": team_id, "round": round_name, "is_draft": is_draft, + "submitted_at": None if is_draft else when, "updated_at": when} + + +def test_progress_classifies_submitted_draft_and_missing(lock_store): + assignments = {"data": [ + _assignment("j1", "t1"), _assignment("j1", "t2"), _assignment("j1", "t3"), + _assignment("j2", "t1"), _assignment("j2", "t2"), + _assignment("j2", "t9", round_name="round2"), # other round — ignored + ]} + scores = {"data": [ + _score("j1", "t1", False), + _score("j1", "t1", True), # stale draft after submit — not "in progress" + _score("j1", "t2", True), # draft only + _score("j2", "t1", False), _score("j2", "t2", False), + _score("j3", "t1", False), # judge no longer assigned → orphan + ]} + judges = {"data": [ + {"user_id": "j1", "name": "Ada", "email": "ada@x.org", "isSelected": True}, + {"user_id": "j2", "name": "Bob", "email": "bob@x.org", "isSelected": True}, + ]} + teams = [{"id": "t1", "name": "Alpha"}, {"id": "t2", "name": "Beta"}, {"id": "t3", "name": "Gamma"}] + + with patch.object(svc, "fetch_judge_assignments_by_event_id", return_value=assignments), \ + patch.object(svc, "fetch_judge_scores_by_event_id", return_value=scores), \ + patch.object(svc, "get_volunteer_from_db_by_event", return_value=judges), \ + patch.object(svc, "get_teams_batch", return_value=teams): + result = svc.get_judging_progress("fall-2026", "round1") + + by_id = {j["judge_id"]: j for j in result["judges"]} + ada, bob = by_id["j1"], by_id["j2"] + assert ada["submitted_team_ids"] == ["t1"] + assert ada["draft_team_ids"] == ["t2"] + assert ada["missing_team_ids"] == ["t3"] + assert ada["complete"] is False and ada["assigned"] == 3 and ada["submitted"] == 1 + assert bob["complete"] is True and bob["missing_team_ids"] == [] + # incomplete judges sort first + assert result["judges"][0]["judge_id"] == "j1" + + teams_by = {t["team_id"]: t for t in result["teams"]} + assert teams_by["t1"]["submitted"] == 2 and teams_by["t1"]["name"] == "Alpha" + assert teams_by["t3"]["submitted"] == 0 + + s = result["summary"] + assert s["judges_total"] == 2 and s["judges_complete"] == 1 + assert s["assignments_total"] == 5 and s["submitted_total"] == 3 and s["drafts_total"] == 1 + assert s["orphan_submitted"] == 1 + assert result["lock"]["locked"] is False + + +def test_progress_rejects_unknown_round(lock_store): + assert "error" in svc.get_judging_progress("fall-2026", "finals") + + +# --------------------------------------------------------------------------- +# Bulk assignments +# --------------------------------------------------------------------------- + +def test_bulk_assign_creates_cross_product_once(lock_store): + inserted, deleted = [], [] + existing = {"data": [_assignment("j1", "t1")]} # already there → kept + + def _insert(a): + inserted.append((a.judge_id, a.team_id, a.panel_id, a.round, a.room, a.demo_time)) + a.id = f"new-{len(inserted)}" + return a + + with patch.object(svc, "fetch_judge_assignments_by_event_id", return_value=existing), \ + patch.object(svc, "insert_judge_assignment", side_effect=_insert), \ + patch.object(svc, "update_judge_assignment", side_effect=lambda a: a), \ + patch.object(svc, "delete_judge_assignment", side_effect=lambda i: deleted.append(i) or True): + res = svc.bulk_create_judge_assignments( + "fall-2026", "round1", "p1", ["j1", "j2", "j2"], ["t1", "t2"], room="Room A") + + assert res["success"] is True + assert res["created"] == 3 and res["kept"] == 1 and res["removed"] == 0 and res["total"] == 4 + pairs = sorted((i[0], i[1]) for i in inserted) + assert pairs == [("j1", "t2"), ("j2", "t1"), ("j2", "t2")] + assert all(i[2] == "p1" and i[3] == "round1" and i[4] == "Room A" for i in inserted) + assert deleted == [] + + +def test_bulk_assign_replace_prunes_pairs_outside_cross_product(lock_store): + existing = {"data": [_assignment("j1", "t1"), _assignment("j1", "tOLD"), _assignment("jGONE", "t1"), + _assignment("j1", "t1", round_name="round2", panel_id="p1")]} # other round untouched + deleted = [] + with patch.object(svc, "fetch_judge_assignments_by_event_id", return_value=existing), \ + patch.object(svc, "insert_judge_assignment", side_effect=lambda a: a), \ + patch.object(svc, "update_judge_assignment", side_effect=lambda a: a), \ + patch.object(svc, "delete_judge_assignment", side_effect=lambda i: deleted.append(i) or True): + res = svc.bulk_create_judge_assignments("fall-2026", "round1", "p1", ["j1"], ["t1"], replace=True) + + assert res["removed"] == 2 + assert sorted(deleted) == ["j1-tOLD", "jGONE-t1"] + + +def test_bulk_assign_validates_inputs(lock_store): + assert svc.bulk_create_judge_assignments("e", "round9", "p", ["j"], ["t"])["success"] is False + assert svc.bulk_create_judge_assignments("e", "round1", None, ["j"], ["t"])["success"] is False + too_many = [f"j{i}" for i in range(101)] # dedupe keeps all 101 distinct ids + assert svc.bulk_create_judge_assignments("e", "round1", "p", too_many, ["t"])["success"] is False diff --git a/api/judging/tests/test_teams_batch_chunking.py b/api/judging/tests/test_teams_batch_chunking.py new file mode 100644 index 0000000..dcd9b2f --- /dev/null +++ b/api/judging/tests/test_teams_batch_chunking.py @@ -0,0 +1,63 @@ +"""get_teams_batch must chunk `__name__ in [...]` by 30 — Firestore's cap — +or a judge/admin read over more teams than that raises and returns [].""" +import os + +os.environ.setdefault("ENVIRONMENT", "test") + +from unittest.mock import patch + +import services.teams_service as ts + + +class _Doc: + def __init__(self, doc_id): + self.id = doc_id + + def to_dict(self): + return {"name": f"Team {self.id}"} + + +class _Query: + def __init__(self, refs, calls): + self._refs, self._calls = refs, calls + + def stream(self): + self._calls.append(len(self._refs)) + return [_Doc(r) for r in self._refs] + + +class _Collection: + def __init__(self, calls): + self._calls = calls + + def document(self, doc_id): + return doc_id + + def where(self, field, op, refs): + assert field == "__name__" and op == "in" + assert len(refs) <= 30 + return _Query(refs, self._calls) + + +class _Db: + def __init__(self, calls): + self._calls = calls + + def collection(self, name): + assert name == "teams" + return _Collection(self._calls) + + +def test_get_teams_batch_chunks_by_30_and_dedupes(): + calls = [] + ids = [f"t{i}" for i in range(65)] + ["t0", "t1"] # dupes collapse + with patch.object(ts, "get_db", return_value=_Db(calls)), \ + patch.object(ts, "doc_to_json", side_effect=lambda docid, doc: {"id": docid, **doc.to_dict()}): + results = ts.get_teams_batch({"team_ids": ids}) + assert calls == [30, 30, 5] + assert len(results) == 65 + assert results[0]["name"] == "Team t0" + + +def test_get_teams_batch_empty_ids(): + assert ts.get_teams_batch({"team_ids": []}) == [] diff --git a/services/teams_service.py b/services/teams_service.py index d880ce2..1258bcd 100644 --- a/services/teams_service.py +++ b/services/teams_service.py @@ -225,13 +225,17 @@ def get_teams_batch(json): logger.warning("get_teams_batch end (no team_ids provided)") return [] try: - docs = db.collection('teams').where( - '__name__', 'in', [db.collection('teams').document(team_id) for team_id in team_ids]).stream() - + # Firestore caps `in` queries at 30 values; a judge assigned to more + # teams than that (or a bulk admin read) used to raise and return []. + unique_ids = [t for t in dict.fromkeys(team_ids) if t] results = [] - for doc in docs: - team_data = doc_to_json(docid=doc.id, doc=doc) - results.append(public_team_view(team_data)) + for start in range(0, len(unique_ids), 30): + chunk = unique_ids[start:start + 30] + docs = db.collection('teams').where( + '__name__', 'in', [db.collection('teams').document(team_id) for team_id in chunk]).stream() + for doc in docs: + team_data = doc_to_json(docid=doc.id, doc=doc) + results.append(public_team_view(team_data)) logger.debug(f"get_teams_batch end (with {len(results)} results)") return results From e166c323277e643ca8961bdec4e6284c6dafbc4a Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Wed, 21 Oct 2026 23:30:28 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix(judging):=20harden=20locks/bulk-assign?= =?UTF-8?q?=20from=20review=20=E2=80=94=20round-scoped=20assignment=20chec?= =?UTF-8?q?k,=20strict=20bulk=20input,=20raise=20on=20failed=20reads,=20pu?= =?UTF-8?q?blic=20lock=20view?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - /score and /draft now require an assignment for THIS event + round (is_judge_assigned_for_round); the any-round check let a Round-1-only judge post a Round 2 score that counted in results. - bulk-assign: replace must be a JSON bool, empty judge/team lists are rejected (empty_cross_product — replace would otherwise wipe the panel), ids must be non-empty strings, room/demo_times typed; replace collapses duplicate docs. - _rows() raises on a fetcher's {error} result instead of treating a failed Firestore read as no rows (bulk would re-insert everything; progress would show an empty board mid-round). - GET /locks and the judge teams payload use public_lock_view (no locked_by / locked_by_name — PropelAuth ids are PII); admins keep the full lock. - Dead except-TypeError branch removed; _locks_db renamed _db. - Tests: batched member lookup (one get_all, missing docs dropped, failure → per-user fallback), get_judge_teams shape (event + public locks + round2 slot preference), failed-read paths, bulk validation, duplicate collapse, round-scoped 403 at the route. Co-Authored-By: Claude Fable 5.1 --- CLAUDE.md | 2 + api/judging/README.md | 9 +- api/judging/judging_service.py | 121 ++++++++++---- api/judging/judging_views.py | 42 +++-- api/judging/tests/test_judging_views_locks.py | 63 ++++++- .../tests/test_round_locks_and_progress.py | 156 +++++++++++++++++- 6 files changed, 340 insertions(+), 53 deletions(-) diff --git a/CLAUDE.md b/CLAUDE.md index 8df42d5..dc8ffcf 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -276,6 +276,8 @@ Built for the "15 judges hitting /judge at once" case; rubric/rounds/results mat - **`POST /api/judge/admin/assignments/bulk`** (`judge.admin`) `{event_id, round, panel_id, judge_ids[], team_ids[], room?, demo_times?{team_id: iso}, replace?}` — creates every missing (judge, team) pair for the panel in one request, idempotent on `(panel_id, round, judge, team)`, re-syncs `room`/`demo_time` on kept pairs, and with `replace: true` deletes the panel's pairs outside the cross product (admin removed a finalist/judge and re-saved). **Scores are never touched by assignment changes.** Caps: 100 judges × 200 teams. - `get_judge_teams` now returns `event {title,start_date,end_date,timezone}` + `locks`, and resolves members through `_batch_users_for_teams` (ONE `db.get_all` for every member of every assigned team) with the old per-user `get_user_by_id` loop kept as the fallback when `users_by_id` is None (tests patch that path). `services/teams_service.py::get_teams_batch` chunks the `__name__ in [...]` query by 30 (Firestore's cap) and dedupes ids. - Tests: `api/judging/tests/test_round_locks_and_progress.py` (service), `test_judging_views_locks.py` (routes, auth-stubbed Flask client — includes the view-signature-vs-URL-params check), `test_teams_batch_chunking.py`. +- **Score/draft writes are assignment-checked per event AND round** (`is_judge_assigned_for_round`), not "any event, any round" (`is_judge_assigned_to_team`, still used by the read-only `/team/` route): the old check let a Round-1-only judge post a Round 2 score for a finalist they'd seen in Round 1, and it counted in Results. Locks are a fairness feature; don't loosen this back. +- **Judge-facing lock payloads are `public_lock_view`** (`locked`, `locked_at`, `updated_at` only) — `locked_by` is a PropelAuth id (PII per this file) and `locked_by_name` is admin-only (progress endpoint). **`_rows()` RAISES on a fetcher's `{"data": [], "error": …}`** instead of treating a failed Firestore read as "no rows" (that would have re-inserted every pair in bulk-assign and shown an empty progress board mid-round). **Bulk-assign refuses** `replace` that isn't a JSON bool (`"false"` is truthy), empty `judge_ids`/`team_ids` (`empty_cross_product` — an empty set with replace would wipe the panel; single removals go through `DELETE /assignments/`), non-string ids, non-string `room`, and non-`{str: str|None}` `demo_times`; with `replace` it also collapses duplicate docs of a kept pair (the old per-pair POSTs could create them). ### `GET /api/github/activity?org&repo` — team dashboard "Code activity" card `common/utils/github.py::get_repo_activity(org, repo)` makes **exactly 3** GitHub API calls (rate-limit budget, not completeness — this is a live-ish snapshot): `g.get_repo(f"{org}/{repo}")`, `repo.get_commits().get_page(0)` (first page only, ≤100 commits; a 409 "empty repository" is a valid all-zeros result for a fresh team repo, not an error), `repo.get_pulls(state="open").totalCount`. Contributors are derived from that single commit page (a `Counter` over each commit's author), not a separate stats call. `api/github/github_service.py::get_github_activity` validates `org`/`repo` against `^[A-Za-z0-9_.-]{1,100}$`, caches successes only for 300s (`_ACTIVITY_CACHE`, separate from the existing `_ISSUES_CACHE`), and translates `UnknownObjectException`→404 `repo_not_found`, `RateLimitExceededException`→503 `github_rate_limited`, anything else→502 `github_unavailable`. **Commits by `activity_excluded_logins()` (env `GITHUB_ACTIVITY_EXCLUDED_LOGINS`, default `gregv` — the GITHUB_TOKEN owner who seeds LICENSE + README via `create_github_repo`) are dropped before any counting**, matched on GitHub login or, for unlinked commits, the git author name; otherwise every new team repo showed 2 commits and the dashboard's "Push code" row was done before the team pushed anything. diff --git a/api/judging/README.md b/api/judging/README.md index 8f1bdb1..51250b3 100644 --- a/api/judging/README.md +++ b/api/judging/README.md @@ -417,9 +417,11 @@ Admins can close scoring for a round. While locked, `POST /api/judge/score` and **GET** `/api/judge/locks/{event_id}` — any authenticated user. ```json -{"locks": {"round1": {"locked": false, "locked_at": null, "locked_by_name": null}, "round2": {"locked": true, "locked_at": "2026-10-11T17:40:00-07:00", "locked_by_name": "Greg V"}}} +{"locks": {"round1": {"locked": false, "locked_at": null, "updated_at": null}, "round2": {"locked": true, "locked_at": "2026-10-11T17:40:00-07:00", "updated_at": "2026-10-11T17:40:00-07:00"}}} ``` +This is the public view: it never includes who locked the round. Admins see `locked_by` / `locked_by_name` in the progress response below. + `GET /api/judge/teams/{judge_id}/{event_id}` also embeds the same `locks` object and an `event` summary (`title`, `start_date`, `end_date`, `timezone`). ### Set a lock (Admin) @@ -451,4 +453,7 @@ Incomplete judges sort first. A draft that has since been submitted is not count "room": "Main Room", "demo_times": {"": "2026-10-11T16:40:00-07:00"}, "replace": true} ``` -Creates every missing (judge, team) pair for the panel in one request (idempotent). `replace: true` also deletes the panel's pairs that are no longer in the cross product. Scores are never modified. Response: `{"success": true, "created": 12, "kept": 63, "updated": 0, "removed": 3, "total": 75}`. +Creates every missing (judge, team) pair for the panel in one request (idempotent). `replace: true` (JSON boolean required) also deletes the panel's pairs that are no longer in the cross product and collapses duplicate docs. Scores are never modified. Validation (400): `replace_must_be_boolean`, `empty_cross_product` (both lists must be non-empty — an empty set with replace would wipe the panel), `ids_must_be_non_empty_strings`, `room_must_be_string`, `demo_times_must_map_team_id_to_iso_string`. Response: `{"success": true, "created": 12, "kept": 63, "updated": 0, "removed": 3, "total": 75}`. + +### Assignment check on writes +`POST /api/judge/score` and `POST /api/judge/draft` require an assignment for the judge **in that event and that round** (403 otherwise). Reads of a team (`GET /api/judge/team/`) still only require an assignment for the team in any round. diff --git a/api/judging/judging_service.py b/api/judging/judging_service.py index bb4b31e..d0f00c0 100644 --- a/api/judging/judging_service.py +++ b/api/judging/judging_service.py @@ -191,7 +191,7 @@ def get_judge_teams(judge_id: str, event_id: str) -> Dict: "round1_teams": round1_teams, "round2_teams": round2_teams, "event": _event_summary(event_id), - "locks": get_round_locks(event_id), + "locks": get_round_locks(event_id, public=True), } except Exception as e: @@ -418,6 +418,22 @@ def is_judge_assigned_to_team(judge_id: str, team_id: str) -> bool: return False +def is_judge_assigned_for_round(judge_id: str, event_id: str, team_id: str, + round_name: str) -> bool: + """Assignment check for score/draft WRITES: the judge must hold an + assignment for this team in THIS event and THIS round. The older + `is_judge_assigned_to_team` only asks "any event, any round", which let a + Round-1-only judge post a Round 2 score for a finalist they'd seen in + Round 1 — and that score counted in the final results.""" + try: + assignments = fetch_judge_assignments_by_event_and_judge(event_id, judge_id) + return any(a.team_id == team_id and a.round == round_name for a in assignments) + except Exception as e: + error(logger, "Error checking round assignment", judge_id=judge_id, + team_id=team_id, event_id=event_id, round_name=round_name, error=str(e)) + return False + + def save_draft_score(judge_id: str, team_id: str, event_id: str, round_name: str, scores_data: Dict, updated_at: str, feedback: str) -> Dict: """Save draft scores (auto-save functionality).""" @@ -514,7 +530,7 @@ def _batch_users_for_teams(teams: list) -> Dict: if not ids: return {} try: - db = _locks_db() + db = _db() refs = [db.collection("users").document(uid) for uid in ids] out = {} for snap in db.get_all(refs): @@ -1109,7 +1125,7 @@ def get_bulk_judge_details(event_id: str) -> Dict: BULK_ASSIGN_MAX_TEAMS = 200 -def _locks_db(): +def _db(): # Lazy import: common.utils.firebase builds Firebase credentials at import # time; keeping it inside the function keeps judging_service importable in # unit tests that patch it. @@ -1145,7 +1161,7 @@ def _normalize_lock(raw: Dict, event_id: str, round_name: str) -> Dict: def get_round_lock(event_id: str, round_name: str) -> Dict: """Current lock state for one round. Missing doc ⇒ unlocked.""" try: - snap = _locks_db().collection(JUDGING_LOCKS_COLLECTION).document( + snap = _db().collection(JUDGING_LOCKS_COLLECTION).document( _lock_doc_id(event_id, round_name)).get() raw = snap.to_dict() if getattr(snap, "exists", False) else None return _normalize_lock(raw, event_id, round_name) @@ -1157,9 +1173,22 @@ def get_round_lock(event_id: str, round_name: str) -> Dict: return _empty_lock(event_id, round_name) -def get_round_locks(event_id: str) -> Dict: - """`{"round1": lock, "round2": lock}` for the judge dashboard + admin.""" - return {r: get_round_lock(event_id, r) for r in JUDGING_ROUNDS} +PUBLIC_LOCK_FIELDS = ("event_id", "round", "locked", "locked_at", "updated_at") + + +def public_lock_view(lock: Dict) -> Dict: + """What a judge (any signed-in user) may see: never the locking admin's + PropelAuth id or name — only whether/when the round closed.""" + return {k: lock.get(k) for k in PUBLIC_LOCK_FIELDS} + + +def get_round_locks(event_id: str, public: bool = False) -> Dict: + """`{"round1": lock, "round2": lock}`. `public=True` strips the actor + fields for judge-facing responses; admins get the full lock.""" + locks = {r: get_round_lock(event_id, r) for r in JUDGING_ROUNDS} + if public: + return {r: public_lock_view(lock) for r, lock in locks.items()} + return locks def is_round_locked(event_id: str, round_name: str) -> bool: @@ -1190,7 +1219,7 @@ def set_round_lock(event_id: str, round_name: str, locked: bool, else: doc.update({"locked_at": None, "locked_by": None, "locked_by_name": None}) - _locks_db().collection(JUDGING_LOCKS_COLLECTION).document( + _db().collection(JUDGING_LOCKS_COLLECTION).document( _lock_doc_id(event_id, round_name)).set(doc, merge=True) try: @@ -1211,10 +1240,20 @@ def set_round_lock(event_id: str, round_name: str, locked: bool, return {"success": False, "error": "Failed to update lock"} +class JudgingReadError(RuntimeError): + """A db fetcher reported a failure instead of rows.""" + + def _rows(result) -> list: - """The event-scoped db fetchers return {"data": [...]}; tolerate a bare - list. Only dict rows are returned (keeps pylint's inference honest too).""" + """The event-scoped db fetchers return {"data": [...]} — or + {"data": [], "error": ...} when the read FAILED. Treating a failed read as + "no rows" would make bulk-assign re-insert every pair (and prune nothing) + and would show an admin an empty progress board mid-round, so raise + instead; callers fall through to their error paths. Bare lists are + tolerated. Only dict rows are returned.""" if isinstance(result, dict): + if result.get("error"): + raise JudgingReadError(str(result.get("error"))) result = result.get("data") or [] return [row for row in (result or []) if isinstance(row, dict)] @@ -1233,14 +1272,8 @@ def _judge_directory(event_id: str) -> Dict: "email": judge.get("email") or "", "is_selected": bool(judge.get("isSelected")), } - except TypeError: - # Older signature without the admin kwarg. - for judge in _rows(get_volunteer_from_db_by_event(event_id, "judge")): - uid = judge.get("user_id") - if uid: - directory[uid] = {"name": judge.get("name") or "", "email": judge.get("email") or "", - "is_selected": bool(judge.get("isSelected"))} except Exception as e: + # Names are decoration on the progress board; never fail it over them. warning(logger, "Judge directory lookup failed", event_id=event_id, error=str(e)) return directory @@ -1396,26 +1429,46 @@ def bulk_create_judge_assignments(event_id: str, round_name: str, panel_id: str, admin removed a judge/finalist and re-saved). Scores are never touched.""" if round_name not in JUDGING_ROUNDS: return {"success": False, "error": "invalid_round"} - if not panel_id: + if not isinstance(panel_id, str) or not panel_id: return {"success": False, "error": "panel_id_required"} - judge_ids = [j for j in dict.fromkeys(judge_ids or []) if j] - team_ids = [t for t in dict.fromkeys(team_ids or []) if t] + if not isinstance(replace, bool): + return {"success": False, "error": "replace_must_be_boolean"} + if not isinstance(judge_ids, list) or not isinstance(team_ids, list): + return {"success": False, "error": "judge_ids_and_team_ids_must_be_lists"} + if not all(isinstance(j, str) and j for j in judge_ids) or \ + not all(isinstance(t, str) and t for t in team_ids): + return {"success": False, "error": "ids_must_be_non_empty_strings"} + judge_ids = list(dict.fromkeys(judge_ids)) + team_ids = list(dict.fromkeys(team_ids)) if len(judge_ids) > BULK_ASSIGN_MAX_JUDGES or len(team_ids) > BULK_ASSIGN_MAX_TEAMS: return {"success": False, "error": "too_many_items"} - demo_times = demo_times if isinstance(demo_times, dict) else {} + if not judge_ids or not team_ids: + # An empty cross product with replace=True would wipe the panel; a + # frontend bug must never be able to do that. Removing single judges + # or teams goes through the explicit DELETE route instead. + return {"success": False, "error": "empty_cross_product"} + if room is not None and not isinstance(room, str): + return {"success": False, "error": "room_must_be_string"} + if demo_times is None: + demo_times = {} + if not isinstance(demo_times, dict) or not all( + isinstance(k, str) and (v is None or isinstance(v, str)) for k, v in demo_times.items()): + return {"success": False, "error": "demo_times_must_map_team_id_to_iso_string"} try: existing = [a for a in _rows(fetch_judge_assignments_by_event_id(event_id)) if a.get("round") == round_name and a.get("panel_id") == panel_id] + # Every doc per pair (duplicates can exist from the old per-pair POSTs). existing_by_pair = {} for a in existing: - existing_by_pair.setdefault((a.get("judge_id"), a.get("team_id")), a) + existing_by_pair.setdefault((a.get("judge_id"), a.get("team_id")), []).append(a) wanted = {(j, t) for j in judge_ids for t in team_ids} - created, kept, updated = 0, 0, 0 + created, kept, updated, removed = 0, 0, 0, 0 for (judge_id, team_id) in sorted(wanted): - current = existing_by_pair.get((judge_id, team_id)) - if current: + docs = existing_by_pair.get((judge_id, team_id)) or [] + if docs: + current = docs[0] kept += 1 # Keep room/demo_time in sync when the admin changed them. new_room = room if room is not None else current.get("room") @@ -1424,6 +1477,12 @@ def bulk_create_judge_assignments(event_id: str, round_name: str, panel_id: str, assignment = JudgeAssignment.deserialize({**current, "room": new_room, "demo_time": new_demo}) update_judge_assignment(assignment) updated += 1 + if replace: + # Collapse duplicates of a wanted pair down to one doc. + for dup in docs[1:]: + if dup.get("id"): + delete_judge_assignment(dup["id"]) + removed += 1 continue assignment = JudgeAssignment() assignment.judge_id = judge_id @@ -1436,12 +1495,14 @@ def bulk_create_judge_assignments(event_id: str, round_name: str, panel_id: str, insert_judge_assignment(assignment) created += 1 - removed = 0 if replace: - for pair, a in existing_by_pair.items(): - if pair not in wanted and a.get("id"): - delete_judge_assignment(a["id"]) - removed += 1 + for pair, docs in existing_by_pair.items(): + if pair in wanted: + continue + for a in docs: + if a.get("id"): + delete_judge_assignment(a["id"]) + removed += 1 return {"success": True, "created": created, "kept": kept, "updated": updated, "removed": removed, "total": len(wanted)} diff --git a/api/judging/judging_views.py b/api/judging/judging_views.py index 4edbd23..fec9886 100644 --- a/api/judging/judging_views.py +++ b/api/judging/judging_views.py @@ -24,6 +24,7 @@ get_bulk_judge_scores, get_bulk_judge_details, get_round_locks, + is_judge_assigned_for_round, is_round_locked, set_round_lock, get_judging_progress, @@ -174,15 +175,17 @@ def submit_score(): user_id=user_id, requested_judge_id=data['judge_id']) return {"error": "Forbidden"}, 403 - # Verify judge is assigned to this team - if not is_judge_assigned_to_team(user_id, data['team_id']): - error(logger, "Judge attempting to score unassigned team", - judge_id=user_id, team_id=data['team_id']) - return {"error": "Forbidden: Judge not assigned to this team"}, 403 - if data['round'] not in JUDGING_ROUNDS: return {"error": "invalid_round"}, 400 + # Verify judge is assigned to this team for THIS event + round (a + # Round-1-only judge must not be able to post a Round 2 score). + if not is_judge_assigned_for_round(user_id, data['event_id'], data['team_id'], data['round']): + error(logger, "Judge attempting to score unassigned team/round", + judge_id=user_id, team_id=data['team_id'], event_id=data['event_id'], + round_name=data['round']) + return {"error": "Forbidden: Judge not assigned to this team for this round"}, 403 + if is_round_locked(data['event_id'], data['round']): info(logger, "Score rejected: round locked", judge_id=user_id, team_id=data['team_id'], event_id=data['event_id'], round_name=data['round']) @@ -275,15 +278,16 @@ def save_draft(): user_id=user_id, requested_judge_id=data['judge_id']) return {"error": "Forbidden"}, 403 - # Verify judge is assigned to this team - if not is_judge_assigned_to_team(user_id, data['team_id']): - error(logger, "Judge attempting to save draft for unassigned team", - judge_id=user_id, team_id=data['team_id']) - return {"error": "Forbidden: Judge not assigned to this team"}, 403 - if data['round'] not in JUDGING_ROUNDS: return {"error": "invalid_round"}, 400 + # Same round-scoped assignment check as /score. + if not is_judge_assigned_for_round(user_id, data['event_id'], data['team_id'], data['round']): + error(logger, "Judge attempting to save draft for unassigned team/round", + judge_id=user_id, team_id=data['team_id'], event_id=data['event_id'], + round_name=data['round']) + return {"error": "Forbidden: Judge not assigned to this team for this round"}, 403 + if is_round_locked(data['event_id'], data['round']): return _round_locked_response(data['event_id'], data['round']) @@ -606,7 +610,7 @@ def get_locks_api(event_id): """Lock state for both rounds. Any signed-in user (judges read it to render a read-only scoring page); it carries no scores.""" info(logger, "API called: GET /locks/{event_id}", event_id=event_id) - return {"locks": get_round_locks(event_id)} + return {"locks": get_round_locks(event_id, public=True)} @bp.route("/admin/locks//", methods=["POST"]) @@ -665,6 +669,16 @@ def bulk_assignments_api(): team_ids = data.get("team_ids") or [] if not isinstance(judge_ids, list) or not isinstance(team_ids, list): return {"error": "judge_ids and team_ids must be lists"}, 400 + if not all(isinstance(j, str) and j for j in judge_ids) or \ + not all(isinstance(t, str) and t for t in team_ids): + return {"error": "ids_must_be_non_empty_strings"}, 400 + replace = data.get("replace", False) + if not isinstance(replace, bool): + # "false" (a string) is truthy — a bool is required so a client bug + # can never turn an additive save into a destructive one. + return {"error": "replace_must_be_boolean"}, 400 + if not judge_ids or not team_ids: + return {"error": "empty_cross_product"}, 400 result = bulk_create_judge_assignments( event_id=data["event_id"], @@ -674,7 +688,7 @@ def bulk_assignments_api(): team_ids=team_ids, room=data.get("room"), demo_times=data.get("demo_times"), - replace=bool(data.get("replace", False)), + replace=replace, ) if not result.get("success"): return result, 400 diff --git a/api/judging/tests/test_judging_views_locks.py b/api/judging/tests/test_judging_views_locks.py index 54fdd13..2e66b4a 100644 --- a/api/judging/tests/test_judging_views_locks.py +++ b/api/judging/tests/test_judging_views_locks.py @@ -80,7 +80,7 @@ def test_every_view_signature_matches_its_url_params(views): def test_locked_round_rejects_score_submit_with_409(views, client, monkeypatch): submit = MagicMock(return_value={"success": True}) - monkeypatch.setattr(views, "is_judge_assigned_to_team", lambda *_: True) + monkeypatch.setattr(views, "is_judge_assigned_for_round", lambda *_: True) monkeypatch.setattr(views, "is_round_locked", lambda e, r: True) monkeypatch.setattr(views, "submit_judge_score", submit) @@ -93,7 +93,7 @@ def test_locked_round_rejects_score_submit_with_409(views, client, monkeypatch): def test_unlocked_round_submits_score(views, client, monkeypatch): submit = MagicMock(return_value={"success": True, "score_id": "s1"}) - monkeypatch.setattr(views, "is_judge_assigned_to_team", lambda *_: True) + monkeypatch.setattr(views, "is_judge_assigned_for_round", lambda *_: True) monkeypatch.setattr(views, "is_round_locked", lambda e, r: False) monkeypatch.setattr(views, "submit_judge_score", submit) @@ -104,7 +104,7 @@ def test_unlocked_round_submits_score(views, client, monkeypatch): def test_locked_round_rejects_draft_save_too(views, client, monkeypatch): save = MagicMock(return_value={"success": True}) - monkeypatch.setattr(views, "is_judge_assigned_to_team", lambda *_: True) + monkeypatch.setattr(views, "is_judge_assigned_for_round", lambda *_: True) monkeypatch.setattr(views, "is_round_locked", lambda e, r: True) monkeypatch.setattr(views, "save_draft_score", save) @@ -114,7 +114,7 @@ def test_locked_round_rejects_draft_save_too(views, client, monkeypatch): def test_unknown_round_is_400_before_lock_check(views, client, monkeypatch): - monkeypatch.setattr(views, "is_judge_assigned_to_team", lambda *_: True) + monkeypatch.setattr(views, "is_judge_assigned_for_round", lambda *_: True) called = MagicMock(return_value=False) monkeypatch.setattr(views, "is_round_locked", called) res = client.post("/api/judge/score", json={**SCORE_BODY, "round": "finals"}, headers=HEADERS) @@ -124,7 +124,7 @@ def test_unknown_round_is_400_before_lock_check(views, client, monkeypatch): def test_get_locks_route(views, client, monkeypatch): monkeypatch.setattr(views, "get_round_locks", - lambda e: {"round1": {"locked": True}, "round2": {"locked": False}}) + lambda e, public=False: {"round1": {"locked": True}, "round2": {"locked": False}}) res = client.get("/api/judge/locks/fall-2026", headers=HEADERS) assert res.status_code == 200 assert res.get_json()["locks"]["round1"]["locked"] is True @@ -170,3 +170,56 @@ def test_bulk_assignments_route_validates_and_delegates(views, client, monkeypat bulk.assert_called_once_with(event_id="e", round_name="round2", panel_id="p", judge_ids=["j1"], team_ids=["t1"], room="Main", demo_times={"t1": "2026-10-11T22:00:00-07:00"}, replace=True) + + +def test_score_requires_an_assignment_for_this_event_and_round(views, client, monkeypatch): + """A Round-1-only judge posting round2 must be refused (403), not scored.""" + calls = [] + + def _assigned(judge_id, event_id, team_id, round_name): + calls.append((judge_id, event_id, team_id, round_name)) + return round_name == "round1" + + submit = MagicMock(return_value={"success": True}) + monkeypatch.setattr(views, "is_judge_assigned_for_round", _assigned) + monkeypatch.setattr(views, "is_round_locked", lambda e, r: False) + monkeypatch.setattr(views, "submit_judge_score", submit) + + res = client.post("/api/judge/score", json={**SCORE_BODY, "round": "round2"}, headers=HEADERS) + assert res.status_code == 403 + assert calls == [("judge-1", "fall-2026", "team-1", "round2")] + submit.assert_not_called() + + res = client.post("/api/judge/score", json=SCORE_BODY, headers=HEADERS) + assert res.status_code == 200 + submit.assert_called_once() + + +def test_bulk_assignments_route_rejects_string_replace_and_empty_lists(views, client, monkeypatch): + bulk = MagicMock(return_value={"success": True}) + monkeypatch.setattr(views, "bulk_create_judge_assignments", bulk) + base = {"event_id": "e", "round": "round1", "panel_id": "p", "judge_ids": ["j1"], "team_ids": ["t1"]} + + res = client.post("/api/judge/admin/assignments/bulk", json={**base, "replace": "false"}, headers=HEADERS) + assert res.status_code == 400 and res.get_json()["error"] == "replace_must_be_boolean" + + res = client.post("/api/judge/admin/assignments/bulk", json={**base, "team_ids": [], "replace": True}, headers=HEADERS) + assert res.status_code == 400 and res.get_json()["error"] == "empty_cross_product" + + res = client.post("/api/judge/admin/assignments/bulk", json={**base, "judge_ids": [{"id": 1}]}, headers=HEADERS) + assert res.status_code == 400 and res.get_json()["error"] == "ids_must_be_non_empty_strings" + + bulk.assert_not_called() + + +def test_public_locks_route_uses_public_view(views, client, monkeypatch): + seen = {} + + def _locks(event_id, public=False): + seen["public"] = public + return {"round1": {"locked": True}, "round2": {"locked": False}} + + monkeypatch.setattr(views, "get_round_locks", _locks) + res = client.get("/api/judge/locks/fall-2026", headers=HEADERS) + assert res.status_code == 200 + assert seen["public"] is True diff --git a/api/judging/tests/test_round_locks_and_progress.py b/api/judging/tests/test_round_locks_and_progress.py index 64495a2..a73ccc6 100644 --- a/api/judging/tests/test_round_locks_and_progress.py +++ b/api/judging/tests/test_round_locks_and_progress.py @@ -65,7 +65,7 @@ def collection(self, name): @pytest.fixture def lock_store(monkeypatch): stores = {} - monkeypatch.setattr(svc, "_locks_db", lambda: _Db(stores)) + monkeypatch.setattr(svc, "_db", lambda: _Db(stores)) monkeypatch.setattr("common.utils.slack.send_slack_audit", lambda **kw: None, raising=False) return stores @@ -103,7 +103,7 @@ def test_lock_read_error_fails_open(monkeypatch): class Boom: def collection(self, *_): raise RuntimeError("firestore down") - monkeypatch.setattr(svc, "_locks_db", lambda: Boom()) + monkeypatch.setattr(svc, "_db", lambda: Boom()) assert svc.is_round_locked("fall-2026", "round1") is False @@ -218,3 +218,155 @@ def test_bulk_assign_validates_inputs(lock_store): assert svc.bulk_create_judge_assignments("e", "round1", None, ["j"], ["t"])["success"] is False too_many = [f"j{i}" for i in range(101)] # dedupe keeps all 101 distinct ids assert svc.bulk_create_judge_assignments("e", "round1", "p", too_many, ["t"])["success"] is False + + +# --------------------------------------------------------------------------- +# Review follow-ups (Oct 2026): public lock view, failed reads, strict bulk input +# --------------------------------------------------------------------------- + +def test_public_lock_view_never_exposes_the_locking_admin(lock_store): + svc.set_round_lock("fall-2026", "round1", True, actor_id="propel-uuid", actor_name="Greg") + public = svc.get_round_locks("fall-2026", public=True)["round1"] + assert public["locked"] is True and public["locked_at"] + assert "locked_by" not in public and "locked_by_name" not in public + admin = svc.get_round_locks("fall-2026")["round1"] + assert admin["locked_by"] == "propel-uuid" + + +def test_progress_fails_loudly_when_the_assignment_read_failed(lock_store): + failed = {"data": [], "error": "deadline exceeded"} + with patch.object(svc, "fetch_judge_assignments_by_event_id", return_value=failed), \ + patch.object(svc, "fetch_judge_scores_by_event_id", return_value={"data": []}): + result = svc.get_judging_progress("fall-2026", "round1") + assert "error" in result and "judges" not in result + + +def test_bulk_assign_does_not_reinsert_when_the_existing_read_failed(lock_store): + inserted = [] + failed = {"data": [], "error": "deadline exceeded"} + with patch.object(svc, "fetch_judge_assignments_by_event_id", return_value=failed), \ + patch.object(svc, "insert_judge_assignment", side_effect=lambda a: inserted.append(a) or a): + res = svc.bulk_create_judge_assignments("fall-2026", "round1", "p1", ["j1"], ["t1"], replace=True) + assert res["success"] is False + assert inserted == [] + + +def test_bulk_assign_rejects_empty_cross_product_and_bad_types(lock_store): + assert svc.bulk_create_judge_assignments("e", "round1", "p", [], ["t"], replace=True)["error"] == "empty_cross_product" + assert svc.bulk_create_judge_assignments("e", "round1", "p", ["j"], [])["error"] == "empty_cross_product" + assert svc.bulk_create_judge_assignments("e", "round1", "p", ["j"], ["t"], replace="false")["error"] == "replace_must_be_boolean" + assert svc.bulk_create_judge_assignments("e", "round1", "p", [{"id": 1}], ["t"])["error"] == "ids_must_be_non_empty_strings" + assert svc.bulk_create_judge_assignments("e", "round1", "p", ["j"], [7])["error"] == "ids_must_be_non_empty_strings" + assert svc.bulk_create_judge_assignments("e", "round1", "p", ["j"], ["t"], room=5)["error"] == "room_must_be_string" + assert svc.bulk_create_judge_assignments("e", "round1", "p", ["j"], ["t"], demo_times={"t": 5})["error"].startswith("demo_times") + + +def test_bulk_assign_replace_collapses_duplicate_docs(lock_store): + existing = {"data": [ + {**_assignment("j1", "t1"), "id": "dup-a"}, + {**_assignment("j1", "t1"), "id": "dup-b"}, + {**_assignment("j1", "tOLD"), "id": "old-a"}, + {**_assignment("j1", "tOLD"), "id": "old-b"}, + ]} + deleted = [] + with patch.object(svc, "fetch_judge_assignments_by_event_id", return_value=existing), \ + patch.object(svc, "insert_judge_assignment", side_effect=lambda a: a), \ + patch.object(svc, "update_judge_assignment", side_effect=lambda a: a), \ + patch.object(svc, "delete_judge_assignment", side_effect=lambda i: deleted.append(i) or True): + res = svc.bulk_create_judge_assignments("fall-2026", "round1", "p1", ["j1"], ["t1"], replace=True) + assert res["kept"] == 1 and res["created"] == 0 + assert sorted(deleted) == ["dup-b", "old-a", "old-b"] + assert res["removed"] == 3 + + +# --------------------------------------------------------------------------- +# Round-scoped assignment check + batched member lookup + get_judge_teams shape +# --------------------------------------------------------------------------- + +class _A: + def __init__(self, team_id, round_name, demo_time=None, room=None): + self.team_id, self.round, self.demo_time, self.room = team_id, round_name, demo_time, room + + +def test_is_judge_assigned_for_round_matches_team_and_round(): + with patch.object(svc, "fetch_judge_assignments_by_event_and_judge", + return_value=[_A("t1", "round1"), _A("t2", "round2")]): + assert svc.is_judge_assigned_for_round("j", "e", "t1", "round1") is True + assert svc.is_judge_assigned_for_round("j", "e", "t1", "round2") is False + assert svc.is_judge_assigned_for_round("j", "e", "t2", "round2") is True + assert svc.is_judge_assigned_for_round("j", "e", "t9", "round1") is False + + +class _UserSnap: + def __init__(self, uid, data): + self.id, self._data, self.exists = uid, data, data is not None + + def to_dict(self): + return dict(self._data) if self._data else None + + +class _UsersDb: + def __init__(self, users): + self._users = users + self.requested = [] + + def collection(self, name): + assert name == "users" + return self + + def document(self, uid): + return uid + + def get_all(self, refs): + self.requested.append(list(refs)) + return [_UserSnap(uid, self._users.get(uid)) for uid in refs] + + +def test_batch_users_for_teams_one_round_trip_and_missing_docs_dropped(monkeypatch): + db = _UsersDb({"u1": {"name": "Ada", "email_address": "ada@x.org", "profile_image": "img"}, + "u2": {"name": "Bob"}}) + monkeypatch.setattr(svc, "_db", lambda: db) + teams = [{"id": "t1", "users": [{"id": "u1"}, "u2", "u1"]}, {"id": "t2", "users": ["u3", {"id": "u2"}]}] + users = svc._batch_users_for_teams(teams) + assert db.requested == [["u1", "u2", "u3"]] # deduped, ONE get_all + assert users == { + "u1": {"id": "u1", "name": "Ada", "email": "ada@x.org", "profile_image": "img"}, + "u2": {"id": "u2", "name": "Bob", "email": "", "profile_image": ""}, + } + # Same member shape as the per-user path; missing u3 is dropped; in-team order kept. + formatted = svc.format_team_for_judge(teams[0], users_by_id=users) + assert [m["id"] for m in formatted["members"]] == ["u1", "u2", "u1"] + assert formatted["members"][0] == users["u1"] + assert svc.format_team_for_judge(teams[1], users_by_id=users)["members"] == [users["u2"]] + + +def test_batch_users_for_teams_falls_back_to_per_user_reads_on_failure(monkeypatch): + class Boom: + def collection(self, *_): + raise RuntimeError("get_all down") + monkeypatch.setattr(svc, "_db", lambda: Boom()) + assert svc._batch_users_for_teams([{"id": "t1", "users": ["u1"]}]) is None + # None → format_team_for_judge takes the legacy per-user path. + with patch("common.utils.firebase.get_user_by_id", return_value={"id": "u1", "name": "Ada"}) as m: + out = svc.format_team_for_judge({"id": "t1", "users": ["u1"]}, users_by_id=None) + m.assert_called_once_with("u1") + assert out["members"][0]["name"] == "Ada" + + +def test_get_judge_teams_embeds_event_and_public_locks_and_prefers_round2_slot(lock_store, monkeypatch): + svc.set_round_lock("fall-2026", "round2", True, actor_id="p", actor_name="Greg") + assignments = [_A("t1", "round1"), _A("t1", "round2", demo_time="2026-10-11T23:00:00-07:00", room="Main")] + monkeypatch.setattr(svc, "_batch_users_for_teams", lambda teams: {}) + with patch.object(svc, "fetch_judge_assignments_by_event_and_judge", return_value=assignments), \ + patch.object(svc, "get_teams_batch", return_value=[{"id": "t1", "name": "Alpha", "users": []}]), \ + patch.object(svc, "fetch_judge_scores_by_judge_and_event", return_value=[]), \ + patch.object(svc, "get_nonprofit_by_id", return_value={"name": "NP"}), \ + patch.object(svc, "get_single_hackathon_event", return_value={"title": "Fall 2026", "timezone": "America/Phoenix"}): + result = svc.get_judge_teams("judge-1", "fall-2026") + + assert result["event"]["title"] == "Fall 2026" + assert result["locks"]["round2"]["locked"] is True + assert "locked_by" not in result["locks"]["round2"] + assert result["round1_teams"][0]["demo_time"] is None + r2 = result["round2_teams"][0] + assert r2["demo_time"] == "2026-10-11T23:00:00-07:00" and r2["room"] == "Main"