Skip to content

Feature/1781 Twilio is integrated with app for event triggered notifications#3410

Merged
compwron merged 17 commits into
rubyforgood:mainfrom
harsohailB:feature/1781
May 15, 2022
Merged

Feature/1781 Twilio is integrated with app for event triggered notifications#3410
compwron merged 17 commits into
rubyforgood:mainfrom
harsohailB:feature/1781

Conversation

@harsohailB
Copy link
Copy Markdown
Collaborator

@harsohailB harsohailB commented May 9, 2022

What github issue is this PR for, if any?

Resolves #1781

What changed, and why?

  • Added Twilio Account SID, Twilio API Key SID, Twilio API Key Secret, and Twilio Phone Number under the "Edit Organization" tab

These are given by Twilio in their console as follows:

Screen Shot 2022-05-09 at 12 22 56 PM

Screen Shot 2022-05-09 at 5 19 10 PM

A POST request like the following is sent to send an SMS which requires these three parameters:

  • Changed CasaOrg model to store the three attributes
curl -X POST https://api.twilio.com/2010-04-01/Accounts/${TWILIO_ACCOUNT_SID}/Messages.json \
                --data-urlencode "Body=Hello from Twilio" \
                --data-urlencode "From=${TWILIO_PHONE_NUMBER}" \
                --data-urlencode "To=${RECIPIENT NUMBER}" \
                -u ${TWILIO_API_KEY_SID}:${TWILIO_API_KEY_SECRET}

How will this affect user permissions?

  • Volunteer permissions: n/a
  • Supervisor permissions: n/a
  • Admin permissions: n/a

How is this tested? (please write tests!) 💖💪

  • Wrote view tests for the "Edit Organization" page which now contains Twilio inputs
  • Wrote requests tests to verify update requests for a CasaOrg (invalid phone number input not accepted)

Screenshots please :)

Screen Shot 2022-05-10 at 11 31 09 AM

@harsohailB harsohailB added the codethechange code.the.change developers label May 9, 2022
@harsohailB harsohailB requested review from 7riumph, compwron and xihai01 May 9, 2022 18:28
@harsohailB harsohailB self-assigned this May 9, 2022
@github-actions github-actions Bot added erb Touches ERB templates ruby Touches Ruby code zzz_archived: Tests no-skin-tone labels May 9, 2022
Comment thread app/validators/casa_org_validator.rb
Comment thread db/migrate/20220508232038_add_columns_to_casa_orgs.rb Outdated
Comment thread spec/requests/casa_org_spec.rb Outdated
Copy link
Copy Markdown
Collaborator

@xihai01 xihai01 left a comment

Choose a reason for hiding this comment

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

awesome stuff! 😄 Left a few comments for you. I also have a suggestion. According to the Twilio docs, they recommend using API keys since they aren't attached to the main account and can be easily issued and revoked: https://www.twilio.com/docs/usage/rest-api-best-practices#control-access-to-your-accounts

@compwron
Copy link
Copy Markdown
Collaborator

compwron commented May 9, 2022

Yes, API keys :)

@harsohailB
Copy link
Copy Markdown
Collaborator Author

awesome stuff! 😄 Left a few comments for you. I also have a suggestion. According to the Twilio docs, they recommend using API keys since they aren't attached to the main account and can be easily issued and revoked: https://www.twilio.com/docs/usage/rest-api-best-practices#control-access-to-your-accounts

Refactored to use API keys instead. Updated the description (curl request + pics) on how I understand they work.

Copy link
Copy Markdown
Collaborator

@7riumph 7riumph left a comment

Choose a reason for hiding this comment

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

Edit: misread an above comment/am not seeing multiple credentials in the codebase

@7riumph 7riumph self-requested a review May 10, 2022 02:32
Copy link
Copy Markdown
Collaborator

@7riumph 7riumph left a comment

Choose a reason for hiding this comment

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

Nice! @xihai01 has great comments too 🔥

@xihai01
Copy link
Copy Markdown
Collaborator

xihai01 commented May 10, 2022

@harsohailB Don't forget to update the screenshot

Copy link
Copy Markdown
Collaborator

@xihai01 xihai01 left a comment

Choose a reason for hiding this comment

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

LGTM

@compwron
Copy link
Copy Markdown
Collaborator

Failing tests are

rspec ./spec/system/casa_org/edit_spec.rb:96 # casa_org/edit can update show_driving_reimbursement flag
rspec ./spec/system/casa_org/edit_spec.rb:106 # casa_org/edit can upload a logo image
rspec ./spec/requests/casa_org_spec.rb:36 # CasaOrg as an admin PATCH /update with valid parameters doesn't revert logo to default if non logo details are updated
rspec ./spec/requests/casa_org_spec.rb:22 # CasaOrg as an admin PATCH /update with valid parameters updates the requested casa_org
rspec ./spec/requests/casa_org_spec.rb:44 # CasaOrg as an admin PATCH /update with valid parameters redirects to the casa_org
rspec ./spec/requests/casa_org_spec.rb:50 # CasaOrg as an admin PATCH /update with valid parameters also responds as json

@compwron compwron changed the title Feature/1781 Feature/1781 Twilio is integrated with app for event triggered notifications May 10, 2022
Copy link
Copy Markdown
Collaborator

@xihai01 xihai01 left a comment

Choose a reason for hiding this comment

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

great!

@harsohailB
Copy link
Copy Markdown
Collaborator Author

During production deployment, we will need to put RAILS_MASTER_KEY as an environment variable in Heroku so the production.yml.enc file can be decrypted.

@xihai01 xihai01 added the zzz_archived: 📱 SMS work relating to SMS notifications #1017 label May 13, 2022
Copy link
Copy Markdown
Collaborator

@compwron compwron left a comment

Choose a reason for hiding this comment

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

:)

@compwron compwron enabled auto-merge May 13, 2022 19:14
@xihai01
Copy link
Copy Markdown
Collaborator

xihai01 commented May 14, 2022

@harsohailB tests are failing. Seems like you need to run a migration
Screenshot from 2022-05-14 09-51-07

@compwron compwron merged commit b56deac into rubyforgood:main May 15, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

codethechange code.the.change developers erb Touches ERB templates ruby Touches Ruby code 🧪 Tests Tests zzz_archived: 📱 SMS work relating to SMS notifications #1017

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Twilio is integrated with app for event triggered notifications

4 participants