From ac0b8ed95c2a3db1601e4608e7b793a05cfda568 Mon Sep 17 00:00:00 2001
From: Rebecca Law <rebecca.law@digital.cabinet-office.gov.uk>
Date: Thu, 15 Apr 2021 13:51:48 +0100
Subject: [PATCH] If the job is for letters do not add the sender_id.

How this happens: a user starts to send a letter job, then in another tab starts a SMS
or email job, the sender_id is set in the session. Then the user goes
back to the letter job tab and creates the job. The sender_id is set in
the metadata of the csv file, and causes an exception when trying to persist
the letter notification.

This PR adds a check to ensure the sender_id is not set for letter jobs.

This will catch a small use case where the user has multiple tabs open
and has started sending an SMS or email job, then tries to send a letter
job.
---
 app/main/views/send.py            |  3 +-
 tests/app/main/views/test_send.py | 47 +++++++++++++++++++++++++++++++
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/app/main/views/send.py b/app/main/views/send.py
index 04356069e..50ab717a7 100644
--- a/app/main/views/send.py
+++ b/app/main/views/send.py
@@ -742,7 +742,8 @@ def check_messages(service_id, template_id, upload_id, row_index=2):
         'original_file_name': data.get('original_file_name', ''),
     }
 
-    if session.get('sender_id'):
+    if session.get('sender_id') and data['template'].template_type != 'letter':
+        # sender_id is not an option for sending letters.
         metadata_kwargs['sender_id'] = session['sender_id']
 
     set_metadata_on_csv_upload(service_id, upload_id, **metadata_kwargs)
diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py
index 302896ffd..e41561e60 100644
--- a/tests/app/main/views/test_send.py
+++ b/tests/app/main/views/test_send.py
@@ -3493,6 +3493,53 @@ def test_check_messages_adds_sender_id_in_session_to_metadata(
     )
 
 
+def test_check_messages_does_not_add_sender_id_in_session_to_metadata_for_letter_template(
+    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_get_jobs,
+    mock_s3_get_metadata,
+    mock_s3_set_metadata,
+    fake_uuid,
+):
+    mocker.patch('app.main.views.send.s3download', return_value="""
+            address_line_1,address_line_2,postcode,
+            First Last,    123 Street,    SW1 1AA
+        """)
+
+    mocker.patch(
+        'app.main.views.send.get_page_count_for_letter',
+        return_value=5,
+    )
+
+    with client_request.session_transaction() as session:
+        session['file_uploads'] = {
+            fake_uuid: {'template_id': fake_uuid}
+        }
+        session['sender_id'] = 'fake-sender'
+
+    client_request.get(
+        'main.check_messages',
+        service_id=SERVICE_ONE_ID,
+        template_id=fake_uuid,
+        upload_id=fake_uuid,
+        _test_page_title=False,
+    )
+
+    mock_s3_set_metadata.assert_called_once_with(
+        SERVICE_ONE_ID,
+        fake_uuid,
+        notification_count=1,
+        template_id=fake_uuid,
+        valid=True,
+        original_file_name='example.csv',
+    )
+
+
 @pytest.mark.parametrize('extra_args', (
     {},
     {'from_test': True},