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

Added Twilio SMS as an Alerting Transport #9305

Merged
merged 6 commits into from Oct 17, 2018

Conversation

Projects
None yet
5 participants
@andyrosen
Contributor

andyrosen commented Oct 7, 2018

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926
After you are done testing, you can remove the changes with ./scripts/github-remove. If there are schema changes, you can ask on discord how to revert.

@CLAassistant

This comment has been minimized.

CLAassistant commented Oct 7, 2018

CLA assistant check
All committers have signed the CLA.

@jozefrebjak

This comment has been minimized.

Contributor

jozefrebjak commented Oct 8, 2018

I am testing this, and I think there is few things to fix it. So firstly I think it will be better to add to documentation that you need use a number with prefix in SMS To Number: for example in my case, +421 with number, and in Twilio SMS Number we don't need to use prefix for number. For example I used number with prefix +1 . Second thing is that this is not sending a TEXT from alert template, but only a ALERT TITLE is this what you want ? Because if not, you need to change:
In line 45 LibreNMS/Alert/Transport/Twilio.php
from:
'text' => $obj['title'],
to:
'text' => $obj['msg'],
I changed this in my environment and it is working perfect for me.

I applied this pull request and in WEB UI I can use this transport in ALERT TRANSPORT but I can't see this option in Global Setting in a tab Alerting Setting.

@@ -67,6 +67,7 @@
<option value="smsfeedback-form">SMSFeedback</option>
<option value="syslog-form">Syslog</option>
<option value="telegram-form">Telegram</option>
<option value="twilio-form">Twilio</option>

This comment has been minimized.

@laf

laf Oct 8, 2018

Member

We use spaces for indentation, please align it to the options above / below.

'twilio-sid' => 'required|string',
'twilio-token' => 'required|string',
'twilio-to' => 'required',
'twilio-sender' => 'required|string',

This comment has been minimized.

@laf

laf Oct 8, 2018

Member

Should this be string if it's a mobile number or can you set it to anything you want?

'type' => 'text',
],
[
'title' => 'SMS To Number',

This comment has been minimized.

@laf

laf Oct 8, 2018

Member

Maybe change this to Mobile Number

$ret = curl_exec($curl);
if (curl_getinfo($curl,CURLINFO_RESPONSE_CODE))

This comment has been minimized.

@laf

laf Oct 8, 2018

Member

We require brackets {} around if statements.

return $this->contactsmsfeedback($obj, $opts);
}
public static function contacttwilio($obj, $opts)

This comment has been minimized.

@laf

laf Oct 8, 2018

Member

Please rename this to contactTwilio()

'sender' => $opts['sender'],
);
$url = 'https://api.twilio.com/2010-04-01/Accounts/' . $params['sid'] . '/Messages.json';

This comment has been minimized.

@laf

laf Oct 8, 2018

Member

Looks like you need to fix quite a bit of the indentation. 4 spaces rather than 1 tab.

{
public function deliverAlert($obj, $opts)
{
if (empty($this->config)) {

This comment has been minimized.

@laf

laf Oct 8, 2018

Member

This is only needed for legacy transports, as this is new then it won't have legacy config. Remove this check + the devliverAlertOld() function

@@ -433,6 +433,17 @@ Click on 'Add Telegram config' and put your chat id and token into the relevant
| Chat ID | 34243432 |
| Token | 3ed32wwf235234 |
## Twilio SMS
Twilio will send your alert via SMS. From your Twilio account you will need your account SID, account token and your Twilio SMS phone number that you would like to send the alerts from.

This comment has been minimized.

@laf

laf Oct 8, 2018

Member

Can you link to the Twilio developer docs so people have a reference point?

@laf laf added the Alerting 🔔 label Oct 8, 2018

@laf

This comment has been minimized.

@laf laf added this to the 1.45 milestone Oct 8, 2018

@andyrosen

This comment has been minimized.

Contributor

andyrosen commented Oct 8, 2018

Thank you. I just pushed the requested changes.

ps This is my first open source contribution, so I apologize for any rookie mistakes...

andyrosen and others added some commits Oct 8, 2018

@laf

This comment has been minimized.

Member

laf commented Oct 9, 2018

ps This is my first open source contribution, so I apologize for any rookie mistakes...

Don't apologise, you've done a top job :)

I just made a couple of small changes + reintroduced the deliverAlert() function as you only need to remove part of it rather than all of it.

@laf

laf approved these changes Oct 17, 2018

LGTM

@laf laf merged commit 4e5a0fa into librenms:master Oct 17, 2018

2 of 3 checks passed

codeclimate 3 issues to fix
Details
WIP ready for review
Details
license/cla Contributor License Agreement is signed.
Details

murrant added a commit that referenced this pull request Oct 18, 2018

@murrant

This comment has been minimized.

Member

murrant commented Oct 28, 2018

This pull request has been mentioned on LibreNMS Community. There might be relevant details there:

https://community.librenms.org/t/v1-45-release-changelog-october-2018/6016/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment