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 94d3aabd8..617ccb84d 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -8,20 +8,21 @@ 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, get_unsigned_secret, expire_api_key) from app.dao.services_dao import ( - dao_fetch_service_by_id_and_user, dao_fetch_service_by_id, dao_fetch_all_services, dao_create_service, 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 @@ -34,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 ( @@ -58,14 +60,13 @@ def get_services(): @service.route('/', methods=['GET']) def get_service_by_id(service_id): - user_id = request.args.get('user_id', None) - if user_id: - fetched = dao_fetch_service_by_id_and_user(service_id, user_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']) @@ -232,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/conftest.py b/tests/app/conftest.py index ce78dc2eb..f79088c9b 100644 --- a/tests/app/conftest.py +++ b/tests/app/conftest.py @@ -3,7 +3,6 @@ import pytest import uuid from datetime import (datetime, date) -import pytest from flask import current_app from app import db diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 435d7aa35..86bfa7a82 100644 --- a/tests/app/dao/test_services_dao.py +++ b/tests/app/dao/test_services_dao.py @@ -1,5 +1,10 @@ import uuid import pytest + +from sqlalchemy.orm.exc import FlushError, NoResultFound +from sqlalchemy.exc import IntegrityError + +from app import db from app.dao.services_dao import ( dao_create_service, dao_add_user_to_service, @@ -7,9 +12,9 @@ from app.dao.services_dao import ( dao_fetch_all_services, dao_fetch_service_by_id, dao_fetch_all_services_by_user, - dao_fetch_service_by_id_and_user, dao_update_service, - delete_service_and_all_associated_db_objects + delete_service_and_all_associated_db_objects, + dao_fetch_stats_for_service ) from app.dao.users_dao import save_model_user from app.models import ( @@ -21,13 +26,16 @@ from app.models import ( Template, Job, Notification, + NotificationHistory, Permission, User, InvitedUser, Service ) -from sqlalchemy.orm.exc import FlushError, NoResultFound -from sqlalchemy.exc import IntegrityError + +from tests.app.conftest import ( + sample_notification as create_notification +) def test_create_service(sample_user): @@ -212,27 +220,6 @@ def test_get_service_by_id_returns_service(service_factory): assert dao_fetch_service_by_id(service.id).name == 'testing' -def test_can_get_service_by_id_and_user(service_factory, sample_user): - service = service_factory.get('service 1', sample_user, email_from='service.1') - assert dao_fetch_service_by_id_and_user(service.id, sample_user.id).name == 'service 1' - - -def test_cannot_get_service_by_id_and_owned_by_different_user(service_factory, sample_user): - service1 = service_factory.get('service 1', sample_user, email_from='service.1') - new_user = User( - name='Test User', - email_address='new_user@digital.cabinet-office.gov.uk', - password='password', - mobile_number='+447700900986' - ) - save_model_user(new_user) - service2 = service_factory.get('service 2', new_user, email_from='service.2') - assert dao_fetch_service_by_id_and_user(service1.id, sample_user.id).name == 'service 1' - with pytest.raises(NoResultFound) as e: - dao_fetch_service_by_id_and_user(service2.id, sample_user.id) - assert 'No row was found for one()' in str(e) - - def test_create_service_creates_a_history_record_with_current_data(sample_user): assert Service.query.count() == 0 assert Service.get_history_model().query.count() == 0 @@ -384,3 +371,52 @@ def test_add_existing_user_to_another_service_doesnot_change_old_permissions(sam other_user_service_two_permissions = Permission.query.filter_by(service=service_two, user=other_user).all() assert len(other_user_service_two_permissions) == 8 + + +def test_fetch_stats_filters_on_service(sample_notification): + service_two = Service(name="service_two", + created_by=sample_notification.service.created_by, + email_from="hello", + active=False, + restricted=False, + message_limit=1000) + dao_create_service(service_two, sample_notification.service.created_by) + + stats = dao_fetch_stats_for_service(service_two.id) + assert len(stats) == 0 + + +def test_fetch_stats_ignores_historical_notification_data(sample_notification): + service_id = sample_notification.service.id + + db.session.delete(sample_notification) + + assert Notification.query.count() == 0 + assert NotificationHistory.query.count() == 1 + + stats = dao_fetch_stats_for_service(service_id) + assert len(stats) == 0 + + +def test_fetch_stats_counts_correctly(notify_db, notify_db_session, sample_template, sample_email_template): + # two created email, one failed email, and one created sms + create_notification(notify_db, notify_db_session, template=sample_email_template, status='created') + create_notification(notify_db, notify_db_session, template=sample_email_template, status='created') + create_notification(notify_db, notify_db_session, template=sample_email_template, status='technical-failure') + create_notification(notify_db, notify_db_session, template=sample_template, status='created') + + stats = dao_fetch_stats_for_service(sample_template.service.id) + stats = sorted(stats, key=lambda x: (x.notification_type, x.status)) + assert len(stats) == 3 + + assert stats[0].notification_type == 'email' + assert stats[0].status == 'created' + assert stats[0].count == 2 + + assert stats[1].notification_type == 'email' + assert stats[1].status == 'technical-failure' + assert stats[1].count == 1 + + assert stats[2].notification_type == 'sms' + assert stats[2].status == 'created' + assert stats[2].count == 1 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 5622b66df..72f99f1ca 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 @@ -14,6 +17,9 @@ from tests.app.conftest import ( ) +Row = collections.namedtuple('row', ('notification_type', 'status', 'count')) + + def test_get_service_list(notify_api, service_factory): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -1086,3 +1092,58 @@ 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, sample_notification): + 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': 1, + 'delivered': 0, + 'failed': 0 + } + + +# 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): + from app.service.rest import format_statistics + + 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 bf8a68565..0f88fe667 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -68,3 +68,12 @@ def os_environ(request): 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) + if idparametrize: + argnames, testdata = idparametrize.args + ids, argvalues = zip(*sorted(testdata.items())) + metafunc.parametrize(argnames, argvalues, ids=ids)