Merge pull request #566 from alphagov/refactor-sending

Refactor and tidy up sending

app/main/uploader.py

@@ -1,3 +1,4 @@
+import uuid
 import botocore
 from boto3 import resource
 from flask import current_app
@@ -5,7 +6,7 @@ from flask import current_app
 FILE_LOCATION_STRUCTURE = 'service-{}-notify/{}.csv'
 
 
-def s3upload(upload_id, service_id, filedata, region):
+def s3upload(service_id, filedata, region):
     s3 = resource('s3')
     bucket_name = current_app.config['CSV_UPLOAD_BUCKET_NAME']
     contents = filedata['data']
@@ -27,10 +28,13 @@ def s3upload(upload_id, service_id, filedata, region):
         s3.create_bucket(Bucket=bucket_name,
                          CreateBucketConfiguration={'LocationConstraint': region})
 
+    upload_id = str(uuid.uuid4())
     upload_file_name = FILE_LOCATION_STRUCTURE.format(service_id, upload_id)
     key = s3.Object(bucket_name, upload_file_name)
     key.put(Body=contents, ServerSideEncryption='AES256')
 
+    return upload_id
+
 
 def s3download(service_id, upload_id):
     contents = ''

app/main/views/send.py

@@ -110,11 +110,9 @@ def send_messages(service_id, template_id):
     form = CsvUploadForm()
     if form.validate_on_submit():
         try:
