From 9d25f326b0470a962f892c1c162ffadd54166784 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Mon, 22 Aug 2016 16:25:35 +0100 Subject: [PATCH 1/4] Using new endpoint for template statistics - gets notifications by template id, returning the most recent to illustrate the last use of that template. --- app/main/views/templates.py | 18 ++++++++++--- tests/__init__.py | 53 +++++++++++++++++++++++++++++++++++-- tests/conftest.py | 18 +++---------- 3 files changed, 69 insertions(+), 20 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 6711e29fd..5005186d1 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -186,9 +186,21 @@ 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_template(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') + try: + last_used_notification = template_statistics_client.get_template_statistics_for_template( + service_id, template['id'] + ) + message = '{} was last used {} ago'.format( + last_used_notification['template']['name'], + get_human_readable_delta( + parse(last_used_notification['created_at']).replace(tzinfo=None), + datetime.utcnow()) + ) + except HTTPError as e: + if e.status_code == 404: + message = '{} has never been used'.format(template['name']) + + flash('{}. Are you sure you want to delete it?'.format(message), 'delete') return render_template( 'views/edit-{}-template.html'.format(template['template_type']), h1='Edit template', diff --git a/tests/__init__.py b/tests/__init__.py index 6e1fef4e6..5892a7884 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -213,6 +213,10 @@ def notification_json( 'next': '/service/{}/notifications'.format(service_id), 'last': '/service/{}/notifications'.format(service_id) } + job_payload = None + if job: + job_payload = {'id': job['id'], 'original_file_name': job['original_file_name']} + data = { 'notifications': [{ 'to': to, @@ -220,7 +224,7 @@ def notification_json( 'id': template['id'], 'name': template['name'], 'template_type': template['template_type']}, - 'job': {'id': job['id'], 'original_file_name': job['original_file_name']} if job else None, + 'job': job_payload, 'sent_at': sent_at, 'status': status, 'created_at': created_at, @@ -228,13 +232,58 @@ def notification_json( 'job_row_number': job_row_number, 'template_version': template['version'] } for i in range(rows)], - 'total': 5, + 'total': rows, 'page_size': 50, 'links': links } return data +def single_notification_json( + service_id, + job=None, + template=None, + to='07123456789', + status=None, + sent_at=None, + job_row_number=None, + created_at=None, + updated_at=None +): + if template is None: + template = template_json(service_id, str(generate_uuid())) + if sent_at is None: + sent_at = str(datetime.utcnow().time()) + if created_at is None: + created_at = str(datetime.utcnow().time()) + if updated_at is None: + updated_at = str((datetime.utcnow() + timedelta(minutes=1)).time()) + if status is None: + status = 'delivered' + job_payload = None + if job: + job_payload = {'id': job['id'], 'original_file_name': job['original_file_name']} + + data = { + 'sent_at': sent_at, + 'billable_units': 1, + 'status': status, + 'created_at': created_at, + 'reference': None, + 'updated_at': updated_at, + 'template_version': 5, + 'service': service_id, + 'id': '29441662-17ce-4ffe-9502-fcaed73b2826', + 'template': template, + 'job_row_number': 0, + 'notification_type': 'sms', + 'api_key': None, + 'job': job_payload, + 'sent_by': 'mmg' + } + return data + + def validate_route_permission(mocker, app_, method, diff --git a/tests/conftest.py b/tests/conftest.py index 25bde5c81..37d5a020f 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -14,7 +14,7 @@ from . import ( notification_json, invite_json, sample_uuid, - generate_uuid) + generate_uuid, single_notification_json) from app.notify_client.models import ( User, InvitedUser @@ -1091,20 +1091,8 @@ def mock_get_template_statistics(mocker, service_one, fake_uuid): 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" - } - ] + notification = single_notification_json(service_id, template=template) + return notification return mocker.patch( 'app.template_statistics_client.get_template_statistics_for_template', side_effect=_get_stats) From 0b95a30e2f9fe12d74d5308c4b38ab14718f4d26 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Aug 2016 11:26:34 +0100 Subject: [PATCH 2/4] Raise the exception if not a 404 --- app/main/views/templates.py | 2 ++ 1 file changed, 2 insertions(+) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 5005186d1..982832e9a 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -199,6 +199,8 @@ def delete_service_template(service_id, template_id): except HTTPError as e: if e.status_code == 404: message = '{} has never been used'.format(template['name']) + else: + raise e flash('{}. Are you sure you want to delete it?'.format(message), 'delete') return render_template( From d149a9691a8420a295d0137b26811e56c266196a Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Aug 2016 12:09:38 +0100 Subject: [PATCH 3/4] Added tests for the 2 use cases: used templates unused templates --- app/main/views/templates.py | 2 +- tests/__init__.py | 8 +- tests/app/main/views/test_templates.py | 146 ++++++++++++++++--------- tests/conftest.py | 2 +- 4 files changed, 100 insertions(+), 58 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 982832e9a..62368afc2 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -7,7 +7,7 @@ from dateutil.parser import parse from notifications_utils.template import Template from notifications_utils.recipients import first_column_heading -from notifications_python_client.errors import HTTPError +from notify_client import HTTPError from app.main import main from app.utils import user_has_permissions diff --git a/tests/__init__.py b/tests/__init__.py index 5892a7884..bfd085d3d 100644 --- a/tests/__init__.py +++ b/tests/__init__.py @@ -243,21 +243,19 @@ def single_notification_json( service_id, job=None, template=None, - to='07123456789', status=None, sent_at=None, - job_row_number=None, created_at=None, updated_at=None ): if template is None: template = template_json(service_id, str(generate_uuid())) if sent_at is None: - sent_at = str(datetime.utcnow().time()) + sent_at = str(datetime.utcnow()) if created_at is None: - created_at = str(datetime.utcnow().time()) + created_at = str(datetime.utcnow()) if updated_at is None: - updated_at = str((datetime.utcnow() + timedelta(minutes=1)).time()) + updated_at = str(datetime.utcnow() + timedelta(minutes=1)) if status is None: status = 'delivered' job_payload = None diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index 8f296c653..ae24d00d5 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -1,24 +1,26 @@ from datetime import datetime +from unittest.mock import Mock import pytest from bs4 import BeautifulSoup from flask import url_for from freezegun import freeze_time +from notify_client import HTTPError -from tests import validate_route_permission +from tests import validate_route_permission, template_json, single_notification_json from app.main.views.templates import get_last_use_message, get_human_readable_delta def test_should_show_page_for_one_template( - app_, - api_user_active, - mock_login, - mock_get_service, - mock_get_service_template, - mock_get_user, - mock_get_user_by_email, - mock_has_permissions, - fake_uuid + app_, + api_user_active, + mock_login, + mock_get_service, + mock_get_service_template, + mock_get_user, + mock_get_user_by_email, + mock_has_permissions, + fake_uuid ): with app_.test_request_context(): with app_.test_client() as client: @@ -74,16 +76,16 @@ def test_should_redirect_when_saving_a_template(app_, def test_should_show_interstitial_when_making_breaking_change( - app_, - api_user_active, - mock_login, - mock_get_service_template, - mock_update_service_template, - mock_get_user, - mock_get_service, - mock_get_user_by_email, - mock_has_permissions, - fake_uuid + app_, + api_user_active, + mock_login, + mock_get_service_template, + mock_update_service_template, + mock_get_user, + mock_get_service, + mock_get_user_by_email, + mock_has_permissions, + fake_uuid ): with app_.test_request_context(): with app_.test_client() as client: @@ -143,9 +145,9 @@ def test_should_not_create_too_big_template(app_, assert resp.status_code == 200 assert ( - "Content has a character count greater" - " than the limit of 459" - ) in resp.get_data(as_text=True) + "Content has a character count greater" + " than the limit of 459" + ) in resp.get_data(as_text=True) def test_should_not_update_too_big_template(app_, @@ -178,9 +180,9 @@ def test_should_not_update_too_big_template(app_, assert resp.status_code == 200 assert ( - "Content has a character count greater" - " than the limit of 459" - ) in resp.get_data(as_text=True) + "Content has a character count greater" + " than the limit of 459" + ) in resp.get_data(as_text=True) def test_should_redirect_when_saving_a_template_email(app_, @@ -223,18 +225,59 @@ def test_should_redirect_when_saving_a_template_email(app_, template_id, name, 'email', content, service_id, subject) -def test_should_show_delete_template_page(app_, - api_user_active, - mock_login, - mock_get_service, - mock_get_service_template, - mock_get_user, - mock_get_user_by_email, - mock_has_permissions, - mock_get_template_statistics_for_template, - fake_uuid): +def test_should_show_delete_template_page_with_time_block(app_, + api_user_active, + mock_login, + mock_get_service, + mock_get_service_template, + mock_get_user, + mock_get_user_by_email, + mock_has_permissions, + fake_uuid, + mocker): with app_.test_request_context(): with app_.test_client() as client: + with freeze_time('2012-01-01 12:00:00'): + template = template_json('1234', '1234', "Test template", "sms", "Something very interesting") + notification = single_notification_json('1234', template=template) + + mocker.patch('app.template_statistics_client.get_template_statistics_for_template', + return_value=notification) + + with freeze_time('2012-01-01 12:10:00'): + client.login(api_user_active) + service_id = fake_uuid + template_id = fake_uuid + response = client.get(url_for( + '.delete_service_template', + service_id=service_id, + template_id=template_id)) + content = response.get_data(as_text=True) + assert response.status_code == 200 + assert 'Test template was last used 10 minutes ago. Are you sure you want to delete it?' in content + assert 'Are you sure' in content + assert 'Two week reminder' in content + assert 'Your vehicle tax is about to expire' in content + mock_get_service_template.assert_called_with(service_id, template_id) + + +def test_should_show_delete_template_page_with_never_used_block(app_, + api_user_active, + mock_login, + mock_get_service, + mock_get_service_template, + mock_get_user, + mock_get_user_by_email, + mock_has_permissions, + fake_uuid, + mocker): + with app_.test_request_context(): + with app_.test_client() as client: + mocker.patch( + 'app.template_statistics_client.get_template_statistics_for_template', + side_effect=HTTPError(response=Mock(status_code=404), message="Default message") + ) + client.login(api_user_active) service_id = fake_uuid template_id = fake_uuid @@ -243,12 +286,13 @@ def test_should_show_delete_template_page(app_, service_id=service_id, template_id=template_id)) - content = response.get_data(as_text=True) - assert response.status_code == 200 - assert 'Are you sure' in content - assert 'Two week reminder' in content - assert 'Your vehicle tax is about to expire' in content - mock_get_service_template.assert_called_with(service_id, template_id) + content = response.get_data(as_text=True) + assert response.status_code == 200 + assert 'Two week reminder has never been used. Are you sure you want to delete it?' in content + assert 'Are you sure' in content + assert 'Two week reminder' in content + assert 'Your vehicle tax is about to expire' in content + mock_get_service_template.assert_called_with(service_id, template_id) def test_should_redirect_when_deleting_a_template(app_, @@ -294,15 +338,15 @@ def test_should_redirect_when_deleting_a_template(app_, @freeze_time('2016-01-01T15:00') def test_should_show_page_for_a_deleted_template( - app_, - api_user_active, - mock_login, - mock_get_service, - mock_get_deleted_template, - mock_get_user, - mock_get_user_by_email, - mock_has_permissions, - fake_uuid + app_, + api_user_active, + mock_login, + mock_get_service, + mock_get_deleted_template, + mock_get_user, + mock_get_user_by_email, + mock_has_permissions, + fake_uuid ): with app_.test_request_context(), app_.test_client() as client: client.login(api_user_active) diff --git a/tests/conftest.py b/tests/conftest.py index 12b9e95b5..e72c9ca42 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,7 +20,7 @@ from app.notify_client.models import ( InvitedUser ) -from notifications_python_client.errors import HTTPError +from notify_client.errors import HTTPError @pytest.fixture(scope='session') From d56c14e147301b38632ef8ed0a14230af486c7c8 Mon Sep 17 00:00:00 2001 From: Martyn Inglis Date: Wed, 24 Aug 2016 12:12:19 +0100 Subject: [PATCH 4/4] Actually use the right exception class. Two HTTPError classes exist. Need to use the one in notifications_python_client. --- app/main/views/templates.py | 2 +- tests/app/main/views/test_templates.py | 2 +- tests/conftest.py | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/main/views/templates.py b/app/main/views/templates.py index 62368afc2..982832e9a 100644 --- a/app/main/views/templates.py +++ b/app/main/views/templates.py @@ -7,7 +7,7 @@ from dateutil.parser import parse from notifications_utils.template import Template from notifications_utils.recipients import first_column_heading -from notify_client import HTTPError +from notifications_python_client.errors import HTTPError from app.main import main from app.utils import user_has_permissions diff --git a/tests/app/main/views/test_templates.py b/tests/app/main/views/test_templates.py index ae24d00d5..56fe515cf 100644 --- a/tests/app/main/views/test_templates.py +++ b/tests/app/main/views/test_templates.py @@ -5,7 +5,7 @@ import pytest from bs4 import BeautifulSoup from flask import url_for from freezegun import freeze_time -from notify_client import HTTPError +from notifications_python_client.errors import HTTPError from tests import validate_route_permission, template_json, single_notification_json from app.main.views.templates import get_last_use_message, get_human_readable_delta diff --git a/tests/conftest.py b/tests/conftest.py index e72c9ca42..12b9e95b5 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -20,7 +20,7 @@ from app.notify_client.models import ( InvitedUser ) -from notify_client.errors import HTTPError +from notifications_python_client.errors import HTTPError @pytest.fixture(scope='session')