Twilio Integration and More update notification #1719

valaparthvi · 2017-08-15T19:25:22Z

Proposed changes in this pull request

Send update notification via sms with Twilio
[List all changes you want to add here. If you fixed an issue, please
add a reference to that issue as well.]

Add gateways.py to accounts. There are 2 gateways, TwilioGateway and FakeGateway. TwilioGateway is used to send actual messages and in testing, FakeGateway is used for local development.
The testing environment will use Test Credentials provided by Twilio, while production environment will Live Credentials provided by Twilio and a list of real phone numbers bought on Twilio.
Add tests for the gateways.
Add new messages to accounts/messages.py. This message will be sent via sms to user's phone.
Make changes to ProfileForm. When user changes/deletes email, send an SMS update notification if verified email is linked to user account. When user changes/deletes phone, send an email update notification if a verified phone is linked to user account. Make corresponding changes to tests.
Add new functions: send_phone_deleted_notification, send_email_deleted_notification, send_phone_changed_notification to accounts/utils.py.
Add Gateway settings to the settings file.
Make changes to AccountUser in accounts/views/api.py. Send update notification to the user's verified phone/email whenever the user changes/deletes phone/email. Add tests for the same.

When should this PR be merged

Currently, test setting uses Twilio Test credentials. These Test Credentials have limited access to resources. So there is no sure way to check if messages are actually sent during tests. This issue needs fixing.
Currently, there are no credentials, so tests using Twilio are failing. Once Twilio credentials are exported, and all test passes.

Risks

-None

Follow-up actions

These variables must be exported to the environment in order to use Twilio.
Live Credentials: TWILIO_ACCOUNT_SID and TWILIO_AUTH_TOKEN.

Checklist (for reviewing)

General

Is this PR explained thoroughly? All code changes must be accounted for in the PR description.
- Review 1
- Review 2
Is the PR labeled correctly? It should have the migration label if a new migration is added.
- Review 1
- Review 2
Is the risk level assessment sufficient? The risks section should contain all risks that might be introduced with the PR and which actions we need to take to mitigate these risks. Possible risks are database migrations, new libraries that need to be installed or changes to deployment scripts.
- Review 1
- Review 2

Functionality

Are all requirements met? Compare implemented functionality with the requirements specification.
- Review 1
- Review 2
Does the UI work as expected? There should be no Javascript errors in the console; all resources should load. There should be no unexpected errors. Deliberately try to break the feature to find out if there are corner cases that are not handled.
- Review 1
- Review 2

Code

Do you fully understand the introduced changes to the code? If not ask for clarification, it might uncover ways to solve a problem in a more elegant and efficient way.
- Review 1
- Review 2
Does the PR introduce any inefficient database requests? Use the debug server to check for duplicate requests.
- Review 1
- Review 2
Are all necessary strings marked for translation? All strings that are exposed to users via the UI must be marked for translation.
- Review 1
- Review 2
Is the code documented sufficiently? Large and complex classes, functions or methods must be annotated with comments following our code-style guidelines.
- Review 1
- Review 2
Has the scalability of this change been evaluated?
- Review 1
- Review 2
Is there a maintenance plan in place?
- Review 1
- Review 2

Tests

Are there sufficient test cases? Ensure that all components are tested individually; models, forms, and serializers should be tested in isolation even if a test for a view covers these components.
- Review 1
- Review 2
If this is a bug fix, are tests for the issue in place? There must be a test case for the bug to ensure the issue won’t regress. Make sure that the tests break without the new code to fix the issue.
- Review 1
- Review 2
If this is a new feature or a significant change to an existing feature? has the manual testing spreadsheet been updated with instructions for manual testing?
- Review 1
- Review 2

Security

Confirm this PR doesn't commit any keys, passwords, tokens, usernames, or other secrets.
- Review 1
- Review 2
Are all UI and API inputs run through forms or serializers?
- Review 1
- Review 2
Are all external inputs validated and sanitized appropriately?
- Review 1
- Review 2
Does all branching logic have a default case?
- Review 1
- Review 2
Does this solution handle outliers and edge cases gracefully?
- Review 1
- Review 2
Are all external communications secured and restricted to SSL?
- Review 1
- Review 2

Documentation

Are changes to the UI documented in the platform docs? If this PR introduces new platform site functionality or changes existing ones, the changes must be documented in the Cadasta Platform Documentation.
- Review 1
- Review 2
Are changes to the API documented in the API docs? If this PR introduces new API functionality or changes existing ones, the changes must be documented in the API docs.
- Review 1
- Review 2
Are reusable components documented? If this PR introduces components that are relevant to other developers (for instance a mixin for a view or a generic form) they should be documented in the Wiki.
- Review 1
- Review 2

