From d7bad32ac32b8b36b1a0c38e576b880d472a3d3c Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Fri, 26 Feb 2016 10:54:06 +0000 Subject: [PATCH 1/2] Removed moto mocks and mock s3 upload and download directly. Also updated csv reading to use correct module and do more work to remove empty fields and trailing commas. --- app/main/views/send.py | 31 +++++++++--- tests/app/main/views/test_send.py | 81 +++++++++++++++++++------------ tests/conftest.py | 7 +++ 3 files changed, 81 insertions(+), 38 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 82a9aa310..856403105 100644 --- a/app/main/views/send.py +++ b/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() - if len(lines) < 2: # must be at least header and one line - message = 'The file {} contained no data'.format(file.filename) + 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 + 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 ) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index d59da4593..11cd63de8 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1,8 +1,8 @@ +import pytest + from io import BytesIO from flask import url_for - -import pytest -import moto +from unittest.mock import ANY template_types = ['email', 'sms'] @@ -57,17 +57,18 @@ 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_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 +87,49 @@ 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') -@moto.mock_s3 +def test_upload_csvfile_removes_empty_lines_and_trailing_commas( + app_, + api_user_active, + mocker, + mock_login, + 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_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 +138,14 @@ def test_send_test_message_to_self( follow_redirects=True ) assert response.status_code == 200 - content = response.get_data(as_text=True) - assert 'Test run' in content - assert '+4412341234' in content + mock_s3_upload.assert_called_with(ANY, '12345', expected_data, 'eu-west-1') -@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_user_by_email, mock_get_service_template ): @@ -131,25 +157,22 @@ 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_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,14 +197,10 @@ 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, diff --git a/tests/conftest.py b/tests/conftest.py index ff8e7659f..b9622bf9e 100644 --- a/tests/conftest.py +++ b/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) From 73cf7935666caf1ce7c7e60a670a7494a10be276 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Fri, 26 Feb 2016 12:11:55 +0000 Subject: [PATCH 2/2] Merge from master --- tests/app/main/views/test_send.py | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 11cd63de8..e638a5481 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -62,6 +62,7 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( api_user_active, mocker, mock_login, + mock_get_service, mock_get_service_template, mock_s3_upload ): @@ -92,6 +93,7 @@ def test_upload_csvfile_removes_empty_lines_and_trailing_commas( api_user_active, mocker, mock_login, + mock_get_service, mock_get_service_template, mock_s3_upload ): @@ -123,6 +125,7 @@ def test_send_test_message_to_self( mocker, api_user_active, mock_login, + mock_get_service, mock_get_service_template, mock_s3_upload ): @@ -146,6 +149,7 @@ def test_download_example_csv( mocker, api_user_active, mock_login, + mock_get_service, mock_get_service_template ): @@ -166,6 +170,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mocker, api_user_active, mock_login, + mock_get_service, mock_get_service_template, mock_s3_upload ): @@ -205,6 +210,7 @@ def test_create_job_should_call_api( job_data, mock_create_job, mock_get_job, + mock_get_service, mock_get_service_template ):