From 338df098a8244ccd76b61efddf9c99083e9bc5de Mon Sep 17 00:00:00 2001
From: Ken Tsang <ken.tsang@digital.cabinet-office.gov.uk>
Date: Wed, 14 Jun 2017 15:31:38 +0100
Subject: [PATCH 01/10] Add check for ascii only in recipients file

---
 app/main/views/send.py            | 15 +++++++++++++--
 tests/app/main/views/test_send.py | 30 ++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index de6be4ff1..1ae378123 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -112,9 +112,17 @@ def send_messages(service_id, template_id):
     form = CsvUploadForm()
     if form.validate_on_submit():
         try:
+            file_data = Spreadsheet.from_file(form.file.data, filename=form.file.data.filename).as_dict
+            if template.template_type == 'letter':
+                def is_ascii(s):
+                    return all(ord(c) < 128 for c in s)
+
+                if is_ascii(str(file_data)) is False:
+                    raise ValueError("Invalid characters in {}".format(form.file.data.filename))
+
             upload_id = s3upload(
                 service_id,
-                Spreadsheet.from_file(form.file.data, filename=form.file.data.filename).as_dict,
+                file_data,
                 current_app.config['AWS_REGION']
             )
             session['upload_data'] = {
@@ -129,6 +137,10 @@ def send_messages(service_id, template_id):
             flash('Couldn’t read {}. Try using a different file format.'.format(
                 form.file.data.filename
             ))
+        except ValueError:
+            flash('Invalid characters in the address fields within {}.'.format(
+                form.file.data.filename
+            ))
 
     column_headings = first_column_headings[template.template_type] + list(template.placeholders)
 
@@ -333,7 +345,6 @@ def send_test_preview(service_id, template_id, filetype):
 
 
 def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False):
-
     if not session.get('upload_data'):
         # if we just return a `redirect` (302) object here, we'll get errors when we try and unpack in the
         # check_messages route - so raise a werkzeug.routing redirect to ensure that doesn't happen.
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 69ebddf74..d3408678b 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1529,6 +1529,36 @@ def test_check_messages_shows_over_max_row_error(
     )
 
 
