feat(6228): add aws sms integration#6427
Conversation
chadbrokaw
changed the title
✅ Deploy Preview for partners-bloom-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for bloom-angelopolis canceled.
|
✅ Deploy Preview for bloom-public-seeds ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
✅ Deploy Preview for bloom-exygy-dev ready!
To edit notification comments on pull requests, go to your Netlify project configuration. |
There was a problem hiding this comment.
Pull request overview
Adds an AWS End User Messaging (Pinpoint SMS Voice V2) SMS delivery path to the API as an alternative to the existing Twilio integration, selected via SMS_PROVIDER at runtime (defaulting to Twilio to preserve existing behavior).
Changes:
- Introduces AWS Pinpoint SMS Voice V2 client support in
SmsService, gated bySMS_PROVIDER=aws. - Adds unit tests covering both Twilio (default) and AWS provider initialization + send behavior.
- Adds required dependency and documents new SMS-related env vars in the API
.env.template.
Reviewed changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| api/src/services/sms.service.ts | Adds AWS client initialization and AWS send path alongside existing Twilio logic. |
| api/test/unit/services/sms.service.spec.ts | New unit tests for Twilio default behavior and AWS provider behavior. |
| api/.env.template | Documents SMS_PROVIDER and AWS SMS configuration env vars. |
| api/package.json | Adds @aws-sdk/client-pinpoint-sms-voice-v2 dependency. |
| api/yarn.lock | Locks new AWS SMS client and transitive dependencies. |
| await this.awsClient.send( | ||
| new SendTextMessageCommand({ | ||
| DestinationPhoneNumber: phoneNumber, | ||
| OriginationIdentity: process.env.AWS_SMS_ORIGINATION_NUMBER, | ||
| MessageBody: `Your Partners Portal account access token: ${singleUseCode}`, | ||
| MessageType: 'TRANSACTIONAL', | ||
| }), | ||
| ); |
| export class SmsService { | ||
| client: TwilioClient; | ||
| awsClient: PinpointSMSVoiceV2Client; |
| "translations:sql": "ts-node scripts/generate-db-translation-sql.ts" | ||
| }, | ||
| "dependencies": { | ||
| "@aws-sdk/client-pinpoint-sms-voice-v2": "^3.1003.0", |
There was a problem hiding this comment.
This is really confusing. According to this document pinpoint will no longer be supported. But it appears like the SDK is still the pinpoint one. So that isn't confusing at all
| public constructor() { | ||
| if (process.env.TWILIO_ACCOUNT_SID && process.env.TWILIO_AUTH_TOKEN) { | ||
| if (process.env.SMS_PROVIDER === 'aws') { | ||
| if (process.env.AWS_SMS_REGION) { |
There was a problem hiding this comment.
suggestion: It would be useful to add a WARN log here if the region is not defined but SMS provider is AWS.
| ): Promise<void> { | ||
| if (!this.client) { | ||
| return; | ||
| if (process.env.SMS_PROVIDER === 'aws') { |
There was a problem hiding this comment.
suggestion: I'm cool with the way this is setup for this PR since we only have one use case for sending SMS messages. But when we get even one more use case it would be good to have just one function to call that sends the SMS no matter what SMS provider we have. A good example is how we are doing emails with the email-provider service.
But that can hold off to future PRs if you want to isolate that change
| if (!this.client) { | ||
| return; | ||
| if (process.env.SMS_PROVIDER === 'aws') { | ||
| if (!this.awsClient) { |
There was a problem hiding this comment.
Same here about logging. We should have a log that warns an SMS message was attempted but no client exists
This PR addresses #6228
Description
SMS_PROVIDERenv var (twiliooraws); defaults totwilioso existing behavior is unchangedHow Can This Be Tested/Reviewed?
This is part 1 of the ticket and will need part 2 in order to be fully operable. For now, please inspect the code and ensure it looks good.
Author Checklist:
yarn generate:clientand/or created a migration when requiredReview Process: