From 4abbb7137a3f56378a29f98d1205040cb8bd18a8 Mon Sep 17 00:00:00 2001 From: Toby Lorne Date: Tue, 2 Apr 2019 08:25:09 +0100 Subject: [PATCH 1/3] Fix sending of performance platform The pp client converts to UTC using the convert_utc_to_bst notify util. This requires a datatime not a date, pass it a datetime, and add an assertion in an existing test. I didn't want to use the midnight conversion util in the test. Signed-off-by: Toby Lorne --- .../total_sent_notifications.py | 5 ++++- .../test_total_sent_notifications.py | 20 ++++++++----------- 2 files changed, 12 insertions(+), 13 deletions(-) diff --git a/app/performance_platform/total_sent_notifications.py b/app/performance_platform/total_sent_notifications.py index 6dc1d039a..324010015 100644 --- a/app/performance_platform/total_sent_notifications.py +++ b/app/performance_platform/total_sent_notifications.py @@ -1,5 +1,6 @@ from app import performance_platform_client from app.dao.fact_notification_status_dao import get_total_sent_notifications_for_day_and_type +from app.utils import get_london_midnight_in_utc def send_total_notifications_sent_for_day_stats(date, notification_type, count): @@ -19,8 +20,10 @@ def get_total_sent_notifications_for_day(day): sms_count = get_total_sent_notifications_for_day_and_type(day, 'sms') letter_count = get_total_sent_notifications_for_day_and_type(day, 'letter') + start_date = get_london_midnight_in_utc(day) + return { - "start_date": day, + "start_date": start_date, "email": { "count": email_count }, diff --git a/tests/app/performance_platform/test_total_sent_notifications.py b/tests/app/performance_platform/test_total_sent_notifications.py index 0945903cc..c291a3794 100644 --- a/tests/app/performance_platform/test_total_sent_notifications.py +++ b/tests/app/performance_platform/test_total_sent_notifications.py @@ -53,15 +53,11 @@ def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict(sam total_count_dict = get_total_sent_notifications_for_day(yesterday) - assert total_count_dict == { - "start_date": yesterday, - "email": { - "count": 3 - }, - "sms": { - "count": 2 - }, - "letter": { - "count": 1 - } - } + assert total_count_dict["email"] == {"count": 3} + assert total_count_dict["sms"] == {"count": 2} + assert total_count_dict["letter"] == {"count": 1} + + # Should return a time around midnight depending on timezones + expected_start = datetime.combine(yesterday, datetime.min.time()) + time_diff = abs(expected_start - total_count_dict["start_date"]) + assert time_diff <= timedelta(minutes=60) From 3739d9055db650259f227edbc7be42bf92d40a4d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 2 Apr 2019 11:49:20 +0100 Subject: [PATCH 2/3] clean up usage of dates/datetimes in performance platform tasks * call variables unambiguous things like `start_time` or `bst_date` to reduce risk of passing in the wrong thing * simplify the count_dict object - remove nested dict and start_date fields as superfluous * use static datetime objects in tests rather than calculating them each time --- app/celery/nightly_tasks.py | 24 +++++----- .../performance_platform_client.py | 6 +-- app/performance_platform/processing_time.py | 20 ++++----- .../total_sent_notifications.py | 20 +++------ tests/app/celery/test_nightly_tasks.py | 26 +++++------ .../test_processing_time.py | 2 +- .../test_total_sent_notifications.py | 45 +++++++++---------- 7 files changed, 64 insertions(+), 79 deletions(-) diff --git a/app/celery/nightly_tasks.py b/app/celery/nightly_tasks.py index e4d9f58e8..5eac8acc2 100644 --- a/app/celery/nightly_tasks.py +++ b/app/celery/nightly_tasks.py @@ -37,6 +37,7 @@ from app.models import ( ) from app.performance_platform import total_sent_notifications, processing_time from app.cronitor import cronitor +from app.utils import get_london_midnight_in_utc @notify_celery.task(name="remove_sms_email_jobs") @@ -152,36 +153,37 @@ def timeout_notifications(): 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) + send_total_sent_notifications_to_performance_platform(bst_date=yesterday.date()) 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.date()) - 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') +def send_total_sent_notifications_to_performance_platform(bst_date): + count_dict = total_sent_notifications.get_total_sent_notifications_for_day(bst_date) + start_time = get_london_midnight_in_utc(bst_date) + + email_sent_count = count_dict['email'] + sms_sent_count = count_dict['sms'] + letter_sent_count = count_dict['letter'] 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) + .format(bst_date, email_sent_count, sms_sent_count, letter_sent_count) ) total_sent_notifications.send_total_notifications_sent_for_day_stats( - start_date, + start_time, 'sms', sms_sent_count ) total_sent_notifications.send_total_notifications_sent_for_day_stats( - start_date, + start_time, 'email', email_sent_count ) total_sent_notifications.send_total_notifications_sent_for_day_stats( - start_date, + start_time, 'letter', letter_sent_count ) diff --git a/app/clients/performance_platform/performance_platform_client.py b/app/clients/performance_platform/performance_platform_client.py index 66cecb4c0..6654c8d30 100644 --- a/app/clients/performance_platform/performance_platform_client.py +++ b/app/clients/performance_platform/performance_platform_client.py @@ -46,17 +46,17 @@ class PerformancePlatformClient: resp.raise_for_status() @staticmethod - def format_payload(*, dataset, date, group_name, group_value, count, period='day'): + def format_payload(*, dataset, start_time, group_name, group_value, count, period='day'): """ :param dataset - the name of the overall graph, as referred to in the endpoint. - :param date - the date we're sending stats for + :param start_time - UTC midnight of the day we're sending stats for :param group_name - the name of the individual groups of data, eg "channel" or "status" :param group_value - the value of the group, eg "sms" or "email" for group_name=channel :param count - the actual numeric value to send :param period - the period that this data covers - "day", "week", "month", "quarter". """ payload = { - '_timestamp': convert_utc_to_bst(date).isoformat(), + '_timestamp': convert_utc_to_bst(start_time).isoformat(), 'service': 'govuk-notify', 'dataType': dataset, 'period': period, diff --git a/app/performance_platform/processing_time.py b/app/performance_platform/processing_time.py index b901bde84..ee2bfde1a 100644 --- a/app/performance_platform/processing_time.py +++ b/app/performance_platform/processing_time.py @@ -9,29 +9,29 @@ from app import performance_platform_client def send_processing_time_to_performance_platform(): today = datetime.utcnow() - start_date = get_midnight_for_day_before(today) - end_date = get_london_midnight_in_utc(today) + start_time = get_midnight_for_day_before(today) + end_time = get_london_midnight_in_utc(today) - send_processing_time_for_start_and_end(start_date, end_date) + send_processing_time_for_start_and_end(start_time, end_time) -def send_processing_time_for_start_and_end(start_date, end_date): - result = dao_get_total_notifications_sent_per_day_for_performance_platform(start_date, end_date) +def send_processing_time_for_start_and_end(start_time, end_time): + result = dao_get_total_notifications_sent_per_day_for_performance_platform(start_time, end_time) current_app.logger.info( 'Sending processing-time to performance platform for date {}. Total: {}, under 10 secs {}'.format( - start_date, result.messages_total, result.messages_within_10_secs + start_time, result.messages_total, result.messages_within_10_secs ) ) - send_processing_time_data(start_date, 'messages-total', result.messages_total) - send_processing_time_data(start_date, 'messages-within-10-secs', result.messages_within_10_secs) + send_processing_time_data(start_time, 'messages-total', result.messages_total) + send_processing_time_data(start_time, 'messages-within-10-secs', result.messages_within_10_secs) -def send_processing_time_data(date, status, count): +def send_processing_time_data(start_time, status, count): payload = performance_platform_client.format_payload( dataset='processing-time', - date=date, + start_time=start_time, group_name='status', group_value=status, count=count diff --git a/app/performance_platform/total_sent_notifications.py b/app/performance_platform/total_sent_notifications.py index 324010015..aa37a9ccf 100644 --- a/app/performance_platform/total_sent_notifications.py +++ b/app/performance_platform/total_sent_notifications.py @@ -1,12 +1,11 @@ from app import performance_platform_client from app.dao.fact_notification_status_dao import get_total_sent_notifications_for_day_and_type -from app.utils import get_london_midnight_in_utc -def send_total_notifications_sent_for_day_stats(date, notification_type, count): +def send_total_notifications_sent_for_day_stats(start_time, notification_type, count): payload = performance_platform_client.format_payload( dataset='notifications', - date=date, + start_time=start_time, group_name='channel', group_value=notification_type, count=count @@ -20,17 +19,8 @@ def get_total_sent_notifications_for_day(day): sms_count = get_total_sent_notifications_for_day_and_type(day, 'sms') letter_count = get_total_sent_notifications_for_day_and_type(day, 'letter') - start_date = get_london_midnight_in_utc(day) - return { - "start_date": start_date, - "email": { - "count": email_count - }, - "sms": { - "count": sms_count - }, - "letter": { - "count": letter_count - }, + "email": email_count, + "sms": sms_count, + "letter": letter_count, } diff --git a/tests/app/celery/test_nightly_tasks.py b/tests/app/celery/test_nightly_tasks.py index fd75b5927..7fb0dc85d 100644 --- a/tests/app/celery/test_nightly_tasks.py +++ b/tests/app/celery/test_nightly_tasks.py @@ -1,4 +1,4 @@ -from datetime import datetime, timedelta +from datetime import datetime, timedelta, date from functools import partial from unittest.mock import call, patch, PropertyMock @@ -251,7 +251,7 @@ def test_send_daily_performance_stats_calls_does_not_send_if_inactive(client, mo assert send_mock.call_count == 0 -@freeze_time("2016-01-11 12:30:00") +@freeze_time("2016-06-11 02:00:00") def test_send_total_sent_notifications_to_performance_platform_calls_with_correct_totals( notify_db_session, sample_template, @@ -261,18 +261,14 @@ def test_send_total_sent_notifications_to_performance_platform_calls_with_correc perf_mock = mocker.patch( 'app.celery.nightly_tasks.total_sent_notifications.send_total_notifications_sent_for_day_stats') # noqa - today = datetime.utcnow() - create_ft_notification_status(bst_date=today, notification_type='sms', service=sample_template.service, - template=sample_template) - create_ft_notification_status(bst_date=today, notification_type='email', service=sample_email_template.service, - template=sample_email_template) + today = date(2016, 6, 11) + create_ft_notification_status(bst_date=today, template=sample_template) + create_ft_notification_status(bst_date=today, template=sample_email_template) # Create some notifications for the day before - yesterday = today - timedelta(days=1) - create_ft_notification_status(bst_date=yesterday, notification_type='sms', service=sample_template.service, - template=sample_template, count=2) - create_ft_notification_status(bst_date=yesterday, notification_type='email', service=sample_email_template.service, - template=sample_email_template, count=3) + yesterday = date(2016, 6, 10) + create_ft_notification_status(bst_date=yesterday, template=sample_template, count=2) + create_ft_notification_status(bst_date=yesterday, template=sample_email_template, count=3) with patch.object( PerformancePlatformClient, @@ -283,9 +279,9 @@ def test_send_total_sent_notifications_to_performance_platform_calls_with_correc send_total_sent_notifications_to_performance_platform(yesterday) perf_mock.assert_has_calls([ - call(yesterday.date(), 'sms', 2), - call(yesterday.date(), 'email', 3), - call(yesterday.date(), 'letter', 0) + call(datetime(2016, 6, 9, 23, 0), 'sms', 2), + call(datetime(2016, 6, 9, 23, 0), 'email', 3), + call(datetime(2016, 6, 9, 23, 0), 'letter', 0) ]) diff --git a/tests/app/performance_platform/test_processing_time.py b/tests/app/performance_platform/test_processing_time.py index 07f78859e..2458e866d 100644 --- a/tests/app/performance_platform/test_processing_time.py +++ b/tests/app/performance_platform/test_processing_time.py @@ -29,7 +29,7 @@ def test_send_processing_time_to_performance_platform_creates_correct_call_to_pe send_stats = mocker.patch('app.performance_platform.total_sent_notifications.performance_platform_client.send_stats_to_performance_platform') # noqa send_processing_time_data( - date=datetime(2016, 10, 15, 23, 0, 0), + start_time=datetime(2016, 10, 15, 23, 0, 0), status='foo', count=142 ) diff --git a/tests/app/performance_platform/test_total_sent_notifications.py b/tests/app/performance_platform/test_total_sent_notifications.py index c291a3794..326ed4bd8 100644 --- a/tests/app/performance_platform/test_total_sent_notifications.py +++ b/tests/app/performance_platform/test_total_sent_notifications.py @@ -1,4 +1,6 @@ -from datetime import datetime, timedelta +from datetime import datetime, date + +from freezegun import freeze_time from app.performance_platform.total_sent_notifications import ( send_total_notifications_sent_for_day_stats, @@ -12,7 +14,7 @@ def test_send_total_notifications_sent_for_day_stats_stats_creates_correct_call( send_stats = mocker.patch('app.performance_platform.total_sent_notifications.performance_platform_client.send_stats_to_performance_platform') # noqa send_total_notifications_sent_for_day_stats( - date=datetime(2016, 10, 15, 23, 0, 0), + start_time=datetime(2016, 10, 15, 23, 0, 0), notification_type='sms', count=142 ) @@ -30,34 +32,29 @@ def test_send_total_notifications_sent_for_day_stats_stats_creates_correct_call( assert request_args['_id'] == expected_base64_id +@freeze_time('2018-06-10 01:00') def test_get_total_sent_notifications_yesterday_returns_expected_totals_dict(sample_service): sms = create_template(sample_service, template_type='sms') email = create_template(sample_service, template_type='email') letter = create_template(sample_service, template_type='letter') - today = datetime.utcnow().date() - yesterday = today - timedelta(days=1) - create_ft_notification_status(bst_date=today, notification_type='sms', - service=sms.service, template=sms) - create_ft_notification_status(bst_date=today, notification_type='email', - service=email.service, template=email) - create_ft_notification_status(bst_date=today, notification_type='letter', - service=letter.service, template=letter) + today = date(2018, 6, 10) + yesterday = date(2018, 6, 9) - create_ft_notification_status(bst_date=yesterday, notification_type='sms', - service=sms.service, template=sms, count=2) - create_ft_notification_status(bst_date=yesterday, notification_type='email', - service=email.service, template=email, count=3) - create_ft_notification_status(bst_date=yesterday, notification_type='letter', - service=letter.service, template=letter, count=1) + # todays is excluded + create_ft_notification_status(bst_date=today, template=sms) + create_ft_notification_status(bst_date=today, template=email) + create_ft_notification_status(bst_date=today, template=letter) + + # yesterdays is included + create_ft_notification_status(bst_date=yesterday, template=sms, count=2) + create_ft_notification_status(bst_date=yesterday, template=email, count=3) + create_ft_notification_status(bst_date=yesterday, template=letter, count=1) total_count_dict = get_total_sent_notifications_for_day(yesterday) - assert total_count_dict["email"] == {"count": 3} - assert total_count_dict["sms"] == {"count": 2} - assert total_count_dict["letter"] == {"count": 1} - - # Should return a time around midnight depending on timezones - expected_start = datetime.combine(yesterday, datetime.min.time()) - time_diff = abs(expected_start - total_count_dict["start_date"]) - assert time_diff <= timedelta(minutes=60) + assert total_count_dict == { + "email": 3, + "sms": 2, + "letter": 1 + } From f1b3ae553f0726b40b3f1e66aa91695bb054074d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 2 Apr 2019 11:52:37 +0100 Subject: [PATCH 3/3] add tests for get_london_midnight_in_utc with a date object we sometimes call it with a timestamp, but sometimes with just a date object. It works with both, lets add the tests to prove that --- tests/app/test_utils.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 8f105b588..04d710af3 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -1,4 +1,4 @@ -from datetime import datetime +from datetime import datetime, date import pytest from freezegun import freeze_time @@ -15,6 +15,9 @@ from app.utils import ( (datetime(2016, 1, 15, 0, 30), datetime(2016, 1, 15, 0, 0)), (datetime(2016, 6, 15, 0, 0), datetime(2016, 6, 14, 23, 0)), (datetime(2016, 9, 15, 11, 59), datetime(2016, 9, 14, 23, 0)), + # works for both dates and datetimes + (date(2016, 1, 15), datetime(2016, 1, 15, 0, 0)), + (date(2016, 6, 15), datetime(2016, 6, 14, 23, 0)), ]) def test_get_london_midnight_in_utc_returns_expected_date(date, expected_date): assert get_london_midnight_in_utc(date) == expected_date