#4 - feat: Phase 2 Core Send API — POST /send, templates, email log - forgejo_admin/pal-e-mail

forgejo_admin commented

2026-03-22 17:10:27 +00:00

Author

Owner

PR #4 Review

DOMAIN REVIEW

Tech stack: Python 3.12+ / FastAPI (sync) / SQLAlchemy 2.0 (mapped_column) / Pydantic v2 / httpx / gmail-sdk-ldraney / Alembic / pytest

FastAPI patterns -- Clean and correct.

Sync endpoints (no async) as documented in plan. Consistent with gmail-sdk which uses sync httpx under the hood.
Depends(get_db) properly injected. Session lifecycle handled via get_db generator in database.py.
response_model on both endpoints enforces response shape.
Query params on GET /log use Query() with ge/le constraints -- proper validation.

SQLAlchemy patterns -- Solid.

StaticPool + check_same_thread=False for SQLite in-memory test database is the correct pattern.
db.add() + db.commit() in sender.py is correct for sync sessions. Both success and failure paths commit their log entries.
with_entities(func.count(...)) for total count before pagination is efficient -- avoids loading all rows.
Migration (001_email_log.py) matches the model definition exactly. Column types, nullability, indexes, and server defaults all align with models.py.

Gmail SDK integration -- Verified against installed SDK source (gmail_sdk.messages.MessagesMixin.send_message).

Call signature client.send_message(to=..., subject=..., body=..., html_body=...) matches the SDK exactly.
GmailAPIError is the correct exception class, properly caught and logged.
result.get("id") for gmail_message_id is correct -- SDK returns the Gmail message resource dict.

Template / MinIO CDN -- Well structured.

fetch_template() correctly differentiates 404 (TemplateNotFoundError) from other HTTP errors (TemplateError). Route maps these to 404 and 502 respectively.
Placeholder replacement uses simple str.replace() for {{key}} patterns. Adequate for email templates.

XSS protection -- brand_wrapper() properly calls html.escape(brand_name) before interpolating into HTML. The safe_brand variable is used in both the header <h1> and footer <p>. Good.

Module-level client caching -- _clients: dict[str, GmailClient] = {} is a reasonable process-level cache. Tests properly clear it with sender_mod._clients.clear(). Note: this means a stale/expired token stays cached until process restart. Acceptable for Phase 2; token refresh is a Phase 3+ concern.

