Use sanitised pdfs for sending and handle invalid pdfs, details below:

- pass new, sanitised pdf for sending
- move invalid pdfs to a newly created bucket
- set status fro notifications that failed pdf validation to a new status validation-failed
- adjust existing tests

app/celery/letters_pdf_tasks.py

@@ -38,9 +38,11 @@ from app.models import (
     NOTIFICATION_DELIVERED,
     NOTIFICATION_VIRUS_SCAN_FAILED,
     NOTIFICATION_TECHNICAL_FAILURE,
-    # NOTIFICATION_VALIDATION_FAILED
+    NOTIFICATION_VALIDATION_FAILED
 )
 
+from app.letters.utils import move_scan_to_invalid_pdf_bucket
+
 
 @notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300)
 @statsd(namespace="tasks")
@@ -187,19 +189,18 @@ def process_virus_scan_passed(self, filename):
 
     if not new_pdf:
         current_app.logger.info('Invalid precompiled pdf received {} ({})'.format(notification.id, filename))
-        # update_notification_status_by_id(notification.id, NOTIFICATION_VALIDATION_FAILED)
+        update_notification_status_by_id(notification.id, NOTIFICATION_VALIDATION_FAILED)
-        # move_scan_to_invalid_pdf_bucket()  # TODO: implement this (and create bucket etc)
+        move_scan_to_invalid_pdf_bucket(scan_pdf_object)
-        # scan_pdf_object.delete()
+        scan_pdf_object.delete()
-        # return
+        return
     else:
         current_app.logger.info(
             "Validation was successful for precompiled pdf {} ({})".format(notification.id, filename))
 
     current_app.logger.info('notification id {} ({}) sanitised and ready to send'.format(notification.id, filename))
 
-    # temporarily upload original pdf while testing sanitise flow.
     _upload_pdf_to_test_or_live_pdf_bucket(
-        old_pdf,  # TODO: change to new_pdf
+        new_pdf,
         filename,
         is_test_letter=is_test_key)
 
@@ -237,7 +238,9 @@ def _sanitise_precomiled_pdf(self, notification, precompiled_pdf):
         return resp.content
     except RequestException as ex:
         if ex.response is not None and ex.response.status_code == 400:
-            # validation error
+            current_app.logger.exception(
+                "sanitise_precomiled_pdf validation error for notification: {}".format(notification.id)
+            )
             return None
 
         try:

app/config.py

@@ -337,6 +337,7 @@ class Development(Config):
     DVLA_RESPONSE_BUCKET_NAME = 'notify.tools-ftp'
     LETTERS_PDF_BUCKET_NAME = 'development-letters-pdf'
     LETTERS_SCAN_BUCKET_NAME = 'development-letters-scan'
+    INVALID_PDF_BUCKET_NAME = 'development-letters-invalid-pdf'
 
     ADMIN_CLIENT_SECRET = 'dev-notify-secret-key'
     SECRET_KEY = 'dev-notify-secret-key'
@@ -379,6 +380,7 @@ class Test(Development):
     DVLA_RESPONSE_BUCKET_NAME = 'test.notify.com-ftp'
     LETTERS_PDF_BUCKET_NAME = 'test-letters-pdf'
     LETTERS_SCAN_BUCKET_NAME = 'test-letters-scan'
+    INVALID_PDF_BUCKET_NAME = 'test-letters-invalid-pdf'
 
     # this is overriden in jenkins and on cloudfoundry
     SQLALCHEMY_DATABASE_URI = os.getenv('SQLALCHEMY_DATABASE_URI', 'postgresql://localhost/test_notification_api')
@@ -407,6 +409,7 @@ class Preview(Config):
     DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp'
     LETTERS_PDF_BUCKET_NAME = 'preview-letters-pdf'
     LETTERS_SCAN_BUCKET_NAME = 'preview-letters-scan'
+    INVALID_PDF_BUCKET_NAME = 'preview-letters-invalid-pdf'
     FROM_NUMBER = 'preview'
     API_RATE_LIMIT_ENABLED = True
     CHECK_PROXY_HEADER = False
