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

src/lib/providers/ministry-platform/docs/README.md

@@ -256,10 +256,17 @@ await mp.createCommunication(content, { authorUserId: userId, fromContactId });
 ```
 
 The content itself is validated too. The payload is rebuilt from the known fields only, so
-extra keys are dropped. `CommunicationType` must be `Email`, `Text` or `Letter`. IDs must be
-positive integers. Subjects and display names must not contain control characters, and
+extra keys are dropped. `CommunicationType` must be `Email` or `SMS`, and an `SMS`
+communication must also carry `TextPhoneNumberId` (`dp_SMS_Numbers.SMS_Number_ID` of the
+outbound number). IDs must be positive integers. Subjects and display names must not contain control characters, and
 addresses must be single plain addresses.
 
+`CommunicationType` is deliberately narrower than MP's `Platform.Messaging.CommunicationType`
+enum (`Unknown | Email | SMS | RssFeed | GlobalMFA`): the template permits only `Email` and
+`SMS`, in both the type and the runtime check. MP has no `Text` or `Letter` member. MP answers
+an unknown type, or an `SMS` send without `TextPhoneNumberId`, with an opaque HTTP 500 rather
+than a 400, so both are refused before anything is sent.
+
 ### Files
 
 ```typescript

src/lib/providers/ministry-platform/services/communication.service.test.ts

@@ -8,6 +8,7 @@ import type { MinistryPlatformClient } from '@/lib/providers/ministry-platform/c
 import type { HttpClient } from '@/lib/providers/ministry-platform/utils/http-client';
 import type {
   Communication,
+  CommunicationContent,
   CommunicationInfo,
   MessageInfo,
 } from '@/lib/providers/ministry-platform/types';
@@ -196,6 +197,105 @@ describe('CommunicationService', () => {
     });
   });
 
+  /**
+   * `CommunicationType` is limited to `Email` and `SMS`, and an `'SMS'` send
+   * must carry `TextPhoneNumberId`. MP answers a bad type or a missing number
+   * with an opaque HTTP 500, so the service refuses them itself.
+   *
+   * Each rejection test asserts that **nothing was sent** — not merely that an
+   * error came back. Asserting on the message alone would still pass if the
+   * guard were moved below the POST, which is the failure mode it exists to
+   * prevent.
+   */
+  describe('createCommunication — payloads Ministry Platform answers with a 500', () => {
+    const smsInfo: CommunicationInfo = {
+      ...communicationInfo,
+      CommunicationType: 'SMS',
+      TextPhoneNumberId: 1,
+    };
+
+    /** An SMS payload with the number stripped, as untyped JSON would arrive. */
+    function smsWithoutNumber(): CommunicationInfo {
+      const payload = { ...smsInfo };
+      delete (payload as { TextPhoneNumberId?: number }).TextPhoneNumberId;
+      return payload as CommunicationInfo;
+    }
+
+    function expectNothingSent() {
+      expect(mockHttpClient.post).not.toHaveBeenCalled();
+      expect(mockHttpClient.postFormData).not.toHaveBeenCalled();
+      // The guard sits above `ensureValidToken`, so a doomed payload costs
+      // neither a token refresh nor a round trip.
+      expect(mockClient.ensureValidToken).not.toHaveBeenCalled();
+    }
+
+    it("should POST SMS with its outbound number — the value MP accepts, not 'Text'", async () => {
+      (mockHttpClient.post as ReturnType<typeof vi.fn>).mockResolvedValueOnce(createdCommunication);
+
+      await communicationService.createCommunication(smsInfo, undefined, commSender);
+
+      expect(mockHttpClient.post).toHaveBeenCalledWith(
+        '/communications',
+        expect.objectContaining({ CommunicationType: 'SMS', TextPhoneNumberId: 1 })
+      );
+    });
+
+    it('should refuse SMS with no TextPhoneNumberId, and send nothing', async () => {
+      await expect(
+        communicationService.createCommunication(smsWithoutNumber(), undefined, commSender)
+      ).rejects.toThrow('Invalid TextPhoneNumberId');
+
+      expectNothingSent();
+    });
+
+    it('should refuse SMS with no TextPhoneNumberId on the attachments path too', async () => {
+      // The guard must sit above the attachments branch, not inside one arm.
+      const flyer = new File(['flyer'], 'flyer.png', { type: 'image/png' });
+
+      await expect(
+        communicationService.createCommunication(smsWithoutNumber(), [flyer], commSender)
+      ).rejects.toThrow('Invalid TextPhoneNumberId');
+
+      expectNothingSent();
+    });
+
+    it('should treat TextPhoneNumberId 0 as absent', async () => {
+      // MP identity columns start at 1, so 0 is never a real number id — and MP
+      // rejects it with the same "required and must be populated" 500.
+      await expect(
+        communicationService.createCommunication(
+          { ...smsInfo, TextPhoneNumberId: 0 },
+          undefined,
+          commSender
+        )
+      ).rejects.toThrow('Invalid TextPhoneNumberId');
+
+      expectNothingSent();
+    });
+
+    it.each([
+      // The values this was previously typed with; neither is in MP's enum.
+      ['Text'],
+      ['Letter'],
+      // Real MP enum members the template deliberately does not permit.
+      ['Unknown'],
+      ['RssFeed'],
+      ['GlobalMFA'],
+      // Matching is exact, not case-insensitive.
+      ['sms'],
+    ])("should refuse CommunicationType '%s', and send nothing", async (type) => {
+      await expect(
+        communicationService.createCommunication(
+          { ...communicationInfo, CommunicationType: type } as unknown as CommunicationInfo,
+          undefined,
+          commSender
+        )
+      ).rejects.toThrow('Invalid CommunicationType');
+
+      expectNothingSent();
+    });
+  });
+
   describe('sendMessage', () => {
     it('should POST JSON to /messages when there are no attachments', async () => {
       (mockHttpClient.post as ReturnType<typeof vi.fn>).mockResolvedValueOnce(createdCommunication);
@@ -494,3 +594,76 @@ describe('CommunicationService', () => {
     });
   });
 });
