Merge pull request #565 from alphagov/accept-common-spreadsheet-formats

Accept common spreadsheet formats, not just CSV

app/main/validators.py

@@ -1,6 +1,7 @@
 import re
 from wtforms import ValidationError
 from notifications_utils.template import Template
+from app.utils import Spreadsheet
 
 
 class Blacklist(object):
@@ -20,8 +21,8 @@ class CsvFileValidator(object):
         self.message = message
 
     def __call__(self, form, field):
-        if not field.data.filename.lower().endswith('.csv'):
+        if not Spreadsheet.can_handle(field.data.filename):
-            raise ValidationError("{} is not a CSV file".format(field.data.filename))
+            raise ValidationError("{} isn’t a spreadsheet that Notify can read".format(field.data.filename))
 
 
 class ValidEmailDomainRegex(object):

app/main/views/send.py

@@ -4,6 +4,8 @@ import json
 import uuid
 import itertools
 from contextlib import suppress
+from zipfile import BadZipFile
+from xlrd.biffh import XLRDError
 
 from flask import (
     request,
@@ -27,7 +29,7 @@ from app.main.uploader import (
     s3download
 )
 from app import job_api_client, service_api_client, current_service, user_api_client, statistics_api_client
-from app.utils import user_has_permissions, get_errors_for_csv
+from app.utils import user_has_permissions, get_errors_for_csv, Spreadsheet
 
 
 def get_send_button_text(template_type, number_of_messages):
@@ -112,10 +114,7 @@ def send_messages(service_id, template_id):
             s3upload(
                 upload_id,
                 service_id,
-                {
+                Spreadsheet.from_file(form.file.data.filename, form.file.data).as_dict,
-                    'file_name': form.file.data.filename,
-                    'data': form.file.data.read().decode('utf-8')
-                },
                 current_app.config['AWS_REGION']
             )
             session['upload_data'] = {
@@ -126,10 +125,10 @@ def send_messages(service_id, template_id):
                                     service_id=service_id,
                                     upload_id=upload_id,
                                     template_type=template.template_type))
