Address intake review follow-ups

Squash merge PR #346.

Author: michaelmwu

apps/api/src/five08/backend/api.py

@@ -812,22 +812,24 @@ def _tally_intake_dry_run_mode(
     return "none"
 
 
-def _strip_url_query(value: str) -> str:
+def _strip_url_query_and_fragment(value: str) -> str:
     parsed = urlsplit(value)
-    if parsed.scheme not in {"http", "https"} or not parsed.netloc or not parsed.query:
+    if parsed.scheme not in {"http", "https"} or not parsed.netloc:
         return value
-    return urlunsplit((parsed.scheme, parsed.netloc, parsed.path, "", parsed.fragment))
+    if not parsed.query and not parsed.fragment:
+        return value
+    return urlunsplit((parsed.scheme, parsed.netloc, parsed.path, "", ""))
 
 
-def _sanitize_tally_raw_payload(value: Any) -> Any:
+def _sanitize_intake_raw_payload(value: Any) -> Any:
     if isinstance(value, Mapping):
         return {
-            str(key): _sanitize_tally_raw_payload(item) for key, item in value.items()
+            str(key): _sanitize_intake_raw_payload(item) for key, item in value.items()
         }
     if isinstance(value, list):
-        return [_sanitize_tally_raw_payload(item) for item in value]
+        return [_sanitize_intake_raw_payload(item) for item in value]
     if isinstance(value, str):
-        return _strip_url_query(value)
+        return _strip_url_query_and_fragment(value)
     return value
 
 
@@ -7701,6 +7703,7 @@ async def google_forms_intake_webhook_handler(request: Request) -> JSONResponse:
         submitted_at=payload.submitted_at,
         payload=normalized_payload,
     )
+    normalized_payload["raw_payload"] = _sanitize_intake_raw_payload(payload_data)
 
     queue = request.app.state.queue
     try:
@@ -7789,8 +7792,8 @@ async def tally_intake_webhook_handler(request: Request) -> JSONResponse:
             "email": email,
             "first_name": first_name,
             "last_name": last_name,
-            "raw_payload": _sanitize_tally_raw_payload(payload_data),
-            "raw_tally_fields": _sanitize_tally_raw_payload(raw_tally_fields),
+            "raw_payload": _sanitize_intake_raw_payload(payload_data),
+            "raw_tally_fields": _sanitize_intake_raw_payload(raw_tally_fields),
         }
     )
 

apps/worker/src/five08/worker/crm/intake_form_processor.py

@@ -14,7 +14,7 @@
 from dataclasses import dataclass
 from datetime import datetime, timezone
 from pathlib import Path
-from typing import Any
+from typing import Any, cast
 from urllib.parse import urljoin, urlsplit, urlunsplit
 from uuid import NAMESPACE_URL, uuid4, uuid5
 
@@ -94,6 +94,13 @@ class IntakeResumeFile:
     source_url: str
 
 
+class _ResumeFileNotProvided:
+    pass
+
+
+_RESUME_FILE_NOT_PROVIDED = _ResumeFileNotProvided()
+
+
 class IntakeFormProcessor:
     """Process a Google Forms member intake submission against CRM."""
 
@@ -464,7 +471,9 @@ def _build_intake_updates(
         payload: Mapping[str, Any],
         include_email: bool = True,
         include_last_name: bool = True,
-        resume_file: IntakeResumeFile | None = None,
+        resume_file: IntakeResumeFile | None | _ResumeFileNotProvided = (
+            _RESUME_FILE_NOT_PROVIDED
+        ),
     ) -> dict[str, Any]:
         updates: dict[str, Any] = {"firstName": first_name}
         if include_last_name:
@@ -698,22 +707,27 @@ def _build_resume_updates(
         self,
         payload: Mapping[str, Any],
         *,
-        resume_file: IntakeResumeFile | None = None,
+        resume_file: IntakeResumeFile | None | _ResumeFileNotProvided = (
+            _RESUME_FILE_NOT_PROVIDED
+        ),
     ) -> dict[str, Any]:
-        if resume_file is None:
-            resume_file = self._prepare_resume_file(payload)
-        if resume_file is None:
+        prepared_resume_file: IntakeResumeFile | None
+        if resume_file is _RESUME_FILE_NOT_PROVIDED:
+            prepared_resume_file = self._prepare_resume_file(payload)
+        else:
+            prepared_resume_file = cast(IntakeResumeFile | None, resume_file)
+        if prepared_resume_file is None:
             return {}
 
         try:
             resume_text = self.document_processor.extract_text(
-                resume_file.content,
-                resume_file.filename,
+                prepared_resume_file.content,
+                prepared_resume_file.filename,
             )
         except Exception as exc:
             logger.warning(
                 "Failed to parse resume masked_url=%s error=%s",
-                self._mask_resume_url_for_log(resume_file.source_url),
+                self._mask_resume_url_for_log(prepared_resume_file.source_url),
                 exc,
             )
             return {}

tests/unit/test_backend_api.py

@@ -9286,6 +9286,10 @@ def test_google_forms_intake_enqueues_job(
                     "first_name": "  Jane  ",
                     "last_name": "  Doe  ",
                     "form_id": "form-1",
+                    "resume_url": (
+                        "https://drive.google.com/resume.pdf?signature=secret"
+                        "#token=fragment-secret"
+                    ),
                 },
                 headers=auth_headers,
             )
