From b28e7efd14b0243ba7c02b6b8172c0a5d5275697 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Jul 2016 12:34:39 +0100 Subject: [PATCH 1/3] dont load template contents for job page rename the notification_status_schema to make it apparent that it involves the template, and then don't use it on the job page - the job page doesn't do anything with the data. won't somebody think of the cpu cycles! (also means it ignores problems with template versions) --- app/job/rest.py | 9 +++++++-- app/notifications/rest.py | 6 +++--- app/schemas.py | 9 +++++---- app/service/rest.py | 4 ++-- app/spec/rest.py | 6 +++--- tests/app/job/test_rest.py | 3 +++ 6 files changed, 23 insertions(+), 14 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 8f6512404..1a1f512f3 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -18,7 +18,12 @@ from app.dao.services_dao import ( from app.dao.templates_dao import (dao_get_template_by_id) from app.dao.notifications_dao import get_notifications_for_job -from app.schemas import job_schema, unarchived_template_schema, notifications_filter_schema, notification_status_schema +from app.schemas import ( + job_schema, + unarchived_template_schema, + notifications_filter_schema, + notification_schema +) from app.celery.tasks import process_job @@ -57,7 +62,7 @@ def get_all_notifications_for_service_job(service_id, job_id): kwargs['service_id'] = service_id kwargs['job_id'] = job_id return jsonify( - notifications=notification_status_schema.dump(pagination.items, many=True).data, + notifications=notification_schema.dump(pagination.items, many=True).data, page_size=page_size, total=pagination.total, links=pagination_links( diff --git a/app/notifications/rest.py b/app/notifications/rest.py index bffc59b98..6ec85e463 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -26,7 +26,7 @@ from app.notifications.process_client_response import ( from app.schemas import ( email_notification_schema, sms_template_notification_schema, - notification_status_schema, + notification_with_template_schema, notifications_filter_schema, notifications_statistics_schema, day_schema, @@ -175,7 +175,7 @@ def get_notifications(notification_id): notification = notifications_dao.get_notification(str(api_user.service_id), notification_id, key_type=api_user.key_type) - return jsonify(data={"notification": notification_status_schema.dump(notification).data}), 200 + return jsonify(data={"notification": notification_with_template_schema.dump(notification).data}), 200 @notifications.route('/notifications', methods=['GET']) @@ -193,7 +193,7 @@ def get_all_notifications(): limit_days=limit_days, key_type=api_user.key_type) return jsonify( - notifications=notification_status_schema.dump(pagination.items, many=True).data, + notifications=notification_with_template_schema.dump(pagination.items, many=True).data, page_size=page_size, total=pagination.total, links=pagination_links( diff --git a/app/schemas.py b/app/schemas.py index 7857a728e..baeceac3d 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -144,7 +144,7 @@ class NotificationModelSchema(BaseSchema): class Meta: model = models.Notification strict = True - exclude = ("_personalisation",) + exclude = ('_personalisation', 'job', 'service', 'template', 'api_key', '') class BaseTemplateSchema(BaseSchema): @@ -280,7 +280,7 @@ class SmsAdminNotificationSchema(SmsNotificationSchema): content = fields.Str(required=True) -class NotificationStatusSchema(BaseSchema): +class NotificationWithTemplateSchema(BaseSchema): template = fields.Nested(TemplateSchema, only=["id", "name", "template_type", "content", "subject"], dump_only=True) job = fields.Nested(JobSchema, only=["id", "original_file_name"], dump_only=True) @@ -516,8 +516,9 @@ sms_template_notification_schema = SmsTemplateNotificationSchema() job_sms_template_notification_schema = JobSmsTemplateNotificationSchema() email_notification_schema = EmailNotificationSchema() job_email_template_notification_schema = JobEmailTemplateNotificationSchema() -notification_status_schema = NotificationStatusSchema() -notification_status_schema_load_json = NotificationStatusSchema(load_json=True) +notification_schema = NotificationModelSchema() +notification_with_template_schema = NotificationWithTemplateSchema() +notification_with_template_schema_load_json = NotificationWithTemplateSchema(load_json=True) invited_user_schema = InvitedUserSchema() permission_schema = PermissionSchema() email_data_request_schema = EmailDataSchema() diff --git a/app/service/rest.py b/app/service/rest.py index 617ccb84d..b6b3e461b 100644 --- a/app/service/rest.py +++ b/app/service/rest.py @@ -33,7 +33,7 @@ from app.schemas import ( user_schema, from_to_date_schema, permission_schema, - notification_status_schema, + notification_with_template_schema, notifications_filter_schema, detailed_service_schema ) @@ -224,7 +224,7 @@ def get_all_notifications_for_service(service_id): kwargs = request.args.to_dict() kwargs['service_id'] = service_id return jsonify( - notifications=notification_status_schema.dump(pagination.items, many=True).data, + notifications=notification_with_template_schema.dump(pagination.items, many=True).data, page_size=page_size, total=pagination.total, links=pagination_links( diff --git a/app/spec/rest.py b/app/spec/rest.py index 400cb5849..9cd36c381 100644 --- a/app/spec/rest.py +++ b/app/spec/rest.py @@ -10,7 +10,7 @@ api_spec = APISpec( version='0.0.0' ) -api_spec.definition('NotificationStatusSchema', properties={ +api_spec.definition('NotificationWithTemplateSchema', properties={ "content_char_count": { "format": "int32", "type": "integer" @@ -106,14 +106,14 @@ api_spec.definition('NotificationStatusSchema', properties={ }) api_spec.definition('NotificationSchema', properties={ "notification": { - "$ref": "#/definitions/NotificationStatusSchema" + "$ref": "#/definitions/NotificationWithTemplateSchema" } }) api_spec.definition('NotificationsSchema', properties={ "notifications": { "type": "array", "items": { - "$ref": "#/definitions/NotificationStatusSchema" + "$ref": "#/definitions/NotificationWithTemplateSchema" } } }) diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index ea65bb447..c7a056f77 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -296,6 +296,9 @@ def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, assert resp['notifications'][2]['job_row_number'] == notification_3.job_row_number assert response.status_code == 200 + # make sure we're not loading templates + assert 'template' not in resp['notifications'][0] + @pytest.mark.parametrize( "expected_notification_count, status_args", From c81b30dba11b684fdad48e1f12e36be3f31403fa Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Jul 2016 14:33:14 +0100 Subject: [PATCH 2/3] separated schemas once more into "with template" and "with personalisation" "with personalisation" should only be used by the public notification api "with template" should be used when we want template name, etc details. also added an xfail test for correctly constructing notification personalisation --- app/job/rest.py | 4 ++-- app/notifications/rest.py | 8 +++---- app/schemas.py | 14 ++++++----- tests/app/notifications/test_rest.py | 36 +++++++++++++++++++++++++++- 4 files changed, 49 insertions(+), 13 deletions(-) diff --git a/app/job/rest.py b/app/job/rest.py index 1a1f512f3..c40172a85 100644 --- a/app/job/rest.py +++ b/app/job/rest.py @@ -22,7 +22,7 @@ from app.schemas import ( job_schema, unarchived_template_schema, notifications_filter_schema, - notification_schema + notification_with_template_schema ) from app.celery.tasks import process_job @@ -62,7 +62,7 @@ def get_all_notifications_for_service_job(service_id, job_id): kwargs['service_id'] = service_id kwargs['job_id'] = job_id return jsonify( - notifications=notification_schema.dump(pagination.items, many=True).data, + notifications=notification_with_template_schema.dump(pagination.items, many=True).data, page_size=page_size, total=pagination.total, links=pagination_links( diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 6ec85e463..897dbc456 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -18,7 +18,7 @@ from app.dao import ( services_dao, notifications_dao ) -from app.models import NOTIFICATION_TYPE, SMS_TYPE +from app.models import SMS_TYPE from app.notifications.process_client_response import ( validate_callback_data, process_sms_client_response @@ -26,7 +26,7 @@ from app.notifications.process_client_response import ( from app.schemas import ( email_notification_schema, sms_template_notification_schema, - notification_with_template_schema, + notification_with_personalisation_schema, notifications_filter_schema, notifications_statistics_schema, day_schema, @@ -175,7 +175,7 @@ def get_notifications(notification_id): notification = notifications_dao.get_notification(str(api_user.service_id), notification_id, key_type=api_user.key_type) - return jsonify(data={"notification": notification_with_template_schema.dump(notification).data}), 200 + return jsonify(data={"notification": notification_with_personalisation_schema.dump(notification).data}), 200 @notifications.route('/notifications', methods=['GET']) @@ -193,7 +193,7 @@ def get_all_notifications(): limit_days=limit_days, key_type=api_user.key_type) return jsonify( - notifications=notification_with_template_schema.dump(pagination.items, many=True).data, + notifications=notification_with_personalisation_schema.dump(pagination.items, many=True).data, page_size=page_size, total=pagination.total, links=pagination_links( diff --git a/app/schemas.py b/app/schemas.py index baeceac3d..659ea996f 100644 --- a/app/schemas.py +++ b/app/schemas.py @@ -281,16 +281,17 @@ class SmsAdminNotificationSchema(SmsNotificationSchema): class NotificationWithTemplateSchema(BaseSchema): - - template = fields.Nested(TemplateSchema, only=["id", "name", "template_type", "content", "subject"], dump_only=True) - job = fields.Nested(JobSchema, only=["id", "original_file_name"], dump_only=True) - personalisation = fields.Dict(required=False) - class Meta: model = models.Notification strict = True exclude = ('_personalisation',) + template = fields.Nested(TemplateSchema, only=["id", "name", "template_type", "content", "subject"], dump_only=True) + job = fields.Nested(JobSchema, only=["id", "original_file_name"], dump_only=True) + personalisation = fields.Dict(required=False) + + +class NotificationWithPersonalisationSchema(NotificationWithTemplateSchema): @pre_dump def handle_personalisation_property(self, in_data): self.personalisation = in_data.personalisation @@ -518,7 +519,8 @@ email_notification_schema = EmailNotificationSchema() job_email_template_notification_schema = JobEmailTemplateNotificationSchema() notification_schema = NotificationModelSchema() notification_with_template_schema = NotificationWithTemplateSchema() -notification_with_template_schema_load_json = NotificationWithTemplateSchema(load_json=True) +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() diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 981def879..4e43282ab 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -11,7 +11,7 @@ from app.dao.api_key_dao import save_model_api_key from app.models import ApiKey, KEY_TYPE_NORMAL, KEY_TYPE_TEAM, KEY_TYPE_TEST from tests import create_authorization_header from tests.app.conftest import sample_notification as create_sample_notification - +from notifications_utils.template import NeededByTemplateError def test_get_sms_notification_by_id(notify_api, sample_notification): with notify_api.test_request_context(): @@ -615,6 +615,40 @@ def test_get_notifications_for_service_returns_merged_template_content(notify_ap } +@pytest.mark.xfail(strict=True, raises=NeededByTemplateError) +def test_get_notification_selects_correct_template_for_personalisation(notify_api, + notify_db, + notify_db_session, + sample_template): + + create_sample_notification(notify_db, + notify_db_session, + service=sample_template.service, + template=sample_template) + + sample_template.content = '((name))' + notify_db.session.commit() + + create_sample_notification(notify_db, + notify_db_session, + service=sample_template.service, + template=sample_template, + personalisation={"name": "foo"}) + + with notify_api.test_request_context(), notify_api.test_client() as client: + auth_header = create_authorization_header(service_id=sample_template.service_id) + + response = client.get(path='/notifications', headers=[auth_header]) + assert response.status_code == 200 + + resp = json.loads(response.get_data(as_text=True)) + assert len(resp['notifications']) == 2 + assert resp['notifications'][0]['template_version'] == 1 + assert resp['notifications'][0]['body'] == 'This is a template' + assert resp['notifications'][1]['template_version'] == 2 + assert resp['notifications'][1]['body'] == 'foo' + + def _create_auth_header_from_key(api_key): token = create_jwt_token(secret=api_key.unsigned_secret, client_id=str(api_key.service_id)) return [('Authorization', 'Bearer {}'.format(token))] From 91393bdb0d38bef45507459e50894c5326572519 Mon Sep 17 00:00:00 2001 From: Leo Hemsted Date: Tue, 26 Jul 2016 14:49:51 +0100 Subject: [PATCH 3/3] ensure /notifications rather than service/:id/notifications in tests for content merging --- tests/app/job/test_rest.py | 3 --- tests/app/notifications/test_rest.py | 5 +++-- 2 files changed, 3 insertions(+), 5 deletions(-) diff --git a/tests/app/job/test_rest.py b/tests/app/job/test_rest.py index c7a056f77..ea65bb447 100644 --- a/tests/app/job/test_rest.py +++ b/tests/app/job/test_rest.py @@ -296,9 +296,6 @@ def test_get_all_notifications_for_job_in_order_of_job_number(notify_api, assert resp['notifications'][2]['job_row_number'] == notification_3.job_row_number assert response.status_code == 200 - # make sure we're not loading templates - assert 'template' not in resp['notifications'][0] - @pytest.mark.parametrize( "expected_notification_count, status_args", diff --git a/tests/app/notifications/test_rest.py b/tests/app/notifications/test_rest.py index 4e43282ab..49813da1b 100644 --- a/tests/app/notifications/test_rest.py +++ b/tests/app/notifications/test_rest.py @@ -13,6 +13,7 @@ from tests import create_authorization_header from tests.app.conftest import sample_notification as create_sample_notification from notifications_utils.template import NeededByTemplateError + def test_get_sms_notification_by_id(notify_api, sample_notification): with notify_api.test_request_context(): with notify_api.test_client() as client: @@ -602,10 +603,10 @@ def test_get_notifications_for_service_returns_merged_template_content(notify_ap with notify_api.test_request_context(): with notify_api.test_client() as client: - auth_header = create_authorization_header() + auth_header = create_authorization_header(service_id=sample_template_with_placeholders.service_id) response = client.get( - path='/service/{}/notifications'.format(sample_template_with_placeholders.service.id), + path='/notifications', headers=[auth_header]) assert response.status_code == 200