Merge pull request #241 from alphagov/utils-csv-processing

Give the user better error messages for CSV files

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

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,

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;
+}

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
@@ -37,19 +37,10 @@ class UKMobileNumber(TelField):
 
     def pre_validate(self, form):
         try:
-            self.data = validate_phone_number(self.data)
+            validate_phone_number(self.data)
         except InvalidPhoneError as e:
             raise ValidationError(e.message)
 
-    def post_validate(self, form, validation_stopped):
-
-        if len(self.data) != 9:
-            return
-        # TODO implement in the render field method.
-        # API's require no spaces in the number
-        # self.data = '+44 7{} {} {}'.format(*re.findall('...', self.data))
-        self.data = format_phone_number(self.data)
-
 
 def mobile_number():
     return UKMobileNumber('Mobile phone number',

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:

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 (
+from app.utils import user_has_permissions, get_errors_for_csv
-    validate_recipient, validate_header_row, InvalidPhoneError, InvalidEmailError, user_has_permissions,
-    InvalidHeaderError)
-from utils.process_csv import first_column_heading
 
 
 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'])
+            s3upload(
-            session['upload_data'] = {"template_id": template_id, "original_file_name": filedata['file_name']}
+                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,75 @@ def send_message_to_self(service_id, template_id):
                             upload_id=upload_id))
 
 
-@main.route("/services/<service_id>/check/<upload_id>",
+@main.route("/services/<service_id>/check/<upload_id>", methods=['GET'])
-            methods=['GET', 'POST'])
 @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)
-        contents = s3download(service_id, upload_id)
+    if not contents:
-        if not contents:
+        flash('There was a problem reading your upload file')
-            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)
 
-        original_file_name = upload_data.get('original_file_name')
+    template = Template(
-        notification_count = upload_data.get('notification_count')
+        templates_dao.get_service_template_or_404(
-        session.pop('upload_data')
+            service_id,
-        try:
+            session['upload_data'].get('template_id')
-            job_api_client.create_job(upload_id, service_id, template_id, original_file_name, notification_count)
+        )['data'],
-        except HTTPError as e:
+        prefix=service['name']
-            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
     )
-    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
+    recipients = RecipientCSV(
-        message = 'The file {} contained no data'.format(file.data.filename)
+        contents,
-        raise ValueError(message)
+        template_type=template.template_type,
-
+        placeholders=template.placeholders,
-    content_lines = []
+        max_initial_rows_shown=5
-    for row in lines:
+    )
-        content_lines.append(','.join(row).rstrip(','))
+
-    return {'file_name': file.data.filename, 'data': content_lines}
+    with suppress(StopIteration):
-
+        template.values = next(recipients.rows)
-
+
-def _get_rows(contents, raw_template):
+    session['upload_data']['notification_count'] = len(list(recipients.rows))
-    reader = csv.DictReader(
+    session['upload_data']['valid'] = not recipients.has_errors
-        contents.split('\n'),
+
-        quoting=csv.QUOTE_NONE,
+    return render_template(
-        skipinitialspace=True
+        '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/<service_id>/check/<upload_id>", 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 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')
+
+    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')
+    )
+
+    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}

app/templates/components/banner.html

@@ -22,3 +22,7 @@
     {% endif %}
   </div>
 {% 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 %}

app/templates/components/file-upload.html

@@ -1,5 +1,5 @@
 {% macro file_upload(field, button_text="Choose file") %}
-  <form method="post" enctype="multipart/form-data" class="form-group{% if field.errors %} error{% endif %}" data-module="file-upload">
+  <form method="post" enctype="multipart/form-data" class="{% if field.errors %}error{% endif %}" data-module="file-upload">
     <label class="file-upload-label" for="{{ field.name }}">
       <span class="visually-hidden">{{ field.label }}</span>
       {% if hint %}

app/templates/views/check.html

@@ -1,38 +1,53 @@
 {% extends "withnav_template.html" %}
+{% from "components/banner.html" import banner_wrapper %}
 {% from "components/email-message.html" import email_message %}
 {% from "components/sms-message.html" import sms_message %}
-{% from "components/table.html" import list_table, field %}
+{% from "components/table.html" import list_table, field, text_field, hidden_field_heading %}
 {% from "components/placeholder.html" import placeholder %}
 {% from "components/file-upload.html" import file_upload %}
 {% from "components/page-footer.html" import page_footer %}
 
 {% block page_title %}