+
+/**
+ * Compile-time half of the fix. These assertions never run — `tsc --noEmit`,
+ * which `next build` also performs, is what checks them: each
+ * `@ts-expect-error` fails the build if the type ever stops rejecting the
+ * payload beneath it.
+ */
+describe('CommunicationInfo — type-level contract', () => {
+  const email: CommunicationInfo = {
+    AuthorUserId: 7,
+    Body: 'Service is cancelled this Sunday.',
+    FromContactId: 100,
+    ReplyToContactId: 100,
+    CommunicationType: 'Email',
+    Contacts: [1, 2, 3],
+    IsBulkEmail: true,
+    SendToContactParents: false,
+    Subject: 'Sunday update',
+    StartDate: '2026-08-21T09:00:00',
+  };
+
+  it('should reject every type but Email and SMS, and demand a number for SMS', () => {
+    const invalidText: CommunicationInfo = {
+      ...email,
+      // @ts-expect-error — 'Text' is not a Platform.Messaging.CommunicationType member.
+      CommunicationType: 'Text',
+    };
+
+    const invalidLetter: CommunicationInfo = {
+      ...email,
+      // @ts-expect-error — 'Letter' never existed in MP's enum either.
+      CommunicationType: 'Letter',
+    };
+
+    const excludedMember: CommunicationInfo = {
+      ...email,
+      // @ts-expect-error — a real MP member, but not one the template permits.
+      CommunicationType: 'GlobalMFA',
+    };
+
+    // @ts-expect-error — 'SMS' requires TextPhoneNumberId.
+    const smsWithoutNumber: CommunicationInfo = {
+      ...email,
+      CommunicationType: 'SMS',
+    };
+
+    expect([invalidText, invalidLetter, excludedMember, smsWithoutNumber]).toHaveLength(4);
+  });
+
+  it('should keep the SMS requirement on CommunicationContent, the MPHelper input', () => {
+    // Guards the distributive Omit: a plain `Omit` over the union loses it.
+    const content = {
+      Body: 'Service is cancelled this Sunday.',
+      ReplyToContactId: 100,
+      Contacts: [1, 2, 3],
+      IsBulkEmail: true,
+      SendToContactParents: false,
+      Subject: 'Sunday update',
+      StartDate: '2026-08-21T09:00:00',
+    };
+
+    // @ts-expect-error — 'SMS' requires TextPhoneNumberId here too.
+    const smsWithoutNumber: CommunicationContent = { ...content, CommunicationType: 'SMS' };
+
+    const sms: CommunicationContent = {
+      ...content,
+      CommunicationType: 'SMS',
+      TextPhoneNumberId: 1,
+    };
+
+    expect([smsWithoutNumber, sms]).toHaveLength(2);
+  });
+});

