From 33d9a461db65820246103c56fd6544f2e64b9074 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 1 Apr 2016 14:31:28 +0100 Subject: [PATCH 1/4] Put two rows in example CSV file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit We reckon that this will help people understand that the first row is headers and the subsequent rows are the users’ data. --- app/main/views/send.py | 35 +++++++++++++++++++------------ tests/app/main/views/test_send.py | 2 +- 2 files changed, 23 insertions(+), 14 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 40622b360..0fba061ae 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -49,6 +49,18 @@ def get_page_headings(template_type): }[template_type] +def get_example_csv_rows(template): + return [ + [ + { + 'email': current_user.email_address, + 'sms': current_user.mobile_number + }[template.template_type] + ] + _get_fake_personalisation(template.placeholders, i) + for i in range(1, 3) + ] + + @main.route("/services//send/", methods=['GET']) @login_required @user_has_permissions('view_activity', @@ -140,16 +152,13 @@ def get_example_csv(service_id, template_id): # code may assume its always safe when it might not be. output = io.StringIO() writer = csv.writer(output) - writer.writerow( - [first_column_heading[template.template_type]] + - list(template.placeholders) + writer.writerows( + [ + [first_column_heading[template.template_type]] + + list(template.placeholders) + ] + + get_example_csv_rows(template) ) - writer.writerow([ - { - 'email': current_user.email_address, - 'sms': current_user.mobile_number - }[template.template_type] - ] + _get_fake_personalisation(template.placeholders)) return output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'} @@ -170,11 +179,11 @@ def send_message_to_self(service_id, template_id): ) if template.template_type == 'sms': writer.writerow( - [current_user.mobile_number] + _get_fake_personalisation(template.placeholders) + [current_user.mobile_number] + _get_fake_personalisation(template.placeholders, 1) ) if template.template_type == 'email': writer.writerow( - [current_user.email_address] + _get_fake_personalisation(template.placeholders) + [current_user.email_address] + _get_fake_personalisation(template.placeholders, 1) ) filedata = { @@ -301,7 +310,7 @@ def start_job(service_id, upload_id): ) -def _get_fake_personalisation(placeholders): +def _get_fake_personalisation(placeholders, index): return [ - "{} 1".format(header) for header in placeholders + "{} {}".format(header, index) for header in placeholders ] diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 520ccc5be..12afc7884 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -129,7 +129,7 @@ def test_download_example_csv( follow_redirects=True ) assert response.status_code == 200 - assert response.get_data(as_text=True) == 'phone number\r\n+4412341234\r\n' + assert response.get_data(as_text=True) == 'phone number\r\n+4412341234\r\n+4412341234\r\n' assert 'text/csv' in response.headers['Content-Type'] From 5fd5544a8c178424d957952bcfd1fd882c4d72bd Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Sun, 3 Apr 2016 11:30:23 +0100 Subject: [PATCH 2/4] Use context managers for StringIO MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s possible that leaving a StringIO stream open could cause a memory leak. Using a context manager closes it automatically when it’s finished with. --- app/main/views/send.py | 66 +++++++++++++++++++----------------------- 1 file changed, 30 insertions(+), 36 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 0fba061ae..9c90a2f83 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -146,20 +146,16 @@ def send_messages(service_id, template_id): @user_has_permissions('send_texts', 'send_emails', 'send_letters', 'manage_templates', any_=True) def get_example_csv(service_id, template_id): template = Template(service_api_client.get_service_template(service_id, template_id)['data']) - # Good practice to use context managers - # http://stackoverflow.com/questions/9718950/do-i-have-to-do-stringio-close - # For this instance it may not be a problem but someone else looking at the - # code may assume its always safe when it might not be. - output = io.StringIO() - writer = csv.writer(output) - writer.writerows( - [ - [first_column_heading[template.template_type]] + - list(template.placeholders) - ] + - get_example_csv_rows(template) - ) - return output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'} + with io.StringIO() as output: + writer = csv.writer(output) + writer.writerows( + [ + [first_column_heading[template.template_type]] + + list(template.placeholders) + ] + + get_example_csv_rows(template) + ) + return output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'} @main.route("/services//send//to-self", methods=['GET']) @@ -167,29 +163,27 @@ def get_example_csv(service_id, template_id): @user_has_permissions('send_texts', 'send_emails', 'send_letters') def send_message_to_self(service_id, template_id): template = Template(service_api_client.get_service_template(service_id, template_id)['data']) - # Good practice to use context managers - # http://stackoverflow.com/questions/9718950/do-i-have-to-do-stringio-close - # For this instance it may not be a problem but someone else looking at the - # code may assume its always safe when it might not be. - output = io.StringIO() - writer = csv.writer(output) - writer.writerow( - [first_column_heading[template.template_type]] + - list(template.placeholders) - ) - if template.template_type == 'sms': - writer.writerow( - [current_user.mobile_number] + _get_fake_personalisation(template.placeholders, 1) - ) - if template.template_type == 'email': - writer.writerow( - [current_user.email_address] + _get_fake_personalisation(template.placeholders, 1) - ) - filedata = { - 'file_name': 'Test run', - 'data': output.getvalue() - } + with io.StringIO() as output: + writer = csv.writer(output) + writer.writerow( + [first_column_heading[template.template_type]] + + list(template.placeholders) + ) + if template.template_type == 'sms': + writer.writerow( + [current_user.mobile_number] + _get_fake_personalisation(template.placeholders, 1) + ) + if template.template_type == 'email': + writer.writerow( + [current_user.email_address] + _get_fake_personalisation(template.placeholders, 1) + ) + + filedata = { + 'file_name': 'Test run', + 'data': output.getvalue() + } + upload_id = str(uuid.uuid4()) s3upload(upload_id, service_id, filedata, current_app.config['AWS_REGION']) From f84797e6d538d1aec15c0ce324c8b76563236df7 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 1 Apr 2016 15:36:35 +0100 Subject: [PATCH 3/4] =?UTF-8?q?Show=20an=20example=20of=20a=20CSV=20on=20t?= =?UTF-8?q?he=20=E2=80=98send=E2=80=99=20page?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This should help: - understanding of what the CSV should contain - understanding what ‘download example’ will do --- app/main/views/send.py | 42 ++++++++------------------- app/templates/views/send.html | 53 +++++++++++++++++++---------------- 2 files changed, 41 insertions(+), 54 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 9c90a2f83..976e87f43 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -49,15 +49,17 @@ def get_page_headings(template_type): }[template_type] -def get_example_csv_rows(template): +def get_example_csv_rows(template, number_of_rows=2): return [ [ { 'email': current_user.email_address, 'sms': current_user.mobile_number }[template.template_type] - ] + _get_fake_personalisation(template.placeholders, i) - for i in range(1, 3) + ] + [ + "{} {}".format(header, i) for header in template.placeholders + ] + for i in range(1, number_of_rows + 1) ] @@ -135,6 +137,7 @@ def send_messages(service_id, template_id): 'views/send.html', template=template, recipient_column=first_column_heading[template.template_type], + example=get_example_csv_rows(template), form=form, service=service, service_id=service_id @@ -144,7 +147,7 @@ def send_messages(service_id, template_id): @main.route("/services//send/.csv", methods=['GET']) @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters', 'manage_templates', any_=True) -def get_example_csv(service_id, template_id): +def get_example_csv(service_id, template_id, number_of_rows=2): template = Template(service_api_client.get_service_template(service_id, template_id)['data']) with io.StringIO() as output: writer = csv.writer(output) @@ -153,7 +156,7 @@ def get_example_csv(service_id, template_id): [first_column_heading[template.template_type]] + list(template.placeholders) ] + - get_example_csv_rows(template) + get_example_csv_rows(template, number_of_rows=number_of_rows) ) return output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'} @@ -164,25 +167,10 @@ def get_example_csv(service_id, template_id): def send_message_to_self(service_id, template_id): template = Template(service_api_client.get_service_template(service_id, template_id)['data']) - with io.StringIO() as output: - writer = csv.writer(output) - writer.writerow( - [first_column_heading[template.template_type]] + - list(template.placeholders) - ) - if template.template_type == 'sms': - writer.writerow( - [current_user.mobile_number] + _get_fake_personalisation(template.placeholders, 1) - ) - if template.template_type == 'email': - writer.writerow( - [current_user.email_address] + _get_fake_personalisation(template.placeholders, 1) - ) - - filedata = { - 'file_name': 'Test run', - 'data': output.getvalue() - } + filedata = { + 'file_name': 'Test run', + 'data': get_example_csv(service_id, template_id, number_of_rows=1)[0] + } upload_id = str(uuid.uuid4()) @@ -302,9 +290,3 @@ def start_job(service_id, upload_id): return redirect( url_for('main.view_job', service_id=service_id, job_id=upload_id) ) - - -def _get_fake_personalisation(placeholders, index): - return [ - "{} {}".format(header, index) for header in placeholders - ] diff --git a/app/templates/views/send.html b/app/templates/views/send.html index d41c715cf..bedd1282f 100644 --- a/app/templates/views/send.html +++ b/app/templates/views/send.html @@ -3,7 +3,7 @@ {% from "components/email-message.html" import email_message %} {% from "components/page-footer.html" import page_footer %} {% from "components/file-upload.html" import file_upload %} -{% from "components/table.html" import list_table, field %} +{% from "components/table.html" import list_table, text_field %} {% block page_title %} Send text messages – GOV.UK Notify @@ -11,7 +11,7 @@ {% block maincolumn_content %} -

Send from a CSV file

+

Send a batch

{% if 'sms' == template.template_type %}
@@ -28,28 +28,33 @@ ) }} {% endif %} -
-
-

