diff --git a/app/main/views/send.py b/app/main/views/send.py index 95f3cd55d..4c06ffbdf 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -668,10 +668,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ abort(404) page_count = get_page_count_for_letter(db_template, template.values) - # TODO: stop checking the request.args for original_file_name once this change has been deployed for a - # while and we are confident it's only coming from metadata - metadata = get_csv_metadata(service_id, upload_id) - original_file_name = metadata.get('original_file_name', request.args.get('original_file_name', '')) + original_file_name = get_csv_metadata(service_id, upload_id).get('original_file_name', '') return dict( recipients=recipients, @@ -732,16 +729,11 @@ def check_messages(service_id, template_id, upload_id, row_index=2): ): return render_template('views/check/column-errors.html', **data) - data['original_file_name'] = SanitiseASCII.encode(data.get('original_file_name', '')) - metadata_kwargs = { 'notification_count': data['count_of_recipients'], 'template_id': template_id, 'valid': True, - 'original_file_name': unicode_truncate( - data['original_file_name'], - 1600, - ), + 'original_file_name': data.get('original_file_name', ''), } if session.get('sender_id'): diff --git a/app/main/views/uploads.py b/app/main/views/uploads.py index 932d65d51..ef9d53da6 100644 --- a/app/main/views/uploads.py +++ b/app/main/views/uploads.py @@ -427,20 +427,13 @@ def check_contact_list(service_id, upload_id): contents = ContactList.download(service_id, upload_id) first_row = contents.splitlines()[0].strip().rstrip(',') if contents else '' - metadata = ContactList.get_metadata(service_id, upload_id) + original_file_name = ContactList.get_metadata(service_id, upload_id).get('original_file_name', '') template_type = { 'emailaddress': 'email', 'phonenumber': 'sms', }.get(Columns.make_key(first_row)) - # TODO: stop looking in the query string for metadata once we are sure all uploaded - # contact lists now have original_file_name in the metadata - original_file_name = metadata.get( - 'original_file_name', - SanitiseASCII.encode(request.args.get('original_file_name', '')) - ) - recipients = RecipientCSV( contents, template=get_sample_template(template_type or 'sms'), @@ -493,10 +486,7 @@ def check_contact_list(service_id, upload_id): metadata_kwargs = { 'row_count': len(recipients), 'valid': True, - 'original_file_name': unicode_truncate( - original_file_name, - 1600, - ), + 'original_file_name': original_file_name, 'template_type': template_type } diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index 332d52332..875c9206f 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -1,5 +1,4 @@ # -*- coding: utf-8 -*- -import sys import uuid from functools import partial from glob import glob @@ -1108,142 +1107,6 @@ def test_upload_valid_csv_only_sets_meta_if_filename_known( assert len(mock_s3_set_metadata.call_args_list) == 0 -def test_file_name_truncated_to_fit_in_s3_metadata( - client_request, - mocker, - mock_get_live_service, - mock_get_service_template_with_placeholders, - mock_get_users_by_service, - mock_get_service_statistics, - mock_get_job_doesnt_exist, - mock_get_jobs, - mock_s3_set_metadata, - fake_uuid, -): - - with client_request.session_transaction() as session: - session['file_uploads'] = { - fake_uuid: {'template_id': fake_uuid} - } - - mocker.patch('app.main.views.send.s3download', return_value=""" - phone number,name,thing,thing,thing - 07700900001, A, foo, foo, foo - """) - - file_name = 'ü😁' * 2000 - - mocker.patch( - 'app.main.views.send.get_csv_metadata', - return_value={'original_file_name': file_name}, - ) - - client_request.get( - 'main.check_messages', - service_id=SERVICE_ONE_ID, - template_id=fake_uuid, - upload_id=fake_uuid, - ) - assert sys.getsizeof( - file_name.encode('utf-8') - ) > 2000 - - assert sys.getsizeof(''.join(( - '{}{}'.format(key, value) for key, value in - mock_s3_set_metadata.call_args_list[0][1].items() - )).encode('utf-8')) == 1726 - - -def test_file_name_is_taken_from_query_string_if_not_in_metadata( - client_request, - mocker, - mock_get_live_service, - mock_get_service_template_with_placeholders, - mock_get_users_by_service, - mock_get_service_statistics, - mock_get_job_doesnt_exist, - mock_get_jobs, - mock_s3_set_metadata, - fake_uuid, -): - - with client_request.session_transaction() as session: - session['file_uploads'] = { - fake_uuid: {'template_id': fake_uuid} - } - - mocker.patch('app.main.views.send.s3download', return_value=""" - phone number,name,thing,thing,thing - 07700900001, A, foo, foo, foo - """) - mocker.patch( - 'app.main.views.send.get_csv_metadata', - return_value={}, - ) - - client_request.get( - 'main.check_messages', - service_id=SERVICE_ONE_ID, - template_id=fake_uuid, - upload_id=fake_uuid, - original_file_name='example.csv', - ) - 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', - ) - - -def test_check_messages_replaces_invalid_characters_in_file_name( - client_request, - mocker, - mock_get_live_service, - mock_get_service_template_with_placeholders, - mock_get_users_by_service, - mock_get_service_statistics, - mock_get_job_doesnt_exist, - mock_get_jobs, - mock_s3_set_metadata, - fake_uuid, -): - with client_request.session_transaction() as session: - session['file_uploads'] = { - fake_uuid: {'template_id': fake_uuid} - } - - mocker.patch('app.main.views.send.s3download', return_value=""" - phone number,name,thing,thing,thing - 07700900001, A, foo, foo, foo - """) - - file_name = 'ΓΌπŸ˜β€™β‚¬' - - mocker.patch( - 'app.main.views.send.get_csv_metadata', - return_value={'original_file_name': file_name}, - ) - - client_request.get( - 'main.check_messages', - service_id=SERVICE_ONE_ID, - template_id=fake_uuid, - upload_id=fake_uuid, - ) - - mock_s3_set_metadata.assert_called_once_with( - SERVICE_ONE_ID, - fake_uuid, - notification_count=1, - original_file_name="u?'?", - template_id=fake_uuid, - valid=True - ) - - def test_show_all_columns_if_there_are_duplicate_recipient_columns( client_request, mocker, diff --git a/tests/app/main/views/uploads/test_upload_contact_list.py b/tests/app/main/views/uploads/test_upload_contact_list.py index 008bd687d..ba2e0fa1a 100644 --- a/tests/app/main/views/uploads/test_upload_contact_list.py +++ b/tests/app/main/views/uploads/test_upload_contact_list.py @@ -394,65 +394,6 @@ def test_upload_csv_shows_ok_page( ) -def test_upload_csv_shows_ok_page_when_filename_is_in_query_string( - client_request, - mock_get_live_service, - mock_get_users_by_service, - mock_get_job_doesnt_exist, - fake_uuid, - mocker -): - mocker.patch('app.models.contact_list.s3download', return_value='\n'.join( - ['email address'] + ['test@example.com'] * 51 - )) - mocker.patch('app.models.contact_list.get_csv_metadata', return_value={}) - mock_metadata_set = mocker.patch('app.models.contact_list.set_metadata_on_csv_upload') - - page = client_request.get( - 'main.check_contact_list', - service_id=SERVICE_ONE_ID, - upload_id=fake_uuid, - original_file_name='good times.xlsx', - _test_page_title=False, - ) - - mock_metadata_set.assert_called_once_with( - SERVICE_ONE_ID, - fake_uuid, - bucket='test-contact-list', - row_count=51, - original_file_name='good times.xlsx', - template_type='email', - valid=True, - ) - - assert normalize_spaces(page.select_one('h1').text) == ( - 'good times.xlsx' - ) - assert normalize_spaces(page.select_one('main p').text) == ( - '51 email addresses found' - ) - assert page.select_one('form')['action'] == url_for( - 'main.save_contact_list', - service_id=SERVICE_ONE_ID, - upload_id=fake_uuid, - ) - - assert normalize_spaces(page.select_one('form [type=submit]').text) == ( - 'Save contact list' - ) - assert normalize_spaces(page.select_one('thead').text) == ( - 'Row in file 1 email address' - ) - assert len(page.select('tbody tr')) == 50 - assert normalize_spaces(page.select_one('tbody tr').text) == ( - '2 test@example.com' - ) - assert normalize_spaces(page.select_one('.table-show-more-link').text) == ( - 'Only showing the first 50 rows' - ) - - def test_save_contact_list( mocker, client_request,