From 3a471e2f61e24813e2a0fcc1cb6a1923de5c5ba5 Mon Sep 17 00:00:00 2001
From: Chris Hill-Scott <me@quis.cc>
Date: Thu, 10 May 2018 17:00:20 +0100
Subject: [PATCH] Stop overwriting S3 metadata with empty filename
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

When you upload a CSV the check page takes the file name from the URL
and writes it to the S3 metadata for that file.

This also happens when you go to the .png version of this page.

The .png version of the page doesn’t have the filename in the URL. So it
re-writes the S3 metadata with an empty filename.

This means that all letter jobs sent recently have no file names. This
commit fixes this problem.
---
 app/main/views/send.py            |  8 ++++----
 tests/app/main/views/test_send.py | 33 +++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index 5940c37be..6ea79fca3 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -476,7 +476,7 @@ def send_test_preview(service_id, template_id, filetype):
     return TemplatePreview.from_utils_template(template, filetype, page=request.args.get('page'))
 
 
-def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_pdf=False):
+def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_pdf=False, write_metadata=False):
 
     try:
         # The happy path is that the job doesn’t already exist, so the
@@ -561,7 +561,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_
     elif preview_row > 2:
         abort(404)
 
-    if any(recipients) and not recipients.has_errors:
+    if any(recipients) and not recipients.has_errors and write_metadata:
         set_metadata_on_csv_upload(
             service_id,
             upload_id,
@@ -604,7 +604,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_
 @user_has_permissions('send_messages', restrict_admin_usage=True)
 def check_messages(service_id, template_id, upload_id, row_index=2):
 
-    data = _check_messages(service_id, template_id, upload_id, row_index)
+    data = _check_messages(service_id, template_id, upload_id, row_index, write_metadata=True)
 
     if (
         data['recipients'].too_many_rows or
@@ -642,7 +642,7 @@ def check_messages_preview(service_id, template_id, upload_id, filetype, row_ind
         abort(404)
 
     template = _check_messages(
-        service_id, template_id, upload_id, row_index, letters_as_pdf=True
+        service_id, template_id, upload_id, row_index, letters_as_pdf=True, write_metadata=False,
     )['template']
     return TemplatePreview.from_utils_template(template, filetype)
 
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 83d1fcccc..2300beaf6 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -666,6 +666,39 @@ def test_upload_valid_csv_shows_preview_and_table(
             assert normalize_spaces(str(row.select('td')[index + 1])) == cell
 
 
+def test_upload_valid_csv_only_sets_meta_if_filename_known(
+    client_request,
+    mocker,
+    mock_get_live_service,
+    mock_get_service_letter_template,
+    mock_get_users_by_service,
+    mock_get_service_statistics,
+    mock_get_job_doesnt_exist,
+    mock_s3_set_metadata,
+    fake_uuid,
+):
+
+    mocker.patch('app.main.views.send.s3download', return_value="""
+        addressline1, addressline2, postcode
+        House       , 1 Street    , SW1A 1AA
+    """)
+    mocker.patch(
+        'app.main.views.send.TemplatePreview.from_utils_template',
+        return_value='foo'
+    )
+
+    client_request.get(
+        'main.check_messages_preview',
+        service_id=SERVICE_ONE_ID,
+        template_id=fake_uuid,
+        upload_id=fake_uuid,
+        filetype='pdf',
+        _test_page_title=False,
+    )
+
+    assert len(mock_s3_set_metadata.call_args_list) == 0
+
+
 def test_file_name_truncated_to_fit_in_s3_metadata(
     client_request,
     mocker,