@@ -421,6 +424,7 @@ class Staging(Config):
     DVLA_RESPONSE_BUCKET_NAME = 'staging-notify.works-ftp'
     LETTERS_PDF_BUCKET_NAME = 'staging-letters-pdf'
     LETTERS_SCAN_BUCKET_NAME = 'staging-letters-scan'
+    INVALID_PDF_BUCKET_NAME = 'staging-letters-invalid-pdf'
     STATSD_ENABLED = True
     FROM_NUMBER = 'stage'
     API_RATE_LIMIT_ENABLED = True
@@ -437,6 +441,7 @@ class Live(Config):
     DVLA_RESPONSE_BUCKET_NAME = 'notifications.service.gov.uk-ftp'
     LETTERS_PDF_BUCKET_NAME = 'production-letters-pdf'
     LETTERS_SCAN_BUCKET_NAME = 'production-letters-scan'
+    INVALID_PDF_BUCKET_NAME = 'production-letters-invalid-pdf'
     STATSD_ENABLED = True
     FROM_NUMBER = 'GOVUK'
     FUNCTIONAL_TEST_PROVIDER_SERVICE_ID = '6c1d81bb-dae2-4ee9-80b0-89a4aae9f649'
@@ -460,6 +465,7 @@ class Sandbox(CloudFoundryConfig):
     DVLA_RESPONSE_BUCKET_NAME = 'notify.works-ftp'
     LETTERS_PDF_BUCKET_NAME = 'cf-sandbox-letters-pdf'
     LETTERS_SCAN_BUCKET_NAME = 'cf-sandbox-letters-scan'
+    INVALID_PDF_BUCKET_NAME = 'cf-sandbox-letters-invalid-pdf'
     FROM_NUMBER = 'sandbox'
     REDIS_ENABLED = False
 

app/letters/utils.py

@@ -107,6 +107,12 @@ def move_error_pdf_to_scan_bucket(source_filename):
     _move_s3_object(scan_bucket, error_file, scan_bucket, source_filename)
 
 
+def move_scan_to_invalid_pdf_bucket(source_filename):
+    invalid_pdf_bucket = current_app.config['INVALID_PDF_BUCKET_NAME']
+    invalid_pdf = source_filename
+    _move_s3_object(invalid_pdf_bucket, invalid_pdf, invalid_pdf_bucket, source_filename)
+
+
 def get_file_names_from_error_bucket():
     s3 = boto3.resource('s3')
     scan_bucket = current_app.config['LETTERS_SCAN_BUCKET_NAME']

app/models.py

@@ -1053,6 +1053,7 @@ NOTIFICATION_TECHNICAL_FAILURE = 'technical-failure'
 NOTIFICATION_TEMPORARY_FAILURE = 'temporary-failure'
 NOTIFICATION_PERMANENT_FAILURE = 'permanent-failure'
 NOTIFICATION_PENDING_VIRUS_CHECK = 'pending-virus-check'
+NOTIFICATION_VALIDATION_FAILED = 'validation-failed'
 NOTIFICATION_VIRUS_SCAN_FAILED = 'virus-scan-failed'
 NOTIFICATION_RETURNED_LETTER = 'returned-letter'
 
@@ -1060,6 +1061,7 @@ NOTIFICATION_STATUS_TYPES_FAILED = [
     NOTIFICATION_TECHNICAL_FAILURE,
     NOTIFICATION_TEMPORARY_FAILURE,
     NOTIFICATION_PERMANENT_FAILURE,
+    NOTIFICATION_VALIDATION_FAILED,
     NOTIFICATION_VIRUS_SCAN_FAILED,
     NOTIFICATION_RETURNED_LETTER,
 ]
@@ -1101,6 +1103,7 @@ NOTIFICATION_STATUS_TYPES = [
     NOTIFICATION_TEMPORARY_FAILURE,
     NOTIFICATION_PERMANENT_FAILURE,
     NOTIFICATION_PENDING_VIRUS_CHECK,
+    NOTIFICATION_VALIDATION_FAILED,
     NOTIFICATION_VIRUS_SCAN_FAILED,
     NOTIFICATION_RETURNED_LETTER,
 ]

