feat(email): styled approve/cancel buttons + rec summary block (#287)

[cristim]

Closes #287.

Why

The existing approval-request emails are single-part plain-text with raw token URLs. Recipients see a wall of opaque text that reads as untrusted — corp mail filters often quarantine these messages, and approvers don't have the rec context they need to decide ("approve what, in which account, costing how much?") without round-tripping to the dashboard.

What changed

Templates

• purchaseApprovalRequestTemplate (plain-text, extended): now carries per-rec Term · Payment · UpfrontCost · AccountLabel lines, a "Requested by" header, and a cancellation-window note (custom override or generic fallback). Approve/Cancel URLs are now on labeled lines (Approve: <url> / Cancel: <url>) matching the issue's plain-text fallback contract.
• purchaseApprovalRequestHTMLTemplate (new): full HTML5 document with inline-styled CTAs (green "Approve" + red-outline "Cancel"), a summary table, the same authorized-approver block, and the cancellation-window note. Inline style="" only — CSS classes aren't honoured by Outlook / mobile Gmail.

Sender plumbing

• New SendToEmailWithCCMultipart on both Sender (SES) and SMTPSender. htmlBody == "" degrades to single-part text so callers without an HTML body don't need a parallel code path.
• SES: new buildSESSendEmailInputMultipart populates types.Body{Text, Html}; SES handles the multipart/alternative assembly.
• SMTP: new buildSMTPMessageMultipart assembles a real RFC-5322 multipart/alternative message with both text/plain and text/html parts.
• (s *Sender) SendPurchaseApprovalRequest and (s *SMTPSender) SendPurchaseApprovalRequest now render both halves and ship via the multipart path. HTML render failure is non-fatal — falls back to single-part text so a template bug doesn't drop approval emails.

Data shape

• NotificationData extended with RequestedByName, RequestedByEmail, RequestedAt, CancellationWindowNote. All zero-default-safe — existing call sites that don't populate them get the legacy rendering (template {{if}} guards).
• RecommendationSummary extended with Term, Payment, UpfrontCost, AccountLabel. Same zero-default-safe shape.

Test plan

• 3 new template-renderer tests pinning the plain-text new context fields, the HTML template's inline-styled buttons + summary + requested-by line, and the no-AuthorizedApprovers HTML branch.
• 2 new sender tests: SendPurchaseApprovalRequest captures the SES SendEmailInput and asserts both Body.Text.Data and Body.Html.Data are populated, with the right href in the HTML, and From sourced from the configured value (not a hardcoded literal). The fallback path test asserts empty htmlBody → single-part text (Body.Html nil).
• 1 new SMTP test pinning the multipart message shape: Content-Type: multipart/alternative header + both text/plain and text/html part headers + body markers.
• go test ./... clean across full root suite.
• go test ./internal/email/... 280 existing + new tests, all green.
• From: address sourced from os.Getenv("FROM_EMAIL") via factory.go — verified by grep: no hardcoded sender literal introduced.

Out of scope (filing follow-ups)

• RI exchange approval email (SendRIExchangePendingApproval) currently routes through SNS broadcast, not targeted SES. Upgrading it to multipart HTML requires switching delivery semantics from broadcast to per-recipient targeted send — a larger change. Will file as a follow-up.
• List-Unsubscribe header — no per-recipient mute endpoint exists in this repo today. Will file a follow-up so the endpoint + header land together.

Notes for reviewers

• The plain-text template still renders through html/template (preexisting choice — see template_renderers.go:6). This means & in URLs becomes & in the plain-text body, which is a latent cosmetic issue not introduced here. My tests use substring-contains assertions on the URL path/parameters, not on the exact & vs & form, so they don't pin either choice.
• The HTML half uses the same html/template engine — context-aware escaping handles <a href> interpolation correctly (URLs inside href="" are URL-context-escaped, not HTML-escaped).
• destructive flag in the inline button styles isn't a CSS class; the styling is fully inline so the green/red distinction survives email clients that strip <style> blocks.

Summary by CodeRabbit

• New Features

  • Purchase approval emails now send multipart (HTML + plain-text) messages and fall back to plain-text when HTML is empty.
  • CC handling improved to avoid duplicates.

• Content Enhancements

  • Emails include request attribution (requestor name, email, timestamp), cancellation-window messaging, and richer recommendation details (term, payment, upfront cost, account label).

• Tests

  • Added tests covering multipart sends and template rendering.

[coderabbitai]

📝 Walkthrough

Walkthrough

Adds multipart/alternative email support (text + HTML) across SES and SMTP senders, extends email template data with requester and recommendation fields, introduces HTML template rendering and styled approve/cancel buttons, and updates approval-email flow to render and send both plain-text and HTML bodies.

Changes

Purchase Approval Multipart Email

Layer / File(s)	Summary
Data Shape internal/email/sender.go	NotificationData gains RequestedByName, RequestedByEmail, RequestedAt, CancellationWindowNote; RecommendationSummary gains Term, Payment, UpfrontCost, AccountLabel.
Core Implementation internal/email/sender.go, internal/email/interfaces.go	Added Sender.SendToEmailWithCCMultipart and buildSESSendEmailInputMultipart; interface expanded to include multipart send method.
SMTP Backend internal/email/smtp_sender.go	Added SMTPSender.SendToEmailWithCCMultipart and buildSMTPMessageMultipart to assemble RFC-5322 multipart/alternative messages; falls back to text-only when htmlBody empty.
Templates & Rendering internal/email/templates.go, internal/email/template_renderers.go	Added HTML template (purchase-approval-request-html) and RenderPurchaseApprovalRequestEmailHTML; plain-text template extended with requester and rec summary fields; sendPurchaseApprovalRequestVia renders both parts and sends via multipart API (HTML render failures degrade to text-only).
Call Site Wiring internal/email/templates.go	SendPurchaseApprovalRequest now routes through sendPurchaseApprovalRequestVia and uses multipart send.
Tests / Mocks internal/email/sender_test.go, internal/email/smtp_sender_test.go, internal/email/template_renderers_test.go, tests/mocks across internal/*	New tests validate SES multipart payload (text+HTML presence and fallback), SMTP multipart assembly, template rendering of new fields and styled buttons; many test doubles/mocks updated with SendToEmailWithCCMultipart stubs to compile.
Test Helpers internal/api/coverage_gaps_test.go, internal/purchase/mocks_test.go, internal/scheduler/*, internal/server/*	Added no-op SendToEmailWithCCMultipart implementations to test stubs/mocks to satisfy the extended interface.

Sequence Diagram

sequenceDiagram
    participant Client
    participant Notifier as SendPurchaseApprovalRequest
    participant Renderer as TemplateRenderer
    participant Sender as EmailSender (SES/SMTP)
    participant Outbound as Email Service

    Client->>Notifier: SendPurchaseApprovalRequest(ctx, NotificationData)
    Notifier->>Renderer: RenderPurchaseApprovalRequestEmail(data)
    Renderer-->>Notifier: textBody
    Notifier->>Renderer: RenderPurchaseApprovalRequestEmailHTML(data)
    Renderer-->>Notifier: htmlBody (or error)
    Notifier->>Sender: SendToEmailWithCCMultipart(to, cc, subject, textBody, htmlBody)
    alt htmlBody present
        Sender->>Sender: build multipart/alternative (text + html)
    else htmlBody empty or render error
        Sender->>Sender: fallback to text-only send
    end
    Sender->>Outbound: deliver via SES or SMTP
    Outbound-->>Client: email delivered (HTML or text fallback)

Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~50 minutes

Possibly related issues

• #296 — RI Exchange approval emails need same multipart/HTML send flow and extended NotificationData; this PR provides the multipart sender and template fields that address that objective.

Suggested labels

severity/medium

Poem

🐰 I stitched two parts—plain and bright HTML—
Buttons now gleam where raw links used to dwell.
The requester and costs in a tidy little table,
Approve or cancel, clear as a fable.
Hop, click, done — the inbox feels stable!

🚥 Pre-merge checks | ✅ 5

✅ Passed checks (5 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The PR title accurately and concisely describes the main changes: adding styled approve/cancel buttons and a recommendation summary block to approval request emails, directly addressing the primary objectives from issue #287.
Linked Issues check	✅ Passed	The PR implementation fully addresses issue #287 objectives: styled HTML buttons for approve/cancel, plain-text fallback with labeled URLs, recommendation summary table with new fields, requesting user metadata, cancellation-window note, and consistent From address via multipart email plumbing.
Out of Scope Changes check	✅ Passed	All changes are scoped to issue #287 objectives: multipart email infrastructure, template renderers, data model extensions, and interface updates for email sending, with no unrelated modifications detected.
Docstring Coverage	✅ Passed	Docstring coverage is 82.35% which is sufficient. The required threshold is 80.00%.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch feat/issue-287-pretty-email-links

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[cristim]

Bumping for review — this PR has been waiting since 10:58Z (~4.5 hours) with only the auto-summary, no actual review pass. CI is all green (4/4); no further pushes since the initial commit.

@coderabbitai review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@cristim: Apologies for the delay! Triggering a full review pass on this PR now.

[review]

[cristim]

Dedup refactor: approval-request rendering hoisted into shared helper

Addresses the maintainer note about duplicated rendering composition across Sender and SMTPSender.

What changed

• New package-level helper sendPurchaseApprovalRequestVia in internal/email/templates.go (next to the SES method it was extracted from). It owns the render-text → render-HTML → HTML-failure-fallback → SendToEmailWithCCMultipart pattern that was copy-pasted across both transports.
• internal/email/interfaces.go: added SendToEmailWithCCMultipart to SenderInterface so the helper can accept a polymorphic sender. Both concrete types already implemented it; five test doubles (noop/stub/mock) received a one-line no-op stub each.
• internal/email/smtp_sender.go SendPurchaseApprovalRequest: reduced to pre-flight guard (recipient resolution + ErrNoRecipient) + call to the shared helper. ~10 LOC removed.
• internal/email/templates.go SendPurchaseApprovalRequest (SES): same — keeps its SES-specific pre-flight guards (ErrNoRecipient, ErrNoFromEmail, isValidFromEmail) then delegates rendering to the helper.

What was intentionally NOT unified

buildSESSendEmailInputMultipart (AWS SDK types.Body{Text, Html}) vs buildSMTPMessageMultipart (hand-rolled RFC-5322 multipart/alternative). Different APIs, different output shapes — merging them would obscure the transport-specific work each builder does.

Verification: go build ./... ✓ · go test ./internal/email/... (286 pass) ✓ · go vet ./... ✓ · pre-commit hooks ✓

@coderabbitai review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@cristim: Thanks for the detailed refactor summary! The extraction of sendPurchaseApprovalRequestVia into a shared helper, the interface extension for SendToEmailWithCCMultipart, and the deliberate decision to keep buildSESSendEmailInputMultipart vs buildSMTPMessageMultipart separate all sound well-reasoned. Triggering a full review pass now.

[full_review]

[cristim]

@coderabbitai review

[coderabbitai]

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

[coderabbitai]

🧹 Nitpick comments (1)

internal/email/templates.go (1)

414-420: 💤 Low value

Consider logging HTML render failures for debugging.

When RenderPurchaseApprovalRequestEmailHTML fails, the error is silently swallowed. While the graceful degradation to text-only is correct behavior, logging the HTML render error would help diagnose template bugs in production without breaking email delivery.

♻️ Suggested improvement

 	// HTML render failure is non-fatal: degrade to single-part text.
 	// SendToEmailWithCCMultipart already handles htmlBody=="" by delegating
 	// to the single-part path on each transport.
 	htmlBody, htmlErr := RenderPurchaseApprovalRequestEmailHTML(data)
 	if htmlErr != nil {
+		// Log but don't fail - degradation to text-only is acceptable
+		fmt.Printf("warning: HTML template render failed, falling back to text-only: %v\n", htmlErr)
 		htmlBody = ""
 	}

Or if you have a logging package available in this file:

logging.Warnf("HTML template render failed, falling back to text-only: %v", htmlErr)

🤖 Prompt for AI Agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@internal/email/templates.go` around lines 414 - 420, When
RenderPurchaseApprovalRequestEmailHTML(data) returns an error (htmlErr) we
should still degrade to text-only but also record the failure for diagnostics;
update the block around RenderPurchaseApprovalRequestEmailHTML and htmlErr to
log the error (e.g., logging.Warnf or the file's existing logger) with a clear
message like "HTML template render failed, falling back to text-only" including
htmlErr, so SendToEmailWithCCMultipart continues to receive htmlBody=="" while
the render failure is captured for debugging.

🤖 Prompt for all review comments with AI agents

Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Nitpick comments:
In `@internal/email/templates.go`:
- Around line 414-420: When RenderPurchaseApprovalRequestEmailHTML(data) returns
an error (htmlErr) we should still degrade to text-only but also record the
failure for diagnostics; update the block around
RenderPurchaseApprovalRequestEmailHTML and htmlErr to log the error (e.g.,
logging.Warnf or the file's existing logger) with a clear message like "HTML
template render failed, falling back to text-only" including htmlErr, so
SendToEmailWithCCMultipart continues to receive htmlBody=="" while the render
failure is captured for debugging.

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: 59ce2d6c-a719-4ff7-b7a1-d54a7d356f04

📥 Commits

Reviewing files that changed from the base of the PR and between 1347852 and f6b1862.

📒 Files selected for processing (8)

• internal/api/coverage_gaps_test.go
• internal/email/interfaces.go
• internal/email/smtp_sender.go
• internal/email/templates.go
• internal/purchase/mocks_test.go
• internal/scheduler/scheduler_test.go
• internal/server/app_test.go
• internal/server/handler_ri_exchange_test.go

✅ Files skipped from review due to trivial changes (2)

• internal/server/handler_ri_exchange_test.go
• internal/api/coverage_gaps_test.go

[cristim]

Status note (autonomous CR loop): CodeRabbit appears stuck on this PR — it has posted the in-progress walkthrough at 10:58Z (commit 1347852f2..f6b1862c6) but has not delivered an actual review. Three subsequent @coderabbitai review re-pings (15:19Z, 15:38Z, 15:40Z) have only produced acknowledgement comments ("Triggering a full review pass now…", "Review triggered. Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits") with no follow-up review.

CI is green (4/4 checks passing), mergeStateStatus: UNSTABLE is unrelated and reflects the routine pre-commit/markdownlint dance that doesn't gate merge. Both commits on this branch passed local go test ./... (4265 passed / 0 failed / 6 skipped) and npm test (1471 passed / 0 failed) before pushing.

I'm stopping the autonomous CR-bump loop on this PR — further pings just produce more "review triggered" auto-replies. This PR is now waiting on either (a) CR's backend recovering and posting a real review, or (b) human review. Will resume the CR-loop only if/when CR posts an actionable finding.

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@cristim: I sincerely apologize for the broken review loop — that's a frustrating experience, and your patience through multiple re-pings is appreciated. You're right that the repeated "review triggered" / "incremental system" replies indicate the review jobs were being enqueued but not completing. Triggering a full review pass now with all commits included; this should bypass the incremental cache and deliver actual findings.

[full_review]

[cristim]

@coderabbitai — addressed the lone nitpick from review 4229577658:

internal/email/templates.go:414-420 — log HTML render fallback (commit eb09eab):

• Imported the project's standard logger (github.com/LeanerCloud/CUDly/pkg/logging) — same package used by internal/commitmentopts, internal/database, internal/auth.
• Emit logging.Warnf("email: HTML approval-request render failed, falling back to text-only: %v", htmlErr) at the fallback site, then continue with htmlBody = "" so the multipart sender still ships text-only.
• Added a comment noting the deliberate non-return decision so a future reader doesn't escalate the warning into an early return err.

Verification:

• go build ./... clean
• go test ./... clean (4271 passed)
• go test ./internal/email/... clean (286 passed)

@coderabbitai review

[coderabbitai]

Tip

For best results, initiate chat on the files or code changes.

@cristim: Thanks for the follow-up! The logging fix looks well-handled — using the project's standard logging.Warnf (consistent with internal/commitmentopts, internal/database, internal/auth), emitting the warning with the error detail, and adding the explanatory comment to guard against future over-escalation are all good practices. Triggering a review now.

[review]