feat(i18n): externalize email templates + lang-aware send chain (#221)

[an9xyz]

Closes #221 (Part of #170, backend i18n).

Summary

Externalize the hardcoded Chinese email templates into per-language assets and
thread a lang through the email send chain.

• New modules/base/common/emailtmpl — per-language (zh-CN/en-US)
  subject/html/text templates embedded via go:embed, rendered through a
  text/template + html/template split: Subject headers are not HTML-escaped,
  while bodies keep XSS-safe escaping of user-controlled fields (inviter name,
  space name, …). Templates are precompiled once; an incomplete set fails loud
  at startup.
• Verify-code email — SendVerifyCode gains a lang arg, renders from
  emailtmpl, and is upgraded to SendTransactionalHTML (plaintext fallback +
  transactional headers reduce silent spam-filter drops).
• Space invite emails — owner/member templates moved to emailtmpl; the
  role label and the anonymous-inviter fallback are localized in-template
  instead of hardcoded in Go. Invite links now carry &lang=<recipient language>.
• i18n.OutboundLanguage(ctx) — negotiated language with
  OCTO_DEFAULT_LANGUAGE fallback. Per feat(i18n): email templates + lang-aware send chain (Part of #170) #221 the value resolves to the default
  for now (verify-code requester == recipient; invites are async with no
  recipient uid); the lang param is threaded end-to-end so a future
  per-recipient resolver plugs in without touching the send or template layers.

Client-visible verify-code/login API errors are already localized via
ResponseErrorL; this change only English-cleans the now-invisible
service-layer error sentinels.

Test plan

• go build ./... / go vet ./...
• emailtmpl + pkg/i18n unit tests, incl. -race (render, language
  fallback, XSS escaping boundary, URL not mangled, localized role label,
  template completeness)
• space invite tests (template + dispatch + e2e)
• user email/i18n tests, modules/base/common, modules/common
• make i18n-extract-check + make i18n-lint green
• gofmt clean

Acceptance (#221)

• Verify-code + space-invite emails render per language; fall back to
  OCTO_DEFAULT_LANGUAGE
• make i18n-extract-check + make i18n-lint green

[Jerry-Xin]

The PR is relevant to octo-server and the implementation is generally sound: email templates are externalized, HTML escaping boundaries are covered, language threading is coherent, and the send chain now includes plaintext fallbacks.

💬 Non-blocking

🟡 Warning — modules/space/email_invite_template.go:20-29 says lang lets the landing page and email use the same language, but assets/web/space_email_invite.html:60-79 and assets/web/space_email_invite.html:206-220 still render fixed Chinese UI copy, and the JS fetches at assets/web/space_email_invite.html:255 and assets/web/space_email_invite.html:279 do not propagate lang. Either localize/propagate it in a follow-up or narrow the comment to avoid overstating current behavior.

🔵 Suggestion — modules/base/common/emailtmpl/loader.go:164-233 only validates discovered template sets at runtime. The full supported-language x message-key completeness check exists in tests, but production would silently fall back if a future supported language has no files at all. Consider moving the expected matrix validation into load() if “fail loud” is meant as a runtime guarantee.

🔵 Suggestion — modules/base/common/service_email.go:229-233 writes localized subjects directly into the Subject header. This preserves existing behavior, but RFC 2047 encoding would be more robust for non-ASCII subjects and user-controlled space names.

✅ Highlights

The text/template vs html/template split in modules/base/common/emailtmpl/loader.go:116-127 is the right security boundary for subject/plaintext versus HTML body rendering.

modules/user/api_emaillogin.go:68-73 correctly derives language from the request context while keeping the SMTP send detached from request cancellation.

I ran targeted tests for the changed packages. modules/base/common, modules/base/common/emailtmpl, and pkg/i18n passed; modules/space and modules/user could not run locally because MySQL on 127.0.0.1:3306 was unavailable.

[lml2468]

Summary

Well-structured externalization of email templates into embedded per-language files, with a clean language-aware send chain. No blocking issues.

What this PR does

1. modules/base/common/emailtmpl/ (new package) — Embedded template loader with text/template for subject/plaintext and html/template for HTML body. The split is intentional and correct: subject through html/template would turn "A & B" into "A &" in the SMTP header; HTML through text/template would drop XSS escaping. sync.Once lazy loading with fail-loud on incomplete template sets.

2. Templates — 3 keys × 3 parts × 2 languages = 18 template files. Localized InviterName fallback ("An Octo admin" / "Octo 管理员") and role labels ("member"/"administrator") moved into templates, eliminating hardcoded Chinese strings in Go.

3. service_email.go — SendVerifyCode gains lang parameter. Render happens BEFORE Redis cache write — correct ordering (if render fails, no code is cached but unsendable). Upgraded to SendTransactionalHTML (plaintext fallback for spam filter resilience). Chinese error messages replaced with English.

4. email_invite_template.go — ~80 lines of inline Go templates deleted, replaced by 2 emailtmpl.Render calls. emailInviteAcceptURL now appends &lang= for landing page consistency.

5. email_invite_sender.go — Interface changed from SendHTMLEmail to SendTransactionalHTML. dispatchInviteEmail resolves language via OutboundLanguage(ctx).

6. pkg/i18n/ctx.go — New OutboundLanguage(ctx) function: resolves language for outbound content with fallback to OCTO_DEFAULT_LANGUAGE.

7. Caller audit — Only email caller of SendVerifyCode is api_emaillogin.go (updated). oidc_bind.go:92 calls SMS SendVerifyCode (different interface). No missed callers.

8. Tests — Comprehensive: rendering, XSS escaping boundary (proving text/html template split), URL non-mangling, language fallback, template completeness (all langs × all keys), localized fallback names, role labels per language.

Findings

P2 (nit)

• All template files — Missing trailing newline (\ No newline at end of file in every .tmpl). Does not affect functionality, but most linters/editors prefer files to end with a newline.

Verdict

APPROVED. Clean architecture, correct template engine split, proper render-before-cache ordering, comprehensive test coverage, no interface contract breaks.

[yujiawei]

Code Review — PR #224 (octo-server)

Verdict: APPROVED. No P0/P1 blockers. The change is well-structured, builds and vets clean, the XSS/escaping boundary is correct, and the SMTP header-injection surface is properly mitigated. All findings below are P2 (maintainability / UX / test-coverage / deliverability) and are safe to address as follow-ups.

Scope reviewed: full diff vs main (merge-base 96530d7), all 27 files. I ran go build + go vet on modules/base/common/emailtmpl/..., pkg/i18n/..., modules/space/..., modules/user/... (all clean) and the DB-free unit tests for emailtmpl and pkg/i18n (all green). DB-backed modules/space / modules/user tests compile but require MySQL/Redis, so those assertions were verified by source inspection only.

What's correct (verified, not assumed)

• Escaping split is right. Subject/Text use text/template, HTML uses html/template (loader.go:116-126, doc loader.go:13-20). TestRenderEscapingBoundary and TestRenderAcceptURLNotMangled confirm the plaintext part keeps a raw & while the HTML part emits & and the pre-escaped AcceptURL (typed template.URL) is not double-mangled.
• No SMTP header injection. User-controlled {{.SpaceName}}/{{.PlannedName}} reach the Subject via text/template (unescaped), but the subject is passed through sanitizeHeader() (strips \r/\n, service_email.go:200-204) before the Subject: header — CRLF/Bcc injection is blocked.
• lang actually flows end-to-end. Verify-code resolves a real per-request language (OutboundLanguage(c.Request.Context()), api_emaillogin.go:72-73); invites resolve the deployment default on the detached background ctx (email_invite_sender.go:79-80).
• Fail-before-cache ordering is correct. SendVerifyCode returns on render failure before writing the code/rate-limit keys to Redis (service_email.go:103-109), so a broken template never burns a code.
• No stale callers / regressions. The email SendVerifyCode has exactly one caller (updated with lang); the SMS SendVerifyCode is a distinct interface and untouched; ErrEmailSendRateLimited is consumed via errors.Is, so its zh→en message change is not a client regression.

Findings (all P2 — non-blocking)

1. The &lang= param appended to invite links is dead-weight today

emailInviteAcceptURL appends &lang=<lang> (email_invite_template.go:27-29) "so the landing page and email render in the same language (#221)", but the consumer assets/web/space_email_invite.html is hardcoded zh-CN and only reads token (L135, zero matches for lang in the file); emailInvitePage just serves the static file (api_email_invite_public.go:18-25). So an &lang=en-US link still lands on a Chinese page. It's currently masked because invite lang always resolves to the deployment default anyway — but the moment a per-recipient resolver lands, this will silently fail. Suggest: drop the append until the page honors it, or link the open follow-up issue in a code comment so it isn't mistaken for working behavior.

2. Subject header is not RFC 2047 word-encoded

Non-ASCII subjects (e.g. Octo 验证码, Octo 邀请你加入团队空间「…」) are emitted as raw UTF-8 with only CR/LF stripping — no mime.QEncoding/=?UTF-8?…?= anywhere in the email path (service_email.go:232, :188). Strict MTAs/clients may mojibake or down-rank the header. This is pre-existing (the base already passed Octo 验证码 through the same path), not introduced here, but the PR routes more subjects through it and is explicitly an i18n effort. Suggest (optional): mime.QEncoding.Encode("utf-8", subject) once before writing the Subject header. Not security-relevant.

3. emailtmpl fallback language diverges from the runtime default

fallbackLanguage = SourceLanguage = "en-US" (loader.go:52), but the runtime default from OutboundLanguage is OCTO_DEFAULT_LANGUAGE, defaulting to zh-CN (config.go:25). Inert today (TestTemplateCompleteness makes the fallback branch unreachable), but if a future language ships a partial set, a zh-CN-default deployment would silently serve en-US bodies for the missing pieces. Suggest: note the deliberate "source-language is the canonical complete set" choice at the const, or fall back to the resolved default before the hard en-US backstop.

4. No startup warm-up for emailtmpl

Render triggers sync.Once load() lazily and loadErr is sticky. A broken embed/parse surfaces as a 500 on the first verify-code/invite request rather than failing the deploy. Suggest (optional): a boot-time warm-up (render each Key once) so packaging errors fail fast at startup, consistent with the PR's stated "fail loud" intent.

5. Invite emails always render in the deployment default

By design (#221 decision A — invites are async, no recipient uid), so not a bug; flagging only so the UX gap (an obviously English-context invite still goes out in zh-CN under a zh-CN default) is tracked for the per-recipient follow-up.

6. PR description nit

The summary's "lang currently always resolves to the deployment default" is accurate for invites but not for verify-code, which negotiates per-request (correctly). The code comment at api_emaillogin.go:69-71 already documents this; only the PR text is slightly off.

7. Copy nit — en-US member subject

Octo invites you to join the team space "{{.SpaceName}}" reads stiffly (a product name as the subject of "invites you"); the zh-CN construction is natural. Optional polish, e.g. You're invited to join the team space "{{.SpaceName}}" on Octo.

8. Test-coverage gaps (DB-free, cheaply fillable)

• OutboundLanguage (ctx.go:84-90) has no direct unit test — its env-error fallback and ctx-wins-over-env branches are only exercised transitively. A t.Setenv-driven table test would guard it.
• The incomplete-set fail-loud branch (loader.go:226-233) is never triggered; TestTemplateCompleteness only proves it isn't hit. Injecting an fs.FS (default to the embed) + fstest.MapFS with a missing part would cover the documented guarantee.
• SendVerifyCode's render-before-cache ordering + multipart-path selection is DB-gated and unguarded; acceptable given the constraint, worth an integration test in the DB suite.

Summary

Solid, defensive i18n refactor with the security-sensitive bits (escaping, header injection) done correctly. The only finding with real user-facing weight is #1 (the dead &lang= link param) — worth a tracked follow-up but not a merge blocker since email bodies do localize and invite lang is default-only today. Approving.

[lml2468]

Re-review (44fc666)

Delta commit addresses all prior feedback:

1. RFC 2047 subject encoding — encodeSubject() via mime.QEncoding.Encode("utf-8", s) applied to both sendEmail and buildTransactionalMessage. Test verifies round-trip decode. Non-ASCII subjects ("Octo 验证码") now survive strict MTAs.

2. Load-time matrix check — loadFrom() now enforces completeness for every supported language × every expected key at init, not just in tests. A supported language shipped with a missing file fails loud at first Render rather than silently falling back to en-US. loadFrom accepts fs.FS for testability.

3. New tests — TestLoadFromCompleteMatrixOK, TestLoadFromIncompleteMatrixFailsLoud, TestLoadFromParseErrorSurfaces (using fstest.MapFS), TestOutboundLanguage (4 cases including ctx-wins-over-env).

4. Template trailing newlines — All 18 template files now end with a newline (my P2 nit resolved).

5. Comment clarification — emailInviteAcceptURL now explicitly documents &lang= as forward-compatible, not yet consumed by the landing page.

No new issues. APPROVED.

[Jerry-Xin]

The PR is relevant to octo-server and cleanly moves email content into language-aware templates with no blocking regressions found.

💬 Non-blocking

🟡 Warning: emailtmpl.Render falls back to the source language for unsupported or empty lang, not directly to OCTO_DEFAULT_LANGUAGE (modules/base/common/emailtmpl/loader.go:147). Current call sites pass i18n.OutboundLanguage, so behavior is correct today, but future callers of SendVerifyCode or emailtmpl.Render could accidentally bypass the runtime default (modules/base/common/service_email.go:85).

🔵 Suggestion: Template validation is lazy on first render, not actually at process startup (modules/base/common/emailtmpl/loader.go:119, modules/base/common/emailtmpl/loader.go:176). If startup failure is part of the intended contract, wire this into startup/config validation explicitly.

✅ Highlights

• Good separation between text/template for subject/plaintext and html/template for HTML escaping.
• Invite and verify-code send paths now consistently use multipart transactional email.
• Coverage is strong around escaping, role localization, URL preservation, template completeness, and outbound language fallback.

Verification: git diff --check passed. Focused tests for modules/base/common, modules/base/common/emailtmpl, pkg/i18n, and modules/common passed. Full modules/space and modules/user tests could not run in this environment because local MySQL at 127.0.0.1:3306 was unavailable.