diff --git a/CLAUDE.md b/CLAUDE.md index 9278599..db509fa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -126,6 +126,9 @@ These are DIFFERENT VALUES. When bundling user data for the frontend, include bo ## Volunteer applications (`volunteers` collection) — staff-owned fields `create_or_update_volunteer` (`services/volunteers_service.py`) has exactly ONE production caller: `handle_submit` (`api/volunteers/volunteers_views.py`), backing only the self-service `/api/{mentor,judge,hacker,volunteer,sponsor}/application//{submit,update}` routes. It still persists the whole `volunteer_data` dict (no allowlist — new form fields flow through for free) **except** `STAFF_OWNED_VOLUNTEER_FIELDS`, which is stripped from the payload once, before the create/update branch. That covers both paths: on update `set(merge=True)` omits the keys so stored values survive; on create it stops a payload from overriding the `isSelected: False` seed to self-approve. The set covers approval (`isSelected`), check-in (`isCheckedIn`/`checkedIn`/`checkInTime`/`checkInTimeList`/`checkOutTime`/`checkoutTimeList`), refund bookkeeping (`deposit_status`, `deposit_refund_*`), `certificates` and `sent_emails`. **Deliberately NOT in the set:** `stripe_payment_intent_id`/`deposit_amount_cents`/`deposit_disposition` — the hacker Stripe Checkout return legitimately writes those via `/update`; adding them breaks deposits. The bug that motivated this (Aug 2026): all five frontend forms shipped `isSelected: false` from their `initialFormData` (sponsor hardcoded it), and the old guard only preserved the stored flag when the key was *absent* — so every application edit silently un-approved an approved mentor/judge/volunteer/sponsor. The same strip also closes a **privilege-escalation hole that was reachable**: before this, ANY logged-in user could POST `isSelected: true` to `/submit` and land an already-approved mentor or judge doc (the create path seeds `isSelected: False` then does `volunteer_doc.update(volunteer_data)`), granting `MentorTeamPanel` write access via `user_is_mentor_for_event` and survey-trust via `get_user_event_roles`. The submit/update routes were also `@auth.optional_user` and `handle_submit` had an `elif 'user_id' in volunteer_data` identity fallback — but the *anonymous* variant was NOT actually exploitable: `send_slack_audit` interpolates `user.user_id` before that fallback, and PropelAuth's `LoggedOutUser` has no `user_id`, so an unauthenticated POST raised AttributeError → caught → 400. That's an accident, not a control, so the routes are now `@auth.require_user` and `handle_submit` takes identity from the token ONLY — never a body `user_id`. Approval changes only via `update_volunteer_selection` (`POST /api/admin/volunteer//select`). Regression tests: `api/volunteers/tests/test_volunteers_service.py`. **Identity resolution is shared:** `find_volunteer_by_caller_identity(propel_user_id, event_id, volunteer_type)` (`services/volunteers_service.py`) is THE 3-way resolver — propel UUID → PropelAuth email → OAuth `user_id` — used by `handle_get` (the GET route), `create_or_update_volunteer` (the write path), and `api/mentors/mentors_service.py::_find_mentor_volunteer` (delegates). Keeping read and write on the same resolver is load-bearing: when the write path matched propel UUID only, a user whose doc was stored under another identity shape saw their app on read, edited it, missed the write lookup, and fell into the CREATE branch — spawning a duplicate `isSelected: False` doc and orphaning the approved one. The email step uses the **verified PropelAuth email only** — never the form-payload email, which would let a caller hijack someone else's application by typing their address. Don't add a fourth copy of this lookup; delegate. (Surveys' `get_user_event_roles` is intentionally separate — it scans all volunteer_types in one pass.) **Notification gate:** `_notifications_disabled()` in `volunteers_service.py` suppresses the Slack/Resend fan-out (`send_admin_notification_email`, `send_slack_volunteer_notification`, `send_volunteer_confirmation_email`, `send_mentor_checkin_notification`) when `ENVIRONMENT=test` — before this, unit tests exercising `create_or_update_volunteer` posted REAL Slack messages and attempted REAL Resend sends. Mirror this gate on any new outbound-notification function in this service. +### Confirmation-email calendar attachments (availability parser) +`get_calendar_email_attachment_from_availability` (`services/volunteers_service.py`) runs on every application submit/update to build `.ics` attachments from `availability`. It only understands the machine-generated slot format the mentor/volunteer forms emit (`"Sunday, Oct 12: ☀️ Morning (9am - 12pm PST)"`). The **judge form's `availability` is a free-text field**, so the function has an early guard: if the string contains no `Weekday, Mon D` prefix it logs INFO and returns `[]`. Don't remove the guard or re-raise its logging to ERROR — that was the Aug 2026 Sentry noise ("CRITICAL: All patterns failed to match slot") firing on every judge application with availability text. Genuine structured-parse failures log a single WARNING per slot; the per-pattern cascade logs are DEBUG. Tests: `api/volunteers/tests/test_volunteers_service.py` (free-text skip + structured regression). + ## Volunteer time tracking (`/api/users/volunteering`) GET/POST in `api/users/users_views.py` → `services/users_service.py`. Both resolve identity through `_resolve_and_ensure_user(propel_id)`, in this order so a broken OAuth provider token can NEVER block volunteering: **(1) `fetch_user_by_propel_id(propel_id)` — direct Firestore lookup on the stored `propel_id` field, NO external call (covers everyone who has saved a profile); (2) the OAuth provider round-trip (`get_oauth_user_from_propel_user_id` → `sub` → `fetch_user_by_user_id`), the best source for the OAuth-format `user_id` + avatar, lazily creating a doc for new users; (3) the PropelAuth user-metadata fallback (`_fetch_propel_metadata` → `auth.fetch_user_metadata_by_user_id`) — RELIABLE, does NOT depend on the provider token — which resolves an existing doc by email (backfilling `propel_id`) or lazily creates one from the metadata (`user_id` set to the propel UUID since we lack the oauth-format id without the provider call; `propel_id` is the canonical match so step 1 hits forever after).** The bug this fixes: the WRITE used to depend SOLELY on step 2; when `get_oauth_user_from_propel_user_id` returns None (expired/unavailable provider token, PropelAuth hiccup, or its 5-min negative cache) the write 404'd ("Couldn't log that time") while the read masked it by returning empty. **Critical:** `get_profile_metadata` (which creates the doc) ALSO depends on the OAuth round-trip, so a user whose OAuth has always failed may have NO doc at all — step 3 (metadata) is what resolves/creates them. `fetch_user_by_propel_id`/`fetch_user_by_email` live in `db/{db,firestore,mem}.py` (single-field equality queries — auto-indexed, no composite index). **Logging:** `get_oauth_user_from_propel_user_id` now logs the PropelAuth response BODY (truncated) on non-200 and a debug line when serving a cached miss — previously the root cause (e.g. "no linked OAuth connection", wrong `PROPEL_AUTH_URL`/`KEY`) was invisible during a tight retry window. Tests: `api/users/tests/test_volunteer_resolve.py` (6 cases). NOTE — date/locale is NOT a factor: `` always yields an ISO `yyyy-MM-dd` value regardless of the user's locale. `get_volunteering_time` now returns `([], 0, 0)` (never None/404) so the page shows a clean zero-state, and filters in a SINGLE pass — an entry may carry `commitmentHours`, `finalHours`, or BOTH (manual logs send both), no concat/duplicate. `save_volunteering_time` accepts an optional `timestamp` (backdated manual logs) + `manual:true` flag; hours are float-cleaned, non-negative, capped at 1000. diff --git a/api/volunteers/tests/test_volunteers_service.py b/api/volunteers/tests/test_volunteers_service.py index c2a66d2..dcb9452 100644 --- a/api/volunteers/tests/test_volunteers_service.py +++ b/api/volunteers/tests/test_volunteers_service.py @@ -1,9 +1,12 @@ +import logging + import pytest from unittest.mock import patch, MagicMock from typing import Dict, Any from services.volunteers_service import ( create_or_update_volunteer, + get_calendar_email_attachment_from_availability, get_volunteer_by_user_id, get_volunteers_by_event, get_user_hackathon_attendance, @@ -502,6 +505,47 @@ def collection_router(name): assert sorted(entry['roles']) == ['Hacker', 'Mentor'] +# --------------------------------------------------------------------------- +# Calendar attachments from availability +# +# Mentor/volunteer forms send machine-generated slot strings; the judge form's +# availability is free text. Free text must be skipped quietly — it used to +# fire an ERROR ("All patterns failed to match slot") into Sentry on every +# judge submission that filled the field. +# --------------------------------------------------------------------------- + +def test_calendar_attachments_skip_free_text_availability(caplog): + free_text = ( + "I will be Present near ASU during Entire November month for Client " + "visit and will be available all month" + ) + with caplog.at_level(logging.DEBUG): + result = get_calendar_email_attachment_from_availability( + free_text, "judge@example.com", volunteer_type="judge" + ) + + assert result == [] + errors = [r for r in caplog.records if r.levelno >= logging.ERROR] + assert not errors, f"free-text availability must not log errors: {errors}" + + +def test_calendar_attachments_parse_structured_availability(): + structured = ( + "Saturday, Oct 11: ☀️ Morning (9am - 12pm PST), " + "Sunday, Oct 12: 🏙️ Afternoon (1pm - 3pm PST)" + ) + result = get_calendar_email_attachment_from_availability( + structured, "mentor@example.com", volunteer_type="mentor", year=2026 + ) + + assert len(result) == 2 + for attachment in result: + assert attachment["type"] == "text/calendar" + assert attachment["filename"].endswith(".ics") + assert "BEGIN:VCALENDAR" in attachment["content"] + assert "ATTENDEE:MAILTO:mentor@example.com" in attachment["content"] + + def test_generate_qr_code(): """Test QR code generation functionality.""" test_content = "https://www.ohack.dev/test-link" diff --git a/services/volunteers_service.py b/services/volunteers_service.py index 634dd4e..0518f84 100644 --- a/services/volunteers_service.py +++ b/services/volunteers_service.py @@ -648,7 +648,14 @@ def get_calendar_email_attachment_from_availability( if not availability: warning(logger, "No availability provided, skipping calendar generation") return [] - + + # Structured slots always start "Weekday, Mon D" — the prefix all 3 parse + # patterns require. Judge applications send free-text availability, which + # is expected input here, not an error. + if not re.search(r'[A-Za-z]+day,\s*[A-Za-z]{3,}\s+\d{1,2}', availability): + info(logger, "Availability is free text (no structured slots); skipping calendar attachments") + return [] + # Use current year if not specified if not year: info(logger, "No year provided, using current year", year=datetime.now().year) @@ -714,7 +721,7 @@ def get_calendar_email_attachment_from_availability( time_name=time_emoji_name, time_range=time_range) else: - warning(logger, "FAILED: Pattern 1 did not match", pattern=pattern1, slot=slot.strip()) + debug(logger, "Pattern 1 did not match", pattern=pattern1, slot=slot.strip()) # Pattern 2: "Sunday, Oct 12-Afternoon" (fallback format) pattern2 = r'([A-Za-z]+),\s*([A-Za-z]+)\s+(\d+)[-\s]([A-Za-z\s]+)' @@ -739,7 +746,7 @@ def get_calendar_email_attachment_from_availability( time_name=time_emoji_name, original_time_name=time_name) else: - warning(logger, "FAILED: Pattern 2 did not match", pattern=pattern2, slot=slot.strip()) + debug(logger, "Pattern 2 did not match", pattern=pattern2, slot=slot.strip()) # Pattern 3: More flexible - just capture everything after the colon pattern3 = r'([A-Za-z]+),\s*([A-Za-z]+)\s+(\d+):\s*(.+)' @@ -772,14 +779,9 @@ def get_calendar_email_attachment_from_availability( day=day_str, time_name=time_emoji_name) else: - error(logger, "CRITICAL: All patterns failed to match slot", - slot=slot.strip(), - pattern1=pattern1, - pattern2=pattern2, - pattern3=pattern3, - slot_length=len(slot.strip()), - slot_chars=[ord(c) for c in slot.strip()[:50]] # Show character codes for debugging - ) + warning(logger, "All patterns failed to match slot", + slot=slot.strip(), + slot_length=len(slot.strip())) continue try: @@ -1073,7 +1075,7 @@ def create_or_update_volunteer( if calendar_attachments and len(calendar_attachments) > 0: info(logger, "Sending email with calendar attachments", email=email, attachment_count=len(calendar_attachments)) else: - warning(logger, "No calendar attachments generated despite availability data", email=email) + info(logger, "No calendar attachments generated from availability data", email=email) send_volunteer_confirmation_email(first_name, last_name, email, volunteer_type, calendar_attachments, event_id, volunteer_id=volunteer_id) @@ -1146,7 +1148,7 @@ def create_or_update_volunteer( if calendar_attachments and len(calendar_attachments) > 0: info(logger, "Sending email with calendar attachments", email=email, attachment_count=len(calendar_attachments)) else: - warning(logger, "No calendar attachments generated despite availability data", email=email) + info(logger, "No calendar attachments generated from availability data", email=email) send_volunteer_confirmation_email(first_name, last_name, email, volunteer_type, calendar_attachments, event_id, volunteer_id=volunteer_id) send_admin_notification_email(volunteer_doc)