lunemec/nanny

Add SMS option for notifiers

Closed this issue · 7 comments

turnkey-commerce commented

Would be nice to also have a texting (SMS) option for cases when email isn't available or desired. Having integration with a service like Twilio could also open other notifications possibilities.

lunemec commented

Could you please test this? #8

I'm unable to create trial twilio number for SMS sending (for some reason, my test number is voice only). I've contacted their support, but it may take some time.

Also, code review is welcome.

turnkey-commerce commented

Thanks for providing that, I'll try to test it this evening.

turnkey-commerce commented

Hi Lukas, I was able to get it to send an sms to my cell phone successfully. However, there was a Segment violation error right after it sent that crashed nanny. Here is the stack trace:

panic: runtime error: invalid memory address or nil pointer dereference
[signal SIGSEGV: segmentation violation code=0x1 addr=0x0 pc=0x8504a3]

goroutine 24 [running]:
nanny/pkg/notifier.(*twilio).Notify(0xc4203fa480, 0xc42038f858, 0x5, 0xc420024b00, 0x14, 0x12a05f200, 0x0, 0xc42001f1b0, 0xc42001f190)
/home/james/Documents/go/src/nanny/pkg/notifier/twilio.go:29 +0xc3
nanny/pkg/nanny.(*Nanny).handle.func1()
/home/james/Documents/go/src/nanny/pkg/nanny/nanny.go:93 +0x161
created by time.goFunc
/usr/local/go/src/time/sleep.go:172 +0x44

turnkey-commerce commented

A couple of other things to note:

  1. The appSid setting in the TOML seems redundant. I left that blank and the notification worked fine.
  2. I noticed on Twilio I had to buy a number to get it to work to supply the From field.
lunemec commented

@turnkey-commerce thanks! I will look at the panic.

The appSid is there in case you have multiple applications in twilio and want to use this field. Twilio API supports it, so why not have it. If the user doesn't need it, it may be left empty.

You have to have Twilio number capable of sms sending, which you use as From number. I guess there aren't any free that support this (at least I haven't found it).

lunemec commented

@turnkey-commerce I fixed the panic, it should work correctly now. Can you pull the branch and test again? Thank you.

turnkey-commerce commented

@lunemec Thanks for the info about the appSid. I tested the fix and it now sends text messages to my phone with no problems. I forced 3 messages over a few minutes during a single sessions with no crashes. Thanks for the fix.