From b78d989d4eb144760d7e1deb8fb65f1a2111c1a4 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Thu, 9 Nov 2017 14:13:42 +0000 Subject: [PATCH] Updates after review - Modified the services_dao to return an int instead of a datetime to make usage easier and removed the BST function on year as it is not relevant for year - Improved tests do there is less logic by ordering the result so there is less reliance on the template id - Renamed variable in stats_template_usage_by_month_dao.py to make it consistent with the method --- app/celery/scheduled_tasks.py | 6 +- app/config.py | 2 +- app/dao/services_dao.py | 8 +- app/dao/stats_template_usage_by_month_dao.py | 4 +- app/utils.py | 18 +-- .../versions/0135_stats_template_usage.py | 14 --- tests/app/celery/test_csv_files/email.csv | 2 - tests/app/celery/test_csv_files/empty.csv | 1 - .../celery/test_csv_files/multiple_email.csv | 11 -- .../celery/test_csv_files/multiple_letter.csv | 11 -- .../celery/test_csv_files/multiple_sms.csv | 11 -- tests/app/celery/test_csv_files/sms.csv | 2 - tests/app/celery/test_scheduled_tasks.py | 104 +++++++++--------- tests/app/dao/test_services_dao.py | 30 +++-- 14 files changed, 76 insertions(+), 148 deletions(-) delete mode 100644 tests/app/celery/test_csv_files/email.csv delete mode 100644 tests/app/celery/test_csv_files/empty.csv delete mode 100644 tests/app/celery/test_csv_files/multiple_email.csv delete mode 100644 tests/app/celery/test_csv_files/multiple_letter.csv delete mode 100644 tests/app/celery/test_csv_files/multiple_sms.csv delete mode 100644 tests/app/celery/test_csv_files/sms.csv diff --git a/app/celery/scheduled_tasks.py b/app/celery/scheduled_tasks.py index 9b1a830d9..e39876df0 100644 --- a/app/celery/scheduled_tasks.py +++ b/app/celery/scheduled_tasks.py @@ -410,13 +410,13 @@ def check_job_status(): @notify_celery.task(name='daily-stats-template_usage_by_month') @statsd(namespace="tasks") -def daily_stats_template_usage_my_month(): +def daily_stats_template_usage_by_month(): results = dao_fetch_monthly_historical_stats_by_template() for result in results: insert_or_update_stats_for_template( result.template_id, - result.month.month, - result.year.year, + result.month, + result.year, result.count ) diff --git a/app/config.py b/app/config.py index afd70dade..16892ac28 100644 --- a/app/config.py +++ b/app/config.py @@ -244,7 +244,7 @@ class Config(object): }, 'daily-stats-template_usage_by_month': { 'task': 'daily-stats-template_usage_by_month', - 'schedule': crontab(hour=00, minute=50), + 'schedule': crontab(hour=0, minute=50), 'options': {'queue': QueueNames.PERIODIC} } } diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 684696d83..d023fb297 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -40,7 +40,7 @@ from app.models import ( ) from app.service.statistics import format_monthly_template_notification_stats from app.statsd_decorators import statsd -from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc, get_london_year_from_utc_column +from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc from app.dao.annual_billing_dao import dao_insert_annual_billing DEFAULT_SERVICE_PERMISSIONS = [ @@ -525,13 +525,13 @@ def dao_fetch_active_users_for_service(service_id): @statsd(namespace="dao") def dao_fetch_monthly_historical_stats_by_template(): month = get_london_month_from_utc_column(NotificationHistory.created_at) - year = get_london_year_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, - month.label('month'), - year.label('year'), + extract('month', month).label('month'), + extract('year', year).label('year'), func.count().label('count') ).filter( NotificationHistory.created_at < end_date diff --git a/app/dao/stats_template_usage_by_month_dao.py b/app/dao/stats_template_usage_by_month_dao.py index ca94dd83f..2884e2c53 100644 --- a/app/dao/stats_template_usage_by_month_dao.py +++ b/app/dao/stats_template_usage_by_month_dao.py @@ -15,10 +15,10 @@ def insert_or_update_stats_for_template(template_id, month, year, count): } ) if result == 0: - new_sms_sender = StatsTemplateUsageByMonth( + monthly_stats = StatsTemplateUsageByMonth( template_id=template_id, month=month, year=year, count=count ) - db.session.add(new_sms_sender) + db.session.add(monthly_stats) diff --git a/app/utils.py b/app/utils.py index 7fe4bdf89..ffb628284 100644 --- a/app/utils.py +++ b/app/utils.py @@ -71,23 +71,7 @@ def get_london_month_from_utc_column(column): """ return func.date_trunc( "month", - func.timezone("Europe/London", func.timezone("UTC", column)) - ) - - -def get_london_year_from_utc_column(column): - """ - Where queries need to count notifications by month it needs to be - the month in BST (British Summer Time). - The database stores all timestamps as UTC without the timezone. - - First set the timezone on created_at to UTC - - then convert the timezone to BST (or Europe/London) - - lastly truncate the datetime to month with which we can group - queries - """ - return func.date_trunc( - "month", - func.timezone("Europe/London", func.timezone("UTC", column)) + func.timezone("Europe/London", column) ) diff --git a/migrations/versions/0135_stats_template_usage.py b/migrations/versions/0135_stats_template_usage.py index 722fc69ce..5a8f5ef7a 100644 --- a/migrations/versions/0135_stats_template_usage.py +++ b/migrations/versions/0135_stats_template_usage.py @@ -31,20 +31,6 @@ def upgrade(): def downgrade(): # ### commands auto generated by Alembic - please adjust! ### - op.create_table('notification_statistics', - sa.Column('id', postgresql.UUID(), autoincrement=False, nullable=False), - sa.Column('service_id', postgresql.UUID(), autoincrement=False, nullable=False), - sa.Column('emails_requested', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('emails_delivered', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('emails_failed', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('sms_requested', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('sms_delivered', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('sms_failed', sa.BIGINT(), autoincrement=False, nullable=False), - sa.Column('day', sa.DATE(), autoincrement=False, nullable=False), - sa.ForeignKeyConstraint(['service_id'], ['services.id'], name='notification_statistics_service_id_fkey'), - sa.PrimaryKeyConstraint('id', name='notification_statistics_pkey'), - sa.UniqueConstraint('service_id', 'day', name='uix_service_to_day') - ) op.drop_index(op.f('ix_stats_template_usage_by_month_year'), table_name='stats_template_usage_by_month') op.drop_index(op.f('ix_stats_template_usage_by_month_template_id'), table_name='stats_template_usage_by_month') op.drop_index(op.f('ix_stats_template_usage_by_month_month'), table_name='stats_template_usage_by_month') diff --git a/tests/app/celery/test_csv_files/email.csv b/tests/app/celery/test_csv_files/email.csv deleted file mode 100644 index f0cefee69..000000000 --- a/tests/app/celery/test_csv_files/email.csv +++ /dev/null @@ -1,2 +0,0 @@ -email_address -test@test.com diff --git a/tests/app/celery/test_csv_files/empty.csv b/tests/app/celery/test_csv_files/empty.csv deleted file mode 100644 index 64e5bd0a3..000000000 --- a/tests/app/celery/test_csv_files/empty.csv +++ /dev/null @@ -1 +0,0 @@ -phone number diff --git a/tests/app/celery/test_csv_files/multiple_email.csv b/tests/app/celery/test_csv_files/multiple_email.csv deleted file mode 100644 index 5da15797d..000000000 --- a/tests/app/celery/test_csv_files/multiple_email.csv +++ /dev/null @@ -1,11 +0,0 @@ -EMAILADDRESS -test1@test.com -test2@test.com -test3@test.com -test4@test.com -test5@test.com -test6@test.com -test7@test.com -test8@test.com -test9@test.com -test0@test.com diff --git a/tests/app/celery/test_csv_files/multiple_letter.csv b/tests/app/celery/test_csv_files/multiple_letter.csv deleted file mode 100644 index 0e9d847b8..000000000 --- a/tests/app/celery/test_csv_files/multiple_letter.csv +++ /dev/null @@ -1,11 +0,0 @@ -address_line_1, address_line_2, address_line_3 -name1, street1, town1, postcode1 -name2, street2, town2, postcode2 -name3, street3, town3, postcode3 -name4, street4, town4, postcode4 -name5, street5, town5, postcode5 -name6, street6, town6, postcode6 -name7, street7, town7, postcode7 -name8, street8, town8, postcode8 -name9, street9, town9, postcode9 -name0, street0, town0, postcode0 \ No newline at end of file diff --git a/tests/app/celery/test_csv_files/multiple_sms.csv b/tests/app/celery/test_csv_files/multiple_sms.csv deleted file mode 100644 index 2ecad9140..000000000 --- a/tests/app/celery/test_csv_files/multiple_sms.csv +++ /dev/null @@ -1,11 +0,0 @@ -PhoneNumber,Name -+441234123121,chris -+441234123122,chris -+441234123123,chris -+441234123124,chris -+441234123125,chris -+441234123126,chris -+441234123127,chris -+441234123128,chris -+441234123129,chris -+441234123120,chris diff --git a/tests/app/celery/test_csv_files/sms.csv b/tests/app/celery/test_csv_files/sms.csv deleted file mode 100644 index 728639972..000000000 --- a/tests/app/celery/test_csv_files/sms.csv +++ /dev/null @@ -1,2 +0,0 @@ -PHONE NUMBER, IGNORE THIS COLUMN -+441234123123, nope diff --git a/tests/app/celery/test_scheduled_tasks.py b/tests/app/celery/test_scheduled_tasks.py index 6f63f7396..d31e2b5a4 100644 --- a/tests/app/celery/test_scheduled_tasks.py +++ b/tests/app/celery/test_scheduled_tasks.py @@ -33,7 +33,8 @@ from app.celery.scheduled_tasks import ( switch_current_sms_provider_on_slow_delivery, timeout_job_statistics, timeout_notifications, - daily_stats_template_usage_my_month) + daily_stats_template_usage_by_month +) 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 @@ -840,7 +841,7 @@ def test_check_job_status_task_raises_job_incomplete_error_for_multiple_jobs(moc ) -def test_daily_stats_template_usage_my_month(notify_db, notify_db_session): +def test_daily_stats_template_usage_by_month(notify_db, notify_db_session): notification_history = functools.partial( create_notification_history, notify_db, @@ -856,36 +857,37 @@ def test_daily_stats_template_usage_my_month(notify_db, notify_db_session): 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_my_month() + daily_stats_template_usage_by_month() - results = db.session.query(StatsTemplateUsageByMonth).all() + result = db.session.query( + StatsTemplateUsageByMonth + ).order_by( + StatsTemplateUsageByMonth.year, + StatsTemplateUsageByMonth.month + ).all() - assert len(results) == 2 + assert len(result) == 2 - for result in results: - if result.template_id == template_one.id: - assert result.template_id == template_one.id - assert result.month == 10 - assert result.year == 2017 - assert result.count == 1 - elif result.template_id == template_two.id: - assert result.template_id == template_two.id - assert result.month == 4 - assert result.year == 2016 - assert result.count == 2 - else: - raise AssertionError() + 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_my_month_no_data(): - daily_stats_template_usage_my_month() +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_my_month_multiple_runs(notify_db, notify_db_session): +def test_daily_stats_template_usage_by_month_multiple_runs(notify_db, notify_db_session): notification_history = functools.partial( create_notification_history, notify_db, @@ -896,12 +898,12 @@ def test_daily_stats_template_usage_my_month_multiple_runs(notify_db, notify_db_ 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(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_my_month() + daily_stats_template_usage_by_month() template_three = create_sample_template(notify_db, notify_db_session) @@ -911,35 +913,33 @@ def test_daily_stats_template_usage_my_month_multiple_runs(notify_db, notify_db_ 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_my_month() + daily_stats_template_usage_by_month() - results = db.session.query(StatsTemplateUsageByMonth).all() + result = db.session.query( + StatsTemplateUsageByMonth + ).order_by( + StatsTemplateUsageByMonth.year, + StatsTemplateUsageByMonth.month + ).all() - assert len(results) == 4 + assert len(result) == 4 - for result in results: - if result.template_id == template_one.id: - assert result.template_id == template_one.id - assert result.month == 10 - assert result.year == 2017 - assert result.count == 1 - elif result.template_id == template_two.id: - assert result.template_id == template_two.id - assert result.month == 4 - assert result.year == 2016 - assert result.count == 4 - elif result.template_id == template_three.id: - if result.month == 10: - assert result.template_id == template_three.id - assert result.month == 10 - assert result.year == 2017 - assert result.count == 1 - elif result.month == 9: - assert result.template_id == template_three.id - assert result.month == 9 - assert result.year == 2017 - assert result.count == 1 - else: - raise AssertionError() - else: - raise AssertionError() + 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 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 8a28e13dd..997301531 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -1015,28 +1015,24 @@ def test_dao_fetch_monthly_historical_stats_by_template(notify_db, notify_db_ses status='delivered' ) - template_one = create_sample_template(notify_db, notify_db_session) - template_two = create_sample_template(notify_db, notify_db_session) + template_one = create_sample_template(notify_db, notify_db_session, template_name='1') + template_two = create_sample_template(notify_db, notify_db_session, template_name='2') 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) - results = dao_fetch_monthly_historical_stats_by_template() + result = sorted(dao_fetch_monthly_historical_stats_by_template(), key=lambda x: (x.month, x.year)) - assert len(results) == 2 + assert len(result) == 2 - for result in results: - if result.template_id == template_one.id: - assert result.template_id == template_one.id - assert result.month.month == 10 - assert result.year.year == 2017 - assert result.count == 1 - elif result.template_id == template_two.id: - assert result.template_id == template_two.id - assert result.month.month == 4 - assert result.year.year == 2016 - assert result.count == 2 - else: - raise AssertionError() + 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