feat(notification): add Twillio notification provider #344
Conversation
Adds Twilio (https://www.twilio.com/) as a notification provider so that users outside of the US can receive stock notifications via SMS.
| if (notifications.twilio.accountSid && notifications.twilio.authToken) { | ||
| Logger.debug('↗ sending twilio message'); | ||
| sendTwilioMessage(link, store); | ||
| } |
There was a problem hiding this comment.
Adding this causes a code complexity linting warning. Not sure on how you'd prefer this be solved. Does this function need a refactor?
There was a problem hiding this comment.
Nah, that's ok. We need to separate these out at some point. It's probably just complaining because of the line length within this function.
| if (notifications.twilio.accountSid && notifications.twilio.authToken) { | ||
| Logger.debug('↗ sending twilio message'); | ||
| sendTwilioMessage(link, store); | ||
| } |
There was a problem hiding this comment.
Nah, that's ok. We need to separate these out at some point. It's probably just complaining because of the line length within this function.
|
this is making the latest build fail, as it appears a Twilio username is now required? |
|
can confirm, latest build wont start now. I dont have any mentions of Twilio in my .env, but its crashing saying error:username is required for Twilio. |
|
|
||
| const config = Config.notifications.twilio; | ||
|
|
||
| const client = twilio(config.accountSid, config.authToken); |
There was a problem hiding this comment.
@jef Sorry - only just seen this was merged. Didn't get GitHub email notifications for some reason.
I've seen reports of users having issues since this has been merged, saying that they are getting an error that Twilio credentials are needed even when they haven't set any of the Twilio config options.
I have a suspicion that this line is the cause. We should move it so we are only initalizing the client inside sendTwilioMessage. I'll try and get a PR up later.
There was a problem hiding this comment.
Ah great, thanks.
Sorry for the trouble
Description
Adds Twilio (https://www.twilio.com/) as a notification provider so that users outside of the US can receive stock notifications via SMS.
Fixes #342
Discord: Wills#6846
Testing
npm run test:notificationand received the test SMS.test:seriesin sock viaSHOW_ONLY_SERIES="test:series".New dependencies