From 5bacfc1df9c7dd34590fe2c60c6cd3fd87d146ad Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 4 Nov 2020 14:25:22 +0000 Subject: [PATCH 1/3] Change how we validate the length of templates. We want to add validation for an email that's too long, that way the user knows why the message is failing. At the moment if an email is too long it will get a technical failure, after the retries fail. This way the email post will get a validation error. Once this: https://github.com/alphagov/notifications-utils/pull/804 is reverted, we can update the utils version. --- app/notifications/validators.py | 20 +++++++++++--------- app/v2/notifications/post_notifications.py | 6 +++++- tests/app/notifications/test_validators.py | 19 +++++++++++++++++++ 3 files changed, 35 insertions(+), 10 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 91747d14d..f51921ab7 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -27,7 +27,6 @@ from app.serialised_models import SerialisedTemplate from gds_metrics.metrics import Histogram - REDIS_EXCEEDED_RATE_LIMIT_DURATION_SECONDS = Histogram( 'redis_exceeded_rate_limit_duration_seconds', 'Time taken to check rate limit', @@ -106,7 +105,7 @@ def check_if_service_can_send_files_by_email(service_contact_link, service_id): if not service_contact_link: raise BadRequestError( message=f"Send files by email has not been set up - add contact details for your service at " - f"{current_app.config['ADMIN_BASE_URL']}/services/{service_id}/service-settings/send-files-by-email" + f"{current_app.config['ADMIN_BASE_URL']}/services/{service_id}/service-settings/send-files-by-email" ) @@ -147,7 +146,7 @@ def check_if_service_can_send_to_number(service, number): def check_content_char_count(template_with_content): if template_with_content.is_message_too_long(): message = f"Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. " \ - f"Your message is {template_with_content.content_count_without_prefix} characters" + f"Your message is {template_with_content.content_count_without_prefix} characters" raise BadRequestError(message=message) @@ -157,8 +156,7 @@ def check_notification_content_is_not_empty(template_with_content): raise BadRequestError(message=message) -def validate_template(template_id, personalisation, service, notification_type): - +def validate_template(template_id, personalisation, service, notification_type, check_char_count=True): try: template = SerialisedTemplate.from_id_and_service_id(template_id, service.id) except NoResultFound: @@ -173,7 +171,11 @@ def validate_template(template_id, personalisation, service, notification_type): check_notification_content_is_not_empty(template_with_content) - check_content_char_count(template_with_content) + # validating the template in post_notifications happens before the file is uploaded for doc download, + # which means the length of the message can be exceeded because it's including the file. + # The document download feature is only available through the api. + if check_char_count: + check_content_char_count(template_with_content) return template, template_with_content @@ -192,7 +194,7 @@ def check_service_email_reply_to_id(service_id, reply_to_id, notification_type): try: return dao_get_reply_to_by_id(service_id, reply_to_id).email_address except NoResultFound: - message = 'email_reply_to_id {} does not exist in database for service id {}'\ + message = 'email_reply_to_id {} does not exist in database for service id {}' \ .format(reply_to_id, service_id) raise BadRequestError(message=message) @@ -202,7 +204,7 @@ def check_service_sms_sender_id(service_id, sms_sender_id, notification_type): try: return dao_get_service_sms_senders_by_id(service_id, sms_sender_id).sms_sender except NoResultFound: - message = 'sms_sender_id {} does not exist in database for service id {}'\ + message = 'sms_sender_id {} does not exist in database for service id {}' \ .format(sms_sender_id, service_id) raise BadRequestError(message=message) @@ -212,7 +214,7 @@ def check_service_letter_contact_id(service_id, letter_contact_id, notification_ try: return dao_get_letter_contact_by_id(service_id, letter_contact_id).contact_block except NoResultFound: - message = 'letter_contact_id {} does not exist in database for service id {}'\ + message = 'letter_contact_id {} does not exist in database for service id {}' \ .format(letter_contact_id, service_id) raise BadRequestError(message=message) diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index 97f6dfeac..cb0d23864 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -52,7 +52,7 @@ from app.notifications.validators import ( validate_address, validate_and_format_recipient, validate_template, -) + check_content_char_count) from app.schema_validation import validate from app.v2.errors import BadRequestError from app.v2.notifications.create_response import ( @@ -132,6 +132,7 @@ def post_notification(notification_type): form.get('personalisation', {}), authenticated_service, notification_type, + check_char_count=False ) reply_to = get_reply_to_text(notification_type, form, template) @@ -189,6 +190,9 @@ def process_sms_or_email_notification( # We changed personalisation which means we need to update the content template_with_content.values = personalisation + # validate content length after url is replaced in personalisation. + check_content_char_count(template_with_content) + resp = create_response_for_post_notification( notification_id=notification_id, client_reference=form.get('reference', None), diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index de6ce99ce..a7be7e5e0 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -404,6 +404,25 @@ def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_servic mock_check_message_is_too_long.assert_called_once_with("content") +def test_validate_template_calls_all_validators_exception_message_too_long(mocker, fake_uuid, sample_service): + template = create_template(sample_service, template_type="email") + mock_check_type = mocker.patch('app.notifications.validators.check_template_is_for_notification_type') + mock_check_if_active = mocker.patch('app.notifications.validators.check_template_is_active') + mock_create_conent = mocker.patch( + 'app.notifications.validators.create_content_for_notification', return_value="content" + ) + 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') + template, template_with_content = validate_template(template.id, {}, sample_service, "email", + check_char_count=False) + + mock_check_type.assert_called_once_with("email", "email") + mock_check_if_active.assert_called_once_with(template) + mock_create_conent.assert_called_once_with(template, {}) + mock_check_not_empty.assert_called_once_with("content") + assert not mock_check_message_is_too_long.called + + @pytest.mark.parametrize('key_type', ['team', 'live', 'test']) def test_that_when_exceed_rate_limit_request_fails( key_type, From 171bc74c69cc5b06cf93cf2759751df950aaca46 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 9 Nov 2020 15:19:00 +0000 Subject: [PATCH 2/3] Rename check_character_count method to check_is_message_to_long. Add different error message for email and text if content is too long. Use utils version with is_message_too_long method implemented for email templates. --- app/notifications/validators.py | 17 +++++-- app/v2/notifications/post_notifications.py | 4 +- requirements-app.txt | 2 +- requirements.txt | 2 +- tests/app/notifications/test_validators.py | 48 ++++++++++++------- .../service/test_send_one_off_notification.py | 5 +- 6 files changed, 52 insertions(+), 26 deletions(-) diff --git a/app/notifications/validators.py b/app/notifications/validators.py index f51921ab7..22277b5fc 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -143,10 +143,19 @@ def check_if_service_can_send_to_number(service, number): return international_phone_info -def check_content_char_count(template_with_content): +def check_is_message_too_long(template_with_content): if template_with_content.is_message_too_long(): - message = f"Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. " \ - f"Your message is {template_with_content.content_count_without_prefix} characters" + message = "Your message is too long. " + if template_with_content.template_type == SMS_TYPE: + message += ( + f"Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. " + f"Your message is {template_with_content.content_count_without_prefix} characters long." + ) + elif template_with_content.template_type == EMAIL_TYPE: + message += ( + f"Emails cannot be longer than 1000000 bytes. " + f"Your message is {template_with_content.content_size_in_bytes} bytes." + ) raise BadRequestError(message=message) @@ -175,7 +184,7 @@ def validate_template(template_id, personalisation, service, notification_type, # which means the length of the message can be exceeded because it's including the file. # The document download feature is only available through the api. if check_char_count: - check_content_char_count(template_with_content) + check_is_message_too_long(template_with_content) return template, template_with_content diff --git a/app/v2/notifications/post_notifications.py b/app/v2/notifications/post_notifications.py index cb0d23864..1c92615a0 100644 --- a/app/v2/notifications/post_notifications.py +++ b/app/v2/notifications/post_notifications.py @@ -52,7 +52,7 @@ from app.notifications.validators import ( validate_address, validate_and_format_recipient, validate_template, - check_content_char_count) + check_is_message_too_long) from app.schema_validation import validate from app.v2.errors import BadRequestError from app.v2.notifications.create_response import ( @@ -191,7 +191,7 @@ def process_sms_or_email_notification( template_with_content.values = personalisation # validate content length after url is replaced in personalisation. - check_content_char_count(template_with_content) + check_is_message_too_long(template_with_content) resp = create_response_for_post_notification( notification_id=notification_id, diff --git a/requirements-app.txt b/requirements-app.txt index 1d76201f8..e5716713f 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -29,7 +29,7 @@ notifications-python-client==5.7.0 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@43.0.0#egg=notifications-utils==43.0.0 +git+https://github.com/alphagov/notifications-utils.git@43.3.0#egg=notifications-utils==43.3.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.8.0 diff --git a/requirements.txt b/requirements.txt index 4296eab2f..a5c4c9358 100644 --- a/requirements.txt +++ b/requirements.txt @@ -31,7 +31,7 @@ notifications-python-client==5.7.0 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@43.0.0#egg=notifications-utils==43.0.0 +git+https://github.com/alphagov/notifications-utils.git@43.3.0#egg=notifications-utils==43.3.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.8.0 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index a7be7e5e0..a2be48875 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -13,7 +13,7 @@ from app.models import ( ) from app.notifications.process_notifications import create_content_for_notification from app.notifications.validators import ( - check_content_char_count, + check_is_message_too_long, check_if_service_can_send_files_by_email, check_notification_content_is_not_empty, check_service_over_daily_message_limit, @@ -319,36 +319,46 @@ def test_service_can_send_to_recipient_fails_when_mobile_number_is_not_on_team(s @pytest.mark.parametrize('char_count', [612, 0, 494, 200, 918]) @pytest.mark.parametrize('show_prefix', [True, False]) @pytest.mark.parametrize('template_type', ['sms', 'email', 'letter']) -def test_check_content_char_count_passes(notify_db_session, show_prefix, char_count, template_type): +def test_check_is_message_too_long_passes(notify_db_session, show_prefix, char_count, template_type): service = create_service(prefix_sms=show_prefix) t = create_template(service=service, content='a' * char_count, template_type=template_type) template = templates_dao.dao_get_template_by_id_and_service_id(template_id=t.id, service_id=service.id) template_with_content = get_template_instance(template=template.__dict__, values={}) - assert check_content_char_count(template_with_content) is None + assert check_is_message_too_long(template_with_content) is None @pytest.mark.parametrize('char_count', [919, 6000]) @pytest.mark.parametrize('show_prefix', [True, False]) -def test_check_content_char_count_fails(notify_db_session, show_prefix, char_count): +def test_check_is_message_too_long_fails(notify_db_session, show_prefix, char_count): with pytest.raises(BadRequestError) as e: service = create_service(prefix_sms=show_prefix) t = create_template(service=service, content='a' * char_count, template_type='sms') template = templates_dao.dao_get_template_by_id_and_service_id(template_id=t.id, service_id=service.id) template_with_content = get_template_instance(template=template.__dict__, values={}) - check_content_char_count(template_with_content) + check_is_message_too_long(template_with_content) assert e.value.status_code == 400 - assert e.value.message == f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \ - f'Your message is {char_count} characters' + expected_message = f'Your message is too long. '\ + f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. '\ + f'Your message is {char_count} characters long.' + assert e.value.message == expected_message assert e.value.fields == [] -@pytest.mark.parametrize('template_type', ['email', 'letter']) -def test_check_content_char_count_passes_for_long_email_or_letter(sample_service, template_type): - t = create_template(service=sample_service, content='a' * 1000, template_type=template_type) +def test_check_is_message_too_long_passes_for_long_email(sample_service): + email_character_count = 1000003 + t = create_template(service=sample_service, content='a' * email_character_count, template_type='email') template = templates_dao.dao_get_template_by_id_and_service_id(template_id=t.id, service_id=t.service_id) template_with_content = get_template_instance(template=template.__dict__, values={}) - assert check_content_char_count(template_with_content) is None + template_with_content.values + with pytest.raises(BadRequestError) as e: + check_is_message_too_long(template_with_content) + assert e.value.status_code == 400 + expected_message = f'Your message is too long. ' \ + f'Emails cannot be longer than 1000000 bytes. ' \ + f'Your message is 1000084 bytes.' + assert e.value.message == expected_message + assert e.value.fields == [] def test_check_notification_content_is_not_empty_passes(notify_api, mocker, sample_service): @@ -386,7 +396,8 @@ def test_validate_template(sample_service): validate_template(template.id, {}, sample_service, "email") -def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_service): +@pytest.mark.parametrize("check_char_count", [True, False]) +def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_service, check_char_count): template = create_template(sample_service, template_type="email") mock_check_type = mocker.patch('app.notifications.validators.check_template_is_for_notification_type') mock_check_if_active = mocker.patch('app.notifications.validators.check_template_is_active') @@ -394,14 +405,19 @@ def test_validate_template_calls_all_validators(mocker, fake_uuid, sample_servic 'app.notifications.validators.create_content_for_notification', return_value="content" ) 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') - template, template_with_content = validate_template(template.id, {}, sample_service, "email") + mock_check_message_is_too_long = mocker.patch('app.notifications.validators.check_is_message_too_long') + template, template_with_content = validate_template(template.id, {}, sample_service, "email", + check_char_count=check_char_count + ) mock_check_type.assert_called_once_with("email", "email") mock_check_if_active.assert_called_once_with(template) mock_create_conent.assert_called_once_with(template, {}) mock_check_not_empty.assert_called_once_with("content") - mock_check_message_is_too_long.assert_called_once_with("content") + if check_char_count: + mock_check_message_is_too_long.assert_called_once_with("content") + else: + assert not mock_check_message_is_too_long.called def test_validate_template_calls_all_validators_exception_message_too_long(mocker, fake_uuid, sample_service): @@ -412,7 +428,7 @@ def test_validate_template_calls_all_validators_exception_message_too_long(mocke 'app.notifications.validators.create_content_for_notification', return_value="content" ) 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') + mock_check_message_is_too_long = mocker.patch('app.notifications.validators.check_is_message_too_long') template, template_with_content = validate_template(template.id, {}, sample_service, "email", check_char_count=False) diff --git a/tests/app/service/test_send_one_off_notification.py b/tests/app/service/test_send_one_off_notification.py index 2f6e1ee60..b10b21ac4 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -317,8 +317,9 @@ def test_send_one_off_notification_raises_if_message_too_long(persist_mock, noti with pytest.raises(BadRequestError) as e: send_one_off_notification(service.id, post_data) - assert e.value.message == f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \ - f'Your message is {1029} characters' + assert e.value.message == f'Your message is too long. ' \ + f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \ + f'Your message is {1029} characters long.' def test_send_one_off_notification_fails_if_created_by_other_service(sample_template): From 2e114b7404fa44f5ce7da7e1b4401c5befb60ad1 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Mon, 16 Nov 2020 14:04:37 +0000 Subject: [PATCH 3/3] Bump utils requirement --- requirements-app.txt | 2 +- requirements.txt | 12 ++++++------ 2 files changed, 7 insertions(+), 7 deletions(-) diff --git a/requirements-app.txt b/requirements-app.txt index e5716713f..a3f059a56 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -29,7 +29,7 @@ notifications-python-client==5.7.0 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@43.3.0#egg=notifications-utils==43.3.0 +git+https://github.com/alphagov/notifications-utils.git@43.4.0#egg=notifications-utils==43.4.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.8.0 diff --git a/requirements.txt b/requirements.txt index a5c4c9358..7f3174aa2 100644 --- a/requirements.txt +++ b/requirements.txt @@ -31,7 +31,7 @@ notifications-python-client==5.7.0 # PaaS awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@43.3.0#egg=notifications-utils==43.3.0 +git+https://github.com/alphagov/notifications-utils.git@43.4.0#egg=notifications-utils==43.4.0 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.8.0 @@ -42,15 +42,15 @@ alembic==1.4.3 amqp==1.4.9 anyjson==0.3.3 attrs==20.3.0 -awscli==1.18.172 +awscli==1.18.178 bcrypt==3.2.0 billiard==3.3.0.23 bleach==3.1.4 blinker==1.4 boto==2.49.0 boto3==1.10.38 -botocore==1.19.12 -certifi==2020.6.20 +botocore==1.19.18 +certifi==2020.11.8 chardet==3.0.4 click==7.1.2 colorama==0.4.3 @@ -82,13 +82,13 @@ python-json-logger==0.1.11 pytz==2020.4 PyYAML==5.3.1 redis==3.5.3 -requests==2.24.0 +requests==2.25.0 rsa==4.5 s3transfer==0.3.3 six==1.15.0 smartypants==2.0.1 statsd==3.3.0 -urllib3==1.25.11 +urllib3==1.26.2 webencodings==0.5.1 Werkzeug==1.0.1 zipp==3.4.0