Merge branch 'master' into update-page-count-after-antivirus-scan

app/celery/letters_pdf_tasks.py

@@ -57,6 +57,7 @@ def create_letters_pdf(self, notification_id):
             notification.template,
             contact_block=notification.reply_to_text,
             org_id=notification.service.dvla_organisation.id,
+            filename=notification.service.dvla_organisation.filename,
             values=notification.personalisation
         )
 
@@ -76,13 +77,13 @@ def create_letters_pdf(self, notification_id):
             )
             self.retry(queue=QueueNames.RETRY)
         except MaxRetriesExceededError:
-            current_app.logger.exception(
+            current_app.logger.error(
                 "RETRY FAILED: task create_letters_pdf failed for notification {}".format(notification_id),
             )
             update_notification_status_by_id(notification_id, 'technical-failure')
 
 
-def get_letters_pdf(template, contact_block, org_id, values):
+def get_letters_pdf(template, contact_block, org_id, filename, values):
     template_for_letter_print = {
         "subject": template.subject,
         "content": template.content
@@ -92,6 +93,7 @@ def get_letters_pdf(template, contact_block, org_id, values):
         'letter_contact_block': contact_block,
         'template': template_for_letter_print,
         'values': values,
+        'filename': filename,
         'dvla_org_id': org_id,
     }
     resp = requests_post(
@@ -189,8 +191,7 @@ def process_virus_scan_passed(self, filename):
     old_pdf = scan_pdf_object.get()['Body'].read()
 
     billable_units = _get_page_count(notification, old_pdf)
-
+    new_pdf = _sanitise_precompiled_pdf(self, notification, old_pdf)
-    new_pdf = _sanitise_precomiled_pdf(self, notification, old_pdf)
 
     # TODO: Remove this once CYSP update their template to not cross over the margins
     if notification.service_id == UUID('fe44178f-3b45-4625-9f85-2264a36dd9ec'):  # CYSP
@@ -212,7 +213,6 @@ def process_virus_scan_passed(self, 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(
         new_pdf,
         filename,
@@ -237,7 +237,8 @@ def _get_page_count(notification, old_pdf):
         current_app.logger.exception(msg='Invalid PDF received for notification_id: {}'.format(notification.id))
         update_letter_pdf_status(
             reference=notification.reference,
-            status=NOTIFICATION_VALIDATION_FAILED
+            status=NOTIFICATION_VALIDATION_FAILED,
+            billable_units=0
         )
         raise e
 
@@ -255,7 +256,7 @@ def _upload_pdf_to_test_or_live_pdf_bucket(pdf_data, filename, is_test_letter):
     )
 
 
-def _sanitise_precomiled_pdf(self, notification, precompiled_pdf):
+def _sanitise_precompiled_pdf(self, notification, precompiled_pdf):
     try:
         resp = requests_post(
             '{}/precompiled/sanitise'.format(
@@ -268,19 +269,19 @@ 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:
-            current_app.logger.exception(
+            current_app.logger.info(
-                "sanitise_precomiled_pdf validation error for notification: {}".format(notification.id)
+                "sanitise_precompiled_pdf validation error for notification: {}".format(notification.id)
             )
             return None
 
         try:
             current_app.logger.exception(
-                "sanitise_precomiled_pdf failed for notification: {}".format(notification.id)
+                "sanitise_precompiled_pdf failed for notification: {}".format(notification.id)
             )
             self.retry(queue=QueueNames.RETRY)
         except MaxRetriesExceededError:
-            current_app.logger.exception(
+            current_app.logger.error(
-                "RETRY FAILED: sanitise_precomiled_pdf failed for notification {}".format(notification.id),
+                "RETRY FAILED: sanitise_precompiled_pdf failed for notification {}".format(notification.id),
             )
 
             notification.status = NOTIFICATION_TECHNICAL_FAILURE
@@ -325,7 +326,7 @@ def process_virus_scan_error(filename):
     raise error
 
 
-def update_letter_pdf_status(reference, status, billable_units=0):
+def update_letter_pdf_status(reference, status, billable_units):
     return dao_update_notifications_by_reference(
         references=[reference],
         update_dict={

app/celery/service_callback_tasks.py

@@ -96,9 +96,11 @@ def _send_data_to_service_callback_api(self, data, service_callback_url, token,
             try:
                 self.retry(queue=QueueNames.RETRY)
             except self.MaxRetriesExceededError:
-                current_app.logger.exception(
+                current_app.logger.error(
-                    """Retry: {} has retried the max num of times
+                    "Retry: {} has retried the max num of times for notification: {}".format(
-                     for notification: {}""".format(function_name, notification_id)
+                        function_name,
+                        notification_id
+                    )
                 )
 
 

app/celery/tasks.py

@@ -386,7 +386,7 @@ def handle_exception(task, notification, notification_id, exc):
         try:
             task.retry(queue=QueueNames.RETRY, exc=exc)
         except task.MaxRetriesExceededError:
-            current_app.logger.exception('Retry' + retry_msg)
+            current_app.logger.error('Max retry failed' + retry_msg)
 
 
 def get_template_class(template_type):
@@ -546,7 +546,11 @@ def send_inbound_sms_to_service(self, inbound_sms_id, service_id):
             try:
                 self.retry(queue=QueueNames.RETRY)
             except self.MaxRetriesExceededError:
-                current_app.logger.exception('Retry: send_inbound_sms_to_service has retried the max number of times')
+                current_app.logger.error(
+                    'Retry: send_inbound_sms_to_service has retried the max number of times for inbound_sms {}'.format(
+                        inbound_sms_id
+                    )
+                )
 
 
 @notify_celery.task(name='process-incomplete-jobs')

app/schemas.py

@@ -205,12 +205,16 @@ class ServiceSchema(BaseSchema):
     created_by = field_for(models.Service, 'created_by', required=True)
     organisation_type = field_for(models.Service, 'organisation_type')
     dvla_organisation = field_for(models.Service, 'dvla_organisation')
+    letter_logo_filename = fields.Method(method_name='get_letter_logo_filename')
     permissions = fields.Method("service_permissions")
     email_branding = field_for(models.Service, 'email_branding')
     organisation = field_for(models.Service, 'organisation')
     override_flag = False
     letter_contact_block = fields.Method(method_name="get_letter_contact")
 
+    def get_letter_logo_filename(self, service):
+        return service.dvla_organisation.filename
+
     def service_permissions(self, service):
         return [p.permission for p in service.permissions]
 

app/template/rest.py

@@ -247,6 +247,7 @@ def preview_letter_template_by_notification_id(service_id, notification_id, file
             'template': template_for_letter_print,
             'values': notification.personalisation,
             'date': notification.created_at.isoformat(),
+            'filename': service.dvla_organisation.filename,
             'dvla_org_id': service.dvla_organisation_id,
         }
 

requirements-app.txt

@@ -6,7 +6,7 @@ celery==3.1.26.post2 # pyup: <4
 docopt==0.6.2
 Flask-Bcrypt==0.7.1
 flask-marshmallow==0.9.0
-Flask-Migrate==2.2.1
+Flask-Migrate==2.3.0
 Flask-SQLAlchemy==2.3.2
 Flask==1.0.2
 click-datetime==0.2
@@ -15,7 +15,7 @@ gunicorn==19.7.1
 iso8601==0.1.12
 jsonschema==2.6.0
 marshmallow-sqlalchemy==0.14.1
-marshmallow==2.15.6
+marshmallow==2.16.0
 psycopg2-binary==2.7.5
 PyJWT==1.6.4
 SQLAlchemy==1.2.12
@@ -23,10 +23,14 @@ SQLAlchemy==1.2.12
 notifications-python-client==5.2.0
 
 # PaaS
-awscli==1.15.82
+awscli==1.15.82  # pyup: ignore
 awscli-cwlogs>=1.4,<1.5
-botocore<1.11.0
+botocore<1.11.0  # pyup: ignore
 
-git+https://github.com/alphagov/notifications-utils.git@30.5.4#egg=notifications-utils==30.5.4
+
+# Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
+itsdangerous==0.24  # pyup: <1.0.0
+
+git+https://github.com/alphagov/notifications-utils.git@30.5.5#egg=notifications-utils==30.5.5
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3

requirements.txt

@@ -8,7 +8,7 @@ celery==3.1.26.post2 # pyup: <4
 docopt==0.6.2
 Flask-Bcrypt==0.7.1
 flask-marshmallow==0.9.0
-Flask-Migrate==2.2.1
+Flask-Migrate==2.3.0
 Flask-SQLAlchemy==2.3.2
 Flask==1.0.2
 click-datetime==0.2
@@ -17,7 +17,7 @@ gunicorn==19.7.1
 iso8601==0.1.12
 jsonschema==2.6.0
 marshmallow-sqlalchemy==0.14.1
-marshmallow==2.15.6
+marshmallow==2.16.0
 psycopg2-binary==2.7.5
 PyJWT==1.6.4
 SQLAlchemy==1.2.12
@@ -25,23 +25,27 @@ SQLAlchemy==1.2.12
 notifications-python-client==5.2.0
 
 # PaaS
-awscli==1.15.82
+awscli==1.15.82  # pyup: ignore
 awscli-cwlogs>=1.4,<1.5
-botocore<1.11.0
+botocore<1.11.0  # pyup: ignore
 
-git+https://github.com/alphagov/notifications-utils.git@30.5.4#egg=notifications-utils==30.5.4
+
+# Putting upgrade on hold due to v1.0.0 using sha512 instead of sha1 by default
+itsdangerous==0.24  # pyup: <1.0.0
+
+git+https://github.com/alphagov/notifications-utils.git@30.5.5#egg=notifications-utils==30.5.5
 
 git+https://github.com/alphagov/boto.git@2.43.0-patch3#egg=boto==2.43.0-patch3
 
 ## The following requirements were added by pip freeze:
-alembic==1.0.0
+alembic==1.0.1
 amqp==1.4.9
 anyjson==0.3.3
 bcrypt==3.1.4
 billiard==3.3.0.23
 bleach==2.1.3
 boto3==1.6.16
-certifi==2018.8.24
+certifi==2018.10.15
 chardet==3.0.4
 Click==7.0
 colorama==0.3.9
@@ -51,7 +55,6 @@ future==0.16.0
 greenlet==0.4.15
 html5lib==1.0.1
 idna==2.7
-itsdangerous==0.24
 Jinja2==2.10
 jmespath==0.9.3
 kombu==3.0.37
@@ -70,12 +73,12 @@ python-json-logger==0.1.8
 pytz==2018.5
 PyYAML==3.13
 redis==2.10.6
-requests==2.19.1
+requests==2.20.0
 rsa==3.4.2
 s3transfer==0.1.13
 six==1.11.0
 smartypants==2.0.1
 statsd==3.2.2
-urllib3==1.23
+urllib3==1.24
 webencodings==0.5.1
 Werkzeug==0.14.1

requirements_for_test.txt

@@ -1,13 +1,13 @@
 -r requirements.txt
 flake8==3.5.0
-pytest==3.8.0
+pytest==3.9.1
-moto==1.3.5
+moto==1.3.6
 pytest-env==0.6.2
 pytest-mock==1.10.0
 pytest-cov==2.6.0
-pytest-xdist==1.23.0
+pytest-xdist==1.23.2
-coveralls==1.5.0
+coveralls==1.5.1
-freezegun==0.3.10
+freezegun==0.3.11
 requests-mock==1.5.2
 # optional requirements for jsonschema
 strict-rfc3339==0.7

tests/app/celery/test_letters_pdf_tasks.py

@@ -23,8 +23,8 @@ from app.celery.letters_pdf_tasks import (
     process_virus_scan_failed,
     process_virus_scan_error,
     replay_letters_in_error,
-    _sanitise_precomiled_pdf,
+    _get_page_count,
-    _get_page_count
+    _sanitise_precompiled_pdf
 )
 from app.letters.utils import get_letter_pdf_filename, ScanErrorType
 from app.models import (
@@ -54,6 +54,7 @@ def test_get_letters_pdf_calls_notifications_template_preview_service_correctly(
         notify_api, mocker, client, sample_letter_template, personalisation):
     contact_block = 'Mr Foo,\n1 Test Street,\nLondon\nN1'
     dvla_org_id = '002'
+    filename = 'opg'
 
     with set_config_values(notify_api, {
         'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview',
@@ -64,12 +65,17 @@ def test_get_letters_pdf_calls_notifications_template_preview_service_correctly(
                 'http://localhost/notifications-template-preview/print.pdf', content=b'\x00\x01', status_code=200)
 
             get_letters_pdf(
-                sample_letter_template, contact_block=contact_block, org_id=dvla_org_id, values=personalisation)
+                sample_letter_template,
+                contact_block=contact_block,
+                org_id=dvla_org_id,
+                filename=filename,
+                values=personalisation)
 
     assert mock_post.last_request.json() == {
         'values': personalisation,
         'letter_contact_block': contact_block,
         'dvla_org_id': dvla_org_id,
+        'filename': filename,
         'template': {
             'subject': sample_letter_template.subject,
             'content': sample_letter_template.content
@@ -86,6 +92,7 @@ def test_get_letters_pdf_calculates_billing_units(
         notify_api, mocker, client, sample_letter_template, page_count, expected_billable_units):
     contact_block = 'Mr Foo,\n1 Test Street,\nLondon\nN1'
     dvla_org_id = '002'
+    filename = 'opg'
 
     with set_config_values(notify_api, {
         'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview',
@@ -100,7 +107,7 @@ def test_get_letters_pdf_calculates_billing_units(
             )
 
             _, billable_units = get_letters_pdf(
-                sample_letter_template, contact_block=contact_block, org_id=dvla_org_id, values=None)
+                sample_letter_template, contact_block=contact_block, org_id=dvla_org_id, filename=filename, values=None)
 
     assert billable_units == expected_billable_units
 
@@ -363,7 +370,7 @@ def test_process_letter_task_check_virus_scan_passed(
 
     mock_get_page_count = mocker.patch('app.celery.letters_pdf_tasks._get_page_count', return_value=1)
     mock_s3upload = mocker.patch('app.celery.letters_pdf_tasks.s3upload')
-    mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precomiled_pdf', return_value=b'pdf_content')
+    mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precompiled_pdf', return_value=b'pdf_content')
 
     process_virus_scan_passed(filename)
 
@@ -408,7 +415,7 @@ def test_process_letter_task_check_virus_scan_passed_when_sanitise_fails(
     sample_letter_notification.status = NOTIFICATION_PENDING_VIRUS_CHECK
     sample_letter_notification.key_type = key_type
     mock_move_s3 = mocker.patch('app.letters.utils._move_s3_object')
-    mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precomiled_pdf', return_value=None)
+    mock_sanitise = mocker.patch('app.celery.letters_pdf_tasks._sanitise_precompiled_pdf', return_value=None)
     mock_get_page_count = mocker.patch('app.celery.letters_pdf_tasks._get_page_count', return_value=2)
 
     process_virus_scan_passed(filename)
@@ -489,7 +496,7 @@ def test_sanitise_precompiled_pdf_returns_data_from_template_preview(rmock, samp
     rmock.post('http://localhost:9999/precompiled/sanitise', content=b'new_pdf', status_code=200)
     mock_celery = Mock(**{'retry.side_effect': Retry})
 
-    res = _sanitise_precomiled_pdf(mock_celery, sample_letter_notification, b'old_pdf')
+    res = _sanitise_precompiled_pdf(mock_celery, sample_letter_notification, b'old_pdf')
 
     assert res == b'new_pdf'
     assert rmock.last_request.text == 'old_pdf'
@@ -500,7 +507,7 @@ def test_sanitise_precompiled_pdf_returns_none_on_validation_error(rmock, sample
     rmock.post('http://localhost:9999/precompiled/sanitise', content=b'new_pdf', status_code=400)
     mock_celery = Mock(**{'retry.side_effect': Retry})
 
-    res = _sanitise_precomiled_pdf(mock_celery, sample_letter_notification, b'old_pdf')
+    res = _sanitise_precompiled_pdf(mock_celery, sample_letter_notification, b'old_pdf')
 
     assert res is None
 
@@ -511,7 +518,7 @@ def test_sanitise_precompiled_pdf_retries_on_http_error(rmock, sample_letter_not
     mock_celery = Mock(**{'retry.side_effect': Retry})
 
     with pytest.raises(Retry):
-        _sanitise_precomiled_pdf(mock_celery, sample_letter_notification, b'old_pdf')
+        _sanitise_precompiled_pdf(mock_celery, sample_letter_notification, b'old_pdf')
 
 
 def test_sanitise_precompiled_pdf_sets_notification_to_technical_failure_after_too_many_errors(
@@ -523,6 +530,6 @@ def test_sanitise_precompiled_pdf_sets_notification_to_technical_failure_after_t
     mock_celery = Mock(**{'retry.side_effect': MaxRetriesExceededError})
 
     with pytest.raises(MaxRetriesExceededError):
-        _sanitise_precomiled_pdf(mock_celery, sample_letter_notification, b'old_pdf')
+        _sanitise_precompiled_pdf(mock_celery, sample_letter_notification, b'old_pdf')
 
     assert sample_letter_notification.status == NOTIFICATION_TECHNICAL_FAILURE

tests/app/service/test_rest.py

@@ -139,6 +139,7 @@ def test_get_service_by_id(admin_request, sample_service):
     assert json_resp['data']['dvla_organisation'] == '001'
     assert json_resp['data']['prefix_sms'] is True
     assert json_resp['data']['postage'] == 'second'
+    assert json_resp['data']['letter_logo_filename'] == 'hm-government'
 
 
 @pytest.mark.parametrize('detailed', [True, False])