diff --git a/app/main/views/send.py b/app/main/views/send.py index 479bb44e0..9901850f8 100644 --- a/app/main/views/send.py +++ b/app/main/views/send.py @@ -1,6 +1,5 @@ import itertools import json -from contextlib import suppress from string import ascii_uppercase from zipfile import BadZipFile @@ -479,10 +478,12 @@ def send_test_preview(service_id, template_id, filetype): def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_pdf=False): - with suppress(HTTPError): + try: # The happy path is that the job doesn’t already exist, so the # API will return a 404 and the client will raise HTTPError. job_api_client.get_job(service_id, upload_id) + + # the job exists already - so go back to the templates page # If we just return a `redirect` (302) object here, we'll get # errors when we try and unpack in the check_messages route. # Rasing a werkzeug.routing redirect means that doesn't happen. @@ -491,6 +492,9 @@ def _check_messages(service_id, template_id, upload_id, preview_row, letters_as_ service_id=service_id, template_id=template_id )) + except HTTPError as e: + if e.status_code != 404: + raise users = user_api_client.get_users_for_service(service_id=service_id) diff --git a/tests/app/main/views/test_send.py b/tests/app/main/views/test_send.py index de6333617..e1dcbb4e4 100644 --- a/tests/app/main/views/test_send.py +++ b/tests/app/main/views/test_send.py @@ -362,6 +362,7 @@ def test_upload_csvfile_with_errors_shows_check_page_with_errors( mock_s3_upload, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, fake_uuid, ): @@ -486,6 +487,7 @@ def test_upload_csvfile_with_missing_columns_shows_error( mock_s3_upload, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, service_one, fake_uuid, file_contents, @@ -566,6 +568,7 @@ def test_upload_valid_csv_shows_preview_and_table( mock_get_service_template_with_placeholders, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, mock_s3_set_metadata, fake_uuid, extra_args, @@ -670,6 +673,7 @@ def test_file_name_truncated_to_fit_in_s3_metadata( mock_get_service_template_with_placeholders, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, mock_s3_set_metadata, fake_uuid, ): @@ -710,6 +714,7 @@ def test_show_all_columns_if_there_are_duplicate_recipient_columns( mock_get_service_template_with_placeholders, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, fake_uuid, ): @@ -754,6 +759,7 @@ def test_404_for_previewing_a_row_out_of_range( mock_get_service_template_with_placeholders, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, mock_s3_set_metadata, fake_uuid, row_index, @@ -1517,6 +1523,7 @@ def test_upload_csvfile_with_valid_phone_shows_all_numbers( mock_get_users_by_service, mock_get_detailed_service_for_today, mock_get_live_service, + mock_get_job_doesnt_exist, mock_s3_set_metadata, service_one, fake_uuid, @@ -1572,6 +1579,7 @@ def test_upload_csvfile_with_international_validates( mock_has_permissions, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, fake_uuid, service_mock, should_allow_international, @@ -1596,70 +1604,51 @@ def test_upload_csvfile_with_international_validates( def test_test_message_can_only_be_sent_now( - logged_in_client, + client_request, mocker, service_one, mock_get_service_template, mock_s3_download, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, mock_s3_set_metadata, fake_uuid ): - with logged_in_client.session_transaction() as session: - 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( + content = client_request.get( 'main.check_messages', service_id=service_one['id'], upload_id=fake_uuid, template_id=fake_uuid, from_test=True - )) + ) - content = response.get_data(as_text=True) assert 'name="scheduled_for"' not in content def test_letter_can_only_be_sent_now( - logged_in_client, + client_request, mocker, service_one, mock_get_service_letter_template, - mock_s3_download, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_s3_set_metadata, + mock_get_job_doesnt_exist, fake_uuid, ): - + mocker.patch('app.main.views.send.s3download', return_value="addressline1, addressline2, postcode\na,b,c") + mocker.patch('app.main.views.send.set_metadata_on_csv_upload') mocker.patch('app.main.views.send.get_page_count_for_letter', return_value=1) - with logged_in_client.session_transaction() as session: - 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( + content = client_request.get( 'main.check_messages', service_id=service_one['id'], upload_id=fake_uuid, template_id=fake_uuid, from_test=True - )) + ) - content = response.get_data(as_text=True) assert 'name="scheduled_for"' not in content @@ -1756,6 +1745,7 @@ def test_should_show_preview_letter_message( mock_get_service_letter_template, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, service_one, fake_uuid, mocker, @@ -1972,6 +1962,7 @@ def test_check_messages_back_link( mock_get_service, mock_has_permissions, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, mock_s3_download, mock_s3_set_metadata, fake_uuid, @@ -2057,6 +2048,7 @@ def test_check_messages_shows_too_many_messages_errors( mock_get_users_by_service, mock_get_service, mock_get_service_template, + mock_get_job_doesnt_exist, mock_has_permissions, fake_uuid, num_requested, @@ -2109,6 +2101,7 @@ def test_check_messages_shows_trial_mode_error( mock_get_service_template, mock_has_permissions, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, fake_uuid, mocker ): @@ -2155,6 +2148,7 @@ def test_check_messages_shows_trial_mode_error_for_letters( mock_has_permissions, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, mock_s3_set_metadata, fake_uuid, mocker, @@ -2203,8 +2197,9 @@ 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, + mock_get_job_doesnt_exist, + fake_uuid, ): mocker.patch('app.main.views.send.s3download', return_value='\n'.join( @@ -2244,6 +2239,7 @@ def test_check_messages_column_error_doesnt_show_optional_columns( fake_uuid, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, ): mocker.patch('app.main.views.send.s3download', return_value='\n'.join( @@ -2283,6 +2279,7 @@ def test_generate_test_letter_doesnt_block_in_trial_mode( fake_uuid, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, mock_s3_set_metadata, ): @@ -2321,6 +2318,7 @@ def test_check_messages_shows_over_max_row_error( mock_get_service_template_with_placeholders, mock_has_permissions, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, mock_s3_download, fake_uuid, mocker @@ -2364,6 +2362,7 @@ def test_non_ascii_characters_in_letter_recipients_file_shows_error( mock_has_permissions, mock_get_service_letter_template, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, fake_uuid, mocker ): @@ -2734,6 +2733,7 @@ def test_reply_to_is_previewed_if_chosen( mock_s3_set_metadata, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, get_default_reply_to_email_address, fake_uuid, endpoint, @@ -2783,6 +2783,7 @@ def test_sms_sender_is_previewed( mock_s3_set_metadata, mock_get_users_by_service, mock_get_detailed_service_for_today, + mock_get_job_doesnt_exist, get_default_sms_sender, fake_uuid, endpoint, diff --git a/tests/conftest.py b/tests/conftest.py index 23e6f31e0..93f2dbf44 100644 --- a/tests/conftest.py +++ b/tests/conftest.py @@ -1678,6 +1678,14 @@ def mock_get_job(mocker, api_user_active): return mocker.patch('app.job_api_client.get_job', side_effect=_get_job) +@pytest.fixture +def mock_get_job_doesnt_exist(mocker): + def _get_job(service_id, job_id): + raise HTTPError(response=Mock(status_code=404, json={}), message={}) + + return mocker.patch('app.job_api_client.get_job', side_effect=_get_job) + + @pytest.fixture(scope='function') def mock_get_scheduled_job(mocker, api_user_active): def _get_job(service_id, job_id):