diff --git a/app/main/views/send.py b/app/main/views/send.py index 968cfa92d..e9066770c 100644 --- a/app/main/views/send.py +++ b/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'] = { - "template_id": template_id, - "original_file_name": form.file.data.filename - } + if 'file_uploads' not in session: + session['file_uploads'] = {} + 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, @@ -469,7 +473,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. @@ -493,7 +497,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, @@ -537,8 +541,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['upload_data']['valid'] = not recipients.has_errors + session['file_uploads'][upload_id]['notification_count'] = len(recipients) + session['file_uploads'][upload_id]['valid'] = not recipients.has_errors return dict( recipients=recipients, template=template, @@ -546,7 +550,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, @@ -611,24 +615,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 redirect(url_for('main.choose_template', service_id=service_id)) - - return send_messages(service_id, session['upload_data'].get('template_id')) + return send_messages(service_id, session['file_uploads'][upload_id].get('template_id')) @main.route("/services//start-job/", 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, @@ -701,7 +707,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'] } diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 61e50dd4e..4db0e94b6 100644 --- a/tests/app/main/views/test_send.py +++ b/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( - test_spreadsheet_files, repeat(True) - )) + - list(zip( - test_non_spreadsheet_files, repeat(False) - )) + list(zip(test_spreadsheet_files, repeat(True))) + + 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['upload_data']['original_file_name'] == 'valid.csv' - assert sess['upload_data']['notification_count'] == 53 + assert sess['file_uploads'][fake_uuid]['template_id'] == fake_uuid + assert sess['file_uploads'][fake_uuid]['original_file_name'] == 'valid.csv' + 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'] = { - 'original_file_name': 'Test message', - 'template_id': fake_uuid, - 'notification_count': 1, - 'valid': True + session['file_uploads'] = { + fake_uuid: { + 'original_file_name': 'Test message', + 'template_id': fake_uuid, + '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'] = { - 'original_file_name': 'Test message', - 'template_id': fake_uuid, - 'notification_count': 1, - 'valid': True + session['file_uploads'] = { + fake_uuid: { + 'original_file_name': 'Test message', + 'template_id': fake_uuid, + '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'] = { - 'original_file_name': original_file_name, - 'template_id': template_id, - 'notification_count': notification_count, - 'valid': True + session['file_uploads'] = { + fake_uuid: { + 'original_file_name': original_file_name, + 'template_id': template_id, + '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'] = { - 'original_file_name': 'example.csv', - 'template_id': fake_uuid, - 'notification_count': 123, - 'valid': True + session['file_uploads'] = { + fake_uuid: { + 'original_file_name': 'example.csv', + 'template_id': fake_uuid, + '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'] = { - 'original_file_name': 'example.csv', - 'template_id': fake_uuid, - 'notification_count': 1, - 'valid': True + session['file_uploads'] = { + fake_uuid: { + 'original_file_name': 'example.csv', + 'template_id': fake_uuid, + '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', - 'template_id': data['template'], - 'notification_count': data['notification_count'], - 'valid': True} + session['file_uploads'] = { + fake_uuid: { + 'original_file_name': 'invalid.csv', + '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', - 'template_id': fake_uuid, - 'notification_count': 1, - 'valid': True} + session['file_uploads'] = { + fake_uuid: { + 'original_file_name': 'valid.csv', + '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', - 'template_id': fake_uuid, - 'notification_count': 1, - 'valid': True} + session['file_uploads'] = { + fake_uuid: { + 'original_file_name': 'valid.csv', + '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'] = { - 'template_id': fake_uuid, - 'original_file_name': 'unicode.csv', + session['file_uploads'] = { + fake_uuid: { + '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'] = { - 'original_file_name': 'example.csv', - 'template_id': fake_uuid(), - 'notification_count': 1, - 'valid': True + session['file_uploads'] = { + fake_uuid: { + 'original_file_name': 'example.csv', + 'template_id': fake_uuid, + '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'] = { - 'original_file_name': 'example.csv', - 'template_id': fake_uuid(), - 'notification_count': 1, - 'valid': True + session['file_uploads'] = { + fake_uuid: { + 'original_file_name': 'example.csv', + 'template_id': fake_uuid, + '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) + ) diff --git a/tests/conftest.py b/tests/conftest.py index 15a97154d..a975d1de1 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -2519,6 +2519,7 @@ def client_request(logged_in_client): endpoint, _expected_status=200, _follow_redirects=False, + _expected_redirect=None, _test_page_title=True, **endpoint_kwargs ): @@ -2527,6 +2528,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 = (