From e507fed15299b716f01fffa7dcda5396265e097b Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Tue, 7 Mar 2017 16:03:10 +0000 Subject: [PATCH] Quietly ignore extra personalisation MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit > If a user makes an API request with additional personalisation fields, > we should simply discard any fields that the template doesn't have. > > This gives a couple of related advantages: > > - modifying template parameters no longer requires downtime for > clients - as they can pass in extra new parameters before a template > change, or continue passing in old unused parameters after removing > them from a template > > - services can pass in large user objects, for example, and then play > around with templates adding and removing fields at will > > we should make sure we still return an error if a user doesn't pass in > a required parameter. – https://www.pivotaltracker.com/story/show/140774195 --- app/notifications/process_notifications.py | 5 ----- app/notifications/rest.py | 5 ----- app/template/rest.py | 7 ------- tests/app/notifications/rest/test_send_notification.py | 6 ++---- tests/app/notifications/test_process_notification.py | 6 ++---- tests/app/template/test_rest.py | 5 +++-- 6 files changed, 7 insertions(+), 27 deletions(-) diff --git a/app/notifications/process_notifications.py b/app/notifications/process_notifications.py index 91ada9110..d049871d0 100644 --- a/app/notifications/process_notifications.py +++ b/app/notifications/process_notifications.py @@ -23,11 +23,6 @@ def check_placeholders(template_object): message = 'Template missing personalisation: {}'.format(", ".join(template_object.missing_data)) raise BadRequestError(fields=[{'template': message}], message=message) - if template_object.additional_data: - message = 'Template personalisation not needed for template: {}'.format( - ", ".join(template_object.additional_data)) - raise BadRequestError(fields=[{'template': message}], message=message) - def persist_notification(template_id, template_version, diff --git a/app/notifications/rest.py b/app/notifications/rest.py index 4f3282cb9..aa9994358 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -300,11 +300,6 @@ def create_template_object_for_notification(template, personalisation): errors = {'template': [message]} raise InvalidRequest(errors, status_code=400) - if template_object.additional_data: - message = 'Personalisation not needed for template: {}'.format(", ".join(template_object.additional_data)) - errors = {'template': [message]} - raise InvalidRequest(errors, status_code=400) - if ( template_object.template_type == SMS_TYPE and template_object.content_count > current_app.config.get('SMS_CHAR_COUNT_LIMIT') diff --git a/app/template/rest.py b/app/template/rest.py index 17d48d797..df0e61252 100644 --- a/app/template/rest.py +++ b/app/template/rest.py @@ -101,13 +101,6 @@ def preview_template_by_id_and_service_id(service_id, template_id): ]}, status_code=400 ) - if template_object.additional_data: - raise InvalidRequest( - {'template': [ - 'Personalisation not needed for template: {}'.format(", ".join(template_object.additional_data)) - ]}, status_code=400 - ) - data['subject'], data['content'] = template_object.subject, str(template_object) return jsonify(data) diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index f5efa1cea..be2a139ac 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -913,14 +913,12 @@ def test_create_template_raises_invalid_request_exception_with_missing_personali assert {'template': ['Missing personalisation: Name']} == e.value.message -def test_create_template_raises_invalid_request_exception_with_too_much_personalisation_data( +def test_create_template_doesnt_raise_with_too_much_personalisation( sample_template_with_placeholders ): from app.notifications.rest import create_template_object_for_notification template = Template.query.get(sample_template_with_placeholders.id) - with pytest.raises(InvalidRequest) as e: - create_template_object_for_notification(template, {'name': 'Jo', 'extra': 'stuff'}) - assert {'template': ['Personalisation not needed for template: foo']} in e.value.message + create_template_object_for_notification(template, {'name': 'Jo', 'extra': 'stuff'}) @pytest.mark.parametrize( diff --git a/tests/app/notifications/test_process_notification.py b/tests/app/notifications/test_process_notification.py index 89557ff94..477c45306 100644 --- a/tests/app/notifications/test_process_notification.py +++ b/tests/app/notifications/test_process_notification.py @@ -36,11 +36,9 @@ def test_create_content_for_notification_fails_with_missing_personalisation(samp create_content_for_notification(template, None) -def test_create_content_for_notification_fails_with_additional_personalisation(sample_template_with_placeholders): +def test_create_content_for_notification_allows_additional_personalisation(sample_template_with_placeholders): template = Template.query.get(sample_template_with_placeholders.id) - with pytest.raises(BadRequestError) as e: - create_content_for_notification(template, {'name': 'Bobby', 'Additional placeholder': 'Data'}) - assert e.value.message == 'Template personalisation not needed for template: Additional placeholder' + create_content_for_notification(template, {'name': 'Bobby', 'Additional placeholder': 'Data'}) @freeze_time("2016-01-01 11:09:00.061258") diff --git a/tests/app/template/test_rest.py b/tests/app/template/test_rest.py index aa00d1c1c..13e21dcfd 100644 --- a/tests/app/template/test_rest.py +++ b/tests/app/template/test_rest.py @@ -374,8 +374,9 @@ def test_should_get_a_single_template( 'about your ((thing))', 'hello ((name)) we’ve received your ((thing))', '/service/{}/template/{}/preview?name=Amala&thing=document&foo=bar', - None, None, - 'Personalisation not needed for template: foo' + 'about your document', + 'hello Amala we’ve received your document', + None, ) ] )