-            upload_id = str(uuid.uuid4())
+            upload_id = s3upload(
-            s3upload(
-                upload_id,
                 service_id,
-                Spreadsheet.from_file(form.file.data.filename, form.file.data).as_dict,
+                Spreadsheet.from_file(form.file.data, filename=form.file.data.filename).as_dict,
                 current_app.config['AWS_REGION']
             )
             session['upload_data'] = {
@@ -144,16 +142,13 @@ def send_messages(service_id, template_id):
 @user_has_permissions('send_texts', 'send_emails', 'send_letters', 'manage_templates', any_=True)
 def get_example_csv(service_id, template_id):
     template = Template(service_api_client.get_service_template(service_id, template_id)['data'])
-    with io.StringIO() as output:
+    return Spreadsheet.from_rows([
-        writer = csv.writer(output)
+        [first_column_heading[template.template_type]] + list(template.placeholders),
-        writer.writerows([
+        get_example_csv_rows(template)
-            [first_column_heading[template.template_type]] + list(template.placeholders),
+    ]).as_csv_data, 200, {
-            get_example_csv_rows(template)
+        'Content-Type': 'text/csv; charset=utf-8',
-        ])
+        'Content-Disposition': 'inline; filename="{}.csv"'.format(template.name)
-        return output.getvalue(), 200, {
+    }
-            'Content-Type': 'text/csv; charset=utf-8',
-            'Content-Disposition': 'inline; filename="{}.csv"'.format(template.name)
-        }
 
 
 @main.route("/services/<service_id>/send/<template_id>/test", methods=['GET', 'POST'])
@@ -161,32 +156,36 @@ def get_example_csv(service_id, template_id):
 @user_has_permissions('send_texts', 'send_emails', 'send_letters')
 def send_test(service_id, template_id):
 
+    file_name = 'Test message'
+
     template = Template(
         service_api_client.get_service_template(service_id, template_id)['data'],
         prefix=current_service['name']
     )
 
     if len(template.placeholders) == 0 or request.method == 'POST':
-        with io.StringIO() as output:
+        upload_id = s3upload(
-            writer = csv.writer(output)
+            service_id,
-            writer.writerows([
+            {
-                [first_column_heading[template.template_type]] + list(template.placeholders),
+                'file_name': file_name,
-                get_example_csv_rows(template, use_example_as_example=False, submitted_fields=request.form)
+                'data': Spreadsheet.from_rows([
-            ])
+                    [first_column_heading[template.template_type]] + list(template.placeholders),
-            filedata = {
+                    get_example_csv_rows(template, use_example_as_example=False, submitted_fields=request.form)
-                'file_name': 'Test message',
+                ]).as_csv_data
-                'data': output.getvalue()
+            },
-            }
+            current_app.config['AWS_REGION']
-            upload_id = str(uuid.uuid4())
+        )
-            s3upload(upload_id, service_id, filedata, current_app.config['AWS_REGION'])
+        session['upload_data'] = {
-            session['upload_data'] = {"template_id": template_id, "original_file_name": filedata['file_name']}
+            "template_id": template_id,
-            return redirect(url_for(
+            "original_file_name": file_name
-                '.check_messages',
+        }
-                upload_id=upload_id,
+        return redirect(url_for(
-                service_id=service_id,
+            '.check_messages',
-                template_type=template.template_type,
+            upload_id=upload_id,
-                from_test=True
+            service_id=service_id,
-            ))
+            template_type=template.template_type,
+            from_test=True
+        ))
 
     return render_template(
         'views/send-test.html',

app/utils.py

@@ -143,7 +143,7 @@ class Spreadsheet():
 
     allowed_file_extensions = ['csv', 'xlsx', 'xls', 'ods', 'xlsm', 'tsv']
 
-    def __init__(self, filename, csv_data):
+    def __init__(self, csv_data, filename=''):
         self.filename = filename
         self.as_csv_data = csv_data
         self.as_dict = {
@@ -164,24 +164,28 @@ class Spreadsheet():
         return '\r\n'.join(file_content.getvalue().decode('utf-8').splitlines())
 
     @classmethod
-    def from_file(cls, filename, file_content):
+    def from_rows(cls, rows, filename=''):
+
+        with StringIO() as converted:
+            output = csv.writer(converted)
+
+            for row in rows:
+                output.writerow(row)
+
+            return cls(converted.getvalue(), filename)
+
+    @classmethod
+    def from_file(cls, file_content, filename=''):
 
         extension = cls.get_extension(filename)
 
         if extension == 'csv':
-            return cls(filename, Spreadsheet.normalise_newlines(file_content))
+            return cls(Spreadsheet.normalise_newlines(file_content), filename)
 
         if extension == 'tsv':
             file_content = StringIO(Spreadsheet.normalise_newlines(file_content))
 
-        with StringIO() as converted:
+        return cls.from_rows(pyexcel.get_sheet(
-
+            file_type=extension,
-            output = csv.writer(converted)
+            file_content=file_content.getvalue()
-
+        ).to_array(), filename)
-            for row in pyexcel.get_sheet(
-                file_type=extension,
-                file_content=file_content.getvalue()
-            ).to_array():
-                output.writerow(row)
-
-            return cls(filename, converted.getvalue())

tests/app/main/views/test_send.py

@@ -53,7 +53,7 @@ def test_upload_files_in_different_formats(
         )
 
     if acceptable_file:
-        assert mock_s3_upload.call_args[0][2]['data'].strip() == (
+        assert mock_s3_upload.call_args[0][1]['data'].strip() == (
             "phone number,name,favourite colour,fruit\r\n"
             "07739 468 050,Pete,Coral,tomato\r\n"
             "07527 125 974,Not Pete,Magenta,Avacado\r\n"
@@ -157,7 +157,7 @@ def test_send_test_sms_message(
                 follow_redirects=True
             )
         assert response.status_code == 200
-        mock_s3_upload.assert_called_with(ANY, fake_uuid, expected_data, 'eu-west-1')
+        mock_s3_upload.assert_called_with(fake_uuid, expected_data, 'eu-west-1')
 
 
 def test_send_test_email_message(
@@ -185,7 +185,7 @@ def test_send_test_email_message(
                 follow_redirects=True
             )
         assert response.status_code == 200
-        mock_s3_upload.assert_called_with(ANY, fake_uuid, expected_data, 'eu-west-1')
+        mock_s3_upload.assert_called_with(fake_uuid, expected_data, 'eu-west-1')
 
 
 def test_send_test_sms_message_with_placeholders(
@@ -221,7 +221,7 @@ def test_send_test_sms_message_with_placeholders(
                 follow_redirects=True
             )
         assert response.status_code == 200
-        mock_s3_upload.assert_called_with(ANY, fake_uuid, expected_data, 'eu-west-1')
+        mock_s3_upload.assert_called_with(fake_uuid, expected_data, 'eu-west-1')
 
 
 def test_api_info_page(

tests/conftest.py

@@ -862,8 +862,8 @@ def mock_get_users_by_service(mocker):
 
 @pytest.fixture(scope='function')
 def mock_s3_upload(mocker):
-    def _upload(upload_id, service_id, filedata, region):
+    def _upload(service_id, filedata, region):
-        pass
+        return fake_uuid()
     return mocker.patch('app.main.views.send.s3upload', side_effect=_upload)