From 4451a8634d291c841d367043d65f41b657c0486d Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 19 Jul 2016 13:53:27 +0100 Subject: [PATCH 1/6] add detailed flag to GET service api client returns current (past 7 days) notification stats as well as service info --- app/__init__.py | 6 +++++- app/main/views/dashboard.py | 10 +++++++--- app/notify_client/service_api_client.py | 6 ++++-- tests/app/main/views/test_dashboard.py | 26 ++++++++++++++++--------- tests/conftest.py | 7 ++++++- 5 files changed, 39 insertions(+), 16 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 40eeb2fe9..eafe21ec8 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, + 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..68bac3f3b 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -154,9 +154,7 @@ def get_dashboard_partials(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'] - )) + statistics=get_dashboard_totals(service_id) ), 'template-statistics': render_template( 'views/dashboard/template-statistics.html', @@ -178,6 +176,12 @@ def get_dashboard_partials(service_id): } +def get_dashboard_totals(service_id): + return add_rates_to(sum_of_statistics( + statistics_api_client.get_statistics_for_service(service_id, limit_days=7)['data'] + )) + + 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..ad31d21d4 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -41,12 +41,14 @@ 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, detailed=False): """ 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/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 6f4ea4738..df7f35f32 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -100,7 +100,6 @@ def test_get_started_is_hidden_once_templates_exist( mock_has_permissions, mock_get_usage ): - mock_template_stats = mocker.patch('app.template_statistics_client.get_template_statistics_for_service', return_value=copy.deepcopy(stub_template_stats)) @@ -376,14 +375,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_, @@ -424,3 +415,20 @@ 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_detailed_service(mocker, + app_, + active_user_with_permissions, + service_one, + mock_get_service, + mock_get_service_statistics, + mock_get_usage, + ): + 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 + # mock_get_service_statistics.assert_not_called() + # mock_get_service.assert_called_once_with(service_one.id, detailed=True) diff --git a/tests/conftest.py b/tests/conftest.py index bc7432c8c..b33844ba4 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -55,10 +55,15 @@ def fake_uuid(): @pytest.fixture(scope='function') def mock_get_service(mocker, api_user_active): - def _get(service_id): + def _get(service_id, detailed=False): service = service_json( service_id, "Test Service", [api_user_active.id], message_limit=1000, active=False, restricted=True) + if detailed: + service['statistics'] = { + 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, + 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} + } return {'data': service} return mocker.patch('app.service_api_client.get_service', side_effect=_get) From 3e6eedd07916df72e71e131abb4263ec96db84bf Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 19 Jul 2016 17:10:48 +0100 Subject: [PATCH 2/6] use new detailed service endpoint for dashboard totals we don't want to use the old statistics endpoints any more also a couple of quality of life changes * moves some logic out of the _totals.html template * tidies up statistics_utils --- app/main/views/dashboard.py | 18 +++++++------- app/statistics_utils.py | 25 +++++++++---------- app/templates/views/dashboard/_totals.html | 28 +++++++++++----------- tests/__init__.py | 10 ++++---- tests/app/main/views/test_dashboard.py | 13 +++++----- 5 files changed, 49 insertions(+), 45 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 68bac3f3b..0f368e360 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 @@ -151,10 +149,13 @@ def get_dashboard_partials(service_id): job_api_client.get_job(service_id, limit_days=7)['data'] )) + service = service_api_client.get_service(service_id, detailed=True) + return { 'totals': render_template( 'views/dashboard/_totals.html', - statistics=get_dashboard_totals(service_id) + service_id=service_id, + statistics=get_dashboard_totals(service) ), 'template-statistics': render_template( 'views/dashboard/template-statistics.html', @@ -176,10 +177,11 @@ def get_dashboard_partials(service_id): } -def get_dashboard_totals(service_id): - return add_rates_to(sum_of_statistics( - statistics_api_client.get_statistics_for_service(service_id, limit_days=7)['data'] - )) +def get_dashboard_totals(service): + for msg_type in service['statistics']: + msg_type['failed_percentage'] = get_formatted_percentage(msg_type['failed'], msg_type['requested']) + msg_type['show_warning'] = msg_type['failed_percentage'] > 3 + return service['statistics'] def calculate_usage(usage): diff --git a/app/statistics_utils.py b/app/statistics_utils.py index cba2484af..122407a67 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 - ), + email_failure_rate=get_formatted_percentage( + delivery_statistics['email_failed'], + delivery_statistics['email_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..ce5df5482 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=id, message_type='email', status='failed'), + link=url_for(".view_notifications", service_id=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/__init__.py b/tests/__init__.py index 7841a02e7..ac64b3848 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -13,11 +13,11 @@ class TestClient(FlaskClient): session['user_id'] = user.id session['_fresh'] = True if mocker: - mocker.patch('app.user_api_client.get_user', return_value=user) - mocker.patch('app.events_api_client.create_event') - if mocker and service: - session['service_id'] = service['id'] - mocker.patch('app.service_api_client.get_service', return_value={'data': service}) + get_user_mock = mocker.patch('app.user_api_client.get_user', return_value=user) + create_event_mock = mocker.patch('app.events_api_client.create_event') + if service: + session['service_id'] = service['id'] + get_service_mock = mocker.patch('app.service_api_client.get_service', return_value={'data': service}) login_user(user, remember=True) def login_fresh(self): diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index df7f35f32..661ee7ee6 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -417,18 +417,19 @@ def test_aggregate_template_stats(): assert item['usage_count'] == 206 -def test_service_dashboard_updates_gets_detailed_service(mocker, +def test_service_dashboard_updates_gets_dashboard_totals(mocker, app_, active_user_with_permissions, service_one, - mock_get_service, mock_get_service_statistics, - mock_get_usage, - ): + mock_get_usage): + dashboard_totals = mocker.patch('app.main.views.dashboard.get_dashboard_totals', return_value={ + 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, + 'sms': {'requested': 0, '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 - # mock_get_service_statistics.assert_not_called() - # mock_get_service.assert_called_once_with(service_one.id, detailed=True) From 3ffd6c744c7058dfe6cfa84ae5b1a8d878a6d4fa Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Jul 2016 11:46:29 +0100 Subject: [PATCH 3/6] separate detailed and normal service_api_client.get_service to make it easier to mock and control return values --- app/main/views/dashboard.py | 9 ++++----- app/notify_client/service_api_client.py | 8 +++++++- app/statistics_utils.py | 6 +++--- app/templates/views/dashboard/_totals.html | 4 ++-- tests/__init__.py | 10 +++++----- tests/app/main/views/test_api_keys.py | 4 ++-- tests/app/main/views/test_dashboard.py | 7 ++++++- tests/conftest.py | 22 ++++++++++++++++------ 8 files changed, 45 insertions(+), 25 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 0f368e360..fcb2a0fdf 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -148,8 +148,7 @@ 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_service(service_id, detailed=True) + service = service_api_client.get_detailed_service(service_id) return { 'totals': render_template( @@ -178,10 +177,10 @@ def get_dashboard_partials(service_id): def get_dashboard_totals(service): - for msg_type in service['statistics']: + for msg_type in service['data']['statistics'].values(): msg_type['failed_percentage'] = get_formatted_percentage(msg_type['failed'], msg_type['requested']) - msg_type['show_warning'] = msg_type['failed_percentage'] > 3 - return service['statistics'] + msg_type['show_warning'] = float(msg_type['failed_percentage']) > 3 + return service['data']['statistics'] def calculate_usage(usage): diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index ad31d21d4..f2896a49e 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -41,7 +41,13 @@ class ServiceAPIClient(NotificationsAPIClient): data = _attach_current_user({}) return self.delete(endpoint, data) - def get_service(self, service_id, detailed=False): + 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. """ diff --git a/app/statistics_utils.py b/app/statistics_utils.py index 122407a67..dc3a803a0 100644 --- a/app/statistics_utils.py +++ b/app/statistics_utils.py @@ -31,9 +31,9 @@ def sum_of_statistics(delivery_statistics): def add_rates_to(delivery_statistics): return dict( - email_failure_rate=get_formatted_percentage( - delivery_statistics['email_failed'], - delivery_statistics['email_requested']), + 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']), diff --git a/app/templates/views/dashboard/_totals.html b/app/templates/views/dashboard/_totals.html index ce5df5482..a5dcc2f51 100644 --- a/app/templates/views/dashboard/_totals.html +++ b/app/templates/views/dashboard/_totals.html @@ -9,8 +9,8 @@ statistics['email']['failed'], statistics['email']['failed_percentage'], statistics['email']['show_warning'], - failure_link=url_for(".view_notifications", service_id=id, message_type='email', status='failed'), - link=url_for(".view_notifications", service_id=id, message_type='email', status='sending,delivered,failed') + 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') ) }}
diff --git a/tests/__init__.py b/tests/__init__.py index ac64b3848..7841a02e7 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -13,11 +13,11 @@ class TestClient(FlaskClient): session['user_id'] = user.id session['_fresh'] = True if mocker: - get_user_mock = mocker.patch('app.user_api_client.get_user', return_value=user) - create_event_mock = mocker.patch('app.events_api_client.create_event') - if service: - session['service_id'] = service['id'] - get_service_mock = mocker.patch('app.service_api_client.get_service', return_value={'data': service}) + mocker.patch('app.user_api_client.get_user', return_value=user) + mocker.patch('app.events_api_client.create_event') + if mocker and service: + session['service_id'] = service['id'] + mocker.patch('app.service_api_client.get_service', return_value={'data': service}) login_user(user, remember=True) def login_fresh(self): 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 661ee7ee6..ce4dca2b3 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -421,8 +421,13 @@ 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_jobs, mock_get_service_statistics, - mock_get_usage): + mock_get_usage, + mock_get_detailed_service): dashboard_totals = mocker.patch('app.main.views.dashboard.get_dashboard_totals', return_value={ 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} diff --git a/tests/conftest.py b/tests/conftest.py index b33844ba4..106924dfd 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -55,20 +55,30 @@ def fake_uuid(): @pytest.fixture(scope='function') def mock_get_service(mocker, api_user_active): - def _get(service_id, detailed=False): + def _get(service_id): service = service_json( service_id, "Test Service", [api_user_active.id], message_limit=1000, active=False, restricted=True) - if detailed: - service['statistics'] = { - 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, - 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} - } return {'data': service} 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): + service = service_json( + service_id, "Test Service", [api_user_active.id], message_limit=1000, + active=False, restricted=True) + service['statistics'] = { + 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, + 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} + } + return {'data': service} + + 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): From fade656e3be4d634dddeaed9f8242167ec50b06f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Jul 2016 13:55:08 +0100 Subject: [PATCH 4/6] add new get_detailed_service mock to tests that the dashboard --- tests/app/main/views/test_accept_invite.py | 5 ++++- tests/app/main/views/test_dashboard.py | 16 +++++++++++----- tests/app/main/views/test_sign_out.py | 1 + tests/conftest.py | 15 ++++++++------- 4 files changed, 24 insertions(+), 13 deletions(-) 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_dashboard.py b/tests/app/main/views/test_dashboard.py index ce4dca2b3..b9a23afda 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -69,6 +69,7 @@ def test_get_started( mock_login, mock_get_jobs, mock_has_permissions, + mock_get_detailed_service, mock_get_usage ): @@ -98,11 +99,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)) @@ -117,13 +118,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', @@ -136,7 +137,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') @@ -210,6 +210,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 @@ -261,6 +262,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(): @@ -295,6 +297,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( @@ -327,6 +330,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( @@ -358,6 +362,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( @@ -386,6 +391,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,10 +430,10 @@ def test_service_dashboard_updates_gets_dashboard_totals(mocker, 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, - mock_get_detailed_service): + mock_get_usage): dashboard_totals = mocker.patch('app.main.views.dashboard.get_dashboard_totals', return_value={ 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} 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/conftest.py b/tests/conftest.py index 106924dfd..4e373241b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -67,14 +67,15 @@ def mock_get_service(mocker, api_user_active): @pytest.fixture(scope='function') def mock_get_detailed_service(mocker, api_user_active): def _get(service_id): - service = service_json( - service_id, "Test Service", [api_user_active.id], message_limit=1000, - active=False, restricted=True) - service['statistics'] = { - 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, - 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} + return { + 'data': { + 'id': service_id, + 'statistics': { + 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, + 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} + } + } } - return {'data': service} return mocker.patch('app.service_api_client.get_detailed_service', side_effect=_get) From 0cc2a90a1a864cccf38de32ef766b5512f1a58e4 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Jul 2016 14:12:03 +0100 Subject: [PATCH 5/6] add tests for get_dashboard_totals --- app/main/views/dashboard.py | 8 ++--- app/statistics_utils.py | 2 +- tests/app/main/views/test_dashboard.py | 49 ++++++++++++++++++++++++-- tests/app/test_statistics_utils.py | 4 +-- 4 files changed, 54 insertions(+), 9 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index fcb2a0fdf..74072aa2b 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -154,7 +154,7 @@ def get_dashboard_partials(service_id): 'totals': render_template( 'views/dashboard/_totals.html', service_id=service_id, - statistics=get_dashboard_totals(service) + statistics=get_dashboard_totals(service['data']['statistics']) ), 'template-statistics': render_template( 'views/dashboard/template-statistics.html', @@ -176,11 +176,11 @@ def get_dashboard_partials(service_id): } -def get_dashboard_totals(service): - for msg_type in service['data']['statistics'].values(): +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 service['data']['statistics'] + return statistics def calculate_usage(usage): diff --git a/app/statistics_utils.py b/app/statistics_utils.py index dc3a803a0..4581e069d 100644 --- a/app/statistics_utils.py +++ b/app/statistics_utils.py @@ -48,7 +48,7 @@ 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 + return "{0:.1f}".format((float(x) / tot * 100)) if tot else '0' def statistics_by_state(statistics): diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index b9a23afda..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 @@ -435,8 +438,8 @@ def test_service_dashboard_updates_gets_dashboard_totals(mocker, mock_get_service_statistics, mock_get_usage): dashboard_totals = mocker.patch('app.main.views.dashboard.get_dashboard_totals', return_value={ - 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, - 'sms': {'requested': 0, 'delivered': 0, 'failed': 0} + 'email': {'requested': 123, 'delivered': 0, 'failed': 0}, + 'sms': {'requested': 456, 'delivered': 0, 'failed': 0} }) with app_.test_request_context(), app_.test_client() as client: @@ -444,3 +447,45 @@ def test_service_dashboard_updates_gets_dashboard_totals(mocker, 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/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') ]) From 6ff77c995e1aaa07e405a1f8809a9b23ad9709dc Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Thu, 21 Jul 2016 17:32:28 +0100 Subject: [PATCH 6/6] add tests for service_api_client --- app/__init__.py | 2 +- .../app/notify_client/test_service_api_client.py | 16 ++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/app/__init__.py b/app/__init__.py index eafe21ec8..71bccc12a 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -325,7 +325,7 @@ def register_errorhandlers(application): @application.errorhandler(HTTPError) def render_http_error(error): application.logger.error("API {} failed with status {} message {}".format( - error.response.url, + error.response.url if error.response else 'unknown', error.status_code, error.message )) 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})