Refactored tasks.py to split out letters_pdf tasks

- Added has_permission helper in models.py to check permission in service
- Moved letters pdf tasks to separate file
- Moved letters pdf tests to own file

app/celery/letters_pdf_tasks.py

@@ -0,0 +1,68 @@
+from flask import current_app
+from requests import (
+    post as requests_post,
+    RequestException
+)
+
+from botocore.exceptions import ClientError as BotoClientError
+from sqlalchemy.orm.exc import NoResultFound
+
+from app import notify_celery
+from app.aws import s3
+from app.config import QueueNames
+from app.dao.notifications_dao import get_notification_by_id, update_notification_status_by_id
+from app.statsd_decorators import statsd
+
+
+@notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300)
+@statsd(namespace="tasks")
+def create_letters_pdf(self, notification_id):
+    try:
+        notification = get_notification_by_id(notification_id)
+        if not notification:
+            raise NoResultFound()
+
+        pdf_data = get_letters_pdf(
+            notification.template,
+            contact_block=notification.reply_to_text,
+            org_id=notification.service.dvla_organisation.id,
+            values=notification.personalisation
+        )
+        current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format(
+            notification.id, notification.reference, notification.created_at, len(pdf_data)))
+        s3.upload_letters_pdf(reference=notification.reference, crown=True, filedata=pdf_data)
+    except (RequestException, BotoClientError):
+        try:
+            current_app.logger.exception(
+                "Letters PDF notification creation for id: {} failed".format(notification_id)
+            )
+            self.retry(queue=QueueNames.RETRY)
+        except self.MaxRetriesExceededError:
+            current_app.logger.exception(
+                "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):
+    template_for_letter_print = {
+        "subject": template.subject,
+        "content": template.content
+    }
+
+    data = {
+        'letter_contact_block': contact_block,
+        'template': template_for_letter_print,
+        'values': values,
+        'dvla_org_id': org_id,
+    }
+    resp = requests_post(
+        '{}/print.pdf'.format(
+            current_app.config['TEMPLATE_PREVIEW_API_HOST']
+        ),
+        json=data,
+        headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])}
+    )
+    resp.raise_for_status()
+
+    return resp.content

app/celery/tasks.py

@@ -4,7 +4,6 @@ from collections import namedtuple
 
 from celery.signals import worker_process_shutdown
 from flask import current_app
-import requests
 
 from notifications_utils.recipients import (
     RecipientCSV
@@ -20,7 +19,6 @@ from requests import (
     RequestException
 )
 from sqlalchemy.exc import SQLAlchemyError
-from sqlalchemy.orm.exc import NoResultFound
 from botocore.exceptions import ClientError as BotoClientError
 
 from app import (
@@ -32,6 +30,7 @@ from app import (
 )
 from app.aws import s3
 from app.celery import provider_tasks
+from app.celery import letters_pdf_tasks
 from app.celery.service_callback_tasks import send_delivery_status_to_service
 from app.config import QueueNames
 from app.dao.inbound_sms_dao import dao_get_inbound_sms_by_id
@@ -48,7 +47,6 @@ from app.dao.notifications_dao import (
     dao_update_notifications_by_reference,
     dao_get_last_notification_added_for_job_id,
     dao_get_notifications_by_references,
-    update_notification_status_by_id
 )
 from app.dao.provider_details_dao import get_current_provider
 from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service
@@ -127,7 +125,7 @@ def process_job(job_id):
 def job_complete(job, service, template_type, resumed=False, start=None):
     if (
         template_type == LETTER_TYPE and
-        'letters_as_pdf' not in [p.permission for p in service.permissions]
+        not service.has_permission('letters_as_pdf')
     ):
         if service.research_mode:
             update_job_to_sent_to_dvla.apply_async([str(job.id)], queue=QueueNames.RESEARCH_MODE)
@@ -320,9 +318,9 @@ def save_letter(
         )
 
         if (
-            'letters_as_pdf' in [p.permission for p in service.permissions] and not service.research_mode
+            service.has_permission('letters_as_pdf') and not service.research_mode
         ):
-            create_letters_pdf.apply_async(
+            letters_pdf_tasks.create_letters_pdf.apply_async(
                 [str(saved_notification.id)],
                 queue=QueueNames.CREATE_LETTERS_PDF
             )
@@ -596,57 +594,3 @@ def process_incomplete_job(job_id):
             process_row(row_number, recipient, personalisation, template, job, job.service)
 
     job_complete(job, job.service, template.template_type, resumed=True)
-
-
-@notify_celery.task(bind=True, name="create-letters-pdf", max_retries=15, default_retry_delay=300)
-@statsd(namespace="tasks")
-def create_letters_pdf(self, notification_id):
-    try:
-        notification = get_notification_by_id(notification_id)
-        if not notification:
-            raise NoResultFound()
-
-        pdf_data = get_letters_pdf(
-            notification.template,
-            contact_block=notification.reply_to_text,
-            org_id=notification.service.dvla_organisation.id,
-            values=notification.personalisation
-        )
-        current_app.logger.info("PDF Letter {} reference {} created at {}, {} bytes".format(
-            notification.id, notification.reference, notification.created_at, len(pdf_data)))
-        s3.upload_letters_pdf(reference=notification.reference, crown=True, filedata=pdf_data)
-    except (RequestException, BotoClientError):
-        try:
-            current_app.logger.exception(
-                "Letters PDF notification creation for id: {} failed".format(notification_id)
-            )
-            self.retry(queue=QueueNames.RETRY)
-        except self.MaxRetriesExceededError:
-            current_app.logger.exception(
-                "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=None):
-    template_for_letter_print = {
-        "subject": template.subject,
-        "content": template.content
-    }
-
-    data = {
-        'letter_contact_block': contact_block,
-        'template': template_for_letter_print,
-        'values': values,
-        'dvla_org_id': org_id,
-    }
-    resp = requests.post(
-        '{}/print.pdf'.format(
-            current_app.config['TEMPLATE_PREVIEW_API_HOST']
-        ),
-        json=data,
-        headers={'Authorization': 'Token {}'.format(current_app.config['TEMPLATE_PREVIEW_API_KEY'])}
-    )
-    resp.raise_for_status()
-
-    return resp.content

app/models.py

@@ -283,6 +283,9 @@ class Service(db.Model, Versioned):
         default_letter_contact = [x for x in self.letter_contacts if x.is_default]
         return default_letter_contact[0].contact_block if default_letter_contact else None
 
+    def has_permission(self, permission):
+        return permission in [p.permission for p in self.permissions]
+
 
 class AnnualBilling(db.Model):
     __tablename__ = "annual_billing"

tests/app/celery/test_letters_pdf_tasks.py

@@ -0,0 +1,99 @@
+import pytest
+import requests_mock
+
+from botocore.exceptions import ClientError
+from celery.exceptions import MaxRetriesExceededError
+from requests import RequestException
+from sqlalchemy.orm.exc import NoResultFound
+
+from app.celery.letters_pdf_tasks import (
+    create_letters_pdf,
+    get_letters_pdf,
+)
+
+from tests.conftest import set_config_values
+
+
+def test_should_have_decorated_tasks_functions():
+    assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf'
+
+
+@pytest.mark.parametrize('personalisation', [{'name': 'test'}, None])
+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'
+
+    with set_config_values(notify_api, {
+        'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview',
+        'TEMPLATE_PREVIEW_API_KEY': 'test-key'
+    }):
+        with requests_mock.Mocker() as request_mock:
+            mock_post = request_mock.post(
+                '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)
+
+    assert mock_post.last_request.json() == {
+        'values': personalisation,
+        'letter_contact_block': contact_block,
+        'dvla_org_id': dvla_org_id,
+        'template': {
+            'subject': sample_letter_template.subject,
+            'content': sample_letter_template.content
+        }
+    }
+
+
+def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification):
+    mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', return_value=b'\x00\x01')
+    mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf')
+
+    create_letters_pdf(sample_letter_notification.id)
+
+    mock_s3.assert_called_with(
+        reference=sample_letter_notification.reference,
+        crown=True,
+        filedata=b'\x00\x01'
+    )
+
+
+def test_create_letters_pdf_non_existent_notification(notify_api, mocker, fake_uuid):
+    with pytest.raises(expected_exception=NoResultFound):
+        create_letters_pdf(fake_uuid)
+
+
+def test_create_letters_pdf_handles_request_errors(mocker, sample_letter_notification):
+    mock_get_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', side_effect=RequestException)
+    mock_retry = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.retry')
+
+    create_letters_pdf(sample_letter_notification.id)
+
+    assert mock_get_letters_pdf.called
+    assert mock_retry.called
+
+
+def test_create_letters_pdf_handles_s3_errors(mocker, sample_letter_notification):
+    mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf')
+    mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name'))
+    mock_retry = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.retry')
+
+    create_letters_pdf(sample_letter_notification.id)
+
+    assert mock_s3.called
+    assert mock_retry.called
+
+
+def test_create_letters_pdf_sets_technical_failure_max_retries(mocker, sample_letter_notification):
+    mock_get_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.get_letters_pdf', side_effect=RequestException)
+    mock_retry = mocker.patch(
+        'app.celery.letters_pdf_tasks.create_letters_pdf.retry', side_effect=MaxRetriesExceededError)
+    mock_update_noti = mocker.patch('app.celery.letters_pdf_tasks.update_notification_status_by_id')
+
+    create_letters_pdf(sample_letter_notification.id)
+
+    assert mock_get_letters_pdf.called
+    assert mock_retry.called
+    assert mock_update_noti.called
+    mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure')

tests/app/celery/test_tasks.py

@@ -9,9 +9,8 @@ from flask import current_app
 from freezegun import freeze_time
 from requests import RequestException
 from sqlalchemy.exc import SQLAlchemyError
-from celery.exceptions import Retry, MaxRetriesExceededError
+from celery.exceptions import Retry
 from botocore.exceptions import ClientError
-from sqlalchemy.orm.exc import NoResultFound
 from notifications_utils.template import SMSMessageTemplate, WithSubjectTemplate, LetterDVLATemplate
 
 from app import (encryption, DATETIME_FORMAT)
@@ -21,8 +20,6 @@ from app.celery.scheduled_tasks import check_job_status
 from app.celery.tasks import (
     build_dvla_file,
     create_dvla_file_contents_for_job,
-    create_letters_pdf,
-    get_letters_pdf,
     job_complete,
     process_job,
     process_row,
@@ -51,7 +48,6 @@ from app.models import (
     SMS_TYPE
 )
 
-from tests.conftest import set_config_values
 from tests.app import load_example_csv
 from tests.app.conftest import (
     sample_service as create_sample_service,
@@ -99,7 +95,6 @@ def test_should_have_decorated_tasks_functions():
     assert save_sms.__wrapped__.__name__ == 'save_sms'
     assert save_email.__wrapped__.__name__ == 'save_email'
     assert save_letter.__wrapped__.__name__ == 'save_letter'
-    assert create_letters_pdf.__wrapped__.__name__ == 'create_letters_pdf'
 
 
 @pytest.fixture
@@ -1048,7 +1043,7 @@ def test_save_letter_saves_letter_to_database(mocker, notify_db_session):
 def test_save_letter_saves_letter_calls_create_letters_as_pdf_with_letters_as_pdf_permission(
         mocker, notify_db_session, sample_letter_job):
     service_permissions_dao.dao_add_service_permission(sample_letter_job.service.id, 'letters_as_pdf')
-    mock_create_letters_pdf = mocker.patch('app.celery.tasks.create_letters_pdf.apply_async')
+    mock_create_letters_pdf = mocker.patch('app.celery.letters_pdf_tasks.create_letters_pdf.apply_async')
 
     personalisation = {
         'addressline1': 'Foo',
@@ -1505,83 +1500,3 @@ def test_process_incomplete_job_letter(mocker, sample_letter_template):
 
     assert mock_build_dvla.called
     assert mock_letter_saver.call_count == 8
-
-
-@pytest.mark.parametrize('personalisation', [{'name': 'test'}, None])
-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'
-
-    with set_config_values(notify_api, {
-        'TEMPLATE_PREVIEW_API_HOST': 'http://localhost/notifications-template-preview',
-        'TEMPLATE_PREVIEW_API_KEY': 'test-key'
-    }):
-        with requests_mock.Mocker() as request_mock:
-            mock_post = request_mock.post(
-                '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)
-
-    assert mock_post.last_request.json() == {
-        'values': personalisation,
-        'letter_contact_block': contact_block,
-        'dvla_org_id': dvla_org_id,
-        'template': {
-            'subject': sample_letter_template.subject,
-            'content': sample_letter_template.content
-        }
-    }
-
-
-def test_create_letters_pdf_calls_upload_letters_pdf(mocker, sample_letter_notification):
-    mocker.patch('app.celery.tasks.get_letters_pdf', return_value=b'\x00\x01')
-    mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf')
-
-    create_letters_pdf(sample_letter_notification.id)
-
-    mock_s3.assert_called_with(
-        reference=sample_letter_notification.reference,
-        crown=True,
-        filedata=b'\x00\x01'
-    )
-
-
-def test_create_letters_pdf_non_existent_notification(notify_api, mocker, fake_uuid):
-    with pytest.raises(expected_exception=NoResultFound):
-        create_letters_pdf(fake_uuid)
-
-
-def test_create_letters_pdf_handles_request_errors(mocker, sample_letter_notification):
-    mock_get_letters_pdf = mocker.patch('app.celery.tasks.get_letters_pdf', side_effect=RequestException)
-    mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry')
-
-    create_letters_pdf(sample_letter_notification.id)
-
-    assert mock_get_letters_pdf.called
-    assert mock_retry.called
-
-
-def test_create_letters_pdf_handles_s3_errors(mocker, sample_letter_notification):
-    mocker.patch('app.celery.tasks.get_letters_pdf')
-    mock_s3 = mocker.patch('app.celery.tasks.s3.upload_letters_pdf', side_effect=ClientError({}, 'operation_name'))
-    mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry')
-
-    create_letters_pdf(sample_letter_notification.id)
-
-    assert mock_s3.called
-    assert mock_retry.called
-
-
-def test_create_letters_pdf_sets_technical_failure_max_retries(mocker, sample_letter_notification):
-    mock_get_letters_pdf = mocker.patch('app.celery.tasks.get_letters_pdf', side_effect=RequestException)
-    mock_retry = mocker.patch('app.celery.tasks.create_letters_pdf.retry', side_effect=MaxRetriesExceededError)
-    mock_update_noti = mocker.patch('app.celery.tasks.update_notification_status_by_id')
-
-    create_letters_pdf(sample_letter_notification.id)
-
-    assert mock_get_letters_pdf.called
-    assert mock_retry.called
-    assert mock_update_noti.called
-    mock_update_noti.assert_called_once_with(sample_letter_notification.id, 'technical-failure')