From 66b157eebaa883d98f42b45dcff2debbb9c441ed Mon Sep 17 00:00:00 2001 From: jonnydcakes <32756543+jonnydcakes@users.noreply.github.com> Date: Sat, 19 Sep 2026 09:51:11 -0500 Subject: [PATCH] =?UTF-8?q?fix(mp):=20type=20CommunicationType=20as=20MP's?= =?UTF-8?q?=20real=20enum=20=E2=80=94=20'Text'=20and=20'Letter'=20500?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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. --- .../ministry-platform/docs/README.md | 6 + .../services/communication.service.test.ts | 149 ++++++++++++++++++ .../services/communication.service.ts | 33 +++- .../ministry-platform/types/provider.types.ts | 62 +++++++- 4 files changed, 245 insertions(+), 5 deletions(-) diff --git a/src/lib/providers/ministry-platform/docs/README.md b/src/lib/providers/ministry-platform/docs/README.md index c0c3549e..9feae9fe 100644 --- a/src/lib/providers/ministry-platform/docs/README.md +++ b/src/lib/providers/ministry-platform/docs/README.md @@ -187,6 +187,12 @@ mp.sendMessage(message: MessageInfo, attachments?: File[]): Promise { }); }); + /** + * `CommunicationType` mirrors MP's real enum, and an `'SMS'` send must carry + * `TextPhoneNumberId`. MP answers both mistakes 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', + Subject: 'Sunday update', + Body: 'Service is cancelled this Sunday.', + 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); + + 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()) + ).rejects.toThrow(/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]) + ).rejects.toThrow(/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 }) + ).rejects.toThrow(/TextPhoneNumberId/); + + expectNothingSent(); + }); + + it("should refuse the previously typed 'Text' value rather than let MP 500", async () => { + await expect( + communicationService.createCommunication({ + ...communicationInfo, + CommunicationType: 'Text', + } as unknown as CommunicationInfo) + ).rejects.toThrow(/Invalid CommunicationType "Text"/); + + expectNothingSent(); + }); + + it("should refuse the previously typed 'Letter' value as well", async () => { + await expect( + communicationService.createCommunication({ + ...communicationInfo, + CommunicationType: 'Letter', + } as unknown as CommunicationInfo) + ).rejects.toThrow(/Invalid CommunicationType "Letter"/); + + expectNothingSent(); + }); + + it('should name the accepted values so a caller can fix the payload without the Swagger', async () => { + await expect( + communicationService.createCommunication({ + ...communicationInfo, + CommunicationType: 'Text', + } as unknown as CommunicationInfo) + ).rejects.toThrow(/Unknown, Email, SMS, RssFeed, GlobalMFA/); + }); + }); + describe('sendMessage', () => { it('should POST JSON to /messages when there are no attachments', async () => { (mockHttpClient.post as ReturnType).mockResolvedValueOnce(createdCommunication); @@ -253,3 +359,46 @@ 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 the invented values 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', + }; + + // @ts-expect-error — 'SMS' requires TextPhoneNumberId. + const smsWithoutNumber: CommunicationInfo = { + ...email, + CommunicationType: 'SMS', + }; + + expect([invalidText, invalidLetter, smsWithoutNumber]).toHaveLength(3); + }); +}); diff --git a/src/lib/providers/ministry-platform/services/communication.service.ts b/src/lib/providers/ministry-platform/services/communication.service.ts index 5ce0b566..be77b1f5 100644 --- a/src/lib/providers/ministry-platform/services/communication.service.ts +++ b/src/lib/providers/ministry-platform/services/communication.service.ts @@ -1,5 +1,5 @@ import { MinistryPlatformClient } from "../client"; -import { CommunicationInfo, Communication, MessageInfo } from "../types"; +import { CommunicationInfo, Communication, MessageInfo, COMMUNICATION_TYPES } from "../types"; export class CommunicationService { private client: MinistryPlatformClient; @@ -10,11 +10,15 @@ export class CommunicationService { /** * Creates a new communication, immediately renders it and schedules for delivery. + * + * @throws {Error} before any network call when the payload is one MP would + * reject — see `assertSendable`. */ public async createCommunication( communication: CommunicationInfo, attachments?: File[] ): Promise { + assertSendable(communication); try { await this.client.ensureValidToken(); @@ -76,4 +80,29 @@ export class CommunicationService { return await this.client.getHttpClient().postFormData('/messages', formData); } -} \ No newline at end of file +} + +/** + * Reject a communication MP would reject, before spending a round trip on it. + * + * MP answers both of these with an HTTP **500** and a stringly-typed message + * rather than a 400, so without this the caller sees a server error with no + * indication that their payload was at fault. The type system already prevents + * both for ordinary callers; this catches the ones that arrive through an `as` + * cast, `JSON.parse`, or a value widened to `string` somewhere upstream. + */ +function assertSendable(communication: CommunicationInfo): void { + const type = communication.CommunicationType; + if (!(COMMUNICATION_TYPES as readonly string[]).includes(type)) { + throw new Error( + `Invalid CommunicationType "${type}". Ministry Platform accepts only: ` + + `${COMMUNICATION_TYPES.join(', ')}.` + ); + } + if (type === 'SMS' && !communication.TextPhoneNumberId) { + throw new Error( + "CommunicationType 'SMS' requires TextPhoneNumberId " + + '(dp_SMS_Numbers.SMS_Number_ID of the outbound number).' + ); + } +} diff --git a/src/lib/providers/ministry-platform/types/provider.types.ts b/src/lib/providers/ministry-platform/types/provider.types.ts index 62915790..bd10c95a 100644 --- a/src/lib/providers/ministry-platform/types/provider.types.ts +++ b/src/lib/providers/ministry-platform/types/provider.types.ts @@ -33,20 +33,76 @@ export interface FileDescription { InclusionType: 'Attachment' | 'Link'; } -export interface CommunicationInfo { +/** + * MP's `Platform.Messaging.CommunicationType` enum, exactly as + * `POST /communications` accepts it. + * + * 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. + * + * 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. + * + * Do not extend this by hand: it mirrors an MP enum. + */ +export const COMMUNICATION_TYPES = [ + 'Unknown', + 'Email', + 'SMS', + 'RssFeed', + 'GlobalMFA', +] 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: Exclude; + TextPhoneNumberId?: number; + }); + export interface MessageAddress { DisplayName: string; Address: string;