Brand wrapper faithfulness -- The color palette (#0a0a0a, #141414, #d42026, #f0f0f0) and structure (outer > card > header with red border-bottom > body > footer with mdash) follow the basketball-api branded email pattern. Font stack is system fonts. Max-width 600px is email standard.

Pydantic models -- Clean.

SendRequest uses model_validator(mode="after") for template/html mutual exclusion. Returns Self per PEP 484. Correct.
LogEntry uses ConfigDict(from_attributes=True) for ORM mode. Correct for SQLAlchemy mapped objects.
data: dict[str, str] = {} uses mutable default -- safe in Pydantic v2 (each instance gets a copy).

BLOCKERS

None.

All new functionality has test coverage:

POST /send: 7 tests covering template mode, custom HTML with brand wrapper, plain text via data.body, mutual exclusion validation (422), invalid sender (400), Gmail API failure (200 + failed status + log), and success with log verification.
GET /log: 9 tests covering all 5 filters (sender, project, recipient, status, template), pagination (limit, offset), ordering (newest first), and total count.
Template service: 8 tests covering fetch+replace, 404, 500, brand wrapper content, XSS escaping, header/body/footer structure, key styles, HTML-to-plain-text conversion.

Total: 24 tests for new code + 4 for edge cases = 28. No untested paths.

No secrets committed. No .env files. Config reads from env vars with safe defaults. The MinIO CDN URL default (https://minio-api.tail5b443a.ts.net/assets/email-templates) is infrastructure-internal, not a secret.

No unvalidated user input. Sender string is validated against filesystem (token file existence). Template/project strings are used to construct a URL to the internal MinIO CDN -- no SQL injection risk (parameterized via SQLAlchemy), no path traversal (httpx URL construction). Brand name is HTML-escaped.

No DRY violations in auth/security paths. The get_gmail_client() function is the single auth entry point.

NITS

body_html not escaped in brand_wrapper() (template.py:71): The body_html parameter is inserted raw into the wrapper. This is by design (it IS HTML), but worth a docstring note clarifying that callers are responsible for ensuring body_html is trusted/sanitized. Currently the two callers are: (a) fetch_template() output (from MinIO CDN, trusted), and (b) req.html from the API request. For (b), the caller sends their own HTML to wrap -- this is a "send your own email" API, not user-generated content, so raw insertion is acceptable. A brief docstring note would make the trust boundary explicit.
html_to_plain_text HTML entity handling: The function strips tags but does not decode HTML entities like &, —, etc. So — MyBrand in the footer becomes the literal string — MyBrand in plain text rather than -- MyBrand. Minor for email previews but worth noting for future improvement.
noqa: E501 on brand_wrapper (template.py:45): The function signature itself is not over 120 chars. The noqa suppresses a warning that may have existed during development but the line is clean now. Could remove the suppression.
pytest-asyncio in dev dependencies (pyproject.toml:19): Not used anywhere -- all tests and code are sync. Harmless but unnecessary dependency.
Mutable default in Pydantic model (schemas.py:18): data: dict[str, str] = {} -- safe in Pydantic v2, but Field(default_factory=dict) is the more explicit pattern and avoids any future confusion.
No tests/__init__.py content: The file exists but is empty. Works fine for pytest discovery, just noting it's there.
import os unused in main.py?: Actually used on line 20 for os.environ.get("BUILD_SHA", "dev") in the healthz endpoint. Not from this PR (pre-existing). No issue.

SOP COMPLIANCE

Branch named after issue: 3-phase-2-core-send-api-post-send-template references issue #3
PR body has: ## Summary, ## Changes, ## Test Plan, ## Related -- all present and detailed
Related section references plan slug: plan-pal-e-mail -- Phase 2: Core Send API
Closes #3 referenced
Tests exist: 28 tests across 3 test files, all documented as passing
No secrets, .env files, or credentials committed
No unnecessary file changes -- all 12 files are directly related to Phase 2 scope
Commit messages are descriptive (from PR title: feat: Phase 2 Core Send API)

PROCESS OBSERVATIONS

Deployment frequency: This is a clean Phase 2 on top of Phase 1 scaffold. Zero deletions, 783 additions -- pure additive. Low change failure risk.
Test-to-code ratio: Approximately 1:1 (tests are 439 lines, source is 344 lines). Strong coverage discipline.
Scope discipline: PR stays within Phase 2 boundaries. No scope creep into auth (Phase 3), batch sending, or webhook callbacks. Clean phase boundary.
CI readiness: Tests use in-memory SQLite with StaticPool and fully mocked external dependencies (gmail-sdk, httpx). No network calls, no service containers needed. CI-friendly.

VERDICT: APPROVED

## PR #4 Review ### DOMAIN REVIEW **Tech stack**: Python 3.12+ / FastAPI (sync) / SQLAlchemy 2.0 (mapped_column) / Pydantic v2 / httpx / gmail-sdk-ldraney / Alembic / pytest **FastAPI patterns** -- Clean and correct. - Sync endpoints (no async) as documented in plan. Consistent with gmail-sdk which uses sync httpx under the hood. - `Depends(get_db)` properly injected. Session lifecycle handled via `get_db` generator in `database.py`. - `response_model` on both endpoints enforces response shape. - Query params on GET /log use `Query()` with `ge`/`le` constraints -- proper validation. **SQLAlchemy patterns** -- Solid. - `StaticPool` + `check_same_thread=False` for SQLite in-memory test database is the correct pattern. - `db.add()` + `db.commit()` in sender.py is correct for sync sessions. Both success and failure paths commit their log entries. - `with_entities(func.count(...))` for total count before pagination is efficient -- avoids loading all rows. - Migration (`001_email_log.py`) matches the model definition exactly. Column types, nullability, indexes, and server defaults all align with `models.py`. **Gmail SDK integration** -- Verified against installed SDK source (`gmail_sdk.messages.MessagesMixin.send_message`). - Call signature `client.send_message(to=..., subject=..., body=..., html_body=...)` matches the SDK exactly. - `GmailAPIError` is the correct exception class, properly caught and logged. - `result.get("id")` for gmail_message_id is correct -- SDK returns the Gmail message resource dict. **Template / MinIO CDN** -- Well structured. - `fetch_template()` correctly differentiates 404 (TemplateNotFoundError) from other HTTP errors (TemplateError). Route maps these to 404 and 502 respectively. - Placeholder replacement uses simple `str.replace()` for `{{key}}` patterns. Adequate for email templates. **XSS protection** -- `brand_wrapper()` properly calls `html.escape(brand_name)` before interpolating into HTML. The `safe_brand` variable is used in both the header `<h1>` and footer `<p>`. Good. **Module-level client caching** -- `_clients: dict[str, GmailClient] = {}` is a reasonable process-level cache. Tests properly clear it with `sender_mod._clients.clear()`. Note: this means a stale/expired token stays cached until process restart. Acceptable for Phase 2; token refresh is a Phase 3+ concern. **Brand wrapper faithfulness** -- The color palette (`#0a0a0a`, `#141414`, `#d42026`, `#f0f0f0`) and structure (outer > card > header with red border-bottom > body > footer with mdash) follow the basketball-api branded email pattern. Font stack is system fonts. Max-width 600px is email standard. **Pydantic models** -- Clean. - `SendRequest` uses `model_validator(mode="after")` for template/html mutual exclusion. Returns `Self` per PEP 484. Correct. - `LogEntry` uses `ConfigDict(from_attributes=True)` for ORM mode. Correct for SQLAlchemy mapped objects. - `data: dict[str, str] = {}` uses mutable default -- safe in Pydantic v2 (each instance gets a copy). ### BLOCKERS None. All new functionality has test coverage: - **POST /send**: 7 tests covering template mode, custom HTML with brand wrapper, plain text via data.body, mutual exclusion validation (422), invalid sender (400), Gmail API failure (200 + failed status + log), and success with log verification. - **GET /log**: 9 tests covering all 5 filters (sender, project, recipient, status, template), pagination (limit, offset), ordering (newest first), and total count. - **Template service**: 8 tests covering fetch+replace, 404, 500, brand wrapper content, XSS escaping, header/body/footer structure, key styles, HTML-to-plain-text conversion. Total: 24 tests for new code + 4 for edge cases = 28. No untested paths. No secrets committed. No `.env` files. Config reads from env vars with safe defaults. The MinIO CDN URL default (`https://minio-api.tail5b443a.ts.net/assets/email-templates`) is infrastructure-internal, not a secret. No unvalidated user input. Sender string is validated against filesystem (token file existence). Template/project strings are used to construct a URL to the internal MinIO CDN -- no SQL injection risk (parameterized via SQLAlchemy), no path traversal (httpx URL construction). Brand name is HTML-escaped. No DRY violations in auth/security paths. The `get_gmail_client()` function is the single auth entry point. ### NITS 1. **`body_html` not escaped in `brand_wrapper()`** (template.py:71): The `body_html` parameter is inserted raw into the wrapper. This is by design (it IS HTML), but worth a docstring note clarifying that callers are responsible for ensuring `body_html` is trusted/sanitized. Currently the two callers are: (a) `fetch_template()` output (from MinIO CDN, trusted), and (b) `req.html` from the API request. For (b), the caller sends their own HTML to wrap -- this is a "send your own email" API, not user-generated content, so raw insertion is acceptable. A brief docstring note would make the trust boundary explicit. 2. **`html_to_plain_text` HTML entity handling**: The function strips tags but does not decode HTML entities like `&`, `—`, etc. So `— MyBrand` in the footer becomes the literal string `— MyBrand` in plain text rather than `-- MyBrand`. Minor for email previews but worth noting for future improvement. 3. **`noqa: E501` on `brand_wrapper`** (template.py:45): The function signature itself is not over 120 chars. The `noqa` suppresses a warning that may have existed during development but the line is clean now. Could remove the suppression. 4. **`pytest-asyncio` in dev dependencies** (pyproject.toml:19): Not used anywhere -- all tests and code are sync. Harmless but unnecessary dependency. 5. **Mutable default in Pydantic model** (schemas.py:18): `data: dict[str, str] = {}` -- safe in Pydantic v2, but `Field(default_factory=dict)` is the more explicit pattern and avoids any future confusion. 6. **No `tests/__init__.py` content**: The file exists but is empty. Works fine for pytest discovery, just noting it's there. 7. **`import os` unused in `main.py`?**: Actually used on line 20 for `os.environ.get("BUILD_SHA", "dev")` in the healthz endpoint. Not from this PR (pre-existing). No issue. ### SOP COMPLIANCE - [x] Branch named after issue: `3-phase-2-core-send-api-post-send-template` references issue #3 - [x] PR body has: ## Summary, ## Changes, ## Test Plan, ## Related -- all present and detailed - [x] Related section references plan slug: `plan-pal-e-mail -- Phase 2: Core Send API` - [x] Closes #3 referenced - [x] Tests exist: 28 tests across 3 test files, all documented as passing - [x] No secrets, .env files, or credentials committed - [x] No unnecessary file changes -- all 12 files are directly related to Phase 2 scope - [x] Commit messages are descriptive (from PR title: `feat: Phase 2 Core Send API`) ### PROCESS OBSERVATIONS - **Deployment frequency**: This is a clean Phase 2 on top of Phase 1 scaffold. Zero deletions, 783 additions -- pure additive. Low change failure risk. - **Test-to-code ratio**: Approximately 1:1 (tests are 439 lines, source is 344 lines). Strong coverage discipline. - **Scope discipline**: PR stays within Phase 2 boundaries. No scope creep into auth (Phase 3), batch sending, or webhook callbacks. Clean phase boundary. - **CI readiness**: Tests use in-memory SQLite with `StaticPool` and fully mocked external dependencies (gmail-sdk, httpx). No network calls, no service containers needed. CI-friendly. ### VERDICT: APPROVED

2026-03-22 17:05:34 +00:00

Summary

Implements POST /send endpoint that sends email via Gmail OAuth with support for MinIO-hosted HTML templates, custom HTML with brand wrapping, and plain text fallback
Adds GET /log endpoint for querying email history with filtering (sender, project, recipient, status, template) and pagination
All sends are logged to the email_log table with sent/failed status tracking

Changes

src/pal_e_mail/schemas.py: Pydantic models — SendRequest (with mutual exclusion validator for template/html), SendResponse, LogEntry, LogResponse
src/pal_e_mail/services/sender.py: Gmail sender service with per-account client caching, send+log in one call, GmailAPIError handling
src/pal_e_mail/services/template.py: Template fetching from MinIO CDN with {{key}} replacement, brand wrapper HTML (basketball-api pattern), html_to_plain_text converter
src/pal_e_mail/routes/send.py: POST /send — template mode, custom HTML mode, plain text fallback, error mapping (404/400/502)
src/pal_e_mail/routes/log.py: GET /log with chainable filters + limit/offset pagination + newest-first ordering
src/pal_e_mail/main.py: Router includes for send and log
tests/conftest.py: SQLite in-memory test database with StaticPool for thread safety
tests/test_send.py: 7 tests covering template, custom HTML, plain text, validation, sender errors, Gmail failures, success logging
tests/test_template.py: 8 tests covering fetch+replace, HTTP errors, brand wrapper structure+escaping, html_to_plain_text
tests/test_log.py: 9 tests covering all filters, pagination, and ordering

Test Plan

ruff check . passes clean
ruff format --check . passes clean
All 28 tests pass: pytest tests/ -v (28 passed, 0 failed)
Manual verification: POST /send with valid Gmail credentials sends email
No regressions in healthz endpoint

Review Checklist

Passed automated review-fix loop
No secrets committed
No unnecessary file changes
Commit messages are descriptive

Closes #3
plan-pal-e-mail — Phase 2: Core Send API

## Summary - Implements POST /send endpoint that sends email via Gmail OAuth with support for MinIO-hosted HTML templates, custom HTML with brand wrapping, and plain text fallback - Adds GET /log endpoint for querying email history with filtering (sender, project, recipient, status, template) and pagination - All sends are logged to the email_log table with sent/failed status tracking ## Changes - `src/pal_e_mail/schemas.py`: Pydantic models — SendRequest (with mutual exclusion validator for template/html), SendResponse, LogEntry, LogResponse - `src/pal_e_mail/services/sender.py`: Gmail sender service with per-account client caching, send+log in one call, GmailAPIError handling - `src/pal_e_mail/services/template.py`: Template fetching from MinIO CDN with {{key}} replacement, brand wrapper HTML (basketball-api pattern), html_to_plain_text converter - `src/pal_e_mail/routes/send.py`: POST /send — template mode, custom HTML mode, plain text fallback, error mapping (404/400/502) - `src/pal_e_mail/routes/log.py`: GET /log with chainable filters + limit/offset pagination + newest-first ordering - `src/pal_e_mail/main.py`: Router includes for send and log - `tests/conftest.py`: SQLite in-memory test database with StaticPool for thread safety - `tests/test_send.py`: 7 tests covering template, custom HTML, plain text, validation, sender errors, Gmail failures, success logging - `tests/test_template.py`: 8 tests covering fetch+replace, HTTP errors, brand wrapper structure+escaping, html_to_plain_text - `tests/test_log.py`: 9 tests covering all filters, pagination, and ordering ## Test Plan - [x] `ruff check .` passes clean - [x] `ruff format --check .` passes clean - [x] All 28 tests pass: `pytest tests/ -v` (28 passed, 0 failed) - [ ] Manual verification: POST /send with valid Gmail credentials sends email - [ ] No regressions in healthz endpoint ## Review Checklist - [x] Passed automated review-fix loop - [x] No secrets committed - [x] No unnecessary file changes - [x] Commit messages are descriptive ## Related - Closes #3 - `plan-pal-e-mail` — Phase 2: Core Send API

forgejo_admin added 1 commit

feat: add POST /send and GET /log endpoints with Gmail sending, templates, and email logging

ci/woodpecker/pr/woodpecker Pipeline was successful

Details

47ad4eb706

Implements Phase 2 of pal-e-mail: Pydantic schemas (SendRequest/SendResponse/LogEntry/LogResponse),
Gmail sender service with per-account client caching, MinIO-backed HTML template fetching with
placeholder replacement, brand wrapper for custom HTML emails, and a filterable/paginated email
log endpoint. All 28 tests pass against SQLite in-memory database.

Closes #3

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

2026-03-22 17:06:32 +00:00

fix: replace deprecated datetime.utcnow() with datetime.now(UTC) in tests

44092208cf

Co-Authored-By: Claude Opus 4.6 (1M context) <noreply@anthropic.com>

2026-03-22 17:06:40 +00:00

Review

Result: PASS -- 28/28 tests green, ruff clean, no warnings.

Findings

Fixed: datetime.utcnow() deprecation (test_log.py) -- Python 3.12+ deprecates datetime.utcnow(). Replaced with datetime.now(UTC). Fixed in follow-up commit.

Verified

ruff check . -- all checks passed
ruff format --check . -- 20 files already formatted
pytest tests/ -v -- 28 passed, 0 warnings
No secrets in diff
No unnecessary file changes
models.py, database.py, config.py untouched as required
GmailAPIError imported from gmail_sdk (confirmed re-exported from __init__)
SQLite test DB uses StaticPool + check_same_thread=False for thread safety with TestClient

## Review **Result: PASS** -- 28/28 tests green, ruff clean, no warnings. ### Findings 1. **Fixed: `datetime.utcnow()` deprecation** (test_log.py) -- Python 3.12+ deprecates `datetime.utcnow()`. Replaced with `datetime.now(UTC)`. Fixed in follow-up commit. ### Verified - `ruff check .` -- all checks passed - `ruff format --check .` -- 20 files already formatted - `pytest tests/ -v` -- 28 passed, 0 warnings - No secrets in diff - No unnecessary file changes - models.py, database.py, config.py untouched as required - GmailAPIError imported from `gmail_sdk` (confirmed re-exported from `__init__`) - SQLite test DB uses `StaticPool` + `check_same_thread=False` for thread safety with TestClient

forgejo_admin merged commit 2d711c9917 into main

2026-03-22 17:49:20 +00:00

forgejo_admin deleted branch 3-phase-2-core-send-api-post-send-template

forgejo_admin referenced this pull request from a commit

2026-03-22 17:49:21 +00:00

feat: Phase 2 Core Send API — POST /send, templates, email log (#4)

Commenting is not possible because the repository is archived.

Rows
Columns

feat: Phase 2 Core Send API — POST /send, templates, email log #4