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/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/app/models.py b/app/models.py index 29272c380..1bcd66a19 100644 --- a/app/models.py +++ b/app/models.py @@ -1165,6 +1165,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 0ed644904..fdd1f95ec 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 validate_and_format_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 @@ -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') @@ -38,7 +41,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] @@ -78,6 +81,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 @@ -86,8 +90,44 @@ 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)) + + 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 "{}" from firetext not associated with exactly one service'.format( + post_data['destination'] + )) + statsd_client.incr('inbound.firetext.failed') + return jsonify({ + "status": "ok" + }), 200 + + service = potential_services[0] + + user_number = validate_and_format_phone_number(post_data['source'], international=True) + 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, + provider=firetext_client.name + ) + ) + + statsd_client.incr('inbound.firetext.successful') 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/migrations/versions/0092_add_inbound_provider.py b/migrations/versions/0092_add_inbound_provider.py new file mode 100644 index 000000000..f7e5f510e --- /dev/null +++ b/migrations/versions/0092_add_inbound_provider.py @@ -0,0 +1,22 @@ +"""empty message + +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 = '0092_add_inbound_provider' +down_revision = '0091_letter_billing' + +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/migrations/versions/0093_data_gov_uk.py b/migrations/versions/0093_data_gov_uk.py new file mode 100644 index 000000000..fbe22d38a --- /dev/null +++ b/migrations/versions/0093_data_gov_uk.py @@ -0,0 +1,32 @@ +"""empty message + +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 = '0093_data_gov_uk' +down_revision = '0092_add_inbound_provider' + +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)) 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 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(), diff --git a/tests/app/notifications/test_receive_notification.py b/tests/app/notifications/test_receive_notification.py index 317d9452e..66f743be4 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 @@ -6,7 +7,8 @@ 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 @@ -68,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']) @@ -92,7 +95,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 +110,86 @@ 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): + 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 == '447999999999' + 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) + + +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')]) + + +@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