From 0d06be05e1572c25ebefff5b7348b1a0050081b7 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 4 Apr 2016 12:21:38 +0100 Subject: [PATCH 1/2] [WIP] Added dao method and rest endpoint for getting template statistics by service. Some cosmetic changes to imports. Added fix for job rest not correctly returning errors. --- app/__init__.py | 2 + app/dao/notifications_dao.py | 94 ++++++++------ app/job/rest.py | 2 + app/models.py | 2 +- app/schemas.py | 35 ++++-- app/template_statistics/__init__.py | 0 app/template_statistics/rest.py | 36 ++++++ tests/app/dao/test_notification_dao.py | 127 ++++++++++++++++++- tests/app/template_statistics/__init__.py | 0 tests/app/template_statistics/test_rest.py | 136 +++++++++++++++++++++ 10 files changed, 390 insertions(+), 44 deletions(-) create mode 100644 app/template_statistics/__init__.py create mode 100644 app/template_statistics/rest.py create mode 100644 tests/app/template_statistics/__init__.py create mode 100644 tests/app/template_statistics/test_rest.py diff --git a/app/__init__.py b/app/__init__.py index 6802f5353..8dd8f120d 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -56,6 +56,7 @@ def create_app(app_name=None): from app.permission.rest import permission as permission_blueprint from app.accept_invite.rest import accept_invite from app.notifications_statistics.rest import notifications_statistics as notifications_statistics_blueprint + from app.template_statistics.rest import template_statistics as template_statistics_blueprint application.register_blueprint(service_blueprint, url_prefix='/service') application.register_blueprint(user_blueprint, url_prefix='/user') @@ -67,6 +68,7 @@ def create_app(app_name=None): application.register_blueprint(permission_blueprint, url_prefix='/permission') application.register_blueprint(accept_invite, url_prefix='/invite') application.register_blueprint(notifications_statistics_blueprint) + application.register_blueprint(template_statistics_blueprint) return application diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index d7b049290..1c94fad53 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -25,6 +25,23 @@ from app.clients import ( STATISTICS_REQUESTED ) +from functools import wraps + + +def transactional(func): + @wraps(func) + def commit_or_rollback(*args, **kwargs): + from flask import current_app + from app import db + try: + func(*args, **kwargs) + db.session.commit() + except Exception as e: + current_app.logger.error(e) + db.session.rollback() + raise + return commit_or_rollback + def dao_get_notification_statistics_for_service(service_id): return NotificationStatistics.query.filter_by( @@ -39,46 +56,55 @@ def dao_get_notification_statistics_for_service_and_day(service_id, day): ).order_by(desc(NotificationStatistics.day)).first() +def dao_get_template_statistics_for_service(service_id, limit_days=None): + filter = [TemplateStatistics.service_id == service_id] + if limit_days: + latest_stat = TemplateStatistics.query.filter_by(service_id=service_id).order_by( + desc(TemplateStatistics.day)).limit(1).first() + if latest_stat: + last_date_to_fetch = latest_stat.day - timedelta(days=limit_days) + else: + last_date_to_fetch = date.today() - timedelta(days=limit_days) + filter.append(TemplateStatistics.day > last_date_to_fetch) + return TemplateStatistics.query.filter(*filter).order_by(desc(TemplateStatistics.day)).all() + + +@transactional def dao_create_notification(notification, notification_type): - try: - if notification.job_id: - db.session.query(Job).filter_by( - id=notification.job_id - ).update({ - Job.notifications_sent: Job.notifications_sent + 1, - Job.updated_at: datetime.utcnow() - }) + if notification.job_id: + db.session.query(Job).filter_by( + id=notification.job_id + ).update({ + Job.notifications_sent: Job.notifications_sent + 1, + Job.updated_at: datetime.utcnow() + }) - update_count = db.session.query(NotificationStatistics).filter_by( + update_count = db.session.query(NotificationStatistics).filter_by( + day=notification.created_at.strftime('%Y-%m-%d'), + service_id=notification.service_id + ).update(update_query(notification_type, 'requested')) + + if update_count == 0: + stats = NotificationStatistics( day=notification.created_at.strftime('%Y-%m-%d'), - service_id=notification.service_id - ).update(update_query(notification_type, 'requested')) - - if update_count == 0: - stats = NotificationStatistics( - day=notification.created_at.strftime('%Y-%m-%d'), - service_id=notification.service_id, - sms_requested=1 if notification_type == TEMPLATE_TYPE_SMS else 0, - emails_requested=1 if notification_type == TEMPLATE_TYPE_EMAIL else 0 - ) - db.session.add(stats) - - update_count = db.session.query(TemplateStatistics).filter_by( - day=date.today(), service_id=notification.service_id, - template_id=notification.template_id - ).update({'usage_count': TemplateStatistics.usage_count + 1}) + sms_requested=1 if notification_type == TEMPLATE_TYPE_SMS else 0, + emails_requested=1 if notification_type == TEMPLATE_TYPE_EMAIL else 0 + ) + db.session.add(stats) - if update_count == 0: - template_stats = TemplateStatistics(template_id=notification.template_id, - service_id=notification.service_id) - db.session.add(template_stats) + update_count = db.session.query(TemplateStatistics).filter_by( + day=date.today(), + service_id=notification.service_id, + template_id=notification.template_id + ).update({'usage_count': TemplateStatistics.usage_count + 1}) - db.session.add(notification) - db.session.commit() - except: - db.session.rollback() - raise + if update_count == 0: + template_stats = TemplateStatistics(template_id=notification.template_id, + service_id=notification.service_id) + db.session.add(template_stats) + + db.session.add(notification) def update_query(notification_type, status): diff --git a/app/job/rest.py b/app/job/rest.py index 78db23716..59eda8666 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -37,6 +37,8 @@ def get_job_by_service_and_job_id(service_id, job_id): def get_jobs_by_service(service_id): jobs = dao_get_jobs_by_service_id(service_id) data, errors = job_schema.dump(jobs, many=True) + if errors: + return jsonify(result="error", message=errors), 400 return jsonify(data=data) diff --git a/app/models.py b/app/models.py index 333d62a20..165903a90 100644 --- a/app/models.py +++ b/app/models.py @@ -357,7 +357,7 @@ class TemplateStatistics(db.Model): id = db.Column(UUID(as_uuid=True), primary_key=True, default=uuid.uuid4) service_id = db.Column(UUID(as_uuid=True), db.ForeignKey('services.id'), index=True, unique=False, nullable=False) - service = db.relationship('Service', backref=db.backref('template_statics', lazy='dynamic')) + service = db.relationship('Service', backref=db.backref('template_statistics', lazy='dynamic')) template_id = db.Column(db.BigInteger, db.ForeignKey('templates.id'), index=True, nullable=False, unique=False) template = db.relationship('Template') usage_count = db.Column(db.BigInteger, index=False, unique=False, nullable=False, default=1) diff --git a/app/schemas.py b/app/schemas.py index 1181a7bef..0f440a771 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -1,15 +1,27 @@ from flask_marshmallow.fields import fields -from . import ma -from . import models -from app.dao.permissions_dao import permission_dao -from marshmallow import (post_load, ValidationError, validates, validates_schema, pre_load) + +from marshmallow import ( + post_load, + ValidationError, + validates, + validates_schema, + pre_load +) + from marshmallow_sqlalchemy import field_for + from utils.recipients import ( - validate_email_address, InvalidEmailError, - validate_phone_number, InvalidPhoneError, + validate_email_address, + InvalidEmailError, + validate_phone_number, + InvalidPhoneError, validate_and_format_phone_number ) +from app import ma +from app import models +from app.dao.permissions_dao import permission_dao + # TODO I think marshmallow provides a better integration and error handling. # Would be better to replace functionality in dao with the marshmallow supported @@ -19,7 +31,7 @@ from utils.recipients import ( class BaseSchema(ma.ModelSchema): - def __init__(self, *args, load_json=False, **kwargs): + def __init__(self, load_json=False, *args, **kwargs): self.load_json = load_json super(BaseSchema, self).__init__(*args, **kwargs) @@ -241,6 +253,14 @@ class NotificationsFilterSchema(ma.Schema): raise ValidationError("Not a positive integer") +class TemplateStatisticsSchema(BaseSchema): + + template = fields.Nested(TemplateSchema, only=["id", "name", "template_type"], dump_only=True) + + class Meta: + model = models.TemplateStatistics + + user_schema = UserSchema() user_schema_load_json = UserSchema(load_json=True) service_schema = ServiceSchema() @@ -264,3 +284,4 @@ permission_schema = PermissionSchema() email_data_request_schema = EmailDataSchema() notifications_statistics_schema = NotificationsStatisticsSchema() notifications_filter_schema = NotificationsFilterSchema() +template_statistics_schema = TemplateStatisticsSchema() diff --git a/app/template_statistics/__init__.py b/app/template_statistics/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py new file mode 100644 index 000000000..b935cdc06 --- /dev/null +++ b/app/template_statistics/rest.py @@ -0,0 +1,36 @@ +from flask import ( + Blueprint, + jsonify, + request, + current_app +) + +from app.dao.notifications_dao import dao_get_template_statistics_for_service + +from app.schemas import template_statistics_schema + +template_statistics = Blueprint('template-statistics', + __name__, + url_prefix='/service//template-statistics') + +from app.errors import register_errors + +register_errors(template_statistics) + + +@template_statistics.route('') +def get_template_statistics_for_service(service_id): + if request.args.get('limit_days'): + try: + limit_days = int(request.args['limit_days']) + except ValueError as e: + error = 'Limit days {} is not an integer'.format(request.args['limit_days']) + current_app.logger.error(error) + return jsonify(result="error", message=[error]), 400 + else: + limit_days = None + stats = dao_get_template_statistics_for_service(service_id, limit_days=limit_days) + data, errors = template_statistics_schema.dump(stats, many=True) + if errors: + return jsonify(result="error", message=errors), 400 + return jsonify(data=data) diff --git a/tests/app/dao/test_notification_dao.py b/tests/app/dao/test_notification_dao.py index 83d79bd33..958b56a06 100644 --- a/tests/app/dao/test_notification_dao.py +++ b/tests/app/dao/test_notification_dao.py @@ -28,7 +28,8 @@ from app.dao.notifications_dao import ( dao_get_notification_statistics_for_service_and_day, update_notification_status_by_id, update_notification_reference_by_id, - update_notification_status_by_reference + update_notification_status_by_reference, + dao_get_template_statistics_for_service ) from tests.app.conftest import sample_job @@ -886,10 +887,132 @@ def test_successful_notification_inserts_followed_by_failure_does_not_increment_ db.session.execute('DROP TABLE TEMPLATE_STATISTICS') dao_create_notification(failing_notification, sample_template.template_type) except Exception as e: - # There should be no additional notification stats or counts assert NotificationStatistics.query.count() == 1 notication_stats = NotificationStatistics.query.filter( NotificationStatistics.service_id == sample_template.service.id ).first() assert notication_stats.sms_requested == 3 + + +@freeze_time("2016-03-30") +def test_get_template_stats_for_service_returns_stats_in_reverse_date_order(sample_template, sample_job): + + template_stats = dao_get_template_statistics_for_service(sample_template.service.id) + assert len(template_stats) == 0 + data = { + 'to': '+44709123456', + 'job_id': sample_job.id, + 'service': sample_template.service, + 'service_id': sample_template.service.id, + 'template': sample_template, + 'template_id': sample_template.id, + 'created_at': datetime.utcnow() + } + + notification = Notification(**data) + dao_create_notification(notification, sample_template.template_type) + + # move on one day + with freeze_time('2016-03-31'): + new_notification = Notification(**data) + dao_create_notification(new_notification, sample_template.template_type) + + # move on one more day + with freeze_time('2016-04-01'): + new_notification = Notification(**data) + dao_create_notification(new_notification, sample_template.template_type) + + template_stats = dao_get_template_statistics_for_service(sample_template.service_id) + assert len(template_stats) == 3 + assert template_stats[0].day == date(2016, 4, 1) + assert template_stats[1].day == date(2016, 3, 31) + assert template_stats[2].day == date(2016, 3, 30) + + +@freeze_time('2016-04-09') +def test_get_template_stats_for_service_returns_stats_can_limit_number_of_days_returned(sample_template): + + template_stats = dao_get_template_statistics_for_service(sample_template.service.id) + assert len(template_stats) == 0 + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + # Retrieve last week of stats + template_stats = dao_get_template_statistics_for_service(sample_template.service_id, limit_days=7) + assert len(template_stats) == 7 + assert template_stats[0].day == date(2016, 4, 9) + assert template_stats[6].day == date(2016, 4, 3) + + +@freeze_time('2016-04-09') +def test_get_template_stats_for_service_returns_stats_returns_all_stats_if_no_limit(sample_template): + + template_stats = dao_get_template_statistics_for_service(sample_template.service.id) + assert len(template_stats) == 0 + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + template_stats = dao_get_template_statistics_for_service(sample_template.service_id) + assert len(template_stats) == 9 + assert template_stats[0].day == date(2016, 4, 9) + assert template_stats[8].day == date(2016, 4, 1) + + +@freeze_time('2016-04-30') +def test_get_template_stats_for_service_returns_results_from_first_day_with_data(sample_template): + + template_stats = dao_get_template_statistics_for_service(sample_template.service.id) + assert len(template_stats) == 0 + + # make 9 stats records from 1st to 9th April - no data after 10th + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + # Retrieve one day of stats - read date is 2016-04-30 + template_stats = dao_get_template_statistics_for_service(sample_template.service_id, limit_days=1) + assert len(template_stats) == 1 + assert template_stats[0].day == date(2016, 4, 9) + + # Retrieve three days of stats + template_stats = dao_get_template_statistics_for_service(sample_template.service_id, limit_days=3) + assert len(template_stats) == 3 + assert template_stats[0].day == date(2016, 4, 9) + assert template_stats[1].day == date(2016, 4, 8) + assert template_stats[2].day == date(2016, 4, 7) + + # Retrieve nine days of stats + template_stats = dao_get_template_statistics_for_service(sample_template.service_id, limit_days=9) + assert len(template_stats) == 9 + assert template_stats[0].day == date(2016, 4, 9) + assert template_stats[8].day == date(2016, 4, 1) + + # Retrieve with no limit + template_stats = dao_get_template_statistics_for_service(sample_template.service_id) + assert len(template_stats) == 9 + assert template_stats[0].day == date(2016, 4, 9) + assert template_stats[8].day == date(2016, 4, 1) + + +def test_get_template_stats_for_service_with_limit_if_no_records_returns_empty_list(sample_template): + template_stats = dao_get_template_statistics_for_service(sample_template.service.id, limit_days=7) + assert len(template_stats) == 0 diff --git a/tests/app/template_statistics/__init__.py b/tests/app/template_statistics/__init__.py new file mode 100644 index 000000000..e69de29bb diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py new file mode 100644 index 000000000..51826d90a --- /dev/null +++ b/tests/app/template_statistics/test_rest.py @@ -0,0 +1,136 @@ +import json +from freezegun import freeze_time + +from app import db +from app.models import TemplateStatistics + +from tests import create_authorization_header + + +@freeze_time('2016-04-09') +def test_get_template_statistics_for_service_for_last_week(notify_api, sample_template): + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header( + path='/service/{}/template-statistics'.format(sample_template.service_id), + method='GET' + ) + + response = client.get( + '/service/{}/template-statistics'.format(sample_template.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 7} + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 7 + assert json_resp['data'][0]['day'] == '2016-04-09' + assert json_resp['data'][6]['day'] == '2016-04-03' + + +@freeze_time('2016-04-30') +def test_get_template_statistics_for_service_for_last_actual_week_with_data(notify_api, sample_template): + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header( + path='/service/{}/template-statistics'.format(sample_template.service_id), + method='GET' + ) + + # Date is frozen at 2016-04-30 and no data written since + response = client.get( + '/service/{}/template-statistics'.format(sample_template.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 7} + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 7 + assert json_resp['data'][0]['day'] == '2016-04-09' + assert json_resp['data'][6]['day'] == '2016-04-03' + + +def test_get_all_template_statistics_for_service(notify_api, sample_template): + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header( + path='/service/{}/template-statistics'.format(sample_template.service_id), + method='GET' + ) + + response = client.get( + '/service/{}/template-statistics'.format(sample_template.service_id), + headers=[('Content-Type', 'application/json'), auth_header] + ) + + assert response.status_code == 200 + json_resp = json.loads(response.get_data(as_text=True)) + assert len(json_resp['data']) == 9 + assert json_resp['data'][0]['day'] == '2016-04-09' + assert json_resp['data'][8]['day'] == '2016-04-01' + + +def test_get_all_template_statistics_with_bad_limit_arg_returns_400(notify_api, sample_template): + + # make 9 stats records from 1st to 9th April + for i in range(1, 10): + past_date = '2016-04-0{}'.format(i) + with freeze_time(past_date): + template_stats = TemplateStatistics(template_id=sample_template.id, + service_id=sample_template.service_id) + db.session.add(template_stats) + db.session.commit() + + with notify_api.test_request_context(): + with notify_api.test_client() as client: + + auth_header = create_authorization_header( + path='/service/{}/template-statistics'.format(sample_template.service_id), + method='GET' + ) + + response = client.get( + '/service/{}/template-statistics'.format(sample_template.service_id), + headers=[('Content-Type', 'application/json'), auth_header], + query_string={'limit_days': 'blurk'} + ) + + assert response.status_code == 400 + json_resp = json.loads(response.get_data(as_text=True)) + assert json_resp['result'] == 'error' + assert json_resp['message'] == ['Limit days blurk is not an integer'] From e4a5e3890a4e0a3e260742b5988d0cb20aa35fdf Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Mon, 4 Apr 2016 14:51:56 +0100 Subject: [PATCH 2/2] Corrected error message format --- app/template_statistics/rest.py | 4 ++-- tests/app/template_statistics/test_rest.py | 2 +- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/template_statistics/rest.py b/app/template_statistics/rest.py index b935cdc06..3446d2623 100644 --- a/app/template_statistics/rest.py +++ b/app/template_statistics/rest.py @@ -24,9 +24,9 @@ def get_template_statistics_for_service(service_id): try: limit_days = int(request.args['limit_days']) except ValueError as e: - error = 'Limit days {} is not an integer'.format(request.args['limit_days']) + error = '{} is not an integer'.format(request.args['limit_days']) current_app.logger.error(error) - return jsonify(result="error", message=[error]), 400 + return jsonify(result="error", message={'limit_days': [error]}), 400 else: limit_days = None stats = dao_get_template_statistics_for_service(service_id, limit_days=limit_days) diff --git a/tests/app/template_statistics/test_rest.py b/tests/app/template_statistics/test_rest.py index 51826d90a..20c99ba67 100644 --- a/tests/app/template_statistics/test_rest.py +++ b/tests/app/template_statistics/test_rest.py @@ -133,4 +133,4 @@ def test_get_all_template_statistics_with_bad_limit_arg_returns_400(notify_api, assert response.status_code == 400 json_resp = json.loads(response.get_data(as_text=True)) assert json_resp['result'] == 'error' - assert json_resp['message'] == ['Limit days blurk is not an integer'] + assert json_resp['message'] == {'limit_days': ['blurk is not an integer']}