diff --git a/app/__init__.py b/app/__init__.py index 40eeb2fe9..71bccc12a 100644 --- a/app/__init__.py +++ b/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 diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 126b174bb..74072aa2b 100644 --- a/app/main/views/dashboard.py +++ b/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 diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index a4360098f..f2896a49e 100644 --- a/app/notify_client/service_api_client.py +++ b/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): """ diff --git a/app/statistics_utils.py b/app/statistics_utils.py index cba2484af..4581e069d 100644 --- a/app/statistics_utils.py +++ b/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': { diff --git a/app/templates/views/dashboard/_totals.html b/app/templates/views/dashboard/_totals.html index 213f73c93..a5dcc2f51 100644 --- a/app/templates/views/dashboard/_totals.html +++ b/app/templates/views/dashboard/_totals.html @@ -4,24 +4,24 @@
{{ 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') ) }}
{{ 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') ) }}
diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index 9544c9cbe..f707e310b 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/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: diff --git a/tests/app/main/views/test_api_keys.py b/tests/app/main/views/test_api_keys.py index 1b2561566..543929478 100644 --- a/tests/app/main/views/test_api_keys.py +++ b/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)) diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 6f4ea4738..f93f70d7c 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/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 diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index 11dd86a96..6cfe07a38 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/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: diff --git a/tests/app/notify_client/test_service_api_client.py b/tests/app/notify_client/test_service_api_client.py index c3ef4aed7..9517ccfef 100644 --- a/tests/app/notify_client/test_service_api_client.py +++ b/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}) diff --git a/tests/app/test_statistics_utils.py b/tests/app/test_statistics_utils.py index 1ea0d6542..cb6b27db8 100644 --- a/tests/app/test_statistics_utils.py +++ b/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') ]) diff --git a/tests/conftest.py b/tests/conftest.py index bc7432c8c..4e373241b 100644 --- a/tests/conftest.py +++ b/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):