From 3e1de2e901470dcfb98346ea70e8d1f996978de6 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 2 Jun 2017 15:58:36 +0100 Subject: [PATCH 1/5] Capture the fire text callbacks. Parse the form data, and stop the message --- app/notifications/receive_notifications.py | 29 ++++++- .../test_receive_notification.py | 77 ++++++++++++++++++- 2 files changed, 104 insertions(+), 2 deletions(-) diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index cbe17e09e..58f4d7f53 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -86,7 +86,34 @@ def create_inbound_mmg_sms_object(service, json): @receive_notifications_blueprint.route('/notifications/sms/receive/firetext', methods=['POST']) def receive_firetext_sms(): post_data = request.form - current_app.logger.info("Received Firetext notification form data: {}".format(post_data)) + + potential_services = dao_fetch_services_by_sms_sender(post_data['destination']) + if len(potential_services) != 1: + current_app.logger.error('Inbound number "{}" not associated with exactly one service'.format( + post_data['source'] + )) + statsd_client.incr('inbound.firetext.failed') + return jsonify({ + "status": "ok" + }), 200 + + service = potential_services[0] + + user_number = normalise_phone_number(post_data['source']) + message = post_data['message'] + timestamp = post_data['time'] + + dao_create_inbound_sms( + InboundSms( + service=service, + notify_number=service.sms_sender, + user_number=user_number, + provider_date=timestamp, + content=message + ) + ) + + statsd_client.incr('inbound.firetext.successful') return jsonify({ "status": "ok" diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 5d93e5fd3..8cdaa2dc2 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -1,4 +1,5 @@ from datetime import datetime +from unittest.mock import call import pytest from flask import json @@ -10,6 +11,7 @@ from app.notifications.receive_notifications import ( ) from app.models import InboundSms +from tests.app.conftest import sample_service from tests.app.db import create_service @@ -92,7 +94,11 @@ def test_receive_notification_error_if_not_single_matching_service(client, notif assert InboundSms.query.count() == 0 -def test_receive_notification_returns_received_to_firetext(client): +def test_receive_notification_returns_received_to_firetext(notify_db_session, client, mocker): + mock = mocker.patch('app.notifications.receive_notifications.statsd_client.incr') + + create_service(service_name='b', sms_sender='07111111111') + data = "source=07999999999&destination=07111111111&message=this is a message&time=2017-01-01 12:00:00" response = client.post( @@ -103,4 +109,73 @@ def test_receive_notification_returns_received_to_firetext(client): assert response.status_code == 200 result = json.loads(response.get_data(as_text=True)) + mock.assert_has_calls([call('inbound.firetext.successful')]) + assert result['status'] == 'ok' + + +def test_receive_notification_from_firetext_persists_message(notify_db_session, client, mocker): + mock = mocker.patch('app.notifications.receive_notifications.statsd_client.incr') + + service = create_service(service_name='b', sms_sender='07111111111') + + data = "source=07999999999&destination=07111111111&message=this is a message&time=2017-01-01 12:00:00" + + response = client.post( + path='/notifications/sms/receive/firetext', + data=data, + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + assert response.status_code == 200 + result = json.loads(response.get_data(as_text=True)) + + persisted = InboundSms.query.first() + + assert result['status'] == 'ok' + assert persisted.notify_number == '07111111111' + assert persisted.user_number == '7999999999' + assert persisted.service == service + assert persisted.content == 'this is a message' + assert persisted.provider_date == datetime(2017, 1, 1, 12, 0, 0, 0) + + +def test_receive_notification_from_firetext_persists_message_with_normalized_phone(notify_db_session, client, mocker): + mock = mocker.patch('app.notifications.receive_notifications.statsd_client.incr') + + create_service(service_name='b', sms_sender='07111111111') + + data = "source=(+44)7999999999&destination=07111111111&message=this is a message&time=2017-01-01 12:00:00" + + response = client.post( + path='/notifications/sms/receive/firetext', + data=data, + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + assert response.status_code == 200 + result = json.loads(response.get_data(as_text=True)) + + persisted = InboundSms.query.first() + + assert result['status'] == 'ok' + assert persisted.user_number == '447999999999' + + +def test_returns_ok_to_firetext_if_mismatched_sms_sender(notify_db_session, client, mocker): + + mock = mocker.patch('app.notifications.receive_notifications.statsd_client.incr') + + create_service(service_name='b', sms_sender='07111111199') + + data = "source=(+44)7999999999&destination=07111111111&message=this is a message&time=2017-01-01 12:00:00" + + response = client.post( + path='/notifications/sms/receive/firetext', + data=data, + headers=[('Content-Type', 'application/x-www-form-urlencoded')]) + + assert response.status_code == 200 + result = json.loads(response.get_data(as_text=True)) + + assert not InboundSms.query.all() + assert result['status'] == 'ok' + mock.assert_has_calls([call('inbound.firetext.failed')]) From 012f8d2675c058af561778402469f806eb9371cc Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 2 Jun 2017 16:37:57 +0100 Subject: [PATCH 2/5] Adds provider onto the inbound sms table so we know where this came from. --- app/models.py | 1 + app/notifications/receive_notifications.py | 8 ++++--- .../versions/0090_add_inbound_provider.py | 22 +++++++++++++++++++ .../test_receive_notification.py | 2 ++ 4 files changed, 30 insertions(+), 3 deletions(-) create mode 100644 migrations/versions/0090_add_inbound_provider.py diff --git a/app/models.py b/app/models.py index 5bc1cf2fc..6d5bf19fb 100644 --- a/app/models.py +++ b/app/models.py @@ -1166,6 +1166,7 @@ class InboundSms(db.Model): user_number = db.Column(db.String, nullable=False) # the end user's number, that the msg was sent from provider_date = db.Column(db.DateTime) provider_reference = db.Column(db.String) + provider = db.Column(db.String, nullable=True) _content = db.Column('content', db.String, nullable=False) @property diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index 58f4d7f53..5fd5ad585 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -4,7 +4,7 @@ import iso8601 from flask import jsonify, Blueprint, current_app, request from notifications_utils.recipients import normalise_phone_number -from app import statsd_client +from app import statsd_client, firetext_client, mmg_client from app.dao.services_dao import dao_fetch_services_by_sms_sender from app.dao.inbound_sms_dao import dao_create_inbound_sms from app.models import InboundSms @@ -78,6 +78,7 @@ def create_inbound_mmg_sms_object(service, json): provider_date=provider_date, provider_reference=json.get('ID'), content=message, + provider=mmg_client.name ) dao_create_inbound_sms(inbound) return inbound @@ -90,7 +91,7 @@ def receive_firetext_sms(): potential_services = dao_fetch_services_by_sms_sender(post_data['destination']) if len(potential_services) != 1: current_app.logger.error('Inbound number "{}" not associated with exactly one service'.format( - post_data['source'] + post_data['destination'] )) statsd_client.incr('inbound.firetext.failed') return jsonify({ @@ -109,7 +110,8 @@ def receive_firetext_sms(): notify_number=service.sms_sender, user_number=user_number, provider_date=timestamp, - content=message + content=message, + provider=firetext_client.name ) ) diff --git a/migrations/versions/0090_add_inbound_provider.py b/migrations/versions/0090_add_inbound_provider.py new file mode 100644 index 000000000..c110ac193 --- /dev/null +++ b/migrations/versions/0090_add_inbound_provider.py @@ -0,0 +1,22 @@ +"""empty message + +Revision ID: 0090_add_inbound_provider +Revises: 0090_inbound_sms +Create Date: 2017-06-02 16:07:35.445423 + +""" + +# revision identifiers, used by Alembic. +revision = '0090_add_inbound_provider' +down_revision = '0090_inbound_sms' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + op.add_column('inbound_sms', sa.Column('provider', sa.String(), nullable=True)) + + +def downgrade(): + op.drop_column('inbound_sms', 'provider') diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 8cdaa2dc2..c346bcf02 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -70,6 +70,7 @@ def test_create_inbound_mmg_sms_object(sample_service): assert inbound_sms.provider_reference == 'bar' assert inbound_sms._content != 'hello there 📩' assert inbound_sms.content == 'hello there 📩' + assert inbound_sms.provider == 'mmg' @pytest.mark.parametrize('notify_number', ['foo', 'baz'], ids=['two_matching_services', 'no_matching_services']) @@ -136,6 +137,7 @@ def test_receive_notification_from_firetext_persists_message(notify_db_session, assert persisted.user_number == '7999999999' assert persisted.service == service assert persisted.content == 'this is a message' + assert persisted.provider == 'firetext' assert persisted.provider_date == datetime(2017, 1, 1, 12, 0, 0, 0) From 199c43c5077b7ab91147ec473168d6ccf811e381 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 2 Jun 2017 16:49:11 +0100 Subject: [PATCH 3/5] Migration script to populate the provider. - initial build of this ONLY support MMG so we can assume that all existing entries are all MMG, so any nulls == MMG. - This PR will put in fire text so not so safe to keep doing this back and forward. --- ...ovider.py => 0091_add_inbound_provider.py} | 4 ++-- .../0092_populate_inbound_provider.py | 22 +++++++++++++++++++ 2 files changed, 24 insertions(+), 2 deletions(-) rename migrations/versions/{0090_add_inbound_provider.py => 0091_add_inbound_provider.py} (84%) create mode 100644 migrations/versions/0092_populate_inbound_provider.py diff --git a/migrations/versions/0090_add_inbound_provider.py b/migrations/versions/0091_add_inbound_provider.py similarity index 84% rename from migrations/versions/0090_add_inbound_provider.py rename to migrations/versions/0091_add_inbound_provider.py index c110ac193..a0864ddbe 100644 --- a/migrations/versions/0090_add_inbound_provider.py +++ b/migrations/versions/0091_add_inbound_provider.py @@ -1,13 +1,13 @@ """empty message -Revision ID: 0090_add_inbound_provider +Revision ID: 0091_add_inbound_provider Revises: 0090_inbound_sms Create Date: 2017-06-02 16:07:35.445423 """ # revision identifiers, used by Alembic. -revision = '0090_add_inbound_provider' +revision = '0091_add_inbound_provider' down_revision = '0090_inbound_sms' from alembic import op diff --git a/migrations/versions/0092_populate_inbound_provider.py b/migrations/versions/0092_populate_inbound_provider.py new file mode 100644 index 000000000..ba6da1f5d --- /dev/null +++ b/migrations/versions/0092_populate_inbound_provider.py @@ -0,0 +1,22 @@ +"""empty message + +Revision ID: 0092_populate_inbound_provider +Revises: 0091_add_inbound_provider +Create Date: 2017-05-22 10:23:43.939050 + +""" + +# revision identifiers, used by Alembic. +revision = '0092_populate_inbound_provider' +down_revision = '0091_add_inbound_provider' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +def upgrade(): + op.execute("UPDATE inbound_sms SET provider='mmg' WHERE provider is null") + + +def downgrade(): + op.execute("UPDATE inbound_sms SET provider=null WHERE provider = 'mmg'") From b296e736f2b77c1165cae19d535cebb03fe02d69 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 5 Jun 2017 11:51:30 +0100 Subject: [PATCH 4/5] Reorder the migrations. --- app/notifications/receive_notifications.py | 4 ++-- ...d_inbound_provider.py => 0092_add_inbound_provider.py} | 8 ++++---- ...ound_provider.py => 0093_populate_inbound_provider.py} | 8 ++++---- tests/app/notifications/test_receive_notification.py | 4 ++-- 4 files changed, 12 insertions(+), 12 deletions(-) rename migrations/versions/{0091_add_inbound_provider.py => 0092_add_inbound_provider.py} (71%) rename migrations/versions/{0092_populate_inbound_provider.py => 0093_populate_inbound_provider.py} (69%) diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index ffe347d95..904e78711 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -38,7 +38,7 @@ def receive_mmg_sms(): # succesfully return 'RECEIVED', 200 - statsd_client.incr('inbound.mmg.succesful') + statsd_client.incr('inbound.mmg.successful') service = potential_services[0] @@ -100,7 +100,7 @@ def receive_firetext_sms(): service = potential_services[0] - user_number = normalise_phone_number(post_data['source']) + user_number = validate_and_format_phone_number(post_data['source'], international=True) message = post_data['message'] timestamp = post_data['time'] diff --git a/migrations/versions/0091_add_inbound_provider.py b/migrations/versions/0092_add_inbound_provider.py similarity index 71% rename from migrations/versions/0091_add_inbound_provider.py rename to migrations/versions/0092_add_inbound_provider.py index a0864ddbe..f7e5f510e 100644 --- a/migrations/versions/0091_add_inbound_provider.py +++ b/migrations/versions/0092_add_inbound_provider.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0091_add_inbound_provider -Revises: 0090_inbound_sms +Revision ID: 0092_add_inbound_provider +Revises: 0091_letter_billing Create Date: 2017-06-02 16:07:35.445423 """ # revision identifiers, used by Alembic. -revision = '0091_add_inbound_provider' -down_revision = '0090_inbound_sms' +revision = '0092_add_inbound_provider' +down_revision = '0091_letter_billing' from alembic import op import sqlalchemy as sa diff --git a/migrations/versions/0092_populate_inbound_provider.py b/migrations/versions/0093_populate_inbound_provider.py similarity index 69% rename from migrations/versions/0092_populate_inbound_provider.py rename to migrations/versions/0093_populate_inbound_provider.py index ba6da1f5d..688d3f7bf 100644 --- a/migrations/versions/0092_populate_inbound_provider.py +++ b/migrations/versions/0093_populate_inbound_provider.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0092_populate_inbound_provider -Revises: 0091_add_inbound_provider +Revision ID: 0093_populate_inbound_provider +Revises: 0092_add_inbound_provider Create Date: 2017-05-22 10:23:43.939050 """ # revision identifiers, used by Alembic. -revision = '0092_populate_inbound_provider' -down_revision = '0091_add_inbound_provider' +revision = '0093_populate_inbound_provider' +down_revision = '0092_add_inbound_provider' from alembic import op import sqlalchemy as sa diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 6ce813354..67949104a 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -116,7 +116,7 @@ def test_receive_notification_returns_received_to_firetext(notify_db_session, cl def test_receive_notification_from_firetext_persists_message(notify_db_session, client, mocker): - mock = mocker.patch('app.notifications.receive_notifications.statsd_client.incr') + mocker.patch('app.notifications.receive_notifications.statsd_client.incr') service = create_service(service_name='b', sms_sender='07111111111') @@ -134,7 +134,7 @@ def test_receive_notification_from_firetext_persists_message(notify_db_session, assert result['status'] == 'ok' assert persisted.notify_number == '07111111111' - assert persisted.user_number == '7999999999' + assert persisted.user_number == '447999999999' assert persisted.service == service assert persisted.content == 'this is a message' assert persisted.provider == 'firetext' From efb045fc68209fb951ca6d57c23a1067c64a395a Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 5 Jun 2017 11:55:13 +0100 Subject: [PATCH 5/5] Removed pre-populate column to run after migration --- .../0093_populate_inbound_provider.py | 22 ------------------- 1 file changed, 22 deletions(-) delete mode 100644 migrations/versions/0093_populate_inbound_provider.py diff --git a/migrations/versions/0093_populate_inbound_provider.py b/migrations/versions/0093_populate_inbound_provider.py deleted file mode 100644 index 688d3f7bf..000000000 --- a/migrations/versions/0093_populate_inbound_provider.py +++ /dev/null @@ -1,22 +0,0 @@ -"""empty message - -Revision ID: 0093_populate_inbound_provider -Revises: 0092_add_inbound_provider -Create Date: 2017-05-22 10:23:43.939050 - -""" - -# revision identifiers, used by Alembic. -revision = '0093_populate_inbound_provider' -down_revision = '0092_add_inbound_provider' - -from alembic import op -import sqlalchemy as sa -from sqlalchemy.dialects import postgresql - -def upgrade(): - op.execute("UPDATE inbound_sms SET provider='mmg' WHERE provider is null") - - -def downgrade(): - op.execute("UPDATE inbound_sms SET provider=null WHERE provider = 'mmg'")