From 4dcb180da19dcf6fd0704a2431f53c7ee78fa4f9 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Wed, 13 Jan 2016 17:32:40 +0000 Subject: [PATCH 1/6] Changed page flow to first save file and then redirect to check. On check numbers in file are validated. Posting to check then uploads file to s3 --- app/main/uploader.py | 15 +++++ app/main/views/jobs.py | 14 ++++- app/main/views/sms.py | 90 ++++++++++++++++++++---------- app/templates/views/check-sms.html | 2 + config.py | 1 + tests/app/main/views/test_jobs.py | 9 ++- tests/app/main/views/test_sms.py | 13 ++++- 7 files changed, 106 insertions(+), 38 deletions(-) create mode 100644 app/main/uploader.py diff --git a/app/main/uploader.py b/app/main/uploader.py new file mode 100644 index 000000000..bedf4ba6d --- /dev/null +++ b/app/main/uploader.py @@ -0,0 +1,15 @@ +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] + 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') + return upload_id diff --git a/app/main/views/jobs.py b/app/main/views/jobs.py index 29755f3e1..b5675ead4 100644 --- a/app/main/views/jobs.py +++ b/app/main/views/jobs.py @@ -1,11 +1,14 @@ # -*- coding: utf-8 -*- import time -from flask import render_template + +from flask import ( + render_template, + session +) from flask_login import login_required from app.main import main - from ._jobs import jobs now = time.strftime('%H:%M') @@ -55,6 +58,11 @@ def showjobs(service_id): @main.route("/services//jobs/") @login_required def showjob(service_id, job_id): + + # TODO the uploaded file name could be part of job definition + # so won't need to be passed on from last view via session + uploaded_file_name = session.get(job_id) + return render_template( 'views/job.html', messages=messages, @@ -68,7 +76,7 @@ def showjob(service_id, job_id): ]) }, cost=u'£0.00', - uploaded_file_name='dispatch_20151114.csv', + uploaded_file_name=uploaded_file_name, uploaded_file_time=now, template_used='Test message 1', flash_message=u'We’ve started sending your messages', diff --git a/app/main/views/sms.py b/app/main/views/sms.py index 0ef985558..f8fb73270 100644 --- a/app/main/views/sms.py +++ b/app/main/views/sms.py @@ -1,5 +1,8 @@ import csv import re +import os + +from datetime import date from flask import ( request, @@ -12,9 +15,11 @@ from flask import ( ) from flask_login import login_required +from werkzeug import secure_filename from app.main import main from app.main.forms import CsvUploadForm +from app.main.uploader import s3upload # TODO move this to the templates directory message_templates = [ @@ -39,26 +44,27 @@ message_templates = [ @login_required def sendsms(service_id): form = CsvUploadForm() - if form.validate_on_submit(): - csv_file = form.file.data - # in memory handing is temporary until next story to save csv file + if form.validate_on_submit(): try: - results = _build_upload_result(csv_file) - except Exception: - message = 'There was a problem with the file: {}'.format( + 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) + return redirect(url_for('.checksms', + service_id=service_id, + recipients=filename)) + except (IOError, 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) return redirect(url_for('.sendsms', service_id=service_id)) - if not results['valid'] and not results['rejects']: - message = "The file {} contained no data".format(csv_file.filename) - flash(message, 'error') - return redirect(url_for('.sendsms', service_id=service_id)) - - session[csv_file.filename] = results - return redirect(url_for('.checksms', service_id=service_id, recipients=csv_file.filename)) - return render_template('views/send-sms.html', message_templates=message_templates, form=form, @@ -69,33 +75,59 @@ def sendsms(service_id): @login_required def checksms(service_id): if request.method == 'GET': - - recipients_filename = request.args.get('recipients') - # upload results in session until file is persisted in next story - upload_result = session.get(recipients_filename) + filename = request.args.get('recipients') + 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') return render_template( 'views/check-sms.html', upload_result=upload_result, + filename=filename, message_template=message_templates[0]['body'], service_id=service_id ) elif request.method == 'POST': - return redirect(url_for('.showjob', service_id=service_id, job_id=456)) + 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('.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)) + + +def _check_file(filename, filepath): + if os.stat(filepath).st_size == 0: + message = 'The file {} contained no data'.format(filename) + raise ValueError(message) + + +def _format_filename(filename): + d = date.today() + basename, extenstion = filename.split('.') + formatted_name = '{}_{}.csv'.format(basename, d.strftime('%Y%m%d')) + return secure_filename(formatted_name) def _build_upload_result(csv_file): pattern = re.compile(r'^\+44\s?\d{4}\s?\d{6}$') - reader = csv.DictReader( - csv_file.read().decode('utf-8').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']}) + with open(csv_file) as f: + reader = csv.DictReader( + f, + 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 b7a564b11..79d924f08 100644 --- a/app/templates/views/check-sms.html +++ b/app/templates/views/check-sms.html @@ -59,6 +59,8 @@ back_link = url_for(".sendsms", service_id=service_id) )}} + + {% endif %} {% endblock %} diff --git a/config.py b/config.py index 80825d4cd..b0f596963 100644 --- a/config.py +++ b/config.py @@ -32,6 +32,7 @@ class Config(object): TOKEN_MAX_AGE_SECONDS = 3600 MAX_CONTENT_LENGTH = 10 * 1024 * 1024 # 10mb + UPLOAD_FOLDER = '/tmp' class Development(Config): diff --git a/tests/app/main/views/test_jobs.py b/tests/app/main/views/test_jobs.py index cb03cf7ee..bb21c5a4b 100644 --- a/tests/app/main/views/test_jobs.py +++ b/tests/app/main/views/test_jobs.py @@ -16,9 +16,12 @@ def test_should_return_list_of_all_jobs(notifications_admin, notifications_admin def test_should_show_page_for_one_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) - response = client.get('/services/123/jobs/456') + # TODO filename will be part of job metadata not in session + with client.session_transaction() as s: + s[456] = 'dispatch_20151114.csv' + user = create_test_user('active') + client.login(user) + response = client.get('/services/123/jobs/456') assert response.status_code == 200 assert 'dispatch_20151114.csv' in response.get_data(as_text=True) diff --git a/tests/app/main/views/test_sms.py b/tests/app/main/views/test_sms.py index c74356f5f..6dd5f08ba 100644 --- a/tests/app/main/views/test_sms.py +++ b/tests/app/main/views/test_sms.py @@ -2,15 +2,22 @@ from io import BytesIO from tests.app.main import create_test_user +def teardown_function(function): + import glob + import os + files = glob.glob("/tmp/*.csv") + for f in files: + os.remove(f) + + def test_upload_empty_csvfile_returns_to_upload_page(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', data=upload_data, - follow_redirects=True) + response = client.post('/services/123/sms/send', + data=upload_data, follow_redirects=True) assert response.status_code == 200 content = response.get_data(as_text=True) From c729f0a29f4fe807b98319839615c97722285147 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Wed, 13 Jan 2016 22:34:36 +0000 Subject: [PATCH 2/6] fix up tests --- app/main/views/sms.py | 14 +++-- tests/app/main/views/test_sms.py | 99 ++++++++++++++++++++++---------- 2 files changed, 76 insertions(+), 37 deletions(-) diff --git a/app/main/views/sms.py b/app/main/views/sms.py index f8fb73270..ac5e6cf67 100644 --- a/app/main/views/sms.py +++ b/app/main/views/sms.py @@ -44,7 +44,6 @@ message_templates = [ @login_required def sendsms(service_id): form = CsvUploadForm() - if form.validate_on_submit(): try: csv_file = form.file.data @@ -98,7 +97,7 @@ def checksms(service_id): # 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('.showjob', service_id=service_id, job_id=upload_id)) + 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)) @@ -118,10 +117,11 @@ def _format_filename(filename): def _build_upload_result(csv_file): - pattern = re.compile(r'^\+44\s?\d{4}\s?\d{6}$') - with open(csv_file) as f: + try: + file = open(csv_file, 'r') + pattern = re.compile(r'^\+44\s?\d{4}\s?\d{6}$') reader = csv.DictReader( - f, + file.read().splitlines(), lineterminator='\n', quoting=csv.QUOTE_NONE) valid, rejects = [], [] @@ -130,4 +130,6 @@ def _build_upload_result(csv_file): valid.append(row) else: rejects.append({"line_number": i+2, "phone": row['phone']}) - return {"valid": valid, "rejects": rejects} + return {"valid": valid, "rejects": rejects} + finally: + file.close() diff --git a/tests/app/main/views/test_sms.py b/tests/app/main/views/test_sms.py index 6dd5f08ba..916ac022e 100644 --- a/tests/app/main/views/test_sms.py +++ b/tests/app/main/views/test_sms.py @@ -1,16 +1,12 @@ from io import BytesIO +from unittest import mock +from unittest.mock import mock_open + from tests.app.main import create_test_user -def teardown_function(function): - import glob - import os - files = glob.glob("/tmp/*.csv") - for f in files: - os.remove(f) - - -def test_upload_empty_csvfile_returns_to_upload_page(notifications_admin, notifications_admin_db, notify_db_session): +def test_upload_empty_csvfile_returns_to_upload_page(notifications_admin, notifications_admin_db, notify_db_session, mocker): + _setup_mocker_for_empty_file(mocker) with notifications_admin.test_request_context(): with notifications_admin.test_client() as client: user = create_test_user('active') @@ -25,19 +21,26 @@ def test_upload_empty_csvfile_returns_to_upload_page(notifications_admin, notifi def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( - notifications_admin, notifications_admin_db, notify_db_session): + notifications_admin, notifications_admin_db, notify_db_session, + mocker): + + 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) - - file_contents = 'phone\n+44 123\n+44 456'.encode('utf-8') - upload_data = {'file': (BytesIO(file_contents), 'invalid.csv')} - response = client.post('/services/123/sms/send', data=upload_data, - follow_redirects=True) + 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) 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 @@ -46,17 +49,24 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( def test_upload_csvfile_with_valid_phone_shows_first3_and_last3_numbers( - notifications_admin, notifications_admin_db, notify_db_session): + notifications_admin, notifications_admin_db, notify_db_session, + mocker): + + 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) - file_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'.encode('utf-8') # noqa - - upload_data = {'file': (BytesIO(file_contents), 'valid.csv')} - response = client.post('/services/123/sms/send', data=upload_data, - follow_redirects=True) + 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) content = response.get_data(as_text=True) @@ -76,18 +86,24 @@ def test_upload_csvfile_with_valid_phone_shows_first3_and_last3_numbers( def test_upload_csvfile_with_valid_phone_shows_all_if_6_or_less_numbers( - notifications_admin, notifications_admin_db, notify_db_session): + notifications_admin, notifications_admin_db, notify_db_session, + mocker): + + 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) - - file_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'.encode('utf-8') # noqa - - upload_data = {'file': (BytesIO(file_contents), 'valid.csv')} - response = client.post('/services/123/sms/send', data=upload_data, - follow_redirects=True) + 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) content = response.get_data(as_text=True) @@ -103,13 +119,34 @@ def test_upload_csvfile_with_valid_phone_shows_all_if_6_or_less_numbers( def test_should_redirect_to_job(notifications_admin, notifications_admin_db, - notify_db_session): + notify_db_session, mocker): + _setup_mocker_for_check(mocker) 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') + response = client.post('/services/123/sms/check', + data={'recipients': 'test.csv'}) assert response.status_code == 302 - assert response.location == 'http://localhost/services/123/jobs/456' + + +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 + From 6c688a325777c137d21432ae2bd2708d5079978b Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Wed, 13 Jan 2016 22:39:44 +0000 Subject: [PATCH 3/6] fix pep8 error --- tests/app/main/views/test_sms.py | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/app/main/views/test_sms.py b/tests/app/main/views/test_sms.py index 916ac022e..afe7f522a 100644 --- a/tests/app/main/views/test_sms.py +++ b/tests/app/main/views/test_sms.py @@ -5,7 +5,10 @@ from unittest.mock import mock_open 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): +def test_upload_empty_csvfile_returns_to_upload_page( + notifications_admin, notifications_admin_db, notify_db_session, + mocker): + _setup_mocker_for_empty_file(mocker) with notifications_admin.test_request_context(): with notifications_admin.test_client() as client: @@ -149,4 +152,3 @@ def _setup_mocker_for_nonemtpy_file(mocker): def _setup_mocker_for_check(mocker): mocker.patch('app.main.views.sms.s3upload').return_value = 456 - From d628d3d4cb0a2cbb5714a6994b1fdf0e3e64be1c Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Wed, 13 Jan 2016 22:46:25 +0000 Subject: [PATCH 4/6] missing import --- requirements.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/requirements.txt b/requirements.txt index f2124af1e..3e87a8d90 100644 --- a/requirements.txt +++ b/requirements.txt @@ -9,6 +9,7 @@ Flask-WTF==0.11 Flask-Login==0.2.11 Flask-Bcrypt==0.6.2 credstash==1.8.0 +boto3==1.2.3 git+https://github.com/alphagov/notify-api-client.git@0.1.4#egg=notify-api-client==0.1.4 From 2f802e31834ecaaea1d13952321e99f3ed5c14e7 Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Wed, 13 Jan 2016 22:54:11 +0000 Subject: [PATCH 5/6] Tests pass locally and fail on travis. This is attempt to improve situation --- app/main/views/sms.py | 6 +++++- tests/app/main/views/test_sms.py | 6 +++--- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/app/main/views/sms.py b/app/main/views/sms.py index ac5e6cf67..a3492c585 100644 --- a/app/main/views/sms.py +++ b/app/main/views/sms.py @@ -116,9 +116,13 @@ 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') + file = _open(csv_file, 'r') pattern = re.compile(r'^\+44\s?\d{4}\s?\d{6}$') reader = csv.DictReader( file.read().splitlines(), diff --git a/tests/app/main/views/test_sms.py b/tests/app/main/views/test_sms.py index afe7f522a..e2d84b701 100644 --- a/tests/app/main/views/test_sms.py +++ b/tests/app/main/views/test_sms.py @@ -37,7 +37,7 @@ def test_upload_csvfile_with_invalid_phone_shows_check_page_with_errors( user = create_test_user('active') client.login(user) upload_data = {'file': file_data} - with mock.patch('app.main.views.sms.open', m_open): + with mock.patch('app.main.views.sms._open', m_open): response = client.post('/services/123/sms/send', data=upload_data, follow_redirects=True) @@ -66,7 +66,7 @@ def test_upload_csvfile_with_valid_phone_shows_first3_and_last3_numbers( user = create_test_user('active') client.login(user) upload_data = {'file': file_data} - with mock.patch('app.main.views.sms.open', m_open): + with mock.patch('app.main.views.sms._open', m_open): response = client.post('/services/123/sms/send', data=upload_data, follow_redirects=True) @@ -103,7 +103,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_if_6_or_less_numbers( user = create_test_user('active') client.login(user) upload_data = {'file': file_data} - with mock.patch('app.main.views.sms.open', m_open): + with mock.patch('app.main.views.sms._open', m_open): response = client.post('/services/123/sms/send', data=upload_data, follow_redirects=True) From a0e5ae59eae4a3dda0870e8cc6efce2beb78604a Mon Sep 17 00:00:00 2001 From: Adam Shimali Date: Thu, 14 Jan 2016 10:26:18 +0000 Subject: [PATCH 6/6] Added abort 400 in case of recipients param being missing. --- app/main/views/sms.py | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) diff --git a/app/main/views/sms.py b/app/main/views/sms.py index a3492c585..d5ca29981 100644 --- a/app/main/views/sms.py +++ b/app/main/views/sms.py @@ -11,7 +11,8 @@ from flask import ( url_for, session, flash, - current_app + current_app, + abort ) from flask_login import login_required @@ -75,6 +76,8 @@ def sendsms(service_id): def checksms(service_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)