V155: CRM contact id on newsletter deliveries + per-broadcast dedup indexes

[timujinne]

Groundwork for the newsletters module sending to CRM contact lists (restructuring spec §7). Three changes to phoenix_kit_newsletters_deliveries:

1. crm_contact_uuid — nullable, bare UUID, no FK (newsletters must not hard-depend on the CRM module being installed; same soft-reference pattern as broadcasts.crm_list_uuid from V152), with a plain index.

2. Recipient CHECK widened, under the same constraint name — keeps the original "somebody is addressable" clause (user_uuid IS NOT NULL OR recipient_email IS NOT NULL) and adds mutual exclusion: a row is never claimed by both a core user and a CRM contact.
   Deliberate deviation: the spec's shorthand calls for a strict XOR between the two owners. That is not enforced here — existing CRM-sourced deliveries carry neither (they are addressed by recipient_email alone, and nothing backfills the new column onto historical rows), so a strict XOR would reject exactly the shape the current send path produces. Retrofitting a heuristic email-based backfill was out of scope. Rationale is recorded in the migration's moduledoc.

3. Three partial unique indexes — (broadcast_uuid, user_uuid), (broadcast_uuid, crm_contact_uuid), (broadcast_uuid, recipient_email), each WHERE ... IS NOT NULL. This is the table's first DB-level per-broadcast duplicate guard: Broadcaster.process_batch/5's insert_all has no ON CONFLICT, and Oban's unique option guards the job, not the row a second enqueue of the same broadcast would insert. The address-level index is the only one that also prevents one mailbox receiving the same broadcast twice through two contacts that share it.

down/1 unwinds in reverse and restores the V152 CHECK verbatim; nothing here is a backfill, so rollback is non-lossy.

Verification

Checked a live database before writing the indexes (14 deliveries across 4 broadcasts): no pre-existing duplicate (broadcast_uuid, user_uuid) or (broadcast_uuid, recipient_email) pairs, so all three create cleanly with no pre-migration cleanup.

V155Test + V152Test → 44 tests, 0 failures (fresh chain, both prefixed and unprefixed). mix compile --warnings-as-errors clean.

Note: the module-side consumer (threading crm_contact_uuid through the send path and adding on_conflict: :nothing) follows in a phoenix_kit_newsletters PR that requires a release carrying this migration.

[timujinne]

Code review

This pair of PRs adds DB-level per-broadcast delivery dedup to newsletters. Core #653 (V154) adds a bare crm_contact_uuid soft-ref column to phoenix_kit_newsletters_deliveries, replaces the recipient_check CHECK (same name) with "addressable AND not double-claimed by both a core user and a CRM contact," and creates three partial unique indexes — (broadcast_uuid, user_uuid|crm_contact_uuid|recipient_email) each WHERE … IS NOT NULL — the table's first per-broadcast dedup guarantee. Newsletters #20 threads the contact uuid from CRMSource.sendable_recipients/1 into the insert rows and switches Broadcaster.process_batch/5 to on_conflict: :nothing, counting skipped duplicates, continuing the throttle offset over inserted rows, and correcting total_recipients to the actual inserted count. The design is sound; I verified the riskiest claims (partial-index arbitration under ON CONFLICT DO NOTHING with no target, RETURNING semantics, finalize independence from total_recipients, changeset↔CHECK parity) directly against Postgres and the code. The issues below are MINOR/NOTE — no blocker.

Verdict: APPROVE-WITH-NOTES

Findings

1. [MINOR] total_recipients reports "inserted this pass", not the broadcast's recipient count — misleading on a resend.lib/phoenix_kit/newsletters/broadcaster.ex:122-127. On a partial/fully-duplicate resend, total_recipients is corrected to the rows inserted this enqueue (0 on a pure resend, "new-only" on a mixed one), while the original enqueue's Oban jobs still send the original N deliveries. The value is rendered as a headline stat (broadcast_details.html.heex:118), so a pure resend shows "Recipients: 0" while sent_count later climbs to N (and any sent/total ratio divides by zero). Finalization never reads total_recipients (see Verified), so this is display-only, but the field name and the UI imply "total recipients of this broadcast." Suggestion: set it to the post-insert delivery count for the broadcast (count(deliveries where broadcast_uuid)), or relabel/document the field as "recipients added in the latest enqueue."

