From fee037a630a550d4b42dab31ee26ecb1e55094c5 Mon Sep 17 00:00:00 2001 From: Keysat Date: Mon, 15 Jun 2026 20:37:49 -0500 Subject: [PATCH] Apply review polish to the digest send path (post-v0.1.0:76) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Non-blocking items from the v76 reviewer pass. No redeploy needed — the box runs v76 and its happy path is unaffected; these ride the next build: - digest_mailer.send_digest: when Gmail is enabled but no sender resolves (CRM_DIGEST_SENDER unset and no admin email), raise NoTransport so the caller returns a clear 400 instead of a generic 502. - gmail_send.send_via_gmail: wrap OSError/URLError (timeout/DNS) as a RuntimeError ("Gmail API unreachable: ...") to match the HTTPError handling; include the sender in the HTTPError message for debuggability. - credentials.py: correct the now-stale GMAIL_COMPOSE_SCOPE comment (the digest mailer sends with this scope; only outreach drafts never send). - test_gmail_send.py: add the HTTPError->RuntimeError branch, default_sender DB fallback (+None case + env override), and the send_digest SMTP-tag path. 19/19 backend tests green. --- backend/digest_mailer.py | 5 ++ backend/email_integration/credentials.py | 6 ++- backend/email_integration/gmail_send.py | 6 ++- backend/test_gmail_send.py | 64 ++++++++++++++++++++++++ 4 files changed, 78 insertions(+), 3 deletions(-) diff --git a/backend/digest_mailer.py b/backend/digest_mailer.py index 5c456e4..69dc27f 100644 --- a/backend/digest_mailer.py +++ b/backend/digest_mailer.py @@ -52,6 +52,11 @@ def send_digest(conn, to_addrs, subject, body, sender_email=None): if t == "gmail-dwd": from email_integration import gmail_send sender = sender_email or default_sender(conn) + if not sender: + # Gmail IS available but we have nobody to send as — a config gap, not a + # send failure. Surface it as NoTransport so the caller returns a clear 400. + raise NoTransport("Gmail is enabled but no sender address is set: " + "set CRM_DIGEST_SENDER or give an active admin an email.") result = gmail_send.send_via_gmail(sender, to_addrs, subject, body, conn=conn) result["transport"] = "gmail-dwd" return result diff --git a/backend/email_integration/credentials.py b/backend/email_integration/credentials.py index 84dfc9e..cde95c4 100644 --- a/backend/email_integration/credentials.py +++ b/backend/email_integration/credentials.py @@ -32,8 +32,10 @@ from . import errors GMAIL_READONLY_SCOPE = "https://www.googleapis.com/auth/gmail.readonly" -# Drafts scope (authorized in Workspace DWD). We only ever CREATE drafts with it; the -# human sends from Gmail. (Google bundles send into this scope, but our code never sends.) +# Compose scope (authorized in Workspace DWD). Two consumers: outreach (compose.py) +# only CREATES drafts — the human sends from Gmail; the daily-digest mailer +# (gmail_send.py) uses this same scope to SEND, since gmail.compose authorizes +# users.messages.send. (The narrow gmail.send scope is NOT on the DWD grant.) GMAIL_COMPOSE_SCOPE = "https://www.googleapis.com/auth/gmail.compose" GOOGLE_TOKEN_URL = "https://oauth2.googleapis.com/token" diff --git a/backend/email_integration/gmail_send.py b/backend/email_integration/gmail_send.py index c7a7d53..544b7a8 100644 --- a/backend/email_integration/gmail_send.py +++ b/backend/email_integration/gmail_send.py @@ -48,6 +48,8 @@ def send_via_gmail(sender_email, to_addrs, subject, body, conn=None): if not sender_email: raise ValueError("no sender_email (DWD impersonation needs a domain user)") + # conn is only consulted by the OAuth provider path; the DWD provider (the one + # used here) reads the service-account key from disk and ignores it. provider = _creds.build_provider(lambda: conn) token = provider.access_token_for(sender_email, _creds.GMAIL_COMPOSE_SCOPE).token raw = _build_raw(sender_email, to_addrs, subject, body) @@ -61,5 +63,7 @@ def send_via_gmail(sender_email, to_addrs, subject, body, conn=None): result = json.loads(resp.read()) except urllib.error.HTTPError as e: detail = e.read().decode("utf-8", "replace")[:300] - raise RuntimeError(f"Gmail API send failed (HTTP {e.code}): {detail}") + raise RuntimeError(f"Gmail API send failed for {sender_email} (HTTP {e.code}): {detail}") + except OSError as e: # URLError/timeout/DNS (URLError subclasses OSError) + raise RuntimeError(f"Gmail API unreachable: {e}") return {"sent_to": to_addrs, "from": sender_email, "message_id": result.get("id")} diff --git a/backend/test_gmail_send.py b/backend/test_gmail_send.py index 8c28136..0761058 100644 --- a/backend/test_gmail_send.py +++ b/backend/test_gmail_send.py @@ -5,9 +5,11 @@ No network: the credential provider and urllib.request.urlopen are monkeypatched Run directly or via backend/run_tests.py. """ import base64 +import io import json import os import sys +import urllib.error import urllib.request sys.path.insert(0, os.path.dirname(os.path.abspath(__file__))) @@ -153,6 +155,68 @@ def main(): gmail_send.gmail_available = orig_avail smtp_send.smtp_configured = orig_smtp_cfg + # 7. HTTPError from Gmail is wrapped as RuntimeError (not raised raw) + orig_build = gmail_send._creds.build_provider + gmail_send._creds.build_provider = lambda factory: _Provider() + orig_open2 = urllib.request.urlopen + + def raise_http(req, timeout=None): + raise urllib.error.HTTPError(req.full_url, 403, "Forbidden", {}, + io.BytesIO(b'{"error":"denied"}')) + + urllib.request.urlopen = raise_http + try: + try: + gmail_send.send_via_gmail("g@x", ["a@x"], "s", "b"); ok = False + except RuntimeError as e: + ok = "403" in str(e) + except urllib.error.HTTPError: + ok = False # should have been wrapped, not raised raw + finally: + urllib.request.urlopen = orig_open2 + gmail_send._creds.build_provider = orig_build + check(ok, "Gmail HTTPError wrapped as RuntimeError carrying the status") + + # 8. default_sender: env wins; else DB first-admin; else None + class _FakeCur: + def __init__(self, row): + self._row = row + + def fetchone(self): + return self._row + + class _Conn: + def __init__(self, row): + self._row = row + + def execute(self, *a, **k): + return _FakeCur(self._row) + + os.environ.pop("CRM_DIGEST_SENDER", None) + check(digest_mailer.default_sender(_Conn({"email": "first@ten31.xyz"})) == "first@ten31.xyz", + "default_sender falls back to first active admin") + check(digest_mailer.default_sender(_Conn(None)) is None, + "default_sender returns None when no admin has an email") + os.environ["CRM_DIGEST_SENDER"] = "env@ten31.xyz" + check(digest_mailer.default_sender(_Conn({"email": "first@ten31.xyz"})) == "env@ten31.xyz", + "CRM_DIGEST_SENDER overrides the DB lookup") + os.environ.pop("CRM_DIGEST_SENDER", None) + + # 9. send_digest tags transport 'smtp' on the fallback path + orig_avail2 = gmail_send.gmail_available + orig_smtp_cfg2 = smtp_send.smtp_configured + orig_smtp_send = smtp_send.send_email + try: + gmail_send.gmail_available = lambda: False + smtp_send.smtp_configured = lambda: True + smtp_send.send_email = lambda to, subj, body, **k: {"sent_to": to, "from": "f@x"} + res = digest_mailer.send_digest(None, ["a@x"], "S", "B") + check(res["transport"] == "smtp", "send_digest tags transport smtp on fallback") + finally: + gmail_send.gmail_available = orig_avail2 + smtp_send.smtp_configured = orig_smtp_cfg2 + smtp_send.send_email = orig_smtp_send + print() if FAILS: print(f"FAILED ({len(FAILS)}):")