oliverroick · 2017-08-16T07:26:30Z

41 tests are failing for this PR. You need to fix them before we can proceed.

oliverroick

There are a few small things in the code. I also can't get it to run in the dev VM, the wrong SMS gateway is imported in utils.

oliverroick · 2017-08-21T16:55:01Z

cadasta/config/settings/default.py

+SMS_GATEWAY = 'accounts.gateways.TwilioGateway'
+TWILIO_ACCOUNT_SID = os.environ['TWILIO_ACCOUNT_SID']
+TWILIO_AUTH_TOKEN = os.environ['TWILIO_AUTH_TOKEN']
+TWILIO_PHONE_NUMBER_LIST = ['+15005550006']


This shouldn't be hardcoded.

oliverroick · 2017-08-21T17:00:53Z

cadasta/accounts/tests/test_gateways.py

@@ -0,0 +1,65 @@
+from django.test import TestCase
+from django.conf import settings
+# from accounts.gateways.twilio import TwilioGateway


Comments need to be removed

oliverroick · 2017-08-22T10:57:53Z

cadasta/config/settings/default.py

@@ -574,3 +574,8 @@

 TOTP_TOKEN_VALIDITY = 3600
 TOTP_DIGITS = 6
+
+SMS_GATEWAY = 'accounts.gateways.TwilioGateway'
+TWILIO_ACCOUNT_SID = os.environ['TWILIO_ACCOUNT_SID']


Since you're overwriting SMS_GATEWAY in the dev settings this breaks in the dev environment. We're not actually using Twilio for dev so there's no need to set this by default. Move these settings to the production settings.

oliverroick · 2017-08-22T11:07:05Z

cadasta/accounts/utils.py

+
+
+def send_sms(to, body):
+    twilioobj = (import_string(settings.SMS_GATEWAY))()


In the dev VM accounts.gateways.FakeGateway should be imported but it isn't. Instead, accounts.gateways.TwilioGateway is imported and everything breaks.

I did not know that dev settings were used for testing up until now. I think the system should work fine now, and there should be no breakage.

valaparthvi · 2017-08-23T19:20:01Z

@oliverroick Updates from my conversation with @amplifi :

Move settings used by all config files to default.py, this includes, TWILIO_ACCOUNT_SID and TWILIO_AUTH_TOKEN, SMS_GATEWAY. production.py will override SMS_GATEWAY.
Remove TwilioGateway logger. Status of messages can always be checked from twilio console, so it's okay to remove that logger. Move FakeGateway logger to default.py since it is used by both dev VM and travis.
An update notification must be sent to phone/email linked to the user's account regardless of phone/email('s) verification status.

amplifi · 2017-08-23T19:24:25Z

👍 and the Ansible provisioning task is updated to populate the Twilio SID and auth token vars. Great work, @valaparthvi!

oliverroick

This looks good now, thanks for making the changes. Also, thanks @amplifi for helping out with this.

There's only one small thing with the settings now that still breaks in the dev VM.

oliverroick · 2017-08-25T11:55:32Z

cadasta/config/settings/default.py

+
+SMS_GATEWAY = 'accounts.gateways.FakeGateway'
+
+TWILIO_ACCOUNT_SID = os.environ['TWILIO_ACCOUNT_SID']


When I don't have the Twilio account settings exported in my dev VM this breaks. We have two options how we can deal with it:

Add this and the next line to both production.py and travis.py and remove them from here.

Write os.environ.get('TWILIO_ACCOUNT_SID') instead. In that case TWILIO_ACCOUNT_SID will be set to None and no exception is thrown.

Ah, that is what you meant when you said dev VM breaks. I was under the assumption that we will always have credentials exported in the dev VM so I did not completely understand the problem earlier, but I like the second idea better. Although exceptions will be thrown in the dev VM when tests are run. These exceptions will be thrown by accounts/tests/test_gateways.py since it uses TwilioGateway.

oliverroick

Looks good, thanks!

…nce we have phone numbers, remove unnecessary settings from default settings

… to settings/default.py

