Skip to content
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

Add Nexmo as a text provider for MX and US numbers #1002

Merged
merged 5 commits into from
Sep 16, 2019

Conversation

nambrot
Copy link
Contributor

@nambrot nambrot commented Sep 14, 2019

Description

This PR adds the ability for us to send text messages via Nexmo for US and MX phone numbers.

Tested

  • Deployed it to integration and saw it working

Related issues

Copy link
Contributor

@cmcewen cmcewen left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM but seems like some tests to fix

return nexmoClient
}

export async function sendSmsWithNexmo(countryCode: string, phoneNumber: string, message: string) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: Config.ts seems like the wrong place for this

Copy link
Contributor Author

@nambrot nambrot Sep 16, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Given the relative urgency of this, do you think we can do this in a future PR? I imagine the pool will receive significant love (i.e. rewrite soon)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok

console.error('Failed to send twilio message', e)
throw new Error('Failed to send twilio message' + e)
console.error('Failed to send text message via txt provider', e)
throw new Error('Failed to send text message via txt provider' + e)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: this is probably my mistake but we shouldn't include e in the new error here

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think including more error information than not is usually more helpful, so I personally actually like it

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I feel like it just adds noise given that we properly log the error in the line above but it's really nbd

@codecov
Copy link

codecov bot commented Sep 16, 2019

Codecov Report

Merging #1002 into master will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1002   +/-   ##
=======================================
  Coverage   66.92%   66.92%           
=======================================
  Files         252      252           
  Lines        7311     7311           
  Branches      482      482           
=======================================
  Hits         4893     4893           
  Misses       2329     2329           
  Partials       89       89
Flag Coverage Δ
#mobile 66.92% <ø> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ccb6147...8267771. Read the comment docs.

@nambrot nambrot merged commit 868c072 into master Sep 16, 2019
aaronmgdr added a commit that referenced this pull request Sep 17, 2019
* master: (132 commits)
  [Wallet] Differentiate dollars and gold transactions (#1021)
  [wip] LinkedList cycle fix (#941)
  Fix slither issues (#572)
  [Wallet] Disable import wallet button when the backup phrase is not valid (#1012)
  Fix fixed fraction division underflow (#952)
  [CircleCI]Run protocol coverage once a day (#1018)
  Expose and test validator set precompiles (#874)
  [celotool] Fund Faucet accounts on network init (#990)
  Add Nexmo as a text provider for MX and US numbers (#1002)
  [CircleCI]Run protocol test with and without coverage (#991)
  Increase memory requests for Alfajores nodes (#966)
  [wallet]Select blockchain url based on testnet (#936)
  Pilot app config and ability to not show testnet banner (#1009)
  Add Linux onboarding steps and a TOC to SETUP.md (#1004)
  Add README-dev.md (#989)
  [Wallet] Exchange flow formatting fixes (#961)
  [celotool] Cleanup genesis block generation (#988)
  [celotool] Some ❤️. Clean up (#948)
  CeloCLI sorted list of Validator Groups should not include groups with zero votes (#907)
  [contractkit] Fix tests (#963)
  ...
aaronmgdr added a commit that referenced this pull request Sep 18, 2019
* master: (72 commits)
  Remove unused Reserve.burnToken function (#984)
  Persistent disks for transaction nodes (#1016)
  [Wallet] Differentiate dollars and gold transactions (#1021)
  [wip] LinkedList cycle fix (#941)
  Fix slither issues (#572)
  [Wallet] Disable import wallet button when the backup phrase is not valid (#1012)
  Fix fixed fraction division underflow (#952)
  [CircleCI]Run protocol coverage once a day (#1018)
  Expose and test validator set precompiles (#874)
  [celotool] Fund Faucet accounts on network init (#990)
  Add Nexmo as a text provider for MX and US numbers (#1002)
  [CircleCI]Run protocol test with and without coverage (#991)
  Increase memory requests for Alfajores nodes (#966)
  [wallet]Select blockchain url based on testnet (#936)
  Pilot app config and ability to not show testnet banner (#1009)
  Add Linux onboarding steps and a TOC to SETUP.md (#1004)
  Add README-dev.md (#989)
  [Wallet] Exchange flow formatting fixes (#961)
  [celotool] Cleanup genesis block generation (#988)
  [celotool] Some ❤️. Clean up (#948)
  ...

# Conflicts:
#	packages/web/src/header/Header.3.tsx
aaronmgdr added a commit that referenced this pull request Sep 18, 2019
* master: (38 commits)
  Remove unused Reserve.burnToken function (#984)
  Persistent disks for transaction nodes (#1016)
  [Wallet] Differentiate dollars and gold transactions (#1021)
  [wip] LinkedList cycle fix (#941)
  Fix slither issues (#572)
  [Wallet] Disable import wallet button when the backup phrase is not valid (#1012)
  Fix fixed fraction division underflow (#952)
  [CircleCI]Run protocol coverage once a day (#1018)
  Expose and test validator set precompiles (#874)
  [celotool] Fund Faucet accounts on network init (#990)
  Add Nexmo as a text provider for MX and US numbers (#1002)
  [CircleCI]Run protocol test with and without coverage (#991)
  Increase memory requests for Alfajores nodes (#966)
  [wallet]Select blockchain url based on testnet (#936)
  Pilot app config and ability to not show testnet banner (#1009)
  Add Linux onboarding steps and a TOC to SETUP.md (#1004)
  Add README-dev.md (#989)
  [Wallet] Exchange flow formatting fixes (#961)
  [celotool] Cleanup genesis block generation (#988)
  [celotool] Some ❤️. Clean up (#948)
  ...
@ashishb ashishb deleted the nambrot/nexmo-verification-pool branch September 24, 2019 01:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Users SBAT receive text messages fast in Mexico via a Twilio alternative
3 participants