Merge pull request #78 from alphagov/upload-direct-to-s3

Changed flow to upload directly to s3 if non empty file.

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(service_id, filedata):
-def s3upload(filepath):
-    filename = filepath.split(os.path.sep)[-1]
     upload_id = str(uuid.uuid4())
     s3 = resource('s3')
-    s3.create_bucket(Bucket=upload_id)
+    bucket_name = 'service-{}-notify'.format(service_id)
-    key = s3.Object(upload_id, filename)
+    s3.create_bucket(Bucket=bucket_name)
-    key.put(Body=open(filepath, 'rb'), ServerSideEncryption='AES256')
+    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

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)
+            filedata = _get_filedata(csv_file)
-            filepath = os.path.join(current_app.config['UPLOAD_FOLDER'],
+            upload_id = s3upload(service_id, filedata)
-                                    filename)
-            csv_file.save(filepath)
-            _check_file(csv_file.filename, filepath)
             return redirect(url_for('.checksms',
                                     service_id=service_id,
-                                    recipients=filename))
+                                    upload_id=upload_id))
-        except (IOError, ValueError) as e:
+        except ValueError as e:
             message = 'There was a problem uploading: {}'.format(
                       csv_file.filename)
             flash(message)
-            if isinstance(e, ValueError):
+            flash(str(e))
-                flash(str(e))
-            os.remove(filepath)
             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/<int:service_id>/sms/check", methods=['GET', 'POST'])
+@main.route("/services/<int:service_id>/sms/check/<upload_id>",
+            methods=['GET', 'POST'])
 @login_required
-def checksms(service_id):
+def checksms(service_id, upload_id):
     if request.method == 'GET':
-        filename = request.args.get('recipients')
+        contents = s3download(service_id, upload_id)
-        if not filename:
+        upload_result = _get_numbers(contents)
-            abort(400)
+        # TODO get original file name
-        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,
+            filename='someupload_file_name.csv',
             message_template=sms_templates[0]['body'],
             service_id=service_id
         )
     elif request.method == 'POST':
-        filename = request.form['recipients']
+        # TODO create the job with template, file location etc.
-        filepath = os.path.join(current_app.config['UPLOAD_FOLDER'],
+        return redirect(url_for('main.showjob',
-                                filename)
+                        service_id=service_id,
-        try:
+                        job_id=upload_id))
-            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))
 
 
-def _check_file(filename, filepath):
+def _get_filedata(file):
-    if os.stat(filepath).st_size == 0:
+    lines = file.read().decode('utf-8').splitlines()
-        message = 'The file {} contained no data'.format(filename)
+    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):
+def _get_numbers(contents):
-    return open(file, 'r')
+    pattern = re.compile(r'^\+44\s?\d{4}\s?\d{6}$')  # need better validation
-
+    reader = csv.DictReader(
-
+        contents.split('\n'),
-def _build_upload_result(csv_file):
+        lineterminator='\n',
-    try:
+        quoting=csv.QUOTE_NONE)
-        file = _open(csv_file, 'r')
+    valid, rejects = [], []
-        pattern = re.compile(r'^\+44\s?\d{4}\s?\d{6}$')
+    for i, row in enumerate(reader):
-        reader = csv.DictReader(
+        if pattern.match(row['phone']):
-            file.read().splitlines(),
+            valid.append(row)
-            lineterminator='\n',
+        else:
-            quoting=csv.QUOTE_NONE)
+            rejects.append({"line_number": i+2, "phone": row['phone']})
-        valid, rejects = [], []
+    return {"valid": valid, "rejects": 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()

app/templates/views/check-sms.html

@@ -59,8 +59,6 @@
 					back_link = url_for(".sendsms", service_id=service_id)
 			)}}
 
-			<input type='hidden' name='recipients' value='{{filename}}'>
-
 		</form>
 	{% endif %}
 {% endblock %}

requirements_for_test.txt

@@ -1,4 +1,6 @@
 -r requirements.txt
 pep8==1.5.7
 pytest==2.8.1
-pytest-mock==0.8.1
+pytest-mock==0.8.1
+moto==0.4.19
+httpretty==0.8.10

tests/app/main/views/test_sms.py

@@ -1,21 +1,18 @@
 from io import BytesIO
-from unittest import mock
+from flask import url_for
-from unittest.mock import mock_open
 
+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,
+        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')
             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,
+        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)
             upload_data = {'file': file_data}
-            with mock.patch('app.main.views.sms._open', m_open):
+            response = client.post(url_for('main.sendsms', service_id=123),
-                response = client.post('/services/123/sms/send',
+                                   data=upload_data,
-                                       data=upload_data,
+                                   follow_redirects=True)
-                                       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(url_for('main.sendsms', service_id=123),
-                response = client.post('/services/123/sms/send',
+                                   data=upload_data,
-                                       data=upload_data,
+                                   follow_redirects=True)
-                                       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,
+        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)
             upload_data = {'file': file_data}
-            with mock.patch('app.main.views.sms._open', m_open):
+            response = client.post(url_for('main.sendsms', service_id=123),
-                response = client.post('/services/123/sms/send',
+                                   data=upload_data,
-                                       data=upload_data,
+                                   follow_redirects=True)
-                                       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,
+@moto.mock_s3
-                                notify_db_session, mocker):
+def test_post_to_check_should_redirect_to_job(notifications_admin, notifications_admin_db, notify_db_session):
-    _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:
+            response = client.post(url_for('main.checksms',
-                s[456] = 'test.csv'
+                                           service_id=123,
-
+                                           upload_id='someid'))
-        response = client.post('/services/123/sms/check',
-                               data={'recipients': 'test.csv'})
-
         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