diff --git a/.gitignore b/.gitignore index e92684323..1c6c16612 100644 --- a/.gitignore +++ b/.gitignore @@ -61,4 +61,6 @@ target/ # Mac *.DS_Store -environment.sh \ No newline at end of file +environment.sh + +celerybeat-schedule \ No newline at end of file diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 90feed5e8..b3f090127 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -7,10 +7,18 @@ from app.dao.templates_dao import dao_get_template_by_id from app.dao.notifications_dao import ( dao_create_notification, dao_update_notification, + delete_failed_notifications_created_more_than_a_week_ago, + delete_successful_notifications_created_more_than_a_day_ago, dao_get_notification_statistics_for_service_and_day ) from app.dao.jobs_dao import dao_update_job, dao_get_job_by_id -from app.models import Notification, TEMPLATE_TYPE_EMAIL, TEMPLATE_TYPE_SMS +from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago +from app.dao.invited_user_dao import delete_invitations_created_more_than_two_days_ago +from app.models import ( + Notification, + TEMPLATE_TYPE_EMAIL, + TEMPLATE_TYPE_SMS +) from flask import current_app from sqlalchemy.exc import SQLAlchemyError from app.aws import s3 @@ -19,6 +27,66 @@ from utils.template import Template from utils.recipients import RecipientCSV +@notify_celery.task(name="delete-verify-codes") +def delete_verify_codes(): + try: + start = datetime.utcnow() + deleted = delete_codes_older_created_more_than_a_day_ago() + current_app.logger.info( + "Delete job started {} finished {} deleted {} verify codes".format(start, datetime.utcnow(), deleted) + ) + except SQLAlchemyError: + current_app.logger.info("Failed to delete verify codes") + raise + + +@notify_celery.task(name="delete-successful-notifications") +def delete_successful_notifications(): + try: + start = datetime.utcnow() + deleted = delete_successful_notifications_created_more_than_a_day_ago() + current_app.logger.info( + "Delete job started {} finished {} deleted {} successful notifications".format( + start, + datetime.utcnow(), + deleted + ) + ) + except SQLAlchemyError: + current_app.logger.info("Failed to delete successful notifications") + raise + + +@notify_celery.task(name="delete-failed-notifications") +def delete_failed_notifications(): + try: + start = datetime.utcnow() + deleted = delete_failed_notifications_created_more_than_a_week_ago() + current_app.logger.info( + "Delete job started {} finished {} deleted {} failed notifications".format( + start, + datetime.utcnow(), + deleted + ) + ) + except SQLAlchemyError: + current_app.logger.info("Failed to delete failed notifications") + raise + + +@notify_celery.task(name="delete-invitations") +def delete_invitations(): + try: + start = datetime.utcnow() + deleted = delete_invitations_created_more_than_two_days_ago() + current_app.logger.info( + "Delete job started {} finished {} deleted {} invitations".format(start, datetime.utcnow(), deleted) + ) + except SQLAlchemyError: + current_app.logger.info("Failed to delete invitations") + raise + + @notify_celery.task(name="process-job") def process_job(job_id): start = datetime.utcnow() @@ -52,9 +120,9 @@ def process_job(job_id): ) for recipient, personalisation in RecipientCSV( - s3.get_job_from_s3(job.bucket_name, job_id), - template_type=template.template_type, - placeholders=template.placeholders + s3.get_job_from_s3(job.bucket_name, job_id), + template_type=template.template_type, + placeholders=template.placeholders ).recipients_and_personalisation: encrypted = encryption.encrypt({ diff --git a/app/dao/invited_user_dao.py b/app/dao/invited_user_dao.py index 18e3c36ed..62fb0d25d 100644 --- a/app/dao/invited_user_dao.py +++ b/app/dao/invited_user_dao.py @@ -1,3 +1,4 @@ +from datetime import datetime, timedelta from app import db from app.models import InvitedUser @@ -18,3 +19,11 @@ def get_invited_user_by_id(invited_user_id): def get_invited_users_for_service(service_id): return InvitedUser.query.filter_by(service_id=service_id).all() + + +def delete_invitations_created_more_than_two_days_ago(): + deleted = db.session.query(InvitedUser).filter( + InvitedUser.created_at <= datetime.utcnow() - timedelta(days=2) + ).delete() + db.session.commit() + return deleted diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 825e2ead6..95c3fb54d 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -2,7 +2,7 @@ from flask import current_app from app import db from app.models import Notification, Job, NotificationStatistics, TEMPLATE_TYPE_SMS, TEMPLATE_TYPE_EMAIL from sqlalchemy import desc -from datetime import datetime +from datetime import datetime, timedelta def dao_get_notification_statistics_for_service(service_id): @@ -92,3 +92,21 @@ def get_notifications_for_service(service_id, page=1): per_page=current_app.config['PAGE_SIZE'] ) return query + + +def delete_successful_notifications_created_more_than_a_day_ago(): + deleted = db.session.query(Notification).filter( + Notification.created_at < datetime.utcnow() - timedelta(days=1), + Notification.status == 'sent' + ).delete() + db.session.commit() + return deleted + + +def delete_failed_notifications_created_more_than_a_week_ago(): + deleted = db.session.query(Notification).filter( + Notification.created_at < datetime.utcnow() - timedelta(days=7), + Notification.status == 'failed' + ).delete() + db.session.commit() + return deleted diff --git a/app/dao/users_dao.py b/app/dao/users_dao.py index cfaf4cdb8..be05ed89d 100644 --- a/app/dao/users_dao.py +++ b/app/dao/users_dao.py @@ -47,6 +47,14 @@ def get_user_code(user, code, code_type): return retval +def delete_codes_older_created_more_than_a_day_ago(): + deleted = db.session.query(VerifyCode).filter( + VerifyCode.created_at < datetime.utcnow() - timedelta(hours=24) + ).delete() + db.session.commit() + return deleted + + def use_user_code(id): verify_code = VerifyCode.query.get(id) verify_code.code_used = True diff --git a/app/notifications/rest.py b/app/notifications/rest.py index bf8da9efd..1fc35fe8e 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -133,6 +133,7 @@ def send_notification(notification_type): service_id, datetime.utcnow().strftime(DATE_FORMAT) ) + if service_stats: total_sms_count = service_stats.sms_requested total_email_count = service_stats.emails_requested diff --git a/config.py b/config.py index 617ae23dd..c2b6ce984 100644 --- a/config.py +++ b/config.py @@ -1,3 +1,4 @@ +from datetime import timedelta from kombu import Exchange, Queue import os @@ -29,19 +30,37 @@ class Config(object): 'region': 'eu-west-1', 'polling_interval': 1, # 1 second 'visibility_timeout': 60, # 60 seconds - 'queue_name_prefix': os.environ['NOTIFICATION_QUEUE_PREFIX']+'-' + 'queue_name_prefix': os.environ['NOTIFICATION_QUEUE_PREFIX'] + '-' } CELERY_ENABLE_UTC = True, CELERY_TIMEZONE = 'Europe/London' CELERY_ACCEPT_CONTENT = ['json'] CELERY_TASK_SERIALIZER = 'json' - # CELERYBEAT_SCHEDULE = { - # 'refresh-queues': { - # 'task': 'refresh-services', - # 'schedule': timedelta(seconds=5) - # } - # } + CELERY_IMPORTS = ('app.celery.tasks',) + CELERYBEAT_SCHEDULE = { + 'delete-verify-codes': { + 'task': 'delete-verify-codes', + 'schedule': timedelta(minutes=63), + 'options': {'queue': 'periodic'} + }, + 'delete-invitations': { + 'task': 'delete-invitations', + 'schedule': timedelta(minutes=66), + 'options': {'queue': 'periodic'} + }, + 'delete-failed-notifications': { + 'task': 'delete-failed-notifications', + 'schedule': timedelta(minutes=60), + 'options': {'queue': 'periodic'} + }, + 'delete-successful-notifications': { + 'task': 'delete-successful-notifications', + 'schedule': timedelta(minutes=31), + 'options': {'queue': 'periodic'} + } + } CELERY_QUEUES = [ + Queue('periodic', Exchange('default'), routing_key='periodic'), Queue('sms', Exchange('default'), routing_key='sms'), Queue('email', Exchange('default'), routing_key='email'), Queue('sms-code', Exchange('default'), routing_key='sms-code'), @@ -52,7 +71,6 @@ class Config(object): Queue('bulk-email', Exchange('default'), routing_key='bulk-email'), Queue('email-invited-user', Exchange('default'), routing_key='email-invited-user') ] - CELERY_IMPORTS = ('app.celery.tasks',) TWILIO_ACCOUNT_SID = os.getenv('TWILIO_ACCOUNT_SID') TWILIO_AUTH_TOKEN = os.getenv('TWILIO_AUTH_TOKEN') TWILIO_NUMBER = os.getenv('TWILIO_NUMBER') diff --git a/scripts/run_celery_beat.sh b/scripts/run_celery_beat.sh new file mode 100755 index 000000000..1d5278e0d --- /dev/null +++ b/scripts/run_celery_beat.sh @@ -0,0 +1,6 @@ +#!/bin/bash + +set -e + +source environment.sh +celery -A run_celery.notify_celery beat --loglevel=INFO diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 72edc4cf6..bb2a7bb3b 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -1,13 +1,19 @@ import uuid import pytest from flask import current_app -from app.celery.tasks import (send_sms, - send_sms_code, - send_email_code, - send_email, - process_job, - email_invited_user, - email_reset_password) +from app.celery.tasks import ( + send_sms, + send_sms_code, + send_email_code, + send_email, + process_job, + email_invited_user, + email_reset_password, + delete_verify_codes, + delete_invitations, + delete_failed_notifications, + delete_successful_notifications +) from app import (firetext_client, aws_ses_client, encryption, DATETIME_FORMAT) from app.clients.email.aws_ses import AwsSesClientException from app.clients.sms.firetext import FiretextClientException @@ -29,6 +35,30 @@ from tests.app.conftest import ( ) +def test_should_call_delete_successful_notifications_in_task(notify_api, mocker): + mocker.patch('app.celery.tasks.delete_successful_notifications_created_more_than_a_day_ago') + delete_successful_notifications() + assert tasks.delete_successful_notifications_created_more_than_a_day_ago.call_count == 1 + + +def test_should_call_delete_failed_notifications_in_task(notify_api, mocker): + mocker.patch('app.celery.tasks.delete_failed_notifications_created_more_than_a_week_ago') + delete_failed_notifications() + assert tasks.delete_failed_notifications_created_more_than_a_week_ago.call_count == 1 + + +def test_should_call_delete_codes_on_delete_verify_codes_task(notify_api, mocker): + mocker.patch('app.celery.tasks.delete_codes_older_created_more_than_a_day_ago') + delete_verify_codes() + assert tasks.delete_codes_older_created_more_than_a_day_ago.call_count == 1 + + +def test_should_call_delete_invotations_on_delete_invitations_task(notify_api, mocker): + mocker.patch('app.celery.tasks.delete_invitations_created_more_than_two_days_ago') + delete_invitations() + assert tasks.delete_invitations_created_more_than_two_days_ago.call_count == 1 + + @freeze_time("2016-01-01 11:09:00.061258") def test_should_process_sms_job(sample_job, mocker): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('sms')) @@ -266,7 +296,6 @@ def test_should_send_sms_without_personalisation(sample_template, mocker): def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notify_db_session, mocker): - user = sample_user(notify_db, notify_db_session, mobile_numnber="+441234123123") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template(notify_db, notify_db_session, service=service) @@ -292,7 +321,6 @@ def test_should_send_sms_if_restricted_service_and_valid_number(notify_db, notif def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, notify_db_session, mocker): - user = sample_user(notify_db, notify_db_session, mobile_numnber="+441234123123") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template(notify_db, notify_db_session, service=service) @@ -318,7 +346,6 @@ def test_should_not_send_sms_if_restricted_service_and_invalid_number(notify_db, def test_should_send_email_if_restricted_service_and_valid_email(notify_db, notify_db_session, mocker): - user = sample_user(notify_db, notify_db_session, email="test@restricted.com") service = sample_service(notify_db, notify_db_session, user=user, restricted=True) template = sample_template(notify_db, notify_db_session, service=service) @@ -418,7 +445,7 @@ def test_should_use_email_template_and_persist(sample_email_template_with_placeh def test_should_use_email_template_and_persist_without_personalisation( - sample_email_template, mocker + sample_email_template, mocker ): mocker.patch('app.encryption.decrypt', return_value={ "template": sample_email_template.id, diff --git a/tests/app/conftest.py b/tests/app/conftest.py index 58f9f1de8..ca637ae0b 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -280,7 +280,9 @@ def sample_notification(notify_db, service=None, template=None, job=None, - to_field=None): + to_field=None, + status='sent', + created_at=datetime.utcnow()): if service is None: service = sample_service(notify_db, notify_db_session) if template is None: @@ -302,7 +304,8 @@ def sample_notification(notify_db, 'service_id': service.id, 'service': service, 'template': template, - 'created_at': datetime.utcnow() + 'status': status, + 'created_at': created_at } notification = Notification(**data) dao_create_notification(notification, template.template_type) diff --git a/tests/app/dao/test_invited_user_dao.py b/tests/app/dao/test_invited_user_dao.py index 726cac1d9..0a3772ed7 100644 --- a/tests/app/dao/test_invited_user_dao.py +++ b/tests/app/dao/test_invited_user_dao.py @@ -1,4 +1,6 @@ +from datetime import datetime, timedelta import uuid +from app import db from app.models import InvitedUser @@ -6,7 +8,8 @@ from app.dao.invited_user_dao import ( save_invited_user, get_invited_user, get_invited_users_for_service, - get_invited_user_by_id + get_invited_user_by_id, + delete_invitations_created_more_than_two_days_ago ) @@ -52,7 +55,6 @@ def test_get_unknown_invited_user_returns_none(notify_db, notify_db_session, sam def test_get_invited_users_for_service(notify_db, notify_db_session, sample_service): - from tests.app.conftest import sample_invited_user invites = [] for i in range(0, 5): @@ -71,7 +73,6 @@ def test_get_invited_users_for_service(notify_db, notify_db_session, sample_serv def test_get_invited_users_for_service_that_has_no_invites(notify_db, notify_db_session, sample_service): - invites = get_invited_users_for_service(sample_service.id) assert len(invites) == 0 @@ -85,3 +86,40 @@ def test_save_invited_user_sets_status_to_cancelled(notify_db, notify_db_session assert InvitedUser.query.count() == 1 cancelled_invited_user = InvitedUser.query.get(sample_invited_user.id) assert cancelled_invited_user.status == 'cancelled' + + +def test_should_delete_all_invitations_more_than_one_day_old( + sample_user, + sample_service): + make_invitation(sample_user, sample_service, age=timedelta(hours=48)) + make_invitation(sample_user, sample_service, age=timedelta(hours=48)) + assert len(InvitedUser.query.all()) == 2 + delete_invitations_created_more_than_two_days_ago() + assert len(InvitedUser.query.all()) == 0 + + +def test_should_not_delete_invitations_less_than_two_days_old( + sample_user, + sample_service): + make_invitation(sample_user, sample_service, age=timedelta(hours=47, minutes=59, seconds=59), + email_address="valid@2.com") + make_invitation(sample_user, sample_service, age=timedelta(hours=48), + email_address="expired@1.com") + + assert len(InvitedUser.query.all()) == 2 + delete_invitations_created_more_than_two_days_ago() + assert len(InvitedUser.query.all()) == 1 + assert InvitedUser.query.first().email_address == "valid@2.com" + + +def make_invitation(user, service, age=timedelta(hours=0), email_address="test@test.com"): + verify_code = InvitedUser( + email_address=email_address, + from_user=user, + service=service, + status='pending', + created_at=datetime.utcnow() - age, + permissions='manage_settings' + ) + db.session.add(verify_code) + db.session.commit() diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index f3df4deb4..e3c32e05c 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -14,9 +14,12 @@ from app.dao.notifications_dao import ( get_notification_for_job, get_notifications_for_job, dao_get_notification_statistics_for_service, + delete_successful_notifications_created_more_than_a_day_ago, + delete_failed_notifications_created_more_than_a_week_ago, dao_get_notification_statistics_for_service_and_day ) from tests.app.conftest import sample_job +from tests.app.conftest import sample_notification def test_should_be_able_to_get_statistics_for_a_service(sample_template): @@ -531,3 +534,44 @@ def test_update_notification(sample_notification, sample_template): dao_update_notification(sample_notification) notification_from_db = Notification.query.get(sample_notification.id) assert notification_from_db.status == 'failed' + + +def test_should_delete_sent_notifications_after_one_day(notify_db, notify_db_session): + created_at = datetime.utcnow() - timedelta(hours=24) + sample_notification(notify_db, notify_db_session, created_at=created_at) + sample_notification(notify_db, notify_db_session, created_at=created_at) + assert len(Notification.query.all()) == 2 + delete_successful_notifications_created_more_than_a_day_ago() + assert len(Notification.query.all()) == 0 + + +def test_should_delete_failed_notifications_after_seven_days(notify_db, notify_db_session): + created_at = datetime.utcnow() - timedelta(hours=24 * 7) + sample_notification(notify_db, notify_db_session, created_at=created_at, status="failed") + sample_notification(notify_db, notify_db_session, created_at=created_at, status="failed") + assert len(Notification.query.all()) == 2 + delete_failed_notifications_created_more_than_a_week_ago() + assert len(Notification.query.all()) == 0 + + +def test_should_not_delete_sent_notifications_before_one_day(notify_db, notify_db_session): + expired = datetime.utcnow() - timedelta(hours=24) + valid = datetime.utcnow() - timedelta(hours=23, minutes=59, seconds=59) + sample_notification(notify_db, notify_db_session, created_at=expired, to_field="expired") + sample_notification(notify_db, notify_db_session, created_at=valid, to_field="valid") + + assert len(Notification.query.all()) == 2 + delete_successful_notifications_created_more_than_a_day_ago() + assert len(Notification.query.all()) == 1 + assert Notification.query.first().to == 'valid' + + +def test_should_not_delete_failed_notifications_before_seven_days(notify_db, notify_db_session): + expired = datetime.utcnow() - timedelta(hours=24 * 7) + valid = datetime.utcnow() - timedelta(hours=(24 * 6) + 23, minutes=59, seconds=59) + sample_notification(notify_db, notify_db_session, created_at=expired, status="failed", to_field="expired") + sample_notification(notify_db, notify_db_session, created_at=valid, status="failed", to_field="valid") + assert len(Notification.query.all()) == 2 + delete_failed_notifications_created_more_than_a_week_ago() + assert len(Notification.query.all()) == 1 + assert Notification.query.first().to == 'valid' diff --git a/tests/app/dao/test_users_dao.py b/tests/app/dao/test_users_dao.py index 7470d0b82..7be1b2ef3 100644 --- a/tests/app/dao/test_users_dao.py +++ b/tests/app/dao/test_users_dao.py @@ -1,5 +1,6 @@ +from datetime import datetime, timedelta from sqlalchemy.exc import DataError - +from app import db import pytest from app.dao.users_dao import ( @@ -8,11 +9,12 @@ from app.dao.users_dao import ( delete_model_user, increment_failed_login_count, reset_failed_login_count, - get_user_by_email + get_user_by_email, + delete_codes_older_created_more_than_a_day_ago ) from tests.app.conftest import sample_user as create_sample_user -from app.models import User +from app.models import User, VerifyCode def test_create_user(notify_api, notify_db, notify_db_session): @@ -86,7 +88,37 @@ def test_reset_failed_login_should_set_failed_logins_to_0(notify_api, notify_db, assert sample_user.failed_login_count == 0 -def test_get_user_by_email(notify_api, notify_db, notify_db_session, sample_user): +def test_get_user_by_email(sample_user): email = sample_user.email_address user_from_db = get_user_by_email(email) assert sample_user == user_from_db + + +def test_should_delete_all_verification_codes_more_than_one_day_old(sample_user): + make_verify_code(sample_user, age=timedelta(hours=24), code="54321") + make_verify_code(sample_user, age=timedelta(hours=24), code="54321") + assert len(VerifyCode.query.all()) == 2 + delete_codes_older_created_more_than_a_day_ago() + assert len(VerifyCode.query.all()) == 0 + + +def test_should_not_delete_verification_codes_less_than_one_day_old(sample_user): + make_verify_code(sample_user, age=timedelta(hours=23, minutes=59, seconds=59), code="12345") + make_verify_code(sample_user, age=timedelta(hours=24), code="54321") + + assert len(VerifyCode.query.all()) == 2 + delete_codes_older_created_more_than_a_day_ago() + assert len(VerifyCode.query.all()) == 1 + assert VerifyCode.query.first()._code == "12345" + + +def make_verify_code(user, age=timedelta(hours=0), code="12335"): + verify_code = VerifyCode( + code_type='sms', + _code=code, + created_at=datetime.utcnow() - age, + expiry_datetime=datetime.utcnow(), + user=user + ) + db.session.add(verify_code) + db.session.commit() diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index e7296fbc4..cdc7c1957 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -1,3 +1,4 @@ +from datetime import datetime import uuid import app.celery.tasks from tests import create_authorization_header @@ -126,9 +127,15 @@ def test_get_all_notifications_for_job_in_order(notify_api, notify_db, notify_db main_job = sample_job(notify_db, notify_db_session, service=sample_service) another_job = sample_job(notify_db, notify_db_session, service=sample_service) - notification_1 = sample_notification(notify_db, notify_db_session, job=main_job, to_field="1") - notification_2 = sample_notification(notify_db, notify_db_session, job=main_job, to_field="2") - notification_3 = sample_notification(notify_db, notify_db_session, job=main_job, to_field="3") + notification_1 = sample_notification( + notify_db, notify_db_session, job=main_job, to_field="1", created_at=datetime.utcnow() + ) + notification_2 = sample_notification( + notify_db, notify_db_session, job=main_job, to_field="2", created_at=datetime.utcnow() + ) + notification_3 = sample_notification( + notify_db, notify_db_session, job=main_job, to_field="3", created_at=datetime.utcnow() + ) sample_notification(notify_db, notify_db_session, job=another_job) auth_header = create_authorization_header( @@ -760,7 +767,9 @@ def test_should_block_api_call_if_over_day_limit(notify_db, notify_db_session, n service = sample_service(notify_db, notify_db_session, limit=1) email_template = sample_email_template(notify_db, notify_db_session, service=service) - sample_notification(notify_db, notify_db_session, template=email_template, service=service) + sample_notification( + notify_db, notify_db_session, template=email_template, service=service, created_at=datetime.utcnow() + ) data = { 'to': 'ok@ok.com', @@ -794,7 +803,9 @@ def test_should_block_api_call_if_over_day_limit_regardless_of_type(notify_db, n service = sample_service(notify_db, notify_db_session, limit=1) email_template = sample_email_template(notify_db, notify_db_session, service=service) sms_template = sample_template(notify_db, notify_db_session, service=service) - sample_notification(notify_db, notify_db_session, template=email_template, service=service) + sample_notification( + notify_db, notify_db_session, template=email_template, service=service, created_at=datetime.utcnow() + ) data = { 'to': '+447234123123',