-
Notifications
You must be signed in to change notification settings - Fork 71
Messagebird Handler Updates #841
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
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #841 +/- ##
==========================================
+ Coverage 74.65% 74.66% +0.01%
==========================================
Files 113 113
Lines 13308 13326 +18
==========================================
+ Hits 9935 9950 +15
- Misses 2658 2660 +2
- Partials 715 716 +1 ☔ View full report in Codecov by Sentry. |
@@ -62,14 +62,16 @@ var statusMapping = map[string]courier.MsgStatus{ | |||
"expired": courier.MsgStatusFailed, | |||
} | |||
|
|||
type ReceivedMessage struct { |
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.
Why not keep expecting the JSON format for this handler?
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.
the default webhook on MB is urlencoded, we were using a internal flow with them that turned it to json (among other things) before we found out they charge extra for these.
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.
Changing that will require you update each existing callback to use the default urlencoded again, I imagine those are now set to use JSON when submitting
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.
surprisingly sending with MB is either json or urlencoded, so that doesn't need to change. Since we only use them for shortcodes, we have a patch of our current courier with some of these changes just so we could move back to their default webhook to cut our SMS costs
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.
I think we can keep expecting JSON, and make that clear on the config page https://github.com/nyaruka/rapidpro/blob/main/temba/channels/types/messagebird/type.py#L35 that the callback should be configured with JSON
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.
Let's take a look and see if we have customers using messagebird that way... it would be nice to support whatever the default setup is for this channel.
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.
its basically like the difference between using the twilio webhook, or using a twilio function to convert the message to json. there's a cost associated that they don't make clear. the actual webhook from the number is always form encoded, you have to manually set up a flow (function) to convert. We did that before we knew there was a cost per run of that flow.
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.
Ok we have no channels of this type in prod probably because the type has always been beta gated. Since the default way to connect this channel sends urlencoded params, that's what we should support. And when we've merged this and tested it.. let's un- beta gate it.
Updated to support Messagebird's default webhook receiving handlers, with both shortcodes and regular numbers.