Skip to content

fix(mp): type CommunicationType as MP's real enum — 'Text' and 'Letter' 500 - #92

Merged
chriskehayias merged 2 commits into
MinistryPlatform-Community:mainfrom
jonnydcakes:fix/communication-type-enum
Sep 29, 2026
Merged

chriskehayias merged 2 commits into
MinistryPlatform-Community:mainfrom
jonnydcakes:fix/communication-type-enum

Conversation

@jonnydcakes

Copy link
Copy Markdown
Contributor

Problem

src/lib/providers/ministry-platform/types/provider.types.ts:41

CommunicationType: 'Email' | 'Text' | 'Letter';

MP's Platform.Messaging.CommunicationType is Unknown | Email | SMS | RssFeed | GlobalMFA.
'Text' and 'Letter' were never members, and MP rejects either with an opaque HTTP 500,
not a 400:

{"Message":"Error converting value \"Text\" to type 'Platform.Messaging.CommunicationType'.
  Path 'CommunicationType' ... Requested value 'Text' was not found."}

So the only value the type offered for texting is one that always fails, and the failure reads
as a server fault rather than a bad payload. 'Email' happens to be a real member — and it is
the only value anyone has ever passed, which is why a two-thirds-wrong union sat in the type
layer unnoticed.

Also: SMS requires TextPhoneNumberId

Once CommunicationType: 'SMS' is used, MP additionally requires it:

HTTP 500
{"Message":"Provided communication object is invalid.
  1. (Error) Property 'TextPhoneNumberId' is required and must be populated."}

TextPhoneNumberId is the outbound number's dp_SMS_Numbers.SMS_Number_ID. The field already
exists on CommunicationInfo but is optional — it is effectively required when
CommunicationType === 'SMS'
. MP's own Swagger marks it optional, so the Swagger cannot be
trusted here.

Verified against a live tenant

Not taken from the Swagger alone — re-checked on a production MP instance on 2026-09-08:

Check Result
dp_Communication_Types 1 Email, 2 SMS Text, 3 RSS Feed, 4 GlobalMFA (Unknown is the zero value, no row)
dp_SMS_Numbers one active default number

Originally found on 2026-06-11 building a passwordless sign-in spike that reuses this
/communications pattern: the email delivered; the SMS succeeded only after switching 'Text'
→ 'SMS' and adding TextPhoneNumberId.

Impact

Latent, not a live outage. Nothing in the template calls createCommunication — the
references are the plumbing (helper.ts, provider.ts, communication.service.ts) and the
tests, which use 'Email'. This is a wrong type surface waiting for the first person who tries
to send a text, who gets a 500 with no indication their payload was at fault. Every downstream
fork of the template inherits it.

The change

  1. COMMUNICATION_TYPES as a const [...] as const with CommunicationType derived from
    it, so the enum has one source of truth and a comment naming what verified it.
  2. CommunicationInfo becomes a discriminated union, so the compiler requires
    TextPhoneNumberId exactly when the type is 'SMS'. A runtime 500 becomes a build error.
  3. Runtime guard in CommunicationService.createCommunication, placed above
    ensureValidToken() so a doomed payload costs neither a token refresh nor a round trip. It
    catches callers arriving through an as cast or untyped JSON, and names the accepted values
    so nobody needs the Swagger to fix their call.

Plus a paragraph in the provider docs/README.md Communications section.

provider.types.ts is hand-maintained (generate-types.ts only writes into models/), so this
will not be undone by npm run mp:generate:models.

Tests

Eight cases added to the existing communication.service.test.ts (21 in that file now), plus a
compile-time block.

Each rejection test asserts that nothing was sent — no post, no postFormData, and no
ensureValidToken — not merely that an error came back. Asserting on the message alone would
still pass if the guard were later moved below the POST, which is the failure mode the guard
exists to prevent.

All four mutations were run and each fails the suite it should:

Mutation Result
Delete the assertSendable(...) call 6 tests fail
Move the guard below ensureValidToken() 5 tests fail
Put 'Text' back in COMMUNICATION_TYPES tsc TS2578 — unused @ts-expect-error
Make TextPhoneNumberId optional for 'SMS' tsc TS2578 — unused @ts-expect-error

Verification on this branch: npx vitest run --coverage → 59 files / 1023 tests pass, coverage
gates green (99.74% statements); npx tsc --noEmit clean; npx eslint . clean;
node scripts/check-lockfile.mjs reports no drift (no dependency changes).

…r' 500

`CommunicationInfo.CommunicationType` was typed `'Email' | 'Text' | 'Letter'`.
Ministry Platform's `Platform.Messaging.CommunicationType` is
`Unknown | Email | SMS | RssFeed | GlobalMFA`, so two of the three values the
type offered were never members, and MP rejects either with an opaque HTTP 500
rather than a 400:

    {"Message":"Error converting value \"Text\" to type
      'Platform.Messaging.CommunicationType'. Path 'CommunicationType' ...
      Requested value 'Text' was not found."}

The only value the type offered for texting was one that always fails, and the
failure reads as a server fault rather than a bad payload. `'Email'` happens to
be a real member and is the only value anyone had passed, which is why the
other two went unnoticed.

Once `'SMS'` is used, MP additionally requires `TextPhoneNumberId`
(`dp_SMS_Numbers.SMS_Number_ID` of the outbound number) and answers its absence
with a second 500 — `Property 'TextPhoneNumberId' is required and must be
populated`. MP's own Swagger marks the field optional, so the Swagger cannot be
trusted here.

Three changes:

- `COMMUNICATION_TYPES` as a `const [...] as const` with `CommunicationType`
  derived from it, so the enum has one source of truth.
- `CommunicationInfo` becomes a discriminated union, so the compiler requires
  `TextPhoneNumberId` exactly when the type is `'SMS'`. A runtime 500 becomes a
  build error.
- A runtime guard in `CommunicationService.createCommunication`, placed above
  `ensureValidToken()` so a doomed payload costs neither a token refresh nor a
  round trip. It catches callers arriving through an `as` cast or untyped JSON,
  and names the accepted values so nobody needs the Swagger to fix their call.

Verified against a live MP tenant on 2026-09-08 rather than from the Swagger
alone: `dp_Communication_Types` holds `1 Email`, `2 SMS Text`, `3 RSS Feed`,
`4 GlobalMFA` (`Unknown` is the zero value and has no row), and an email plus an
SMS were both delivered end to end — the SMS only after switching `'Text'` to
`'SMS'` and supplying `TextPhoneNumberId`.

Tests: eight cases added to `communication.service.test.ts`. Each rejection test
asserts that *nothing was sent* — not merely that an error came back — since
asserting on the message alone would still pass if the guard were moved below
the POST. The type half is covered by `@ts-expect-error` cases that fail
`tsc --noEmit`, which `next build` also performs.
@codecov

codecov Bot commented Sep 19, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@jonnydcakes

Copy link
Copy Markdown
Contributor Author

@chriskehayias - just wanted to bring your attention to this bug/pr

…to Email and SMS

Brings this PR up to date with main's CommunicationService rework
(buildCommunication, trusted sender, fixed-message validation) and adapts
the fix to it:

- COMMUNICATION_TYPES is narrowed to ['Email', 'SMS']. Unknown, RssFeed and
  GlobalMFA are real MP enum members, but the template deliberately does not
  permit them, in the type or at runtime.
- main's runtime allowlist, new Set(['Email', 'Text', 'Letter']), repeated
  the original bug and actively refused 'SMS'. It is now built from
  COMMUNICATION_TYPES, and the SMS TextPhoneNumberId requirement moves into
  buildCommunication in place of assertSendable. Errors keep main's
  fixed-message form (Invalid CommunicationType / Invalid
  TextPhoneNumberId), naming the field and never echoing the value.
- CommunicationContent, the MPHelper.createCommunication input, uses a
  distributive Omit. A plain Omit over the union merged its members, so
  an 'SMS' payload without TextPhoneNumberId compiled again at the public
  API.
- Tests adapted to pass the trusted sender; the rejection cases now cover
  every excluded value, plus a compile-time case on CommunicationContent.
- docs/README.md: main's "Email, Text or Letter" corrected.

Co-Authored-By: Claude Opus 5.5 (1M context) <noreply@anthropic.com>
@chriskehayias

Copy link
Copy Markdown
Contributor

Thanks @jonnydcakes — great catch, and the live-tenant verification made this easy to trust. Your commit is unchanged; I merged main into the branch and adapted the fix on top, because main had moved a lot since you opened this:

  • main had repeated the bug at runtime. The CommunicationService rework added buildCommunication with its own allowlist, new Set(['Email', 'Text', 'Letter']), which actively refused 'SMS'. It is now built from your COMMUNICATION_TYPES, and your SMS TextPhoneNumberId check lives in buildCommunication instead of assertSendable.
  • Limited to Email and SMS. As a project decision, the template permits only these two, in the type and at runtime. Unknown, RssFeed and GlobalMFA are real MP members but are refused.
  • The public API is covered too. MPHelper.createCommunication takes CommunicationContent = Omit<CommunicationInfo, ...>, and a plain Omit over your union merged its members, so an 'SMS' payload without TextPhoneNumberId compiled again there. It now uses a distributive Omit, with a @ts-expect-error case against it.
  • Error messages follow main's fixed-message convention (Invalid CommunicationType, Invalid TextPhoneNumberId), which names the field without echoing the caller's value.
  • Your tests were adapted to pass the now-required trusted sender, and still assert that nothing was sent.

Verified: tsc, eslint ., the full suite with coverage gates (2046 passing), npm run build and the prerender check.

@chriskehayias
chriskehayias merged commit 2d14533 into MinistryPlatform-Community:main Sep 29, 2026
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants