From da93ba296eb58334912da113e5905317dc73716e Mon Sep 17 00:00:00 2001 From: Ken Tsang Date: Tue, 12 Dec 2017 11:56:42 +0000 Subject: [PATCH] Refactored notifications_dao - Introduce a `_raise` flag for `get_notification_by_id` so that sql alchemy will raise the NoResults error rather than the app - Refactor `dao_set_created_live_letter_api_notifications_to_pending` to use a join for getting services that don't have `letters_as_pdf` as marginally faster. --- app/celery/letters_pdf_tasks.py | 5 +--- app/dao/notifications_dao.py | 24 +++++++++---------- .../test_letter_api_notification_dao.py | 4 +--- 3 files changed, 13 insertions(+), 20 deletions(-) diff --git a/app/celery/letters_pdf_tasks.py b/app/celery/letters_pdf_tasks.py index 5b675f34b..24c7347ae 100644 --- a/app/celery/letters_pdf_tasks.py +++ b/app/celery/letters_pdf_tasks.py @@ -5,7 +5,6 @@ from requests import ( ) from botocore.exceptions import ClientError as BotoClientError -from sqlalchemy.orm.exc import NoResultFound from app import notify_celery from app.aws import s3 @@ -18,9 +17,7 @@ from app.statsd_decorators import statsd @statsd(namespace="tasks") def create_letters_pdf(self, notification_id): try: - notification = get_notification_by_id(notification_id) - if not notification: - raise NoResultFound() + notification = get_notification_by_id(notification_id, _raise=True) pdf_data = get_letters_pdf( notification.template, diff --git a/app/dao/notifications_dao.py b/app/dao/notifications_dao.py index 1ecef5f7c..29b49ba7c 100644 --- a/app/dao/notifications_dao.py +++ b/app/dao/notifications_dao.py @@ -226,8 +226,11 @@ def get_notification_with_personalisation(service_id, notification_id, key_type) @statsd(namespace="dao") -def get_notification_by_id(notification_id): - return Notification.query.filter_by(id=notification_id).first() +def get_notification_by_id(notification_id, _raise=False): + if _raise: + return Notification.query.filter_by(id=notification_id).one() + else: + return Notification.query.filter_by(id=notification_id).first() def get_notifications(filter_dict=None): @@ -547,24 +550,19 @@ def dao_set_created_live_letter_api_notifications_to_pending(): Note - do not process services that have letters_as_pdf permission as they will get processed when the letters PDF zip task is created """ - - # Ignore services that have letters_as_pdf permission - services_without_letters_as_pdf = [ - s.id for s in Service.query.filter( - ~Service.permissions.any( - ServicePermission.permission == 'letters_as_pdf' - ) - ).all() - ] - notifications = db.session.query( Notification + ).join( + Service ).filter( Notification.notification_type == LETTER_TYPE, Notification.status == NOTIFICATION_CREATED, Notification.key_type == KEY_TYPE_NORMAL, Notification.api_key != None, # noqa - Notification.service_id.in_(services_without_letters_as_pdf) + # Ignore services that have letters_as_pdf permission + ~Service.permissions.any( + ServicePermission.permission == 'letters_as_pdf' + ) ).with_for_update( ).all() diff --git a/tests/app/dao/notification_dao/test_letter_api_notification_dao.py b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py index 96f79ceff..c52de01e1 100644 --- a/tests/app/dao/notification_dao/test_letter_api_notification_dao.py +++ b/tests/app/dao/notification_dao/test_letter_api_notification_dao.py @@ -25,9 +25,7 @@ def test_should_only_get_letter_notifications( assert ret == [sample_letter_notification] -def test_should_ignore_letters_as_pdf( - sample_letter_notification, -): +def test_should_ignore_letters_as_pdf(sample_letter_notification): service = create_service(service_permissions=[LETTER_TYPE, 'letters_as_pdf']) template = create_template(service, template_type=LETTER_TYPE) create_notification(template)