From b068a850fa9e0f97ab28c5642bea7f1d5ae80b23 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Mon, 7 Jan 2019 02:12:39 +0000 Subject: [PATCH 01/31] Update pytest-cov from 2.6.0 to 2.6.1 --- 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 8e750c8e0..50ddceb17 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -4,7 +4,7 @@ pytest==3.10.1 moto==1.3.7 pytest-env==0.6.2 pytest-mock==1.10.0 -pytest-cov==2.6.0 +pytest-cov==2.6.1 pytest-xdist==1.24.1 coveralls==1.5.1 freezegun==0.3.11 From 4a26ee18139dcef875d911f9f89141393a7cfd3e Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Wed, 9 Jan 2019 12:22:51 +0000 Subject: [PATCH 02/31] Set statement timeout on all DB connections A recent issue with a long-running query (#2288) highlighted the fact that even though the original HTTP connection might be closed (for example after gorouter timeout of 15 minutes, which returns a 504 response to the client), the request worker will not be stopped. This means that the worker is spending time and potentially DB resources generating a response that will never be delivered. Gunicorn's timeout setting only applies to sync workers and there doesn't seem to be an option to interrupt individual requests in gevent/eventlet deployments. Since the most likely (and potentially most dangerous) scenario for this is a long-running DB query, we can set a statement timeout on our DB connections. This will raise a sqlalchemy.exc.OperationalError (wrapping psycopg2.extensions.QueryCanceledError), interrupting the request after the given timeout has been reached. This is a Postgres client setting, so the database itself will abort the transaction when it reaches the set timeout. Since this will also apply to our celery tasks (including potentially long-running nightly tasks) we set a timeout of 20 minutes to begin with. This can potentially be split in the future to set a different value for each app, so that we could limit API requests even more. --- app/__init__.py | 15 ++++++++++++++- app/config.py | 1 + 2 files changed, 15 insertions(+), 1 deletion(-) diff --git a/app/__init__.py b/app/__init__.py index b4a470ac0..1f94c697e 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -4,7 +4,7 @@ import string import uuid from flask import _request_ctx_stack, request, g, jsonify -from flask_sqlalchemy import SQLAlchemy +from flask_sqlalchemy import SQLAlchemy as _SQLAlchemy from flask_marshmallow import Marshmallow from flask_migrate import Migrate from time import monotonic @@ -27,6 +27,19 @@ from app.encryption import Encryption DATETIME_FORMAT = "%Y-%m-%dT%H:%M:%S.%fZ" DATE_FORMAT = "%Y-%m-%d" + +class SQLAlchemy(_SQLAlchemy): + """We need to subclass SQLAlchemy in order to override create_engine options""" + + def apply_driver_hacks(self, app, info, options): + super().apply_driver_hacks(app, info, options) + if 'connect_args' not in options: + options['connect_args'] = {} + options['connect_args']["options"] = "-c statement_timeout={}".format( + int(app.config['SQLALCHEMY_STATEMENT_TIMEOUT']) * 1000 + ) + + db = SQLAlchemy() migrate = Migrate() ma = Marshmallow() diff --git a/app/config.py b/app/config.py index 138d18b65..a0569534e 100644 --- a/app/config.py +++ b/app/config.py @@ -122,6 +122,7 @@ class Config(object): SQLALCHEMY_POOL_SIZE = int(os.environ.get('SQLALCHEMY_POOL_SIZE', 5)) SQLALCHEMY_POOL_TIMEOUT = 30 SQLALCHEMY_POOL_RECYCLE = 300 + SQLALCHEMY_STATEMENT_TIMEOUT = 1200 PAGE_SIZE = 50 API_PAGE_SIZE = 250 TEST_MESSAGE_FILENAME = 'Test message' From 56bae2b07758ac2079013d5acef1c46eb1525a81 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 9 Jan 2019 17:49:19 +0000 Subject: [PATCH 03/31] Allow users to set postage per precompiled letter --- .../process_letter_notifications.py | 3 ++- app/notifications/process_notifications.py | 14 ++++++++----- app/v2/notifications/notification_schemas.py | 3 ++- .../test_post_letter_notifications.py | 21 ++++++++++++++----- 4 files changed, 29 insertions(+), 12 deletions(-) diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py index 984ad257e..94e52bbd8 100644 --- a/app/notifications/process_letter_notifications.py +++ b/app/notifications/process_letter_notifications.py @@ -20,6 +20,7 @@ def create_letter_notification(letter_data, template, api_key, status, reply_to_ client_reference=letter_data.get('reference'), status=status, reply_to_text=reply_to_text, - billable_units=billable_units + billable_units=billable_units, + postage=letter_data.get('postage') ) return notification diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 8fc2f15f6..88cec5b0d 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -75,7 +75,8 @@ def persist_notification( created_by_id=None, status=NOTIFICATION_CREATED, reply_to_text=None, - billable_units=None + billable_units=None, + postage=None ): notification_created_at = created_at or datetime.utcnow() if not notification_id: @@ -112,11 +113,14 @@ def persist_notification( elif notification_type == EMAIL_TYPE: notification.normalised_to = format_email_address(notification.to) elif notification_type == LETTER_TYPE: - template = dao_get_template_by_id(template_id, template_version) - if service.has_permission(CHOOSE_POSTAGE) and template.postage: - notification.postage = template.postage + if postage: + notification.postage = postage else: - notification.postage = service.postage + template = dao_get_template_by_id(template_id, template_version) + if service.has_permission(CHOOSE_POSTAGE) and template.postage: + notification.postage = template.postage + else: + notification.postage = service.postage # if simulated create a Notification model to return but do not persist the Notification to the dB if not simulated: diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 39c78d727..1ee7d1dc4 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -239,7 +239,8 @@ post_precompiled_letter_request = { "title": "POST v2/notifications/letter", "properties": { "reference": {"type": "string"}, - "content": {"type": "string"} + "content": {"type": "string"}, + "postage": {"type": "string"} }, "required": ["reference", "content"], "additionalProperties": False diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 734928088..163f63384 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -469,16 +469,27 @@ def test_post_precompiled_letter_with_invalid_base64(client, notify_user, mocker assert not Notification.query.first() -@pytest.mark.parametrize('postage', ['first', 'second']) -def test_post_precompiled_letter_notification_returns_201(client, notify_user, mocker, postage): +@pytest.mark.parametrize('service_postage, notification_postage, expected_postage', [ + ('second', 'second', 'second'), + ('second', 'first', 'first'), + ('second', None, 'second'), + ('first', 'first', 'first'), + ('first', 'second', 'second'), + ('first', None, 'first'), +]) +def test_post_precompiled_letter_notification_returns_201( + client, notify_user, mocker, service_postage, notification_postage, expected_postage +): sample_service = create_service(service_permissions=['letter', 'precompiled_letter']) - sample_service.postage = postage + sample_service.postage = service_postage s3mock = mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf') mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task') data = { "reference": "letter-reference", "content": "bGV0dGVyLWNvbnRlbnQ=" } + if notification_postage: + data["postage"] = notification_postage auth_header = create_authorization_header(service_id=sample_service.id) response = client.post( path="v2/notifications/letter", @@ -493,10 +504,10 @@ def test_post_precompiled_letter_notification_returns_201(client, notify_user, m assert notification.billable_units == 0 assert notification.status == NOTIFICATION_PENDING_VIRUS_CHECK - assert notification.postage == postage + assert notification.postage == expected_postage notification_history = NotificationHistory.query.one() - assert notification_history.postage == postage + assert notification_history.postage == expected_postage resp_json = json.loads(response.get_data(as_text=True)) assert resp_json == {'id': str(notification.id), 'reference': 'letter-reference'} From 5a1094b6fd961aeda11d51fa3f67c2dd5b559cd7 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 10 Jan 2019 16:04:06 +0000 Subject: [PATCH 04/31] Throw error if postage parameter for precompiled POST request incorrect --- app/schema_validation/__init__.py | 7 +++++++ app/v2/notifications/notification_schemas.py | 2 +- .../test_post_letter_notifications.py | 20 +++++++++++++++++++ 3 files changed, 28 insertions(+), 1 deletion(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index 382d9229c..dfa03446f 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -29,6 +29,13 @@ def validate(json_to_validate, schema): validate_email_address(instance) return True + @format_checker.checks('postage', raises=ValidationError) + def validate_schema_postage(instance): + if isinstance(instance, str): + if instance not in ["first", "second"]: + raise ValidationError("invalid. It must be either first or second.") + return True + @format_checker.checks('datetime_within_next_day', raises=ValidationError) def validate_schema_date_with_hour(instance): if isinstance(instance, str): diff --git a/app/v2/notifications/notification_schemas.py b/app/v2/notifications/notification_schemas.py index 1ee7d1dc4..733eb8aef 100644 --- a/app/v2/notifications/notification_schemas.py +++ b/app/v2/notifications/notification_schemas.py @@ -240,7 +240,7 @@ post_precompiled_letter_request = { "properties": { "reference": {"type": "string"}, "content": {"type": "string"}, - "postage": {"type": "string"} + "postage": {"type": "string", "format": "postage"} }, "required": ["reference", "content"], "additionalProperties": False diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index 163f63384..db6cd4159 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -511,3 +511,23 @@ def test_post_precompiled_letter_notification_returns_201( resp_json = json.loads(response.get_data(as_text=True)) assert resp_json == {'id': str(notification.id), 'reference': 'letter-reference'} + + +def test_post_letter_notification_throws_error_for_invalid_postage(client, notify_user, mocker): + sample_service = create_service(service_permissions=['letter', 'precompiled_letter']) + data = { + "reference": "letter-reference", + "content": "bGV0dGVyLWNvbnRlbnQ=", + "postage": "space unicorn" + } + auth_header = create_authorization_header(service_id=sample_service.id) + response = client.post( + path="v2/notifications/letter", + data=json.dumps(data), + headers=[('Content-Type', 'application/json'), auth_header]) + + assert response.status_code == 400, response.get_data(as_text=True) + resp_json = json.loads(response.get_data(as_text=True)) + assert resp_json['errors'][0]['message'] == "postage invalid. It must be either first or second." + + assert not Notification.query.first() From 507138cc94ea0757342d82c50c2710f4714cc196 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 10 Jan 2019 16:24:51 +0000 Subject: [PATCH 05/31] Create a new query for template monthly stats. --- app/dao/fact_notification_status_dao.py | 84 ++++++++++++++++++- app/service/rest.py | 15 +++- .../dao/test_fact_notification_status_dao.py | 29 ++++++- tests/app/service/test_statistics_rest.py | 18 +--- 4 files changed, 123 insertions(+), 23 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index bf6ec3c8e..7231dc7d8 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -4,12 +4,12 @@ from flask import current_app from notifications_utils.timezones import convert_bst_to_utc from sqlalchemy import func from sqlalchemy.dialects.postgresql import insert -from sqlalchemy.sql.expression import literal +from sqlalchemy.sql.expression import literal, extract from sqlalchemy.types import DateTime, Integer from app import db -from app.models import Notification, NotificationHistory, FactNotificationStatus, KEY_TYPE_TEST, Service -from app.utils import get_london_midnight_in_utc, midnight_n_days_ago +from app.models import Notification, NotificationHistory, FactNotificationStatus, KEY_TYPE_TEST, Service, Template +from app.utils import get_london_midnight_in_utc, midnight_n_days_ago, get_london_month_from_utc_column def fetch_notification_status_for_day(process_day, service_id=None): @@ -291,3 +291,81 @@ def fetch_stats_for_all_services_by_date_range(start_date, end_date, include_fro else: query = stats return query.all() + + +def fetch_monthly_template_usage_for_service(start_date, end_date, service_id): + # services_dao.replaces dao_fetch_monthly_historical_usage_by_template_for_service + stats = db.session.query( + FactNotificationStatus.template_id.label('template_id'), + Template.name.label('name'), + Template.template_type.label('template_type'), + Template.is_precompiled_letter.label('is_precompiled_letter'), + extract('month', FactNotificationStatus.bst_date).label('month'), + extract('year', FactNotificationStatus.bst_date).label('year'), + func.sum(FactNotificationStatus.notification_count).label('count') + ).join( + Template, FactNotificationStatus.template_id == Template.id + ).filter( + FactNotificationStatus.service_id == service_id, + FactNotificationStatus.bst_date >= start_date, + FactNotificationStatus.bst_date <= end_date, + ).group_by( + FactNotificationStatus.template_id, + Template.name, + Template.template_type, + Template.is_precompiled_letter, + extract('month', FactNotificationStatus.bst_date).label('month'), + extract('year', FactNotificationStatus.bst_date).label('year'), + ) + + if start_date <= datetime.utcnow() <= end_date: + today = get_london_midnight_in_utc(datetime.utcnow()) + month = get_london_month_from_utc_column(Notification.created_at) + + stats_for_today = db.session.query( + Notification.template_id.label('template_id'), + Template.name.label('name'), + Template.template_type.label('template_type'), + Template.is_precompiled_letter.label('is_precompiled_letter'), + extract('month', month).label('month'), + extract('year', month).label('year'), + func.count().label('count') + ).join( + Template, Notification.template_id == Template.id, + ).filter( + Notification.created_at >= today, + Notification.service_id == service_id, + # we don't want to include test keys + Notification.key_type != KEY_TYPE_TEST + ).group_by( + Notification.template_id, + Template.hidden, + Template.name, + Template.template_type, + month + ) + + all_stats_table = stats.union_all(stats_for_today).subquery() + query = db.session.query( + all_stats_table.c.template_id, + all_stats_table.c.name, + all_stats_table.c.is_precompiled_letter, + all_stats_table.c.template_type, + all_stats_table.c.month, + all_stats_table.c.year, + func.cast(func.sum(all_stats_table.c.count), Integer).label('count'), + ).group_by( + all_stats_table.c.template_id, + all_stats_table.c.name, + all_stats_table.c.is_precompiled_letter, + all_stats_table.c.template_type, + all_stats_table.c.month, + all_stats_table.c.year, + ).order_by( + all_stats_table.c.year, + all_stats_table.c.month, + all_stats_table.c.name + ) + else: + query = stats + return query.all() diff --git a/app/service/rest.py b/app/service/rest.py index 48fd6ae75..e00c99aa7 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -24,7 +24,8 @@ from app.dao.fact_notification_status_dao import ( fetch_notification_status_for_service_by_month, fetch_notification_status_for_service_for_day, fetch_notification_status_for_service_for_today_and_7_previous_days, - fetch_stats_for_all_services_by_date_range) + fetch_stats_for_all_services_by_date_range, fetch_monthly_template_usage_for_service +) from app.dao.inbound_numbers_dao import dao_allocate_number_for_service from app.dao.organisation_dao import dao_get_organisation_by_service_id from app.dao.service_data_retention_dao import ( @@ -579,10 +580,16 @@ def resume_service(service_id): @service_blueprint.route('//notifications/templates_usage/monthly', methods=['GET']) def get_monthly_template_usage(service_id): try: - data = dao_fetch_monthly_historical_usage_by_template_for_service( - service_id, - int(request.args.get('year', 'NaN')) + start_date, end_date = get_financial_year(int(request.args.get('year', 'NaN'))) + data = fetch_monthly_template_usage_for_service( + start_date=start_date, + end_date=end_date, + service_id=service_id ) + # data = dao_fetch_monthly_historical_usage_by_template_for_service( + # service_id, + # int(request.args.get('year', 'NaN')) + # ) stats = list() for i in data: diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index 64b6b5bc3..0e541f7a9 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -11,7 +11,8 @@ from app.dao.fact_notification_status_dao import ( fetch_notification_status_for_service_for_today_and_7_previous_days, fetch_notification_status_totals_for_all_services, fetch_notification_statuses_for_job, - fetch_stats_for_all_services_by_date_range) + fetch_stats_for_all_services_by_date_range, fetch_monthly_template_usage_for_service +) from app.models import FactNotificationStatus, KEY_TYPE_TEST, KEY_TYPE_TEAM, EMAIL_TYPE, SMS_TYPE, LETTER_TYPE from freezegun import freeze_time from tests.app.db import create_notification, create_service, create_template, create_ft_notification_status, create_job @@ -338,3 +339,29 @@ def test_fetch_stats_for_all_services_by_date_range(notify_db_session): assert not results[4].notification_type assert not results[4].status assert not results[4].count + + +def test_fetch_monthly_template_usage_for_service(sample_service): + template_one = create_template(service=sample_service, template_type='sms', template_name='one') + template_two = create_template(service=sample_service, template_type='email', template_name='one') + template_three = create_template(service=sample_service, template_type='letter', template_name='one') + + create_ft_notification_status(bst_date=date(2018, 1, 1), + service=sample_service, + template=template_one, + count=2) + create_ft_notification_status(bst_date=date(2018, 2, 1), + service=sample_service, + template=template_two, + count=3) + create_ft_notification_status(bst_date=date(2018, 3, 1), + service=sample_service, + template=template_three, + count=5) + + results = fetch_monthly_template_usage_for_service( + datetime(2017, 4, 1), datetime(2018, 3, 31), sample_service.id + ) + + print(results) + assert len(results) == 3 diff --git a/tests/app/service/test_statistics_rest.py b/tests/app/service/test_statistics_rest.py index 5b9719637..edf9e5b9a 100644 --- a/tests/app/service/test_statistics_rest.py +++ b/tests/app/service/test_statistics_rest.py @@ -28,13 +28,7 @@ def test_get_template_usage_by_month_returns_correct_data( admin_request, sample_template ): - create_notification(sample_template, created_at=datetime(2016, 4, 1), status='created') - create_notification(sample_template, created_at=datetime(2017, 4, 1), status='sending') - create_notification(sample_template, created_at=datetime(2017, 4, 1), status='permanent-failure') - create_notification(sample_template, created_at=datetime(2017, 4, 1), status='temporary-failure') - - daily_stats_template_usage_by_month() - + create_ft_notification_status(bst_date=date(2017, 4, 2), template=sample_template, count=3) create_notification(sample_template, created_at=datetime.utcnow()) resp_json = admin_request.get( @@ -85,14 +79,8 @@ def test_get_template_usage_by_month_returns_two_templates(admin_request, sample template_name=PRECOMPILED_TEMPLATE_NAME, hidden=True ) - - create_notification(template_one, created_at=datetime(2017, 4, 1), status='created') - create_notification(sample_template, created_at=datetime(2017, 4, 1), status='sending') - create_notification(sample_template, created_at=datetime(2017, 4, 1), status='permanent-failure') - create_notification(sample_template, created_at=datetime(2017, 4, 1), status='temporary-failure') - - daily_stats_template_usage_by_month() - + create_ft_notification_status(bst_date=datetime(2017, 4, 1), template=template_one, count=1) + create_ft_notification_status(bst_date=datetime(2017, 4, 1), template=sample_template, count=3) create_notification(sample_template, created_at=datetime.utcnow()) resp_json = admin_request.get( From 685bff40d1dc354b2a642f290929559cfd4f751d Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Thu, 10 Jan 2019 17:31:32 +0000 Subject: [PATCH 06/31] Stop validate function from being too complex by moving subfunctions out of it --- app/schema_validation/__init__.py | 88 +++++++++++++++++-------------- 1 file changed, 47 insertions(+), 41 deletions(-) diff --git a/app/schema_validation/__init__.py b/app/schema_validation/__init__.py index dfa03446f..98e67a50a 100644 --- a/app/schema_validation/__init__.py +++ b/app/schema_validation/__init__.py @@ -8,48 +8,54 @@ from notifications_utils.recipients import (validate_phone_number, validate_emai InvalidEmailError) +format_checker = FormatChecker() + + +@format_checker.checks("validate_uuid", raises=Exception) +def validate_uuid(instance): + if isinstance(instance, str): + UUID(instance) + return True + + +@format_checker.checks('phone_number', raises=InvalidPhoneError) +def validate_schema_phone_number(instance): + if isinstance(instance, str): + validate_phone_number(instance, international=True) + return True + + +@format_checker.checks('email_address', raises=InvalidEmailError) +def validate_schema_email_address(instance): + if isinstance(instance, str): + validate_email_address(instance) + return True + + +@format_checker.checks('postage', raises=ValidationError) +def validate_schema_postage(instance): + if isinstance(instance, str): + if instance not in ["first", "second"]: + raise ValidationError("invalid. It must be either first or second.") + return True + + +@format_checker.checks('datetime_within_next_day', raises=ValidationError) +def validate_schema_date_with_hour(instance): + if isinstance(instance, str): + try: + dt = iso8601.parse_date(instance).replace(tzinfo=None) + if dt < datetime.utcnow(): + raise ValidationError("datetime can not be in the past") + if dt > datetime.utcnow() + timedelta(hours=24): + raise ValidationError("datetime can only be 24 hours in the future") + except ParseError: + raise ValidationError("datetime format is invalid. It must be a valid ISO8601 date time format, " + "https://en.wikipedia.org/wiki/ISO_8601") + return True + + def validate(json_to_validate, schema): - format_checker = FormatChecker() - - @format_checker.checks("validate_uuid", raises=Exception) - def validate_uuid(instance): - if isinstance(instance, str): - UUID(instance) - return True - - @format_checker.checks('phone_number', raises=InvalidPhoneError) - def validate_schema_phone_number(instance): - if isinstance(instance, str): - validate_phone_number(instance, international=True) - return True - - @format_checker.checks('email_address', raises=InvalidEmailError) - def validate_schema_email_address(instance): - if isinstance(instance, str): - validate_email_address(instance) - return True - - @format_checker.checks('postage', raises=ValidationError) - def validate_schema_postage(instance): - if isinstance(instance, str): - if instance not in ["first", "second"]: - raise ValidationError("invalid. It must be either first or second.") - return True - - @format_checker.checks('datetime_within_next_day', raises=ValidationError) - def validate_schema_date_with_hour(instance): - if isinstance(instance, str): - try: - dt = iso8601.parse_date(instance).replace(tzinfo=None) - if dt < datetime.utcnow(): - raise ValidationError("datetime can not be in the past") - if dt > datetime.utcnow() + timedelta(hours=24): - raise ValidationError("datetime can only be 24 hours in the future") - except ParseError: - raise ValidationError("datetime format is invalid. It must be a valid ISO8601 date time format, " - "https://en.wikipedia.org/wiki/ISO_8601") - return True - validator = Draft7Validator(schema, format_checker=format_checker) errors = list(validator.iter_errors(json_to_validate)) if errors.__len__() > 0: From a9b755b08cc11da64905a4666b43d821783f6d9f Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 11 Jan 2019 09:23:05 +0000 Subject: [PATCH 07/31] Move letters which can't be opened to invalid PDF bucket If a precompiled letter can't be opened (e.g. because it isn't a valid PDF) we were setting its billable units to 0, but not moving it to the invalid PDF bucket. If a precompiled letter failed sanitisation, we were moving it to the invalid PDF bucket but not setting its billable units to 0. This commit makes sure that we always set the billable units to 0 and move the PDF to the right bucket if it fails sanitisation or can't be opened. --- app/celery/letters_pdf_tasks.py | 29 ++++++++------ tests/app/celery/test_letters_pdf_tasks.py | 45 ++++++++++++++++++---- 2 files changed, 55 insertions(+), 19 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 485f258bf..a57be4044 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -188,7 +188,12 @@ def process_virus_scan_passed(self, filename): scan_pdf_object = s3.get_s3_object(current_app.config['LETTERS_SCAN_BUCKET_NAME'], filename) old_pdf = scan_pdf_object.get()['Body'].read() - billable_units = _get_page_count(notification, old_pdf) + try: + billable_units = _get_page_count(notification, old_pdf) + except PdfReadError: + _move_invalid_letter_and_update_status(notification.reference, filename, scan_pdf_object) + return + new_pdf = _sanitise_precompiled_pdf(self, notification, old_pdf) # TODO: Remove this once CYSP update their template to not cross over the margins @@ -198,12 +203,7 @@ def process_virus_scan_passed(self, filename): if not new_pdf: current_app.logger.info('Invalid precompiled pdf received {} ({})'.format(notification.id, filename)) - - notification.status = NOTIFICATION_VALIDATION_FAILED - dao_update_notification(notification) - - move_scan_to_invalid_pdf_bucket(filename) - scan_pdf_object.delete() + _move_invalid_letter_and_update_status(notification.reference, filename, scan_pdf_object) return else: current_app.logger.info( @@ -233,14 +233,19 @@ def _get_page_count(notification, old_pdf): return billable_units except PdfReadError as e: current_app.logger.exception(msg='Invalid PDF received for notification_id: {}'.format(notification.id)) - update_letter_pdf_status( - reference=notification.reference, - status=NOTIFICATION_VALIDATION_FAILED, - billable_units=0 - ) raise e +def _move_invalid_letter_and_update_status(notification_reference, filename, scan_pdf_object): + move_scan_to_invalid_pdf_bucket(filename) + scan_pdf_object.delete() + + update_letter_pdf_status( + reference=notification_reference, + status=NOTIFICATION_VALIDATION_FAILED, + billable_units=0) + + def _upload_pdf_to_test_or_live_pdf_bucket(pdf_data, filename, is_test_letter): target_bucket_config = 'TEST_LETTERS_BUCKET_NAME' if is_test_letter else 'LETTERS_PDF_BUCKET_NAME' target_bucket_name = current_app.config[target_bucket_config] diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 13815ce05..ce7c9cd79 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/tests/app/celery/test_letters_pdf_tasks.py @@ -23,7 +23,6 @@ from app.celery.letters_pdf_tasks import ( process_virus_scan_failed, process_virus_scan_error, replay_letters_in_error, - _get_page_count, _sanitise_precompiled_pdf ) from app.letters.utils import get_letter_pdf_filename, ScanErrorType @@ -417,6 +416,7 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( process_virus_scan_passed(filename) assert sample_letter_notification.status == NOTIFICATION_VALIDATION_FAILED + assert sample_letter_notification.billable_units == 0 mock_sanitise.assert_called_once_with( ANY, sample_letter_notification, @@ -432,13 +432,44 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails( ) -def test_get_page_count_set_notification_to_permanent_failure_when_not_pdf( - sample_letter_notification +@freeze_time('2018-01-01 18:00') +@mock_s3 +@pytest.mark.parametrize('key_type,is_test_letter', [ + (KEY_TYPE_NORMAL, False), (KEY_TYPE_TEST, True) +]) +def test_process_letter_task_check_virus_scan_passed_when_file_cannot_be_opened( + sample_letter_notification, mocker, key_type, is_test_letter ): - with pytest.raises(expected_exception=PdfReadError): - _get_page_count(sample_letter_notification, b'pdf_content') - updated_notification = Notification.query.filter_by(id=sample_letter_notification.id).first() - assert updated_notification.status == NOTIFICATION_VALIDATION_FAILED + filename = 'NOTIFY.{}'.format(sample_letter_notification.reference) + source_bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME'] + target_bucket_name = current_app.config['INVALID_PDF_BUCKET_NAME'] + + conn = boto3.resource('s3', region_name='eu-west-1') + conn.create_bucket(Bucket=source_bucket_name) + conn.create_bucket(Bucket=target_bucket_name) + + s3 = boto3.client('s3', region_name='eu-west-1') + s3.put_object(Bucket=source_bucket_name, Key=filename, Body=b'pdf_content') + + sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK + sample_letter_notification.key_type = key_type + mock_move_s3 = mocker.patch('app.letters.utils._move_s3_object') + + mock_get_page_count = mocker.patch('app.celery.letters_pdf_tasks._get_page_count', side_effect=PdfReadError) + mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precompiled_pdf') + + process_virus_scan_passed(filename) + + mock_sanitise.assert_not_called() + mock_get_page_count.assert_called_once_with( + sample_letter_notification, b'pdf_content' + ) + mock_move_s3.assert_called_once_with( + source_bucket_name, filename, + target_bucket_name, filename + ) + assert sample_letter_notification.status == NOTIFICATION_VALIDATION_FAILED + assert sample_letter_notification.billable_units == 0 def test_process_letter_task_check_virus_scan_failed(sample_letter_notification, mocker): From c3c9d1eac987cd6b443641a7d0ec9fba148c005f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 11 Jan 2019 17:09:42 +0000 Subject: [PATCH 08/31] Add unit tests. Fix data types in result set. --- app/dao/fact_notification_status_dao.py | 4 +-- app/service/rest.py | 1 - .../dao/test_fact_notification_status_dao.py | 36 +++++++++++++++---- 3 files changed, 32 insertions(+), 9 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 7231dc7d8..f5466bbec 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -351,8 +351,8 @@ def fetch_monthly_template_usage_for_service(start_date, end_date, service_id): all_stats_table.c.name, all_stats_table.c.is_precompiled_letter, all_stats_table.c.template_type, - all_stats_table.c.month, - all_stats_table.c.year, + func.cast(all_stats_table.c.month, Integer).label('month'), + func.cast(all_stats_table.c.year, Integer).label('year'), func.cast(func.sum(all_stats_table.c.count), Integer).label('count'), ).group_by( all_stats_table.c.template_id, diff --git a/app/service/rest.py b/app/service/rest.py index e00c99aa7..4aa0447b0 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -49,7 +49,6 @@ from app.dao.services_dao import ( dao_create_service, dao_fetch_all_services, dao_fetch_all_services_by_user, - dao_fetch_monthly_historical_usage_by_template_for_service, dao_fetch_service_by_id, dao_fetch_todays_stats_for_service, dao_fetch_todays_stats_for_all_services, diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index 0e541f7a9..9b1f92f63 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -341,10 +341,11 @@ def test_fetch_stats_for_all_services_by_date_range(notify_db_session): assert not results[4].count +@freeze_time('2018-01-04 14:00') def test_fetch_monthly_template_usage_for_service(sample_service): - template_one = create_template(service=sample_service, template_type='sms', template_name='one') - template_two = create_template(service=sample_service, template_type='email', template_name='one') - template_three = create_template(service=sample_service, template_type='letter', template_name='one') + template_one = create_template(service=sample_service, template_type='sms', template_name='1_one') + template_two = create_template(service=sample_service, template_type='email', template_name='2_two') + template_three = create_template(service=sample_service, template_type='letter', template_name='3_three') create_ft_notification_status(bst_date=date(2018, 1, 1), service=sample_service, @@ -353,15 +354,38 @@ def test_fetch_monthly_template_usage_for_service(sample_service): create_ft_notification_status(bst_date=date(2018, 2, 1), service=sample_service, template=template_two, - count=3) + count=4) create_ft_notification_status(bst_date=date(2018, 3, 1), service=sample_service, template=template_three, count=5) - + create_notification(template=template_one) results = fetch_monthly_template_usage_for_service( datetime(2017, 4, 1), datetime(2018, 3, 31), sample_service.id ) - print(results) assert len(results) == 3 + + assert results[0].template_id == template_one.id + assert results[0].name == template_one.name + assert results[0].is_precompiled_letter is False + assert results[0].template_type == template_one.template_type + assert results[0].month == 1 + assert results[0].year == 2018 + assert results[0].count == 3 + + assert results[1].template_id == template_two.id + assert results[1].name == template_two.name + assert results[1].is_precompiled_letter is False + assert results[1].template_type == template_two.template_type + assert results[1].month == 2 + assert results[1].year == 2018 + assert results[1].count == 4 + + assert results[2].template_id == template_three.id + assert results[2].name == template_three.name + assert results[2].is_precompiled_letter is False + assert results[2].template_type == template_three.template_type + assert results[2].month == 3 + assert results[2].year == 2018 + assert results[2].count == 5 From b5a3ef9576e160c58c8715a738b397d7e0ffbb9f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 14 Jan 2019 15:28:26 +0000 Subject: [PATCH 09/31] Added order by. Added more unit tests. Remove comments. --- app/dao/fact_notification_status_dao.py | 14 ++- app/service/rest.py | 5 - .../dao/test_fact_notification_status_dao.py | 116 ++++++++++++++---- 3 files changed, 105 insertions(+), 30 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index f5466bbec..6bb341409 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -8,7 +8,10 @@ from sqlalchemy.sql.expression import literal, extract from sqlalchemy.types import DateTime, Integer from app import db -from app.models import Notification, NotificationHistory, FactNotificationStatus, KEY_TYPE_TEST, Service, Template +from app.models import ( + Notification, NotificationHistory, FactNotificationStatus, KEY_TYPE_TEST, Service, Template, + NOTIFICATION_CANCELLED +) from app.utils import get_london_midnight_in_utc, midnight_n_days_ago, get_london_month_from_utc_column @@ -309,6 +312,7 @@ def fetch_monthly_template_usage_for_service(start_date, end_date, service_id): FactNotificationStatus.service_id == service_id, FactNotificationStatus.bst_date >= start_date, FactNotificationStatus.bst_date <= end_date, + FactNotificationStatus.notification_status != NOTIFICATION_CANCELLED ).group_by( FactNotificationStatus.template_id, Template.name, @@ -316,6 +320,10 @@ def fetch_monthly_template_usage_for_service(start_date, end_date, service_id): Template.is_precompiled_letter, extract('month', FactNotificationStatus.bst_date).label('month'), extract('year', FactNotificationStatus.bst_date).label('year'), + ).order_by( + extract('year', FactNotificationStatus.bst_date), + extract('month', FactNotificationStatus.bst_date), + Template.name ) if start_date <= datetime.utcnow() <= end_date: @@ -335,8 +343,8 @@ def fetch_monthly_template_usage_for_service(start_date, end_date, service_id): ).filter( Notification.created_at >= today, Notification.service_id == service_id, - # we don't want to include test keys - Notification.key_type != KEY_TYPE_TEST + Notification.key_type != KEY_TYPE_TEST, + Notification.status != NOTIFICATION_CANCELLED ).group_by( Notification.template_id, Template.hidden, diff --git a/app/service/rest.py b/app/service/rest.py index 4aa0447b0..279d00ed5 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -585,11 +585,6 @@ def get_monthly_template_usage(service_id): end_date=end_date, service_id=service_id ) - # data = dao_fetch_monthly_historical_usage_by_template_for_service( - # service_id, - # int(request.args.get('year', 'NaN')) - # ) - stats = list() for i in data: stats.append( diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index 9b1f92f63..a7adffce0 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -341,51 +341,123 @@ def test_fetch_stats_for_all_services_by_date_range(notify_db_session): assert not results[4].count -@freeze_time('2018-01-04 14:00') +@freeze_time('2018-03-30 14:00') def test_fetch_monthly_template_usage_for_service(sample_service): - template_one = create_template(service=sample_service, template_type='sms', template_name='1_one') - template_two = create_template(service=sample_service, template_type='email', template_name='2_two') - template_three = create_template(service=sample_service, template_type='letter', template_name='3_three') + template_one = create_template(service=sample_service, template_type='sms', template_name='a') + template_two = create_template(service=sample_service, template_type='email', template_name='b') + template_three = create_template(service=sample_service, template_type='letter', template_name='c') + + create_ft_notification_status(bst_date=date(2017, 12, 10), + service=sample_service, + template=template_two, + count=3) + create_ft_notification_status(bst_date=date(2017, 12, 10), + service=sample_service, + template=template_one, + count=6) create_ft_notification_status(bst_date=date(2018, 1, 1), service=sample_service, template=template_one, - count=2) - create_ft_notification_status(bst_date=date(2018, 2, 1), - service=sample_service, - template=template_two, count=4) + create_ft_notification_status(bst_date=date(2018, 3, 1), service=sample_service, template=template_three, count=5) - create_notification(template=template_one) + create_notification(template=template_three, created_at=datetime.utcnow() - timedelta(days=1)) + create_notification(template=template_three, created_at=datetime.utcnow()) results = fetch_monthly_template_usage_for_service( datetime(2017, 4, 1), datetime(2018, 3, 31), sample_service.id ) - assert len(results) == 3 + assert len(results) == 4 assert results[0].template_id == template_one.id assert results[0].name == template_one.name assert results[0].is_precompiled_letter is False assert results[0].template_type == template_one.template_type - assert results[0].month == 1 - assert results[0].year == 2018 - assert results[0].count == 3 - + assert results[0].month == 12 + assert results[0].year == 2017 + assert results[0].count == 6 assert results[1].template_id == template_two.id assert results[1].name == template_two.name assert results[1].is_precompiled_letter is False assert results[1].template_type == template_two.template_type + assert results[1].month == 12 + assert results[1].year == 2017 + assert results[1].count == 3 + + assert results[2].template_id == template_one.id + assert results[2].name == template_one.name + assert results[2].is_precompiled_letter is False + assert results[2].template_type == template_one.template_type + assert results[2].month == 1 + assert results[2].year == 2018 + assert results[2].count == 4 + + assert results[3].template_id == template_three.id + assert results[3].name == template_three.name + assert results[3].is_precompiled_letter is False + assert results[3].template_type == template_three.template_type + assert results[3].month == 3 + assert results[3].year == 2018 + assert results[3].count == 6 + + +@freeze_time('2018-03-30 14:00') +def test_fetch_monthly_template_usage_for_service_does_join_to_notifications_if_today_is_not_in_date_range( + sample_service +): + template_one = create_template(service=sample_service, template_type='sms', template_name='a') + template_two = create_template(service=sample_service, template_type='email', template_name='b') + create_ft_notification_status(bst_date=date(2018, 2, 1), + service=template_two.service, + template=template_two, + count=15) + create_ft_notification_status(bst_date=date(2018, 2, 2), + service=template_one.service, + template=template_one, + count=20) + create_ft_notification_status(bst_date=date(2018, 3, 1), + service=template_one.service, + template=template_one, + count=3) + create_notification(template=template_one, created_at=datetime.utcnow()) + results = fetch_monthly_template_usage_for_service( + datetime(2018, 1, 1), datetime(2018, 2, 20), template_one.service_id + ) + + assert len(results) == 2 + + assert results[0].template_id == template_one.id + assert results[0].name == template_one.name + assert results[0].is_precompiled_letter == template_one.is_precompiled_letter + assert results[0].template_type == template_one.template_type + assert results[0].month == 2 + assert results[0].year == 2018 + assert results[0].count == 20 + assert results[1].template_id == template_two.id + assert results[1].name == template_two.name + assert results[1].is_precompiled_letter == template_two.is_precompiled_letter + assert results[1].template_type == template_two.template_type assert results[1].month == 2 assert results[1].year == 2018 - assert results[1].count == 4 + assert results[1].count == 15 - assert results[2].template_id == template_three.id - assert results[2].name == template_three.name - assert results[2].is_precompiled_letter is False - assert results[2].template_type == template_three.template_type - assert results[2].month == 3 - assert results[2].year == 2018 - assert results[2].count == 5 + +@freeze_time('2018-03-30 14:00') +def test_fetch_monthly_template_usage_for_service_does_not_include_cancelled_status( + sample_template +): + create_ft_notification_status(bst_date=date(2018, 3, 1), + service=sample_template.service, + template=sample_template, + notification_status='cancelled', + count=15) + create_notification(template=sample_template, created_at=datetime.utcnow(), status='cancelled') + results = fetch_monthly_template_usage_for_service( + datetime(2018, 1, 1), datetime(2018, 3, 31), sample_template.service_id + ) + + assert len(results) == 0 From efad58edd8f9099f9e731830f3f5dddbedd6f3e8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 14 Jan 2019 16:30:36 +0000 Subject: [PATCH 10/31] There is no need to have a separate table to store template monthly statistics. It's easy enough to aggregate the stats from ft_notification_status. This removes the nightly task, and all the dao methods. The next PR will remove the table. --- app/celery/scheduled_tasks.py | 19 - app/config.py | 5 - app/dao/services_dao.py | 74 --- app/dao/stats_template_usage_by_month_dao.py | 60 --- tests/app/celery/test_scheduled_tasks.py | 150 ------ tests/app/dao/test_services_dao.py | 436 +----------------- .../test_stats_template_usage_by_month_dao.py | 155 ------- tests/app/service/test_statistics_rest.py | 17 - 8 files changed, 1 insertion(+), 915 deletions(-) delete mode 100644 app/dao/stats_template_usage_by_month_dao.py delete mode 100644 tests/app/dao/test_stats_template_usage_by_month_dao.py diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index cde969034..818206e76 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -40,10 +40,6 @@ from app.dao.provider_details_dao import ( dao_toggle_sms_provider ) from app.dao.service_callback_api_dao import get_service_delivery_status_callback_api_for_service -from app.dao.services_dao import ( - dao_fetch_monthly_historical_stats_by_template -) -from app.dao.stats_template_usage_by_month_dao import insert_or_update_stats_for_template from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago from app.exceptions import NotificationTechnicalFailureException from app.models import ( @@ -405,21 +401,6 @@ def check_job_status(): raise JobIncompleteError("Job(s) {} have not completed.".format(job_ids)) -@notify_celery.task(name='daily-stats-template-usage-by-month') -@statsd(namespace="tasks") -def daily_stats_template_usage_by_month(): - results = dao_fetch_monthly_historical_stats_by_template() - - for result in results: - if result.template_id: - insert_or_update_stats_for_template( - result.template_id, - result.month, - result.year, - result.count - ) - - @notify_celery.task(name='raise-alert-if-no-letter-ack-file') @statsd(namespace="tasks") def letter_raise_alert_if_no_ack_file_for_zip(): diff --git a/app/config.py b/app/config.py index a0569534e..f69e30869 100644 --- a/app/config.py +++ b/app/config.py @@ -195,11 +195,6 @@ class Config(object): 'schedule': crontab(hour=0, minute=5), 'options': {'queue': QueueNames.PERIODIC} }, - 'daily-stats-template-usage-by-month': { - 'task': 'daily-stats-template-usage-by-month', - 'schedule': crontab(hour=0, minute=10), - 'options': {'queue': QueueNames.PERIODIC} - }, 'create-nightly-billing': { 'task': 'create-nightly-billing', 'schedule': crontab(hour=0, minute=15), diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index fb5cba297..342ba7dfb 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -11,9 +11,7 @@ from app.dao.dao_utils import ( transactional, version_class ) -from app.dao.date_util import get_financial_year from app.dao.service_sms_sender_dao import insert_service_sms_sender -from app.dao.stats_template_usage_by_month_dao import dao_get_template_usage_stats_by_service from app.models import ( AnnualBilling, ApiKey, @@ -389,75 +387,3 @@ def dao_fetch_monthly_historical_stats_by_template(): year, month ).all() - - -@statsd(namespace="dao") -def dao_fetch_monthly_historical_usage_by_template_for_service(service_id, year): - - results = dao_get_template_usage_stats_by_service(service_id, year) - - stats = [] - for result in results: - stat = type("", (), {})() - stat.template_id = result.template_id - stat.template_type = result.template_type - stat.name = str(result.name) - stat.month = result.month - stat.year = result.year - stat.count = result.count - stat.is_precompiled_letter = result.is_precompiled_letter - stats.append(stat) - - month = get_london_month_from_utc_column(Notification.created_at) - year_func = func.date_trunc("year", Notification.created_at) - start_date = datetime.combine(date.today(), time.min) - - fy_start, fy_end = get_financial_year(year) - - if fy_start < datetime.now() < fy_end: - today_results = db.session.query( - Notification.template_id, - Template.is_precompiled_letter, - Template.name, - Template.template_type, - extract('month', month).label('month'), - extract('year', year_func).label('year'), - func.count().label('count') - ).join( - Template, Notification.template_id == Template.id, - ).filter( - Notification.created_at >= start_date, - Notification.service_id == service_id, - # we don't want to include test keys - Notification.key_type != KEY_TYPE_TEST - ).group_by( - Notification.template_id, - Template.hidden, - Template.name, - Template.template_type, - month, - year_func - ).order_by( - Notification.template_id - ).all() - - for today_result in today_results: - add_to_stats = True - for stat in stats: - if today_result.template_id == stat.template_id and today_result.month == stat.month \ - and today_result.year == stat.year: - stat.count = stat.count + today_result.count - add_to_stats = False - - if add_to_stats: - new_stat = type("StatsTemplateUsageByMonth", (), {})() - new_stat.template_id = today_result.template_id - new_stat.template_type = today_result.template_type - new_stat.name = today_result.name - new_stat.month = int(today_result.month) - new_stat.year = int(today_result.year) - new_stat.count = today_result.count - new_stat.is_precompiled_letter = today_result.is_precompiled_letter - stats.append(new_stat) - - return stats diff --git a/app/dao/stats_template_usage_by_month_dao.py b/app/dao/stats_template_usage_by_month_dao.py deleted file mode 100644 index 541ab7193..000000000 --- a/app/dao/stats_template_usage_by_month_dao.py +++ /dev/null @@ -1,60 +0,0 @@ -from notifications_utils.statsd_decorators import statsd -from sqlalchemy import or_, and_, desc - -from app import db -from app.dao.dao_utils import transactional -from app.models import StatsTemplateUsageByMonth, Template - - -@transactional -@statsd(namespace="dao") -def insert_or_update_stats_for_template(template_id, month, year, count): - result = db.session.query( - StatsTemplateUsageByMonth - ).filter( - StatsTemplateUsageByMonth.template_id == template_id, - StatsTemplateUsageByMonth.month == month, - StatsTemplateUsageByMonth.year == year - ).update( - { - 'count': count - } - ) - if result == 0: - monthly_stats = StatsTemplateUsageByMonth( - template_id=template_id, - month=month, - year=year, - count=count - ) - - db.session.add(monthly_stats) - - -@statsd(namespace="dao") -def dao_get_template_usage_stats_by_service(service_id, year): - return db.session.query( - StatsTemplateUsageByMonth.template_id, - Template.name, - Template.template_type, - Template.is_precompiled_letter, - StatsTemplateUsageByMonth.month, - StatsTemplateUsageByMonth.year, - StatsTemplateUsageByMonth.count - ).join( - Template, StatsTemplateUsageByMonth.template_id == Template.id - ).filter( - Template.service_id == service_id - ).filter( - or_( - and_( - StatsTemplateUsageByMonth.month.in_([4, 5, 6, 7, 8, 9, 10, 11, 12]), - StatsTemplateUsageByMonth.year == year - ), and_( - StatsTemplateUsageByMonth.month.in_([1, 2, 3]), - StatsTemplateUsageByMonth.year == year + 1 - ) - ) - ).order_by( - desc(StatsTemplateUsageByMonth.month) - ).all() diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index b2fee242a..e97b4643c 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -1,4 +1,3 @@ -import functools from datetime import datetime, timedelta from functools import partial from unittest.mock import call, patch, PropertyMock @@ -31,7 +30,6 @@ from app.celery.scheduled_tasks import ( send_total_sent_notifications_to_performance_platform, switch_current_sms_provider_on_slow_delivery, timeout_notifications, - daily_stats_template_usage_by_month, letter_raise_alert_if_no_ack_file_for_zip, replay_created_notifications ) @@ -46,8 +44,6 @@ from app.dao.provider_details_dao import ( ) from app.exceptions import NotificationTechnicalFailureException from app.models import ( - NotificationHistory, - StatsTemplateUsageByMonth, JOB_STATUS_IN_PROGRESS, JOB_STATUS_ERROR, LETTER_TYPE, @@ -69,7 +65,6 @@ from tests.app.db import ( from tests.app.conftest import ( sample_job as create_sample_job, sample_notification_history as create_notification_history, - sample_template as create_sample_template, datetime_in_past ) @@ -806,151 +801,6 @@ def test_check_job_status_task_sets_jobs_to_error(mocker, sample_template): assert job_2.job_status == JOB_STATUS_IN_PROGRESS -def test_daily_stats_template_usage_by_month(notify_db, notify_db_session): - notification_history = functools.partial( - create_notification_history, - notify_db, - notify_db_session, - status='delivered' - ) - - template_one = create_sample_template(notify_db, notify_db_session) - template_two = create_sample_template(notify_db, notify_db_session) - - notification_history(created_at=datetime(2017, 10, 1), sample_template=template_one) - notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) - notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) - notification_history(created_at=datetime.now(), sample_template=template_two) - - daily_stats_template_usage_by_month() - - result = db.session.query( - StatsTemplateUsageByMonth - ).order_by( - StatsTemplateUsageByMonth.year, - StatsTemplateUsageByMonth.month - ).all() - - assert len(result) == 2 - - assert result[0].template_id == template_two.id - assert result[0].month == 4 - assert result[0].year == 2016 - assert result[0].count == 2 - - assert result[1].template_id == template_one.id - assert result[1].month == 10 - assert result[1].year == 2017 - assert result[1].count == 1 - - -def test_daily_stats_template_usage_by_month_no_data(): - daily_stats_template_usage_by_month() - - results = db.session.query(StatsTemplateUsageByMonth).all() - - assert len(results) == 0 - - -def test_daily_stats_template_usage_by_month_multiple_runs(notify_db, notify_db_session): - notification_history = functools.partial( - create_notification_history, - notify_db, - notify_db_session, - status='delivered' - ) - - template_one = create_sample_template(notify_db, notify_db_session) - template_two = create_sample_template(notify_db, notify_db_session) - - notification_history(created_at=datetime(2017, 11, 1), sample_template=template_one) - notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) - notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) - notification_history(created_at=datetime.now(), sample_template=template_two) - - daily_stats_template_usage_by_month() - - template_three = create_sample_template(notify_db, notify_db_session) - - notification_history(created_at=datetime(2017, 10, 1), sample_template=template_three) - notification_history(created_at=datetime(2017, 9, 1), sample_template=template_three) - notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) - notification_history(created_at=datetime(2016, 4, 1), sample_template=template_two) - notification_history(created_at=datetime.now(), sample_template=template_two) - - daily_stats_template_usage_by_month() - - result = db.session.query( - StatsTemplateUsageByMonth - ).order_by( - StatsTemplateUsageByMonth.year, - StatsTemplateUsageByMonth.month - ).all() - - assert len(result) == 4 - - assert result[0].template_id == template_two.id - assert result[0].month == 4 - assert result[0].year == 2016 - assert result[0].count == 4 - - assert result[1].template_id == template_three.id - assert result[1].month == 9 - assert result[1].year == 2017 - assert result[1].count == 1 - - assert result[2].template_id == template_three.id - assert result[2].month == 10 - assert result[2].year == 2017 - assert result[2].count == 1 - - assert result[3].template_id == template_one.id - assert result[3].month == 11 - assert result[3].year == 2017 - assert result[3].count == 1 - - -def test_dao_fetch_monthly_historical_stats_by_template_null_template_id_not_counted(notify_db, notify_db_session): - notification_history = functools.partial( - create_notification_history, - notify_db, - notify_db_session, - status='delivered' - ) - - template_one = create_sample_template(notify_db, notify_db_session, template_name='1') - history = notification_history(created_at=datetime(2017, 2, 1), sample_template=template_one) - - NotificationHistory.query.filter( - NotificationHistory.id == history.id - ).update( - { - 'template_id': None - } - ) - - daily_stats_template_usage_by_month() - - result = db.session.query( - StatsTemplateUsageByMonth - ).all() - - assert len(result) == 0 - - notification_history(created_at=datetime(2017, 2, 1), sample_template=template_one) - - daily_stats_template_usage_by_month() - - result = db.session.query( - StatsTemplateUsageByMonth - ).order_by( - StatsTemplateUsageByMonth.year, - StatsTemplateUsageByMonth.month - ).all() - - assert len(result) == 1 - - def mock_s3_get_list_match(bucket_name, subfolder='', suffix='', last_modified=None): if subfolder == '2018-01-11/zips_sent': return ['NOTIFY.20180111175007.ZIP.TXT', 'NOTIFY.20180111175008.ZIP.TXT'] diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 024b1c019..766f2ef59 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -1,5 +1,5 @@ import uuid -from datetime import datetime, timedelta +from datetime import datetime import pytest from freezegun import freeze_time @@ -7,7 +7,6 @@ from sqlalchemy.exc import IntegrityError, SQLAlchemyError from sqlalchemy.orm.exc import FlushError, NoResultFound from app import db -from app.celery.scheduled_tasks import daily_stats_template_usage_by_month from app.dao.inbound_numbers_dao import ( dao_set_inbound_number_to_service, dao_get_available_inbound_numbers, @@ -32,7 +31,6 @@ from app.dao.services_dao import ( dao_fetch_active_users_for_service, dao_fetch_service_by_inbound_number, dao_fetch_monthly_historical_stats_by_template, - dao_fetch_monthly_historical_usage_by_template_for_service ) from app.dao.users_dao import save_model_user, create_user_code from app.models import ( @@ -901,441 +899,9 @@ def test_dao_fetch_monthly_historical_stats_by_template(notify_db_session): assert result[1].count == 1 -def test_dao_fetch_monthly_historical_usage_by_template_for_service_no_stats_today( - notify_db_session, -): - service = create_service() - template_one = create_template(service=service, template_name='1') - template_two = create_template(service=service, template_name='2') - - n = create_notification(created_at=datetime(2017, 10, 1), template=template_one, status='delivered') - create_notification(created_at=datetime(2017, 4, 1), template=template_two, status='delivered') - create_notification(created_at=datetime(2017, 4, 1), template=template_two, status='delivered') - create_notification(created_at=datetime.now(), template=template_two, status='delivered') - - daily_stats_template_usage_by_month() - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 2 - - assert result[0].template_id == template_two.id - assert result[0].name == template_two.name - assert result[0].template_type == template_two.template_type - assert result[0].month == 4 - assert result[0].year == 2017 - assert result[0].count == 2 - - assert result[1].template_id == template_one.id - assert result[1].name == template_one.name - assert result[1].template_type == template_two.template_type - assert result[1].month == 10 - assert result[1].year == 2017 - assert result[1].count == 1 - - -@freeze_time("2017-11-10 11:09:00.000000") -def test_dao_fetch_monthly_historical_usage_by_template_for_service_add_to_historical( - notify_db_session, -): - service = create_service() - template_one = create_template(service=service, template_name='1') - template_two = create_template(service=service, template_name='2') - template_three = create_template(service=service, template_name='3') - - date = datetime.now() - day = date.day - month = date.month - year = date.year - - n = create_notification(created_at=datetime(2017, 9, 1), template=template_one, status='delivered') - create_notification(created_at=datetime(year, month, day) - timedelta(days=1), template=template_two, - status='delivered') - create_notification(created_at=datetime(year, month, day) - timedelta(days=1), template=template_two, - status='delivered') - - daily_stats_template_usage_by_month() - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 2 - - assert result[0].template_id == template_one.id - assert result[0].name == template_one.name - assert result[0].template_type == template_one.template_type - assert result[0].month == 9 - assert result[0].year == 2017 - assert result[0].count == 1 - - assert result[1].template_id == template_two.id - assert result[1].name == template_two.name - assert result[1].template_type == template_two.template_type - assert result[1].month == 11 - assert result[1].year == 2017 - assert result[1].count == 2 - - create_notification( - template=template_three, - created_at=datetime.now(), - status='delivered' - ) - create_notification( - template=template_two, - created_at=datetime.now(), - status='delivered' - ) - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 3 - - assert result[0].template_id == template_one.id - assert result[0].name == template_one.name - assert result[0].template_type == template_one.template_type - assert result[0].month == 9 - assert result[0].year == 2017 - assert result[0].count == 1 - - assert result[1].template_id == template_two.id - assert result[1].name == template_two.name - assert result[1].template_type == template_two.template_type - assert result[1].month == month - assert result[1].year == year - assert result[1].count == 3 - - assert result[2].template_id == template_three.id - assert result[2].name == template_three.name - assert result[2].template_type == template_three.template_type - assert result[2].month == 11 - assert result[2].year == 2017 - assert result[2].count == 1 - - -@freeze_time("2017-11-10 11:09:00.000000") -def test_dao_fetch_monthly_historical_usage_by_template_for_service_does_add_old_notification( - notify_db_session, -): - template_one, template_three, template_two = create_email_sms_letter_template() - - date = datetime.now() - day = date.day - month = date.month - year = date.year - - n = create_notification(created_at=datetime(2017, 9, 1), template=template_one, status='delivered') - create_notification(created_at=datetime(year, month, day) - timedelta(days=1), template=template_two, - status='delivered') - create_notification(created_at=datetime(year, month, day) - timedelta(days=1), template=template_two, - status='delivered') - - daily_stats_template_usage_by_month() - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 2 - - assert result[0].template_id == template_one.id - assert result[0].name == template_one.name - assert result[0].template_type == template_one.template_type - assert result[0].month == 9 - assert result[0].year == 2017 - assert result[0].count == 1 - - assert result[1].template_id == template_two.id - assert result[1].name == template_two.name - assert result[1].template_type == template_two.template_type - assert result[1].month == 11 - assert result[1].year == 2017 - assert result[1].count == 2 - - create_notification( - template=template_three, - created_at=datetime.utcnow() - timedelta(days=2), - status='delivered' - ) - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 2 - - -@freeze_time("2017-11-10 11:09:00.000000") -def test_dao_fetch_monthly_historical_usage_by_template_for_service_get_this_year_only( - notify_db_session, -): - template_one, template_three, template_two = create_email_sms_letter_template() - - date = datetime.now() - day = date.day - month = date.month - year = date.year - - n = create_notification(created_at=datetime(2016, 9, 1), template=template_one, status='delivered') - create_notification(created_at=datetime(year, month, day) - timedelta(days=1), template=template_two, - status='delivered') - create_notification(created_at=datetime(year, month, day) - timedelta(days=1), template=template_two, - status='delivered') - - daily_stats_template_usage_by_month() - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 1 - - assert result[0].template_id == template_two.id - assert result[0].name == template_two.name - assert result[0].template_type == template_two.template_type - assert result[0].month == 11 - assert result[0].year == 2017 - assert result[0].count == 2 - - create_notification( - template=template_three, - created_at=datetime.utcnow() - timedelta(days=2) - ) - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 1 - - create_notification( - template=template_three, - created_at=datetime.utcnow() - ) - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 2 - - def create_email_sms_letter_template(): service = create_service() template_one = create_template(service=service, template_name='1', template_type='email') template_two = create_template(service=service, template_name='2', template_type='sms') template_three = create_template(service=service, template_name='3', template_type='letter') return template_one, template_three, template_two - - -@freeze_time("2017-11-10 11:09:00.000000") -def test_dao_fetch_monthly_historical_usage_by_template_for_service_combined_historical_current( - notify_db_session, -): - template_one = create_template(service=create_service(), template_name='1') - - date = datetime.now() - day = date.day - month = date.month - year = date.year - - n = create_notification(status='delivered', created_at=datetime(year, month, day) - timedelta(days=30), - template=template_one) - - daily_stats_template_usage_by_month() - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 1 - - assert result[0].template_id == template_one.id - assert result[0].name == template_one.name - assert result[0].template_type == template_one.template_type - assert result[0].month == 10 - assert result[0].year == 2017 - assert result[0].count == 1 - - create_notification( - template=template_one, - created_at=datetime.utcnow() - ) - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 2 - - assert result[0].template_id == template_one.id - assert result[0].name == template_one.name - assert result[0].template_type == template_one.template_type - assert result[0].month == 10 - assert result[0].year == 2017 - assert result[0].count == 1 - - assert result[1].template_id == template_one.id - assert result[1].name == template_one.name - assert result[1].template_type == template_one.template_type - assert result[1].month == 11 - assert result[1].year == 2017 - assert result[1].count == 1 - - -@freeze_time("2017-11-10 11:09:00.000000") -def test_dao_fetch_monthly_historical_usage_by_template_for_service_does_not_return_double_precision_values( - notify_db_session, -): - template_one = create_template(service=create_service()) - - n = create_notification( - template=template_one, - created_at=datetime.utcnow() - ) - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.month, x.year) - ) - - assert len(result) == 1 - - assert result[0].template_id == template_one.id - assert result[0].name == template_one.name - assert result[0].template_type == template_one.template_type - assert result[0].month == 11 - assert len(str(result[0].month)) == 2 - assert result[0].year == 2017 - assert len(str(result[0].year)) == 4 - assert result[0].count == 1 - - -@freeze_time("2018-03-10 11:09:00.000000") -def test_dao_fetch_monthly_historical_usage_by_template_for_service_returns_financial_year( - notify_db, - notify_db_session, -): - service = create_service() - template_one = create_template(service=service, template_name='1', template_type='email') - - date = datetime.now() - day = date.day - year = date.year - - create_notification(template=template_one, status='delivered', created_at=datetime(year - 1, 1, day)) - create_notification(template=template_one, status='delivered', created_at=datetime(year - 1, 3, day)) - create_notification(template=template_one, status='delivered', created_at=datetime(year - 1, 4, day)) - create_notification(template=template_one, status='delivered', created_at=datetime(year - 1, 5, day)) - create_notification(template=template_one, status='delivered', created_at=datetime(year, 1, day)) - create_notification(template=template_one, status='delivered', created_at=datetime(year, 2, day)) - - daily_stats_template_usage_by_month() - - n = create_notification( - template=template_one, - created_at=datetime.utcnow() - ) - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2017), - key=lambda x: (x.year, x.month) - ) - - assert len(result) == 5 - - assert result[0].month == 4 - assert result[0].year == 2017 - assert result[1].month == 5 - assert result[1].year == 2017 - assert result[2].month == 1 - assert result[2].year == 2018 - assert result[3].month == 2 - assert result[3].year == 2018 - assert result[4].month == 3 - assert result[4].year == 2018 - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(n.service_id, 2014), - key=lambda x: (x.year, x.month) - ) - - assert len(result) == 0 - - -@freeze_time("2018-03-10 11:09:00.000000") -def test_dao_fetch_monthly_historical_usage_by_template_for_service_only_returns_for_service( - notify_db_session -): - template_one = create_template(service=create_service(), template_name='1', template_type='email') - - date = datetime.now() - day = date.day - year = date.year - - create_notification(template=template_one, created_at=datetime(year, 1, day)) - create_notification(template=template_one, created_at=datetime(year, 2, day)) - create_notification(template=template_one, created_at=datetime(year, 3, day)) - - service_two = create_service(service_name='other_service', user=create_user()) - template_two = create_template(service=service_two, template_name='1', template_type='email') - - create_notification(template=template_two) - create_notification(template=template_two) - - daily_stats_template_usage_by_month() - - x = dao_fetch_monthly_historical_usage_by_template_for_service(template_one.service_id, 2017) - - result = sorted( - x, - key=lambda x: (x.year, x.month) - ) - - assert len(result) == 3 - - result = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(service_two.id, 2017), - key=lambda x: (x.year, x.month) - ) - - assert len(result) == 1 - - -@freeze_time("2018-01-01 11:09:00.000000") -def test_dao_fetch_monthly_historical_usage_by_template_for_service_ignores_test_api_keys(notify_db_session): - service = create_service() - template_1 = create_template(service, template_name='1') - template_2 = create_template(service, template_name='2') - template_3 = create_template(service, template_name='3') - - create_notification(template_1, key_type=KEY_TYPE_TEST) - create_notification(template_2, key_type=KEY_TYPE_TEAM) - create_notification(template_3, key_type=KEY_TYPE_NORMAL) - - results = sorted( - dao_fetch_monthly_historical_usage_by_template_for_service(service.id, 2017), - key=lambda x: x.name - ) - - assert len(results) == 2 - # template_1 only used with test keys - assert results[0].template_id == template_2.id - assert results[0].count == 1 - - assert results[1].template_id == template_3.id - assert results[1].count == 1 diff --git a/tests/app/dao/test_stats_template_usage_by_month_dao.py b/tests/app/dao/test_stats_template_usage_by_month_dao.py deleted file mode 100644 index 676e00952..000000000 --- a/tests/app/dao/test_stats_template_usage_by_month_dao.py +++ /dev/null @@ -1,155 +0,0 @@ -from app import db -from app.dao.stats_template_usage_by_month_dao import ( - insert_or_update_stats_for_template, - dao_get_template_usage_stats_by_service -) -from app.models import StatsTemplateUsageByMonth, LETTER_TYPE, PRECOMPILED_TEMPLATE_NAME - -from tests.app.db import create_service, create_template - - -def test_create_stats_for_template(notify_db_session, sample_template): - assert StatsTemplateUsageByMonth.query.count() == 0 - - insert_or_update_stats_for_template(sample_template.id, 1, 2017, 10) - stats_by_month = StatsTemplateUsageByMonth.query.filter( - StatsTemplateUsageByMonth.template_id == sample_template.id - ).all() - - assert len(stats_by_month) == 1 - assert stats_by_month[0].template_id == sample_template.id - assert stats_by_month[0].month == 1 - assert stats_by_month[0].year == 2017 - assert stats_by_month[0].count == 10 - - -def test_update_stats_for_template(notify_db_session, sample_template): - assert StatsTemplateUsageByMonth.query.count() == 0 - - insert_or_update_stats_for_template(sample_template.id, 1, 2017, 10) - insert_or_update_stats_for_template(sample_template.id, 1, 2017, 20) - insert_or_update_stats_for_template(sample_template.id, 2, 2017, 30) - - stats_by_month = StatsTemplateUsageByMonth.query.filter( - StatsTemplateUsageByMonth.template_id == sample_template.id - ).order_by(StatsTemplateUsageByMonth.template_id).all() - - assert len(stats_by_month) == 2 - - assert stats_by_month[0].template_id == sample_template.id - assert stats_by_month[0].month == 1 - assert stats_by_month[0].year == 2017 - assert stats_by_month[0].count == 20 - - assert stats_by_month[1].template_id == sample_template.id - assert stats_by_month[1].month == 2 - assert stats_by_month[1].year == 2017 - assert stats_by_month[1].count == 30 - - -def test_dao_get_template_usage_stats_by_service(sample_service): - - email_template = create_template(service=sample_service, template_type="email") - - new_service = create_service(service_name="service_one") - - template_new_service = create_template(service=new_service) - - db.session.add(StatsTemplateUsageByMonth( - template_id=email_template.id, - month=4, - year=2017, - count=10 - )) - - db.session.add(StatsTemplateUsageByMonth( - template_id=template_new_service.id, - month=4, - year=2017, - count=10 - )) - - result = dao_get_template_usage_stats_by_service(sample_service.id, 2017) - - assert len(result) == 1 - - -def test_dao_get_template_usage_stats_by_service_for_precompiled_letters(sample_service): - - letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) - - precompiled_letter_template = create_template( - service=sample_service, template_name=PRECOMPILED_TEMPLATE_NAME, hidden=True, template_type=LETTER_TYPE) - - db.session.add(StatsTemplateUsageByMonth( - template_id=letter_template.id, - month=5, - year=2017, - count=10 - )) - - db.session.add(StatsTemplateUsageByMonth( - template_id=precompiled_letter_template.id, - month=4, - year=2017, - count=20 - )) - - result = dao_get_template_usage_stats_by_service(sample_service.id, 2017) - - assert len(result) == 2 - assert [ - (letter_template.id, 'letter Template Name', 'letter', False, 5, 2017, 10), - (precompiled_letter_template.id, PRECOMPILED_TEMPLATE_NAME, 'letter', True, 4, 2017, 20) - ] == result - - -def test_dao_get_template_usage_stats_by_service_specific_year(sample_service): - - email_template = create_template(service=sample_service, template_type="email") - - db.session.add(StatsTemplateUsageByMonth( - template_id=email_template.id, - month=3, - year=2017, - count=10 - )) - - db.session.add(StatsTemplateUsageByMonth( - template_id=email_template.id, - month=4, - year=2017, - count=10 - )) - - db.session.add(StatsTemplateUsageByMonth( - template_id=email_template.id, - month=3, - year=2018, - count=10 - )) - - db.session.add(StatsTemplateUsageByMonth( - template_id=email_template.id, - month=4, - year=2018, - count=10 - )) - - result = dao_get_template_usage_stats_by_service(sample_service.id, 2017) - - assert len(result) == 2 - - assert result[0].template_id == email_template.id - assert result[0].name == email_template.name - assert result[0].template_type == email_template.template_type - assert result[0].month == 4 - assert result[0].year == 2017 - assert result[0].count == 10 - - assert result[1].template_id == email_template.id - assert result[1].name == email_template.name - assert result[1].template_type == email_template.template_type - assert result[1].month == 3 - assert result[1].year == 2018 - assert result[1].count == 10 diff --git a/tests/app/service/test_statistics_rest.py b/tests/app/service/test_statistics_rest.py index edf9e5b9a..519612de5 100644 --- a/tests/app/service/test_statistics_rest.py +++ b/tests/app/service/test_statistics_rest.py @@ -4,7 +4,6 @@ from datetime import datetime, date import pytest from freezegun import freeze_time -from app.celery.scheduled_tasks import daily_stats_template_usage_by_month from app.models import ( EMAIL_TYPE, SMS_TYPE, @@ -55,22 +54,6 @@ def test_get_template_usage_by_month_returns_correct_data( assert resp_json[1]["count"] == 1 -@freeze_time('2017-11-11 02:00') -def test_get_template_usage_by_month_returns_no_data(admin_request, sample_template): - create_notification(sample_template, created_at=datetime(2016, 4, 1), status='created') - - daily_stats_template_usage_by_month() - - create_notification(sample_template, created_at=datetime.utcnow()) - - resp_json = admin_request.get( - 'service.get_monthly_template_usage', - service_id=sample_template.service_id, - year=2015 - ) - assert resp_json['stats'] == [] - - @freeze_time('2017-11-11 02:00') def test_get_template_usage_by_month_returns_two_templates(admin_request, sample_template, sample_service): template_one = create_template( From 876346f4693b664f296b4454edb335585eb5bdec Mon Sep 17 00:00:00 2001 From: Alexey Bezhan Date: Mon, 14 Jan 2019 16:58:57 +0000 Subject: [PATCH 11/31] Add an option to group notification stats for 7 days by template Currently, admin app requests service statistics (with notification counts grouped by status) and template statistics (with counts by template) in order to display the service dashboard. Service statistics are gathered from FactNotificationStatus table (counts for the last 7 days) combined with Notification (counts for today). Template statistics are currently gathered from redis cache, which contains a separate counter per template per day. It's hard for us to maintain consistency between redis and DB counts. Currently it doesn't update the count for cancelled letters, counter resets in the middle of the day might produce a wrong result for the rest of the week and cleared redis cache can't be repopulated for services with low data retention periods). Since FactNotificationStatus already contains separate counts for each template_id we can use the existing logic with some additional filters to get separate counts for each template and status combination, which would allow us to populate the service dashboard page from one query response. --- app/dao/fact_notification_status_dao.py | 18 ++++++-- .../dao/test_fact_notification_status_dao.py | 46 ++++++++++++++++++- 2 files changed, 60 insertions(+), 4 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 6bb341409..44c1a7b8b 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -107,12 +107,13 @@ def fetch_notification_status_for_service_for_day(bst_day, service_id): ).all() -def fetch_notification_status_for_service_for_today_and_7_previous_days(service_id, limit_days=7): +def fetch_notification_status_for_service_for_today_and_7_previous_days(service_id, by_template=False, limit_days=7): start_date = midnight_n_days_ago(limit_days) now = datetime.utcnow() stats_for_7_days = db.session.query( FactNotificationStatus.notification_type.label('notification_type'), FactNotificationStatus.notification_status.label('status'), + *([FactNotificationStatus.template_id.label('template_id')] if by_template else []), FactNotificationStatus.notification_count.label('count') ).filter( FactNotificationStatus.service_id == service_id, @@ -123,6 +124,7 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_ stats_for_today = db.session.query( Notification.notification_type.cast(db.Text), Notification.status, + *([Notification.template_id] if by_template else []), func.count().label('count') ).filter( Notification.created_at >= get_london_midnight_in_utc(now), @@ -130,14 +132,24 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_ Notification.key_type != KEY_TYPE_TEST ).group_by( Notification.notification_type, + *([Notification.template_id] if by_template else []), Notification.status ) + all_stats_table = stats_for_7_days.union_all(stats_for_today).subquery() - return db.session.query( + + query = db.session.query( + *([Template.name, Template.is_precompiled_letter, all_stats_table.c.template_id] if by_template else []), all_stats_table.c.notification_type, all_stats_table.c.status, func.cast(func.sum(all_stats_table.c.count), Integer).label('count'), - ).group_by( + ) + + if by_template: + query = query.filter(all_stats_table.c.template_id == Template.id) + + return query.group_by( + *([Template.name, Template.is_precompiled_letter, all_stats_table.c.template_id] if by_template else []), all_stats_table.c.notification_type, all_stats_table.c.status, ).all() diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index a7adffce0..7b5832130 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -2,6 +2,7 @@ from datetime import timedelta, datetime, date from uuid import UUID import pytest +import mock from app.dao.fact_notification_status_dao import ( update_fact_notification_status, @@ -188,6 +189,7 @@ def test_fetch_notification_status_for_service_for_day(notify_db_session): def test_fetch_notification_status_for_service_for_today_and_7_previous_days(notify_db_session): service_1 = create_service(service_name='service_1') sms_template = create_template(service=service_1, template_type=SMS_TYPE) + sms_template_2 = create_template(service=service_1, template_type=SMS_TYPE) email_template = create_template(service=service_1, template_type=EMAIL_TYPE) create_ft_notification_status(date(2018, 10, 29), 'sms', service_1, count=10) @@ -197,6 +199,7 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days(not create_ft_notification_status(date(2018, 10, 26), 'letter', service_1, count=5) create_notification(sms_template, created_at=datetime(2018, 10, 31, 11, 0, 0)) + create_notification(sms_template_2, created_at=datetime(2018, 10, 31, 11, 0, 0)) create_notification(sms_template, created_at=datetime(2018, 10, 31, 12, 0, 0), status='delivered') create_notification(email_template, created_at=datetime(2018, 10, 31, 13, 0, 0), status='delivered') @@ -220,13 +223,54 @@ def test_fetch_notification_status_for_service_for_today_and_7_previous_days(not assert results[2].notification_type == 'sms' assert results[2].status == 'created' - assert results[2].count == 2 + assert results[2].count == 3 assert results[3].notification_type == 'sms' assert results[3].status == 'delivered' assert results[3].count == 19 +@freeze_time('2018-10-31T18:00:00') +def test_fetch_notification_status_by_template_for_service_for_today_and_7_previous_days(notify_db_session): + service_1 = create_service(service_name='service_1') + sms_template = create_template(template_name='sms Template 1', service=service_1, template_type=SMS_TYPE) + sms_template_2 = create_template(template_name='sms Template 2', service=service_1, template_type=SMS_TYPE) + email_template = create_template(service=service_1, template_type=EMAIL_TYPE) + + # create unused email template + create_template(service=service_1, template_type=EMAIL_TYPE) + + create_ft_notification_status(date(2018, 10, 29), 'sms', service_1, count=10) + create_ft_notification_status(date(2018, 10, 29), 'sms', service_1, count=11) + create_ft_notification_status(date(2018, 10, 24), 'sms', service_1, count=8) + create_ft_notification_status(date(2018, 10, 29), 'sms', service_1, notification_status='created') + create_ft_notification_status(date(2018, 10, 29), 'email', service_1, count=3) + create_ft_notification_status(date(2018, 10, 26), 'letter', service_1, count=5) + + create_notification(sms_template, created_at=datetime(2018, 10, 31, 11, 0, 0)) + create_notification(sms_template, created_at=datetime(2018, 10, 31, 12, 0, 0), status='delivered') + create_notification(sms_template_2, created_at=datetime(2018, 10, 31, 12, 0, 0), status='delivered') + create_notification(email_template, created_at=datetime(2018, 10, 31, 13, 0, 0), status='delivered') + + # too early, shouldn't be included + create_notification(service_1.templates[0], created_at=datetime(2018, 10, 30, 12, 0, 0), status='delivered') + + results = fetch_notification_status_for_service_for_today_and_7_previous_days(service_1.id, by_template=True) + + assert [ + ('email Template Name', False, mock.ANY, 'email', 'delivered', 1), + ('email Template Name', False, mock.ANY, 'email', 'delivered', 3), + ('letter Template Name', False, mock.ANY, 'letter', 'delivered', 5), + ('sms Template 1', False, mock.ANY, 'sms', 'created', 1), + ('sms Template Name', False, mock.ANY, 'sms', 'created', 1), + ('sms Template 1', False, mock.ANY, 'sms', 'delivered', 1), + ('sms Template 2', False, mock.ANY, 'sms', 'delivered', 1), + ('sms Template Name', False, mock.ANY, 'sms', 'delivered', 8), + ('sms Template Name', False, mock.ANY, 'sms', 'delivered', 10), + ('sms Template Name', False, mock.ANY, 'sms', 'delivered', 11), + ] == sorted(results, key=lambda x: (x.notification_type, x.status, x.name, x.count)) + + @pytest.mark.parametrize( "start_date, end_date, expected_email, expected_letters, expected_sms, expected_created_sms", [ From 5ebeb9937ad801f0db799acd573867bc168bc404 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Mon, 14 Jan 2019 17:45:56 +0000 Subject: [PATCH 12/31] Avoid call to database to get template in persist_notifications --- app/celery/tasks.py | 1 + app/notifications/process_letter_notifications.py | 1 + app/notifications/process_notifications.py | 10 ++++------ app/notifications/rest.py | 1 + app/service/send_notification.py | 1 + tests/app/notifications/test_process_notification.py | 1 + tests/app/service/test_send_one_off_notification.py | 4 ++++ 7 files changed, 13 insertions(+), 6 deletions(-) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 96d26fe7d..1948c0964 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -307,6 +307,7 @@ def save_letter( saved_notification = persist_notification( template_id=notification['template'], template_version=notification['template_version'], + template_postage=template.postage, recipient=recipient, service=service, personalisation=notification['personalisation'], diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py index 94e52bbd8..06d1127bf 100644 --- a/app/notifications/process_letter_notifications.py +++ b/app/notifications/process_letter_notifications.py @@ -7,6 +7,7 @@ def create_letter_notification(letter_data, template, api_key, status, reply_to_ notification = persist_notification( template_id=template.id, template_version=template.version, + template_postage=template.postage, # we only accept addresses_with_underscores from the API (from CSV we also accept dashes, spaces etc) recipient=letter_data['personalisation']['address_line_1'], service=template.service, diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 88cec5b0d..f8850c3e9 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -32,8 +32,6 @@ from app.dao.notifications_dao import ( dao_created_scheduled_notification ) -from app.dao.templates_dao import dao_get_template_by_id - from app.v2.errors import BadRequestError from app.utils import ( cache_key_for_service_template_counter, @@ -76,7 +74,8 @@ def persist_notification( status=NOTIFICATION_CREATED, reply_to_text=None, billable_units=None, - postage=None + postage=None, + template_postage=None ): notification_created_at = created_at or datetime.utcnow() if not notification_id: @@ -116,9 +115,8 @@ def persist_notification( if postage: notification.postage = postage else: - template = dao_get_template_by_id(template_id, template_version) - if service.has_permission(CHOOSE_POSTAGE) and template.postage: - notification.postage = template.postage + if service.has_permission(CHOOSE_POSTAGE) and template_postage: + notification.postage = template_postage else: notification.postage = service.postage diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 04286688a..aa4be0ea9 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -124,6 +124,7 @@ def send_notification(notification_type): simulated = simulated_recipient(notification_form['to'], notification_type) notification_model = persist_notification(template_id=template.id, template_version=template.version, + template_postage=template.postage, recipient=request.get_json()['to'], service=authenticated_service, personalisation=notification_form.get('personalisation', None), diff --git a/app/service/send_notification.py b/app/service/send_notification.py index a00d151a4..26307b8c3 100644 --- a/app/service/send_notification.py +++ b/app/service/send_notification.py @@ -77,6 +77,7 @@ def send_one_off_notification(service_id, post_data): notification = persist_notification( template_id=template.id, template_version=template.version, + template_postage=template.postage, recipient=post_data['to'], service=service, personalisation=personalisation, diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 192644bde..9710146e0 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -504,6 +504,7 @@ def test_persist_letter_notification_finds_correct_postage( persist_notification( template_id=template.id, template_version=template.version, + template_postage=template.postage, recipient="Jane Doe, 10 Downing Street, London", service=service, personalisation=None, diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 0c20611dc..b70459fc8 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -90,6 +90,7 @@ def test_send_one_off_notification_calls_persist_correctly_for_sms( persist_mock.assert_called_once_with( template_id=template.id, template_version=template.version, + template_postage=None, recipient=post_data['to'], service=template.service, personalisation={'name': 'foo'}, @@ -127,6 +128,7 @@ def test_send_one_off_notification_calls_persist_correctly_for_email( persist_mock.assert_called_once_with( template_id=template.id, template_version=template.version, + template_postage=None, recipient=post_data['to'], service=template.service, personalisation={'name': 'foo'}, @@ -153,6 +155,7 @@ def test_send_one_off_notification_calls_persist_correctly_for_letter( template = create_template( service=service, template_type=LETTER_TYPE, + postage='first', subject="Test subject", content="Hello (( Name))\nYour thing is due soon", ) @@ -174,6 +177,7 @@ def test_send_one_off_notification_calls_persist_correctly_for_letter( persist_mock.assert_called_once_with( template_id=template.id, template_version=template.version, + template_postage='first', recipient=post_data['to'], service=template.service, personalisation=post_data['personalisation'], From 52831813d89dfb237e6cf15a0c0d51cee032bf80 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 15 Jan 2019 11:55:45 +0000 Subject: [PATCH 13/31] Change template statistics endpoint to use fact_notification_status_dao --- app/dao/fact_notification_status_dao.py | 6 +- app/template_statistics/rest.py | 16 +- .../dao/test_fact_notification_status_dao.py | 2 +- tests/app/template_statistics/test_rest.py | 177 +++--------------- 4 files changed, 44 insertions(+), 157 deletions(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 44c1a7b8b..3e4339016 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -139,7 +139,11 @@ def fetch_notification_status_for_service_for_today_and_7_previous_days(service_ all_stats_table = stats_for_7_days.union_all(stats_for_today).subquery() query = db.session.query( - *([Template.name, Template.is_precompiled_letter, all_stats_table.c.template_id] if by_template else []), + *([ + Template.name.label("template_name"), + Template.is_precompiled_letter, + all_stats_table.c.template_id + ] if by_template else []), all_stats_table.c.notification_type, all_stats_table.c.status, func.cast(func.sum(all_stats_table.c.count), Integer).label('count'), diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index 1c0f3b27d..f1c2ff1f0 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -14,6 +14,7 @@ from app.dao.templates_dao import ( dao_get_multiple_template_details, dao_get_template_by_id_and_service_id ) +from app.dao.fact_notification_status_dao import fetch_notification_status_for_service_for_today_and_7_previous_days from app.schemas import notification_with_template_schema from app.utils import cache_key_for_service_template_usage_per_day, last_n_days @@ -39,8 +40,21 @@ def get_template_statistics_for_service_by_day(service_id): if whole_days < 0 or whole_days > 7: raise InvalidRequest({'whole_days': ['whole_days must be between 0 and 7']}, status_code=400) + data = fetch_notification_status_for_service_for_today_and_7_previous_days( + service_id, by_template=True, limit_days=whole_days + ) - return jsonify(data=_get_template_statistics_for_last_n_days(service_id, whole_days)) + return jsonify(data=[ + { + 'count': row.count, + 'template_id': str(row.template_id), + 'template_name': row.template_name, + 'template_type': row.notification_type, + 'is_precompiled_letter': row.is_precompiled_letter, + 'status': row.status + } + for row in data + ]) @template_statistics.route('/') diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index 7b5832130..5267261b5 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -268,7 +268,7 @@ def test_fetch_notification_status_by_template_for_service_for_today_and_7_previ ('sms Template Name', False, mock.ANY, 'sms', 'delivered', 8), ('sms Template Name', False, mock.ANY, 'sms', 'delivered', 10), ('sms Template Name', False, mock.ANY, 'sms', 'delivered', 11), - ] == sorted(results, key=lambda x: (x.notification_type, x.status, x.name, x.count)) + ] == sorted(results, key=lambda x: (x.notification_type, x.status, x.template_name, x.count)) @pytest.mark.parametrize( diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 1a37cc6e7..45659a712 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -1,15 +1,10 @@ import uuid -from datetime import datetime -from unittest.mock import Mock, call, ANY +from unittest.mock import Mock import pytest -from flask import current_app from freezegun import freeze_time -from tests.app.db import ( - create_notification, - create_template, -) +from tests.app.db import create_notification def set_up_get_all_from_hash(mock_redis, side_effect): @@ -80,169 +75,46 @@ def test_get_template_statistics_for_service_by_day_accepts_old_query_string( assert len(json_resp['data']) == 1 -@freeze_time('2018-01-01 12:00:00') -def test_get_template_statistics_for_service_by_day_gets_out_of_redis_if_available( - admin_request, - mocker, - sample_template -): - mock_redis = mocker.patch('app.template_statistics.rest.redis_store') - set_up_get_all_from_hash(mock_redis, [ - {sample_template.id: 3} - ]) - - json_resp = admin_request.get( - 'template_statistics.get_template_statistics_for_service_by_day', - service_id=sample_template.service_id, - whole_days=0 - ) - - assert len(json_resp['data']) == 1 - assert json_resp['data'][0]['count'] == 3 - assert json_resp['data'][0]['template_id'] == str(sample_template.id) - mock_redis.get_all_from_hash.assert_called_once_with( - 'service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-01') - ) - - @freeze_time('2018-01-02 12:00:00') -def test_get_template_statistics_for_service_by_day_goes_to_db_if_not_in_redis( +def test_get_template_statistics_for_service_by_day_goes_to_db( admin_request, mocker, sample_template ): - mock_redis = mocker.patch('app.template_statistics.rest.redis_store') # first time it is called redis returns data, second time returns none - set_up_get_all_from_hash(mock_redis, [ - {sample_template.id: 2}, - None - ]) mock_dao = mocker.patch( - 'app.template_statistics.rest.dao_get_template_usage', + 'app.template_statistics.rest.fetch_notification_status_for_service_for_today_and_7_previous_days', return_value=[ - Mock(id=sample_template.id, count=3) + Mock( + template_id=sample_template.id, + count=3, + template_name=sample_template.name, + notification_type=sample_template.template_type, + status='created', + is_precompiled_letter=False + ) ] ) - json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', service_id=sample_template.service_id, whole_days=1 ) - assert len(json_resp['data']) == 1 - assert json_resp['data'][0]['count'] == 5 - assert json_resp['data'][0]['template_id'] == str(sample_template.id) - # first redis call - assert mock_redis.get_all_from_hash.mock_calls == [ - call('service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-01')), - call('service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-02')) - ] + assert json_resp['data'] == [{ + "template_id": str(sample_template.id), + "count": 3, + "template_name": sample_template.name, + "template_type": sample_template.template_type, + "status": "created", + "is_precompiled_letter": False + + }] # dao only called for 2nd, since redis returned values for first call mock_dao.assert_called_once_with( - str(sample_template.service_id), day=datetime(2018, 1, 2) + str(sample_template.service_id), limit_days=1, by_template=True ) - mock_redis.set_hash_and_expire.assert_called_once_with( - 'service-{}-template-usage-{}'.format(sample_template.service_id, '2018-01-02'), - # sets the data that the dao returned - {str(sample_template.id): 3}, - current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] - ) - - -def test_get_template_statistics_for_service_by_day_combines_templates_correctly( - admin_request, - mocker, - sample_service -): - t1 = create_template(sample_service, template_name='1') - t2 = create_template(sample_service, template_name='2') - t3 = create_template(sample_service, template_name='3') # noqa - mock_redis = mocker.patch('app.template_statistics.rest.redis_store') - - # first time it is called redis returns data, second time returns none - set_up_get_all_from_hash(mock_redis, [ - {t1.id: 2}, - None, - {t1.id: 1, t2.id: 4}, - ]) - mock_dao = mocker.patch( - 'app.template_statistics.rest.dao_get_template_usage', - return_value=[ - Mock(id=t1.id, count=8) - ] - ) - - json_resp = admin_request.get( - 'template_statistics.get_template_statistics_for_service_by_day', - service_id=sample_service.id, - whole_days=2 - ) - - assert len(json_resp['data']) == 2 - assert json_resp['data'][0]['template_id'] == str(t1.id) - assert json_resp['data'][0]['count'] == 11 - assert json_resp['data'][1]['template_id'] == str(t2.id) - assert json_resp['data'][1]['count'] == 4 - - assert mock_redis.get_all_from_hash.call_count == 3 - # dao only called for 2nd day - assert mock_dao.call_count == 1 - - -@freeze_time('2018-03-28 00:00:00') -def test_get_template_statistics_for_service_by_day_gets_stats_for_correct_days( - admin_request, - mocker, - sample_template -): - mock_redis = mocker.patch('app.template_statistics.rest.redis_store') - - # first time it is called redis returns data, second time returns none - set_up_get_all_from_hash(mock_redis, [ - {sample_template.id: 1}, # last weds - None, - {sample_template.id: 1}, - {sample_template.id: 1}, - {sample_template.id: 1}, - {sample_template.id: 1}, - None, - None, # current day - ]) - mock_dao = mocker.patch( - 'app.template_statistics.rest.dao_get_template_usage', - return_value=[ - Mock(id=sample_template.id, count=2) - ] - ) - - json_resp = admin_request.get( - 'template_statistics.get_template_statistics_for_service_by_day', - service_id=sample_template.service_id, - whole_days=7 - ) - - assert len(json_resp['data']) == 1 - assert json_resp['data'][0]['count'] == 11 - assert json_resp['data'][0]['template_id'] == str(sample_template.id) - - assert mock_redis.get_all_from_hash.call_count == 8 - - assert '2018-03-21' in mock_redis.get_all_from_hash.mock_calls[0][1][0] # last wednesday - assert '2018-03-22' in mock_redis.get_all_from_hash.mock_calls[1][1][0] - assert '2018-03-23' in mock_redis.get_all_from_hash.mock_calls[2][1][0] - assert '2018-03-24' in mock_redis.get_all_from_hash.mock_calls[3][1][0] - assert '2018-03-25' in mock_redis.get_all_from_hash.mock_calls[4][1][0] - assert '2018-03-26' in mock_redis.get_all_from_hash.mock_calls[5][1][0] - assert '2018-03-27' in mock_redis.get_all_from_hash.mock_calls[6][1][0] - assert '2018-03-28' in mock_redis.get_all_from_hash.mock_calls[7][1][0] # current day (wednesday) - - mock_dao.mock_calls == [ - call(ANY, day=datetime(2018, 3, 22)), - call(ANY, day=datetime(2018, 3, 27)), - call(ANY, day=datetime(2018, 3, 28)) - ] def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_templates( @@ -250,7 +122,6 @@ def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_tem mocker, sample_service ): - mock_redis = mocker.patch('app.template_statistics.rest.redis_store') json_resp = admin_request.get( 'template_statistics.get_template_statistics_for_service_by_day', @@ -259,9 +130,7 @@ def test_get_template_statistics_for_service_by_day_returns_empty_list_if_no_tem ) assert len(json_resp['data']) == 0 - assert mock_redis.get_all_from_hash.call_count == 8 - # make sure we don't try and set any empty hashes in redis - assert mock_redis.set_hash_and_expire.call_count == 0 + # get_template_statistics_for_template From 3ce0024eeccc773444b2fabe5b9748c4e85415e3 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 15 Jan 2019 12:15:20 +0000 Subject: [PATCH 14/31] Remove unused functions for getting template statistics --- app/dao/notifications_dao.py | 34 ----- app/dao/templates_dao.py | 15 -- app/template_statistics/rest.py | 68 +-------- .../notification_dao/test_notification_dao.py | 2 - .../test_notification_dao_template_usage.py | 136 +----------------- tests/app/dao/test_templates_dao.py | 16 --- 6 files changed, 6 insertions(+), 265 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 9426a28c0..15c9e997a 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -30,7 +30,6 @@ from app.models import ( Notification, NotificationHistory, ScheduledNotification, - Template, KEY_TYPE_TEST, LETTER_TYPE, NOTIFICATION_CREATED, @@ -51,39 +50,6 @@ from app.utils import get_london_midnight_in_utc from app.utils import midnight_n_days_ago, escape_special_characters -@statsd(namespace="dao") -def dao_get_template_usage(service_id, day): - start = get_london_midnight_in_utc(day) - end = get_london_midnight_in_utc(day + timedelta(days=1)) - - notifications_aggregate_query = db.session.query( - func.count().label('count'), - Notification.template_id - ).filter( - Notification.created_at >= start, - Notification.created_at < end, - Notification.service_id == service_id, - Notification.key_type != KEY_TYPE_TEST, - ).group_by( - Notification.template_id - ).subquery() - - query = db.session.query( - Template.id, - Template.name, - Template.template_type, - Template.is_precompiled_letter, - func.coalesce(notifications_aggregate_query.c.count, 0).label('count') - ).outerjoin( - notifications_aggregate_query, - notifications_aggregate_query.c.template_id == Template.id - ).filter( - Template.service_id == service_id - ).order_by(Template.name) - - return query.all() - - @statsd(namespace="dao") def dao_get_last_template_usage(template_id, template_type, service_id): # By adding the service_id to the filter the performance of the query is greatly improved. diff --git a/app/dao/templates_dao.py b/app/dao/templates_dao.py index e5e93199f..66cbe865a 100644 --- a/app/dao/templates_dao.py +++ b/app/dao/templates_dao.py @@ -129,18 +129,3 @@ def dao_get_template_versions(service_id, template_id): ).order_by( desc(TemplateHistory.version) ).all() - - -def dao_get_multiple_template_details(template_ids): - query = db.session.query( - Template.id, - Template.template_type, - Template.name, - Template.is_precompiled_letter - ).filter( - Template.id.in_(template_ids) - ).order_by( - Template.name - ) - - return query.all() diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index f1c2ff1f0..fc179b49a 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -1,25 +1,10 @@ -from flask import ( - Blueprint, - jsonify, - request, - current_app -) - -from app import redis_store -from app.dao.notifications_dao import ( - dao_get_template_usage, - dao_get_last_template_usage -) -from app.dao.templates_dao import ( - dao_get_multiple_template_details, - dao_get_template_by_id_and_service_id -) +from flask import Blueprint, jsonify, request +from app.dao.notifications_dao import dao_get_last_template_usage +from app.dao.templates_dao import dao_get_template_by_id_and_service_id from app.dao.fact_notification_status_dao import fetch_notification_status_for_service_for_today_and_7_previous_days from app.schemas import notification_with_template_schema -from app.utils import cache_key_for_service_template_usage_per_day, last_n_days from app.errors import register_errors, InvalidRequest -from collections import Counter template_statistics = Blueprint('template_statistics', __name__, @@ -67,50 +52,3 @@ def get_template_statistics_for_template_id(service_id, template_id): data = notification_with_template_schema.dump(notification).data return jsonify(data=data) - - -def _get_template_statistics_for_last_n_days(service_id, whole_days): - template_stats_by_id = Counter() - - # 0 whole_days = last 1 days (ie since midnight today) = today. - # 7 whole days = last 8 days (ie since midnight this day last week) = a week and a bit - for day in last_n_days(whole_days + 1): - # "{SERVICE_ID}-template-usage-{YYYY-MM-DD}" - key = cache_key_for_service_template_usage_per_day(service_id, day) - stats = redis_store.get_all_from_hash(key) - if stats: - stats = { - k.decode('utf-8'): int(v) for k, v in stats.items() - } - else: - # key didn't exist (or redis was down) - lets populate from DB. - stats = { - str(row.id): row.count for row in dao_get_template_usage(service_id, day=day) - } - # if there is data in db, but not in redis - lets put it in redis so we don't have to do - # this calc again next time. If there isn't any data, we can't put it in redis. - # Zero length hashes aren't a thing in redis. (There'll only be no data if the service has no templates) - # Nothing is stored if redis is down. - if stats: - redis_store.set_hash_and_expire( - key, - stats, - current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] - ) - template_stats_by_id += Counter(stats) - - # attach count from stats to name/type/etc from database - template_details = dao_get_multiple_template_details(template_stats_by_id.keys()) - return [ - { - 'count': template_stats_by_id[str(template.id)], - 'template_id': str(template.id), - 'template_name': template.name, - 'template_type': template.template_type, - 'is_precompiled_letter': template.is_precompiled_letter - } - for template in template_details - # we don't want to return templates with no count to the front-end, - # but they're returned from the DB and might be put in redis like that (if there was no data that day) - if template_stats_by_id[str(template.id)] != 0 - ] diff --git a/tests/app/dao/notification_dao/test_notification_dao.py b/tests/app/dao/notification_dao/test_notification_dao.py index 197b6556a..90dc73035 100644 --- a/tests/app/dao/notification_dao/test_notification_dao.py +++ b/tests/app/dao/notification_dao/test_notification_dao.py @@ -16,7 +16,6 @@ from app.dao.notifications_dao import ( dao_get_last_template_usage, dao_get_notifications_by_to_field, dao_get_scheduled_notifications, - dao_get_template_usage, dao_timeout_notifications, dao_update_notification, dao_update_notifications_by_reference, @@ -70,7 +69,6 @@ from tests.app.db import ( def test_should_have_decorated_notifications_dao_functions(): assert dao_get_last_template_usage.__wrapped__.__name__ == 'dao_get_last_template_usage' # noqa - assert dao_get_template_usage.__wrapped__.__name__ == 'dao_get_template_usage' # noqa assert dao_create_notification.__wrapped__.__name__ == 'dao_create_notification' # noqa assert update_notification_status_by_id.__wrapped__.__name__ == 'update_notification_status_by_id' # noqa assert dao_update_notification.__wrapped__.__name__ == 'dao_update_notification' # noqa diff --git a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py index 88aaa783d..4006fd9c2 100644 --- a/tests/app/dao/notification_dao/test_notification_dao_template_usage.py +++ b/tests/app/dao/notification_dao/test_notification_dao_template_usage.py @@ -1,23 +1,7 @@ -import uuid -from datetime import datetime, timedelta, date - +from datetime import datetime, timedelta import pytest -from freezegun import freeze_time - -from app.dao.notifications_dao import ( - dao_get_last_template_usage, - dao_get_template_usage -) -from app.models import ( - KEY_TYPE_NORMAL, - KEY_TYPE_TEST, - KEY_TYPE_TEAM -) -from tests.app.db import ( - create_notification, - create_service, - create_template -) +from app.dao.notifications_dao import dao_get_last_template_usage +from tests.app.db import create_notification, create_template def test_last_template_usage_should_get_right_data(sample_notification): @@ -70,117 +54,3 @@ def test_last_template_usage_should_be_able_to_get_no_template_usage_history_if_ sample_template): results = dao_get_last_template_usage(sample_template.id, 'sms', sample_template.service_id) assert not results - - -@freeze_time('2018-01-01') -def test_should_by_able_to_get_template_count(sample_template, sample_email_template): - create_notification(sample_template) - create_notification(sample_template) - create_notification(sample_template) - create_notification(sample_email_template) - create_notification(sample_email_template) - - results = dao_get_template_usage(sample_template.service_id, date.today()) - assert results[0].name == sample_email_template.name - assert results[0].template_type == sample_email_template.template_type - assert results[0].count == 2 - - assert results[1].name == sample_template.name - assert results[1].template_type == sample_template.template_type - assert results[1].count == 3 - - -@freeze_time('2018-01-01') -def test_template_usage_should_ignore_test_keys( - sample_team_api_key, - sample_test_api_key, - sample_api_key, - sample_template -): - - create_notification(sample_template, api_key=sample_api_key, key_type=KEY_TYPE_NORMAL) - create_notification(sample_template, api_key=sample_team_api_key, key_type=KEY_TYPE_TEAM) - create_notification(sample_template, api_key=sample_test_api_key, key_type=KEY_TYPE_TEST) - create_notification(sample_template) - - results = dao_get_template_usage(sample_template.service_id, date.today()) - assert results[0].name == sample_template.name - assert results[0].template_type == sample_template.template_type - assert results[0].count == 3 - - -def test_template_usage_should_filter_by_service(notify_db_session): - service_1 = create_service(service_name='test1') - service_2 = create_service(service_name='test2') - service_3 = create_service(service_name='test3') - - template_1 = create_template(service_1) - template_2 = create_template(service_2) # noqa - template_3a = create_template(service_3, template_name='a') - template_3b = create_template(service_3, template_name='b') # noqa - - # two for service_1, one for service_3 - create_notification(template_1) - create_notification(template_1) - - create_notification(template_3a) - - res1 = dao_get_template_usage(service_1.id, date.today()) - res2 = dao_get_template_usage(service_2.id, date.today()) - res3 = dao_get_template_usage(service_3.id, date.today()) - - assert len(res1) == 1 - assert res1[0].count == 2 - - assert len(res2) == 1 - assert res2[0].count == 0 - - assert len(res3) == 2 - assert res3[0].count == 1 - assert res3[1].count == 0 - - -def test_template_usage_should_by_able_to_get_zero_count_from_notifications_history_if_no_rows(sample_service): - results = dao_get_template_usage(sample_service.id, date.today()) - assert len(results) == 0 - - -def test_template_usage_should_by_able_to_get_zero_count_from_notifications_history_if_no_service(): - results = dao_get_template_usage(str(uuid.uuid4()), date.today()) - assert len(results) == 0 - - -def test_template_usage_should_by_able_to_get_template_count_for_specific_day(sample_template): - # too early - create_notification(sample_template, created_at=datetime(2017, 6, 7, 22, 59, 0)) - # just right - create_notification(sample_template, created_at=datetime(2017, 6, 7, 23, 0, 0)) - create_notification(sample_template, created_at=datetime(2017, 6, 7, 23, 0, 0)) - create_notification(sample_template, created_at=datetime(2017, 6, 8, 22, 59, 0)) - create_notification(sample_template, created_at=datetime(2017, 6, 8, 22, 59, 0)) - create_notification(sample_template, created_at=datetime(2017, 6, 8, 22, 59, 0)) - # too late - create_notification(sample_template, created_at=datetime(2017, 6, 8, 23, 0, 0)) - - results = dao_get_template_usage(sample_template.service_id, day=date(2017, 6, 8)) - - assert len(results) == 1 - assert results[0].count == 5 - - -def test_template_usage_should_by_able_to_get_template_count_for_specific_timezone_boundary(sample_template): - # too early - create_notification(sample_template, created_at=datetime(2018, 3, 24, 23, 59, 0)) - # just right - create_notification(sample_template, created_at=datetime(2018, 3, 25, 0, 0, 0)) - create_notification(sample_template, created_at=datetime(2018, 3, 25, 0, 0, 0)) - create_notification(sample_template, created_at=datetime(2018, 3, 25, 22, 59, 0)) - create_notification(sample_template, created_at=datetime(2018, 3, 25, 22, 59, 0)) - create_notification(sample_template, created_at=datetime(2018, 3, 25, 22, 59, 0)) - # too late - create_notification(sample_template, created_at=datetime(2018, 3, 25, 23, 0, 0)) - - results = dao_get_template_usage(sample_template.service_id, day=date(2018, 3, 25)) - - assert len(results) == 1 - assert results[0].count == 5 diff --git a/tests/app/dao/test_templates_dao.py b/tests/app/dao/test_templates_dao.py index cbe6c6b72..f585e2b5d 100644 --- a/tests/app/dao/test_templates_dao.py +++ b/tests/app/dao/test_templates_dao.py @@ -11,7 +11,6 @@ from app.dao.templates_dao import ( dao_get_all_templates_for_service, dao_update_template, dao_get_template_versions, - dao_get_multiple_template_details, dao_redact_template, dao_update_template_reply_to ) from app.models import ( @@ -511,21 +510,6 @@ def test_get_template_versions_is_empty_for_hidden_templates(notify_db, notify_d assert len(versions) == 0 -def test_get_multiple_template_details_returns_templates_for_list_of_ids(sample_service): - t1 = create_template(sample_service) - t2 = create_template(sample_service) - create_template(sample_service) # t3 - - res = dao_get_multiple_template_details([t1.id, t2.id]) - - assert {x.id for x in res} == {t1.id, t2.id} - # make sure correct properties are on each row - assert res[0].id - assert res[0].template_type - assert res[0].name - assert not res[0].is_precompiled_letter - - @pytest.mark.parametrize("template_type,postage", [('letter', 'third'), ('sms', 'second')]) def test_template_postage_constraint_on_create(sample_service, sample_user, template_type, postage): data = { From d36c4d8a7872a9fd38fb59bb4ec55aeac438594d Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 15 Jan 2019 14:38:45 +0000 Subject: [PATCH 15/31] Remove now unused methods that populated template usage redis cache --- app/commands.py | 54 +------------- app/notifications/process_notifications.py | 19 +---- app/utils.py | 7 -- tests/app/commands/test_populate_redis.py | 73 ------------------- .../test_process_notification.py | 48 ------------ 5 files changed, 4 insertions(+), 197 deletions(-) delete mode 100644 tests/app/commands/test_populate_redis.py diff --git a/app/commands.py b/app/commands.py index 41f58c4dd..b14e5680d 100644 --- a/app/commands.py +++ b/app/commands.py @@ -1,4 +1,3 @@ -import sys import functools import uuid from datetime import datetime, timedelta @@ -9,10 +8,9 @@ import flask from click_datetime import Datetime as click_dt from flask import current_app, json from sqlalchemy.orm.exc import NoResultFound -from sqlalchemy import func from notifications_utils.statsd_decorators import statsd -from app import db, DATETIME_FORMAT, encryption, redis_store +from app import db, DATETIME_FORMAT, encryption from app.celery.scheduled_tasks import send_total_sent_notifications_to_performance_platform from app.celery.service_callback_tasks import send_delivery_status_to_service from app.celery.letters_pdf_tasks import create_letters_pdf @@ -34,11 +32,7 @@ from app.dao.services_dao import ( from app.dao.users_dao import delete_model_user, delete_user_verify_codes from app.models import PROVIDERS, User, Notification from app.performance_platform.processing_time import send_processing_time_for_start_and_end -from app.utils import ( - cache_key_for_service_template_usage_per_day, - get_london_midnight_in_utc, - get_midnight_for_day_before, -) +from app.utils import get_london_midnight_in_utc, get_midnight_for_day_before @click.group(name='command', help='Additional commands') @@ -430,50 +424,6 @@ def migrate_data_to_ft_billing(start_date, end_date): current_app.logger.info('Total inserted/updated records = {}'.format(total_updated)) -@notify_command() -@click.option('-s', '--service_id', required=True, type=click.UUID) -@click.option('-d', '--day', required=True, type=click_dt(format='%Y-%m-%d')) -def populate_redis_template_usage(service_id, day): - """ - Recalculate and replace the stats in redis for a day. - To be used if redis data is lost for some reason. - """ - if not current_app.config['REDIS_ENABLED']: - current_app.logger.error('Cannot populate redis template usage - redis not enabled') - sys.exit(1) - - # the day variable is set by click to be midnight of that day - start_time = get_london_midnight_in_utc(day) - end_time = get_london_midnight_in_utc(day + timedelta(days=1)) - - usage = { - str(row.template_id): row.count - for row in db.session.query( - Notification.template_id, - func.count().label('count') - ).filter( - Notification.service_id == service_id, - Notification.created_at >= start_time, - Notification.created_at < end_time - ).group_by( - Notification.template_id - ) - } - current_app.logger.info('Populating usage dict for service {} day {}: {}'.format( - service_id, - day, - usage.items()) - ) - if usage: - key = cache_key_for_service_template_usage_per_day(service_id, day) - redis_store.set_hash_and_expire( - key, - usage, - current_app.config['EXPIRE_CACHE_EIGHT_DAYS'], - raise_exception=True - ) - - @notify_command(name='rebuild-ft-billing-for-day') @click.option('-s', '--service_id', required=False, type=click.UUID) @click.option('-d', '--day', help="The date to recalculate, as YYYY-MM-DD", required=True, diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 8fc2f15f6..c71fb1631 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -9,7 +9,7 @@ from notifications_utils.recipients import ( validate_and_format_phone_number, format_email_address ) -from notifications_utils.timezones import convert_bst_to_utc, convert_utc_to_bst +from notifications_utils.timezones import convert_bst_to_utc from app import redis_store from app.celery import provider_tasks @@ -35,11 +35,7 @@ from app.dao.notifications_dao import ( from app.dao.templates_dao import dao_get_template_by_id from app.v2.errors import BadRequestError -from app.utils import ( - cache_key_for_service_template_counter, - cache_key_for_service_template_usage_per_day, - get_template_instance, -) +from app.utils import cache_key_for_service_template_counter, get_template_instance def create_content_for_notification(template, personalisation): @@ -127,23 +123,12 @@ def persist_notification( if redis_store.get_all_from_hash(cache_key_for_service_template_counter(service.id)): redis_store.increment_hash_value(cache_key_for_service_template_counter(service.id), template_id) - increment_template_usage_cache(service.id, template_id, notification_created_at) - current_app.logger.info( "{} {} created at {}".format(notification_type, notification_id, notification_created_at) ) return notification -def increment_template_usage_cache(service_id, template_id, created_at): - key = cache_key_for_service_template_usage_per_day(service_id, convert_utc_to_bst(created_at)) - redis_store.increment_hash_value(key, template_id) - # set key to expire in eight days - we don't know if we've just created the key or not, so must assume that we - # have and reset the expiry. Eight days is longer than any notification is in the notifications table, so we'll - # always capture the full week's numbers - redis_store.expire(key, current_app.config['EXPIRE_CACHE_EIGHT_DAYS']) - - def send_notification_to_queue(notification, research_mode, queue=None): if research_mode or notification.key_type == KEY_TYPE_TEST: queue = QueueNames.RESEARCH_MODE diff --git a/app/utils.py b/app/utils.py index b00a53bda..25bbab968 100644 --- a/app/utils.py +++ b/app/utils.py @@ -72,13 +72,6 @@ def cache_key_for_service_template_counter(service_id, limit_days=7): return "{}-template-counter-limit-{}-days".format(service_id, limit_days) -def cache_key_for_service_template_usage_per_day(service_id, datetime): - """ - You should pass a BST datetime into this function - """ - return "service-{}-template-usage-{}".format(service_id, datetime.date().isoformat()) - - def get_public_notify_type_text(notify_type, plural=False): from app.models import (SMS_TYPE, UPLOAD_DOCUMENT, PRECOMPILED_LETTER) notify_type_text = notify_type diff --git a/tests/app/commands/test_populate_redis.py b/tests/app/commands/test_populate_redis.py deleted file mode 100644 index 25001642a..000000000 --- a/tests/app/commands/test_populate_redis.py +++ /dev/null @@ -1,73 +0,0 @@ -from datetime import datetime - -from freezegun import freeze_time -import pytest - -from app.commands import populate_redis_template_usage - -from tests.conftest import set_config -from tests.app.db import create_notification, create_template, create_service - - -def test_populate_redis_template_usage_does_nothing_if_redis_disabled(mocker, notify_api, sample_service): - mock_redis = mocker.patch('app.commands.redis_store') - with set_config(notify_api, 'REDIS_ENABLED', False): - with pytest.raises(SystemExit) as exit_signal: - populate_redis_template_usage.callback.__wrapped__(sample_service.id, datetime.utcnow()) - - assert mock_redis.mock_calls == [] - # sys.exit with nonzero exit code - assert exit_signal.value.code != 0 - - -def test_populate_redis_template_usage_does_nothing_if_no_data(mocker, notify_api, sample_service): - mock_redis = mocker.patch('app.commands.redis_store') - with set_config(notify_api, 'REDIS_ENABLED', True): - populate_redis_template_usage.callback.__wrapped__(sample_service.id, datetime.utcnow()) - - assert mock_redis.mock_calls == [] - - -@freeze_time('2017-06-12') -def test_populate_redis_template_usage_only_populates_for_today(mocker, notify_api, sample_template): - mock_redis = mocker.patch('app.commands.redis_store') - # created at in utc - create_notification(sample_template, created_at=datetime(2017, 6, 9, 23, 0, 0)) - create_notification(sample_template, created_at=datetime(2017, 6, 9, 23, 0, 0)) - create_notification(sample_template, created_at=datetime(2017, 6, 10, 0, 0, 0)) - create_notification(sample_template, created_at=datetime(2017, 6, 10, 23, 0, 0)) # actually on 11th BST - - with set_config(notify_api, 'REDIS_ENABLED', True): - populate_redis_template_usage.callback.__wrapped__(sample_template.service_id, datetime(2017, 6, 10)) - - mock_redis.set_hash_and_expire.assert_called_once_with( - 'service-{}-template-usage-2017-06-10'.format(sample_template.service_id), - {str(sample_template.id): 3}, - notify_api.config['EXPIRE_CACHE_EIGHT_DAYS'], - raise_exception=True - ) - - -@freeze_time('2017-06-12') -def test_populate_redis_template_usage_only_populates_for_given_service(mocker, notify_api, notify_db_session): - mock_redis = mocker.patch('app.commands.redis_store') - # created at in utc - s1 = create_service(service_name='a') - s2 = create_service(service_name='b') - t1 = create_template(s1) - t2 = create_template(s2) - - create_notification(t1, created_at=datetime(2017, 6, 10)) - create_notification(t1, created_at=datetime(2017, 6, 10)) - - create_notification(t2, created_at=datetime(2017, 6, 10)) - - with set_config(notify_api, 'REDIS_ENABLED', True): - populate_redis_template_usage.callback.__wrapped__(s1.id, datetime(2017, 6, 10)) - - mock_redis.set_hash_and_expire.assert_called_once_with( - 'service-{}-template-usage-2017-06-10'.format(s1.id), - {str(t1.id): 2}, - notify_api.config['EXPIRE_CACHE_EIGHT_DAYS'], - raise_exception=True - ) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 192644bde..5d02d7bee 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -213,7 +213,6 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) @freeze_time("2016-01-01 11:09:00.061258") def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(sample_template, sample_api_key, mocker): mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') - mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=None) mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value=None) @@ -229,16 +228,11 @@ def test_persist_notification_doesnt_touch_cache_for_old_keys_that_dont_exist(sa reference="ref" ) mock_incr.assert_not_called() - mock_incr_hash_value.assert_called_once_with( - "service-{}-template-usage-2016-01-01".format(sample_template.service_id), - sample_template.id - ) @freeze_time("2016-01-01 11:09:00.061258") def test_persist_notification_increments_cache_if_key_exists(sample_template, sample_api_key, mocker): mock_incr = mocker.patch('app.notifications.process_notifications.redis_store.incr') - mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=1) mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value={sample_template.id, 1}) @@ -255,10 +249,6 @@ def test_persist_notification_increments_cache_if_key_exists(sample_template, sa reference="ref2") mock_incr.assert_called_once_with(str(sample_template.service_id) + "-2016-01-01-count", ) - assert mock_incr_hash_value.mock_calls == [ - call("{}-template-counter-limit-7-days".format(sample_template.service_id), sample_template.id), - call("service-{}-template-usage-2016-01-01".format(sample_template.service_id), sample_template.id), - ] @pytest.mark.parametrize(( @@ -516,44 +506,6 @@ def test_persist_letter_notification_finds_correct_postage( assert persisted_notification.postage == expected_postage -@pytest.mark.parametrize('utc_time, day_in_key', [ - ('2016-01-01 23:00:00', '2016-01-01'), - ('2016-06-01 22:59:00', '2016-06-01'), - ('2016-06-01 23:00:00', '2016-06-02'), -]) -def test_persist_notification_increments_and_expires_redis_template_usage( - utc_time, - day_in_key, - sample_template, - sample_api_key, - mocker -): - mock_incr_hash_value = mocker.patch('app.notifications.process_notifications.redis_store.increment_hash_value') - mock_expire = mocker.patch('app.notifications.process_notifications.redis_store.expire') - mocker.patch('app.notifications.process_notifications.redis_store.get', return_value=None) - mocker.patch('app.notifications.process_notifications.redis_store.get_all_from_hash', return_value=None) - - with freeze_time(utc_time): - persist_notification( - template_id=sample_template.id, - template_version=sample_template.version, - recipient='+447111111122', - service=sample_template.service, - personalisation={}, - notification_type='sms', - api_key_id=sample_api_key.id, - key_type=sample_api_key.key_type, - ) - mock_incr_hash_value.assert_called_once_with( - 'service-{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key), - sample_template.id - ) - mock_expire.assert_called_once_with( - 'service-{}-template-usage-{}'.format(str(sample_template.service_id), day_in_key), - current_app.config['EXPIRE_CACHE_EIGHT_DAYS'] - ) - - def test_persist_notification_with_billable_units_stores_correct_info( sample_template, ): From ac3832a91860f182b40d4831019b3792ff7ecb04 Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Tue, 15 Jan 2019 14:46:40 +0000 Subject: [PATCH 16/31] Remove old redis template cache --- app/notifications/process_notifications.py | 4 +--- app/utils.py | 4 ---- tests/app/notifications/test_process_notification.py | 6 ------ 3 files changed, 1 insertion(+), 13 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index c71fb1631..8afc5859b 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -35,7 +35,7 @@ from app.dao.notifications_dao import ( from app.dao.templates_dao import dao_get_template_by_id from app.v2.errors import BadRequestError -from app.utils import cache_key_for_service_template_counter, get_template_instance +from app.utils import get_template_instance def create_content_for_notification(template, personalisation): @@ -120,8 +120,6 @@ def persist_notification( if key_type != KEY_TYPE_TEST: if redis_store.get(redis.daily_limit_cache_key(service.id)): redis_store.incr(redis.daily_limit_cache_key(service.id)) - if redis_store.get_all_from_hash(cache_key_for_service_template_counter(service.id)): - redis_store.increment_hash_value(cache_key_for_service_template_counter(service.id), template_id) current_app.logger.info( "{} {} created at {}".format(notification_type, notification_id, notification_created_at) diff --git a/app/utils.py b/app/utils.py index 25bbab968..d8916341f 100644 --- a/app/utils.py +++ b/app/utils.py @@ -68,10 +68,6 @@ def get_london_month_from_utc_column(column): ) -def cache_key_for_service_template_counter(service_id, limit_days=7): - return "{}-template-counter-limit-{}-days".format(service_id, limit_days) - - def get_public_notify_type_text(notify_type, plural=False): from app.models import (SMS_TYPE, UPLOAD_DOCUMENT, PRECOMPILED_LETTER) notify_type_text = notify_type diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 5d02d7bee..9ab89c162 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -1,13 +1,11 @@ import datetime import uuid -from unittest.mock import call import pytest from boto3.exceptions import Boto3Error from sqlalchemy.exc import SQLAlchemyError from freezegun import freeze_time from collections import namedtuple -from flask import current_app from app.models import ( Notification, @@ -25,7 +23,6 @@ from app.notifications.process_notifications import ( simulated_recipient ) from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address -from app.utils import cache_key_for_service_template_counter from app.v2.errors import BadRequestError from tests.app.conftest import sample_api_key as create_api_key @@ -172,8 +169,6 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert Notification.query.count() == 0 assert NotificationHistory.query.count() == 0 mocked_redis = mocker.patch('app.notifications.process_notifications.redis_store.get') - mock_service_template_cache = mocker.patch( - 'app.notifications.process_notifications.redis_store.get_all_from_hash') n_id = uuid.uuid4() created_at = datetime.datetime(2016, 11, 11, 16, 8, 18) persist_notification( @@ -200,7 +195,6 @@ def test_persist_notification_with_optionals(sample_job, sample_api_key, mocker) assert persisted_notification.job_row_number == 10 assert persisted_notification.created_at == created_at mocked_redis.assert_called_once_with(str(sample_job.service_id) + "-2016-01-01-count") - mock_service_template_cache.assert_called_once_with(cache_key_for_service_template_counter(sample_job.service_id)) assert persisted_notification.client_reference == "ref from client" assert persisted_notification.reference is None assert persisted_notification.international is False From a4d89359c527e59bc09dd58c9dbea63042e8dc98 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 15 Jan 2019 16:13:38 +0000 Subject: [PATCH 17/31] Adding a filter to exclude test keys for the template monthly usage query. Added a test. --- app/dao/fact_notification_status_dao.py | 3 ++- .../dao/test_fact_notification_status_dao.py | 18 ++++++++++++++++++ 2 files changed, 20 insertions(+), 1 deletion(-) diff --git a/app/dao/fact_notification_status_dao.py b/app/dao/fact_notification_status_dao.py index 6bb341409..c90e1685c 100644 --- a/app/dao/fact_notification_status_dao.py +++ b/app/dao/fact_notification_status_dao.py @@ -312,7 +312,8 @@ def fetch_monthly_template_usage_for_service(start_date, end_date, service_id): FactNotificationStatus.service_id == service_id, FactNotificationStatus.bst_date >= start_date, FactNotificationStatus.bst_date <= end_date, - FactNotificationStatus.notification_status != NOTIFICATION_CANCELLED + FactNotificationStatus.key_type != KEY_TYPE_TEST, + FactNotificationStatus.notification_status != NOTIFICATION_CANCELLED, ).group_by( FactNotificationStatus.template_id, Template.name, diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index a7adffce0..33579596c 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -461,3 +461,21 @@ def test_fetch_monthly_template_usage_for_service_does_not_include_cancelled_sta ) assert len(results) == 0 + + +@freeze_time('2018-03-30 14:00') +def test_fetch_monthly_template_usage_for_service_does_not_include_test_notifications( + sample_template +): + create_ft_notification_status(bst_date=date(2018, 3, 1), + service=sample_template.service, + template=sample_template, + notification_status='delivered', + key_type='test', + count=15) + create_notification(template=sample_template, created_at=datetime.utcnow(), status='cancelled') + results = fetch_monthly_template_usage_for_service( + datetime(2018, 1, 1), datetime(2018, 3, 31), sample_template.service_id + ) + + assert len(results) == 0 From 3dca36ecfc03ae79805294e24bdf88d6b81168ad Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 15 Jan 2019 16:16:19 +0000 Subject: [PATCH 18/31] Actually test the right thing :) --- tests/app/dao/test_fact_notification_status_dao.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/app/dao/test_fact_notification_status_dao.py b/tests/app/dao/test_fact_notification_status_dao.py index 33579596c..c84f65b06 100644 --- a/tests/app/dao/test_fact_notification_status_dao.py +++ b/tests/app/dao/test_fact_notification_status_dao.py @@ -473,7 +473,10 @@ def test_fetch_monthly_template_usage_for_service_does_not_include_test_notifica notification_status='delivered', key_type='test', count=15) - create_notification(template=sample_template, created_at=datetime.utcnow(), status='cancelled') + create_notification(template=sample_template, + created_at=datetime.utcnow(), + status='delivered', + key_type='test',) results = fetch_monthly_template_usage_for_service( datetime(2018, 1, 1), datetime(2018, 3, 31), sample_template.service_id ) From e148eca6ff033206a6642ad96808ade904a77985 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 15 Jan 2019 16:55:56 +0000 Subject: [PATCH 19/31] Drop stats_template_usage_by_month table as it is no longer needed. --- app/models.py | 42 ------------------- .../0250_drop_stats_template_table.py | 36 ++++++++++++++++ 2 files changed, 36 insertions(+), 42 deletions(-) create mode 100644 migrations/versions/0250_drop_stats_template_table.py diff --git a/app/models.py b/app/models.py index 555a27ddd..b4ef0b8d3 100644 --- a/app/models.py +++ b/app/models.py @@ -1834,48 +1834,6 @@ class AuthType(db.Model): name = db.Column(db.String, primary_key=True) -class StatsTemplateUsageByMonth(db.Model): - __tablename__ = "stats_template_usage_by_month" - - template_id = db.Column( - UUID(as_uuid=True), - db.ForeignKey('templates.id'), - unique=False, - index=True, - nullable=False, - primary_key=True - ) - month = db.Column( - db.Integer, - nullable=False, - index=True, - unique=False, - primary_key=True, - default=datetime.datetime.month - ) - year = db.Column( - db.Integer, - nullable=False, - index=True, - unique=False, - primary_key=True, - default=datetime.datetime.year - ) - count = db.Column( - db.Integer, - nullable=False, - default=0 - ) - - def serialize(self): - return { - 'template_id': str(self.template_id), - 'month': self.month, - 'year': self.year, - 'count': self.count - } - - class DailySortedLetter(db.Model): __tablename__ = "daily_sorted_letter" diff --git a/migrations/versions/0250_drop_stats_template_table.py b/migrations/versions/0250_drop_stats_template_table.py new file mode 100644 index 000000000..f44af5384 --- /dev/null +++ b/migrations/versions/0250_drop_stats_template_table.py @@ -0,0 +1,36 @@ +""" + +Revision ID: 0250_drop_stats_template_table +Revises: 0249_another_letter_org +Create Date: 2019-01-15 16:47:08.049369 + +""" +from alembic import op +import sqlalchemy as sa +from sqlalchemy.dialects import postgresql + +revision = '0250_drop_stats_template_table' +down_revision = '0249_another_letter_org' + + +def upgrade(): + op.drop_index('ix_stats_template_usage_by_month_month', table_name='stats_template_usage_by_month') + op.drop_index('ix_stats_template_usage_by_month_template_id', table_name='stats_template_usage_by_month') + op.drop_index('ix_stats_template_usage_by_month_year', table_name='stats_template_usage_by_month') + op.drop_table('stats_template_usage_by_month') + + +def downgrade(): + op.create_table('stats_template_usage_by_month', + sa.Column('template_id', postgresql.UUID(), autoincrement=False, nullable=False), + sa.Column('month', sa.INTEGER(), autoincrement=False, nullable=False), + sa.Column('year', sa.INTEGER(), autoincrement=False, nullable=False), + sa.Column('count', sa.INTEGER(), autoincrement=False, nullable=False), + sa.ForeignKeyConstraint(['template_id'], ['templates.id'], + name='stats_template_usage_by_month_template_id_fkey'), + sa.PrimaryKeyConstraint('template_id', 'month', 'year', name='stats_template_usage_by_month_pkey') + ) + op.create_index('ix_stats_template_usage_by_month_year', 'stats_template_usage_by_month', ['year'], unique=False) + op.create_index('ix_stats_template_usage_by_month_template_id', 'stats_template_usage_by_month', ['template_id'], + unique=False) + op.create_index('ix_stats_template_usage_by_month_month', 'stats_template_usage_by_month', ['month'], unique=False) From 9ab97d34816148eb689e2861f32f7fccc8a16afd Mon Sep 17 00:00:00 2001 From: Pea Tyczynska Date: Wed, 16 Jan 2019 16:57:57 +0000 Subject: [PATCH 20/31] Return notification postage in response for .post_precompiled_letter_notification --- app/v2/notifications/post_notifications.py | 3 ++- tests/app/v2/notifications/test_post_letter_notifications.py | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index b40e4e4b6..511155a7e 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -94,7 +94,8 @@ def post_precompiled_letter_notification(): resp = { 'id': notification.id, - 'reference': notification.client_reference + 'reference': notification.client_reference, + 'postage': notification.postage } return jsonify(resp), 201 diff --git a/tests/app/v2/notifications/test_post_letter_notifications.py b/tests/app/v2/notifications/test_post_letter_notifications.py index db6cd4159..22232aa73 100644 --- a/tests/app/v2/notifications/test_post_letter_notifications.py +++ b/tests/app/v2/notifications/test_post_letter_notifications.py @@ -510,7 +510,7 @@ def test_post_precompiled_letter_notification_returns_201( assert notification_history.postage == expected_postage resp_json = json.loads(response.get_data(as_text=True)) - assert resp_json == {'id': str(notification.id), 'reference': 'letter-reference'} + assert resp_json == {'id': str(notification.id), 'reference': 'letter-reference', 'postage': expected_postage} def test_post_letter_notification_throws_error_for_invalid_postage(client, notify_user, mocker): From 4427827b2ff7cc790d1e3400a9eeca7b8c22b991 Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Tue, 15 Jan 2019 17:42:14 +0000 Subject: [PATCH 21/31] Handle celery PIDs more reliably This addresses some problems that existed in the previous approach: 1. There was a race condition that could occur between the time we were looking for the existence of the .pid files and actually reading them. 2. If for some reason the .pid file was left behind after a process had died, the script would never know because we do: kill -s ${1} ${APP_PID} || true --- scripts/run_multi_worker_app_paas.sh | 17 +++++++---------- 1 file changed, 7 insertions(+), 10 deletions(-) diff --git a/scripts/run_multi_worker_app_paas.sh b/scripts/run_multi_worker_app_paas.sh index 5ddc933ec..f195e59dd 100755 --- a/scripts/run_multi_worker_app_paas.sh +++ b/scripts/run_multi_worker_app_paas.sh @@ -54,7 +54,7 @@ function on_exit { # https://unix.stackexchange.com/a/298942/230401 PROCESS_COUNT="${#APP_PIDS[@]}" if [[ "${PROCESS_COUNT}" -eq "0" ]]; then - echo "No more .pid files found, exiting" + echo "No celery process is running any more, exiting" return 0 fi @@ -66,21 +66,18 @@ function on_exit { } function get_celery_pids { - if [[ $(ls /home/vcap/app/celery*.pid) ]]; then - APP_PIDS=`cat /home/vcap/app/celery*.pid` - else - APP_PIDS=() - fi + # get the PIDs of the process whose parent is the root process + # print only pid and their command, get the ones with "celery" in their name + # and keep only these PIDs + APP_PIDS=$(pgrep -P 1 | xargs ps -o pid=,command= -p | grep celery | cut -f1 -d/) } function send_signal_to_celery_processes { # refresh pids to account for the case that some workers may have terminated but others not get_celery_pids # send signal to all remaining apps - for APP_PID in ${APP_PIDS}; do - echo "Sending signal ${1} to process with pid ${APP_PID}" - kill -s ${1} ${APP_PID} || true - done + echo ${APP_PIDS} | tr -d '\n' | tr -s ' ' | xargs echo "Sending signal ${1} to processes with pids: " + echo ${APP_PIDS} | xargs kill -s ${1} } function start_application { From b23851226066d2968a02560f20f56bc07347fdb7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 17 Jan 2019 17:05:14 +0000 Subject: [PATCH 22/31] Add 5 new letter logos --- .../versions/0251_another_letter_org.py | 39 +++++++++++++++++++ 1 file changed, 39 insertions(+) create mode 100644 migrations/versions/0251_another_letter_org.py diff --git a/migrations/versions/0251_another_letter_org.py b/migrations/versions/0251_another_letter_org.py new file mode 100644 index 000000000..2344da9d5 --- /dev/null +++ b/migrations/versions/0251_another_letter_org.py @@ -0,0 +1,39 @@ +"""empty message + +Revision ID: 0251_another_letter_org +Revises: 0250_drop_stats_template_table + +""" + +# revision identifiers, used by Alembic. +revision = '0251_another_letter_org' +down_revision = '0250_drop_stats_template_table' + +from alembic import op + + +NEW_ORGANISATIONS = [ + ('522', 'Anglesey Council', 'anglesey'), + ('523', 'Angus Council', 'angus'), + ('524', 'Cheshire East Council', 'cheshire-east'), + ('525', 'Newham Council', 'newham'), + ('526', 'Warwickshire Council', 'warwickshire'), +] + + +def upgrade(): + for numeric_id, name, filename in NEW_ORGANISATIONS: + op.execute(""" + INSERT + INTO dvla_organisation + VALUES ('{}', '{}', '{}') + """.format(numeric_id, name, filename)) + + +def downgrade(): + for numeric_id, _, _ in NEW_ORGANISATIONS: + op.execute(""" + DELETE + FROM dvla_organisation + WHERE id = '{}' + """.format(numeric_id)) From 6ac1f39fd0a8becc270e0b727ac983ad6b88609c Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 17 Jan 2019 17:20:21 +0000 Subject: [PATCH 23/31] Remove dao_fetch_monthly_historical_stats_by_template, a query using NotificationHistory that is no longer used. --- app/dao/services_dao.py | 29 +++-------------------------- tests/app/dao/test_services_dao.py | 26 -------------------------- 2 files changed, 3 insertions(+), 52 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 342ba7dfb..52536c116 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -1,8 +1,8 @@ import uuid -from datetime import date, datetime, timedelta, time +from datetime import date, datetime, timedelta from notifications_utils.statsd_decorators import statsd -from sqlalchemy import asc, func, extract +from sqlalchemy import asc, func from sqlalchemy.orm import joinedload from flask import current_app @@ -35,7 +35,7 @@ from app.models import ( SMS_TYPE, LETTER_TYPE, ) -from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc, midnight_n_days_ago +from app.utils import get_london_midnight_in_utc, midnight_n_days_ago DEFAULT_SERVICE_PERMISSIONS = [ SMS_TYPE, @@ -364,26 +364,3 @@ def dao_fetch_active_users_for_service(service_id): ) return query.all() - - -@statsd(namespace="dao") -def dao_fetch_monthly_historical_stats_by_template(): - month = get_london_month_from_utc_column(NotificationHistory.created_at) - year = func.date_trunc("year", NotificationHistory.created_at) - end_date = datetime.combine(date.today(), time.min) - - return db.session.query( - NotificationHistory.template_id, - extract('month', month).label('month'), - extract('year', year).label('year'), - func.count().label('count') - ).filter( - NotificationHistory.created_at < end_date - ).group_by( - NotificationHistory.template_id, - month, - year - ).order_by( - year, - month - ).all() diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 766f2ef59..4a1ec0b4b 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -30,7 +30,6 @@ from app.dao.services_dao import ( dao_resume_service, dao_fetch_active_users_for_service, dao_fetch_service_by_inbound_number, - dao_fetch_monthly_historical_stats_by_template, ) from app.dao.users_dao import save_model_user, create_user_code from app.models import ( @@ -874,31 +873,6 @@ def _assert_service_permissions(service_permissions, expected): assert set(expected) == set(p.permission for p in service_permissions) -def test_dao_fetch_monthly_historical_stats_by_template(notify_db_session): - service = create_service() - template_one = create_template(service=service, template_name='1') - template_two = create_template(service=service, template_name='2') - - create_notification(created_at=datetime(2017, 10, 1), template=template_one, status='delivered') - create_notification(created_at=datetime(2016, 4, 1), template=template_two, status='delivered') - create_notification(created_at=datetime(2016, 4, 1), template=template_two, status='delivered') - create_notification(created_at=datetime.now(), template=template_two, status='delivered') - - result = sorted(dao_fetch_monthly_historical_stats_by_template(), key=lambda x: (x.month, x.year)) - - assert len(result) == 2 - - assert result[0].template_id == template_two.id - assert result[0].month == 4 - assert result[0].year == 2016 - assert result[0].count == 2 - - assert result[1].template_id == template_one.id - assert result[1].month == 10 - assert result[1].year == 2017 - assert result[1].count == 1 - - def create_email_sms_letter_template(): service = create_service() template_one = create_template(service=service, template_name='1', template_type='email') From d3d56a322459452787e0d4d2890823f9cb45283f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 14 Jan 2019 17:22:41 +0000 Subject: [PATCH 24/31] separate nightly tasks and other scheduled tasks. other tasks is anything that is run on a different frequency than nightly --- app/celery/nightly_tasks.py | 317 +++++++++++++++++++++++ app/celery/scheduled_tasks.py | 306 +--------------------- app/commands.py | 2 +- app/config.py | 3 +- tests/app/celery/test_scheduled_tasks.py | 18 ++ tests/app/celery/test_tasks.py | 19 -- 6 files changed, 340 insertions(+), 325 deletions(-) create mode 100644 app/celery/nightly_tasks.py diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py new file mode 100644 index 000000000..a917ebfd0 --- /dev/null +++ b/app/celery/nightly_tasks.py @@ -0,0 +1,317 @@ +from datetime import ( + datetime, + timedelta +) + +import pytz +from flask import current_app +from notifications_utils.statsd_decorators import statsd +from sqlalchemy import func +from sqlalchemy.exc import SQLAlchemyError + +from app import notify_celery, performance_platform_client, zendesk_client +from app.aws import s3 +from app.celery.service_callback_tasks import ( + send_delivery_status_to_service, + 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.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, +) +from app.dao.service_callback_api_dao import get_service_delivery_status_callback_api_for_service +from app.exceptions import NotificationTechnicalFailureException +from app.models import ( + Notification, + NOTIFICATION_SENDING, + LETTER_TYPE, + KEY_TYPE_NORMAL +) +from app.performance_platform import total_sent_notifications, processing_time + + +@notify_celery.task(name="remove_csv_files") +@statsd(namespace="tasks") +def remove_csv_files(job_types): + jobs = dao_get_jobs_older_than_data_retention(notification_types=job_types) + for job in jobs: + s3.remove_job_from_s3(job.service_id, job.id) + dao_archive_job(job) + current_app.logger.info("Job ID {} has been removed from s3.".format(job.id)) + + +@notify_celery.task(name="delete-sms-notifications") +@statsd(namespace="tasks") +def delete_sms_notifications_older_than_seven_days(): + try: + start = datetime.utcnow() + deleted = delete_notifications_created_more_than_a_week_ago_by_type('sms') + current_app.logger.info( + "Delete {} job started {} finished {} deleted {} sms notifications".format( + 'sms', + start, + datetime.utcnow(), + deleted + ) + ) + except SQLAlchemyError: + current_app.logger.exception("Failed to delete sms notifications") + raise + + +@notify_celery.task(name="delete-email-notifications") +@statsd(namespace="tasks") +def delete_email_notifications_older_than_seven_days(): + try: + start = datetime.utcnow() + deleted = delete_notifications_created_more_than_a_week_ago_by_type('email') + current_app.logger.info( + "Delete {} job started {} finished {} deleted {} email notifications".format( + 'email', + start, + datetime.utcnow(), + deleted + ) + ) + except SQLAlchemyError: + current_app.logger.exception("Failed to delete email notifications") + raise + + +@notify_celery.task(name="delete-letter-notifications") +@statsd(namespace="tasks") +def delete_letter_notifications_older_than_seven_days(): + try: + start = datetime.utcnow() + deleted = delete_notifications_created_more_than_a_week_ago_by_type('letter') + current_app.logger.info( + "Delete {} job started {} finished {} deleted {} letter notifications".format( + 'letter', + start, + datetime.utcnow(), + deleted + ) + ) + except SQLAlchemyError: + current_app.logger.exception("Failed to delete letter notifications") + raise + + +@notify_celery.task(name='timeout-sending-notifications') +@statsd(namespace="tasks") +def timeout_notifications(): + technical_failure_notifications, temporary_failure_notifications = \ + dao_timeout_notifications(current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD')) + + notifications = technical_failure_notifications + temporary_failure_notifications + for notification in notifications: + # queue callback task only if the service_callback_api exists + service_callback_api = get_service_delivery_status_callback_api_for_service(service_id=notification.service_id) + if service_callback_api: + encrypted_notification = create_delivery_status_callback_data(notification, service_callback_api) + send_delivery_status_to_service.apply_async([str(notification.id), encrypted_notification], + queue=QueueNames.CALLBACKS) + + current_app.logger.info( + "Timeout period reached for {} notifications, status has been updated.".format(len(notifications))) + if technical_failure_notifications: + message = "{} notifications have been updated to technical-failure because they " \ + "have timed out and are still in created.Notification ids: {}".format( + len(technical_failure_notifications), [str(x.id) for x in technical_failure_notifications]) + raise NotificationTechnicalFailureException(message) + + +@notify_celery.task(name='send-daily-performance-platform-stats') +@statsd(namespace="tasks") +def send_daily_performance_platform_stats(): + if performance_platform_client.active: + yesterday = datetime.utcnow() - timedelta(days=1) + send_total_sent_notifications_to_performance_platform(yesterday) + processing_time.send_processing_time_to_performance_platform() + + +def send_total_sent_notifications_to_performance_platform(day): + count_dict = total_sent_notifications.get_total_sent_notifications_for_day(day) + email_sent_count = count_dict.get('email').get('count') + sms_sent_count = count_dict.get('sms').get('count') + letter_sent_count = count_dict.get('letter').get('count') + start_date = count_dict.get('start_date') + + current_app.logger.info( + "Attempting to update Performance Platform for {} with {} emails, {} text messages and {} letters" + .format(start_date, email_sent_count, sms_sent_count, letter_sent_count) + ) + + total_sent_notifications.send_total_notifications_sent_for_day_stats( + start_date, + 'sms', + sms_sent_count + ) + + total_sent_notifications.send_total_notifications_sent_for_day_stats( + start_date, + 'email', + email_sent_count + ) + + total_sent_notifications.send_total_notifications_sent_for_day_stats( + start_date, + 'letter', + letter_sent_count + ) + + +@notify_celery.task(name="delete-inbound-sms") +@statsd(namespace="tasks") +def delete_inbound_sms_older_than_seven_days(): + try: + start = datetime.utcnow() + deleted = delete_inbound_sms_created_more_than_a_week_ago() + current_app.logger.info( + "Delete inbound sms job started {} finished {} deleted {} inbound sms notifications".format( + start, + datetime.utcnow(), + deleted + ) + ) + except SQLAlchemyError: + current_app.logger.exception("Failed to delete inbound sms notifications") + raise + + +@notify_celery.task(name="remove_transformed_dvla_files") +@statsd(namespace="tasks") +def remove_transformed_dvla_files(): + jobs = dao_get_jobs_older_than_data_retention(notification_types=[LETTER_TYPE]) + for job in jobs: + s3.remove_transformed_dvla_file(job.id) + current_app.logger.info("Transformed dvla file for job {} has been removed from s3.".format(job.id)) + + +@notify_celery.task(name="delete_dvla_response_files") +@statsd(namespace="tasks") +def delete_dvla_response_files_older_than_seven_days(): + try: + start = datetime.utcnow() + bucket_objects = s3.get_s3_bucket_objects( + current_app.config['DVLA_RESPONSE_BUCKET_NAME'], + 'root/dispatch' + ) + older_than_seven_days = s3.filter_s3_bucket_objects_within_date_range(bucket_objects) + + for f in older_than_seven_days: + s3.remove_s3_object(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], f['Key']) + + current_app.logger.info( + "Delete dvla response files started {} finished {} deleted {} files".format( + start, + datetime.utcnow(), + len(older_than_seven_days) + ) + ) + except SQLAlchemyError: + current_app.logger.exception("Failed to delete dvla response files") + raise + + +@notify_celery.task(name="raise-alert-if-letter-notifications-still-sending") +@statsd(namespace="tasks") +def raise_alert_if_letter_notifications_still_sending(): + today = datetime.utcnow().date() + + # Do nothing on the weekend + if today.isoweekday() in [6, 7]: + return + + if today.isoweekday() in [1, 2]: + offset_days = 4 + else: + offset_days = 2 + still_sending = Notification.query.filter( + Notification.notification_type == LETTER_TYPE, + Notification.status == NOTIFICATION_SENDING, + Notification.key_type == KEY_TYPE_NORMAL, + func.date(Notification.sent_at) <= today - timedelta(days=offset_days) + ).count() + + if still_sending: + message = "There are {} letters in the 'sending' state from {}".format( + still_sending, + (today - timedelta(days=offset_days)).strftime('%A %d %B') + ) + # Only send alerts in production + if current_app.config['NOTIFY_ENVIRONMENT'] in ['live', 'production', 'test']: + zendesk_client.create_ticket( + subject="[{}] Letters still sending".format(current_app.config['NOTIFY_ENVIRONMENT']), + message=message, + ticket_type=zendesk_client.TYPE_INCIDENT + ) + else: + current_app.logger.info(message) + + +@notify_celery.task(name='raise-alert-if-no-letter-ack-file') +@statsd(namespace="tasks") +def letter_raise_alert_if_no_ack_file_for_zip(): + # get a list of zip files since yesterday + zip_file_set = set() + + for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], + subfolder=datetime.utcnow().strftime('%Y-%m-%d') + '/zips_sent', + suffix='.TXT'): + subname = key.split('/')[-1] # strip subfolder in name + zip_file_set.add(subname.upper().rstrip('.TXT')) + + # get acknowledgement file + ack_file_set = set() + + yesterday = datetime.now(tz=pytz.utc) - timedelta(days=1) # AWS datetime format + + for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['DVLA_RESPONSE_BUCKET_NAME'], + subfolder='root/dispatch', suffix='.ACK.txt', last_modified=yesterday): + ack_file_set.add(key) + + today_str = datetime.utcnow().strftime('%Y%m%d') + + ack_content_set = set() + for key in ack_file_set: + if today_str in key: + content = s3.get_s3_file(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], key) + for zip_file in content.split('\n'): # each line + s = zip_file.split('|') + ack_content_set.add(s[0].upper()) + + message = ( + "Letter ack file does not contain all zip files sent. " + "Missing ack for zip files: {}, " + "pdf bucket: {}, subfolder: {}, " + "ack bucket: {}" + ).format( + str(sorted(zip_file_set - ack_content_set)), + current_app.config['LETTERS_PDF_BUCKET_NAME'], + datetime.utcnow().strftime('%Y-%m-%d') + '/zips_sent', + current_app.config['DVLA_RESPONSE_BUCKET_NAME'] + ) + # strip empty element before comparison + ack_content_set.discard('') + zip_file_set.discard('') + + if len(zip_file_set - ack_content_set) > 0: + if current_app.config['NOTIFY_ENVIRONMENT'] in ['live', 'production', 'test']: + zendesk_client.create_ticket( + subject="Letter acknowledge error", + message=message, + ticket_type=zendesk_client.TYPE_INCIDENT + ) + current_app.logger.error(message) + + if len(ack_content_set - zip_file_set) > 0: + current_app.logger.info( + "letter ack contains zip that is not for today: {}".format(ack_content_set - zip_file_set) + ) diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 818206e76..af072d91b 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -3,34 +3,20 @@ from datetime import ( timedelta ) -import pytz from flask import current_app from notifications_utils.statsd_decorators import statsd -from sqlalchemy import and_, func +from sqlalchemy import and_ from sqlalchemy.exc import SQLAlchemyError from app import notify_celery -from app import performance_platform_client, zendesk_client -from app.aws import s3 -from app.celery.service_callback_tasks import ( - send_delivery_status_to_service, - create_delivery_status_callback_data, -) from app.celery.tasks import process_job from app.config import QueueNames, TaskNames -from app.dao.inbound_sms_dao import delete_inbound_sms_created_more_than_a_week_ago from app.dao.invited_org_user_dao import delete_org_invitations_created_more_than_two_days_ago from app.dao.invited_user_dao import delete_invitations_created_more_than_two_days_ago -from app.dao.jobs_dao import ( - dao_set_scheduled_jobs_to_pending, - dao_get_jobs_older_than_data_retention, - dao_archive_job -) +from app.dao.jobs_dao import dao_set_scheduled_jobs_to_pending from app.dao.jobs_dao import dao_update_job from app.dao.notifications_dao import ( - dao_timeout_notifications, is_delivery_slow_for_provider, - delete_notifications_created_more_than_a_week_ago_by_type, dao_get_scheduled_notifications, set_scheduled_notification_to_processed, notifications_not_yet_sent @@ -39,35 +25,18 @@ from app.dao.provider_details_dao import ( get_current_provider, dao_toggle_sms_provider ) -from app.dao.service_callback_api_dao import get_service_delivery_status_callback_api_for_service from app.dao.users_dao import delete_codes_older_created_more_than_a_day_ago -from app.exceptions import NotificationTechnicalFailureException from app.models import ( Job, - Notification, - NOTIFICATION_SENDING, - LETTER_TYPE, JOB_STATUS_IN_PROGRESS, JOB_STATUS_ERROR, SMS_TYPE, EMAIL_TYPE, - KEY_TYPE_NORMAL ) from app.notifications.process_notifications import send_notification_to_queue -from app.performance_platform import total_sent_notifications, processing_time from app.v2.errors import JobIncompleteError -@notify_celery.task(name="remove_csv_files") -@statsd(namespace="tasks") -def remove_csv_files(job_types): - jobs = dao_get_jobs_older_than_data_retention(notification_types=job_types) - for job in jobs: - s3.remove_job_from_s3(job.service_id, job.id) - dao_archive_job(job) - current_app.logger.info("Job ID {} has been removed from s3.".format(job.id)) - - @notify_celery.task(name="run-scheduled-jobs") @statsd(namespace="tasks") def run_scheduled_jobs(): @@ -109,63 +78,6 @@ def delete_verify_codes(): raise -@notify_celery.task(name="delete-sms-notifications") -@statsd(namespace="tasks") -def delete_sms_notifications_older_than_seven_days(): - try: - start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('sms') - current_app.logger.info( - "Delete {} job started {} finished {} deleted {} sms notifications".format( - 'sms', - start, - datetime.utcnow(), - deleted - ) - ) - except SQLAlchemyError: - current_app.logger.exception("Failed to delete sms notifications") - raise - - -@notify_celery.task(name="delete-email-notifications") -@statsd(namespace="tasks") -def delete_email_notifications_older_than_seven_days(): - try: - start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('email') - current_app.logger.info( - "Delete {} job started {} finished {} deleted {} email notifications".format( - 'email', - start, - datetime.utcnow(), - deleted - ) - ) - except SQLAlchemyError: - current_app.logger.exception("Failed to delete email notifications") - raise - - -@notify_celery.task(name="delete-letter-notifications") -@statsd(namespace="tasks") -def delete_letter_notifications_older_than_seven_days(): - try: - start = datetime.utcnow() - deleted = delete_notifications_created_more_than_a_week_ago_by_type('letter') - current_app.logger.info( - "Delete {} job started {} finished {} deleted {} letter notifications".format( - 'letter', - start, - datetime.utcnow(), - deleted - ) - ) - except SQLAlchemyError: - current_app.logger.exception("Failed to delete letter notifications") - raise - - @notify_celery.task(name="delete-invitations") @statsd(namespace="tasks") def delete_invitations(): @@ -181,70 +93,6 @@ def delete_invitations(): raise -@notify_celery.task(name='timeout-sending-notifications') -@statsd(namespace="tasks") -def timeout_notifications(): - technical_failure_notifications, temporary_failure_notifications = \ - dao_timeout_notifications(current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD')) - - notifications = technical_failure_notifications + temporary_failure_notifications - for notification in notifications: - # queue callback task only if the service_callback_api exists - service_callback_api = get_service_delivery_status_callback_api_for_service(service_id=notification.service_id) - if service_callback_api: - encrypted_notification = create_delivery_status_callback_data(notification, service_callback_api) - send_delivery_status_to_service.apply_async([str(notification.id), encrypted_notification], - queue=QueueNames.CALLBACKS) - - current_app.logger.info( - "Timeout period reached for {} notifications, status has been updated.".format(len(notifications))) - if technical_failure_notifications: - message = "{} notifications have been updated to technical-failure because they " \ - "have timed out and are still in created.Notification ids: {}".format( - len(technical_failure_notifications), [str(x.id) for x in technical_failure_notifications]) - raise NotificationTechnicalFailureException(message) - - -@notify_celery.task(name='send-daily-performance-platform-stats') -@statsd(namespace="tasks") -def send_daily_performance_platform_stats(): - if performance_platform_client.active: - yesterday = datetime.utcnow() - timedelta(days=1) - send_total_sent_notifications_to_performance_platform(yesterday) - processing_time.send_processing_time_to_performance_platform() - - -def send_total_sent_notifications_to_performance_platform(day): - count_dict = total_sent_notifications.get_total_sent_notifications_for_day(day) - email_sent_count = count_dict.get('email').get('count') - sms_sent_count = count_dict.get('sms').get('count') - letter_sent_count = count_dict.get('letter').get('count') - start_date = count_dict.get('start_date') - - current_app.logger.info( - "Attempting to update Performance Platform for {} with {} emails, {} text messages and {} letters" - .format(start_date, email_sent_count, sms_sent_count, letter_sent_count) - ) - - total_sent_notifications.send_total_notifications_sent_for_day_stats( - start_date, - 'sms', - sms_sent_count - ) - - total_sent_notifications.send_total_notifications_sent_for_day_stats( - start_date, - 'email', - email_sent_count - ) - - total_sent_notifications.send_total_notifications_sent_for_day_stats( - start_date, - 'letter', - letter_sent_count - ) - - @notify_celery.task(name='switch-current-sms-provider-on-slow-delivery') @statsd(namespace="tasks") def switch_current_sms_provider_on_slow_delivery(): @@ -273,95 +121,6 @@ def switch_current_sms_provider_on_slow_delivery(): dao_toggle_sms_provider(current_provider.identifier) -@notify_celery.task(name="delete-inbound-sms") -@statsd(namespace="tasks") -def delete_inbound_sms_older_than_seven_days(): - try: - start = datetime.utcnow() - deleted = delete_inbound_sms_created_more_than_a_week_ago() - current_app.logger.info( - "Delete inbound sms job started {} finished {} deleted {} inbound sms notifications".format( - start, - datetime.utcnow(), - deleted - ) - ) - except SQLAlchemyError: - current_app.logger.exception("Failed to delete inbound sms notifications") - raise - - -@notify_celery.task(name="remove_transformed_dvla_files") -@statsd(namespace="tasks") -def remove_transformed_dvla_files(): - jobs = dao_get_jobs_older_than_data_retention(notification_types=[LETTER_TYPE]) - for job in jobs: - s3.remove_transformed_dvla_file(job.id) - current_app.logger.info("Transformed dvla file for job {} has been removed from s3.".format(job.id)) - - -@notify_celery.task(name="delete_dvla_response_files") -@statsd(namespace="tasks") -def delete_dvla_response_files_older_than_seven_days(): - try: - start = datetime.utcnow() - bucket_objects = s3.get_s3_bucket_objects( - current_app.config['DVLA_RESPONSE_BUCKET_NAME'], - 'root/dispatch' - ) - older_than_seven_days = s3.filter_s3_bucket_objects_within_date_range(bucket_objects) - - for f in older_than_seven_days: - s3.remove_s3_object(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], f['Key']) - - current_app.logger.info( - "Delete dvla response files started {} finished {} deleted {} files".format( - start, - datetime.utcnow(), - len(older_than_seven_days) - ) - ) - except SQLAlchemyError: - current_app.logger.exception("Failed to delete dvla response files") - raise - - -@notify_celery.task(name="raise-alert-if-letter-notifications-still-sending") -@statsd(namespace="tasks") -def raise_alert_if_letter_notifications_still_sending(): - today = datetime.utcnow().date() - - # Do nothing on the weekend - if today.isoweekday() in [6, 7]: - return - - if today.isoweekday() in [1, 2]: - offset_days = 4 - else: - offset_days = 2 - still_sending = Notification.query.filter( - Notification.notification_type == LETTER_TYPE, - Notification.status == NOTIFICATION_SENDING, - Notification.key_type == KEY_TYPE_NORMAL, - func.date(Notification.sent_at) <= today - timedelta(days=offset_days) - ).count() - - if still_sending: - message = "There are {} letters in the 'sending' state from {}".format( - still_sending, - (today - timedelta(days=offset_days)).strftime('%A %d %B') - ) - # Only send alerts in production - if current_app.config['NOTIFY_ENVIRONMENT'] in ['live', 'production', 'test']: - zendesk_client.create_ticket( - subject="[{}] Letters still sending".format(current_app.config['NOTIFY_ENVIRONMENT']), - message=message, - ticket_type=zendesk_client.TYPE_INCIDENT - ) - else: - current_app.logger.info(message) - - @notify_celery.task(name='check-job-status') @statsd(namespace="tasks") def check_job_status(): @@ -401,67 +160,6 @@ def check_job_status(): raise JobIncompleteError("Job(s) {} have not completed.".format(job_ids)) -@notify_celery.task(name='raise-alert-if-no-letter-ack-file') -@statsd(namespace="tasks") -def letter_raise_alert_if_no_ack_file_for_zip(): - # get a list of zip files since yesterday - zip_file_set = set() - - for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], - subfolder=datetime.utcnow().strftime('%Y-%m-%d') + '/zips_sent', - suffix='.TXT'): - subname = key.split('/')[-1] # strip subfolder in name - zip_file_set.add(subname.upper().rstrip('.TXT')) - - # get acknowledgement file - ack_file_set = set() - - yesterday = datetime.now(tz=pytz.utc) - timedelta(days=1) # AWS datetime format - - for key in s3.get_list_of_files_by_suffix(bucket_name=current_app.config['DVLA_RESPONSE_BUCKET_NAME'], - subfolder='root/dispatch', suffix='.ACK.txt', last_modified=yesterday): - ack_file_set.add(key) - - today_str = datetime.utcnow().strftime('%Y%m%d') - - ack_content_set = set() - for key in ack_file_set: - if today_str in key: - content = s3.get_s3_file(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], key) - for zip_file in content.split('\n'): # each line - s = zip_file.split('|') - ack_content_set.add(s[0].upper()) - - message = ( - "Letter ack file does not contain all zip files sent. " - "Missing ack for zip files: {}, " - "pdf bucket: {}, subfolder: {}, " - "ack bucket: {}" - ).format( - str(sorted(zip_file_set - ack_content_set)), - current_app.config['LETTERS_PDF_BUCKET_NAME'], - datetime.utcnow().strftime('%Y-%m-%d') + '/zips_sent', - current_app.config['DVLA_RESPONSE_BUCKET_NAME'] - ) - # strip empty element before comparison - ack_content_set.discard('') - zip_file_set.discard('') - - if len(zip_file_set - ack_content_set) > 0: - if current_app.config['NOTIFY_ENVIRONMENT'] in ['live', 'production', 'test']: - zendesk_client.create_ticket( - subject="Letter acknowledge error", - message=message, - ticket_type=zendesk_client.TYPE_INCIDENT - ) - current_app.logger.error(message) - - if len(ack_content_set - zip_file_set) > 0: - current_app.logger.info( - "letter ack contains zip that is not for today: {}".format(ack_content_set - zip_file_set) - ) - - @notify_celery.task(name='replay-created-notifications') @statsd(namespace="tasks") def replay_created_notifications(): diff --git a/app/commands.py b/app/commands.py index b14e5680d..c977f333b 100644 --- a/app/commands.py +++ b/app/commands.py @@ -11,7 +11,7 @@ from sqlalchemy.orm.exc import NoResultFound from notifications_utils.statsd_decorators import statsd from app import db, DATETIME_FORMAT, encryption -from app.celery.scheduled_tasks import send_total_sent_notifications_to_performance_platform +from app.celery.nightly_tasks import send_total_sent_notifications_to_performance_platform from app.celery.service_callback_tasks import send_delivery_status_to_service from app.celery.letters_pdf_tasks import create_letters_pdf from app.config import QueueNames diff --git a/app/config.py b/app/config.py index f69e30869..b07d04b13 100644 --- a/app/config.py +++ b/app/config.py @@ -159,6 +159,7 @@ class Config(object): CELERY_TASK_SERIALIZER = 'json' CELERY_IMPORTS = ('app.celery.tasks', 'app.celery.scheduled_tasks', 'app.celery.reporting_tasks') CELERYBEAT_SCHEDULE = { + # app/celery/scheduled_tasks.py 'run-scheduled-jobs': { 'task': 'run-scheduled-jobs', 'schedule': crontab(minute=1), @@ -189,7 +190,7 @@ class Config(object): 'schedule': crontab(minute='0, 15, 30, 45'), 'options': {'queue': QueueNames.PERIODIC} }, - # nightly tasks: + # app/celery/nightly_tasks.py 'timeout-sending-notifications': { 'task': 'timeout-sending-notifications', 'schedule': crontab(hour=0, minute=5), diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index e97b4643c..a120741e7 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -905,3 +905,21 @@ def test_replay_created_notifications(notify_db_session, sample_service, mocker) queue='send-email-tasks') sms_delivery_queue.assert_called_once_with([str(old_sms.id)], queue="send-sms-tasks") + + +def test_check_job_status_task_does_not_raise_error(sample_template): + create_job( + template=sample_template, + notification_count=3, + created_at=datetime.utcnow() - timedelta(hours=2), + scheduled_for=datetime.utcnow() - timedelta(minutes=31), + processing_started=datetime.utcnow() - timedelta(minutes=31), + job_status=JOB_STATUS_FINISHED) + create_job( + template=sample_template, + notification_count=3, + created_at=datetime.utcnow() - timedelta(minutes=31), + processing_started=datetime.utcnow() - timedelta(minutes=31), + job_status=JOB_STATUS_FINISHED) + + check_job_status() diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 84ad84cab..13c6f9b4f 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -15,7 +15,6 @@ from notifications_utils.columns import Row from app import (encryption, DATETIME_FORMAT) from app.celery import provider_tasks from app.celery import tasks -from app.celery.scheduled_tasks import check_job_status from app.celery.tasks import ( process_job, process_row, @@ -1396,24 +1395,6 @@ def test_send_inbound_sms_to_service_does_not_retries_if_request_returns_404(not mocked.call_count == 0 -def test_check_job_status_task_does_not_raise_error(sample_template): - create_job( - template=sample_template, - notification_count=3, - created_at=datetime.utcnow() - timedelta(hours=2), - scheduled_for=datetime.utcnow() - timedelta(minutes=31), - processing_started=datetime.utcnow() - timedelta(minutes=31), - job_status=JOB_STATUS_FINISHED) - create_job( - template=sample_template, - notification_count=3, - created_at=datetime.utcnow() - timedelta(minutes=31), - processing_started=datetime.utcnow() - timedelta(minutes=31), - job_status=JOB_STATUS_FINISHED) - - check_job_status() - - def test_process_incomplete_job_sms(mocker, sample_template): mocker.patch('app.celery.tasks.s3.get_job_from_s3', return_value=load_example_csv('multiple_sms')) From d783e2b23632e7f944c41118d0eddd532e646fa3 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 16 Jan 2019 17:32:19 +0000 Subject: [PATCH 25/31] move tests from test_scheduled_tasks to test_nightly_tasks --- tests/app/celery/test_nightly_tasks.py | 583 ++++++++++++++++++++++ tests/app/celery/test_scheduled_tasks.py | 597 +---------------------- 2 files changed, 587 insertions(+), 593 deletions(-) create mode 100644 tests/app/celery/test_nightly_tasks.py diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py new file mode 100644 index 000000000..0720b4b89 --- /dev/null +++ b/tests/app/celery/test_nightly_tasks.py @@ -0,0 +1,583 @@ +from datetime import datetime, timedelta +from functools import partial +from unittest.mock import call, patch, PropertyMock + +import pytest +import pytz +from flask import current_app +from freezegun import freeze_time +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, + raise_alert_if_letter_notifications_still_sending, + _remove_csv_files, + remove_transformed_dvla_files, + s3, + send_daily_performance_platform_stats, + send_total_sent_notifications_to_performance_platform, + timeout_notifications, + letter_raise_alert_if_no_ack_file_for_zip, +) +from app.celery.service_callback_tasks import create_delivery_status_callback_data +from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient +from app.config import QueueNames +from app.exceptions import NotificationTechnicalFailureException +from app.models import ( + LETTER_TYPE, + SMS_TYPE, + EMAIL_TYPE +) +from app.utils import get_london_midnight_in_utc +from tests.app.aws.test_s3 import single_s3_object_stub +from tests.app.db import ( + create_notification, + create_service, + create_template, + create_job, + create_service_callback_api, + create_service_data_retention +) + +from tests.app.conftest import ( + sample_job as create_sample_job, + sample_notification_history as create_notification_history, + datetime_in_past +) + + +def mock_s3_get_list_match(bucket_name, subfolder='', suffix='', last_modified=None): + if subfolder == '2018-01-11/zips_sent': + return ['NOTIFY.20180111175007.ZIP.TXT', 'NOTIFY.20180111175008.ZIP.TXT'] + if subfolder == 'root/dispatch': + return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] + + +def mock_s3_get_list_diff(bucket_name, subfolder='', suffix='', last_modified=None): + if subfolder == '2018-01-11/zips_sent': + return ['NOTIFY.20180111175007.ZIP.TXT', 'NOTIFY.20180111175008.ZIP.TXT', 'NOTIFY.20180111175009.ZIP.TXT', + 'NOTIFY.20180111175010.ZIP.TXT'] + if subfolder == 'root/dispatch': + return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] + + +@freeze_time('2016-10-18T10:00:00') +def test_will_remove_csv_files_for_jobs_older_than_seven_days( + notify_db, notify_db_session, mocker, sample_template +): + """ + Jobs older than seven days are deleted, but only two day's worth (two-day window) + """ + mocker.patch('app.celery.nightly_tasks.s3.remove_job_from_s3') + + seven_days_ago = datetime.utcnow() - timedelta(days=7) + just_under_seven_days = seven_days_ago + timedelta(seconds=1) + eight_days_ago = seven_days_ago - timedelta(days=1) + nine_days_ago = eight_days_ago - timedelta(days=1) + just_under_nine_days = nine_days_ago + timedelta(seconds=1) + nine_days_one_second_ago = nine_days_ago - timedelta(seconds=1) + + create_sample_job(notify_db, notify_db_session, created_at=nine_days_one_second_ago, archived=True) + job1_to_delete = create_sample_job(notify_db, notify_db_session, created_at=eight_days_ago) + job2_to_delete = create_sample_job(notify_db, notify_db_session, created_at=just_under_nine_days) + dont_delete_me_1 = create_sample_job(notify_db, notify_db_session, created_at=seven_days_ago) + create_sample_job(notify_db, notify_db_session, created_at=just_under_seven_days) + + _remove_csv_files(job_types=[sample_template.template_type]) + + assert s3.remove_job_from_s3.call_args_list == [ + call(job1_to_delete.service_id, job1_to_delete.id), + call(job2_to_delete.service_id, job2_to_delete.id), + ] + assert job1_to_delete.archived is True + assert dont_delete_me_1.archived is False + + +@freeze_time('2016-10-18T10:00:00') +def test_will_remove_csv_files_for_jobs_older_than_retention_period( + notify_db, notify_db_session, mocker +): + """ + Jobs older than retention period are deleted, but only two day's worth (two-day window) + """ + 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) + sms_template_service_1 = create_template(service=service_1) + email_template_service_1 = create_template(service=service_1, template_type='email') + + sms_template_service_2 = create_template(service=service_2) + email_template_service_2 = create_template(service=service_2, template_type='email') + + four_days_ago = datetime.utcnow() - timedelta(days=4) + eight_days_ago = datetime.utcnow() - timedelta(days=8) + thirty_one_days_ago = datetime.utcnow() - timedelta(days=31) + + _create_job = partial( + create_sample_job, + notify_db, + notify_db_session, + ) + + job1_to_delete = _create_job(service=service_1, template=sms_template_service_1, created_at=four_days_ago) + job2_to_delete = _create_job(service=service_1, template=email_template_service_1, created_at=eight_days_ago) + _create_job(service=service_1, template=email_template_service_1, created_at=four_days_ago) + + _create_job(service=service_2, template=email_template_service_2, created_at=eight_days_ago) + job3_to_delete = _create_job(service=service_2, template=email_template_service_2, created_at=thirty_one_days_ago) + job4_to_delete = _create_job(service=service_2, template=sms_template_service_2, created_at=eight_days_ago) + + _remove_csv_files(job_types=[SMS_TYPE, EMAIL_TYPE]) + + s3.remove_job_from_s3.assert_has_calls([ + call(job1_to_delete.service_id, job1_to_delete.id), + call(job2_to_delete.service_id, job2_to_delete.id), + call(job3_to_delete.service_id, job3_to_delete.id), + call(job4_to_delete.service_id, job4_to_delete.id) + ], any_order=True) + + +@freeze_time('2017-01-01 10:00:00') +def test_remove_csv_files_filters_by_type(mocker, sample_service): + mocker.patch('app.celery.nightly_tasks.s3.remove_job_from_s3') + """ + Jobs older than seven days are deleted, but only two day's worth (two-day window) + """ + letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) + sms_template = create_template(service=sample_service, template_type=SMS_TYPE) + + eight_days_ago = datetime.utcnow() - timedelta(days=8) + + job_to_delete = create_job(template=letter_template, created_at=eight_days_ago) + create_job(template=sms_template, created_at=eight_days_ago) + + _remove_csv_files(job_types=[LETTER_TYPE]) + + assert s3.remove_job_from_s3.call_args_list == [ + call(job_to_delete.service_id, job_to_delete.id), + ] + + +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.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() + 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.assert_called_once_with('letter') + + +def test_update_status_of_notifications_after_timeout(notify_api, sample_template): + with notify_api.test_request_context(): + not1 = create_notification( + template=sample_template, + status='sending', + created_at=datetime.utcnow() - timedelta( + seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) + not2 = create_notification( + template=sample_template, + status='created', + created_at=datetime.utcnow() - timedelta( + seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) + not3 = create_notification( + template=sample_template, + status='pending', + created_at=datetime.utcnow() - timedelta( + seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) + with pytest.raises(NotificationTechnicalFailureException) as e: + timeout_notifications() + assert str(not2.id) in e.value.message + assert not1.status == 'temporary-failure' + assert not2.status == 'technical-failure' + assert not3.status == 'temporary-failure' + + +def test_not_update_status_of_notification_before_timeout(notify_api, sample_template): + with notify_api.test_request_context(): + not1 = create_notification( + template=sample_template, + status='sending', + created_at=datetime.utcnow() - timedelta( + seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') - 10)) + timeout_notifications() + assert not1.status == 'sending' + + +def test_should_not_update_status_of_letter_notifications(client, sample_letter_template): + created_at = datetime.utcnow() - timedelta(days=5) + not1 = create_notification(template=sample_letter_template, status='sending', created_at=created_at) + not2 = create_notification(template=sample_letter_template, status='created', created_at=created_at) + + timeout_notifications() + + assert not1.status == 'sending' + assert not2.status == 'created' + + +def test_timeout_notifications_sends_status_update_to_service(client, sample_template, mocker): + callback_api = create_service_callback_api(service=sample_template.service) + mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async') + notification = create_notification( + template=sample_template, + status='sending', + created_at=datetime.utcnow() - timedelta( + seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) + timeout_notifications() + + encrypted_data = create_delivery_status_callback_data(notification, callback_api) + mocked.assert_called_once_with([str(notification.id), encrypted_data], queue=QueueNames.CALLBACKS) + + +def test_send_daily_performance_stats_calls_does_not_send_if_inactive(client, mocker): + send_mock = mocker.patch( + 'app.celery.nightly_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') # noqa + + with patch.object( + PerformancePlatformClient, + 'active', + new_callable=PropertyMock + ) as mock_active: + mock_active.return_value = False + send_daily_performance_platform_stats() + + assert send_mock.call_count == 0 + + +@freeze_time("2016-01-11 12:30:00") +def test_send_total_sent_notifications_to_performance_platform_calls_with_correct_totals( + notify_db, + notify_db_session, + sample_template, + mocker +): + perf_mock = mocker.patch( + 'app.celery.nightly_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') # noqa + + notification_history = partial( + create_notification_history, + notify_db, + notify_db_session, + sample_template, + status='delivered' + ) + + notification_history(notification_type='email') + notification_history(notification_type='sms') + + # Create some notifications for the day before + yesterday = datetime(2016, 1, 10, 15, 30, 0, 0) + with freeze_time(yesterday): + notification_history(notification_type='sms') + notification_history(notification_type='sms') + notification_history(notification_type='email') + notification_history(notification_type='email') + notification_history(notification_type='email') + + with patch.object( + PerformancePlatformClient, + 'active', + new_callable=PropertyMock + ) as mock_active: + mock_active.return_value = True + send_total_sent_notifications_to_performance_platform(yesterday) + + perf_mock.assert_has_calls([ + call(get_london_midnight_in_utc(yesterday), 'sms', 2), + call(get_london_midnight_in_utc(yesterday), 'email', 3) + ]) + + +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 + + +@freeze_time('2017-01-01 10:00:00') +def test_remove_dvla_transformed_files_removes_expected_files(mocker, sample_service): + mocker.patch('app.celery.nightly_tasks.s3.remove_transformed_dvla_file') + + letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) + + job = partial(create_job, template=letter_template) + + seven_days_ago = datetime.utcnow() - timedelta(days=7) + just_under_seven_days = seven_days_ago + timedelta(seconds=1) + just_over_seven_days = seven_days_ago - timedelta(seconds=1) + eight_days_ago = seven_days_ago - timedelta(days=1) + nine_days_ago = eight_days_ago - timedelta(days=1) + ten_days_ago = nine_days_ago - timedelta(days=1) + just_under_nine_days = nine_days_ago + timedelta(seconds=1) + just_over_nine_days = nine_days_ago - timedelta(seconds=1) + just_over_ten_days = ten_days_ago - timedelta(seconds=1) + + job(created_at=just_under_seven_days) + job(created_at=just_over_seven_days) + job_to_delete_1 = job(created_at=eight_days_ago) + job_to_delete_2 = job(created_at=nine_days_ago) + job_to_delete_3 = job(created_at=just_under_nine_days) + job_to_delete_4 = job(created_at=just_over_nine_days) + job(created_at=just_over_ten_days) + remove_transformed_dvla_files() + + s3.remove_transformed_dvla_file.assert_has_calls([ + call(job_to_delete_1.id), + call(job_to_delete_2.id), + call(job_to_delete_3.id), + call(job_to_delete_4.id), + ], any_order=True) + + +def test_remove_dvla_transformed_files_does_not_remove_files(mocker, sample_service): + mocker.patch('app.celery.nightly_tasks.s3.remove_transformed_dvla_file') + + letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) + + job = partial(create_job, template=letter_template) + + yesterday = datetime.utcnow() - timedelta(days=1) + six_days_ago = datetime.utcnow() - timedelta(days=6) + seven_days_ago = six_days_ago - timedelta(days=1) + just_over_nine_days = seven_days_ago - timedelta(days=2, seconds=1) + + job(created_at=yesterday) + job(created_at=six_days_ago) + job(created_at=seven_days_ago) + job(created_at=just_over_nine_days) + + remove_transformed_dvla_files() + + s3.remove_transformed_dvla_file.assert_has_calls([]) + + +@freeze_time("2016-01-01 11:00:00") +def test_delete_dvla_response_files_older_than_seven_days_removes_old_files(notify_api, mocker): + AFTER_SEVEN_DAYS = datetime_in_past(days=8) + single_page_s3_objects = [{ + "Contents": [ + single_s3_object_stub('bar/foo1.txt', AFTER_SEVEN_DAYS), + single_s3_object_stub('bar/foo2.txt', AFTER_SEVEN_DAYS), + ] + }] + mocker.patch( + 'app.celery.nightly_tasks.s3.get_s3_bucket_objects', return_value=single_page_s3_objects[0]["Contents"] + ) + remove_s3_mock = mocker.patch('app.celery.nightly_tasks.s3.remove_s3_object') + + delete_dvla_response_files_older_than_seven_days() + + remove_s3_mock.assert_has_calls([ + call(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], single_page_s3_objects[0]["Contents"][0]["Key"]), + call(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], single_page_s3_objects[0]["Contents"][1]["Key"]) + ]) + + +@freeze_time("2016-01-01 11:00:00") +def test_delete_dvla_response_files_older_than_seven_days_does_not_remove_files(notify_api, mocker): + START_DATE = datetime_in_past(days=9) + JUST_BEFORE_START_DATE = datetime_in_past(days=9, seconds=1) + END_DATE = datetime_in_past(days=7) + JUST_AFTER_END_DATE = END_DATE + timedelta(seconds=1) + + single_page_s3_objects = [{ + "Contents": [ + single_s3_object_stub('bar/foo1.txt', JUST_BEFORE_START_DATE), + single_s3_object_stub('bar/foo2.txt', START_DATE), + single_s3_object_stub('bar/foo3.txt', END_DATE), + single_s3_object_stub('bar/foo4.txt', JUST_AFTER_END_DATE), + ] + }] + mocker.patch( + 'app.celery.nightly_tasks.s3.get_s3_bucket_objects', return_value=single_page_s3_objects[0]["Contents"] + ) + remove_s3_mock = mocker.patch('app.celery.nightly_tasks.s3.remove_s3_object') + delete_dvla_response_files_older_than_seven_days() + + remove_s3_mock.assert_not_called() + + +@freeze_time("2018-01-17 17:00:00") +def test_alert_if_letter_notifications_still_sending(sample_letter_template, mocker): + two_days_ago = datetime(2018, 1, 15, 13, 30) + create_notification(template=sample_letter_template, status='sending', sent_at=two_days_ago) + + mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") + + raise_alert_if_letter_notifications_still_sending() + + mock_create_ticket.assert_called_once_with( + subject="[test] Letters still sending", + message="There are 1 letters in the 'sending' state from Monday 15 January", + ticket_type=ZendeskClient.TYPE_INCIDENT + ) + + +def test_alert_if_letter_notifications_still_sending_a_day_ago_no_alert(sample_letter_template, mocker): + today = datetime.utcnow() + one_day_ago = today - timedelta(days=1) + create_notification(template=sample_letter_template, status='sending', sent_at=one_day_ago) + + mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") + + raise_alert_if_letter_notifications_still_sending() + assert not mock_create_ticket.called + + +@freeze_time("2018-01-17 17:00:00") +def test_alert_if_letter_notifications_still_sending_only_alerts_sending(sample_letter_template, mocker): + two_days_ago = datetime(2018, 1, 15, 13, 30) + create_notification(template=sample_letter_template, status='sending', sent_at=two_days_ago) + create_notification(template=sample_letter_template, status='delivered', sent_at=two_days_ago) + create_notification(template=sample_letter_template, status='failed', sent_at=two_days_ago) + + mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") + + raise_alert_if_letter_notifications_still_sending() + + mock_create_ticket.assert_called_once_with( + subject="[test] Letters still sending", + message="There are 1 letters in the 'sending' state from Monday 15 January", + ticket_type='incident' + ) + + +@freeze_time("2018-01-17 17:00:00") +def test_alert_if_letter_notifications_still_sending_alerts_for_older_than_offset(sample_letter_template, mocker): + three_days_ago = datetime(2018, 1, 14, 13, 30) + create_notification(template=sample_letter_template, status='sending', sent_at=three_days_ago) + + mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") + + raise_alert_if_letter_notifications_still_sending() + + mock_create_ticket.assert_called_once_with( + subject="[test] Letters still sending", + message="There are 1 letters in the 'sending' state from Monday 15 January", + ticket_type='incident' + ) + + +@freeze_time("2018-01-14 17:00:00") +def test_alert_if_letter_notifications_still_sending_does_nothing_on_the_weekend(sample_letter_template, mocker): + yesterday = datetime(2018, 1, 13, 13, 30) + create_notification(template=sample_letter_template, status='sending', sent_at=yesterday) + + mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") + + raise_alert_if_letter_notifications_still_sending() + + assert not mock_create_ticket.called + + +@freeze_time("2018-01-15 17:00:00") +def test_monday_alert_if_letter_notifications_still_sending_reports_thursday_letters(sample_letter_template, mocker): + thursday = datetime(2018, 1, 11, 13, 30) + yesterday = datetime(2018, 1, 14, 13, 30) + create_notification(template=sample_letter_template, status='sending', sent_at=thursday) + create_notification(template=sample_letter_template, status='sending', sent_at=yesterday) + + mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") + + raise_alert_if_letter_notifications_still_sending() + + mock_create_ticket.assert_called_once_with( + subject="[test] Letters still sending", + message="There are 1 letters in the 'sending' state from Thursday 11 January", + ticket_type='incident' + ) + + +@freeze_time("2018-01-16 17:00:00") +def test_tuesday_alert_if_letter_notifications_still_sending_reports_friday_letters(sample_letter_template, mocker): + friday = datetime(2018, 1, 12, 13, 30) + yesterday = datetime(2018, 1, 14, 13, 30) + create_notification(template=sample_letter_template, status='sending', sent_at=friday) + create_notification(template=sample_letter_template, status='sending', sent_at=yesterday) + + mock_create_ticket = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") + + raise_alert_if_letter_notifications_still_sending() + + mock_create_ticket.assert_called_once_with( + subject="[test] Letters still sending", + message="There are 1 letters in the 'sending' state from Friday 12 January", + ticket_type='incident' + ) + + +@freeze_time('2018-01-11T23:00:00') +def test_letter_not_raise_alert_if_ack_files_match_zip_list(mocker, notify_db): + mock_file_list = mocker.patch("app.aws.s3.get_list_of_files_by_suffix", side_effect=mock_s3_get_list_match) + mock_get_file = mocker.patch("app.aws.s3.get_s3_file", + return_value='NOTIFY.20180111175007.ZIP|20180111175733\n' + 'NOTIFY.20180111175008.ZIP|20180111175734') + + letter_raise_alert_if_no_ack_file_for_zip() + + yesterday = datetime.now(tz=pytz.utc) - timedelta(days=1) # Datatime format on AWS + subfoldername = datetime.utcnow().strftime('%Y-%m-%d') + '/zips_sent' + assert mock_file_list.call_count == 2 + assert mock_file_list.call_args_list == [ + call(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], subfolder=subfoldername, suffix='.TXT'), + call(bucket_name=current_app.config['DVLA_RESPONSE_BUCKET_NAME'], subfolder='root/dispatch', + suffix='.ACK.txt', last_modified=yesterday), + ] + assert mock_get_file.call_count == 1 + + +@freeze_time('2018-01-11T23:00:00') +def test_letter_raise_alert_if_ack_files_not_match_zip_list(mocker, notify_db): + mock_file_list = mocker.patch("app.aws.s3.get_list_of_files_by_suffix", side_effect=mock_s3_get_list_diff) + mock_get_file = mocker.patch("app.aws.s3.get_s3_file", + return_value='NOTIFY.20180111175007.ZIP|20180111175733\n' + 'NOTIFY.20180111175008.ZIP|20180111175734') + mock_zendesk = mocker.patch("app.celery.nightly_tasks.zendesk_client.create_ticket") + + letter_raise_alert_if_no_ack_file_for_zip() + + assert mock_file_list.call_count == 2 + assert mock_get_file.call_count == 1 + + message = "Letter ack file does not contain all zip files sent. " \ + "Missing ack for zip files: {}, " \ + "pdf bucket: {}, subfolder: {}, " \ + "ack bucket: {}".format(str(['NOTIFY.20180111175009.ZIP', 'NOTIFY.20180111175010.ZIP']), + current_app.config['LETTERS_PDF_BUCKET_NAME'], + datetime.utcnow().strftime('%Y-%m-%d') + '/zips_sent', + current_app.config['DVLA_RESPONSE_BUCKET_NAME']) + + mock_zendesk.assert_called_once_with( + subject="Letter acknowledge error", + message=message, + ticket_type='incident' + ) + + +@freeze_time('2018-01-11T23:00:00') +def test_letter_not_raise_alert_if_no_files_do_not_cause_error(mocker, notify_db): + mock_file_list = mocker.patch("app.aws.s3.get_list_of_files_by_suffix", side_effect=None) + mock_get_file = mocker.patch("app.aws.s3.get_s3_file", + return_value='NOTIFY.20180111175007.ZIP|20180111175733\n' + 'NOTIFY.20180111175008.ZIP|20180111175734') + + letter_raise_alert_if_no_ack_file_for_zip() + + assert mock_file_list.call_count == 2 + assert mock_get_file.call_count == 0 diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index a120741e7..bf4eb9507 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -1,40 +1,20 @@ from datetime import datetime, timedelta -from functools import partial -from unittest.mock import call, patch, PropertyMock +from unittest.mock import call import pytest -import pytz -from flask import current_app from freezegun import freeze_time -from notifications_utils.clients.zendesk.zendesk_client import ZendeskClient from app import db from app.celery import scheduled_tasks from app.celery.scheduled_tasks import ( check_job_status, - delete_dvla_response_files_older_than_seven_days, - delete_email_notifications_older_than_seven_days, - delete_inbound_sms_older_than_seven_days, delete_invitations, - delete_notifications_created_more_than_a_week_ago_by_type, - delete_letter_notifications_older_than_seven_days, - delete_sms_notifications_older_than_seven_days, delete_verify_codes, - raise_alert_if_letter_notifications_still_sending, - remove_csv_files, - remove_transformed_dvla_files, run_scheduled_jobs, - s3, - send_daily_performance_platform_stats, send_scheduled_notifications, - send_total_sent_notifications_to_performance_platform, switch_current_sms_provider_on_slow_delivery, - timeout_notifications, - letter_raise_alert_if_no_ack_file_for_zip, replay_created_notifications ) -from app.celery.service_callback_tasks import create_delivery_status_callback_data -from app.clients.performance_platform.performance_platform_client import PerformancePlatformClient from app.config import QueueNames, TaskNames from app.dao.jobs_dao import dao_get_job_by_id from app.dao.notifications_dao import dao_get_scheduled_notifications @@ -42,31 +22,19 @@ from app.dao.provider_details_dao import ( dao_update_provider_details, get_current_provider ) -from app.exceptions import NotificationTechnicalFailureException from app.models import ( JOB_STATUS_IN_PROGRESS, JOB_STATUS_ERROR, - LETTER_TYPE, - SMS_TYPE, - EMAIL_TYPE + JOB_STATUS_FINISHED, ) -from app.utils import get_london_midnight_in_utc from app.v2.errors import JobIncompleteError -from tests.app.aws.test_s3 import single_s3_object_stub + from tests.app.db import ( create_notification, - create_service, create_template, create_job, - create_service_callback_api, - create_service_data_retention -) - -from tests.app.conftest import ( - sample_job as create_sample_job, - sample_notification_history as create_notification_history, - datetime_in_past ) +from tests.app.conftest import sample_job as create_sample_job def _create_slow_delivery_notification(template, provider='mmg'): @@ -82,31 +50,6 @@ def _create_slow_delivery_notification(template, provider='mmg'): ) -@pytest.mark.skip(reason="This doesn't actually test the celery task wraps the function") -def test_should_have_decorated_tasks_functions(): - """ - TODO: This test needs to be reviewed as this doesn't actually - test that the celery task is wrapping the function. We're also - running similar tests elsewhere which also need review. - """ - assert delete_verify_codes.__wrapped__.__name__ == 'delete_verify_codes' - assert delete_notifications_created_more_than_a_week_ago_by_type.__wrapped__.__name__ == \ - 'delete_notifications_created_more_than_a_week_ago_by_type' - assert timeout_notifications.__wrapped__.__name__ == 'timeout_notifications' - assert delete_invitations.__wrapped__.__name__ == 'delete_invitations' - assert run_scheduled_jobs.__wrapped__.__name__ == 'run_scheduled_jobs' - assert remove_csv_files.__wrapped__.__name__ == 'remove_csv_files' - assert send_daily_performance_platform_stats.__wrapped__.__name__ == 'send_daily_performance_platform_stats' - assert switch_current_sms_provider_on_slow_delivery.__wrapped__.__name__ == \ - 'switch_current_sms_provider_on_slow_delivery' - assert delete_inbound_sms_older_than_seven_days.__wrapped__.__name__ == \ - 'delete_inbound_sms_older_than_seven_days' - assert remove_transformed_dvla_files.__wrapped__.__name__ == \ - 'remove_transformed_dvla_files' - assert delete_dvla_response_files_older_than_seven_days.__wrapped__.__name__ == \ - 'delete_dvla_response_files_older_than_seven_days' - - @pytest.fixture(scope='function') def prepare_current_provider(restore_provider_details): initial_provider = get_current_provider('sms') @@ -115,25 +58,6 @@ def prepare_current_provider(restore_provider_details): db.session.commit() -def test_should_call_delete_sms_notifications_more_than_week_in_task(notify_api, mocker): - mocked = mocker.patch('app.celery.scheduled_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_sms_notifications_older_than_seven_days() - 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.scheduled_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_email_notifications_older_than_seven_days() - 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.scheduled_tasks.delete_notifications_created_more_than_a_week_ago_by_type') - delete_letter_notifications_older_than_seven_days() - mocked.assert_called_once_with('letter') - - def test_should_call_delete_codes_on_delete_verify_codes_task(notify_api, mocker): mocker.patch('app.celery.scheduled_tasks.delete_codes_older_created_more_than_a_day_ago') delete_verify_codes() @@ -146,67 +70,6 @@ def test_should_call_delete_invotations_on_delete_invitations_task(notify_api, m assert scheduled_tasks.delete_invitations_created_more_than_two_days_ago.call_count == 1 -def test_update_status_of_notifications_after_timeout(notify_api, sample_template): - with notify_api.test_request_context(): - not1 = create_notification( - template=sample_template, - status='sending', - created_at=datetime.utcnow() - timedelta( - seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) - not2 = create_notification( - template=sample_template, - status='created', - created_at=datetime.utcnow() - timedelta( - seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) - not3 = create_notification( - template=sample_template, - status='pending', - created_at=datetime.utcnow() - timedelta( - seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) - with pytest.raises(NotificationTechnicalFailureException) as e: - timeout_notifications() - assert str(not2.id) in e.value.message - assert not1.status == 'temporary-failure' - assert not2.status == 'technical-failure' - assert not3.status == 'temporary-failure' - - -def test_not_update_status_of_notification_before_timeout(notify_api, sample_template): - with notify_api.test_request_context(): - not1 = create_notification( - template=sample_template, - status='sending', - created_at=datetime.utcnow() - timedelta( - seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') - 10)) - timeout_notifications() - assert not1.status == 'sending' - - -def test_should_not_update_status_of_letter_notifications(client, sample_letter_template): - created_at = datetime.utcnow() - timedelta(days=5) - not1 = create_notification(template=sample_letter_template, status='sending', created_at=created_at) - not2 = create_notification(template=sample_letter_template, status='created', created_at=created_at) - - timeout_notifications() - - assert not1.status == 'sending' - assert not2.status == 'created' - - -def test_timeout_notifications_sends_status_update_to_service(client, sample_template, mocker): - callback_api = create_service_callback_api(service=sample_template.service) - mocked = mocker.patch('app.celery.service_callback_tasks.send_delivery_status_to_service.apply_async') - notification = create_notification( - template=sample_template, - status='sending', - created_at=datetime.utcnow() - timedelta( - seconds=current_app.config.get('SENDING_NOTIFICATIONS_TIMEOUT_PERIOD') + 10)) - timeout_notifications() - - encrypted_data = create_delivery_status_callback_data(notification, callback_api) - mocked.assert_called_once_with([str(notification.id), encrypted_data], queue=QueueNames.CALLBACKS) - - def test_should_update_scheduled_jobs_and_put_on_queue(notify_db, notify_db_session, mocker): mocked = mocker.patch('app.celery.tasks.process_job.apply_async') @@ -258,143 +121,6 @@ def test_should_update_all_scheduled_jobs_and_put_on_queue(notify_db, notify_db_ ]) -@freeze_time('2016-10-18T10:00:00') -def test_will_remove_csv_files_for_jobs_older_than_seven_days( - notify_db, notify_db_session, mocker, sample_template -): - """ - Jobs older than seven days are deleted, but only two day's worth (two-day window) - """ - mocker.patch('app.celery.scheduled_tasks.s3.remove_job_from_s3') - - seven_days_ago = datetime.utcnow() - timedelta(days=7) - just_under_seven_days = seven_days_ago + timedelta(seconds=1) - eight_days_ago = seven_days_ago - timedelta(days=1) - nine_days_ago = eight_days_ago - timedelta(days=1) - just_under_nine_days = nine_days_ago + timedelta(seconds=1) - nine_days_one_second_ago = nine_days_ago - timedelta(seconds=1) - - create_sample_job(notify_db, notify_db_session, created_at=nine_days_one_second_ago, archived=True) - job1_to_delete = create_sample_job(notify_db, notify_db_session, created_at=eight_days_ago) - job2_to_delete = create_sample_job(notify_db, notify_db_session, created_at=just_under_nine_days) - dont_delete_me_1 = create_sample_job(notify_db, notify_db_session, created_at=seven_days_ago) - create_sample_job(notify_db, notify_db_session, created_at=just_under_seven_days) - - remove_csv_files(job_types=[sample_template.template_type]) - - assert s3.remove_job_from_s3.call_args_list == [ - call(job1_to_delete.service_id, job1_to_delete.id), - call(job2_to_delete.service_id, job2_to_delete.id), - ] - assert job1_to_delete.archived is True - assert dont_delete_me_1.archived is False - - -@freeze_time('2016-10-18T10:00:00') -def test_will_remove_csv_files_for_jobs_older_than_retention_period( - notify_db, notify_db_session, mocker -): - """ - Jobs older than retention period are deleted, but only two day's worth (two-day window) - """ - mocker.patch('app.celery.scheduled_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) - sms_template_service_1 = create_template(service=service_1) - email_template_service_1 = create_template(service=service_1, template_type='email') - - sms_template_service_2 = create_template(service=service_2) - email_template_service_2 = create_template(service=service_2, template_type='email') - - four_days_ago = datetime.utcnow() - timedelta(days=4) - eight_days_ago = datetime.utcnow() - timedelta(days=8) - thirty_one_days_ago = datetime.utcnow() - timedelta(days=31) - - _create_job = partial( - create_sample_job, - notify_db, - notify_db_session, - ) - - job1_to_delete = _create_job(service=service_1, template=sms_template_service_1, created_at=four_days_ago) - job2_to_delete = _create_job(service=service_1, template=email_template_service_1, created_at=eight_days_ago) - _create_job(service=service_1, template=email_template_service_1, created_at=four_days_ago) - - _create_job(service=service_2, template=email_template_service_2, created_at=eight_days_ago) - job3_to_delete = _create_job(service=service_2, template=email_template_service_2, created_at=thirty_one_days_ago) - job4_to_delete = _create_job(service=service_2, template=sms_template_service_2, created_at=eight_days_ago) - - remove_csv_files(job_types=[SMS_TYPE, EMAIL_TYPE]) - - s3.remove_job_from_s3.assert_has_calls([ - call(job1_to_delete.service_id, job1_to_delete.id), - call(job2_to_delete.service_id, job2_to_delete.id), - call(job3_to_delete.service_id, job3_to_delete.id), - call(job4_to_delete.service_id, job4_to_delete.id) - ], any_order=True) - - -def test_send_daily_performance_stats_calls_does_not_send_if_inactive(client, mocker): - send_mock = mocker.patch( - 'app.celery.scheduled_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') # noqa - - with patch.object( - PerformancePlatformClient, - 'active', - new_callable=PropertyMock - ) as mock_active: - mock_active.return_value = False - send_daily_performance_platform_stats() - - assert send_mock.call_count == 0 - - -@freeze_time("2016-01-11 12:30:00") -def test_send_total_sent_notifications_to_performance_platform_calls_with_correct_totals( - notify_db, - notify_db_session, - sample_template, - mocker -): - perf_mock = mocker.patch( - 'app.celery.scheduled_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') # noqa - - notification_history = partial( - create_notification_history, - notify_db, - notify_db_session, - sample_template, - status='delivered' - ) - - notification_history(notification_type='email') - notification_history(notification_type='sms') - - # Create some notifications for the day before - yesterday = datetime(2016, 1, 10, 15, 30, 0, 0) - with freeze_time(yesterday): - notification_history(notification_type='sms') - notification_history(notification_type='sms') - notification_history(notification_type='email') - notification_history(notification_type='email') - notification_history(notification_type='email') - - with patch.object( - PerformancePlatformClient, - 'active', - new_callable=PropertyMock - ) as mock_active: - mock_active.return_value = True - send_total_sent_notifications_to_performance_platform(yesterday) - - perf_mock.assert_has_calls([ - call(get_london_midnight_in_utc(yesterday), 'sms', 2), - call(get_london_midnight_in_utc(yesterday), 'email', 3) - ]) - - def test_switch_providers_on_slow_delivery_switches_once_then_does_not_switch_if_already_switched( notify_api, mocker, @@ -440,245 +166,6 @@ def test_should_send_all_scheduled_notifications_to_deliver_queue(sample_templat assert not scheduled_notifications -def test_should_call_delete_inbound_sms_older_than_seven_days(notify_api, mocker): - mocker.patch('app.celery.scheduled_tasks.delete_inbound_sms_created_more_than_a_week_ago') - delete_inbound_sms_older_than_seven_days() - assert scheduled_tasks.delete_inbound_sms_created_more_than_a_week_ago.call_count == 1 - - -@freeze_time('2017-01-01 10:00:00') -def test_remove_csv_files_filters_by_type(mocker, sample_service): - mocker.patch('app.celery.scheduled_tasks.s3.remove_job_from_s3') - """ - Jobs older than seven days are deleted, but only two day's worth (two-day window) - """ - letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) - sms_template = create_template(service=sample_service, template_type=SMS_TYPE) - - eight_days_ago = datetime.utcnow() - timedelta(days=8) - - job_to_delete = create_job(template=letter_template, created_at=eight_days_ago) - create_job(template=sms_template, created_at=eight_days_ago) - - remove_csv_files(job_types=[LETTER_TYPE]) - - assert s3.remove_job_from_s3.call_args_list == [ - call(job_to_delete.service_id, job_to_delete.id), - ] - - -@freeze_time('2017-01-01 10:00:00') -def test_remove_dvla_transformed_files_removes_expected_files(mocker, sample_service): - mocker.patch('app.celery.scheduled_tasks.s3.remove_transformed_dvla_file') - - letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) - - job = partial(create_job, template=letter_template) - - seven_days_ago = datetime.utcnow() - timedelta(days=7) - just_under_seven_days = seven_days_ago + timedelta(seconds=1) - just_over_seven_days = seven_days_ago - timedelta(seconds=1) - eight_days_ago = seven_days_ago - timedelta(days=1) - nine_days_ago = eight_days_ago - timedelta(days=1) - ten_days_ago = nine_days_ago - timedelta(days=1) - just_under_nine_days = nine_days_ago + timedelta(seconds=1) - just_over_nine_days = nine_days_ago - timedelta(seconds=1) - just_over_ten_days = ten_days_ago - timedelta(seconds=1) - - job(created_at=just_under_seven_days) - job(created_at=just_over_seven_days) - job_to_delete_1 = job(created_at=eight_days_ago) - job_to_delete_2 = job(created_at=nine_days_ago) - job_to_delete_3 = job(created_at=just_under_nine_days) - job_to_delete_4 = job(created_at=just_over_nine_days) - job(created_at=just_over_ten_days) - remove_transformed_dvla_files() - - s3.remove_transformed_dvla_file.assert_has_calls([ - call(job_to_delete_1.id), - call(job_to_delete_2.id), - call(job_to_delete_3.id), - call(job_to_delete_4.id), - ], any_order=True) - - -def test_remove_dvla_transformed_files_does_not_remove_files(mocker, sample_service): - mocker.patch('app.celery.scheduled_tasks.s3.remove_transformed_dvla_file') - - letter_template = create_template(service=sample_service, template_type=LETTER_TYPE) - - job = partial(create_job, template=letter_template) - - yesterday = datetime.utcnow() - timedelta(days=1) - six_days_ago = datetime.utcnow() - timedelta(days=6) - seven_days_ago = six_days_ago - timedelta(days=1) - just_over_nine_days = seven_days_ago - timedelta(days=2, seconds=1) - - job(created_at=yesterday) - job(created_at=six_days_ago) - job(created_at=seven_days_ago) - job(created_at=just_over_nine_days) - - remove_transformed_dvla_files() - - s3.remove_transformed_dvla_file.assert_has_calls([]) - - -@freeze_time("2016-01-01 11:00:00") -def test_delete_dvla_response_files_older_than_seven_days_removes_old_files(notify_api, mocker): - AFTER_SEVEN_DAYS = datetime_in_past(days=8) - single_page_s3_objects = [{ - "Contents": [ - single_s3_object_stub('bar/foo1.txt', AFTER_SEVEN_DAYS), - single_s3_object_stub('bar/foo2.txt', AFTER_SEVEN_DAYS), - ] - }] - mocker.patch( - 'app.celery.scheduled_tasks.s3.get_s3_bucket_objects', return_value=single_page_s3_objects[0]["Contents"] - ) - remove_s3_mock = mocker.patch('app.celery.scheduled_tasks.s3.remove_s3_object') - - delete_dvla_response_files_older_than_seven_days() - - remove_s3_mock.assert_has_calls([ - call(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], single_page_s3_objects[0]["Contents"][0]["Key"]), - call(current_app.config['DVLA_RESPONSE_BUCKET_NAME'], single_page_s3_objects[0]["Contents"][1]["Key"]) - ]) - - -@freeze_time("2016-01-01 11:00:00") -def test_delete_dvla_response_files_older_than_seven_days_does_not_remove_files(notify_api, mocker): - START_DATE = datetime_in_past(days=9) - JUST_BEFORE_START_DATE = datetime_in_past(days=9, seconds=1) - END_DATE = datetime_in_past(days=7) - JUST_AFTER_END_DATE = END_DATE + timedelta(seconds=1) - - single_page_s3_objects = [{ - "Contents": [ - single_s3_object_stub('bar/foo1.txt', JUST_BEFORE_START_DATE), - single_s3_object_stub('bar/foo2.txt', START_DATE), - single_s3_object_stub('bar/foo3.txt', END_DATE), - single_s3_object_stub('bar/foo4.txt', JUST_AFTER_END_DATE), - ] - }] - mocker.patch( - 'app.celery.scheduled_tasks.s3.get_s3_bucket_objects', return_value=single_page_s3_objects[0]["Contents"] - ) - remove_s3_mock = mocker.patch('app.celery.scheduled_tasks.s3.remove_s3_object') - delete_dvla_response_files_older_than_seven_days() - - remove_s3_mock.assert_not_called() - - -@freeze_time("2018-01-17 17:00:00") -def test_alert_if_letter_notifications_still_sending(sample_letter_template, mocker): - two_days_ago = datetime(2018, 1, 15, 13, 30) - create_notification(template=sample_letter_template, status='sending', sent_at=two_days_ago) - - mock_create_ticket = mocker.patch("app.celery.scheduled_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - mock_create_ticket.assert_called_once_with( - subject="[test] Letters still sending", - message="There are 1 letters in the 'sending' state from Monday 15 January", - ticket_type=ZendeskClient.TYPE_INCIDENT - ) - - -def test_alert_if_letter_notifications_still_sending_a_day_ago_no_alert(sample_letter_template, mocker): - today = datetime.utcnow() - one_day_ago = today - timedelta(days=1) - create_notification(template=sample_letter_template, status='sending', sent_at=one_day_ago) - - mock_create_ticket = mocker.patch("app.celery.scheduled_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - assert not mock_create_ticket.called - - -@freeze_time("2018-01-17 17:00:00") -def test_alert_if_letter_notifications_still_sending_only_alerts_sending(sample_letter_template, mocker): - two_days_ago = datetime(2018, 1, 15, 13, 30) - create_notification(template=sample_letter_template, status='sending', sent_at=two_days_ago) - create_notification(template=sample_letter_template, status='delivered', sent_at=two_days_ago) - create_notification(template=sample_letter_template, status='failed', sent_at=two_days_ago) - - mock_create_ticket = mocker.patch("app.celery.scheduled_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - mock_create_ticket.assert_called_once_with( - subject="[test] Letters still sending", - message="There are 1 letters in the 'sending' state from Monday 15 January", - ticket_type='incident' - ) - - -@freeze_time("2018-01-17 17:00:00") -def test_alert_if_letter_notifications_still_sending_alerts_for_older_than_offset(sample_letter_template, mocker): - three_days_ago = datetime(2018, 1, 14, 13, 30) - create_notification(template=sample_letter_template, status='sending', sent_at=three_days_ago) - - mock_create_ticket = mocker.patch("app.celery.scheduled_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - mock_create_ticket.assert_called_once_with( - subject="[test] Letters still sending", - message="There are 1 letters in the 'sending' state from Monday 15 January", - ticket_type='incident' - ) - - -@freeze_time("2018-01-14 17:00:00") -def test_alert_if_letter_notifications_still_sending_does_nothing_on_the_weekend(sample_letter_template, mocker): - yesterday = datetime(2018, 1, 13, 13, 30) - create_notification(template=sample_letter_template, status='sending', sent_at=yesterday) - - mock_create_ticket = mocker.patch("app.celery.scheduled_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - assert not mock_create_ticket.called - - -@freeze_time("2018-01-15 17:00:00") -def test_monday_alert_if_letter_notifications_still_sending_reports_thursday_letters(sample_letter_template, mocker): - thursday = datetime(2018, 1, 11, 13, 30) - yesterday = datetime(2018, 1, 14, 13, 30) - create_notification(template=sample_letter_template, status='sending', sent_at=thursday) - create_notification(template=sample_letter_template, status='sending', sent_at=yesterday) - - mock_create_ticket = mocker.patch("app.celery.scheduled_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - mock_create_ticket.assert_called_once_with( - subject="[test] Letters still sending", - message="There are 1 letters in the 'sending' state from Thursday 11 January", - ticket_type='incident' - ) - - -@freeze_time("2018-01-16 17:00:00") -def test_tuesday_alert_if_letter_notifications_still_sending_reports_friday_letters(sample_letter_template, mocker): - friday = datetime(2018, 1, 12, 13, 30) - yesterday = datetime(2018, 1, 14, 13, 30) - create_notification(template=sample_letter_template, status='sending', sent_at=friday) - create_notification(template=sample_letter_template, status='sending', sent_at=yesterday) - - mock_create_ticket = mocker.patch("app.celery.scheduled_tasks.zendesk_client.create_ticket") - - raise_alert_if_letter_notifications_still_sending() - - mock_create_ticket.assert_called_once_with( - subject="[test] Letters still sending", - message="There are 1 letters in the 'sending' state from Friday 12 January", - ticket_type='incident' - ) - - def test_check_job_status_task_raises_job_incomplete_error(mocker, sample_template): mock_celery = mocker.patch('app.celery.tasks.notify_celery.send_task') job = create_job(template=sample_template, notification_count=3, @@ -801,82 +288,6 @@ def test_check_job_status_task_sets_jobs_to_error(mocker, sample_template): assert job_2.job_status == JOB_STATUS_IN_PROGRESS -def mock_s3_get_list_match(bucket_name, subfolder='', suffix='', last_modified=None): - if subfolder == '2018-01-11/zips_sent': - return ['NOTIFY.20180111175007.ZIP.TXT', 'NOTIFY.20180111175008.ZIP.TXT'] - if subfolder == 'root/dispatch': - return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] - - -def mock_s3_get_list_diff(bucket_name, subfolder='', suffix='', last_modified=None): - if subfolder == '2018-01-11/zips_sent': - return ['NOTIFY.20180111175007.ZIP.TXT', 'NOTIFY.20180111175008.ZIP.TXT', 'NOTIFY.20180111175009.ZIP.TXT', - 'NOTIFY.20180111175010.ZIP.TXT'] - if subfolder == 'root/dispatch': - return ['root/dispatch/NOTIFY.20180111175733.ACK.txt'] - - -@freeze_time('2018-01-11T23:00:00') -def test_letter_not_raise_alert_if_ack_files_match_zip_list(mocker, notify_db): - mock_file_list = mocker.patch("app.aws.s3.get_list_of_files_by_suffix", side_effect=mock_s3_get_list_match) - mock_get_file = mocker.patch("app.aws.s3.get_s3_file", - return_value='NOTIFY.20180111175007.ZIP|20180111175733\n' - 'NOTIFY.20180111175008.ZIP|20180111175734') - - letter_raise_alert_if_no_ack_file_for_zip() - - yesterday = datetime.now(tz=pytz.utc) - timedelta(days=1) # Datatime format on AWS - subfoldername = datetime.utcnow().strftime('%Y-%m-%d') + '/zips_sent' - assert mock_file_list.call_count == 2 - assert mock_file_list.call_args_list == [ - call(bucket_name=current_app.config['LETTERS_PDF_BUCKET_NAME'], subfolder=subfoldername, suffix='.TXT'), - call(bucket_name=current_app.config['DVLA_RESPONSE_BUCKET_NAME'], subfolder='root/dispatch', - suffix='.ACK.txt', last_modified=yesterday), - ] - assert mock_get_file.call_count == 1 - - -@freeze_time('2018-01-11T23:00:00') -def test_letter_raise_alert_if_ack_files_not_match_zip_list(mocker, notify_db): - mock_file_list = mocker.patch("app.aws.s3.get_list_of_files_by_suffix", side_effect=mock_s3_get_list_diff) - mock_get_file = mocker.patch("app.aws.s3.get_s3_file", - return_value='NOTIFY.20180111175007.ZIP|20180111175733\n' - 'NOTIFY.20180111175008.ZIP|20180111175734') - mock_zendesk = mocker.patch("app.celery.scheduled_tasks.zendesk_client.create_ticket") - - letter_raise_alert_if_no_ack_file_for_zip() - - assert mock_file_list.call_count == 2 - assert mock_get_file.call_count == 1 - - message = "Letter ack file does not contain all zip files sent. " \ - "Missing ack for zip files: {}, " \ - "pdf bucket: {}, subfolder: {}, " \ - "ack bucket: {}".format(str(['NOTIFY.20180111175009.ZIP', 'NOTIFY.20180111175010.ZIP']), - current_app.config['LETTERS_PDF_BUCKET_NAME'], - datetime.utcnow().strftime('%Y-%m-%d') + '/zips_sent', - current_app.config['DVLA_RESPONSE_BUCKET_NAME']) - - mock_zendesk.assert_called_once_with( - subject="Letter acknowledge error", - message=message, - ticket_type='incident' - ) - - -@freeze_time('2018-01-11T23:00:00') -def test_letter_not_raise_alert_if_no_files_do_not_cause_error(mocker, notify_db): - mock_file_list = mocker.patch("app.aws.s3.get_list_of_files_by_suffix", side_effect=None) - mock_get_file = mocker.patch("app.aws.s3.get_s3_file", - return_value='NOTIFY.20180111175007.ZIP|20180111175733\n' - 'NOTIFY.20180111175008.ZIP|20180111175734') - - letter_raise_alert_if_no_ack_file_for_zip() - - assert mock_file_list.call_count == 2 - assert mock_get_file.call_count == 0 - - def test_replay_created_notifications(notify_db_session, sample_service, mocker): email_delivery_queue = mocker.patch('app.celery.provider_tasks.deliver_email.apply_async') sms_delivery_queue = mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async') From 754c65a6a258dd126c809d820d11805bd6a8a4fb Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 16 Jan 2019 14:11:03 +0000 Subject: [PATCH 26/31] create cronitor decorator that alerts if tasks fail make a decorator that pings cronitor before and after each task run. Designed for use with nightly tasks, so we have visibility if they fail. We have a bunch of cronitor monitors set up - 5 character keys that go into a URL that we then make a GET to with a self-explanatory url path (run/fail/complete). the cronitor URLs are defined in the credentials repo as a dictionary of celery task names to URL slugs. If the name passed in to the decorator isn't in that dict, it won't run. to use it, all you need to do is call `@cronitor(my_task_name)` instead of `@notify_celery.task`, and make sure that the task name and the matching slug are included in the credentials repo (or locally, json dumped and stored in the CRONITOR_KEYS environment variable) --- app/celery/nightly_tasks.py | 29 +++++++++++++- app/celery/reporting_tasks.py | 3 ++ app/config.py | 23 ++++++----- app/cronitor.py | 50 ++++++++++++++++++++++++ manifest-api-base.yml | 1 + manifest-delivery-base.yml | 1 + tests/app/celery/test_reporting_tasks.py | 4 -- 7 files changed, 96 insertions(+), 15 deletions(-) create mode 100644 app/cronitor.py diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index a917ebfd0..0c9be4e8b 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -30,15 +30,30 @@ from app.exceptions import NotificationTechnicalFailureException from app.models import ( Notification, NOTIFICATION_SENDING, + EMAIL_TYPE, + SMS_TYPE, LETTER_TYPE, KEY_TYPE_NORMAL ) from app.performance_platform import total_sent_notifications, processing_time +from app.cronitor import cronitor -@notify_celery.task(name="remove_csv_files") +@notify_celery.task(name="remove_sms_email_jobs") +@cronitor("remove_sms_email_jobs") @statsd(namespace="tasks") -def remove_csv_files(job_types): +def remove_sms_email_csv_files(job_types): + _remove_csv_files([EMAIL_TYPE, SMS_TYPE]) + + +@notify_celery.task(name="remove_letter_jobs") +@cronitor("remove_letter_jobs") +@statsd(namespace="tasks") +def remove_letter_csv_files(job_types): + _remove_csv_files([LETTER_TYPE]) + + +def _remove_csv_files(job_types): jobs = dao_get_jobs_older_than_data_retention(notification_types=job_types) for job in jobs: s3.remove_job_from_s3(job.service_id, job.id) @@ -47,6 +62,7 @@ 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(): try: @@ -66,6 +82,7 @@ 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(): try: @@ -85,6 +102,7 @@ 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(): try: @@ -104,6 +122,7 @@ def delete_letter_notifications_older_than_seven_days(): @notify_celery.task(name='timeout-sending-notifications') +@cronitor('timeout-sending-notifications') @statsd(namespace="tasks") def timeout_notifications(): technical_failure_notifications, temporary_failure_notifications = \ @@ -128,6 +147,7 @@ def timeout_notifications(): @notify_celery.task(name='send-daily-performance-platform-stats') +@cronitor('send-daily-performance-platform-stats') @statsd(namespace="tasks") def send_daily_performance_platform_stats(): if performance_platform_client.active: @@ -168,6 +188,7 @@ 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(): try: @@ -186,6 +207,7 @@ def delete_inbound_sms_older_than_seven_days(): @notify_celery.task(name="remove_transformed_dvla_files") +@cronitor("remove_transformed_dvla_files") @statsd(namespace="tasks") def remove_transformed_dvla_files(): jobs = dao_get_jobs_older_than_data_retention(notification_types=[LETTER_TYPE]) @@ -194,6 +216,7 @@ def remove_transformed_dvla_files(): current_app.logger.info("Transformed dvla file for job {} has been removed from s3.".format(job.id)) +# TODO: remove me, i'm not being run by anything @notify_celery.task(name="delete_dvla_response_files") @statsd(namespace="tasks") def delete_dvla_response_files_older_than_seven_days(): @@ -221,6 +244,7 @@ def delete_dvla_response_files_older_than_seven_days(): @notify_celery.task(name="raise-alert-if-letter-notifications-still-sending") +@cronitor("raise-alert-if-letter-notifications-still-sending") @statsd(namespace="tasks") def raise_alert_if_letter_notifications_still_sending(): today = datetime.utcnow().date() @@ -257,6 +281,7 @@ def raise_alert_if_letter_notifications_still_sending(): @notify_celery.task(name='raise-alert-if-no-letter-ack-file') +@cronitor('raise-alert-if-no-letter-ack-file') @statsd(namespace="tasks") def letter_raise_alert_if_no_ack_file_for_zip(): # get a list of zip files since yesterday diff --git a/app/celery/reporting_tasks.py b/app/celery/reporting_tasks.py index 4d14c0e64..80c5d1bc0 100644 --- a/app/celery/reporting_tasks.py +++ b/app/celery/reporting_tasks.py @@ -4,6 +4,7 @@ from flask import current_app from notifications_utils.statsd_decorators import statsd from app import notify_celery +from app.cronitor import cronitor from app.dao.fact_billing_dao import ( fetch_billing_data_for_day, update_fact_billing @@ -12,6 +13,7 @@ from app.dao.fact_notification_status_dao import fetch_notification_status_for_d @notify_celery.task(name="create-nightly-billing") +@cronitor("create-nightly-billing") @statsd(namespace="tasks") def create_nightly_billing(day_start=None): # day_start is a datetime.date() object. e.g. @@ -34,6 +36,7 @@ def create_nightly_billing(day_start=None): @notify_celery.task(name="create-nightly-notification-status") +@cronitor("create-nightly-notification-status") @statsd(namespace="tasks") def create_nightly_notification_status(day_start=None): # day_start is a datetime.date() object. e.g. diff --git a/app/config.py b/app/config.py index b07d04b13..7ce5cfb0f 100644 --- a/app/config.py +++ b/app/config.py @@ -5,10 +5,6 @@ import json from celery.schedules import crontab from kombu import Exchange, Queue -from app.models import ( - EMAIL_TYPE, SMS_TYPE, LETTER_TYPE, -) - if os.environ.get('VCAP_SERVICES'): # on cloudfoundry, config is a json blob in VCAP_SERVICES - unpack it, and populate # standard environment variables from it @@ -108,6 +104,10 @@ class Config(object): DEBUG = False NOTIFY_LOG_PATH = os.getenv('NOTIFY_LOG_PATH') + # Cronitor + CRONITOR_ENABLED = False + CRONITOR_KEYS = json.loads(os.environ.get('CRONITOR_KEYS', '{}')) + ########################### # Default config values ### ########################### @@ -157,7 +157,12 @@ class Config(object): CELERY_TIMEZONE = 'Europe/London' CELERY_ACCEPT_CONTENT = ['json'] CELERY_TASK_SERIALIZER = 'json' - CELERY_IMPORTS = ('app.celery.tasks', 'app.celery.scheduled_tasks', 'app.celery.reporting_tasks') + CELERY_IMPORTS = ( + 'app.celery.tasks', + 'app.celery.scheduled_tasks', + 'app.celery.reporting_tasks', + 'app.celery.nightly_tasks', + ) CELERYBEAT_SCHEDULE = { # app/celery/scheduled_tasks.py 'run-scheduled-jobs': { @@ -238,17 +243,15 @@ class Config(object): 'options': {'queue': QueueNames.PERIODIC} }, 'remove_sms_email_jobs': { - 'task': 'remove_csv_files', + 'task': 'remove_sms_email_jobs', 'schedule': crontab(hour=4, minute=0), 'options': {'queue': QueueNames.PERIODIC}, - 'kwargs': {'job_types': [EMAIL_TYPE, SMS_TYPE]} }, 'remove_letter_jobs': { - 'task': 'remove_csv_files', + 'task': 'remove_letter_jobs', 'schedule': crontab(hour=4, minute=20), # this has to run AFTER remove_transformed_dvla_files # since we mark jobs as archived 'options': {'queue': QueueNames.PERIODIC}, - 'kwargs': {'job_types': [LETTER_TYPE]} }, 'raise-alert-if-letter-notifications-still-sending': { 'task': 'raise-alert-if-letter-notifications-still-sending', @@ -436,6 +439,8 @@ class Live(Config): API_RATE_LIMIT_ENABLED = True CHECK_PROXY_HEADER = True + CRONITOR_ENABLED = True + class CloudFoundryConfig(Config): pass diff --git a/app/cronitor.py b/app/cronitor.py new file mode 100644 index 000000000..7f496fe1e --- /dev/null +++ b/app/cronitor.py @@ -0,0 +1,50 @@ +import requests +from functools import wraps +from flask import current_app + + +def cronitor(task_name): + # check if task_name is in config + def decorator(func): + def ping_cronitor(command): + if not current_app.config['CRONITOR_ENABLED']: + return + + task_slug = current_app.config['CRONITOR_KEYS'].get(task_name) + if not task_slug: + current_app.logger.error( + 'Cronitor enabled but task_name {} not found in environment'.format(task_name) + ) + + if command not in {'run', 'complete', 'fail'}: + raise ValueError('command {} not a valid cronitor command'.format(command)) + + resp = requests.get( + 'https://cronitor.link/{}/{}'.format(task_slug, command), + # cronitor limits msg to 1000 characters + params={ + 'host': current_app.config['API_HOST_NAME'], + } + ) + if resp.status_code != 200: + current_app.logger.warning('Cronitor API returned {} for task {}, body {}'.format( + resp.status_code, + task_name, + resp.text + )) + + @wraps(func) + def inner_decorator(*args, **kwargs): + ping_cronitor('run') + try: + ret = func(*args, **kwargs) + status = 'complete' + return ret + except Exception: + status = 'fail' + raise + finally: + ping_cronitor(status) + + return inner_decorator + return decorator diff --git a/manifest-api-base.yml b/manifest-api-base.yml index 10096f97e..3ef91e6bd 100644 --- a/manifest-api-base.yml +++ b/manifest-api-base.yml @@ -22,6 +22,7 @@ env: SECRET_KEY: null ROUTE_SECRET_KEY_1: null ROUTE_SECRET_KEY_2: null + CRONITOR_KEYS: null PERFORMANCE_PLATFORM_ENDPOINTS: null diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 5ce75e7fc..1751b3e66 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -20,6 +20,7 @@ env: SECRET_KEY: null ROUTE_SECRET_KEY_1: null ROUTE_SECRET_KEY_2: null + CRONITOR_KEYS: null PERFORMANCE_PLATFORM_ENDPOINTS: null diff --git a/tests/app/celery/test_reporting_tasks.py b/tests/app/celery/test_reporting_tasks.py index 8918a33ce..ade175db2 100644 --- a/tests/app/celery/test_reporting_tasks.py +++ b/tests/app/celery/test_reporting_tasks.py @@ -20,10 +20,6 @@ from app import db from tests.app.db import create_service, create_template, create_notification -def test_reporting_should_have_decorated_tasks_functions(): - assert create_nightly_billing.__wrapped__.__name__ == 'create_nightly_billing' - - def mocker_get_rate( non_letter_rates, letter_rates, notification_type, date, crown=None, rate_multiplier=None, post_class="second" ): From e1760adcd3c785dbaa891df15880f678b6fe4efd Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Fri, 18 Jan 2019 15:29:04 +0000 Subject: [PATCH 27/31] suppress cronitor request errors --- app/cronitor.py | 24 +++++---- tests/app/test_cronitor.py | 101 +++++++++++++++++++++++++++++++++++++ 2 files changed, 114 insertions(+), 11 deletions(-) create mode 100644 tests/app/test_cronitor.py diff --git a/app/cronitor.py b/app/cronitor.py index 7f496fe1e..83a12f61f 100644 --- a/app/cronitor.py +++ b/app/cronitor.py @@ -15,22 +15,24 @@ def cronitor(task_name): current_app.logger.error( 'Cronitor enabled but task_name {} not found in environment'.format(task_name) ) + return if command not in {'run', 'complete', 'fail'}: raise ValueError('command {} not a valid cronitor command'.format(command)) - resp = requests.get( - 'https://cronitor.link/{}/{}'.format(task_slug, command), - # cronitor limits msg to 1000 characters - params={ - 'host': current_app.config['API_HOST_NAME'], - } - ) - if resp.status_code != 200: - current_app.logger.warning('Cronitor API returned {} for task {}, body {}'.format( - resp.status_code, + try: + resp = requests.get( + 'https://cronitor.link/{}/{}'.format(task_slug, command), + # cronitor limits msg to 1000 characters + params={ + 'host': current_app.config['API_HOST_NAME'], + } + ) + resp.raise_for_status() + except requests.RequestException as e: + current_app.logger.warning('Cronitor API failed for task {} due to {}'.format( task_name, - resp.text + repr(e) )) @wraps(func) diff --git a/tests/app/test_cronitor.py b/tests/app/test_cronitor.py new file mode 100644 index 000000000..8e1aaa6b4 --- /dev/null +++ b/tests/app/test_cronitor.py @@ -0,0 +1,101 @@ +from urllib import parse + +import requests +import pytest + +from app.cronitor import cronitor + +from tests.conftest import set_config_values + + +def _cronitor_url(key, command): + return parse.urlunparse(parse.ParseResult( + scheme='https', + netloc='cronitor.link', + path='{}/{}'.format(key, command), + params='', + query=parse.urlencode({'host': 'http://localhost:6011'}), + fragment='' + )) + + +RUN_LINK = _cronitor_url('secret', 'run') +FAIL_LINK = _cronitor_url('secret', 'fail') +COMPLETE_LINK = _cronitor_url('secret', 'complete') + + +@cronitor('hello') +def successful_task(): + return 1 + + +@cronitor('hello') +def crashing_task(): + raise ValueError + + +def test_cronitor_sends_run_and_complete(notify_api, rmock): + rmock.get(RUN_LINK, status_code=200) + rmock.get(COMPLETE_LINK, status_code=200) + + with set_config_values(notify_api, { + 'CRONITOR_ENABLED': True, + 'CRONITOR_KEYS': {'hello': 'secret'} + }): + assert successful_task() == 1 + + assert rmock.call_count == 2 + assert rmock.request_history[0].url == RUN_LINK + assert rmock.request_history[1].url == COMPLETE_LINK + + +def test_cronitor_sends_run_and_fail_if_exception(notify_api, rmock): + rmock.get(RUN_LINK, status_code=200) + rmock.get(FAIL_LINK, status_code=200) + + with set_config_values(notify_api, { + 'CRONITOR_ENABLED': True, + 'CRONITOR_KEYS': {'hello': 'secret'} + }): + with pytest.raises(ValueError): + crashing_task() + + assert rmock.call_count == 2 + assert rmock.request_history[0].url == RUN_LINK + assert rmock.request_history[1].url == FAIL_LINK + + +def test_cronitor_does_nothing_if_cronitor_not_enabled(notify_api, rmock): + with set_config_values(notify_api, { + 'CRONITOR_ENABLED': False, + 'CRONITOR_KEYS': {'hello': 'secret'} + }): + assert successful_task() == 1 + + assert rmock.called is False + + +def test_cronitor_does_nothing_if_name_not_recognised(notify_api, rmock, caplog): + with set_config_values(notify_api, { + 'CRONITOR_ENABLED': True, + 'CRONITOR_KEYS': {'not-hello': 'other'} + }): + assert successful_task() == 1 + + error_log = caplog.records[0] + assert error_log.levelname == 'ERROR' + assert error_log.msg == 'Cronitor enabled but task_name hello not found in environment' + assert rmock.called is False + + +def test_cronitor_doesnt_crash_if_request_fails(notify_api, rmock): + rmock.get(RUN_LINK, exc=requests.exceptions.ConnectTimeout) + rmock.get(COMPLETE_LINK, status_code=500) + + with set_config_values(notify_api, { + 'CRONITOR_ENABLED': True, + 'CRONITOR_KEYS': {'hello': 'secret'} + }): + assert successful_task() == 1 + + assert rmock.call_count == 2 From f5198bf71dea97284472a5a34ce5cfed6628a597 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 22 Jan 2019 10:31:37 +0000 Subject: [PATCH 28/31] remove unnecessary job_types arg from remove_csv_files celery tasks --- app/celery/nightly_tasks.py | 4 +- tests/app/celery/test_nightly_tasks.py | 69 +++++++++++--------------- 2 files changed, 30 insertions(+), 43 deletions(-) diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index 0c9be4e8b..e452befd1 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -42,14 +42,14 @@ from app.cronitor import cronitor @notify_celery.task(name="remove_sms_email_jobs") @cronitor("remove_sms_email_jobs") @statsd(namespace="tasks") -def remove_sms_email_csv_files(job_types): +def remove_sms_email_csv_files(): _remove_csv_files([EMAIL_TYPE, SMS_TYPE]) @notify_celery.task(name="remove_letter_jobs") @cronitor("remove_letter_jobs") @statsd(namespace="tasks") -def remove_letter_csv_files(job_types): +def remove_letter_csv_files(): _remove_csv_files([LETTER_TYPE]) diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index 0720b4b89..93e047448 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -16,7 +16,8 @@ from app.celery.nightly_tasks import ( delete_letter_notifications_older_than_seven_days, delete_sms_notifications_older_than_seven_days, raise_alert_if_letter_notifications_still_sending, - _remove_csv_files, + remove_letter_csv_files, + remove_sms_email_csv_files, remove_transformed_dvla_files, s3, send_daily_performance_platform_stats, @@ -44,11 +45,7 @@ from tests.app.db import ( create_service_data_retention ) -from tests.app.conftest import ( - sample_job as create_sample_job, - sample_notification_history as create_notification_history, - datetime_in_past -) +from tests.app.conftest import datetime_in_past def mock_s3_get_list_match(bucket_name, subfolder='', suffix='', last_modified=None): @@ -82,13 +79,13 @@ def test_will_remove_csv_files_for_jobs_older_than_seven_days( just_under_nine_days = nine_days_ago + timedelta(seconds=1) nine_days_one_second_ago = nine_days_ago - timedelta(seconds=1) - create_sample_job(notify_db, notify_db_session, created_at=nine_days_one_second_ago, archived=True) - job1_to_delete = create_sample_job(notify_db, notify_db_session, created_at=eight_days_ago) - job2_to_delete = create_sample_job(notify_db, notify_db_session, created_at=just_under_nine_days) - dont_delete_me_1 = create_sample_job(notify_db, notify_db_session, created_at=seven_days_ago) - create_sample_job(notify_db, notify_db_session, created_at=just_under_seven_days) + create_job(sample_template, created_at=nine_days_one_second_ago, archived=True) + job1_to_delete = create_job(sample_template, created_at=eight_days_ago) + job2_to_delete = create_job(sample_template, created_at=just_under_nine_days) + dont_delete_me_1 = create_job(sample_template, created_at=seven_days_ago) + create_job(sample_template, created_at=just_under_seven_days) - _remove_csv_files(job_types=[sample_template.template_type]) + remove_sms_email_csv_files() assert s3.remove_job_from_s3.call_args_list == [ call(job1_to_delete.service_id, job1_to_delete.id), @@ -120,21 +117,15 @@ def test_will_remove_csv_files_for_jobs_older_than_retention_period( eight_days_ago = datetime.utcnow() - timedelta(days=8) thirty_one_days_ago = datetime.utcnow() - timedelta(days=31) - _create_job = partial( - create_sample_job, - notify_db, - notify_db_session, - ) + job1_to_delete = create_job(sms_template_service_1, created_at=four_days_ago) + job2_to_delete = create_job(email_template_service_1, created_at=eight_days_ago) + create_job(email_template_service_1, created_at=four_days_ago) - job1_to_delete = _create_job(service=service_1, template=sms_template_service_1, created_at=four_days_ago) - job2_to_delete = _create_job(service=service_1, template=email_template_service_1, created_at=eight_days_ago) - _create_job(service=service_1, template=email_template_service_1, created_at=four_days_ago) + create_job(email_template_service_2, created_at=eight_days_ago) + job3_to_delete = create_job(email_template_service_2, created_at=thirty_one_days_ago) + job4_to_delete = create_job(sms_template_service_2, created_at=eight_days_ago) - _create_job(service=service_2, template=email_template_service_2, created_at=eight_days_ago) - job3_to_delete = _create_job(service=service_2, template=email_template_service_2, created_at=thirty_one_days_ago) - job4_to_delete = _create_job(service=service_2, template=sms_template_service_2, created_at=eight_days_ago) - - _remove_csv_files(job_types=[SMS_TYPE, EMAIL_TYPE]) + remove_sms_email_csv_files() s3.remove_job_from_s3.assert_has_calls([ call(job1_to_delete.service_id, job1_to_delete.id), @@ -158,7 +149,7 @@ def test_remove_csv_files_filters_by_type(mocker, sample_service): job_to_delete = create_job(template=letter_template, created_at=eight_days_ago) create_job(template=sms_template, created_at=eight_days_ago) - _remove_csv_files(job_types=[LETTER_TYPE]) + remove_letter_csv_files() assert s3.remove_job_from_s3.call_args_list == [ call(job_to_delete.service_id, job_to_delete.id), @@ -265,30 +256,26 @@ def test_send_total_sent_notifications_to_performance_platform_calls_with_correc notify_db, notify_db_session, sample_template, + sample_email_template, mocker ): + sms = sample_template + email = sample_email_template + perf_mock = mocker.patch( 'app.celery.nightly_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') # noqa - notification_history = partial( - create_notification_history, - notify_db, - notify_db_session, - sample_template, - status='delivered' - ) - - notification_history(notification_type='email') - notification_history(notification_type='sms') + create_notification(email, status='delivered') + create_notification(sms, status='delivered') # Create some notifications for the day before yesterday = datetime(2016, 1, 10, 15, 30, 0, 0) with freeze_time(yesterday): - notification_history(notification_type='sms') - notification_history(notification_type='sms') - notification_history(notification_type='email') - notification_history(notification_type='email') - notification_history(notification_type='email') + create_notification(sms, status='delivered') + create_notification(sms, status='delivered') + create_notification(email, status='delivered') + create_notification(email, status='delivered') + create_notification(email, status='delivered') with patch.object( PerformancePlatformClient, From afcdf1f9a1ea8a324f0ca19abb07e096c4914a06 Mon Sep 17 00:00:00 2001 From: Toby Lorne Date: Wed, 23 Jan 2019 14:26:43 +0000 Subject: [PATCH 29/31] Exit if celery processes are not running In 4427827b2ff7cc790d1e3400a9eeca7b8c22b991 and celery monitoring was changed from using PID files to actually looking at processes. If celery workers get OOM killed (for instance) the container init script would not restart them, this is because `get_celery_pids` would not contain any processes that contained the string celery. This would cause the pipe to fail (-o pipefail). APP_PIDS would not get updated but the script would continue to run. This caused the script to not restart the celery processes. We think the correct behaviour when celery processes are killed (i.e. there are no more celery processes running in a container) is to kill the container. The PaaS should then schedule new ones which may remediate the cause of the celery processes being killed. Upon detection of no celery processes running, some diagnostic information from the environment is sent to the logs, e.g.: ``` CF_INSTANCE_ADDR=10.0.32.4:61012 CF_INSTANCE_INTERNAL_IP=10.255.184.9 CF_INSTANCE_GUID=81c57dbc-e706-411e-6a5f-2013 CF_INSTANCE_PORT=61012 CF_INSTANCE_IP=10.0.32.4 ``` Then the script (which is the container entrypoint) exits 1. Co-author: @servingupaces @tlwr --- scripts/run_multi_worker_app_paas.sh | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/scripts/run_multi_worker_app_paas.sh b/scripts/run_multi_worker_app_paas.sh index f195e59dd..b44ab28d3 100755 --- a/scripts/run_multi_worker_app_paas.sh +++ b/scripts/run_multi_worker_app_paas.sh @@ -69,7 +69,10 @@ function get_celery_pids { # get the PIDs of the process whose parent is the root process # print only pid and their command, get the ones with "celery" in their name # and keep only these PIDs + + set +o pipefail # so grep returning no matches does not premature fail pipe APP_PIDS=$(pgrep -P 1 | xargs ps -o pid=,command= -p | grep celery | cut -f1 -d/) + set -o pipefail # pipefail should be set everywhere else } function send_signal_to_celery_processes { @@ -98,9 +101,28 @@ function start_logs_tail { echo "tail pid: ${LOGS_TAIL_PID}" } +function ensure_celery_is_running { + if [ "${APP_PIDS}" = "" ]; then + echo "There are no celery processes running, this container is bad" + + echo "Exporting CF information for diagnosis" + + env | grep CF + + echo "Sleeping 15 seconds for logs to get shipped" + + sleep 15 + + exit 1 + fi +} + function run { while true; do get_celery_pids + + ensure_celery_is_running + for APP_PID in ${APP_PIDS}; do kill -0 ${APP_PID} 2&>/dev/null || return 1 done From fa4cff5eb75ec47851002b2aefade133ce6b240c Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Wed, 23 Jan 2019 16:00:00 +0000 Subject: [PATCH 30/31] Bump sender memory to 3GB --- manifest-delivery-base.yml | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/manifest-delivery-base.yml b/manifest-delivery-base.yml index 1751b3e66..bf136b1df 100644 --- a/manifest-delivery-base.yml +++ b/manifest-delivery-base.yml @@ -68,7 +68,7 @@ applications: - name: notify-delivery-worker-sender command: scripts/run_multi_worker_app_paas.sh celery multi start 3 -c 10 -A run_celery.notify_celery --loglevel=INFO -Q send-sms-tasks,send-email-tasks - memory: 2G + memory: 3G env: NOTIFY_APP_NAME: delivery-worker-sender From 3528aab25ba17c4fc38c7d38170c1bdcc80efc7a Mon Sep 17 00:00:00 2001 From: Athanasios Voutsadakis Date: Wed, 23 Jan 2019 16:23:58 +0000 Subject: [PATCH 31/31] Kill the other processes started by the script We use exec to start awslogs_agent and then a tail to print logs to stdout. CF docs[1] recommend to use exec to start processes which seems to imply that as long as there are commands running the container will remain up and running. This commit ensures that if there are no celery tasks running we will kill any other processes that we have started, so that the container will no longer be considered healthy by cloudfoundry and will be replaced. 1: https://docs.cloudfoundry.org/devguide/deploy-apps/manifest.html#start-commands --- scripts/run_multi_worker_app_paas.sh | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/scripts/run_multi_worker_app_paas.sh b/scripts/run_multi_worker_app_paas.sh index b44ab28d3..6824923ea 100755 --- a/scripts/run_multi_worker_app_paas.sh +++ b/scripts/run_multi_worker_app_paas.sh @@ -113,6 +113,10 @@ function ensure_celery_is_running { sleep 15 + echo "Killing awslogs_agent and tail" + kill -9 ${AWSLOGS_AGENT_PID} + kill -9 ${LOGS_TAIL_PID} + exit 1 fi }