From ee10b3a6c2c5ec94aed5361121377c0e8668420b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Jul 2016 15:19:58 +0100 Subject: [PATCH 1/3] use detailed service endpoint in notification summary page to help get rid of notification statistics tables, move over the job page. fortunately its required data format is almost identical to the return value of the detailed service endpoint, so little work is required - only to add a sending column --- app/main/views/jobs.py | 57 +++++++++++++++++++++++++----------------- 1 file changed, 34 insertions(+), 23 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index a1d579ef1..f043ddfca 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -1,5 +1,4 @@ # -*- coding: utf-8 -*- - import time import dateutil from datetime import datetime, timedelta, timezone @@ -21,7 +20,6 @@ from app import ( job_api_client, notification_api_client, service_api_client, - statistics_api_client, current_service, format_datetime_short) from app.main import main @@ -30,7 +28,7 @@ from app.utils import ( generate_previous_next_dict, user_has_permissions, generate_notifications_csv) -from app.statistics_utils import sum_of_statistics, statistics_by_state, add_rate_to_jobs +from app.statistics_utils import add_rate_to_jobs from app.utils import get_help_argument @@ -171,9 +169,6 @@ def view_notifications(service_id, message_type): template_type=[message_type], status=filter_args.get('status'), limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS']) - service_statistics_by_state = statistics_by_state(sum_of_statistics( - statistics_api_client.get_statistics_for_service(service_id, limit_days=7)['data'] - )) view_dict = dict( message_type=message_type, status=request.args.get('status') @@ -223,23 +218,11 @@ def view_notifications(service_id, message_type): message_type=message_type, status=request.args.get('status') ), - status_filters=[ - [ - item[0], item[1], - url_for( - '.view_notifications', - service_id=current_service['id'], - message_type=message_type, - status=item[1] - ), - service_statistics_by_state[message_type][item[0]] - ] for item in [ - ['processed', 'sending,delivered,failed'], - ['sending', 'sending'], - ['delivered', 'delivered'], - ['failed', 'failed'], - ] - ] + status_filters=get_status_filters( + current_service, + message_type, + service_api_client.get_detailed_service(service_id)['data']['statistics'] + ) ) @@ -261,6 +244,34 @@ def view_notification(service_id, job_id, notification_id): ) +def get_status_filters(service, message_type, statistics): + stats = statistics[message_type] + stats['sending'] = stats['requested'] - stats['delivered'] - stats['failed'] + + filters = [ + # key, label, option + ('requested', 'processed', 'sending,delivered,failed'), + ('sending', 'sending', 'sending'), + ('delivered', 'delivered', 'delivered'), + ('failed', 'failed', 'failed'), + ] + return [ + # return list containing label, option, link, count + ( + label, + option, + url_for( + '.view_notifications', + service_id=service['id'], + message_type=message_type, + status=option + ), + stats[key] + ) + for key, label, option in filters + ] + + def _get_job_counts(job, help_argument): return [ ( From 57e03349d209f1e9ec8fd11ebeb4f6f874605a0b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Jul 2016 15:54:30 +0100 Subject: [PATCH 2/3] remove get_statistics_for_service from statistics_api_client --- app/notify_client/statistics_api_client.py | 9 ------ tests/app/main/views/test_accept_invite.py | 1 - tests/app/main/views/test_dashboard.py | 7 ----- tests/app/main/views/test_jobs.py | 7 ++--- tests/app/main/views/test_sign_out.py | 1 - .../notify_client/test_statistics_client.py | 28 ------------------- tests/conftest.py | 9 ------ 7 files changed, 2 insertions(+), 60 deletions(-) diff --git a/app/notify_client/statistics_api_client.py b/app/notify_client/statistics_api_client.py index e9250b1f9..cc3fdc480 100644 --- a/app/notify_client/statistics_api_client.py +++ b/app/notify_client/statistics_api_client.py @@ -13,15 +13,6 @@ class StatisticsApiClient(BaseAPIClient): self.client_id = app.config['ADMIN_CLIENT_USER_NAME'] self.secret = app.config['ADMIN_CLIENT_SECRET'] - def get_statistics_for_service(self, service_id, limit_days=None): - params = {} - if limit_days is not None: - params['limit_days'] = limit_days - return self.get( - url='/service/{}/notifications-statistics'.format(service_id), - params=params - ) - def get_statistics_for_service_for_day(self, service_id, day): url = '/service/{}/notifications-statistics/day/{}'.format(service_id, day) try: diff --git a/tests/app/main/views/test_accept_invite.py b/tests/app/main/views/test_accept_invite.py index f707e310b..137846f55 100644 --- a/tests/app/main/views/test_accept_invite.py +++ b/tests/app/main/views/test_accept_invite.py @@ -311,7 +311,6 @@ def test_new_invited_user_verifies_and_added_to_service(app_, mock_accept_invite, mock_get_service, mock_get_service_templates, - mock_get_service_statistics, mock_get_template_statistics, mock_get_jobs, mock_has_permissions, diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index f93f70d7c..dc5a35d80 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -65,7 +65,6 @@ def test_get_started( api_user_active, mock_get_service, mock_get_service_templates_when_no_templates_exist, - mock_get_service_statistics, mock_get_aggregate_service_statistics, mock_get_user, mock_get_user_by_email, @@ -95,7 +94,6 @@ def test_get_started_is_hidden_once_templates_exist( 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, @@ -166,7 +164,6 @@ def test_should_show_all_templates_on_template_statistics_page( api_user_active, mock_get_service, mock_get_service_templates, - mock_get_service_statistics, mock_get_user, mock_get_user_by_email, mock_login, @@ -207,7 +204,6 @@ def test_should_show_recent_jobs_on_dashboard( 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, @@ -253,7 +249,6 @@ def _test_dashboard_menu(mocker, app_, usr, service, permissions): mocker.patch('app.user_api_client.get_user', return_value=usr) mocker.patch('app.user_api_client.get_user_by_email', return_value=usr) mocker.patch('app.service_api_client.get_service', return_value={'data': service}) - mocker.patch('app.statistics_api_client.get_statistics_for_service', return_value={'data': [{}]}) client.login(usr) return client.get(url_for('main.service_dashboard', service_id=service['id'])) @@ -392,7 +387,6 @@ def test_route_for_service_permissions(mocker, mock_get_user, mock_get_service_templates, mock_get_jobs, - mock_get_service_statistics, mock_get_template_statistics, mock_get_detailed_service, mock_get_usage): @@ -435,7 +429,6 @@ def test_service_dashboard_updates_gets_dashboard_totals(mocker, 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}, diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 2a42ba219..3229e6eab 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -57,7 +57,6 @@ def test_should_show_page_for_one_job( service_one, active_user_with_permissions, mock_get_service_template, - mock_get_service_statistics, mock_get_job, mocker, mock_get_notifications, @@ -111,7 +110,6 @@ def test_should_show_not_show_csv_download_in_tour( service_one, active_user_with_permissions, mock_get_service_template, - mock_get_service_statistics, mock_get_job, mocker, mock_get_notifications, @@ -147,7 +145,6 @@ def test_should_show_updates_for_one_job_as_json( service_one, active_user_with_permissions, mock_get_notifications, - mock_get_service_statistics, mock_get_job, mocker, fake_uuid @@ -203,7 +200,7 @@ def test_can_show_notifications( service_one, active_user_with_permissions, mock_get_notifications, - mock_get_service_statistics, + mock_get_detailed_service, mocker, message_type, page_title, @@ -263,7 +260,7 @@ def test_should_show_notifications_for_a_service_with_next_previous( service_one, active_user_with_permissions, mock_get_notifications_with_previous_next, - mock_get_service_statistics, + mock_get_detailed_service, mocker ): with app_.test_request_context(): diff --git a/tests/app/main/views/test_sign_out.py b/tests/app/main/views/test_sign_out.py index 6cfe07a38..667a3e332 100644 --- a/tests/app/main/views/test_sign_out.py +++ b/tests/app/main/views/test_sign_out.py @@ -17,7 +17,6 @@ def test_sign_out_user(app_, mock_get_user_by_email, mock_login, mock_get_service_templates, - mock_get_service_statistics, mock_get_jobs, mock_has_permissions, mock_get_template_statistics, diff --git a/tests/app/notify_client/test_statistics_client.py b/tests/app/notify_client/test_statistics_client.py index 200958d2b..db378a729 100644 --- a/tests/app/notify_client/test_statistics_client.py +++ b/tests/app/notify_client/test_statistics_client.py @@ -4,34 +4,6 @@ from datetime import datetime from app.notify_client.statistics_api_client import StatisticsApiClient -def test_notifications_statistics_client_calls_correct_api_endpoint(mocker, api_user_active): - - some_service_id = uuid.uuid4() - expected_url = '/service/{}/notifications-statistics'.format(some_service_id) - - client = StatisticsApiClient() - - mock_get = mocker.patch('app.notify_client.statistics_api_client.StatisticsApiClient.get') - - client.get_statistics_for_service(some_service_id) - - mock_get.assert_called_once_with(url=expected_url, params={}) - - -def test_notifications_statistics_client_calls_correct_api_endpoint_with_params(mocker, api_user_active): - - some_service_id = uuid.uuid4() - expected_url = '/service/{}/notifications-statistics'.format(some_service_id) - - client = StatisticsApiClient() - - mock_get = mocker.patch('app.notify_client.statistics_api_client.StatisticsApiClient.get') - - client.get_statistics_for_service(some_service_id, limit_days=99) - - mock_get.assert_called_once_with(url=expected_url, params={'limit_days': 99}) - - def test_notifications_statistics_client_for_stats_by_day_calls_correct_api_endpoint(mocker, api_user_active): some_service_id = uuid.uuid4() diff --git a/tests/conftest.py b/tests/conftest.py index 4e373241b..17ed7068d 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -200,15 +200,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_service_statistics(mocker): - def _create(service_id, limit_days=None): - return {'data': [{}]} - - return mocker.patch( - 'app.statistics_api_client.get_statistics_for_service', side_effect=_create) - - @pytest.fixture(scope='function') def mock_get_service_statistics_for_day(mocker): From eab66dc6be151f4ccdc677488667ea4167fa5c51 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 20 Jul 2016 16:18:04 +0100 Subject: [PATCH 3/3] add tests for get_status_filters --- tests/app/main/views/test_jobs.py | 49 +++++++++++++++++++++++++++---- 1 file changed, 44 insertions(+), 5 deletions(-) diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index 3229e6eab..7e3378e63 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -1,12 +1,13 @@ +import json +from urllib.parse import quote + import pytest from flask import url_for from bs4 import BeautifulSoup -import json + from app.utils import generate_notifications_csv -from app.main.views.jobs import get_time_left -from tests import (notification_json, job_json) -from tests.conftest import fake_uuid -from tests.conftest import mock_get_job as mock_get_job1 +from app.main.views.jobs import get_time_left, get_status_filters +from tests import notification_json from freezegun import freeze_time @@ -318,3 +319,41 @@ def test_should_download_notifications_for_a_job(app_, @freeze_time("2016-01-10 12:00:00.000000") def test_time_left(job_created_at, expected_message): assert get_time_left(job_created_at) == expected_message + + +STATISTICS = { + 'sms': { + 'requested': 6, + 'failed': 2, + 'delivered': 1 + } +} + + +def test_get_status_filters_calculates_stats(app_): + with app_.test_request_context(): + ret = get_status_filters({'id': 'foo'}, 'sms', STATISTICS) + + assert {label: count for label, _option, _link, count in ret} == { + 'processed': 6, + 'sending': 3, + 'failed': 2, + 'delivered': 1 + } + + +def test_get_status_filters_in_right_order(app_): + with app_.test_request_context(): + ret = get_status_filters({'id': 'foo'}, 'sms', STATISTICS) + + assert [label for label, _option, _link, _count in ret] == [ + 'processed', 'sending', 'delivered', 'failed' + ] + + +def test_get_status_filters_constructs_links(app_): + with app_.test_request_context(): + ret = get_status_filters({'id': 'foo'}, 'sms', STATISTICS) + + link = ret[0][2] + assert link == '/services/foo/notifications/sms?status={}'.format(quote('sending,delivered,failed'))