2. [MINOR] down/1 is lossy in practice and the rollback coupling is undocumented.lib/phoenix_kit/migrations/postgres/v154.ex:78-83, 123-129. The moduledoc states "Non-lossy — nothing here is a backfill, so there's no derived data to lose on rollback." That holds only for the schema state V154 starts from; once CRM broadcasts have run, deliveries carry real crm_contact_uuid values that DROP COLUMN destroys. Separately, newsletters #20 writes crm_contact_uuid on every CRM send, so rolling V154 back alone breaks the module — a coherent rollback must downgrade both together, which the moduledoc doesn't note. Suggestion: soften the claim to "non-lossy for pre-existing schema; populated contact refs are dropped on rollback" and mention the module dependency.

3. [NOTE] Index/CHECK creation is non-concurrent.v154.ex:143-150, 168-186. Three CREATE UNIQUE INDEX plus ADD CONSTRAINT … CHECK run non-concurrently inside the migration transaction, each taking a strong lock and full-scanning phoenix_kit_newsletters_deliveries. Trivial on the dev table cited in the moduledoc (14 rows), a real write-blocking window on a large production deliveries table. This matches existing chain conventions (and CONCURRENTLY can't be used inside the DDL transaction), so just flagging it for operator awareness on large installs.

Verified (adversarial focus, came back clean)

• on_conflict: :nothing without a conflict_target is well-defined here. Confirmed against Postgres: an exact (broadcast, contact) duplicate and a cross-contact same-email row are both skipped (num_rows: 0) under ON CONFLICT DO NOTHING with no target — all three partial indexes arbitrate. The shared-mailbox dedup is the documented intent (v154.ex moduledoc §dedup indexes), not an accident. The only other unique indexes on the table are the PK (fresh UUIDv7.generate(), collision-proof) and the partial message_id index (NULL at insert_all time → inert), so nothing unintended can be silently swallowed. A CHECK/NOT-NULL violation raises rather than skipping, so it isn't masked either.
• Duplicate count is correct.RETURNING yields zero rows for conflict-skipped inserts (verified), so length(deliveries) - length(inserted) (broadcaster.ex:304) equals exactly the conflict count — every non-inserted row is a genuine duplicate.
• total_recipients cannot desynchronize finalization across batches.maybe_finalize_broadcast (delivery_worker.ex:516-520) and repair_stuck_sending_broadcasts (newsletters.ex:434) flip sending→sent purely on Delivery.non_terminal_broadcast_uuids_query/0, never on total_recipients; batched duplicate skips only affect the display value (finding 1).
• Changeset matches the DB CHECK exactly.validate_recipient/1 + validate_not_both_owners/1 (delivery.ex) reject and accept the same row shapes as the CHECK across all six cases (both-nil, contact-only, user+contact+email reject; user-only, email-only, contact+email accept).
• up/1 is atomic and idempotent. Ecto wraps the version in a DDL transaction (no @disable_ddl_transaction), so a mid-version failure rolls back all index/constraint/column changes; IF NOT EXISTS / DROP-then-ADD make re-runs safe. down/1 restores the V152 CHECK verbatim and drops index→column in reverse order.
• Tests are honest.broadcaster_idempotency_test.exs drives the real Broadcaster.send/1 → insert_all path and asserts a second enqueue (status reset to draft, re-send) adds no delivery rows and yields total_recipients 0 / 1 — not a fresh-broadcast assertion. Oban.insert_all([]) on an all-duplicate batch is a safe no-op (Oban 2.23 → Repo.insert_all(_, _, [], _) → {0, []}).

[timujinne]

Documentation note addressed in 198991c: the down/1 "non-lossy" claim is now scoped to "relative to the original V152 schema" and states plainly that once the module has written real crm_contact_uuid values, rolling back drops them like any populated column. It also records the pairing requirement — V154 and the newsletters release that depends on it must be rolled back together, since reverting the migration alone removes the column that module's insert_all targets. No DDL or test changes; migration suite re-run as a sanity check (13 tests, 0 failures).

[timujinne]

Delta review of the section added after the first pass (198991ce..816b39cc, source_params):

This delta extends the still-unreleased V154 accumulator with one new section: source_params JSONB NOT NULL DEFAULT '{}'::jsonb on phoenix_kit_newsletters_broadcasts, backing the user_group recipient source (source_type = "user_group") where a broadcast targets core users by role, stored as %{"role_names" => [...]}. The change is small, self-contained, idempotent (ADD/DROP ... IF NOT EXISTS/IF EXISTS), and the down/1 unwind order is correct — source_params is added last in up/1 and dropped first in down/1, and since it lives on phoenix_kit_newsletters_broadcasts while the earlier three sections (crm_contact_uuid, the widened CHECK, the dedup indexes) all touch phoenix_kit_newsletters_deliveries, there is no ordering interaction with them. ALTER TABLE ADD COLUMN ... NOT NULL DEFAULT '<const>' is metadata-only on PG 11+ (this repo requires PG 15+), so no table rewrite on a large broadcasts table, and '{}' is the right "no params" value for pre-existing rows, which are all newsletters_list/crm_list per V152. The migration test covers the column shape and a round-trip payload. Overall this is solid and consistent with the chain's conventions; the notes below are about the documented consumer contract, not the DDL itself.

Verdict: APPROVE-WITH-NOTES

Findings

1. [MINOR]lib/phoenix_kit/migrations/postgres/v154.ex:83-90 — The moduledoc specifies %{"role_names" => [...]} as "the shape the newsletters-side resolver reads/writes," but role names are mutable natural keys. PhoenixKit.Users.Role carries a stable uuid PK (role.ex:33) and exposes update_role/2, which permits renaming a role (custom roles freely; validate_system_role_protection/1 only guards the is_system_role flag, not the name) and roles can be deleted. A saved user_group broadcast therefore holds a soft reference to a renameable/deletable string: rename "Support" → "SupportAgent" (or delete it) and the broadcast silently resolves to zero recipients on the next send/re-send, with no FK cascade and no cheap "find broadcasts referencing this role" query (it would need a source_params @> '{"role_names":["Support"]}' GIN scan that this migration doesn't index). The moduledoc justifies JSONB-versus-a-scalar-uuid-column but never addresses names-versus-role-uuids or the staleness. Suggestion: at minimum add a one-line note that role_names are soft refs to a mutable key and the consumer must tolerate stale names; ideally, since the consumer ships in a separate newsletters PR, prefer storing role UUIDs there (the column is generic enough to absorb either shape) so the reference is to an immutable surrogate key.

2. [NOTE]lib/phoenix_kit/migrations/postgres/v154.ex:234-239 — A text[] column would model "a set of role names" more directly and is cheaper to query than JSONB, but given the column is named generically source_params (not role_names) and is meant to carry different per-source shapes, JSONB is the defensible choice and matches the crm_lists.metadata / crm_list_members.metadata convention cited in the moduledoc. No change needed; flagging only because the alternative is worth a conscious rejection, which the moduledoc already does.

3. [NOTE]test/phoenix_kit/migrations/v154_test.exs:255 — assert default =~ ~r/'\{\}'::jsonb/ pins the textual representation of the default ('{}'::jsonb). Postgres happens to store it exactly that way, so the test passes, but it is mildly brittle to future PG default-normalization quirks; asserting just the type + NOT NULL + non-null default would be slightly more robust. Not worth changing on its own.

[ddon]

@timujinne conflicts

[timujinne]

Conflict resolved by merging current main. Two things happened at once here:

• main's V154 slot was taken by the OpenGraph migration (Add V154 OG migration + admin list-UI, breadcrumb and sidebar enhancements #650), so this PR's migration is renumbered V154 → V155 (module, file, table-comment stamps, tests, and the changelog block in postgres.ex all updated; @current_version is now 155). PR title updated to match.
• The migration content itself is unchanged — same four sections (crm_contact_uuid + index, widened recipient CHECK, three partial dedup indexes, source_params JSONB).

Verified locally: V155 + V152 migration suites and the timezone-label suite — 58 tests, 0 failures. GitHub now reports the branch as clean.