Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

feat: Add twilio webhook endpoint for failed SMS deliveries #3110

Merged
merged 15 commits into from
Dec 6, 2021
Merged

feat: Add twilio webhook endpoint for failed SMS deliveries #3110

merged 15 commits into from
Dec 6, 2021

Conversation

zatkiller
Copy link
Contributor

@zatkiller zatkiller commented Nov 29, 2021 •
edited
Loading

Problem

Currently, there is no error handling for failed delivery of Twilio SMSes.

Closes #3103

Solution

Add a new endpoint which can be exposed to the Twilio Webhook status updates and logs any SMS failures

Features:

  • New endpoint at v3/notifications/twilio which logs the errors if SMS delivery status is failed or undelivered
  • Validation of request from Twilio using Twilio SDK

Tests

  • Update sms.service tests to include statusCallback
  • Add twilio.controller.spec.ts
  • Add notifications.route.spec.ts

@zatkiller zatkiller requested a review from karrui November 29, 2021 09:18
karrui
karrui requested changes Dec 1, 2021
View reviewed changes
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

some changes requested. Can we also add tests for the new API?

src/app/modules/twilio/twilio.controller.ts Outdated Show resolved Hide resolved
src/app/modules/twilio/twilio.controller.ts Outdated Show resolved Hide resolved
src/app/modules/twilio/twilio.service.ts Outdated Show resolved Hide resolved
src/app/modules/twilio/twilio.service.ts Outdated Show resolved Hide resolved
src/app/modules/twilio/twilio.controller.ts Outdated Show resolved Hide resolved
src/app/modules/twilio/twilio.controller.ts Outdated Show resolved Hide resolved
src/app/services/sms/sms.service.ts Outdated Show resolved Hide resolved
src/app/services/sms/sms.service.ts Outdated Show resolved Hide resolved
src/types/twilio.ts Show resolved Hide resolved
@zatkiller zatkiller requested a review from karrui December 1, 2021 09:23
@zatkiller zatkiller temporarily deployed to staging December 2, 2021 09:23 Inactive
@zatkiller zatkiller temporarily deployed to staging December 3, 2021 09:00 Inactive
karrui
karrui approved these changes Dec 3, 2021
View reviewed changes
Copy link
Contributor

@karrui karrui left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm, a couple nits

src/app/modules/twilio/twilio.controller.ts Outdated Show resolved Hide resolved
src/app/services/sms/sms.service.ts Show resolved Hide resolved
@zatkiller zatkiller merged commit c1e499c into develop Dec 6, 2021
@zatkiller zatkiller deleted the feat/twilio-error-webhook branch December 6, 2021 04:52
@tshuli tshuli mentioned this pull request Dec 6, 2021
Merged
tshuli added a commit that referenced this pull request Dec 7, 2021
@tshuli
Revert "feat: Add twilio webhook endpoint for failed SMS deliveries (#…
70d81e0 
…3110)"

This reverts commit c1e499c.
@tshuli tshuli mentioned this pull request Dec 7, 2021
Merged
@zatkiller zatkiller mentioned this pull request Dec 7, 2021
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@karrui karrui karrui approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Create endpoint for Twilio webhooks
2 participants
@zatkiller @karrui