-        except ValueError as e:
+        except (UnicodeDecodeError, BadZipFile, XLRDError):
-            flash('There was a problem uploading: {}'.format(form.file.data.filename))
+            flash('Couldn’t read {}. Try using a different file format.'.format(
-            flash(str(e))
+                form.file.data.filename
-            return redirect(url_for('.send_messages', service_id=service_id, template_id=template_id))
+            ))
 
     return render_template(
         'views/send.html',

app/utils.py

@@ -1,8 +1,14 @@
 import re
 import csv
-from io import StringIO
+from io import BytesIO, StringIO
+from os import path
 from functools import wraps
 from flask import (abort, session, request, url_for)
+import pyexcel
+import pyexcel.ext.io
+import pyexcel.ext.xls
+import pyexcel.ext.xlsx
+import pyexcel.ext.ods3
 
 
 class BrowsableItem(object):
@@ -131,3 +137,51 @@ def email_safe(string):
     return "".join([
         character.lower() if character.isalnum() or character == "." else "" for character in re.sub("\s+", ".", string.strip())  # noqa
     ])
+
+
+class Spreadsheet():
+
+    allowed_file_extensions = ['csv', 'xlsx', 'xls', 'ods', 'xlsm', 'tsv']
+
+    def __init__(self, filename, csv_data):
+        self.filename = filename
+        self.as_csv_data = csv_data
+        self.as_dict = {
+            'file_name': self.filename,
+            'data': self.as_csv_data
+        }
+
+    @classmethod
+    def can_handle(cls, filename):
+        return cls.get_extension(filename) in cls.allowed_file_extensions
+
+    @staticmethod
+    def get_extension(filename):
+        return path.splitext(filename)[1].lower().lstrip('.')
+
+    @staticmethod
+    def normalise_newlines(file_content):
+        return '\r\n'.join(file_content.getvalue().decode('utf-8').splitlines())
+
+    @classmethod
+    def from_file(cls, filename, file_content):
+
+        extension = cls.get_extension(filename)
+
+        if extension == 'csv':
+            return cls(filename, Spreadsheet.normalise_newlines(file_content))
+
+        if extension == 'tsv':
+            file_content = StringIO(Spreadsheet.normalise_newlines(file_content))
+
+        with StringIO() as converted:
+
+            output = csv.writer(converted)
+
+            for row in pyexcel.get_sheet(
+                file_type=extension,
+                file_content=file_content.getvalue()
+            ).to_array():
+                output.writerow(row)
+
+            return cls(filename, converted.getvalue())

requirements.txt

@@ -8,6 +8,11 @@ Pygments==2.0.2
 py-gfm==0.1.2
 blinker==1.4
 monotonic==0.3
+pyexcel==0.2.1
+pyexcel-io==0.1.0
+pyexcel-xls==0.1.0
+pyexcel-xlsx==0.1.0
+pyexcel-ods3==0.1.1
 
 git+https://github.com/alphagov/notifications-python-client.git@1.0.0#egg=notifications-python-client==1.0.0
 

tests/app/main/views/test_send.py

@@ -1,6 +1,9 @@
 import pytest
 import re
+from itertools import repeat
 from io import BytesIO
+from os import path
+from glob import glob
 from bs4 import BeautifulSoup
 from flask import url_for
 from unittest.mock import ANY
@@ -8,6 +11,60 @@ from tests import validate_route_permission, job_json_with_created_by
 
 template_types = ['email', 'sms']
 
+# The * ignores hidden files, eg .DS_Store
+test_spreadsheet_files = glob(path.join('tests', 'spreadsheet_files', '*'))
+test_non_spreadsheet_files = glob(path.join('tests', 'non_spreadsheet_files', '*'))
+
+
+def test_that_test_files_exist():
+    assert len(test_spreadsheet_files) == 8
+    assert len(test_non_spreadsheet_files) == 6
+
+
+@pytest.mark.parametrize(
+    "filename, acceptable_file",
+    list(zip(
+        test_spreadsheet_files, repeat(True)
+    )) +
+    list(zip(
+        test_non_spreadsheet_files, repeat(False)
+    ))
+)
+def test_upload_files_in_different_formats(
+    filename,
+    acceptable_file,
+    app_,
+    api_user_active,
+    mocker,
+    mock_login,
+    mock_get_service,
+    mock_get_service_template,
+    mock_s3_upload,
+    mock_has_permissions,
+    fake_uuid
+):
+
+    with app_.test_request_context(), app_.test_client() as client, open(filename, 'rb') as uploaded:
+        client.login(api_user_active)
+        response = client.post(
+            url_for('main.send_messages', service_id=fake_uuid, template_id=fake_uuid),
+            data={'file': (BytesIO(uploaded.read()), filename)},
+            content_type='multipart/form-data'
+        )
+
+    if acceptable_file:
+        assert mock_s3_upload.call_args[0][2]['data'].strip() == (
+            "phone number,name,favourite colour,fruit\r\n"
+            "07739 468 050,Pete,Coral,tomato\r\n"
+            "07527 125 974,Not Pete,Magenta,Avacado\r\n"
+            "07512 058 823,Still Not Pete,Crimson,Pear"
+        )
+    else:
+        assert not mock_s3_upload.called
+        assert (
+            'Couldn’t read {}. Try using a different file format.'.format(filename)
+        ) in response.get_data(as_text=True)
+
 
 def test_upload_csvfile_with_errors_shows_check_page_with_errors(
     app_,
@@ -61,20 +118,18 @@ def test_upload_csv_invalid_extension(app_,
                                       mock_get_users_by_service,
                                       mock_get_service_statistics,
                                       fake_uuid):
-    contents = u'phone number,name\n+44 123,test1\n+44 456,test2'
     with app_.test_request_context():
-        filename = 'invalid.txt'
         with app_.test_client() as client:
             client.login(api_user_active)
             resp = client.post(
                 url_for('main.send_messages', service_id=fake_uuid, template_id=fake_uuid),
-                data={'file': (BytesIO(contents.encode('utf-8')), filename)},
+                data={'file': (BytesIO('contents'.encode('utf-8')), 'invalid.txt')},
                 content_type='multipart/form-data',
                 follow_redirects=True
             )
 
         assert resp.status_code == 200
-        assert "{} is not a CSV file".format(filename) in resp.get_data(as_text=True)
+        assert "invalid.txt isn’t a spreadsheet that Notify can read" in resp.get_data(as_text=True)
 
 
 def test_send_test_sms_message(

tests/spreadsheet_files/newline_unix.csv

@@ -0,0 +1,4 @@
+phone number,name,favourite colour,fruit
+07739 468 050,Pete,Coral,tomato
+07527 125 974,Not Pete,Magenta,Avacado
+07512 058 823,Still Not Pete,Crimson,Pear

tests/spreadsheet_files/newline_windows.csv

@@ -0,0 +1 @@
+phone number,name,favourite colour,fruit

tests/spreadsheet_files/tab separated.tsv

@@ -0,0 +1 @@
+phone number	name	favourite colour	fruit