Merge pull request #203 from alphagov/s3-mocks

Moto s3 mocks removed

app/main/views/send.py

@@ -79,7 +79,7 @@ def send_messages(service_id, template_id):
     form = CsvUploadForm()
     if form.validate_on_submit():
         try:
-            csv_file = form.file.data
+            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'])
@@ -88,7 +88,7 @@ def send_messages(service_id, template_id):
                                     service_id=service_id,
                                     upload_id=upload_id))
         except ValueError as e:
-            flash('There was a problem uploading: {}'.format(csv_file.filename))
+            flash('There was a problem uploading: {}'.format(csv_file.data.filename))
             flash(str(e))
             return redirect(url_for('.send_messages', service_id=service_id, template_id=template_id))
 
@@ -199,17 +199,34 @@ def check_messages(service_id, upload_id):
 
 
 def _get_filedata(file):
-    lines = file.read().decode('utf-8').splitlines()
+    import itertools
-    if len(lines) < 2:  # must be at least header and one line
+    reader = csv.reader(
-        message = 'The file {} contained no data'.format(file.filename)
+        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
+        message = 'The file {} contained no data'.format(file.data.filename)
         raise ValueError(message)
-    return {'file_name': file.filename, 'data': lines}
+
+    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'),
-        lineterminator='\n',
         quoting=csv.QUOTE_NONE,
         skipinitialspace=True
     )

tests/app/main/views/test_send.py

@@ -1,8 +1,8 @@
+import pytest
+
 from io import BytesIO
 from flask import url_for
-
+from unittest.mock import ANY
-import pytest
-import moto
 
 template_types = ['email', 'sms']
 
@@ -57,17 +57,19 @@ def test_upload_empty_csvfile_returns_to_upload_page(
         assert 'The file emtpy.csv contained no data' in content
 
 
-@pytest.mark.skipif(True, reason='Errors on travis')
-@moto.mock_s3
 def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(
     app_,
     api_user_active,
+    mocker,
     mock_login,
-    mock_get_service_template
+    mock_get_service,
+    mock_get_service_template,
+    mock_s3_upload
 ):
 
-    contents = 'phone\n+44 123\n+44 456'
+    contents = 'to,name\n+44 123,test1\n+44 456,test2'
     file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv')
+    mocker.patch('app.main.views.send.s3download', return_value=contents)
 
     with app_.test_request_context():
         with app_.test_client() as client:
@@ -86,18 +88,51 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors(
         assert 'Upload a CSV file' in content
 
 
-@pytest.mark.skipif(True, reason='Errors on travis')
+def test_upload_csvfile_removes_empty_lines_and_trailing_commas(
-@moto.mock_s3
+    app_,
+    api_user_active,
+    mocker,
+    mock_login,
+    mock_get_service,
+    mock_get_service_template,
+    mock_s3_upload
+):
+
+    contents = 'to,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': ['to,name', '++44 7700 900981,test1', '+44 7700 900981,test2'],
+                     'file_name': 'invalid.csv'}
+
+    mocker.patch('app.main.views.send.s3download',
+                 return_value='to,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(
     app_,
     mocker,
     api_user_active,
     mock_login,
-    mock_get_user,
+    mock_get_service,
-    mock_get_user_by_email,
+    mock_get_service_template,
-    mock_get_service_template
+    mock_s3_upload
 ):
 
+    expected_data = {'data': ['to', '+4412341234'], 'file_name': 'Test run'}
+    mocker.patch('app.main.views.send.s3download', return_value='to\r\n+4412341234')
+
     with app_.test_request_context():
         with app_.test_client() as client:
             client.login(api_user_active)
@@ -106,20 +141,15 @@ def test_send_test_message_to_self(
                 follow_redirects=True
             )
         assert response.status_code == 200
-        content = response.get_data(as_text=True)
+        mock_s3_upload.assert_called_with(ANY, '12345', expected_data, 'eu-west-1')
-        assert 'Test run' in content
-        assert '+4412341234' in content
 
 
-@pytest.mark.skipif(True, reason='Errors on travis')
-@moto.mock_s3
 def test_download_example_csv(
     app_,
     mocker,
     api_user_active,
     mock_login,
-    mock_get_user,
+    mock_get_service,
-    mock_get_user_by_email,
     mock_get_service_template
 ):
 
@@ -131,25 +161,23 @@ def test_download_example_csv(
                 follow_redirects=True
             )
         assert response.status_code == 200
-        assert response.get_data(as_text=True) == 'phone\r\n+4412341234\r\n'
+        assert response.get_data(as_text=True) == 'to\r\n+4412341234\r\n'
         assert 'text/csv' in response.headers['Content-Type']
 
 
-@pytest.mark.skipif(True, reason='Errors on travis')
-@moto.mock_s3
 def test_upload_csvfile_with_valid_phone_shows_all_numbers(
     app_,
     mocker,
     api_user_active,
     mock_login,
-    mock_get_user,
+    mock_get_service,
-    mock_get_user_by_email,
+    mock_get_service_template,
-    mock_get_service_template
+    mock_s3_upload
 ):
 
-    contents = 'phone\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
+    contents = 'to\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
-
     file_data = (BytesIO(contents.encode('utf-8')), 'valid.csv')
+    mocker.patch('app.main.views.send.s3download', return_value=contents)
 
     with app_.test_request_context():
         with app_.test_client() as client:
@@ -174,18 +202,15 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers(
             assert '+44 7700 900986' in content
 
 
-@pytest.mark.skipif(True, reason='Errors on travis')
-@moto.mock_s3
 def test_create_job_should_call_api(
     app_,
     service_one,
     api_user_active,
-    mock_get_user,
-    mock_get_user_by_email,
     mock_login,
     job_data,
     mock_create_job,
     mock_get_job,
+    mock_get_service,
     mock_get_service_template
 ):
 

tests/conftest.py

@@ -526,3 +526,10 @@ def mock_get_users_by_service(mocker):
                  'failed_login_count': 0}]
         return data
     return mocker.patch('app.user_api_client.get_users_for_service', side_effect=_get_users_for_service, autospec=True)
+
+
+@pytest.fixture(scope='function')
+def mock_s3_upload(mocker):
+    def _upload(upload_id, service_id, filedata, region):
+        pass
+    return mocker.patch('app.main.views.send.s3upload', side_effect=_upload)