From 259db9c271577e95e5df11ea481c6dacc2280374 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Sat, 30 Dec 2017 16:54:39 +0000 Subject: [PATCH 01/12] Put the download csv link on the activity page. Still needs some tests, probably some refactoring as well. --- app/main/views/jobs.py | 56 ++++++--------------- app/main/views/notifications.py | 38 ++++++++++++-- app/templates/views/notifications.html | 6 ++- app/utils.py | 70 ++++++++++++++++++++------ tests/app/test_utils.py | 10 ++-- 5 files changed, 116 insertions(+), 64 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 511716530..7d3be9284 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -1,6 +1,4 @@ # -*- coding: utf-8 -*- -from orderedset import OrderedSet -from itertools import chain from flask import ( render_template, @@ -18,7 +16,6 @@ from notifications_utils.template import ( Template, WithSubjectTemplate, ) -from werkzeug.datastructures import MultiDict from app import ( job_api_client, @@ -35,39 +32,11 @@ from app.utils import ( user_has_permissions, generate_notifications_csv, get_time_left, - REQUESTED_STATUSES, - FAILURE_STATUSES, - SENDING_STATUSES, - DELIVERED_STATUSES, get_letter_timings, -) + parse_filter_args, set_status_filters) from app.statistics_utils import add_rate_to_job -def _parse_filter_args(filter_dict): - if not isinstance(filter_dict, MultiDict): - filter_dict = MultiDict(filter_dict) - - return MultiDict( - ( - key, - (','.join(filter_dict.getlist(key))).split(',') - ) - for key in filter_dict.keys() - if ''.join(filter_dict.getlist(key)) - ) - - -def _set_status_filters(filter_args): - status_filters = filter_args.get('status', []) - return list(OrderedSet(chain( - (status_filters or REQUESTED_STATUSES), - DELIVERED_STATUSES if 'delivered' in status_filters else [], - SENDING_STATUSES if 'sending' in status_filters else [], - FAILURE_STATUSES if 'failed' in status_filters else [] - ))) - - @main.route("/services//jobs") @login_required @user_has_permissions('view_activity', admin_override=True) @@ -104,8 +73,8 @@ def view_job(service_id, job_id): if job['job_status'] == 'cancelled': abort(404) - filter_args = _parse_filter_args(request.args) - filter_args['status'] = _set_status_filters(filter_args) + filter_args = parse_filter_args(request.args) + filter_args['status'] = set_status_filters(filter_args) total_notifications = job.get('notification_count', 0) processed_notifications = job.get('notifications_delivered', 0) + job.get('notifications_failed', 0) @@ -146,8 +115,8 @@ def view_job_csv(service_id, job_id): template_id=job['template'], version=job['template_version'] )['data'] - filter_args = _parse_filter_args(request.args) - filter_args['status'] = _set_status_filters(filter_args) + filter_args = parse_filter_args(request.args) + filter_args['status'] = set_status_filters(filter_args) return Response( stream_with_context( @@ -206,6 +175,12 @@ def view_notifications(service_id, message_type): page=request.args.get('page', 1), to=request.form.get('to', ''), search_form=SearchNotificationsForm(to=request.form.get('to', '')), + download_link=url_for( + '.download_notifications_csv', + service_id=current_service['id'], + message_type=message_type, + status=request.args.get('status') + ) ) @@ -226,8 +201,8 @@ def get_notifications(service_id, message_type, status_override=None): abort(404, "Invalid page argument ({}) reverting to page 1.".format(request.args['page'], None)) if message_type not in ['email', 'sms', 'letter']: abort(404) - filter_args = _parse_filter_args(request.args) - filter_args['status'] = _set_status_filters(filter_args) + filter_args = parse_filter_args(request.args) + filter_args['status'] = set_status_filters(filter_args) if request.path.endswith('csv'): return Response( generate_notifications_csv( @@ -250,7 +225,6 @@ def get_notifications(service_id, message_type, status_override=None): limit_days=current_app.config['ACTIVITY_STATS_LIMIT_DAYS'], to=request.form.get('to', ''), ) - url_args = { 'message_type': message_type, 'status': request.args.get('status') @@ -361,8 +335,8 @@ def _get_job_counts(job): def get_job_partials(job, template): - filter_args = _parse_filter_args(request.args) - filter_args['status'] = _set_status_filters(filter_args) + filter_args = parse_filter_args(request.args) + filter_args['status'] = set_status_filters(filter_args) notifications = notification_api_client.get_notifications_for_service( job['service'], job['id'], status=filter_args['status'] ) diff --git a/app/main/views/notifications.py b/app/main/views/notifications.py index ed6fe756b..1482c68e4 100644 --- a/app/main/views/notifications.py +++ b/app/main/views/notifications.py @@ -1,18 +1,20 @@ # -*- coding: utf-8 -*- +from datetime import datetime + from flask import ( abort, render_template, jsonify, request, url_for, -) + Response, stream_with_context) from flask_login import login_required from app import ( notification_api_client, job_api_client, - current_service -) + current_service, + format_date_numeric) from app.main import main from app.template_previews import TemplatePreview from app.utils import ( @@ -23,7 +25,7 @@ from app.utils import ( get_letter_timings, FAILURE_STATUSES, DELIVERED_STATUSES, -) + generate_notifications_csv, parse_filter_args, set_status_filters) @main.route("/services//notification/") @@ -134,3 +136,31 @@ def get_all_personalisation_from_notification(notification): notification['personalisation']['phone_number'] = notification['to'] return notification['personalisation'] + + +@main.route("/services//download-notifications.csv") +@login_required +@user_has_permissions('view_activity', admin_override=True) +def download_notifications_csv(service_id): + filter_args = parse_filter_args(request.args) + filter_args['status'] = set_status_filters(filter_args) + + return Response( + stream_with_context( + generate_notifications_csv( + service_id=service_id, + job_id=None, + status=filter_args.get('status'), + page=request.args.get('page', 1), + page_size=5000, + format_for_csv=True + ) + ), + mimetype='text/csv', + headers={ + 'Content-Disposition': 'inline; filename="{} - {} - {} report.csv"'.format( + format_date_numeric(datetime.now().strftime("%Y-%m-%dT%H:%M:%S.%fZ")), + filter_args['message_type'][0], + current_service['name']) + } + ) diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index b76641b34..a03fb4874 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -43,7 +43,11 @@ {% endif %} - +

