fix(email): short-circuit to no-op sender when EMAIL_ENABLED=false (closes #332)#333
Conversation
…loses #332) `internal/email/factory.go::NewSenderFromEnvironment` now returns a no-op `SenderInterface` at the top of the function when `EMAIL_ENABLED=false`, before dispatching by `SECRET_PROVIDER`. Background: the factory and the secret resolver share `SECRET_PROVIDER` for two different purposes (cloud email backend selection vs. secret-store backend selection). The local-dev `docker-compose.yml` already declares `EMAIL_ENABLED: "false"` and explains "Email disabled for local development (no SNS topic)", but the factory never reads the flag — it just dispatches on `SECRET_PROVIDER`. So: - `SECRET_PROVIDER=aws` + no AWS creds boots an SES sender that nothing exercises (passes by luck, not design). - `SECRET_PROVIDER=env` (the documented local-dev resolver per `internal/secrets/resolver.go:50`) + `EMAIL_ENABLED=false` hard-fails on startup: `failed to initialize email sender: unsupported email provider: env`. The factory now respects the documented `EMAIL_ENABLED` contract. The no-op sender implements all 15 `SenderInterface` methods, logs each invocation at debug level so local-dev traces still show where an email would have gone, and is guarded by a compile-time `var _ SenderInterface = (*NopSender)(nil)` assertion. Verification: - `go build ./...` clean - `go test ./...` clean (0 failures)
…diagnosis `sendPurchaseApprovalRequestVia` in `internal/email/templates.go` silently swallowed `RenderPurchaseApprovalRequestEmailHTML` errors when degrading to text-only delivery. The graceful fallback is correct (text-only is the safer alternative to dropping the approval email entirely), but the silent swallow hides template-syntax bugs from production diagnostics. This commit imports the project's standard `pkg/logging` package and emits a `Warnf` at the fallback site so an HTML render regression surfaces in logs without breaking email delivery. The fallback itself is unchanged — `htmlBody = ""` still routes through the multipart sender's text-only path. A comment notes the deliberate non-return decision so a future reader doesn't escalate the warning into an early `return err`. Originally a CodeRabbit nitpick on PR #298 that raced with the merge (commit eb09eab on the closed feat/issue-287 branch). Folded into this PR per follow-up tracking — both touch internal/email/ and the change is six lines. Verification: - `go build ./...` clean - `go test ./internal/email/...` clean
cristim
added
triaged
|
@coderabbitai review |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
📝 WalkthroughWalkthroughThe PR adds a no-op email sender and integrates an EMAIL_ENABLED early-exit in the factory to return it when disabled; it also logs HTML template render failures for purchase-approval emails while keeping text-only fallback. ChangesEmail Disabling with No-op Sender
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 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.
Inline comments:
In `@internal/email/factory.go`:
- Around line 47-50: Replace the literal string check of EMAIL_ENABLED with
boolean parsing: read os.Getenv("EMAIL_ENABLED"), call strconv.ParseBool on it
(import strconv), and if the parsed value is false return NewNopSender() and log
the same message; handle parse errors by either treating them as enabled (and
optionally logging a warning) or defaulting to false per your policy. Ensure you
reference the same env var name "EMAIL_ENABLED" and keep NewNopSender() as the
no-op return path.
In `@internal/email/nop_sender.go`:
- Around line 26-33: NopSender currently logs raw recipient and CC email
addresses in SendToEmail and SendToEmailWithCCMultipart (and other no-op sender
methods), which leaks PII; update those logging calls to avoid printing raw
emails by either removing the address fields or replacing them with non-PII info
(e.g., masked addresses, recipient count, or domains only); implement a small
helper (e.g., maskEmail or summarizeRecipients) and use it in
NopSender.SendToEmail and NopSender.SendToEmailWithCCMultipart (and the other
no-op methods flagged) so logs retain useful context without exposing full email
addresses.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2d427b08-aaa4-448c-9218-950a35d64802
📒 Files selected for processing (3)
internal/email/factory.gointernal/email/nop_sender.gointernal/email/templates.go
… no-op logs (CR pass on PR #333) Addresses both actionable items from CodeRabbit's review on PR #333: 1. **EMAIL_ENABLED parsing** (`internal/email/factory.go`): the literal `== "false"` check only matched the lower-case spelling. `"False"`, `"FALSE"`, `"0"`, etc. would all be treated as enabled, even though the user clearly intended the no-op path. Replaced with `strconv.ParseBool` so the full set of false-y spellings is honoured (false/False/FALSE/0/f/F per stdlib semantics). Unset / empty values keep the default (enabled) so existing deployments that don't set the var are unaffected. Unparseable values emit a warning and fall through to the enabled path so the misconfiguration is visible in logs without bringing the app down. 2. **PII redaction** (`internal/email/nop_sender.go`): the previous logs included raw recipient email addresses, subjects, and (in one place) admin email addresses. Even in dev, log files leak into shared environments (terminal scrollback, screen-shares, support tickets), and email addresses are sufficient identifying information to require treating them as PII. Replaced all recipient/subject/email-data formatting with just method-name + recipient counts (e.g. `to=1 cc=3`) — enough trace context to know which email path was invoked without leaking who it would have gone to. Documented the policy in the type-level doc comment so a future maintainer doesn't add the addresses back. Verification: - `go build ./...` clean - `go test ./...` clean (0 failures) - `go test ./internal/email/...` clean
|
@coderabbitai — addressed both actionable items from review
Verification: @coderabbitai review |
|
Triggering a fresh review of the updated commit now. ✅ Actions performedReview triggered.
|
Tests added alongside the runtime changes from this PR.
`internal/email/factory_test.go` gains
`TestNewSenderFromEnvironment_EmailEnabled` — a table-driven test with
fifteen sub-cases covering every branch of the new dispatch:
- Six false-y EMAIL_ENABLED spellings (false, False, FALSE, 0, f, F)
all short-circuit to NopSender, with SECRET_PROVIDER set to a
deliberately-unsupported "env" backend so the assertion proves
the short-circuit fires BEFORE provider dispatch.
- Six true-y spellings (true, True, TRUE, 1, t, T) all fall through
to the AWS sender via SECRET_PROVIDER=aws.
- Unset / empty values fall through to the default-enabled path.
- An unparseable value ("maybe") emits a warning (visible in test
output) and falls through to enabled — protecting an accidental
misconfiguration from taking the app down.
`internal/email/nop_sender_test.go` (new file) covers the NopSender
contract:
- TestNopSender_AllMethodsReturnNil — exercises every one of the 15
SenderInterface methods and asserts they all return nil. A future
bug that returns an error would propagate into the calling
application path, breaking the EMAIL_ENABLED=false promise that
no work happens.
- TestNopSender_NilSafe — empty / nil inputs (empty CC list, empty
bodies) must not panic.
- TestNopSender_NilContext — a nil context.Context must not panic
either; the no-op shouldn't be the surface that leaks a panic.
- TestNopSender_SatisfiesInterface — runtime echo of the
compile-time `var _ SenderInterface = (*NopSender)(nil)` guard,
so a refactor that silently removes the guard is still caught.
Verification:
- `go test ./internal/email/...` clean (19 new sub-cases)
- `go test ./...` clean (0 failures)
|
Added unit tests in commit b04594e:
|
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
|
Triggering a fresh review of the updated commit now. ✅ Actions performedReview triggered.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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.
Inline comments:
In `@internal/email/factory_test.go`:
- Around line 258-260: In the "unset_falls_through" subtest, capture the
original EMAIL_ENABLED value before calling os.Unsetenv and register a t.Cleanup
that restores the environment after the test; i.e., inside the
t.Run("unset_falls_through", ...) before os.Unsetenv("EMAIL_ENABLED") save orig
:= os.LookupEnv("EMAIL_ENABLED") and call t.Cleanup(func(){ if present restore
with os.Setenv else call os.Unsetenv }) so the test (and helper aws(t)) does not
leak mutated env state to later tests.
🪄 Autofix (Beta)
❌ Autofix failed (check again to retry)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 416d50af-d1a3-49e1-8793-42bc7d814ab1
📒 Files selected for processing (2)
internal/email/factory_test.gointernal/email/nop_sender_test.go
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. The branch was updated while autofix was in progress. Please try again. |
|
Note Autofix is a beta feature. Expect some limitations and changes as we gather feedback and continue to improve it. Fixes Applied SuccessfullyFixed 1 file(s) based on 1 unresolved review comment. Files modified:
Commit: The changes have been pushed to the Time taken: |
Fixed 1 file(s) based on 1 unresolved review comment. Co-authored-by: CodeRabbit <noreply@coderabbit.ai>
Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
PR #333 (closes #332) landed a botched merge of `unset_falls_through` in TestNewSenderFromEnvironment_EmailEnabled — two overlapping copies of the same sub-test ended up concatenated (one missing its closing braces, one preceded by stray fragments), plus duplicate `ctx :=` / `sender, err :=` lines. The result didn't parse: gofmt -l internal/email/factory_test.go internal/email/factory_test.go:309:2: missing ',' before newline ... internal/email/factory_test.go:312:6: expected '(', found TestNewSenderWithConfig_AWS This trips `gofmt` and `go vet` in the pre-commit workflow on every open PR against `feat/multicloud-web-frontend` (e.g. #326, #335, #336). Keep the `prev/hadPrev` version of the sub-test (the one that actually does `os.Unsetenv` first, which is the case the test name describes), drop the orphaned `orig/hadOrig` fragment, and remove the duplicate ctx/sender declarations. Verified locally: gofmt clean, `go vet ./internal/email/...` clean, `go test ./internal/email/...` 306 tests pass.
…env vars (closes #334) (#335) * fix(local-dev): docker-compose + .env.example cover the new required env vars (closes #334) A fresh `docker-compose up -d` on this branch fails on startup because the compose file is missing several env vars the app now requires. This commit adds them with local-dev defaults and documents the same contract in `.env.example` so anyone running outside docker-compose (e.g. directly via Air or `go run ./cmd/server`) has a one-stop reference. Failure chain that this fixes (each gated the next): 1. scheduled-task auth init: SCHEDULED_TASK_AUTH_MODE unset 2. admin password resolution: ADMIN_PASSWORD_SECRET required 3. (after #333 lands) SECRET_PROVIDER=aws + empty AWS creds was working by luck; switching to `env` is now the correct local-dev resolver path 4. frontend admin-setup modal asks for an API key (sourced from API_KEY_SECRET_ARN → fails when the var is empty) `docker-compose.yml` (`app` service environment block): - SECRET_PROVIDER: aws → env (internal/secrets.EnvResolver, per internal/secrets/resolver.go:50 — pairs with EMAIL_ENABLED=false so the no-op email sender from #333 kicks in). - SCHEDULED_TASK_AUTH_MODE: disabled (internal/server/scheduledauth has no default and refuses to start when unset). - ADMIN_PASSWORD_SECRET / API_KEY_SECRET_ARN as VAR-NAME indirections pointing at ADMIN_PASSWORD_DEV / ADMIN_API_KEY_DEV (the EnvResolver pattern). Concrete dev values for both, plus ADMIN_EMAIL. - CREDENTIAL_ENCRYPTION_ALLOW_DEV_KEY=1 (gate the all-zero dev key per credentials.LoadKey — refuses to start without it). `.env.example` documents: - the new SECRET_PROVIDER=env contract (replaces the now-stale "env will fail" warning that pre-dated #333), - SCHEDULED_TASK_AUTH_MODE and EMAIL_ENABLED with one-line rationale, - the VAR-NAME-indirection pattern for *_SECRET / *_SECRET_ARN with the concrete dev values co-located so future readers can trace the chain in one file. Depends on PR #333 (no-op email sender) — without that, the email factory crashes on SECRET_PROVIDER=env. Sequencing intentional. Verification: - docker-compose up -d brings postgres + app + frontend to healthy - curl http://localhost:8080/api/health → HTTP 200 - admin-setup modal accepts the documented dev defaults * fix(local-dev): align .env.example ADMIN_EMAIL with docker-compose default (CR pass on PR #335) CodeRabbit nitpick on PR #335: `.env.example` still listed `ADMIN_EMAIL=admin@example.com` while `docker-compose.yml` defaults to `admin@cudly.local`. The drift made the two reference points disagree about which placeholder a fresh checkout should use. Aligning on `admin@cudly.local` keeps both files telling the same story.
1 participant
Summary
Two related fixes to
internal/email/so the package matches itsdocumented contract and produces useful diagnostics when degraded.
Changes
1. Factory short-circuits to no-op sender on
EMAIL_ENABLED=false(closes #332)internal/email/factory.go::NewSenderFromEnvironmentnow returns ano-op
SenderInterfaceat the top of the function whenEMAIL_ENABLED=false, before dispatching bySECRET_PROVIDER.The factory and the secret resolver share
SECRET_PROVIDERfor twodifferent purposes (cloud email backend vs. secret-store backend).
The local-dev
docker-compose.ymlalready declaresEMAIL_ENABLED: "false"and explains "Email disabled for localdevelopment (no SNS topic)", but the factory never reads the flag.
So
SECRET_PROVIDER=env(the documented local-dev resolver perinternal/secrets/resolver.go:50) +EMAIL_ENABLED=falsehard-failson startup with
failed to initialize email sender: unsupported email provider: env, even though no email will ever be sent.The new
internal/email/nop_sender.goimplements all 15SenderInterfacemethods, logs each invocation at debug level solocal-dev traces still show where an email would have gone, and is
guarded by a compile-time
var _ SenderInterface = (*NopSender)(nil)assertion.2. Log HTML approval-request render fallback
sendPurchaseApprovalRequestViaininternal/email/templates.gosilently swallowed
RenderPurchaseApprovalRequestEmailHTMLerrorswhen degrading to text-only delivery. Now emits
logging.Warnf("email: HTML approval-request render failed, falling back to text-only: %v", htmlErr)at the fallback site sotemplate regressions surface in logs without breaking email delivery.
The graceful fallback itself is unchanged —
htmlBody = ""stillroutes through the multipart sender's text-only path.
Originally a CodeRabbit nitpick on PR #298 that raced with the merge
(commit
eb09eabe4on the closedfeat/issue-287branch). Foldedinto this PR since both changes touch
internal/email/and thechange is six lines.
Closes #332.
Test plan
go build ./...cleango test ./...clean (0 failures)go test ./internal/email/...cleanSECRET_PROVIDER=envandEMAIL_ENABLED=false; backend reaches/api/healthHTTP 200; logs show"Email sending is disabled (EMAIL_ENABLED=false); using no-op sender".Summary by CodeRabbit
New Features
Improvements
Tests