-  {{ "Check and confirm" if upload_result.valid else page_heading }} – GOV.UK Notify
+  {{ page_heading if errors else "Check and confirm" }} – GOV.UK Notify
 {% endblock %}
 
 {% block maincolumn_content %}
 
-
+  {% if errors %}
-  {% if template.additional_data %}
+    <div class="bottom-gutter">
-    {{ banner(
+      {% call banner_wrapper(type='dangerous') %}
-      "Remove these columns from your CSV file:" + ", ".join(template.missing_data),
+        {% if errors|length == 1 %}
-      type="dangerous"
+          <h1 class='banner-title'>
-    ) }}
+            There was a problem with {{ original_file_name }}
-  {% elif not upload_result.valid %}
+          </h1>
-    {{ banner(
+          <p>
-      "Your CSV file contained missing or invalid data",
+            You need to {{ errors[0] }}
-      type="dangerous"
+          </p>
-    ) }}
+        {% else %}
+          <h1 class='banner-title'>
+            There were some problems with {{ original_file_name }}
+          </h1>
+          <p>
+            You need to:
+          </p>
+          <ul class="list-bullet">
+            {% for error in errors %}
+              <li>{{ error }}</li>
+            {% endfor %}
+          </ul>
+        {% endif %}
+      {% endcall %}
+    </div>
+  {% else %}
+    <h1 class="heading-large">
+      Check and confirm
+    </h1>
   {% endif %}
 
-  <h1 class="heading-large">
-    {{ "Check and confirm" if upload_result.valid else page_heading }}
-  </h1>
-
   {% if 'email' == template.template_type %}
     {{ email_message(
       template.subject,
-      template.replaced if upload_result.valid else template.formatted_as_markup,
+      template.formatted_as_markup if errors else template.replaced,
       from_address='{}@notifications.service.gov.uk'.format(service.email_from),
       from_name=service.name
     )}}
@@ -40,51 +55,52 @@
     <div class="grid-row">
       <div class="column-two-thirds">
         {{ sms_message(
-          template.replaced if upload_result.valid else template.formatted_as_markup
+          template.formatted_as_markup if errors else template.replaced
         )}}
       </div>
     </div>
   {% endif %}
 
-  {% if upload_result.valid %}
+  {% if errors %}
+    {{file_upload(form.file, button_text='Re-upload your file')}}
+  {% else %}
     <form method="post" enctype="multipart/form-data">
       <input type="hidden" name="csrf_token" value="{{ csrf_token() }}" />
-      <input type="submit" class="button" value="{{ "Send {} message{}".format(upload_result.rows|count, '' if upload_result.rows|count == 1 else 's') }}" />
+      <input type="submit" class="button" value="{{ "Send {} message{}".format(count_of_recipients, '' if count_of_recipients == 1 else 's') }}" />
       <a href="{{url_for('.send_messages', service_id=service_id, template_id=template.id)}}" class="page-footer-back-link">Back</a>
     </form>
-  {% else %}
-    {{file_upload(form.file, button_text='Upload a CSV file')}}
   {% endif %}
 
   {% call(item) list_table(
-    upload_result.rows,
+    recipients.rows_annotated_and_truncated,
     caption=original_file_name,
-    field_headings=column_headers
+    field_headings=['Row'] + recipients.column_headers_with_placeholders_highlighted
   ) %}
-    {% if item.get('phone number', '')|valid_phone_number %}
+    {% call field() %}
-      {% call field() %}
+      {{ item.index + 1 }}
-        {{ item['phone number'] }}
+    {% endcall %}
-      {% endcall %}
+    {% for column in recipients.column_headers %}
-    {% elif item.get('email address') %}
+      {% if item[column].error %}
-      {% call field() %}
-        {{ item['email address'] }}
-      {% endcall %}
-    {% else %}
-      {% call field(status='missing') %}
-        {{ item['phone number'] }}
-      {% endcall %}
-    {% endif %}
-    {% for column in template.placeholders %}
-      {% if item.get(column) %}
         {% call field() %}
-          {{ item.get(column) }}
+          <span class="table-field-error">
+            <span class="table-field-error-label">{{ item[column].error }}</span>
+            {{ item[column].data if item[column].data != None }}
+          </span>
+        {% endcall %}
+      {% elif item[column].ignore %}
+        {% call field(status='default') %}
+          {{ item[column].data if item[column].data != None }}
         {% endcall %}
       {% else %}
