diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index d888e4c3a..0696f1f6b 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/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_precomiled_pdf(self, notification, old_pdf) + new_pdf = _sanitise_precompiled_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( - "sanitise_precomiled_pdf validation error for notification: {}".format(notification.id) + current_app.logger.info( + "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( - "RETRY FAILED: sanitise_precomiled_pdf failed for notification {}".format(notification.id), + current_app.logger.error( + "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={ diff --git a/app/celery/service_callback_tasks.py b/app/celery/service_callback_tasks.py index 258ba9f9b..640630733 100644 --- a/app/celery/service_callback_tasks.py +++ b/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( - """Retry: {} has retried the max num of times - for notification: {}""".format(function_name, notification_id) + current_app.logger.error( + "Retry: {} has retried the max num of times for notification: {}".format( + function_name, + notification_id + ) ) diff --git a/app/celery/tasks.py b/app/celery/tasks.py index fe220d9f5..32932b320 100644 --- a/app/celery/tasks.py +++ b/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') diff --git a/app/schemas.py b/app/schemas.py index 5692b78f8..f4730afa5 100644 --- a/app/schemas.py +++ b/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] diff --git a/app/template/rest.py b/app/template/rest.py index c2e3763b6..749d30ccb 100644 --- a/app/template/rest.py +++ b/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, } diff --git a/requirements-app.txt b/requirements-app.txt index f55b54765..2445849d6 100644 --- a/requirements-app.txt +++ b/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 diff --git a/requirements.txt b/requirements.txt index e848c5cf2..20bc18f34 100644 --- a/requirements.txt +++ b/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 diff --git a/requirements_for_test.txt b/requirements_for_test.txt index dba78dbbb..312241053 100644 --- a/requirements_for_test.txt +++ b/requirements_for_test.txt @@ -1,13 +1,13 @@ -r requirements.txt flake8==3.5.0 -pytest==3.8.0 -moto==1.3.5 +pytest==3.9.1 +moto==1.3.6 pytest-env==0.6.2 pytest-mock==1.10.0 pytest-cov==2.6.0 -pytest-xdist==1.23.0 -coveralls==1.5.0 -freezegun==0.3.10 +pytest-xdist==1.23.2 +coveralls==1.5.1 +freezegun==0.3.11 requests-mock==1.5.2 # optional requirements for jsonschema strict-rfc3339==0.7 diff --git a/tests/app/celery/test_letters_pdf_tasks.py b/tests/app/celery/test_letters_pdf_tasks.py index 2be4bc8d6..da44e927c 100644 --- a/tests/app/celery/test_letters_pdf_tasks.py +++ b/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 diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index ed2b273d0..7dfaad89a 100644 --- a/tests/app/service/test_rest.py +++ b/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])