+ Download this report +   + Data available for 7 days +

{{ ajax_block( partials, url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status, page=page), diff --git a/app/utils.py b/app/utils.py index ba19eddc7..3812a936f 100644 --- a/app/utils.py +++ b/app/utils.py @@ -1,5 +1,7 @@ import re import csv +from itertools import chain + import pytz from io import StringIO from os import path @@ -28,7 +30,8 @@ from notifications_utils.template import ( LetterImageTemplate, LetterPreviewTemplate, ) - +from orderedset._orderedset import OrderedSet +from werkzeug.datastructures import MultiDict SENDING_STATUSES = ['created', 'pending', 'sending'] DELIVERED_STATUSES = ['delivered', 'sent'] @@ -132,21 +135,36 @@ def generate_notifications_csv(**kwargs): yield ','.join(fieldnames) + '\n' while kwargs['page']: + # if job_id then response looks different notifications_resp = notification_api_client.get_notifications_for_service(**kwargs) notifications = notifications_resp['notifications'] - for notification in notifications: - values = [ - notification['row_number'], - notification['recipient'], - notification['template_name'], - notification['template_type'], - notification['job_name'], - notification['status'], - notification['created_at'] - ] - line = ','.join(str(i) for i in values) + '\n' - yield line - + if kwargs['job_id']: + for notification in notifications: + values = [ + notification['row_number'], + notification['recipient'], + notification['template_name'], + notification['template_type'], + notification['job_name'], + notification['status'], + notification['created_at'] + ] + line = ','.join(str(i) for i in values) + '\n' + yield line + else: + # Change here + for notification in notifications: + values = [ + notification.get('row_number', None), + notification['to'], + notification['template']['name'], + notification['template']['template_type'], + notification.get('job_name', None), + notification['status'], + notification['created_at'] + ] + line = ','.join(str(i) for i in values) + '\n' + yield line if notifications_resp['links'].get('next'): kwargs['page'] += 1 else: @@ -381,3 +399,27 @@ def get_cdn_domain(): domain = parsed_uri.netloc[len(subdomain + '.'):] return "static-logos.{}".format(domain) + + +def parse_filter_args(filter_dict): + if not isinstance(filter_dict, MultiDict): + filter_dict = MultiDict(filter_dict) + + return MultiDict( + ( + key, + (','.join(filter_dict.getlist(key))).split(',') + ) + for key in filter_dict.keys() + if ''.join(filter_dict.getlist(key)) + ) + + +def set_status_filters(filter_args): + status_filters = filter_args.get('status', []) + return list(OrderedSet(chain( + (status_filters or REQUESTED_STATUSES), + DELIVERED_STATUSES if 'delivered' in status_filters else [], + SENDING_STATUSES if 'sending' in status_filters else [], + FAILURE_STATUSES if 'failed' in status_filters else [] + ))) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 980f7a6c7..4dcb2bd15 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -1,3 +1,4 @@ +import uuid from pathlib import Path from io import StringIO from collections import OrderedDict @@ -58,7 +59,8 @@ def _get_notifications_csv( @pytest.fixture(scope='function') def _get_notifications_csv_mock( mocker, - api_user_active + api_user_active, + job_id=None ): return mocker.patch( 'app.notification_api_client.get_notifications_for_service', @@ -122,13 +124,13 @@ def test_can_create_spreadsheet_from_dict_with_filename(): def test_generate_notifications_csv_returns_correct_csv_file(_get_notifications_csv_mock): - csv_content = generate_notifications_csv(service_id='1234') + csv_content = generate_notifications_csv(service_id='1234', job_id=uuid.uuid4()) csv_file = DictReader(StringIO('\n'.join(csv_content))) assert csv_file.fieldnames == ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] def test_generate_notifications_csv_only_calls_once_if_no_next_link(_get_notifications_csv_mock): - list(generate_notifications_csv(service_id='1234')) + list(generate_notifications_csv(service_id='1234', job_id=uuid.uuid4())) assert _get_notifications_csv_mock.call_count == 1 @@ -146,7 +148,7 @@ def test_generate_notifications_csv_calls_twice_if_next_link(mocker): ] ) - csv_content = generate_notifications_csv(service_id=service_id) + csv_content = generate_notifications_csv(service_id=service_id, job_id=uuid.uuid4()) csv = DictReader(StringIO('\n'.join(csv_content))) assert len(list(csv)) == 10 From e71bbc1bb595e18c2207575b3b2be6c6e93a7e0f Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 3 Jan 2018 13:21:00 +0000 Subject: [PATCH 02/12] [WIp] --- tests/app/test_utils.py | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index 4dcb2bd15..bd3fdaf8d 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -1,4 +1,3 @@ -import uuid from pathlib import Path from io import StringIO from collections import OrderedDict @@ -16,6 +15,7 @@ from app.utils import ( get_letter_timings, get_cdn_domain ) +from tests.conftest import fake_uuid def _get_notifications_csv( @@ -29,7 +29,8 @@ def _get_notifications_csv( status='Delivered', created_at='Thursday 19 April at 12:00', rows=1, - with_links=False + with_links=False, + job_id=fake_uuid ): links = {} if with_links: @@ -60,7 +61,7 @@ def _get_notifications_csv( def _get_notifications_csv_mock( mocker, api_user_active, - job_id=None + job_id=fake_uuid ): return mocker.patch( 'app.notification_api_client.get_notifications_for_service', @@ -124,13 +125,13 @@ def test_can_create_spreadsheet_from_dict_with_filename(): def test_generate_notifications_csv_returns_correct_csv_file(_get_notifications_csv_mock): - csv_content = generate_notifications_csv(service_id='1234', job_id=uuid.uuid4()) + csv_content = generate_notifications_csv(service_id='1234', job_id=fake_uuid) csv_file = DictReader(StringIO('\n'.join(csv_content))) assert csv_file.fieldnames == ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] def test_generate_notifications_csv_only_calls_once_if_no_next_link(_get_notifications_csv_mock): - list(generate_notifications_csv(service_id='1234', job_id=uuid.uuid4())) + list(generate_notifications_csv(service_id='1234', job_id=fake_uuid)) assert _get_notifications_csv_mock.call_count == 1 @@ -148,7 +149,7 @@ def test_generate_notifications_csv_calls_twice_if_next_link(mocker): ] ) - csv_content = generate_notifications_csv(service_id=service_id, job_id=uuid.uuid4()) + csv_content = generate_notifications_csv(service_id=service_id, job_id=fake_uuid) csv = DictReader(StringIO('\n'.join(csv_content))) assert len(list(csv)) == 10 From 08e5f6846153d340e2ea4cc1936e58b5eb41c19d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 12 Jan 2018 12:06:07 +0000 Subject: [PATCH 03/12] A little refactoring --- app/main/views/jobs.py | 3 +- app/utils.py | 62 ++++++++++++++++++++++-------------------- 2 files changed, 35 insertions(+), 30 deletions(-) diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 7d3be9284..8beb59f44 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -33,7 +33,8 @@ from app.utils import ( generate_notifications_csv, get_time_left, get_letter_timings, - parse_filter_args, set_status_filters) + parse_filter_args, set_status_filters +) from app.statistics_utils import add_rate_to_job diff --git a/app/utils.py b/app/utils.py index 95e881249..98f87f5bc 100644 --- a/app/utils.py +++ b/app/utils.py @@ -128,43 +128,20 @@ def get_errors_for_csv(recipients, template_type): def generate_notifications_csv(**kwargs): from app import notification_api_client + fieldnames, list_of_keys = _create_key_and_fieldnames_for_csv(kwargs) if 'page' not in kwargs: kwargs['page'] = 1 - fieldnames = ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] yield ','.join(fieldnames) + '\n' while kwargs['page']: - # if job_id then response looks different notifications_resp = notification_api_client.get_notifications_for_service(**kwargs) notifications = notifications_resp['notifications'] - if kwargs['job_id']: - for notification in notifications: - values = [ - notification['row_number'], - notification['recipient'], - notification['template_name'], - notification['template_type'], - notification['job_name'], - notification['status'], - notification['created_at'] - ] - line = ','.join(str(i) for i in values) + '\n' - yield line - else: - # Change here - for notification in notifications: - values = [ - notification.get('row_number', None), - notification['to'], - notification['template']['name'], - notification['template']['template_type'], - notification.get('job_name', None), - notification['status'], - notification['created_at'] - ] - line = ','.join(str(i) for i in values) + '\n' - yield line + + for notification in notifications: + values = [notification[x] for x in list_of_keys] + line = ','.join(str(i) for i in values) + '\n' + yield line if notifications_resp['links'].get('next'): kwargs['page'] += 1 else: @@ -172,6 +149,33 @@ def generate_notifications_csv(**kwargs): raise Exception("Should never reach here") +def _create_key_and_fieldnames_for_csv(kwargs): + # if job_id then response looks different + if kwargs['job_id']: + list_of_keys = [ + 'row_number', + 'recipient', + 'template_name', + 'template_type', + 'job_name', + 'status', + 'created_at' + ] + fieldnames = ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] + else: + list_of_keys = [ + 'recipient', + 'template_name', + 'template_type', + 'job_name', + 'status', + 'created_at', + 'updated_at' + ] + fieldnames = ['Recipient', 'Template', 'Type', 'Job', 'Status', 'Time', 'Completed at'] + return fieldnames, list_of_keys + + def get_page_from_request(): if 'page' in request.args: try: From 59ac514655e2417207fba6e0f1ab69a4c8458cab Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 12 Jan 2018 14:03:31 +0000 Subject: [PATCH 04/12] Fix bug. --- app/utils.py | 68 ++++++++++++++++++++--------------------- tests/app/test_utils.py | 10 ++++-- 2 files changed, 41 insertions(+), 37 deletions(-) diff --git a/app/utils.py b/app/utils.py index 98f87f5bc..b38b7665b 100644 --- a/app/utils.py +++ b/app/utils.py @@ -128,20 +128,47 @@ def get_errors_for_csv(recipients, template_type): def generate_notifications_csv(**kwargs): from app import notification_api_client - fieldnames, list_of_keys = _create_key_and_fieldnames_for_csv(kwargs) - if 'page' not in kwargs: kwargs['page'] = 1 + + if kwargs['job_id']: + fieldnames = ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] + else: + fieldnames = ['Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] + yield ','.join(fieldnames) + '\n' while kwargs['page']: + # if job_id then response looks different notifications_resp = notification_api_client.get_notifications_for_service(**kwargs) notifications = notifications_resp['notifications'] - - for notification in notifications: - values = [notification[x] for x in list_of_keys] - line = ','.join(str(i) for i in values) + '\n' - yield line + if kwargs['job_id']: + for notification in notifications: + values = [ + notification['row_number'], + notification['recipient'], + notification['template_name'], + notification['template_type'], + notification['job_name'], + notification['status'], + notification['created_at'] + ] + line = ','.join(str(i) for i in values) + '\n' + yield line + else: + # Change here + for notification in notifications: + values = [ + notification['to'], + notification['template']['name'], + notification['template']['template_type'], + notification.get('job_name', None), + notification['status'], + notification['created_at'], + notification['updated_at'] + ] + line = ','.join(str(i) for i in values) + '\n' + yield line if notifications_resp['links'].get('next'): kwargs['page'] += 1 else: @@ -149,33 +176,6 @@ def generate_notifications_csv(**kwargs): raise Exception("Should never reach here") -def _create_key_and_fieldnames_for_csv(kwargs): - # if job_id then response looks different - if kwargs['job_id']: - list_of_keys = [ - 'row_number', - 'recipient', - 'template_name', - 'template_type', - 'job_name', - 'status', - 'created_at' - ] - fieldnames = ['Row number', 'Recipient', 'Template', 'Type', 'Job', 'Status', 'Time'] - else: - list_of_keys = [ - 'recipient', - 'template_name', - 'template_type', - 'job_name', - 'status', - 'created_at', - 'updated_at' - ] - fieldnames = ['Recipient', 'Template', 'Type', 'Job', 'Status', 'Time', 'Completed at'] - return fieldnames, list_of_keys - - def get_page_from_request(): if 'page' in request.args: try: diff --git a/tests/app/test_utils.py b/tests/app/test_utils.py index bd3fdaf8d..6b9f3207b 100644 --- a/tests/app/test_utils.py +++ b/tests/app/test_utils.py @@ -43,12 +43,15 @@ def _get_notifications_csv( data = { 'notifications': [{ "row_number": row_number, + "to": recipient, "recipient": recipient, "template_name": template_name, "template_type": template_type, + "template": {"name": template_name, "template_type": template_type}, "job_name": job_name, "status": status, - "created_at": created_at + "created_at": created_at, + "updated_at": None } for i in range(rows)], 'total': rows, 'page_size': 50, @@ -136,7 +139,8 @@ def test_generate_notifications_csv_only_calls_once_if_no_next_link(_get_notific assert _get_notifications_csv_mock.call_count == 1 -def test_generate_notifications_csv_calls_twice_if_next_link(mocker): +@pytest.mark.parametrize("job_id", ["some", None]) +def test_generate_notifications_csv_calls_twice_if_next_link(mocker, job_id): service_id = '1234' response_with_links = _get_notifications_csv(service_id, rows=7, with_links=True) response_with_no_links = _get_notifications_csv(service_id, rows=3, with_links=False) @@ -149,7 +153,7 @@ def test_generate_notifications_csv_calls_twice_if_next_link(mocker): ] ) - csv_content = generate_notifications_csv(service_id=service_id, job_id=fake_uuid) + csv_content = generate_notifications_csv(service_id=service_id, job_id=fake_uuid if job_id else None) csv = DictReader(StringIO('\n'.join(csv_content))) assert len(list(csv)) == 10 From 135b0ad131935b79140b54a9b6412b1687949001 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Fri, 12 Jan 2018 16:15:53 +0000 Subject: [PATCH 05/12] Revert download link. There is some inconsistencies about how it is working, that needs to be solved. --- app/templates/views/notifications.html | 10 +++++----- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index a03fb4874..81c8c30d8 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -43,11 +43,11 @@ {% endif %} -

- Download this report -   - Data available for 7 days -

+ + + + + {{ ajax_block( partials, url_for('.get_notifications_as_json', service_id=current_service.id, message_type=message_type, status=status, page=page), From ebd5721d42cdbcf9fe6237e56b196fedcce7d967 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 11 Jan 2018 17:33:04 +0000 Subject: [PATCH 06/12] Correct description of how service name appears This has changed since we made prefixing text messages its own setting. --- .../views/service-settings/name.html | 15 ++++++----- tests/app/main/views/test_service_settings.py | 25 +++++++++++++------ 2 files changed, 27 insertions(+), 13 deletions(-) diff --git a/app/templates/views/service-settings/name.html b/app/templates/views/service-settings/name.html index 419cd7d03..223d079e9 100644 --- a/app/templates/views/service-settings/name.html +++ b/app/templates/views/service-settings/name.html @@ -10,13 +10,16 @@

Change your service name

-
-

Users will see your service name:

-
    -
  • at the start of every text message, eg ‘Vehicle tax: we received your payment, thank you’
    • -
  • as your email sender name
    • -
+ {% if current_service.prefix_sms %} +

Users will see your service name:

+
    +
  • at the start of every text message, eg ‘{{ current_service.name }}: This is an example message’
    • +
  • as your email sender name
    • +
+ {% else %} +

Users will see your service name as your email sender name.

+ {% endif %}
diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index fb8cb7d76..3d3ed2cb3 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -236,16 +236,27 @@ def test_escapes_letter_contact_block( def test_should_show_service_name( - logged_in_client, - service_one, + client_request, ): - response = logged_in_client.get(url_for( - 'main.service_name_change', service_id=service_one['id'])) - assert response.status_code == 200 - page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser') + page = client_request.get('main.service_name_change', service_id=SERVICE_ONE_ID) assert page.find('h1').text == 'Change your service name' assert page.find('input', attrs={"type": "text"})['value'] == 'service one' - app.service_api_client.get_service.assert_called_with(service_one['id']) + assert page.select_one('main p').text == 'Users will see your service name:' + assert normalize_spaces(page.select_one('main ul').text) == ( + 'at the start of every text message, eg ‘service one: This is an example message’ ' + 'as your email sender name' + ) + app.service_api_client.get_service.assert_called_with(SERVICE_ONE_ID) + + +def test_should_show_service_name_with_no_prefixing( + client_request, + service_one, +): + service_one['prefix_sms'] = False + page = client_request.get('main.service_name_change', service_id=SERVICE_ONE_ID) + assert page.find('h1').text == 'Change your service name' + assert page.select_one('main p').text == 'Users will see your service name as your email sender name.' def test_should_redirect_after_change_service_name( From 7386d5ec1cd9eef61762baddd0724fbb8313e186 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Thu, 11 Jan 2018 17:19:24 +0000 Subject: [PATCH 07/12] Fix 500 error from notifications/letter.json page The letters notification page makes ajax calls to update the status - these were failing because it's making a POST request without a CSRF token. (The email and SMS notification pages contain a search form that includes the hidden CSRF input, so this was only occurring with the letters page.) This commit adds a hidden form for the letters page which just contains the CSRF token. --- app/templates/views/notifications.html | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/templates/views/notifications.html b/app/templates/views/notifications.html index 81c8c30d8..c1eb2985d 100644 --- a/app/templates/views/notifications.html +++ b/app/templates/views/notifications.html @@ -42,6 +42,10 @@ + {% else %} +
+ +
{% endif %} From b3673a94a26162b7bd0e8b339fe396779fda0a3e Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 11 Jan 2018 17:20:21 +0000 Subject: [PATCH 08/12] Link to callbacks from inbound page MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit If you’ve got an inbound number and are using the API you’ll probably be interested in this new feature we’ve got. And if you do know about it, you might not be able to find it because we’ve moved it. --- app/templates/views/service-settings/set-inbound-sms.html | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/app/templates/views/service-settings/set-inbound-sms.html b/app/templates/views/service-settings/set-inbound-sms.html index 6fef29a27..bc6459783 100644 --- a/app/templates/views/service-settings/set-inbound-sms.html +++ b/app/templates/views/service-settings/set-inbound-sms.html @@ -19,6 +19,12 @@ If you want to turn this feature off, get in touch with the GOV.UK Notify team.

+ {% if current_user.has_permissions(['manage_api_keys'], admin_override=True) %} +

+ You can set up callbacks for received text messages on the + API integration page. +

+ {% endif %} {% else %}

Receiving text messages from your users is an From 67b54d850f4aa20b72b756299b07446741cefa51 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 11 Jan 2018 17:22:27 +0000 Subject: [PATCH 09/12] Add link back to API integration from callbacks Matches what we do on the API keys and whitelist pages. --- app/templates/views/api/callbacks.html | 4 +++ tests/app/main/views/test_service_settings.py | 36 +++++++++++++++++++ tests/conftest.py | 25 +++++++++++++ 3 files changed, 65 insertions(+) diff --git a/app/templates/views/api/callbacks.html b/app/templates/views/api/callbacks.html index 54a86bafe..126342b37 100644 --- a/app/templates/views/api/callbacks.html +++ b/app/templates/views/api/callbacks.html @@ -30,4 +30,8 @@ {% endcall %} {% endcall %} + {{ page_footer( + secondary_link=url_for('.api_integration', service_id=current_service.id), + secondary_link_text='Back to API integration' + ) }} {% endblock %} diff --git a/tests/app/main/views/test_service_settings.py b/tests/app/main/views/test_service_settings.py index 3d3ed2cb3..237fe1e42 100644 --- a/tests/app/main/views/test_service_settings.py +++ b/tests/app/main/views/test_service_settings.py @@ -11,6 +11,7 @@ from app.utils import email_safe from tests import validate_route_permission, service_json from tests.conftest import ( active_user_with_permissions, + active_user_no_api_key_permission, platform_admin_user, normalize_spaces, multiple_reply_to_email_addresses, @@ -1927,6 +1928,41 @@ def test_set_inbound_sms_when_inbound_number_is_not_set( assert response.status_code == 200 +@pytest.mark.parametrize('user, expected_paragraphs', [ + (active_user_with_permissions, [ + 'Your service can receive text messages sent to 07700900123.', + 'If you want to turn this feature off, get in touch with the GOV.UK Notify team.', + 'You can set up callbacks for received text messages on the API integration page.', + ]), + (active_user_no_api_key_permission, [ + 'Your service can receive text messages sent to 07700900123.', + 'If you want to turn this feature off, get in touch with the GOV.UK Notify team.', + ]), +]) +def test_set_inbound_sms_when_inbound_number_is_set( + client, + service_one, + mocker, + fake_uuid, + user, + expected_paragraphs, +): + service_one['permissions'] = ['inbound_sms'] + mocker.patch('app.inbound_number_client.get_inbound_sms_number_for_service', return_value={ + 'data': {'number': '07700900123'} + }) + client.login(user(fake_uuid), mocker, service_one) + response = client.get(url_for( + 'main.service_set_inbound_sms', service_id=SERVICE_ONE_ID + )) + paragraphs = BeautifulSoup(response.data.decode('utf-8'), 'html.parser').select('main p') + + assert len(paragraphs) == len(expected_paragraphs) + + for index, p in enumerate(expected_paragraphs): + assert normalize_spaces(paragraphs[index].text) == p + + def test_empty_letter_contact_block_returns_error( logged_in_client, service_one, diff --git a/tests/conftest.py b/tests/conftest.py index b6bd4030b..67e99d810 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1236,6 +1236,31 @@ def active_user_manage_template_permission(fake_uuid): return user +@pytest.fixture +def active_user_no_api_key_permission(fake_uuid): + from app.notify_client.user_api_client import User + + user_data = { + 'id': fake_uuid, + 'name': 'Test User With Permissions', + 'password': 'somepassword', + 'password_changed_at': str(datetime.utcnow()), + 'email_address': 'test@user.gov.uk', + 'mobile_number': '07700 900762', + 'state': 'active', + 'failed_login_count': 0, + 'permissions': {SERVICE_ONE_ID: [ + 'manage_templates', + 'manage_settings', + 'view_activity', + ]}, + 'platform_admin': False, + 'auth_type': 'sms_auth' + } + user = User(user_data) + return user + + @pytest.fixture(scope='function') def api_user_locked(fake_uuid): from app.notify_client.user_api_client import User From b4435e5b7f709218b852a7872a6cda27c74de8b0 Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Thu, 11 Jan 2018 20:26:25 +0000 Subject: [PATCH 10/12] Update pytest-xdist from 1.21.0 to 1.22.0 --- requirements_for_test.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements_for_test.txt b/requirements_for_test.txt index 7e0d0651e..5ed459a80 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -2,7 +2,7 @@ pytest==3.3.2 pytest-mock==1.6.3 pytest-cov==2.5.1 -pytest-xdist==1.21.0 +pytest-xdist==1.22.0 coveralls==1.2.0 httpretty==0.8.14 beautifulsoup4==4.6.0 From b79cd42110dd02871161b6d8cabe289fe46c7ebd Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Thu, 11 Jan 2018 21:24:17 +0000 Subject: [PATCH 11/12] Update pyexcel-io from 0.5.5 to 0.5.6 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index 6bb28a328..ef2d82f87 100644 --- a/requirements.txt +++ b/requirements.txt @@ -6,7 +6,7 @@ Flask-Login==0.4.1 boto3==1.5.12 blinker==1.4 pyexcel==0.5.6 -pyexcel-io==0.5.5 +pyexcel-io==0.5.6 pyexcel-xls==0.5.5 pyexcel-xlsx==0.5.5 pyexcel-ods3==0.5.2 From a914c2e783cbe25f815c84174de540b13fa7f2ae Mon Sep 17 00:00:00 2001 From: pyup-bot Date: Thu, 11 Jan 2018 21:38:23 +0000 Subject: [PATCH 12/12] Update pyexcel from 0.5.6 to 0.5.7 --- requirements.txt | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/requirements.txt b/requirements.txt index ef2d82f87..e7af49127 100644 --- a/requirements.txt +++ b/requirements.txt @@ -5,7 +5,7 @@ Flask-Login==0.4.1 boto3==1.5.12 blinker==1.4 -pyexcel==0.5.6 +pyexcel==0.5.7 pyexcel-io==0.5.6 pyexcel-xls==0.5.5 pyexcel-xlsx==0.5.5