-        {% call field(status='missing') %}
+        {{ text_field(item[column].data) }}
-          missing
-        {% endcall %}
       {% endif %}
     {% endfor %}
   {% endcall %}
 
+  {% if count_of_displayed_recipients < count_of_recipients %}
+    <p class="table-show-more-link">
+      {{ count_of_recipients - count_of_displayed_recipients }} more {{ "row" if 1 == (count_of_recipients - count_of_displayed_recipients) else "rows"}} not shown
+    </p>
+  {% endif %}
+
 {% endblock %}

app/utils.py

@@ -3,8 +3,6 @@ import re
 from functools import wraps
 from flask import (abort, session)
 
-from utils.process_csv import get_recipient_from_row, first_column_heading
-
 
 class BrowsableItem(object):
     """
@@ -32,84 +30,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)
@@ -121,3 +41,44 @@ def user_has_permissions(*permissions, or_=False):
                 abort(403)
         return wrap_func
     return wrap
+
+
+def get_errors_for_csv(recipients, template_type):
+
+    errors = []
+
+    missing_column_headers = list(recipients.missing_column_headers)
+
+    if len(missing_column_headers) == 1:
+        errors.append("add a column called ‘{}’".format("".join(missing_column_headers)))
+    elif len(missing_column_headers) == 2:
+        errors.append("add 2 columns, ‘{}’".format("’ and ‘".join(missing_column_headers)))
+    elif len(missing_column_headers) > 2:
+        errors.append(
+            "add columns called ‘{}’, and ‘{}’".format(
+                "’, ‘".join(missing_column_headers[0:-1]),
+                missing_column_headers[-1]
+            )
+        )
+
+    if recipients.rows_with_bad_recipients:
+        number_of_bad_recipients = len(list(recipients.rows_with_bad_recipients))
+        if 'sms' == template_type:
+            if 1 == number_of_bad_recipients:
+                errors.append("fix 1 phone number")
+            else:
+                errors.append("fix {} phone numbers".format(number_of_bad_recipients))
+        elif 'email' == template_type:
+            if 1 == number_of_bad_recipients:
+                errors.append("fix 1 email address")
+            else:
+                errors.append("fix {} email addresses".format(number_of_bad_recipients))
+
+    if recipients.rows_with_missing_data:
+        number_of_rows_with_missing_data = len(list(recipients.rows_with_missing_data))
+        if 1 == number_of_rows_with_missing_data:
+            errors.append("fill in 1 empty cell")
+        else:
+            errors.append("fill in {} empty cells".format(number_of_rows_with_missing_data))
+
+    return errors

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

tests/app/main/test_errors_for_csv.py

@@ -0,0 +1,79 @@
+from collections import namedtuple
+
+import pytest
+
+from app.utils import get_errors_for_csv
+
+
+MockRecipients = namedtuple(
+    'RecipientCSV',
+    [
+        'missing_column_headers',
+        'rows_with_bad_recipients',
+        'rows_with_missing_data'
+    ]
+)
+
+
+@pytest.mark.parametrize(
+    "missing_column_headers,rows_with_bad_recipients,rows_with_missing_data,template_type,expected_errors",
+    [
+        (
+            [], [], [],
+            'sms',
+            []
+        ),
+        (
+            [], {2}, [],
+            'sms',
+            ['fix 1 phone number']
+        ),
+        (
+            [], {2, 4, 6}, [],
+            'sms',
+            ['fix 3 phone numbers']
+        ),
+        (
+            [], {1}, [],
+            'email',
+            ['fix 1 email address']
+        ),
+        (
+            [], {2, 4, 6}, [],
+            'email',
+            ['fix 3 email addresses']
+        ),
+        (
+            ['name'], {2}, {3},
+            'sms',
+            [
+                'add a column called ‘name’',
+                'fix 1 phone number',
+                'fill in 1 empty cell'
+            ]
+        ),
+        (
+            ['name', 'date'], [], [],
+            'sms',
+            ['add 2 columns, ‘name’ and ‘date’']
+        ),
+        (
+            ['name', 'date', 'time'], {2, 4, 6, 8}, {3, 6, 9, 12},
+            'sms',
+            [
+                'add columns called ‘name’, ‘date’, and ‘time’',
+                'fix 4 phone numbers',
+                'fill in 4 empty cells'
+            ]
+        )
+    ]
+)
+def test_get_errors_for_csv(
+    missing_column_headers, rows_with_bad_recipients, rows_with_missing_data,
+    template_type,
+    expected_errors
+):
+    assert get_errors_for_csv(
+        MockRecipients(missing_column_headers, rows_with_bad_recipients, rows_with_missing_data),
+        template_type
+    ) == expected_errors

tests/app/main/test_permissions.py

@@ -1,7 +1,7 @@
 import pytest
 from flask import url_for
 
-from app.utils import user_has_permissions, validate_header_row, validate_recipient, InvalidHeaderError
+from app.utils import user_has_permissions
 from app.main.views.index import index
 from werkzeug.exceptions import Forbidden
 
@@ -68,11 +68,3 @@ def test_exact_permissions(app_,
             decorator = user_has_permissions('manage_users', 'manage_templates', 'manage_settings')
             decorated_index = decorator(index)
             response = decorated_index()
-
-
-def test_validate_header_row():
-    row = {'bad': '+44 7700 900981'}
-    try:
-        validate_header_row(row, 'sms')
-    except InvalidHeaderError as e:
-        assert e.message == 'Invalid header name, should be phone number'

tests/app/main/test_phone_number_form_field.py

@@ -1,75 +0,0 @@
-import pytest
-from wtforms import Form
-from app.main.forms import UKMobileNumber
-
-
-class FormExample(Form):
-    phone_number = UKMobileNumber()
-
-invalid_phone_numbers = sum([
-    [
-        (phone_number, error) for phone_number in group
-    ] for error, group in [
-        ('Too many digits', (
-            '0712345678910',
-            '0044712345678910',
-            '0044712345678910',
-            '+44 (0)7123 456 789 10',
-        )),
-        ('Not enough digits', (
-            '0712345678',
-            '004471234567',
-            '00447123456',
-            '+44 (0)7123 456 78',
-        )),
-        ('Must be a UK mobile number (eg 07700 900460)', (
-            '08081 570364',
-            '+44 8081 570364',
-            '0117 496 0860',
-            '+44 117 496 0860',
-            '020 7946 0991',
-            '+44 20 7946 0991',
-            '71234567890',
-        )),
-        ('Must not contain letters or symbols', (
-            '07890x32109',
-            '07123 456789...',
-            '07123 ☟☜⬇⬆☞☝',
-            '07123☟☜⬇⬆☞☝',
-            '07";DROP TABLE;"',
-            '+44 07ab cde fgh',
-        ))
-    ]
-], [])
-
-valid_phone_numbers = [
-    '07123456789',
-    '07123 456789',
-    '07123-456-789',
-    '00447123456789',
-    '00 44 7123456789',
-    '+447123456789',
-    '+44 7123 456 789',
-    '+44 (0)7123 456 789'
-]
-
-
-@pytest.mark.parametrize("phone_number", valid_phone_numbers)
-def test_phone_number_accepts_valid_values(phone_number):
-    form = FormExample(phone_number=phone_number)
-    form.validate()
-    assert form.errors == {}
-
-
-@pytest.mark.parametrize("phone_number, error_message", invalid_phone_numbers)
-def test_phone_number_rejects_invalid_values(phone_number, error_message):
-    form = FormExample(phone_number=phone_number)
-    form.validate()
-    assert form.phone_number.errors[0] == error_message
-
-
-@pytest.mark.parametrize("phone_number", valid_phone_numbers)
-def test_phone_number_outputs_in_correct_format(phone_number):
-    form = FormExample(phone_number=phone_number)
-    form.validate()
-    assert form.phone_number.data == '+447123456789'

tests/app/main/test_phone_validation.py

@@ -1,68 +0,0 @@
-from app.utils import (
-    validate_phone_number,
-    InvalidPhoneError
-)
-
-import pytest
-
-valid_phone_numbers = [
-    '07123456789',
-    '07123 456789',
-    '07123-456-789',
-    '00447123456789',
-    '00 44 7123456789',
-    '+447123456789',
-    '+44 7123 456 789',
-    '+44 (0)7123 456 789'
-]
-
-invalid_phone_numbers = sum([
-    [
-        (phone_number, error) for phone_number in group
-    ] for error, group in [
-        ('Too many digits', (
-            '0712345678910',
-            '0044712345678910',
-            '0044712345678910',
-            '+44 (0)7123 456 789 10',
-        )),
-        ('Not enough digits', (
-            '0712345678',
-            '004471234567',
-            '00447123456',
-            '+44 (0)7123 456 78',
-        )),
-        ('Must be a UK mobile number (eg 07700 900460)', (
-            '08081 570364',
-            '+44 8081 570364',
-            '0117 496 0860',
-            '+44 117 496 0860',
-            '020 7946 0991',
-            '+44 20 7946 0991',
-            '71234567890',
-        )),
-        ('Must not contain letters or symbols', (
-            '07890x32109',
-            '07123 456789...',
-            '07123 ☟☜⬇⬆☞☝',
-            '07123☟☜⬇⬆☞☝',
-            '07";DROP TABLE;"',
-            '+44 07ab cde fgh',
-        ))
-    ]
-], [])
-
-
-@pytest.mark.parametrize("phone_number", valid_phone_numbers)
-def test_phone_number_accepts_valid_values(phone_number):
-    try:
-        validate_phone_number(phone_number)
-    except InvalidPhoneError:
-        pytest.fail('Unexpected InvalidPhoneError')
-
-
-@pytest.mark.parametrize("phone_number, error_message", invalid_phone_numbers)
-def test_phone_number_rejects_invalid_values(phone_number, error_message):
-    with pytest.raises(InvalidPhoneError) as e:
-        validate_phone_number(phone_number)
-    assert error_message == str(e.value)

tests/app/main/views/test_register.py

@@ -55,7 +55,7 @@ def test_process_register_returns_400_when_mobile_number_is_invalid(app_,
                                                  'password': 'validPassword!'})
 
     assert response.status_code == 200
-    assert 'Must be a UK mobile number (eg 07700 900460)' in response.get_data(as_text=True)
+    assert 'Must not contain letters or symbols' in response.get_data(as_text=True)
 
 
 def test_should_return_400_when_email_is_not_gov_uk(app_,

tests/app/main/views/test_send.py

@@ -33,33 +33,7 @@ def test_choose_template(
         assert '{} template two content'.format(template_type) in content
 
 
-def test_upload_empty_csvfile_returns_to_upload_page(
+def test_upload_csvfile_with_errors_shows_check_page_with_errors(
-    app_,
-    api_user_active,
-    mock_login,
-    mock_get_user,
-    mock_get_service,
-    mock_get_service_templates,
-    mock_check_verify_code,
-    mock_get_service_template,
-    mock_has_permissions
-):
-    with app_.test_request_context():
-        with app_.test_client() as client:
-            client.login(api_user_active)
-            upload_data = {'file': (BytesIO(''.encode('utf-8')), 'emtpy.csv')}
-            response = client.post(
-                url_for('main.send_messages', service_id=12345, template_id=54321),
-                data=upload_data,
-                follow_redirects=True
-            )
-
-        assert response.status_code == 200
-        content = response.get_data(as_text=True)
-        assert 'The file emtpy.csv contained no data' in content
-
-
-def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(
     app_,
     api_user_active,
     mocker,
@@ -85,43 +59,11 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(
             )
         assert response.status_code == 200
         content = response.get_data(as_text=True)
-        assert 'Your CSV file contained missing or invalid data' in content
+        assert 'There was a problem with invalid.csv' in content
         assert '+44 123' in content
         assert '+44 456' in content
-        assert 'Upload a CSV file' in content
+        assert 'Not a UK mobile number' in content
-
+        assert 'Re-upload your file' in content
-
-def test_upload_csvfile_removes_empty_lines_and_trailing_commas(
-    app_,
-    api_user_active,
-    mocker,
-    mock_login,
-    mock_get_service,
-    mock_get_service_template,
-    mock_s3_upload,
-    mock_has_permissions
-):
-
-    contents = 'phone number,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n'
-    file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv')
-
-    expected_data = {'data': ['phone number,name', '++44 7700 900981,test1', '+44 7700 900981,test2'],
-                     'file_name': 'invalid.csv'}
-
-    mocker.patch('app.main.views.send.s3download',
-                 return_value='phone number,name\n++44 7700 900981,test1\n+44 7700 900981,test2')
-
-    with app_.test_request_context():
-        with app_.test_client() as client:
-            client.login(api_user_active)
-            upload_data = {'file': file_data}
-            response = client.post(
-                url_for('main.send_messages', service_id=12345, template_id=54321),
-                data=upload_data,
-                follow_redirects=True
-            )
-        assert response.status_code == 200
-        mock_s3_upload.assert_called_with(ANY, '12345', expected_data, 'eu-west-1')
 
 
 def test_send_test_message_to_self(
@@ -160,7 +102,7 @@ def test_send_test_message_to_self(
     mock_has_permissions
 ):
 
-    expected_data = {'data': ['email address', 'test@user.gov.uk'], 'file_name': 'Test run'}
+    expected_data = {'data': 'email address\r\ntest@user.gov.uk\r\n', 'file_name': 'Test run'}
     mocker.patch('app.main.views.send.s3download', return_value='email address\r\ntest@user.gov.uk')
 
     with app_.test_request_context():
@@ -207,31 +149,32 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers(
     mock_has_permissions
 ):
 
-    contents = 'phone number\n+44 7700 900981\n+44 7700 900982\n+44 7700 900983\n+44 7700 900984\n+44 7700 900985\n+44 7700 900986'  # noqa
+    mocker.patch(
-    file_data = (BytesIO(contents.encode('utf-8')), 'valid.csv')
+        'app.main.views.send.s3download',
-    mocker.patch('app.main.views.send.s3download', return_value=contents)
+        return_value='phone number\n+44 7700 900981\n+44 7700 900982\n+44 7700 900983\n+44 7700 900984\n+44 7700 900985\n+44 7700 900986'  # noqa
+    )
 
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(api_user_active)
-            upload_data = {'file': file_data}
+            response = client.post(
-            response = client.post(url_for('main.send_messages', service_id=12345, template_id=54321),
+                url_for('main.send_messages', service_id=12345, template_id=54321),
-                                   data=upload_data,
+                data={'file': (BytesIO(), 'valid.csv')},
-                                   follow_redirects=True)
+                follow_redirects=True
+            )
             with client.session_transaction() as sess:
                 assert int(sess['upload_data']['template_id']) == 54321
                 assert sess['upload_data']['original_file_name'] == 'valid.csv'
                 assert sess['upload_data']['notification_count'] == 6
 
             content = response.get_data(as_text=True)
-
             assert response.status_code == 200
             assert '+44 7700 900981' in content
             assert '+44 7700 900982' in content
             assert '+44 7700 900983' in content
             assert '+44 7700 900984' in content
             assert '+44 7700 900985' in content
-            assert '+44 7700 900986' in content
+            assert '1 more row not shown' in content
 
 
 def test_create_job_should_call_api(
@@ -260,8 +203,9 @@ def test_create_job_should_call_api(
             with client.session_transaction() as session:
                 session['upload_data'] = {'original_file_name': original_file_name,
                                           'template_id': template_id,
-                                          'notification_count': notification_count}
+                                          'notification_count': notification_count,
-            url = url_for('main.check_messages', service_id=service_one['id'], upload_id=job_id)
+                                          'valid': True}
+            url = url_for('main.start_job', service_id=service_one['id'], upload_id=job_id)
             response = client.post(url, data=job_data, follow_redirects=True)
 
         assert response.status_code == 200
@@ -284,11 +228,11 @@ def test_check_messages_should_revalidate_file_when_uploading_file(
 ):
 
     service_id = service_one['id']
-    contents = 'phone number,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n'
-    file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv')
-    upload_data = {'file': file_data}
 
-    mocker.patch('app.main.views.send.s3download', return_value=contents)
+    mocker.patch(
+        'app.main.views.send.s3download',
+        return_value='phone number,name,,,\n++44 7700 900981,test1,,,\n+44 7700 900981,test2,,,\n ,,, \n ,,, \t \t \n'
+    )
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(api_user_active)
@@ -296,7 +240,10 @@ def test_check_messages_should_revalidate_file_when_uploading_file(
                 session['upload_data'] = {'original_file_name': 'invalid.csv',
                                           'template_id': job_data['template'],
                                           'notification_count': job_data['notification_count']}
-            url = url_for('main.check_messages', service_id=service_id, upload_id=job_data['id'])
+            response = client.post(
-            response = client.post(url, data=upload_data, follow_redirects=True)
+                url_for('main.check_messages', service_id=service_id, upload_id=job_data['id']),
+                data={'file': (BytesIO(), 'invalid.csv')},
+                follow_redirects=True
+            )
             assert response.status_code == 200
-            assert 'Your CSV file contained missing or invalid data' in response.get_data(as_text=True)
+            assert 'There was a problem with invalid.csv' in response.get_data(as_text=True)