From 7f8e935b44d3beeb5ca610a2d96b0e96aaf7c2f8 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 23 Oct 2017 15:08:08 +0100 Subject: [PATCH 1/2] Improve the performance of /platform-admin Using a new endpoint in API to get the aggregate platform admin stats. Relies on https://github.com/alphagov/notifications-api/pull/1332 --- app/main/views/platform_admin.py | 37 ++++++- app/notify_client/service_api_client.py | 3 + tests/app/main/views/test_platform_admin.py | 115 +++++++++++++++----- tests/conftest.py | 11 ++ 4 files changed, 136 insertions(+), 30 deletions(-) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index f372a4f8d..f50f7fa81 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -28,13 +28,16 @@ def platform_admin(): api_args['start_date'] = form.start_date.data api_args['end_date'] = form.end_date.data or datetime.utcnow().date() - services = service_api_client.get_services(api_args)['data'] + platform_stats = service_api_client.get_aggregate_platform_stats(api_args) + + for stat in platform_stats.values(): + stat['failure_rate'] = get_formatted_percentage(stat['failed'], stat['requested']) return render_template( 'views/platform-admin/index.html', include_from_test_key=form.include_from_test_key.data, form=form, - global_stats=create_global_stats(services), + global_stats=platform_stats, ) @@ -120,6 +123,36 @@ def create_global_stats(services): return stats +def platform_failure_percentages(services): + stats = { + 'email': { + 'delivered': 0, + 'failed': 0, + 'requested': 0 + }, + 'sms': { + 'delivered': 0, + 'failed': 0, + 'requested': 0 + }, + 'letter': { + 'delivered': 0, + 'failed': 0, + 'requested': 0 + } + } + + for service in services: + for msg_type, status in itertools.product(('sms', 'email', 'letter'), ('delivered', 'failed', 'requested')): + import pdb + pdb.set_trace() + stats[msg_type][status] += service[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: yield { diff --git a/app/notify_client/service_api_client.py b/app/notify_client/service_api_client.py index d8c034eaa..3908a457c 100644 --- a/app/notify_client/service_api_client.py +++ b/app/notify_client/service_api_client.py @@ -353,6 +353,9 @@ class ServiceAPIClient(NotifyAdminAPIClient): } ) + def get_aggregate_platform_stats(self, params_dict=None): + return self.get("/service/platform-stats", params=params_dict) + class ServicesBrowsableItem(BrowsableItem): @property diff --git a/tests/app/main/views/test_platform_admin.py b/tests/app/main/views/test_platform_admin.py index a9e278bd7..07f89079b 100644 --- a/tests/app/main/views/test_platform_admin.py +++ b/tests/app/main/views/test_platform_admin.py @@ -104,15 +104,14 @@ def test_should_render_platform_admin_page( @pytest.mark.parametrize('endpoint', [ - ('main.platform_admin'), - ('main.live_services'), - ('main.trial_services'), + 'main.live_services', + 'main.trial_services', ]) @pytest.mark.parametrize('include_from_test_key, inc', [ ("Y", True), ("N", False) ]) -def test_platform_admin_toggle_including_from_test_key( +def test_live_trial_services_toggle_including_from_test_key( include_from_test_key, client, platform_admin_user, @@ -131,12 +130,33 @@ def test_platform_admin_toggle_including_from_test_key( 'include_from_test_key': inc}) +@pytest.mark.parametrize('include_from_test_key, inc', [ + ("Y", True), + ("N", False) +]) +def test_platform_admin_toggle_including_from_test_key( + include_from_test_key, + client, + platform_admin_user, + mocker, + mock_get_aggregate_platform_stats, + inc +): + mock_get_user(mocker, user=platform_admin_user) + client.login(platform_admin_user) + response = client.get(url_for('main.platform_admin', include_from_test_key=include_from_test_key)) + + assert response.status_code == 200 + mock_get_aggregate_platform_stats.assert_called_once_with({'detailed': True, + 'only_active': False, + 'include_from_test_key': inc}) + + @pytest.mark.parametrize('endpoint', [ - 'main.platform_admin', 'main.live_services', 'main.trial_services' ]) -def test_platform_admin_with_date_filter( +def test_live_trial_services_with_date_filter( client, platform_admin_user, mocker, @@ -159,14 +179,60 @@ def test_platform_admin_with_date_filter( }) +def test_platform_admin_with_date_filter( + client, + platform_admin_user, + mocker, + mock_get_aggregate_platform_stats +): + mock_get_user(mocker, user=platform_admin_user) + client.login(platform_admin_user) + response = client.get(url_for('main.platform_admin', start_date='2016-12-20', end_date='2016-12-28')) + + assert response.status_code == 200 + resp_data = response.get_data(as_text=True) + assert 'Platform admin' in resp_data + mock_get_aggregate_platform_stats.assert_called_once_with({ + 'include_from_test_key': False, + 'end_date': datetime.date(2016, 12, 28), + 'start_date': datetime.date(2016, 12, 20), + 'detailed': True, + 'only_active': False, + }) + + +def test_should_show_total_on_platform_admin_page( + client, + platform_admin_user, + mocker, + mock_get_aggregate_platform_stats +): + stats = { + 'email': {'requested': 61, 'delivered': 55, 'failed': 6}, + 'sms': {'requested': 121, 'delivered': 110, 'failed': 11}, + 'letter': {'requested': 45, 'delivered': 32, 'failed': 13} + } + expected = ( + '61 emails sent 6 failed – 9.8%', + '121 text messages sent 11 failed – 9.1%', + '45 letters sent 13 failed – 28.9%' + ) + + mock_get_aggregate_platform_stats.return_value = stats + + 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 + page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + assert ( + normalize_spaces(page.select('.big-number-with-status')[0].text), + normalize_spaces(page.select('.big-number-with-status')[1].text), + normalize_spaces(page.select('.big-number-with-status')[2].text), + ) == expected + + @pytest.mark.parametrize('endpoint, expected_big_numbers', [ - ( - 'main.platform_admin', ( - '61 emails sent 6 failed – 5.5%', - '121 text messages sent 11 failed – 5.0%', - '45 letters sent 13 failed – 28.9%' - ), - ), ( 'main.live_services', ( '55 emails sent 5 failed – 5.0%', @@ -182,7 +248,7 @@ def test_platform_admin_with_date_filter( ), ), ]) -def test_should_show_total_on_platform_admin_pages( +def test_should_show_total_on_live_trial_services_pages( client, platform_admin_user, mocker, @@ -453,23 +519,16 @@ def test_should_show_correct_sent_totals_for_platform_admin( client, platform_admin_user, mocker, - mock_get_detailed_services, + mock_get_aggregate_platform_stats, fake_uuid, ): - services = [service_json(fake_uuid, 'My Service', [])] - services[0]['statistics'] = create_stats( - emails_requested=100, - emails_delivered=20, - emails_failed=40, - sms_requested=100, - sms_delivered=10, - sms_failed=30, - letters_requested=60, - letters_delivered=40, - letters_failed=5 - ) + stats = { + 'email': {'requested': 100, 'delivered': 20, 'failed': 40}, + 'sms': {'requested': 100, 'delivered': 10, 'failed': 30}, + 'letter': {'requested': 60, 'delivered': 40, 'failed': 5} - mock_get_detailed_services.return_value = {'data': services} + } + mock_get_aggregate_platform_stats.return_value = stats mock_get_user(mocker, user=platform_admin_user) client.login(platform_admin_user) response = client.get(url_for('main.platform_admin')) diff --git a/tests/conftest.py b/tests/conftest.py index c58dad210..1282c9f92 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2213,3 +2213,14 @@ def normalize_spaces(input): if isinstance(input, str): return ' '.join(input.split()) return normalize_spaces(' '.join(item.text for item in input)) + + +@pytest.fixture(scope='function') +def mock_get_aggregate_platform_stats(mocker): + stats = { + 'email': {'requested': 0, 'delivered': 0, 'failed': 0}, + 'sms': {'requested': 0, 'delivered': 0, 'failed': 0}, + 'letter': {'requested': 0, 'delivered': 0, 'failed': 0} + + } + return mocker.patch('app.service_api_client.get_aggregate_platform_stats', return_value=stats) From aaeec3f7ac8b7e0ec9a8f60c8586f971c9d1bb7d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Tue, 24 Oct 2017 11:35:30 +0100 Subject: [PATCH 2/2] Remove the extra function - oops --- app/main/views/platform_admin.py | 30 ------------------------------ 1 file changed, 30 deletions(-) diff --git a/app/main/views/platform_admin.py b/app/main/views/platform_admin.py index f50f7fa81..24195fd07 100644 --- a/app/main/views/platform_admin.py +++ b/app/main/views/platform_admin.py @@ -123,36 +123,6 @@ def create_global_stats(services): return stats -def platform_failure_percentages(services): - stats = { - 'email': { - 'delivered': 0, - 'failed': 0, - 'requested': 0 - }, - 'sms': { - 'delivered': 0, - 'failed': 0, - 'requested': 0 - }, - 'letter': { - 'delivered': 0, - 'failed': 0, - 'requested': 0 - } - } - - for service in services: - for msg_type, status in itertools.product(('sms', 'email', 'letter'), ('delivered', 'failed', 'requested')): - import pdb - pdb.set_trace() - stats[msg_type][status] += service[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: yield {