From 5b7af002953acbc9df64036fc0a0cb541e3a155b Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 9 Aug 2016 17:28:55 +0100 Subject: [PATCH 1/3] re-add content_char_count to public GET /notification endpoints we shouldn't remove stuff from endpoints or we'll break clients --- app/schemas.py | 7 ++++++- tests/app/notifications/test_rest.py | 2 ++ 2 files changed, 8 insertions(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index 168faa02c..c7061a5bb 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -308,8 +308,13 @@ class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): renderer=PassThrough() ) in_data['body'] = template.replaced - if in_data['template']['template_type'] == 'email': + template_type = in_data['template']['template_type'] + if template_type == 'email': in_data['subject'] = template.replaced_subject + in_data['content_char_count'] = None + else: + in_data['content_char_count'] = len(in_data['body']) + in_data.pop('personalisation', None) in_data['template'].pop('content', None) in_data['template'].pop('subject', None) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 49813da1b..09fea3219 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -557,6 +557,7 @@ def test_get_notification_by_id_returns_merged_template_content(notify_db, assert response.status_code == 200 assert notification['body'] == 'Hello world\nYour thing is due soon' assert 'subject' not in notification + assert notification['content_char_count'] == 34 def test_get_notification_by_id_returns_merged_template_content_for_email( @@ -580,6 +581,7 @@ def test_get_notification_by_id_returns_merged_template_content_for_email( assert response.status_code == 200 assert notification['body'] == 'Hello world\nThis is an email from GOV.UK' assert notification['subject'] == 'world' + assert notification['content_char_count'] is None def test_get_notifications_for_service_returns_merged_template_content(notify_api, From f184bb7996a623b26a7c3f7a089ac6591753d0ad Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 9 Aug 2016 17:31:38 +0100 Subject: [PATCH 2/3] add test for notification status return value to ensure we never break compatibility --- tests/app/notifications/test_rest.py | 38 ++++++++++++++++++++++++++++ 1 file changed, 38 insertions(+) diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 09fea3219..cbbaf5ca2 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -618,6 +618,44 @@ def test_get_notifications_for_service_returns_merged_template_content(notify_ap } +def test_get_notification_public_api_format_is_not_changed(notify_api, sample_notification): + with notify_api.test_request_context(), notify_api.test_client() as client: + auth_header = create_authorization_header(service_id=sample_notification.service_id) + + response = client.get( + '/notifications/{}'.format(sample_notification.id), + headers=[auth_header]) + + assert response.status_code == 200 + notification = json.loads(response.get_data(as_text=True))['data']['notification'] + # you should never remove things from this list! + assert set(notification.keys()) == { + # straight from db + 'id', + 'to', + 'job_row_number', + 'template_version', + 'billable_units', + 'notification_type', + 'created_at', + 'sent_at', + 'sent_by', + 'updated_at', + 'status', + 'reference', + + # relationships + 'template', + 'service', + 'job', + 'api_key', + + # other + 'body', + 'content_char_count' + } + + @pytest.mark.xfail(strict=True, raises=NeededByTemplateError) def test_get_notification_selects_correct_template_for_personalisation(notify_api, notify_db, From 52b2f4f58367ba465425dde0b4ac2d834a8bc001 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Wed, 10 Aug 2016 10:59:22 +0100 Subject: [PATCH 3/3] require fields in marshmallow schema to try and make it as hard to break backwards compatability as possible --- app/schemas.py | 15 ++++++++++++++- 1 file changed, 14 insertions(+), 1 deletion(-) diff --git a/app/schemas.py b/app/schemas.py index c7061a5bb..93171dec7 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -294,6 +294,20 @@ class NotificationWithTemplateSchema(BaseSchema): class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): + class Meta(NotificationWithTemplateSchema.Meta): + # mark as many fields as possible as required since this is a public api. + # WARNING: Does _not_ reference fields computed in handle_template_merge, such as + # 'body', 'subject' [for emails], and 'content_char_count' + fields = ( + # db rows + 'id', 'to', 'job_row_number', 'template_version', 'billable_units', 'notification_type', 'created_at', + 'sent_at', 'sent_by', 'updated_at', 'status', 'reference', + # computed fields + 'personalisation', + # relationships + 'service', 'job', 'api_key', 'template' + ) + @pre_dump def handle_personalisation_property(self, in_data): self.personalisation = in_data.personalisation @@ -533,7 +547,6 @@ job_email_template_notification_schema = JobEmailTemplateNotificationSchema() notification_schema = NotificationModelSchema() notification_with_template_schema = NotificationWithTemplateSchema() notification_with_personalisation_schema = NotificationWithPersonalisationSchema() -notification_with_personalisation_schema_load_json = NotificationWithPersonalisationSchema(load_json=True) invited_user_schema = InvitedUserSchema() permission_schema = PermissionSchema() email_data_request_schema = EmailDataSchema()