[Dotdigital] Additional actions #3018
Conversation
Fix case for mobile identifiers
There was a problem hiding this comment.
Hi @pvpcookie thanks for the PR.
I've provided feedback - please take a look.
Nothing major, just lots of minor things, repeated in a few places.
Best regards,
Joe
| from: { | ||
| label: 'From', | ||
| description: 'Enter a custom From name, or leave blank to use a random number. From name format varies by region. [Learn more](https://support.dotdigital.com/en/articles/8199187-sender-ids-and-originators)', | ||
| type: 'string', | ||
| required: false, | ||
| }, |
There was a problem hiding this comment.
Could or should this be a dynamic dropdown list?
| } | ||
| }, | ||
| from: { | ||
| label: 'From', |
There was a problem hiding this comment.
| label: 'From', | |
| label: 'From Name', |
| defaultSubscription: 'type = "track" and event = "Send Transactional SMS"', | ||
| fields: { | ||
| to: { | ||
| label: 'To', |
There was a problem hiding this comment.
| label: 'To', | |
| label: 'To Phone Number', |
| }, | ||
| message: { | ||
| label: 'Message', | ||
| description: 'The content of the SMS, up to 160 non-encoded characters per message.', |
There was a problem hiding this comment.
What happens if the message is more than 160 chars?
| const checkAndCleanMobileNumber = (mobileNumber: string) => { | ||
| mobileNumber = mobileNumber.replace(' ', '').replace('-', '') | ||
| if (mobileNumber && mobileNumber.startsWith('+')) { | ||
| mobileNumber = mobileNumber.replace('+', '') | ||
| } | ||
|
|
||
| return mobileNumber | ||
| } |
There was a problem hiding this comment.
I think we should throw an error if the phone number is invalid.
For example:
throw new PayloadValidationError('Invalid To Phone Number value')
There was a problem hiding this comment.
Something like this might be a better check
function removeNonNumeric(mobileNumber: string): string {
const numericOnly = mobileNumber.replace(/\D+/g, '');
if (numericOnly.length < MIN_PHONE_NUMBER_CHAR_LENGTH) {
throw new PayloadValidationError('Invalid To Phone Number value')
}
return numericOnly
}
| fields: { | ||
| email: { | ||
| label: 'Email', | ||
| description: "If the contact does not exist in your Dotdigital account, the campaign won't be sent.", |
There was a problem hiding this comment.
Does this field support multiple email addresses or a single address? Can we make this clear?
| try{ | ||
| const sendCampaignPayload: sendCampaignPayload = { | ||
| campaignID: campaignId, | ||
| contactIDs: [contactId], | ||
| sendDate: sendDate | ||
| } | ||
| let endpoint = `/v2/campaigns/send` | ||
|
|
||
| if(sendTimeOptimised) { | ||
| endpoint = `/v2/campaigns/send-time-optimised` | ||
| delete sendCampaignPayload.sendDate | ||
| } | ||
|
|
||
| const response: ModifiedResponse = await this.post( | ||
| endpoint, sendCampaignPayload | ||
| ); | ||
| return response; | ||
| } catch (error) { | ||
| throw error as APIError ?? 'Failed to send campaign'; | ||
| } |
There was a problem hiding this comment.
Please remove the try/catch block around your request.
It should look something like this:
```
const sendCampaignPayload: sendCampaignPayload = {
campaignID: campaignId,
contactIDs: [contactId],
sendDate: sendDate
}
let endpoint = `/v2/campaigns/send`
if(sendTimeOptimised) {
endpoint = `/v2/campaigns/send-time-optimised`
delete sendCampaignPayload.sendDate
}
const response: ModifiedResponse = await this.post(
endpoint, sendCampaignPayload
)
return response
| password: 'api_password' | ||
| } | ||
|
|
||
| describe('Send sms', () => { |
There was a problem hiding this comment.
Recommend adding more tests to check for wrong size message body and bad phone numbers.
| try { | ||
| const toAddresses = cleanEmails(payload.toAddresses) | ||
| const ccAddresses = cleanEmails(payload.ccAddresses) | ||
| const bccAddresses = cleanEmails(payload.bccAddresses) | ||
| const fromAddress = checkAndCleanEmail(payload.fromAddress) | ||
| return await this.post('/v2/email', { | ||
| ToAddresses: toAddresses, | ||
| CCAddresses: ccAddresses, | ||
| BCCAddresses: bccAddresses, | ||
| Subject: payload.subject, | ||
| FromAddress: fromAddress, | ||
| HtmlContent: payload.htmlContent, | ||
| PlainTextContent: payload.plainTextContent | ||
| }) | ||
| } catch (error) { | ||
| throw (error as APIError) ?? 'Failed to send transactional email' | ||
| } |
There was a problem hiding this comment.
Please remove the try/catch block. Segment will surface any errors or successes to the customer.
It should look something like this.
const toAddresses = cleanEmails(payload.toAddresses)
const ccAddresses = cleanEmails(payload.ccAddresses)
const bccAddresses = cleanEmails(payload.bccAddresses)
const fromAddress = checkAndCleanEmail(payload.fromAddress)
return await this.post('/v2/email', {
ToAddresses: toAddresses,
CCAddresses: ccAddresses,
BCCAddresses: bccAddresses,
Subject: payload.subject,
FromAddress: fromAddress,
HtmlContent: payload.htmlContent,
PlainTextContent: payload.plainTextContent
})
| ## What's being changed | ||
|
|
||
| Lorem ipsum... | ||
|
|
||
| ## Why it's being changed | ||
|
|
||
| Lorem ipsum... | ||
|
|
||
| ## How to review / test this change | ||
|
|
||
| - Lorem | ||
| - ipsum | ||
|
|
||
| ## Notes | ||
|
|
||
| Optional |
There was a problem hiding this comment.
Please remove this file (I wasn't able to figure out what it's for!)
|
Also, tests are failing: You can run tests using this command:
Also, please run this command and commit the changes Also, please check that there's no errors when you run this command: |
What's being changed
we have added 4 new actions:
Why it's being changed
We wanted to add the ablility to send email and sms's via the segment io actions system.