feat(server): implement GET /sms/status

[philbooth]

Fixes #1646, implementing an endpoint for the content server to call before showing the SMS view.

@mozilla/fxa-devs r?

/cc @shane-tomlinson

[philbooth]

Hmm, tests are failing, reviewers hold your fire!

[philbooth]

Breakage appears to be caused by recent db changes, I've updated shrinkwrap here. I could just change the assertions but I'd be happier if the person that updated the db could do it, just to be sure everything is as expected.

Closing this until #1682 is fixed.

[philbooth]

Reviewers, fill your boots!

[philbooth]

I may need to change this depending on the outcome of @shane-tomlinson's conversation with legal.

[philbooth]

I wrote this comment before I changed the name of that file in #1676 but only just spotted the mismatch, sorry.

[shane-tomlinson]

I haven't tested by integrating into the front end, but the API looks exactly as expected. A couple of small nits, nothing that I think really needs to be re-reviewed.

r+ - merge when ready.

[shane-tomlinson]

Is the getLocation argument used?

[philbooth]

That looks like some kind of weird copy/paste error.

[shane-tomlinson]

I realize there are only a couple of function calls, but I found the isOk check in getLocation a bit confusing before my brain has fully woken up. An alternative that might be easier to follow is to use Promise.all then array destructuring or Promise.spread on the result:

return Promise.all([
  getStatus(),
  getLocation()
])
.spread(createResponse)
.then(reply, reply);

function getStatus () {
  return sms.status()
    .then(statuts => status.isOk)
    .catch(...);
}

function getLocation () {
  return getGeoData(request.app.clientAddress)
    .then(result => REGIONS.test(result.location.countryCode))
    .catch(...);
}

function createResponse(isStatusOk, isLocationOk) {
  return { ok: isStatusOk && isLocationOk };
}

[philbooth]

The point is to shortcut the call to getGeoData if we already know the status is false.

[philbooth]

The shortcut should be inverted actually, it's more likely that the getGeoData check will fail than the balance check.

[shane-tomlinson]

Think it makes sense to abstract the different country tests? The logic between this and the US variant are pretty similar.

[philbooth]

Think it makes sense to abstract the different country tests?

Although I'm in favour of abstracting setup and teardown, I see abstracted assertions as a massive anti-pattern. They make it harder for the casual reader to quickly grok the invariants and they encourage exhaustive testing, which can obscure where the important boundary conditions are covered.

I much prefer to only assert the important stuff, and to do it all longhand so that readers can quickly understand what the edge cases are.

^imho

[shane-tomlinson]

Can you document this endpoint in API.md?

[shane-tomlinson]

Is location guaranteed to be a member of result?

[philbooth]

No idea but it seems like the kind of thing we should treat as an error if it's missing.

[shane-tomlinson]

Woah, I just noticed the diff size:

+181,801 That's an awful lot. Did something go wrong with the shrinkwrap update?

[philbooth]

Did something go wrong with the shrinkwrap update?

In the trade we refer to them as "merge conflicts".

[shane-tomlinson]

r+ -