From 494e49ee45ce624a88f969d7598e8c72b70b0034 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Thu, 14 Jan 2016 16:00:13 +0000 Subject: [PATCH] Changed flow to upload directly to s3 if non empty file. Then on check page file read back from s3 and checked. --- app/main/uploader.py | 21 ++++--- app/main/views/sms.py | 97 ++++++++++++------------------ app/templates/views/check-sms.html | 2 - requirements_for_test.txt | 4 +- tests/app/main/views/test_sms.py | 81 +++++++------------------ 5 files changed, 78 insertions(+), 127 deletions(-) diff --git a/app/main/uploader.py b/app/main/uploader.py index bedf4ba6d..a1e88e216 100644 --- a/app/main/uploader.py +++ b/app/main/uploader.py @@ -1,15 +1,22 @@ -import os import uuid from boto3 import resource -# TODO add service name to bucket name as well -def s3upload(filepath): - filename = filepath.split(os.path.sep)[-1] +def s3upload(service_id, filedata): upload_id = str(uuid.uuid4()) s3 = resource('s3') - s3.create_bucket(Bucket=upload_id) - key = s3.Object(upload_id, filename) - key.put(Body=open(filepath, 'rb'), ServerSideEncryption='AES256') + bucket_name = 'service-{}-notify'.format(service_id) + s3.create_bucket(Bucket=bucket_name) + contents = '\n'.join(filedata['data']) + key = s3.Object(bucket_name, upload_id) + key.put(Body=contents, ServerSideEncryption='AES256') return upload_id + + +def s3download(service_id, upload_id): + s3 = resource('s3') + bucket_name = 'service-{}-notify'.format(service_id) + key = s3.Object(bucket_name, upload_id) + contents = key.get()['Body'].read().decode('utf-8') + return contents diff --git a/app/main/views/sms.py b/app/main/views/sms.py index bf276cee5..fa0ad41ec 100644 --- a/app/main/views/sms.py +++ b/app/main/views/sms.py @@ -20,7 +20,10 @@ from werkzeug import secure_filename from app.main import main from app.main.forms import CsvUploadForm -from app.main.uploader import s3upload +from app.main.uploader import ( + s3upload, + s3download +) from ._templates import templates @@ -36,21 +39,16 @@ def sendsms(service_id): if form.validate_on_submit(): try: csv_file = form.file.data - filename = _format_filename(csv_file.filename) - filepath = os.path.join(current_app.config['UPLOAD_FOLDER'], - filename) - csv_file.save(filepath) - _check_file(csv_file.filename, filepath) + filedata = _get_filedata(csv_file) + upload_id = s3upload(service_id, filedata) return redirect(url_for('.checksms', service_id=service_id, - recipients=filename)) - except (IOError, ValueError) as e: + upload_id=upload_id)) + except ValueError as e: message = 'There was a problem uploading: {}'.format( csv_file.filename) flash(message) - if isinstance(e, ValueError): - flash(str(e)) - os.remove(filepath) + flash(str(e)) return redirect(url_for('.sendsms', service_id=service_id)) return render_template('views/send-sms.html', @@ -59,45 +57,34 @@ def sendsms(service_id): service_id=service_id) -@main.route("/services//sms/check", methods=['GET', 'POST']) +@main.route("/services//sms/check/", + methods=['GET', 'POST']) @login_required -def checksms(service_id): +def checksms(service_id, upload_id): if request.method == 'GET': - filename = request.args.get('recipients') - if not filename: - abort(400) - filepath = os.path.join(current_app.config['UPLOAD_FOLDER'], - filename) - upload_result = _build_upload_result(filepath) - if upload_result.get('rejects'): - flash('There was a problem with some of the numbers') - + contents = s3download(service_id, upload_id) + upload_result = _get_numbers(contents) + # TODO get original file name return render_template( 'views/check-sms.html', upload_result=upload_result, - filename=filename, + filename='someupload_file_name.csv', message_template=sms_templates[0]['body'], service_id=service_id ) elif request.method == 'POST': - filename = request.form['recipients'] - filepath = os.path.join(current_app.config['UPLOAD_FOLDER'], - filename) - try: - upload_id = s3upload(filepath) - # TODO when job is created record filename in job itself - # so downstream pages can get the original filename that way - session[upload_id] = filename - return redirect(url_for('main.showjob', service_id=service_id, job_id=upload_id)) - except: - flash('There as a problem saving the file') - return redirect(url_for('.checksms', recipients=filename)) + # TODO create the job with template, file location etc. + return redirect(url_for('main.showjob', + service_id=service_id, + job_id=upload_id)) -def _check_file(filename, filepath): - if os.stat(filepath).st_size == 0: - message = 'The file {} contained no data'.format(filename) +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) raise ValueError(message) + return {'filename': file.filename, 'data': lines} def _format_filename(filename): @@ -107,24 +94,16 @@ def _format_filename(filename): return secure_filename(formatted_name) -def _open(file): - return open(file, 'r') - - -def _build_upload_result(csv_file): - try: - file = _open(csv_file, 'r') - pattern = re.compile(r'^\+44\s?\d{4}\s?\d{6}$') - reader = csv.DictReader( - file.read().splitlines(), - lineterminator='\n', - quoting=csv.QUOTE_NONE) - valid, rejects = [], [] - for i, row in enumerate(reader): - if pattern.match(row['phone']): - valid.append(row) - else: - rejects.append({"line_number": i+2, "phone": row['phone']}) - return {"valid": valid, "rejects": rejects} - finally: - file.close() +def _get_numbers(contents): + pattern = re.compile(r'^\+44\s?\d{4}\s?\d{6}$') # need better validation + reader = csv.DictReader( + contents.split('\n'), + lineterminator='\n', + quoting=csv.QUOTE_NONE) + valid, rejects = [], [] + for i, row in enumerate(reader): + if pattern.match(row['phone']): + valid.append(row) + else: + rejects.append({"line_number": i+2, "phone": row['phone']}) + return {"valid": valid, "rejects": rejects} diff --git a/app/templates/views/check-sms.html b/app/templates/views/check-sms.html index 79d924f08..b7a564b11 100644 --- a/app/templates/views/check-sms.html +++ b/app/templates/views/check-sms.html @@ -59,8 +59,6 @@ back_link = url_for(".sendsms", service_id=service_id) )}} - - {% endif %} {% endblock %} diff --git a/requirements_for_test.txt b/requirements_for_test.txt index f7931ea45..e23499713 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,4 +1,6 @@ -r requirements.txt pep8==1.5.7 pytest==2.8.1 -pytest-mock==0.8.1 \ No newline at end of file +pytest-mock==0.8.1 +moto==0.4.19 +httpretty==0.8.10 diff --git a/tests/app/main/views/test_sms.py b/tests/app/main/views/test_sms.py index e2d84b701..57948c3e6 100644 --- a/tests/app/main/views/test_sms.py +++ b/tests/app/main/views/test_sms.py @@ -1,21 +1,18 @@ from io import BytesIO -from unittest import mock -from unittest.mock import mock_open +from flask import url_for +import moto from tests.app.main import create_test_user def test_upload_empty_csvfile_returns_to_upload_page( - notifications_admin, notifications_admin_db, notify_db_session, - mocker): - - _setup_mocker_for_empty_file(mocker) + notifications_admin, notifications_admin_db, notify_db_session): with notifications_admin.test_request_context(): with notifications_admin.test_client() as client: user = create_test_user('active') client.login(user) upload_data = {'file': (BytesIO(''.encode('utf-8')), 'emtpy.csv')} - response = client.post('/services/123/sms/send', + response = client.post(url_for('main.sendsms', service_id=123), data=upload_data, follow_redirects=True) assert response.status_code == 200 @@ -23,34 +20,30 @@ def test_upload_empty_csvfile_returns_to_upload_page( assert 'The file emtpy.csv contained no data' in content +@moto.mock_s3 def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( - notifications_admin, notifications_admin_db, notify_db_session, - mocker): + notifications_admin, notifications_admin_db, notify_db_session): contents = 'phone\n+44 123\n+44 456' file_data = (BytesIO(contents.encode('utf-8')), 'invalid.csv') - m_open = mock_open(read_data=contents) - _setup_mocker_for_nonemtpy_file(mocker) with notifications_admin.test_request_context(): with notifications_admin.test_client() as client: user = create_test_user('active') client.login(user) upload_data = {'file': file_data} - with mock.patch('app.main.views.sms._open', m_open): - response = client.post('/services/123/sms/send', - data=upload_data, - follow_redirects=True) + response = client.post(url_for('main.sendsms', service_id=123), + data=upload_data, + follow_redirects=True) assert response.status_code == 200 content = response.get_data(as_text=True) - - assert 'There was a problem with some of the numbers' in content assert 'The following numbers are invalid' in content assert '+44 123' in content assert '+44 456' in content assert 'Go back and resolve errors' in content +@moto.mock_s3 def test_upload_csvfile_with_valid_phone_shows_first3_and_last3_numbers( notifications_admin, notifications_admin_db, notify_db_session, mocker): @@ -58,18 +51,15 @@ def test_upload_csvfile_with_valid_phone_shows_first3_and_last3_numbers( 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\n+44 7700 900987\n+44 7700 900988\n+44 7700 900989' # noqa file_data = (BytesIO(contents.encode('utf-8')), 'valid.csv') - m_open = mock_open(read_data=contents) - _setup_mocker_for_nonemtpy_file(mocker) with notifications_admin.test_request_context(): with notifications_admin.test_client() as client: user = create_test_user('active') client.login(user) upload_data = {'file': file_data} - with mock.patch('app.main.views.sms._open', m_open): - response = client.post('/services/123/sms/send', - data=upload_data, - follow_redirects=True) + response = client.post(url_for('main.sendsms', service_id=123), + data=upload_data, + follow_redirects=True) content = response.get_data(as_text=True) @@ -88,25 +78,21 @@ def test_upload_csvfile_with_valid_phone_shows_first3_and_last3_numbers( assert '+44 7700 900989' in content +@moto.mock_s3 def test_upload_csvfile_with_valid_phone_shows_all_if_6_or_less_numbers( - notifications_admin, notifications_admin_db, notify_db_session, - mocker): + notifications_admin, notifications_admin_db, notify_db_session): 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 file_data = (BytesIO(contents.encode('utf-8')), 'valid.csv') - m_open = mock_open(read_data=contents) - _setup_mocker_for_nonemtpy_file(mocker) - with notifications_admin.test_request_context(): with notifications_admin.test_client() as client: user = create_test_user('active') client.login(user) upload_data = {'file': file_data} - with mock.patch('app.main.views.sms._open', m_open): - response = client.post('/services/123/sms/send', - data=upload_data, - follow_redirects=True) + response = client.post(url_for('main.sendsms', service_id=123), + data=upload_data, + follow_redirects=True) content = response.get_data(as_text=True) @@ -121,34 +107,13 @@ def test_upload_csvfile_with_valid_phone_shows_all_if_6_or_less_numbers( assert '+44 7700 900986' in content -def test_should_redirect_to_job(notifications_admin, notifications_admin_db, - notify_db_session, mocker): - _setup_mocker_for_check(mocker) +@moto.mock_s3 +def test_post_to_check_should_redirect_to_job(notifications_admin, notifications_admin_db, notify_db_session): with notifications_admin.test_request_context(): with notifications_admin.test_client() as client: user = create_test_user('active') client.login(user) - with client.session_transaction() as s: - s[456] = 'test.csv' - - response = client.post('/services/123/sms/check', - data={'recipients': 'test.csv'}) - + response = client.post(url_for('main.checksms', + service_id=123, + upload_id='someid')) assert response.status_code == 302 - - -def _setup_mocker_for_empty_file(mocker): - mocker.patch('werkzeug.datastructures.FileStorage.save') - mocker.patch('os.remove') - ret = ValueError('The file emtpy.csv contained no data') - mocker.patch('app.main.views.sms._check_file', side_effect=ret) - - -def _setup_mocker_for_nonemtpy_file(mocker): - mocker.patch('werkzeug.datastructures.FileStorage.save') - mocker.patch('os.remove') - mocker.patch('app.main.views.sms._check_file') - - -def _setup_mocker_for_check(mocker): - mocker.patch('app.main.views.sms.s3upload').return_value = 456