diff --git a/app/__init__.py b/app/__init__.py index d0e360b0d..9932ac77f 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -39,7 +39,6 @@ from app.notify_client.api_key_api_client import ApiKeyApiClient from app.notify_client.invite_api_client import InviteApiClient from app.notify_client.job_api_client import JobApiClient from app.notify_client.notification_api_client import NotificationApiClient -from app.notify_client.statistics_api_client import StatisticsApiClient from app.notify_client.status_api_client import StatusApiClient from app.notify_client.template_statistics_api_client import TemplateStatisticsApiClient from app.notify_client.user_api_client import UserApiClient @@ -57,7 +56,6 @@ job_api_client = JobApiClient() notification_api_client = NotificationApiClient() status_api_client = StatusApiClient() invite_api_client = InviteApiClient() -statistics_api_client = StatisticsApiClient() template_statistics_client = TemplateStatisticsApiClient() events_api_client = EventsApiClient() provider_client = ProviderClient() @@ -86,7 +84,6 @@ def create_app(): notification_api_client.init_app(application) status_api_client.init_app(application) invite_api_client.init_app(application) - statistics_api_client.init_app(application) template_statistics_client.init_app(application) events_api_client.init_app(application) provider_client.init_app(application) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index 39745d996..ca753782a 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -1,50 +1,66 @@ +import itertools from datetime import datetime import pytz from flask import render_template from flask_login import login_required -from app import statistics_api_client, service_api_client +from app import service_api_client from app.main import main from app.utils import user_has_permissions -from app.statistics_utils import sum_of_statistics, add_rates_to +from app.statistics_utils import get_formatted_percentage @main.route("/platform-admin") @login_required @user_has_permissions(admin_override=True) def platform_admin(): + services = service_api_client.get_services({'detailed': True})['data'] return render_template( 'views/platform-admin.html', - **get_statistics() + **get_statistics(services) ) -def get_statistics(): - day = datetime.now(tz=pytz.timezone('Europe/London')).date() - all_stats = statistics_api_client.get_statistics_for_all_services_for_day(day)['data'] - services = service_api_client.get_services()['data'] - service_stats = format_stats_by_service(all_stats, services) +def get_statistics(services): return { - 'global_stats': add_rates_to(sum_of_statistics(all_stats)), - 'service_stats': service_stats + 'global_stats': create_global_stats(services), + 'service_stats': format_stats_by_service(services) } -def format_stats_by_service(all_stats, services): - services = {service['id']: service for service in services} - return [ - { - 'id': stats['service'], - 'name': services[stats['service']]['name'], - 'sending': ( - (stats['sms_requested'] - stats['sms_delivered'] - stats['sms_failed']) + - (stats['emails_requested'] - stats['emails_delivered'] - stats['emails_failed']) - ), - 'delivered': stats['sms_delivered'] + stats['emails_delivered'], - 'failed': stats['sms_failed'] + stats['emails_failed'], - 'restricted': services[stats['service']]['restricted'], - 'research_mode': services[stats['service']]['research_mode'] +def create_global_stats(services): + stats = { + 'email': { + 'delivered': 0, + 'failed': 0, + 'requested': 0 + }, + 'sms': { + 'delivered': 0, + 'failed': 0, + 'requested': 0 + } + } + for service in services: + for msg_type, status in itertools.product(('sms', 'email'), ('delivered', 'failed', 'requested')): + stats[msg_type][status] += service['statistics'][msg_type][status] + + for stat in stats.values(): + stat['failure_rate'] = get_formatted_percentage(stat['failed'], stat['requested']) + + return stats + + +def format_stats_by_service(services): + for service in services: + stats = service['statistics'].values() + yield { + 'id': service['id'], + 'name': service['name'], + 'sending': sum((stat['requested'] - stat['delivered'] - stat['failed']) for stat in stats), + 'delivered': sum(stat['delivered'] for stat in stats), + 'failed': sum(stat['failed'] for stat in stats), + 'restricted': service['restricted'], + 'research_mode': service['research_mode'] } - for stats in all_stats - ] diff --git a/app/notify_client/statistics_api_client.py b/app/notify_client/statistics_api_client.py deleted file mode 100644 index 873729f5d..000000000 --- a/app/notify_client/statistics_api_client.py +++ /dev/null @@ -1,19 +0,0 @@ -from notifications_python_client.base import BaseAPIClient - - -class StatisticsApiClient(BaseAPIClient): - def __init__(self, base_url=None, client_id=None, secret=None): - super(self.__class__, self).__init__(base_url=base_url or 'base_url', - client_id=client_id or 'client_id', - secret=secret or 'secret') - - def init_app(self, app): - self.base_url = app.config['API_HOST_NAME'] - self.client_id = app.config['ADMIN_CLIENT_USER_NAME'] - self.secret = app.config['ADMIN_CLIENT_SECRET'] - - def get_statistics_for_all_services_for_day(self, day): - params = { - 'day': day - } - return self.get(url='/notifications/statistics', params=params) diff --git a/app/templates/views/platform-admin.html b/app/templates/views/platform-admin.html index 05e66bcc4..f8c93e534 100644 --- a/app/templates/views/platform-admin.html +++ b/app/templates/views/platform-admin.html @@ -26,20 +26,20 @@
{{ big_number_with_status( - global_stats.emails_delivered, - message_count_label(global_stats.emails_delivered, 'email'), - global_stats.emails_failed, - global_stats.emails_failure_rate, - global_stats.emails_failure_rate|float > 3, + global_stats.email.delivered, + message_count_label(global_stats.email.delivered, 'email'), + global_stats.email.failed, + global_stats.email.failure_rate, + global_stats.email.failure_rate|float > 3, ) }}
{{ big_number_with_status( - global_stats.sms_delivered, - message_count_label(global_stats.sms_delivered, 'sms'), - global_stats.sms_failed, - global_stats.sms_failure_rate, - global_stats.sms_failure_rate|float > 3, + global_stats.sms.delivered, + message_count_label(global_stats.sms.delivered, 'sms'), + global_stats.sms.failed, + global_stats.sms.failure_rate, + global_stats.sms.failure_rate|float > 3, ) }}
diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index fcdfb142c..a6b92979e 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -2,10 +2,13 @@ from datetime import date from flask import url_for from freezegun import freeze_time +import pytest +from bs4 import BeautifulSoup from tests.conftest import mock_get_user +from tests import service_json -from app.main.views.platform_admin import get_statistics, format_stats_by_service +from app.main.views.platform_admin import get_statistics, format_stats_by_service, create_global_stats def test_should_redirect_if_not_logged_in(app_): @@ -27,12 +30,44 @@ def test_should_403_if_not_platform_admin(app_, active_user_with_permissions, mo assert response.status_code == 403 +@pytest.mark.parametrize('restricted, research_mode, displayed', [ + (True, False, ''), + (False, False, 'Live'), + (False, True, 'research mode'), + (True, True, 'research mode') +]) +def test_should_show_research_and_restricted_mode( + restricted, + research_mode, + displayed, + app_, + platform_admin_user, + mocker, + mock_get_detailed_services, + fake_uuid +): + services = [service_json(fake_uuid, 'My Service', [], restricted=restricted, research_mode=research_mode)] + services[0]['statistics'] = create_stats() + + mock_get_detailed_services.return_value = {'data': services} + with app_.test_request_context(): + with app_.test_client() as client: + mock_get_user(mocker, user=platform_admin_user) + client.login(platform_admin_user) + response = client.get(url_for('main.platform_admin')) + + assert response.status_code == 200 + mock_get_detailed_services.assert_called_once_with({'detailed': True}) + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + # get second column, which contains flags as text. + assert page.tbody.select('td:nth-of-type(2)')[0].text.strip() == displayed + + def test_should_render_platform_admin_page( app_, platform_admin_user, mocker, - mock_get_services, - mock_get_all_service_statistics + mock_get_detailed_services, ): with app_.test_request_context(): with app_.test_client() as client: @@ -40,33 +75,49 @@ def test_should_render_platform_admin_page( client.login(platform_admin_user) response = client.get(url_for('main.platform_admin')) - assert response.status_code == 200 - resp_data = response.get_data(as_text=True) - assert 'Platform admin' in resp_data - assert 'Today' in resp_data - assert 'Services' in resp_data + assert response.status_code == 200 + resp_data = response.get_data(as_text=True) + assert 'Platform admin' in resp_data + assert 'Today' in resp_data + assert 'Services' in resp_data + mock_get_detailed_services.assert_called_once_with({'detailed': True}) -def test_get_statistics_should_summarise_all_stats(mock_get_all_service_statistics, mock_get_services): - resp = get_statistics()['global_stats'] +def test_create_global_stats_sets_failure_rates(fake_uuid): + services = [ + service_json(fake_uuid, 'a', []), + service_json(fake_uuid, 'b', []) + ] + services[0]['statistics'] = create_stats( + emails_requested=1, + emails_delivered=1, + emails_failed=0, + ) + services[1]['statistics'] = create_stats( + emails_requested=2, + emails_delivered=1, + emails_failed=1, + ) - assert 'emails_delivered' in resp - assert 'emails_failed' in resp - assert 'emails_failure_rate' in resp - assert 'sms_delivered' in resp - assert 'sms_failed' in resp - assert 'sms_failure_rate' in resp + stats = create_global_stats(services) - -@freeze_time('2000-06-30T23:30:00', tz_offset=0) -def test_get_statistics_should_query_for_today_forced_to_GMT(mock_get_all_service_statistics, mock_get_services): - get_statistics() - - mock_get_all_service_statistics.assert_called_once_with(date(2000, 7, 1)) + assert stats == { + 'email': { + 'delivered': 2, + 'failed': 1, + 'requested': 3, + 'failure_rate': '33.3' + }, + 'sms': { + 'delivered': 0, + 'failed': 0, + 'requested': 0, + 'failure_rate': '0' + } + } def create_stats( - service, emails_requested=0, emails_delivered=0, emails_failed=0, @@ -75,59 +126,31 @@ def create_stats( sms_failed=0 ): return { - 'service': service, - 'emails_requested': emails_requested, - 'emails_delivered': emails_delivered, - 'emails_failed': emails_failed, - 'sms_requested': sms_requested, - 'sms_delivered': sms_delivered, - 'sms_failed': sms_failed, + 'sms': { + 'requested': sms_requested, + 'delivered': sms_delivered, + 'failed': sms_failed, + }, + 'email': { + 'requested': emails_requested, + 'delivered': emails_delivered, + 'failed': emails_failed, + } } -def test_format_stats_by_service_gets_correct_stats_for_each_service(): - services = [ - {'name': 'a', 'id': 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa', 'restricted': False, 'research_mode': True}, - {'name': 'b', 'id': 'bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb', 'restricted': True, 'research_mode': False} - ] - all_stats = [ - create_stats('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa', emails_requested=1), - create_stats('bbbbbbbb-bbbb-bbbb-bbbb-bbbbbbbbbbbb', emails_requested=2) - ] +def test_format_stats_by_service_sums_values_for_sending(fake_uuid): + services = [service_json(fake_uuid, 'a', [])] + services[0]['statistics'] = create_stats( + emails_requested=10, + emails_delivered=3, + emails_failed=5, + sms_requested=50, + sms_delivered=7, + sms_failed=11 + ) - ret = format_stats_by_service(all_stats, services) - - assert len(ret) == 2 - assert ret[0]['name'] == 'a' - assert ret[0]['sending'] == 1 - assert ret[0]['delivered'] == 0 - assert ret[0]['failed'] == 0 - assert ret[0]['restricted'] is False - - assert ret[1]['name'] == 'b' - assert ret[1]['sending'] == 2 - assert ret[1]['delivered'] == 0 - assert ret[1]['failed'] == 0 - assert ret[1]['restricted'] is True - - -def test_format_stats_by_service_sums_values_for_sending(): - services = [ - {'name': 'a', 'id': 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa', 'restricted': False, 'research_mode': False}, - ] - all_stats = [ - create_stats( - 'aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa', - emails_requested=10, - emails_delivered=3, - emails_failed=5, - sms_requested=50, - sms_delivered=7, - sms_failed=11 - ) - ] - - ret = format_stats_by_service(all_stats, services) + ret = list(format_stats_by_service(services)) assert len(ret) == 1 assert ret[0]['sending'] == 34 diff --git a/tests/conftest.py b/tests/conftest.py index 12b9e95b5..6f92be0a9 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -96,6 +96,18 @@ def mock_get_detailed_service_for_today(mocker, api_user_active): return mocker.patch('app.service_api_client.get_detailed_service_for_today', side_effect=_get) +@pytest.fixture(scope='function') +def mock_get_detailed_services(mocker, fake_uuid): + service_one = service_json(SERVICE_ONE_ID, "service_one", [fake_uuid], 1000, True, False) + service_one['statistics'] = { + 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, + 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} + } + services = {'data': [service_one]} + + return mocker.patch('app.service_api_client.get_services', return_value=services) + + @pytest.fixture(scope='function') def mock_get_live_service(mocker, api_user_active): def _get(service_id): @@ -215,15 +227,6 @@ def mock_delete_service(mocker, mock_get_service): 'app.service_api_client.delete_service', side_effect=_delete) -@pytest.fixture(scope='function') -def mock_get_all_service_statistics(mocker): - def _create(day): - return {'data': []} - - return mocker.patch( - 'app.statistics_api_client.get_statistics_for_all_services_for_day', side_effect=_create) - - @pytest.fixture(scope='function') def mock_get_service_template(mocker): def _get(service_id, template_id, version=None):