Add Twilio message callbacks #77
Conversation
There was a problem hiding this comment.
Pulled and tested on my end. I'll let @tomerovadia take a look, but it looks good to me.
| } | ||
|
|
||
| // Update the Postgres DB with the message status and current timestamp | ||
| const slackMessageInfo = await logTwilioStatusToDb(messageSid, messageStatus); |
There was a problem hiding this comment.
Out of curiosity, why doesn't this need a catch? I see the function itself has a try/finally within it like other DBApiUtil functions, but I think we have try/catches around calls to other DBApiUtil functions elsewhere.
There was a problem hiding this comment.
We only need a try/catch if there's something we want to do when there's an error other than just return a 500. In this case, there's nothing to do -- if we can't query the database, we can't get the slack message info, so we can't add a reaction. So we might as well just let the error bubble up and return a 500 -- try/catch is just for when there's some useful fallback behavior or graceful failure mode.
There was a problem hiding this comment.
What about catching so we can log to Sentry?
| ` | ||
| UPDATE messages | ||
| SET | ||
| twilio_callback_status = $1, |
There was a problem hiding this comment.
What are your thoughts on modifying the successfully_sent field as well?
There was a problem hiding this comment.
I think we can leave successfully_sent as is -- to indicate that we sent the message off to Twilio. I'm not sure there's much use in updating both successfully_sent and the twilio_status when we get a twilio status callback.
This PR does a couple things:
1234.567000from slack gets stored as1234.567. But if you try to pass1234.567to the slack API when you're trying to do something with that message, Slack will say that's an invalid timestamp -- the timestamps are supposed to be strings with the same number of digits of precision as slack tell you, so dropping the trailing 0's makes it invalid. See this for details: Timestamp type inconsistency slackhq/slack-api-docs#7 (comment)IMPORTANT: this now requires the bot to have the
reactions:writepermission. I've added this to the VoteAmerica staging and prod instances, but you'll have to add it to your dev instances to test this out.