fixed sessions overriding while uploading csvs

app/main/views/send.py

@@ -126,10 +126,14 @@ def send_messages(service_id, template_id):
                 Spreadsheet.from_file(form.file.data, filename=form.file.data.filename).as_dict,
                 current_app.config['AWS_REGION']
             )
-            session['upload_data'] = {
+            if 'file_uploads' not in session:
-                "template_id": template_id,
+                session['file_uploads'] = {}
-                "original_file_name": form.file.data.filename
+            session['file_uploads'].update({
-            }
+                upload_id: {
+                    "template_id": template_id,
+                    "original_file_name": form.file.data.filename
+                }
+            })
             return redirect(url_for('.check_messages',
                                     service_id=service_id,
                                     upload_id=upload_id,
@@ -468,7 +472,7 @@ def send_test_preview(service_id, template_id, filetype):
 
 def _check_messages(service_id, template_type, upload_id, preview_row, letters_as_pdf=False):
 
-    if not session.get('upload_data'):
+    if not session.get('file_uploads', {}).get(upload_id):
         # if we just return a `redirect` (302) object here, we'll get errors when we try and unpack in the
         # check_messages route - so raise a werkzeug.routing redirect to ensure that doesn't happen.
 
@@ -492,7 +496,7 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a
     template = get_template(
         service_api_client.get_service_template(
             service_id,
-            session['upload_data'].get('template_id')
+            session['file_uploads'][upload_id].get('template_id')
         )['data'],
         current_service,
         show_recipient=True,
@@ -536,8 +540,8 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a
     elif preview_row > 2:
         abort(404)
 
-    session['upload_data']['notification_count'] = len(recipients)
+    session['file_uploads'][upload_id]['notification_count'] = len(recipients)
-    session['upload_data']['valid'] = not recipients.has_errors
+    session['file_uploads'][upload_id]['valid'] = not recipients.has_errors
     return dict(
         recipients=recipients,
         template=template,
@@ -545,7 +549,7 @@ def _check_messages(service_id, template_type, upload_id, preview_row, letters_a
         row_errors=get_errors_for_csv(recipients, template.template_type),
         count_of_recipients=len(recipients),
         count_of_displayed_recipients=len(list(recipients.displayed_rows)),
-        original_file_name=session['upload_data'].get('original_file_name'),
+        original_file_name=session['file_uploads'][upload_id].get('original_file_name'),
         upload_id=upload_id,
         form=CsvUploadForm(),
         remaining_messages=remaining_messages,
@@ -610,24 +614,26 @@ def check_messages_preview(service_id, template_type, upload_id, filetype, row_i
 @login_required
 @user_has_permissions('send_messages', restrict_admin_usage=True)
 def recheck_messages(service_id, template_type, upload_id, row_index=0):
+    if not session.get('file_uploads', {}).get(upload_id):
+        return redirect(url_for('main.choose_template', service_id=service_id), code=301)
 
-    if not session.get('upload_data'):
+    return send_messages(service_id, session['file_uploads'][upload_id].get('template_id'))
-        return redirect(url_for('main.choose_template', service_id=service_id))
-
-    return send_messages(service_id, session['upload_data'].get('template_id'))
 
 
 @main.route("/services/<service_id>/start-job/<upload_id>", methods=['POST'])
 @login_required
 @user_has_permissions('send_messages', restrict_admin_usage=True)
 def start_job(service_id, upload_id):
-    upload_data = session['upload_data']
+    try:
+        upload_data = session['file_uploads'][upload_id]
+    except KeyError:
+        return redirect(url_for('main.choose_template', service_id=service_id), code=301)
 
     if request.files or not upload_data.get('valid'):
         # The csv was invalid, validate the csv again
         return send_messages(service_id, upload_data.get('template_id'))
 
-    session.pop('upload_data')
+    session['file_uploads'].pop(upload_id)
 
     job_api_client.create_job(
         upload_id,
@@ -700,7 +706,9 @@ def make_and_upload_csv_file(service_id, template):
         ).as_dict,
         current_app.config['AWS_REGION'],
     )
-    session['upload_data'] = {
+    if 'file_uploads' not in session:
+        session['file_uploads'] = {}
+    session['file_uploads'][upload_id] = {
         "template_id": template.id,
         "original_file_name": current_app.config['TEST_MESSAGE_FILENAME']
     }

tests/app/main/views/test_send.py

@@ -262,12 +262,8 @@ def test_should_not_allow_files_to_be_uploaded_without_the_correct_permission(
 
 @pytest.mark.parametrize(
     "filename, acceptable_file",
-    list(zip(
+    list(zip(test_spreadsheet_files, repeat(True))) +
-        test_spreadsheet_files, repeat(True)
+    list(zip(test_non_spreadsheet_files, repeat(False)))
-    )) +
-    list(zip(
-        test_non_spreadsheet_files, repeat(False)
-    ))
 )
 def test_upload_files_in_different_formats(
     filename,
@@ -279,7 +275,6 @@ def test_upload_files_in_different_formats(
     mock_s3_upload,
     fake_uuid,
 ):
-
     with open(filename, 'rb') as uploaded:
         response = logged_in_client.post(
             url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid),
@@ -383,12 +378,14 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors(
         content_type='multipart/form-data',
         follow_redirects=True
     )
+
     reupload = logged_in_client.post(
-        url_for('main.check_messages', service_id=service_one['id'], template_type='sms', upload_id='abc123'),
+        url_for('main.check_messages', service_id=service_one['id'], template_type='sms', upload_id=fake_uuid),
         data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')},
         content_type='multipart/form-data',
         follow_redirects=True
     )
+
     for response in [initial_upload, reupload]:
         assert response.status_code == 200
         content = response.get_data(as_text=True)
@@ -579,7 +576,9 @@ def test_upload_valid_csv_shows_preview_and_table(
 ):
 
     with client_request.session_transaction() as session:
-        session['upload_data'] = {'template_id': fake_uuid}
+        session['file_uploads'] = {
+            fake_uuid: {'template_id': fake_uuid}
+        }
 
     mocker.patch('app.main.views.send.s3download', return_value="""
         phone number,name,thing,thing,thing
@@ -666,7 +665,9 @@ def test_show_all_columns_if_there_are_duplicate_recipient_columns(
 ):
 
     with client_request.session_transaction() as session:
-        session['upload_data'] = {'template_id': fake_uuid}
+        session['file_uploads'] = {
+            fake_uuid: {'template_id': fake_uuid}
+        }
 
     mocker.patch('app.main.views.send.s3download', return_value="""
         phone number, phone_number, PHONENUMBER
@@ -710,7 +711,9 @@ def test_404_for_previewing_a_row_out_of_range(
 ):
 
     with client_request.session_transaction() as session:
-        session['upload_data'] = {'template_id': fake_uuid}
+        session['file_uploads'] = {
+            fake_uuid: {'template_id': fake_uuid}
+        }
 
     mocker.patch('app.main.views.send.s3download', return_value="""
         phone number,name,thing,thing,thing
@@ -1443,9 +1446,9 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers(
         follow_redirects=True
     )
     with logged_in_client.session_transaction() as sess:
-        assert sess['upload_data']['template_id'] == fake_uuid
+        assert sess['file_uploads'][fake_uuid]['template_id'] == fake_uuid
-        assert sess['upload_data']['original_file_name'] == 'valid.csv'
+        assert sess['file_uploads'][fake_uuid]['original_file_name'] == 'valid.csv'
-        assert sess['upload_data']['notification_count'] == 53
+        assert sess['file_uploads'][fake_uuid]['notification_count'] == 53
 
     content = response.get_data(as_text=True)
     assert response.status_code == 200
@@ -1503,14 +1506,16 @@ def test_test_message_can_only_be_sent_now(
     mock_get_detailed_service_for_today,
     fake_uuid
 ):
-
     with logged_in_client.session_transaction() as session:
-        session['upload_data'] = {
+        session['file_uploads'] = {
-            'original_file_name': 'Test message',
+            fake_uuid: {
-            'template_id': fake_uuid,
+                'original_file_name': 'Test message',
-            'notification_count': 1,
+                'template_id': fake_uuid,
-            'valid': True
+                'notification_count': 1,
+                'valid': True
+            }
         }
+
     response = logged_in_client.get(url_for(
         'main.check_messages',
         service_id=service_one['id'],
@@ -1537,11 +1542,13 @@ def test_letter_can_only_be_sent_now(
     mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1)
 
     with logged_in_client.session_transaction() as session:
-        session['upload_data'] = {
+        session['file_uploads'] = {
-            'original_file_name': 'Test message',
+            fake_uuid: {
-            'template_id': fake_uuid,
+                'original_file_name': 'Test message',
-            'notification_count': 1,
+                'template_id': fake_uuid,
-            'valid': True
+                'notification_count': 1,
+                'valid': True
+            }
         }
 
     response = logged_in_client.get(url_for(
@@ -1577,12 +1584,15 @@ def test_create_job_should_call_api(
     template_id = data['template']
     notification_count = data['notification_count']
     with logged_in_client.session_transaction() as session:
-        session['upload_data'] = {
+        session['file_uploads'] = {
-            'original_file_name': original_file_name,
+            fake_uuid: {
-            'template_id': template_id,
+                'original_file_name': original_file_name,
-            'notification_count': notification_count,
+                'template_id': template_id,
-            'valid': True
+                'notification_count': notification_count,
+                'valid': True
+            }
         }
+
     url = url_for('main.start_job', service_id=service_one['id'], upload_id=job_id)
     response = logged_in_client.post(url, data={'scheduled_for': when}, follow_redirects=True)
 
@@ -1604,14 +1614,16 @@ def test_can_start_letters_job(
     service_one,
     fake_uuid
 ):
-
     with logged_in_platform_admin_client.session_transaction() as session:
-        session['upload_data'] = {
+        session['file_uploads'] = {
-            'original_file_name': 'example.csv',
+            fake_uuid: {
-            'template_id': fake_uuid,
+                'original_file_name': 'example.csv',
-            'notification_count': 123,
+                'template_id': fake_uuid,
-            'valid': True
+                'notification_count': 123,
+                'valid': True
+            }
         }
+
     response = logged_in_platform_admin_client.post(
         url_for('main.start_job', service_id=service_one['id'], upload_id=fake_uuid),
         data={}
@@ -1667,12 +1679,15 @@ def test_should_show_preview_letter_message(
     service_id = service_one['id']
     template_id = fake_uuid
     with logged_in_platform_admin_client.session_transaction() as session:
-        session['upload_data'] = {
+        session['file_uploads'] = {
-            'original_file_name': 'example.csv',
+            fake_uuid: {
-            'template_id': fake_uuid,
+                'original_file_name': 'example.csv',
-            'notification_count': 1,
+                'template_id': fake_uuid,
-            'valid': True
+                'notification_count': 1,
+                'valid': True
+            }
         }
+
     response = logged_in_platform_admin_client.get(
         url_for(
             'main.check_messages_preview',
@@ -1736,10 +1751,15 @@ def test_check_messages_should_revalidate_file_when_uploading_file(
     )
     data = mock_get_job(SERVICE_ONE_ID, fake_uuid)['data']
     with logged_in_client.session_transaction() as session:
-        session['upload_data'] = {'original_file_name': 'invalid.csv',
+        session['file_uploads'] = {
-                                  'template_id': data['template'],
+            fake_uuid: {
-                                  'notification_count': data['notification_count'],
+                'original_file_name': 'invalid.csv',
-                                  'valid': True}
+                'template_id': data['template'],
+                'notification_count': data['notification_count'],
+                'valid': True
+            }
+        }
+
     response = logged_in_client.post(
         url_for('main.start_job', service_id=SERVICE_ONE_ID, upload_id=data['id']),
         data={'file': (BytesIO(''.encode('utf-8')), 'invalid.csv')},
@@ -1901,10 +1921,15 @@ def test_check_messages_back_link(
 ):
     template_mock(mocker)
     with logged_in_client.session_transaction() as session:
-        session['upload_data'] = {'original_file_name': 'valid.csv',
+        session['file_uploads'] = {
-                                  'template_id': fake_uuid,
+            fake_uuid: {
-                                  'notification_count': 1,
+                'original_file_name': 'valid.csv',
-                                  'valid': True}
+                'template_id': fake_uuid,
+                'notification_count': 1,
+                'valid': True
+            }
+        }
+
     response = logged_in_client.get(url_for(
         'main.check_messages',
         service_id=fake_uuid,
@@ -1990,10 +2015,15 @@ def test_check_messages_shows_too_many_messages_errors(
     })
 
     with logged_in_client.session_transaction() as session:
-        session['upload_data'] = {'original_file_name': 'valid.csv',
+        session['file_uploads'] = {
-                                  'template_id': fake_uuid,
+            fake_uuid: {
-                                  'notification_count': 1,
+                'original_file_name': 'valid.csv',
-                                  'valid': True}
+                'template_id': fake_uuid,
+                'notification_count': 1,
+                'valid': True
+            }
+        }
+
     response = logged_in_client.get(url_for(
         'main.check_messages',
         service_id=fake_uuid,
@@ -2018,18 +2048,25 @@ def test_check_messages_shows_trial_mode_error(
     mock_get_service_template,
     mock_has_permissions,
     mock_get_detailed_service_for_today,
+    fake_uuid,
     mocker
 ):
     mocker.patch('app.main.views.send.s3download', return_value=(
         'phone number,\n07900900321'  # Not in team
     ))
+
     with logged_in_client.session_transaction() as session:
-        session['upload_data'] = {'template_id': ''}
+        session['file_uploads'] = {
+            fake_uuid: {
+                'template_id': '',
+            }
+        }
+
     response = logged_in_client.get(url_for(
         'main.check_messages',
         service_id=uuid.uuid4(),
         template_type='sms',
-        upload_id=uuid.uuid4()
+        upload_id=fake_uuid
     ))
     assert response.status_code == 200
     page = BeautifulSoup(response.data.decode('utf-8'), 'html.parser')
@@ -2057,13 +2094,13 @@ def test_check_messages_shows_trial_mode_error_for_letters(
     mock_has_permissions,
     mock_get_users_by_service,
     mock_get_detailed_service_for_today,
+    fake_uuid,
     mocker,
     service_mock,
     error_should_be_shown,
     number_of_rows,
     expected_error_message,
 ):
-
     service_mock(mocker, api_user_active)
 
     mocker.patch('app.main.views.send.s3download', return_value='\n'.join(
@@ -2072,13 +2109,17 @@ def test_check_messages_shows_trial_mode_error_for_letters(
     ))
 
     with client_request.session_transaction() as session:
-        session['upload_data'] = {'template_id': ''}
+        session['file_uploads'] = {
+            fake_uuid: {
+                'template_id': '',
+            }
+        }
 
     page = client_request.get(
         'main.check_messages',
         service_id=SERVICE_ONE_ID,
         template_type='letter',
-        upload_id=uuid.uuid4(),
+        upload_id=fake_uuid,
         _test_page_title=False,
     )
 
@@ -2100,6 +2141,7 @@ def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters(
     mock_get_service_letter_template,
     mock_has_permissions,
     mock_get_users_by_service,
+    fake_uuid,
     mock_get_detailed_service_for_today,
 ):
 
@@ -2109,13 +2151,18 @@ def test_check_messages_shows_data_errors_before_trial_mode_errors_for_letters(
     ))
 
     with client_request.session_transaction() as session:
-        session['upload_data'] = {'template_id': '', 'original_file_name': 'example.xlsx'}
+        session['file_uploads'] = {
+            fake_uuid: {
+                'template_id': '',
+                'original_file_name': 'example.xlsx',
+            }
+        }
 
     page = client_request.get(
         'main.check_messages',
         service_id=SERVICE_ONE_ID,
         template_type='letter',
-        upload_id=uuid.uuid4(),
+        upload_id=fake_uuid,
         _test_page_title=False,
     )
 
@@ -2131,6 +2178,7 @@ def test_check_messages_column_error_doesnt_show_optional_columns(
     client_request,
     mock_get_service_letter_template,
     mock_has_permissions,
+    fake_uuid,
     mock_get_users_by_service,
     mock_get_detailed_service_for_today,
 ):
@@ -2141,13 +2189,18 @@ def test_check_messages_column_error_doesnt_show_optional_columns(
     ))
 
     with client_request.session_transaction() as session:
-        session['upload_data'] = {'template_id': '', 'original_file_name': ''}
+        session['file_uploads'] = {
+            fake_uuid: {
+                'template_id': '',
+                'original_file_name': '',
+            }
+        }
 
     page = client_request.get(
         'main.check_messages',
         service_id=SERVICE_ONE_ID,
         template_type='letter',
-        upload_id=uuid.uuid4(),
+        upload_id=fake_uuid,
         _test_page_title=False,
     )
 
@@ -2164,6 +2217,7 @@ def test_generate_test_letter_doesnt_block_in_trial_mode(
     mock_get_service,
     mock_get_service_letter_template,
     mock_has_permissions,
+    fake_uuid,
     mock_get_users_by_service,
     mock_get_detailed_service_for_today,
 ):
@@ -2174,13 +2228,17 @@ def test_generate_test_letter_doesnt_block_in_trial_mode(
     """)
 
     with client_request.session_transaction() as session:
-        session['upload_data'] = {'template_id': ''}
+        session['file_uploads'] = {
+            fake_uuid: {
+                'template_id': '',
+            }
+        }
 
     page = client_request.get(
         'main.check_messages',
         service_id=SERVICE_ONE_ID,
         template_type='letter',
-        upload_id=uuid.uuid4(),
+        upload_id=fake_uuid,
         from_test=True,
         _test_page_title=False,
     )
@@ -2209,7 +2267,12 @@ def test_check_messages_shows_over_max_row_error(
     mock_recipients.too_many_rows.return_value = True
 
     with logged_in_client.session_transaction() as session:
-        session['upload_data'] = {'template_id': fake_uuid}
+        session['file_uploads'] = {
+            fake_uuid: {
+                'template_id': fake_uuid,
+            }
+        }
+
     response = logged_in_client.get(url_for(
         'main.check_messages',
         service_id=fake_uuid,
@@ -2250,9 +2313,11 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error(
     )
 
     with logged_in_client.session_transaction() as session:
-        session['upload_data'] = {
+        session['file_uploads'] = {
-            'template_id': fake_uuid,
+            fake_uuid: {
-            'original_file_name': 'unicode.csv',
+                'template_id': fake_uuid,
+                'original_file_name': 'unicode.csv',
+            }
         }
 
     response = logged_in_client.get(url_for(
@@ -2613,11 +2678,11 @@ def test_reply_to_is_previewed_if_chosen(
     mock_get_users_by_service,
     mock_get_detailed_service_for_today,
     get_default_reply_to_email_address,
+    fake_uuid,
     endpoint,
     extra_args,
     reply_to_address,
 ):
-
     mocker.patch('app.main.views.send.s3download', return_value="""
         email_address,date,thing
         notify@digital.cabinet-office.gov.uk,foo,bar
@@ -2626,11 +2691,13 @@ def test_reply_to_is_previewed_if_chosen(
     with client_request.session_transaction() as session:
         session['recipient'] = 'notify@digital.cabinet-office.gov.uk'
         session['placeholders'] = {}
-        session['upload_data'] = {
+        session['file_uploads'] = {
-            'original_file_name': 'example.csv',
+            fake_uuid: {
-            'template_id': fake_uuid(),
+                'original_file_name': 'example.csv',
-            'notification_count': 1,
+                'template_id': fake_uuid,
-            'valid': True
+                'notification_count': 1,
+                'valid': True
+            }
         }
         session['sender_id'] = reply_to_address
 
@@ -2664,6 +2731,7 @@ def test_sms_sender_is_previewed(
     mock_get_users_by_service,
     mock_get_detailed_service_for_today,
     get_default_sms_sender,
+    fake_uuid,
     endpoint,
     extra_args,
     sms_sender,
@@ -2677,11 +2745,13 @@ def test_sms_sender_is_previewed(
     with client_request.session_transaction() as session:
         session['recipient'] = '7700900986'
         session['placeholders'] = {}
-        session['upload_data'] = {
+        session['file_uploads'] = {
-            'original_file_name': 'example.csv',
+            fake_uuid: {
-            'template_id': fake_uuid(),
+                'original_file_name': 'example.csv',
-            'notification_count': 1,
+                'template_id': fake_uuid,
-            'valid': True
+                'notification_count': 1,
+                'valid': True
+            }
         }
         session['sender_id'] = sms_sender
 
@@ -2697,3 +2767,74 @@ def test_sms_sender_is_previewed(
         assert sms_sender_on_page.text.strip() == 'From: GOVUK'
     else:
         assert not sms_sender_on_page
+
+
+@pytest.mark.parametrize('endpoint, request_type, extra_args', [
+    (
+        'main.check_messages',
+        'GET',
+        {
+            'template_type': 'email',
+            'upload_id': fake_uuid(),
+        }
+    ),
+    (
+        'main.check_messages_preview',
+        'GET',
+        {
+            'template_type': 'email',
+            'upload_id': fake_uuid(),
+            'filetype': 'png'
+        }
+    ),
+    (
+        'main.recheck_messages',
+        'POST',
+        {
+            'template_type': 'email',
+            'upload_id': fake_uuid()
+        }
+    ),
+    (
+        'main.start_job',
+        'POST',
+        {
+            'upload_id': fake_uuid()
+        }
+    )
+])
+@pytest.mark.parametrize('session_data', [
+    {},
+    {
+        '6ce466d0-fd6a-11e5-82f5-e0accb9d11a4': {
+            'template_id': '1234'
+        }
+    }
+])
+def test_redirects_to_choose_template_if_no_session_exists_for_upload_id(
+    client_request,
+    endpoint,
+    request_type,
+    session_data,
+    extra_args,
+    fake_uuid
+):
+    with client_request.session_transaction() as session:
+        session['file_uploads'] = session_data
+
+    if request_type == 'GET':
+        client_request.get(
+            endpoint,
+            service_id=SERVICE_ONE_ID,
+            **extra_args,
+            _expected_status=301,
+            _expected_redirect=url_for('main.choose_template', service_id=SERVICE_ONE_ID, _external=True)
+        )
+    else:
+        client_request.post(
+            endpoint,
+            service_id=SERVICE_ONE_ID,
+            **extra_args,
+            _expected_status=301,
+            _expected_redirect=url_for('main.choose_template', service_id=SERVICE_ONE_ID, _external=True)
+        )

tests/conftest.py

@@ -2516,6 +2516,7 @@ def client_request(logged_in_client):
             endpoint,
             _expected_status=200,
             _follow_redirects=False,
+            _expected_redirect=None,
             _test_page_title=True,
             **endpoint_kwargs
         ):
@@ -2524,6 +2525,8 @@ def client_request(logged_in_client):
                 follow_redirects=_follow_redirects,
             )
             assert resp.status_code == _expected_status
+            if _expected_redirect:
+                assert resp.location == _expected_redirect
             page = BeautifulSoup(resp.data.decode('utf-8'), 'html.parser')
             if _test_page_title:
                 page_title, h1 = (