* Twilio Gateway * Enable Twilio test account for Travis * Pass twilio settings into tox env * Pass all the tests * 100% test coverage * Remove unnecessary lines of code * Currently removed TWILIO_PHONE_NUMBER from settings, add that later once we have phone numbers, remove unnecessary settings from default settings * Remove comments * Make tests pass * Make tests pass, remove TwilioGateway logger, move FakeGateway logger to settings/default.py * Send email/sms regardless of phone/email('s) verification status * Add changes addressed to PR * Rebasing and changes

* VerificationDevice model and Removal of 48hr email verification period (#1606) * Registration with Phone number (#1662) * Add phone to User Profile (#1698) * Add Ansible provisioning for Twilio * Allow user to login with phone and add Resend Token Page (#1708) * Reset Password with Phone (#1717) * Twilio Integration and More update notification (#1719) * Add API endpoint for phone verification(2) (#1748)

valaparthvi requested a review from oliverroick August 15, 2017 19:25

valaparthvi mentioned this pull request Aug 20, 2017

Provide more email notifications #453

Open

oliverroick requested changes Aug 22, 2017

View reviewed changes

amplifi added PR: needs review PR: needs testing labels Aug 22, 2017

oliverroick requested changes Aug 25, 2017

View reviewed changes

oliverroick approved these changes Aug 28, 2017

View reviewed changes

valaparthvi and others added 13 commits August 28, 2017 20:50

Twilio Gateway

ebb85cd

Enable Twilio test account for Travis

4a38109

Pass twilio settings into tox env

3b557a1

Pass all the tests

3557b59

100% test coverage

a9f71cc

Remove unnecessary lines of code

e3bf289

Currently removed TWILIO_PHONE_NUMBER from settings, add that later o…

c57132a

…nce we have phone numbers, remove unnecessary settings from default settings

Remove comments

a70df7f

Make tests pass

1600681

Make tests pass, remove TwilioGateway logger, move FakeGateway logger…

cdf74a6

… to settings/default.py

Send email/sms regardless of phone/email('s) verification status

9f6a8c9

Add changes addressed to PR

e6f872e

Rebasing and changes

160a03f

valaparthvi force-pushed the phone/Twilio branch from d10329c to 160a03f Compare August 28, 2017 16:18

valaparthvi merged commit 396a852 into additional-login-options Aug 28, 2017

valaparthvi mentioned this pull request Aug 31, 2017

code cleanup #1767

Merged

46 tasks

valaparthvi deleted the phone/Twilio branch September 21, 2017 05:46

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Twilio Integration and More update notification #1719

Twilio Integration and More update notification #1719

valaparthvi commented Aug 15, 2017 •

edited

Loading

oliverroick commented Aug 16, 2017

oliverroick left a comment

oliverroick Aug 21, 2017

oliverroick Aug 21, 2017

oliverroick Aug 22, 2017

oliverroick Aug 22, 2017

valaparthvi Aug 23, 2017 •

edited

Loading

valaparthvi commented Aug 23, 2017

amplifi commented Aug 23, 2017

oliverroick left a comment

oliverroick Aug 25, 2017

valaparthvi Aug 26, 2017 •

edited

Loading

oliverroick left a comment



		def send_sms(to, body):
		twilioobj = (import_string(settings.SMS_GATEWAY))()


		SMS_GATEWAY = 'accounts.gateways.FakeGateway'

		TWILIO_ACCOUNT_SID = os.environ['TWILIO_ACCOUNT_SID']

Twilio Integration and More update notification #1719

Twilio Integration and More update notification #1719

Conversation

valaparthvi commented Aug 15, 2017 • edited Loading

Proposed changes in this pull request

When should this PR be merged

Risks

Follow-up actions

Checklist (for reviewing)

General

Functionality

Code

Tests

Security

Documentation

oliverroick commented Aug 16, 2017

oliverroick left a comment

Choose a reason for hiding this comment

oliverroick Aug 21, 2017

Choose a reason for hiding this comment

oliverroick Aug 21, 2017

Choose a reason for hiding this comment

oliverroick Aug 22, 2017

Choose a reason for hiding this comment

oliverroick Aug 22, 2017

Choose a reason for hiding this comment

valaparthvi Aug 23, 2017 • edited Loading

Choose a reason for hiding this comment

valaparthvi commented Aug 23, 2017

amplifi commented Aug 23, 2017

oliverroick left a comment

Choose a reason for hiding this comment

oliverroick Aug 25, 2017

Choose a reason for hiding this comment

valaparthvi Aug 26, 2017 • edited Loading

Choose a reason for hiding this comment

oliverroick left a comment

Choose a reason for hiding this comment

valaparthvi commented Aug 15, 2017 •

edited

Loading

valaparthvi Aug 23, 2017 •

edited

Loading

valaparthvi Aug 26, 2017 •

edited

Loading