diff --git a/app/celery/tasks.py b/app/celery/tasks.py index 8610504b4..1145f8e0e 100644 --- a/app/celery/tasks.py +++ b/app/celery/tasks.py @@ -42,7 +42,7 @@ from app.dao.returned_letters_dao import insert_or_update_returned_letters from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_inbound_api_dao import get_service_inbound_api_for_service from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id -from app.dao.services_dao import dao_fetch_service_by_id, fetch_todays_total_message_count +from app.dao.services_dao import fetch_todays_total_message_count from app.dao.templates_dao import dao_get_template_by_id from app.exceptions import DVLAException, NotificationTechnicalFailureException from app.models import ( @@ -65,6 +65,7 @@ from app.models import ( ) from app.notifications.process_notifications import persist_notification from app.service.utils import service_allowed_to_send_to +from app.serialised_models import SerialisedService, SerialisedTemplate from app.utils import DATETIME_FORMAT @@ -190,13 +191,17 @@ def save_sms(self, encrypted_notification, sender_id=None): notification = encryption.decrypt(encrypted_notification) - service = dao_fetch_service_by_id(service_id) - template = dao_get_template_by_id(notification['template'], version=notification['template_version']) + service = SerialisedService.from_id(service_id) + template = SerialisedTemplate.from_id_and_service_id( + notification['template'], + service_id=service.id, + version=notification['template_version'], + ) if sender_id: reply_to_text = dao_get_service_sms_senders_by_id(service_id, sender_id).sms_sender else: - reply_to_text = template.get_reply_to_text() + reply_to_text = template.reply_to_text if not service_allowed_to_send_to(notification['to'], service, KEY_TYPE_NORMAL): current_app.logger.debug( @@ -246,13 +251,17 @@ def save_email(self, sender_id=None): notification = encryption.decrypt(encrypted_notification) - service = dao_fetch_service_by_id(service_id) - template = dao_get_template_by_id(notification['template'], version=notification['template_version']) + service = SerialisedService.from_id(service_id) + template = SerialisedTemplate.from_id_and_service_id( + notification['template'], + service_id=service.id, + version=notification['template_version'], + ) if sender_id: reply_to_text = dao_get_reply_to_by_id(service_id, sender_id).email_address else: - reply_to_text = template.get_reply_to_text() + reply_to_text = template.reply_to_text if not service_allowed_to_send_to(notification['to'], service, KEY_TYPE_NORMAL): current_app.logger.info("Email {} failed as restricted service".format(notification_id)) @@ -300,7 +309,7 @@ def save_api_sms(self, encrypted_notification): def save_api_email_or_sms(self, encrypted_notification): notification = encryption.decrypt(encrypted_notification) - service = dao_fetch_service_by_id(notification['service_id']) + service = SerialisedService.from_id(notification['service_id']) q = QueueNames.SEND_EMAIL if notification['notification_type'] == EMAIL_TYPE else QueueNames.SEND_SMS provider_task = provider_tasks.deliver_email if notification['notification_type'] == EMAIL_TYPE \ else provider_tasks.deliver_sms @@ -356,8 +365,12 @@ def save_letter( Columns(notification['personalisation']) ) - service = dao_fetch_service_by_id(service_id) - template = dao_get_template_by_id(notification['template'], version=notification['template_version']) + service = SerialisedService.from_id(service_id) + template = SerialisedTemplate.from_id_and_service_id( + notification['template'], + service_id=service.id, + version=notification['template_version'], + ) try: # if we don't want to actually send the letter, then start it off in SENDING so we don't pick it up @@ -378,7 +391,7 @@ def save_letter( job_row_number=notification['row_number'], notification_id=notification_id, reference=create_random_identifier(), - reply_to_text=template.get_reply_to_text(), + reply_to_text=template.reply_to_text, status=status ) diff --git a/app/serialised_models.py b/app/serialised_models.py index 295b298ad..31870a3d5 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -51,18 +51,19 @@ class SerialisedTemplate(SerialisedModel): @classmethod @memory_cache - def from_id_and_service_id(cls, template_id, service_id): - return cls(cls.get_dict(template_id, service_id)['data']) + def from_id_and_service_id(cls, template_id, service_id, version=None): + return cls(cls.get_dict(template_id, service_id, version)['data']) @staticmethod - @redis_cache.set('service-{service_id}-template-{template_id}-version-None') - def get_dict(template_id, service_id): + @redis_cache.set('service-{service_id}-template-{template_id}-version-{version}') + def get_dict(template_id, service_id, version): from app.dao import templates_dao from app.schemas import template_schema fetched_template = templates_dao.dao_get_template_by_id_and_service_id( template_id=template_id, - service_id=service_id + service_id=service_id, + version=version, ) template_dict = template_schema.dump(fetched_template).data diff --git a/tests/app/celery/test_tasks.py b/tests/app/celery/test_tasks.py index 8551656c7..f7393b1df 100644 --- a/tests/app/celery/test_tasks.py +++ b/tests/app/celery/test_tasks.py @@ -49,6 +49,7 @@ from app.models import ( SMS_TYPE, ReturnedLetter, NOTIFICATION_CREATED) +from app.serialised_models import SerialisedService, SerialisedTemplate from app.utils import DATETIME_FORMAT from tests.app import load_example_csv @@ -1888,3 +1889,119 @@ def test_save_api_email_dont_retry_if_notification_already_exists(sample_service assert notifications[0].created_at == datetime(2020, 3, 25, 14, 30) # should only have sent the notification once. mock_provider_task.assert_called_once_with([data['id']], queue=expected_queue) + + +@pytest.mark.parametrize('task_function, delivery_mock, recipient, template_args', ( + ( + save_email, + 'app.celery.provider_tasks.deliver_email.apply_async', + 'test@example.com', + {'template_type': 'email', 'subject': 'Hello'}, + ), ( + save_sms, + 'app.celery.provider_tasks.deliver_sms.apply_async', + '07700 900890', + {'template_type': 'sms'} + ), ( + save_letter, + 'app.celery.letters_pdf_tasks.get_pdf_for_templated_letter.apply_async', + '123 Example Street\nCity of Town\nXM4 5HQ', + {'template_type': 'letter', 'subject': 'Hello'} + ), +)) +def test_save_tasks_use_cached_service_and_template( + notify_db_session, + mocker, + task_function, + delivery_mock, + recipient, + template_args, +): + service = create_service() + template = create_template(service=service, **template_args) + + notification = _notification_json(template, to=recipient) + delivery_mock = mocker.patch(delivery_mock) + service_dict_mock = mocker.patch( + 'app.serialised_models.SerialisedService.get_dict', + wraps=SerialisedService.get_dict, + ) + template_dict_mock = mocker.patch( + 'app.serialised_models.SerialisedTemplate.get_dict', + wraps=SerialisedTemplate.get_dict, + ) + + for _ in range(3): + task_function( + service.id, + uuid.uuid4(), + encryption.encrypt(notification), + ) + + # We talk to the database once for the service and once for the + # template; subsequent calls are caught by the in memory cache + assert service_dict_mock.call_args_list == [ + call(service.id), + ] + assert template_dict_mock.call_args_list == [ + call(str(template.id), str(service.id), 1), + ] + + # But we save 3 notifications and enqueue 3 tasks + assert len(Notification.query.all()) == 3 + assert len(delivery_mock.call_args_list) == 3 + + +@freeze_time('2020-03-25 14:30') +@pytest.mark.parametrize('notification_type, task_function, expected_queue, recipient', ( + ('sms', save_api_sms, QueueNames.SEND_SMS, '+447700900855'), + ('email', save_api_email, QueueNames.SEND_EMAIL, 'jane.citizen@example.com'), +)) +def test_save_api_tasks_use_cache( + sample_service, + mocker, + notification_type, + task_function, + expected_queue, + recipient, +): + mock_provider_task = mocker.patch( + f'app.celery.provider_tasks.deliver_{notification_type}.apply_async' + ) + service_dict_mock = mocker.patch( + 'app.serialised_models.SerialisedService.get_dict', + wraps=SerialisedService.get_dict, + ) + + template = create_template(sample_service, template_type=notification_type) + api_key = create_api_key(service=template.service) + + def create_encrypted_notification(): + return encryption.encrypt({ + "to": recipient, + "id": str(uuid.uuid4()), + "template_id": str(template.id), + "template_version": template.version, + "service_id": str(template.service_id), + "personalisation": None, + "notification_type": template.template_type, + "api_key_id": str(api_key.id), + "key_type": api_key.key_type, + "client_reference": 'our email', + "reply_to_text": "our.email@gov.uk", + "document_download_count": 0, + "status": NOTIFICATION_CREATED, + "created_at": datetime.utcnow().strftime(DATETIME_FORMAT), + }) + + assert len(Notification.query.all()) == 0 + + for _ in range(3): + task_function(encrypted_notification=create_encrypted_notification()) + + assert service_dict_mock.call_args_list == [ + call(str(template.service_id)) + ] + + assert len(Notification.query.all()) == 3 + assert len(mock_provider_task.call_args_list) == 3 diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 07b5617b3..37d28ef9b 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/tests/app/v2/notifications/test_post_notifications.py @@ -233,7 +233,7 @@ def test_should_cache_template_lookups_in_memory(mocker, client, sample_template assert mock_get_template.call_count == 1 assert mock_get_template.call_args_list == [ - call(service_id=str(sample_template.service_id), template_id=str(sample_template.id)) + call(service_id=str(sample_template.service_id), template_id=str(sample_template.id), version=None) ] assert Notification.query.count() == 5