From 2d1babf2bb3f236e4569cbe78e3601c6e89bc361 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 18 Jul 2016 12:03:44 +0100 Subject: [PATCH] 'detailed' flag on GET /service/ if passed in, returns the service object with additional statistics dictionary, which will be used in the admin app to populate dashboard components. A new schema has been created for this to avoid clashing/ causing confusion with the existing schema, which is already used for PUT/POST as well, and this schema can be easily tailored to reduce ambiguity and lazy-loading --- app/dao/services_dao.py | 19 ++++++++--- app/schemas.py | 18 ++++++++++ app/service/rest.py | 41 ++++++++++++++++++++--- tests/app/service/test_rest.py | 60 ++++++++++++++++++++++++++++++++++ tests/conftest.py | 1 + 5 files changed, 131 insertions(+), 8 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index d1bb8efbe..7815a0656 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -1,15 +1,13 @@ import uuid -from app import db -from app.models import Service -from sqlalchemy import asc +from sqlalchemy import asc, func from sqlalchemy.orm import joinedload +from app import db from app.dao.dao_utils import ( transactional, version_class ) - from app.models import ( NotificationStatistics, TemplateStatistics, @@ -135,3 +133,16 @@ def delete_service_and_all_associated_db_objects(service): db.session.commit() list(map(db.session.delete, users)) db.session.commit() + + +def dao_fetch_stats_for_service(service_id): + return db.session.query( + Notification.notification_type, + Notification.status, + func.count(Notification.id).label('count') + ).filter( + Notification.service_id == service_id + ).group_by( + Notification.notification_type, + Notification.status, + ).all() diff --git a/app/schemas.py b/app/schemas.py index eed567401..7857a728e 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -123,6 +123,23 @@ class ServiceSchema(BaseSchema): raise ValidationError('Only alphanumeric characters allowed') +class DetailedServiceSchema(BaseSchema): + statistics = fields.Dict() + + class Meta: + model = models.Service + exclude = ( + 'api_keys', + 'templates', + 'users', + 'created_by', + 'jobs', + 'template_statistics', + 'service_provider_stats', + 'service_notification_stats' + ) + + class NotificationModelSchema(BaseSchema): class Meta: model = models.Notification @@ -486,6 +503,7 @@ user_schema = UserSchema() user_schema_load_json = UserSchema(load_json=True) service_schema = ServiceSchema() service_schema_load_json = ServiceSchema(load_json=True) +detailed_service_schema = DetailedServiceSchema() template_schema = TemplateSchema() template_schema_load_json = TemplateSchema(load_json=True) api_key_schema = ApiKeySchema() diff --git a/app/service/rest.py b/app/service/rest.py index 5ac83c3e0..617ccb84d 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -8,6 +8,7 @@ from flask import ( ) from sqlalchemy.orm.exc import NoResultFound +from app.models import EMAIL_TYPE, SMS_TYPE from app.dao.api_key_dao import ( save_model_api_key, get_model_api_keys, @@ -20,7 +21,8 @@ from app.dao.services_dao import ( dao_update_service, dao_fetch_all_services_by_user, dao_add_user_to_service, - dao_remove_user_from_service + dao_remove_user_from_service, + dao_fetch_stats_for_service ) from app.dao import notifications_dao from app.dao.provider_statistics_dao import get_fragment_count @@ -33,6 +35,7 @@ from app.schemas import ( permission_schema, notification_status_schema, notifications_filter_schema, + detailed_service_schema ) from app.utils import pagination_links from app.errors import ( @@ -57,10 +60,13 @@ def get_services(): @service.route('/', methods=['GET']) def get_service_by_id(service_id): - fetched = dao_fetch_service_by_id(service_id) + if 'detailed' in request.args: + return get_detailed_service(service_id) + else: + fetched = dao_fetch_service_by_id(service_id) - data = service_schema.dump(fetched).data - return jsonify(data=data) + data = service_schema.dump(fetched).data + return jsonify(data=data) @service.route('', methods=['POST']) @@ -227,3 +233,30 @@ def get_all_notifications_for_service(service_id): **kwargs ) ), 200 + + +def get_detailed_service(service_id): + service = dao_fetch_service_by_id(service_id) + statistics = dao_fetch_stats_for_service(service_id) + service.statistics = format_statistics(statistics) + data = detailed_service_schema.dump(service).data + return jsonify(data=data) + + +def format_statistics(statistics): + # statistics come in a named tuple with uniqueness from 'notification_type', 'status' - however missing + # statuses/notification types won't be represented and the status types need to be simplified/summed up + # so we can return emails/sms * created, sent, and failed + counts = { + template_type: { + status: 0 for status in ('requested', 'delivered', 'failed') + } for template_type in (EMAIL_TYPE, SMS_TYPE) + } + for row in statistics: + counts[row.notification_type]['requested'] += row.count + if row.status == 'delivered': + counts[row.notification_type]['delivered'] += row.count + elif row.status in ('failed', 'technical-failure', 'temporary-failure', 'permanent-failure'): + counts[row.notification_type]['failed'] += row.count + + return counts diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 5622b66df..4765191f1 100644 --- a/tests/app/service/test_rest.py +++ b/tests/app/service/test_rest.py @@ -1,5 +1,8 @@ import json +import collections import uuid + +import pytest from flask import url_for from app.dao.users_dao import save_model_user @@ -12,6 +15,7 @@ from tests.app.conftest import ( sample_user as create_sample_user, sample_notification as create_sample_notification ) +from app.service.rest import format_statistics def test_get_service_list(notify_api, service_factory): @@ -1086,3 +1090,59 @@ def test_set_sms_sender_for_service_rejects_invalid_characters(notify_api, sampl assert resp.status_code == 400 assert result['result'] == 'error' assert result['message'] == {'sms_sender': ['Only alphanumeric characters allowed']} + + +def test_get_detailed_service(notify_api, sample_service): + with notify_api.test_request_context(), notify_api.test_client() as client: + resp = client.get( + '/service/{}?detailed=False'.format(sample_service.id), + headers=[create_authorization_header()] + ) + + assert resp.status_code == 200 + service = json.loads(resp.get_data(as_text=True))['data'] + assert service['id'] == str(sample_service.id) + assert 'statistics' in service.keys() + assert set(service['statistics'].keys()) == set(['sms', 'email']) + assert service['statistics']['sms'] == { + 'requested': 0, + 'delivered': 0, + 'failed': 0 + } + + +Row = collections.namedtuple('row', ('notification_type', 'status', 'count')) + + +# email_counts and sms_counts are 3-tuple of requested, delivered, failed +@pytest.mark.idparametrize('stats, email_counts, sms_counts', { + 'empty': ([], [0, 0, 0], [0, 0, 0]), + 'always_increment_requested': ([ + Row('email', 'delivered', 1), + Row('email', 'failed', 1) + ], [2, 1, 1], [0, 0, 0]), + 'dont_mix_email_and_sms': ([ + Row('email', 'delivered', 1), + Row('sms', 'delivered', 1) + ], [1, 1, 0], [1, 1, 0]), + 'convert_fail_statuses_to_failed': ([ + Row('email', 'failed', 1), + Row('email', 'technical-failure', 1), + Row('email', 'temporary-failure', 1), + Row('email', 'permanent-failure', 1), + ], [4, 0, 4], [0, 0, 0]), +}) +def test_format_statistics(stats, email_counts, sms_counts): + ret = format_statistics(stats) + + assert ret['email'] == { + status: count + for status, count + in zip(['requested', 'delivered', 'failed'], email_counts) + } + + assert ret['sms'] == { + status: count + for status, count + in zip(['requested', 'delivered', 'failed'], sms_counts) + } diff --git a/tests/conftest.py b/tests/conftest.py index 511cdf5fd..0f88fe667 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -69,6 +69,7 @@ def sqs_client_conn(request): boto3.setup_default_session(region_name='eu-west-1') return boto3.resource('sqs') + def pytest_generate_tests(metafunc): # Copied from https://gist.github.com/pfctdayelise/5719730 idparametrize = getattr(metafunc.function, 'idparametrize', None)