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 7b6a052ae..13742418a 100644 --- a/app/dao/jobs_dao.py +++ b/app/dao/jobs_dao.py @@ -1,5 +1,8 @@ -from sqlalchemy import desc +from datetime import date, timedelta + +from sqlalchemy import desc, cast, Date as sql_date from app import db +from app.dao import days_ago from app.models import Job @@ -7,8 +10,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(cast(Job.created_at, sql_date) >= 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): diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 57232b632..72fc3a2f7 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, @@ -35,11 +36,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 +54,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 +130,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 +152,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 +193,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 +211,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 +248,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 +260,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 +285,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 +314,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: @@ -337,7 +343,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/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/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..41069b954 --- /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 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 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/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_jobs_dao.py b/tests/app/dao/test_jobs_dao.py index 5628cd73f..9f265ab0d 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 ( @@ -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): @@ -64,6 +66,46 @@ 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_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) + 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_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 diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 674f018a4..d1ef6c6ab 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,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( - 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 + 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): - 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 +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): - 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 +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, 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: