From 0def0b7fd089aaa62bcc3892ddab25771e49182b Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 29 Apr 2019 15:44:42 +0100 Subject: [PATCH] We want to staop inserting and updating NotificationHistory each time we insert/update Notification. This PR adds a function to upsert (insert or update if exists) NotificationHistory all the rows from Notification that we are about to delete in the nightly task. This will happen just before the delete function. Since it is a upsert query the function can be called more than once. This should allow us remove all the insert/updates to NotificationHistory. However, there is a consern that this will double the length of time the tasks take. So do we do these upserts in a separate task or in the same one? --- app/dao/notifications_dao.py | 53 ++++++- ...t_notification_dao_delete_notifications.py | 142 +++++++++++++----- 2 files changed, 160 insertions(+), 35 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index e99b5f1d1..9d28ceeb8 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -19,6 +19,7 @@ from sqlalchemy import (desc, func, asc) from sqlalchemy.orm import joinedload from sqlalchemy.sql import functions from sqlalchemy.sql.expression import case +from sqlalchemy.dialects.postgresql import insert from werkzeug.datastructures import MultiDict from app import db, create_uuid @@ -310,6 +311,8 @@ def delete_notifications_older_than_retention_by_type(notification_type, qry_lim notification_type, f.service_id, days_of_retention, qry_limit ) + insert_update_notification_history(notification_type, days_of_retention, f.service_id) + current_app.logger.info( "Deleting {} notifications for service id: {}".format(notification_type, f.service_id)) deleted += _delete_notifications( @@ -328,7 +331,7 @@ def delete_notifications_older_than_retention_by_type(notification_type, qry_lim _delete_letters_from_s3( notification_type, service_id, seven_days_ago, qry_limit ) - + insert_update_notification_history(notification_type, seven_days_ago, service_id) deleted += _delete_notifications( deleted, notification_type, seven_days_ago, service_id, qry_limit ) @@ -361,6 +364,54 @@ def _delete_notifications( return deleted +def insert_update_notification_history(notification_type, date_to_delete_from, service_id): + notifications = db.session.query( + Notification.id, + Notification.job_id, + Notification.job_row_number, + Notification.service_id, + Notification.template_id, + Notification.template_version, + Notification.api_key_id, + Notification.key_type, + Notification.billable_units, + Notification.notification_type, + Notification.created_at, + Notification.sent_at, + Notification.sent_by, + Notification.updated_at, + Notification.status, + Notification.reference, + Notification.client_reference, + Notification.international, + Notification.phone_prefix, + Notification.rate_multiplier, + Notification.created_by_id, + Notification.postage + ).filter( + Notification.notification_type == notification_type, + Notification.service_id == service_id, + Notification.created_at < date_to_delete_from, + Notification.key_type != KEY_TYPE_TEST + ).all() + + stmt = insert(NotificationHistory).values( + notifications + ) + + stmt = stmt.on_conflict_do_update( + constraint="notification_history_pkey", + set_={"notification_status": stmt.excluded.status, + "billable_units": stmt.excluded.billable_units, + "updated_at": stmt.excluded.updated_at, + "sent_at": stmt.excluded.sent_at, + "sent_by": stmt.excluded.sent_by + } + ) + db.session.connection().execute(stmt) + db.session.commit() + + def _delete_letters_from_s3( notification_type, service_id, date_to_delete_from, query_limit ): 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 3e913a673..a60675179 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,12 @@ from datetime import ( import pytest from flask import current_app from freezegun import freeze_time -from app.dao.notifications_dao import delete_notifications_older_than_retention_by_type + +from app import db +from app.dao.notifications_dao import ( + delete_notifications_older_than_retention_by_type, + insert_update_notification_history +) from app.models import Notification, NotificationHistory from tests.app.db import ( create_template, @@ -85,6 +90,34 @@ def test_should_not_delete_notification_history(sample_service, notification_typ @pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) def test_delete_notifications_for_days_of_retention(sample_service, notification_type, mocker): mock_get_s3 = mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") + create_test_data(notification_type, sample_service) + assert len(Notification.query.all()) == 9 + 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': + mock_get_s3.assert_called_with(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + subfolder="{}/NOTIFY.LETTER_REF.D.2.C.C".format(str(datetime.utcnow().date())) + ) + assert mock_get_s3.call_count == 2 + else: + mock_get_s3.assert_not_called() + + +@pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) +def test_delete_notifications_inserts_notification_history(sample_service, notification_type, mocker): + mocker.patch("app.dao.notifications_dao.get_s3_bucket_objects") + create_test_data(notification_type, sample_service) + NotificationHistory.query.delete() + assert len(Notification.query.all()) == 9 + delete_notifications_older_than_retention_by_type(notification_type) + assert len(Notification.query.all()) == 7 + + history = NotificationHistory.query.all() + assert len(history) == 2 + + +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( @@ -107,44 +140,14 @@ def test_delete_notifications_for_days_of_retention(sample_service, notification 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) - assert len(Notification.query.all()) == 9 - 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': - mock_get_s3.assert_called_with(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], - subfolder="{}/NOTIFY.LETTER_REF.D.2.C.C".format(str(datetime.utcnow().date())) - ) - assert mock_get_s3.call_count == 2 - else: - mock_get_s3.assert_not_called() + 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") - create_service_data_retention(service_id=sample_service.id, notification_type=notification_type, - days_of_retention=15) - email_template, letter_template, sms_template = _create_templates(sample_service) - service_with_default_data_retention = create_service(service_name='default data retention') - 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') - create_notification(template=email_template, status='delivered', - created_at=datetime.utcnow() - timedelta(days=14)) - create_notification(template=sms_template, status='permanent-failure', - created_at=datetime.utcnow() - timedelta(days=14)) - create_notification(template=letter_template, status='temporary-failure', - created_at=datetime.utcnow() - timedelta(days=14)) - 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', - created_at=datetime.utcnow() - timedelta(days=8)) + create_test_data(notification_type, sample_service, 15) assert len(Notification.query.all()) == 9 delete_notifications_older_than_retention_by_type(notification_type) assert len(Notification.query.filter_by().all()) == 8 @@ -200,6 +203,77 @@ def test_delete_notifications_calls_subquery( assert Notification.query.count() == 0 +@pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) +def test_insert_update_notification_history(sample_service, notification_type): + template = create_template(sample_service, template_type=notification_type) + notification_1 = create_notification(template=template, created_at=datetime.utcnow() - timedelta(days=3)) + notification_2 = create_notification(template=template, created_at=datetime.utcnow() - timedelta(days=8)) + notification_3 = create_notification(template=template, created_at=datetime.utcnow() - timedelta(days=9)) + other_types = ['sms', 'email', 'letter'] + other_types.remove(notification_type) + for template_type in other_types: + t = create_template(service=sample_service, template_type=template_type) + create_notification(template=t, created_at=datetime.utcnow() - timedelta(days=3)) + create_notification(template=t, created_at=datetime.utcnow() - timedelta(days=8)) + + NotificationHistory.query.delete() + history = NotificationHistory.query.all() + assert len(history) == 0 + + insert_update_notification_history( + notification_type=notification_type, date_to_delete_from=datetime.utcnow() - timedelta(days=7), + service_id=sample_service.id) + history = NotificationHistory.query.all() + assert len(history) == 2 + + history_ids = [x.id for x in history] + assert notification_1.id not in history_ids + assert notification_2.id in history_ids + assert notification_3.id in history_ids + + +@pytest.mark.parametrize('notification_type', ['sms', 'email', 'letter']) +def test_insert_update_notification_history_only_insert_update_given_service(sample_service, notification_type): + other_service = create_service(service_name='another service') + other_template = create_template(service=other_service, template_type=notification_type) + template = create_template(service=sample_service, template_type=notification_type) + notification_1 = create_notification(template=template, created_at=datetime.utcnow() - timedelta(days=3)) + notification_2 = create_notification(template=template, created_at=datetime.utcnow() - timedelta(days=8)) + notification_3 = create_notification(template=other_template, created_at=datetime.utcnow() - timedelta(days=3)) + notification_4 = create_notification(template=other_template, created_at=datetime.utcnow() - timedelta(days=8)) + + NotificationHistory.query.delete() + history = NotificationHistory.query.all() + + assert len(history) == 0 + + insert_update_notification_history( + notification_type, datetime.utcnow() - timedelta(days=7), sample_service.id) + history = NotificationHistory.query.all() + assert len(history) == 1 + + history_ids = [x.id for x in history] + assert notification_1.id not in history_ids + assert notification_2.id in history_ids + assert notification_3.id not in history_ids + assert notification_4.id not in history_ids + + +def test_insert_update_notification_history_updates_history_with_new_status(sample_template): + notification_1 = create_notification(template=sample_template, created_at=datetime.utcnow() - timedelta(days=3)) + notification_2 = create_notification(template=sample_template, created_at=datetime.utcnow() - timedelta(days=8)) + notification_2.status = 'delivered' + db.session.add(notification_2) + db.session.commit() + insert_update_notification_history( + 'sms', datetime.utcnow() - timedelta(days=7), sample_template.service_id) + history_1 = NotificationHistory.query.get(notification_1.id) + assert history_1.id == notification_1.id + history_2 = NotificationHistory.query.get(notification_2.id) + assert history_2.id == notification_2.id + assert history_2.status == 'delivered' + + def _create_templates(sample_service): email_template = create_template(service=sample_service, template_type='email') sms_template = create_template(service=sample_service)