From 3e1de2e901470dcfb98346ea70e8d1f996978de6 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 2 Jun 2017 15:58:36 +0100 Subject: [PATCH 01/10] 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 0631b6c988e55b495c9fd6542006c26825684992 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 2 Jun 2017 12:21:12 +0100 Subject: [PATCH 02/10] Add dao to delete inbound sms after seven days --- app/dao/inbound_sms_dao.py | 19 +++++++++++++++ app/dao/notifications_dao.py | 7 ++++-- tests/app/dao/test_inbound_sms_dao.py | 35 ++++++++++++++++++++++++--- tests/app/db.py | 24 ++++++++++-------- 4 files changed, 70 insertions(+), 15 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 597748731..3411aeb22 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -1,6 +1,13 @@ +from datetime import ( + timedelta, + datetime +) + + from app import db from app.dao.dao_utils import transactional from app.models import InboundSms +from app.statsd_decorators import statsd @transactional @@ -28,3 +35,15 @@ def dao_count_inbound_sms_for_service(service_id): return InboundSms.query.filter( InboundSms.service_id == service_id ).count() + + +@statsd(namespace="dao") +@transactional +def delete_inbound_sms_created_more_than_a_week_ago(): + seven_days_ago = datetime.utcnow() - timedelta(days=7) + + deleted = db.session.query(InboundSms).filter( + InboundSms.created_at < seven_days_ago + ).delete(synchronize_session='fetch') + + return deleted diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index f4ff2a24f..22da11966 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -2,7 +2,8 @@ import functools from datetime import ( datetime, timedelta, - date) + date +) from flask import current_app @@ -26,6 +27,7 @@ from app.models import ( NotificationHistory, NotificationStatistics, Template, + ScheduledNotification, NOTIFICATION_CREATED, NOTIFICATION_DELIVERED, NOTIFICATION_SENDING, @@ -35,7 +37,8 @@ from app.models import ( NOTIFICATION_PERMANENT_FAILURE, KEY_TYPE_NORMAL, KEY_TYPE_TEST, LETTER_TYPE, - NOTIFICATION_SENT, ScheduledNotification) + NOTIFICATION_SENT, +) from app.dao.dao_utils import transactional from app.statsd_decorators import statsd diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index f0fc6d8b3..2731562c4 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -1,11 +1,16 @@ -from datetime import datetime +from datetime import datetime, timedelta from freezegun import freeze_time -from app.dao.inbound_sms_dao import dao_get_inbound_sms_for_service, dao_count_inbound_sms_for_service - +from app.dao.inbound_sms_dao import ( + dao_get_inbound_sms_for_service, + dao_count_inbound_sms_for_service, + delete_inbound_sms_created_more_than_a_week_ago +) from tests.app.db import create_inbound_sms, create_service +from app.models import InboundSms + def test_get_all_inbound_sms(sample_service): inbound = create_inbound_sms(sample_service) @@ -57,3 +62,27 @@ def test_count_inbound_sms_for_service(notify_db_session): create_inbound_sms(service_two) assert dao_count_inbound_sms_for_service(service_one.id) == 2 + + +@freeze_time("2017-01-01 12:00:00") +def test_should_delete_inbound_sms_older_than_seven_days(sample_service): + older_than_seven_days = datetime.utcnow() - timedelta(days=7, seconds=1) + create_inbound_sms(sample_service, created_at=older_than_seven_days) + delete_inbound_sms_created_more_than_a_week_ago() + + assert len(InboundSms.query.all()) == 0 + + +@freeze_time("2017-01-01 12:00:00") +def test_should_not_delete_inbound_sms_before_seven_days(sample_service): + yesterday = datetime.utcnow() - timedelta(days=1) + just_before_seven_days = datetime.utcnow() - timedelta(days=6, hours=23, minutes=59, seconds=59) + older_than_seven_days = datetime.utcnow() - timedelta(days=7, seconds=1) + + create_inbound_sms(sample_service, created_at=yesterday) + create_inbound_sms(sample_service, created_at=just_before_seven_days) + create_inbound_sms(sample_service, created_at=older_than_seven_days) + + delete_inbound_sms_created_more_than_a_week_ago() + + assert len(InboundSms.query.all()) == 2 diff --git a/tests/app/db.py b/tests/app/db.py index 8fcced1b1..2e658023d 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -2,6 +2,7 @@ from datetime import datetime import uuid +from app.dao.inbound_sms_dao import dao_create_inbound_sms from app.dao.jobs_dao import dao_create_job from app.models import ( InboundSms, @@ -12,6 +13,7 @@ from app.models import ( ScheduledNotification, ServicePermission, Job, + InboundSms, EMAIL_TYPE, SMS_TYPE, KEY_TYPE_NORMAL, @@ -151,14 +153,15 @@ def create_notification( return notification -def create_job(template, - notification_count=1, - created_at=None, - job_status='pending', - scheduled_for=None, - processing_started=None, - original_file_name='some.csv'): - +def create_job( + template, + notification_count=1, + created_at=None, + job_status='pending', + scheduled_for=None, + processing_started=None, + original_file_name='some.csv' +): data = { 'id': uuid.uuid4(), 'service_id': template.service_id, @@ -193,11 +196,12 @@ def create_inbound_sms( user_number='447700900111', provider_date=None, provider_reference=None, - content='Hello' + content='Hello', + created_at=None ): inbound = InboundSms( service=service, - created_at=datetime.utcnow(), + created_at=created_at or datetime.utcnow(), notify_number=notify_number or service.sms_sender, user_number=user_number, provider_date=provider_date or datetime.utcnow(), From 56c3f3cf7c9801393e43b427ee4448b3de9c2738 Mon Sep 17 00:00:00 2001 From: Imdad Ahad Date: Fri, 2 Jun 2017 14:28:52 +0100 Subject: [PATCH 03/10] Add task to delete inbound sms everyday at 1am --- app/celery/scheduled_tasks.py | 19 ++++++++++++++++ app/config.py | 6 +++++ tests/app/celery/test_scheduled_tasks.py | 29 +++++++++++++++++------- 3 files changed, 46 insertions(+), 8 deletions(-) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 457877ed0..6a9cfd055 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -9,6 +9,7 @@ from sqlalchemy.exc import SQLAlchemyError from app.aws import s3 from app import notify_celery from app import performance_platform_client +from app.dao.inbound_sms_dao import delete_inbound_sms_created_more_than_a_week_ago from app.dao.invited_user_dao import delete_invitations_created_more_than_two_days_ago from app.dao.jobs_dao import dao_set_scheduled_jobs_to_pending, dao_get_jobs_older_than_limited_by from app.dao.notifications_dao import ( @@ -226,3 +227,21 @@ def timeout_job_statistics(): if updated: current_app.logger.info( "Timeout period reached for {} job statistics, failure count has been updated.".format(updated)) + + +@notify_celery.task(name="delete-inbound-sms") +@statsd(namespace="tasks") +def delete_inbound_sms_older_than_seven_days(): + try: + start = datetime.utcnow() + deleted = delete_inbound_sms_created_more_than_a_week_ago() + current_app.logger.info( + "Delete inbound sms job started {} finished {} deleted {} inbound sms notifications".format( + start, + datetime.utcnow(), + deleted + ) + ) + except SQLAlchemyError as e: + current_app.logger.exception("Failed to delete inbound sms notifications") + raise diff --git a/app/config.py b/app/config.py index 1cac131e2..01aca1964 100644 --- a/app/config.py +++ b/app/config.py @@ -2,6 +2,7 @@ from datetime import timedelta from celery.schedules import crontab from kombu import Exchange, Queue import os + from app.models import KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST if os.environ.get('VCAP_SERVICES'): @@ -168,6 +169,11 @@ class Config(object): 'schedule': crontab(minute=40, hour=0), 'options': {'queue': QueueNames.PERIODIC} }, + 'delete-inbound-sms': { + 'task': 'delete-inbound-sms', + 'schedule': crontab(minute=0, hour=1), + 'options': {'queue': QueueNames.PERIODIC} + }, 'send-daily-performance-platform-stats': { 'task': 'send-daily-performance-platform-stats', 'schedule': crontab(minute=0, hour=2), diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 7a347e35c..f706db28c 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -5,19 +5,23 @@ from functools import partial from flask import current_app from freezegun import freeze_time -from app.celery.scheduled_tasks import s3, timeout_job_statistics, delete_sms_notifications_older_than_seven_days, \ - delete_letter_notifications_older_than_seven_days, delete_email_notifications_older_than_seven_days, \ - send_scheduled_notifications from app.celery import scheduled_tasks from app.celery.scheduled_tasks import ( + delete_email_notifications_older_than_seven_days, + delete_inbound_sms_older_than_seven_days, + delete_invitations, + delete_notifications_created_more_than_a_week_ago_by_type, + delete_letter_notifications_older_than_seven_days, + delete_sms_notifications_older_than_seven_days, delete_verify_codes, remove_csv_files, - delete_notifications_created_more_than_a_week_ago_by_type, - delete_invitations, - timeout_notifications, run_scheduled_jobs, + s3, send_daily_performance_platform_stats, - switch_current_sms_provider_on_slow_delivery + send_scheduled_notifications, + switch_current_sms_provider_on_slow_delivery, + timeout_job_statistics, + timeout_notifications ) from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient from app.dao.jobs_dao import dao_get_job_by_id @@ -71,7 +75,8 @@ def prepare_current_provider(restore_provider_details): def test_should_have_decorated_tasks_functions(): assert delete_verify_codes.__wrapped__.__name__ == 'delete_verify_codes' - assert delete_notifications_created_more_than_a_week_ago_by_type.__wrapped__.__name__ == 'delete_notifications_created_more_than_a_week_ago_by_type' # noqa + assert delete_notifications_created_more_than_a_week_ago_by_type.__wrapped__.__name__ == \ + 'delete_notifications_created_more_than_a_week_ago_by_type' assert timeout_notifications.__wrapped__.__name__ == 'timeout_notifications' assert delete_invitations.__wrapped__.__name__ == 'delete_invitations' assert run_scheduled_jobs.__wrapped__.__name__ == 'run_scheduled_jobs' @@ -79,6 +84,8 @@ def test_should_have_decorated_tasks_functions(): assert send_daily_performance_platform_stats.__wrapped__.__name__ == 'send_daily_performance_platform_stats' assert switch_current_sms_provider_on_slow_delivery.__wrapped__.__name__ == \ 'switch_current_sms_provider_on_slow_delivery' + assert delete_inbound_sms_older_than_seven_days.__wrapped__.__name__ == \ + 'delete_inbound_sms_older_than_seven_days' def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker): @@ -440,3 +447,9 @@ def test_timeout_job_statistics_called_with_notification_timeout(notify_api, moc dao_mock = mocker.patch('app.celery.scheduled_tasks.dao_timeout_job_statistics') timeout_job_statistics() dao_mock.assert_called_once_with(999) + + +def test_should_call_delete_inbound_sms_older_than_seven_days(notify_api, mocker): + mocker.patch('app.celery.scheduled_tasks.delete_inbound_sms_created_more_than_a_week_ago') + delete_inbound_sms_older_than_seven_days() + assert scheduled_tasks.delete_inbound_sms_created_more_than_a_week_ago.call_count == 1 From 012f8d2675c058af561778402469f806eb9371cc Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Fri, 2 Jun 2017 16:37:57 +0100 Subject: [PATCH 04/10] 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 05/10] 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 06/10] 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 07/10] 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'") From 6b01cfd5b580354de811fa7266811bfb04e31086 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 5 Jun 2017 16:18:40 +0100 Subject: [PATCH 08/10] Add data.gov.uk to the list of organisations MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We need to send an email with data.gov.uk branding. The image for the logo doesn’t exist yet, but doing this migration so we’re ready when it the logo does exist. --- migrations/versions/0092_data_gov_uk.py | 31 +++++++++++++++++++++++++ 1 file changed, 31 insertions(+) create mode 100644 migrations/versions/0092_data_gov_uk.py diff --git a/migrations/versions/0092_data_gov_uk.py b/migrations/versions/0092_data_gov_uk.py new file mode 100644 index 000000000..96c07d578 --- /dev/null +++ b/migrations/versions/0092_data_gov_uk.py @@ -0,0 +1,31 @@ +"""empty message + +Revision ID: 0092_data_gov_uk +Revises: 0091_letter_billing +Create Date: 2017-06-05 16:15:17.744908 + +""" + +# revision identifiers, used by Alembic. +revision = '0092_data_gov_uk' +down_revision = '0091_letter_billing' + +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +DATA_GOV_UK_ID = '123496d4-44cb-4324-8e0a-4187101f4bdc' + +def upgrade(): + op.execute("""INSERT INTO organisation VALUES ( + '{}', + '', + 'data_gov_uk_x2.png', + '' + )""".format(DATA_GOV_UK_ID)) + + +def downgrade(): + op.execute(""" + DELETE FROM organisation WHERE "id" = '{}' + """.format(DATA_GOV_UK_ID)) From d488c592f4757e1f1cebdfc87cc651c0931ea5b9 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 6 Jun 2017 10:59:01 +0100 Subject: [PATCH 09/10] Fix conflict in db migration script --- .../{0092_data_gov_uk.py => 0093_data_gov_uk.py} | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) rename migrations/versions/{0092_data_gov_uk.py => 0093_data_gov_uk.py} (79%) diff --git a/migrations/versions/0092_data_gov_uk.py b/migrations/versions/0093_data_gov_uk.py similarity index 79% rename from migrations/versions/0092_data_gov_uk.py rename to migrations/versions/0093_data_gov_uk.py index 96c07d578..fbe22d38a 100644 --- a/migrations/versions/0092_data_gov_uk.py +++ b/migrations/versions/0093_data_gov_uk.py @@ -1,14 +1,14 @@ """empty message -Revision ID: 0092_data_gov_uk -Revises: 0091_letter_billing +Revision ID: 0093_data_gov_uk +Revises: 0092_add_inbound_provider Create Date: 2017-06-05 16:15:17.744908 """ # revision identifiers, used by Alembic. -revision = '0092_data_gov_uk' -down_revision = '0091_letter_billing' +revision = '0093_data_gov_uk' +down_revision = '0092_add_inbound_provider' from alembic import op import sqlalchemy as sa @@ -16,6 +16,7 @@ from sqlalchemy.dialects import postgresql DATA_GOV_UK_ID = '123496d4-44cb-4324-8e0a-4187101f4bdc' + def upgrade(): op.execute("""INSERT INTO organisation VALUES ( '{}', From 29455b6d3bf366636af602911bd47383162ecf76 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Tue, 6 Jun 2017 11:50:30 +0100 Subject: [PATCH 10/10] Strip leading 44 from inbound SMS numbers to normalise to how we store things. --- app/notifications/receive_notifications.py | 19 +++++++++++++++---- .../test_receive_notification.py | 18 +++++++++++++++--- 2 files changed, 30 insertions(+), 7 deletions(-) diff --git a/app/notifications/receive_notifications.py b/app/notifications/receive_notifications.py index 904e78711..fdd1f95ec 100644 --- a/app/notifications/receive_notifications.py +++ b/app/notifications/receive_notifications.py @@ -27,10 +27,13 @@ def receive_mmg_sms(): } """ post_data = request.get_json() - potential_services = dao_fetch_services_by_sms_sender(post_data['Number']) + + inbound_number = strip_leading_forty_four(post_data['Number']) + + potential_services = dao_fetch_services_by_sms_sender(inbound_number) if len(potential_services) != 1: - current_app.logger.error('Inbound number "{}" not associated with exactly one service'.format( + current_app.logger.error('Inbound number "{}" from MMG not associated with exactly one service'.format( post_data['Number'] )) statsd_client.incr('inbound.mmg.failed') @@ -88,9 +91,11 @@ def create_inbound_mmg_sms_object(service, json): def receive_firetext_sms(): post_data = request.form - potential_services = dao_fetch_services_by_sms_sender(post_data['destination']) + inbound_number = strip_leading_forty_four(post_data['destination']) + + potential_services = dao_fetch_services_by_sms_sender(inbound_number) if len(potential_services) != 1: - current_app.logger.error('Inbound number "{}" not associated with exactly one service'.format( + current_app.logger.error('Inbound number "{}" from firetext not associated with exactly one service'.format( post_data['destination'] )) statsd_client.incr('inbound.firetext.failed') @@ -120,3 +125,9 @@ def receive_firetext_sms(): return jsonify({ "status": "ok" }), 200 + + +def strip_leading_forty_four(number): + if number.startswith('44'): + return number.replace('44', '0', 1) + return number diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 67949104a..66f743be4 100644 --- a/tests/app/notifications/test_receive_notification.py +++ b/tests/app/notifications/test_receive_notification.py @@ -7,11 +7,11 @@ from flask import json from app.notifications.receive_notifications import ( format_mmg_message, format_mmg_datetime, - create_inbound_mmg_sms_object + create_inbound_mmg_sms_object, + strip_leading_forty_four ) from app.models import InboundSms -from tests.app.conftest import sample_service from tests.app.db import create_service @@ -163,7 +163,6 @@ def test_receive_notification_from_firetext_persists_message_with_normalized_pho 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') @@ -181,3 +180,16 @@ def test_returns_ok_to_firetext_if_mismatched_sms_sender(notify_db_session, clie assert not InboundSms.query.all() assert result['status'] == 'ok' mock.assert_has_calls([call('inbound.firetext.failed')]) + + +@pytest.mark.parametrize( + 'number, expected', + [ + ('447123123123', '07123123123'), + ('447123123144', '07123123144'), + ('07123123123', '07123123123'), + ('447444444444', '07444444444') + ] +) +def test_strip_leading_country_code(number, expected): + assert strip_leading_forty_four(number) == expected