Refactor process_letter_notification to make the code easier to read.

- Separated the logic of precompiled and template letters.
- Remove the check for research mode, research mode is not relevant to api calls. The test key is used for testing.
Refactor upload_pdf_letter to accept a precompile boolean to save a query to template.

app/letters/utils.py

@@ -64,16 +64,16 @@ def get_reference_from_filename(filename):
     return filename_parts[1]
 
 
-def upload_letter_pdf(notification, pdf_data):
+def upload_letter_pdf(notification, pdf_data, precompiled=False):
     current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format(
         notification.id, notification.reference, notification.created_at, len(pdf_data)))
 
     upload_file_name = get_letter_pdf_filename(
         notification.reference,
         notification.service.crown,
-        is_scan_letter=notification.template.is_precompiled_letter)
+        is_scan_letter=precompiled)
 
-    if notification.template.is_precompiled_letter:
+    if precompiled:
         bucket_name = current_app.config['LETTERS_SCAN_BUCKET_NAME']
     else:
         bucket_name = current_app.config['LETTERS_PDF_BUCKET_NAME']

app/v2/notifications/post_notifications.py

@@ -220,23 +220,17 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text
     if not api_key.service.research_mode and api_key.service.restricted and api_key.key_type != KEY_TYPE_TEST:
         raise BadRequestError(message='Cannot send letters when service is in trial mode', status_code=403)
 
-    should_send = not (api_key.service.research_mode or api_key.key_type == KEY_TYPE_TEST)
+    if precompiled:
+        return process_precompiled_letter_notifications(letter_data=letter_data,
+                                                        api_key=api_key,
+                                                        template=template,
+                                                        reply_to_text=reply_to_text)
+
+    should_send = not (api_key.key_type == KEY_TYPE_TEST)
 
     # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up
     status = NOTIFICATION_CREATED if should_send else NOTIFICATION_SENDING
 
