From 0a5bf0bc442ea5216fd3dd1c397440868211b74a Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Mar 2016 09:54:53 +0000 Subject: [PATCH 1/4] Update to utils 2.0.0 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit …and remove the code from this app that has moved into utils. --- app/utils.py | 78 ------------------------------------------------ requirements.txt | 2 +- 2 files changed, 1 insertion(+), 79 deletions(-) diff --git a/app/utils.py b/app/utils.py index 96bf0a8fd..bdaa9d5f0 100644 --- a/app/utils.py +++ b/app/utils.py @@ -32,84 +32,6 @@ class BrowsableItem(object): pass -class InvalidEmailError(Exception): - def __init__(self, message): - self.message = message - - -class InvalidPhoneError(Exception): - def __init__(self, message): - self.message = message - - -class InvalidHeaderError(Exception): - def __init__(self, message): - self.message = message - - -def validate_phone_number(number): - sanitised_number = number.replace('(', '') - sanitised_number = sanitised_number.replace(')', '') - sanitised_number = sanitised_number.replace(' ', '') - sanitised_number = sanitised_number.replace('-', '') - - if sanitised_number.startswith('+'): - sanitised_number = sanitised_number[1:] - - valid_prefixes = ['07', '447', '4407', '00447'] - if not sum(sanitised_number.startswith(prefix) for prefix in valid_prefixes): - raise InvalidPhoneError('Must be a UK mobile number (eg 07700 900460)') - - for digit in sanitised_number: - try: - int(digit) - except(ValueError): - raise InvalidPhoneError('Must not contain letters or symbols') - - # Split number on first 7 - sanitised_number = sanitised_number.split('7', 1)[1] - - if len(sanitised_number) > 9: - raise InvalidPhoneError('Too many digits') - - if len(sanitised_number) < 9: - raise InvalidPhoneError('Not enough digits') - - return sanitised_number - - -def format_phone_number(number): - import re - if len(number) > 9: - raise InvalidPhoneError('Too many digits') - - if len(number) < 9: - raise InvalidPhoneError('Not enough digits') - return '+447{}{}{}'.format(*re.findall('...', number)) - - -def validate_email_address(email_address): - if re.match(r"(^[a-zA-Z0-9_.+-]+@[a-zA-Z0-9-]+\.[a-zA-Z0-9-.]+$)", email_address): - return - raise InvalidEmailError('Not a valid email address') - - -def validate_recipient(row, template_type): - validate_header_row(row, template_type) - return { - 'email': validate_email_address, - 'sms': validate_phone_number - }[template_type](get_recipient_from_row(row, template_type)) - - -def validate_header_row(row, template_type): - try: - column_heading = first_column_heading[template_type] - row[column_heading] - except KeyError as e: - raise InvalidHeaderError('Invalid header name, should be {}'.format(column_heading)) - - def user_has_permissions(*permissions, or_=False): def wrap(func): @wraps(func) diff --git a/requirements.txt b/requirements.txt index 7a0048d08..85ed0e61d 100644 --- a/requirements.txt +++ b/requirements.txt @@ -14,4 +14,4 @@ Pygments==2.0.2 git+https://github.com/alphagov/notifications-python-client.git@0.3.1#egg=notifications-python-client==0.3.1 -git+https://github.com/alphagov/notifications-utils.git@1.0.0#egg=notifications-utils==1.0.0 +git+https://github.com/alphagov/notifications-utils.git@2.0.0#egg=notifications-utils==2.0.0 From eb3734f1d18d478a2d235a41d4fbf8c10381ddd0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 7 Mar 2016 18:47:05 +0000 Subject: [PATCH 2/4] Give the user better error messages for CSV files Makes uses of the additions to utils in https://github.com/alphagov/notifications-utils/pull/9 This commit strips out a lot of the complex stuff that the views and templates in this app were doing. There is now a cleaner separation of concerns: - utils returns the number and type of errors in the csv - `get_errors_for_csv` helper in this app maps the number and type of errors onto human-friendly error messages - the view and template just doing the glueing-together of all the pieces This is (hopefully) easier to understand, definitely makes the component parts easier to test in isolation, and makes it easier to give more specific error messages. --- app/__init__.py | 2 +- app/assets/stylesheets/components/banner.scss | 12 ++ app/assets/stylesheets/components/table.scss | 31 ++- app/main/forms.py | 2 +- app/main/uploader.py | 2 +- app/main/views/send.py | 193 ++++++++---------- app/templates/components/banner.html | 4 + app/templates/components/file-upload.html | 2 +- app/templates/views/check.html | 104 ++++++---- app/utils.py | 43 +++- tests/app/main/test_errors_for_csv.py | 79 +++++++ .../{test_utils.py => test_permissions.py} | 10 +- tests/app/main/views/test_register.py | 2 +- tests/app/main/views/test_send.py | 109 +++------- 14 files changed, 340 insertions(+), 255 deletions(-) create mode 100644 tests/app/main/test_errors_for_csv.py rename tests/app/main/{test_utils.py => test_permissions.py} (89%) diff --git a/app/__init__.py b/app/__init__.py index 94a1c1d86..8db5ca6d1 100644 --- a/app/__init__.py +++ b/app/__init__.py @@ -19,7 +19,7 @@ from app.notify_client.status_api_client import StatusApiClient from app.notify_client.invite_api_client import InviteApiClient from app.its_dangerous_session import ItsdangerousSessionInterface from app.asset_fingerprinter import AssetFingerprinter -from app.utils import validate_phone_number, InvalidPhoneError +from utils.recipients import validate_phone_number, InvalidPhoneError import app.proxy_fix from config import configs from utils import logging diff --git a/app/assets/stylesheets/components/banner.scss b/app/assets/stylesheets/components/banner.scss index 77b706779..884523bdd 100644 --- a/app/assets/stylesheets/components/banner.scss +++ b/app/assets/stylesheets/components/banner.scss @@ -12,6 +12,18 @@ position: relative; clear: both; + &-title { + @include bold-24; + } + + p { + margin: 10px 0 5px 0; + } + + .list-bullet { + @include copy-19; + } + } .banner-with-tick, diff --git a/app/assets/stylesheets/components/table.scss b/app/assets/stylesheets/components/table.scss index 36c137bcc..8a37c677e 100644 --- a/app/assets/stylesheets/components/table.scss +++ b/app/assets/stylesheets/components/table.scss @@ -16,10 +16,26 @@ %table-field, .table-field { + vertical-align: top; + &:last-child { padding-right: 0; } + &-error { + + border-left: 5px solid $error-colour; + padding-left: 7px; + display: block; + + &-label { + display: block; + color: $error-colour; + font-weight: bold; + } + + } + &-status { &-default { @@ -55,13 +71,6 @@ background-image: file-url('tick.png'); } - &-missing { - color: $error-colour; - font-weight: bold; - border-left: 5px solid $error-colour; - padding-left: 7px; - } - } } @@ -92,9 +101,15 @@ } .table-show-more-link { - @include bold-16; + @include core-16; + color: $secondary-text-colour; margin-top: -20px; border-bottom: 1px solid $border-colour; padding-bottom: 10px; text-align: center; } + +a.table-show-more-link { + @include bold-16; + color: $link-colour; +} diff --git a/app/main/forms.py b/app/main/forms.py index ad9036e1c..33e4d1c0c 100644 --- a/app/main/forms.py +++ b/app/main/forms.py @@ -16,7 +16,7 @@ from wtforms.validators import DataRequired, Email, Length, Regexp from app.main.validators import Blacklist, CsvFileValidator -from app.utils import ( +from utils.recipients import ( validate_phone_number, format_phone_number, InvalidPhoneError diff --git a/app/main/uploader.py b/app/main/uploader.py index bfecb75b8..3dfd4d87b 100644 --- a/app/main/uploader.py +++ b/app/main/uploader.py @@ -8,7 +8,7 @@ BUCKET_NAME = 'service-{}-notify' def s3upload(upload_id, service_id, filedata, region): s3 = resource('s3') bucket_name = BUCKET_NAME.format(service_id) - contents = '\n'.join(filedata['data']) + contents = filedata['data'] exists = True try: diff --git a/app/main/views/send.py b/app/main/views/send.py index 3784ba7c2..8125502db 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -1,6 +1,7 @@ import csv import io import uuid +from contextlib import suppress from flask import ( request, @@ -15,7 +16,8 @@ from flask import ( from flask_login import login_required, current_user from notifications_python_client.errors import HTTPError -from utils.template import Template, NeededByTemplateError, NoPlaceholderForDataError +from utils.template import Template +from utils.recipients import RecipientCSV, first_column_heading from app.main import main from app.main.forms import CsvUploadForm @@ -26,10 +28,7 @@ from app.main.uploader import ( from app.main.dao import templates_dao from app.main.dao import services_dao from app import job_api_client -from app.utils import ( - validate_recipient, validate_header_row, InvalidPhoneError, InvalidEmailError, user_has_permissions, - InvalidHeaderError) -from utils.process_csv import first_column_heading +from app.utils import user_has_permissions, get_errors_for_csv send_messages_page_headings = { @@ -100,16 +99,25 @@ def send_messages(service_id, template_id): form = CsvUploadForm() if form.validate_on_submit(): try: - csv_file = form.file - filedata = _get_filedata(csv_file) upload_id = str(uuid.uuid4()) - s3upload(upload_id, service_id, filedata, current_app.config['AWS_REGION']) - session['upload_data'] = {"template_id": template_id, "original_file_name": filedata['file_name']} + s3upload( + upload_id, + service_id, + { + 'file_name': form.file.data.filename, + 'data': form.file.data.getvalue().decode('utf-8') + }, + current_app.config['AWS_REGION'] + ) + session['upload_data'] = { + "template_id": template_id, + "original_file_name": form.file.data.filename + } return redirect(url_for('.check_messages', service_id=service_id, upload_id=upload_id)) except ValueError as e: - flash('There was a problem uploading: {}'.format(csv_file.data.filename)) + flash('There was a problem uploading: {}'.format(form.file.data.filename)) flash(str(e)) return redirect(url_for('.send_messages', service_id=service_id, template_id=template_id)) @@ -118,7 +126,6 @@ def send_messages(service_id, template_id): templates_dao.get_service_template_or_404(service_id, template_id)['data'], prefix=service['name'] ) - recipient_column = first_column_heading[template.template_type] return render_template( 'views/send.html', @@ -174,7 +181,7 @@ def send_message_to_self(service_id, template_id): filedata = { 'file_name': 'Test run', - 'data': output.getvalue().splitlines() + 'data': output.getvalue() } upload_id = str(uuid.uuid4()) s3upload(upload_id, service_id, filedata, current_app.config['AWS_REGION']) @@ -185,107 +192,81 @@ def send_message_to_self(service_id, template_id): upload_id=upload_id)) -@main.route("/services//check/", - methods=['GET', 'POST']) +@main.route("/services//check/", methods=['GET']) @login_required @user_has_permissions('send_texts', 'send_emails', 'send_letters') def check_messages(service_id, upload_id): - upload_data = session['upload_data'] - template_id = upload_data.get('template_id') service = services_dao.get_service_by_id_or_404(service_id) - if request.method == 'GET': - contents = s3download(service_id, upload_id) - if not contents: - flash('There was a problem reading your upload file') - raw_template = templates_dao.get_service_template_or_404(service_id, template_id)['data'] - upload_result = _get_rows(contents, raw_template) - session['upload_data']['notification_count'] = len(upload_result['rows']) - template = Template( - raw_template, - values=upload_result['rows'][0] if upload_result['valid'] else {}, - drop_values={first_column_heading[raw_template['template_type']]}, - prefix=service['name'] - ) - return render_template( - 'views/check.html', - upload_result=upload_result, - template=template, - page_heading=get_page_headings(template.template_type), - column_headers=[first_column_heading[template.template_type]] + list(template.placeholders_as_markup), - original_file_name=upload_data.get('original_file_name'), - service_id=service_id, - service=service, - form=CsvUploadForm() - ) - elif request.method == 'POST': - if request.files: - # The csv was invalid, validate the csv again - return send_messages(service_id, template_id) + contents = s3download(service_id, upload_id) + if not contents: + flash('There was a problem reading your upload file') - original_file_name = upload_data.get('original_file_name') - notification_count = upload_data.get('notification_count') - session.pop('upload_data') - try: - job_api_client.create_job(upload_id, service_id, template_id, original_file_name, notification_count) - except HTTPError as e: - if e.status_code == 404: - abort(404) - else: - raise e - - return redirect( - url_for('main.view_job', service_id=service_id, job_id=upload_id) - ) - - -def _get_filedata(file): - import itertools - reader = csv.reader( - file.data.getvalue().decode('utf-8').splitlines(), - quoting=csv.QUOTE_NONE, - skipinitialspace=True + template = Template( + templates_dao.get_service_template_or_404( + service_id, + session['upload_data'].get('template_id') + )['data'], + prefix=service['name'] ) - lines = [] - for row in reader: - non_empties = itertools.dropwhile(lambda x: x.strip() == '', row) - has_content = [] - for item in non_empties: - has_content.append(item) - if has_content: - lines.append(row) - if len(lines) < 2: # must be header row and at least one data row - message = 'The file {} contained no data'.format(file.data.filename) - raise ValueError(message) - - content_lines = [] - for row in lines: - content_lines.append(','.join(row).rstrip(',')) - return {'file_name': file.data.filename, 'data': content_lines} - - -def _get_rows(contents, raw_template): - reader = csv.DictReader( - contents.split('\n'), - quoting=csv.QUOTE_NONE, - skipinitialspace=True + recipients = RecipientCSV( + contents, + template_type=template.template_type, + placeholders=template.placeholders, + max_initial_rows_shown=5 + ) + + with suppress(StopIteration): + template.values = next(recipients.rows) + + session['upload_data']['notification_count'] = len(list(recipients.rows)) + session['upload_data']['valid'] = not recipients.has_errors + + return render_template( + 'views/check.html', + recipients=recipients, + template=template, + page_heading=get_page_headings(template.template_type), + errors=get_errors_for_csv(recipients, template.template_type), + count_of_recipients=session['upload_data']['notification_count'], + count_of_displayed_recipients=len(list(recipients.rows_annotated_and_truncated)), + original_file_name=session['upload_data'].get('original_file_name'), + service_id=service_id, + service=service, + form=CsvUploadForm() + ) + + +@main.route("/services//check/", methods=['POST']) +@login_required +@user_has_permissions('send_texts', 'send_emails', 'send_letters') +def start_job(service_id, upload_id): + + upload_data = session['upload_data'] + services_dao.get_service_by_id_or_404(service_id) + + if request.files or not session['upload_data'].get('valid'): + # The csv was invalid, validate the csv again + return send_messages(service_id, upload_data.get('template_id')) + + session.pop('upload_data') + + try: + job_api_client.create_job( + upload_id, + service_id, + upload_data.get('template_id'), + upload_data.get('original_file_name'), + upload_data.get('notification_count') + ) + except HTTPError as e: + if e.status_code == 404: + abort(404) + else: + raise e + + return redirect( + url_for('main.view_job', service_id=service_id, job_id=upload_id) ) - valid = True - rows = [] - for row in reader: - rows.append(row) - try: - validate_recipient( - row, template_type=raw_template['template_type'] - ) - Template( - raw_template, - values=row, - drop_values={first_column_heading[raw_template['template_type']]} - ).replaced - except (InvalidEmailError, InvalidPhoneError, NeededByTemplateError, - NoPlaceholderForDataError, InvalidHeaderError): - valid = False - return {"valid": valid, "rows": rows} diff --git a/app/templates/components/banner.html b/app/templates/components/banner.html index 74402779e..0c0ef7abc 100644 --- a/app/templates/components/banner.html +++ b/app/templates/components/banner.html @@ -22,3 +22,7 @@ {% endif %} {% endmacro %} + +{% macro banner_wrapper(type=None, with_tick=False, delete_button=None, subhead=None) %} + {{ banner(caller()|safe, type=type, with_tick=with_tick, delete_button=delete_button, subhead=subhead) }} +{% endmacro %} diff --git a/app/templates/components/file-upload.html b/app/templates/components/file-upload.html index 0d9e65fb9..6dcb22a2e 100644 --- a/app/templates/components/file-upload.html +++ b/app/templates/components/file-upload.html @@ -1,5 +1,5 @@ {% macro file_upload(field, button_text="Choose file") %} -
+