src/lib/providers/ministry-platform/services/communication.service.ts

@@ -1,5 +1,12 @@
 import { MinistryPlatformClient } from "../client";
-import { CommunicationInfo, Communication, MessageAddress, MessageInfo } from "../types";
+import {
+    COMMUNICATION_TYPES,
+    CommunicationInfo,
+    CommunicationType,
+    Communication,
+    MessageAddress,
+    MessageInfo,
+} from "../types";
 import { sanitizeNumericId } from "../utils/filter-sanitize";
 import { errorName } from "./guards";
 
@@ -30,7 +37,8 @@ export interface MessageSender {
     fromAddress: MessageAddress;
 }
 
-const COMMUNICATION_TYPES: ReadonlySet<string> = new Set(['Email', 'Text', 'Letter']);
+/** The `CommunicationType` values that may be sent; see `COMMUNICATION_TYPES`. */
+const ALLOWED_COMMUNICATION_TYPES: ReadonlySet<string> = new Set(COMMUNICATION_TYPES);
 
 /** C0 controls and DEL: CR/LF in a subject or display name is header injection. */
 const CONTROL_CHARS = /[\u0000-\u001f\u007f]/;
@@ -100,22 +108,36 @@ function buildCommunication(communication: unknown, sender: unknown): Communicat
     const { authorUserId, fromContactId } = sender as Record<string, unknown>;
     const c = communication as Record<string, unknown>;
 
