-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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.
Straight forward! Looks good to me.
Thanks!
if (notifications.twilio.accountSid && notifications.twilio.authToken) { | ||
Logger.debug('↗ sending twilio message'); | ||
sendTwilioMessage(link, store); | ||
} |
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.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
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:notification
and received the test SMS.test:series
in sock viaSHOW_ONLY_SERIES="test:series"
.New dependencies