+def test_special_characters_in_dvla_recipients_file_shows_error(
+    logged_in_client,
+    mock_get_users_by_service,
+    mock_get_service,
+    mock_get_service_letter_template,
+    mock_get_detailed_service_for_today,
+    fake_uuid,
+    mocker
+):
+    mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1)
+
+    mock_recipients = mocker.patch('app.utils.Spreadsheet.from_file').return_value
+    mock_recipients.as_dict = {
+            'file_name': 'invalid_characters.csv', 'data':
+            'address line 1,address line 2,address line 3,address line 4,address line 5,address line 6,postcode\r\n'\
+            'B. √Name,345 Example Street,,,,,ZM4 6HQ©'
+        }
+
+    response = logged_in_client.post(
+        url_for('main.send_messages', service_id=SERVICE_ONE_ID, template_id=fake_uuid),
+        data={'file': (None, 'invalid_characters.csv')},
+    )
+
+    assert response.status_code == 200
+    page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
+    assert ' '.join(
+        page.find('div', class_='banner-dangerous').text.split()
+    ) == 'Invalid characters in the address fields within invalid_characters.csv.'
+
+
 def test_check_messages_redirects_if_no_upload_data(logged_in_client, service_one, mocker):
     checker = mocker.patch('app.main.views.send.get_check_messages_back_url', return_value='foo')
     response = logged_in_client.get(url_for(

From c8f72bc51342c0120e88bcf1e08319def8686d04 Mon Sep 17 00:00:00 2001
From: Ken Tsang <ken.tsang@digital.cabinet-office.gov.uk>
Date: Thu, 15 Jun 2017 10:48:22 +0100
Subject: [PATCH 02/10] Refactor to use address validation in utils

---
 app/main/views/send.py            | 14 +-------------
 tests/app/main/views/test_send.py | 11 ++++++++---
 2 files changed, 9 insertions(+), 16 deletions(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index 1ae378123..7595ac627 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -112,17 +112,9 @@ def send_messages(service_id, template_id):
     form = CsvUploadForm()
     if form.validate_on_submit():
         try:
-            file_data = Spreadsheet.from_file(form.file.data, filename=form.file.data.filename).as_dict
-            if template.template_type == 'letter':
-                def is_ascii(s):
-                    return all(ord(c) < 128 for c in s)
-
-                if is_ascii(str(file_data)) is False:
-                    raise ValueError("Invalid characters in {}".format(form.file.data.filename))
-
             upload_id = s3upload(
                 service_id,
-                file_data,
+                Spreadsheet.from_file(form.file.data, filename=form.file.data.filename).as_dict,
                 current_app.config['AWS_REGION']
             )
             session['upload_data'] = {
@@ -137,10 +129,6 @@ def send_messages(service_id, template_id):
             flash('Couldn’t read {}. Try using a different file format.'.format(
                 form.file.data.filename
             ))
-        except ValueError:
-            flash('Invalid characters in the address fields within {}.'.format(
-                form.file.data.filename
-            ))
 
     column_headings = first_column_headings[template.template_type] + list(template.placeholders)
 
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index d3408678b..401bf0490 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1529,7 +1529,7 @@ def test_check_messages_shows_over_max_row_error(
     )
 
 
-def test_special_characters_in_dvla_recipients_file_shows_error(
+def test_special_characters_in_letter_recipients_file_shows_error(
     logged_in_client,
     mock_get_users_by_service,
     mock_get_service,
@@ -1543,20 +1543,25 @@ def test_special_characters_in_dvla_recipients_file_shows_error(
     mock_recipients = mocker.patch('app.utils.Spreadsheet.from_file').return_value
     mock_recipients.as_dict = {
             'file_name': 'invalid_characters.csv', 'data':
-            'address line 1,address line 2,address line 3,address line 4,address line 5,address line 6,postcode\r\n'\
+            'address line 1,address line 2,address line 3,address line 4,address line 5,address line 6,postcode\r\n'
             'B. √Name,345 Example Street,,,,,ZM4 6HQ©'
         }
 
     response = logged_in_client.post(
         url_for('main.send_messages', service_id=SERVICE_ONE_ID, template_id=fake_uuid),
         data={'file': (None, 'invalid_characters.csv')},
+        follow_redirects=True
     )
 
     assert response.status_code == 200
     page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
     assert ' '.join(
         page.find('div', class_='banner-dangerous').text.split()
-    ) == 'Invalid characters in the address fields within invalid_characters.csv.'
+    ) == (
+            'There is a problem with your data '
+            'You need to fix 1 address '
+            'Skip to file contents'
+        )
 
 
 def test_check_messages_redirects_if_no_upload_data(logged_in_client, service_one, mocker):

From 6e3bedb63391687a120f3b9fbd06e8e6aa618d76 Mon Sep 17 00:00:00 2001
From: Ken Tsang <ken.tsang@digital.cabinet-office.gov.uk>
Date: Thu, 15 Jun 2017 12:44:19 +0100
Subject: [PATCH 03/10] Refactored  non ascii letter recipients test

---
 app/main/views/send.py            | 1 +
 tests/app/main/views/test_send.py | 4 ++--
 2 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index 7595ac627..de6be4ff1 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -333,6 +333,7 @@ def send_test_preview(service_id, template_id, filetype):
 
 
 def _check_messages(service_id, template_type, upload_id, letters_as_pdf=False):
+
     if not session.get('upload_data'):
         # if we just return a `redirect` (302) object here, we'll get errors when we try and unpack in the
         # check_messages route - so raise a werkzeug.routing redirect to ensure that doesn't happen.
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 401bf0490..6cb04711c 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1529,7 +1529,7 @@ def test_check_messages_shows_over_max_row_error(
     )
 
 
-def test_special_characters_in_letter_recipients_file_shows_error(
+def test_non_ascii_characters_in_letter_recipients_file_shows_error(
     logged_in_client,
     mock_get_users_by_service,
     mock_get_service,
@@ -1544,7 +1544,7 @@ def test_special_characters_in_letter_recipients_file_shows_error(
     mock_recipients.as_dict = {
             'file_name': 'invalid_characters.csv', 'data':
             'address line 1,address line 2,address line 3,address line 4,address line 5,address line 6,postcode\r\n'
-            'B. √Name,345 Example Street,,,,,ZM4 6HQ©'
+            'Fran\u00e7oise Fr\u00e9d\u00e9rich,345 Example Street,,,,,ZM4 6HQ'
         }
 
     response = logged_in_client.post(

From 5f8266ede65bbbaf93ac4021da07da55b234c07f Mon Sep 17 00:00:00 2001
From: Ken Tsang <ken.tsang@digital.cabinet-office.gov.uk>
Date: Thu, 15 Jun 2017 15:00:47 +0100
Subject: [PATCH 04/10] Update test

---
 tests/app/main/views/test_send.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 6cb04711c..4568dcd45 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1544,7 +1544,7 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error(
     mock_recipients.as_dict = {
             'file_name': 'invalid_characters.csv', 'data':
             'address line 1,address line 2,address line 3,address line 4,address line 5,address line 6,postcode\r\n'
-            'Fran\u00e7oise Fr\u00e9d\u00e9rich,345 Example Street,,,,,ZM4 6HQ'
+            '\u041F\u0435\u0442\u044F,345 Example Street,,,,,ZM4 6HQ'
         }
 
     response = logged_in_client.post(

From 7d8ccc4cb56b2bc71f643d5bf80427b9860293a0 Mon Sep 17 00:00:00 2001
From: Ken Tsang <ken.tsang@digital.cabinet-office.gov.uk>
Date: Fri, 16 Jun 2017 10:29:36 +0100
Subject: [PATCH 05/10] Update requirements to get latest utils

---
 requirements.txt                  | 2 +-
 tests/app/main/views/test_send.py | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/requirements.txt b/requirements.txt
index ccaca0007..b7c16d11c 100644
--- a/requirements.txt
+++ b/requirements.txt
@@ -28,4 +28,4 @@ notifications-python-client>=3.1,<3.2
 awscli>=1.11,<1.12
 awscli-cwlogs>=1.4,<1.5
 
-git+https://github.com/alphagov/notifications-utils.git@17.3.1#egg=notifications-utils==17.3.1
+git+https://github.com/alphagov/notifications-utils.git@17.3.2#egg=notifications-utils==17.3.2
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 4568dcd45..830da76c9 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1544,7 +1544,7 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error(
     mock_recipients.as_dict = {
             'file_name': 'invalid_characters.csv', 'data':
             'address line 1,address line 2,address line 3,address line 4,address line 5,address line 6,postcode\r\n'
-            '\u041F\u0435\u0442\u044F,345 Example Street,,,,,ZM4 6HQ'
+            '\u041F\u0435\u0442\u044F,345 Example Street,,,,,AA1 6BB'
         }
 
     response = logged_in_client.post(

From 99ab7ddaddc3eab2f13600d2d3a05c35223b3a3d Mon Sep 17 00:00:00 2001
From: Ken Tsang <ken.tsang@digital.cabinet-office.gov.uk>
Date: Fri, 16 Jun 2017 11:00:53 +0100
Subject: [PATCH 06/10] Updated test

---
 tests/app/main/views/test_send.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 830da76c9..dc34a274b 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1549,7 +1549,7 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error(
 
     response = logged_in_client.post(
         url_for('main.send_messages', service_id=SERVICE_ONE_ID, template_id=fake_uuid),
-        data={'file': (None, 'invalid_characters.csv')},
+        data={'file': (BytesIO(''.encode('utf-8')), 'invalid_characters.csv')},
         follow_redirects=True
     )
 

From 11ce2bfa39f3632ff10d1fd7891d3000a6f90868 Mon Sep 17 00:00:00 2001
From: Ken Tsang <ken.tsang@digital.cabinet-office.gov.uk>
Date: Fri, 16 Jun 2017 11:45:16 +0100
Subject: [PATCH 07/10] Updated test to include correct mocks

---
 tests/app/main/views/test_send.py | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index dc34a274b..826643370 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1531,13 +1531,24 @@ def test_check_messages_shows_over_max_row_error(
 
 def test_non_ascii_characters_in_letter_recipients_file_shows_error(
     logged_in_client,
+    api_user_active,
+    mock_login,
     mock_get_users_by_service,
     mock_get_service,
+    mock_has_permissions,
     mock_get_service_letter_template,
     mock_get_detailed_service_for_today,
     fake_uuid,
     mocker
 ):
+    from tests.conftest import mock_s3_download
+    mock_s3_download(
+        mocker,
+        content=u"""
+        address line 1,address line 2,address line 3,address line 4,address line 5,address line 6,postcode
+        \u041F\u0435\u0442\u044F,345 Example Street,,,,,AA1 6BB
+        """
+    )
     mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1)
 
     mock_recipients = mocker.patch('app.utils.Spreadsheet.from_file').return_value

From 8997c95512a17b5ed7467d50fa3be960cee6e7b8 Mon Sep 17 00:00:00 2001
From: Ken Tsang <ken.tsang@digital.cabinet-office.gov.uk>
Date: Fri, 16 Jun 2017 11:51:58 +0100
Subject: [PATCH 08/10] Refactor non ascii test

---
 tests/app/main/views/test_send.py | 21 ++++++++-------------
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 826643370..e4ffacc59 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1549,20 +1549,15 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error(
         \u041F\u0435\u0442\u044F,345 Example Street,,,,,AA1 6BB
         """
     )
-    mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1)
 
-    mock_recipients = mocker.patch('app.utils.Spreadsheet.from_file').return_value
-    mock_recipients.as_dict = {
-            'file_name': 'invalid_characters.csv', 'data':
-            'address line 1,address line 2,address line 3,address line 4,address line 5,address line 6,postcode\r\n'
-            '\u041F\u0435\u0442\u044F,345 Example Street,,,,,AA1 6BB'
-        }
-
-    response = logged_in_client.post(
-        url_for('main.send_messages', service_id=SERVICE_ONE_ID, template_id=fake_uuid),
-        data={'file': (BytesIO(''.encode('utf-8')), 'invalid_characters.csv')},
-        follow_redirects=True
-    )
+    with logged_in_client.session_transaction() as session:
+        session['upload_data'] = {'template_id': fake_uuid}
+    response = logged_in_client.get(url_for(
+        'main.check_messages',
+        service_id=fake_uuid,
+        template_type='letter',
+        upload_id=fake_uuid
+    ))
 
     assert response.status_code == 200
     page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')

From 8061d3f29d3965f36f3eaef6eb6f29b8409ab7f7 Mon Sep 17 00:00:00 2001
From: Ken Tsang <ken.tsang@digital.cabinet-office.gov.uk>
Date: Fri, 16 Jun 2017 12:34:55 +0100
Subject: [PATCH 09/10] Added assert to check row error message

---
 tests/app/main/views/test_send.py | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index e4ffacc59..3028bd2e3 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1568,6 +1568,10 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error(
             'You need to fix 1 address '
             'Skip to file contents'
         )
+    assert page.select_one(
+        '#content > div.grid-row > main > table > tbody > '
+        'tr:nth-of-type(1) > td:nth-of-type(2) > div > span > span'
+        ).get_text(strip=True) == u'Can\u2019t include \u041F, \u0435, \u0442 or \u044F'
 
 
 def test_check_messages_redirects_if_no_upload_data(logged_in_client, service_one, mocker):

From 54f65fa57b621aff2d143426e8285d3c915ae754 Mon Sep 17 00:00:00 2001
From: Ken Tsang <ken.tsang@digital.cabinet-office.gov.uk>
Date: Fri, 16 Jun 2017 13:47:00 +0100
Subject: [PATCH 10/10] Added utf-8 support, refactored test

---
 tests/app/main/views/test_send.py | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 3028bd2e3..16a2a6275 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -1,3 +1,4 @@
+# -*- coding: utf-8 -*-
 import uuid
 from io import BytesIO
 from os import path
@@ -1546,7 +1547,7 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error(
         mocker,
         content=u"""
         address line 1,address line 2,address line 3,address line 4,address line 5,address line 6,postcode
-        \u041F\u0435\u0442\u044F,345 Example Street,,,,,AA1 6BB
+        Петя,345 Example Street,,,,,AA1 6BB
         """
     )
 
@@ -1568,10 +1569,7 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error(
             'You need to fix 1 address '
             'Skip to file contents'
         )
-    assert page.select_one(
-        '#content > div.grid-row > main > table > tbody > '
-        'tr:nth-of-type(1) > td:nth-of-type(2) > div > span > span'
-        ).get_text(strip=True) == u'Can\u2019t include \u041F, \u0435, \u0442 or \u044F'
+    assert page.find('span', class_='table-field-error-label').text == u'Can’t include П, е, т or я'
 
 
 def test_check_messages_redirects_if_no_upload_data(logged_in_client, service_one, mocker):