From ad2328fc056c1b9e222ac73b5123a7a56699b6f6 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Jun 2020 10:20:51 +0100 Subject: [PATCH 1/6] Serialise template immediately after fetching MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This commit changes the code in post notification endpoint to handle a serialised template (ie a `dict`) rather than a database object. This is the first step towards being able to cache the template and not hit the database on every request. There should be no functional changes here, it’s just refactoring. There are some changes to the tests where the signature of functions has changed. Importing of the template schema has to be done at a function level, otherwise Marshmallow gets weird. This commit also copies the `JSONModel` class from the admin app, which turns serialised data (a dict made from JSON) into an object on which certain predefined properties are allowed. This means we can still do the caching of serialised data, without having to change too much of the code in the app, or make it ugly by sprinkling dict lookups everywhere. We’re not copying all of JSONModel from the admin app, just the bits we need. We don’t need to compare or hash these objects, they’re just used for lookups. And redefining `__getattribute__` scares Leo. --- app/json_models.py | 48 +++++++++++++++++++ .../process_letter_notifications.py | 11 ++++- app/notifications/process_notifications.py | 17 ++++++- app/notifications/rest.py | 2 +- app/notifications/validators.py | 9 ++-- app/v2/notifications/post_notifications.py | 42 ++++++++++------ .../test_process_letter_notifications.py | 39 +++++++++++++-- .../test_process_notification.py | 22 ++++++--- tests/app/notifications/test_validators.py | 7 +-- 9 files changed, 159 insertions(+), 38 deletions(-) create mode 100644 app/json_models.py diff --git a/app/json_models.py b/app/json_models.py new file mode 100644 index 000000000..917cc4179 --- /dev/null +++ b/app/json_models.py @@ -0,0 +1,48 @@ +from abc import ABC, abstractmethod + +from app.dao import templates_dao + + +class JSONModel(ABC): + + @property + @abstractmethod + def ALLOWED_PROPERTIES(self): + pass + + def __init__(self, _dict): + self._dict = _dict + for property in self.ALLOWED_PROPERTIES: + setattr(self, property, _dict[property]) + + def __dir__(self): + return super().__dir__() + list(sorted(self.ALLOWED_PROPERTIES)) + + +class TemplateJSONModel(JSONModel): + ALLOWED_PROPERTIES = { + 'archived', + 'id', + 'postage', + 'process_type', + 'reply_to_text', + 'template_type', + 'version', + } + + @classmethod + def from_id_and_service_id(cls, template_id, service_id): + return cls(cls.get_dict(template_id, service_id)) + + @staticmethod + def get_dict(template_id, service_id): + 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 + ) + + template_dict = template_schema.dump(fetched_template).data + + return template_dict diff --git a/app/notifications/process_letter_notifications.py b/app/notifications/process_letter_notifications.py index cf967eb23..2af7f5f7a 100644 --- a/app/notifications/process_letter_notifications.py +++ b/app/notifications/process_letter_notifications.py @@ -6,7 +6,14 @@ from app.notifications.process_notifications import persist_notification def create_letter_notification( - letter_data, template, api_key, status, reply_to_text=None, billable_units=None, updated_at=None + letter_data, + template, + service, + api_key, + status, + reply_to_text=None, + billable_units=None, + updated_at=None, ): notification = persist_notification( template_id=template.id, @@ -14,7 +21,7 @@ def create_letter_notification( template_postage=template.postage, # we only accept addresses_with_underscores from the API (from CSV we also accept dashes, spaces etc) recipient=PostalAddress.from_personalisation(letter_data['personalisation']).normalised, - service=template.service, + service=service, personalisation=letter_data['personalisation'], notification_type=LETTER_TYPE, api_key_id=api_key.id, diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index b185a88c3..c48e7e5e9 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -9,6 +9,11 @@ from notifications_utils.recipients import ( validate_and_format_phone_number, format_email_address ) +from notifications_utils.template import ( + PlainTextEmailTemplate, + SMSMessageTemplate, + LetterPrintTemplate, +) from notifications_utils.timezones import convert_bst_to_utc from app import redis_store @@ -44,7 +49,17 @@ REDIS_GET_AND_INCR_DAILY_LIMIT_DURATION_SECONDS = Histogram( def create_content_for_notification(template, personalisation): - template_object = template._as_utils_template_with_personalisation(personalisation) + if template.template_type == EMAIL_TYPE: + template_object = PlainTextEmailTemplate(template._dict, personalisation) + if template.template_type == SMS_TYPE: + template_object = SMSMessageTemplate(template._dict, personalisation) + if template.template_type == LETTER_TYPE: + template_object = LetterPrintTemplate( + template._dict, + personalisation, + contact_block=template.reply_to_text, + ) + check_placeholders(template_object) return template_object diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 692c960fd..81e70f6e2 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -128,7 +128,7 @@ def send_notification(notification_type): api_key_id=api_user.id, key_type=api_user.key_type, simulated=simulated, - reply_to_text=template.get_reply_to_text() + reply_to_text=template.reply_to_text ) if not simulated: queue_name = QueueNames.PRIORITY if template.process_type == PRIORITY else None diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 32aaa3b9c..9ce9ed6c9 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -8,7 +8,7 @@ from notifications_utils.recipients import ( ) from notifications_utils.clients.redis import rate_limit_cache_key, daily_limit_cache_key -from app.dao import services_dao, templates_dao +from app.dao import services_dao from app.dao.service_sms_sender_dao import dao_get_service_sms_senders_by_id from app.models import ( INTERNATIONAL_SMS_TYPE, SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, @@ -21,6 +21,7 @@ from app.notifications.process_notifications import create_content_for_notificat from app.utils import get_public_notify_type_text from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_letter_contact_dao import dao_get_letter_contact_by_id +from app.json_models import TemplateJSONModel from gds_metrics.metrics import Histogram @@ -156,11 +157,9 @@ def check_notification_content_is_not_empty(template_with_content): def validate_template(template_id, personalisation, service, notification_type): + try: - template = templates_dao.dao_get_template_by_id_and_service_id( - template_id=template_id, - service_id=service.id - ) + template = TemplateJSONModel.from_id_and_service_id(template_id, service.id) except NoResultFound: message = 'Template not found' raise BadRequestError(message=message, diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 09aecb0a1..72019f48a 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -98,14 +98,13 @@ def post_precompiled_letter_notification(): 'address_line_1': 'Provided as PDF' } - reply_to = get_reply_to_text(LETTER_TYPE, form, template) - notification = process_letter_notification( letter_data=form, api_key=api_user, + service=authenticated_service, template=template, template_with_content=None, # not required for precompiled - reply_to_text=reply_to, + reply_to_text='', # not required for precompiled precompiled=True ) @@ -147,6 +146,7 @@ def post_notification(notification_type): notification = process_letter_notification( letter_data=form, api_key=api_user, + service=authenticated_service, template=template, template_with_content=template_with_content, reply_to_text=reply_to @@ -156,7 +156,8 @@ def post_notification(notification_type): form=form, notification_type=notification_type, api_key=api_user, - template=template_with_content, + template=template, + template_with_content=template_with_content, template_process_type=template.process_type, service=authenticated_service, reply_to_text=reply_to @@ -166,7 +167,15 @@ def post_notification(notification_type): def process_sms_or_email_notification( - *, form, notification_type, api_key, template, template_process_type, service, reply_to_text=None + *, + form, + notification_type, + api_key, + template, + template_with_content, + template_process_type, + service, + reply_to_text=None, ): notification_id = uuid.uuid4() form_send_to = form['email_address'] if notification_type == EMAIL_TYPE else form['phone_number'] @@ -186,19 +195,19 @@ def process_sms_or_email_notification( ) if document_download_count: # We changed personalisation which means we need to update the content - template.values = personalisation + template_with_content.values = personalisation key_type = api_key.key_type service_in_research_mode = service.research_mode resp = create_response_for_post_notification( notification_id=notification_id, client_reference=form.get('reference', None), template_id=template.id, - template_version=template._template['version'], + template_version=template.version, service_id=service.id, notification_type=notification_type, reply_to=reply_to_text, scheduled_for=form.get("scheduled_for", None), - template_with_content=template) + template_with_content=template_with_content) if str(service.id) in current_app.config.get('HIGH_VOLUME_SERVICE') and api_key.key_type == KEY_TYPE_NORMAL \ and notification_type == EMAIL_TYPE: @@ -229,7 +238,7 @@ def process_sms_or_email_notification( persist_notification( notification_id=notification_id, template_id=template.id, - template_version=template._template['version'], + template_version=template.version, recipient=form_send_to, service=service, personalisation=personalisation, @@ -276,7 +285,7 @@ def save_email_to_queue( data = { "id": notification_id, "template_id": str(template.id), - "template_version": template._template['version'], + "template_version": template.version, "to": form['email_address'], "service_id": str(service_id), "personalisation": personalisation, @@ -328,7 +337,7 @@ def process_document_uploads(personalisation_data, service, simulated=False): def process_letter_notification( - *, letter_data, api_key, template, template_with_content, reply_to_text, precompiled=False + *, letter_data, api_key, service, template, template_with_content, reply_to_text, precompiled=False ): if api_key.key_type == KEY_TYPE_TEAM: raise BadRequestError(message='Cannot send letters with a team api key', status_code=403) @@ -339,6 +348,7 @@ def process_letter_notification( if precompiled: return process_precompiled_letter_notifications(letter_data=letter_data, api_key=api_key, + service=service, template=template, reply_to_text=reply_to_text) @@ -360,6 +370,7 @@ def process_letter_notification( queue = QueueNames.CREATE_LETTERS_PDF if not test_key else QueueNames.RESEARCH_MODE notification = create_letter_notification(letter_data=letter_data, + service=service, template=template, api_key=api_key, status=status, @@ -414,7 +425,7 @@ def validate_address(api_key, letter_data): ) -def process_precompiled_letter_notifications(*, letter_data, api_key, template, reply_to_text): +def process_precompiled_letter_notifications(*, letter_data, api_key, service, template, reply_to_text): try: status = NOTIFICATION_PENDING_VIRUS_CHECK letter_content = base64.b64decode(letter_data['content']) @@ -422,6 +433,7 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, template, raise BadRequestError(message='Cannot decode letter content (invalid base64 encoding)', status_code=400) notification = create_letter_notification(letter_data=letter_data, + service=service, template=template, api_key=api_key, status=status, @@ -460,7 +472,7 @@ def get_reply_to_text(notification_type, form, template): service_email_reply_to_id = form.get("email_reply_to_id", None) reply_to = check_service_email_reply_to_id( str(authenticated_service.id), service_email_reply_to_id, notification_type - ) or template.get_reply_to_text() + ) or template.reply_to_text elif notification_type == SMS_TYPE: service_sms_sender_id = form.get("sms_sender_id", None) @@ -470,10 +482,10 @@ def get_reply_to_text(notification_type, form, template): if sms_sender_id: reply_to = try_validate_and_format_phone_number(sms_sender_id) else: - reply_to = template.get_reply_to_text() + reply_to = template.reply_to_text elif notification_type == LETTER_TYPE: - reply_to = template.get_reply_to_text() + reply_to = template.reply_to_text return reply_to diff --git a/tests/app/notifications/test_process_letter_notifications.py b/tests/app/notifications/test_process_letter_notifications.py index 08117a2b2..5f266b285 100644 --- a/tests/app/notifications/test_process_letter_notifications.py +++ b/tests/app/notifications/test_process_letter_notifications.py @@ -2,6 +2,7 @@ from app.models import LETTER_TYPE from app.models import Notification from app.models import NOTIFICATION_CREATED from app.notifications.process_letter_notifications import create_letter_notification +from app.json_models import TemplateJSONModel def test_create_letter_notification_creates_notification(sample_letter_template, sample_api_key): @@ -13,7 +14,17 @@ def test_create_letter_notification_creates_notification(sample_letter_template, } } - notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED) + template = TemplateJSONModel.from_id_and_service_id( + sample_letter_template.id, sample_letter_template.service_id + ) + + notification = create_letter_notification( + data, + template, + sample_letter_template.service, + sample_api_key, + NOTIFICATION_CREATED, + ) assert notification == Notification.query.one() assert notification.job is None @@ -38,7 +49,17 @@ def test_create_letter_notification_sets_reference(sample_letter_template, sampl 'reference': 'foo' } - notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED) + template = TemplateJSONModel.from_id_and_service_id( + sample_letter_template.id, sample_letter_template.service_id + ) + + notification = create_letter_notification( + data, + template, + sample_letter_template.service, + sample_api_key, + NOTIFICATION_CREATED, + ) assert notification.client_reference == 'foo' @@ -52,7 +73,17 @@ def test_create_letter_notification_sets_billable_units(sample_letter_template, }, } - notification = create_letter_notification(data, sample_letter_template, sample_api_key, NOTIFICATION_CREATED, - billable_units=3) + template = TemplateJSONModel.from_id_and_service_id( + sample_letter_template.id, sample_letter_template.service_id + ) + + notification = create_letter_notification( + data, + template, + sample_letter_template.service, + sample_api_key, + NOTIFICATION_CREATED, + billable_units=3, + ) assert notification.billable_units == 3 diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 5fe5a513a..eebabfe71 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -11,7 +11,6 @@ from app.models import ( Notification, NotificationHistory, ScheduledNotification, - Template, LETTER_TYPE ) from app.notifications.process_notifications import ( @@ -21,32 +20,41 @@ from app.notifications.process_notifications import ( send_notification_to_queue, simulated_recipient ) +from app.json_models import TemplateJSONModel from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address from app.v2.errors import BadRequestError from tests.app.db import create_service, create_template, create_api_key def test_create_content_for_notification_passes(sample_email_template): - template = Template.query.get(sample_email_template.id) + template = TemplateJSONModel.from_id_and_service_id( + sample_email_template.id, sample_email_template.service_id + ) content = create_content_for_notification(template, None) - assert str(content) == template.content + '\n' + assert str(content) == template._dict['content'] + '\n' def test_create_content_for_notification_with_placeholders_passes(sample_template_with_placeholders): - template = Template.query.get(sample_template_with_placeholders.id) + template = TemplateJSONModel.from_id_and_service_id( + sample_template_with_placeholders.id, sample_template_with_placeholders.service_id + ) content = create_content_for_notification(template, {'name': 'Bobby'}) - assert content.content == template.content + assert content.content == template._dict['content'] assert 'Bobby' in str(content) def test_create_content_for_notification_fails_with_missing_personalisation(sample_template_with_placeholders): - template = Template.query.get(sample_template_with_placeholders.id) + template = TemplateJSONModel.from_id_and_service_id( + sample_template_with_placeholders.id, sample_template_with_placeholders.service_id + ) with pytest.raises(BadRequestError): create_content_for_notification(template, None) def test_create_content_for_notification_allows_additional_personalisation(sample_template_with_placeholders): - template = Template.query.get(sample_template_with_placeholders.id) + template = TemplateJSONModel.from_id_and_service_id( + sample_template_with_placeholders.id, sample_template_with_placeholders.service_id + ) create_content_for_notification(template, {'name': 'Bobby', 'Additional placeholder': 'Data'}) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 53c024f5e..c29470c5e 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -23,6 +23,7 @@ from app.notifications.validators import ( validate_and_format_recipient, validate_template, ) +from app.json_models import TemplateJSONModel from app.utils import get_template_instance from app.v2.errors import ( @@ -314,7 +315,7 @@ def test_check_content_char_count_passes_for_long_email_or_letter(sample_service def test_check_notification_content_is_not_empty_passes(notify_api, mocker, sample_service): template_id = create_template(sample_service, content="Content is not empty").id - template = templates_dao.dao_get_template_by_id_and_service_id( + template = TemplateJSONModel.from_id_and_service_id( template_id=template_id, service_id=sample_service.id ) @@ -330,7 +331,7 @@ def test_check_notification_content_is_not_empty_fails( notify_api, mocker, sample_service, template_content, notification_values ): template_id = create_template(sample_service, content=template_content).id - template = templates_dao.dao_get_template_by_id_and_service_id( + template = TemplateJSONModel.from_id_and_service_id( template_id=template_id, service_id=sample_service.id ) @@ -356,7 +357,7 @@ def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_servic ) mock_check_not_empty = mocker.patch('app.notifications.validators.check_notification_content_is_not_empty') mock_check_message_is_too_long = mocker.patch('app.notifications.validators.check_content_char_count') - validate_template(template.id, {}, sample_service, "email") + template, template_with_content = validate_template(template.id, {}, sample_service, "email") mock_check_type.assert_called_once_with("email", "email") mock_check_if_active.assert_called_once_with(template) From 608812d3141189d49f7c8f14a6c6a127fdf07f00 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Jun 2020 10:20:52 +0100 Subject: [PATCH 2/6] =?UTF-8?q?Don=E2=80=99t=20store=20the=20underlying=20?= =?UTF-8?q?dict?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This will give us smaller objects to cache, and forces us to be explicit about which properties we’re using. --- app/json_models.py | 3 ++- app/notifications/process_notifications.py | 23 ++++++++++++++++--- .../test_process_notification.py | 4 ++-- 3 files changed, 24 insertions(+), 6 deletions(-) diff --git a/app/json_models.py b/app/json_models.py index 917cc4179..27cc44cee 100644 --- a/app/json_models.py +++ b/app/json_models.py @@ -11,7 +11,6 @@ class JSONModel(ABC): pass def __init__(self, _dict): - self._dict = _dict for property in self.ALLOWED_PROPERTIES: setattr(self, property, _dict[property]) @@ -22,10 +21,12 @@ class JSONModel(ABC): class TemplateJSONModel(JSONModel): ALLOWED_PROPERTIES = { 'archived', + 'content', 'id', 'postage', 'process_type', 'reply_to_text', + 'subject', 'template_type', 'version', } diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index c48e7e5e9..3f1bc5515 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -50,12 +50,29 @@ REDIS_GET_AND_INCR_DAILY_LIMIT_DURATION_SECONDS = Histogram( def create_content_for_notification(template, personalisation): if template.template_type == EMAIL_TYPE: - template_object = PlainTextEmailTemplate(template._dict, personalisation) + template_object = PlainTextEmailTemplate( + { + 'content': template.content, + 'subject': template.subject, + 'template_type': template.template_type, + }, + personalisation, + ) if template.template_type == SMS_TYPE: - template_object = SMSMessageTemplate(template._dict, personalisation) + template_object = SMSMessageTemplate( + { + 'content': template.content, + 'template_type': template.template_type, + }, + personalisation, + ) if template.template_type == LETTER_TYPE: template_object = LetterPrintTemplate( - template._dict, + { + 'content': template.content, + 'subject': template.subject, + 'template_type': template.template_type, + }, personalisation, contact_block=template.reply_to_text, ) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index eebabfe71..6968e6220 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -31,7 +31,7 @@ def test_create_content_for_notification_passes(sample_email_template): sample_email_template.id, sample_email_template.service_id ) content = create_content_for_notification(template, None) - assert str(content) == template._dict['content'] + '\n' + assert str(content) == template.content + '\n' def test_create_content_for_notification_with_placeholders_passes(sample_template_with_placeholders): @@ -39,7 +39,7 @@ def test_create_content_for_notification_with_placeholders_passes(sample_templat sample_template_with_placeholders.id, sample_template_with_placeholders.service_id ) content = create_content_for_notification(template, {'name': 'Bobby'}) - assert content.content == template._dict['content'] + assert content.content == template.content assert 'Bobby' in str(content) From e6b7e0e16cbbb867fb089536e43922b55d310b1b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Jun 2020 10:20:53 +0100 Subject: [PATCH 3/6] Rename JSONModel to SerialisedModel 1/2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This class doesn’t actually wrap JSON, it wraps serialised data. So this name feels better. This commit only renames the file for an easier diff. --- app/notifications/validators.py | 2 +- app/{json_models.py => serialised_models.py} | 0 tests/app/notifications/test_process_notification.py | 2 +- tests/app/notifications/test_validators.py | 2 +- 4 files changed, 3 insertions(+), 3 deletions(-) rename app/{json_models.py => serialised_models.py} (100%) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 9ce9ed6c9..8bc19d2d9 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -21,7 +21,7 @@ from app.notifications.process_notifications import create_content_for_notificat from app.utils import get_public_notify_type_text from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_letter_contact_dao import dao_get_letter_contact_by_id -from app.json_models import TemplateJSONModel +from app.serialised_models import TemplateJSONModel from gds_metrics.metrics import Histogram diff --git a/app/json_models.py b/app/serialised_models.py similarity index 100% rename from app/json_models.py rename to app/serialised_models.py diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 6968e6220..49c114c74 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -20,7 +20,7 @@ from app.notifications.process_notifications import ( send_notification_to_queue, simulated_recipient ) -from app.json_models import TemplateJSONModel +from app.serialised_models import TemplateJSONModel from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address from app.v2.errors import BadRequestError from tests.app.db import create_service, create_template, create_api_key diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index c29470c5e..e2e200f87 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -23,7 +23,7 @@ from app.notifications.validators import ( validate_and_format_recipient, validate_template, ) -from app.json_models import TemplateJSONModel +from app.serialised_models import TemplateJSONModel from app.utils import get_template_instance from app.v2.errors import ( From 5a2f2a9ec295053306477ec04ed6377118e506ea Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Jun 2020 10:20:53 +0100 Subject: [PATCH 4/6] Rename JSONModel to SerialisedModel 2/2 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit This class doesn’t actually wrap JSON, it wraps serialised data. So this name feels better. --- app/notifications/validators.py | 4 ++-- app/serialised_models.py | 4 ++-- .../notifications/test_process_letter_notifications.py | 8 ++++---- tests/app/notifications/test_process_notification.py | 10 +++++----- tests/app/notifications/test_validators.py | 6 +++--- 5 files changed, 16 insertions(+), 16 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 8bc19d2d9..7eacd8ede 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -21,7 +21,7 @@ from app.notifications.process_notifications import create_content_for_notificat from app.utils import get_public_notify_type_text from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id from app.dao.service_letter_contact_dao import dao_get_letter_contact_by_id -from app.serialised_models import TemplateJSONModel +from app.serialised_models import SerialisedTemplate from gds_metrics.metrics import Histogram @@ -159,7 +159,7 @@ def check_notification_content_is_not_empty(template_with_content): def validate_template(template_id, personalisation, service, notification_type): try: - template = TemplateJSONModel.from_id_and_service_id(template_id, service.id) + template = SerialisedTemplate.from_id_and_service_id(template_id, service.id) except NoResultFound: message = 'Template not found' raise BadRequestError(message=message, diff --git a/app/serialised_models.py b/app/serialised_models.py index 27cc44cee..d3b1cd5ca 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -3,7 +3,7 @@ from abc import ABC, abstractmethod from app.dao import templates_dao -class JSONModel(ABC): +class SerialisedModel(ABC): @property @abstractmethod @@ -18,7 +18,7 @@ class JSONModel(ABC): return super().__dir__() + list(sorted(self.ALLOWED_PROPERTIES)) -class TemplateJSONModel(JSONModel): +class SerialisedTemplate(SerialisedModel): ALLOWED_PROPERTIES = { 'archived', 'content', diff --git a/tests/app/notifications/test_process_letter_notifications.py b/tests/app/notifications/test_process_letter_notifications.py index 5f266b285..7744aa142 100644 --- a/tests/app/notifications/test_process_letter_notifications.py +++ b/tests/app/notifications/test_process_letter_notifications.py @@ -2,7 +2,7 @@ from app.models import LETTER_TYPE from app.models import Notification from app.models import NOTIFICATION_CREATED from app.notifications.process_letter_notifications import create_letter_notification -from app.json_models import TemplateJSONModel +from app.serialised_models import SerialisedTemplate def test_create_letter_notification_creates_notification(sample_letter_template, sample_api_key): @@ -14,7 +14,7 @@ def test_create_letter_notification_creates_notification(sample_letter_template, } } - template = TemplateJSONModel.from_id_and_service_id( + template = SerialisedTemplate.from_id_and_service_id( sample_letter_template.id, sample_letter_template.service_id ) @@ -49,7 +49,7 @@ def test_create_letter_notification_sets_reference(sample_letter_template, sampl 'reference': 'foo' } - template = TemplateJSONModel.from_id_and_service_id( + template = SerialisedTemplate.from_id_and_service_id( sample_letter_template.id, sample_letter_template.service_id ) @@ -73,7 +73,7 @@ def test_create_letter_notification_sets_billable_units(sample_letter_template, }, } - template = TemplateJSONModel.from_id_and_service_id( + template = SerialisedTemplate.from_id_and_service_id( sample_letter_template.id, sample_letter_template.service_id ) diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 49c114c74..94521959a 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -20,14 +20,14 @@ from app.notifications.process_notifications import ( send_notification_to_queue, simulated_recipient ) -from app.serialised_models import TemplateJSONModel +from app.serialised_models import SerialisedTemplate from notifications_utils.recipients import validate_and_format_phone_number, validate_and_format_email_address from app.v2.errors import BadRequestError from tests.app.db import create_service, create_template, create_api_key def test_create_content_for_notification_passes(sample_email_template): - template = TemplateJSONModel.from_id_and_service_id( + template = SerialisedTemplate.from_id_and_service_id( sample_email_template.id, sample_email_template.service_id ) content = create_content_for_notification(template, None) @@ -35,7 +35,7 @@ def test_create_content_for_notification_passes(sample_email_template): def test_create_content_for_notification_with_placeholders_passes(sample_template_with_placeholders): - template = TemplateJSONModel.from_id_and_service_id( + template = SerialisedTemplate.from_id_and_service_id( sample_template_with_placeholders.id, sample_template_with_placeholders.service_id ) content = create_content_for_notification(template, {'name': 'Bobby'}) @@ -44,7 +44,7 @@ def test_create_content_for_notification_with_placeholders_passes(sample_templat def test_create_content_for_notification_fails_with_missing_personalisation(sample_template_with_placeholders): - template = TemplateJSONModel.from_id_and_service_id( + template = SerialisedTemplate.from_id_and_service_id( sample_template_with_placeholders.id, sample_template_with_placeholders.service_id ) with pytest.raises(BadRequestError): @@ -52,7 +52,7 @@ def test_create_content_for_notification_fails_with_missing_personalisation(samp def test_create_content_for_notification_allows_additional_personalisation(sample_template_with_placeholders): - template = TemplateJSONModel.from_id_and_service_id( + template = SerialisedTemplate.from_id_and_service_id( sample_template_with_placeholders.id, sample_template_with_placeholders.service_id ) create_content_for_notification(template, {'name': 'Bobby', 'Additional placeholder': 'Data'}) diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index e2e200f87..1c4db4190 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -23,7 +23,7 @@ from app.notifications.validators import ( validate_and_format_recipient, validate_template, ) -from app.serialised_models import TemplateJSONModel +from app.serialised_models import SerialisedTemplate from app.utils import get_template_instance from app.v2.errors import ( @@ -315,7 +315,7 @@ def test_check_content_char_count_passes_for_long_email_or_letter(sample_service def test_check_notification_content_is_not_empty_passes(notify_api, mocker, sample_service): template_id = create_template(sample_service, content="Content is not empty").id - template = TemplateJSONModel.from_id_and_service_id( + template = SerialisedTemplate.from_id_and_service_id( template_id=template_id, service_id=sample_service.id ) @@ -331,7 +331,7 @@ def test_check_notification_content_is_not_empty_fails( notify_api, mocker, sample_service, template_content, notification_values ): template_id = create_template(sample_service, content=template_content).id - template = TemplateJSONModel.from_id_and_service_id( + template = SerialisedTemplate.from_id_and_service_id( template_id=template_id, service_id=sample_service.id ) From 8d61c1ef4bf7b3c11ee0795c8ba7dccec31cbff0 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Jun 2020 10:20:54 +0100 Subject: [PATCH 5/6] Add description of SerialisedModel --- app/serialised_models.py | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/app/serialised_models.py b/app/serialised_models.py index d3b1cd5ca..f864171f7 100644 --- a/app/serialised_models.py +++ b/app/serialised_models.py @@ -5,6 +5,14 @@ from app.dao import templates_dao class SerialisedModel(ABC): + """ + A SerialisedModel takes a dictionary, typically created by + serialising a database object. It then takes the value of specified + keys from the dictionary and adds them to itself as properties, so + that it can be interacted with like a normal database model object, + but with no risk that it will actually go back to the database. + """ + @property @abstractmethod def ALLOWED_PROPERTIES(self): From 58a9862cd1392c0c3384ce37d4b8eeb51dd7da32 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Mon, 22 Jun 2020 14:03:59 +0100 Subject: [PATCH 6/6] Avoid extra query when serialising Template created_by MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit It’s a UUID column, but by default Marshmallow wants to select the id from the users table, not from the templates table, because the two are foreign-keyed. Adding the property explicity like this forces it to select from the `created_by_id` column, but still serialises it to the `created_by` field to avoid any breaking change. --- app/schemas.py | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index c985d1812..85956e0cf 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -343,7 +343,9 @@ class BaseTemplateSchema(BaseSchema): class TemplateSchema(BaseTemplateSchema): - created_by = field_for(models.Template, 'created_by', required=True) + created_by_id = field_for( + models.Template, 'created_by_id', dump_to='created_by', dump_only=True + ) process_type = field_for(models.Template, 'process_type') redact_personalisation = fields.Method("redact") @@ -357,6 +359,9 @@ class TemplateSchema(BaseTemplateSchema): if not subject or subject.strip() == '': raise ValidationError('Invalid template subject', 'subject') + class Meta(BaseTemplateSchema.Meta): + exclude = BaseTemplateSchema.Meta.exclude + ('created_by',) + class TemplateHistorySchema(BaseSchema):