diff --git a/CLAUDE.md b/CLAUDE.md index 6b2b500..3252a58 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -100,6 +100,9 @@ Two options: 1. **Sort in Python after a single-field where** (preferred when the result set is small): `sorted([... for d in coll.where(...).stream()], key=lambda x: x["pos"])`. No index needed because single-field equality is auto-indexed. 2. **Add the composite index to `firestore.indexes.json`** AND deploy. Don't forget the deploy step — committing to the repo doesn't apply it. +### `set(..., merge=True)` DEEP-merges map fields — removing a nested key needs DELETE_FIELD +`ref.set({"some_map": {...}}, merge=True)` does NOT replace `some_map` — it recursively merges, so a key you dropped from the Python dict stays in Firestore. To delete a nested key you must write `firestore.DELETE_FIELD` at that exact path: `ref.set({"some_map": {key: firestore.DELETE_FIELD}}, merge=True)`. MockFirestore (test/`ENVIRONMENT=test`) REPLACES maps instead of deep-merging, so this passes locally and only breaks in prod. This caused the "mentor coverage cleared but stays checked" bug — `toggle_mentor_coverage` (`api/mentors/mentors_service.py`) popped the slug then wrote the dict, which never removed it. Pattern to copy is there now: write only the changed nested keys, DELETE_FIELD to remove. Mixing DELETE_FIELD sentinels and real values in the same nested map is allowed; DELETE_FIELD on a non-existent path is a no-op. + ### Lazy user profile creation in the `users` collection A user authenticated via PropelAuth may NOT exist in the Firestore `users` collection. The collection is populated lazily — only when someone hits `GET /api/users/profile` or saves profile metadata. Never assume `fetch_users()` includes everyone with a `propel_user_id` referenced elsewhere (assignees, editors, mentions, etc.). diff --git a/api/mentors/mentors_service.py b/api/mentors/mentors_service.py index bc2c963..d25e1e1 100644 --- a/api/mentors/mentors_service.py +++ b/api/mentors/mentors_service.py @@ -17,8 +17,10 @@ from datetime import datetime from typing import Optional, Tuple +from firebase_admin import firestore + from db.db import get_db -from common.utils.firestore_helpers import clear_all_caches as clear_cache +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 services.users_service import get_propel_user_details_by_id @@ -27,6 +29,25 @@ logger = logging.getLogger("myapp") + +def clear_cache(): + """Bust the team/doc caches (so get_team — and this endpoint's own + _build_response — is fresh) AND the hackathon event/list caches. + + Mentor writes change fields the CACHED event page (#teams) renders via + get_single_hackathon_event: coverage X/6, mentor_open_flag_count, the judging + consensus dots, last-touched. clear_all_caches() only clears the *registered* + caches (e.g. teams' _GET_TEAM_CACHE), NOT get_single_hackathon_event's cache, + so without this the event page lagged up to 10 min behind the always-fresh + team page. Mirrors teams_service._clear_cache(). 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("mentors clear_cache: hackathon cache clear failed: %s", e) + # Canonical 6-item team-coverage list. Slugs MUST stay in lockstep with the # frontend MENTOR_COVERAGE_ITEMS in src/components/Teams/mentorCoverage.js. MENTOR_COVERAGE_ITEMS = [ @@ -45,6 +66,13 @@ ] MENTOR_COVERAGE_SLUGS = {item["slug"]: item for item in MENTOR_COVERAGE_ITEMS} +# Each coverage item wants independent sign-off from this many distinct mentors +# (capped here too — we don't want to pester a team with more than this). An item +# only counts toward the X/6 coverage total + the "fully covered" milestone once +# it reaches this many checks. Keep in lockstep with the frontend +# COVERAGE_TARGET_MENTORS in src/components/Teams/mentorCoverage.js. +COVERAGE_TARGET_MENTORS = 3 + ALLOWED_FLAG_SEVERITIES = {"needs_attention", "blocked"} # Five judging criteria. The first four are the main 10-pt rubric on # /about/judges; "accessibility" is the special-category prize on the same @@ -183,10 +211,32 @@ def _open_flag_count(flags): return sum(1 for f in flags if not f.get("resolved_at")) +def _coverage_checks(entry): + """Per-mentor checks map for one coverage item: {propel_id: {name, checked_at}}. + + Tolerates the legacy single-mentor shape ({done, checked_by_propel_id, + checked_by_name, checked_at}) by surfacing it as one check, so existing data + keeps counting until a mentor next touches the item (which migrates it).""" + if not isinstance(entry, dict): + return {} + checks = entry.get("checks") + if isinstance(checks, dict): + return checks + if entry.get("done"): + pid = entry.get("checked_by_propel_id") or "_legacy" + return {pid: {"name": entry.get("checked_by_name"), + "checked_at": entry.get("checked_at")}} + return {} + + def _coverage_done_count(checklist): + """Count items that have reached COVERAGE_TARGET_MENTORS distinct checks.""" if not isinstance(checklist, dict): return 0 - return sum(1 for slug in MENTOR_COVERAGE_SLUGS if checklist.get(slug, {}).get("done")) + return sum( + 1 for slug in MENTOR_COVERAGE_SLUGS + if len(_coverage_checks(checklist.get(slug))) >= COVERAGE_TARGET_MENTORS + ) def _mentor_channel_for_event(event_id): @@ -224,16 +274,18 @@ def _build_response(team_id, **extra): def toggle_mentor_coverage(propel_user_id, team_id, item_slug, done, note=None): """ - Mark/unmark a coverage item. Mentor coverage is reversible (situations - evolve mid-event), unlike the team-completion checklist. Quiet by - default — only the first time a team hits 6/6 does a Slack message fire. + Record (done=True) or clear (done=False) the CALLING mentor's own check on a + coverage item. Each item collects independent sign-off from up to + COVERAGE_TARGET_MENTORS distinct mentors — a mentor can only add or clear their + own check, never another mentor's. An item counts toward the X/6 total (and the + one-time "fully covered" Slack milestone) once it reaches the target. Quiet by + default — only the first time every item is fully covered does Slack fire. """ if item_slug not in MENTOR_COVERAGE_SLUGS: return {"error": f"Unknown coverage item: {item_slug}"}, 400 if not isinstance(done, bool): return {"error": "Field 'done' must be a boolean"}, 400 - event_id_guess = None ref, team_data, _ = _team_doc_or_404(team_id) if ref is None: return {"error": "Team not found"}, 404 @@ -244,24 +296,42 @@ def toggle_mentor_coverage(propel_user_id, team_id, item_slug, done, note=None): name = _caller_attribution(propel_user_id) now_iso = datetime.now().isoformat() checklist = dict(team_data.get("mentor_checklist") or {}) + existing_entry = checklist.get(item_slug) + checks = dict(_coverage_checks(existing_entry)) # propel_id -> {name, checked_at} + is_legacy = isinstance(existing_entry, dict) and "checks" not in existing_entry + + # set(merge=True) deep-merges map fields, so we write ONLY the nested keys that + # change. Removing my check means a DELETE_FIELD sentinel at my propel_id path + # (popping the key wouldn't persist). Legacy single-mentor entries are migrated + # into the `checks` map — and their top-level keys deleted — on first touch. + entry_write = {"checks": {}} + if is_legacy and isinstance(existing_entry, dict): + for k in ("done", "checked_at", "checked_by_propel_id", "checked_by_name", "note"): + if k in existing_entry: + entry_write[k] = firestore.DELETE_FIELD + for pid, val in checks.items(): # preserve any OTHER legacy checker + if pid != propel_user_id: + entry_write["checks"][pid] = val + if done: - new_entry = { - "done": True, - "checked_at": now_iso, - "checked_by_propel_id": propel_user_id, - "checked_by_name": name, - } + if propel_user_id not in checks and len(checks) >= COVERAGE_TARGET_MENTORS: + return ({"error": f"This item already has {COVERAGE_TARGET_MENTORS} mentor " + "checks — that's plenty, no need to add another."}, 409) + my_check = {"name": name, "checked_at": now_iso} if note: - new_entry["note"] = str(note)[:MAX_RATING_NOTE_LEN] - checklist[item_slug] = new_entry + my_check["note"] = str(note)[:MAX_RATING_NOTE_LEN] + checks[propel_user_id] = my_check + entry_write["checks"][propel_user_id] = my_check else: - # Unchecking: drop the entry entirely (the absence is the "not done" state). - checklist.pop(item_slug, None) + checks.pop(propel_user_id, None) + entry_write["checks"][propel_user_id] = firestore.DELETE_FIELD - new_count = sum(1 for s in MENTOR_COVERAGE_SLUGS if checklist.get(s, {}).get("done")) + # Reflect the change in our in-memory copy for counting + milestone. + checklist[item_slug] = {"checks": checks} + new_count = _coverage_done_count(checklist) total = len(MENTOR_COVERAGE_ITEMS) - update = {"mentor_checklist": checklist} + update = {"mentor_checklist": {item_slug: entry_write}} _touch(update, name, now_iso) # First-time-complete milestone: stamp it once so we never re-broadcast. @@ -280,8 +350,8 @@ def toggle_mentor_coverage(propel_user_id, team_id, item_slug, done, note=None): send_slack( message=( f":sparkles: *{team_data.get('name', 'This team')}* has full mentor coverage " - f"({total}/{total} items)! Nice work team & mentors. " - f"Final coverage marker: *{name}*." + f"— all {total} items signed off by {COVERAGE_TARGET_MENTORS} mentors each! " + f"Nice work team & mentors. Final coverage marker: *{name}*." ), channel=slack_channel, ) diff --git a/services/teams_service.py b/services/teams_service.py index aca6c6c..711d304 100644 --- a/services/teams_service.py +++ b/services/teams_service.py @@ -10,7 +10,7 @@ from common.utils.slack import send_slack_audit, send_slack, create_slack_channel, invite_user_to_channel from common.utils.github import create_github_repo, validate_github_username, get_all_repos from common.utils.firebase import get_hackathon_by_event_id -from common.utils.oauth_providers import extract_slack_user_id +from common.utils.oauth_providers import extract_slack_user_id, is_slack_user_id from db.db import get_db, get_user_doc_reference from services.users_service import ( get_propel_user_details_by_id, @@ -461,9 +461,15 @@ def update_team_and_user(transaction): send_slack_audit(action="join_team", message="Added", payload=json) message = "Joined Team" if team_slack_channel and slack_user_id: - invite_user_to_channel(slack_user_id, team_slack_channel) - # Send a simple message that pings the user in the channel to let them know they were added - send_slack(f"<@{slack_member_id}> has joined the team!", team_slack_channel) + if is_slack_user_id(slack_user_id): + invite_user_to_channel(slack_user_id, team_slack_channel) + # Send a simple message that pings the user in the channel to let them know they were added + send_slack(f"<@{slack_member_id}> has joined the team!", team_slack_channel) + else: + # Non-Slack login (e.g. Google) has no Slack ID to @-mention or invite — + # use the display name so we don't post a mangled "@oauth2" mention. + display_name = (user.name or slack_user.get("name") or "A member") + send_slack(f"{display_name} has joined the team!", team_slack_channel) else: message = "User was already in the team" except Exception as e: