Merge pull request #907 from alphagov/plat-admin

Platform admin stats rework

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)

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
-    ]

app/notify_client/statistics_api_client.py

@@ -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)

app/templates/views/platform-admin.html

@@ -26,20 +26,20 @@
   <div class="grid-row bottom-gutter">
     <div class="column-half">
       {{ 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,
       ) }}
     </div>
     <div class="column-half">
       {{ 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,
       ) }}
     </div>
   </div>

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

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):