-    if precompiled:
-        try:
-            if should_send or (precompiled and api_key.key_type == KEY_TYPE_TEST):
-                status = NOTIFICATION_PENDING_VIRUS_CHECK
-            letter_content = base64.b64decode(letter_data['content'])
-            pages = pdf_page_count(io.BytesIO(letter_content))
-        except ValueError:
-            raise BadRequestError(message='Cannot decode letter content (invalid base64 encoding)', status_code=400)
-        except PdfReadError:
-            current_app.logger.exception(msg='Invalid PDF received')
-            raise BadRequestError(message='Letter content is not a valid PDF', status_code=400)
-
     notification = create_letter_notification(letter_data=letter_data,
                                               template=template,
                                               api_key=api_key,
@@ -244,45 +238,52 @@ def process_letter_notification(*, letter_data, api_key, template, reply_to_text
                                               reply_to_text=reply_to_text)
 
     if should_send:
-        if precompiled:
+        create_letters_pdf.apply_async(
-            filename = upload_letter_pdf(notification, letter_content)
+            [str(notification.id)],
-            pages_per_sheet = 2
+            queue=QueueNames.CREATE_LETTERS_PDF
-            notification.billable_units = math.ceil(pages / pages_per_sheet)
+        )
-            dao_update_notification(notification)
+    elif current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']:
-
-            current_app.logger.info(
-                'Calling task scan-file for {}'.format(filename)
-            )
-
-            # call task to add the filename to anti virus queue
-            notify_celery.send_task(
-                name=TaskNames.SCAN_FILE,
-                kwargs={'filename': filename},
-                queue=QueueNames.ANTIVIRUS,
-            )
-        else:
-            create_letters_pdf.apply_async(
-                [str(notification.id)],
-                queue=QueueNames.CREATE_LETTERS_PDF
-            )
-    elif (api_key.service.research_mode and
-          current_app.config['NOTIFY_ENVIRONMENT'] in ['preview', 'development']):
         create_fake_letter_response_file.apply_async(
             (notification.reference,),
             queue=QueueNames.RESEARCH_MODE
         )
     else:
-        if precompiled and api_key.key_type == KEY_TYPE_TEST:
+        update_notification_status_by_reference(notification.reference, NOTIFICATION_DELIVERED)
-            filename = upload_letter_pdf(notification, letter_content)
 
-            # call task to add the filename to anti virus queue
+    return notification
-            notify_celery.send_task(
+
-                name=TaskNames.SCAN_FILE,
+
-                kwargs={'filename': filename},
+def process_precompiled_letter_notifications(*, letter_data, api_key, template, reply_to_text):
-                queue=QueueNames.ANTIVIRUS,
+    try:
-            )
+        status = NOTIFICATION_PENDING_VIRUS_CHECK
-        else:
+        letter_content = base64.b64decode(letter_data['content'])
-            update_notification_status_by_reference(notification.reference, NOTIFICATION_DELIVERED)
+        pages = pdf_page_count(io.BytesIO(letter_content))
+    except ValueError:
+        raise BadRequestError(message='Cannot decode letter content (invalid base64 encoding)', status_code=400)
+    except PdfReadError:
+        current_app.logger.exception(msg='Invalid PDF received')
+        raise BadRequestError(message='Letter content is not a valid PDF', status_code=400)
+
+    notification = create_letter_notification(letter_data=letter_data,
+                                              template=template,
+                                              api_key=api_key,
+                                              status=status,
+                                              reply_to_text=reply_to_text)
+
+    filename = upload_letter_pdf(notification, letter_content, precompiled=True)
+    pages_per_sheet = 2
+    notification.billable_units = math.ceil(pages / pages_per_sheet)
+
+    dao_update_notification(notification)
+
+    current_app.logger.info('Calling task scan-file for {}'.format(filename))
+
+    # call task to add the filename to anti virus queue
+    notify_celery.send_task(
+        name=TaskNames.SCAN_FILE,
+        kwargs={'filename': filename},
+        queue=QueueNames.ANTIVIRUS,
+    )
 
     return notification
 

tests/app/letters/test_letter_utils.py

@@ -128,7 +128,7 @@ def test_upload_letter_pdf_to_correct_bucket(
         is_scan_letter=is_precompiled_letter
     )
 
-    upload_letter_pdf(sample_letter_notification, b'\x00\x01')
+    upload_letter_pdf(sample_letter_notification, b'\x00\x01', precompiled=is_precompiled_letter)
 
     mock_s3.assert_called_once_with(
         bucket_name=current_app.config[bucket_config_name],

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -96,12 +96,11 @@ def test_post_letter_notification_returns_201(client, sample_letter_template, mo
 
 
 @pytest.mark.parametrize('env', [
-    'development',
+    'staging',
-    'preview',
+    'live',
 ])
-def test_post_letter_notification_calls_create_fake_response_in_research_and_test_key_correct_env(
+def test_post_letter_notification_with_test_key_set_status_to_delivered(
         notify_api, client, sample_letter_template, mocker, env):
-    sample_letter_template.service.research_mode = True
 
     data = {
         'template_id': str(sample_letter_template.id),
@@ -126,83 +125,44 @@ def test_post_letter_notification_calls_create_fake_response_in_research_and_tes
 
     notification = Notification.query.one()
 
+    assert not fake_create_letter_task.called
+    assert not fake_create_dvla_response_task.called
+    assert notification.status == NOTIFICATION_DELIVERED
+
+
+@pytest.mark.parametrize('env', [
+    'development',
+    'preview',
+])
+def test_post_letter_notification_with_test_key_sets_status_to_sending_and_sends_fake_response_file(
+        notify_api, client, sample_letter_template, mocker, env):
+
+    data = {
+        'template_id': str(sample_letter_template.id),
+        'personalisation': {
+            'address_line_1': 'Her Royal Highness Queen Elizabeth II',
+            'address_line_2': 'Buckingham Palace',
+            'address_line_3': 'London',
+            'postcode': 'SW1 1AA',
+            'name': 'Lizzie'
+        },
+        'reference': 'foo'
+    }
+
+    fake_create_letter_task = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
+    fake_create_dvla_response_task = mocker.patch(
+        'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async')
+
+    with set_config_values(notify_api, {
+        'NOTIFY_ENVIRONMENT': env
+    }):
+        letter_request(client, data, service_id=sample_letter_template.service_id, key_type=KEY_TYPE_TEST)
+
+    notification = Notification.query.one()
+
+    assert not fake_create_letter_task.called
+    assert fake_create_dvla_response_task.called
     assert notification.status == NOTIFICATION_SENDING
-    assert not fake_create_letter_task.called
-    fake_create_dvla_response_task.assert_called_once_with((notification.reference,), queue=QueueNames.RESEARCH_MODE)
-
-
-@pytest.mark.parametrize('env', [
-    'staging',
-    'live',
-])
-def test_post_letter_notification_sets_status_delivered_in_research_and_test_key_incorrect_env(
-        notify_api, client, sample_letter_template, mocker, env):
-    sample_letter_template.service.research_mode = True
-
-    data = {
-        'template_id': str(sample_letter_template.id),
-        'personalisation': {
-            'address_line_1': 'Her Royal Highness Queen Elizabeth II',
-            'address_line_2': 'Buckingham Palace',
-            'address_line_3': 'London',
-            'postcode': 'SW1 1AA',
-            'name': 'Lizzie'
-        },
-        'reference': 'foo'
-    }
-
-    fake_create_letter_task = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
-    fake_create_dvla_response_task = mocker.patch(
-        'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async')
-
-    with set_config_values(notify_api, {
-        'NOTIFY_ENVIRONMENT': env
-    }):
-        letter_request(client, data, service_id=sample_letter_template.service_id, key_type=KEY_TYPE_TEST)
-
-    notification = Notification.query.one()
-
-    assert not fake_create_letter_task.called
-    assert not fake_create_dvla_response_task.called
-    assert notification.status == NOTIFICATION_DELIVERED
-
-
-@pytest.mark.parametrize('env', [
-    'development',
-    'preview',
-    'staging',
-    'live',
-])
-def test_post_letter_notification_sets_status_to_delivered_using_test_key_and_not_research_all_env(
-        notify_api, client, sample_letter_template, mocker, env):
-    sample_letter_template.service.research_mode = False
-
-    data = {
-        'template_id': str(sample_letter_template.id),
-        'personalisation': {
-            'address_line_1': 'Her Royal Highness Queen Elizabeth II',
-            'address_line_2': 'Buckingham Palace',
-            'address_line_3': 'London',
-            'postcode': 'SW1 1AA',
-            'name': 'Lizzie'
-        },
-        'reference': 'foo'
-    }
-
-    fake_create_letter_task = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
-    fake_create_dvla_response_task = mocker.patch(
-        'app.celery.research_mode_tasks.create_fake_letter_response_file.apply_async')
-
-    with set_config_values(notify_api, {
-        'NOTIFY_ENVIRONMENT': env
-    }):
-        letter_request(client, data, service_id=sample_letter_template.service_id, key_type=KEY_TYPE_TEST)
-
-    notification = Notification.query.one()
-
-    assert not fake_create_letter_task.called
-    assert not fake_create_dvla_response_task.called
-    assert notification.status == NOTIFICATION_DELIVERED
 
 
 def test_post_letter_notification_returns_400_and_missing_template(
@@ -234,11 +194,11 @@ def test_post_letter_notification_returns_400_for_empty_personalisation(
 
     assert error_json['status_code'] == 400
     assert all([e['error'] == 'ValidationError' for e in error_json['errors']])
-    assert set([e['message'] for e in error_json['errors']]) == set([
+    assert set([e['message'] for e in error_json['errors']]) == {
         'personalisation address_line_1 is required',
         'personalisation address_line_2 is required',
         'personalisation postcode is required'
-    ])
+    }
 
 
 def test_notification_returns_400_for_missing_template_field(
@@ -415,7 +375,7 @@ def test_post_letter_notification_is_delivered_and_has_pdf_uploaded_to_test_lett
 
     notification = Notification.query.one()
     assert notification.status == NOTIFICATION_PENDING_VIRUS_CHECK
-    s3mock.assert_called_once_with(ANY, b'letter-content')
+    s3mock.assert_called_once_with(ANY, b'letter-content', precompiled=True)
     mock_celery.assert_called_once_with(
         name=TaskNames.SCAN_FILE,
         kwargs={'filename': 'test.pdf'},
@@ -498,7 +458,7 @@ def test_post_precompiled_letter_notification_returns_201(client, notify_user, m
 
     assert response.status_code == 201, response.get_data(as_text=True)
 
-    s3mock.assert_called_once_with(ANY, b'letter-content')
+    s3mock.assert_called_once_with(ANY, b'letter-content', precompiled=True)
 
     notification = Notification.query.first()