Merge pull request #345 from alphagov/notification-job-status-counts

Notification job status counts

app/dao/notifications_dao.py

@@ -1,4 +1,4 @@
-from sqlalchemy import (desc, func, Integer, and_, or_, asc)
+from sqlalchemy import (desc, func, Integer, or_, asc)
 from sqlalchemy.sql.expression import cast
 
 from datetime import (
@@ -35,11 +35,11 @@ from app.dao.dao_utils import transactional
 
 
 def dao_get_notification_statistics_for_service(service_id, limit_days=None):
-    filter = [NotificationStatistics.service_id == service_id]
+    query_filter = [NotificationStatistics.service_id == service_id]
     if limit_days is not None:
-        filter.append(NotificationStatistics.day >= days_ago(limit_days))
+        query_filter.append(NotificationStatistics.day >= days_ago(limit_days))
     return NotificationStatistics.query.filter(
-        *filter
+        *query_filter
     ).order_by(
         desc(NotificationStatistics.day)
     ).all()
@@ -53,9 +53,7 @@ def dao_get_notification_statistics_for_service_and_day(service_id, day):
 
 
 def dao_get_notification_statistics_for_day(day):
-    return NotificationStatistics.query.filter_by(
+    return NotificationStatistics.query.filter_by(day=day).all()
-        day=day
-    ).all()
 
 
 def dao_get_potential_notification_statistics_for_day(day):
@@ -131,10 +129,10 @@ def dao_get_7_day_agg_notification_statistics_for_service(service_id,
 
 
 def dao_get_template_statistics_for_service(service_id, limit_days=None):
-    filter = [TemplateStatistics.service_id == service_id]
+    query_filter = [TemplateStatistics.service_id == service_id]
     if limit_days is not None:
-        filter.append(TemplateStatistics.day >= days_ago(limit_days))
+        query_filter.append(TemplateStatistics.day >= days_ago(limit_days))
-    return TemplateStatistics.query.filter(*filter).order_by(
+    return TemplateStatistics.query.filter(*query_filter).order_by(
         desc(TemplateStatistics.updated_at)).all()
 
 
@@ -153,7 +151,7 @@ def dao_create_notification(notification, notification_type, provider_identifier
     update_count = db.session.query(NotificationStatistics).filter_by(
         day=notification.created_at.date(),
         service_id=notification.service_id
-    ).update(update_query(notification_type, 'requested'))
+    ).update(update_notification_stats_query(notification_type, 'requested'))
 
     if update_count == 0:
         stats = NotificationStatistics(
@@ -194,7 +192,7 @@ def dao_create_notification(notification, notification_type, provider_identifier
     db.session.add(notification)
 
 
-def update_query(notification_type, status):
+def update_notification_stats_query(notification_type, status):
     mapping = {
         TEMPLATE_TYPE_SMS: {
             STATISTICS_REQUESTED: NotificationStatistics.sms_requested,
@@ -212,6 +210,26 @@ def update_query(notification_type, status):
     }
 
 
+def _update_statistics(notification, notification_statistics_status):
+    db.session.query(NotificationStatistics).filter_by(
+        day=notification.created_at.date(),
+        service_id=notification.service_id
+    ).update(
+        update_notification_stats_query(notification.template.template_type, notification_statistics_status)
+    )
+    if notification.job_id:
+        db.session.query(Job).filter_by(id=notification.job_id
+                                        ).update(update_job_stats_query(notification_statistics_status))
+
+
+def update_job_stats_query(status):
+    mapping = {
+        STATISTICS_FAILURE: Job.notifications_failed,
+        STATISTICS_DELIVERED: Job.notifications_delivered
+    }
+    return {mapping[status]: mapping[status] + 1}
+
+
 def dao_update_notification(notification):
     notification.updated_at = datetime.utcnow()
     db.session.add(notification)
@@ -229,12 +247,7 @@ def update_notification_status_by_id(notification_id, status, notification_stati
     if count == 1 and notification_statistics_status:
         notification = Notification.query.get(notification_id)
 
-        db.session.query(NotificationStatistics).filter_by(
+        _update_statistics(notification, notification_statistics_status)
-            day=notification.created_at.date(),
-            service_id=notification.service_id
-        ).update(
-            update_query(notification.template.template_type, notification_statistics_status)
-        )
 
     db.session.commit()
     return count
@@ -246,16 +259,8 @@ def update_notification_status_by_reference(reference, status, notification_stat
         {Notification.status: status})
 
     if count == 1:
-        notification = Notification.query.filter_by(
+        notification = Notification.query.filter_by(reference=reference).first()
-            reference=reference
+        _update_statistics(notification, notification_statistics_status)
-        ).first()
-
-        db.session.query(NotificationStatistics).filter_by(
-            day=notification.created_at.date(),
-            service_id=notification.service_id
-        ).update(
-            update_query(notification.template.template_type, notification_statistics_status)
-        )
 
     db.session.commit()
     return count
@@ -279,7 +284,7 @@ def get_notifications_for_job(service_id, job_id, filter_dict=None, page=1, page
     if page_size is None:
         page_size = current_app.config['PAGE_SIZE']
     query = Notification.query.filter_by(service_id=service_id, job_id=job_id)
-    query = filter_query(query, filter_dict)
+    query = _filter_query(query, filter_dict)
     return query.order_by(asc(Notification.job_row_number)).paginate(
         page=page,
         per_page=page_size
@@ -308,14 +313,14 @@ def get_notifications_for_service(service_id,
         filters.append(func.date(Notification.created_at) >= days_ago)
 
     query = Notification.query.filter(*filters)
-    query = filter_query(query, filter_dict)
+    query = _filter_query(query, filter_dict)
     return query.order_by(desc(Notification.created_at)).paginate(
         page=page,
         per_page=page_size
     )
 
 
-def filter_query(query, filter_dict=None):
+def _filter_query(query, filter_dict=None):
     if filter_dict is None:
         filter_dict = MultiDict()
     else:

app/models.py

@@ -258,6 +258,9 @@ class Job(db.Model):
     status = db.Column(db.Enum(*JOB_STATUS_TYPES, name='job_status_types'), nullable=False, default='pending')
     notification_count = db.Column(db.Integer, nullable=False)
     notifications_sent = db.Column(db.Integer, nullable=False, default=0)
+    notifications_delivered = db.Column(db.Integer, nullable=False, default=0)
+    notifications_failed = db.Column(db.Integer, nullable=False, default=0)
+
     processing_started = db.Column(
         db.DateTime,
         index=False,

migrations/versions/0021_add_delivered_failed_counts.py

@@ -0,0 +1,48 @@
+"""empty message
+
+Revision ID: 0021_add_delivered_failed_counts
+Revises: 0020_template_history_fix
+Create Date: 2016-05-23 15:05:25.109346
+
+"""
+
+# revision identifiers, used by Alembic.
+revision = '0021_add_delivered_failed_counts'
+down_revision = '0020_template_history_fix'
+
+from alembic import op
+import sqlalchemy as sa
+
+
+def upgrade():
+    ### commands auto generated by Alembic - please adjust! ###
+    op.add_column('jobs', sa.Column('notifications_delivered', sa.Integer(), nullable=True))
+    op.add_column('jobs', sa.Column('notifications_failed', sa.Integer(), nullable=True))
+    conn = op.get_bind()
+    results = conn.execute("select distinct job_id from notifications")
+    res = results.fetchall()
+
+    for x in res:
+        if x.job_id:
+            op.execute("update jobs set notifications_delivered = ("
+                       "select count(status) from notifications where status = 'delivered' and job_id = '{}' "
+                       "group by status, job_id)"
+                       "where jobs.id = '{}'".format(x.job_id, x.job_id))
+
+            op.execute("update jobs set notifications_failed = ("
+                       "select count(status) from notifications "
+                       "where status in ('failed','technical-failure', 'temporary-failure', 'permanent-failure') "
+                       "and job_id = '{}' group by status, job_id)"
+                       "where jobs.id = '{}'".format(x.job_id, x.job_id))
+    op.execute("update jobs set notifications_delivered = 0 where notifications_delivered is null")
+    op.execute("update jobs set notifications_failed = 0 where notifications_failed is null")
+    op.alter_column('jobs', 'notifications_delivered', nullable=False)
+    op.alter_column('jobs', 'notifications_failed', nullable=False)
+    ### end Alembic commands ###
+
+
+def downgrade():
+    ### commands auto generated by Alembic - please adjust! ###
+    op.drop_column('jobs', 'notifications_failed')
+    op.drop_column('jobs', 'notifications_delivered')
+    ### end Alembic commands ###

tests/app/conftest.py

@@ -281,7 +281,7 @@ def sample_email_job(notify_db,
             service=service)
     job_id = uuid.uuid4()
     data = {
-        'id': uuid.uuid4(),
+        'id': job_id,
         'service_id': service.id,
         'service': service,
         'template_id': template.id,

tests/app/dao/test_jobs_dao.py

@@ -31,6 +31,8 @@ def test_create_job(sample_template):
     assert Job.query.count() == 1
     job_from_db = Job.query.get(job_id)
     assert job == job_from_db
+    assert job_from_db.notifications_delivered == 0
+    assert job_from_db.notifications_failed == 0
 
 
 def test_get_job_by_id(sample_job):

tests/app/dao/test_notification_dao.py

@@ -33,8 +33,7 @@ from app.dao.notifications_dao import (
 
 from notifications_utils.template import get_sms_fragment_count
 
-from tests.app.conftest import sample_job
+from tests.app.conftest import (sample_notification)
-from tests.app.conftest import sample_notification
 
 
 def test_should_by_able_to_update_reference_by_id(sample_notification):
@@ -57,38 +56,43 @@ def test_should_by_able_to_update_status_by_reference(sample_email_template, ses
     update_notification_reference_by_id(notification.id, 'reference')
     update_notification_status_by_reference('reference', 'delivered', 'delivered')
     assert Notification.query.get(notification.id).status == 'delivered'
-    assert NotificationStatistics.query.filter_by(
+    stats = NotificationStatistics.query.filter_by(service_id=sample_email_template.service.id).one()
-        service_id=sample_email_template.service.id
+    assert stats.emails_delivered == 1
-    ).one().emails_delivered == 1
+    assert stats.emails_requested == 1
-    assert NotificationStatistics.query.filter_by(
+    assert stats.emails_failed == 0
-        service_id=sample_email_template.service.id
-    ).one().emails_requested == 1
-    assert NotificationStatistics.query.filter_by(
-        service_id=sample_email_template.service.id
-    ).one().emails_failed == 0
 
 
-def test_should_by_able_to_update_status_by_id(sample_notification):
+def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_provider):
-    assert Notification.query.get(sample_notification.id).status == 'sending'
+    data = _notification_json(sample_template, job_id=sample_job.id)
-    count = update_notification_status_by_id(sample_notification.id, 'delivered', 'delivered')
+    notification = Notification(**data)
+    dao_create_notification(notification, sample_template.template_type, 'mmg')
+    assert Notification.query.get(notification.id).status == 'sending'
+    count = update_notification_status_by_id(notification.id, 'delivered', 'delivered')
     assert count == 1
-    assert Notification.query.get(sample_notification.id).status == 'delivered'
+    assert Notification.query.get(notification.id).status == 'delivered'
-    stats = NotificationStatistics.query.filter_by(service_id=sample_notification.service.id).one()
+    stats = NotificationStatistics.query.filter_by(service_id=notification.service.id).one()
     assert stats.sms_delivered == 1
     assert stats.sms_requested == 1
     assert stats.sms_failed == 0
+    job = Job.query.get(sample_job.id)
+    assert job.notification_count == 1
+    assert job.notifications_sent == 1
+    assert job.notifications_delivered == 1
+    assert job.notifications_failed == 0
 
 
-def test_should_not_update_status_by_id_if_not_sending(notify_db, notify_db_session):
+def test_should_not_update_status_by_id_if_not_sending_and_does_not_update_job(notify_db, notify_db_session):
     notification = sample_notification(notify_db, notify_db_session, status='delivered')
+    job = Job.query.get(notification.job_id)
     assert Notification.query.get(notification.id).status == 'delivered'
     count = update_notification_status_by_id(notification.id, 'failed', 'failure')
     assert count == 0
     assert Notification.query.get(notification.id).status == 'delivered'
+    assert job == Job.query.get(notification.job_id)
 
 
-def test_should_not_update_status_one_notification_status_is_delivered(sample_email_template, ses_provider):
+def test_should_not_update_status_one_notification_status_is_delivered(sample_email_template, sample_job, ses_provider):
-    data = _notification_json(sample_email_template)
+    data = _notification_json(sample_email_template, job_id=sample_job.id)
 
     notification = Notification(**data)
     dao_create_notification(
@@ -101,28 +105,37 @@ def test_should_not_update_status_one_notification_status_is_delivered(sample_em
     update_notification_status_by_reference('reference', 'delivered', 'delivered')
     assert Notification.query.get(notification.id).status == 'delivered'
 
-    update_notification_status_by_reference('reference', 'delivered', 'temporary-failure')
+    update_notification_status_by_reference('reference', 'failed', 'temporary-failure')
     assert Notification.query.get(notification.id).status == 'delivered'
     stats = NotificationStatistics.query.filter_by(service_id=sample_email_template.service.id).one()
     assert stats.emails_delivered == 1
     assert stats.emails_requested == 1
     assert stats.emails_failed == 0
+    job = Job.query.get(notification.job_id)
+    assert job.notification_count == 1
+    assert job.notifications_delivered == 1
+    assert job.notifications_failed == 0
+    assert job.notifications_sent == 1
 
 
 def test_should_be_able_to_record_statistics_failure_for_sms(sample_notification):
     assert Notification.query.get(sample_notification.id).status == 'sending'
-    count = update_notification_status_by_id(sample_notification.id, 'delivered', 'failure')
+    count = update_notification_status_by_id(sample_notification.id, 'permanent-failure', 'failure')
     assert count == 1
-    assert Notification.query.get(sample_notification.id).status == 'delivered'
+    assert Notification.query.get(sample_notification.id).status == 'permanent-failure'
     stats = NotificationStatistics.query.filter_by(service_id=sample_notification.service.id).one()
     assert stats.sms_delivered == 0
     assert stats.sms_requested == 1
     assert stats.sms_failed == 1
+    job_stats = Job.query.filter_by(id=sample_notification.job_id).first()
+    assert job_stats.notification_count == 1
+    assert job_stats.notifications_sent == 1
+    assert job_stats.notifications_failed == 1
+    assert job_stats.notifications_delivered == 0
 
 
-def test_should_be_able_to_record_statistics_failure_for_email(sample_email_template, ses_provider):
+def test_should_be_able_to_record_statistics_failure_for_email(sample_email_template, sample_job, ses_provider):
-    data = _notification_json(sample_email_template)
+    data = _notification_json(sample_email_template, job_id=sample_job.id)
-
     notification = Notification(**data)
     dao_create_notification(notification, sample_email_template.template_type, ses_provider.identifier)
 
@@ -134,6 +147,11 @@ def test_should_be_able_to_record_statistics_failure_for_email(sample_email_temp
     assert stats.emails_delivered == 0
     assert stats.emails_requested == 1
     assert stats.emails_failed == 1
+    job_stats = Job.query.filter_by(id=notification.job_id).first()
+    assert job_stats.notification_count == 1
+    assert job_stats.notifications_sent == 1
+    assert job_stats.notifications_failed == 1
+    assert job_stats.notifications_delivered == 0
 
 
 def test_should_return_zero_count_if_no_notification_with_id():
@@ -301,7 +319,11 @@ def test_save_notification_creates_sms_and_template_stats(sample_template, sampl
     assert data['template_version'] == notification_from_db.template_version
     assert data['created_at'] == notification_from_db.created_at
     assert 'sending' == notification_from_db.status
-    assert Job.query.get(sample_job.id).notifications_sent == 1
+    job = Job.query.get(sample_job.id)
+    assert job.notifications_sent == 1
+    assert job.notification_count == 1
+    assert job.notifications_failed == 0
+    assert job.notifications_delivered == 0
 
     stats = NotificationStatistics.query.filter(
         NotificationStatistics.service_id == sample_template.service.id
@@ -339,7 +361,11 @@ def test_save_notification_and_create_email_and_template_stats(sample_email_temp
     assert data['template_version'] == notification_from_db.template_version
     assert data['created_at'] == notification_from_db.created_at
     assert 'sending' == notification_from_db.status
-    assert Job.query.get(sample_job.id).notifications_sent == 1
+    job = Job.query.get(sample_job.id)
+    assert job.notifications_sent == 1
+    assert job.notification_count == 1
+    assert job.notifications_delivered == 0
+    assert job.notifications_failed == 0
 
     stats = NotificationStatistics.query.filter(
         NotificationStatistics.service_id == sample_email_template.service.id
@@ -511,6 +537,7 @@ def test_should_not_increment_job_if_notification_fails_to_persist(sample_templa
 
 
 def test_save_notification_and_increment_correct_job(notify_db, notify_db_session, sample_template, mmg_provider):
+    from tests.app.conftest import sample_job
     job_1 = sample_job(notify_db, notify_db_session, sample_template.service)
     job_2 = sample_job(notify_db, notify_db_session, sample_template.service)
 
@@ -585,7 +612,6 @@ def test_get_notification_for_job(sample_notification):
 
 
 def test_get_all_notifications_for_job(notify_db, notify_db_session, sample_job):
-    from tests.app.conftest import sample_notification
     for i in range(0, 5):
         try:
             sample_notification(notify_db,