From 6940291c966845b6896d8a96dcfa19fb9c14ef12 Mon Sep 17 00:00:00 2001 From: Katie Smith Date: Fri, 23 Oct 2020 16:10:03 +0100 Subject: [PATCH] Put filename in metadata when sending via a CSV When sending from an uploaded CSV `.send_messages` now puts the filename in the metadata. It previously used the query string to pass the filename to `.check_messages`, where it can be lost. --- app/main/views/send.py | 10 +++++- tests/app/main/views/test_send.py | 51 +++++++++++++++++++++++++++++-- 2 files changed, 58 insertions(+), 3 deletions(-) diff --git a/app/main/views/send.py b/app/main/views/send.py index 8e6fe3cb9..95f3cd55d 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -159,12 +159,20 @@ def send_messages(service_id, template_id): Spreadsheet.from_file_form(form).as_dict, current_app.config['AWS_REGION'] ) + file_name_metadata = unicode_truncate( + SanitiseASCII.encode(form.file.data.filename), + 1600 + ) + set_metadata_on_csv_upload( + service_id, + upload_id, + original_file_name=file_name_metadata + ) return redirect(url_for( '.check_messages', service_id=service_id, upload_id=upload_id, template_id=template.id, - original_file_name=form.file.data.filename, )) except (UnicodeDecodeError, BadZipFile, XLRDError): flash('Could not read {}. Try using a different file format.'.format( diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index a2c30a4ec..332d52332 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -356,6 +356,7 @@ def test_upload_files_in_different_formats( service_one, mocker, mock_get_service_template, + mock_s3_set_metadata, mock_s3_upload, fake_uuid, ): @@ -374,6 +375,11 @@ def test_upload_files_in_different_formats( "07527 125 974,Not Pete,Magenta,Avacado\r\n" "07512 058 823,Still Not Pete,Crimson,Pear" ) + mock_s3_set_metadata.assert_called_once_with( + SERVICE_ONE_ID, + fake_uuid, + original_file_name=filename + ) else: assert not mock_s3_upload.called assert normalize_spaces(page.select_one('.banner-dangerous').text) == ( @@ -381,6 +387,34 @@ def test_upload_files_in_different_formats( ) +def test_send_messages_sanitises_and_truncates_file_name_for_metadata( + logged_in_client, + service_one, + mocker, + mock_get_service_template_with_placeholders, + mock_s3_set_metadata, + mock_s3_get_metadata, + mock_s3_upload, + mock_s3_download, + mock_get_job_doesnt_exist, + fake_uuid, +): + filename = f"😁{'a' * 2000}.csv" + + logged_in_client.post( + url_for('main.send_messages', service_id=service_one['id'], template_id=fake_uuid), + data={'file': (BytesIO(''.encode('utf-8')), filename)}, + content_type='multipart/form-data', + follow_redirects=False + ) + + assert len( + mock_s3_set_metadata.call_args_list[0][1]['original_file_name'] + ) < len(filename) + + assert mock_s3_set_metadata.call_args_list[0][1]['original_file_name'].startswith('?') + + @pytest.mark.parametrize('exception, expected_error_message', [ (partial(UnicodeDecodeError, 'codec', b'', 1, 2, 'reason'), ( 'Could not read example.xlsx. Try using a different file format.' @@ -442,6 +476,7 @@ def test_upload_csv_file_with_errors_shows_check_page_with_errors( service_one, mocker, mock_get_service_template_with_placeholders, + mock_s3_set_metadata, mock_s3_get_metadata, mock_s3_upload, mock_get_users_by_service, @@ -483,6 +518,7 @@ def test_upload_csv_file_with_empty_message_shows_check_page_with_errors( service_one, mocker, mock_get_empty_service_template_with_optional_placeholder, + mock_s3_set_metadata, mock_s3_get_metadata, mock_s3_upload, mock_get_users_by_service, @@ -539,6 +575,7 @@ def test_upload_csv_file_with_very_long_placeholder_shows_check_page_with_errors service_one, mocker, mock_get_service_template_with_placeholders, + mock_s3_set_metadata, mock_s3_get_metadata, mock_s3_upload, mock_get_users_by_service, @@ -597,6 +634,7 @@ def test_upload_csv_file_with_bad_postal_address_shows_check_page_with_errors( service_one, mocker, mock_get_service_letter_template, + mock_s3_set_metadata, mock_s3_get_metadata, mock_s3_upload, mock_get_users_by_service, @@ -660,6 +698,7 @@ def test_upload_csv_file_with_international_letters_permission_shows_appropriate service_one, mocker, mock_get_service_letter_template, + mock_s3_set_metadata, mock_s3_get_metadata, mock_s3_upload, mock_get_users_by_service, @@ -834,6 +873,7 @@ def test_upload_csv_file_with_missing_columns_shows_error( client_request, mocker, mock_get_service_template_with_placeholders, + mock_s3_set_metadata, mock_s3_get_metadata, mock_s3_upload, mock_get_users_by_service, @@ -883,6 +923,7 @@ def test_upload_valid_csv_redirects_to_check_page( client_request, mock_get_service_template_with_placeholders, mock_s3_upload, + mock_s3_set_metadata, fake_uuid, ): client_request.post( @@ -894,7 +935,6 @@ def test_upload_valid_csv_redirects_to_check_page( service_id=SERVICE_ONE_ID, template_id=fake_uuid, upload_id=fake_uuid, - original_file_name='valid.csv', _external=True, ), ) @@ -2529,7 +2569,13 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( with logged_in_client.session_transaction() as session: assert 'file_uploads' not in session - mock_s3_set_metadata.assert_called_once_with( + assert mock_s3_set_metadata.call_count == 2 + assert mock_s3_set_metadata.call_args_list[0] == mocker.call( + SERVICE_ONE_ID, + fake_uuid, + original_file_name='example.csv' + ) + assert mock_s3_set_metadata.call_args_list[1] == mocker.call( SERVICE_ONE_ID, fake_uuid, notification_count=53, @@ -2557,6 +2603,7 @@ def test_upload_csvfile_with_international_validates( api_user_active, logged_in_client, mock_get_service_template, + mock_s3_set_metadata, mock_s3_get_metadata, mock_s3_upload, mock_has_permissions,