migrations/versions/0238_add_validation_failed.py

@@ -0,0 +1,28 @@
+"""
+
+Revision ID: 0238_add_validation_failed
+Revises: 0237_add_filename_to_dvla_org
+Create Date: 2018-09-03 11:24:58.773824
+
+"""
+from alembic import op
+import sqlalchemy as sa
+
+
+revision = '0238_add_validation_failed'
+down_revision = '0237_add_filename_to_dvla_org'
+
+
+def upgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.execute("INSERT INTO notification_status_types (name) VALUES ('validation-failed')")
+    # ### end Alembic commands ###
+
+
+def downgrade():
+    # ### commands auto generated by Alembic - please adjust! ###
+    op.execute("UPDATE notifications SET notification_status = 'permanent-failure' WHERE notification_status = 'validation-failed'")
+    op.execute("UPDATE notification_history SET notification_status = 'permanent-failure' WHERE notification_status = 'validation-failed'")
+
+    op.execute("DELETE FROM notification_status_types WHERE name = 'validation-failed'")
+    # ### end Alembic commands ###

tests/app/celery/test_letters_pdf_tasks.py

@@ -354,16 +354,16 @@ def test_process_letter_task_check_virus_scan_passed(
     sample_letter_notification.status = 'pending-virus-check'
     sample_letter_notification.key_type = key_type
     mock_s3upload = mocker.patch('app.celery.letters_pdf_tasks.s3upload')
-    mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precomiled_pdf')
+    mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precomiled_pdf', return_value="success")
 
     process_virus_scan_passed(filename)
 
     assert sample_letter_notification.status == noti_status
     mock_s3upload.assert_called_once_with(
-        filedata=b'pdf_content',
-        region='eu-west-1',
         bucket_name=target_bucket_name,
-        file_location=destination_folder + filename
+        filedata="success",
+        file_location=destination_folder + filename,
+        region='eu-west-1',
     )
     mock_sanitise.assert_called_once_with(
         ANY,

tests/app/v2/notifications/test_get_notifications.py

@@ -437,7 +437,7 @@ def test_get_all_notifications_filter_by_status_invalid_status(client, sample_no
     assert len(json_response['errors']) == 1
     assert json_response['errors'][0]['message'] == "status elephant is not one of [cancelled, created, sending, " \
         "sent, delivered, pending, failed, technical-failure, temporary-failure, permanent-failure, " \
-        "pending-virus-check, virus-scan-failed, returned-letter, accepted, received]"
+        "pending-virus-check, validation-failed, virus-scan-failed, returned-letter, accepted, received]"
 
 
 def test_get_all_notifications_filter_by_multiple_statuses(client, sample_template):

tests/app/v2/notifications/test_notification_schemas.py

@@ -44,7 +44,7 @@ def test_get_notifications_request_invalid_statuses(
     partial_error_status = "is not one of " \
         "[cancelled, created, sending, sent, delivered, pending, failed, " \
         "technical-failure, temporary-failure, permanent-failure, pending-virus-check, " \
-        "virus-scan-failed, returned-letter, accepted, received]"
+        "validation-failed, virus-scan-failed, returned-letter, accepted, received]"
 
     with pytest.raises(ValidationError) as e:
         validate({'status': invalid_statuses + valid_statuses}, get_notifications_request)
@@ -92,7 +92,7 @@ def test_get_notifications_request_invalid_statuses_and_template_types():
     for invalid_status in ["elephant", "giraffe"]:
         assert "status {} is not one of [cancelled, created, sending, sent, delivered, " \
             "pending, failed, technical-failure, temporary-failure, permanent-failure, " \
-            "pending-virus-check, virus-scan-failed, returned-letter, accepted, received]".format(
+            "pending-virus-check, validation-failed, virus-scan-failed, returned-letter, accepted, received]".format(
                 invalid_status
             ) in error_messages