From 45cacd82d3a941ec33c3e360310bb35ac2bac8d4 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Wed, 17 Feb 2016 15:49:07 +0000 Subject: [PATCH] Validate CSVs fully MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit extends the existing function to validate each row’s phone number to also validate that all the required data is present. It does this using the checking that the `Template` class can do when given a template and a `dict` of values. --- app/__init__.py | 10 ++ .../stylesheets/components/page-footer.scss | 2 +- app/main/dao/services_dao.py | 2 +- app/main/dao/templates_dao.py | 2 +- app/main/forms.py | 2 +- app/main/views/sms.py | 56 ++++----- app/templates/views/check-sms.html | 117 +++++++++--------- app/templates/views/send-sms.html | 11 +- app/{main => }/utils.py | 3 - tests/app/main/test_phone_validation.py | 2 +- tests/app/main/views/test_sms.py | 4 +- 11 files changed, 98 insertions(+), 113 deletions(-) rename app/{main => }/utils.py (98%) diff --git a/app/__init__.py b/app/__init__.py index c0c431c0b..b28fc9adc 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -19,6 +19,7 @@ from app.notify_client.job_api_client import JobApiClient from app.notify_client.status_api_client import StatusApiClient from app.its_dangerous_session import ItsdangerousSessionInterface from app.asset_fingerprinter import AssetFingerprinter +from app.utils import validate_phone_number, InvalidPhoneError import app.proxy_fix from config import configs from utils import logging @@ -65,6 +66,7 @@ def create_app(config_name, config_overrides=None): application.add_template_filter(nl2br) application.add_template_filter(format_datetime) application.add_template_filter(syntax_highlight_json) + application.add_template_filter(valid_phone_number) application.after_request(useful_headers_after_request) register_errorhandlers(application) @@ -139,6 +141,14 @@ def format_datetime(date): return native.strftime('%A %d %B %Y at %H:%M') +def valid_phone_number(phone_number): + try: + validate_phone_number(phone_number) + return True + except InvalidPhoneError: + return False + + # https://www.owasp.org/index.php/List_of_useful_HTTP_headers def useful_headers_after_request(response): response.headers.add('X-Frame-Options', 'deny') diff --git a/app/assets/stylesheets/components/page-footer.scss b/app/assets/stylesheets/components/page-footer.scss index 1df6405f1..7dfe8ece0 100644 --- a/app/assets/stylesheets/components/page-footer.scss +++ b/app/assets/stylesheets/components/page-footer.scss @@ -1,6 +1,6 @@ .page-footer { - margin-bottom: 50px; + margin-bottom: 30px; &-back-link { @include button($grey-1); diff --git a/app/main/dao/services_dao.py b/app/main/dao/services_dao.py index 5c6fd25dc..52c217679 100644 --- a/app/main/dao/services_dao.py +++ b/app/main/dao/services_dao.py @@ -1,6 +1,6 @@ from flask import url_for from app import notifications_api_client -from app.main.utils import BrowsableItem +from app.utils import BrowsableItem def insert_new_service(service_name, user_id): diff --git a/app/main/dao/templates_dao.py b/app/main/dao/templates_dao.py index 4eb05ed9d..032c45fbf 100644 --- a/app/main/dao/templates_dao.py +++ b/app/main/dao/templates_dao.py @@ -1,6 +1,6 @@ from flask import url_for, abort from app import notifications_api_client -from app.main.utils import BrowsableItem +from app.utils import BrowsableItem def insert_service_template(name, type_, content, service_id): diff --git a/app/main/forms.py b/app/main/forms.py index d97784863..cf1aa76c8 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -14,7 +14,7 @@ from wtforms.validators import DataRequired, Email, Length, Regexp from app.main.validators import Blacklist, CsvFileValidator -from app.main.utils import ( +from app.utils import ( validate_phone_number, format_phone_number, InvalidPhoneError diff --git a/app/main/views/sms.py b/app/main/views/sms.py index d191063ff..2e4154af3 100644 --- a/app/main/views/sms.py +++ b/app/main/views/sms.py @@ -20,7 +20,7 @@ from flask import ( from flask_login import login_required, current_user from werkzeug import secure_filename from notifications_python_client.errors import HTTPError -from utils.template import Template +from utils.template import Template, NeededByTemplateError, NoPlaceholderForDataError from app.main import main from app.main.forms import CsvUploadForm @@ -30,7 +30,7 @@ from app.main.uploader import ( ) from app.main.dao import templates_dao from app import job_api_client -from app.main.utils import ( +from app.utils import ( validate_phone_number, InvalidPhoneError ) @@ -68,7 +68,7 @@ def send_sms(service_id, template_id): return redirect(url_for('.send_sms', service_id=service_id, template_id=template_id)) template = Template( - templates_dao.get_service_template_or_404(service_id, template_id)['data'] + templates_dao.get_service_template_or_404(service_id, template_id)['data'] ) example_data = [dict( @@ -111,19 +111,22 @@ def check_sms(service_id, upload_id): contents = s3download(service_id, upload_id) if not contents: flash('There was a problem reading your upload file') - upload_result = _get_numbers(contents) upload_data = session['upload_data'] template_id = upload_data.get('template_id') + raw_template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] + upload_result = _get_rows(contents, raw_template) template = Template( - templates_dao.get_service_template_or_404(service_id, template_id)['data'], - values=upload_result['valid'][0] if upload_result['valid'] else {}, + raw_template, + values=upload_result['rows'][0] if upload_result['valid'] else {}, drop_values={'phone'} ) return render_template( 'views/check-sms.html', upload_result=upload_result, template=template, - column_headers=['phone number'] + template.placeholders_as_markup, + column_headers=['phone number'] + list( + template.placeholders if upload_result['valid'] else template.placeholders_as_markup + ), original_file_name=upload_data.get('original_file_name'), service_id=service_id, form=CsvUploadForm() @@ -155,36 +158,19 @@ def _get_filedata(file): return {'file_name': file.filename, 'data': lines} -def _format_filename(filename): - d = date.today() - basename, extenstion = filename.split('.') - formatted_name = '{}_{}.csv'.format(basename, d.strftime('%Y%m%d')) - return secure_filename(formatted_name) - - -def _get_numbers(contents): +def _get_rows(contents, raw_template): reader = csv.DictReader( contents.split('\n'), lineterminator='\n', - quoting=csv.QUOTE_NONE) - valid, rejects = [], [] - for i, row in enumerate(reader): + quoting=csv.QUOTE_NONE + ) + valid = True + rows = [] + for row in reader: + rows.append(row) try: validate_phone_number(row['phone']) - valid.append(row) - except InvalidPhoneError: - rejects.append({"line_number": i+2, "phone": row['phone']}) - return {"valid": valid, "rejects": rejects} - - -def _get_column_headers(template_content, marked_up=False): - headers = re.findall( - r"\(\(([^\)]+)\)\)", # anything that looks like ((registration number)) - template_content - ) - if marked_up: - return [ - Markup("{}".format(header)) - for header in headers - ] - return headers + Template(raw_template, values=row, drop_values={'phone'}).replaced + except (InvalidPhoneError, NeededByTemplateError, NoPlaceholderForDataError): + valid = False + return {"valid": valid, "rows": rows} diff --git a/app/templates/views/check-sms.html b/app/templates/views/check-sms.html index 87656b2af..c41c4759f 100644 --- a/app/templates/views/check-sms.html +++ b/app/templates/views/check-sms.html @@ -11,74 +11,73 @@ {% block maincolumn_content %} -

