in send flow replace suppress with try catch

suppress was suppressing 404 errors (the happy path) - but it was also
suppressing 503s from tests where we hadn't mocked endpoints

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)
 

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:
+    content = client_request.get(
-        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'],
         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:
+    content = client_request.get(
-        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'],
         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,

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):