From 67f171f2f8dc51bf9059b9602e58a4dc4ca704c1 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 3 Jun 2019 17:27:08 +0100 Subject: [PATCH] refactor tests re-order notification dao delete notifications test to move "fixtures" to the top of the file changed create_service_data_retention to take an ORM object, not an id. brings it in line with other db.py test functions --- tests/app/celery/test_nightly_tasks.py | 4 +- ...t_notification_dao_delete_notifications.py | 92 +++++++++---------- tests/app/dao/test_inbound_sms_dao.py | 6 +- .../dao/test_service_data_retention_dao.py | 4 +- tests/app/db.py | 4 +- tests/app/inbound_sms/test_rest.py | 8 +- .../test_service_data_retention_rest.py | 18 ++-- 7 files changed, 65 insertions(+), 71 deletions(-) diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index f0c8b8c4b..8ef9a58fb 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -105,8 +105,8 @@ def test_will_remove_csv_files_for_jobs_older_than_retention_period( mocker.patch('app.celery.nightly_tasks.s3.remove_job_from_s3') service_1 = create_service(service_name='service 1') service_2 = create_service(service_name='service 2') - create_service_data_retention(service_id=service_1.id, notification_type=SMS_TYPE, days_of_retention=3) - create_service_data_retention(service_id=service_2.id, notification_type=EMAIL_TYPE, days_of_retention=30) + create_service_data_retention(service=service_1, notification_type=SMS_TYPE, days_of_retention=3) + create_service_data_retention(service=service_2, notification_type=EMAIL_TYPE, days_of_retention=30) sms_template_service_1 = create_template(service=service_1) email_template_service_1 = create_template(service=service_1, template_type='email') 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 22190a4a1..9cdc6ce61 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 @@ -20,6 +20,40 @@ from tests.app.db import ( ) +def create_test_data(notification_type, sample_service, days_of_retention=3): + service_with_default_data_retention = create_service(service_name='default data retention') + email_template, letter_template, sms_template = _create_templates(sample_service) + default_email_template, default_letter_template, default_sms_template = _create_templates( + service_with_default_data_retention) + create_notification(template=email_template, status='delivered') + create_notification(template=sms_template, status='permanent-failure') + create_notification(template=letter_template, status='temporary-failure', + reference='LETTER_REF', sent_at=datetime.utcnow()) + create_notification(template=email_template, status='delivered', + created_at=datetime.utcnow() - timedelta(days=4)) + create_notification(template=sms_template, status='permanent-failure', + created_at=datetime.utcnow() - timedelta(days=4)) + create_notification(template=letter_template, status='temporary-failure', + reference='LETTER_REF', sent_at=datetime.utcnow(), + created_at=datetime.utcnow() - timedelta(days=4)) + create_notification(template=default_email_template, status='delivered', + created_at=datetime.utcnow() - timedelta(days=8)) + create_notification(template=default_sms_template, status='permanent-failure', + created_at=datetime.utcnow() - timedelta(days=8)) + create_notification(template=default_letter_template, status='temporary-failure', + reference='LETTER_REF', sent_at=datetime.utcnow(), + created_at=datetime.utcnow() - timedelta(days=8)) + create_service_data_retention(service=sample_service, notification_type=notification_type, + days_of_retention=days_of_retention) + + +def _create_templates(sample_service): + email_template = create_template(service=sample_service, template_type='email') + sms_template = create_template(service=sample_service) + letter_template = create_template(service=sample_service, template_type='letter') + return email_template, letter_template, sms_template + + @pytest.mark.parametrize('month, delete_run_time', [(4, '2016-04-10 23:40'), (1, '2016-01-11 00:40')]) @pytest.mark.parametrize( @@ -70,9 +104,8 @@ def test_should_delete_notifications_by_type_after_seven_days( assert notification.created_at.date() >= date(2016, month, 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(sample_service, notification_type, mocker): +def test_should_not_delete_notification_history(sample_service, mocker): mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") with freeze_time('2016-01-01 12:00'): email_template, letter_template, sms_template = _create_templates(sample_service) @@ -81,7 +114,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() == 0 - delete_notifications_older_than_retention_by_type(notification_type) + delete_notifications_older_than_retention_by_type('sms') assert Notification.query.count() == 2 assert NotificationHistory.query.count() == 1 @@ -140,33 +173,6 @@ def test_delete_notifications_updates_notification_history(sample_email_template assert history[0].sent_by == 'ses' -def create_test_data(notification_type, sample_service, days_of_retention=3): - service_with_default_data_retention = create_service(service_name='default data retention') - email_template, letter_template, sms_template = _create_templates(sample_service) - default_email_template, default_letter_template, default_sms_template = _create_templates( - service_with_default_data_retention) - create_notification(template=email_template, status='delivered') - create_notification(template=sms_template, status='permanent-failure') - create_notification(template=letter_template, status='temporary-failure', - reference='LETTER_REF', sent_at=datetime.utcnow()) - create_notification(template=email_template, status='delivered', - created_at=datetime.utcnow() - timedelta(days=4)) - create_notification(template=sms_template, status='permanent-failure', - created_at=datetime.utcnow() - timedelta(days=4)) - create_notification(template=letter_template, status='temporary-failure', - reference='LETTER_REF', sent_at=datetime.utcnow(), - created_at=datetime.utcnow() - timedelta(days=4)) - create_notification(template=default_email_template, status='delivered', - created_at=datetime.utcnow() - timedelta(days=8)) - create_notification(template=default_sms_template, status='permanent-failure', - created_at=datetime.utcnow() - timedelta(days=8)) - create_notification(template=default_letter_template, status='temporary-failure', - reference='LETTER_REF', sent_at=datetime.utcnow(), - created_at=datetime.utcnow() - timedelta(days=8)) - create_service_data_retention(service_id=sample_service.id, notification_type=notification_type, - days_of_retention=days_of_retention) - - @pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) def test_delete_notifications_keep_data_for_days_of_retention_is_longer(sample_service, notification_type, mocker): mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") @@ -192,7 +198,7 @@ def test_delete_notifications_with_test_keys(sample_template, mocker): def test_delete_notifications_delete_notification_type_for_default_time_if_no_days_of_retention_for_type( sample_service, mocker ): - create_service_data_retention(service_id=sample_service.id, notification_type='sms', + create_service_data_retention(service=sample_service, notification_type='sms', days_of_retention=15) email_template, letter_template, sms_template = _create_templates(sample_service) create_notification(template=email_template, status='delivered') @@ -220,9 +226,8 @@ def test_delete_notifications_does_try_to_delete_from_s3_when_letter_has_not_bee mock_get_s3.assert_not_called() -@pytest.mark.parametrize('notification_type', ['sms']) @freeze_time("2016-01-10 12:00:00.000000") -def test_should_not_delete_notification_if_history_does_not_exist(sample_service, notification_type, mocker): +def test_should_not_delete_notification_if_history_does_not_exist(sample_service, mocker): mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") mocker.patch("app.dao.notifications_dao.insert_update_notification_history") with freeze_time('2016-01-01 12:00'): @@ -232,19 +237,15 @@ def test_should_not_delete_notification_if_history_does_not_exist(sample_service create_notification(template=letter_template, status='temporary-failure') assert Notification.query.count() == 3 assert NotificationHistory.query.count() == 0 - delete_notifications_older_than_retention_by_type(notification_type) + delete_notifications_older_than_retention_by_type('sms') assert Notification.query.count() == 3 assert NotificationHistory.query.count() == 0 -def test_delete_notifications_calls_subquery( - notify_db_session, mocker -): - service = create_service() - sms_template = create_template(service=service) - create_notification(template=sms_template, created_at=datetime.now() - timedelta(days=8)) - create_notification(template=sms_template, created_at=datetime.now() - timedelta(days=8)) - create_notification(template=sms_template, created_at=datetime.now() - timedelta(days=8)) +def test_delete_notifications_calls_subquery_multiple_times(sample_template): + create_notification(template=sample_template, created_at=datetime.now() - timedelta(days=8)) + create_notification(template=sample_template, created_at=datetime.now() - timedelta(days=8)) + create_notification(template=sample_template, created_at=datetime.now() - timedelta(days=8)) assert Notification.query.count() == 3 delete_notifications_older_than_retention_by_type('sms', qry_limit=1) @@ -317,10 +318,3 @@ def test_insert_update_notification_history_updates_history_with_new_status(samp assert history.id == notification_2.id assert history.status == 'delivered' assert not NotificationHistory.query.get(notification_1.id) - - -def _create_templates(sample_service): - email_template = create_template(service=sample_service, template_type='email') - sms_template = create_template(service=sample_service) - letter_template = create_template(service=sample_service, template_type='letter') - return email_template, letter_template, sms_template diff --git a/tests/app/dao/test_inbound_sms_dao.py b/tests/app/dao/test_inbound_sms_dao.py index 09a15c0af..684773358 100644 --- a/tests/app/dao/test_inbound_sms_dao.py +++ b/tests/app/dao/test_inbound_sms_dao.py @@ -97,10 +97,10 @@ def test_should_delete_inbound_sms_according_to_data_retention(notify_db_session services = [short_retention_service, no_retention_service, long_retention_service] - 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) + create_service_data_retention(long_retention_service, notification_type='sms', days_of_retention=30) + create_service_data_retention(short_retention_service, notification_type='sms', days_of_retention=3) # email retention doesn't affect anything - create_service_data_retention(short_retention_service.id, notification_type='email', days_of_retention=4) + create_service_data_retention(short_retention_service, notification_type='email', days_of_retention=4) dates = [ datetime(2017, 6, 4, 23, 00), # just before three days diff --git a/tests/app/dao/test_service_data_retention_dao.py b/tests/app/dao/test_service_data_retention_dao.py index 2c5551bb3..d197b4457 100644 --- a/tests/app/dao/test_service_data_retention_dao.py +++ b/tests/app/dao/test_service_data_retention_dao.py @@ -141,8 +141,8 @@ def test_update_service_data_retention_does_not_update_row_if_data_retention_is_ ('letter', 'sms')] ) def test_fetch_service_data_retention_by_notification_type(sample_service, notification_type, alternate): - data_retention = create_service_data_retention(service_id=sample_service.id, notification_type=notification_type) - create_service_data_retention(service_id=sample_service.id, notification_type=alternate) + data_retention = create_service_data_retention(service=sample_service, notification_type=notification_type) + create_service_data_retention(service=sample_service, notification_type=alternate) result = fetch_service_data_retention_by_notification_type(sample_service.id, notification_type) assert result == data_retention diff --git a/tests/app/db.py b/tests/app/db.py index 749462eb2..50a3a8560 100644 --- a/tests/app/db.py +++ b/tests/app/db.py @@ -806,12 +806,12 @@ def ses_notification_callback(): def create_service_data_retention( - service_id, + service, notification_type='sms', days_of_retention=3 ): data_retention = insert_service_data_retention( - service_id=service_id, + service_id=service.id, notification_type=notification_type, days_of_retention=days_of_retention ) diff --git a/tests/app/inbound_sms/test_rest.py b/tests/app/inbound_sms/test_rest.py index 8e913a36a..7b249a800 100644 --- a/tests/app/inbound_sms/test_rest.py +++ b/tests/app/inbound_sms/test_rest.py @@ -113,7 +113,7 @@ def test_post_to_get_inbound_sms_for_service_respects_data_retention( too_old_date, returned_date ): - create_service_data_retention(sample_service.id, 'sms', days_of_retention) + create_service_data_retention(sample_service, 'sms', days_of_retention) create_inbound_sms(sample_service, created_at=too_old_date) returned_inbound = create_inbound_sms(sample_service, created_at=returned_date) @@ -217,7 +217,7 @@ def test_get_most_recent_inbound_sms_for_service_respects_data_retention( admin_request, sample_service ): - create_service_data_retention(sample_service.id, 'sms', 5) + create_service_data_retention(sample_service, 'sms', 5) for i in range(10): created = datetime.utcnow() - timedelta(days=i) create_inbound_sms(sample_service, user_number='44770090000{}'.format(i), created_at=created) @@ -240,7 +240,7 @@ def test_get_most_recent_inbound_sms_for_service_respects_data_retention_if_olde admin_request, sample_service ): - create_service_data_retention(sample_service.id, 'sms', 14) + create_service_data_retention(sample_service, 'sms', 14) create_inbound_sms(sample_service, created_at=datetime(2017, 4, 1, 12, 0)) response = admin_request.get('inbound_sms.get_most_recent_inbound_sms_for_service', service_id=sample_service.id) @@ -254,7 +254,7 @@ def test_get_inbound_sms_for_service_respects_data_retention( admin_request, sample_service ): - create_service_data_retention(sample_service.id, 'sms', 5) + create_service_data_retention(sample_service, 'sms', 5) for i in range(10): created = datetime.utcnow() - timedelta(days=i) create_inbound_sms(sample_service, user_number='44770090000{}'.format(i), created_at=created) diff --git a/tests/app/service/test_service_data_retention_rest.py b/tests/app/service/test_service_data_retention_rest.py index 7797f62cb..dbd7fe196 100644 --- a/tests/app/service/test_service_data_retention_rest.py +++ b/tests/app/service/test_service_data_retention_rest.py @@ -7,10 +7,10 @@ from tests.app.db import create_service_data_retention def test_get_service_data_retention(client, sample_service): - sms_data_retention = create_service_data_retention(service_id=sample_service.id) - email_data_retention = create_service_data_retention(service_id=sample_service.id, notification_type='email', + sms_data_retention = create_service_data_retention(service=sample_service) + email_data_retention = create_service_data_retention(service=sample_service, notification_type='email', days_of_retention=10) - letter_data_retention = create_service_data_retention(service_id=sample_service.id, notification_type='letter', + letter_data_retention = create_service_data_retention(service=sample_service, notification_type='letter', days_of_retention=30) response = client.get( @@ -36,7 +36,7 @@ def test_get_service_data_retention_returns_empty_list(client, sample_service): def test_get_data_retention_for_service_notification_type(client, sample_service): - data_retention = create_service_data_retention(service_id=sample_service.id) + data_retention = create_service_data_retention(service=sample_service) response = client.get('/service/{}/data-retention/notification-type/{}'.format(sample_service.id, 'sms'), headers=[('Content-Type', 'application/json'), create_authorization_header()], ) @@ -45,10 +45,10 @@ def test_get_data_retention_for_service_notification_type(client, sample_service def test_get_service_data_retention_by_id(client, sample_service): - sms_data_retention = create_service_data_retention(service_id=sample_service.id) - create_service_data_retention(service_id=sample_service.id, notification_type='email', + sms_data_retention = create_service_data_retention(service=sample_service) + create_service_data_retention(service=sample_service, notification_type='email', days_of_retention=10) - create_service_data_retention(service_id=sample_service.id, notification_type='letter', + create_service_data_retention(service=sample_service, notification_type='letter', days_of_retention=30) response = client.get( '/service/{}/data-retention/{}'.format(str(sample_service.id), sms_data_retention.id), @@ -105,7 +105,7 @@ def test_create_service_data_retention_returns_400_when_notification_type_is_inv def test_create_service_data_retention_returns_400_when_data_retention_for_notification_type_already_exists( client, sample_service ): - create_service_data_retention(service_id=sample_service.id) + create_service_data_retention(service=sample_service) data = { "notification_type": "sms", "days_of_retention": 3 @@ -123,7 +123,7 @@ def test_create_service_data_retention_returns_400_when_data_retention_for_notif def test_modify_service_data_retention(client, sample_service): - data_retention = create_service_data_retention(service_id=sample_service.id) + data_retention = create_service_data_retention(service=sample_service) data = { "days_of_retention": 3 }