From 3966efb1097992ef5ca6575788bea9e10a562597 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Fri, 27 Oct 2017 01:03:34 +0100 Subject: [PATCH 01/11] Update marshmallow from 2.13.6 to 2.14.0 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 9d576ab16..9abddb54b 100644 --- a/requirements.txt +++ b/requirements.txt @@ -12,7 +12,7 @@ gunicorn==19.7.1 iso8601==0.1.12 jsonschema==2.6.0 marshmallow-sqlalchemy==0.13.2 -marshmallow==2.13.6 +marshmallow==2.14.0 monotonic==1.3 psycopg2==2.7.3.2 PyJWT==1.5.3 From 4eec11b633bb20d54babaf83fd2d144356e9abd8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 30 Oct 2017 13:36:49 +0000 Subject: [PATCH 02/11] Added an optional parameter in the form for POST /v2/notifications/sms and /service//send-notification to pass in the SMS sender id. The send_sms_to_provider still needs to use the SMS sender being passed in to the POST. As part of https://www.pivotaltracker.com/story/show/152106587 --- app/dao/notifications_dao.py | 13 +++- app/notifications/process_notifications.py | 7 +- app/notifications/validators.py | 19 ++++- app/service/send_notification.py | 16 ++-- app/v2/notifications/notification_schemas.py | 3 +- app/v2/notifications/post_notifications.py | 24 ++++-- .../notification_dao/test_notification_dao.py | 22 ++++-- .../test_process_notification.py | 19 ++++- tests/app/notifications/test_validators.py | 66 +++++++++++++--- .../service/test_send_one_off_notification.py | 45 ++++++++++- tests/app/test_model.py | 1 - .../notifications/test_post_notifications.py | 78 ++++++++++++++----- 12 files changed, 251 insertions(+), 62 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1c010c86b..423c73164 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -43,8 +43,8 @@ from app.models import ( NOTIFICATION_TECHNICAL_FAILURE, NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, - NOTIFICATION_SENT -) + NOTIFICATION_SENT, + NotificationSmsSender) from app.dao.dao_utils import transactional from app.statsd_decorators import statsd @@ -650,3 +650,12 @@ def dao_get_last_notification_added_for_job_id(job_id): ).first() return last_notification_added + + +@transactional +def dao_create_notification_sms_sender_mapping(notification_id, sms_sender_id): + notification_to_sms_sender = NotificationSmsSender( + notification_id=notification_id, + service_sms_sender_id=sms_sender_id + ) + db.session.add(notification_to_sms_sender) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 21fda224e..76e18ebf4 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -17,7 +17,8 @@ from app.models import SMS_TYPE, Notification, KEY_TYPE_TEST, EMAIL_TYPE, NOTIFI from app.dao.notifications_dao import (dao_create_notification, dao_delete_notifications_and_history_by_id, dao_created_scheduled_notification, - dao_create_notification_email_reply_to_mapping) + dao_create_notification_email_reply_to_mapping, + dao_create_notification_sms_sender_mapping) from app.v2.errors import BadRequestError from app.utils import get_template_instance, cache_key_for_service_template_counter, convert_bst_to_utc @@ -146,3 +147,7 @@ def persist_scheduled_notification(notification_id, scheduled_for): def persist_email_reply_to_id_for_notification(notification_id, email_reply_to_id): dao_create_notification_email_reply_to_mapping(notification_id, email_reply_to_id) + + +def persist_sms_sender_id_for_notification(notification_id, sms_sender_id): + dao_create_notification_sms_sender_mapping(notification_id, sms_sender_id) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 181d48b6e..973aafaf5 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -8,6 +8,7 @@ from notifications_utils.recipients import ( from notifications_utils.clients.redis import rate_limit_cache_key, daily_limit_cache_key from app.dao import services_dao, templates_dao +from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id from app.models import ( INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE, KEY_TYPE_TEST, KEY_TYPE_TEAM, SCHEDULE_NOTIFICATIONS @@ -135,11 +136,27 @@ def validate_template(template_id, personalisation, service, notification_type): return template, template_with_content -def check_service_email_reply_to_id(service_id, reply_to_id): +def check_service_email_reply_to_id(service_id, reply_to_id, notification_type): if not (reply_to_id is None): + if notification_type != EMAIL_TYPE: + message = 'You sent a email_reply_to_id for a {} notification type'.format(notification_type) + raise BadRequestError(message=message) try: dao_get_reply_to_by_id(service_id, reply_to_id) except NoResultFound: message = 'email_reply_to_id {} does not exist in database for service id {}'\ .format(reply_to_id, service_id) raise BadRequestError(message=message) + + +def check_service_sms_sender_id(service_id, sms_sender_id, notification_type): + if not (sms_sender_id is None): + if notification_type != SMS_TYPE: + message = 'You sent a sms_sender_id for a {} notification type'.format(notification_type) + raise BadRequestError(message=message) + try: + dao_get_service_sms_senders_by_id(service_id, sms_sender_id) + except NoResultFound: + message = 'sms_sender_id {} does not exist in database for service id {}'\ + .format(sms_sender_id, service_id) + raise BadRequestError(message=message) diff --git a/app/service/send_notification.py b/app/service/send_notification.py index d1ed9a5a3..fa6dd1bb2 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -2,13 +2,13 @@ from app.config import QueueNames from app.notifications.validators import ( check_service_over_daily_message_limit, validate_and_format_recipient, - validate_template, - check_service_email_reply_to_id) + validate_template) from app.notifications.process_notifications import ( - create_content_for_notification, persist_notification, send_notification_to_queue, - persist_email_reply_to_id_for_notification) + persist_email_reply_to_id_for_notification, + persist_sms_sender_id_for_notification +) from app.models import ( KEY_TYPE_NORMAL, PRIORITY, @@ -64,9 +64,11 @@ def send_one_off_notification(service_id, post_data): created_by_id=post_data['created_by'] ) sender_id = post_data.get('sender_id', None) - if sender_id and template.template_type == EMAIL_TYPE: - check_service_email_reply_to_id(service_id, sender_id) - persist_email_reply_to_id_for_notification(notification.id, sender_id) + if sender_id: + if template.template_type == EMAIL_TYPE: + persist_email_reply_to_id_for_notification(notification.id, sender_id) + if template.template_type == SMS_TYPE: + persist_sms_sender_id_for_notification(notification.id, sender_id) queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None send_notification_to_queue( diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 1689a7319..258b5df0d 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -119,7 +119,8 @@ post_sms_request = { "phone_number": {"type": "string", "format": "phone_number"}, "template_id": uuid, "personalisation": personalisation, - "scheduled_for": {"type": ["string", "null"], "format": "datetime"} + "scheduled_for": {"type": ["string", "null"], "format": "datetime"}, + "sms_sender_id": uuid }, "required": ["phone_number", "template_id"] } diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index b4c837306..87d55159d 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -20,8 +20,8 @@ from app.notifications.process_notifications import ( persist_notification, persist_scheduled_notification, send_notification_to_queue, - simulated_recipient -) + simulated_recipient, + persist_sms_sender_id_for_notification) from app.notifications.process_letter_notifications import ( create_letter_notification ) @@ -31,7 +31,8 @@ from app.notifications.validators import ( check_service_can_schedule_notification, check_service_has_permission, validate_template, - check_service_email_reply_to_id + check_service_email_reply_to_id, + check_service_sms_sender_id ) from app.schema_validation import validate from app.v2.errors import BadRequestError @@ -63,12 +64,14 @@ def post_notification(notification_type): scheduled_for = form.get("scheduled_for", None) service_email_reply_to_id = form.get("email_reply_to_id", None) + service_sms_sender_id = form.get("sms_sender_id", None) check_service_can_schedule_notification(authenticated_service.permissions, scheduled_for) check_rate_limiting(authenticated_service, api_user) - check_service_email_reply_to_id(str(authenticated_service.id), service_email_reply_to_id) + check_service_email_reply_to_id(str(authenticated_service.id), service_email_reply_to_id, notification_type) + check_service_sms_sender_id(str(authenticated_service.id), service_sms_sender_id, notification_type) template, template_with_content = validate_template( form['template_id'], @@ -142,9 +145,7 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ simulated=simulated ) - email_reply_to_id = form.get("email_reply_to_id", None) - if email_reply_to_id: - persist_email_reply_to_id_for_notification(notification.id, email_reply_to_id) + persist_sender_to_notification_mapping(form, notification) scheduled_for = form.get("scheduled_for", None) if scheduled_for: @@ -163,6 +164,15 @@ def process_sms_or_email_notification(*, form, notification_type, api_key, templ return notification +def persist_sender_to_notification_mapping(form, notification): + email_reply_to_id = form.get("email_reply_to_id", None) + if email_reply_to_id: + persist_email_reply_to_id_for_notification(notification.id, email_reply_to_id) + sms_sender_id = form.get("sms_sender_id", None) + if sms_sender_id: + persist_sms_sender_id_for_notification(notification.id, sms_sender_id) + + def process_letter_notification(*, letter_data, api_key, template): if api_key.key_type == KEY_TYPE_TEAM: raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 562fd8555..9c5edc130 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -14,7 +14,6 @@ from app.models import ( NotificationHistory, NotificationStatistics, ScheduledNotification, - ServiceEmailReplyTo, EMAIL_TYPE, NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_TYPES_FAILED, @@ -23,7 +22,7 @@ from app.models import ( KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, - JOB_STATUS_IN_PROGRESS) + JOB_STATUS_IN_PROGRESS, NotificationSmsSender) from app.dao.notifications_dao import ( dao_create_notification, @@ -51,15 +50,18 @@ from app.dao.notifications_dao import ( set_scheduled_notification_to_processed, update_notification_status_by_id, update_notification_status_by_reference, - dao_get_last_notification_added_for_job_id, dao_update_notifications_by_reference) + dao_get_last_notification_added_for_job_id, + dao_update_notifications_by_reference, + dao_create_notification_sms_sender_mapping +) from app.dao.services_dao import dao_update_service from tests.app.db import ( create_api_key, create_job, create_notification, - create_reply_to_email -) + create_reply_to_email, + create_service_sms_sender) from tests.app.conftest import ( sample_notification, sample_template, @@ -2100,3 +2102,13 @@ def test_dao_update_notifications_by_reference_returns_zero_when_no_notification "billable_units": 2} ) assert updated_count == 0 + + +def test_dao_create_notification_sms_sender_mapping(sample_notification): + sms_sender = create_service_sms_sender(service=sample_notification.service, sms_sender='123456') + dao_create_notification_sms_sender_mapping(notification_id=sample_notification.id, + sms_sender_id=sms_sender.id) + notification_to_senders = NotificationSmsSender.query.all() + assert len(notification_to_senders) == 1 + assert notification_to_senders[0].notification_id == sample_notification.id + assert notification_to_senders[0].service_sms_sender_id == sms_sender.id diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index e8e67568b..941ed14f0 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -8,19 +8,20 @@ from freezegun import freeze_time from collections import namedtuple from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_service_id -from app.models import Template, Notification, NotificationHistory, ScheduledNotification, NotificationEmailReplyTo +from app.models import Template, Notification, NotificationHistory, ScheduledNotification, NotificationEmailReplyTo, \ + NotificationSmsSender from app.notifications.process_notifications import ( create_content_for_notification, persist_notification, send_notification_to_queue, simulated_recipient, persist_scheduled_notification, - persist_email_reply_to_id_for_notification) + persist_email_reply_to_id_for_notification, persist_sms_sender_id_for_notification) from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address from app.utils import cache_key_for_service_template_counter from app.v2.errors import BadRequestError from tests.app.conftest import sample_api_key as create_api_key -from tests.app.db import create_reply_to_email +from tests.app.db import create_reply_to_email, create_service_sms_sender def test_create_content_for_notification_passes(sample_email_template): @@ -452,3 +453,15 @@ def test_persist_email_reply_to_id_for_notification(sample_service, sample_notif assert len(email_reply_to) == 1 assert email_reply_to[0].notification_id == sample_notification.id assert email_reply_to[0].service_email_reply_to_id == reply_to_address[0].id + + +def test_persist_sms_sender_id_for_notification(sample_service, sample_notification): + sms_sender = create_service_sms_sender(service=sample_service, sms_sender="123456") + persist_sms_sender_id_for_notification(notification_id=sample_notification.id, + sms_sender_id=sms_sender.id) + + notification_to_sms_sender = NotificationSmsSender.query.all() + + assert len(notification_to_sms_sender) == 1 + assert notification_to_sms_sender[0].notification_id == sample_notification.id + assert notification_to_sms_sender[0].service_sms_sender_id == sms_sender.id diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index fecfc4da5..d9c643620 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -11,8 +11,8 @@ from app.notifications.validators import ( check_sms_content_char_count, check_service_over_api_rate_limit, validate_and_format_recipient, - check_service_email_reply_to_id -) + check_service_email_reply_to_id, + check_service_sms_sender_id) from app.v2.errors import ( BadRequestError, @@ -23,7 +23,7 @@ from tests.app.conftest import ( sample_service as create_service, sample_service_whitelist, sample_api_key) -from tests.app.db import create_reply_to_email +from tests.app.db import create_reply_to_email, create_service_sms_sender @pytest.mark.parametrize('key_type', ['test', 'team', 'normal']) @@ -331,14 +331,15 @@ def test_allows_api_calls_with_international_numbers_if_service_does_allow_int_s assert result == '201212341234' -def test_check_service_email_reply_to_id_where_reply_to_id_is_none(): - assert check_service_email_reply_to_id(None, None) is None +@pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) +def test_check_service_email_reply_to_id_where_reply_to_id_is_none(notification_type): + assert check_service_email_reply_to_id(None, None, notification_type) is None def test_check_service_email_reply_to_id_where_service_id_is_not_found(sample_service, fake_uuid): reply_to_address = create_reply_to_email(sample_service, "test@test.com") with pytest.raises(BadRequestError) as e: - check_service_email_reply_to_id(fake_uuid, reply_to_address.id) + check_service_email_reply_to_id(fake_uuid, reply_to_address.id, EMAIL_TYPE) assert e.value.status_code == 400 assert e.value.message == 'email_reply_to_id {} does not exist in database for service id {}'\ .format(reply_to_address.id, fake_uuid) @@ -346,12 +347,57 @@ def test_check_service_email_reply_to_id_where_service_id_is_not_found(sample_se def test_check_service_email_reply_to_id_where_reply_to_id_is_not_found(sample_service, fake_uuid): with pytest.raises(BadRequestError) as e: - check_service_email_reply_to_id(sample_service.id, fake_uuid) + check_service_email_reply_to_id(sample_service.id, fake_uuid, EMAIL_TYPE) assert e.value.status_code == 400 assert e.value.message == 'email_reply_to_id {} does not exist in database for service id {}'\ .format(fake_uuid, sample_service.id) -def test_check_service_email_reply_to_id_where_reply_to_id_is_found(sample_service): - reply_to_email = create_reply_to_email(sample_service, 'test@test.com') - assert check_service_email_reply_to_id(sample_service.id, reply_to_email.id) is None +def test_check_service_sms_sender_id_where_sms_sender_id_is_found(sample_service): + sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') + assert check_service_sms_sender_id(sample_service.id, sms_sender.id, SMS_TYPE) is None + + +@pytest.mark.parametrize('notification_type', ['email', 'letter']) +def test_check_service_sms_sender_id_when_channel_type_is_wrong(sample_service, notification_type): + sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') + with pytest.raises(BadRequestError) as e: + check_service_sms_sender_id(sample_service.id, sms_sender.id, notification_type) + assert e.value.status_code == 400 + assert e.value.message == 'You sent a sms_sender_id for a {} notification type'.format(notification_type) + + +@pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) +def test_check_service_sms_sender_id_where_reply_to_id_is_valid(notification_type): + assert check_service_sms_sender_id(None, None, notification_type) is None + + +def test_check_service_sms_sender_id_where_reply_to_id_is_found(sample_service): + sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') + assert check_service_sms_sender_id(sample_service.id, sms_sender.id, SMS_TYPE) is None + + +def test_check_service_sms_sender_id_where_service_id_is_not_found(sample_service, fake_uuid): + sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') + with pytest.raises(BadRequestError) as e: + check_service_sms_sender_id(fake_uuid, sms_sender.id, SMS_TYPE) + assert e.value.status_code == 400 + assert e.value.message == 'sms_sender_id {} does not exist in database for service id {}'\ + .format(sms_sender.id, fake_uuid) + + +def test_check_service_sms_sender_id_where_reply_to_id_is_not_found(sample_service, fake_uuid): + with pytest.raises(BadRequestError) as e: + check_service_sms_sender_id(sample_service.id, fake_uuid, SMS_TYPE) + assert e.value.status_code == 400 + assert e.value.message == 'sms_sender_id {} does not exist in database for service id {}'\ + .format(fake_uuid, sample_service.id) + + +@pytest.mark.parametrize('notification_type', ['email', 'letter']) +def test_check_service_email_reply_to_id_when_channel_type_is_wrong(sample_service, notification_type): + sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') + with pytest.raises(BadRequestError) as e: + check_service_sms_sender_id(sample_service.id, sms_sender.id, notification_type) + assert e.value.status_code == 400 + assert e.value.message == 'You sent a sms_sender_id for a {} notification type'.format(notification_type) diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 1fe9dc155..64650434c 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -3,6 +3,7 @@ from unittest.mock import Mock import pytest from notifications_utils.recipients import InvalidPhoneError +from sqlalchemy.exc import SQLAlchemyError from app.v2.errors import BadRequestError, TooManyRequestsError from app.config import QueueNames @@ -12,9 +13,11 @@ from app.models import ( PRIORITY, SMS_TYPE, NotificationEmailReplyTo, - Notification) + Notification, + NotificationSmsSender +) -from tests.app.db import create_user, create_reply_to_email +from tests.app.db import create_user, create_reply_to_email, create_service_sms_sender @pytest.fixture @@ -206,7 +209,7 @@ def test_send_one_off_notification_should_add_email_reply_to_id_for_email(sample def test_send_one_off_notification_should_throw_exception_if_reply_to_id_doesnot_exist( - sample_email_template, celery_mock + sample_email_template ): data = { 'to': 'ok@ok.com', @@ -215,5 +218,39 @@ def test_send_one_off_notification_should_throw_exception_if_reply_to_id_doesnot 'created_by': str(sample_email_template.service.created_by_id) } - with pytest.raises(expected_exception=BadRequestError): + with pytest.raises(expected_exception=SQLAlchemyError): send_one_off_notification(service_id=sample_email_template.service.id, post_data=data) + + +def test_send_one_off_notification_should_add_sms_sender_mapping_for_sms(sample_template, celery_mock): + sms_sender = create_service_sms_sender(service=sample_template.service, sms_sender='123456') + data = { + 'to': '07700 900 001', + 'template_id': str(sample_template.id), + 'sender_id': sms_sender.id, + 'created_by': str(sample_template.service.created_by_id) + } + + notification_id = send_one_off_notification(service_id=sample_template.service.id, post_data=data) + notification = Notification.query.get(notification_id['id']) + celery_mock.assert_called_once_with( + notification=notification, + research_mode=False, + queue=None + ) + mapping_row = NotificationSmsSender.query.filter_by(notification_id=notification_id['id']).first() + assert mapping_row.service_sms_sender_id == sms_sender.id + + +def test_send_one_off_notification_should_throw_exception_if_sms_sender_id_doesnot_exist( + sample_template +): + data = { + 'to': '07700 900 001', + 'template_id': str(sample_template.id), + 'sender_id': str(uuid.uuid4()), + 'created_by': str(sample_template.service.created_by_id) + } + + with pytest.raises(expected_exception=SQLAlchemyError): + send_one_off_notification(service_id=sample_template.service.id, post_data=data) diff --git a/tests/app/test_model.py b/tests/app/test_model.py index 8809a51bb..7062e7a30 100644 --- a/tests/app/test_model.py +++ b/tests/app/test_model.py @@ -28,7 +28,6 @@ from tests.app.db import ( create_service, create_inbound_number, create_reply_to_email, - create_service_sms_sender, create_letter_contact ) from tests.conftest import set_config diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 9d63c6bfb..a76702706 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -8,8 +8,8 @@ from app.models import ( ScheduledNotification, SCHEDULE_NOTIFICATIONS, EMAIL_TYPE, - SMS_TYPE -) + SMS_TYPE, + NotificationSmsSender) from flask import json, current_app from app.models import Notification @@ -27,8 +27,8 @@ from tests.app.conftest import ( from tests.app.db import ( create_service, create_template, - create_reply_to_email -) + create_reply_to_email, + create_service_sms_sender) @pytest.mark.parametrize("reference", [None, "reference_from_client"]) @@ -93,6 +93,34 @@ def test_post_sms_notification_uses_inbound_number_as_sender(client, notify_db_s mocked.assert_called_once_with([str(notification_id)], queue='send-sms-tasks') +def test_post_sms_notification_returns_201_with_sms_sender_id( + client, sample_template_with_placeholders, mocker +): + + sms_sender = create_service_sms_sender(service=sample_template_with_placeholders.service, sms_sender='123456') + mocked = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') + data = { + 'phone_number': '+447700900855', + 'template_id': str(sample_template_with_placeholders.id), + 'personalisation': {' Name': 'Jo'}, + 'sms_sender_id': str(sms_sender.id) + } + auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) + + response = client.post( + path='/v2/notifications/sms', + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + assert response.status_code == 201 + resp_json = json.loads(response.get_data(as_text=True)) + assert validate(resp_json, post_sms_response) == resp_json + notification_to_sms_sender = NotificationSmsSender.query.all() + assert len(notification_to_sms_sender) == 1 + assert str(notification_to_sms_sender[0].notification_id) == resp_json['id'] + assert notification_to_sms_sender[0].service_sms_sender_id == sms_sender.id + mocked.assert_called_once_with([resp_json['id']], queue='send-sms-tasks') + + @pytest.mark.parametrize("notification_type, key_send_to, send_to", [("sms", "phone_number", "+447700900855"), ("email", "email_address", "sample@email.com")]) @@ -501,30 +529,40 @@ def test_post_notification_raises_bad_request_if_not_valid_notification_type(cli assert 'The requested URL was not found on the server.' in error_json['message'] -@pytest.mark.parametrize("reference", [None, "reference_from_client"]) -def test_post_sms_notification_with_invalid_reply_to_email_id( +@pytest.mark.parametrize("notification_type", + ['sms', 'email']) +def test_post_notification_with_wrong_type_of_sender( client, - sample_template_with_placeholders, - reference, + sample_template, + sample_email_template, + notification_type, fake_uuid): - data = { - 'phone_number': '+447700900855', - 'template_id': str(sample_template_with_placeholders.id), - 'personalisation': {' Name': 'Jo'}, - 'email_reply_to_id': fake_uuid - } - if reference: - data.update({"reference": reference}) - auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) + if notification_type == EMAIL_TYPE: + template = sample_email_template + form_label = 'sms_sender_id' + data = { + 'email_address': 'test@test.com', + 'template_id': str(sample_email_template.id), + form_label: fake_uuid + } + elif notification_type == SMS_TYPE: + template = sample_template + form_label = 'email_reply_to_id' + data = { + 'phone_number': '+447700900855', + 'template_id': str(template.id), + form_label: fake_uuid + } + auth_header = create_authorization_header(service_id=template.service_id) response = client.post( - path='/v2/notifications/sms', + path='/v2/notifications/{}'.format(notification_type), data=json.dumps(data), headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 400 resp_json = json.loads(response.get_data(as_text=True)) - assert 'email_reply_to_id {} does not exist in database for service id {}'.\ - format(fake_uuid, sample_template_with_placeholders.service_id) in resp_json['errors'][0]['message'] + assert 'You sent a {} for a {} notification type'.\ + format(form_label, notification_type) in resp_json['errors'][0]['message'] assert 'BadRequestError' in resp_json['errors'][0]['error'] From 0887910b1b648a0d06be4ce4067d2cb051eb83b2 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 30 Oct 2017 14:55:44 +0000 Subject: [PATCH 03/11] Get the sms sender from the notificaiton_sms_sender mapping table if that does not exist get the default sms sender to pass on to the sms provider. --- app/dao/notifications_dao.py | 15 ++++++++++- app/delivery/send_to_providers.py | 10 ++++--- .../notification_dao/test_notification_dao.py | 17 ++++++++++-- tests/app/db.py | 10 ++++++- tests/app/delivery/test_send_to_providers.py | 26 +++++++++++++++++-- 5 files changed, 69 insertions(+), 9 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 423c73164..4de97170e 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -44,7 +44,9 @@ from app.models import ( NOTIFICATION_TEMPORARY_FAILURE, NOTIFICATION_PERMANENT_FAILURE, NOTIFICATION_SENT, - NotificationSmsSender) + NotificationSmsSender, + ServiceSmsSender +) from app.dao.dao_utils import transactional from app.statsd_decorators import statsd @@ -659,3 +661,14 @@ def dao_create_notification_sms_sender_mapping(notification_id, sms_sender_id): service_sms_sender_id=sms_sender_id ) db.session.add(notification_to_sms_sender) + + +def dao_get_notification_sms_sender_mapping(notification_id): + sms_sender = ServiceSmsSender.query.join( + NotificationSmsSender + ).filter( + NotificationSmsSender.notification_id == notification_id + ).first() + + if sms_sender: + return sms_sender.sms_sender diff --git a/app/delivery/send_to_providers.py b/app/delivery/send_to_providers.py index c253748ad..324c21582 100644 --- a/app/delivery/send_to_providers.py +++ b/app/delivery/send_to_providers.py @@ -11,8 +11,8 @@ from notifications_utils.template import HTMLEmailTemplate, PlainTextEmailTempla from app import clients, statsd_client, create_uuid from app.dao.notifications_dao import ( dao_update_notification, - dao_get_notification_email_reply_for_notification -) + dao_get_notification_email_reply_for_notification, + dao_get_notification_sms_sender_mapping) from app.dao.provider_details_dao import ( get_provider_details_by_notification_type, dao_toggle_sms_provider @@ -69,11 +69,15 @@ def send_sms_to_provider(notification): raise else: try: + sms_sender = dao_get_notification_sms_sender_mapping(notification.id) + if not sms_sender: + sms_sender = service.get_default_sms_sender() + provider.send_sms( to=validate_and_format_phone_number(notification.to, international=notification.international), content=str(template), reference=str(notification.id), - sender=service.get_default_sms_sender() + sender=sms_sender ) except Exception as e: dao_toggle_sms_provider(provider.name) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 9c5edc130..0cf5fff7b 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -52,8 +52,8 @@ from app.dao.notifications_dao import ( update_notification_status_by_reference, dao_get_last_notification_added_for_job_id, dao_update_notifications_by_reference, - dao_create_notification_sms_sender_mapping -) + dao_create_notification_sms_sender_mapping, + dao_get_notification_sms_sender_mapping) from app.dao.services_dao import dao_update_service from tests.app.db import ( @@ -2112,3 +2112,16 @@ def test_dao_create_notification_sms_sender_mapping(sample_notification): assert len(notification_to_senders) == 1 assert notification_to_senders[0].notification_id == sample_notification.id assert notification_to_senders[0].service_sms_sender_id == sms_sender.id + + +def test_dao_get_notification_sms_sender_mapping(sample_notification): + sms_sender = create_service_sms_sender(service=sample_notification.service, sms_sender='123456') + dao_create_notification_sms_sender_mapping(notification_id=sample_notification.id, + sms_sender_id=sms_sender.id) + notification_to_sender = dao_get_notification_sms_sender_mapping(sample_notification.id) + assert notification_to_sender == '123456' + + +def test_dao_get_notification_sms_sender_mapping_returns_none(sample_notification): + notification_to_sender = dao_get_notification_sms_sender_mapping(sample_notification.id) + assert not notification_to_sender diff --git a/tests/app/db.py b/tests/app/db.py index 0cdf7cfee..4be3a3799 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -29,7 +29,11 @@ from app.models import ( KEY_TYPE_NORMAL ) from app.dao.users_dao import save_model_user -from app.dao.notifications_dao import dao_create_notification, dao_created_scheduled_notification +from app.dao.notifications_dao import ( + dao_create_notification, + dao_created_scheduled_notification, + dao_create_notification_sms_sender_mapping +) from app.dao.templates_dao import dao_create_template from app.dao.services_dao import dao_create_service from app.dao.service_permissions_dao import dao_add_service_permission @@ -128,6 +132,7 @@ def create_notification( scheduled_for=None, normalised_to=None, one_off=False, + sms_sender_id=None ): if created_at is None: created_at = datetime.utcnow() @@ -184,6 +189,9 @@ def create_notification( if status != 'created': scheduled_notification.pending = False dao_created_scheduled_notification(scheduled_notification) + if sms_sender_id: + dao_create_notification_sms_sender_mapping(notification_id=notification.id, + sms_sender_id=sms_sender_id) return notification diff --git a/tests/app/delivery/test_send_to_providers.py b/tests/app/delivery/test_send_to_providers.py index 6733c5846..2d6a10cdf 100644 --- a/tests/app/delivery/test_send_to_providers.py +++ b/tests/app/delivery/test_send_to_providers.py @@ -32,8 +32,8 @@ from tests.app.db import ( create_notification, create_inbound_number, create_reply_to_email, - create_reply_to_email_for_notification -) + create_reply_to_email_for_notification, + create_service_sms_sender) def test_should_return_highest_priority_active_provider(restore_provider_details): @@ -343,6 +343,28 @@ def test_should_send_sms_with_downgraded_content(notify_db_session, mocker): ) +def test_send_sms_should_use_service_sms_sender( + sample_service, + sample_template, + mocker): + mocker.patch('app.mmg_client.send_sms') + mocker.patch('app.delivery.send_to_providers.create_initial_notification_statistic_tasks') + + sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456', is_default=False) + db_notification = create_notification(template=sample_template, sms_sender_id=sms_sender.id) + + send_to_providers.send_sms_to_provider( + db_notification, + ) + + app.mmg_client.send_sms.assert_called_once_with( + to=ANY, + content=ANY, + reference=ANY, + sender=sms_sender.sms_sender + ) + + @pytest.mark.parametrize('research_mode,key_type', [ (True, KEY_TYPE_NORMAL), (False, KEY_TYPE_TEST) From db6668eb6105bed51e6e33ade9460e8cd5259033 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 30 Oct 2017 15:17:01 +0000 Subject: [PATCH 04/11] Added the notification_to_sms_sender mapping table to the purge notifications query --- app/dao/notifications_dao.py | 14 +++++--- app/dao/services_dao.py | 2 +- .../notification_dao/test_notification_dao.py | 34 ++++++++++++++++++- 3 files changed, 44 insertions(+), 6 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 4de97170e..56794c394 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -33,6 +33,7 @@ from app.models import ( ServiceEmailReplyTo, Template, EMAIL_TYPE, + SMS_TYPE, KEY_TYPE_NORMAL, KEY_TYPE_TEST, LETTER_TYPE, @@ -374,17 +375,22 @@ def delete_notifications_created_more_than_a_week_ago_by_type(notification_type) seven_days_ago = date.today() - timedelta(days=7) # Following could be refactored when NotificationSmsReplyTo and NotificationLetterContact in models.py - if notification_type == EMAIL_TYPE: + if notification_type in [EMAIL_TYPE, SMS_TYPE]: subq = db.session.query(Notification.id).filter( func.date(Notification.created_at) < seven_days_ago, Notification.notification_type == notification_type ).subquery() - deleted = db.session.query( - NotificationEmailReplyTo + if notification_type == EMAIL_TYPE: + notification_sender_mapping_table = NotificationEmailReplyTo + if notification_type == SMS_TYPE: + notification_sender_mapping_table = NotificationSmsSender + db.session.query( + notification_sender_mapping_table ).filter( - NotificationEmailReplyTo.notification_id.in_(subq) + notification_sender_mapping_table.notification_id.in_(subq) ).delete(synchronize_session='fetch') + deleted = db.session.query(Notification).filter( func.date(Notification.created_at) < seven_days_ago, Notification.notification_type == notification_type, diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index f9347b6b7..986a0b7bb 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -10,7 +10,7 @@ from app.dao.dao_utils import ( transactional, version_class ) -from app.dao.notifications_dao import get_financial_year +from app.dao.date_util import get_financial_year from app.dao.service_sms_sender_dao import insert_service_sms_sender from app.models import ( NotificationStatistics, diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 0cf5fff7b..cfbc2dd77 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -22,7 +22,7 @@ from app.models import ( KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, - JOB_STATUS_IN_PROGRESS, NotificationSmsSender) + JOB_STATUS_IN_PROGRESS, NotificationSmsSender, SMS_TYPE) from app.dao.notifications_dao import ( dao_create_notification, @@ -1044,6 +1044,38 @@ def test_should_delete_notification_to_email_reply_to_after_seven_days( assert notification.created_at.date() >= date(2016, 1, 3) +@freeze_time("2016-01-10 12:00:00.000000") +def test_should_delete_notification_to_sms_sender_after_seven_days( + sample_template +): + assert len(Notification.query.all()) == 0 + + sms_sender = create_service_sms_sender(service=sample_template.service, sms_sender='123456', is_default=False) + + # create one notification a day between 1st and 10th from 11:00 to 19:00 of each type + for i in range(1, 11): + past_date = '2016-01-{0:02d} {0:02d}:00:00.000000'.format(i) + with freeze_time(past_date): + notification = create_notification(template=sample_template, sms_sender_id=sms_sender.id) + + all_notifications = Notification.query.all() + assert len(all_notifications) == 10 + + all_notification_sms_senders = NotificationSmsSender.query.all() + assert len(all_notification_sms_senders) == 10 + + # Records before 3rd should be deleted + delete_notifications_created_more_than_a_week_ago_by_type(SMS_TYPE) + remaining_notifications = Notification.query.filter_by(notification_type=SMS_TYPE).all() + remaining_notification_to_sms_sender = NotificationSmsSender.query.filter_by().all() + + assert len(remaining_notifications) == 8 + assert len(remaining_notification_to_sms_sender) == 8 + + for notification in remaining_notifications: + assert notification.created_at.date() >= date(2016, 1, 3) + + @pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) @freeze_time("2016-01-10 12:00:00.000000") def test_should_not_delete_notification_history(notify_db, notify_db_session, sample_service, notification_type): From d671adc1eb7e37f6dc93bc17a76cc937600c6902 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 30 Oct 2017 16:51:27 +0000 Subject: [PATCH 05/11] Fix codestyle --- app/dao/notifications_dao.py | 1 - 1 file changed, 1 deletion(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 56794c394..59263cd9f 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -390,7 +390,6 @@ def delete_notifications_created_more_than_a_week_ago_by_type(notification_type) notification_sender_mapping_table.notification_id.in_(subq) ).delete(synchronize_session='fetch') - deleted = db.session.query(Notification).filter( func.date(Notification.created_at) < seven_days_ago, Notification.notification_type == notification_type, From a95d6e8b2ef2d5da967ce9c99f50e7077ddac88a Mon Sep 17 00:00:00 2001 From: chrisw Date: Tue, 31 Oct 2017 10:36:53 +0000 Subject: [PATCH 06/11] allow updating auth type --- app/schemas.py | 1 + tests/app/user/test_rest.py | 12 ++++++++++++ 2 files changed, 13 insertions(+) diff --git a/app/schemas.py b/app/schemas.py index b646b7c5c..b6ffe738c 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -107,6 +107,7 @@ class UserSchema(BaseSchema): class UserUpdateAttributeSchema(BaseSchema): + auth_type = field_for(models.User, 'auth_type') class Meta: model = models.User diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 28fcef05b..2a36d488e 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -546,3 +546,15 @@ def test_update_user_resets_failed_login_count_if_updating_password(client, samp assert resp.status_code == 200 assert user.failed_login_count == 0 + + +def test_update_user_auth_type(admin_request, sample_user): + assert sample_user.auth_type == 'sms_auth' + resp = admin_request.post( + 'user.update_user_attribute', + user_id=sample_user.id, + _data={'auth_type': 'email_auth'}, + ) + + assert resp['data']['id'] == str(sample_user.id) + assert resp['data']['auth_type'] == 'email_auth' From f01c088f2fd79fc8b4795bcff6ae4d71e5def968 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 31 Oct 2017 15:03:12 +0000 Subject: [PATCH 07/11] Update utils with a fix for address formats for letters. --- app/celery/tasks.py | 1 + requirements.txt | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index b5e732df5..c4ac1740a 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -326,6 +326,7 @@ def build_dvla_file(self, job_id): current_app.logger.info("All notifications for job {} are not persisted".format(job_id)) self.retry(queue=QueueNames.RETRY, exc="All notifications for job {} are not persisted".format(job_id)) except Exception as e: + # ? should this retry? current_app.logger.exception("build_dvla_file threw exception") raise e diff --git a/requirements.txt b/requirements.txt index 9d576ab16..094a657c8 100644 --- a/requirements.txt +++ b/requirements.txt @@ -26,6 +26,6 @@ notifications-python-client==4.5.0 awscli>=1.11,<1.12 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@21.5.0#egg=notifications-utils==21.5.0 +git+https://github.com/alphagov/notifications-utils.git@21.5.1#egg=notifications-utils==21.5.1 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3 From c8a210afd1665c5866527d0ae33685e020fb0467 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 31 Oct 2017 15:20:14 +0000 Subject: [PATCH 08/11] Added a command to re-run the build_dvla_file for a given job id. There was an instance where the file failed to create because the address in the personalisation had punctuation which caused the address lines to merge into one. Utils has been updated to fix that problem, this task will allow us to re-run the task so that the notifications can be sent. --- app/commands.py | 11 +++++++++++ application.py | 4 ++-- 2 files changed, 13 insertions(+), 2 deletions(-) diff --git a/app/commands.py b/app/commands.py index a217f480a..f1e125e01 100644 --- a/app/commands.py +++ b/app/commands.py @@ -351,3 +351,14 @@ class PopulateAnnualBilling(Command): db.session.commit() print("Populated annual billing {} for {} services".format(fy, services_result1.rowcount)) + + +class ReRunBuildDvlaFileForJob(Command): + option_list = ( + Option('-j', '--job_id', dest='job_id', help="Enter the job id to rebuild the dvla file for"), + ) + + def run(self, job_id): + from app.celery.tasks import build_dvla_file + from app.config import QueueNames + build_dvla_file.apply_async([job_id], queue=QueueNames.JOBS) diff --git a/application.py b/application.py index fb4f815c7..b606aee81 100644 --- a/application.py +++ b/application.py @@ -23,8 +23,8 @@ manager.add_command('populate_service_sms_sender', commands.PopulateServiceSmsSe manager.add_command('populate_service_letter_contact', commands.PopulateServiceLetterContact) manager.add_command('populate_service_and_service_history_free_sms_fragment_limit', commands.PopulateServiceAndServiceHistoryFreeSmsFragmentLimit) -manager.add_command('populate_annual_billing', - commands.PopulateAnnualBilling) +manager.add_command('populate_annual_billing', commands.PopulateAnnualBilling) +manager.add_command('rerun_build_dvla_file', commands.ReRunBuildDvlaFileForJob) @manager.command From 2e65417dc2d71623e13d4961a95893677a29d743 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 31 Oct 2017 16:51:49 +0000 Subject: [PATCH 09/11] Explicitly target environment on cf-push --- Makefile | 1 + 1 file changed, 1 insertion(+) diff --git a/Makefile b/Makefile index aac64e7c4..732d669a4 100644 --- a/Makefile +++ b/Makefile @@ -304,6 +304,7 @@ cf-rollback: ## Rollbacks the app to the previous release .PHONY: cf-push cf-push: $(if ${CF_APP},,$(error Must specify CF_APP)) + cf target -o ${CF_ORG} -s ${CF_SPACE} cf push ${CF_APP} -f ${CF_MANIFEST_FILE} .PHONY: check-if-migrations-to-run From 830619194edb2c09ec84bbef1d61dd227505305f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 1 Nov 2017 11:01:20 +0000 Subject: [PATCH 10/11] Renamed some tests. Fix some imports. Added test for a function --- app/notifications/validators.py | 8 ++-- .../notification_dao/test_notification_dao.py | 16 ++++--- .../test_process_notification.py | 18 +++++--- tests/app/notifications/test_validators.py | 44 +++++++++---------- .../notifications/test_post_notifications.py | 39 +++++++++++++++- 5 files changed, 86 insertions(+), 39 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 973aafaf5..4f05977ce 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -137,9 +137,9 @@ def validate_template(template_id, personalisation, service, notification_type): def check_service_email_reply_to_id(service_id, reply_to_id, notification_type): - if not (reply_to_id is None): + if reply_to_id: if notification_type != EMAIL_TYPE: - message = 'You sent a email_reply_to_id for a {} notification type'.format(notification_type) + message = 'email_reply_to_id is not a valid option for {} notification'.format(notification_type) raise BadRequestError(message=message) try: dao_get_reply_to_by_id(service_id, reply_to_id) @@ -150,9 +150,9 @@ def check_service_email_reply_to_id(service_id, reply_to_id, notification_type): def check_service_sms_sender_id(service_id, sms_sender_id, notification_type): - if not (sms_sender_id is None): + if sms_sender_id: if notification_type != SMS_TYPE: - message = 'You sent a sms_sender_id for a {} notification type'.format(notification_type) + message = 'sms_sender_id is not a valid option for {} notification'.format(notification_type) raise BadRequestError(message=message) try: dao_get_service_sms_senders_by_id(service_id, sms_sender_id) diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index cfbc2dd77..eb8bac3a5 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -12,9 +12,11 @@ from app.models import ( Notification, NotificationEmailReplyTo, NotificationHistory, + NotificationSmsSender, NotificationStatistics, ScheduledNotification, EMAIL_TYPE, + SMS_TYPE, NOTIFICATION_STATUS_TYPES, NOTIFICATION_STATUS_TYPES_FAILED, NOTIFICATION_SENT, @@ -22,16 +24,20 @@ from app.models import ( KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST, - JOB_STATUS_IN_PROGRESS, NotificationSmsSender, SMS_TYPE) + JOB_STATUS_IN_PROGRESS +) from app.dao.notifications_dao import ( dao_create_notification, dao_create_notification_email_reply_to_mapping, + dao_create_notification_sms_sender_mapping, dao_created_scheduled_notification, dao_delete_notifications_and_history_by_id, + dao_get_last_notification_added_for_job_id, dao_get_last_template_usage, dao_get_notification_email_reply_for_notification, dao_get_notifications_by_to_field, + dao_get_notification_sms_sender_mapping, dao_get_notification_statistics_for_service_and_day, dao_get_potential_notification_statistics_for_day, dao_get_scheduled_notifications, @@ -39,6 +45,7 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, dao_update_notification, dao_update_notifications_for_job_to_sent_to_dvla, + dao_update_notifications_by_reference, delete_notifications_created_more_than_a_week_ago_by_type, get_notification_by_id, get_notification_for_job, @@ -49,11 +56,8 @@ from app.dao.notifications_dao import ( is_delivery_slow_for_provider, set_scheduled_notification_to_processed, update_notification_status_by_id, - update_notification_status_by_reference, - dao_get_last_notification_added_for_job_id, - dao_update_notifications_by_reference, - dao_create_notification_sms_sender_mapping, - dao_get_notification_sms_sender_mapping) + update_notification_status_by_reference +) from app.dao.services_dao import dao_update_service from tests.app.db import ( diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 941ed14f0..ff9521906 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -8,15 +8,23 @@ from freezegun import freeze_time from collections import namedtuple from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_service_id -from app.models import Template, Notification, NotificationHistory, ScheduledNotification, NotificationEmailReplyTo, \ - NotificationSmsSender +from app.models import ( + Notification, + NotificationHistory, + NotificationEmailReplyTo, + NotificationSmsSender, + ScheduledNotification, + Template +) from app.notifications.process_notifications import ( create_content_for_notification, persist_notification, - send_notification_to_queue, - simulated_recipient, + persist_email_reply_to_id_for_notification, persist_scheduled_notification, - persist_email_reply_to_id_for_notification, persist_sms_sender_id_for_notification) + persist_sms_sender_id_for_notification, + send_notification_to_queue, + simulated_recipient +) from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address from app.utils import cache_key_for_service_template_counter from app.v2.errors import BadRequestError diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index d9c643620..ede6d940d 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -311,9 +311,9 @@ def test_should_not_rate_limit_if_limiting_is_disabled( @pytest.mark.parametrize('key_type', ['test', 'normal']) def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_int_sms( - key_type, - notify_db, - notify_db_session, + key_type, + notify_db, + notify_db_session, ): service = create_service(notify_db, notify_db_session, permissions=[SMS_TYPE]) with pytest.raises(BadRequestError) as e: @@ -336,12 +336,17 @@ def test_check_service_email_reply_to_id_where_reply_to_id_is_none(notification_ assert check_service_email_reply_to_id(None, None, notification_type) is None +def test_check_service_email_reply_to_where_email_reply_to_is_found(sample_service): + reply_to_address = create_reply_to_email(sample_service, "test@test.com") + assert check_service_email_reply_to_id(sample_service.id, reply_to_address.id, EMAIL_TYPE) is None + + def test_check_service_email_reply_to_id_where_service_id_is_not_found(sample_service, fake_uuid): reply_to_address = create_reply_to_email(sample_service, "test@test.com") with pytest.raises(BadRequestError) as e: check_service_email_reply_to_id(fake_uuid, reply_to_address.id, EMAIL_TYPE) assert e.value.status_code == 400 - assert e.value.message == 'email_reply_to_id {} does not exist in database for service id {}'\ + assert e.value.message == 'email_reply_to_id {} does not exist in database for service id {}' \ .format(reply_to_address.id, fake_uuid) @@ -349,30 +354,25 @@ def test_check_service_email_reply_to_id_where_reply_to_id_is_not_found(sample_s with pytest.raises(BadRequestError) as e: check_service_email_reply_to_id(sample_service.id, fake_uuid, EMAIL_TYPE) assert e.value.status_code == 400 - assert e.value.message == 'email_reply_to_id {} does not exist in database for service id {}'\ + assert e.value.message == 'email_reply_to_id {} does not exist in database for service id {}' \ .format(fake_uuid, sample_service.id) -def test_check_service_sms_sender_id_where_sms_sender_id_is_found(sample_service): - sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') - assert check_service_sms_sender_id(sample_service.id, sms_sender.id, SMS_TYPE) is None - - -@pytest.mark.parametrize('notification_type', ['email', 'letter']) -def test_check_service_sms_sender_id_when_channel_type_is_wrong(sample_service, notification_type): - sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') +@pytest.mark.parametrize('notification_type', ['sms', 'letter']) +def test_check_service_email_reply_to_id_when_channel_type_is_wrong(sample_service, notification_type): + reply_to_address = create_reply_to_email(sample_service, "test@test.com") with pytest.raises(BadRequestError) as e: - check_service_sms_sender_id(sample_service.id, sms_sender.id, notification_type) + check_service_email_reply_to_id(sample_service.id, reply_to_address.id, notification_type) assert e.value.status_code == 400 - assert e.value.message == 'You sent a sms_sender_id for a {} notification type'.format(notification_type) + assert e.value.message == 'email_reply_to_id is not a valid option for {} notification'.format(notification_type) @pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) -def test_check_service_sms_sender_id_where_reply_to_id_is_valid(notification_type): +def test_check_service_sms_sender_id_where_sms_sender_id_is_none(notification_type): assert check_service_sms_sender_id(None, None, notification_type) is None -def test_check_service_sms_sender_id_where_reply_to_id_is_found(sample_service): +def test_check_service_sms_sender_id_where_sms_sender_id_is_found(sample_service): sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') assert check_service_sms_sender_id(sample_service.id, sms_sender.id, SMS_TYPE) is None @@ -382,22 +382,22 @@ def test_check_service_sms_sender_id_where_service_id_is_not_found(sample_servic with pytest.raises(BadRequestError) as e: check_service_sms_sender_id(fake_uuid, sms_sender.id, SMS_TYPE) assert e.value.status_code == 400 - assert e.value.message == 'sms_sender_id {} does not exist in database for service id {}'\ + assert e.value.message == 'sms_sender_id {} does not exist in database for service id {}' \ .format(sms_sender.id, fake_uuid) -def test_check_service_sms_sender_id_where_reply_to_id_is_not_found(sample_service, fake_uuid): +def test_check_service_sms_sender_id_where_sms_sender_is_not_found(sample_service, fake_uuid): with pytest.raises(BadRequestError) as e: check_service_sms_sender_id(sample_service.id, fake_uuid, SMS_TYPE) assert e.value.status_code == 400 - assert e.value.message == 'sms_sender_id {} does not exist in database for service id {}'\ + assert e.value.message == 'sms_sender_id {} does not exist in database for service id {}' \ .format(fake_uuid, sample_service.id) @pytest.mark.parametrize('notification_type', ['email', 'letter']) -def test_check_service_email_reply_to_id_when_channel_type_is_wrong(sample_service, notification_type): +def test_check_service_sms_sender_id_when_channel_type_is_wrong(sample_service, notification_type): sms_sender = create_service_sms_sender(service=sample_service, sms_sender='123456') with pytest.raises(BadRequestError) as e: check_service_sms_sender_id(sample_service.id, sms_sender.id, notification_type) assert e.value.status_code == 400 - assert e.value.message == 'You sent a sms_sender_id for a {} notification type'.format(notification_type) + assert e.value.message == 'sms_sender_id is not a valid option for {} notification'.format(notification_type) diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index a76702706..d4055df3f 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -16,6 +16,7 @@ from app.models import Notification from app.schema_validation import validate from app.v2.errors import RateLimitError from app.v2.notifications.notification_schemas import post_sms_response, post_email_response +from app.v2.notifications.post_notifications import persist_sender_to_notification_mapping from tests import create_authorization_header from tests.app.conftest import ( sample_template as create_sample_template, @@ -28,7 +29,7 @@ from tests.app.db import ( create_service, create_template, create_reply_to_email, - create_service_sms_sender) + create_service_sms_sender, create_notification) @pytest.mark.parametrize("reference", [None, "reference_from_client"]) @@ -561,7 +562,7 @@ def test_post_notification_with_wrong_type_of_sender( headers=[('Content-Type', 'application/json'), auth_header]) assert response.status_code == 400 resp_json = json.loads(response.get_data(as_text=True)) - assert 'You sent a {} for a {} notification type'.\ + assert '{} is not a valid option for {} notification'.\ format(form_label, notification_type) in resp_json['errors'][0]['message'] assert 'BadRequestError' in resp_json['errors'][0]['error'] @@ -630,3 +631,37 @@ def test_post_email_notification_with_valid_reply_to_id_returns_201(client, samp assert email_reply_to.notification_id == notification.id assert email_reply_to.service_email_reply_to_id == reply_to_email.id + + +def test_persist_sender_to_notification_mapping_for_email(notify_db_session): + service = create_service() + template = create_template(service=service, template_type=EMAIL_TYPE) + sender = create_reply_to_email(service=service, email_address='reply@test.com', is_default=False) + form = { + "email_address": "recipient@test.com", + "template_id": str(template.id), + 'email_reply_to_id': str(sender.id) + } + notification = create_notification(template=template) + persist_sender_to_notification_mapping(form=form, notification=notification) + notification_to_email_reply_to = NotificationEmailReplyTo.query.all() + assert len(notification_to_email_reply_to) == 1 + assert notification_to_email_reply_to[0].notification_id == notification.id + assert notification_to_email_reply_to[0].service_email_reply_to_id == sender.id + + +def test_persist_sender_to_notification_mapping_for_sms(notify_db_session): + service = create_service() + template = create_template(service=service, template_type=SMS_TYPE) + sender = create_service_sms_sender(service=service, sms_sender='12345', is_default=False) + form = { + 'phone_number': '+447700900855', + 'template_id': str(template.id), + 'sms_sender_id': str(sender.id) + } + notification = create_notification(template=template) + persist_sender_to_notification_mapping(form=form, notification=notification) + notification_to_sms_sender = NotificationSmsSender.query.all() + assert len(notification_to_sms_sender) == 1 + assert notification_to_sms_sender[0].notification_id == notification.id + assert notification_to_sms_sender[0].service_sms_sender_id == sender.id From f5e79302cdca7bd0332be988c6a74cb279c65698 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 1 Nov 2017 15:02:50 +0000 Subject: [PATCH 11/11] Remove NotificationStatistics NotificationStatistics was added as a spike but didn't work out as expected. This is finally removing all that unused code. I'll drop the table in the next PR --- app/dao/notifications_dao.py | 61 +---- app/dao/provider_statistics_dao.py | 2 +- app/dao/services_dao.py | 2 - app/models.py | 19 -- app/notifications/rest.py | 14 +- app/schemas.py | 13 - app/service/utils.py | 3 +- tests/app/conftest.py | 30 --- .../notification_dao/test_notification_dao.py | 28 --- tests/app/dao/test_services_dao.py | 2 - .../app/notifications/rest/test_callbacks.py | 5 - .../rest/test_notification_statistics.py | 225 ------------------ 12 files changed, 5 insertions(+), 399 deletions(-) delete mode 100644 tests/app/notifications/rest/test_notification_statistics.py diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 59263cd9f..c28b2a686 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -14,7 +14,7 @@ from notifications_utils.recipients import ( InvalidEmailError, ) from werkzeug.datastructures import MultiDict -from sqlalchemy import (desc, func, or_, and_, asc) +from sqlalchemy import (desc, func, or_, asc) from sqlalchemy.orm import joinedload from sqlalchemy.sql.expression import case from sqlalchemy.sql import functions @@ -22,13 +22,10 @@ from notifications_utils.international_billing_rates import INTERNATIONAL_BILLIN from app import db, create_uuid from app.dao import days_ago -from app.dao.date_util import get_financial_year from app.models import ( - Service, Notification, NotificationEmailReplyTo, NotificationHistory, - NotificationStatistics, ScheduledNotification, ServiceEmailReplyTo, Template, @@ -53,62 +50,6 @@ from app.dao.dao_utils import transactional from app.statsd_decorators import statsd -def dao_get_notification_statistics_for_service_and_day(service_id, day): - # only used by stat-updating code in tasks.py - return NotificationStatistics.query.filter_by( - service_id=service_id, - day=day - ).order_by(desc(NotificationStatistics.day)).first() - - -@statsd(namespace="dao") -def dao_get_potential_notification_statistics_for_day(day): - all_services = db.session.query( - Service.id, - NotificationStatistics - ).outerjoin( - NotificationStatistics, - and_( - Service.id == NotificationStatistics.service_id, - or_( - NotificationStatistics.day == day, - NotificationStatistics.day == None # noqa - ) - ) - ).order_by( - asc(Service.created_at) - ) - - notification_statistics = [] - for service_notification_stats_pair in all_services: - if service_notification_stats_pair.NotificationStatistics: - notification_statistics.append( - service_notification_stats_pair.NotificationStatistics - ) - else: - notification_statistics.append( - create_notification_statistics_dict( - service_notification_stats_pair, - day - ) - ) - return notification_statistics - - -def create_notification_statistics_dict(service_id, day): - return { - 'id': None, - 'emails_requested': 0, - 'emails_delivered': 0, - 'emails_failed': 0, - 'sms_requested': 0, - 'sms_delivered': 0, - 'sms_failed': 0, - 'day': day.isoformat(), - 'service': service_id - } - - @statsd(namespace="dao") def dao_get_template_usage(service_id, limit_days=None): query_filter = [] diff --git a/app/dao/provider_statistics_dao.py b/app/dao/provider_statistics_dao.py index 761e79de6..705d08fec 100644 --- a/app/dao/provider_statistics_dao.py +++ b/app/dao/provider_statistics_dao.py @@ -1,6 +1,7 @@ from sqlalchemy import func from app import db +from app.dao.date_util import get_financial_year from app.models import ( NotificationHistory, SMS_TYPE, @@ -8,7 +9,6 @@ from app.models import ( NOTIFICATION_STATUS_TYPES_BILLABLE, KEY_TYPE_TEST ) -from app.dao.notifications_dao import get_financial_year def get_fragment_count(service_id, year=None): diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index b22c07e58..93905e7e6 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -13,7 +13,6 @@ from app.dao.dao_utils import ( from app.dao.date_util import get_financial_year from app.dao.service_sms_sender_dao import insert_service_sms_sender from app.models import ( - NotificationStatistics, ProviderStatistics, VerifyCode, ApiKey, @@ -232,7 +231,6 @@ def delete_service_and_all_associated_db_objects(service): _delete_commit(TemplateRedacted.query.filter(TemplateRedacted.template_id.in_(subq))) _delete_commit(ServiceSmsSender.query.filter_by(service=service)) - _delete_commit(NotificationStatistics.query.filter_by(service=service)) _delete_commit(ProviderStatistics.query.filter_by(service=service)) _delete_commit(InvitedUser.query.filter_by(service=service)) _delete_commit(Permission.query.filter_by(service=service)) diff --git a/app/models.py b/app/models.py index ff8cb1acf..76bf0edfd 100644 --- a/app/models.py +++ b/app/models.py @@ -516,25 +516,6 @@ class KeyTypes(db.Model): name = db.Column(db.String(255), primary_key=True) -class NotificationStatistics(db.Model): - __tablename__ = 'notification_statistics' - - id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) - day = db.Column(db.Date, index=True, nullable=False, unique=False, default=datetime.date.today) - service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, nullable=False) - service = db.relationship('Service', backref=db.backref('service_notification_stats', lazy='dynamic')) - emails_requested = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) - emails_delivered = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) - emails_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) - sms_requested = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) - sms_delivered = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) - sms_failed = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=0) - - __table_args__ = ( - UniqueConstraint('service_id', 'day', name='uix_service_to_day'), - ) - - class TemplateProcessTypes(db.Model): __tablename__ = 'template_process_type' name = db.Column(db.String(255), primary_key=True) diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 85ee022a7..5a640b0e7 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -34,9 +34,7 @@ from app.schemas import ( email_notification_schema, sms_template_notification_schema, notification_with_personalisation_schema, - notifications_filter_schema, - notifications_statistics_schema, - day_schema + notifications_filter_schema ) from app.service.utils import service_allowed_to_send_to from app.utils import pagination_links, get_template_instance, get_public_notify_type_text @@ -86,16 +84,6 @@ def get_all_notifications(): ), 200 -@notifications.route('/notifications/statistics') -def get_notification_statistics_for_day(): - data = day_schema.load(request.args).data - statistics = notifications_dao.dao_get_potential_notification_statistics_for_day( - day=data['day'] - ) - data, errors = notifications_statistics_schema.dump(statistics, many=True) - return jsonify(data=data), 200 - - @notifications.route('/notifications/', methods=['POST']) def send_notification(notification_type): diff --git a/app/schemas.py b/app/schemas.py index b6ffe738c..a143e3bbb 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -325,18 +325,6 @@ class TemplateHistorySchema(BaseSchema): model = models.TemplateHistory -class NotificationsStatisticsSchema(BaseSchema): - class Meta: - model = models.NotificationStatistics - strict = True - - @pre_dump - def handle_date_str(self, in_data): - if isinstance(in_data, dict) and 'day' in in_data: - in_data['day'] = datetime.strptime(in_data['day'], '%Y-%m-%d').date() - return in_data - - class ApiKeySchema(BaseSchema): created_by = field_for(models.ApiKey, 'created_by', required=True) @@ -673,7 +661,6 @@ notification_with_personalisation_schema = NotificationWithPersonalisationSchema invited_user_schema = InvitedUserSchema() permission_schema = PermissionSchema() email_data_request_schema = EmailDataSchema() -notifications_statistics_schema = NotificationsStatisticsSchema() notifications_filter_schema = NotificationsFilterSchema() service_history_schema = ServiceHistorySchema() api_key_history_schema = ApiKeyHistorySchema() diff --git a/app/service/utils.py b/app/service/utils.py index 475d420e2..687f8939e 100644 --- a/app/service/utils.py +++ b/app/service/utils.py @@ -1,12 +1,13 @@ import itertools +from app.dao.date_util import get_financial_year from app.models import ( ServiceWhitelist, MOBILE_TYPE, EMAIL_TYPE, KEY_TYPE_TEST, KEY_TYPE_TEAM, KEY_TYPE_NORMAL) from notifications_utils.recipients import allowed_to_send_to -from app.dao.notifications_dao import get_financial_year + from datetime import datetime diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 7f3bd3008..e03b2b7dd 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -24,7 +24,6 @@ from app.models import ( ProviderDetails, ProviderDetailsHistory, ProviderRates, - NotificationStatistics, ScheduledNotification, ServiceWhitelist, KEY_TYPE_NORMAL, KEY_TYPE_TEST, KEY_TYPE_TEAM, @@ -840,35 +839,6 @@ def sample_provider_statistics(notify_db, return stats -@pytest.fixture(scope='function') -def sample_notification_statistics(notify_db, - notify_db_session, - service=None, - day=None, - emails_requested=2, - emails_delivered=1, - emails_failed=1, - sms_requested=2, - sms_delivered=1, - sms_failed=1): - if service is None: - service = sample_service(notify_db, notify_db_session) - if day is None: - day = date.today() - stats = NotificationStatistics( - service=service, - day=day, - emails_requested=emails_requested, - emails_delivered=emails_delivered, - emails_failed=emails_failed, - sms_requested=sms_requested, - sms_delivered=sms_delivered, - sms_failed=sms_failed) - notify_db.session.add(stats) - notify_db.session.commit() - return stats - - @pytest.fixture(scope='function') def mock_firetext_client(mocker, statsd_client=None): client = FiretextClient() diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index eb8bac3a5..cec15552d 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -13,7 +13,6 @@ from app.models import ( NotificationEmailReplyTo, NotificationHistory, NotificationSmsSender, - NotificationStatistics, ScheduledNotification, EMAIL_TYPE, SMS_TYPE, @@ -38,8 +37,6 @@ from app.dao.notifications_dao import ( dao_get_notification_email_reply_for_notification, dao_get_notifications_by_to_field, dao_get_notification_sms_sender_mapping, - dao_get_notification_statistics_for_service_and_day, - dao_get_potential_notification_statistics_for_day, dao_get_scheduled_notifications, dao_get_template_usage, dao_timeout_notifications, @@ -79,7 +76,6 @@ from tests.app.conftest import ( def test_should_have_decorated_notifications_dao_functions(): assert dao_get_last_template_usage.__wrapped__.__name__ == 'dao_get_last_template_usage' # noqa assert dao_get_template_usage.__wrapped__.__name__ == 'dao_get_template_usage' # noqa - assert dao_get_potential_notification_statistics_for_day.__wrapped__.__name__ == 'dao_get_potential_notification_statistics_for_day' # noqa assert dao_create_notification.__wrapped__.__name__ == 'dao_create_notification' # noqa assert update_notification_status_by_id.__wrapped__.__name__ == 'update_notification_status_by_id' # noqa assert dao_update_notification.__wrapped__.__name__ == 'dao_update_notification' # noqa @@ -581,31 +577,10 @@ def test_should_return_zero_count_if_no_notification_with_reference(): assert not update_notification_status_by_reference('something', 'delivered') -def test_should_return_none_if_no_statistics_for_a_service_for_a_day(sample_template, mmg_provider): - data = _notification_json(sample_template) - - notification = Notification(**data) - dao_create_notification(notification) - assert not dao_get_notification_statistics_for_service_and_day( - sample_template.service.id, (datetime.utcnow() - timedelta(days=1)).date()) - - -def test_should_be_able_to_get_all_statistics_for_a_service(sample_template, mmg_provider): - data = _notification_json(sample_template) - - notification_1 = Notification(**data) - notification_2 = Notification(**data) - notification_3 = Notification(**data) - dao_create_notification(notification_1) - dao_create_notification(notification_2) - dao_create_notification(notification_3) - - def test_create_notification_creates_notification_with_personalisation(notify_db, notify_db_session, sample_template_with_placeholders, sample_job, mmg_provider): assert Notification.query.count() == 0 - assert NotificationStatistics.query.count() == 0 data = sample_notification(notify_db=notify_db, notify_db_session=notify_db_session, template=sample_template_with_placeholders, @@ -628,7 +603,6 @@ def test_create_notification_creates_notification_with_personalisation(notify_db def test_save_notification_creates_sms(sample_template, sample_job, mmg_provider): assert Notification.query.count() == 0 - assert NotificationStatistics.query.count() == 0 data = _notification_json(sample_template, job_id=sample_job.id) @@ -649,7 +623,6 @@ def test_save_notification_creates_sms(sample_template, sample_job, mmg_provider def test_save_notification_and_create_email(sample_email_template, sample_job): assert Notification.query.count() == 0 - assert NotificationStatistics.query.count() == 0 data = _notification_json(sample_email_template, job_id=sample_job.id) @@ -773,7 +746,6 @@ def test_not_save_notification_and_not_create_stats_on_commit_error(sample_templ assert Notification.query.count() == 0 assert Job.query.get(sample_job.id).notifications_sent == 0 - assert NotificationStatistics.query.count() == 0 def test_save_notification_and_increment_job(sample_template, sample_job, mmg_provider): diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 70bc79eef..76c06d088 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -35,7 +35,6 @@ from app.dao.services_dao import ( from app.dao.service_permissions_dao import dao_add_service_permission, dao_remove_service_permission from app.dao.users_dao import save_model_user from app.models import ( - NotificationStatistics, ProviderStatistics, VerifyCode, ApiKey, @@ -447,7 +446,6 @@ def test_delete_service_and_associated_objects(notify_db, assert ServicePermission.query.count() == 3 delete_service_and_all_associated_db_objects(sample_service) - assert NotificationStatistics.query.count() == 0 assert ProviderStatistics.query.count() == 0 assert VerifyCode.query.count() == 0 assert ApiKey.query.count() == 0 diff --git a/tests/app/notifications/rest/test_callbacks.py b/tests/app/notifications/rest/test_callbacks.py index 03fede0fa..ff2cb417e 100644 --- a/tests/app/notifications/rest/test_callbacks.py +++ b/tests/app/notifications/rest/test_callbacks.py @@ -9,7 +9,6 @@ import app.celery.tasks from app.dao.notifications_dao import ( get_notification_by_id ) -from app.models import NotificationStatistics from tests.app.notifications.test_notifications_ses_callback import ses_confirmation_callback from tests.app.conftest import sample_notification as create_sample_notification @@ -431,10 +430,6 @@ def test_firetext_callback_should_record_statsd(client, notify_db, notify_db_ses app.statsd_client.incr.assert_any_call("callback.firetext.delivered") -def get_notification_stats(service_id): - return NotificationStatistics.query.filter_by(service_id=service_id).one() - - def _sample_sns_s3_callback(): return json.dumps({ "SigningCertURL": "foo.pem", diff --git a/tests/app/notifications/rest/test_notification_statistics.py b/tests/app/notifications/rest/test_notification_statistics.py deleted file mode 100644 index 6e9de437a..000000000 --- a/tests/app/notifications/rest/test_notification_statistics.py +++ /dev/null @@ -1,225 +0,0 @@ -from datetime import date, timedelta - -from flask import json -from freezegun import freeze_time -from datetime import datetime - -from tests import create_authorization_header -from tests.app.conftest import ( - sample_notification_statistics as create_sample_notification_statistics, - sample_service as create_sample_service -) - - -def test_get_notification_statistics(notify_api, sample_notification_statistics): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header( - service_id=sample_notification_statistics.service_id - ) - - response = client.get( - '/notifications/statistics?day={}'.format(date.today().isoformat()), - headers=[auth_header] - ) - - notifications = json.loads(response.get_data(as_text=True)) - - assert len(notifications['data']) == 1 - stats = notifications['data'][0] - assert stats['emails_requested'] == 2 - assert stats['emails_delivered'] == 1 - assert stats['emails_failed'] == 1 - assert stats['sms_requested'] == 2 - assert stats['sms_delivered'] == 1 - assert stats['sms_failed'] == 1 - assert stats['service'] == str(sample_notification_statistics.service_id) - assert response.status_code == 200 - - -@freeze_time('1955-11-05T12:00:00') -def test_get_notification_statistics_only_returns_today(notify_api, notify_db, notify_db_session, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - yesterdays_notification_statistics = create_sample_notification_statistics( - notify_db, - notify_db_session, - service=sample_service, - day=date.today() - timedelta(days=1) - ) - todays_notification_statistics = create_sample_notification_statistics( - notify_db, - notify_db_session, - service=sample_service, - day=date.today() - ) - tomorrows_notification_statistics = create_sample_notification_statistics( - notify_db, - notify_db_session, - service=sample_service, - day=date.today() + timedelta(days=1) - ) - - auth_header = create_authorization_header( - service_id=sample_service.id - ) - - response = client.get( - '/notifications/statistics?day={}'.format(date.today().isoformat()), - headers=[auth_header] - ) - - notifications = json.loads(response.get_data(as_text=True)) - - assert len(notifications['data']) == 1 - assert notifications['data'][0]['day'] == date.today().isoformat() - assert response.status_code == 200 - - -def test_get_notification_statistics_fails_if_no_date(notify_api, sample_notification_statistics): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header( - service_id=sample_notification_statistics.service_id - ) - - response = client.get( - '/notifications/statistics', - headers=[auth_header] - ) - - resp = json.loads(response.get_data(as_text=True)) - assert resp['result'] == 'error' - assert resp['message'] == {'day': ['Missing data for required field.']} - assert response.status_code == 400 - - -def test_get_notification_statistics_fails_if_invalid_date(notify_api, sample_notification_statistics): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header( - service_id=sample_notification_statistics.service_id - ) - - response = client.get( - '/notifications/statistics?day=2016-99-99', - headers=[auth_header] - ) - - resp = json.loads(response.get_data(as_text=True)) - assert resp['result'] == 'error' - assert resp['message'] == {'day': ['Not a valid date.']} - assert response.status_code == 400 - - -def test_get_notification_statistics_returns_zeros_if_not_in_db(notify_api, sample_service): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - auth_header = create_authorization_header( - service_id=sample_service.id - ) - - response = client.get( - '/notifications/statistics?day={}'.format(date.today().isoformat()), - headers=[auth_header] - ) - - notifications = json.loads(response.get_data(as_text=True)) - - assert len(notifications['data']) == 1 - stats = notifications['data'][0] - assert stats['emails_requested'] == 0 - assert stats['emails_delivered'] == 0 - assert stats['emails_failed'] == 0 - assert stats['sms_requested'] == 0 - assert stats['sms_delivered'] == 0 - assert stats['sms_failed'] == 0 - assert stats['service'] == str(sample_service.id) - assert response.status_code == 200 - - -def test_get_notification_statistics_returns_both_existing_stats_and_generated_zeros( - notify_api, - notify_db, - notify_db_session -): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - service_with_stats = create_sample_service( - notify_db, - notify_db_session, - service_name='service_with_stats', - email_from='service_with_stats' - ) - service_without_stats = create_sample_service( - notify_db, - notify_db_session, - service_name='service_without_stats', - email_from='service_without_stats' - ) - notification_statistics = create_sample_notification_statistics( - notify_db, - notify_db_session, - service=service_with_stats, - day=date.today() - ) - auth_header = create_authorization_header( - service_id=service_with_stats.id - ) - - response = client.get( - '/notifications/statistics?day={}'.format(date.today().isoformat()), - headers=[auth_header] - ) - - notifications = json.loads(response.get_data(as_text=True)) - - assert len(notifications['data']) == 2 - retrieved_stats = notifications['data'][0] - generated_stats = notifications['data'][1] - - assert retrieved_stats['emails_requested'] == 2 - assert retrieved_stats['emails_delivered'] == 1 - assert retrieved_stats['emails_failed'] == 1 - assert retrieved_stats['sms_requested'] == 2 - assert retrieved_stats['sms_delivered'] == 1 - assert retrieved_stats['sms_failed'] == 1 - assert retrieved_stats['service'] == str(service_with_stats.id) - - assert generated_stats['emails_requested'] == 0 - assert generated_stats['emails_delivered'] == 0 - assert generated_stats['emails_failed'] == 0 - assert generated_stats['sms_requested'] == 0 - assert generated_stats['sms_delivered'] == 0 - assert generated_stats['sms_failed'] == 0 - assert generated_stats['service'] == str(service_without_stats.id) - - assert response.status_code == 200 - - -def test_get_notification_statistics_returns_zeros_when_only_stats_for_different_date( - notify_api, - sample_notification_statistics -): - with notify_api.test_request_context(): - with notify_api.test_client() as client: - with freeze_time('1985-10-26T00:06:00'): - auth_header = create_authorization_header( - service_id=sample_notification_statistics.service_id - ) - response = client.get( - '/notifications/statistics?day={}'.format(datetime.utcnow().isoformat()), - headers=[auth_header] - ) - - notifications = json.loads(response.get_data(as_text=True)) - - assert response.status_code == 200 - assert len(notifications['data']) == 1 - assert notifications['data'][0]['emails_requested'] == 0 - assert notifications['data'][0]['emails_delivered'] == 0 - assert notifications['data'][0]['emails_failed'] == 0 - assert notifications['data'][0]['sms_requested'] == 0 - assert notifications['data'][0]['sms_delivered'] == 0 - assert notifications['data'][0]['sms_failed'] == 0 - assert notifications['data'][0]['service'] == str(sample_notification_statistics.service_id)