-
Notifications
You must be signed in to change notification settings - Fork 18
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.
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
Update successful delivery status for sms only if error does not exist #1150
Conversation
@miazima just wondering, does Twilio handle retries of delivery internally? My main concern for this is that if it does retry, then we should actually update with the latest status. |
As discussed, Twilio does internally retry the failed requests but the callbacks that are received are the finalised delivery status of the message. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm! Tested both scenarios:
- Error response then success response should still remain as error
- Success response then error response should be updated to error
* develop: refactor: use shared function to initialize models (#1172) chore: setup scaffolding for backend tests (#940) 1.23.0 fix(frontend): fix frontend test flakiness (#1162) fix: update successful delivery status only if error does not exist (#1150) chore: upgrade dependencies (#1153) feat: add unit tests for error states in critical workflows (#1118) feat: support whitelisting domains through `agencies` table (#1141) feat: add tests for happy paths in critical workflows (#1110) fix: prevent campaign names from causing dashboard rows to overflow (#1147) fix(email): Fix SendGrid fallback integration (#1026)
* develop: feat: refactor msg template components; add telegram character limit (#1148) refactor: use shared function to initialize models (#1172) chore: setup scaffolding for backend tests (#940) 1.23.0 fix(frontend): fix frontend test flakiness (#1162) fix: update successful delivery status only if error does not exist (#1150) chore: upgrade dependencies (#1153) feat: add unit tests for error states in critical workflows (#1118) feat: support whitelisting domains through `agencies` table (#1141) feat: add tests for happy paths in critical workflows (#1110) fix: prevent campaign names from causing dashboard rows to overflow (#1147)
* develop: fix: fix error when updating Telegram ID for an existing phone number (#1178) chore: upgrade React; use new JSX transform; sort imports (#1129) fix(backend): docker build and tsc output directory structure (#1177) feat: refactor msg template components; add telegram character limit (#1148) refactor: use shared function to initialize models (#1172) chore: setup scaffolding for backend tests (#940) 1.23.0 fix(frontend): fix frontend test flakiness (#1162) fix: update successful delivery status only if error does not exist (#1150) chore: upgrade dependencies (#1153)
Problem
Some SMS messages delivery statuses logged contained both the error code and a
SUCCESS
status. This will be confusing to users who see this status in their delivery report and does not provide a conclusive delivery status of the message.Closes #1126
Solution
Bug Fixes:
SMS messages can be delivered in multiple segments when they exceed the character limit of one SMS. Hence each segment has separate deliverability and results in multiple callbacks. In the case where the first segment of the message failed to deliver but the next segment is successful, there will be a situation where the delivery status is overridden to
SUCCESS
but theerrorCode
still remains.Tests
/callback/sms
endpoint with a unsuccessful delivery status - observe that the message status reflectsERROR
in the db/callback/sms
endpoint with a successful delivery status - observe that the message status still reflectsERROR
in the db** You can replicate the bug by following this scenario on dev branch, but you will observe that the delivery status now reflects both the error code and a
SUCCESS
status.