diff --git a/src/lib/providers/ministry-platform/docs/README.md b/src/lib/providers/ministry-platform/docs/README.md index 664a4682..7310f904 100644 --- a/src/lib/providers/ministry-platform/docs/README.md +++ b/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 diff --git a/src/lib/providers/ministry-platform/services/communication.service.test.ts b/src/lib/providers/ministry-platform/services/communication.service.test.ts index ecb5ccfd..ef4e480c 100644 --- a/src/lib/providers/ministry-platform/services/communication.service.test.ts +++ b/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).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).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); + }); +}); diff --git a/src/lib/providers/ministry-platform/services/communication.service.ts b/src/lib/providers/ministry-platform/services/communication.service.ts index 2e5c53a0..2d89bbff 100644 --- a/src/lib/providers/ministry-platform/services/communication.service.ts +++ b/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 = new Set(['Email', 'Text', 'Letter']); +/** The `CommunicationType` values that may be sent; see `COMMUNICATION_TYPES`. */ +const ALLOWED_COMMUNICATION_TYPES: ReadonlySet = 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; const c = communication as Record; - 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, + }; if (c.TextPhoneNumberId !== undefined) { payload.TextPhoneNumberId = sanitizeNumericId(c.TextPhoneNumberId, 'TextPhoneNumberId'); } diff --git a/src/lib/providers/ministry-platform/types/provider.types.ts b/src/lib/providers/ministry-platform/types/provider.types.ts index b61e9cbd..97efd331 100644 --- a/src/lib/providers/ministry-platform/types/provider.types.ts +++ b/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` merges the members, so an `'SMS'` payload + * without `TextPhoneNumberId` would compile again. */ -export type CommunicationContent = Omit; +export type CommunicationContent = DistributiveOmit; + +/** `Omit` applied to each member of a union rather than to the merged union. */ +type DistributiveOmit = T extends unknown ? Omit : never; /** * What a caller of `MPHelper.sendMessage` supplies: a {@link MessageInfo}