From 1681b17fab5914f7445b2ae75bef17b5c1efa105 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Fri, 24 Nov 2017 14:31:41 +0000 Subject: [PATCH 1/3] Disabled the template_history endpoint - Removed the code for the template_history endpoint and replaced with a redirect to the new page so that anyone is forwarded on - Updated the template to point to the new template_usage page --- app/main/views/dashboard.py | 31 ++------------------ app/templates/views/dashboard/dashboard.html | 2 +- 2 files changed, 3 insertions(+), 30 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 553ad8b3d..b666b3f8b 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -12,6 +12,7 @@ from flask import ( ) from flask_login import login_required from notifications_utils.recipients import format_phone_number_human_readable +from werkzeug.routing import RequestRedirect from app.main import main from app import ( @@ -75,35 +76,7 @@ def service_dashboard_updates(service_id): @user_has_permissions('view_activity', admin_override=True) def template_history(service_id): - year, current_financial_year = requested_and_current_financial_year(request) - stats = template_statistics_client.get_monthly_template_statistics_for_service(service_id, year) - - months = [ - { - 'name': yyyy_mm_to_datetime(month).strftime('%B'), - 'templates_used': aggregate_usage( - format_template_stats_to_list(stats.get(month)), sort_key='requested_count' - ), - } - for month in get_months_for_financial_year(year, time_format='%Y-%m') - ] - - return render_template( - 'views/dashboard/all-template-statistics.html', - months=months, - most_used_template_count=max( - max(( - template['requested_count'] - for template in month['templates_used'] - ), default=0) - for month in months - ), - years=get_tuples_of_financial_years( - partial(url_for, '.template_history', service_id=service_id), - end=current_financial_year, - ), - selected_year=year, - ) + raise RequestRedirect("/services/{}/template-usage".format(service_id)) @main.route("/services//template-usage") diff --git a/app/templates/views/dashboard/dashboard.html b/app/templates/views/dashboard/dashboard.html index c7d419443..01bf9324c 100644 --- a/app/templates/views/dashboard/dashboard.html +++ b/app/templates/views/dashboard/dashboard.html @@ -40,7 +40,7 @@ {% if partials['has_template_statistics'] %} {{ ajax_block(partials, updates_url, 'template-statistics') }} {{ show_more( - url_for('.template_history', service_id=current_service.id), + url_for('.template_usage', service_id=current_service.id), 'See templates used by month' ) }} {% endif %} From 155e432aa672d57259407c22c89cef042cd068b1 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Fri, 24 Nov 2017 15:20:40 +0000 Subject: [PATCH 2/3] Disabled the template_history endpoint - Updated tests and added a new mock_get_monthly_template_usage - Deleted get_monthly_template_statistics_for_service - Added new test to test the redirection of the old endpoint --- app/main/views/dashboard.py | 2 +- .../template_statistics_api_client.py | 6 ----- tests/app/main/views/test_dashboard.py | 26 +++++++++++++++---- tests/conftest.py | 24 +++++++---------- 4 files changed, 32 insertions(+), 26 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index b666b3f8b..42ed6947d 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -81,7 +81,7 @@ def template_history(service_id): @main.route("/services//template-usage") @login_required -@user_has_permissions(admin_override=True) +@user_has_permissions('view_activity', admin_override=True) def template_usage(service_id): year, current_financial_year = requested_and_current_financial_year(request) diff --git a/app/notify_client/template_statistics_api_client.py b/app/notify_client/template_statistics_api_client.py index 30f311f7f..e6afb411a 100644 --- a/app/notify_client/template_statistics_api_client.py +++ b/app/notify_client/template_statistics_api_client.py @@ -20,12 +20,6 @@ class TemplateStatisticsApiClient(NotifyAdminAPIClient): params=params )['data'] - def get_monthly_template_statistics_for_service(self, service_id, year): - - return self.get( - url='/service/{}/notifications/templates/monthly?year={}'.format(service_id, year) - )['data'] - def get_monthly_template_usage_for_service(self, service_id, year): return self.get( diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index 671d0dbae..995cb201c 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -359,17 +359,33 @@ def test_should_show_recent_templates_on_dashboard( partial(url_for), partial(url_for, year='2016'), ]) -def test_should_show_monthly_breakdown_of_template_usage( - logged_in_client, - mock_get_monthly_template_statistics, - partial_url, +def test_should_show_redirect_from_template_history( + logged_in_client, + partial_url, ): response = logged_in_client.get( partial_url('main.template_history', service_id=SERVICE_ONE_ID, _external=True) ) + assert response.status_code == 301 + + +@freeze_time("2016-07-01 12:00") # 4 months into 2016 financial year +@pytest.mark.parametrize('partial_url', [ + partial(url_for), + partial(url_for, year='2016'), +]) +def test_should_show_monthly_breakdown_of_template_usage( + logged_in_client, + mock_get_monthly_template_usage, + partial_url, +): + response = logged_in_client.get( + partial_url('main.template_usage', service_id=SERVICE_ONE_ID, _external=True) + ) + assert response.status_code == 200 - mock_get_monthly_template_statistics.assert_called_once_with(SERVICE_ONE_ID, 2016) + mock_get_monthly_template_usage.assert_called_once_with(SERVICE_ONE_ID, 2016) page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') table_rows = page.select('tbody tr') diff --git a/tests/conftest.py b/tests/conftest.py index a7364b646..f2ed5f89b 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2001,22 +2001,18 @@ def mock_get_template_statistics(mocker, service_one, fake_uuid): @pytest.fixture(scope='function') -def mock_get_monthly_template_statistics(mocker, service_one, fake_uuid): +def mock_get_monthly_template_usage(mocker, service_one, fake_uuid): def _stats(service_id, year): - return { - datetime.utcnow().strftime('%Y-%m'): { - fake_uuid: { - "counts": { - "sending": 1, - "delivered": 1, - }, - "name": 'My first template', - "type": 'sms', - } - } - } + return [{ + "template_id": fake_uuid, + "month": 4, + "year": year, + "count": 2, + "name": 'My first template', + "type": 'sms' + }] return mocker.patch( - 'app.template_statistics_client.get_monthly_template_statistics_for_service', + 'app.template_statistics_client.get_monthly_template_usage_for_service', side_effect=_stats ) From 629593291c74ee2c69aaf55f7273a23da61eff01 Mon Sep 17 00:00:00 2001 From: Richard Chapman Date: Fri, 24 Nov 2017 16:34:45 +0000 Subject: [PATCH 3/3] Used url_for and redirect - url_for instead of constructing the url manually - Used redirect instead of RequestRedirect to be more consistent with other code --- app/main/views/dashboard.py | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 42ed6947d..4c2b05b91 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -12,7 +12,7 @@ from flask import ( ) from flask_login import login_required from notifications_utils.recipients import format_phone_number_human_readable -from werkzeug.routing import RequestRedirect +from werkzeug.utils import redirect from app.main import main from app import ( @@ -76,7 +76,7 @@ def service_dashboard_updates(service_id): @user_has_permissions('view_activity', admin_override=True) def template_history(service_id): - raise RequestRedirect("/services/{}/template-usage".format(service_id)) + return redirect(url_for('main.template_usage', service_id=service_id), code=301) @main.route("/services//template-usage")