From dff60175a4b956e299615ce1ebaf2fd5ba045b70 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 23 May 2016 15:44:57 +0100 Subject: [PATCH 1/3] Add jobs.notifications_delivered and jobs.notifications_failed counts to the jobs table. --- app/models.py | 3 ++ .../0021_add_delivered_failed_counts.py | 48 +++++++++++++++++++ tests/app/dao/test_jobs_dao.py | 2 + 3 files changed, 53 insertions(+) create mode 100644 migrations/versions/0021_add_delivered_failed_counts.py diff --git a/app/models.py b/app/models.py index 120f7ef40..894398ea8 100644 --- a/app/models.py +++ b/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, diff --git a/migrations/versions/0021_add_delivered_failed_counts.py b/migrations/versions/0021_add_delivered_failed_counts.py new file mode 100644 index 000000000..641026785 --- /dev/null +++ b/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 ### diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 5628cd73f..79d9d08c5 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/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): From e4eecb894aad649d62804741914bb8cd2d246612 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 24 May 2016 11:31:44 +0100 Subject: [PATCH 2/3] Update job.notifications_delivered and job.notifications_failed count when updating the status of a notification. --- app/dao/notifications_dao.py | 67 ++++++++++---------- tests/app/conftest.py | 2 +- tests/app/dao/test_notification_dao.py | 85 +++++++++++++++++--------- 3 files changed, 93 insertions(+), 61 deletions(-) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 57232b632..4a8d279cb 100644 --- a/app/dao/notifications_dao.py +++ b/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( - day=day - ).all() + return NotificationStatistics.query.filter_by(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)) - return TemplateStatistics.query.filter(*filter).order_by( + query_filter.append(TemplateStatistics.day >= days_ago(limit_days)) + 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( - day=notification.created_at.date(), - service_id=notification.service_id - ).update( - update_query(notification.template.template_type, notification_statistics_status) - ) + _update_statistics(notification, 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( - reference=reference - ).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) - ) + notification = Notification.query.filter_by(reference=reference).first() + _update_statistics(notification, 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: diff --git a/tests/app/conftest.py b/tests/app/conftest.py index b9bcf91a3..cdb4cb0ec 100644 --- a/tests/app/conftest.py +++ b/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, diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 674f018a4..83e9559eb 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/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,40 @@ 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( - service_id=sample_email_template.service.id - ).one().emails_delivered == 1 - assert NotificationStatistics.query.filter_by( - 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 + 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 -def test_should_by_able_to_update_status_by_id(sample_notification): - assert Notification.query.get(sample_notification.id).status == 'sending' - count = update_notification_status_by_id(sample_notification.id, 'delivered', 'delivered') +def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_provider): + data = _notification_json(sample_template, job_id=sample_job.id) + 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' - stats = NotificationStatistics.query.filter_by(service_id=sample_notification.service.id).one() + assert Notification.query.get(notification.id).status == 'delivered' + 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 -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): - data = _notification_json(sample_email_template) +def test_should_not_update_status_one_notification_status_is_delivered(sample_email_template, sample_job, ses_provider): + data = _notification_json(sample_email_template, job_id=sample_job.id) notification = Notification(**data) dao_create_notification( @@ -101,28 +102,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): - data = _notification_json(sample_email_template) - +def test_should_be_able_to_record_statistics_failure_for_email(sample_email_template, sample_job, ses_provider): + 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 +144,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 +316,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 +358,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 +534,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 +609,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, @@ -910,7 +933,7 @@ def test_sms_fragment_count(char_count, expected_sms_fragment_count): assert get_sms_fragment_count(char_count) == expected_sms_fragment_count -def _notification_json(sample_template, job_id=None, id=None): +def _notification_json(sample_template, job_id=None, id=None, reference=None, status=None): data = { 'to': '+44709123456', 'service': sample_template.service, @@ -925,4 +948,8 @@ def _notification_json(sample_template, job_id=None, id=None): data.update({'job_id': job_id}) if id: data.update({'id': id}) + if reference: + data.update({'reference': reference}) + if status: + data.update({'status': status}) return data From d6c7f0360784bb4662e5894c9bb5726f781038b8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 24 May 2016 16:37:05 +0100 Subject: [PATCH 3/3] Added another assert to the test --- tests/app/dao/test_notification_dao.py | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 83e9559eb..d1ef6c6ab 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -76,6 +76,9 @@ def test_should_by_able_to_update_status_by_id(sample_template, sample_job, mmg_ 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_and_does_not_update_job(notify_db, notify_db_session): @@ -933,7 +936,7 @@ def test_sms_fragment_count(char_count, expected_sms_fragment_count): assert get_sms_fragment_count(char_count) == expected_sms_fragment_count -def _notification_json(sample_template, job_id=None, id=None, reference=None, status=None): +def _notification_json(sample_template, job_id=None, id=None): data = { 'to': '+44709123456', 'service': sample_template.service, @@ -948,8 +951,4 @@ def _notification_json(sample_template, job_id=None, id=None, reference=None, st data.update({'job_id': job_id}) if id: data.update({'id': id}) - if reference: - data.update({'reference': reference}) - if status: - data.update({'status': status}) return data