From dff60175a4b956e299615ce1ebaf2fd5ba045b70 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 23 May 2016 15:44:57 +0100 Subject: [PATCH 1/6] 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/6] 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/6] 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 From 2a7872cce1320e229b048282c09186e3ff3a8986 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 24 May 2016 17:21:04 +0100 Subject: [PATCH 4/6] Add a limit days query param for get all jobs. --- app/dao/jobs_dao.py | 13 +++++++++++-- app/job/rest.py | 16 +++++++++++++--- tests/app/dao/test_jobs_dao.py | 21 ++++++++++++++++++++- tests/app/job/test_rest.py | 28 ++++++++++++++++++++++++++++ 4 files changed, 72 insertions(+), 6 deletions(-) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 7b6a052ae..723f845b7 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -1,3 +1,5 @@ +from datetime import date, timedelta + from sqlalchemy import desc from app import db from app.models import Job @@ -7,8 +9,11 @@ def dao_get_job_by_service_id_and_job_id(service_id, job_id): return Job.query.filter_by(service_id=service_id, id=job_id).one() -def dao_get_jobs_by_service_id(service_id): - return Job.query.filter_by(service_id=service_id).order_by(desc(Job.created_at)).all() +def dao_get_jobs_by_service_id(service_id, limit_days=None): + query_filter = [Job.service_id == service_id] + if limit_days is not None: + query_filter.append(Job.created_at >= _days_ago(limit_days)) + return Job.query.filter(*query_filter).order_by(desc(Job.created_at)).all() def dao_get_job_by_id(job_id): @@ -23,3 +28,7 @@ def dao_create_job(job): def dao_update_job(job): db.session.add(job) db.session.commit() + + +def _days_ago(number_of_days): + return date.today() - timedelta(days=number_of_days) diff --git a/app/job/rest.py b/app/job/rest.py index 5c4147177..9b2d44382 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -1,8 +1,8 @@ from flask import ( Blueprint, jsonify, - request -) + request, + current_app) from app.dao.jobs_dao import ( dao_create_job, @@ -36,7 +36,17 @@ def get_job_by_service_and_job_id(service_id, job_id): @job.route('', methods=['GET']) def get_jobs_by_service(service_id): - jobs = dao_get_jobs_by_service_id(service_id) + if request.args.get('limit_days'): + try: + limit_days = int(request.args['limit_days']) + except ValueError as e: + error = '{} is not an integer'.format(request.args['limit_days']) + current_app.logger.error(error) + return jsonify(result="error", message={'limit_days': [error]}), 400 + else: + limit_days = None + + jobs = dao_get_jobs_by_service_id(service_id, limit_days) data, errors = job_schema.dump(jobs, many=True) if errors: return jsonify(result="error", message=errors), 400 diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 5628cd73f..472c3359c 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -1,4 +1,4 @@ -from datetime import datetime +from datetime import (datetime, timedelta) import uuid from app.dao.jobs_dao import ( @@ -64,6 +64,25 @@ def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): assert one_job_from_db != other_job_from_db +def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): + from tests.app.conftest import sample_job as create_job + + one_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template) + old_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template, + created_at=datetime.now() - timedelta(days=8)) + + jobs = dao_get_jobs_by_service_id(one_job.service_id) + + assert len(jobs) == 2 + assert one_job in jobs + assert old_job in jobs + + jobs_limit_days = dao_get_jobs_by_service_id(one_job.service_id, limit_days=7) + assert len(jobs_limit_days) == 1 + assert one_job in jobs_limit_days + assert old_job not in jobs_limit_days + + def test_get_jobs_for_service_in_created_at_order(notify_db, notify_db_session, sample_template): from tests.app.conftest import sample_job as create_job diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index 6e45a0195..761cecbf7 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -1,5 +1,7 @@ import json import uuid +from datetime import datetime, timedelta + import app.celery.tasks from tests import create_authorization_header @@ -22,6 +24,32 @@ def test_get_jobs(notify_api, notify_db, notify_db_session, sample_template): assert len(resp_json['data']) == 5 +def test_get_jobs_with_limit_days(notify_api, notify_db, notify_db_session, sample_template): + create_job( + notify_db, + notify_db_session, + service=sample_template.service, + template=sample_template, + ) + create_job( + notify_db, + notify_db_session, + service=sample_template.service, + template=sample_template, + created_at=datetime.now() - timedelta(days=7)) + + service_id = sample_template.service.id + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + path = '/service/{}/job'.format(service_id) + auth_header = create_authorization_header(service_id=service_id) + response = client.get(path, headers=[auth_header], query_string={'limit_days': 5}) + assert response.status_code == 200 + resp_json = json.loads(response.get_data(as_text=True)) + assert len(resp_json['data']) == 1 + + def test_get_job_with_invalid_service_id_returns404(notify_api, sample_api_key, sample_service): with notify_api.test_request_context(): with notify_api.test_client() as client: From a0b974f4ac41d17143c3bdee7ee935644a572cd1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 25 May 2016 09:13:32 +0100 Subject: [PATCH 5/6] Remove the status from the group by clause of the update statement. This script has been applied to all environments except live, this can be pushed through to live without issue because of how alembic works. --- migrations/versions/0021_add_delivered_failed_counts.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/migrations/versions/0021_add_delivered_failed_counts.py b/migrations/versions/0021_add_delivered_failed_counts.py index 641026785..41069b954 100644 --- a/migrations/versions/0021_add_delivered_failed_counts.py +++ b/migrations/versions/0021_add_delivered_failed_counts.py @@ -26,13 +26,13 @@ def upgrade(): 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)" + "group by 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)" + "and job_id = '{}' group by 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") From a67793cf8ae7c4a2277a4cdfe343b319880cce66 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 25 May 2016 11:13:49 +0100 Subject: [PATCH 6/6] Updated date query so that the created_at date is truncated so that we get all jobs created number of days requested. --- app/dao/__init__.py | 5 +++++ app/dao/jobs_dao.py | 9 +++------ app/dao/notifications_dao.py | 7 ++----- tests/app/dao/test_jobs_dao.py | 23 ++++++++++++++++++++++- 4 files changed, 32 insertions(+), 12 deletions(-) diff --git a/app/dao/__init__.py b/app/dao/__init__.py index 483e45fd1..a56fbce58 100644 --- a/app/dao/__init__.py +++ b/app/dao/__init__.py @@ -28,3 +28,8 @@ class DAOClass(object): db.session.delete(inst) if _commit: db.session.commit() + + +def days_ago(number_of_days): + from datetime import date, timedelta + return date.today() - timedelta(days=number_of_days) diff --git a/app/dao/jobs_dao.py b/app/dao/jobs_dao.py index 723f845b7..13742418a 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -1,7 +1,8 @@ from datetime import date, timedelta -from sqlalchemy import desc +from sqlalchemy import desc, cast, Date as sql_date from app import db +from app.dao import days_ago from app.models import Job @@ -12,7 +13,7 @@ def dao_get_job_by_service_id_and_job_id(service_id, job_id): def dao_get_jobs_by_service_id(service_id, limit_days=None): query_filter = [Job.service_id == service_id] if limit_days is not None: - query_filter.append(Job.created_at >= _days_ago(limit_days)) + query_filter.append(cast(Job.created_at, sql_date) >= days_ago(limit_days)) return Job.query.filter(*query_filter).order_by(desc(Job.created_at)).all() @@ -28,7 +29,3 @@ def dao_create_job(job): def dao_update_job(job): db.session.add(job) db.session.commit() - - -def _days_ago(number_of_days): - return date.today() - timedelta(days=number_of_days) diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 57232b632..1c85a05dd 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 ( @@ -11,6 +11,7 @@ from flask import current_app from werkzeug.datastructures import MultiDict from app import db +from app.dao import days_ago from app.models import ( Service, Notification, @@ -337,7 +338,3 @@ def delete_notifications_created_more_than_a_week_ago(status): ).delete(synchronize_session='fetch') db.session.commit() return deleted - - -def days_ago(number_of_days): - return date.today() - timedelta(days=number_of_days) diff --git a/tests/app/dao/test_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 472c3359c..4a7856086 100644 --- a/tests/app/dao/test_jobs_dao.py +++ b/tests/app/dao/test_jobs_dao.py @@ -64,7 +64,7 @@ def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): assert one_job_from_db != other_job_from_db -def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): +def test_get_jobs_for_service_with_limit_days_param(notify_db, notify_db_session, sample_template): from tests.app.conftest import sample_job as create_job one_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template) @@ -83,6 +83,27 @@ def test_get_jobs_for_service(notify_db, notify_db_session, sample_template): assert old_job not in jobs_limit_days +def test_get_jobs_for_service_with_limit_days_edge_case(notify_db, notify_db_session, sample_template): + from tests.app.conftest import sample_job as create_job + + one_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template) + job_two = create_job(notify_db, notify_db_session, sample_template.service, sample_template, + created_at=(datetime.now() - timedelta(days=7)).date()) + one_second_after_midnight = datetime.combine((datetime.now() - timedelta(days=7)).date(), + datetime.strptime("000001", "%H%M%S").time()) + just_after_midnight_job = create_job(notify_db, notify_db_session, sample_template.service, sample_template, + created_at=one_second_after_midnight) + job_eight_days_old = create_job(notify_db, notify_db_session, sample_template.service, sample_template, + created_at=datetime.now() - timedelta(days=8)) + + jobs_limit_days = dao_get_jobs_by_service_id(one_job.service_id, limit_days=7) + assert len(jobs_limit_days) == 3 + assert one_job in jobs_limit_days + assert job_two in jobs_limit_days + assert just_after_midnight_job in jobs_limit_days + assert job_eight_days_old not in jobs_limit_days + + def test_get_jobs_for_service_in_created_at_order(notify_db, notify_db_session, sample_template): from tests.app.conftest import sample_job as create_job