- You need - {{ template.placeholders|length + 1 }} - {% if template.placeholders %} - columns - {% else %} - column - {% endif %} - in your file: -

-

- {{ recipient_column }} - {{ template.placeholders_as_markup|join(" ") }} -

-

- Download an example -

-
-
+

+ You need + {{ template.placeholders|length + 1 }} + {% if template.placeholders %} + columns + {% else %} + column + {% endif %} + in your file, like this: +

+ + {% call(item) list_table( + example, + caption="Example", + caption_visible=False, + field_headings=[recipient_column] + template.placeholders|list + ) %} + {% for column in item %} + {{ text_field(column) }} + {% endfor %} + {% endcall %} + +

+ Download this example +

+ {{file_upload(form.file, button_text='Upload your CSV file')}} -{% endblock %} +{% endblock %} \ No newline at end of file From 94e8b2a685906e14e57dacc3b536b2a28dbac19a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Fri, 1 Apr 2016 15:38:53 +0100 Subject: [PATCH 4/4] Use template name as file name for example CSV `11.csv` or `9a1d8a7f-c202-43b9-9239-68939d34fec6.csv` is not very friendly. `Two week reminder.csv` is much nicer. --- app/main/views/send.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 976e87f43..6c30a43b1 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -158,7 +158,10 @@ def get_example_csv(service_id, template_id, number_of_rows=2): ] + get_example_csv_rows(template, number_of_rows=number_of_rows) ) - return output.getvalue(), 200, {'Content-Type': 'text/csv; charset=utf-8'} + return output.getvalue(), 200, { + 'Content-Type': 'text/csv; charset=utf-8', + 'Content-Disposition': 'inline; filename="{}.csv"'.format(template.name) + } @main.route("/services//send//to-self", methods=['GET'])