-    if (typeof c.CommunicationType !== 'string' || !COMMUNICATION_TYPES.has(c.CommunicationType)) {
+    if (typeof c.CommunicationType !== 'string' || !ALLOWED_COMMUNICATION_TYPES.has(c.CommunicationType)) {
         throw invalid('CommunicationType');
     }
 
-    const payload: CommunicationInfo = {
+    const fields = {
         AuthorUserId: sanitizeNumericId(authorUserId, 'sender authorUserId'),
         Body: requireString(c.Body, 'Body'),
         FromContactId: sanitizeNumericId(fromContactId, 'sender fromContactId'),
         ReplyToContactId: sanitizeNumericId(c.ReplyToContactId, 'ReplyToContactId'),
-        CommunicationType: c.CommunicationType as CommunicationInfo['CommunicationType'],
         Contacts: requireIdList(c.Contacts, 'Contacts'),
         IsBulkEmail: requireBoolean(c.IsBulkEmail, 'IsBulkEmail'),
         SendToContactParents: requireBoolean(c.SendToContactParents, 'SendToContactParents'),
         Subject: requireHeaderText(c.Subject, 'Subject'),
         StartDate: requireString(c.StartDate, 'StartDate'),
     };
+
+    // MP's Swagger marks TextPhoneNumberId optional, but an SMS send without it
+    // fails with an opaque 500 ("required and must be populated").
+    if (c.CommunicationType === 'SMS') {
+        return {
+            ...fields,
+            CommunicationType: 'SMS',
+            TextPhoneNumberId: sanitizeNumericId(c.TextPhoneNumberId, 'TextPhoneNumberId'),
+        };
+    }
+
+    const payload: CommunicationInfo = {
+        ...fields,
+        CommunicationType: c.CommunicationType as Exclude<CommunicationType, 'SMS'>,
+    };
     if (c.TextPhoneNumberId !== undefined) {
         payload.TextPhoneNumberId = sanitizeNumericId(c.TextPhoneNumberId, 'TextPhoneNumberId');
     }

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

@@ -33,20 +33,74 @@ export interface FileDescription {
   InclusionType: 'Attachment' | 'Link';
 }
 
-export interface CommunicationInfo {
+/**
+ * The `CommunicationType` values this template lets `POST /communications`
+ * send: a deliberate subset of MP's `Platform.Messaging.CommunicationType`
+ * enum, which is `Unknown | Email | SMS | RssFeed | GlobalMFA`.
+ *
+ * Verified against a live MP tenant on 2026-09-08 rather than taken from the
+ * Swagger alone: `dp_Communication_Types` holds `1 Email`, `2 SMS Text`,
+ * `3 RSS Feed` and `4 GlobalMFA`; `Unknown` is the zero value and has no row.
+ * Only `Email` and `SMS` are permitted: `Unknown` is not a real channel,
+ * `GlobalMFA` is MP's own multi-factor channel, and `RssFeed` is not a message
+ * to contacts. `CommunicationService` enforces the same list at runtime.
+ *
+ * This was previously typed `'Email' | 'Text' | 'Letter'`. Neither `'Text'`
+ * nor `'Letter'` was ever a member, 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."}
+ * ```
+ *
+ * `'Email'` happens to be a real member, and it is the only value anyone had
+ * ever passed — which is why the other two went unnoticed.
+ *
+ * Only ever add a value that is a member of MP's enum.
+ */
+export const COMMUNICATION_TYPES = ['Email', 'SMS'] as const;
+
+export type CommunicationType = (typeof COMMUNICATION_TYPES)[number];
+
+interface CommunicationInfoBase {
   AuthorUserId: number;
   Body: string;
   FromContactId: number;
   ReplyToContactId: number;
-  CommunicationType: 'Email' | 'Text' | 'Letter';
   Contacts: number[];
   IsBulkEmail: boolean;
   SendToContactParents: boolean;
   Subject: string;
   StartDate: string;
-  TextPhoneNumberId?: number;
 }
 
+/**
+ * A communication to hand to `POST /communications`.
+ *
+ * Modelled as a discriminated union rather than one interface because
+ * `TextPhoneNumberId` is **conditionally required**: MP's Swagger marks it
+ * optional, but an `'SMS'` send without it fails with
+ * `Property 'TextPhoneNumberId' is required and must be populated` — another
+ * 500, not a 400. Making the compiler ask for it turns a runtime failure into
+ * a build failure; `CommunicationService.createCommunication` re-checks at
+ * runtime for callers that arrive through an `as` cast or untyped JSON.
+ */
+export type CommunicationInfo =
+  | (CommunicationInfoBase & {
+      CommunicationType: 'SMS';
+      /**
+       * `dp_SMS_Numbers.SMS_Number_ID` of the outbound number. Required for
+       * SMS; a tenant's numbers are listed in `dp_SMS_Numbers`.
+       */
+      TextPhoneNumberId: number;
+    })
+  | (CommunicationInfoBase & {
+      CommunicationType: 'Email';
+      TextPhoneNumberId?: number;
+    });
+
 export interface MessageAddress {
   DisplayName: string;
   Address: string;
@@ -66,8 +120,15 @@ export interface MessageInfo {
  * {@link CommunicationInfo} without the author/from fields, which are stamped
  * from the separate trusted `CommunicationSender` argument instead. Values for
  * them left on the object are ignored.
+ *
+ * The `Omit` is applied to each member of the union separately. A plain
+ * `Omit<CommunicationInfo, ...>` merges the members, so an `'SMS'` payload
+ * without `TextPhoneNumberId` would compile again.
  */
-export type CommunicationContent = Omit<CommunicationInfo, 'AuthorUserId' | 'FromContactId'>;
+export type CommunicationContent = DistributiveOmit<CommunicationInfo, 'AuthorUserId' | 'FromContactId'>;
+
+/** `Omit` applied to each member of a union rather than to the merged union. */
+type DistributiveOmit<T, K extends PropertyKey> = T extends unknown ? Omit<T, K> : never;
 
 /**
  * What a caller of `MPHelper.sendMessage` supplies: a {@link MessageInfo}