diff --git a/app/notifications/validators.py b/app/notifications/validators.py index 91747d14d..22277b5fc 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" ) @@ -144,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) @@ -157,8 +165,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 +180,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_is_message_too_long(template_with_content) return template, template_with_content @@ -192,7 +203,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 +213,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 +223,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..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_is_message_too_long) 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_is_message_too_long(template_with_content) + resp = create_response_for_post_notification( notification_id=notification_id, client_reference=form.get('reference', None), diff --git a/requirements-app.txt b/requirements-app.txt index 1d76201f8..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.0.0#egg=notifications-utils==43.0.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 4296eab2f..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.0.0#egg=notifications-utils==43.0.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 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index de6ce99ce..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,38 @@ 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): + 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_is_message_too_long') + 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']) 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):