Check and confirm

- {% if upload_result.rejects %} -

The following numbers are invalid

- {% for rejected in upload_result.rejects %} -

Line {{rejected.line_number}}: {{rejected.phone }} - {% endfor %} -

Go back and resolve errors

+ {% if template.additional_data %} + {{ banner( + "Remove these columns from your CSV file:" + ", ".join(template.missing_data), + type="dangerous" + ) }} + {% elif not upload_result.valid %} + {{ banner( + "Your CSV file contained missing or invalid data", + type="dangerous" + ) }} + {% endif %} - {% else %} +

+ {{ "Check and confirm" if upload_result.valid else "Send text messages" }} +

-
-
- {% if template.missing_data or template.additional_data %} - {{ sms_message(template.formatted_as_markup)}} - {% else %} - {{ sms_message(template.replaced)}} - {% endif %} -
+
+
+ {% if template.missing_data or template.additional_data %} + {{ sms_message(template.formatted_as_markup)}} + {% else %} + {{ sms_message(template.replaced)}} + {% endif %}
+
- {% if template.missing_data %} - {{ banner( - "Add these columns to your CSV file: " + ", ".join(template.missing_data), - type="dangerous" + {% if upload_result.valid %} +
+ + + Back +
+ {% else %} +
+ {{file_upload(form.file, button_text='Choose a CSV file')}} + {{ page_footer( + "Upload" ) }} - {% elif template.additional_data %} - {{ banner( - "Remove these columns from your CSV file:" + ", ".join(template.missing_data), - type="dangerous" - ) }} - {% else %} - - - - Back -
- {% endif %} + + {% endif %} - {% if template.missing_data or template.additional_data %} -
- {{file_upload(form.file, button_text='Choose a CSV file')}} - {{ page_footer( - "Upload" - ) }} -
- {% endif %} - - {% call(item) list_table( - upload_result.valid, - caption=original_file_name, - field_headings=column_headers - ) %} + {% call(item) list_table( + upload_result.rows, + caption=original_file_name, + field_headings=column_headers + ) %} + {% if item.phone|valid_phone_number %} {% call field() %} {{ item.phone }} {% endcall %} - {% for column in template.placeholders %} - {% if item.get(column) %} - {% call field() %} - {{ item.get(column) }} - {% endcall %} - {% else %} - {% call field(status='missing') %} - missing - {% endcall %} - {% endif %} - {% endfor %} - {% endcall %} + {% else %} + {% call field(status='missing') %} + {{ item.phone }} + {% endcall %} + {% endif %} + {% for column in template.placeholders %} + {% if item.get(column) %} + {% call field() %} + {{ item.get(column) }} + {% endcall %} + {% else %} + {% call field(status='missing') %} + missing + {% endcall %} + {% endif %} + {% endfor %} + {% endcall %} - {% endif %} {% endblock %} diff --git a/app/templates/views/send-sms.html b/app/templates/views/send-sms.html index 74c01d724..15671cc60 100644 --- a/app/templates/views/send-sms.html +++ b/app/templates/views/send-sms.html @@ -22,13 +22,6 @@ {{file_upload(form.file, button_text='Choose a CSV file')}} - {{ banner( - 'You can only send messages to yourself until you request to go live'.format( - url_for('.service_request_to_go_live', service_id=service_id) - )|safe, - type='important' - ) }} - {{ page_footer( "Continue to preview" ) }} @@ -36,10 +29,10 @@ {% if column_headers %} {% call(item) list_table( example_data, - caption='Preview', + caption='Example', field_headings=column_headers, field_headings_visible=True, - caption_visible=False, + caption_visible=True, empty_message="Your data here" ) %} {% call field() %} diff --git a/app/main/utils.py b/app/utils.py similarity index 98% rename from app/main/utils.py rename to app/utils.py index 9b023c91d..88a127a23 100644 --- a/app/main/utils.py +++ b/app/utils.py @@ -1,6 +1,3 @@ -from flask import current_app - - class BrowsableItem(object): """ Maps for the template browse-list. diff --git a/tests/app/main/test_phone_validation.py b/tests/app/main/test_phone_validation.py index f1b54c5fb..62595d5da 100644 --- a/tests/app/main/test_phone_validation.py +++ b/tests/app/main/test_phone_validation.py @@ -1,4 +1,4 @@ -from app.main.utils import ( +from app.utils import ( validate_phone_number, InvalidPhoneError ) diff --git a/tests/app/main/views/test_sms.py b/tests/app/main/views/test_sms.py index 57af2f99e..d3609afb4 100644 --- a/tests/app/main/views/test_sms.py +++ b/tests/app/main/views/test_sms.py @@ -63,10 +63,10 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(app_, follow_redirects=True) assert response.status_code == 200 content = response.get_data(as_text=True) - assert 'The following numbers are invalid' in content + assert 'Your CSV file contained missing or invalid data' in content assert '+44 123' in content assert '+44 456' in content - assert 'Go back and resolve errors' in content + assert 'Choose a CSV file' in content @moto.mock_s3