Skip to content
This repository has been archived by the owner. It is now read-only.

feat(mailer): add support for sending SMS messages #254

Closed
wants to merge 1 commit into from

Conversation

@philbooth
Copy link
Contributor

@philbooth philbooth commented Feb 1, 2017

Related to mozilla/fxa-auth-server#1628.

  • Define the API.
  • Include SMS messages in the auth mailer templates for l10n.
  • Implement phone number validation.
  • Unit tests.
  • Fix Invalid Text Message error from Nexmo.
  • Test out sending actual messages to the UK, the USA and Canada.
  • Export the sms interface for direct calling.
  • Include the proper install link.
  • Include the finalised message text.
  • Include the finalised sender ids.

/cc @shane-tomlinson @vbudhram

@philbooth philbooth added this to the FxA-53: Email Confirmation Flow - SMS (Phase 2) milestone Feb 1, 2017
@philbooth philbooth self-assigned this Feb 1, 2017
@philbooth philbooth force-pushed the phil/send-sms branch 3 times, most recently from 8f8431c to 0f63622 Feb 2, 2017
@philbooth philbooth force-pushed the phil/send-sms branch from 0f63622 to 0732b87 Feb 2, 2017
@@ -0,0 +1,5 @@
[
[ "CA", "16474909977" ],

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Feb 2, 2017
Member

What're these?

This comment has been minimized.

@philbooth

philbooth Feb 2, 2017
Author Contributor

What're these?

Virtual numbers that I'm using for testing.

As per the documentation, legal values for the sender id depend on the region we're sending to. For the UK we can use an alphanumeric string up to 11 characters long, hence Firefox. For the USA it has to be a phone number, so I just plugged in the virtual phone number I'm using for USA testing, for now. For Canada, we should be able to use the short code that was already associated with our Nexmo account, according to the docs. However, in practice I found this got rejected by the network provider, so I've switched to the virtual number I'm using for testing there too.

For all of this, note the last item in the issue check-list: Include the finalised sender ids. It's one of the things we need to pin down before this PR can be merged.

const sendSms = P.promisify(nexmo.message.sendSms, { context: nexmo.message })

// TODO: Sort out the real link
const LINK = 'https://moz.la/vwxyz'

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Feb 2, 2017
Member

Ya, we need a real link. @davismtl - what adjust link should we point people at? Should we run it through the moz.la shortener?

This comment has been minimized.

@davismtl

davismtl Feb 2, 2017

Hey, I recommend looking at this document: https://docs.google.com/spreadsheets/d/151_fWwEPxmPN_AbHCp-sqAkshDCLaABRrVaowxoFS10/edit#gid=0

This is our Adjust master tracker for owned channels. You should both have edit access now.

There are some rows with FxA in there. You will see the nomenclature of the URLs.

I recommend shortner too since it would be too long in SMS.

This comment has been minimized.

@davismtl

davismtl Feb 2, 2017

And here are the deeplinking parameters too for the next phases: https://docs.adjust.com/en/deeplinking/

Copy link
Member

@shane-tomlinson shane-tomlinson left a comment

👍 I'm excited to see this come together @philbooth!

const numberUtil = libpn.PhoneNumberUtil.getInstance()
const parsedNumber = numberUtil.parse(phoneNumber)

if (! numberUtil.isValidNumber(parsedNumber)) {

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Feb 2, 2017
Member

I'm assuming this will also be done on the auth server.

This comment has been minimized.

@philbooth

philbooth Feb 2, 2017
Author Contributor

Why?

This comment has been minimized.

@philbooth

philbooth Feb 2, 2017
Author Contributor

Or did you mean that you assume some validation will be done on the auth server, rather than explicitly this validation?

I do have rudimentary validation at the API level, yes. /^\+[1-9]\d{1,14}$/ is the regex, if you're interested.

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Feb 2, 2017
Member

Or did you mean that you assume some validation will be done on the auth server,

Yes.

const status = result.status

if (status !== '0') {
fail(`Delivery failed: ${status} ${result['error-text']}`, 500)

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Feb 2, 2017
Member

This error will be returned to the auth-server I'm guessing. What does the auth-server do with it? Does the auth-server convert errors to the standardized format where each error gets its own errno?

This comment has been minimized.

@philbooth

philbooth Feb 2, 2017
Author Contributor

The auth server work isn't finished yet. When it is, it will return an appropriate error.

apiKey: config.apiKey,
apiSecret: config.apiSecret
})
const sendSms = P.promisify(nexmo.message.sendSms, { context: nexmo.message })

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Feb 2, 2017
Member

This may be pre-mature, any value in passing in the sendSms function so that we can swap out providers if need be, e.g., if we decide to use Basket or Amazon SNS instead of Nexmo? Basket may duplicate a lot of the functionality here, I'm not sure how it works.

This comment has been minimized.

@philbooth

philbooth Feb 2, 2017
Author Contributor

I don't see the point in doing it now. It's a possibility I'm aware of and that we spoke about, but writing code to handle multiple SMS APIs feels like wasted cycles at this stage. I don't foresee it being difficult to do, if/when we need it. None of the implementation/provider details leak outside of this module.

@@ -0,0 +1 @@
sms.1.txt

This comment has been minimized.

@shane-tomlinson

shane-tomlinson Feb 2, 2017
Member

What's this file for? Necessary to load the templates?

This comment has been minimized.

@philbooth

philbooth Feb 2, 2017
Author Contributor

The generic template code assumes HTML versions of every template. Obviously that makes no sense for SMS, so I made it a symlink, which is what this is. But it doesn't need to be a symlink, it could be an empty file, or anything really.

Or I could add some conditional logic to the template code that ignores HTML files for sms.* templates, but that felt a bit like unnecessary complexity when I thought about it before.

@philbooth philbooth closed this Feb 2, 2017
@philbooth philbooth deleted the phil/send-sms branch Feb 2, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.