Uh oh!
There was an error while loading. Please reload this page.
Send profiles into core + Settings → Email Sending + migration V152 - #647
Merged
ddon merged 38 commits intoJul 18, 2026
Conversation
…ed nothing
Two defects, one of them a live break in the released code.
## 1. The universal SMTP provider could not send (regression, v1.7.190)
gen_smtp supplies no TLS options of its own, and OTP's `:ssl` now defaults to
`verify: :verify_peer` with no CA store. So, against a real relay:
- port 465 (`ssl: true`) died on connect with
`{:options, :incompatible, [verify: :verify_peer, cacerts: :undefined]}`;
- every other port (`tls: :always`) failed the STARTTLS handshake (`:tls_failed`).
The previous `tls: :if_available` masked this: when the handshake failed gen_smtp
silently fell back to **plaintext** — so mail went out, with the relay password
on the wire. Making the transport fail-closed turned that silent downgrade into
an outright failure, which is how the bug surfaced.
`swoosh_config_for/1` now passes proper TLS options in both modes — system CA
certs, `verify_peer`, SNI, and the hostname check `:ssl` does not do on its own
(on `sockopts` for implicit TLS, since gen_smtp hands those straight to
`:ssl.connect/4`; on `tls_options` for STARTTLS). Verified against a live relay:
both 465 and 587 connect, and a real send now succeeds over verified TLS.
## 2. "Test Connection" verified nothing for aws_ses and smtp
Neither provider declared a validation, so `do_validate/2` fell through to `:ok`
and `record_validation` stamped the connection "connected" without a single byte
leaving the box. An operator who pasted a wrong key or a bad SMTP password got a
green check and a failing send.
New `PhoenixKit.Integrations.Validators`:
- `aws_ses/1` asks SES itself for the account send quota (raw
`ExAws.Operation.Query`, so no `ex_aws_ses` dependency) — proving the
credentials are real *and* authorised for SES *in that region*;
- `smtp/1` opens a real session and authenticates (`auth: :always`, so a relay
that does not demand auth cannot mask a wrong password). It builds its options
from `Mailer.swoosh_config_for/1`, so it exercises exactly the transport a real
send uses — one source of truth, no drift between "tested" and "sent".
Verified live against real credentials: SES and SMTP both validate `:ok`; a wrong
secret, a wrong password and an unreachable host are each rejected with a
specific message.
The SMTP provider test previously asserted `:ok = validate_connection(...)` on
fabricated credentials — it only passed because validation was a no-op. It now
asserts what actually matters (credentials remain retrievable from the
"configured" status) plus a new case proving an unreachable relay is rejected.Review verdict on the first cut was Needs-rework, and it was right: the branch's
own thesis is "a check that always says yes is worse than no check", and the
check it shipped could say yes when it should say no, and no when it should say
yes. Every item below was reproduced live against a real relay and a real SES
account before and after the fix.
## Said yes when it should have said no
- **Blank region → silently probed us-east-1.** `has_credentials?/1` for
`:key_secret` only looked at `access_key`, so an SES connection with no region
reached the validator; ExAws defaults a missing region to us-east-1 and the
check went green — while the send path builds "email..amazonaws.com" from the
same blank region and raises. The `:key_secret` gate now requires every field
the provider declares required, and the validator refuses a blank region.
- **Fail-open to `verify: :verify_none` when no CA store is found.** Latent in
dev (the container has 150 CA certs), fires exactly where you cannot watch it:
minimal production images. Both the sender and the check would then trust any
certificate — and the check would still show green. Now a relay that expects a
password fails closed with `:no_ca_store`; only a credential-less relay
degrades.
## Said no when it should have said yes
- **`auth: :always` rejected relays that offer no AUTH verb** (internal
smarthosts that authenticate by IP). Sending works there; the check went red.
gen_smtp throws `{:missing_requirement, auth}` for that case, which is now
treated as a pass. Relays that *do* advertise AUTH still fail closed on a bad
password.
- **SES `AccessDenied` was reported as "Invalid credentials".** The least-
privilege policy AWS itself recommends (grant only `ses:SendEmail`) cannot read
the send quota, so a correctly configured integration got a permanent red
cross. The XML error code is now decoded: signature/token errors mean invalid
credentials, `AccessDenied` passes with a note, throttling says so, and
anything else is surfaced verbatim.
- **A valid key could be reported invalid.** Observed live: after AWS rejects a
bad signature, a *correct* request from the same key comes back
SignatureDoesNotMatch for a moment — which is exactly what an operator produces
by pasting a wrong key, fixing it and pressing Test again. A verdict of
"invalid credentials" is now confirmed with a second attempt. Adversarial
sequence: was intermittently red, now 6/6 green.
## Could hang the caller for twenty minutes
`timeout:` bounds only gen_smtp's TCP connect; every read after it waits on a
hard-coded `?TIMEOUT` of 1_200_000 ms, in the *calling* process — and both call
sites are LiveView callbacks. A tarpit relay would park a LiveView process for
twenty minutes. ExAws had the same disease more mildly (ten retries with
backoff). Both checks now run under a hard `Task.yield`/`Task.shutdown` deadline
(15s, configurable), and ExAws is capped at two attempts with 5s timeouts. There
is a regression test with a socket that accepts and then says nothing.
## Structure
The transport is extracted to `PhoenixKit.Mailer.SmtpTransport` — a pure function
of the credentials map, depending on nothing. `Mailer` and `Validators` both use
it, so "tested" and "sent" are literally the same options, and the
Integrations → Validators → Mailer → Integrations cycle is gone.
Two more things it fixes, both found by testing rather than reading:
- **`depth` is not decoration.** gen_smtp's socket layer ships `{depth, 0}`
(smtp_socket.erl:43,52) and merges it into whatever we pass. Depth 0 means "no
intermediate CAs", so every real chain fails verification and the handshake
dies with `:tls_failed`. Omit the key and the connection fails 4/4; set it to
anything >= 1 and it succeeds. There is a test guarding it.
- **MX lookups are off.** gen_smtp resolves the relay's MX records and connects
to the targets, while we pin SNI and the hostname check to the configured name
— a guaranteed certificate mismatch for any relay whose name carries MX records.
86 tests / 0 failures; --warnings-as-errors, credo --strict and dialyzer clean.…veView
`with_deadline/1` ran the check under `Task.async/1`, which *links*. Every call
site is a LiveView callback, and a LiveView process does not trap exits — so any
crash inside the check killed the operator's page outright. The `{:exit, reason}`
clause meant to turn that into an error message could never run: the caller was
already dead by the time `Task.yield/2` would have returned it.
Verified rather than reasoned about — a non-trapping caller running exactly what
`with_deadline/1` ran:
task raises -> CALLER DIES
task exits abnormally -> CALLER DIES
task killed externally -> CALLER DIES
task exits :normal -> survives, {:exit, :normal}
task succeeds -> survives
deadline fires -> survives (Task.shutdown unlinks first)
The SMTP check happened to be safe by accident — `open_smtp/1` both rescues and
catches `:exit`. The SES check was not: `send_quota_request/2` only rescues, and
hackney is entitled to exit under it.
The harness moves to `PhoenixKit.Integrations.Probe`, built on `spawn_monitor/1`
— which is what LiveView's own `start_async` uses, for this exact reason. A crash
is now an error message, and the `:DOWN` branch is reachable code instead of a
comforting lie. Because it is a module rather than a private helper, the property
can actually be tested: three of its seven tests run the probe from a spawned,
non-trapping caller and fail with `{:caller_died, _}` if anyone swaps `Task.async`
back in.
Two smaller things fixed in passing, both consequences of the same seam:
- **The check lost the operator's language.** Gettext keeps the locale in the
process dictionary, which a spawned process does not inherit. Errors rendered
*inside* the check came back in the default language while errors rendered in
the caller ("Region is required", "Invalid port") came back translated — the
operator saw a mix. The locale is now carried across explicitly.
- **A reply landing in the instant the deadline fires was left in the caller's
mailbox.** The caller is a LiveView; it would log the stray as an unexpected
message. It is flushed.
Blast radius of the tightened `:key_secret` gate, checked live on the dev app
before committing: the existing SES connection still resolves and still sends; a
region-less connection now returns `{:error, :not_configured}` instead of raising
inside Swoosh's AmazonSES adapter (which interpolates the blank region into the
hostname), so it fails cleanly where it previously crashed — it could never send
either way; and `aws_ses` is the only `:key_secret` provider, so nothing else is
touched.
Timings on a real relay and a real SES account, all far inside the 15s deadline:
SES bad key 1.7s (includes the confirm-retry), SES good 1.1s, SMTP bad password
0.6s, SMTP good 0.3s.
93 tests / 0 failures in the affected suites; credo --strict and dialyzer clean;
the full suite gains no failures (every failure in it also fails without this
change).…te function ExDoc resolves `Mod.fun/arity` references and warns when the target is private, which do_validate/2 is. Name the module instead — the sentence is about where validation falls through, not about that particular function.
…zer after the merge
Review follows the repo convention
(dev_docs/pull_requests/<year>/<pr>-<slug>/CLAUDE_REVIEW.md). It records what
three rounds found and fixed, and is explicit about who reviewed what: GLM-5.2
was unreachable throughout, so rounds 1-2 were Opus agents and round 3 is
self-verified against the running dev app. It also corrects the round-2 note,
which filed auth: :always under reviewer error - that hypothesis was mine, and
the reviewer had in fact recommended keeping it.
Merging main brought two dialyzer consequences:
- qr_login.ex trips call_without_opaque at Task.yield/2. It is upstream code and
a false positive of the same class the file already suppresses for gettext:
Task.Supervisor.async_nolink/2 returns a %Task{} dialyzer knows structurally,
Task.yield/2 declares Task.t() opaque. Surfaced now because the dep upgrades in
867bc5b rebuilt the PLT. Suppressed, with the reasoning written down.
- It is also independent confirmation of this PRs own HIGH finding. 303286d
reached the identical conclusion in the same week, in its own words: "async_
nolink (not Task.async): the lookup must never be able to crash the calling
LiveView via a link". Probe stays on spawn_monitor rather than switching to the
house pattern - a connection check touches neither repo nor mock, so it needs
neither $callers nor supervision, and async_nolink would cost a suppression in
new code. That reasoning now lives in the moduledoc.…cket
A late review caught two things the green gates did not, because neither had a
test. Both are reproduced below against the running app.
## The retry cap did not cap retries — it crashed them
`retries: [max_attempts: 2]` looks like a cap. ExAws merges config overrides with
`Map.merge` (config.ex:121), which is SHALLOW, so the list REPLACES the default
`[max_attempts: 10, base_backoff_in_ms: 10, max_backoff_in_ms: 10_000]` and takes
both backoff keys with it. `ExAws.Request.backoff/2` then evaluates
`nil * :math.pow(2, attempt)` and raises. The rescue below it swallowed the
ArithmeticError, so the check performed *zero* retries and reported an arithmetic
error while the real cause scrolled past above it:
before: HTTP ERROR: :nxdomain ... ATTEMPT: 1
SES connection check failed: %ArithmeticError{}
after: HTTP ERROR: :nxdomain ... ATTEMPT: 1
HTTP ERROR: :nxdomain ... ATTEMPT: 2
{:error, "Could not reach AWS SES"} (278 ms)
The comment above it claimed "two attempts survives a single blip". It survived
nothing. All three keys are mandatory; the comment now says so.
## The probe relocated the hang instead of removing it
The last commit replaced `Task.async/1` with `spawn_monitor/1` because Task links,
and a crashing check killed the LiveView. That much was right. But `spawn_monitor`
watches in one direction only: the deadline lives in the caller's `receive/after`,
so when the LiveView goes away mid-check — the operator hit refresh — nothing is
left alive to fire it. The check stays parked in gen_smtp's 20-minute `?TIMEOUT`
holding its socket, and, being unlinked, it is now unreachable rather than merely
slow. Linked, it would at least have been reaped. Measured:
caller dies mid-check, spawn_monitor -> check still alive (socket held 20 min)
caller dies mid-check, link+unlink -> check reaped with it
The moduledoc justified the choice by claiming LiveView "monitors rather than
links". That was wrong, and I had asserted it from reading the `:DOWN` handler in
channel.ex without opening async.ex. LiveView does BOTH: `Task.start_link/1`, a
monitor on top for result delivery, and the work wrapped in
`try/after Process.unlink/1` so the child unlinks before it dies. The link reaps
the child when the parent goes; the unlink stops the child taking the parent with
it. Handling one direction and not the other is worse than handling neither.
`Probe` now does the same, and unlinks before killing at the deadline (`:kill` is
untrappable, so the check cannot unlink itself and the link would carry `:killed`
straight back). The check also waits for a go-ahead so it cannot finish, or die,
before the monitor is in place.
## The new logic was the untested logic
That is why both survived. Fixed at the root rather than patched around:
- The CA store is now a parameter of `SmtpTransport.config/2` — it was always a
pure function of (credentials, CA store), the store was just ambient — so the
fail-closed branch, the security-critical one, is finally reachable from a test.
- `request_send_quota/3` takes its requester, so the confirm-retry (the behaviour
that keeps a *valid* key from being called invalid) is tested without AWS.
- `interpret_ses_error/1` is pure and now public, so the mapping from an AWS error
body to an operator-facing verdict is tested against real SES codes.
- A fake relay that greets and advertises no AUTH verb proves the
`{:missing_requirement, _, :auth}` carve-out, which was asserted and never run.
86 -> 107 tests, 0 failures.
## Also fixed
- `send_quota_request/2` rescued but did not catch exits; hackney reaches its pool
through `GenServer.call`, which exits. Harmless while the probe was unlinked
(it lost the diagnosis, not the caller); load-bearing again now that the link is
back.
- The check no longer inherits gen_smtp's default `{retries, 1}`
(gen_smtp_client.erl:38), which probed a temporarily-failing relay twice and
could push a slow failure past our own deadline — the operator would be told
"did not respond in time" instead of what went wrong. A real send still wants
the retry; a check does not.
- `InvalidAccessKeyId`, `ExpiredToken` and `TokenRefreshRequired` are invalid
credentials, not "AWS SES error: <code>" — and they now go through the
confirm-retry like the rest.
- The confirm-retry waits a full second, not 700ms: SES throttles GetSendQuota at
about one request per second and the retry doubles our rate against it, so a
genuinely invalid key could come back as "AWS SES is busy".
- The `FunctionClauseError` rescue is a wide net that logged nothing; it now leaves
a trace, so a real bug elsewhere cannot vanish as "Incomplete SMTP settings".
- The tarpit test leaked its acceptor: `spawn_link` plus a `:normal` test exit does
not propagate, so the acceptor, its listener and the accepted socket outlived
every run for the life of the VM.
- providers.ex carried a comment describing the credential gate as it was before
this branch tightened it.
credo --strict, dialyzer and mix docs clean. Live on the dev app: both integrations
validate, both reject bad credentials, and a real send still goes out.… bare green tick
SES answers `AccessDenied` to GetSendQuota when the key is scoped to `ses:SendEmail`
alone — AWS's own least-privilege guidance. Reporting that as "invalid credentials"
put a permanent red cross on a correctly configured integration, so the check passed
it instead and wrote the caveat to `Logger.info`.
Which is where the caveat stayed. The operator saw an unqualified "Connection
verified", and `AccessDenied` proves only that the signature is valid for *some* AWS
principal — a key from the wrong account lands there too. That is this branch's own
thesis ("a check that always says yes is worse than no check") reopened at the one
door it had closed.
A check can now pass with something to say: `{:ok, note}` alongside `:ok`. The
connection is still connected and still sends — `get_credentials/1` resolves it
exactly as before — but the note is stored in `validation_status` and rendered next
to the badge, in both the connection list and the form, in warning rather than error
colours. The operator reads "Connection verified — Credentials are valid, but not
authorised for GetSendQuota; sending was not verified", which is the whole truth and
is what they need to decide whether to care.
Threaded through `Validators` -> `Probe` -> `do_validate/2` -> `validate_connection/2`
/ `validate_credentials/2` -> `record_validation/2` -> the templates. Dialyzer caught
the one place the widening had not reached (`record_validation/2`'s contract silently
narrowed `result` at the call site, which made the new clause unreachable) — which is
what it is for.
Live on the dev app: recording a pass-with-note leaves the connection `"connected"`
and still resolvable for sending, and the note comes back out of the database.
107 tests / 0 failures in the affected suites; credo --strict, dialyzer and mix docs
clean; the full suite gains no failures.…view Two of this branch fixes change the SEND path, not just the check, and neither was written down: SMTP now stops on images with no CA bundle (it used to proceed with certificate verification disabled, which is why slim images never noticed they had none), and configured relays are no longer MX-resolved. Both are in CHANGELOG under Unreleased, and :no_ca_store is now in deliver_via_integration/3 own @doc, which enumerated every other error it can return. The published review is updated with the final round - the one that arrived after the branch had been declared ready and found a shipped crash plus a leak introduced by the previous round fix. Its closing line is the part worth keeping: green PRs did not catch either, because neither had a test.
…alidators Only CHANGELOG.md conflicted: both sides wanted the top of the file. Both belong - our Unreleased block sits above upstream new 1.7.193 release section.
Only CHANGELOG.md conflicted (top-of-file, as before); providers.ex auto-merged. Our stale-comment fix and all Phase-1 work intact.
…nection-validators # Conflicts: # CHANGELOG.md
Pure port of PhoenixKit.Newsletters.SendProfile, ProviderOptions, and the send-profile CRUD functions from phoenix_kit_newsletters into core as PhoenixKit.Email.SendProfile/ProviderOptions/SendProfiles, ahead of folding newsletter send configuration into core Email module (Phase 1 of the sending-foundation work). No behavior changes: schema fields, changeset validations, and context functions are unchanged. Table renamed to phoenix_kit_email_send_profiles and the is_default unique index reference renamed to idx_email_send_profiles_default — both created by the migration in the next task (A2), which this port does not include. The DB-backed schema/ context test is tagged @moduletag :skip until that migration lands; the pure-logic ProviderOptions test suite runs and passes unconditionally.
Creates phoenix_kit_email_send_profiles (same shape V145 gave phoenix_kit_newsletters_send_profiles), copies every row across by its existing uuid, and drops the V145 table. idx_nl_send_profiles_* indexes become idx_email_send_profiles_* to match the PhoenixKit.Email.SendProfile schema ported in the previous commit. down/1 reverses the move. Bumps @current_version to 151. V145Test's table/index assertions move to the new V151Test, since V145's own table no longer exists once V151 has run; V145Test keeps only the send_profile_uuid broadcast-column check, which V151 doesn't touch. Un-skips the A1 SendProfile integration test now that phoenix_kit_email_send_profiles exists. Not run against the dev database — reserved for the CP1 checkpoint.
Per the "one open migration" rule (no more than one unreleased core vNNN), V151 stays open until the whole newsletters/CRM/core restructuring ships — later stages append their own section here instead of opening V152. Split up/down into a named section pair (up_send_profiles_to_core_email/down_send_profiles_to_core_email) so the next section can be added the same way, and note the convention in the moduledoc and changelog entry. No behavior change.
GetSendQuota and /v3/account both already prove the credentials work, but their payloads were discarded — "Test Connection" gave a bare green check even though the response was sitting right there with the send quota (SES) or remaining credits (Brevo), exactly the numbers an operator needs to catch a campaign about to run out mid-send. SES: parse Max24HourSend/SentLast24Hours/MaxSendRate from the GetSendQuota XML body into a compact note (-1 reads as "unlimited", matching AWS's own convention). Brevo: give brevo_api its own validation strategy (it was riding the generic status-code-only HTTP check) that GETs /v3/account and reports each plan's credits, with the reset date when Brevo provides one.
…te-successes
Two GLM review catches, both verified against the live system:
- Brevo's real /v3/account sends endDate as an ISO-8601 STRING
("2017-04-11T00:00:00.000Z" — official reference, Postman collection,
SDKs agree), not a unix integer. reset_date/1 guarded is_integer, so
the "resets YYYY-MM-DD" branch was dead code in production and the
unit test masked it with a wire shape Brevo never produces. Now
parses ISO-8601 first (integer kept as belt-and-braces), the test
uses the real string shape, and unparseable dates degrade to no
reset note rather than crashing.
- record_validation bumped connected_at only on a bare :ok, but SES
and Brevo successes now return {:ok, note} — so a successful re-test
left a stale "Connected N ago" on the form, the exact symptom the
comment above the gate warns about. {:ok, note} successes are
successful connections (the credentials provably worked), so they
bump the timestamp too. Verified live: a Brevo re-test now writes
connected_at (DB-checked).…n seam, and default transport routing Stage-1 task A4. The admin previously had no send-profiles UI at all (A3 deleted newsletters' own copy) — this adds core's replacement. - New "Settings -> Email Sending" page at /admin/settings/email-sending (deliberately not "emails", which the optional emails module still owns until A5 collapses the two): sender identity (from_name/from_email Settings keys, with fallback display), a read-only transport panel (static app-config mailer vs. connected email-capable integrations, showing the B1 quota/credits notes), a default-send-integration picker, and a test-send action. - Send Profiles list/new/edit LiveViews under the same settings zone, ported from the deleted newsletters LiveViews onto PhoenixKit.Email.SendProfiles/SendProfile/ProviderOptions (A1), preserving the per-provider advanced-fields UI. - New PhoenixKit.Module callback `email_settings_sections/0` (default []) + PhoenixKit.ModuleRegistry.all_email_settings_sections/0, so a module can contribute its own settings section as a live_component rendered on the core page, gated by its declared permission. - PhoenixKit.Mailer.deliver_email/2 now checks Settings key "default_email_integration_uuid": when set and it resolves to a connected integration, delivery routes through deliver_via_integration/3 (preserving the existing tracking/ interception hooks); otherwise the built-in/parent-app mailer path is unchanged. get_from_email/0 and get_from_name/0 made public so the settings page can show the resolved fallback value. - Setting.optional_settings gains from_name, from_email, and default_email_integration_uuid so these keys can be saved blank (to fall back) without tripping the "must provide either value or value_json" required-value validation. Quality gate: mix compile --warnings-as-errors, mix format, mix credo --strict, mix dialyzer all clean. 86 tests across the new/touched suites pass; full mix test run shows only the pre-existing pool-flake failures (verified identical against e4662f4 with these changes stashed).
…profiles-core # Conflicts: # CHANGELOG.md
Upstream PR BeamLabEU#640 (feature/v149-parties-supplier-info) already claims V151 for the supplier-info columns + CRM citext emails, so the accumulator steps aside to V152. down() now stamps '151' (their migration becomes the previous step). The chain has a deliberate hole at 151 until the very next commit merges that branch in.
…fo' into feature/email-send-profiles-core # Conflicts: # lib/phoenix_kit/migrations/postgres.ex
…side live_components
The Verify button hardcoded phx-click with no phx-target, which is fine in
a plain LiveView (events bubble to the view's own handle_event) but breaks
any caller embedding the component inside a live_component: the click
bubbles past the component to a parent that has no clause for it and
crashes. New attr defaults to nil (renders nothing — byte-identical markup
for every existing caller); live_component callers pass phx_target={@Myself}.
Needed by the emails module's Settings->Email Sending sections (A5).HEEx assigns the component body to the default slot even when that body holds nothing but named-slot tags and their surrounding whitespace, so the either/or guard (render inner_block IF non-empty ELSE label) replaced the label with whitespace for every label= + <:description> caller app-wide — found live on the Email Sending profile editor and the emails module's new settings sections. Label and default slot now render in sequence: label-only, slot-only and label+description callers all show what they wrote, and the one label+body caller (CRM contact form) now shows both instead of losing its label to the same bug. Regression tests added for all three shapes.
…sport gate
- set_default_send_profile/1 bypasses changesets (raw update_all), so the
loser of two concurrent make-default clicks hit the partial unique index
as a raised Postgrex.Error and crashed the LiveView, whose handler only
catches {:error, _}. The exact constraint violation now normalizes to
{:error, :concurrent_default_change}; anything else re-raises.
- default_send_integration_uuid/0: documented why credentials-present is
the correct gate and a status check would be wrong — disconnect/2 wipes
credentials (so 'disconnected' already cannot send), while an
error-status connection with live credentials deliberately still routes:
a stale failed test silently rerouting all mail would be the worse
surprise.
- send_profile_form now assigns current_path like its two sibling pages
(was reading the raw @url_path — divergent locale-prefix handling).…tatus, defaults disambiguation - admin_page_header: `back` was a documented no-op — restore a working ghost back-link (+ optional back_label) so Send Profiles, the send profile form, and every other page passing `back=` gets real parentage. - send_profiles: convert the row actions to <.table_row_menu> with data-confirm on Delete, and drop the hand-rolled confirm-modal state machine in favor of the browser-native dialog. Convert the list to <.table_default toggleable> (kills the duplicate hand-rolled mobile card block). Rename the "Default" badge to "Default Newsletter Profile" and explain the split from the transactional integration in the page subtitle. - email_sending: give the transport panel a detect-and-display treatment (alert + Configured/Not-configured badge) instead of a plain paragraph. Rename "Default Send Integration" to "Default Transactional Integration" with a cross-link to Send Profiles, and swap the raw selects/inputs for <.select>/<.input>. - send_profile_form: wrap the form in <.form_section>, collapse the fieldset+label+input boilerplate into <.input label=>, and use a grouped <.select> for the integration picker instead of a raw <optgroup> select. - Tests: add a render-test for the restored back-affordance; update the send-profiles LiveView tests for the new row-menu selectors and the direct (non-modal) delete event. Verified live against the running app (real send-profile data) and full mix quality gate: format, credo --strict, dialyzer, and the full test suite all pass; only pre-existing, unrelated failures remain (MultiSession role seeding, Sitemap settings) untouched by this change.
… parity
- send_profile_form: required marker was baked into the gettext string
("Name *"/"Integration *"), which extraction can't style — split into
the codebase's stylized <span class="text-error">*</span> pattern
(matches integration_form.html.heex / organization.html.heex), using
<.label> above a labelless <.input>/<.select> for the two required
fields.
- send_profiles: the mobile card view showed enabled/disabled as plain
text while the desktop table used the colored <.enabled_badge> — the
old hand-rolled mobile block had the badge, the toggleable-table
conversion lost it. Render the badge component for the card's Status
field to restore parity.Second section of the V152 accumulator (one-open-migration rule): phoenix_kit_crm_lists, phoenix_kit_crm_list_members (denormalized member email with a per-list partial-unique index so a removed member still holds its slot against re-import), and locale/ opted_out_at/consent on phoenix_kit_crm_contacts for contact-level opt-out. down/1 unwinds this section before the send-profiles one, reversing up/1's order.
Two confirmed multi-lens-review findings: - permission_key_for_admin_view resolved nil for ALL five Integrations / Email Sending settings LiveViews (the three new pages AND the two pre-existing Integrations ones — same hole, wider than the finding): their PhoenixKitWeb namespace hits no inference layer, and unmapped views fail closed, so custom roles holding the settings permission were denied pages whose nav links they could see. All five now mapped explicitly to settings. - V152's moduledoc invited appending sections with no warning that the version-comment tracking makes an appended section silently unapplicable on any database already stamped '152'. The doc now states the two consequences outright: a RELEASED accumulator is closed (next DDL opens V153), and branch databases must rollback/re-apply after every appended section.
LiveView's client throws on phx-change for an input outside a form and the event never reaches the server — reads as 'search silently does nothing'. The no-on_submit branch rendered exactly that trap (a bare div), and a test even pinned it (refute <form). First real caller — the CRM list-members search — shipped against it and hit it live. The form now always wraps the input; Enter fires on_submit when given, otherwise the same on_change event, so Enter always means 'search now'.
…der canon email_sending.ex/.html.heex, send_profiles.ex/.html.heex, and send_profile_form.ex/.html.heex were a hybrid of the old and new header patterns: table toolbars were already correct, but send_profiles and send_profile_form still rendered an in-body <.admin_page_header> with back=/back_label=, and email_sending inlined its title/subtitle instead of using assigns. Move title/subtitle into page_title/page_subtitle assigns throughout, add page_section/page_section_path (replacing back=/back_label=) on the two nested pages, drop admin_page_header and the redundant Cancel link on the form, matching the Users/emails reference. Also threads page_section/page_section_path through admin.html.heex so pages using the shared admin layout wrapper pick it up.
Cross-repo decision: page_section collapses on mobile, so the form keeps a footer Cancel next to Save as the mobile-visible way back, alongside the desktop breadcrumb (mirrors newsletters' broadcast_editor/list_editor).
GET /admin/crm/contacts?page=9999999999 killed the BEAM in production. pagination_range(9_999_999_999, 56) computed start_page = max(1, 9_999_999_997) = 9_999_999_997 and end_page = min(56, 10_000_000_001) = 56, producing the range 9_999_999_997..56 — Elixir's a..b auto-picks step -1 whenever a > b, so the :for loop below it allocated a <.link> per step across ~10 billion elements until the VM ran out of memory. A prior review of this same crash path missed it because it only exercised handle_params without actually rendering the pagination component. Fix: clamp current_page into [1, max(total_pages, 1)] before computing start_page/end_page, so start_page <= end_page always holds. The explicit //1 step is a second, independent guard — even if some future edge case still produced start_page > end_page, an explicit positive step returns an empty range instead of auto-picking -1. Both call sites (pagination_controls/1 and pagination/1) share this private helper, so both are covered by the same fix.
pagination/1 already wraps its whole output in `if @total_pages > 1`,
so an empty/single-page list renders nothing. pagination_controls/1
had no matching guard: at total_pages=0 it rendered a "« Prev" button
(for a page that doesn't exist) and a clickable "1" — garbage controls
for a list with nothing to paginate. Added the same `:if={@total_pages
> 1}` guard on the root div, and rewrote the test that asserted the
old (bad) behavior to assert the component now renders nothing.
Also fixed a stale comment on pagination_range/2: the clamp target is
[1, max(total_pages, 1)], not [1, total_pages] (total_pages itself is
clamped to at least 1 first).…profiles-core # Conflicts: # CHANGELOG.md
Send Profiles was only reachable via a link inside the Email Sending page body — add it as a proper sidebar sub-item (path email-sending/profiles, parent admin_settings_email_sending), shown whenever the Email Sending section is active, matching the Media/Media Dimensions nesting pattern already used elsewhere in core_settings_tabs/0.
Per the one-open-migration rule, V152 is still unreleased so this new column folds into the existing up_crm_contact_lists CREATE TABLE rather than opening a new vNNN. Nullable VARCHAR(10), same shape as phoenix_kit_crm_contacts.locale — lets the admin UI tag a list with a content language and bulk-apply it to the list's contacts. Dev DB was already stamped '152' before this amend, so the edited CREATE TABLE IF NOT EXISTS can't retroactively add the column there — applied ALTER TABLE phoenix_kit_crm_lists ADD COLUMN IF NOT EXISTS locale VARCHAR(10) by hand against the dev DB. Test DB was reset fresh (V01->V152) so the amended CREATE TABLE picks it up naturally. V152Test updated with the column assertion.
Stage-4 groundwork: newsletters_broadcasts gets source_type (newsletters_list default / crm_list) and a bare crm_list_uuid (no FK, same soft-reference pattern as send_profile_uuid). list_uuid drops its NOT NULL for crm_list broadcasts. newsletters_deliveries gets a citext recipient_email snapshot and drops user_uuid's NOT NULL, since most CRM contacts have no core User row. down/1 intentionally does not restore either NOT NULL — see the section's moduledoc.
…profiles-core # Conflicts: # CHANGELOG.md
ddon pushed a commit
that referenced
this pull request
Jul 18, 2026
…bump to 1.7.203 Review found one real issue across the send-profiles-into-core PR: a stored connection with a blank required field (SMTP host, SES region) could reach Swoosh.Mailer.deliver/2 uncaught and crash with the full adapter config — secrets included — inlined in the exception message. swoosh_config_for/1 now validates required fields and fails closed instead. Everything else (V152 prefix-safety, the uuid-preserving copy migration, pagination OOM fix, table_default form fix, permission map, LiveView mount usage) was independently verified against the PR description and holds up. Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
timujinne
commented
Jul 19, 2026
timujinne
left a comment
ContributorAuthor
There was a problem hiding this comment.
Code review
This PR moves newsletter Send Profiles into core (PhoenixKit.Email.{SendProfile, SendProfiles, ProviderOptions}), adds the core Settings → Email Sending page plus a Send Profiles CRUD (list / new / edit) and a nested sidebar entry, and lands migration V152 as a three-section accumulator: copy phoenix_kit_newsletters_send_profiles → phoenix_kit_email_send_profiles (UUID-preserving) and drop the source; new CRM list/member tables plus opt-out columns on crm_contacts; and Stage-4 groundwork letting broadcasts source recipients from a CRM list (source_type, crm_list_uuid, nullable list_uuid/user_uuid, new recipient_email). It also adds real "Test Connection" validators (SES/SMTP/Brevo) backed by a deadline-bounded Probe, a generic SMTP transport, a deliver_via_integration/3 mailer seam, an OOM clamp in pagination_range/2, an always-rendered form in search_toolbar, and explicit permission-map entries for the five Integrations/Email-Sending LiveViews. Overall this is a large, carefully-documented, well-tested change; the migration is prefix-safe and idempotent, the SMTP transport is properly fail-closed on a missing CA store, and the permission map and pagination edge cases hold up. I found one real security defect in the mailer seam and one data-integrity gap worth a note.
Verdict: REQUEST-CHANGES
Findings
[MAJOR]
lib/phoenix_kit/mailer.ex:360(swoosh_config_for/1), reached at:346— decrypted provider secrets can leak into an exception message. All three clauses build the secret-bearing Swoosh config straight from the credential map with no required-field guard: SES takesregion: creds["aws_region"], SMTP delegates toSmtpTransport.config/1(which deliberately returns{:ok, options}even with a blankhost, since it is shared with the Test-Connection probe), and Brevo takesapi_key: creds["api_key"]. A stored connection whosestatusis"connected"but with a required field later blanked (e.g. SMTPhostor SESaws_region) passes theconnected?/1gate, reachesSwoosh.Mailer.deliver/2, which callsSwoosh.Adapter.validate_config/2— and that raisesArgumentError, "expected #{inspect(key)} to be set, got: #{inspect(config)}"(deps/swoosh/lib/swoosh/adapter.ex:61).inspect(config)is the full config, so the decryptedsecret(SES) orpassword(SMTP) lands in the uncaught exception and whatever surfaces it (logs, error tracker, the{:error, reason} -> ... inspect(reason)flash on the Email Sending page). Suggestion: validate that every required credential key is present before constructing the config and return{:error, {:incomplete_credentials, missing_keys}}(field names only, never values), so the secret-bearing keyword list is never built — and neverinspected — for an incomplete connection.[NOTE]
lib/phoenix_kit/migrations/postgres/v152.ex:314(up_broadcast_crm_source) — the new CRM tables get realCHECKconstraints (status IN ('active','archived'), memberstatus/sourceenums), but the broadcasts section deliberately carries none:source_typeis a freeVARCHAR(20)with an Ecto-only enum, and nothing guarantees aphoenix_kit_newsletters_deliveriesrow is addressable now that bothuser_uuidand the newrecipient_emailare nullable (a row with bothNULLis unaddressable). The moduledoc states the no-CHECK choice is intentional, matching the existingbroadcasts.statuspattern, so this is a confirmation rather than a defect — but aCHECK (source_type IN ('newsletters_list','crm_list'))and aCHECK (user_uuid IS NOT NULL OR recipient_email IS NOT NULL)would be cheap defense-in-depth and would close the asymmetry with the CRM tables added in the same migration.
timujinne
commented
Jul 19, 2026
ContributorAuthor
Both review findings are addressed in 44f803a — the REQUEST-CHANGES item first:
Gates: 215 targeted tests (mailer, integrations, V152, settings incl. LiveView) 0 failures; compile/docs/credo --strict/dialyzer clean. |
This was referenced Jul 19, 2026
Closed
ddon pushed a commit
that referenced
this pull request
Jul 19, 2026
…s flash Follow-up to the PR #647 post-merge credential-leak fix (82f2ffc): - phoenix_kit_newsletters_deliveries gains CHECK (user_uuid IS NOT NULL OR recipient_email IS NOT NULL) in V152's broadcast section — a both-NULL delivery row is unaddressable. Idempotently guarded via information_schema (same pattern as V125), dropped first in down/1. The newsletters Broadcaster already filters such rows before insert_all, so this is a pure DB backstop. - The Email Sending test-send flash now renders a specific message for {:error, {:incomplete_credentials, fields}} (field names only) instead of falling through to the generic inspect(reason).
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Send Profiles move into core + Settings → Email Sending + V152
Core half of the ecosystem restructuring (see merge order at the bottom). Send profiles stop being a newsletters-private concept and become core infrastructure every module can send through; the emails/newsletters settings converge into one core-owned Settings → Email Sending page.
Send profiles → core
PhoenixKit.Email.SendProfileschema +PhoenixKit.Email.SendProfilescontext (moved from phoenix_kit_newsletters, uuid-preserving copy migration), including per-providerProviderOptionsand the default-profile invariant (set_default_send_profile/1hardened against the two-tab race via an atomic conditionalupdate_all).GetSendQuotaand Brevo/account(plan + credits) shown per profile; validators that cannot verify sending say so instead of a bare green tick.Settings → Email Sending (single tab seam)
Settings → Email Sendingpage: transport detection, default-profile picker, module sections seam — the emails module's sections mount here as live_components (its own Settings tab is removed on the module side).page_title/page_subtitleassigns, nested pages usepage_section/page_section_path(clickable breadcrumb);admin.html.heexnow forwards both — previously a plugin module assigningpage_sectionwas a silent no-op.Migration V152 (single open migration, three sections)
email_send_profiles— create in core, uuid-preserving copy from newsletters' table, drop the old one.phoenix_kit_crm_lists(incl.locale) +phoenix_kit_crm_list_members(denormalized citext member email,UNIQUE (list_uuid, email)), contact extensions (locale,opted_out_at,consent).broadcasts.source_type/crm_list_uuid(soft ref, no FK — same pattern assend_profile_uuid),list_uuid→ nullable;deliveries.recipient_email(citext),user_uuid→ nullable.down/1intentionally does not restore the NOT NULLs (documented: rollback would fail on rows the feature itself created).Fixes worth calling out
pagination_range/2could OOM the whole BEAM: an unclampedcurrent_page(e.g.?page=9999999999) built a descending9999999997..56Range — ~10 billion iterations in the component'sfor. Now clamps into[1, max(total_pages, 1)]with an explicit//1step as a second guard;pagination_controls/1additionally gained thetotal_pages > 1outer guard. Regression tests included (reproduced against a live app before fixing).table_default's search toolbar rendered no<form>when onlyon_changewas given —phx-changeon a formless input dies silently client-side. The form is now always rendered.Verification
Merge order
core (this) → hex release → phoenix_kit_emails #16 → phoenix_kit_newsletters #15 / phoenix_kit_crm (the module PRs pin a core floor that needs the release carrying V152).