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.

Sign up for GitHub

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

Jump to bottom

[NEW ENV VAR] Add twilio to neighborhood map #96

Closed
wants to merge 30 commits into from
Closed

[NEW ENV VAR] Add twilio to neighborhood map #96

wants to merge 30 commits into from

Conversation

jorganisak
Copy link
Contributor

@jorganisak jorganisak commented Jun 15, 2020 •
edited
Loading

#18

  • Adds button next to "Email Aid Resource Links" on Neighborhood Finder page called "SMS Aid Resource Links"
  • Moves both buttons to new component called SendResourcesButtons
  • Created new component called SendSmsDialog - contains validation for phone number input
  • Body of SMS is editable within SendSmsDialog, user inputs phone number
  • Adds endpoint at POST /api/send-sms with payload {smsMessage, phoneNumber}
  • Endpoint will 404 if TWILIO_AUTH_TOKEN, TWILIO_PHONE_NUMBER, TWILIO_SID are not present in env vars

sms_resources_demo

@mjmaurer mjmaurer requested a review from piratefsh June 15, 2020 02:18
@mjmaurer mjmaurer changed the title Add twilio to neighborhood map Add twilio to neighborhood map [NEW ENV VAR] Jun 15, 2020
@mjmaurer mjmaurer changed the title Add twilio to neighborhood map [NEW ENV VAR] [NEW ENV VAR] Add twilio to neighborhood map Jun 15, 2020
mjmaurer
mjmaurer reviewed Jun 15, 2020
View reviewed changes
Copy link
Contributor

@mjmaurer mjmaurer left a comment

Choose a reason for hiding this comment

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

thanks so much! left some thoughts

index.js Outdated Show resolved Hide resolved
index.js Outdated Show resolved Hide resolved
src/api/sms/send-sms.js Outdated Show resolved Hide resolved
src/webapp/components/SendSmsDialog.js Show resolved Hide resolved
.env.example Outdated Show resolved Hide resolved
@mjmaurer
Copy link
Contributor

also, please consider using something like https://www.screentogif.com/ to show off a local demo in this PR!

@mjmaurer
Copy link
Contributor

@jorganisak what are the blockers for this? intake defaults?

@jorganisak
Copy link
Contributor Author

@mjmaurer - yes. pinging @mab253 to get some copy..

@jorganisak
Copy link
Contributor Author

After some guidance from @mab253 -

  • Got the copy! We're going to use a portion of what's being sent out via SMS for the intake pause

  • Also, since twilio functions are moving into this repo, seems like a good opportunity to keep twilio use-cases in one place

  • So I'll remove the API endpoint and twilio hook-in in this PR, and merge in and build off the work in added twilio serverless functions! for phone line + sms  #103 to send the SMS

@mjmaurer
Copy link
Contributor

awesome! thanks so much

@mjmaurer mjmaurer mentioned this pull request Jun 26, 2020
Merged
jorganisak
jorganisak commented Jun 27, 2020
View reviewed changes
package.json Outdated
@@ -99,10 +100,10 @@
"eslint-plugin-react": "7.19.0",
"eslint-plugin-react-hooks": "^2.3.0",
"i18next-scanner": "2.11.0",
"jest": "^26.0.1",
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Was getting a conflict between react-scripts and this version of jest when starting the webapp locally - react-scripts depends on version ~24. Removed my node_modules and this line, ran npm install, and the tests in place still run and pass!

@jorganisak
Copy link
Contributor Author

@mjmaurer -

I think this is ready for re-review! 🪂

@jorganisak jorganisak linked an issue Jun 27, 2020 that may be closed by this pull request
Add an option text aid resources with the webapp #18
Open
mjmaurer
mjmaurer approved these changes Jun 28, 2020
View reviewed changes
Copy link
Contributor

@mjmaurer mjmaurer left a comment

Choose a reason for hiding this comment

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

awesome PR! thanks so much. all good for me

@jorganisak
Copy link
Contributor Author

@mab253 @mjmaurer -

Merged master into this and tested the feature, I think this is good to merge!

There's a couple things that have to happen on deploy though, since this is the first time we're requesting the twilio functions directly from the react app.

  1. send-sms twilio function deployed to twilio functions
  • looks like twilio serverless:deploy according to readme, but wanted to check in as i know this is a new system
  1. new config vars in heroku app env like:
REACT_APP_TWILIO_FUNCTIONS_URL='https://e51707e1e9be.ngrok.io'
REACT_APP_TWILIO_PHONE_NUMBER='+1 347 418 0185'

REACT_APP_TWILIO_FUCNTIONS_URL is mutual-aid-3223-dev.twil.io on production and probabaly staging too?
REACT_APP_TWILIO_PHONE_NUMBER is +1 917 341 7675 on production, maybe use the dev number for staging?

  • The heroku app can be deployed without these new configs, so its safe to be in master with or without the function deployed.
  • The "Send SMS" button won't show if the configs aren't there, so their addition can act as a sort of feature flag to turn the sms feature on whenever we deploy the twilio functions next.

@mjmaurer
Copy link
Contributor

still LG! thanks for making it not break without twilio deployed

@piratefsh
Copy link
Collaborator

hi folks, what's the status of this PR? seems like there was some follow up needed before deploy.

@piratefsh
Copy link
Collaborator

hey folks, am closing this PR to clean up the repo for now as there is no activity. feel free to reopen if you want to revisit in the future!

@piratefsh piratefsh closed this Nov 8, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mab253 mab253 mab253 left review comments

@mjmaurer mjmaurer mjmaurer approved these changes

@piratefsh piratefsh Awaiting requested review from piratefsh

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Add an option text aid resources with the webapp
4 participants
@jorganisak @mjmaurer @piratefsh @mab253