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

@@ -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
         )

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}
         }
     }

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

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)

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)
     )
 
 

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')

tests/app/celery/test_csv_files/email.csv

@@ -1,2 +0,0 @@
-email_address
-test@test.com

tests/app/celery/test_csv_files/empty.csv

@@ -1 +0,0 @@
-phone number

tests/app/celery/test_csv_files/multiple_email.csv

@@ -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

tests/app/celery/test_csv_files/multiple_letter.csv

@@ -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

tests/app/celery/test_csv_files/multiple_sms.csv

@@ -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

tests/app/celery/test_csv_files/sms.csv

@@ -1,2 +0,0 @@
-PHONE NUMBER, IGNORE THIS COLUMN
-+441234123123, nope

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

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