From c9b2211bd349e24b64c095bfc781cd0028fdf724 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 17 Oct 2017 11:41:12 +0100 Subject: [PATCH 1/2] Let users download a CSV of inbound messages MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit In user research, we’ve seen users copy/pasting the contents of the inbound SMS page into a spreadsheet, in order to keep a record of the messages they receive. They even went as far as to write a macro which fixed the errors caused by copying and pasting. It would be much easier if we just gave them the data already in a spreadsheet format. Which is what this commit does. One caveat is that, because spreadsheets can contain executable code (ie formulas), and because we’re populating the spreadsheet with user-submitted data (albeit via SMS) we need to be careful about injection attacks. The details of how these attacks work are detailed here (interesting reading): http://georgemauer.net/2017/10/07/csv-injection.html The mitigation is to not allow characters which initialise a formula at the start of the cell. --- app/__init__.py | 17 ++++- app/assets/stylesheets/_grids.scss | 5 ++ app/main/views/dashboard.py | 32 ++++++++- .../views/dashboard/_inbox_messages.html | 5 ++ tests/app/main/views/test_dashboard.py | 69 +++++++++++++++++++ 5 files changed, 126 insertions(+), 2 deletions(-) diff --git a/app/__init__.py b/app/__init__.py index 0f3645d6c..4d08261e8 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -231,7 +231,7 @@ def format_datetime(date): def format_datetime_24h(date): return '{} at {}'.format( format_date(date), - gmt_timezones(date).strftime('%H:%M') + format_time_24h(date), ) @@ -256,6 +256,21 @@ def format_datetime_relative(date): ) +def format_datetime_numeric(date): + return '{} {}'.format( + format_date_numeric(date), + format_time_24h(date), + ) + + +def format_date_numeric(date): + return gmt_timezones(date).strftime('%Y-%m-%d') + + +def format_time_24h(date): + return gmt_timezones(date).strftime('%H:%M') + + def get_human_day(time): # Add 1 hour to get ‘midnight today’ instead of ‘midnight tomorrow’ diff --git a/app/assets/stylesheets/_grids.scss b/app/assets/stylesheets/_grids.scss index 4fbd20f8f..52c71c5b7 100644 --- a/app/assets/stylesheets/_grids.scss +++ b/app/assets/stylesheets/_grids.scss @@ -39,6 +39,11 @@ margin-top: $gutter * 4 / 3; } +.top-gutter-2-3 { + @extend %top-gutter; + margin-top: $gutter-half; +} + %bottom-gutter, .bottom-gutter { @extend %contain-floats; diff --git a/app/main/views/dashboard.py b/app/main/views/dashboard.py index 78e231fdf..028bc2469 100644 --- a/app/main/views/dashboard.py +++ b/app/main/views/dashboard.py @@ -6,7 +6,8 @@ from flask import ( session, jsonify, request, - abort + abort, + Response, ) from flask_login import login_required @@ -20,6 +21,9 @@ from app import ( service_api_client, template_statistics_client, inbound_number_client, + format_datetime_short, + format_date_numeric, + format_datetime_numeric, ) from app.statistics_utils import get_formatted_percentage, add_rate_to_job from app.utils import ( @@ -27,6 +31,7 @@ from app.utils import ( get_current_financial_year, FAILURE_STATUSES, REQUESTED_STATUSES, + Spreadsheet, ) @@ -161,6 +166,31 @@ def inbox_updates(service_id): return jsonify(get_inbox_partials(service_id)) +@main.route("/services//inbox.csv") +@login_required +@user_has_permissions('view_activity', admin_override=True) +def inbox_download(service_id): + return Response( + Spreadsheet.from_rows( + [[ + 'Phone number', + 'Message', + 'Received', + ]] + [[ + message['user_number'], + message['content'].lstrip(('=+-@')), + format_datetime_numeric(message['created_at']), + ] for message in service_api_client.get_inbound_sms(service_id)] + ).as_csv_data, + mimetype='text/csv', + headers={ + 'Content-Disposition': 'inline; filename="Received text messages {}.csv"'.format( + format_date_numeric(datetime.utcnow().isoformat()) + ) + } + ) + + def get_inbox_partials(service_id): if 'inbound_sms' not in current_service['permissions']: diff --git a/app/templates/views/dashboard/_inbox_messages.html b/app/templates/views/dashboard/_inbox_messages.html index b0ca019dc..b46b16a8c 100644 --- a/app/templates/views/dashboard/_inbox_messages.html +++ b/app/templates/views/dashboard/_inbox_messages.html @@ -2,6 +2,11 @@ {% from "components/message-count-label.html" import message_count_label %}
+ {% if messages %} +

