From 4831e068d9c7a89f7257da2d1c208629660bcc8f Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 6 Jun 2016 11:09:46 +0100 Subject: [PATCH 1/3] Add warning message showing date template last used when deleting --- app/main/views/templates.py | 42 +++++++++++++++++++++++--- tests/app/main/views/test_templates.py | 2 ++ 2 files changed, 40 insertions(+), 4 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 70fb1f932..05fd34ddd 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -1,9 +1,10 @@ +from datetime import datetime, timedelta from string import ascii_uppercase -from flask import request, render_template, redirect, url_for, flash, abort, session +from flask import request, render_template, redirect, url_for, flash, abort from flask_login import login_required -from notifications_utils.template import Template, TemplateChange +from notifications_utils.template import Template from notifications_utils.recipients import first_column_heading from notifications_python_client.errors import HTTPError @@ -11,7 +12,7 @@ from app.main import main from app.utils import user_has_permissions from app.main.forms import SMSTemplateForm, EmailTemplateForm from app.main.views.send import get_example_csv_rows -from app import service_api_client, current_service +from app import service_api_client, current_service, template_statistics_client form_objects = { @@ -183,7 +184,10 @@ def delete_service_template(service_id, template_id): template['template_content'] = template['content'] form = form_objects[template['template_type']](**template) - flash('Are you sure you want to delete ‘{}’?'.format(form.name.data), 'delete') + + template_statistics = template_statistics_client.get_template_statistics_for_service(service_id) + last_use_message = get_last_use_message(form.name.data, template['id'], template_statistics) + flash('{}. Are you sure you want to it?'.format(last_use_message), 'delete') return render_template( 'views/edit-{}-template.html'.format(template['template_type']), h1='Edit template', @@ -216,3 +220,33 @@ def view_template_versions(service_id, template_id): ) for template in service_api_client.get_service_template_versions(service_id, template_id)['data'] ] ) + + +def get_last_use_message(template_name, template_id, template_statistics): + try: + most_recent_use = max( + template_stats['updated_at'] + for template_stats in template_statistics + if template_stats['id'] == template_id + ) + except ValueError: + return '{} has never been used' + + return '{} was last used {} ago'.format( + get_human_readable_delta(most_recent_use, datetime.now()) + ) + + +def get_human_readable_delta(from_time, until_time): + delta = until_time - from_time + if delta < timedelta(seconds=60): + return 'under a minute' + elif delta < timedelta(hours=1): + minutes = int(delta.seconds / 60) + return '{} minute{}'.format(minutes, 's' if minutes == 1 else '') + elif delta < timedelta(days=1): + hours = int(delta.seconds / 3600) + return '{} hour{}'.format(hours, 's' if hours == 1 else '') + else: + days = delta.days + return '{} day{}'.format(days, 's' if days == 1 else '') diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 922357eac..cbb414556 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -226,6 +226,7 @@ def test_should_show_delete_template_page(app_, mock_get_user, mock_get_user_by_email, mock_has_permissions, + mock_get_template_statistics, fake_uuid): with app_.test_request_context(): with app_.test_client() as client: @@ -292,6 +293,7 @@ def test_route_permissions(mocker, api_user_active, service_one, mock_get_service_template, + mock_get_template_statistics, fake_uuid): routes = [ 'main.add_service_template', From 9db20819ef9b032dc29a6b44b01dbb7e3a6ca38c Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Mon, 6 Jun 2016 11:42:40 +0100 Subject: [PATCH 2/3] tests for last used message also now parsing the datetime correctly and removing its UTC tz info to make comparisons work --- app/main/views/templates.py | 18 +++++---- tests/app/main/views/test_templates.py | 53 ++++++++++++++++++++++++-- tests/conftest.py | 3 +- 3 files changed, 61 insertions(+), 13 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 05fd34ddd..6e304ea1a 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -3,6 +3,7 @@ from string import ascii_uppercase from flask import request, render_template, redirect, url_for, flash, abort from flask_login import login_required +from dateutil.parser import parse from notifications_utils.template import Template from notifications_utils.recipients import first_column_heading @@ -187,7 +188,7 @@ def delete_service_template(service_id, template_id): template_statistics = template_statistics_client.get_template_statistics_for_service(service_id) last_use_message = get_last_use_message(form.name.data, template['id'], template_statistics) - flash('{}. Are you sure you want to it?'.format(last_use_message), 'delete') + flash('{}. Are you sure you want to delete it?'.format(last_use_message), 'delete') return render_template( 'views/edit-{}-template.html'.format(template['template_type']), h1='Edit template', @@ -225,15 +226,16 @@ def view_template_versions(service_id, template_id): def get_last_use_message(template_name, template_id, template_statistics): try: most_recent_use = max( - template_stats['updated_at'] + parse(template_stats['updated_at']).replace(tzinfo=None) for template_stats in template_statistics - if template_stats['id'] == template_id + if template_stats['template']['id'] == template_id ) except ValueError: - return '{} has never been used' + return '{} has never been used'.format(template_name) return '{} was last used {} ago'.format( - get_human_readable_delta(most_recent_use, datetime.now()) + template_name, + get_human_readable_delta(most_recent_use, datetime.utcnow()) ) @@ -243,10 +245,10 @@ def get_human_readable_delta(from_time, until_time): return 'under a minute' elif delta < timedelta(hours=1): minutes = int(delta.seconds / 60) - return '{} minute{}'.format(minutes, 's' if minutes == 1 else '') + return '{} minute{}'.format(minutes, '' if minutes == 1 else 's') elif delta < timedelta(days=1): hours = int(delta.seconds / 3600) - return '{} hour{}'.format(hours, 's' if hours == 1 else '') + return '{} hour{}'.format(hours, '' if hours == 1 else 's') else: days = delta.days - return '{} day{}'.format(days, 's' if days == 1 else '') + return '{} day{}'.format(days, '' if days == 1 else 's') diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index cbb414556..c7b5242dc 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -1,9 +1,12 @@ -import json -import uuid -from bs4 import BeautifulSoup -from tests import validate_route_permission +from datetime import datetime +import pytest +from bs4 import BeautifulSoup from flask import url_for +from freezegun import freeze_time + +from tests import validate_route_permission +from app.main.views.templates import get_last_use_message, get_human_readable_delta def test_should_show_page_for_one_templates(app_, @@ -362,3 +365,45 @@ def test_route_invalid_permissions(mocker, ['view_activity'], api_user_active, service_one) + + +@pytest.mark.parametrize('template_statistics', [ + [{'template': {'id': 'bar'}, 'updated_at': '2000-01-01T12:00:00.000000+00:00'}], + [] +]) +def test_get_last_use_message_returns_no_template_message(template_statistics): + assert get_last_use_message('My Template', 'foo', template_statistics) == 'My Template has never been used' + + +@freeze_time('2000-01-01T15:00') +def test_get_last_use_message_uses_most_recent_statistics_for_template(): + template_statistics = [ + { + 'template': {'id': 'foo'}, + 'updated_at': '2000-01-01T12:00:00.000000+00:00' + }, + { + 'template': {'id': 'foo'}, + 'updated_at': '2000-01-01T09:00:00.000000+00:00' + }, + { + 'template': {'id': 'bar'}, + 'updated_at': '2000-01-01T15:00:00.000000+00:00' + }, + ] + assert get_last_use_message('My Template', 'foo', template_statistics) == 'My Template was last used 3 hours ago' + + +@pytest.mark.parametrize('from_time, until_time, message', [ + (datetime(2000, 1, 1, 12, 0), datetime(2000, 1, 1, 12, 0, 59), 'under a minute'), + (datetime(2000, 1, 1, 12, 0), datetime(2000, 1, 1, 12, 1), '1 minute'), + (datetime(2000, 1, 1, 12, 0), datetime(2000, 1, 1, 12, 2, 35), '2 minutes'), + (datetime(2000, 1, 1, 12, 0), datetime(2000, 1, 1, 12, 59), '59 minutes'), + (datetime(2000, 1, 1, 12, 0), datetime(2000, 1, 1, 13, 0), '1 hour'), + (datetime(2000, 1, 1, 12, 0), datetime(2000, 1, 1, 14, 0), '2 hours'), + (datetime(2000, 1, 1, 12, 0), datetime(2000, 1, 2, 11, 59), '23 hours'), + (datetime(2000, 1, 1, 12, 0), datetime(2000, 1, 2, 12, 0), '1 day'), + (datetime(2000, 1, 1, 12, 0), datetime(2000, 1, 3, 14, 0), '2 days'), +]) +def test_get_human_readable_delta(from_time, until_time, message): + assert get_human_readable_delta(from_time, until_time) == message diff --git a/tests/conftest.py b/tests/conftest.py index caa401010..c29354a70 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -981,7 +981,8 @@ def mock_get_template_statistics(mocker, service_one, fake_uuid): }, "service": template['service'], "id": str(generate_uuid()), - "day": "2016-04-04" + "day": "2016-04-04", + "updated_at": "2016-04-04T12:00:00.000000+00:00" } def _get_stats(service_id, limit_days=None): From c4305d161029c4ba561575ba59c9e94819e02501 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 7 Jun 2016 14:28:02 +0100 Subject: [PATCH 3/3] only get template statistics for specific template --- app/main/views/templates.py | 7 +++--- .../template_statistics_api_client.py | 5 ++++ .../test_template_statistics_client.py | 15 +++++++++++- tests/app/main/views/test_templates.py | 20 +++++----------- tests/conftest.py | 23 +++++++++++++++++++ 5 files changed, 51 insertions(+), 19 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 6e304ea1a..4ffe6c2b0 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -186,8 +186,8 @@ def delete_service_template(service_id, template_id): template['template_content'] = template['content'] form = form_objects[template['template_type']](**template) - template_statistics = template_statistics_client.get_template_statistics_for_service(service_id) - last_use_message = get_last_use_message(form.name.data, template['id'], template_statistics) + template_statistics = template_statistics_client.get_template_statistics_for_service(service_id, template['id']) + last_use_message = get_last_use_message(form.name.data, template_statistics) flash('{}. Are you sure you want to delete it?'.format(last_use_message), 'delete') return render_template( 'views/edit-{}-template.html'.format(template['template_type']), @@ -223,12 +223,11 @@ def view_template_versions(service_id, template_id): ) -def get_last_use_message(template_name, template_id, template_statistics): +def get_last_use_message(template_name, template_statistics): try: most_recent_use = max( parse(template_stats['updated_at']).replace(tzinfo=None) for template_stats in template_statistics - if template_stats['template']['id'] == template_id ) except ValueError: return '{} has never been used'.format(template_name) diff --git a/app/notify_client/template_statistics_api_client.py b/app/notify_client/template_statistics_api_client.py index 2ac088f76..b0d54b22c 100644 --- a/app/notify_client/template_statistics_api_client.py +++ b/app/notify_client/template_statistics_api_client.py @@ -20,3 +20,8 @@ class TemplateStatisticsApiClient(BaseAPIClient): url='/service/{}/template-statistics'.format(service_id), params=params )['data'] + + def get_template_statistics_for_template(self, service_id, template_id): + return self.get( + url='/service/{}/template-statistics/{}'.format(service_id, template_id) + )['data'] diff --git a/tests/app/main/notify_client/test_template_statistics_client.py b/tests/app/main/notify_client/test_template_statistics_client.py index 938eb4254..b79d67d29 100644 --- a/tests/app/main/notify_client/test_template_statistics_client.py +++ b/tests/app/main/notify_client/test_template_statistics_client.py @@ -3,7 +3,7 @@ import uuid from app.notify_client.template_statistics_api_client import TemplateStatisticsApiClient -def test_template_statistics_client_calls_correct_api_endpoint(mocker, api_user_active): +def test_template_statistics_client_calls_correct_api_endpoint_for_service(mocker, api_user_active): some_service_id = uuid.uuid4() expected_url = '/service/{}/template-statistics'.format(some_service_id) @@ -15,3 +15,16 @@ def test_template_statistics_client_calls_correct_api_endpoint(mocker, api_user_ client.get_template_statistics_for_service(some_service_id) mock_get.assert_called_once_with(url=expected_url, params={}) + + +def test_template_statistics_client_calls_correct_api_endpoint_for_template(mocker, api_user_active): + some_service_id = uuid.uuid4() + some_template_id = uuid.uuid4() + expected_url = '/service/{}/template-statistics/{}'.format(some_service_id, some_template_id) + + client = TemplateStatisticsApiClient() + mock_get = mocker.patch('app.notify_client.template_statistics_api_client.TemplateStatisticsApiClient.get') + + client.get_template_statistics_for_template(some_service_id, some_template_id) + + mock_get.assert_called_once_with(url=expected_url) diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index c7b5242dc..9d33c3893 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -230,6 +230,7 @@ def test_should_show_delete_template_page(app_, mock_get_user_by_email, mock_has_permissions, mock_get_template_statistics, + mock_get_template_statistics_for_template, fake_uuid): with app_.test_request_context(): with app_.test_client() as client: @@ -297,6 +298,7 @@ def test_route_permissions(mocker, service_one, mock_get_service_template, mock_get_template_statistics, + mock_get_template_statistics_for_template, fake_uuid): routes = [ 'main.add_service_template', @@ -367,31 +369,21 @@ def test_route_invalid_permissions(mocker, service_one) -@pytest.mark.parametrize('template_statistics', [ - [{'template': {'id': 'bar'}, 'updated_at': '2000-01-01T12:00:00.000000+00:00'}], - [] -]) -def test_get_last_use_message_returns_no_template_message(template_statistics): - assert get_last_use_message('My Template', 'foo', template_statistics) == 'My Template has never been used' +def test_get_last_use_message_returns_no_template_message(): + assert get_last_use_message('My Template', []) == 'My Template has never been used' @freeze_time('2000-01-01T15:00') -def test_get_last_use_message_uses_most_recent_statistics_for_template(): +def test_get_last_use_message_uses_most_recent_statistics(): template_statistics = [ { - 'template': {'id': 'foo'}, 'updated_at': '2000-01-01T12:00:00.000000+00:00' }, { - 'template': {'id': 'foo'}, 'updated_at': '2000-01-01T09:00:00.000000+00:00' }, - { - 'template': {'id': 'bar'}, - 'updated_at': '2000-01-01T15:00:00.000000+00:00' - }, ] - assert get_last_use_message('My Template', 'foo', template_statistics) == 'My Template was last used 3 hours ago' + assert get_last_use_message('My Template', template_statistics) == 'My Template was last used 3 hours ago' @pytest.mark.parametrize('from_time, until_time, message', [ diff --git a/tests/conftest.py b/tests/conftest.py index c29354a70..de8dc8ea3 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -992,6 +992,29 @@ def mock_get_template_statistics(mocker, service_one, fake_uuid): 'app.template_statistics_client.get_template_statistics_for_service', side_effect=_get_stats) +@pytest.fixture(scope='function') +def mock_get_template_statistics_for_template(mocker, service_one): + def _get_stats(service_id, template_id): + template = template_json(service_id, template_id, "Test template", "sms", "Something very interesting") + return [ + { + "usage_count": 1, + "template": { + "name": template['name'], + "template_type": template['template_type'], + "id": template['id'] + }, + "service": template['service'], + "id": str(generate_uuid()), + "day": "2016-04-04", + "updated_at": "2016-04-04T12:00:00.000000+00:00" + } + ] + + return mocker.patch( + 'app.template_statistics_client.get_template_statistics_for_service', side_effect=_get_stats) + + @pytest.fixture(scope='function') def mock_get_usage(mocker, service_one, fake_uuid): def _get_usage(service_id):