From aad78cedf76951c82770c1364c2ccc7ddf488676 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 25 Feb 2019 17:10:59 +0000 Subject: [PATCH 01/12] Update flake8 from 3.7.6 to 3.7.7 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index fa6a0089d..2f64621bc 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,5 +1,5 @@ -r requirements.txt -flake8==3.7.6 +flake8==3.7.7 moto==1.3.7 pytest==3.10.1 # pyup: <4 pytest-env==0.6.2 From 38f0ea6cca5223dfcce3800bea5e22f1eead3164 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Feb 2019 17:57:35 +0000 Subject: [PATCH 02/12] remove functions to not talk about 7 days remind us that data retention is flexible --- app/celery/nightly_tasks.py | 20 ++++++------- app/dao/notifications_dao.py | 2 +- tests/app/celery/test_nightly_tasks.py | 28 +++++++++---------- .../notification_dao/test_notification_dao.py | 4 +-- ...t_notification_dao_delete_notifications.py | 16 +++++------ 5 files changed, 35 insertions(+), 35 deletions(-) diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index e452befd1..56593ab1d 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -16,14 +16,14 @@ from app.celery.service_callback_tasks import ( create_delivery_status_callback_data, ) from app.config import QueueNames -from app.dao.inbound_sms_dao import delete_inbound_sms_created_more_than_a_week_ago +from app.dao.inbound_sms_dao import delete_inbound_sms_older_than_retention from app.dao.jobs_dao import ( dao_get_jobs_older_than_data_retention, dao_archive_job ) from app.dao.notifications_dao import ( dao_timeout_notifications, - delete_notifications_created_more_than_a_week_ago_by_type, + delete_notifications_older_than_retention_by_type, ) from app.dao.service_callback_api_dao import get_service_delivery_status_callback_api_for_service from app.exceptions import NotificationTechnicalFailureException @@ -64,10 +64,10 @@ def _remove_csv_files(job_types): @notify_celery.task(name="delete-sms-notifications") @cronitor("delete-sms-notifications") @statsd(namespace="tasks") -def delete_sms_notifications_older_than_seven_days(): +def delete_sms_notifications_older_than_retention(): try: start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('sms') + deleted = delete_notifications_older_than_retention_by_type('sms') current_app.logger.info( "Delete {} job started {} finished {} deleted {} sms notifications".format( 'sms', @@ -84,10 +84,10 @@ def delete_sms_notifications_older_than_seven_days(): @notify_celery.task(name="delete-email-notifications") @cronitor("delete-email-notifications") @statsd(namespace="tasks") -def delete_email_notifications_older_than_seven_days(): +def delete_email_notifications_older_than_retention(): try: start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('email') + deleted = delete_notifications_older_than_retention_by_type('email') current_app.logger.info( "Delete {} job started {} finished {} deleted {} email notifications".format( 'email', @@ -104,10 +104,10 @@ def delete_email_notifications_older_than_seven_days(): @notify_celery.task(name="delete-letter-notifications") @cronitor("delete-letter-notifications") @statsd(namespace="tasks") -def delete_letter_notifications_older_than_seven_days(): +def delete_letter_notifications_older_than_retention(): try: start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('letter') + deleted = delete_notifications_older_than_retention_by_type('letter') current_app.logger.info( "Delete {} job started {} finished {} deleted {} letter notifications".format( 'letter', @@ -190,10 +190,10 @@ def send_total_sent_notifications_to_performance_platform(day): @notify_celery.task(name="delete-inbound-sms") @cronitor("delete-inbound-sms") @statsd(namespace="tasks") -def delete_inbound_sms_older_than_seven_days(): +def delete_inbound_sms(): try: start = datetime.utcnow() - deleted = delete_inbound_sms_created_more_than_a_week_ago() + deleted = delete_inbound_sms_older_than_retention() current_app.logger.info( "Delete inbound sms job started {} finished {} deleted {} inbound sms notifications".format( start, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 722f03165..4b9b6c7e9 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -293,7 +293,7 @@ def _filter_query(query, filter_dict=None): @statsd(namespace="dao") -def delete_notifications_created_more_than_a_week_ago_by_type(notification_type, qry_limit=10000): +def delete_notifications_older_than_retention_by_type(notification_type, qry_limit=10000): current_app.logger.info( 'Deleting {} notifications for services with flexible data retention'.format(notification_type)) diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index 93e047448..179aa95ff 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -11,10 +11,10 @@ from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient from app.celery import nightly_tasks from app.celery.nightly_tasks import ( delete_dvla_response_files_older_than_seven_days, - delete_email_notifications_older_than_seven_days, - delete_inbound_sms_older_than_seven_days, - delete_letter_notifications_older_than_seven_days, - delete_sms_notifications_older_than_seven_days, + delete_email_notifications_older_than_retention, + delete_inbound_sms, + delete_letter_notifications_older_than_retention, + delete_sms_notifications_older_than_retention, raise_alert_if_letter_notifications_still_sending, remove_letter_csv_files, remove_sms_email_csv_files, @@ -157,21 +157,21 @@ def test_remove_csv_files_filters_by_type(mocker, sample_service): def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker): - mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_sms_notifications_older_than_seven_days() + mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + delete_sms_notifications_older_than_retention() mocked.assert_called_once_with('sms') def test_should_call_delete_email_notifications_more_than_week_in_task(notify_api, mocker): mocked_notifications = mocker.patch( - 'app.celery.nightly_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_email_notifications_older_than_seven_days() + 'app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + delete_email_notifications_older_than_retention() mocked_notifications.assert_called_once_with('email') def test_should_call_delete_letter_notifications_more_than_week_in_task(notify_api, mocker): - mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_letter_notifications_older_than_seven_days() + mocked = mocker.patch('app.celery.nightly_tasks.delete_notifications_older_than_retention_by_type') + delete_letter_notifications_older_than_retention() mocked.assert_called_once_with('letter') @@ -291,10 +291,10 @@ def test_send_total_sent_notifications_to_performance_platform_calls_with_correc ]) -def test_should_call_delete_inbound_sms_older_than_seven_days(notify_api, mocker): - mocker.patch('app.celery.nightly_tasks.delete_inbound_sms_created_more_than_a_week_ago') - delete_inbound_sms_older_than_seven_days() - assert nightly_tasks.delete_inbound_sms_created_more_than_a_week_ago.call_count == 1 +def test_should_call_delete_inbound_sms(notify_api, mocker): + mocker.patch('app.celery.nightly_tasks.delete_inbound_sms_older_than_retention') + delete_inbound_sms() + assert nightly_tasks.delete_inbound_sms_older_than_retention.call_count == 1 @freeze_time('2017-01-01 10:00:00') diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 0776a45fe..91df8ec72 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -19,7 +19,7 @@ from app.dao.notifications_dao import ( dao_timeout_notifications, dao_update_notification, dao_update_notifications_by_reference, - delete_notifications_created_more_than_a_week_ago_by_type, + delete_notifications_older_than_retention_by_type, get_notification_by_id, get_notification_for_job, get_notification_with_personalisation, @@ -79,7 +79,7 @@ def test_should_have_decorated_notifications_dao_functions(): assert get_notification_with_personalisation.__wrapped__.__name__ == 'get_notification_with_personalisation' # noqa assert get_notifications_for_service.__wrapped__.__name__ == 'get_notifications_for_service' # noqa assert get_notification_by_id.__wrapped__.__name__ == 'get_notification_by_id' # noqa - 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_older_than_retention_by_type.__wrapped__.__name__ == 'delete_notifications_older_than_retention_by_type' # noqa assert dao_delete_notifications_and_history_by_id.__wrapped__.__name__ == 'dao_delete_notifications_and_history_by_id' # noqa diff --git a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py index 4b3d8c9e8..3e913a673 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py +++ b/tests/app/dao/notification_dao/test_notification_dao_delete_notifications.py @@ -6,7 +6,7 @@ from datetime import ( import pytest from flask import current_app from freezegun import freeze_time -from app.dao.notifications_dao import delete_notifications_created_more_than_a_week_ago_by_type +from app.dao.notifications_dao import delete_notifications_older_than_retention_by_type from app.models import Notification, NotificationHistory from tests.app.db import ( create_template, @@ -48,7 +48,7 @@ def test_should_delete_notifications_by_type_after_seven_days( assert len(all_notifications) == 30 # Records from before 3rd should be deleted with freeze_time(delete_run_time): - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) remaining_sms_notifications = Notification.query.filter_by(notification_type='sms').all() remaining_letter_notifications = Notification.query.filter_by(notification_type='letter').all() remaining_email_notifications = Notification.query.filter_by(notification_type='email').all() @@ -77,7 +77,7 @@ def test_should_not_delete_notification_history(sample_service, notification_typ create_notification(template=letter_template, status='permanent-failure') assert Notification.query.count() == 3 assert NotificationHistory.query.count() == 3 - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) assert Notification.query.count() == 2 assert NotificationHistory.query.count() == 3 @@ -109,7 +109,7 @@ def test_delete_notifications_for_days_of_retention(sample_service, notification created_at=datetime.utcnow() - timedelta(days=8)) create_service_data_retention(service_id=sample_service.id, notification_type=notification_type) assert len(Notification.query.all()) == 9 - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) assert len(Notification.query.all()) == 7 assert len(Notification.query.filter_by(notification_type=notification_type).all()) == 1 if notification_type == 'letter': @@ -146,7 +146,7 @@ def test_delete_notifications_keep_data_for_days_of_retention_is_longer(sample_s create_notification(template=default_letter_template, status='temporary-failure', created_at=datetime.utcnow() - timedelta(days=8)) assert len(Notification.query.all()) == 9 - delete_notifications_created_more_than_a_week_ago_by_type(notification_type) + delete_notifications_older_than_retention_by_type(notification_type) assert len(Notification.query.filter_by().all()) == 8 assert len(Notification.query.filter_by(notification_type=notification_type).all()) == 2 if notification_type == 'letter': @@ -171,7 +171,7 @@ def test_delete_notifications_delete_notification_type_for_default_time_if_no_da create_notification(template=letter_template, status='temporary-failure', created_at=datetime.utcnow() - timedelta(days=14)) assert len(Notification.query.all()) == 6 - delete_notifications_created_more_than_a_week_ago_by_type('email') + delete_notifications_older_than_retention_by_type('email') assert len(Notification.query.filter_by().all()) == 5 assert len(Notification.query.filter_by(notification_type='email').all()) == 1 @@ -182,7 +182,7 @@ def test_delete_notifications_does_try_to_delete_from_s3_when_letter_has_not_bee create_notification(template=letter_template, status='sending', reference='LETTER_REF') - delete_notifications_created_more_than_a_week_ago_by_type('email', qry_limit=1) + delete_notifications_older_than_retention_by_type('email', qry_limit=1) mock_get_s3.assert_not_called() @@ -196,7 +196,7 @@ def test_delete_notifications_calls_subquery( create_notification(template=sms_template, created_at=datetime.now() - timedelta(days=8)) assert Notification.query.count() == 3 - delete_notifications_created_more_than_a_week_ago_by_type('sms', qry_limit=1) + delete_notifications_older_than_retention_by_type('sms', qry_limit=1) assert Notification.query.count() == 0 From e7b8f83fc897e9e0110b48357bd8be5dd1b0333f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Feb 2019 17:57:55 +0000 Subject: [PATCH 03/12] make inbound sms honour data retention code inspired by the delete notification code, but with some clean up since we don't deal with different types etc, and only need to run the query for services with inbound numbers also, update tests.app.db.create_inbound_sms to create inbound numbers and assign them to services to ensure the test db is always accurate and reflects real world usage --- app/dao/inbound_sms_dao.py | 67 +++++++++++++++++++++------ tests/app/dao/test_inbound_sms_dao.py | 50 +++++++++++--------- tests/app/db.py | 11 ++++- 3 files changed, 91 insertions(+), 37 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 22b0d468b..c65eb8e81 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -1,8 +1,3 @@ -from datetime import ( - timedelta, - datetime, - date -) from flask import current_app from notifications_utils.statsd_decorators import statsd from sqlalchemy import desc, and_ @@ -10,8 +5,8 @@ from sqlalchemy.orm import aliased from app import db from app.dao.dao_utils import transactional -from app.models import InboundSms -from app.utils import get_london_midnight_in_utc +from app.models import InboundSms, Service, ServiceDataRetention, SMS_TYPE +from app.utils import midnight_n_days_ago @transactional @@ -20,7 +15,7 @@ def dao_create_inbound_sms(inbound_sms): def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None): - start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6)) + start_date = midnight_n_days_ago(6) q = InboundSms.query.filter( InboundSms.service_id == service_id, InboundSms.created_at >= start_date @@ -60,21 +55,63 @@ def dao_get_paginated_inbound_sms_for_service_for_public_api( def dao_count_inbound_sms_for_service(service_id): - start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6)) + start_date = midnight_n_days_ago(6) return InboundSms.query.filter( InboundSms.service_id == service_id, InboundSms.created_at >= start_date ).count() +def _delete_inbound_sms(datetime_to_delete_from, query_filter): + query_limit = 10000 + + subquery = db.session.query( + InboundSms.id + ).filter( + InboundSms.created_at < datetime_to_delete_from, + *query_filter + ).limit( + query_limit + ).subquery() + + deleted = 0 + # set to nonzero just to enter the loop + number_deleted = 1 + while number_deleted > 0: + number_deleted = InboundSms.query.filter(InboundSms.id.in_(subquery)).delete(synchronize_session='fetch') + deleted += number_deleted + + return deleted + + @statsd(namespace="dao") @transactional -def delete_inbound_sms_created_more_than_a_week_ago(): - seven_days_ago = datetime.utcnow() - timedelta(days=7) +def delete_inbound_sms_older_than_retention(): + current_app.logger.info('Deleting inbound sms for services with flexible data retention') - deleted = db.session.query(InboundSms).filter( - InboundSms.created_at < seven_days_ago - ).delete(synchronize_session='fetch') + flexible_data_retention = ServiceDataRetention.query.join( + ServiceDataRetention.service, + Service.inbound_number + ).filter( + ServiceDataRetention.notification_type == SMS_TYPE + ).all() + + deleted = 0 + for f in flexible_data_retention: + n_days_ago = midnight_n_days_ago(f.days_of_retention) + + current_app.logger.info("Deleting inbound sms for service id: {}".format(f.service_id)) + deleted += _delete_inbound_sms(n_days_ago, query_filter=[InboundSms.service_id == f.service_id]) + + current_app.logger.info('Deleting inbound sms for services without flexible data retention') + + seven_days_ago = midnight_n_days_ago(7) + + deleted += _delete_inbound_sms(seven_days_ago, query_filter=[ + InboundSms.service_id.notin_(x.service_id for x in flexible_data_retention), + ]) + + current_app.logger.info('Deleted {} inbound sms'.format(deleted)) return deleted @@ -108,7 +145,7 @@ def dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service( ORDER BY t1.created_at DESC; LIMIT 50 OFFSET :page """ - start_date = get_london_midnight_in_utc(date.today() - timedelta(days=6)) + start_date = midnight_n_days_ago(6) t2 = aliased(InboundSms) q = db.session.query( InboundSms diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 6fd43a47b..cb0822cdf 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -1,19 +1,18 @@ -from datetime import datetime, timedelta +from datetime import datetime +from itertools import product from freezegun import freeze_time 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, + delete_inbound_sms_older_than_retention, dao_get_inbound_sms_by_id, dao_get_paginated_inbound_sms_for_service_for_public_api, dao_get_paginated_most_recent_inbound_sms_by_user_number_for_service ) from tests.conftest import set_config -from tests.app.db import create_inbound_sms, create_service - -from app.models import InboundSms +from tests.app.db import create_inbound_sms, create_service, create_service_data_retention def test_get_all_inbound_sms(sample_service): @@ -93,28 +92,37 @@ def test_count_inbound_sms_for_service_filters_messages_older_than_seven_days(sa assert dao_count_inbound_sms_for_service(sample_service.id) == 1 -@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() +@freeze_time("2017-06-08 12:00:00") +def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session): + service_without_retention = create_service(service_name='without retention') + service_with_three_day_retention = create_service(service_name='three days') + service_with_month_retention = create_service(service_name='thirty days') - assert len(InboundSms.query.all()) == 0 + services = [service_with_three_day_retention, service_without_retention, service_with_month_retention] + create_service_data_retention(service_with_month_retention.id, notification_type='sms', days_of_retention=30) + create_service_data_retention(service_with_three_day_retention.id, notification_type='sms', days_of_retention=3) + # email retention doesn't affect anything + create_service_data_retention(service_with_three_day_retention.id, notification_type='email', days_of_retention=4) -@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) + dates = [ + datetime(2017, 6, 4, 23, 00), # just before three days + datetime(2017, 6, 4, 22, 59), # older than three days + datetime(2017, 5, 31, 23, 00), # just before seven days + datetime(2017, 5, 31, 22, 59), # older than seven days + datetime(2017, 5, 7, 22, 59), # older than thirty days + ] - 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) + for date, service in product(dates, services): + create_inbound_sms(service, created_at=date) - delete_inbound_sms_created_more_than_a_week_ago() + deleted_count = delete_inbound_sms_older_than_retention() - assert len(InboundSms.query.all()) == 2 + # four deleted for the 3-day service, two for the default seven days one, one for the 30 day + assert deleted_count == 7 + assert dao_count_inbound_sms_for_service(service_without_retention.id) == 3 + assert dao_count_inbound_sms_for_service(service_with_three_day_retention.id) == 1 + assert dao_count_inbound_sms_for_service(service_with_month_retention.id) == 4 def test_get_inbound_sms_by_id_returns(sample_service): diff --git a/tests/app/db.py b/tests/app/db.py index 2aa6e3ae1..023e5fe30 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -1,3 +1,4 @@ +import random import uuid from datetime import datetime, date @@ -318,10 +319,18 @@ def create_inbound_sms( provider="mmg", created_at=None ): + if not service.inbound_number: + create_inbound_number( + # create random inbound number + notify_number or '07{:09}'.format(random.randint(0, 1e9 - 1)), + provider=provider, + service_id=service.id + ) + inbound = InboundSms( service=service, created_at=created_at or datetime.utcnow(), - notify_number=notify_number or service.get_default_sms_sender(), + notify_number=service.get_inbound_number(), user_number=user_number, provider_date=provider_date or datetime.utcnow(), provider_reference=provider_reference or 'foo', From 88303140ec9983ff95fd1edd33eb56bf9fccdf98 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 27 Feb 2019 17:17:35 +0000 Subject: [PATCH 04/12] give dao_get_inbound_sms_for_service optional day limit defaults to 6 to preserve backwards compatibility --- app/dao/inbound_sms_dao.py | 9 +++++---- tests/app/dao/test_inbound_sms_dao.py | 28 ++++++++++++++++----------- 2 files changed, 22 insertions(+), 15 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index c65eb8e81..cb086899a 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -14,14 +14,15 @@ def dao_create_inbound_sms(inbound_sms): db.session.add(inbound_sms) -def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None): - start_date = midnight_n_days_ago(6) +def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None, days_ago_to_start=6): q = InboundSms.query.filter( - InboundSms.service_id == service_id, - InboundSms.created_at >= start_date + InboundSms.service_id == service_id ).order_by( InboundSms.created_at.desc() ) + if days_ago_to_start is not None: + start_date = midnight_n_days_ago(days_ago_to_start) + q = q.filter(InboundSms.created_at >= start_date) if user_number: q = q.filter(InboundSms.user_number == user_number) diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index cb0822cdf..f35103f1c 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -94,23 +94,23 @@ def test_count_inbound_sms_for_service_filters_messages_older_than_seven_days(sa @freeze_time("2017-06-08 12:00:00") def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session): - service_without_retention = create_service(service_name='without retention') - service_with_three_day_retention = create_service(service_name='three days') - service_with_month_retention = create_service(service_name='thirty days') + no_retention_service = create_service(service_name='no retention') + short_retention_service = create_service(service_name='three days') + long_retention_service = create_service(service_name='thirty days') - services = [service_with_three_day_retention, service_without_retention, service_with_month_retention] + services = [short_retention_service, no_retention_service, long_retention_service] - create_service_data_retention(service_with_month_retention.id, notification_type='sms', days_of_retention=30) - create_service_data_retention(service_with_three_day_retention.id, notification_type='sms', days_of_retention=3) + create_service_data_retention(long_retention_service.id, notification_type='sms', days_of_retention=30) + create_service_data_retention(short_retention_service.id, notification_type='sms', days_of_retention=3) # email retention doesn't affect anything - create_service_data_retention(service_with_three_day_retention.id, notification_type='email', days_of_retention=4) + create_service_data_retention(short_retention_service.id, notification_type='email', days_of_retention=4) dates = [ datetime(2017, 6, 4, 23, 00), # just before three days datetime(2017, 6, 4, 22, 59), # older than three days datetime(2017, 5, 31, 23, 00), # just before seven days datetime(2017, 5, 31, 22, 59), # older than seven days - datetime(2017, 5, 7, 22, 59), # older than thirty days + datetime(2017, 5, 1, 0, 0), # older than thirty days ] for date, service in product(dates, services): @@ -120,9 +120,15 @@ def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session # four deleted for the 3-day service, two for the default seven days one, one for the 30 day assert deleted_count == 7 - assert dao_count_inbound_sms_for_service(service_without_retention.id) == 3 - assert dao_count_inbound_sms_for_service(service_with_three_day_retention.id) == 1 - assert dao_count_inbound_sms_for_service(service_with_month_retention.id) == 4 + assert { + x.created_at for x in dao_get_inbound_sms_for_service(short_retention_service.id, days_ago_to_start=None) + } == set(dates[:1]) + assert { + x.created_at for x in dao_get_inbound_sms_for_service(no_retention_service.id, days_ago_to_start=None) + } == set(dates[:3]) + assert { + x.created_at for x in dao_get_inbound_sms_for_service(long_retention_service.id, days_ago_to_start=None) + } == set(dates[:4]) def test_get_inbound_sms_by_id_returns(sample_service): From b4d1a590b71b22d89cb3ab634e377fc20c379b84 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 28 Feb 2019 13:59:28 +0000 Subject: [PATCH 05/12] rename days_ago_to_start to limit_days consistency with the rest of the app --- app/dao/inbound_sms_dao.py | 6 +++--- tests/app/dao/test_inbound_sms_dao.py | 6 +++--- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index cb086899a..88ab0e356 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -14,14 +14,14 @@ def dao_create_inbound_sms(inbound_sms): db.session.add(inbound_sms) -def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None, days_ago_to_start=6): +def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None, limit_days=6): q = InboundSms.query.filter( InboundSms.service_id == service_id ).order_by( InboundSms.created_at.desc() ) - if days_ago_to_start is not None: - start_date = midnight_n_days_ago(days_ago_to_start) + if limit_days is not None: + start_date = midnight_n_days_ago(limit_days) q = q.filter(InboundSms.created_at >= start_date) if user_number: diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index f35103f1c..e8419dae6 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -121,13 +121,13 @@ def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session # four deleted for the 3-day service, two for the default seven days one, one for the 30 day assert deleted_count == 7 assert { - x.created_at for x in dao_get_inbound_sms_for_service(short_retention_service.id, days_ago_to_start=None) + x.created_at for x in dao_get_inbound_sms_for_service(short_retention_service.id, limit_days=None) } == set(dates[:1]) assert { - x.created_at for x in dao_get_inbound_sms_for_service(no_retention_service.id, days_ago_to_start=None) + x.created_at for x in dao_get_inbound_sms_for_service(no_retention_service.id, limit_days=None) } == set(dates[:3]) assert { - x.created_at for x in dao_get_inbound_sms_for_service(long_retention_service.id, days_ago_to_start=None) + x.created_at for x in dao_get_inbound_sms_for_service(long_retention_service.id, limit_days=None) } == set(dates[:4]) From 7683e340cc10f99acd76ee99faf8833884957e75 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 28 Feb 2019 15:28:13 +0000 Subject: [PATCH 06/12] get all inbound sms should default to 7 days, not 6 to be consistent with other checks. --- app/dao/inbound_sms_dao.py | 2 +- tests/app/dao/test_inbound_sms_dao.py | 11 +++-------- 2 files changed, 4 insertions(+), 9 deletions(-) diff --git a/app/dao/inbound_sms_dao.py b/app/dao/inbound_sms_dao.py index 88ab0e356..0b6334479 100644 --- a/app/dao/inbound_sms_dao.py +++ b/app/dao/inbound_sms_dao.py @@ -14,7 +14,7 @@ def dao_create_inbound_sms(inbound_sms): db.session.add(inbound_sms) -def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None, limit_days=6): +def dao_get_inbound_sms_for_service(service_id, limit=None, user_number=None, limit_days=7): q = InboundSms.query.filter( InboundSms.service_id == service_id ).order_by( diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index e8419dae6..24eb1761c 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -58,15 +58,10 @@ def test_get_all_inbound_sms_filters_on_service(notify_db_session): def test_get_all_inbound_sms_filters_on_time(sample_service, notify_db_session): - create_inbound_sms(sample_service, user_number='447700900111', content='111 1', created_at=datetime(2017, 1, 2)) - sms_two = create_inbound_sms( - sample_service, - user_number='447700900111', - content='111 2', - created_at=datetime(2017, 1, 3) - ) + create_inbound_sms(sample_service, created_at=datetime(2017, 8, 6, 22, 59)) # sunday evening + sms_two = create_inbound_sms(sample_service, created_at=datetime(2017, 8, 6, 23, 0)) # monday (7th) morning - with freeze_time('2017-01-09'): + with freeze_time('2017-08-14 12:00'): res = dao_get_inbound_sms_for_service(sample_service.id) assert len(res) == 1 From 6f5822ae5b969c3eeb4fce8f9fb6324697311383 Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 6 Mar 2019 11:35:32 +0000 Subject: [PATCH 07/12] Downgrade log level for missing notifications in SES receipt The timestamps available in the SES receipt don't always correspond to the time the notification has been sent. We've seen callbacks with a current timestamp in both 'mail' and 'bounce' objects that referenced a notification sent a week ago, which means we can't rely on it to skip archived notifications. One possible approach would be to look up the notification reference in the notification_history table, but this goes against our plans to stop relying on it in the future. This changes the SES receipts logic to retry missing notifications once (if the callback timestamp is within the last 5 minutes the task will retry after a 5 minute delay) to capture callbacks arriving before the notification reference has been persisted to the DB. Otherwise, we log the missing notification as a warning instead of error. --- app/celery/process_ses_receipts_tasks.py | 6 +++--- tests/app/celery/test_process_ses_receipts_tasks.py | 2 +- 2 files changed, 4 insertions(+), 4 deletions(-) diff --git a/app/celery/process_ses_receipts_tasks.py b/app/celery/process_ses_receipts_tasks.py index ebe0cfaf9..3380047f3 100644 --- a/app/celery/process_ses_receipts_tasks.py +++ b/app/celery/process_ses_receipts_tasks.py @@ -42,10 +42,10 @@ def process_ses_results(self, response): notification = notifications_dao.dao_get_notification_by_reference(reference) except NoResultFound: message_time = iso8601.parse_date(ses_message['mail']['timestamp']).replace(tzinfo=None) - if datetime.utcnow() - message_time < timedelta(minutes=10): + if datetime.utcnow() - message_time < timedelta(minutes=5): self.retry(queue=QueueNames.RETRY) - elif datetime.utcnow() - message_time < timedelta(days=3): - current_app.logger.error( + else: + current_app.logger.warning( "notification not found for reference: {} (update to {})".format(reference, notification_status) ) return diff --git a/tests/app/celery/test_process_ses_receipts_tasks.py b/tests/app/celery/test_process_ses_receipts_tasks.py index 3cd27cfd1..0905f1069 100644 --- a/tests/app/celery/test_process_ses_receipts_tasks.py +++ b/tests/app/celery/test_process_ses_receipts_tasks.py @@ -117,7 +117,7 @@ def test_ses_callback_should_retry_if_notification_is_new(client, notify_db, moc def test_ses_callback_should_log_if_notification_is_missing(client, notify_db, mocker): mock_retry = mocker.patch('app.celery.process_ses_receipts_tasks.process_ses_results.retry') - mock_logger = mocker.patch('app.celery.process_ses_receipts_tasks.current_app.logger.error') + mock_logger = mocker.patch('app.celery.process_ses_receipts_tasks.current_app.logger.warning') with freeze_time('2017-11-17T12:34:03.646Z'): assert process_ses_results(ses_notification_callback(reference='ref')) is None From d7e03e00d32e2b71c55e3b44d2c740e0e3dec12b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 19 Feb 2019 11:47:30 +0000 Subject: [PATCH 08/12] Storing more info about an organisation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Currently we have - a thing in the database called an ‘organisation’ which we don’t use - the idea of an organisation which we derive from the user’s email address and is used to set the default branding for their service and determine whether they’ve signed the MOU We should make these two things into one thing, by storing everything we know about an organisation against that organisation in the database. This will be much less laborious than storing it in a YAML file that needs a deploy every time it’s updated. An organisation can now have: - domains which we can use to automatically associate services with it (eg anyone whose email address ends in `dwp.gsi.gov.uk` gets services they create associated to the DWP organisation) - default letter branding for any new services - default email branding for any new services --- app/dao/organisation_dao.py | 19 ++++- app/models.py | 47 +++++++++++- .../versions/0278_add_more_stuff_to_orgs.py | 57 +++++++++++++++ tests/app/dao/test_organisation_dao.py | 38 ++++++++-- tests/app/organisation/test_rest.py | 73 ++++++++++++++++++- 5 files changed, 221 insertions(+), 13 deletions(-) create mode 100644 migrations/versions/0278_add_more_stuff_to_orgs.py diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index 94cdeb237..e5e1cbb62 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -2,6 +2,7 @@ from app import db from app.dao.dao_utils import transactional from app.models import ( Organisation, + Domain, InvitedOrganisationUser, User ) @@ -34,10 +35,26 @@ def dao_create_organisation(organisation): @transactional def dao_update_organisation(organisation_id, **kwargs): - return Organisation.query.filter_by(id=organisation_id).update( + + domains = kwargs.pop('domains', []) + + organisation = Organisation.query.filter_by(id=organisation_id).update( kwargs ) + if domains: + + Domain.query.filter_by(organisation_id=organisation_id).delete() + + db.session.bulk_save_objects([ + Domain(domain=domain, organisation_id=organisation_id) + for domain in domains + ]) + + db.session.commit() + + return organisation + @transactional def dao_add_service_to_organisation(service, organisation_id): diff --git a/app/models.py b/app/models.py index b9c7799bd..a897bd761 100644 --- a/app/models.py +++ b/app/models.py @@ -316,6 +316,12 @@ organisation_to_service = db.Table( ) +class Domain(db.Model): + __tablename__ = "domain" + domain = db.Column(db.String(255), primary_key=True) + organisation_id = db.Column('organisation_id', UUID(as_uuid=True), db.ForeignKey('organisation.id'), nullable=False) + + class Organisation(db.Model): __tablename__ = "organisation" id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4, unique=False) @@ -329,15 +335,50 @@ class Organisation(db.Model): secondary='organisation_to_service', uselist=True) + agreement_signed = db.Column(db.Boolean, nullable=True) + agreement_signed_at = db.Column(db.DateTime, nullable=True) + agreement_signed_by_id = db.Column( + UUID(as_uuid=True), + db.ForeignKey('users.id'), + nullable=True, + ) + agreement_signed_version = db.Column(db.Float, nullable=True) + crown = db.Column(db.Boolean, nullable=True) + organisation_type = db.Column(db.String(255), nullable=True) + + domains = db.relationship( + 'Domain', + ) + + email_branding = db.relationship('EmailBranding') + email_branding_id = db.Column( + UUID(as_uuid=True), + db.ForeignKey('email_branding.id'), + nullable=True, + ) + + letter_branding = db.relationship('LetterBranding') + letter_branding_id = db.Column( + UUID(as_uuid=True), + db.ForeignKey('letter_branding.id'), + nullable=True, + ) + def serialize(self): - serialized = { + return { "id": str(self.id), "name": self.name, "active": self.active, + "crown": self.crown, + "organisation_type": self.organisation_type, + "letter_branding_id": self.letter_branding_id, + "email_branding_id": self.email_branding_id, + "agreement_signed": self.agreement_signed, + "agreement_signed_at": self.agreement_signed_at, + "agreement_signed_by_id": self.agreement_signed_by_id, + "agreement_signed_version": self.agreement_signed_version, } - return serialized - class Service(db.Model, Versioned): __tablename__ = 'services' diff --git a/migrations/versions/0278_add_more_stuff_to_orgs.py b/migrations/versions/0278_add_more_stuff_to_orgs.py new file mode 100644 index 000000000..f3bd91469 --- /dev/null +++ b/migrations/versions/0278_add_more_stuff_to_orgs.py @@ -0,0 +1,57 @@ +""" + +Revision ID: 0278_add_more_stuff_to_orgs +Revises: 0277_consent_to_research_null +Create Date: 2019-02-26 10:15:22.430340 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0278_add_more_stuff_to_orgs' +down_revision = '0277_consent_to_research_null' + + +def upgrade(): + op.create_table( + 'domain', + sa.Column('domain', sa.String(length=255), nullable=False), + sa.Column('organisation_id', postgresql.UUID(as_uuid=True), nullable=False), + sa.ForeignKeyConstraint(['organisation_id'], ['organisation.id'], ), + sa.PrimaryKeyConstraint('domain') + ) + op.create_index(op.f('ix_domain_domain'), 'domain', ['domain'], unique=True) + + op.add_column('organisation', sa.Column('email_branding_id', postgresql.UUID(as_uuid=True), nullable=True)) + op.create_foreign_key('fk_organisation_email_branding_id', 'organisation', 'email_branding', ['email_branding_id'], ['id']) + + op.add_column('organisation', sa.Column('letter_branding_id', postgresql.UUID(as_uuid=True), nullable=True)) + op.create_foreign_key('fk_organisation_letter_branding_id', 'organisation', 'letter_branding', ['letter_branding_id'], ['id']) + + op.add_column('organisation', sa.Column('agreement_signed', sa.Boolean(), nullable=True)) + op.add_column('organisation', sa.Column('agreement_signed_at', sa.DateTime(), nullable=True)) + op.add_column('organisation', sa.Column('agreement_signed_by_id', postgresql.UUID(as_uuid=True), nullable=True)) + op.add_column('organisation', sa.Column('agreement_signed_version', sa.Float(), nullable=True)) + op.add_column('organisation', sa.Column('crown', sa.Boolean(), nullable=True)) + op.add_column('organisation', sa.Column('organisation_type', sa.String(length=255), nullable=True)) + op.create_foreign_key('fk_organisation_agreement_user_id', 'organisation', 'users', ['agreement_signed_by_id'], ['id']) + + +def downgrade(): + op.drop_constraint('fk_organisation_agreement_user_id', 'organisation', type_='foreignkey') + op.drop_column('organisation', 'organisation_type') + op.drop_column('organisation', 'crown') + op.drop_column('organisation', 'agreement_signed_version') + op.drop_column('organisation', 'agreement_signed_by_id') + op.drop_column('organisation', 'agreement_signed_at') + op.drop_column('organisation', 'agreement_signed') + + op.drop_column('organisation', 'email_branding_id') + op.drop_constraint('fk_organisation_email_branding_id', 'organisation', type_='foreignkey') + + op.drop_column('organisation', 'letter_branding_id') + op.drop_constraint('fk_organisation_letter_branding_id', 'organisation', type_='foreignkey') + + op.drop_index(op.f('ix_domain_domain'), table_name='domain') + op.drop_table('domain') diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py index 62c81041a..7e1f90e2f 100644 --- a/tests/app/dao/test_organisation_dao.py +++ b/tests/app/dao/test_organisation_dao.py @@ -1,3 +1,4 @@ +import datetime import uuid import pytest @@ -16,7 +17,13 @@ from app.dao.organisation_dao import ( ) from app.models import Organisation -from tests.app.db import create_organisation, create_service, create_user +from tests.app.db import ( + create_email_branding, + create_letter_branding, + create_organisation, + create_service, + create_user, +) def test_get_organisations_gets_all_organisations_alphabetically_with_active_organisations_first( @@ -47,19 +54,38 @@ def test_get_organisation_by_id_gets_correct_organisation(notify_db, notify_db_s assert organisation_from_db == organisation -def test_update_organisation(notify_db, notify_db_session): - updated_name = 'new name' +def test_update_organisation( + notify_db, + notify_db_session, +): create_organisation() organisation = Organisation.query.one() + user = create_user() + email_branding = create_email_branding() + letter_branding = create_letter_branding() - assert organisation.name != updated_name + data = { + 'name': 'new name', + "crown": True, + "organisation_type": 'local', + "agreement_signed": True, + "agreement_signed_at": datetime.datetime.utcnow(), + "agreement_signed_by_id": user.id, + "agreement_signed_version": 999.99, + "letter_branding_id": letter_branding.id, + "email_branding_id": email_branding.id, + } - dao_update_organisation(organisation.id, **{'name': updated_name}) + for attribute, value in data.items(): + assert getattr(organisation, attribute) != value + + dao_update_organisation(organisation.id, **data) organisation = Organisation.query.one() - assert organisation.name == updated_name + for attribute, value in data.items(): + assert getattr(organisation, attribute) == value def test_add_service_to_organisation(notify_db, notify_db_session, sample_service, sample_organisation): diff --git a/tests/app/organisation/test_rest.py b/tests/app/organisation/test_rest.py index dee403245..1bd0fecf7 100644 --- a/tests/app/organisation/test_rest.py +++ b/tests/app/organisation/test_rest.py @@ -32,10 +32,29 @@ def test_get_organisation_by_id(admin_request, notify_db_session): organisation_id=org.id ) - assert set(response.keys()) == {'id', 'name', 'active'} + assert set(response.keys()) == { + 'id', + 'name', + 'active', + 'crown', + 'organisation_type', + 'agreement_signed', + 'agreement_signed_at', + 'agreement_signed_by_id', + 'agreement_signed_version', + 'letter_branding_id', + 'email_branding_id', + } assert response['id'] == str(org.id) assert response['name'] == 'test_org_1' assert response['active'] is True + assert response['crown'] is None + assert response['organisation_type'] is None + assert response['agreement_signed'] is None + assert response['agreement_signed_by_id'] is None + assert response['agreement_signed_version'] is None + assert response['letter_branding_id'] is None + assert response['email_branding_id'] is None def test_post_create_organisation(admin_request, notify_db_session): @@ -91,12 +110,27 @@ def test_post_create_organisation_with_missing_name_gives_validation_error(admin assert response['errors'][0]['message'] == 'name is a required property' -def test_post_update_organisation_updates_fields(admin_request, notify_db_session): +@pytest.mark.parametrize('agreement_signed', ( + None, True, False +)) +@pytest.mark.parametrize('crown', ( + None, True, False +)) +def test_post_update_organisation_updates_fields( + admin_request, + notify_db_session, + agreement_signed, + crown, +): org = create_organisation() data = { 'name': 'new organisation name', - 'active': False + 'active': False, + 'agreement_signed': agreement_signed, + 'crown': crown, } + assert org.agreement_signed is None + assert org.crown is None admin_request.post( 'organisation.update_organisation', @@ -111,6 +145,39 @@ def test_post_update_organisation_updates_fields(admin_request, notify_db_sessio assert organisation[0].id == org.id assert organisation[0].name == data['name'] assert organisation[0].active == data['active'] + assert organisation[0].agreement_signed == agreement_signed + assert organisation[0].crown == crown + assert organisation[0].domains == [] + + +@pytest.mark.parametrize('domain_list', ( + ['example.com'], + ['example.com', 'example.org', 'example.net'], + [], +)) +def test_post_update_organisation_updates_domains( + admin_request, + notify_db_session, + domain_list, +): + org = create_organisation(name='test_org_2') + data = { + 'domains': domain_list, + } + + admin_request.post( + 'organisation.update_organisation', + _data=data, + organisation_id=org.id, + _expected_status=204 + ) + + organisation = Organisation.query.all() + + assert len(organisation) == 1 + assert [ + domain.domain for domain in organisation[0].domains + ] == domain_list def test_post_update_organisation_raises_400_on_existing_org_name( From c0fb9267bd531cd90750272c006bd0a35d37e2b6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 19 Feb 2019 12:02:18 +0000 Subject: [PATCH 09/12] Automatically associate new service with an org This is the same thing we do in the admin app at the moment with YAML: https://github.com/alphagov/notifications-admin/blob/2f4e933b6595d384bebea253ba47f8091b462ca3/app/utils.py#L556-L562 --- app/dao/organisation_dao.py | 14 ++++++++ app/dao/services_dao.py | 26 +++++++++++++- tests/app/dao/test_organisation_dao.py | 29 +++++++++++++++ tests/app/dao/test_services_dao.py | 6 ++-- tests/app/db.py | 13 ++++++- tests/app/service/test_rest.py | 49 +++++++++++++++++++++++++- 6 files changed, 131 insertions(+), 6 deletions(-) diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index e5e1cbb62..eca2787b4 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -24,6 +24,20 @@ def dao_get_organisation_by_id(organisation_id): return Organisation.query.filter_by(id=organisation_id).one() +def dao_get_organisation_by_email_address(email_address): + + email_address = email_address.lower() + + for domain in Domain().query.all(): + if ( + email_address.endswith("@{}".format(domain.domain)) or + email_address.endswith(".{}".format(domain.domain)) + ): + return Organisation.query.filter_by(id=domain.organisation_id).one() + + return None + + def dao_get_organisation_by_service_id(service_id): return Organisation.query.join(Organisation.services).filter_by(id=service_id).first() diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index eb766c06b..70d7a8a83 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -11,6 +11,7 @@ from app.dao.dao_utils import ( transactional, version_class ) +from app.dao.organisation_dao import dao_get_organisation_by_email_address from app.dao.service_sms_sender_dao import insert_service_sms_sender from app.dao.service_user_dao import dao_get_service_user from app.models import ( @@ -151,14 +152,25 @@ def dao_fetch_service_by_id_and_user(service_id, user_id): @transactional @version_class(Service) -def dao_create_service(service, user, service_id=None, service_permissions=None, letter_branding=None): +def dao_create_service( + service, + user, + service_id=None, + service_permissions=None, + letter_branding=None, +): # the default property does not appear to work when there is a difference between the sqlalchemy schema and the # db schema (ie: during a migration), so we have to set sms_sender manually here. After the GOVUK sms_sender # migration is completed, this code should be able to be removed. + if not user: + raise ValueError("Can't create a service without a user") + if service_permissions is None: service_permissions = DEFAULT_SERVICE_PERMISSIONS + organisation = dao_get_organisation_by_email_address(user.email_address) + from app.dao.permissions_dao import permission_dao service.users.append(user) permission_dao.add_default_service_permissions_for_user(user, service) @@ -173,8 +185,20 @@ def dao_create_service(service, user, service_id=None, service_permissions=None, # do we just add the default - or will we get a value from FE? insert_service_sms_sender(service, current_app.config['FROM_NUMBER']) + if letter_branding: service.letter_branding = letter_branding + + if organisation: + + service.organisation = organisation + + if organisation.email_branding_id: + service.email_branding = organisation.email_branding_id + + if organisation.letter_branding_id and not service.letter_branding: + service.letter_branding = organisation.letter_branding_id + db.session.add(service) diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py index 7e1f90e2f..6375cf7b6 100644 --- a/tests/app/dao/test_organisation_dao.py +++ b/tests/app/dao/test_organisation_dao.py @@ -6,6 +6,7 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError from app.dao.organisation_dao import ( dao_get_organisations, + dao_get_organisation_by_email_address, dao_get_organisation_by_id, dao_get_organisation_by_service_id, dao_get_organisation_services, @@ -18,6 +19,7 @@ from app.dao.organisation_dao import ( from app.models import Organisation from tests.app.db import ( + create_domain, create_email_branding, create_letter_branding, create_organisation, @@ -197,3 +199,30 @@ def test_add_user_to_organisation_when_user_does_not_exist(sample_organisation): def test_add_user_to_organisation_when_organisation_does_not_exist(sample_user): with pytest.raises(expected_exception=SQLAlchemyError): dao_add_user_to_organisation(organisation_id=uuid.uuid4(), user_id=sample_user.id) + + +@pytest.mark.parametrize('domain, expected_org', ( + ('unknown.gov.uk', False), + ('example.gov.uk', True), +)) +def test_get_organisation_by_email_address( + admin_request, + sample_user, + domain, + expected_org, +): + + org = create_organisation() + create_domain('example.gov.uk', org.id) + create_domain('test.gov.uk', org.id) + + another_org = create_organisation(name='Another') + create_domain('cabinet-office.gov.uk', another_org.id) + create_domain('cabinetoffice.gov.uk', another_org.id) + + found_org = dao_get_organisation_by_email_address('test@{}'.format(domain)) + + if expected_org: + assert found_org is org + else: + assert found_org is None diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 6461fd93c..cbc6cb24c 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -4,7 +4,7 @@ from datetime import datetime import pytest from freezegun import freeze_time from sqlalchemy.exc import IntegrityError -from sqlalchemy.orm.exc import FlushError, NoResultFound +from sqlalchemy.orm.exc import NoResultFound from app import db from app.dao.inbound_numbers_dao import ( @@ -167,9 +167,9 @@ def test_cannot_create_service_with_no_user(notify_db_session): message_limit=1000, restricted=False, created_by=user) - with pytest.raises(FlushError) as excinfo: + with pytest.raises(ValueError) as excinfo: dao_create_service(service, None) - assert "Can't flush None value found in collection Service.users" in str(excinfo.value) + assert "Can't create a service without a user" in str(excinfo.value) def test_should_add_user_to_service(notify_db_session): diff --git a/tests/app/db.py b/tests/app/db.py index 1b301a2f2..0b3b24c2b 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -51,7 +51,8 @@ from app.models import ( Complaint, InvitedUser, TemplateFolder, - LetterBranding + LetterBranding, + Domain, ) @@ -497,6 +498,16 @@ def create_annual_billing( return annual_billing +def create_domain(domain, organisation_id): + + domain = Domain(domain=domain, organisation_id=organisation_id) + + db.session.add(domain) + db.session.commit() + + return domain + + def create_organisation(name='test_org_1', active=True): data = { 'name': name, diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 22208dd52..e216f8801 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -44,7 +44,9 @@ from tests.app.db import ( create_inbound_number, create_service_sms_sender, create_service_with_defined_sms_sender, - create_letter_branding + create_letter_branding, + create_organisation, + create_domain, ) from tests.app.db import create_user @@ -253,6 +255,51 @@ def test_create_service(admin_request, sample_user): assert service_sms_senders[0].sms_sender == current_app.config['FROM_NUMBER'] +@pytest.mark.parametrize('domain, expected_org', ( + (None, False), + ('', False), + ('unknown.gov.uk', False), + ('unknown-example.gov.uk', False), + ('example.gov.uk', True), + ('test.gov.uk', True), + ('test.example.gov.uk', True), +)) +def test_create_service_with_domain_sets_organisation( + admin_request, + sample_user, + domain, + expected_org, +): + + org = create_organisation() + create_domain('example.gov.uk', org.id) + create_domain('test.gov.uk', org.id) + + another_org = create_organisation(name='Another') + create_domain('cabinet-office.gov.uk', another_org.id) + create_domain('cabinetoffice.gov.uk', another_org.id) + + sample_user.email_address = 'test@{}'.format(domain) + + data = { + 'name': 'created service', + 'user_id': str(sample_user.id), + 'message_limit': 1000, + 'restricted': False, + 'active': False, + 'email_from': 'created.service', + 'created_by': str(sample_user.id), + 'service_domain': domain, + } + + json_resp = admin_request.post('service.create_service', _data=data, _expected_status=201) + + if expected_org: + assert json_resp['data']['organisation'] == str(org.id) + else: + assert json_resp['data']['organisation'] is None + + def test_create_service_with_domain_sets_letter_branding(admin_request, sample_user): letter_branding = create_letter_branding( name='test domain', filename='test-domain', domain='test.domain' From 6e8ce786030f0bcea42ad8af7718551b6776b100 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 7 Mar 2019 11:23:42 +0000 Subject: [PATCH 10/12] Choose most specific domains first MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If we had organisations for GDS and Cabinet Office, then we’d always want someone whose email address ends in `@cabinet-office.gov.uk` to match to `cabinet-office.gov.uk` before matching to `digital.cabinet-office.gov.uk`. Sorting the list by shortest first addresses this. --- app/dao/organisation_dao.py | 5 ++++- tests/app/service/test_rest.py | 4 ++++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index eca2787b4..bdd430c8c 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -1,3 +1,5 @@ +from sqlalchemy.sql.expression import func + from app import db from app.dao.dao_utils import transactional from app.models import ( @@ -28,7 +30,8 @@ def dao_get_organisation_by_email_address(email_address): email_address = email_address.lower() - for domain in Domain().query.all(): + for domain in Domain.query.order_by(func.char_length(Domain.domain).desc()).all(): + if ( email_address.endswith("@{}".format(domain.domain)) or email_address.endswith(".{}".format(domain.domain)) diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index e216f8801..744f509e9 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -271,6 +271,10 @@ def test_create_service_with_domain_sets_organisation( expected_org, ): + red_herring_org = create_organisation(name='Sub example') + create_domain('specific.example.gov.uk', red_herring_org.id) + create_domain('aaaaaaaa.example.gov.uk', red_herring_org.id) + org = create_organisation() create_domain('example.gov.uk', org.id) create_domain('test.gov.uk', org.id) From 73ca8b73f92e6967e397ef5314a646538f30c4cc Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 8 Mar 2019 14:38:49 +0000 Subject: [PATCH 11/12] Make sure domains are always lowercased Because otherwise we might get garbage duplicate data. --- app/dao/organisation_dao.py | 4 ++-- tests/app/dao/test_organisation_dao.py | 29 ++++++++++++++++++++++++++ 2 files changed, 31 insertions(+), 2 deletions(-) diff --git a/app/dao/organisation_dao.py b/app/dao/organisation_dao.py index bdd430c8c..26b6dca1f 100644 --- a/app/dao/organisation_dao.py +++ b/app/dao/organisation_dao.py @@ -59,12 +59,12 @@ def dao_update_organisation(organisation_id, **kwargs): kwargs ) - if domains: + if isinstance(domains, list): Domain.query.filter_by(organisation_id=organisation_id).delete() db.session.bulk_save_objects([ - Domain(domain=domain, organisation_id=organisation_id) + Domain(domain=domain.lower(), organisation_id=organisation_id) for domain in domains ]) diff --git a/tests/app/dao/test_organisation_dao.py b/tests/app/dao/test_organisation_dao.py index 6375cf7b6..667fc1438 100644 --- a/tests/app/dao/test_organisation_dao.py +++ b/tests/app/dao/test_organisation_dao.py @@ -90,6 +90,35 @@ def test_update_organisation( assert getattr(organisation, attribute) == value +@pytest.mark.parametrize('domain_list, expected_domains', ( + (['abc', 'def'], {'abc', 'def'}), + (['ABC', 'DEF'], {'abc', 'def'}), + ([], set()), + (None, {'123', '456'}), + pytest.param( + ['abc', 'ABC'], {'abc'}, + marks=pytest.mark.xfail(raises=IntegrityError) + ), +)) +def test_update_organisation_domains_lowercases( + notify_db, + notify_db_session, + domain_list, + expected_domains, +): + create_organisation() + + organisation = Organisation.query.one() + + # Seed some domains + dao_update_organisation(organisation.id, domains=['123', '456']) + + # This should overwrite the seeded domains + dao_update_organisation(organisation.id, domains=domain_list) + + assert {domain.domain for domain in organisation.domains} == expected_domains + + def test_add_service_to_organisation(notify_db, notify_db_session, sample_service, sample_organisation): assert sample_organisation.services == [] From 98b389baa2a41435d7b6ff9669927b2c088ea1b7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 8 Mar 2019 15:03:00 +0000 Subject: [PATCH 12/12] Fix organisation domain migration Have to drop constraint before dropping columns. --- migrations/versions/0278_add_more_stuff_to_orgs.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/migrations/versions/0278_add_more_stuff_to_orgs.py b/migrations/versions/0278_add_more_stuff_to_orgs.py index f3bd91469..745c43fed 100644 --- a/migrations/versions/0278_add_more_stuff_to_orgs.py +++ b/migrations/versions/0278_add_more_stuff_to_orgs.py @@ -47,11 +47,11 @@ def downgrade(): op.drop_column('organisation', 'agreement_signed_at') op.drop_column('organisation', 'agreement_signed') - op.drop_column('organisation', 'email_branding_id') op.drop_constraint('fk_organisation_email_branding_id', 'organisation', type_='foreignkey') + op.drop_column('organisation', 'email_branding_id') - op.drop_column('organisation', 'letter_branding_id') op.drop_constraint('fk_organisation_letter_branding_id', 'organisation', type_='foreignkey') + op.drop_column('organisation', 'letter_branding_id') op.drop_index(op.f('ix_domain_domain'), table_name='domain') op.drop_table('domain')