From 362a21fec36dc1bbc716251f6426f5516bdccec7 Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Sat, 19 Sep 2026 17:42:04 -0700 Subject: [PATCH 01/11] Plan: project submissions, deadlines, mentor availability, GitHub activity, Hackers' Choice (WS-A) Backend slice of the DevPost-replacement program: data contracts + API table, ordered tasks, and the bugs-found table (incl. the missing membership check on /devpost and /demo-video and the demo video never reaching judges). Full plan lives in frontend-ohack.dev/docs/plans/team-dashboard-devpost-replacement.md. Co-Authored-By: Claude Fable 5.1 --- docs/plans/submissions-peer-vote.md | 67 +++++++++++++++++++++++++++++ 1 file changed, 67 insertions(+) create mode 100644 docs/plans/submissions-peer-vote.md diff --git a/docs/plans/submissions-peer-vote.md b/docs/plans/submissions-peer-vote.md new file mode 100644 index 0000000..7483860 --- /dev/null +++ b/docs/plans/submissions-peer-vote.md @@ -0,0 +1,67 @@ +# Backend plan: project submissions, deadlines, mentor availability, GitHub activity, Hackers' Choice (WS-A) + +**For execution by Sonnet 5.** This is the backend slice of the DevPost-replacement program. The full plan (product design, all workstreams, verification) lives in the frontend repo at `frontend-ohack.dev/docs/plans/team-dashboard-devpost-replacement.md`, with detailed backend steps in `…/team-dashboard-devpost-replacement.appendix.md` (Appendix A). Where the appendix conflicts with Part 3 below, Part 3 wins. Judging process is untouched except for surfacing `demo_video_url` to judges. Conventions: `messages_views`/`messages_service` are frozen — new routes live in the new `api/submissions/` and `api/peer_votes/` blueprints; Python 3.9 typing; `ENVIRONMENT=test pytest api//tests`. + +## Part 3 — Data contracts & API (reconciled; single source for all workstreams) + +**`teams/{id}`** new keys (absent ⇒ default): `project_tagline` str≤140 · `project_story` markdown ≤20000 (server strips `script|iframe|object|embed|style|link|meta|form|base` tags, `on*=` attrs, `javascript:`/`data:` targets; generic `<`/`List` preserved; frontend renders via react-markdown **without** rehype-raw) · `project_built_with` [str≤30]≤25 · `project_links` [{label≤40,url https≤2048}]≤10 · `project_thumbnail_url` (must start with `{CDN_SERVER}/teams/{team_id}/`) · `project_images` [own-CDN url]≤8 · `project_updated_at` · `project_submitted_at` ISO|null · `project_submission_status` `draft|submitted|late` (**absent ⇒ legacy: frontend renders no submission tag**) · `mentor_help_wanted` bool (**absent ⇒ true**) + `_updated_at/_by_name`. +**`hackathons/{doc}`**: `deadlines` (above; update path writes `DELETE_FIELD` for explicit nulls); `constraints.peer_vote_enabled` bool (default false), `peer_vote_slate_size` int 3–10 (default 5), `peer_vote_max_picks` int 1–(slate−1) (default 2), `peer_vote_requires_submission` bool; `reminders_sent { "submission_24h": {sent_at, deadline, teams_notified, by} }`. +**`peer_votes/{event_id}__{safe_propel_id}`**: `{event_id, voter_propel_id, voter_volunteer_id?, slate:[team_id], shown_at, picks:null|[team_id], voted_at, created_at, updated_at, voided:false, voided_at?, voided_by?}` — full `set()` on every write. `hackathons/{doc}/peer_vote/exposure {counts:{team_id:n}}` (transactional `Increment`); `…/peer_vote/summary {winner_team_id, winner_team_name, published_at, published_by, ballots}`. + +**Endpoints** (auth = PropelAuth `@auth.require_user`; admin = `volunteer.admin`; member gate = `user_is_on_team`; admin bypass via `is_admin(auth_user)` from `services/hackathon_planning_service.py`): +| Route | Auth | Notes | +|---|---|---| +| `POST /api/team//project` | member | partial update of `project_*`; sets `draft` on first save; 400 `invalid_project{errors[]}`, 403 `not_team_member`, 409 `submissions_closed{deadline,late_until,now}`; returns `{success, team, window}` | +| `POST /api/team//project/submit` | member | 400 `incomplete{missing}`; idempotent; sets `submitted|late`; Slack to team channel + audit | +| `POST /api/team//devpost`, `/demo-video` | member (**was any user**) | same 403/409 semantics via `self_serve_team_edit` | +| `POST /api/team//mentor-availability {open:bool}` | member | no deadline gate; audit only; returns `{success, team}` | +| `GET /api/hackathons//submissions/window` | public | `{state: open|late|closed|no_deadline, submission, late_until, now, timezone}` (server clock) | +| `GET /api/github/activity?org&repo` | public | `{success, repo{html_url,default_branch,pushed_at,open_issues_count,stargazers_count}, commits{total_recent,last_24h,last_commit_at,last_commit_message,last_author}, contributors[{login,avatar_url,contributions}]≤8, open_prs}`; exactly 3 GitHub calls; TTL 300 success-only; 404 `repo_not_found`, 503 `github_rate_limited` | +| `GET /api/volunteer//me?type=hacker` | auth | `{is_hacker, volunteer:{name,isSelected}|null}` | +| `GET /api/hackathons//peer-vote/slate` | auth | `{status: upcoming|open|voted|closed|not_eligible|disabled, opens_at, closes_at, max_picks, slate:[{team_id,name,project_tagline,project_thumbnail_url,demo_video_url,github_links,users_count}], picks, own_team_ids, reason?: not_enough_submissions}`; persists slate on first open GET | +| `POST …/peer-vote/ballot {picks}` | auth | 403 `not_eligible`/`peer_vote_disabled`, 409 `voting_closed`/`ballot_voided`, 400 `no_slate`/`invalid_picks`; re-vote allowed until close | +| `GET …/peer-vote/results` · `POST …/peer-vote/ballots//void` · `POST …/peer-vote/publish` | admin | results: `{ballots, voided, eligible_estimate, window, settings, published, teams:[{team_id,name,shown,exposure_shown,approvals,approval_rate,wilson_lower_bound,rank}]}` | +| `GET …/peer-vote/summary` | public | `{published:false}` or `{published:true, winner_team_id, winner_team_name, published_at, ballots}` | +| `POST /api/hackathons//deadlines/remind {kind, hours_before, only_if_due?, force?}` | admin **or** `X-Api-Key` (`BACKEND_CRON_TOKEN`) | 409 `no_deadline`/`already_sent`; `{notified[], skipped[], simulated}` | +| `POST /api/hackathons/deadlines/remind-due` | API key | hourly cron; iterates current events × {24,6,1} with `only_if_due` | +| `GET /api/judge/team/` (+ list) | existing | + `demo_video_url` (and legacy `video_url` filled from it); nothing else changes for judges | +| `PATCH /api/messages/hackathon` | existing admin | now accepts `deadlines` + `constraints.peer_vote_*` | +| `GET /api/messages/hackathon/` | existing public | carries `deadlines`, `constraints.peer_vote_*`, team `project_*` **minus `project_story`** (payload size), `mentor_help_wanted`, `awards` | +Thumbnail upload uses the **existing** `POST /api/messages/upload-image` (`require_user`; form `file`, `directory=teams//project`, `filename`) → `{success, url}`. No signed-URL mint. +Every write: `clear_all_caches()` + `hackathons_service.clear_cache()` (mentor-service two-step) + `send_slack_audit`. Voting window defaults when unset: `opens_at = late_submission_until || submission`, `closes_at = end_date 23:59:59 event tz`; no `deadlines` or `peer_vote_enabled=false` ⇒ `disabled`. **Deploy order:** backend first; every frontend call treats 404 as "feature off". + +--- + +### WS-A Backend (`backend-ohack.dev`, branch `feat/submissions-peer-vote`, Python 3.9 typing, `ENVIRONMENT=test pytest api//tests`) +1. **Validators + save_hackathon** — `common/utils/validators.py`: `normalize_deadline_iso(value, tz)`, `validate_deadlines(d, tz)` (unknown key → error; ordering `submission ≤ late_until`, `voting_opens < voting_closes`), peer-vote constraint ranges, `sanitize_markdown(text, max)`, `validate_https_url(url, max)`; wire into `validate_hackathon_data_partial` (after the `github_org` block; constraints block before `cleaned["constraints"]=c`). `services/hackathons_service.py::save_hackathon` after the passthrough loop (L1216–1218): write `deadlines` (update ⇒ `DELETE_FIELD` for `None`; create ⇒ drop `None`). `get_single_hackathon_event`: `t.pop("project_story", None)` per team after `_enrich_teams_users_batch`. Tests: `test/common/utils/test_validators.py` (+), `api/messages/tests/test_hackathon_deadlines.py` (new). +2. **`api/submissions/`** (`__init__.py`, `submissions_service.py`, `submissions_views.py` `url_prefix="/api"`, `tests/`, `README.md`): `clear_cache()` (copy mentors L33–49), `compute_submission_window(event, now)`, `_authorize_team_write(propel, team_id, admin, enforce_deadline)` → (error|None, team, event, window), `validate_project_payload`, `save_project`, `submit_project`, `self_serve_team_edit` (Task 3), `set_mentor_help_wanted`, reminders (Task 8), `get_submission_window_for_event`. Views per Part 3. Lazy imports inside functions (surveys style). Tests: window states w/ tz + injected `now`; sanitizer keeps `List`, strips ` world", 1000) + assert "" not in cleaned + assert "Hello" in cleaned and "world" in cleaned + + +def test_sanitize_markdown_strips_onerror_attribute(): + cleaned = sanitize_markdown('', 1000) + assert "onerror" not in cleaned + + +def test_sanitize_markdown_neutralizes_javascript_href(): + cleaned = sanitize_markdown('click', 1000) + assert "javascript:" not in cleaned + + +def test_sanitize_markdown_preserves_generic_angle_brackets(): + cleaned = sanitize_markdown("We used List and Map internally.", 1000) + assert "List" in cleaned + assert "Map" in cleaned + + +def test_sanitize_markdown_truncates_to_max_length(): + cleaned = sanitize_markdown("x" * 50, 10) + assert len(cleaned) == 10 + + +def test_sanitize_markdown_none_passthrough(): + assert sanitize_markdown(None, 100) is None + + +def test_validate_https_url_accepts_https(): + assert validate_https_url("https://example.com/path") is True + + +def test_validate_https_url_rejects_http_and_non_url(): + assert validate_https_url("http://example.com") is False + assert validate_https_url("not a url") is False + assert validate_https_url("") is False + assert validate_https_url(None) is False + + +def test_validate_https_url_enforces_max_length(): + long_url = "https://example.com/" + ("a" * 2100) + assert validate_https_url(long_url, max_length=2048) is False + + +# --------------------------------------------------------------------------- +# peer_vote_* constraints wired into validate_hackathon_data_partial. +# --------------------------------------------------------------------------- + +def test_partial_accepts_valid_peer_vote_constraints(): + cleaned, skipped = validate_hackathon_data_partial( + _hackathon_data({ + "peer_vote_enabled": True, + "peer_vote_slate_size": 5, + "peer_vote_max_picks": 2, + "peer_vote_requires_submission": True, + }) + ) + assert skipped == [] + assert cleaned["constraints"]["peer_vote_enabled"] is True + assert cleaned["constraints"]["peer_vote_slate_size"] == 5 + assert cleaned["constraints"]["peer_vote_max_picks"] == 2 + + +def test_partial_rejects_slate_size_out_of_range(): + cleaned, skipped = validate_hackathon_data_partial( + _hackathon_data({"peer_vote_slate_size": 20}) + ) + assert any(s["field"] == "constraints.peer_vote_slate_size" for s in skipped) + assert "peer_vote_slate_size" not in cleaned["constraints"] + + +def test_partial_rejects_max_picks_not_below_slate_size(): + cleaned, skipped = validate_hackathon_data_partial( + _hackathon_data({"peer_vote_slate_size": 3, "peer_vote_max_picks": 3}) + ) + assert any(s["field"] == "constraints.peer_vote_max_picks" for s in skipped) + assert "peer_vote_max_picks" not in cleaned["constraints"] + + +def test_partial_rejects_non_bool_peer_vote_enabled(): + cleaned, skipped = validate_hackathon_data_partial( + _hackathon_data({"peer_vote_enabled": "yes"}) + ) + assert any(s["field"] == "constraints.peer_vote_enabled" for s in skipped) + assert "peer_vote_enabled" not in cleaned["constraints"] + + +# --------------------------------------------------------------------------- +# deadlines wired into validate_hackathon_data_partial (uses the hackathon's +# own timezone, defaulting to America/Phoenix). +# --------------------------------------------------------------------------- + +def test_partial_normalizes_deadlines_with_event_timezone(): + data = _hackathon_data() + data["timezone"] = "America/New_York" + data["deadlines"] = {"submission": "2026-10-10T15:00:00"} + cleaned, skipped = validate_hackathon_data_partial(data) + assert skipped == [] + assert cleaned["deadlines"]["submission"] == "2026-10-10T15:00:00-04:00" + + +def test_partial_skips_deadlines_with_bad_ordering_but_keeps_other_fields(): + data = _hackathon_data() + data["deadlines"] = { + "submission": "2026-10-10T15:00:00", + "late_submission_until": "2026-10-10T10:00:00", + } + cleaned, skipped = validate_hackathon_data_partial(data) + assert any(s["field"] == "deadlines" for s in skipped) + assert "deadlines" not in cleaned + assert cleaned["title"] == "Test Hackathon" From e6530d7d4733b15b88d893a869fbf623352e455f Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:19:08 -0700 Subject: [PATCH 03/11] Add api/submissions blueprint: project write-ups, deadline gate, mentor-availability, deadline reminders; fix devpost/demo-video auth gap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit New self-serve, deadline-aware team writes, split from the admin-only api.teams.teams_service.edit_team path: - POST /api/team//project — partial update of project_tagline/ project_story/project_built_with/project_links/project_thumbnail_url/ project_images; sets project_submission_status=draft on first save; 400 invalid_project, 403 not_team_member, 409 submissions_closed. - POST /api/team//project/submit — requires tagline+story; idempotent; submitted|late depending on the event's submission window. - POST /api/team//mentor-availability — signal-only "open to mentors / heads-down" toggle, no deadline gate. - GET /api/hackathons//submissions/window — public server-clock window state for the dashboard's deadline strip. - Deadline reminders: build_reminder_message (never nags a done team), send_deadline_reminders (idempotent per event+kind+hours, only_if_due for the hourly cron), send_due_reminders_for_current_events. Security fix (Part 9 bug #1): POST /api/team//devpost and /demo-video used to call edit_team directly with NO membership check — any logged-in user could overwrite any team's Devpost link or demo video. Both now route through self_serve_team_edit, which enforces team membership (or admin) and the submission deadline. Tests: api/submissions/tests (service + view-signature + route dispatch), api/teams/tests (devpost/demo-video regression coverage). Co-Authored-By: Claude Sonnet 5 --- api/submissions/README.md | 105 +++ api/submissions/__init__.py | 0 api/submissions/submissions_service.py | 627 ++++++++++++++++ api/submissions/submissions_views.py | 98 +++ api/submissions/tests/__init__.py | 0 .../tests/test_submissions_service.py | 694 ++++++++++++++++++ .../tests/test_submissions_views.py | 188 +++++ api/teams/teams_views.py | 26 +- api/teams/tests/__init__.py | 0 .../test_teams_devpost_demo_video_views.py | 128 ++++ 10 files changed, 1858 insertions(+), 8 deletions(-) create mode 100644 api/submissions/README.md create mode 100644 api/submissions/__init__.py create mode 100644 api/submissions/submissions_service.py create mode 100644 api/submissions/submissions_views.py create mode 100644 api/submissions/tests/__init__.py create mode 100644 api/submissions/tests/test_submissions_service.py create mode 100644 api/submissions/tests/test_submissions_views.py create mode 100644 api/teams/tests/__init__.py create mode 100644 api/teams/tests/test_teams_devpost_demo_video_views.py diff --git a/api/submissions/README.md b/api/submissions/README.md new file mode 100644 index 0000000..e7df774 --- /dev/null +++ b/api/submissions/README.md @@ -0,0 +1,105 @@ +# Submissions API + +Team project write-ups + submission deadlines. Introduced Sep 2026 so +ohack.dev's team dashboard can replace DevPost's project page and deadline +mechanics (judging itself is unchanged — see `api/judging/`). + +## Base URL +`/api` + +## Ownership + +- `api.submissions.submissions_service` owns SELF-SERVE, deadline-aware + writes to a team's `project_*` fields (and now also the pre-existing + `/team//devpost` and `/team//demo-video` routes, via + `self_serve_team_edit`). +- `api.teams.teams_service.edit_team` remains the ADMIN write path (no + deadline gate, no membership check — gated at the route by + `volunteer.admin`). An admin can still override `project_*` fields + (including `project_submission_status`) through `PATCH /api/team/edit`. + +Every write here goes through `_authorize_team_write`, which: +1. 404s if the team doesn't exist. +2. 403s `{"error": "not_team_member"}` if the caller isn't on the team and + isn't an admin (`is_admin(auth_user)` from + `services.hackathon_planning_service`). +3. Computes the event's submission window and, unless the caller is an admin + or the route opted out (`enforce_deadline=False`, used only by + mentor-availability), 409s `{"error": "submissions_closed", "deadline", + "late_until", "now"}` once it's closed. + +## Data model + +New optional keys on `teams/{id}` (absent ⇒ legacy team, no dashboard +UI implied): `project_tagline`, `project_story`, `project_built_with`, +`project_links`, `project_thumbnail_url`, `project_images`, +`project_updated_at`, `project_submitted_at`, `project_submission_status` +(`draft|submitted|late`), `mentor_help_wanted` (absent ⇒ treated as `True`). + +New optional key on `hackathons/{id}`: `deadlines` — see +`common.utils.validators.validate_deadlines` for the shape and +`services.hackathons_service.save_hackathon` for how a `None` value becomes a +Firestore `DELETE_FIELD` on update (or is simply omitted on create). + +## Endpoints + +### `POST /api/team//project` +Member (or admin) only. Partial update — only keys present in the body are +validated and written. 400 `{"error": "invalid_project", "errors": [...]}` +on a bad field, 403/409 per `_authorize_team_write`. Sets +`project_submission_status = "draft"` on the very first save (never regressed +by a hacker's own write afterward). Returns +`{"success": true, "team": , "window": }`. + +### `POST /api/team//project/submit` +Member (or admin) only. Requires `project_tagline` and `project_story` to +already be saved (400 `{"error": "incomplete", "missing": [...]}` otherwise). +Idempotent — resubmitting returns `{"success": true, "already_submitted": +true, "team": ...}`. Sets `project_submission_status` to `"submitted"` (window +open/no-deadline) or `"late"` (window in its late-grace period, or an admin +forcing a submission through after full close). + +### `POST /api/team//mentor-availability` +Member (or admin) only. Body `{"open": bool}`. No deadline gate — a team can +flip this any time. Signal only; changes no other backend behaviour. + +### `POST /api/team//devpost`, `POST /api/team//demo-video` +(Defined in `api/teams/teams_views.py`, delegate to +`self_serve_team_edit` here.) **Security fix (Part 9 bug #1):** these used to +call `edit_team` directly with no membership check at all — any logged-in +user could overwrite any team's DevPost link or demo video. They now run +through the same `_authorize_team_write` gate as everything else in this +module. + +### `GET /api/hackathons//submissions/window` +Public. `{"state": "open"|"late"|"closed"|"no_deadline", "submission", +"late_until", "now", "timezone"}`. Backs the dashboard's deadline strip and +the admin `DeadlinesSection` preview — the frontend should treat the server's +`now`/`state` as authoritative rather than deriving state from `deadlines` +client-side, though `deriveVoteWindow`-style client mirroring is fine for a +non-authoritative live countdown between polls. + +## Sanitization + +`project_tagline` / `project_story` are stored as raw markdown. The frontend +renders them with `react-markdown` **without** `rehype-raw`, so any HTML tag +in the stored text is already inert on read — `sanitize_markdown` (in +`common.utils.validators`) is defence-in-depth only: it strips a small +denylist of tags (`script|iframe|object|embed|style|link|meta|form|base`), +`on*=` attributes, and neutralizes `javascript:`/`vbscript:`/`data:` link +targets. It deliberately preserves generic `<` — e.g. `List` in a +project story survives untouched. + +## Images + +`project_thumbnail_url` / `project_images[]` must be URLs already on the +site's own CDN under `teams//` (uploaded via the existing +`POST /api/messages/upload-image` with `directory=teams//project` — +there is no separate signed-URL mint for this). A URL already saved on the +team's doc is trusted without re-verifying against GCS; a new one must exist, +be an image, and be ≤5MB (`common.utils.cdn.get_blob_metadata`). + +## Tests +``` +ENVIRONMENT=test pytest api/submissions/tests +``` diff --git a/api/submissions/__init__.py b/api/submissions/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/api/submissions/submissions_service.py b/api/submissions/submissions_service.py new file mode 100644 index 0000000..836afcd --- /dev/null +++ b/api/submissions/submissions_service.py @@ -0,0 +1,627 @@ +""" +Team project write-ups + submission deadlines (Sep 2026 — the team dashboard +that replaces DevPost as the team's single home). See docs/plans/ +team-dashboard-devpost-replacement.md (frontend repo) Part 3 for the full +contract; this module is the backend half. + +Ownership split (deliberately kept separate from api.teams.teams_service): + - Everything here is SELF-SERVE and deadline-aware: a team member writes + their own project_* fields, subject to the event's submission window. + - api.teams.teams_service.edit_team stays the ADMIN write path (no deadline + gate, no membership check — org-permission gated at the route). + - self_serve_team_edit (below) is the bridge used by the existing + /devpost and /demo-video routes, which used to call edit_team directly + with NO membership check at all (Part 9 bug #1 — any logged-in user could + overwrite any team's DevPost link or demo video). + +Sanitization decision: project_tagline/project_story are stored as raw +markdown. The frontend renders them with react-markdown WITHOUT rehype-raw, +so any HTML tag in the stored text is inert on read. sanitize_markdown() +(common.utils.validators) is defence-in-depth only, for the case this content +is ever rendered somewhere less careful: it strips a small denylist of +dangerous tags/attributes and neutralizes javascript:/vbscript:/data: link +targets. It deliberately does NOT strip generic "<" — code like +"List" in a project story must survive untouched. +""" +import logging +import os +from datetime import datetime, timedelta, timezone + +from db.db import get_db +from common.utils.firestore_helpers import clear_all_caches +from common.utils.slack import send_slack, send_slack_audit +from common.utils.firebase import get_hackathon_by_event_id +from common.utils.validators import sanitize_markdown, sanitize_string, validate_https_url +from services.teams_service import get_team + +logger = logging.getLogger("myapp") + +# Sub-fields of the team doc this module owns (partial-update semantics: a +# save only ever touches the keys present in the request payload). +PROJECT_FIELDS = ( + "project_tagline", + "project_story", + "project_built_with", + "project_links", + "project_thumbnail_url", + "project_images", +) + +PROJECT_LIMITS = { + "tagline": 140, + "story": 20000, + "built_with_n": 25, + "built_with_len": 30, + "links_n": 10, + "label": 40, + "url": 2048, + "images_n": 8, +} + +# A submitted project's status never regresses to "draft" by a hacker's own +# action; only submit_project (draft -> submitted|late) and an admin override +# via PATCH /api/team/edit change it after that. +SUBMITTED_STATUSES = {"submitted", "late"} + +# submit_project requires at least a tagline and a story — an empty write-up +# with just a demo video isn't a "project" yet. +REQUIRED_SUBMIT_FIELDS = ("project_tagline", "project_story") + +MAX_IMAGE_BYTES = 5 * 1024 * 1024 + + +def clear_cache() -> None: + """Bust the team/doc caches AND the hackathon event caches. + + Mirrors api/mentors/mentors_service.py's clear_cache(): a project save + changes fields the CACHED event page (get_single_hackathon_event) also + renders (submission tag, tagline), so both the registered per-function + caches AND the hackathon service's own cache need clearing. Lazy import + avoids a circular import at module load. + """ + clear_all_caches() + try: + from services.hackathons_service import clear_cache as clear_hackathon_caches + clear_hackathon_caches() + except Exception as e: # pragma: no cover - best-effort cache bust + logger.warning("submissions clear_cache: hackathon cache clear failed: %s", e) + + +def _notifications_disabled() -> bool: + """Mirror of the ENVIRONMENT=test gate used across services (see + services/volunteers_service.py) so unit tests never hit the real Slack API.""" + return os.environ.get("ENVIRONMENT") == "test" + + +def _cdn_server() -> str: + return os.getenv("CDN_SERVER", "https://cdn.ohack.dev").rstrip("/") + + +def compute_submission_window(event, now=None): + """{"state": open|late|closed|no_deadline, "submission", "late_until", "now", "timezone"}. + + `event` is a hackathon dict (as returned by get_hackathon_by_event_id) — + may be {} if the event couldn't be resolved, in which case this degrades + to no_deadline rather than raising. `now` is an injectable + timezone-aware datetime for tests; defaults to real UTC now. + """ + event = event or {} + tz_name = event.get("timezone") or "America/Phoenix" + now_dt = now or datetime.now(timezone.utc) + deadlines = event.get("deadlines") or {} + submission = deadlines.get("submission") + late_until = deadlines.get("late_submission_until") + + if not submission: + return { + "state": "no_deadline", + "submission": None, + "late_until": late_until, + "now": now_dt.isoformat(), + "timezone": tz_name, + } + + submission_dt = datetime.fromisoformat(submission) + if now_dt <= submission_dt: + state = "open" + elif late_until and now_dt <= datetime.fromisoformat(late_until): + state = "late" + else: + state = "closed" + + return { + "state": state, + "submission": submission, + "late_until": late_until, + "now": now_dt.isoformat(), + "timezone": tz_name, + } + + +def submissions_closed(window) -> bool: + return (window or {}).get("state") == "closed" + + +def _team_or_404(team_id): + """(ref, team_dict|None). Mirrors api/mentors/mentors_service.py's + _team_doc_or_404 but returns the id-stamped dict instead of a 3-tuple with + the db handle (callers here don't need it).""" + db = get_db() + ref = db.collection("teams").document(team_id) + snap = ref.get() + if not snap.exists: + return ref, None + data = snap.to_dict() or {} + data["id"] = snap.id + return ref, data + + +def _authorize_team_write(propel_user_id, team_id, admin=False, enforce_deadline=True): + """Shared gate for every self-serve team write in this module. + + Returns (error, team, event, window) where `error` is either None (proceed) + or a ready-to-return (payload, status) tuple: + - (.., 404) team not found + - (.., 403) {"error": "not_team_member"} — caller isn't on the team and + isn't an admin + - (.., 409) {"error": "submissions_closed", "deadline", "late_until", + "now"} — enforce_deadline=True, caller isn't an admin, and the + window has closed + + Admins bypass both the membership check and the deadline gate. + """ + from api.teams.teams_service import user_is_on_team + + _ref, team = _team_or_404(team_id) + if team is None: + return ({"error": "Team not found"}, 404), None, None, None + + if not admin and not user_is_on_team(propel_user_id, team_id): + return ({"error": "not_team_member"}, 403), team, None, None + + event = get_hackathon_by_event_id(team.get("hackathon_event_id")) or {} + window = compute_submission_window(event) + + if enforce_deadline and not admin and submissions_closed(window): + return ( + { + "error": "submissions_closed", + "deadline": window.get("submission"), + "late_until": window.get("late_until"), + "now": window.get("now"), + }, + 409, + ), team, event, window + + return None, team, event, window + + +def _validate_own_cdn_image(url, team_id, existing_urls): + """True/(False, reason) for a project_thumbnail_url / project_images[] + entry. Must be an own-CDN URL under teams// (the existing + POST /api/messages/upload-image route, directory=teams//project). + A URL already on the team's doc is trusted without re-verifying against + GCS (it was verified when first saved); a NEW url must exist, be an + image, and be under the size cap.""" + prefix = f"{_cdn_server()}/teams/{team_id}/" + if not isinstance(url, str) or not url.startswith(prefix): + return False, f"must be an ohack CDN URL under teams/{team_id}/" + if url in existing_urls: + return True, None + try: + from common.utils.cdn import get_blob_metadata + blob_path = url[len(_cdn_server()) + 1:] + meta = get_blob_metadata(blob_path) + except Exception as e: + logger.warning("_validate_own_cdn_image: failed to verify %s: %s", url, e) + return False, "upload_not_found" + if not meta.get("exists"): + return False, "upload_not_found" + if not (meta.get("content_type") or "").startswith("image/"): + return False, "must be an image" + if (meta.get("size") or 0) > MAX_IMAGE_BYTES: + return False, "must be 5MB or smaller" + return True, None + + +def validate_project_payload(payload, team_id, existing=None): + """(clean, errors[{field, reason}]) — a partial update: only keys present + in `payload` are validated/returned. `existing` is the team's current doc + (so an already-saved thumbnail/image URL isn't re-verified against GCS).""" + if not isinstance(payload, dict): + return {}, [{"field": "payload", "reason": "must be an object"}] + + existing = existing or {} + errors = [] + clean = {} + + if "project_tagline" in payload: + val = payload.get("project_tagline") + if val in (None, ""): + clean["project_tagline"] = None + elif not isinstance(val, str) or len(val) > PROJECT_LIMITS["tagline"]: + errors.append({"field": "project_tagline", "reason": f"must be a string <= {PROJECT_LIMITS['tagline']} chars"}) + else: + clean["project_tagline"] = sanitize_markdown(val, PROJECT_LIMITS["tagline"]) + + if "project_story" in payload: + val = payload.get("project_story") + if val in (None, ""): + clean["project_story"] = None + elif not isinstance(val, str) or len(val) > PROJECT_LIMITS["story"]: + errors.append({"field": "project_story", "reason": f"must be a string <= {PROJECT_LIMITS['story']} chars"}) + else: + clean["project_story"] = sanitize_markdown(val, PROJECT_LIMITS["story"]) + + if "project_built_with" in payload: + val = payload.get("project_built_with") + if val is None: + clean["project_built_with"] = [] + elif ( + not isinstance(val, list) + or len(val) > PROJECT_LIMITS["built_with_n"] + or not all(isinstance(x, str) and len(x) <= PROJECT_LIMITS["built_with_len"] for x in val) + ): + errors.append({ + "field": "project_built_with", + "reason": f"must be a list of at most {PROJECT_LIMITS['built_with_n']} strings, each <= {PROJECT_LIMITS['built_with_len']} chars", + }) + else: + clean["project_built_with"] = [sanitize_string(x, PROJECT_LIMITS["built_with_len"]) for x in val] + + if "project_links" in payload: + val = payload.get("project_links") + if val is None: + clean["project_links"] = [] + elif not isinstance(val, list) or len(val) > PROJECT_LIMITS["links_n"]: + errors.append({"field": "project_links", "reason": f"must be a list of at most {PROJECT_LIMITS['links_n']} items"}) + else: + cleaned_links = [] + bad = False + for link in val: + if not isinstance(link, dict): + bad = True + break + label = link.get("label", "") + url = link.get("url", "") + if not isinstance(label, str) or len(label) > PROJECT_LIMITS["label"]: + bad = True + break + if not validate_https_url(url, PROJECT_LIMITS["url"]): + bad = True + break + cleaned_links.append({"label": sanitize_string(label, PROJECT_LIMITS["label"]), "url": url}) + if bad: + errors.append({ + "field": "project_links", + "reason": f"each link needs a label <= {PROJECT_LIMITS['label']} chars and an https url <= {PROJECT_LIMITS['url']} chars", + }) + else: + clean["project_links"] = cleaned_links + + existing_image_urls = {existing.get("project_thumbnail_url")} | set(existing.get("project_images") or []) + existing_image_urls.discard(None) + + if "project_thumbnail_url" in payload: + val = payload.get("project_thumbnail_url") + if val in (None, ""): + clean["project_thumbnail_url"] = None + else: + ok, reason = _validate_own_cdn_image(val, team_id, existing_image_urls) + if not ok: + errors.append({"field": "project_thumbnail_url", "reason": reason}) + else: + clean["project_thumbnail_url"] = val + + if "project_images" in payload: + val = payload.get("project_images") + if val is None: + clean["project_images"] = [] + elif not isinstance(val, list) or len(val) > PROJECT_LIMITS["images_n"]: + errors.append({"field": "project_images", "reason": f"must be a list of at most {PROJECT_LIMITS['images_n']} images"}) + else: + cleaned_images = [] + bad_reason = None + for url in val: + ok, reason = _validate_own_cdn_image(url, team_id, existing_image_urls) + if not ok: + bad_reason = reason + break + cleaned_images.append(url) + if bad_reason: + errors.append({"field": "project_images", "reason": bad_reason}) + else: + clean["project_images"] = cleaned_images + + return clean, errors + + +def save_project(propel_user_id, team_id, payload, admin=False): + """Partial update of project_* fields. Sets project_submission_status to + 'draft' on the very first save (never regressed to draft afterward). + 409s (via _authorize_team_write) once the submission window has closed, + unless the caller is an admin.""" + err, team, _event, window = _authorize_team_write(propel_user_id, team_id, admin=admin, enforce_deadline=True) + if err: + return err + + clean, errors = validate_project_payload(payload, team_id, existing=team) + if errors: + return {"error": "invalid_project", "errors": errors}, 400 + + update = dict(clean) + update["project_updated_at"] = datetime.now(timezone.utc).isoformat() + if not team.get("project_submission_status"): + update["project_submission_status"] = "draft" + + db = get_db() + db.collection("teams").document(team_id).set(update, merge=True) + clear_cache() + send_slack_audit( + action="project_save", + message=f"Team {team_id} saved project fields: {sorted(clean.keys())}", + payload={"team_id": team_id}, + ) + + fresh = (get_team(team_id) or {}).get("team") or {} + return {"success": True, "team": fresh, "window": window}, 200 + + +def submit_project(propel_user_id, team_id, admin=False): + """Marks the project submitted|late. Idempotent — resubmitting an already + submitted/late project is a no-op 200 with already_submitted=True. Blocked + (409, via _authorize_team_write) once the window is fully closed unless + the caller is an admin, in which case the forced submission is recorded + as 'late' regardless of how long past close it is.""" + err, team, _event, window = _authorize_team_write(propel_user_id, team_id, admin=admin, enforce_deadline=True) + if err: + return err + + if team.get("project_submission_status") in SUBMITTED_STATUSES: + fresh = (get_team(team_id) or {}).get("team") or {} + return {"success": True, "already_submitted": True, "team": fresh}, 200 + + missing = [f for f in REQUIRED_SUBMIT_FIELDS if not (team.get(f) or "").strip()] + if missing: + return {"error": "incomplete", "missing": missing}, 400 + + status = "late" if window.get("state") in ("late", "closed") else "submitted" + now_iso = datetime.now(timezone.utc).isoformat() + + db = get_db() + db.collection("teams").document(team_id).set( + {"project_submission_status": status, "project_submitted_at": now_iso}, + merge=True, + ) + clear_cache() + + slack_channel = team.get("slack_channel") + if slack_channel and not _notifications_disabled(): + label = "a little late, but it's in" if status == "late" else "on time" + try: + send_slack( + message=f":rocket: Project submitted — {label}! You can keep editing the write-up until submissions fully close.", + channel=slack_channel, + ) + except Exception as e: + logger.warning("submit_project: send_slack failed for team %s: %s", team_id, e) + send_slack_audit( + action="project_submit", + message=f"Team {team_id} submitted project (status={status})", + payload={"team_id": team_id, "status": status}, + ) + + fresh = (get_team(team_id) or {}).get("team") or {} + return {"success": True, "team": fresh, "status": status}, 200 + + +def self_serve_team_edit(propel_user_id, team_id, fields, admin=False): + """Bridge for the /devpost and /demo-video routes (Part 9 bug #1 fix): + gate on team membership + the submission deadline exactly like + save_project, then delegate the actual write to the existing admin + edit_team (which already knows how to stamp *_submitted timestamps for + devpost_link/demo_video_url). Lazy import: api.teams.teams_service must + never import this module, so importing it here (not at module top) keeps + the dependency one-directional.""" + err, _team, _event, _window = _authorize_team_write(propel_user_id, team_id, admin=admin, enforce_deadline=True) + if err: + return err + + from api.teams.teams_service import edit_team + edit_result = edit_team({"id": team_id, **fields}) + fresh = (get_team(team_id) or {}).get("team") or {} + return {**edit_result, "team": fresh} + + +def set_mentor_help_wanted(propel_user_id, team_id, open_flag, admin=False): + """Team-facing 'open to mentors / heads-down' signal. No deadline gate — + a team can flip this any time, including after submitting. Signal only: + it changes no other behavior (see mentor surfaces in the frontend for the + quiet UI treatment).""" + if not isinstance(open_flag, bool): + return {"error": "'open' must be a boolean"}, 400 + + err, _team, _event, _window = _authorize_team_write(propel_user_id, team_id, admin=admin, enforce_deadline=False) + if err: + return err + + name = None + try: + from services.users_service import get_propel_user_details_by_id + details = get_propel_user_details_by_id(propel_user_id) or () + name = details[4] if len(details) > 4 else None + except Exception as e: + logger.warning("set_mentor_help_wanted: could not resolve caller name: %s", e) + + now_iso = datetime.now(timezone.utc).isoformat() + db = get_db() + db.collection("teams").document(team_id).set( + { + "mentor_help_wanted": open_flag, + "mentor_help_wanted_updated_at": now_iso, + "mentor_help_wanted_updated_by_name": name, + }, + merge=True, + ) + clear_cache() + send_slack_audit( + action="mentor_help_wanted", + message=f"Team {team_id} set mentor_help_wanted={open_flag}", + payload={"team_id": team_id, "open": open_flag, "by": propel_user_id}, + ) + + fresh = (get_team(team_id) or {}).get("team") or {} + return {"success": True, "team": fresh}, 200 + + +def get_submission_window_for_event(event_id): + """Public GET /api/hackathons//submissions/window.""" + event = get_hackathon_by_event_id(event_id) + if not event: + return {"error": "Event not found"}, 404 + return compute_submission_window(event), 200 + + +# --------------------------------------------------------------------------- +# Deadline reminders (T-24h/6h/1h Slack nudges) — admin button + hourly cron. +# --------------------------------------------------------------------------- + +REMINDER_KINDS = {"submission"} +REMINDER_HOURS = {24, 6, 1} + + +def build_reminder_message(team, event, deadline_iso, hours_before): + """A tailored nudge naming only what THIS team still owes, or None when + the team has nothing left to do (already submitted — never nag a done team).""" + if team.get("project_submission_status") in SUBMITTED_STATUSES: + return None + + missing = [] + if not (team.get("project_tagline") or "").strip(): + missing.append("write a tagline for your project") + if not (team.get("project_story") or "").strip(): + missing.append("write your project story") + if not team.get("demo_video_url"): + missing.append("add a demo video") + missing.append("submit your project") + + bullets = "\n".join(f"• {item}" for item in missing) + event_id = event.get("event_id") or team.get("hackathon_event_id") or "" + link = f"https://www.ohack.dev/hack/{event_id}/manageteam" + hours_label = f"{hours_before} hour" + ("s" if hours_before != 1 else "") + + return ( + f":alarm_clock: *{hours_label} left to submit your project!*\n" + f"Still to do:\n{bullets}\n" + f"{link}" + ) + + +def send_deadline_reminders(event_id, kind, hours_before, *, only_if_due=False, force=False, actor="cron"): + """Sends a Slack reminder to every active team's channel for one + (kind, hours_before) pair. Idempotent per event+kind+hours_before unless + `force` — `reminders_sent[f"{kind}_{hours_before}h"]` on the hackathon doc + is the idempotency key. `only_if_due` (used by the hourly cron) skips + silently ({"success": true, "skipped": "not_due"}) outside the + [deadline - hours_before, deadline) window rather than erroring, so the + cron can call this for every (event, hours) pair every hour without + spamming teams the other 23 hours of the day.""" + if kind not in REMINDER_KINDS: + return {"error": f"kind must be one of {sorted(REMINDER_KINDS)}"}, 400 + try: + hours_before = int(hours_before) + except (TypeError, ValueError): + return {"error": f"hours_before must be one of {sorted(REMINDER_HOURS)}"}, 400 + if hours_before not in REMINDER_HOURS: + return {"error": f"hours_before must be one of {sorted(REMINDER_HOURS)}"}, 400 + + event = get_hackathon_by_event_id(event_id) + if not event: + return {"error": "Event not found"}, 404 + + deadline_iso = (event.get("deadlines") or {}).get("submission") + if not deadline_iso: + return {"error": "no_deadline"}, 409 + + reminder_key = f"{kind}_{hours_before}h" + already = (event.get("reminders_sent") or {}).get(reminder_key) + if already and not force: + return {"error": "already_sent", "sent_at": already.get("sent_at")}, 409 + + now_dt = datetime.now(timezone.utc) + deadline_dt = datetime.fromisoformat(deadline_iso) + due_at = deadline_dt - timedelta(hours=hours_before) + if only_if_due and not (due_at <= now_dt < deadline_dt): + return {"success": True, "kind": kind, "hours_before": hours_before, "skipped": "not_due", "simulated": _notifications_disabled()}, 200 + + db = get_db() + simulated = _notifications_disabled() + notified = [] + skipped = [] + for doc in db.collection("teams").where("hackathon_event_id", "==", event_id).stream(): + team = doc.to_dict() or {} + team_id = doc.id + if team.get("active") is False: + skipped.append({"team_id": team_id, "reason": "inactive"}) + continue + slack_channel = team.get("slack_channel") + if not slack_channel: + skipped.append({"team_id": team_id, "reason": "no_slack_channel"}) + continue + message = build_reminder_message(team, event, deadline_iso, hours_before) + if message is None: + skipped.append({"team_id": team_id, "reason": "already_done"}) + continue + if not simulated: + try: + send_slack(message=message, channel=slack_channel) + except Exception as e: + logger.warning("send_deadline_reminders: send_slack failed for team %s: %s", team_id, e) + skipped.append({"team_id": team_id, "reason": "send_failed"}) + continue + notified.append(team_id) + + event_doc_id = event.get("id") or event_id + db.collection("hackathons").document(event_doc_id).set( + {"reminders_sent": {reminder_key: { + "sent_at": now_dt.isoformat(), + "deadline": deadline_iso, + "teams_notified": notified, + "by": actor, + }}}, + merge=True, + ) + clear_cache() + send_slack_audit( + action="deadline_reminder", + message=f"Sent {kind} {hours_before}h reminders for {event_id}: {len(notified)} teams notified, {len(skipped)} skipped", + payload={"event_id": event_id, "kind": kind, "hours_before": hours_before, "by": actor}, + ) + + return { + "success": True, + "kind": kind, + "hours_before": hours_before, + "deadline": deadline_iso, + "teams_total": len(notified) + len(skipped), + "notified": notified, + "skipped": skipped, + "simulated": simulated, + }, 200 + + +def send_due_reminders_for_current_events(): + """Hourly cron entry point: every currently-running event x every + reminder hour, only_if_due=True so most calls are no-ops.""" + from services.hackathons_service import get_hackathon_list + + events = (get_hackathon_list("current") or {}).get("hackathons") or [] + results = [] + for event in events: + event_id = event.get("event_id") + if not event_id: + continue + for hours_before in sorted(REMINDER_HOURS, reverse=True): + payload, status = send_deadline_reminders(event_id, "submission", hours_before, only_if_due=True, actor="cron") + results.append({"event_id": event_id, "hours_before": hours_before, "status": status, "result": payload}) + return {"success": True, "results": results}, 200 diff --git a/api/submissions/submissions_views.py b/api/submissions/submissions_views.py new file mode 100644 index 0000000..9d22826 --- /dev/null +++ b/api/submissions/submissions_views.py @@ -0,0 +1,98 @@ +""" +Flask routes for team project write-ups + submission deadlines. + +Membership + deadline enforcement live in submissions_service ( +_authorize_team_write); these views only extract the request body and check +admin status via is_admin(auth_user) for the bypass. +""" +import logging +from flask import Blueprint, request + +from common.auth import auth, auth_user +from services.hackathon_planning_service import is_admin +from common.utils.api_key import check_api_key +from api.submissions.submissions_service import ( + save_project, + submit_project, + set_mentor_help_wanted, + get_submission_window_for_event, + send_deadline_reminders, + send_due_reminders_for_current_events, +) + +logger = logging.getLogger(__name__) +logger.setLevel(logging.INFO) + +bp_name = "api-submissions" +bp = Blueprint(bp_name, __name__, url_prefix="/api") + + +def _unauthorized(): + return {"error": "Unauthorized"}, 401 + + +@bp.route("/team//project", methods=["POST"]) +@auth.require_user +def save_project_api(teamid): + """Partial update of a team's project write-up fields. + Body: any of project_tagline, project_story, project_built_with, + project_links, project_thumbnail_url, project_images.""" + if not (auth_user and auth_user.user_id): + return _unauthorized() + payload = request.get_json() or {} + return save_project(auth_user.user_id, teamid, payload, admin=is_admin(auth_user)) + + +@bp.route("/team//project/submit", methods=["POST"]) +@auth.require_user +def submit_project_api(teamid): + """Marks the team's project submitted (or late, inside the grace window).""" + if not (auth_user and auth_user.user_id): + return _unauthorized() + return submit_project(auth_user.user_id, teamid, admin=is_admin(auth_user)) + + +@bp.route("/team//mentor-availability", methods=["POST"]) +@auth.require_user +def set_mentor_availability_api(teamid): + """Body: { open: bool } — team-facing 'open to mentors / heads-down' signal.""" + if not (auth_user and auth_user.user_id): + return _unauthorized() + body = request.get_json() or {} + return set_mentor_help_wanted(auth_user.user_id, teamid, body.get("open"), admin=is_admin(auth_user)) + + +@bp.route("/hackathons//submissions/window", methods=["GET"]) +def submissions_window_api(event_id): + """Public — the dashboard's deadline strip and the DeadlinesSection admin + preview both read this so the countdown is driven by the server clock.""" + return get_submission_window_for_event(event_id) + + +@bp.route("/hackathons//deadlines/remind", methods=["POST"]) +@auth.optional_user +def remind_api(event_id): + """Admin button (Bearer token) OR the hourly cron (X-Api-Key: + BACKEND_CRON_TOKEN) — @auth.optional_user populates auth_user when a + valid token is present but doesn't 401 when it's absent, so the API-key + branch can still be reached.""" + is_admin_caller = bool(auth_user and getattr(auth_user, "user_id", None) and is_admin(auth_user)) + if not is_admin_caller and not check_api_key(request, "BACKEND_CRON_TOKEN"): + return {"error": "Forbidden"}, 403 + + body = request.get_json() or {} + kind = body.get("kind", "submission") + hours_before = body.get("hours_before") + only_if_due = bool(body.get("only_if_due", False)) + force = bool(body.get("force", False)) + actor = auth_user.user_id if is_admin_caller else "cron" + return send_deadline_reminders(event_id, kind, hours_before, only_if_due=only_if_due, force=force, actor=actor) + + +@bp.route("/hackathons/deadlines/remind-due", methods=["POST"]) +def remind_due_api(): + """Hourly GitHub Actions cron — API key only, no admin bypass (nothing to + bypass: it always iterates every currently-running event).""" + if not check_api_key(request, "BACKEND_CRON_TOKEN"): + return {"error": "Forbidden"}, 403 + return send_due_reminders_for_current_events() diff --git a/api/submissions/tests/__init__.py b/api/submissions/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/api/submissions/tests/test_submissions_service.py b/api/submissions/tests/test_submissions_service.py new file mode 100644 index 0000000..8405fb0 --- /dev/null +++ b/api/submissions/tests/test_submissions_service.py @@ -0,0 +1,694 @@ +""" +Unit tests for api.submissions.submissions_service. + +Uses a tiny in-memory fake Firestore (FakeDb) instead of MagicMock chains so +that a save (`set(..., merge=True)`) is visible to the NEXT read in the same +test — needed for idempotency / "edit after submit keeps status" assertions. +`get_team` (imported from services.teams_service) is monkeypatched to read +from the same in-memory store so the returned "team" reflects the write. +""" +import os +from datetime import datetime, timedelta, timezone + +import pytest + +os.environ.setdefault("ENVIRONMENT", "test") + +import api.submissions.submissions_service as svc + + +# --------------------------------------------------------------------------- +# Fake Firestore +# --------------------------------------------------------------------------- + +class FakeSnapshot: + 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 FakeDocRef: + def __init__(self, store, doc_id): + self._store = store + self._id = doc_id + + def get(self): + return FakeSnapshot(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 FakeCollection: + def __init__(self, store): + self._store = store + + def document(self, doc_id): + return FakeDocRef(self._store, doc_id) + + +class FakeDb: + def __init__(self, store): + self._store = store + + def collection(self, name): + assert name == "teams" + return FakeCollection(self._store) + + +@pytest.fixture +def team_store(): + return {} + + +@pytest.fixture +def wire(monkeypatch, team_store): + """Wires get_db/get_team to the fake store and no-ops external side + effects (Slack, audit, cache). Returns the store for assertions/setup.""" + monkeypatch.setattr(svc, "get_db", lambda: FakeDb(team_store)) + monkeypatch.setattr(svc, "clear_cache", lambda: None) + monkeypatch.setattr(svc, "send_slack_audit", lambda **kwargs: None) + monkeypatch.setattr(svc, "send_slack", lambda **kwargs: None) + + def fake_get_team(team_id): + data = team_store.get(team_id) + if data is None: + return {} + return {"team": {**data, "id": team_id}} + + monkeypatch.setattr(svc, "get_team", fake_get_team) + return team_store + + +def _seed_team(store, team_id="team-1", **extra): + store[team_id] = { + "hackathon_event_id": "event-1", + "slack_channel": "team-1-channel", + **extra, + } + + +def _member(monkeypatch, is_member=True): + monkeypatch.setattr("api.teams.teams_service.user_is_on_team", lambda propel, team_id: is_member) + + +def _event(monkeypatch, deadlines=None, timezone_name="America/Phoenix"): + event = {"timezone": timezone_name, "deadlines": deadlines or {}} + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda event_id: event) + return event + + +# --------------------------------------------------------------------------- +# compute_submission_window +# --------------------------------------------------------------------------- + +def test_window_no_deadline_when_unset(): + window = svc.compute_submission_window({"timezone": "America/Phoenix"}) + assert window["state"] == "no_deadline" + + +def test_window_open_before_submission(): + now = datetime(2026, 10, 10, 10, 0, tzinfo=timezone.utc) + event = {"timezone": "UTC", "deadlines": {"submission": "2026-10-10T15:00:00+00:00"}} + assert svc.compute_submission_window(event, now=now)["state"] == "open" + + +def test_window_late_between_submission_and_late_until(): + now = datetime(2026, 10, 10, 16, 0, tzinfo=timezone.utc) + event = { + "timezone": "UTC", + "deadlines": { + "submission": "2026-10-10T15:00:00+00:00", + "late_submission_until": "2026-10-10T18:00:00+00:00", + }, + } + assert svc.compute_submission_window(event, now=now)["state"] == "late" + + +def test_window_closed_after_late_until(): + now = datetime(2026, 10, 10, 19, 0, tzinfo=timezone.utc) + event = { + "timezone": "UTC", + "deadlines": { + "submission": "2026-10-10T15:00:00+00:00", + "late_submission_until": "2026-10-10T18:00:00+00:00", + }, + } + assert svc.compute_submission_window(event, now=now)["state"] == "closed" + + +def test_window_closed_immediately_after_submission_with_no_late_window(): + now = datetime(2026, 10, 10, 15, 0, 1, tzinfo=timezone.utc) + event = {"timezone": "UTC", "deadlines": {"submission": "2026-10-10T15:00:00+00:00"}} + assert svc.compute_submission_window(event, now=now)["state"] == "closed" + + +# --------------------------------------------------------------------------- +# validate_project_payload — sanitization + limits +# --------------------------------------------------------------------------- + +def test_validate_project_payload_sanitizes_tagline_script_tag(): + clean, errors = svc.validate_project_payload({"project_tagline": "Hi "}, "team-1") + assert errors == [] + assert " for the queue."}, "team-1" + ) + assert errors == [] + assert "List" in clean["project_story"] + + +def test_validate_project_payload_rejects_tagline_over_limit(): + clean, errors = svc.validate_project_payload({"project_tagline": "x" * 141}, "team-1") + assert any(e["field"] == "project_tagline" for e in errors) + assert "project_tagline" not in clean + + +def test_validate_project_payload_rejects_too_many_built_with_tags(): + clean, errors = svc.validate_project_payload({"project_built_with": [f"tag{i}" for i in range(26)]}, "team-1") + assert any(e["field"] == "project_built_with" for e in errors) + + +def test_validate_project_payload_rejects_too_many_links(): + links = [{"label": "l", "url": "https://example.com"} for _ in range(11)] + clean, errors = svc.validate_project_payload({"project_links": links}, "team-1") + assert any(e["field"] == "project_links" for e in errors) + + +def test_validate_project_payload_rejects_http_link_url(): + clean, errors = svc.validate_project_payload( + {"project_links": [{"label": "Repo", "url": "http://example.com"}]}, "team-1" + ) + assert any(e["field"] == "project_links" for e in errors) + + +def test_validate_project_payload_rejects_thumbnail_off_own_cdn(): + clean, errors = svc.validate_project_payload( + {"project_thumbnail_url": "https://evil.example.com/x.png"}, "team-1" + ) + assert any(e["field"] == "project_thumbnail_url" for e in errors) + + +def test_validate_project_payload_trusts_existing_thumbnail_without_reverify(monkeypatch): + url = f"{svc._cdn_server()}/teams/team-1/project/thumb.png" + # If GCS verification were attempted it would raise via this stub. + def boom(path): + raise AssertionError("should not re-verify an already-saved URL") + monkeypatch.setattr("common.utils.cdn.get_blob_metadata", boom) + clean, errors = svc.validate_project_payload( + {"project_thumbnail_url": url}, "team-1", existing={"project_thumbnail_url": url} + ) + assert errors == [] + assert clean["project_thumbnail_url"] == url + + +def test_validate_project_payload_verifies_new_own_cdn_thumbnail(monkeypatch): + url = f"{svc._cdn_server()}/teams/team-1/project/thumb.png" + monkeypatch.setattr( + "common.utils.cdn.get_blob_metadata", + lambda path: {"exists": True, "size": 1000, "content_type": "image/png"}, + ) + clean, errors = svc.validate_project_payload({"project_thumbnail_url": url}, "team-1") + assert errors == [] + assert clean["project_thumbnail_url"] == url + + +def test_validate_project_payload_rejects_missing_upload(monkeypatch): + url = f"{svc._cdn_server()}/teams/team-1/project/thumb.png" + monkeypatch.setattr( + "common.utils.cdn.get_blob_metadata", + lambda path: {"exists": False, "size": None, "content_type": None}, + ) + clean, errors = svc.validate_project_payload({"project_thumbnail_url": url}, "team-1") + assert any(e["reason"] == "upload_not_found" for e in errors) + + +# --------------------------------------------------------------------------- +# save_project — auth, deadline gate, draft stamping +# --------------------------------------------------------------------------- + +def test_save_project_403_for_non_member(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=False) + _event(monkeypatch) + result, status = svc.save_project("propel-1", "team-1", {"project_tagline": "hi"}) + assert status == 403 + assert result["error"] == "not_team_member" + + +def test_save_project_409_when_closed(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=True) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: { + "timezone": "UTC", + "deadlines": {"submission": "2026-10-10T15:00:00+00:00"}, + }) + monkeypatch.setattr(svc, "compute_submission_window", lambda event, now=None: {"state": "closed", "submission": "2026-10-10T15:00:00+00:00", "late_until": None, "now": "2026-10-10T20:00:00+00:00", "timezone": "UTC"}) + result, status = svc.save_project("propel-1", "team-1", {"project_tagline": "hi"}) + assert status == 409 + assert result["error"] == "submissions_closed" + assert result["deadline"] == "2026-10-10T15:00:00+00:00" + + +def test_save_project_admin_bypasses_deadline(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=False) # admin bypasses membership too + monkeypatch.setattr(svc, "compute_submission_window", lambda event, now=None: {"state": "closed", "submission": "x", "late_until": None, "now": "y", "timezone": "UTC"}) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {}) + result, status = svc.save_project("admin-propel", "team-1", {"project_tagline": "hi"}, admin=True) + assert status == 200 + assert result["success"] is True + + +def test_save_project_first_save_sets_draft_status(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=True) + _event(monkeypatch) + result, status = svc.save_project("propel-1", "team-1", {"project_tagline": "Hi"}) + assert status == 200 + assert wire["team-1"]["project_submission_status"] == "draft" + assert wire["team-1"]["project_tagline"] == "Hi" + + +def test_save_project_400_on_invalid_payload(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=True) + _event(monkeypatch) + result, status = svc.save_project("propel-1", "team-1", {"project_tagline": "x" * 200}) + assert status == 400 + assert result["error"] == "invalid_project" + + +def test_save_project_after_submit_keeps_status(wire, monkeypatch): + _seed_team(wire, project_submission_status="submitted", project_tagline="T", project_story="S") + _member(monkeypatch, is_member=True) + _event(monkeypatch) + result, status = svc.save_project("propel-1", "team-1", {"project_tagline": "Updated tagline"}) + assert status == 200 + assert wire["team-1"]["project_submission_status"] == "submitted" + assert wire["team-1"]["project_tagline"] == "Updated tagline" + + +# --------------------------------------------------------------------------- +# submit_project +# --------------------------------------------------------------------------- + +def test_submit_project_400_incomplete_without_story(wire, monkeypatch): + _seed_team(wire, project_tagline="Only a tagline") + _member(monkeypatch, is_member=True) + _event(monkeypatch) + result, status = svc.submit_project("propel-1", "team-1") + assert status == 400 + assert result["error"] == "incomplete" + assert "project_story" in result["missing"] + + +def test_submit_project_sets_submitted_when_open(wire, monkeypatch): + _seed_team(wire, project_tagline="T", project_story="S") + _member(monkeypatch, is_member=True) + _event(monkeypatch) + result, status = svc.submit_project("propel-1", "team-1") + assert status == 200 + assert wire["team-1"]["project_submission_status"] == "submitted" + assert "project_submitted_at" in wire["team-1"] + + +def test_submit_project_sets_late_inside_grace_window(wire, monkeypatch): + _seed_team(wire, project_tagline="T", project_story="S") + _member(monkeypatch, is_member=True) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {}) + monkeypatch.setattr(svc, "compute_submission_window", lambda event, now=None: {"state": "late", "submission": "x", "late_until": "y", "now": "z", "timezone": "UTC"}) + result, status = svc.submit_project("propel-1", "team-1") + assert status == 200 + assert wire["team-1"]["project_submission_status"] == "late" + + +def test_submit_project_idempotent_when_already_submitted(wire, monkeypatch): + _seed_team(wire, project_submission_status="submitted", project_tagline="T", project_story="S") + _member(monkeypatch, is_member=True) + _event(monkeypatch) + result, status = svc.submit_project("propel-1", "team-1") + assert status == 200 + assert result["already_submitted"] is True + + +def test_submit_project_403_for_non_member(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=False) + _event(monkeypatch) + result, status = svc.submit_project("propel-1", "team-1") + assert status == 403 + + +def test_submit_project_409_when_closed_for_non_admin(wire, monkeypatch): + _seed_team(wire, project_tagline="T", project_story="S") + _member(monkeypatch, is_member=True) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {}) + monkeypatch.setattr(svc, "compute_submission_window", lambda event, now=None: {"state": "closed", "submission": "x", "late_until": None, "now": "z", "timezone": "UTC"}) + result, status = svc.submit_project("propel-1", "team-1") + assert status == 409 + assert result["error"] == "submissions_closed" + + +def test_submit_project_admin_forced_after_close_is_recorded_late(wire, monkeypatch): + _seed_team(wire, project_tagline="T", project_story="S") + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {}) + monkeypatch.setattr(svc, "compute_submission_window", lambda event, now=None: {"state": "closed", "submission": "x", "late_until": None, "now": "z", "timezone": "UTC"}) + result, status = svc.submit_project("admin-propel", "team-1", admin=True) + assert status == 200 + assert wire["team-1"]["project_submission_status"] == "late" + + +# --------------------------------------------------------------------------- +# self_serve_team_edit — Part 9 bug #1 (was: any logged-in user could edit +# any team's devpost/demo-video link). +# --------------------------------------------------------------------------- + +def test_self_serve_team_edit_403_for_non_member(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=False) + _event(monkeypatch) + result, status = svc.self_serve_team_edit("propel-1", "team-1", {"devpost_link": "https://devpost.com/x"}) + assert status == 403 + assert result["error"] == "not_team_member" + + +def test_self_serve_team_edit_allows_member(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=True) + _event(monkeypatch) + monkeypatch.setattr( + "api.teams.teams_service.edit_team", + lambda json: {"success": True, "message": "Team updated successfully", "team_id": json["id"]}, + ) + result = svc.self_serve_team_edit("propel-1", "team-1", {"devpost_link": "https://devpost.com/x"}) + assert result["success"] is True + assert "team" in result + + +def test_self_serve_team_edit_409_when_closed(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=True) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {}) + monkeypatch.setattr(svc, "compute_submission_window", lambda event, now=None: {"state": "closed", "submission": "x", "late_until": None, "now": "z", "timezone": "UTC"}) + result, status = svc.self_serve_team_edit("propel-1", "team-1", {"demo_video_url": "https://youtu.be/x"}) + assert status == 409 + + +# --------------------------------------------------------------------------- +# set_mentor_help_wanted +# --------------------------------------------------------------------------- + +def test_set_mentor_help_wanted_rejects_non_bool(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=True) + result, status = svc.set_mentor_help_wanted("propel-1", "team-1", "yes") + assert status == 400 + + +def test_set_mentor_help_wanted_no_deadline_gate_even_when_closed(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=True) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {}) + monkeypatch.setattr(svc, "compute_submission_window", lambda event, now=None: {"state": "closed", "submission": "x", "late_until": None, "now": "z", "timezone": "UTC"}) + result, status = svc.set_mentor_help_wanted("propel-1", "team-1", False) + assert status == 200 + assert wire["team-1"]["mentor_help_wanted"] is False + + +def test_set_mentor_help_wanted_403_for_non_member(wire, monkeypatch): + _seed_team(wire) + _member(monkeypatch, is_member=False) + result, status = svc.set_mentor_help_wanted("propel-1", "team-1", True) + assert status == 403 + + +# --------------------------------------------------------------------------- +# get_submission_window_for_event +# --------------------------------------------------------------------------- + +def test_get_submission_window_for_event_404_unknown_event(monkeypatch): + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: None) + result, status = svc.get_submission_window_for_event("nope") + assert status == 404 + + +def test_get_submission_window_for_event_returns_window(monkeypatch): + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"timezone": "UTC", "deadlines": {}}) + result, status = svc.get_submission_window_for_event("event-1") + assert status == 200 + assert result["state"] == "no_deadline" + + +# --------------------------------------------------------------------------- +# Deadline reminders — build_reminder_message / send_deadline_reminders / +# send_due_reminders_for_current_events. A separate, more general fake db is +# used here (needs .where().stream() across multiple team docs, which the +# id-only FakeDb above doesn't support). +# --------------------------------------------------------------------------- + +class _ReminderFakeDocRef: + """Only .set() is exercised — send_deadline_reminders reads + reminders_sent off the already-mocked get_hackathon_by_event_id() return + value, never off a Firestore read of this ref.""" + + def __init__(self, store, key): + self._store = store + self._key = key + + def set(self, data, merge=False): + if merge and self._key in self._store: + existing = self._store[self._key] + for k, v in data.items(): + if isinstance(v, dict) and isinstance(existing.get(k), dict): + existing[k] = {**existing[k], **v} + else: + existing[k] = v + else: + self._store[self._key] = dict(data) + + +class _ReminderFakeTeamDoc: + def __init__(self, doc_id, data): + self.id = doc_id + self._data = data + + def to_dict(self): + return dict(self._data) + + +class _ReminderFakeQuery: + def __init__(self, teams, field, value): + self._teams = teams + self._field = field + self._value = value + + def stream(self): + return [ + _ReminderFakeTeamDoc(tid, data) + for tid, data in self._teams.items() + if data.get(self._field) == self._value + ] + + +class _ReminderFakeCollection: + def __init__(self, teams, hackathons, name): + self._teams = teams + self._hackathons = hackathons + self._name = name + + def document(self, doc_id): + assert self._name == "hackathons" + return _ReminderFakeDocRef(self._hackathons, doc_id) + + def where(self, field, op, value): + assert self._name == "teams" + return _ReminderFakeQuery(self._teams, field, value) + + +class _ReminderFakeDb: + def __init__(self, teams, hackathons): + self._teams = teams + self._hackathons = hackathons + + def collection(self, name): + return _ReminderFakeCollection(self._teams, self._hackathons, name) + + +@pytest.fixture +def reminder_wire(monkeypatch): + teams = {} + hackathons = {} + monkeypatch.setattr(svc, "get_db", lambda: _ReminderFakeDb(teams, hackathons)) + monkeypatch.setattr(svc, "clear_cache", lambda: None) + monkeypatch.setattr(svc, "send_slack_audit", lambda **kwargs: None) + sent_messages = [] + monkeypatch.setattr(svc, "send_slack", lambda message, channel: sent_messages.append((channel, message))) + return {"teams": teams, "hackathons": hackathons, "sent": sent_messages} + + +def test_build_reminder_message_none_when_already_submitted(): + team = {"project_submission_status": "submitted"} + assert svc.build_reminder_message(team, {"event_id": "e1"}, "x", 24) is None + + +def test_build_reminder_message_lists_missing_items(): + msg = svc.build_reminder_message({}, {"event_id": "e1"}, "x", 6) + assert "tagline" in msg + assert "demo video" in msg + assert "submit your project" in msg + assert "6 hours left" in msg + + +def test_build_reminder_message_singular_hour_label(): + msg = svc.build_reminder_message({}, {"event_id": "e1"}, "x", 1) + assert "1 hour left" in msg + assert "1 hours" not in msg + + +def test_send_deadline_reminders_404_unknown_event(monkeypatch): + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: None) + result, status = svc.send_deadline_reminders("nope", "submission", 24) + assert status == 404 + + +def test_send_deadline_reminders_409_when_no_deadline_configured(monkeypatch): + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"deadlines": {}}) + result, status = svc.send_deadline_reminders("event-1", "submission", 24) + assert status == 409 + assert result["error"] == "no_deadline" + + +def test_send_deadline_reminders_rejects_bad_kind_and_hours(monkeypatch): + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"deadlines": {"submission": "2026-01-01T00:00:00+00:00"}}) + _, status = svc.send_deadline_reminders("event-1", "judging", 24) + assert status == 400 + _, status = svc.send_deadline_reminders("event-1", "submission", 5) + assert status == 400 + + +def test_send_deadline_reminders_notifies_and_records_idempotency_key(reminder_wire, monkeypatch): + now = datetime.now(timezone.utc) + deadline = (now + timedelta(hours=24)).isoformat() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"id": "evtdoc-1", "event_id": "event-1", "deadlines": {"submission": deadline}}) + reminder_wire["teams"]["team-1"] = {"hackathon_event_id": "event-1", "slack_channel": "#team-1"} + + result, status = svc.send_deadline_reminders("event-1", "submission", 24) + + assert status == 200 + assert result["notified"] == ["team-1"] + assert reminder_wire["hackathons"]["evtdoc-1"]["reminders_sent"]["submission_24h"]["teams_notified"] == ["team-1"] + + +def test_send_deadline_reminders_skips_team_without_slack_channel(reminder_wire, monkeypatch): + now = datetime.now(timezone.utc) + deadline = (now + timedelta(hours=24)).isoformat() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"id": "evtdoc-1", "event_id": "event-1", "deadlines": {"submission": deadline}}) + reminder_wire["teams"]["team-1"] = {"hackathon_event_id": "event-1"} + + result, status = svc.send_deadline_reminders("event-1", "submission", 24) + + assert result["notified"] == [] + assert result["skipped"] == [{"team_id": "team-1", "reason": "no_slack_channel"}] + + +def test_send_deadline_reminders_skips_already_done_team(reminder_wire, monkeypatch): + now = datetime.now(timezone.utc) + deadline = (now + timedelta(hours=24)).isoformat() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"id": "evtdoc-1", "event_id": "event-1", "deadlines": {"submission": deadline}}) + reminder_wire["teams"]["team-1"] = {"hackathon_event_id": "event-1", "slack_channel": "#t1", "project_submission_status": "submitted"} + + result, status = svc.send_deadline_reminders("event-1", "submission", 24) + + assert result["notified"] == [] + assert result["skipped"] == [{"team_id": "team-1", "reason": "already_done"}] + + +def test_send_deadline_reminders_409_already_sent_then_force_succeeds(reminder_wire, monkeypatch): + now = datetime.now(timezone.utc) + deadline = (now + timedelta(hours=24)).isoformat() + + def event_with_reminders(): + return { + "id": "evtdoc-1", "event_id": "event-1", "deadlines": {"submission": deadline}, + "reminders_sent": {"submission_24h": {"sent_at": "earlier"}}, + } + + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event_with_reminders()) + reminder_wire["teams"]["team-1"] = {"hackathon_event_id": "event-1", "slack_channel": "#t1"} + + result, status = svc.send_deadline_reminders("event-1", "submission", 24) + assert status == 409 + assert result["error"] == "already_sent" + + result, status = svc.send_deadline_reminders("event-1", "submission", 24, force=True) + assert status == 200 + assert result["notified"] == ["team-1"] + + +def test_send_deadline_reminders_only_if_due_skips_outside_window(reminder_wire, monkeypatch): + now = datetime.now(timezone.utc) + # Deadline is 10 hours away; a 24h reminder isn't due yet (due window is + # [deadline-24h, deadline)). + deadline = (now + timedelta(hours=10)).isoformat() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"id": "evtdoc-1", "event_id": "event-1", "deadlines": {"submission": deadline}}) + reminder_wire["teams"]["team-1"] = {"hackathon_event_id": "event-1", "slack_channel": "#t1"} + + result, status = svc.send_deadline_reminders("event-1", "submission", 1, only_if_due=True) + assert status == 200 + assert result["skipped"] == "not_due" + assert reminder_wire["teams"]["team-1"] # untouched, no reminder recorded + + +def test_send_deadline_reminders_only_if_due_sends_inside_window(reminder_wire, monkeypatch): + now = datetime.now(timezone.utc) + deadline = (now + timedelta(hours=23)).isoformat() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"id": "evtdoc-1", "event_id": "event-1", "deadlines": {"submission": deadline}}) + reminder_wire["teams"]["team-1"] = {"hackathon_event_id": "event-1", "slack_channel": "#t1"} + + result, status = svc.send_deadline_reminders("event-1", "submission", 24, only_if_due=True) + assert status == 200 + assert result["notified"] == ["team-1"] + + +def test_send_deadline_reminders_simulated_flag_reflects_test_environment(reminder_wire, monkeypatch): + now = datetime.now(timezone.utc) + deadline = (now + timedelta(hours=24)).isoformat() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"id": "evtdoc-1", "event_id": "event-1", "deadlines": {"submission": deadline}}) + reminder_wire["teams"]["team-1"] = {"hackathon_event_id": "event-1", "slack_channel": "#t1"} + + result, status = svc.send_deadline_reminders("event-1", "submission", 24) + assert result["simulated"] is True + # ENVIRONMENT=test means _notifications_disabled() is True, so send_slack + # is never actually invoked even though the team is still "notified". + assert reminder_wire["sent"] == [] + + +def test_send_due_reminders_for_current_events_iterates_hours_and_events(reminder_wire, monkeypatch): + now = datetime.now(timezone.utc) + deadline = (now + timedelta(hours=23)).isoformat() + monkeypatch.setattr( + "services.hackathons_service.get_hackathon_list", + lambda kind: {"hackathons": [{"event_id": "event-1"}]}, + ) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"id": "evtdoc-1", "event_id": "event-1", "deadlines": {"submission": deadline}}) + reminder_wire["teams"]["team-1"] = {"hackathon_event_id": "event-1", "slack_channel": "#t1"} + + result, status = svc.send_due_reminders_for_current_events() + + assert status == 200 + hours_checked = {r["hours_before"] for r in result["results"]} + assert hours_checked == {24, 6, 1} + # Only the 24h reminder was due (deadline is 23h away); confirm it fired. + fired = [r for r in result["results"] if r["hours_before"] == 24][0] + assert fired["result"]["notified"] == ["team-1"] diff --git a/api/submissions/tests/test_submissions_views.py b/api/submissions/tests/test_submissions_views.py new file mode 100644 index 0000000..29e5837 --- /dev/null +++ b/api/submissions/tests/test_submissions_views.py @@ -0,0 +1,188 @@ +""" +Route-level tests for the submissions blueprint. Copies the signature-check +fixture from api/volunteers/tests/test_volunteers_views.py: propelauth's +decorators don't inject the user into the view, so a `def view(user, ...)` +mismatch only surfaces at real Flask dispatch time, not 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.submissions.submissions_views" +FAKE_USER = types.SimpleNamespace(user_id="hacker-propel-uuid", email="hacker@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) + 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 app(views): + flask_app = Flask(__name__) + flask_app.register_blueprint(views.bp) + return flask_app + + +@pytest.fixture +def client(app): + return app.test_client() + + +HEADERS = {"Authorization": "Bearer test"} + + +def test_every_view_signature_matches_its_url_params(app): + 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 == [], f"view params != URL params: {mismatches}" + + +def test_save_project_route_dispatches_with_token_identity(views, client, monkeypatch): + service = MagicMock(return_value=({"success": True}, 200)) + monkeypatch.setattr(views, "save_project", service) + monkeypatch.setattr(views, "is_admin", lambda user: False) + + res = client.post("/api/team/team-1/project", json={"project_tagline": "Hi"}, headers=HEADERS) + + assert res.status_code == 200, res.get_json() + service.assert_called_once_with(FAKE_USER.user_id, "team-1", {"project_tagline": "Hi"}, admin=False) + + +def test_submit_project_route_dispatches(views, client, monkeypatch): + service = MagicMock(return_value=({"success": True, "status": "submitted"}, 200)) + monkeypatch.setattr(views, "submit_project", service) + monkeypatch.setattr(views, "is_admin", lambda user: False) + + res = client.post("/api/team/team-1/project/submit", headers=HEADERS) + + assert res.status_code == 200, res.get_json() + service.assert_called_once_with(FAKE_USER.user_id, "team-1", admin=False) + + +def test_mentor_availability_route_dispatches(views, client, monkeypatch): + service = MagicMock(return_value=({"success": True}, 200)) + monkeypatch.setattr(views, "set_mentor_help_wanted", service) + monkeypatch.setattr(views, "is_admin", lambda user: False) + + res = client.post("/api/team/team-1/mentor-availability", json={"open": False}, headers=HEADERS) + + assert res.status_code == 200, res.get_json() + service.assert_called_once_with(FAKE_USER.user_id, "team-1", False, admin=False) + + +def test_submissions_window_route_is_public_and_dispatches(views, client, monkeypatch): + service = MagicMock(return_value=({"state": "open"}, 200)) + monkeypatch.setattr(views, "get_submission_window_for_event", service) + + res = client.get("/api/hackathons/event-1/submissions/window") + + assert res.status_code == 200, res.get_json() + service.assert_called_once_with("event-1") + + +def test_admin_flag_passed_through_when_org_permission_present(views, client, monkeypatch): + service = MagicMock(return_value=({"success": True}, 200)) + monkeypatch.setattr(views, "save_project", service) + monkeypatch.setattr(views, "is_admin", lambda user: True) + + res = client.post("/api/team/team-1/project", json={}, headers=HEADERS) + + assert res.status_code == 200, res.get_json() + service.assert_called_once_with(FAKE_USER.user_id, "team-1", {}, admin=True) + + +def test_remind_route_403s_without_admin_or_api_key(views, client, monkeypatch): + monkeypatch.setattr(views, "is_admin", lambda user: False) + monkeypatch.setenv("BACKEND_CRON_TOKEN", "secret-token") + + res = client.post("/api/hackathons/event-1/deadlines/remind", json={"hours_before": 24}) + + assert res.status_code == 403 + + +def test_remind_route_allows_admin(views, client, monkeypatch): + monkeypatch.setattr(views, "is_admin", lambda user: True) + service = MagicMock(return_value=({"success": True}, 200)) + monkeypatch.setattr(views, "send_deadline_reminders", service) + + res = client.post("/api/hackathons/event-1/deadlines/remind", json={"hours_before": 24}, headers=HEADERS) + + assert res.status_code == 200, res.get_json() + service.assert_called_once_with("event-1", "submission", 24, only_if_due=False, force=False, actor=FAKE_USER.user_id) + + +def test_remind_route_allows_api_key_without_login(views, client, monkeypatch): + monkeypatch.setattr(views, "is_admin", lambda user: False) + monkeypatch.setenv("BACKEND_CRON_TOKEN", "secret-token") + service = MagicMock(return_value=({"success": True}, 200)) + monkeypatch.setattr(views, "send_deadline_reminders", service) + + res = client.post( + "/api/hackathons/event-1/deadlines/remind", + json={"hours_before": 6, "only_if_due": True}, + headers={"X-Api-Key": "secret-token"}, + ) + + assert res.status_code == 200, res.get_json() + service.assert_called_once_with("event-1", "submission", 6, only_if_due=True, force=False, actor="cron") + + +def test_remind_due_route_requires_api_key(views, client, monkeypatch): + monkeypatch.setenv("BACKEND_CRON_TOKEN", "secret-token") + res = client.post("/api/hackathons/deadlines/remind-due") + assert res.status_code == 403 + + +def test_remind_due_route_dispatches_with_valid_key(views, client, monkeypatch): + monkeypatch.setenv("BACKEND_CRON_TOKEN", "secret-token") + service = MagicMock(return_value=({"success": True, "results": []}, 200)) + monkeypatch.setattr(views, "send_due_reminders_for_current_events", service) + + res = client.post("/api/hackathons/deadlines/remind-due", headers={"X-Api-Key": "secret-token"}) + + assert res.status_code == 200, res.get_json() + service.assert_called_once_with() diff --git a/api/teams/teams_views.py b/api/teams/teams_views.py index b9595e8..d96b80b 100644 --- a/api/teams/teams_views.py +++ b/api/teams/teams_views.py @@ -78,19 +78,25 @@ def edit_team_api(): @auth.require_user def add_devpost_to_team_api(teamid): """ - Add a Devpost link to a team. - Requires user to be authenticated. + Add a Devpost link to a team. Self-serve — the caller must be on the team + (or an admin). Part 9 bug #1 fix: this used to call edit_team directly + with NO membership check, so any logged-in user could overwrite any + team's Devpost link. Routed through submissions.self_serve_team_edit, + which also 409s once the event's submission window has closed for a + non-admin caller. Lazy import: api.teams.teams_service must never import + api.submissions (one-directional dependency). """ logger.info(f"POST /team/{teamid}/devpost called") if auth_user and auth_user.user_id: - # Get the Devpost link from the request logger.info(f"Adding Devpost link to team {teamid}") - devpost_link = request.get_json().get("devpost_link") + devpost_link = (request.get_json() or {}).get("devpost_link") logger.info(f"Devpost link: {devpost_link}") if not devpost_link: return {"error": "Devpost link is required"}, 400 - return edit_team({"id": teamid, "devpost_link": devpost_link}) + from services.hackathon_planning_service import is_admin + from api.submissions.submissions_service import self_serve_team_edit + return self_serve_team_edit(auth_user.user_id, teamid, {"devpost_link": devpost_link}, admin=is_admin(auth_user)) logger.error("Could not obtain user details for POST /team//devpost") return {"error": "Unauthorized"}, 401 @@ -99,8 +105,9 @@ def add_devpost_to_team_api(teamid): @auth.require_user def add_demo_video_to_team_api(teamid): """ - Add or clear a demo video URL for a team. - Requires user to be authenticated (team self-serve, mirrors devpost endpoint). + Add or clear a demo video URL for a team. Self-serve — the caller must be + on the team (or an admin); see add_devpost_to_team_api's docstring for the + Part 9 bug #1 context this fixes too. Pass demo_video_url as empty string or null to clear. """ logger.info(f"POST /team/{teamid}/demo-video called") @@ -108,7 +115,10 @@ def add_demo_video_to_team_api(teamid): body = request.get_json() or {} demo_video_url = body.get("demo_video_url", "") # Allow empty string to clear the field; edit_team normalizes to None - return edit_team({"id": teamid, "demo_video_url": demo_video_url}) + + from services.hackathon_planning_service import is_admin + from api.submissions.submissions_service import self_serve_team_edit + return self_serve_team_edit(auth_user.user_id, teamid, {"demo_video_url": demo_video_url}, admin=is_admin(auth_user)) logger.error("Could not obtain user details for POST /team//demo-video") return {"error": "Unauthorized"}, 401 diff --git a/api/teams/tests/__init__.py b/api/teams/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/api/teams/tests/test_teams_devpost_demo_video_views.py b/api/teams/tests/test_teams_devpost_demo_video_views.py new file mode 100644 index 0000000..2f4457d --- /dev/null +++ b/api/teams/tests/test_teams_devpost_demo_video_views.py @@ -0,0 +1,128 @@ +""" +Regression tests for Part 9 bug #1: POST /api/team//devpost and +POST /api/team//demo-video used to call edit_team directly with NO +membership check, so any logged-in user could overwrite any team's Devpost +link or demo video. They now route through +api.submissions.submissions_service.self_serve_team_edit, which enforces +team membership (or admin) and the submission deadline. These tests check +the view-layer wiring; self_serve_team_edit's own gating logic is covered in +api/submissions/tests/test_submissions_service.py. + +Uses the same propelauth-stub pattern as +api/volunteers/tests/test_volunteers_views.py. +""" +import functools +import importlib +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.teams.teams_views" +FAKE_USER = types.SimpleNamespace(user_id="caller-propel-uuid", email="caller@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) + 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 app(views): + flask_app = Flask(__name__) + flask_app.register_blueprint(views.bp) + return flask_app + + +@pytest.fixture +def client(app): + return app.test_client() + + +HEADERS = {"Authorization": "Bearer test"} + + +def test_devpost_route_403s_non_member(client, monkeypatch): + """The actual regression: a non-member must be rejected, not silently + allowed to overwrite the team's Devpost link.""" + monkeypatch.setattr("services.hackathon_planning_service.is_admin", lambda user: False) + monkeypatch.setattr( + "api.submissions.submissions_service._authorize_team_write", + lambda propel, team_id, admin=False, enforce_deadline=True: (({"error": "not_team_member"}, 403), None, None, None), + ) + + res = client.post("/api/team/team-1/devpost", json={"devpost_link": "https://devpost.com/x"}, headers=HEADERS) + + assert res.status_code == 403 + assert res.get_json()["error"] == "not_team_member" + + +def test_devpost_route_allows_member(client, monkeypatch): + monkeypatch.setattr("services.hackathon_planning_service.is_admin", lambda user: False) + monkeypatch.setattr( + "api.submissions.submissions_service.self_serve_team_edit", + MagicMock(return_value={"success": True, "team": {"id": "team-1"}}), + ) + + res = client.post("/api/team/team-1/devpost", json={"devpost_link": "https://devpost.com/x"}, headers=HEADERS) + + assert res.status_code == 200 + assert res.get_json()["success"] is True + + +def test_devpost_route_requires_link(client, monkeypatch): + monkeypatch.setattr("services.hackathon_planning_service.is_admin", lambda user: False) + res = client.post("/api/team/team-1/devpost", json={}, headers=HEADERS) + assert res.status_code == 400 + + +def test_demo_video_route_403s_non_member(client, monkeypatch): + monkeypatch.setattr("services.hackathon_planning_service.is_admin", lambda user: False) + monkeypatch.setattr( + "api.submissions.submissions_service.self_serve_team_edit", + lambda propel, team_id, fields, admin=False: ({"error": "not_team_member"}, 403), + ) + + res = client.post("/api/team/team-1/demo-video", json={"demo_video_url": "https://youtu.be/x"}, headers=HEADERS) + + assert res.status_code == 403 + + +def test_demo_video_route_allows_member_and_passes_admin_flag(client, monkeypatch): + monkeypatch.setattr("services.hackathon_planning_service.is_admin", lambda user: True) + service = MagicMock(return_value={"success": True, "team": {"id": "team-1"}}) + monkeypatch.setattr("api.submissions.submissions_service.self_serve_team_edit", service) + + res = client.post("/api/team/team-1/demo-video", json={"demo_video_url": ""}, headers=HEADERS) + + assert res.status_code == 200 + service.assert_called_once_with(FAKE_USER.user_id, "team-1", {"demo_video_url": ""}, admin=True) From 22596545868aabbb8b4bd21815e9070184bbcdb4 Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:19:14 -0700 Subject: [PATCH 04/11] Widen GET /api/volunteer//me to type=hacker services/volunteers_service.py::get_volunteer_self_status mirrors api.mentors.mentors_service.get_mentor_self_status's shape/leanness ({f"is_{type}": bool, "volunteer": {"name","isSelected"}|None}) for any volunteer_type via the existing find_volunteer_by_caller_identity resolver. type=mentor still delegates to the original mentor-specific service unchanged; type=hacker is new. Backs the team dashboard's "am I an approved hacker for this event" gate and Hackers' Choice eligibility. Part of the team-dashboard-devpost-replacement plan, WS-A task 5. Co-Authored-By: Claude Sonnet 5 --- .../tests/test_volunteers_service.py | 36 ++++++++++++++++++- api/volunteers/volunteers_views.py | 20 +++++++---- services/volunteers_service.py | 17 +++++++++ 3 files changed, 65 insertions(+), 8 deletions(-) diff --git a/api/volunteers/tests/test_volunteers_service.py b/api/volunteers/tests/test_volunteers_service.py index 423234e..d209501 100644 --- a/api/volunteers/tests/test_volunteers_service.py +++ b/api/volunteers/tests/test_volunteers_service.py @@ -671,4 +671,38 @@ def test_generate_qr_code_special_characters(): # Assertions assert qr_image_bytes is not None assert isinstance(qr_image_bytes, bytes) - assert len(qr_image_bytes) > 0 \ No newline at end of file + assert len(qr_image_bytes) > 0 + +# --------------------------------------------------------------------------- +# get_volunteer_self_status — generic self-check (type=hacker etc.) +# --------------------------------------------------------------------------- +from services.volunteers_service import get_volunteer_self_status # noqa: E402 + + +@patch('services.volunteers_service.find_volunteer_by_caller_identity') +def test_get_volunteer_self_status_selected_hacker(mock_find): + mock_find.return_value = {"name": "Jamie Hacker", "isSelected": True} + result = get_volunteer_self_status(MOCK_USER_ID, MOCK_EVENT_ID, "hacker") + assert result == {"is_hacker": True, "volunteer": {"name": "Jamie Hacker", "isSelected": True}} + mock_find.assert_called_once_with(MOCK_USER_ID, MOCK_EVENT_ID, "hacker") + + +@patch('services.volunteers_service.find_volunteer_by_caller_identity') +def test_get_volunteer_self_status_not_selected_returns_null_volunteer(mock_find): + mock_find.return_value = {"name": "Jamie Hacker", "isSelected": False} + result = get_volunteer_self_status(MOCK_USER_ID, MOCK_EVENT_ID, "hacker") + assert result == {"is_hacker": False, "volunteer": None} + + +@patch('services.volunteers_service.find_volunteer_by_caller_identity') +def test_get_volunteer_self_status_no_doc_returns_false(mock_find): + mock_find.return_value = None + result = get_volunteer_self_status(MOCK_USER_ID, MOCK_EVENT_ID, "hacker") + assert result == {"is_hacker": False, "volunteer": None} + + +@patch('services.volunteers_service.find_volunteer_by_caller_identity') +def test_get_volunteer_self_status_leaks_no_pii_beyond_name(mock_find): + mock_find.return_value = {"name": "Jamie", "isSelected": True, "email": "jamie@example.com", "phone": "555-1234"} + result = get_volunteer_self_status(MOCK_USER_ID, MOCK_EVENT_ID, "hacker") + assert set(result["volunteer"].keys()) == {"name", "isSelected"} diff --git a/api/volunteers/volunteers_views.py b/api/volunteers/volunteers_views.py index adeeb74..1411e7c 100644 --- a/api/volunteers/volunteers_views.py +++ b/api/volunteers/volunteers_views.py @@ -229,18 +229,24 @@ def get_my_volunteer_status_for_event(event_id): """ Lightweight self-check for the calling user against a single event. Default type is 'mentor' (used by the per-team mentor panel to decide - interactive vs. read-only). - Returns { is_mentor: bool, volunteer: {name, email, isSelected, checkInTime?} | null }. + interactive vs. read-only); type=hacker backs the team dashboard's + "am I an approved hacker for this event" gate (findteam/manageteam, + Hackers' Choice eligibility). + Returns { is_mentor: bool, volunteer: {name, email, isSelected, checkInTime?} | null } + for type=mentor, or { is_hacker: bool, volunteer: {name, isSelected} | null } for + type=hacker. """ user = auth_user if not user or not user.user_id: return _error_response("Authentication required", 401) vtype = request.args.get('type', 'mentor') - if vtype != 'mentor': - # Keep the surface narrow for now; extend later if needed. - return _error_response("Only type=mentor is supported", 400) - from api.mentors.mentors_service import get_mentor_self_status - return get_mentor_self_status(user.user_id, event_id) + if vtype == 'mentor': + from api.mentors.mentors_service import get_mentor_self_status + return get_mentor_self_status(user.user_id, event_id) + if vtype == 'hacker': + from services.volunteers_service import get_volunteer_self_status + return get_volunteer_self_status(user.user_id, event_id, 'hacker'), 200 + return _error_response("Only type=mentor or type=hacker is supported", 400) # Sponsor routes @bp.route('/sponsor/application//submit', methods=['POST']) diff --git a/services/volunteers_service.py b/services/volunteers_service.py index 6eb758a..fa9c8ad 100644 --- a/services/volunteers_service.py +++ b/services/volunteers_service.py @@ -163,6 +163,23 @@ def find_volunteer_by_caller_identity(propel_user_id: str, event_id: str, volunt volunteer = get_volunteer_by_user_id(oauth_user_id, event_id, volunteer_type) return volunteer + +def get_volunteer_self_status(propel_user_id: str, event_id: str, volunteer_type: str) -> Dict[str, Any]: + """ + Generic lightweight self-check, keyed by volunteer_type, for + GET /api/volunteer//me. Mirrors api.mentors.mentors_service. + get_mentor_self_status's shape/leanness but works for any volunteer_type + (introduced for type=hacker, used by the team dashboard + Hackers' Choice + eligibility gate). The volunteer subset returned is intentionally lean — + only what the caller already knows about themselves. + """ + doc = find_volunteer_by_caller_identity(propel_user_id, event_id, volunteer_type) + is_selected = bool(doc and doc.get("isSelected")) + return { + f"is_{volunteer_type}": is_selected, + "volunteer": {"name": doc.get("name"), "isSelected": True} if is_selected else None, + } + # Function to clear all caches related to a volunteer def _clear_volunteer_caches(user_id: str, email: str, event_id: str, volunteer_type: str): """Clear all caches related to a specific volunteer.""" From 4ff54cea60e063544ed43bfeb3a14f4b0be7ef37 Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:19:22 -0700 Subject: [PATCH 05/11] Add GET /api/github/activity (team dashboard "Code activity" card) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit common/utils/github.py::get_repo_activity(org, repo) makes exactly 3 GitHub API calls (get_repo, get_commits().get_page(0), get_pulls( state="open").totalCount) and derives last-commit info, last-24h commit count, and top-8 contributors from that single commit page. A 409 "empty repository" from get_commits is a valid all-zeros result (a fresh team repo), not an error. api/github/github_service.py::get_github_activity validates org/repo names, caches successes only (5 min TTL, separate from the existing issues cache), and translates UnknownObjectException/ RateLimitExceededException into 404/503. Part 9 bug #7 fixed while touching this file: GET /api/github/issues let a request through with no `org` (200 instead of 400), and its log line printed len(issues) where `issues` is the response dict — that logged the dict's key count, not the issue count. Tests: api/github/tests (exactly-3-calls assertion via a fake Github client, 24h math, contributor cap, empty-repo, service-layer caching and error translation, the /issues and /activity route fixes). Co-Authored-By: Claude Sonnet 5 --- api/github/github_service.py | 59 ++++- api/github/github_views.py | 37 ++- api/github/tests/__init__.py | 0 api/github/tests/test_github_activity.py | 275 +++++++++++++++++++++++ api/github/tests/test_github_views.py | 59 +++++ common/utils/github.py | 99 +++++++- 6 files changed, 523 insertions(+), 6 deletions(-) create mode 100644 api/github/tests/__init__.py create mode 100644 api/github/tests/test_github_activity.py create mode 100644 api/github/tests/test_github_views.py diff --git a/api/github/github_service.py b/api/github/github_service.py index 670779b..26f58a9 100644 --- a/api/github/github_service.py +++ b/api/github/github_service.py @@ -1,8 +1,9 @@ +import re import logging from typing import Dict, Any, List from cachetools import TTLCache from db.db import get_db -from common.utils.github import create_issue, get_issues +from common.utils.github import create_issue, get_issues, get_repo_activity logger = logging.getLogger("api.github.github_service") logger.setLevel(logging.DEBUG) @@ -13,6 +14,62 @@ # GitHub failures retry on the next request. _ISSUES_CACHE = TTLCache(maxsize=512, ttl=600) +# Team dashboard "Code activity" card (GET /api/github/activity). Separate, +# shorter-TTL cache from _ISSUES_CACHE since this is meant to feel live-ish. +_ACTIVITY_CACHE = TTLCache(maxsize=512, ttl=300) + +# GitHub org/repo name charset. Loose but enough to reject path-injection-ish +# input before it reaches PyGithub. +_GITHUB_NAME_RE = re.compile(r"^[A-Za-z0-9_.-]{1,100}$") + + +def get_github_activity(org_name: str, repo_name: str) -> Dict[str, Any]: + """ + GET /api/github/activity backing service. Success-only cache (mirrors + get_github_issues): a rate-limit or repo-not-found response is never + cached, so the next request retries against GitHub instead of pinning a + transient failure for the TTL window. + + Returns (payload, status): + 200 {success, repo, commits, contributors, open_prs} + 400 {"error": "invalid_repo"} — org/repo fails the name regex + 404 {"error": "repo_not_found"} + 503 {"error": "github_rate_limited", "reset_at"} + 502 {"error": "github_unavailable"} — anything else from PyGithub + """ + if not org_name or not repo_name or not _GITHUB_NAME_RE.match(org_name) or not _GITHUB_NAME_RE.match(repo_name): + return {"error": "invalid_repo"}, 400 + + cache_key = (org_name, repo_name) + cached = _ACTIVITY_CACHE.get(cache_key) + if cached is not None: + return cached, 200 + + from github import UnknownObjectException, RateLimitExceededException, GithubException + + try: + result = get_repo_activity(org_name, repo_name) + except UnknownObjectException: + logger.info("get_github_activity: repo not found org=%s repo=%s", org_name, repo_name) + return {"error": "repo_not_found"}, 404 + except RateLimitExceededException as e: + reset_at = None + try: + reset_at = e.headers.get("x-ratelimit-reset") if e.headers else None + except Exception: + reset_at = None + logger.warning("get_github_activity: rate limited org=%s repo=%s", org_name, repo_name) + return {"error": "github_rate_limited", "reset_at": reset_at}, 503 + except GithubException as e: + logger.error("get_github_activity: GitHub error org=%s repo=%s: %s", org_name, repo_name, e) + return {"error": "github_unavailable"}, 502 + except Exception as e: + logger.error("get_github_activity: unexpected error org=%s repo=%s: %s", org_name, repo_name, e) + return {"error": "github_unavailable"}, 502 + + _ACTIVITY_CACHE[cache_key] = result + return result, 200 + def get_github_organization_data(org_name: str) -> Dict[str, Any]: """ Get GitHub organization data including repositories and contributors. diff --git a/api/github/github_views.py b/api/github/github_views.py index e5ef428..75b8501 100644 --- a/api/github/github_views.py +++ b/api/github/github_views.py @@ -6,7 +6,8 @@ get_github_contributors_by_org, get_github_contributors_by_repo, create_github_issue, - get_github_issues + get_github_issues, + get_github_activity, ) from common.auth import auth, auth_user, getOrgId @@ -180,7 +181,9 @@ def get_issues_api(): Query Parameters: repo: Required. The repository name. - org: Optional. The organization name. + org: Required. The organization name (the service 400s without it + anyway — this used to let a missing org through to a 200 error + body instead of a real 400). state: Optional. The state of the issues ('open', 'closed', 'all'). Defaults to 'open'. Returns: @@ -193,13 +196,39 @@ def get_issues_api(): if not repo_name: return jsonify({"error": "repo parameter is required"}), 400 + if not org_name: + return jsonify({"error": "org parameter is required"}), 400 logger.info("Getting issues for repo: %s, org: %s, state: %s", repo_name, org_name, state) issues = get_github_issues(repo_name=repo_name, org_name=org_name, state=state) - logger.info("Retrieved %d issues for repo: %s", len(issues), repo_name) + # `issues` is the service's response dict ({"success", "issues": [...]} + # or {"error": ...}) — logging len(issues) here was logging the dict's + # KEY COUNT, not the issue count. + issue_count = len(issues.get("issues", [])) if isinstance(issues, dict) else 0 + logger.info("Retrieved %d issues for repo: %s", issue_count, repo_name) return jsonify(issues) except Exception as e: logger.error("Error getting issues: %s", str(e)) - return jsonify({"error": str(e)}), 500 \ No newline at end of file + return jsonify({"error": str(e)}), 500 + + +@bp.route("/activity", methods=["GET"]) +def get_activity_api(): + """ + Team dashboard "Code activity" card: last commit, commits in the last + 24h, top contributors, open PR count. Public (same trust level as + /issues — no team-membership check; the payload has nothing private). + + Query Parameters: + org: Required. The GitHub organization name. + repo: Required. The repository name. + """ + org_name = request.args.get('org') + repo_name = request.args.get('repo') + if not org_name or not repo_name: + return jsonify({"error": "org and repo parameters are required"}), 400 + + payload, status = get_github_activity(org_name, repo_name) + return jsonify(payload), status \ No newline at end of file diff --git a/api/github/tests/__init__.py b/api/github/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/api/github/tests/test_github_activity.py b/api/github/tests/test_github_activity.py new file mode 100644 index 0000000..a7eb6c8 --- /dev/null +++ b/api/github/tests/test_github_activity.py @@ -0,0 +1,275 @@ +""" +Tests for common.utils.github.get_repo_activity and its +api.github.github_service.get_github_activity wrapper. + +A fake Github client stands in for PyGithub so we can assert EXACTLY 3 +"API calls" are made (get_repo, get_commits().get_page(0), get_pulls().totalCount) +and control commit timestamps/authors precisely. +""" +import os +from datetime import datetime, timedelta, timezone + +os.environ.setdefault("ENVIRONMENT", "test") + +import pytest +from github import GithubException, UnknownObjectException, RateLimitExceededException + +import common.utils.github as github_utils + + +# --------------------------------------------------------------------------- +# Fakes +# --------------------------------------------------------------------------- + +class FakeAuthor: + def __init__(self, login=None, avatar_url=None): + self.login = login + self.avatar_url = avatar_url + + +class FakeGitAuthor: + def __init__(self, name, date): + self.name = name + self.date = date + + +class FakeGitCommit: + def __init__(self, message, author): + self.message = message + self.author = author + + +class FakeCommit: + def __init__(self, message, date, login=None, avatar_url=None, git_author_name="Someone"): + self.author = FakeAuthor(login, avatar_url) if login else None + self.commit = FakeGitCommit(message, FakeGitAuthor(git_author_name, date)) + + +class FakePullsList: + def __init__(self, calls, count): + self._calls = calls + self._count = count + + @property + def totalCount(self): + self._calls.append("get_pulls.totalCount") + return self._count + + +class FakeCommitsPaginator: + def __init__(self, calls, commits, raise_empty_409=False): + self._calls = calls + self._commits = commits + self._raise_empty_409 = raise_empty_409 + + def get_page(self, page): + self._calls.append(f"get_commits.get_page({page})") + if self._raise_empty_409: + raise GithubException(409, {"message": "Git Repository is empty."}, None) + return self._commits + + +class FakeRepo: + def __init__(self, calls, commits, open_prs=0, raise_empty_409=False, **attrs): + self._calls = calls + self._commits = commits + self._open_prs = open_prs + self._raise_empty_409 = raise_empty_409 + self.html_url = attrs.get("html_url", "https://github.com/opportunity-hack/test-repo") + self.default_branch = attrs.get("default_branch", "main") + self.pushed_at = attrs.get("pushed_at") + self.open_issues_count = attrs.get("open_issues_count", 0) + self.stargazers_count = attrs.get("stargazers_count", 0) + + def get_commits(self): + return FakeCommitsPaginator(self._calls, self._commits, raise_empty_409=self._raise_empty_409) + + def get_pulls(self, state="open"): + return FakePullsList(self._calls, self._open_prs) + + +class FakeGithubClient: + def __init__(self, calls, repo=None, unknown=False, rate_limited=False): + self._calls = calls + self._repo = repo + self._unknown = unknown + self._rate_limited = rate_limited + + def get_repo(self, full_name): + self._calls.append(f"get_repo({full_name})") + if self._unknown: + raise UnknownObjectException(404, {"message": "Not Found"}, None) + if self._rate_limited: + raise RateLimitExceededException(403, {"message": "rate limited"}, {"x-ratelimit-reset": "12345"}) + return self._repo + + +# --------------------------------------------------------------------------- +# get_repo_activity — exact call count, math, contributors, empty repo +# --------------------------------------------------------------------------- + +def _patch_github(monkeypatch, client): + monkeypatch.setattr(github_utils, "Github", lambda *a, **kw: client) + + +def test_get_repo_activity_makes_exactly_three_calls(monkeypatch): + calls = [] + now = datetime.now(timezone.utc) + commits = [FakeCommit("Fix bug", now - timedelta(hours=1), login="alice")] + repo = FakeRepo(calls, commits, open_prs=2) + _patch_github(monkeypatch, FakeGithubClient(calls, repo=repo)) + + result = github_utils.get_repo_activity("opportunity-hack", "test-repo") + + assert calls == ["get_repo(opportunity-hack/test-repo)", "get_commits.get_page(0)", "get_pulls.totalCount"] + assert result["open_prs"] == 2 + + +def test_get_repo_activity_last_24h_math(monkeypatch): + calls = [] + now = datetime.now(timezone.utc) + commits = [ + FakeCommit("Recent", now - timedelta(hours=2), login="alice"), + FakeCommit("Also recent", now - timedelta(hours=23), login="bob"), + FakeCommit("Old", now - timedelta(days=3), login="carol"), + ] + repo = FakeRepo(calls, commits) + _patch_github(monkeypatch, FakeGithubClient(calls, repo=repo)) + + result = github_utils.get_repo_activity("org", "repo") + + assert result["commits"]["total_recent"] == 3 + assert result["commits"]["last_24h"] == 2 + assert result["commits"]["last_author"] == "alice" + assert result["commits"]["last_commit_message"] == "Recent" + + +def test_get_repo_activity_top_contributors_capped_at_eight(monkeypatch): + calls = [] + now = datetime.now(timezone.utc) + commits = [FakeCommit(f"c{i}", now, login=f"user{i % 10}") for i in range(30)] + repo = FakeRepo(calls, commits) + _patch_github(monkeypatch, FakeGithubClient(calls, repo=repo)) + + result = github_utils.get_repo_activity("org", "repo") + + assert len(result["contributors"]) == 8 + logins = {c["login"] for c in result["contributors"]} + assert logins.issubset({f"user{i}" for i in range(10)}) + + +def test_get_repo_activity_falls_back_to_commit_author_name_without_github_login(monkeypatch): + calls = [] + now = datetime.now(timezone.utc) + commits = [FakeCommit("No github account", now, login=None, git_author_name="Jane Doe")] + repo = FakeRepo(calls, commits) + _patch_github(monkeypatch, FakeGithubClient(calls, repo=repo)) + + result = github_utils.get_repo_activity("org", "repo") + + assert result["commits"]["last_author"] == "Jane Doe" + assert result["contributors"][0]["login"] == "Jane Doe" + + +def test_get_repo_activity_empty_repo_returns_zeros_not_an_error(monkeypatch): + calls = [] + repo = FakeRepo(calls, [], raise_empty_409=True) + _patch_github(monkeypatch, FakeGithubClient(calls, repo=repo)) + + result = github_utils.get_repo_activity("org", "brand-new-repo") + + assert result["success"] is True + assert result["commits"]["total_recent"] == 0 + assert result["commits"]["last_24h"] == 0 + assert result["commits"]["last_commit_at"] is None + assert result["contributors"] == [] + + +def test_get_repo_activity_reraises_non_409_github_exception(monkeypatch): + calls = [] + repo = FakeRepo(calls, [], raise_empty_409=False) + repo.get_commits = lambda: (_ for _ in ()).throw(GithubException(500, {"message": "boom"}, None)) + _patch_github(monkeypatch, FakeGithubClient(calls, repo=repo)) + + with pytest.raises(GithubException): + github_utils.get_repo_activity("org", "repo") + + +def test_get_repo_activity_propagates_unknown_object(monkeypatch): + calls = [] + _patch_github(monkeypatch, FakeGithubClient(calls, unknown=True)) + with pytest.raises(UnknownObjectException): + github_utils.get_repo_activity("org", "nope") + + +def test_get_repo_activity_propagates_rate_limit(monkeypatch): + calls = [] + _patch_github(monkeypatch, FakeGithubClient(calls, rate_limited=True)) + with pytest.raises(RateLimitExceededException): + github_utils.get_repo_activity("org", "repo") + + +# --------------------------------------------------------------------------- +# api.github.github_service.get_github_activity — translation + caching +# --------------------------------------------------------------------------- + +import api.github.github_service as github_service + + +def test_get_github_activity_rejects_invalid_repo_name(): + payload, status = github_service.get_github_activity("org", "bad repo name!") + assert status == 400 + assert payload["error"] == "invalid_repo" + + +def test_get_github_activity_404s_on_unknown_repo(monkeypatch): + github_service._ACTIVITY_CACHE.clear() + monkeypatch.setattr(github_service, "get_repo_activity", lambda org, repo: (_ for _ in ()).throw(UnknownObjectException(404, {}, None))) + payload, status = github_service.get_github_activity("org", "repo") + assert status == 404 + assert payload["error"] == "repo_not_found" + + +def test_get_github_activity_503s_on_rate_limit(monkeypatch): + github_service._ACTIVITY_CACHE.clear() + monkeypatch.setattr( + github_service, + "get_repo_activity", + lambda org, repo: (_ for _ in ()).throw(RateLimitExceededException(403, {}, {"x-ratelimit-reset": "999"})), + ) + payload, status = github_service.get_github_activity("org", "repo") + assert status == 503 + assert payload["error"] == "github_rate_limited" + + +def test_get_github_activity_caches_success_only(monkeypatch): + github_service._ACTIVITY_CACHE.clear() + call_count = {"n": 0} + + def fake_activity(org, repo): + call_count["n"] += 1 + return {"success": True, "repo": {}, "commits": {}, "contributors": [], "open_prs": 0} + + monkeypatch.setattr(github_service, "get_repo_activity", fake_activity) + + payload1, status1 = github_service.get_github_activity("org", "repo") + payload2, status2 = github_service.get_github_activity("org", "repo") + + assert status1 == status2 == 200 + assert call_count["n"] == 1 # second call served from cache + + +def test_get_github_activity_does_not_cache_errors(monkeypatch): + github_service._ACTIVITY_CACHE.clear() + call_count = {"n": 0} + + def fake_activity(org, repo): + call_count["n"] += 1 + raise UnknownObjectException(404, {}, None) + + monkeypatch.setattr(github_service, "get_repo_activity", fake_activity) + + github_service.get_github_activity("org", "repo") + github_service.get_github_activity("org", "repo") + + assert call_count["n"] == 2 # never cached, so it's retried diff --git a/api/github/tests/test_github_views.py b/api/github/tests/test_github_views.py new file mode 100644 index 0000000..595eb9e --- /dev/null +++ b/api/github/tests/test_github_views.py @@ -0,0 +1,59 @@ +""" +Route-level tests for the /issues org-required fix (Part 9 bug #7) and the +new /activity route. +""" +import os + +os.environ.setdefault("ENVIRONMENT", "test") + +from flask import Flask +from unittest.mock import MagicMock + +from api.github import github_views + + +def _app(): + app = Flask(__name__) + app.register_blueprint(github_views.bp) + return app + + +def test_issues_requires_org(monkeypatch): + client = _app().test_client() + res = client.get("/api/github/issues?repo=some-repo") + assert res.status_code == 400 + assert "org" in res.get_json()["error"] + + +def test_issues_requires_repo(): + client = _app().test_client() + res = client.get("/api/github/issues?org=some-org") + assert res.status_code == 400 + assert "repo" in res.get_json()["error"] + + +def test_issues_logs_issue_count_not_dict_key_count(monkeypatch, caplog): + monkeypatch.setattr( + github_views, + "get_github_issues", + lambda repo_name, org_name, state: {"success": True, "issues": [{"issue_number": 1}, {"issue_number": 2}]}, + ) + client = _app().test_client() + with caplog.at_level("INFO"): + res = client.get("/api/github/issues?org=o&repo=r") + assert res.status_code == 200 + assert any("Retrieved 2 issues" in r.message for r in caplog.records) + + +def test_activity_route_requires_org_and_repo(): + client = _app().test_client() + assert client.get("/api/github/activity?repo=r").status_code == 400 + assert client.get("/api/github/activity?org=o").status_code == 400 + + +def test_activity_route_dispatches_to_service(monkeypatch): + monkeypatch.setattr(github_views, "get_github_activity", MagicMock(return_value=({"success": True}, 200))) + client = _app().test_client() + res = client.get("/api/github/activity?org=o&repo=r") + assert res.status_code == 200 + assert res.get_json()["success"] is True diff --git a/common/utils/github.py b/common/utils/github.py index 27cca3c..84f1c78 100644 --- a/common/utils/github.py +++ b/common/utils/github.py @@ -1,5 +1,7 @@ import os +from collections import Counter +from datetime import datetime, timedelta, timezone from github import Github from dotenv import load_dotenv from github import GithubException @@ -9,6 +11,10 @@ logger.setLevel(logging.DEBUG) load_dotenv() +# Contributor list is capped to keep the team-dashboard "Code activity" card +# small; it's a snapshot of the most recent commit page, not a full history. +MAX_ACTIVITY_CONTRIBUTORS = 8 + def create_github_repo( repository_name, hackathon_event_id, @@ -192,7 +198,98 @@ def validate_github_username(github_username): -def get_all_repos(org_name): +def get_repo_activity(org_name, repo_name): + """ + A team's "Code activity" card (team dashboard, Sep 2026): last commit, + commits in the last 24h, top contributors, open PR count. Deliberately + exactly 3 GitHub API calls (rate-limit budget matters more here than + completeness — this is a live-ish snapshot, not a full history): + 1. g.get_repo(f"{org}/{repo}") + 2. repo.get_commits().get_page(0) (first page only, <=100 commits) + 3. repo.get_pulls(state="open").totalCount + + Contributors are derived from that single commit page (a Counter over + each commit's author), not a separate contributors-stats call. + + Raises UnknownObjectException (repo doesn't exist) and + RateLimitExceededException to the caller — api.github.github_service + translates those into 404/503. An empty repository (GitHub returns 409 + for get_commits on one) is NOT an error here — it's a valid all-zeros + result for a freshly created team repo. + """ + g = Github(os.getenv('GITHUB_TOKEN'), per_page=100) + repo = g.get_repo(f"{org_name}/{repo_name}") + + try: + commits = list(repo.get_commits().get_page(0)) + except GithubException as e: + if e.status == 409: + commits = [] + else: + raise + + open_prs = repo.get_pulls(state="open").totalCount + + now = datetime.now(timezone.utc) + last_24h = 0 + last_commit_at = None + last_commit_message = None + last_author = None + contributor_counts = Counter() + contributor_avatars = {} + + for i, commit in enumerate(commits): + git_commit = getattr(commit, "commit", None) + git_author = getattr(git_commit, "author", None) if git_commit else None + commit_date = getattr(git_author, "date", None) if git_author else None + if commit_date and commit_date.tzinfo is None: + commit_date = commit_date.replace(tzinfo=timezone.utc) + + if i == 0: + last_commit_at = commit_date + last_commit_message = getattr(git_commit, "message", None) if git_commit else None + last_author = ( + (commit.author.login if commit.author else None) + or (getattr(git_author, "name", None) if git_author else None) + ) + + if commit_date and (now - commit_date) <= timedelta(hours=24): + last_24h += 1 + + login = commit.author.login if commit.author else None + name = login or (getattr(git_author, "name", None) if git_author else None) or "Unknown" + avatar = commit.author.avatar_url if commit.author else None + contributor_counts[name] += 1 + if avatar: + contributor_avatars[name] = avatar + + contributors = [ + {"login": name, "avatar_url": contributor_avatars.get(name), "contributions": count} + for name, count in contributor_counts.most_common(MAX_ACTIVITY_CONTRIBUTORS) + ] + + return { + "success": True, + "repo": { + "html_url": repo.html_url, + "default_branch": repo.default_branch, + "pushed_at": repo.pushed_at.isoformat() if getattr(repo, "pushed_at", None) else None, + "open_issues_count": repo.open_issues_count, + "stargazers_count": repo.stargazers_count, + }, + "commits": { + "total_recent": len(commits), + "last_24h": last_24h, + "last_commit_at": last_commit_at.isoformat() if last_commit_at else None, + "last_commit_message": last_commit_message, + "last_author": last_author, + }, + "contributors": contributors, + "open_prs": open_prs, + } + + +def get_all_repos(org_name): g = Github(os.getenv('GITHUB_TOKEN')) org = g.get_organization(org_name) repos = org.get_repos() From 6e64bcc0e8eabdd70913905859fe4b79938bfe07 Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:19:33 -0700 Subject: [PATCH 06/11] Add api/peer_votes blueprint: Hackers' Choice peer vote MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An assigned-slate approval vote, deliberately not a popularity contest: each eligible voter (an isSelected hacker; optionally gated on their own team having submitted, via constraints.peer_vote_requires_submission) gets a deterministic, exposure-balanced slate of submitted projects (never their own team) seeded on sha256(event_id:propel_id), and approves up to peer_vote_max_picks of them. Scoring is the Wilson score interval lower bound of approvals/shown (not a raw rate), so a team shown to a handful of people isn't buried by exposure alone. No tallies are ever shown to a voter, only to admins. - GET .../peer-vote/slate, POST .../ballot (voter-facing, require_user) - GET .../results, POST .../ballots//void, POST .../publish (admin) — publish appends "Hackers' Choice" to the winning team's awards[] exactly once and writes a public summary doc - GET .../summary (public) constraints.peer_vote_enabled defaults to False and is checked on every voter-facing route; a disabled/unconfigured event returns {"status":"disabled"} from the slate route regardless of anything else. The slate's first-ever build for a voter runs inside a Firestore transaction that re-checks ballot existence, so a double-request can't double-persist a slate or double-increment exposure counts. Tests: own-team exclusion, unsubmitted/inactive teams excluded, slate capped and exposure-balanced, repeat-GET idempotence, the full ballot validation matrix, voided-ballot exclusion from results, Wilson lower bound spec values, idempotent publish, summary gating. Part of the team-dashboard-devpost-replacement plan, WS-A task 7. Co-Authored-By: Claude Sonnet 5 --- api/peer_votes/__init__.py | 0 api/peer_votes/peer_votes_service.py | 555 ++++++++++++++++++ api/peer_votes/peer_votes_views.py | 86 +++ api/peer_votes/tests/__init__.py | 0 .../tests/test_peer_votes_service.py | 538 +++++++++++++++++ 5 files changed, 1179 insertions(+) create mode 100644 api/peer_votes/__init__.py create mode 100644 api/peer_votes/peer_votes_service.py create mode 100644 api/peer_votes/peer_votes_views.py create mode 100644 api/peer_votes/tests/__init__.py create mode 100644 api/peer_votes/tests/test_peer_votes_service.py diff --git a/api/peer_votes/__init__.py b/api/peer_votes/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/api/peer_votes/peer_votes_service.py b/api/peer_votes/peer_votes_service.py new file mode 100644 index 0000000..b78530b --- /dev/null +++ b/api/peer_votes/peer_votes_service.py @@ -0,0 +1,555 @@ +""" +Hackers' Choice — an assigned-slate approval vote (Sep 2026). See +docs/plans/team-dashboard-devpost-replacement.md (frontend repo) Part 2.5 for +the product rationale (anti-popularity: exposure-balanced slates, approval +not ranking, no visible tallies) and Part 3 for the wire contract. + +Judging is completely untouched by this module — Hackers' Choice is a +SEPARATE peer-vote award, not a judging round. + +Design notes: +- Each eligible voter (isSelected hacker) gets a deterministic slate of + `peer_vote_slate_size` submitted projects, never their own team, built once + and persisted on first GET (so re-opening the page always shows the same + slate). The seed is sha256(f"{event_id}:{propel_id}") so it's stable across + requests without needing to store anything before the first GET. +- Exposure counts (how many times each team has been shown) bias the slate + toward the least-shown projects, so a small early team isn't buried by a + few loud/popular ones. +- Scoring is the Wilson score interval LOWER bound (z=1.96) of + approvals/shown, not a raw approval rate — a team shown to 2 people who both + approved should not outrank a team shown to 40 people with a 90% approval + rate. No tallies are ever shown to voters, only to admins (get_results). +""" +import hashlib +import logging +import math +import random +from collections import Counter +from datetime import datetime, timezone + +from firebase_admin import firestore + +from db.db import get_db +from common.utils.firestore_helpers import clear_all_caches +# NOTE: common.utils.slack calls load_dotenv() at import time, which +# populates FIREBASE_CERT_CONFIG (among others) from .env into the process +# environment. common.utils.firebase reads that var at ITS OWN import time +# (module-level, no lazy fallback) — importing it before anything has called +# load_dotenv() raises a JSONDecodeError on a real deployment that relies on +# .env. Every existing service that imports both (e.g. api/mentors/ +# mentors_service.py) imports slack first for this reason; keep this order. +from common.utils.slack import send_slack_audit +from common.utils.firebase import get_hackathon_by_event_id +from common.utils.validators import normalize_deadline_iso +from services.teams_service import get_team + +logger = logging.getLogger("myapp") + +PEER_VOTES_COLLECTION = "peer_votes" +AWARD_NAME = "Hackers' Choice" +DEFAULT_SLATE_SIZE = 5 +DEFAULT_MAX_PICKS = 2 + +# Mirror of api.submissions.submissions_service.SUBMITTED_STATUSES — a team +# must have a real write-up before it can appear in anyone's slate or get +# voted on. Duplicated (not imported) to keep the two blueprints +# one-directionally independent; the values are the small, stable +# project_submission_status catalog. +SUBMITTED_STATUSES = {"submitted", "late"} + + +def clear_cache() -> None: + """Mirrors api/mentors/mentors_service.py's clear_cache(): bust every + registered cache (including services.teams_service._GET_TEAM_CACHE) plus + the hackathon event cache, since publish_results changes a team's + `awards` array that the event page also renders.""" + clear_all_caches() + try: + from services.hackathons_service import clear_cache as clear_hackathon_caches + clear_hackathon_caches() + except Exception as e: # pragma: no cover - best-effort cache bust + logger.warning("peer_votes clear_cache: hackathon cache clear failed: %s", e) + + +def _ballot_doc_id(event_id, propel_id): + safe_propel_id = (propel_id or "").replace("/", "_") + return f"{event_id}__{safe_propel_id}" + + +def _peer_vote_subdoc(db, hackathon_doc_id, name): + return db.collection("hackathons").document(hackathon_doc_id).collection("peer_vote").document(name) + + +def _settings(event): + """{"enabled", "slate_size", "max_picks", "requires_submission"} — reads + constraints.peer_vote_* off the hackathon doc with the Part 3 defaults. + MUST read peer_vote_enabled (a disabled event returns status:"disabled" + from every voter-facing route regardless of anything else).""" + constraints = (event or {}).get("constraints") or {} + return { + "enabled": bool(constraints.get("peer_vote_enabled", False)), + "slate_size": constraints.get("peer_vote_slate_size") or DEFAULT_SLATE_SIZE, + "max_picks": constraints.get("peer_vote_max_picks") or DEFAULT_MAX_PICKS, + "requires_submission": bool(constraints.get("peer_vote_requires_submission", False)), + } + + +def compute_voting_window(event, now=None): + """{"state": upcoming|open|closed, "opens_at", "closes_at"}. + + Defaults when unset: opens_at = deadlines.voting_opens, falling back to + late_submission_until, falling back to submission; closes_at = + deadlines.voting_closes, falling back to the event's end_date at + 23:59:59 in the event timezone. No usable opens_at/closes_at at all + (a brand new event with no deadlines configured) -> closed, never open. + """ + event = event or {} + tz_name = event.get("timezone") or "America/Phoenix" + now_dt = now or datetime.now(timezone.utc) + deadlines = event.get("deadlines") or {} + + opens_at = deadlines.get("voting_opens") or deadlines.get("late_submission_until") or deadlines.get("submission") + closes_at = deadlines.get("voting_closes") + if not closes_at: + end_date = event.get("end_date") + if end_date: + try: + closes_at = normalize_deadline_iso(f"{end_date}T23:59:59", tz_name) + except ValueError: + closes_at = None + + if not opens_at or not closes_at: + return {"state": "closed", "opens_at": opens_at, "closes_at": closes_at} + + opens_dt = datetime.fromisoformat(opens_at) + closes_dt = datetime.fromisoformat(closes_at) + if now_dt < opens_dt: + state = "upcoming" + elif now_dt <= closes_dt: + state = "open" + else: + state = "closed" + return {"state": state, "opens_at": opens_at, "closes_at": closes_at} + + +def _voter_eligibility(propel_id, event_id, event): + """(eligible, volunteer|None, reason|None). Eligible = an isSelected + hacker volunteer record for this event; when peer_vote_requires_submission + is on, the voter's own team must also have submitted.""" + from services.volunteers_service import find_volunteer_by_caller_identity + + volunteer = find_volunteer_by_caller_identity(propel_id, event_id, "hacker") + if not volunteer or not volunteer.get("isSelected"): + return False, None, "not_selected_hacker" + + settings = _settings(event) + if settings["requires_submission"]: + own_ids = _own_team_ids(propel_id, event_id) + submitted = False + if own_ids: + db = get_db() + refs = [db.collection("teams").document(tid) for tid in own_ids] + for snap in db.get_all(refs): + if snap.exists and (snap.to_dict() or {}).get("project_submission_status") in SUBMITTED_STATUSES: + submitted = True + break + if not submitted: + return False, volunteer, "own_team_not_submitted" + + return True, volunteer, None + + +def _submitted_teams_for_event(event_id): + """Active, submitted/late teams for the event. Single equality query + + Python filter (no composite index needed).""" + db = get_db() + docs = db.collection("teams").where("hackathon_event_id", "==", event_id).stream() + teams = [] + for doc in docs: + data = doc.to_dict() or {} + if data.get("active") is False: + continue + if data.get("project_submission_status") not in SUBMITTED_STATUSES: + continue + data["id"] = doc.id + teams.append(data) + return teams + + +def _own_team_ids(propel_id, event_id): + from api.teams.teams_service import get_my_teams_by_event_id + + result = get_my_teams_by_event_id(propel_id, event_id) or {} + return {t["id"] for t in result.get("teams", []) if t.get("id")} + + +def build_slate(candidates, exposure, event_id, propel_id, n): + """Deterministic per-voter shuffle (seeded on event_id+propel_id) then a + STABLE sort by current exposure ascending, so the least-shown candidates + win ties in the same order every time this voter is shown a slate. + `candidates` is a list of team dicts with an "id" key; returns a list of + team ids, capped at n.""" + seed = int(hashlib.sha256(f"{event_id}:{propel_id}".encode("utf-8")).hexdigest(), 16) + rng = random.Random(seed) + shuffled = list(candidates) + rng.shuffle(shuffled) + shuffled.sort(key=lambda c: exposure.get(c["id"], 0)) + return [c["id"] for c in shuffled[:n]] + + +def _in_transaction(db, body): + """Runs body(transaction) inside a real Firestore transaction. Tests + monkeypatch this directly (to `lambda db, body: body(FakeTx())`) since a + real @firestore.transactional callable needs a live Firestore client.""" + @firestore.transactional + def _run(transaction): + return body(transaction) + + return _run(db.transaction()) + + +def _tx_get_one(transaction, ref): + """First (only) snapshot from transaction.get(ref) — the real + google-cloud-firestore Transaction.get() returns a generator for a + DocumentReference (it delegates to client.get_all()).""" + return next(iter(transaction.get(ref)), None) + + +def _slate_team_view(team): + return { + "team_id": team.get("id"), + "name": team.get("name"), + "project_tagline": team.get("project_tagline"), + "project_thumbnail_url": team.get("project_thumbnail_url"), + "demo_video_url": team.get("demo_video_url"), + "github_links": team.get("github_links") or [], + "users_count": len(team.get("users") or []), + } + + +def _hydrate_slate(team_ids, candidates_by_id=None): + candidates_by_id = candidates_by_id or {} + views = [] + for tid in team_ids: + team = candidates_by_id.get(tid) + if team is None: + team = (get_team(tid) or {}).get("team") or {"id": tid} + views.append(_slate_team_view(team)) + return views + + +def _slate_response_from_ballot(ballot, window, settings, own_team_ids, candidates_by_id=None): + picks = ballot.get("picks") + status = "voted" if picks else window["state"] + return { + "status": status, + "opens_at": window["opens_at"], + "closes_at": window["closes_at"], + "max_picks": settings["max_picks"], + "slate": _hydrate_slate(ballot.get("slate") or [], candidates_by_id), + "picks": picks, + "own_team_ids": list(own_team_ids), + } + + +def get_slate(propel_id, event_id): + """GET /api/hackathons//peer-vote/slate. + + A ballot doc is persisted the FIRST time a voter's slate is materialized + (inside a transaction, re-checking existence to survive a double-click / + double-request race) and never rebuilt after that — re-opening the page + always shows the same slate, and exposure is only incremented once. + """ + event = get_hackathon_by_event_id(event_id) + settings = _settings(event or {}) + if not event or not settings["enabled"]: + return {"status": "disabled"} + + window = compute_voting_window(event) + eligible, _volunteer, reason = _voter_eligibility(propel_id, event_id, event) + if not eligible: + return {"status": "not_eligible", "reason": reason, "opens_at": window["opens_at"], "closes_at": window["closes_at"], "max_picks": settings["max_picks"]} + + db = get_db() + event_doc_id = event.get("id") or event_id + ballot_ref = db.collection(PEER_VOTES_COLLECTION).document(_ballot_doc_id(event_id, propel_id)) + own_team_ids = _own_team_ids(propel_id, event_id) + + existing = ballot_ref.get() + if existing.exists: + return _slate_response_from_ballot(existing.to_dict() or {}, window, settings, own_team_ids) + + if window["state"] == "upcoming": + return {"status": "upcoming", "opens_at": window["opens_at"], "closes_at": window["closes_at"], "max_picks": settings["max_picks"], "own_team_ids": list(own_team_ids)} + if window["state"] == "closed": + return {"status": "closed", "opens_at": window["opens_at"], "closes_at": window["closes_at"], "max_picks": settings["max_picks"], "own_team_ids": list(own_team_ids)} + + candidates = [t for t in _submitted_teams_for_event(event_id) if t["id"] not in own_team_ids] + if len(candidates) < 2: + return { + "status": "open", + "opens_at": window["opens_at"], + "closes_at": window["closes_at"], + "max_picks": settings["max_picks"], + "slate": [], + "own_team_ids": list(own_team_ids), + "reason": "not_enough_submissions", + } + + candidates_by_id = {c["id"]: c for c in candidates} + exposure_ref = _peer_vote_subdoc(db, event_doc_id, "exposure") + + def _persist(transaction): + tx_existing = _tx_get_one(transaction, ballot_ref) + if tx_existing is not None and tx_existing.exists: + return tx_existing.to_dict() or {} + + exposure_snap = _tx_get_one(transaction, exposure_ref) + exposure = (exposure_snap.to_dict() or {}).get("counts", {}) if exposure_snap is not None and exposure_snap.exists else {} + slate_ids = build_slate(candidates, exposure, event_id, propel_id, settings["slate_size"]) + now_iso = datetime.now(timezone.utc).isoformat() + ballot_data = { + "event_id": event_id, + "voter_propel_id": propel_id, + "slate": slate_ids, + "shown_at": now_iso, + "picks": None, + "voted_at": None, + "created_at": now_iso, + "updated_at": now_iso, + "voided": False, + } + transaction.set(ballot_ref, ballot_data) + transaction.set(exposure_ref, {"counts": {tid: firestore.Increment(1) for tid in slate_ids}}, merge=True) + return ballot_data + + ballot_data = _in_transaction(db, _persist) + return _slate_response_from_ballot(ballot_data, window, settings, own_team_ids, candidates_by_id=candidates_by_id) + + +def submit_ballot(propel_id, event_id, picks): + """POST /api/hackathons//peer-vote/ballot. Re-votable until + close — a full-doc set() replaces `picks`, keeping the original + `voted_at` (first vote only).""" + event = get_hackathon_by_event_id(event_id) + settings = _settings(event or {}) + if not event or not settings["enabled"]: + return {"error": "peer_vote_disabled"}, 403 + + window = compute_voting_window(event) + eligible, _volunteer, reason = _voter_eligibility(propel_id, event_id, event) + if not eligible: + return {"error": "not_eligible", "reason": reason}, 403 + + db = get_db() + ballot_ref = db.collection(PEER_VOTES_COLLECTION).document(_ballot_doc_id(event_id, propel_id)) + snap = ballot_ref.get() + if not snap.exists: + return {"error": "no_slate"}, 400 + ballot = snap.to_dict() or {} + + if ballot.get("voided"): + return {"error": "ballot_voided"}, 409 + if window["state"] == "closed": + return {"error": "voting_closed"}, 409 + + slate = ballot.get("slate") or [] + max_picks = settings["max_picks"] + if ( + not isinstance(picks, list) + or not picks + or len(picks) > max_picks + or len(set(picks)) != len(picks) + or any(p not in slate for p in picks) + ): + return {"error": "invalid_picks"}, 400 + + now_iso = datetime.now(timezone.utc).isoformat() + update = {"picks": picks, "updated_at": now_iso} + if not ballot.get("voted_at"): + update["voted_at"] = now_iso + ballot_ref.set(update, merge=True) + + send_slack_audit( + action="peer_vote_ballot", + message=f"Hackers' Choice ballot recorded for event {event_id}", + payload={"event_id": event_id, "picks": picks}, + ) + return {"success": True, "picks": picks}, 200 + + +def wilson_lower_bound(approvals, shown, z=1.96): + """Wilson score interval LOWER bound for a binomial proportion — + approvals/shown, penalized for a small sample. wilson_lower_bound(0,0)==0; + (5,5)≈0.566; (1,1)≈0.207.""" + if not shown: + return 0.0 + n = float(shown) + p = approvals / n + denom = 1 + (z * z) / n + center = p + (z * z) / (2 * n) + margin = z * math.sqrt((p * (1 - p) + (z * z) / (4 * n)) / n) + return (center - margin) / denom + + +def compute_results(ballots, teams_by_id, exposure): + """Pure. Ranks by Wilson lower bound desc, then raw approvals desc, then + name — so a tie only ever breaks toward the more-approved, then + alphabetically (stable, no hidden randomness in the admin view).""" + approvals = Counter() + for ballot in ballots: + if ballot.get("voided"): + continue + for team_id in (ballot.get("picks") or []): + approvals[team_id] += 1 + + results = [] + for team_id, team in teams_by_id.items(): + shown = exposure.get(team_id, 0) + approved = approvals.get(team_id, 0) + results.append({ + "team_id": team_id, + "name": team.get("name"), + "shown": shown, + "exposure_shown": shown, + "approvals": approved, + "approval_rate": (approved / shown) if shown else 0.0, + "wilson_lower_bound": wilson_lower_bound(approved, shown), + }) + + results.sort(key=lambda r: (-r["wilson_lower_bound"], -r["approvals"], r["name"] or "")) + for i, r in enumerate(results, start=1): + r["rank"] = i + return results + + +def get_results(event_id): + """GET /api/hackathons//peer-vote/results (admin).""" + event = get_hackathon_by_event_id(event_id) + if not event: + return {"error": "Event not found"}, 404 + + settings = _settings(event) + window = compute_voting_window(event) + event_doc_id = event.get("id") or event_id + + db = get_db() + all_ballots = [d.to_dict() or {} for d in db.collection(PEER_VOTES_COLLECTION).where("event_id", "==", event_id).stream()] + voided_ballots = [b for b in all_ballots if b.get("voided")] + active_ballots = [b for b in all_ballots if not b.get("voided")] + + exposure_snap = _peer_vote_subdoc(db, event_doc_id, "exposure").get() + exposure = (exposure_snap.to_dict() or {}).get("counts", {}) if exposure_snap.exists else {} + + teams_by_id = {t["id"]: t for t in _submitted_teams_for_event(event_id)} + for team_id in exposure.keys(): + if team_id not in teams_by_id: + teams_by_id[team_id] = (get_team(team_id) or {}).get("team") or {"id": team_id} + + results = compute_results(active_ballots, teams_by_id, exposure) + + from services.volunteers_service import get_all_hackers_by_event_id + eligible_estimate = sum(1 for h in get_all_hackers_by_event_id(event_id) if h.get("isSelected")) + + summary_exists = _peer_vote_subdoc(db, event_doc_id, "summary").get().exists + + return { + "ballots": len(active_ballots), + "voided": len(voided_ballots), + "eligible_estimate": eligible_estimate, + "window": window, + "settings": settings, + "published": summary_exists, + "teams": results, + }, 200 + + +def void_ballot(event_id, voter_propel_id, actor): + """POST /api/hackathons//peer-vote/ballots//void (admin).""" + db = get_db() + ref = db.collection(PEER_VOTES_COLLECTION).document(_ballot_doc_id(event_id, voter_propel_id)) + snap = ref.get() + if not snap.exists: + return {"error": "Ballot not found"}, 404 + + now_iso = datetime.now(timezone.utc).isoformat() + ref.set({"voided": True, "voided_at": now_iso, "voided_by": actor}, merge=True) + send_slack_audit( + action="peer_vote_void", + message=f"Hackers' Choice ballot voided for event {event_id}", + payload={"event_id": event_id, "voter_propel_id": voter_propel_id, "by": actor}, + ) + return {"success": True}, 200 + + +def publish_results(event_id, actor, team_id=None): + """POST /api/hackathons//peer-vote/publish (admin). Idempotent: + re-publishing the same winner never appends AWARD_NAME twice. `team_id` + lets an admin override the computed rank-1 winner (e.g. a tie broken by + judgment); defaults to rank 1.""" + event = get_hackathon_by_event_id(event_id) + if not event: + return {"error": "Event not found"}, 404 + + results_payload, status = get_results(event_id) + if status != 200: + return results_payload, status + + ranked = results_payload.get("teams") or [] + if not ranked: + return {"error": "no_ballots"}, 409 + + winner_id = team_id or ranked[0]["team_id"] + winner = next((t for t in ranked if t["team_id"] == winner_id), None) + if winner is None: + return {"error": "Winning team not found in results"}, 400 + + db = get_db() + team_ref = db.collection("teams").document(winner_id) + team_snap = team_ref.get() + team_data = (team_snap.to_dict() or {}) if team_snap.exists else {} + awards = list(team_data.get("awards") or []) + if AWARD_NAME not in awards: + awards.append(AWARD_NAME) + team_ref.set({"awards": awards}, merge=True) + + now_iso = datetime.now(timezone.utc).isoformat() + winner_name = winner.get("name") or team_data.get("name") + _peer_vote_subdoc(db, event.get("id") or event_id, "summary").set({ + "winner_team_id": winner_id, + "winner_team_name": winner_name, + "published_at": now_iso, + "published_by": actor, + "ballots": results_payload.get("ballots", 0), + }) + + send_slack_audit( + action="peer_vote_publish", + message=f"Hackers' Choice published for event {event_id}: {winner_name} ({winner_id})", + payload={"event_id": event_id, "winner_team_id": winner_id}, + ) + clear_cache() + + return {"success": True, "winner_team_id": winner_id, "winner_team_name": winner_name}, 200 + + +def get_public_summary(event_id): + """GET /api/hackathons//peer-vote/summary (public).""" + event = get_hackathon_by_event_id(event_id) + if not event: + return {"published": False}, 200 + + db = get_db() + summary_snap = _peer_vote_subdoc(db, event.get("id") or event_id, "summary").get() + if not summary_snap.exists: + return {"published": False}, 200 + + data = summary_snap.to_dict() or {} + return { + "published": True, + "winner_team_id": data.get("winner_team_id"), + "winner_team_name": data.get("winner_team_name"), + "published_at": data.get("published_at"), + "ballots": data.get("ballots"), + }, 200 diff --git a/api/peer_votes/peer_votes_views.py b/api/peer_votes/peer_votes_views.py new file mode 100644 index 0000000..458b122 --- /dev/null +++ b/api/peer_votes/peer_votes_views.py @@ -0,0 +1,86 @@ +""" +Flask routes for Hackers' Choice (the peer-vote award). Voter-facing routes +require login only (eligibility — isSelected hacker — is enforced in the +service); admin routes additionally check is_admin(auth_user). The public +summary route needs no auth at all. +""" +import logging +from flask import Blueprint, request + +from common.auth import auth, auth_user +from services.hackathon_planning_service import is_admin +from api.peer_votes.peer_votes_service import ( + get_slate, + submit_ballot, + get_results, + void_ballot, + publish_results, + get_public_summary, +) + +logger = logging.getLogger(__name__) +logger.setLevel(logging.INFO) + +bp_name = "api-peer-votes" +bp = Blueprint(bp_name, __name__, url_prefix="/api/hackathons") + + +def _unauthorized(): + return {"error": "Unauthorized"}, 401 + + +def _forbidden(): + return {"error": "Forbidden"}, 403 + + +@bp.route("//peer-vote/slate", methods=["GET"]) +@auth.require_user +def get_slate_api(event_id): + if not (auth_user and auth_user.user_id): + return _unauthorized() + return get_slate(auth_user.user_id, event_id) + + +@bp.route("//peer-vote/ballot", methods=["POST"]) +@auth.require_user +def submit_ballot_api(event_id): + if not (auth_user and auth_user.user_id): + return _unauthorized() + body = request.get_json() or {} + return submit_ballot(auth_user.user_id, event_id, body.get("picks")) + + +@bp.route("//peer-vote/results", methods=["GET"]) +@auth.require_user +def get_results_api(event_id): + if not (auth_user and auth_user.user_id): + return _unauthorized() + if not is_admin(auth_user): + return _forbidden() + return get_results(event_id) + + +@bp.route("//peer-vote/ballots//void", methods=["POST"]) +@auth.require_user +def void_ballot_api(event_id, propel_id): + if not (auth_user and auth_user.user_id): + return _unauthorized() + if not is_admin(auth_user): + return _forbidden() + return void_ballot(event_id, propel_id, auth_user.user_id) + + +@bp.route("//peer-vote/publish", methods=["POST"]) +@auth.require_user +def publish_results_api(event_id): + if not (auth_user and auth_user.user_id): + return _unauthorized() + if not is_admin(auth_user): + return _forbidden() + body = request.get_json() or {} + return publish_results(event_id, auth_user.user_id, team_id=body.get("team_id")) + + +@bp.route("//peer-vote/summary", methods=["GET"]) +def get_summary_api(event_id): + return get_public_summary(event_id) diff --git a/api/peer_votes/tests/__init__.py b/api/peer_votes/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/api/peer_votes/tests/test_peer_votes_service.py b/api/peer_votes/tests/test_peer_votes_service.py new file mode 100644 index 0000000..dd16497 --- /dev/null +++ b/api/peer_votes/tests/test_peer_votes_service.py @@ -0,0 +1,538 @@ +""" +Unit tests for api.peer_votes.peer_votes_service. + +Uses a tiny in-memory fake Firestore (tuple-keyed FakeDb, supporting +collection/document/where/stream/get_all + a FakeTransaction that +_in_transaction is monkeypatched to use) so multi-step flows (slate +persistence, exposure increments, publish) are visible across calls within +one test, the same way real Firestore would behave. +""" +import os +from datetime import datetime, timedelta, timezone + +import pytest +from google.cloud.firestore_v1.transforms import Increment + +os.environ.setdefault("ENVIRONMENT", "test") + +import api.peer_votes.peer_votes_service as svc + + +# --------------------------------------------------------------------------- +# Fake Firestore +# --------------------------------------------------------------------------- + +def _resolve_value(existing_value, new_value): + if isinstance(new_value, Increment): + base = existing_value if isinstance(existing_value, (int, float)) else 0 + return base + new_value.value + if isinstance(new_value, dict): + base_dict = existing_value if isinstance(existing_value, dict) else {} + merged = dict(base_dict) + for k, v in new_value.items(): + merged[k] = _resolve_value(base_dict.get(k), v) + return merged + return new_value + + +class FakeSnapshot: + 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 FakeDocRef: + def __init__(self, store, key): + self._store = store + self._key = key + + def get(self): + return FakeSnapshot(self._key[-1], self._store.get(self._key)) + + def set(self, data, merge=False): + if merge and self._key in self._store and self._store[self._key] is not None: + existing = self._store[self._key] + for k, v in data.items(): + existing[k] = _resolve_value(existing.get(k), v) + else: + resolved = {} + for k, v in data.items(): + resolved[k] = _resolve_value(None, v) + self._store[self._key] = resolved + + def collection(self, name): + return FakeCollection(self._store, self._key + (name,)) + + +class FakeQuery: + def __init__(self, store, path, filters): + self._store = store + self._path = path + self._filters = filters + + def where(self, field, op, value): + return FakeQuery(self._store, self._path, self._filters + [(field, op, value)]) + + def stream(self): + prefix_len = len(self._path) + 1 + results = [] + for key, data in list(self._store.items()): + if data is None or len(key) != prefix_len or key[:-1] != self._path: + continue + if all(data.get(field) == value for field, op, value in self._filters): + results.append(FakeSnapshot(key[-1], data)) + return results + + +class FakeCollection: + def __init__(self, store, path): + self._store = store + self._path = path + + def document(self, doc_id): + return FakeDocRef(self._store, self._path + (doc_id,)) + + def where(self, field, op, value): + return FakeQuery(self._store, self._path, [(field, op, value)]) + + +class FakeDb: + def __init__(self, store): + self._store = store + + def collection(self, name): + return FakeCollection(self._store, (name,)) + + def get_all(self, refs): + return [ref.get() for ref in refs] + + +class FakeTransaction: + def __init__(self, store): + self._store = store + + def get(self, ref): + return iter([ref.get()]) + + def set(self, ref, data, merge=False): + ref.set(data, merge=merge) + + +@pytest.fixture +def store(): + return {} + + +@pytest.fixture +def wire(monkeypatch, store): + monkeypatch.setattr(svc, "get_db", lambda: FakeDb(store)) + monkeypatch.setattr(svc, "_in_transaction", lambda db, body: body(FakeTransaction(store))) + monkeypatch.setattr(svc, "clear_cache", lambda: None) + monkeypatch.setattr(svc, "send_slack_audit", lambda **kwargs: None) + + def fake_get_team(team_id): + data = store.get(("teams", team_id)) + if data is None: + return {} + return {"team": {**data, "id": team_id}} + + monkeypatch.setattr(svc, "get_team", fake_get_team) + return store + + +def _seed_hackathon(event_id="event-1", doc_id="evtdoc-1", **extra): + return {"id": doc_id, "event_id": event_id, "timezone": "UTC", "deadlines": {}, "constraints": {}, "end_date": "2026-10-11", **extra} + + +# Window relative to "now" so these tests don't rot as the calendar moves — +# always currently open, regardless of when the suite runs. +_NOW = datetime.now(timezone.utc) +OPEN_WINDOW = { + "voting_opens": (_NOW - timedelta(days=1)).isoformat(), + "voting_closes": (_NOW + timedelta(days=1)).isoformat(), +} + + +def _seed_team(store, team_id, event_id="event-1", **extra): + store[("teams", team_id)] = {"hackathon_event_id": event_id, "name": team_id, **extra} + + +def _eligible(monkeypatch, isSelected=True): + monkeypatch.setattr( + "services.volunteers_service.find_volunteer_by_caller_identity", + lambda propel, event_id, vtype: {"name": "Hacker", "isSelected": isSelected}, + ) + + +def _own_teams(monkeypatch, team_ids): + monkeypatch.setattr( + "api.teams.teams_service.get_my_teams_by_event_id", + lambda propel, event_id: {"teams": [{"id": tid} for tid in team_ids]}, + ) + + +# --------------------------------------------------------------------------- +# wilson_lower_bound — spec values +# --------------------------------------------------------------------------- + +def test_wilson_lower_bound_zero_shown_is_zero(): + assert svc.wilson_lower_bound(0, 0) == 0.0 + + +def test_wilson_lower_bound_five_of_five(): + assert svc.wilson_lower_bound(5, 5) == pytest.approx(0.566, abs=0.001) + + +def test_wilson_lower_bound_one_of_one(): + assert svc.wilson_lower_bound(1, 1) == pytest.approx(0.207, abs=0.001) + + +# --------------------------------------------------------------------------- +# build_slate — exposure balance + determinism +# --------------------------------------------------------------------------- + +def test_build_slate_prefers_least_exposed(): + candidates = [{"id": f"t{i}"} for i in range(5)] + exposure = {"t0": 10, "t1": 0, "t2": 5, "t3": 0, "t4": 8} + slate = svc.build_slate(candidates, exposure, "event-1", "propel-1", 2) + assert set(slate) == {"t1", "t3"} + + +def test_build_slate_deterministic_for_same_voter(): + candidates = [{"id": f"t{i}"} for i in range(6)] + a = svc.build_slate(candidates, {}, "event-1", "propel-1", 3) + b = svc.build_slate(candidates, {}, "event-1", "propel-1", 3) + assert a == b + + +def test_build_slate_caps_at_n(): + candidates = [{"id": f"t{i}"} for i in range(10)] + slate = svc.build_slate(candidates, {}, "event-1", "propel-1", 4) + assert len(slate) == 4 + + +# --------------------------------------------------------------------------- +# get_slate — disabled/eligibility/window/own-team-exclusion/persistence +# --------------------------------------------------------------------------- + +def test_get_slate_disabled_when_constraint_unset(wire, monkeypatch): + event = _seed_hackathon(event_id="event-1") + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + result = svc.get_slate("propel-1", "event-1") + assert result == {"status": "disabled"} + + +def test_get_slate_disabled_when_event_missing(wire, monkeypatch): + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: None) + result = svc.get_slate("propel-1", "event-1") + assert result == {"status": "disabled"} + + +def test_get_slate_not_eligible_when_not_selected_hacker(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True}, deadlines=OPEN_WINDOW) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch, isSelected=False) + result = svc.get_slate("propel-1", "event-1") + assert result["status"] == "not_eligible" + + +def test_get_slate_excludes_own_team_and_unsubmitted_and_inactive(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True, "peer_vote_slate_size": 5}, deadlines=OPEN_WINDOW) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _own_teams(monkeypatch, ["own-team"]) + + _seed_team(wire, "own-team", project_submission_status="submitted") + _seed_team(wire, "candidate-a", project_submission_status="submitted") + _seed_team(wire, "candidate-b", project_submission_status="submitted") + _seed_team(wire, "draft-team", project_submission_status="draft") + _seed_team(wire, "inactive-team", project_submission_status="submitted", active=False) + + result = svc.get_slate("propel-1", "event-1") + slate_ids = {t["team_id"] for t in result["slate"]} + assert "own-team" not in slate_ids + assert "draft-team" not in slate_ids + assert "inactive-team" not in slate_ids + assert slate_ids == {"candidate-a", "candidate-b"} + + +def test_get_slate_not_enough_submissions_when_fewer_than_two_candidates(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True}, deadlines=OPEN_WINDOW) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _own_teams(monkeypatch, []) + _seed_team(wire, "only-team", project_submission_status="submitted") + + result = svc.get_slate("propel-1", "event-1") + assert result["status"] == "open" + assert result["slate"] == [] + assert result["reason"] == "not_enough_submissions" + # nothing should have been persisted + assert ("peer_votes", "event-1__propel-1") not in wire + + +def test_get_slate_second_call_returns_identical_slate_and_increments_exposure_once(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True, "peer_vote_slate_size": 2}, deadlines=OPEN_WINDOW) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _own_teams(monkeypatch, []) + for i in range(4): + _seed_team(wire, f"team-{i}", project_submission_status="submitted") + + first = svc.get_slate("propel-1", "event-1") + second = svc.get_slate("propel-1", "event-1") + + assert first["slate"] == second["slate"] + exposure = wire[("hackathons", "evtdoc-1", "peer_vote", "exposure")]["counts"] + for team in first["slate"]: + assert exposure[team["team_id"]] == 1 + + +def test_get_slate_upcoming_and_closed_states(wire, monkeypatch): + _eligible(monkeypatch) + _own_teams(monkeypatch, []) + + upcoming_event = _seed_hackathon(constraints={"peer_vote_enabled": True}, deadlines={ + "voting_opens": "2099-01-01T00:00:00+00:00", "voting_closes": "2099-01-02T00:00:00+00:00", + }) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: upcoming_event) + assert svc.get_slate("propel-1", "event-1")["status"] == "upcoming" + + closed_event = _seed_hackathon(constraints={"peer_vote_enabled": True}, deadlines={ + "voting_opens": "2020-01-01T00:00:00+00:00", "voting_closes": "2020-01-02T00:00:00+00:00", + }) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: closed_event) + assert svc.get_slate("propel-1", "event-1")["status"] == "closed" + + +# --------------------------------------------------------------------------- +# submit_ballot — the pick validation matrix +# --------------------------------------------------------------------------- + +def _seed_ballot(store, event_id, propel_id, slate, picks=None, voided=False): + store[("peer_votes", f"{event_id}__{propel_id}")] = { + "event_id": event_id, "voter_propel_id": propel_id, "slate": slate, + "picks": picks, "voted_at": None, "voided": voided, + } + + +def _open_event(**overrides): + base = dict(constraints={"peer_vote_enabled": True, "peer_vote_max_picks": 2}, deadlines={ + "voting_opens": "2020-01-01T00:00:00+00:00", "voting_closes": "2099-01-01T00:00:00+00:00", + }) + base.update(overrides) + return _seed_hackathon(**base) + + +def test_submit_ballot_no_slate_yet(wire, monkeypatch): + event = _open_event() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + result, status = svc.submit_ballot("propel-1", "event-1", ["t1"]) + assert status == 400 + assert result["error"] == "no_slate" + + +def test_submit_ballot_rejects_pick_not_in_slate(wire, monkeypatch): + event = _open_event() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _seed_ballot(wire, "event-1", "propel-1", slate=["t1", "t2", "t3"]) + result, status = svc.submit_ballot("propel-1", "event-1", ["t1", "t9"]) + assert status == 400 + assert result["error"] == "invalid_picks" + + +def test_submit_ballot_rejects_too_many_picks(wire, monkeypatch): + event = _open_event() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _seed_ballot(wire, "event-1", "propel-1", slate=["t1", "t2", "t3"]) + result, status = svc.submit_ballot("propel-1", "event-1", ["t1", "t2", "t3"]) + assert status == 400 + + +def test_submit_ballot_rejects_empty_picks(wire, monkeypatch): + event = _open_event() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _seed_ballot(wire, "event-1", "propel-1", slate=["t1", "t2"]) + result, status = svc.submit_ballot("propel-1", "event-1", []) + assert status == 400 + + +def test_submit_ballot_rejects_duplicate_picks(wire, monkeypatch): + event = _open_event() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _seed_ballot(wire, "event-1", "propel-1", slate=["t1", "t2"]) + result, status = svc.submit_ballot("propel-1", "event-1", ["t1", "t1"]) + assert status == 400 + + +def test_submit_ballot_accepts_valid_picks(wire, monkeypatch): + event = _open_event() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _seed_ballot(wire, "event-1", "propel-1", slate=["t1", "t2", "t3"]) + result, status = svc.submit_ballot("propel-1", "event-1", ["t1", "t3"]) + assert status == 200 + assert wire[("peer_votes", "event-1__propel-1")]["picks"] == ["t1", "t3"] + + +def test_submit_ballot_409_when_voided(wire, monkeypatch): + event = _open_event() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _seed_ballot(wire, "event-1", "propel-1", slate=["t1", "t2"], voided=True) + result, status = svc.submit_ballot("propel-1", "event-1", ["t1"]) + assert status == 409 + assert result["error"] == "ballot_voided" + + +def test_submit_ballot_409_when_closed(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True}, deadlines={ + "voting_opens": "2020-01-01T00:00:00+00:00", "voting_closes": "2020-01-02T00:00:00+00:00", + }) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _seed_ballot(wire, "event-1", "propel-1", slate=["t1", "t2"]) + result, status = svc.submit_ballot("propel-1", "event-1", ["t1"]) + assert status == 409 + assert result["error"] == "voting_closed" + + +def test_submit_ballot_403_when_disabled(wire, monkeypatch): + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: _seed_hackathon()) + _eligible(monkeypatch) + result, status = svc.submit_ballot("propel-1", "event-1", ["t1"]) + assert status == 403 + assert result["error"] == "peer_vote_disabled" + + +def test_submit_ballot_re_vote_keeps_original_voted_at(wire, monkeypatch): + event = _open_event() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + wire[("peer_votes", "event-1__propel-1")] = { + "event_id": "event-1", "slate": ["t1", "t2"], "picks": ["t1"], + "voted_at": "2026-01-01T00:00:00+00:00", "voided": False, + } + svc.submit_ballot("propel-1", "event-1", ["t2"]) + assert wire[("peer_votes", "event-1__propel-1")]["voted_at"] == "2026-01-01T00:00:00+00:00" + assert wire[("peer_votes", "event-1__propel-1")]["picks"] == ["t2"] + + +# --------------------------------------------------------------------------- +# compute_results / get_results — voided exclusion +# --------------------------------------------------------------------------- + +def test_compute_results_excludes_voided_ballots(): + ballots = [ + {"picks": ["t1"], "voided": False}, + {"picks": ["t1", "t2"], "voided": True}, # excluded entirely + ] + teams_by_id = {"t1": {"name": "Team One"}, "t2": {"name": "Team Two"}} + exposure = {"t1": 2, "t2": 2} + results = svc.compute_results(ballots, teams_by_id, exposure) + by_id = {r["team_id"]: r for r in results} + assert by_id["t1"]["approvals"] == 1 + assert by_id["t2"]["approvals"] == 0 + + +def test_get_results_reports_voided_count_separately(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True}) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + monkeypatch.setattr("services.volunteers_service.get_all_hackers_by_event_id", lambda eid: []) + _seed_team(wire, "t1", project_submission_status="submitted") + wire[("peer_votes", "event-1__u1")] = {"event_id": "event-1", "picks": ["t1"], "voided": False} + wire[("peer_votes", "event-1__u2")] = {"event_id": "event-1", "picks": ["t1"], "voided": True} + + result, status = svc.get_results("event-1") + assert status == 200 + assert result["ballots"] == 1 + assert result["voided"] == 1 + + +# --------------------------------------------------------------------------- +# publish_results — idempotent +# --------------------------------------------------------------------------- + +def test_publish_results_appends_award_once(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True}) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + monkeypatch.setattr("services.volunteers_service.get_all_hackers_by_event_id", lambda eid: []) + _seed_team(wire, "t1", project_submission_status="submitted") + wire[("peer_votes", "event-1__u1")] = {"event_id": "event-1", "picks": ["t1"], "voided": False} + + result1, status1 = svc.publish_results("event-1", "admin-1") + result2, status2 = svc.publish_results("event-1", "admin-1") + + assert status1 == status2 == 200 + assert result1["winner_team_id"] == "t1" + assert wire[("teams", "t1")]["awards"].count("Hackers' Choice") == 1 + + +def test_publish_results_409_with_no_ballots(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True}) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + monkeypatch.setattr("services.volunteers_service.get_all_hackers_by_event_id", lambda eid: []) + result, status = svc.publish_results("event-1", "admin-1") + assert status == 409 + assert result["error"] == "no_ballots" + + +def test_void_ballot_excludes_from_future_results(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True}) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + monkeypatch.setattr("services.volunteers_service.get_all_hackers_by_event_id", lambda eid: []) + _seed_team(wire, "t1", project_submission_status="submitted") + wire[("peer_votes", "event-1__u1")] = {"event_id": "event-1", "picks": ["t1"], "voided": False} + + void_result, void_status = svc.void_ballot("event-1", "u1", "admin-1") + assert void_status == 200 + + results, status = svc.get_results("event-1") + assert results["voided"] == 1 + assert results["ballots"] == 0 + + +def test_void_ballot_404_for_unknown_voter(wire, monkeypatch): + result, status = svc.void_ballot("event-1", "nope", "admin-1") + assert status == 404 + + +# --------------------------------------------------------------------------- +# get_public_summary — gating +# --------------------------------------------------------------------------- + +def test_get_public_summary_unpublished_by_default(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True}) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + result, status = svc.get_public_summary("event-1") + assert status == 200 + assert result == {"published": False} + + +def test_get_public_summary_after_publish(wire, monkeypatch): + event = _seed_hackathon(constraints={"peer_vote_enabled": True}) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + monkeypatch.setattr("services.volunteers_service.get_all_hackers_by_event_id", lambda eid: []) + _seed_team(wire, "t1", project_submission_status="submitted") + wire[("peer_votes", "event-1__u1")] = {"event_id": "event-1", "picks": ["t1"], "voided": False} + svc.publish_results("event-1", "admin-1") + + result, status = svc.get_public_summary("event-1") + assert status == 200 + assert result["published"] is True + assert result["winner_team_id"] == "t1" + + +def test_get_public_summary_unknown_event(monkeypatch): + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: None) + result, status = svc.get_public_summary("nope") + assert result == {"published": False} From 156ec260cc85802256680cb8bef9eb0b2ed1ad32 Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:19:40 -0700 Subject: [PATCH 07/11] Judging: surface team demo video to judges; fix 2 pre-existing bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit No rubric/scoring/round/results logic changes. get_team_details and format_team_for_judge now return demo_video_url (the field a team's dashboard actually writes) and fill the legacy video_url key from it, so judge pages can finally show a team's demo video next to the existing GitHub/Devpost links. Bug fixes found and fixed along the way (Part 9 #3, #4): - get_bulk_judge_details always returned an empty judges list — it called an undefined name, fetch_judge_scores_by_event (the correctly imported fetch_judge_scores_by_event_id was never used), which NameError'd straight into the function's own blanket except. - 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) instead of by the assignment's own id. Added fetch_judge_assignment_by_id (direct doc-get) to db/firestore.py + db/db.py and used that instead. Tests: api/judging/tests/test_format_team.py. Part of the team-dashboard-devpost-replacement plan, WS-A task 9. Co-Authored-By: Claude Sonnet 5 --- api/judging/judging_service.py | 43 +++++--- api/judging/tests/__init__.py | 0 api/judging/tests/test_format_team.py | 150 ++++++++++++++++++++++++++ db/db.py | 3 + db/firestore.py | 13 +++ 5 files changed, 192 insertions(+), 17 deletions(-) create mode 100644 api/judging/tests/__init__.py create mode 100644 api/judging/tests/test_format_team.py diff --git a/api/judging/judging_service.py b/api/judging/judging_service.py index c95f9c5..12125aa 100644 --- a/api/judging/judging_service.py +++ b/api/judging/judging_service.py @@ -4,6 +4,7 @@ from common.log import get_logger, debug, warning, error from db.db import ( fetch_judge_assignments_by_judge_id, + fetch_judge_assignment_by_id, fetch_judge_assignments_by_event_and_judge, fetch_judge_scores_by_judge_and_event, fetch_judge_score, @@ -257,12 +258,15 @@ def get_team_details(team_id: str) -> Dict: "members": members, # Now populated with actual member data "github_url": github_url, "devpost_url": team_data.get('devpost_link', ''), - "slack_channel": team_data.get('slack_channel', ''), - - - - # Not used - "video_url": team_data.get('video_url', ''), + "slack_channel": team_data.get('slack_channel', ''), + # demo_video_url is the real field written by the team dashboard's + # DemoVideoEditor / TeamStatusPanel; video_url is filled from it + # too so judge-side consumers reading either name keep working + # (Part 9 bug #2 — judges never received the team's demo video + # because this used to read a phantom `video_url` key that + # save/edit_team never writes). + "demo_video_url": team_data.get('demo_video_url', ''), + "video_url": team_data.get('demo_video_url') or team_data.get('video_url', ''), "demo_url": team_data.get('demo_url', ''), "technologies": team_data.get('technologies', []), "features": team_data.get('features', []) @@ -507,7 +511,10 @@ def format_team_for_judge(team: Dict, score_lookup: Dict = None, nonprofit_id: s "github_url": github_url, "devpost_url": team.get('devpost_link', ''), "slack_channel": team.get('slack_channel', ''), - "video_url": team.get('video_url', ''), + # See get_team_details' matching comment (Part 9 bug #2): demo_video_url + # is the real field; video_url is filled from it for legacy readers. + "demo_video_url": team.get('demo_video_url', ''), + "video_url": team.get('demo_video_url') or team.get('video_url', ''), "demo_time": None, # Will be overridden for round2 "judged": score_obj is not None, "score": score_obj.total_score if score_obj else None, @@ -579,14 +586,12 @@ def update_judge_assignment_details(assignment_id: str, demo_time: str = None, try: debug(logger, "Updating judge assignment", assignment_id=assignment_id) - # First fetch the existing assignment - # This is inefficient but works with current db interface - assignments = fetch_judge_assignments_by_judge_id("") - assignment = None - for a in assignments: - if a.id == assignment_id: - assignment = a - break + # First fetch the existing assignment. (Part 9 bug #4: this used to + # call fetch_judge_assignments_by_judge_id("") — an empty judge_id + # matches no real assignment — so `assignment` was always None and + # every call 400'd "Assignment not found". fetch_judge_assignment_by_id + # is a direct doc-get.) + assignment = fetch_judge_assignment_by_id(assignment_id) if not assignment: return {"success": False, "error": "Assignment not found"} @@ -954,8 +959,12 @@ def get_bulk_judge_details(event_id: str) -> Dict: # Get all assignments for the event assignments = fetch_judge_assignments_by_event_id(event_id) - # Get all of the scores for the event - scores = fetch_judge_scores_by_event(event_id) + # Get all of the scores for the event. (Part 9 bug #3: this called an + # undefined name, fetch_judge_scores_by_event — the correctly-named + # fetch_judge_scores_by_event_id was imported but never used here — + # so every call silently NameError'd into the outer except and this + # function always returned an empty judges list.) + scores = fetch_judge_scores_by_event_id(event_id) if "error" in judges_result: diff --git a/api/judging/tests/__init__.py b/api/judging/tests/__init__.py new file mode 100644 index 0000000..e69de29 diff --git a/api/judging/tests/test_format_team.py b/api/judging/tests/test_format_team.py new file mode 100644 index 0000000..d9d70fa --- /dev/null +++ b/api/judging/tests/test_format_team.py @@ -0,0 +1,150 @@ +""" +Part 9 bug fixes in api/judging/judging_service.py: + #2 judges never received the team's demo video (get_team_details / + format_team_for_judge read a phantom `video_url` key) + #3 get_bulk_judge_details always returned empty (NameError on an undefined + function name, swallowed by a blanket try/except) + #4 update_judge_assignment_details always 400'd (looked assignments up by + an empty judge_id instead of the assignment's own id) + +No rubric/scoring/results behavior is touched by any of these. +""" +import os + +os.environ.setdefault("ENVIRONMENT", "test") + +from unittest.mock import patch + +import api.judging.judging_service as svc + + +# --------------------------------------------------------------------------- +# #2 — demo_video_url surfaced to judges (both formatters) +# --------------------------------------------------------------------------- + +def test_get_team_details_surfaces_demo_video_url(): + fake_team = {"team": { + "id": "team-1", "name": "Team One", "users": [], + "demo_video_url": "https://youtu.be/abc123", + "devpost_link": "https://devpost.com/x", + }} + with patch.object(svc, "get_team", return_value=fake_team): + result = svc.get_team_details("team-1") + + assert result["team"]["demo_video_url"] == "https://youtu.be/abc123" + assert result["team"]["video_url"] == "https://youtu.be/abc123" + + +def test_get_team_details_video_url_falls_back_to_legacy_field_when_no_demo_video(): + fake_team = {"team": {"id": "team-1", "name": "Team One", "users": [], "video_url": "https://legacy.example.com/v.mp4"}} + with patch.object(svc, "get_team", return_value=fake_team): + result = svc.get_team_details("team-1") + + assert result["team"]["demo_video_url"] == "" + assert result["team"]["video_url"] == "https://legacy.example.com/v.mp4" + + +def test_get_team_details_empty_when_no_video_at_all(): + fake_team = {"team": {"id": "team-1", "name": "Team One", "users": []}} + with patch.object(svc, "get_team", return_value=fake_team): + result = svc.get_team_details("team-1") + + assert result["team"]["demo_video_url"] == "" + assert result["team"]["video_url"] == "" + + +def test_format_team_for_judge_surfaces_demo_video_url(): + team = {"id": "team-1", "name": "Team One", "users": [], "demo_video_url": "https://vimeo.com/123"} + result = svc.format_team_for_judge(team) + assert result["demo_video_url"] == "https://vimeo.com/123" + assert result["video_url"] == "https://vimeo.com/123" + + +def test_format_team_for_judge_devpost_and_github_unchanged(): + """Confirms the fix is additive — existing judge-facing fields (github, + devpost) are untouched by the video_url change.""" + team = { + "id": "team-1", "name": "Team One", "users": [], + "devpost_link": "https://devpost.com/x", + "github_links": [{"link": "https://github.com/org/repo"}], + } + result = svc.format_team_for_judge(team) + assert result["devpost_url"] == "https://devpost.com/x" + assert result["github_url"] == "https://github.com/org/repo" + + +# --------------------------------------------------------------------------- +# #3 — get_bulk_judge_details no longer NameErrors into an always-empty result +# --------------------------------------------------------------------------- + +def test_get_bulk_judge_details_returns_judges_after_namefix(): + judges_result = {"data": [{"id": "j1", "user_id": "u1", "name": "Judge One", "event_id": "event-1"}]} + with patch.object(svc, "get_volunteer_from_db_by_event", return_value=judges_result), \ + patch.object(svc, "fetch_judge_assignments_by_event_id", return_value=[]), \ + patch.object(svc, "fetch_judge_scores_by_event_id", return_value=[]) as mock_scores: + result = svc.get_bulk_judge_details("event-1") + + # The bug: this call used to raise NameError before ever reaching here. + mock_scores.assert_called_once_with("event-1") + assert result.get("error") is None + assert len(result["judges"]) == 1 + assert result["judges"][0]["id"] == "j1" + + +def test_get_bulk_judge_details_still_handles_service_error_gracefully(): + with patch.object(svc, "get_volunteer_from_db_by_event", return_value={"error": "boom"}), \ + patch.object(svc, "fetch_judge_assignments_by_event_id", return_value=[]), \ + patch.object(svc, "fetch_judge_scores_by_event_id", return_value=[]): + result = svc.get_bulk_judge_details("event-1") + + assert result["judges"] == [] + assert "error" in result + + +# --------------------------------------------------------------------------- +# #4 — update_judge_assignment_details looks up by the assignment's own id +# --------------------------------------------------------------------------- + +class _FakeAssignment: + def __init__(self, assignment_id): + self.id = assignment_id + self.judge_id = "judge-1" + self.event_id = "event-1" + self.team_id = "team-1" + self.round = "round1" + self.demo_time = None + self.room = None + self.updated_at = None + + +def test_update_judge_assignment_details_finds_assignment_by_id(): + fake = _FakeAssignment("assignment-1") + with patch.object(svc, "fetch_judge_assignment_by_id", return_value=fake) as mock_fetch, \ + patch.object(svc, "update_judge_assignment", side_effect=lambda a: a): + result = svc.update_judge_assignment_details("assignment-1", demo_time="10:00 AM", room="Room A") + + mock_fetch.assert_called_once_with("assignment-1") + assert result["success"] is True + assert result["assignment"]["id"] == "assignment-1" + assert result["assignment"]["demo_time"] == "10:00 AM" + assert result["assignment"]["room"] == "Room A" + + +def test_update_judge_assignment_details_404_like_response_for_unknown_id(): + with patch.object(svc, "fetch_judge_assignment_by_id", return_value=None): + result = svc.update_judge_assignment_details("nope") + + assert result["success"] is False + assert result["error"] == "Assignment not found" + + +def test_update_judge_assignment_details_never_queries_by_empty_judge_id(): + """The regression itself: no code path here should call + fetch_judge_assignments_by_judge_id("") anymore.""" + fake = _FakeAssignment("assignment-1") + with patch.object(svc, "fetch_judge_assignment_by_id", return_value=fake), \ + patch.object(svc, "update_judge_assignment", side_effect=lambda a: a), \ + patch.object(svc, "fetch_judge_assignments_by_judge_id") as mock_by_judge_id: + svc.update_judge_assignment_details("assignment-1", demo_time="9:00 AM") + + mock_by_judge_id.assert_not_called() diff --git a/db/db.py b/db/db.py index 9c2d70a..6e77f11 100644 --- a/db/db.py +++ b/db/db.py @@ -159,6 +159,9 @@ def get_user_doc_reference(user_id): def fetch_judge_assignments_by_judge_id(judge_id): return db.fetch_judge_assignments_by_judge_id(judge_id) +def fetch_judge_assignment_by_id(assignment_id): + return db.fetch_judge_assignment_by_id(assignment_id) + def fetch_judge_assignments_by_event_and_judge(event_id, judge_id): return db.fetch_judge_assignments_by_event_and_judge(event_id, judge_id) diff --git a/db/firestore.py b/db/firestore.py index 63c39c5..703d8c3 100644 --- a/db/firestore.py +++ b/db/firestore.py @@ -894,6 +894,19 @@ def fetch_judge_assignments_by_judge_id(self, judge_id): assignments.append(JudgeAssignment.deserialize(d)) return assignments + def fetch_judge_assignment_by_id(self, assignment_id): + """Direct doc-get by id (Part 9 bug #4: update_judge_assignment_details + used to look assignments up via fetch_judge_assignments_by_judge_id("") + — an empty judge_id — which can never match a real assignment, so the + route always 400'd 'Assignment not found').""" + db = self.get_db() + doc = db.collection('judge_assignments').document(assignment_id).get() + if not doc.exists: + return None + d = doc.to_dict() + d['id'] = doc.id + return JudgeAssignment.deserialize(d) + def fetch_judge_assignments_by_event_and_judge(self, event_id, judge_id): db = self.get_db() assignments = [] From 8cca0cd706e9f69cdbc42160b4c9486cc78da0aa Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Sat, 19 Sep 2026 18:19:50 -0700 Subject: [PATCH 08/11] Register submissions + peer_votes blueprints; add reminder cron; docs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - api/__init__.py: register api.submissions.submissions_views and api.peer_votes.peer_votes_views after broadcasts_views. Verified the full app (all blueprints) registers with no route conflicts (306 total routes). - .github/workflows/deadline-reminders.yml: hourly cron hitting POST /api/hackathons/deadlines/remind-due with X-Api-Key. Requires the BACKEND_CRON_TOKEN secret to be set on Fly (backend env) and in GitHub Actions — NOT done by this change; see open questions. - CLAUDE.md: new "Project submissions + Hackers' Choice (Sep 2026)" section (data contracts, endpoints, deploy-order note, documented-only Part 9 findings #5/#6/#14/#15), plus a new gotcha entry for the common.utils.slack-before-common.utils.firebase import-order requirement (load_dotenv() timing) that bit this work directly. Co-Authored-By: Claude Sonnet 5 --- .github/workflows/deadline-reminders.yml | 28 ++++++++++++++++ CLAUDE.md | 42 ++++++++++++++++++++++++ api/__init__.py | 4 +++ 3 files changed, 74 insertions(+) create mode 100644 .github/workflows/deadline-reminders.yml diff --git a/.github/workflows/deadline-reminders.yml b/.github/workflows/deadline-reminders.yml new file mode 100644 index 0000000..56bb354 --- /dev/null +++ b/.github/workflows/deadline-reminders.yml @@ -0,0 +1,28 @@ +name: ⏰ Submission deadline reminders + +# Hourly cron that pings POST /api/hackathons/deadlines/remind-due. The +# route itself iterates every currently-running event x {24,6,1} hour +# thresholds and no-ops (`only_if_due=True`) outside each due window, so +# calling it once an hour never double-sends reminders. Requires the +# BACKEND_CRON_TOKEN repo secret to match the backend's own +# BACKEND_CRON_TOKEN env var (checked via common.utils.api_key.check_api_key). + +on: + schedule: + - cron: "7 * * * *" + workflow_dispatch: {} + +permissions: + contents: read + +jobs: + remind: + name: 🔔 Send due reminders + runs-on: ubuntu-latest + steps: + - name: 📣 POST remind-due + run: | + curl -fsS -X POST "${BACKEND_URL:-https://api.ohack.dev}/api/hackathons/deadlines/remind-due" \ + -H "X-Api-Key: ${{ secrets.BACKEND_CRON_TOKEN }}" + env: + BACKEND_URL: ${{ vars.BACKEND_URL }} diff --git a/CLAUDE.md b/CLAUDE.md index 7e75484..5e475e3 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -71,6 +71,9 @@ Deployed to Fly.io (`fly.toml`, app: `backend-ohack`, region: `sjc`). Uses gunic ## Gotchas (load-bearing — every one of these has bitten us) +### `common.utils.firebase` needs `load_dotenv()` to have already run +`common/utils/firebase.py` reads `FIREBASE_CERT_CONFIG` at **module import time** (`json.loads(safe_get_env_var(...))`), with no lazy fallback — if that env var isn't in `os.environ` yet, the import raises `JSONDecodeError` immediately. `common/utils/slack.py` (and `cdn.py`, `github.py`, `openai_api.py`) call `load_dotenv()` at their own import time, which populates `.env` into the process env. **Import `common.utils.slack` (or another dotenv-calling module) before `common.utils.firebase`** in any new module that needs both — every existing service that imports both (`api/mentors/mentors_service.py`, `api/submissions/submissions_service.py`, `api/peer_votes/peer_votes_service.py`) does slack-then-firebase for exactly this reason. Getting the order backwards works fine in production (something else has already called `load_dotenv()` by the time your module loads) but breaks standalone script/test invocations that import your module directly. + ### Python 3.9 — no PEP 604 union syntax Backend runs on Python 3.9. `def foo() -> X | None:` raises `TypeError` at *import time*, blowing up every endpoint that imports the module. Use `Optional[X]` from `typing`. Audit any new `services/` module before committing. @@ -226,3 +229,42 @@ The public profile payload (`GET /api/users//profile/public`) is now ## Problem statement "Who's helping" roster (Sep 2026, frontend #359) `GET /api/problem-statements//helpers` (`api/problemstatements/problem_statement_views.py`, public) → `services/problem_statements_service.py::get_problem_statement_helpers`. The raw `helping` array is append-only history (`{user: , slack_user, type, timestamp}`) and real docs carry the same person 2–3× from double clicks, so `normalize_helping_entries` collapses to one row per person (earliest `timestamp` → `since`, latest `type` wins, junk entries dropped, oldest first) and `_enrich_helpers_batch` attaches `name/nickname/profile_image` with ONE `db.get_all` (same public-safe field set as team rosters — no email/propel_id). 60s TTL cache (`_helpers_cache`, `clear_helpers_cache(ps_id)`) cleared inside `save_helping_status`, which now also updates a returning helper's entry in place (keeps their original timestamp, collapses their duplicates) instead of appending. 404 when the doc doesn't exist. Tests: `api/problemstatements/tests/test_helpers.py` (run per-directory). The frontend keeps a counts-only fallback when this route is missing, so deploy order doesn't matter. + +## Project submissions + Hackers' Choice (Sep 2026) + +Backend half of replacing DevPost with an in-house team dashboard (frontend plan: `docs/plans/team-dashboard-devpost-replacement.md`). Judging itself (rubric, rounds, scoring, results) is **completely unchanged** — the only judging-facing addition is the team's demo video (see the judging bug-fix note below). Two new blueprints, both registered in `api/__init__.py` after `broadcasts_views`: + +### `api/submissions/` — team project write-ups + submission deadlines +Self-serve, deadline-aware writes to a team's `project_*` fields, split deliberately from `api.teams.teams_service.edit_team` (the admin write path — no deadline gate, no membership check, org-permission gated at the route; an admin can still override any `project_*` field including `project_submission_status` via `PATCH /api/team/edit`). Every write goes through `_authorize_team_write(propel_user_id, team_id, admin, enforce_deadline)`: 404 unknown team → 403 `not_team_member` (non-member, non-admin) → 409 `submissions_closed{deadline,late_until,now}` once the event's submission window has closed, unless `admin=True` or the caller opted out (`enforce_deadline=False`, used only by mentor-availability). Full contract, sanitization rationale, and CDN-image validation rules: `api/submissions/README.md`. + +New team-doc fields (absent ⇒ legacy team, no dashboard implied): `project_tagline`, `project_story` (raw markdown; the frontend renders it via react-markdown **without** rehype-raw, so `sanitize_markdown` in `common/utils/validators.py` is defence-in-depth, not the primary XSS boundary — it strips a small tag/attribute denylist and neutralizes `javascript:`/`vbscript:`/`data:` targets, and deliberately preserves generic `<` so `List` in a write-up survives), `project_built_with`, `project_links`, `project_thumbnail_url`, `project_images`, `project_updated_at`, `project_submitted_at`, `project_submission_status` (`draft|submitted|late`), `mentor_help_wanted` (absent ⇒ **True**, a signal-only "open to mentors / heads-down" toggle with no deadline gate and no other behavior change). + +New hackathon-doc field: `deadlines` (`{submission, late_submission_until, voting_opens, voting_closes}`, each a tz-normalized ISO string or `None`). `common/utils/validators.py::validate_deadlines` normalizes naive datetimes into the event's own timezone (`normalize_deadline_iso`) and enforces `submission <= late_submission_until` / `voting_opens < voting_closes` when both sides of a pair are present in the same payload; an unknown key or a bad value skips the *whole* `deadlines` field in `validate_hackathon_data_partial` (goes into `skipped_fields`) rather than partially applying it. `services/hackathons_service.py::save_hackathon` turns an explicit `None` into a Firestore `DELETE_FIELD` sentinel on an **update** (so a cleared deadline actually clears, not just leaves the old value under `merge=True`) but simply omits it on **create** (nothing to delete yet). `get_single_hackathon_event` strips `project_story` off every team in the response (it can run to ~20k chars and the endpoint already fans out to every team on the event) — the dashboard/team page fetch it via the per-team routes instead. + +Deadline reminders (`build_reminder_message`, `send_deadline_reminders`, `send_due_reminders_for_current_events`, same file): a per-team Slack nudge naming only what THAT team still owes (never nags an already-submitted team — returns `None`). Idempotency key is `reminders_sent[f"{kind}_{hours_before}h"]` on the hackathon doc; `only_if_due=True` (used by the hourly cron, `.github/workflows/deadline-reminders.yml`, `X-Api-Key: BACKEND_CRON_TOKEN` via `common/utils/api_key.check_api_key`) no-ops outside `[deadline - hours_before, deadline)` instead of erroring, so the cron can safely call `POST /api/hackathons/deadlines/remind-due` every hour for every current event × {24,6,1}h without spamming teams the other 23 hours. The admin "Send reminder now" button hits `POST /api/hackathons//deadlines/remind` with a Bearer token (`@auth.optional_user` + `is_admin(auth_user)`) instead of the API key. **`BACKEND_CRON_TOKEN` must be set on both Fly (backend env) and as a GitHub Actions secret** — this PR does not set either; do that before merging or the cron 403s. + +### `api/peer_votes/` — Hackers' Choice (assigned-slate approval vote) +Anti-popularity peer award, deliberately NOT a raw "vote for your favorite": each eligible voter (an `isSelected` hacker; `constraints.peer_vote_requires_submission` additionally requires the voter's own team to have submitted) gets a **deterministic, exposure-balanced slate** of `constraints.peer_vote_slate_size` (default 5) submitted projects, **never their own team** — seeded on `sha256(f"{event_id}:{propel_id}")` so the same voter always sees the same slate, sorted by current exposure ascending so under-shown projects surface first. Voters **approve, not rank**: pick up to `constraints.peer_vote_max_picks` (default 2). Scoring is the **Wilson score interval lower bound** (z=1.96) of approvals/shown, not a raw rate — `wilson_lower_bound(0,0)==0`, `(5,5)≈0.566`, `(1,1)≈0.207` — so a team shown to 2 people who both approved doesn't outrank a team shown to 40 with a 90% rate. **No tallies are ever shown to a voter**, only to admins (`GET .../peer-vote/results`). One ballot per voter (deterministic doc id `f"{event_id}__{propel_id}"` in the top-level `peer_votes` collection, `/` replaced with `_`), re-votable until close, admin-voidable, admin-publishable (`POST .../peer-vote/publish` appends `"Hackers' Choice"` to the winning team's `awards[]` **once** — idempotent — and writes a public summary doc at `hackathons/{doc}/peer_vote/summary`; exposure counts live at `hackathons/{doc}/peer_vote/exposure`). `constraints.peer_vote_enabled` (default **False**) is read on every voter-facing route — a disabled or unconfigured event returns `{"status": "disabled"}` from the slate route and 403 `peer_vote_disabled` from the ballot route, regardless of anything else. Full contract: `api/peer_votes/peer_votes_service.py` module docstring + function docstrings. + +Firestore transaction note: `get_slate`'s first-ever materialization for a voter (persisting the slate + incrementing exposure) runs inside `_in_transaction(db, body)`, which re-checks the ballot doc's existence **inside** the transaction (not just the optimistic outer read before it) so two near-simultaneous requests from the same voter can't double-build a slate or double-increment exposure. `_in_transaction` is the one seam tests monkeypatch (real `@firestore.transactional` needs a live Firestore client) — swap it for `lambda db, body: body(FakeTransaction(...))`. + +### Judging — video-only change + 3 bug fixes (`api/judging/judging_service.py`) +No rubric, scoring, round, or results logic changed. `get_team_details`/`format_team_for_judge` now also return `demo_video_url` (the real field a team's dashboard writes) and fill the legacy `video_url` key from it (`team.get('demo_video_url') or team.get('video_url', '')`) so both old and new judge-page code read a working value. Bugs found and fixed along the way (all pre-existing, none related to the video change): +- `get_bulk_judge_details` always returned an empty judges list — it called an undefined name, `fetch_judge_scores_by_event` (the correctly-named `fetch_judge_scores_by_event_id` was imported but never used), which NameError'd straight into the function's own blanket `except`. One-line rename fixes it. +- `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). + +### `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`. + +### `GET /api/volunteer//me?type=hacker` +Widened the existing mentor self-check route. `services/volunteers_service.py::get_volunteer_self_status(propel_user_id, event_id, volunteer_type)` mirrors `api.mentors.mentors_service.get_mentor_self_status`'s shape/leanness (`{f"is_{type}": bool, "volunteer": {"name","isSelected"}|None}`) for any `volunteer_type`, via the existing `find_volunteer_by_caller_identity` resolver. Backs the team dashboard's "am I an approved hacker for this event" gate and Hackers' Choice eligibility. `type=mentor` still delegates to the original mentor-specific service unchanged. + +### Security fix: `/devpost` and `/demo-video` now check team membership +`POST /api/team//devpost` and `POST /api/team//demo-video` (`api/teams/teams_views.py`) used to call `edit_team` **directly with no membership check at all** — any logged-in user could overwrite any team's Devpost link or demo video. Both now lazy-import `api.submissions.submissions_service.self_serve_team_edit`, which runs the same `_authorize_team_write` gate as every other self-serve write in this feature (403 non-member, 409 closed-window unless admin). `api.teams.teams_service` itself must never import `api.submissions` (keep the dependency one-directional) — the import lives in the view function, not the service module. + +### Documented-only findings (not fixed — out of scope or not trivial) +- **Legacy `save_team`** (`services/teams_service.py`, backing the old `POST /api/messages/team` — the route's own docstring says "kept for backward compatibility, new teams should use /api/team/queue") calls `create_github_repo` with an outdated positional argument order/count (missing `org_name`/`devpost_url`, has two extra params the current signature doesn't take). Every call to this legacy path already fails. Not fixed here: the correct fix needs a hackathon-event lookup this function doesn't currently do, and the path appears to have no live callers left — a real fix belongs with a decision about whether to delete the legacy route instead of repairing it. +- **`hackathon.devpost_url`** is read in `services/volunteers_service.py` but `save_hackathon` never writes a top-level `devpost_url` field on the hackathon doc — it's `links[]`-only. Not a bug introduced or fixed here; DevPost is now optional/deprecated for the team dashboard anyway, so `links[]` stays the source of truth. +- **`GET /api/hacker/applications/`** (`api/volunteers/volunteers_views.py`, public, `@auth.optional_user`) exposes `user_id` and `isSelected` for every hacker applicant via `get_all_hackers_by_event_id`. `findteam.js` matchmaking on the frontend depends on `user_id` being present, so this isn't trivially fixable without a frontend change too — a lean projection (drop `user_id`/`isSelected` from the public shape, resolve matching some other way) is a good follow-up but out of scope here. +- **Judge-page field naming split**: some frontend judge-page code reads `devpost_url`/`video_url` while the team doc itself uses `devpost_link`/`demo_video_url`. The judging API now fills both `demo_video_url` and `video_url` (see above) so either naming convention on the frontend keeps working; a follow-up could rename one side for consistency but isn't required. diff --git a/api/__init__.py b/api/__init__.py index e3848d3..5378b85 100644 --- a/api/__init__.py +++ b/api/__init__.py @@ -192,6 +192,8 @@ def add_headers(response): from api.praisebot import praisebot_views from api.jobs import jobs_views from api.broadcasts import broadcasts_views + from api.submissions import submissions_views + from api.peer_votes import peer_votes_views app.register_blueprint(messages_views.bp) app.register_blueprint(exception_views.bp) @@ -219,5 +221,7 @@ def add_headers(response): app.register_blueprint(praisebot_views.bp) app.register_blueprint(jobs_views.bp) app.register_blueprint(broadcasts_views.bp) + app.register_blueprint(submissions_views.bp) + app.register_blueprint(peer_votes_views.bp) return app From 32f1701bce553fdc05d267b7ea9ddeb7545a09db Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Sat, 19 Sep 2026 21:06:29 -0700 Subject: [PATCH 09/11] Fix Hackers' Choice peer-vote scoring, publish, and ballot integrity bugs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review findings on api/peer_votes/peer_votes_service.py: - HIGH: publish_results crowned an arbitrary "winner" when 0 ballots were cast (compute_results emits a row per submitted team regardless of vote count, so the sort fell through to team name). Now 409s "no_ballots" when results["ballots"] == 0 OR the rank-1 team has 0 approvals. - HIGH: compute_results scored teams against the raw exposure-doc count (how many slates a team was ever persisted into) instead of ballots actually cast. `shown` now counts non-voided ballots that have picks and include the team in their slate; the raw count is kept separately as exposure_shown. Voiding a ballot now removes it from both shown and approvals. - LOW: compute_voting_window's end_date fallback double-appended "T23:59:59" onto an end_date that already carried a time, producing an unparseable string and silently falling back to permanently closed. Only bare dates get the suffix now. - LOW: naive or "Z"-suffixed stored deadline strings crashed compute_voting_window (TypeError/ValueError) instead of degrading to "closed"; now parsed via normalize_deadline_iso with a warning log on failure. - LOW: ballot writes (submit_ballot, void_ballot) used set(merge=True); switched to a full set() of the rebuilt doc per spec. - LOW: a voided ballot rendered status "voted" with stale picks on the voter's own slate page; now renders "voided" with picks:null. get_results now also returns ballots_detail (voter_propel_id, voted_at, voided, picks_count only — no names/emails/picks) so the admin UI can drive void. - LOW: peer_vote_max_picks could be >= slate_size if the two fields were set inconsistently across separate saves (the validator only checks the relationship within one payload); _settings() now clamps at read time. Regression tests added for every case above. Co-Authored-By: Claude Fable 5.1 --- api/peer_votes/peer_votes_service.py | 152 ++++++++-- .../tests/test_peer_votes_service.py | 280 ++++++++++++++++++ 2 files changed, 407 insertions(+), 25 deletions(-) diff --git a/api/peer_votes/peer_votes_service.py b/api/peer_votes/peer_votes_service.py index b78530b..9f5badb 100644 --- a/api/peer_votes/peer_votes_service.py +++ b/api/peer_votes/peer_votes_service.py @@ -85,16 +85,44 @@ def _settings(event): """{"enabled", "slate_size", "max_picks", "requires_submission"} — reads constraints.peer_vote_* off the hackathon doc with the Part 3 defaults. MUST read peer_vote_enabled (a disabled event returns status:"disabled" - from every voter-facing route regardless of anything else).""" + from every voter-facing route regardless of anything else). + + LOW finding #12: common.utils.validators.validate_hackathon_data_partial + only enforces max_picks < slate_size when BOTH are present in the SAME + PATCH payload — it has no access to the stored doc, so an earlier save + that raised max_picks followed by a later save that lowers slate_size can + leave an inconsistent pair on the hackathon doc. Re-clamp at read time, + here, so every voter-facing route (slate/ballot) sees a consistent pair + regardless of how the doc got there. + """ constraints = (event or {}).get("constraints") or {} + slate_size = constraints.get("peer_vote_slate_size") or DEFAULT_SLATE_SIZE + max_picks = constraints.get("peer_vote_max_picks") or DEFAULT_MAX_PICKS + max_picks = max(1, min(max_picks, slate_size - 1)) return { "enabled": bool(constraints.get("peer_vote_enabled", False)), - "slate_size": constraints.get("peer_vote_slate_size") or DEFAULT_SLATE_SIZE, - "max_picks": constraints.get("peer_vote_max_picks") or DEFAULT_MAX_PICKS, + "slate_size": slate_size, + "max_picks": max_picks, "requires_submission": bool(constraints.get("peer_vote_requires_submission", False)), } +def _safe_normalize_deadline(value, tz_name, label): + """None/"" -> None; normalizes a naive or "Z"-suffixed value to an aware + ISO string; an unparseable value is logged and treated as absent rather + than raising (HIGH finding #5 — datetime.fromisoformat on a naive stored + string, compared against an aware `now`, raises TypeError; a + "Z"-suffixed string raises ValueError on Python 3.9/3.10 — either way an + unhandled crash used to reach the caller as a 500).""" + if not value: + return None + try: + return normalize_deadline_iso(value, tz_name) + except ValueError as e: + logger.warning("compute_voting_window: unparseable %s %r: %s", label, value, e) + return None + + def compute_voting_window(event, now=None): """{"state": upcoming|open|closed, "opens_at", "closes_at"}. @@ -103,21 +131,30 @@ def compute_voting_window(event, now=None): deadlines.voting_closes, falling back to the event's end_date at 23:59:59 in the event timezone. No usable opens_at/closes_at at all (a brand new event with no deadlines configured) -> closed, never open. + + LOW finding #6: the end_date fallback used to blindly append + "T23:59:59" to end_date before normalizing. If end_date already carries a + time component (or an offset), that produced an invalid, double-timed + string ("...T18:00:00T23:59:59"), which failed to parse and silently + fell back to permanently "closed". end_date is now normalized as-is when + it already looks like it has a time, and only gets "T23:59:59" appended + when it's a bare date. """ event = event or {} tz_name = event.get("timezone") or "America/Phoenix" now_dt = now or datetime.now(timezone.utc) deadlines = event.get("deadlines") or {} - opens_at = deadlines.get("voting_opens") or deadlines.get("late_submission_until") or deadlines.get("submission") - closes_at = deadlines.get("voting_closes") + opens_raw = deadlines.get("voting_opens") or deadlines.get("late_submission_until") or deadlines.get("submission") + opens_at = _safe_normalize_deadline(opens_raw, tz_name, "voting_opens") + + closes_at = _safe_normalize_deadline(deadlines.get("voting_closes"), tz_name, "voting_closes") if not closes_at: end_date = event.get("end_date") if end_date: - try: - closes_at = normalize_deadline_iso(f"{end_date}T23:59:59", tz_name) - except ValueError: - closes_at = None + has_time = "T" in end_date or " " in end_date.strip() + raw = end_date if has_time else f"{end_date}T23:59:59" + closes_at = _safe_normalize_deadline(raw, tz_name, "end_date-derived voting_closes") if not opens_at or not closes_at: return {"state": "closed", "opens_at": opens_at, "closes_at": closes_at} @@ -240,8 +277,15 @@ def _hydrate_slate(team_ids, candidates_by_id=None): def _slate_response_from_ballot(ballot, window, settings, own_team_ids, candidates_by_id=None): - picks = ballot.get("picks") - status = "voted" if picks else window["state"] + """LOW finding #11: a voided ballot must render status "voided" (with + picks nulled out), never "voted" — an admin voiding a ballot shouldn't + leave the voter's own slate page still showing their old picks as live.""" + if ballot.get("voided"): + status = "voided" + picks = None + else: + picks = ballot.get("picks") + status = "voted" if picks else window["state"] return { "status": status, "opens_at": window["opens_at"], @@ -365,11 +409,19 @@ def submit_ballot(propel_id, event_id, picks): ): return {"error": "invalid_picks"}, 400 + # LOW finding #8: the spec calls for a full set() (not a partial + # set(merge=True)) on every ballot write, so the persisted doc is always + # a complete, self-describing record rather than relying on merge + # semantics to preserve fields this write doesn't mention. Build the full + # doc from the existing one (carrying event_id/voter_propel_id/slate/ + # shown_at/created_at forward) and only overwrite the fields that change. now_iso = datetime.now(timezone.utc).isoformat() - update = {"picks": picks, "updated_at": now_iso} + full_doc = dict(ballot) + full_doc["picks"] = picks + full_doc["updated_at"] = now_iso if not ballot.get("voted_at"): - update["voted_at"] = now_iso - ballot_ref.set(update, merge=True) + full_doc["voted_at"] = now_iso + ballot_ref.set(full_doc) send_slack_audit( action="peer_vote_ballot", @@ -396,23 +448,43 @@ def wilson_lower_bound(approvals, shown, z=1.96): def compute_results(ballots, teams_by_id, exposure): """Pure. Ranks by Wilson lower bound desc, then raw approvals desc, then name — so a tie only ever breaks toward the more-approved, then - alphabetically (stable, no hidden randomness in the admin view).""" - approvals = Counter() - for ballot in ballots: - if ballot.get("voided"): - continue - for team_id in (ballot.get("picks") or []): - approvals[team_id] += 1 + alphabetically (stable, no hidden randomness in the admin view). + + HIGH finding #2: `shown` used to be read straight off the exposure doc, + which counts every slate the team was PERSISTED into (i.e. every voter + who ever opened the vote page), not every ballot that was actually CAST. + A team could be exposure-shown to 10 people but only have 2 real ballots + (both approving), and would still be scored against a denominator of 10 + — badly under-stating its Wilson lower bound relative to a team that + happened to get fewer opens but a higher cast-ballot rate. `shown` is now + computed from non-voided ballots that actually have picks (i.e. were + cast), which also means voiding a ballot removes it from both `shown` + and `approvals`. `exposure_shown` keeps the raw persisted-slate count + as a separate, purely informational field. + """ + active = [b for b in ballots if not b.get("voided")] + shown_counts = Counter( + team_id + for ballot in active + if ballot.get("picks") + for team_id in ballot.get("slate", []) + ) + approvals = Counter( + team_id + for ballot in active + for team_id in (ballot.get("picks") or []) + ) results = [] for team_id, team in teams_by_id.items(): - shown = exposure.get(team_id, 0) + shown = shown_counts.get(team_id, 0) + exposure_shown = exposure.get(team_id, 0) approved = approvals.get(team_id, 0) results.append({ "team_id": team_id, "name": team.get("name"), "shown": shown, - "exposure_shown": shown, + "exposure_shown": exposure_shown, "approvals": approved, "approval_rate": (approved / shown) if shown else 0.0, "wilson_lower_bound": wilson_lower_bound(approved, shown), @@ -454,6 +526,20 @@ def get_results(event_id): summary_exists = _peer_vote_subdoc(db, event_doc_id, "summary").get().exists + # LOW finding #11: the admin UI needs a per-voter list to drive `void`, + # but must never see WHO voted for WHAT — no names, no emails, and no + # `picks` (only how many). voter_propel_id is already an opaque id, not a + # name/email, so it's safe to surface for the void action's own use. + ballots_detail = [ + { + "voter_propel_id": b.get("voter_propel_id"), + "voted_at": b.get("voted_at"), + "voided": bool(b.get("voided")), + "picks_count": len(b.get("picks") or []), + } + for b in all_ballots + ] + return { "ballots": len(active_ballots), "voided": len(voided_ballots), @@ -462,6 +548,7 @@ def get_results(event_id): "settings": settings, "published": summary_exists, "teams": results, + "ballots_detail": ballots_detail, }, 200 @@ -473,8 +560,15 @@ def void_ballot(event_id, voter_propel_id, actor): if not snap.exists: return {"error": "Ballot not found"}, 404 + # LOW finding #8: full set(), not a partial merge — see submit_ballot's + # comment for why. + ballot = snap.to_dict() or {} now_iso = datetime.now(timezone.utc).isoformat() - ref.set({"voided": True, "voided_at": now_iso, "voided_by": actor}, merge=True) + full_doc = dict(ballot) + full_doc["voided"] = True + full_doc["voided_at"] = now_iso + full_doc["voided_by"] = actor + ref.set(full_doc) send_slack_audit( action="peer_vote_void", message=f"Hackers' Choice ballot voided for event {event_id}", @@ -496,8 +590,16 @@ def publish_results(event_id, actor, team_id=None): if status != 200: return results_payload, status + # HIGH finding #1: compute_results emits a row for every SUBMITTED team + # regardless of ballot count, so with zero ballots cast the sort key + # (wilson_lower_bound, approvals) is 0 for every team and the sort falls + # through to team NAME — crowning an alphabetically-first team as the + # "winner" of a vote nobody voted in. Refuse to publish when there were + # no ballots at all, OR when the computed rank-1 team has zero approvals + # (covers the case where slates were opened/persisted but nobody + # actually cast a ballot). ranked = results_payload.get("teams") or [] - if not ranked: + if not ranked or results_payload.get("ballots", 0) == 0 or ranked[0].get("approvals", 0) == 0: return {"error": "no_ballots"}, 409 winner_id = team_id or ranked[0]["team_id"] diff --git a/api/peer_votes/tests/test_peer_votes_service.py b/api/peer_votes/tests/test_peer_votes_service.py index dd16497..cad5287 100644 --- a/api/peer_votes/tests/test_peer_votes_service.py +++ b/api/peer_votes/tests/test_peer_votes_service.py @@ -9,6 +9,7 @@ """ import os from datetime import datetime, timedelta, timezone +from unittest.mock import MagicMock import pytest from google.cloud.firestore_v1.transforms import Increment @@ -175,6 +176,87 @@ def _own_teams(monkeypatch, team_ids): ) +# --------------------------------------------------------------------------- +# compute_voting_window — HIGH finding #5 (naive/"Z"/garbage stored deadline +# strings must not crash) and LOW finding #6 (end_date fallback must not +# double-append a time onto an end_date that already has one). +# --------------------------------------------------------------------------- + +def test_compute_voting_window_naive_voting_opens_is_localized(): + now = datetime(2026, 10, 10, 10, 0, tzinfo=timezone.utc) + event = _seed_hackathon(timezone="UTC", deadlines={ + "voting_opens": "2026-10-10T05:00:00", # naive, no offset + "voting_closes": "2026-10-12T00:00:00+00:00", + }) + window = svc.compute_voting_window(event, now=now) + assert window["state"] == "open" + assert window["opens_at"] == "2026-10-10T05:00:00+00:00" + + +def test_compute_voting_window_z_suffixed_voting_closes_is_normalized(): + now = datetime(2026, 10, 10, 10, 0, tzinfo=timezone.utc) + event = _seed_hackathon(timezone="UTC", deadlines={ + "voting_opens": "2026-10-09T00:00:00+00:00", + "voting_closes": "2026-10-11T00:00:00Z", + }) + window = svc.compute_voting_window(event, now=now) + assert window["state"] == "open" + assert window["closes_at"] == "2026-10-11T00:00:00+00:00" + + +def test_compute_voting_window_garbage_deadline_treated_as_closed(): + event = _seed_hackathon(timezone="UTC", deadlines={ + "voting_opens": "not-a-date", + "voting_closes": "2026-10-12T00:00:00+00:00", + }) + window = svc.compute_voting_window(event) + assert window["state"] == "closed" + assert window["opens_at"] is None + + +def test_compute_voting_window_end_date_only_date_appends_end_of_day(): + now = datetime(2026, 10, 11, 20, 0, tzinfo=timezone.utc) + event = _seed_hackathon(timezone="UTC", end_date="2026-10-11", deadlines={ + "voting_opens": "2026-10-09T00:00:00+00:00", + }) + window = svc.compute_voting_window(event, now=now) + assert window["closes_at"] == "2026-10-11T23:59:59+00:00" + assert window["state"] == "open" + + +def test_compute_voting_window_end_date_with_existing_time_is_not_double_appended(): + """LOW finding #6 regression: end_date already carrying a time used to + get "T23:59:59" appended anyway ("...T18:00:00T23:59:59"), which failed + to parse and silently fell back to permanently closed.""" + now = datetime(2026, 10, 11, 10, 0, tzinfo=timezone.utc) + event = _seed_hackathon(timezone="UTC", end_date="2026-10-11T18:00:00", deadlines={ + "voting_opens": "2026-10-09T00:00:00+00:00", + }) + window = svc.compute_voting_window(event, now=now) + assert window["closes_at"] == "2026-10-11T18:00:00+00:00" + assert window["state"] == "open" + + +# --------------------------------------------------------------------------- +# _settings — LOW finding #12: max_picks must never reach a voter-facing +# route >= slate_size, even if the stored doc has an inconsistent pair (the +# validator only checks the relationship when both fields are in the SAME +# PATCH payload). +# --------------------------------------------------------------------------- + +def test_settings_clamps_max_picks_below_stored_slate_size(): + event = _seed_hackathon(constraints={"peer_vote_slate_size": 3, "peer_vote_max_picks": 5}) + settings = svc._settings(event) + assert settings["slate_size"] == 3 + assert settings["max_picks"] == 2 + + +def test_settings_leaves_consistent_pair_untouched(): + event = _seed_hackathon(constraints={"peer_vote_slate_size": 5, "peer_vote_max_picks": 2}) + settings = svc._settings(event) + assert settings["max_picks"] == 2 + + # --------------------------------------------------------------------------- # wilson_lower_bound — spec values # --------------------------------------------------------------------------- @@ -292,6 +374,24 @@ def test_get_slate_second_call_returns_identical_slate_and_increments_exposure_o assert exposure[team["team_id"]] == 1 +def test_get_slate_renders_voided_status_not_voted(wire, monkeypatch): + """LOW finding #11 regression: an admin voiding a ballot must not leave + the voter's own slate page still rendering "voted" with their old picks + — it must show status "voided" and null out picks.""" + event = _seed_hackathon(constraints={"peer_vote_enabled": True}, deadlines=OPEN_WINDOW) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + _own_teams(monkeypatch, []) + wire[("peer_votes", "event-1__propel-1")] = { + "event_id": "event-1", "voter_propel_id": "propel-1", "slate": ["t1", "t2"], + "picks": ["t1"], "voted_at": "2026-01-01T00:00:00+00:00", "voided": True, + } + + result = svc.get_slate("propel-1", "event-1") + assert result["status"] == "voided" + assert result["picks"] is None + + def test_get_slate_upcoming_and_closed_states(wire, monkeypatch): _eligible(monkeypatch) _own_teams(monkeypatch, []) @@ -427,6 +527,66 @@ def test_submit_ballot_re_vote_keeps_original_voted_at(wire, monkeypatch): assert wire[("peer_votes", "event-1__propel-1")]["picks"] == ["t2"] +def test_submit_ballot_writes_full_doc_without_merge(wire, monkeypatch): + """LOW finding #8: the spec calls for a full set() (no merge=True) on + every ballot write. Spy on FakeDocRef.set to assert both that merge is + never passed as True AND that the written doc still carries every field + from the original ballot (event_id/slate/created_at) — a bug that + resurrected the old partial-set behavior would either flip merge back to + True, or write a doc missing these fields since a non-merge set() with a + partial dict would have silently dropped them.""" + event = _open_event() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + _eligible(monkeypatch) + wire[("peer_votes", "event-1__propel-1")] = { + "event_id": "event-1", "voter_propel_id": "propel-1", "slate": ["t1", "t2", "t3"], + "shown_at": "2026-01-01T00:00:00+00:00", "created_at": "2026-01-01T00:00:00+00:00", + "picks": None, "voted_at": None, "voided": False, + } + + calls = [] + original_set = FakeDocRef.set + + def spy_set(self, data, merge=False): + calls.append((dict(data), merge)) + return original_set(self, data, merge=merge) + + monkeypatch.setattr(FakeDocRef, "set", spy_set) + + svc.submit_ballot("propel-1", "event-1", ["t1"]) + + data, merge = calls[-1] + assert merge is False + assert data["slate"] == ["t1", "t2", "t3"] + assert data["event_id"] == "event-1" + assert data["created_at"] == "2026-01-01T00:00:00+00:00" + assert data["picks"] == ["t1"] + + +def test_void_ballot_writes_full_doc_without_merge(wire, monkeypatch): + wire[("peer_votes", "event-1__u1")] = { + "event_id": "event-1", "voter_propel_id": "u1", "slate": ["t1"], + "picks": ["t1"], "created_at": "2026-01-01T00:00:00+00:00", "voided": False, + } + + calls = [] + original_set = FakeDocRef.set + + def spy_set(self, data, merge=False): + calls.append((dict(data), merge)) + return original_set(self, data, merge=merge) + + monkeypatch.setattr(FakeDocRef, "set", spy_set) + + svc.void_ballot("event-1", "u1", "admin-1") + + data, merge = calls[-1] + assert merge is False + assert data["slate"] == ["t1"] + assert data["created_at"] == "2026-01-01T00:00:00+00:00" + assert data["voided"] is True + + # --------------------------------------------------------------------------- # compute_results / get_results — voided exclusion # --------------------------------------------------------------------------- @@ -444,6 +604,50 @@ def test_compute_results_excludes_voided_ballots(): assert by_id["t2"]["approvals"] == 0 +def test_compute_results_shown_reflects_cast_ballots_not_raw_exposure(): + """HIGH finding #2 — the reviewer's scenario: team A was persisted into + 10 slates (exposure) but only 2 ballots were actually cast, both + approving it; team B was persisted into only 3 slates but got 1 approving + ballot. Scoring on raw exposure would badly under-rate A's 100%-of-2 + approval rate against a denominator of 10; scoring on cast ballots (as + fixed) ranks A above B. exposure_shown keeps the raw count separately.""" + ballots = [ + {"slate": ["a", "x"], "picks": ["a"], "voided": False}, + {"slate": ["a", "y"], "picks": ["a"], "voided": False}, + {"slate": ["b", "z"], "picks": ["b"], "voided": False}, + ] + teams_by_id = {"a": {"name": "Team A"}, "b": {"name": "Team B"}} + exposure = {"a": 10, "b": 3} + results = svc.compute_results(ballots, teams_by_id, exposure) + by_id = {r["team_id"]: r for r in results} + + assert by_id["a"]["shown"] == 2 + assert by_id["a"]["exposure_shown"] == 10 + assert by_id["b"]["shown"] == 1 + assert by_id["b"]["exposure_shown"] == 3 + # A's rank must beat B's after the fix. + assert by_id["a"]["rank"] < by_id["b"]["rank"] + + +def test_compute_results_voiding_a_ballot_removes_it_from_shown_and_approvals(): + ballots_before = [ + {"slate": ["a"], "picks": ["a"], "voided": False}, + {"slate": ["a"], "picks": ["a"], "voided": False}, + ] + ballots_after_void = [ + {"slate": ["a"], "picks": ["a"], "voided": False}, + {"slate": ["a"], "picks": ["a"], "voided": True}, # this one got voided + ] + teams_by_id = {"a": {"name": "Team A"}} + exposure = {"a": 2} + + before = svc.compute_results(ballots_before, teams_by_id, exposure)[0] + after = svc.compute_results(ballots_after_void, teams_by_id, exposure)[0] + + assert before["shown"] == 2 and before["approvals"] == 2 + assert after["shown"] == 1 and after["approvals"] == 1 + + def test_get_results_reports_voided_count_separately(wire, monkeypatch): event = _seed_hackathon(constraints={"peer_vote_enabled": True}) monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) @@ -458,6 +662,38 @@ def test_get_results_reports_voided_count_separately(wire, monkeypatch): assert result["voided"] == 1 +def test_get_results_includes_ballots_detail_with_no_names_or_picks(wire, monkeypatch): + """LOW finding #11: the admin UI needs a per-voter list to drive `void` + — voter_propel_id (an opaque id, not PII) + voted_at + voided + a COUNT + of picks, but never the picks themselves or any name/email.""" + event = _seed_hackathon(constraints={"peer_vote_enabled": True}) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + monkeypatch.setattr("services.volunteers_service.get_all_hackers_by_event_id", lambda eid: []) + _seed_team(wire, "t1", project_submission_status="submitted") + wire[("peer_votes", "event-1__u1")] = { + "event_id": "event-1", "voter_propel_id": "u1", "picks": ["t1"], + "voted_at": "2026-01-01T00:00:00+00:00", "voided": False, + } + wire[("peer_votes", "event-1__u2")] = { + "event_id": "event-1", "voter_propel_id": "u2", "picks": None, + "voted_at": None, "voided": True, + } + + result, status = svc.get_results("event-1") + assert status == 200 + detail_by_voter = {d["voter_propel_id"]: d for d in result["ballots_detail"]} + assert detail_by_voter["u1"] == { + "voter_propel_id": "u1", "voted_at": "2026-01-01T00:00:00+00:00", + "voided": False, "picks_count": 1, + } + assert detail_by_voter["u2"]["voided"] is True + assert detail_by_voter["u2"]["picks_count"] == 0 + for detail in result["ballots_detail"]: + assert "picks" not in detail + assert "name" not in detail + assert "email" not in detail + + # --------------------------------------------------------------------------- # publish_results — idempotent # --------------------------------------------------------------------------- @@ -486,6 +722,50 @@ def test_publish_results_409_with_no_ballots(wire, monkeypatch): assert result["error"] == "no_ballots" +def test_publish_results_409_with_submitted_teams_but_zero_ballots(wire, monkeypatch): + """HIGH finding #1 — the actual reviewer scenario: compute_results emits + a row for every SUBMITTED team regardless of ballot count, so with 3 + submitted teams and 0 ballots, every row's wilson_lower_bound/approvals + tie at 0 and the sort falls through to team NAME, crowning an arbitrary + "winner". Must 409 instead, and must not write an award or a summary + doc.""" + event = _seed_hackathon(constraints={"peer_vote_enabled": True}) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + monkeypatch.setattr("services.volunteers_service.get_all_hackers_by_event_id", lambda eid: []) + _seed_team(wire, "aaa-team", project_submission_status="submitted") + _seed_team(wire, "bbb-team", project_submission_status="submitted") + _seed_team(wire, "ccc-team", project_submission_status="submitted") + # No peer_votes docs seeded at all — zero ballots cast. + + result, status = svc.publish_results("event-1", "admin-1") + + assert status == 409 + assert result["error"] == "no_ballots" + assert "awards" not in wire.get(("teams", "aaa-team"), {}) + assert ("hackathons", "evtdoc-1", "peer_vote", "summary") not in wire + + +def test_publish_results_409_when_slates_opened_but_nobody_voted(wire, monkeypatch): + """Ballots exist (slates were persisted — people opened the vote page) + but nobody actually cast a pick. results["ballots"] is non-zero here, so + this exercises the SECOND half of the HIGH-1 fix: the rank-1 team having + zero approvals.""" + event = _seed_hackathon(constraints={"peer_vote_enabled": True}) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) + monkeypatch.setattr("services.volunteers_service.get_all_hackers_by_event_id", lambda eid: []) + _seed_team(wire, "t1", project_submission_status="submitted") + _seed_team(wire, "t2", project_submission_status="submitted") + wire[("peer_votes", "event-1__u1")] = { + "event_id": "event-1", "voter_propel_id": "u1", "slate": ["t1", "t2"], + "picks": None, "voided": False, + } + + result, status = svc.publish_results("event-1", "admin-1") + + assert status == 409 + assert result["error"] == "no_ballots" + + def test_void_ballot_excludes_from_future_results(wire, monkeypatch): event = _seed_hackathon(constraints={"peer_vote_enabled": True}) monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: event) From f312bb266866d0d441b0c320f505bc600017a976 Mon Sep 17 00:00:00 2001 From: Greg V <6913307+gregv@users.noreply.github.com> Date: Sat, 19 Sep 2026 21:07:07 -0700 Subject: [PATCH 10/11] Fix submission deadline parsing, admin project override, and sanitizer bypasses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit common/utils/validators.py::sanitize_markdown (LOW): - Loop the tag-strip pass to a fixpoint so a nested bypass like "ipt>" can't reassemble into a live tag on a single pass. - Match on*= attributes preceded by "/" as well as whitespace, catching "". - Neutralize javascript:/vbscript:/data: targets in unquoted HTML attributes, not just quoted ones. - Also neutralize those targets in markdown link/image syntax ("[text](javascript:...)" -> "[text](#)"). - List/Map still survive untouched; regexes stay linear. api/submissions/submissions_service.py (HIGH/MEDIUM/LOW): - compute_submission_window now parses stored submission/late_submission_until strings through normalize_deadline_iso before comparing against `now` — a naive or "Z"-suffixed stored value used to crash (TypeError/ValueError) instead of degrading to no_deadline; unparseable values are logged and treated as absent. - send_deadline_reminders' only_if_due window narrowed from [deadline-h, deadline) to a one-hour-wide [deadline-h, deadline-h+1h) — the old window let a deadline set with only a few hours' notice fall due for both the 24h and 6h tiers on the same cron tick and fire both at once. - submit_project now checks the already-submitted idempotent path BEFORE the deadline gate, so a team that submitted on time never gets a spurious 409 revisiting the endpoint after close. - self_serve_team_edit (the /devpost and /demo-video bridge) now also busts the hackathons_service event cache after delegating to edit_team, which alone only clears the generic per-function caches — fixes a stale DevPost link/demo video on the event page for up to 10 minutes. api/teams/teams_service.py::edit_team (MEDIUM): - field_mappings now carries project_tagline, project_story, project_built_with, project_links, project_thumbnail_url, project_images, and project_submission_status, making the documented admin override path (PATCH /api/team/edit) actually work. project_submission_status is validated against draft/submitted/late (400 + no write otherwise); tagline/story get the same sanitize_markdown treatment as the self-serve save_project path; a status change stamps project_updated_at. Regression tests added for every case above. Co-Authored-By: Claude Fable 5.1 --- api/submissions/submissions_service.py | 76 +++++++++-- .../tests/test_submissions_service.py | 120 +++++++++++++++++- api/teams/teams_service.py | 45 ++++++- .../tests/test_edit_team_project_fields.py | 95 ++++++++++++++ common/utils/validators.py | 38 +++++- test/common/utils/test_validators.py | 33 +++++ 6 files changed, 383 insertions(+), 24 deletions(-) create mode 100644 api/teams/tests/test_edit_team_project_fields.py diff --git a/api/submissions/submissions_service.py b/api/submissions/submissions_service.py index 836afcd..f504e33 100644 --- a/api/submissions/submissions_service.py +++ b/api/submissions/submissions_service.py @@ -31,7 +31,12 @@ from common.utils.firestore_helpers import clear_all_caches from common.utils.slack import send_slack, send_slack_audit from common.utils.firebase import get_hackathon_by_event_id -from common.utils.validators import sanitize_markdown, sanitize_string, validate_https_url +from common.utils.validators import ( + normalize_deadline_iso, + sanitize_markdown, + sanitize_string, + validate_https_url, +) from services.teams_service import get_team logger = logging.getLogger("myapp") @@ -97,6 +102,21 @@ def _cdn_server() -> str: return os.getenv("CDN_SERVER", "https://cdn.ohack.dev").rstrip("/") +def _safe_normalize_deadline(value, tz_name, label): + """None/"" -> None; a naive or "Z"-suffixed value is normalized to an + aware ISO string; an unparseable value is logged and treated as absent + rather than raising (HIGH finding #5 — datetime.fromisoformat on a naive + stored string, compared against an aware `now`, raises TypeError; a + "Z"-suffixed string raises ValueError on Python 3.9/3.10).""" + if not value: + return None + try: + return normalize_deadline_iso(value, tz_name) + except ValueError as e: + logger.warning("compute_submission_window: unparseable %s %r: %s", label, value, e) + return None + + def compute_submission_window(event, now=None): """{"state": open|late|closed|no_deadline, "submission", "late_until", "now", "timezone"}. @@ -109,8 +129,8 @@ def compute_submission_window(event, now=None): tz_name = event.get("timezone") or "America/Phoenix" now_dt = now or datetime.now(timezone.utc) deadlines = event.get("deadlines") or {} - submission = deadlines.get("submission") - late_until = deadlines.get("late_submission_until") + submission = _safe_normalize_deadline(deadlines.get("submission"), tz_name, "submission") + late_until = _safe_normalize_deadline(deadlines.get("late_submission_until"), tz_name, "late_submission_until") if not submission: return { @@ -369,11 +389,15 @@ def save_project(propel_user_id, team_id, payload, admin=False): def submit_project(propel_user_id, team_id, admin=False): """Marks the project submitted|late. Idempotent — resubmitting an already - submitted/late project is a no-op 200 with already_submitted=True. Blocked - (409, via _authorize_team_write) once the window is fully closed unless - the caller is an admin, in which case the forced submission is recorded - as 'late' regardless of how long past close it is.""" - err, team, _event, window = _authorize_team_write(propel_user_id, team_id, admin=admin, enforce_deadline=True) + submitted/late project is a no-op 200 with already_submitted=True, EVEN + once the submission window has fully closed (LOW finding #10 — the + already-submitted check must run before the deadline gate, not after, or + a team that submitted on time gets a spurious 409 just by revisiting the + dashboard after close). A fresh (not-yet-submitted) team is still blocked + with 409 once the window is fully closed, unless the caller is an admin, + in which case the forced submission is recorded as 'late' regardless of + how long past close it is.""" + err, team, _event, window = _authorize_team_write(propel_user_id, team_id, admin=admin, enforce_deadline=False) if err: return err @@ -381,6 +405,17 @@ def submit_project(propel_user_id, team_id, admin=False): fresh = (get_team(team_id) or {}).get("team") or {} return {"success": True, "already_submitted": True, "team": fresh}, 200 + if not admin and submissions_closed(window): + return ( + { + "error": "submissions_closed", + "deadline": window.get("submission"), + "late_until": window.get("late_until"), + "now": window.get("now"), + }, + 409, + ) + missing = [f for f in REQUIRED_SUBMIT_FIELDS if not (team.get(f) or "").strip()] if missing: return {"error": "incomplete", "missing": missing}, 400 @@ -422,13 +457,22 @@ def self_serve_team_edit(propel_user_id, team_id, fields, admin=False): edit_team (which already knows how to stamp *_submitted timestamps for devpost_link/demo_video_url). Lazy import: api.teams.teams_service must never import this module, so importing it here (not at module top) keeps - the dependency one-directional.""" + the dependency one-directional. + + MEDIUM finding #4: edit_team only busts the generic per-function caches + (common.utils.firestore_helpers.clear_all_caches) — it has no reason to + know about the separately-cached get_single_hackathon_event (10-min TTL), + so a self-serve DevPost/demo-video save left the event page showing stale + data for up to 10 minutes. Call this module's own clear_cache() (which + busts both) after edit_team returns. + """ err, _team, _event, _window = _authorize_team_write(propel_user_id, team_id, admin=admin, enforce_deadline=True) if err: return err from api.teams.teams_service import edit_team edit_result = edit_team({"id": team_id, **fields}) + clear_cache() fresh = (get_team(team_id) or {}).get("team") or {} return {**edit_result, "team": fresh} @@ -523,9 +567,14 @@ def send_deadline_reminders(event_id, kind, hours_before, *, only_if_due=False, `force` — `reminders_sent[f"{kind}_{hours_before}h"]` on the hackathon doc is the idempotency key. `only_if_due` (used by the hourly cron) skips silently ({"success": true, "skipped": "not_due"}) outside the - [deadline - hours_before, deadline) window rather than erroring, so the - cron can call this for every (event, hours) pair every hour without - spamming teams the other 23 hours of the day.""" + one-hour-wide [deadline - hours_before, deadline - hours_before + 1h) + window rather than erroring, so the cron can call this for every + (event, hours) pair every hour without spamming teams the other 23 hours + of the day. The window is deliberately only one hour wide (it used to be + [deadline - hours_before, deadline), i.e. open all the way up to the + deadline itself) — with the old window, a deadline set with only a few + hours' notice would have its 24h AND 6h tiers both fall "due" on the very + first cron tick and fire together (LOW finding #9).""" if kind not in REMINDER_KINDS: return {"error": f"kind must be one of {sorted(REMINDER_KINDS)}"}, 400 try: @@ -551,7 +600,8 @@ def send_deadline_reminders(event_id, kind, hours_before, *, only_if_due=False, now_dt = datetime.now(timezone.utc) deadline_dt = datetime.fromisoformat(deadline_iso) due_at = deadline_dt - timedelta(hours=hours_before) - if only_if_due and not (due_at <= now_dt < deadline_dt): + due_window_end = due_at + timedelta(hours=1) + if only_if_due and not (due_at <= now_dt < due_window_end): return {"success": True, "kind": kind, "hours_before": hours_before, "skipped": "not_due", "simulated": _notifications_disabled()}, 200 db = get_db() diff --git a/api/submissions/tests/test_submissions_service.py b/api/submissions/tests/test_submissions_service.py index 8405fb0..5a52222 100644 --- a/api/submissions/tests/test_submissions_service.py +++ b/api/submissions/tests/test_submissions_service.py @@ -9,6 +9,7 @@ """ import os from datetime import datetime, timedelta, timezone +from unittest.mock import MagicMock import pytest @@ -150,6 +151,50 @@ def test_window_closed_immediately_after_submission_with_no_late_window(): assert svc.compute_submission_window(event, now=now)["state"] == "closed" +# --------------------------------------------------------------------------- +# HIGH finding #5 — naive / "Z"-suffixed / garbage stored deadline strings +# must not crash compute_submission_window (datetime.fromisoformat on a +# naive string compared against an aware `now` raises TypeError; "Z" raises +# ValueError on Python 3.9/3.10). +# --------------------------------------------------------------------------- + +def test_window_naive_submission_deadline_is_localized_and_compared(): + now = datetime(2026, 10, 10, 10, 0, tzinfo=timezone.utc) + # Naive string, no offset — must be localized to the event tz, not raise. + event = {"timezone": "UTC", "deadlines": {"submission": "2026-10-10T15:00:00"}} + window = svc.compute_submission_window(event, now=now) + assert window["state"] == "open" + assert window["submission"] == "2026-10-10T15:00:00+00:00" + + +def test_window_z_suffixed_submission_deadline_is_normalized(): + now = datetime(2026, 10, 10, 21, 0, tzinfo=timezone.utc) + event = {"timezone": "UTC", "deadlines": {"submission": "2026-10-10T22:00:00Z"}} + window = svc.compute_submission_window(event, now=now) + assert window["state"] == "open" + assert window["submission"] == "2026-10-10T22:00:00+00:00" + + +def test_window_garbage_submission_deadline_treated_as_no_deadline(): + event = {"timezone": "UTC", "deadlines": {"submission": "not-a-date"}} + window = svc.compute_submission_window(event) + assert window["state"] == "no_deadline" + assert window["submission"] is None + + +def test_window_garbage_late_until_treated_as_absent(): + now = datetime(2026, 10, 10, 16, 0, tzinfo=timezone.utc) + event = { + "timezone": "UTC", + "deadlines": {"submission": "2026-10-10T15:00:00+00:00", "late_submission_until": "garbage"}, + } + window = svc.compute_submission_window(event, now=now) + # late_submission_until couldn't be parsed, so it's treated as absent — + # the window falls straight to "closed" rather than raising. + assert window["state"] == "closed" + assert window["late_until"] is None + + # --------------------------------------------------------------------------- # validate_project_payload — sanitization + limits # --------------------------------------------------------------------------- @@ -342,6 +387,20 @@ def test_submit_project_idempotent_when_already_submitted(wire, monkeypatch): assert result["already_submitted"] is True +def test_submit_project_idempotent_even_after_deadline_closed(wire, monkeypatch): + """LOW finding #10 regression: a team that submitted on time must still + get the idempotent 200 (not a 409) if they revisit the submit endpoint + after the window has fully closed — the already-submitted check has to + run BEFORE the deadline gate.""" + _seed_team(wire, project_submission_status="submitted", project_tagline="T", project_story="S") + _member(monkeypatch, is_member=True) + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {}) + monkeypatch.setattr(svc, "compute_submission_window", lambda event, now=None: {"state": "closed", "submission": "x", "late_until": None, "now": "z", "timezone": "UTC"}) + result, status = svc.submit_project("propel-1", "team-1") + assert status == 200 + assert result["already_submitted"] is True + + def test_submit_project_403_for_non_member(wire, monkeypatch): _seed_team(wire) _member(monkeypatch, is_member=False) @@ -405,6 +464,38 @@ def test_self_serve_team_edit_409_when_closed(wire, monkeypatch): assert status == 409 +def test_self_serve_team_edit_also_clears_hackathon_event_cache(monkeypatch, team_store): + """MEDIUM finding #4 regression: self_serve_team_edit delegates the write + to edit_team, which only busts the generic per-function caches — it must + ALSO bust services.hackathons_service's own get_single_hackathon_event + cache (via this module's clear_cache()), or the event page shows a stale + DevPost link / demo video for up to 10 minutes. + + Deliberately does not use the `wire` fixture, which stubs svc.clear_cache + to a no-op — this test needs the REAL clear_cache() to run so it can + assert on what it calls. + """ + monkeypatch.setattr(svc, "get_db", lambda: FakeDb(team_store)) + monkeypatch.setattr(svc, "send_slack_audit", lambda **kwargs: None) + monkeypatch.setattr(svc, "send_slack", lambda **kwargs: None) + monkeypatch.setattr(svc, "clear_all_caches", lambda: None) + monkeypatch.setattr(svc, "get_team", lambda team_id: {"team": {"id": team_id}}) + + _seed_team(team_store) + _member(monkeypatch, is_member=True) + _event(monkeypatch) + monkeypatch.setattr( + "api.teams.teams_service.edit_team", + lambda json: {"success": True, "message": "Team updated successfully", "team_id": json["id"]}, + ) + hackathon_clear = MagicMock() + monkeypatch.setattr("services.hackathons_service.clear_cache", hackathon_clear) + + svc.self_serve_team_edit("propel-1", "team-1", {"devpost_link": "https://devpost.com/x"}) + + hackathon_clear.assert_called_once() + + # --------------------------------------------------------------------------- # set_mentor_help_wanted # --------------------------------------------------------------------------- @@ -651,8 +742,12 @@ def test_send_deadline_reminders_only_if_due_skips_outside_window(reminder_wire, def test_send_deadline_reminders_only_if_due_sends_inside_window(reminder_wire, monkeypatch): + # The due window is now exactly one hour wide: [deadline-24h, deadline-23h). + # Center the deadline in that window (23h30m away) so the small delay + # between building `deadline` here and send_deadline_reminders computing + # its own `now_dt` can never push the check outside the window. now = datetime.now(timezone.utc) - deadline = (now + timedelta(hours=23)).isoformat() + deadline = (now + timedelta(hours=23, minutes=30)).isoformat() monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"id": "evtdoc-1", "event_id": "event-1", "deadlines": {"submission": deadline}}) reminder_wire["teams"]["team-1"] = {"hackathon_event_id": "event-1", "slack_channel": "#t1"} @@ -661,6 +756,25 @@ def test_send_deadline_reminders_only_if_due_sends_inside_window(reminder_wire, assert result["notified"] == ["team-1"] +def test_send_deadline_reminders_only_if_due_does_not_double_fire_when_close_to_deadline(reminder_wire, monkeypatch): + """LOW finding #9 regression: with the old [deadline-h, deadline) window, + a deadline only 3 hours away would fall inside BOTH the 24h and 6h due + windows on the very first cron tick, sending two reminders at once. Each + tier's due window is now a narrow 1-hour slot, so neither one (whose + slot already elapsed, since the deadline is unusually close) fires.""" + now = datetime.now(timezone.utc) + deadline = (now + timedelta(hours=3)).isoformat() + monkeypatch.setattr(svc, "get_hackathon_by_event_id", lambda eid: {"id": "evtdoc-1", "event_id": "event-1", "deadlines": {"submission": deadline}}) + reminder_wire["teams"]["team-1"] = {"hackathon_event_id": "event-1", "slack_channel": "#t1"} + + result_24h, status_24h = svc.send_deadline_reminders("event-1", "submission", 24, only_if_due=True) + result_6h, status_6h = svc.send_deadline_reminders("event-1", "submission", 6, only_if_due=True) + + assert status_24h == status_6h == 200 + assert result_24h["skipped"] == "not_due" + assert result_6h["skipped"] == "not_due" + + def test_send_deadline_reminders_simulated_flag_reflects_test_environment(reminder_wire, monkeypatch): now = datetime.now(timezone.utc) deadline = (now + timedelta(hours=24)).isoformat() @@ -676,7 +790,7 @@ def test_send_deadline_reminders_simulated_flag_reflects_test_environment(remind def test_send_due_reminders_for_current_events_iterates_hours_and_events(reminder_wire, monkeypatch): now = datetime.now(timezone.utc) - deadline = (now + timedelta(hours=23)).isoformat() + deadline = (now + timedelta(hours=23, minutes=30)).isoformat() monkeypatch.setattr( "services.hackathons_service.get_hackathon_list", lambda kind: {"hackathons": [{"event_id": "event-1"}]}, @@ -689,6 +803,6 @@ def test_send_due_reminders_for_current_events_iterates_hours_and_events(reminde assert status == 200 hours_checked = {r["hours_before"] for r in result["results"]} assert hours_checked == {24, 6, 1} - # Only the 24h reminder was due (deadline is 23h away); confirm it fired. + # Only the 24h reminder was due (deadline is ~23h30m away); confirm it fired. fired = [r for r in result["results"] if r["hours_before"] == 24][0] assert fired["result"]["notified"] == ["team-1"] diff --git a/api/teams/teams_service.py b/api/teams/teams_service.py index 7c53287..514e23a 100644 --- a/api/teams/teams_service.py +++ b/api/teams/teams_service.py @@ -15,11 +15,24 @@ from common.utils.slack import create_slack_channel, invite_user_to_channel, send_slack, send_slack_audit from common.utils.firebase import get_hackathon_by_event_id from common.utils.oauth_providers import extract_slack_user_id, is_oauth_user_id, normalize_slack_user_id +from common.utils.validators import sanitize_markdown -from common.utils.slack import add_bot_to_channel +from common.utils.slack import add_bot_to_channel logger = logging.getLogger("myapp") +# Admin override of a team's project_* write-up fields via PATCH +# /api/team/edit (MEDIUM finding #3 — documented in api/submissions/README.md +# "Ownership" as always having been the intended admin path, but edit_team's +# field_mappings never actually carried these fields). Limits/catalog are +# duplicated from api.submissions.submissions_service's PROJECT_LIMITS / +# SUBMITTED_STATUSES (not imported — teams_service must never import +# api.submissions, a one-directional dependency documented in that module's +# docstring). +PROJECT_TAGLINE_MAX_LEN = 140 +PROJECT_STORY_MAX_LEN = 20000 +PROJECT_SUBMISSION_STATUSES = {"draft", "submitted", "late"} + # Slack user IDs for OHack admins who are auto-invited to every team channel # and CCed on completion broadcasts. Single source of truth for both call sites. TEAM_COMPLETION_SLACK_ADMINS = [ @@ -294,6 +307,13 @@ def edit_team(json): "admin_notes": "admin_notes", "devpost_link": "devpost_link", "demo_video_url": "demo_video_url", + "project_tagline": "project_tagline", + "project_story": "project_story", + "project_built_with": "project_built_with", + "project_links": "project_links", + "project_thumbnail_url": "project_thumbnail_url", + "project_images": "project_images", + "project_submission_status": "project_submission_status", } # Normalize demo_video_url: trim, cap at 500 chars, empty string => clear @@ -303,6 +323,29 @@ def edit_team(json): trimmed = raw.strip()[:500] json["demo_video_url"] = trimmed if trimmed else None + # project_submission_status must be one of the catalog values — a bad + # value 400s rather than writing a status the dashboard/gallery/funnel + # don't know how to render. + if "project_submission_status" in json: + status_val = json.get("project_submission_status") + if status_val not in PROJECT_SUBMISSION_STATUSES: + return { + "message": f"Error: project_submission_status must be one of {sorted(PROJECT_SUBMISSION_STATUSES)}", + "success": False, + }, 400 + if status_val != team_data.get("project_submission_status"): + update_data["project_updated_at"] = datetime.now().isoformat() + + # project_tagline / project_story get the same defence-in-depth + # sanitize_markdown treatment as the self-serve save_project path. + if "project_tagline" in json: + val = json.get("project_tagline") + json["project_tagline"] = sanitize_markdown(val, PROJECT_TAGLINE_MAX_LEN) if isinstance(val, str) and val else None + + if "project_story" in json: + val = json.get("project_story") + json["project_story"] = sanitize_markdown(val, PROJECT_STORY_MAX_LEN) if isinstance(val, str) and val else None + # If this is the first time setting devpost_link, set devpost_link_submitted date if "devpost_link" in json and "devpost_link" not in team_data: update_data["devpost_link_submitted"] = datetime.now().isoformat() diff --git a/api/teams/tests/test_edit_team_project_fields.py b/api/teams/tests/test_edit_team_project_fields.py new file mode 100644 index 0000000..ea37ad4 --- /dev/null +++ b/api/teams/tests/test_edit_team_project_fields.py @@ -0,0 +1,95 @@ +""" +Regression tests for MEDIUM finding #3: PATCH /api/team/edit +(api.teams.teams_service.edit_team) is documented (README "Ownership", +CLAUDE.md, submissions_service.py) as the admin override path for a team's +project_* write-up fields, but edit_team's field_mappings never actually +carried them. Covers: all seven project_* fields now flow through +field_mappings, project_submission_status is validated against the +draft/submitted/late catalog (400 on a bad value, no write), tagline/story +get the same sanitize_markdown treatment as the self-serve save_project +path, and a status CHANGE stamps project_updated_at (no stamp when +unchanged). +""" +import os +from unittest.mock import MagicMock + +os.environ.setdefault("ENVIRONMENT", "test") + +import api.teams.teams_service as svc + + +def _wire(monkeypatch, existing_data): + mock_snapshot = MagicMock() + mock_snapshot.to_dict.return_value = dict(existing_data) if existing_data is not None else None + mock_doc = MagicMock() + mock_doc.get.return_value = mock_snapshot + mock_collection = MagicMock() + mock_collection.document.return_value = mock_doc + mock_db = MagicMock() + mock_db.collection.return_value = mock_collection + monkeypatch.setattr(svc, "get_db", lambda: mock_db) + monkeypatch.setattr(svc, "clear_cache", lambda: None) + monkeypatch.setattr(svc, "send_slack_audit", lambda **kwargs: None) + return mock_doc + + +def test_edit_team_persists_project_submission_status(monkeypatch): + mock_doc = _wire(monkeypatch, {"name": "Team A", "project_submission_status": "draft"}) + + result = svc.edit_team({"id": "team-1", "project_submission_status": "submitted"}) + + assert result["success"] is True + written = mock_doc.set.call_args[0][0] + assert written["project_submission_status"] == "submitted" + assert "project_updated_at" in written + + +def test_edit_team_rejects_invalid_submission_status(monkeypatch): + mock_doc = _wire(monkeypatch, {"name": "Team A"}) + + result, status = svc.edit_team({"id": "team-1", "project_submission_status": "bogus"}) + + assert status == 400 + assert result["success"] is False + mock_doc.set.assert_not_called() + + +def test_edit_team_does_not_stamp_project_updated_at_when_status_unchanged(monkeypatch): + mock_doc = _wire(monkeypatch, {"name": "Team A", "project_submission_status": "submitted"}) + + svc.edit_team({"id": "team-1", "project_submission_status": "submitted"}) + + written = mock_doc.set.call_args[0][0] + assert "project_updated_at" not in written + + +def test_edit_team_sanitizes_project_tagline_and_story(monkeypatch): + mock_doc = _wire(monkeypatch, {"name": "Team A"}) + + svc.edit_team({ + "id": "team-1", + "project_tagline": "Hi ", + "project_story": "We used List for the queue.", + }) + + written = mock_doc.set.call_args[0][0] + assert "" in written["project_story"] + + +def test_edit_team_persists_remaining_project_fields(monkeypatch): + mock_doc = _wire(monkeypatch, {"name": "Team A"}) + + svc.edit_team({ + "id": "team-1", + "project_built_with": ["Python", "React"], + "project_links": [{"label": "Repo", "url": "https://github.com/x/y"}], + "project_thumbnail_url": "https://cdn.ohack.dev/teams/team-1/project/thumb.png", + "project_images": ["https://cdn.ohack.dev/teams/team-1/project/1.png"], + }) + + written = mock_doc.set.call_args[0][0] + assert written["project_built_with"] == ["Python", "React"] + assert written["project_links"] == [{"label": "Repo", "url": "https://github.com/x/y"}] + assert written["project_thumbnail_url"] == "https://cdn.ohack.dev/teams/team-1/project/thumb.png" + assert written["project_images"] == ["https://cdn.ohack.dev/teams/team-1/project/1.png"] diff --git a/common/utils/validators.py b/common/utils/validators.py index 3b58e3d..7aa6950 100644 --- a/common/utils/validators.py +++ b/common/utils/validators.py @@ -123,9 +123,19 @@ def validate_https_url(url, max_length=2048): # only these specific tag names are removed. _MARKDOWN_STRIP_TAG_NAMES = r"script|iframe|object|embed|style|link|meta|form|base" _MARKDOWN_TAG_RE = re.compile(rf"]*>", re.IGNORECASE) -_MARKDOWN_ON_ATTR_RE = re.compile(r"""\son\w+\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]+)""", re.IGNORECASE) +# `[\s/]` (not just `\s`) so an attribute glued directly to a self-closing +# slash — ``, no space before "onerror" — still matches. +_MARKDOWN_ON_ATTR_RE = re.compile(r"""[\s/]on\w+\s*=\s*(?:"[^"]*"|'[^']*'|[^\s>]+)""", re.IGNORECASE) +# Quoted OR unquoted attribute values: `href="javascript:..."` and the +# unquoted `href=javascript:...` (no quotes at all) both get neutralized. _MARKDOWN_DANGEROUS_HREF_RE = re.compile( - r"""(href|src)\s*=\s*("|')\s*(?:javascript|vbscript|data):[^"']*\2""", re.IGNORECASE + r"""(href|src)\s*=\s*(?:(["'])\s*(?:javascript|vbscript|data):[^"']*\2|(?:javascript|vbscript|data):[^\s>]*)""", + re.IGNORECASE, +) +# Markdown link/image syntax `[text](javascript:...)` / `![alt](data:...)` — +# not an HTML attribute, so _MARKDOWN_DANGEROUS_HREF_RE never sees it. +_MARKDOWN_DANGEROUS_MD_LINK_RE = re.compile( + r"""\]\(\s*(?:javascript|vbscript|data):[^)]*\)""", re.IGNORECASE ) _CONTROL_CHARS_RE = re.compile(r"[\x00-\x08\x0b\x0c\x0e-\x1f\x7f]") @@ -137,10 +147,16 @@ def sanitize_markdown(text, max_length): HTML is already inert on read. This still strips a denylist of dangerous tags/attributes server-side in case the content is ever rendered elsewhere: script|iframe|object|embed|style|link|meta|form|base tags - (open and close), `on*=` attributes, and javascript:/vbscript:/data: - link or image targets (rewritten to "#"). Generic `<` (e.g. "List") - is preserved. Control characters (except \\n and \\t) are stripped and the - result is NFC-normalized, then truncated to max_length. + (open and close), `on*=` attributes (whitespace- or slash-preceded), + javascript:/vbscript:/data: link or image targets in HTML attributes + (quoted or unquoted) rewritten to "#", and the same targets in markdown + link/image syntax (`[text](javascript:...)`) rewritten to `](#)`. + Generic `<` (e.g. "List", "Map") is preserved. The tag-strip + pass is looped to a fixpoint so a nested bypass like + "ipt>" — where stripping the inner "