@@ -9302,6 +9306,17 @@ def test_google_forms_intake_enqueues_job(
     assert call_kwargs["args"][0]["email"] == "member@example.com"
     assert call_kwargs["args"][0]["first_name"] == "Jane"
     assert call_kwargs["args"][0]["last_name"] == "Doe"
+    assert call_kwargs["args"][0]["resume_url"] == (
+        "https://drive.google.com/resume.pdf?signature=secret#token=fragment-secret"
+    )
+    assert call_kwargs["args"][0]["raw_payload"] == {
+        **_GOOGLE_FORMS_INTAKE_PAYLOAD,
+        "email": "  member@example.com  ",
+        "first_name": "  Jane  ",
+        "last_name": "  Doe  ",
+        "form_id": "form-1",
+        "resume_url": "https://drive.google.com/resume.pdf",
+    }
 
 
 def test_google_forms_intake_rejects_unapproved_form_id(
@@ -9637,15 +9652,16 @@ def test_tally_intake_strips_signed_urls_from_raw_payload_before_enqueue(
     """Queued raw Tally payloads should not retain signed URL query tokens."""
     tally_payload = json.loads(json.dumps(_TALLY_INTAKE_PAYLOAD))
     tally_payload["data"]["submissionPdfUrl"] = (
-        "https://tally.so/r/abc.pdf?accessToken=secret&signature=sig"
+        "https://tally.so/r/abc.pdf?accessToken=secret&signature=sig#token=frag"
     )
     tally_payload["data"]["submissionPreviewUrl"] = (
-        "https://tally.so/r/abc?accessToken=secret&signature=sig"
+        "https://tally.so/r/abc?accessToken=secret&signature=sig#token=frag"
     )
     for field in tally_payload["data"]["fields"]:
         if field["key"] == "question_resume":
             field["value"][0]["url"] = (
                 "https://storage.googleapis.com/tally/resume.pdf?signature=sig"
+                "#token=frag"
             )
 
     with (
@@ -9664,7 +9680,7 @@ def test_tally_intake_strips_signed_urls_from_raw_payload_before_enqueue(
     intake_payload = mock_enqueue.call_args.kwargs["args"][0]
     assert (
         intake_payload["resume_url"]
-        == "https://storage.googleapis.com/tally/resume.pdf?signature=sig"
+        == "https://storage.googleapis.com/tally/resume.pdf?signature=sig#token=frag"
     )
     raw_payload = intake_payload["raw_payload"]
     assert raw_payload["data"]["submissionPdfUrl"] == "https://tally.so/r/abc.pdf"

tests/unit/test_intake_form_processor.py

@@ -136,6 +136,33 @@ def test_intake_form_processor_dry_run_create_reports_resume_upload_plan() -> No
     mock_persist.assert_not_called()
 
 
+def test_create_prospect_does_not_retry_failed_resume_prepare() -> None:
+    """Create flow should not download/scan a failed resume more than once."""
+    processor = IntakeFormProcessor()
+    processor.api = MagicMock()
+    processor.api.request.side_effect = [
+        {"list": []},
+        {"id": "contact-1"},
+    ]
+
+    with (
+        patch.object(processor, "_prepare_resume_file", return_value=None) as prepare,
+        patch.object(processor, "_persist_intake_submission"),
+    ):
+        result = processor.process_intake(
+            payload={
+                "email": "new@example.com",
+                "first_name": "New",
+                "last_name": "Person",
+                "resume_url": "https://tally.so/resume.pdf",
+                "form_id": "form-1",
+            }
+        )
+
+    assert result["success"] is True
+    prepare.assert_called_once()
+
+
 def test_intake_form_processor_dry_run_update_does_not_write_crm_or_db() -> None:
     """Dry-run update should return planned updates without PUT or persistence."""
     processor = IntakeFormProcessor()
@@ -173,6 +200,34 @@ def test_intake_form_processor_dry_run_update_does_not_write_crm_or_db() -> None
     mock_persist.assert_not_called()
 
 
+def test_update_prospect_does_not_retry_failed_resume_prepare() -> None:
+    """Update flow should not download/scan a failed resume more than once."""
+    processor = IntakeFormProcessor()
+    processor.api = MagicMock()
+    processor.api.request.side_effect = [
+        {"list": [{"id": "contact-1", "type": "Prospect"}]},
+        {},
+    ]
+
+    with (
+        patch.object(processor, "_prepare_resume_file", return_value=None) as prepare,
+        patch.object(processor, "_persist_intake_submission"),
+    ):
+        result = processor.process_intake(
+            payload={
+                "email": "existing@example.com",
+                "first_name": "Existing",
+                "last_name": "Person",
+                "github_username": "existing-dev",
+                "resume_url": "https://tally.so/resume.pdf",
+                "form_id": "form-1",
+            }
+        )
+
+    assert result["success"] is True
+    prepare.assert_called_once()
+
+
 def test_intake_form_processor_uploads_resume_after_create() -> None:
     """Created prospects should receive the downloaded Tally resume attachment."""
     processor = IntakeFormProcessor()