Merge pull request #802 from alphagov/dashboard-stats-untangling

Remove notification-statistics endpoints from totals boxes

app/__init__.py

@@ -324,7 +324,11 @@ def register_errorhandlers(application):
 
     @application.errorhandler(HTTPError)
     def render_http_error(error):
-        application.logger.error("API called failed with status {} message {}".format(error.status_code, error.message))
+        application.logger.error("API {} failed with status {} message {}".format(
+            error.response.url if error.response else 'unknown',
+            error.status_code,
+            error.message
+        ))
         error_code = error.status_code
         if error_code not in [401, 404, 403, 500]:
             error_code = 500

app/main/views/dashboard.py

@@ -1,11 +1,9 @@
 from datetime import datetime, date, timedelta
 from collections import namedtuple
 from itertools import groupby
-import json
 
 from flask import (
     render_template,
-    redirect,
     url_for,
     session,
     jsonify,
@@ -21,7 +19,7 @@ from app import (
     service_api_client,
     template_statistics_client
 )
-from app.statistics_utils import sum_of_statistics, add_rates_to, add_rate_to_jobs
+from app.statistics_utils import add_rates_to, get_formatted_percentage, add_rate_to_jobs
 from app.utils import user_has_permissions
 
 
@@ -150,13 +148,13 @@ def get_dashboard_partials(service_id):
         lambda job: job['original_file_name'] != current_app.config['TEST_MESSAGE_FILENAME'],
         job_api_client.get_job(service_id, limit_days=7)['data']
     ))