+ Download these messages +

+ {% endif %} {% call(item, row_number) list_table( messages, caption="Inbox", diff --git a/tests/app/main/views/test_dashboard.py b/tests/app/main/views/test_dashboard.py index fddcd33ec..15f2cfd52 100644 --- a/tests/app/main/views/test_dashboard.py +++ b/tests/app/main/views/test_dashboard.py @@ -7,6 +7,7 @@ from flask import url_for import pytest from bs4 import BeautifulSoup from freezegun import freeze_time +from datetime import datetime from app.main.views.dashboard import ( get_dashboard_totals, @@ -161,6 +162,10 @@ def test_inbox_showing_inbound_messages( assert normalize_spaces(page.select('.table-show-more-link')) == ( '8 messages from 5 users' ) + assert page.select_one('a[download]')['href'] == url_for( + 'main.inbox_download', + service_id=SERVICE_ONE_ID, + ) def test_empty_inbox( @@ -184,6 +189,7 @@ def test_empty_inbox( assert normalize_spaces(page.select('tbody tr')) == ( 'When users text your service’s phone number (0781239871) you’ll see the messages here' ) + assert not page.select('a[download]') @pytest.mark.parametrize('endpoint', [ @@ -246,6 +252,69 @@ def test_view_inbox_updates( mock_get_partials.assert_called_once_with(SERVICE_ONE_ID) +@freeze_time("2016-07-01 13:00") +def test_download_inbox( + logged_in_client, + mock_get_inbound_sms, +): + response = logged_in_client.get( + url_for('main.inbox_download', service_id=SERVICE_ONE_ID) + ) + assert response.status_code == 200 + assert response.headers['Content-Type'] == ( + 'text/csv; ' + 'charset=utf-8' + ) + assert response.headers['Content-Disposition'] == ( + 'inline; ' + 'filename="Received text messages 2016-07-01.csv"' + ) + assert response.get_data(as_text=True) == ( + 'Phone number,Message,Received\r\n' + '07900900000,message-1,2016-07-01 13:00\r\n' + '07900900000,message-2,2016-07-01 12:59\r\n' + '07900900000,message-3,2016-07-01 12:59\r\n' + '07900900002,message-4,2016-07-01 10:59\r\n' + '07900900004,message-5,2016-07-01 08:59\r\n' + '07900900006,message-6,2016-07-01 06:59\r\n' + '07900900008,message-7,2016-07-01 04:59\r\n' + '07900900008,message-8,2016-07-01 04:59\r\n' + ) + + +@freeze_time("2016-07-01 13:00") +@pytest.mark.parametrize('message_content, expected_cell', [ + ('=2+5', '2+5'), + ('==2+5', '2+5'), + ('-2+5', '2+5'), + ('+2+5', '2+5'), + ('@2+5', '2+5'), + ('looks safe,=2+5', '"looks safe,=2+5"'), +]) +def test_download_inbox_strips_formulae( + mocker, + logged_in_client, + fake_uuid, + message_content, + expected_cell, +): + + mocker.patch( + 'app.service_api_client.get_inbound_sms', + return_value=[{ + 'user_number': 'elevenchars', + 'notify_number': 'foo', + 'content': message_content, + 'created_at': datetime.utcnow().isoformat(), + 'id': fake_uuid, + }], + ) + response = logged_in_client.get( + url_for('main.inbox_download', service_id=SERVICE_ONE_ID) + ) + assert expected_cell in response.get_data(as_text=True).split('\r\n')[1] + + def test_should_show_recent_templates_on_dashboard( logged_in_client, mocker, From c096397390e8c6f6a0db283167e887acdfa5dd90 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 17 Oct 2017 12:00:51 +0100 Subject: [PATCH 2/2] Truncate inbound message content MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Inbound text messages can run over multiple lines. This makes the page harder to scan. Your phone, and the outbound messages page, only show the first line of the text message, and truncate the rest with an ellipsis. This commit does the same for inbound text messages. It also stops the timestamp for the inbound messages being squashed and wrapping over multiple lines, which looks messy. We couldn’t do this before, because it would have stopped people from being able to copy/paste the full message content from this page. --- app/assets/stylesheets/views/dashboard.scss | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/app/assets/stylesheets/views/dashboard.scss b/app/assets/stylesheets/views/dashboard.scss index c3bc7b9b7..9115cacf0 100644 --- a/app/assets/stylesheets/views/dashboard.scss +++ b/app/assets/stylesheets/views/dashboard.scss @@ -67,6 +67,10 @@ display: block; color: $secondary-text-colour; pointer-events: none; + overflow: hidden; + white-space: nowrap; + text-overflow: ellipsis; + max-width: 580px; } }