+    service = service_api_client.get_detailed_service(service_id)
 
     return {
         'totals': render_template(
             'views/dashboard/_totals.html',
-            statistics=add_rates_to(sum_of_statistics(
-                statistics_api_client.get_statistics_for_service(service_id, limit_days=7)['data']
-            ))
+            service_id=service_id,
+            statistics=get_dashboard_totals(service['data']['statistics'])
         ),
         'template-statistics': render_template(
             'views/dashboard/template-statistics.html',
@@ -178,6 +176,13 @@ def get_dashboard_partials(service_id):
     }
 
 
+def get_dashboard_totals(statistics):
+    for msg_type in statistics.values():
+        msg_type['failed_percentage'] = get_formatted_percentage(msg_type['failed'], msg_type['requested'])
+        msg_type['show_warning'] = float(msg_type['failed_percentage']) > 3
+    return statistics
+
+
 def calculate_usage(usage):
     # TODO: Don't hardcode these - get em from the API
     sms_free_allowance = 250000

app/notify_client/service_api_client.py

@@ -41,12 +41,20 @@ class ServiceAPIClient(NotificationsAPIClient):
         data = _attach_current_user({})
         return self.delete(endpoint, data)
 
-    def get_service(self, service_id, *params):
+    def get_service(self, service_id):
+        return self._get_service(service_id, False)
+
+    def get_detailed_service(self, service_id):
+        return self._get_service(service_id, True)
+
+    def _get_service(self, service_id, detailed):
         """
         Retrieve a service.
         """
+        params = {'detailed': True} if detailed else {}
         return self.get(
-            '/service/{0}'.format(service_id))
+            '/service/{0}'.format(service_id),
+            params=params)
 
     def get_services(self, *params):
         """

app/statistics_utils.py

@@ -31,18 +31,12 @@ def sum_of_statistics(delivery_statistics):
 def add_rates_to(delivery_statistics):
 
     return dict(
-        emails_failure_rate=(
-            "{0:.1f}".format(
-                float(delivery_statistics['emails_failed']) / delivery_statistics['emails_requested'] * 100
-            )
-            if delivery_statistics['emails_requested'] else 0
-        ),
-        sms_failure_rate=(
-            "{0:.1f}".format(
-                float(delivery_statistics['sms_failed']) / delivery_statistics['sms_requested'] * 100
-            )
-            if delivery_statistics['sms_requested'] else 0
-        ),
+        emails_failure_rate=get_formatted_percentage(
+            delivery_statistics['emails_failed'],
+            delivery_statistics['emails_requested']),
+        sms_failure_rate=get_formatted_percentage(
+            delivery_statistics['sms_failed'],
+            delivery_statistics['sms_requested']),
         week_end_datetime=parser.parse(
             delivery_statistics.get('week_end', str(datetime.utcnow()))
         ),
@@ -50,6 +44,13 @@ def add_rates_to(delivery_statistics):
     )
 
 
+def get_formatted_percentage(x, tot):
+    """
+    Return a percentage to one decimal place (respecting )
+    """
+    return "{0:.1f}".format((float(x) / tot * 100)) if tot else '0'
+
+
 def statistics_by_state(statistics):
     return {
         'sms': {

app/templates/views/dashboard/_totals.html

@@ -4,24 +4,24 @@
 <div class="grid-row">
   <div class="column-half">
     {{ big_number_with_status(
-      statistics.emails_requested,
-      message_count_label(statistics.emails_requested, 'email', suffix=''),
-      statistics.emails_failed,
-      statistics.get('emails_failure_rate', 0.0),
-      statistics.get('emails_failure_rate', 0)|float > 3,
-      failure_link=url_for(".view_notifications", service_id=current_service.id, message_type='email', status='failed'),
-      link=url_for(".view_notifications", service_id=current_service.id, message_type='email', status='sending,delivered,failed')
+      statistics['email']['requested'],
+      message_count_label(statistics['email']['requested'], 'email', suffix=''),
+      statistics['email']['failed'],
+      statistics['email']['failed_percentage'],
+      statistics['email']['show_warning'],
+      failure_link=url_for(".view_notifications", service_id=service_id, message_type='email', status='failed'),
+      link=url_for(".view_notifications", service_id=service_id, message_type='email', status='sending,delivered,failed')
     ) }}
   </div>
   <div class="column-half">
     {{ big_number_with_status(
-      statistics.sms_requested,
-      message_count_label(statistics.sms_requested, 'sms', suffix=''),
-      statistics.sms_failed,
-      statistics.get('sms_failure_rate', 0.0),
-      statistics.get('sms_failure_rate', 0)|float > 3,
-      failure_link=url_for(".view_notifications", service_id=current_service.id, message_type='sms', status='failed'),
-      link=url_for(".view_notifications", service_id=current_service.id, message_type='sms', status='sending,delivered,failed')
+      statistics['sms']['requested'],
+      message_count_label(statistics['sms']['requested'], 'sms', suffix=''),
+      statistics['sms']['failed'],
+      statistics['sms']['failed_percentage'],
+      statistics['sms']['show_warning'],
+      failure_link=url_for(".view_notifications", service_id=service_id, message_type='sms', status='failed'),
+      link=url_for(".view_notifications", service_id=service_id, message_type='sms', status='sending,delivered,failed')
     ) }}
   </div>
 </div>

tests/app/main/views/test_accept_invite.py

@@ -314,7 +314,10 @@ def test_new_invited_user_verifies_and_added_to_service(app_,
                                                         mock_get_service_statistics,
                                                         mock_get_template_statistics,
                                                         mock_get_jobs,
-                                                        mock_has_permissions):
+                                                        mock_has_permissions,
+                                                        mock_get_users_by_service,
+                                                        mock_get_detailed_service,
+                                                        mock_get_usage):
 
     with app_.test_request_context():
         with app_.test_client() as client:

tests/app/main/views/test_api_keys.py

@@ -6,14 +6,14 @@ from tests import validate_route_permission
 
 
 def test_should_show_empty_api_keys_page(app_,
-                                         api_user_active,
+                                         api_user_pending,
                                          mock_login,
                                          mock_get_no_api_keys,
                                          mock_get_service,
                                          mock_has_permissions):
     with app_.test_request_context():
         with app_.test_client() as client:
-            client.login(api_user_active)
+            client.login(api_user_pending)
             service_id = str(uuid.uuid4())
             response = client.get(url_for('main.api_keys', service_id=service_id))
 

tests/app/main/views/test_dashboard.py

@@ -1,9 +1,12 @@
 import copy
 from flask import url_for
 
+import pytest
 from bs4 import BeautifulSoup
 from freezegun import freeze_time
 
+from app.main.views.dashboard import get_dashboard_totals
+
 from tests import validate_route_permission
 from tests.conftest import SERVICE_ONE_ID
 
@@ -69,6 +72,7 @@ def test_get_started(
     mock_login,
     mock_get_jobs,
     mock_has_permissions,
+    mock_get_detailed_service,
     mock_get_usage
 ):
 
@@ -98,12 +102,11 @@ def test_get_started_is_hidden_once_templates_exist(
     mock_login,
     mock_get_jobs,
     mock_has_permissions,
+    mock_get_detailed_service,
     mock_get_usage
 ):
-
     mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service',
                                        return_value=copy.deepcopy(stub_template_stats))
-
     with app_.test_request_context(), app_.test_client() as client:
         client.login(api_user_active)
         response = client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID))
@@ -118,13 +121,13 @@ def test_should_show_recent_templates_on_dashboard(app_,
                                                    api_user_active,
                                                    mock_get_service,
                                                    mock_get_service_templates,
-                                                   mock_get_service_statistics,
                                                    mock_get_aggregate_service_statistics,
                                                    mock_get_user,
                                                    mock_get_user_by_email,
                                                    mock_login,
                                                    mock_get_jobs,
                                                    mock_has_permissions,
+                                                   mock_get_detailed_service,
                                                    mock_get_usage):
 
     mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service',
@@ -137,7 +140,6 @@ def test_should_show_recent_templates_on_dashboard(app_,
 
         assert response.status_code == 200
         response.get_data(as_text=True)
-        mock_get_service_statistics.assert_called_once_with(SERVICE_ONE_ID, limit_days=7)
         mock_template_stats.assert_called_once_with(SERVICE_ONE_ID, limit_days=7)
 
         page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
@@ -211,6 +213,7 @@ def test_should_show_recent_jobs_on_dashboard(
     mock_get_user_by_email,
     mock_login,
     mock_get_template_statistics,
+    mock_get_detailed_service,
     mock_get_jobs,
     mock_has_permissions,
     mock_get_usage
@@ -262,6 +265,7 @@ def test_menu_send_messages(mocker,
                             mock_get_service_templates,
                             mock_get_jobs,
                             mock_get_template_statistics,
+                            mock_get_detailed_service,
                             mock_get_usage):
 
     with app_.test_request_context():
@@ -296,6 +300,7 @@ def test_menu_manage_service(mocker,
                              mock_get_service_templates,
                              mock_get_jobs,
                              mock_get_template_statistics,
+                             mock_get_detailed_service,
                              mock_get_usage):
     with app_.test_request_context():
         resp = _test_dashboard_menu(
@@ -328,6 +333,7 @@ def test_menu_manage_api_keys(mocker,
                               mock_get_service_templates,
                               mock_get_jobs,
                               mock_get_template_statistics,
+                              mock_get_detailed_service,
                               mock_get_usage):
     with app_.test_request_context():
         resp = _test_dashboard_menu(
@@ -359,6 +365,7 @@ def test_menu_all_services_for_platform_admin_user(mocker,
                                                    mock_get_service_templates,
                                                    mock_get_jobs,
                                                    mock_get_template_statistics,
+                                                   mock_get_detailed_service,
                                                    mock_get_usage):
     with app_.test_request_context():
         resp = _test_dashboard_menu(
@@ -376,14 +383,6 @@ def test_menu_all_services_for_platform_admin_user(mocker,
         assert url_for('main.view_notifications', service_id=service_one['id'], message_type='sms') in page
         assert url_for('main.api_keys', service_id=service_one['id']) not in page
 
-        # Should this be here??
-        # template_json = mock_get_service_templates(service_one['id'])['data'][0]
-
-        # assert url_for(
-        #    'main.edit_service_template',
-        #    service_id=service_one['id'],
-        #    template_id=template_json['id']) in page
-
 
 def test_route_for_service_permissions(mocker,
                                        app_,
@@ -395,6 +394,7 @@ def test_route_for_service_permissions(mocker,
                                        mock_get_jobs,
                                        mock_get_service_statistics,
                                        mock_get_template_statistics,
+                                       mock_get_detailed_service,
                                        mock_get_usage):
     routes = [
         'main.service_dashboard']
@@ -424,3 +424,68 @@ def test_aggregate_template_stats():
             assert item['usage_count'] == 13
         elif item['template'].id == 2:
             assert item['usage_count'] == 206
+
+
+def test_service_dashboard_updates_gets_dashboard_totals(mocker,
+                                                         app_,
+                                                         active_user_with_permissions,
+                                                         service_one,
+                                                         mock_get_user,
+                                                         mock_get_service_templates,
+                                                         mock_get_template_statistics,
+                                                         mock_get_detailed_service,
+                                                         mock_get_jobs,
+                                                         mock_get_service_statistics,
+                                                         mock_get_usage):
+    dashboard_totals = mocker.patch('app.main.views.dashboard.get_dashboard_totals', return_value={
+        'email': {'requested': 123, 'delivered': 0, 'failed': 0},
+        'sms': {'requested': 456, 'delivered': 0, 'failed': 0}
+    })
+
+    with app_.test_request_context(), app_.test_client() as client:
+        client.login(active_user_with_permissions, mocker, service_one)
+        response = client.get(url_for('main.service_dashboard', service_id=SERVICE_ONE_ID))
+
+    assert response.status_code == 200
+
+    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+    numbers = [number.text.strip() for number in page.find_all('div', class_='big-number-number')]
+    assert '123' in numbers
+    assert '456' in numbers
+
+    table_rows = page.find_all('tbody')[0].find_all('tr')
+
+
+def test_get_dashboard_totals_adds_percentages():
+    stats = {
+        'sms': {
+            'requested': 3,
+            'delivered': 0,
+            'failed': 2
+        },
+        'email': {
+            'requested': 0,
+            'delivered': 0,
+            'failed': 0
+        }
+    }
+    assert get_dashboard_totals(stats)['sms']['failed_percentage'] == '66.7'
+    assert get_dashboard_totals(stats)['email']['failed_percentage'] == '0'
+
+
+@pytest.mark.parametrize(
+    'failures,expected', [
+        (2, False),
+        (3, False),
+        (4, True)
+    ]
+)
+def test_get_dashboard_totals_adds_warning(failures, expected):
+    stats = {
+        'sms': {
+            'requested': 100,
+            'delivered': 0,
+            'failed': failures
+        }
+    }
+    assert get_dashboard_totals(stats)['sms']['show_warning'] == expected

tests/app/main/views/test_sign_out.py

@@ -21,6 +21,7 @@ def test_sign_out_user(app_,
                        mock_get_jobs,
                        mock_has_permissions,
                        mock_get_template_statistics,
+                       mock_get_detailed_service,
                        mock_get_usage):
     with app_.test_request_context():
         with app_.test_client() as client:

tests/app/notify_client/test_service_api_client.py

@@ -19,3 +19,19 @@ def test_client_posts_archived_true_when_deleting_template(mocker):
 
     client.delete_service_template(service_id, template_id)
     mock_post.assert_called_once_with(expected_url, data=expected_data)
+
+
+def test_client_gets_service_with_no_params(mocker):
+    client = ServiceAPIClient()
+    mock_post = mocker.patch('app.notify_client.service_api_client.ServiceAPIClient.get')
+
+    client.get_service('foo')
+    mock_post.assert_called_once_with('/service/foo', params={})
+
+
+def test_client_gets_service_with_detailed_params(mocker):
+    client = ServiceAPIClient()
+    mock_post = mocker.patch('app.notify_client.service_api_client.ServiceAPIClient.get')
+
+    client.get_detailed_service('foo')
+    mock_post.assert_called_once_with('/service/foo', params={'detailed': True})

tests/app/test_statistics_utils.py

@@ -53,7 +53,7 @@ def test_sum_of_statistics_sums_inputs():
 
 
 @pytest.mark.parametrize('emails_failed,emails_requested,expected_failure_rate', [
-    (0, 0, 0),
+    (0, 0, '0'),
     (0, 1, '0.0'),
     (1, 3, '33.3')
 ])
@@ -69,7 +69,7 @@ def test_add_rates_sets_email_failure_rate(emails_failed, emails_requested, expe
 
 
 @pytest.mark.parametrize('sms_failed,sms_requested,expected_failure_rate', [
-    (0, 0, 0),
+    (0, 0, '0'),
     (0, 1, '0.0'),
     (1, 3, '33.3')
 ])

tests/conftest.py

@@ -64,6 +64,22 @@ def mock_get_service(mocker, api_user_active):
     return mocker.patch('app.service_api_client.get_service', side_effect=_get)
 
 
+@pytest.fixture(scope='function')
+def mock_get_detailed_service(mocker, api_user_active):
+    def _get(service_id):
+        return {
+            'data': {
+                'id': service_id,
+                'statistics': {
+                    'email': {'requested': 0, 'delivered': 0, 'failed': 0},
+                    'sms': {'requested': 0, 'delivered': 0, 'failed': 0}
+                }
+            }
+        }
+
+    return mocker.patch('app.service_api_client.get_detailed_service', side_effect=_get)
+
+
 @pytest.fixture(scope='function')
 def mock_get_live_service(mocker, api_user_active):
     def _get(service_id):