diff --git a/app/authentication/auth.py b/app/authentication/auth.py index cb94d6c93..02ae025bf 100644 --- a/app/authentication/auth.py +++ b/app/authentication/auth.py @@ -125,10 +125,12 @@ def requires_auth(): g.service_id = api_key.service_id _request_ctx_stack.top.authenticated_service = service _request_ctx_stack.top.api_user = api_key - current_app.logger.info('API authorised for service {} with api key {}, using issuer {}'.format( + + current_app.logger.info('API authorised for service {} with api key {}, using issuer {} for URL: {}'.format( service.id, api_key.id, - request.headers.get('User-Agent') + request.headers.get('User-Agent'), + request.base_url )) return else: diff --git a/app/notifications/rest.py b/app/notifications/rest.py index ac8c63791..088f8479c 100644 --- a/app/notifications/rest.py +++ b/app/notifications/rest.py @@ -8,7 +8,6 @@ from flask import ( from app import api_user, authenticated_service from app.config import QueueNames from app.dao import ( - templates_dao, notifications_dao ) from app.errors import ( @@ -25,10 +24,9 @@ from app.notifications.process_notifications import ( simulated_recipient ) from app.notifications.validators import ( - check_template_is_for_notification_type, - check_template_is_active, check_rate_limiting, service_has_permission, + validate_template ) from app.schemas import ( email_notification_schema, @@ -102,14 +100,12 @@ def send_notification(notification_type): check_rate_limiting(authenticated_service, api_user) - template = templates_dao.dao_get_template_by_id_and_service_id( + template, template_with_content = validate_template( template_id=notification_form['template'], - service_id=authenticated_service.id) - - check_template_is_for_notification_type(notification_type, template.template_type) - check_template_is_active(template) - - template_object = create_template_object_for_notification(template, notification_form.get('personalisation', {})) + personalisation=notification_form.get('personalisation', {}), + service=authenticated_service, + notification_type=notification_type + ) _service_allowed_to_send_to(notification_form, authenticated_service) if not service_has_permission(notification_type, authenticated_service.permissions): @@ -147,7 +143,7 @@ def send_notification(notification_type): data=get_notification_return_data( notification_model.id, notification_form, - template_object) + template_with_content) ), 201 diff --git a/app/notifications/validators.py b/app/notifications/validators.py index f8e1722ca..ef79cf79e 100644 --- a/app/notifications/validators.py +++ b/app/notifications/validators.py @@ -124,9 +124,10 @@ def validate_and_format_recipient(send_to, key_type, service, notification_type, return validate_and_format_email_address(email_address=send_to) -def check_sms_content_char_count(content_count): - if content_count > SMS_CHAR_COUNT_LIMIT: - message = 'Content for template has a character count greater than the limit of {}'.format(SMS_CHAR_COUNT_LIMIT) +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" raise BadRequestError(message=message) @@ -151,9 +152,10 @@ def validate_template(template_id, personalisation, service, notification_type): check_template_is_active(template) template_with_content = create_content_for_notification(template, personalisation) + check_notification_content_is_not_empty(template_with_content) - if template.template_type == SMS_TYPE: - check_sms_content_char_count(template_with_content.content_count) + + check_content_char_count(template_with_content) return template, template_with_content diff --git a/requirements-app.txt b/requirements-app.txt index 1f10f4a36..f719bba6b 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -27,4 +27,4 @@ notifications-python-client==5.5.1 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@36.6.1#egg=notifications-utils==36.6.1 +git+https://github.com/alphagov/notifications-utils.git@36.6.2#egg=notifications-utils==36.6.2 diff --git a/requirements.txt b/requirements.txt index 237396dd5..c91f4bfb1 100644 --- a/requirements.txt +++ b/requirements.txt @@ -29,7 +29,7 @@ notifications-python-client==5.5.1 awscli-cwlogs>=1.4,<1.5 -git+https://github.com/alphagov/notifications-utils.git@36.6.1#egg=notifications-utils==36.6.1 +git+https://github.com/alphagov/notifications-utils.git@36.6.2#egg=notifications-utils==36.6.2 ## The following requirements were added by pip freeze: alembic==1.4.1 diff --git a/tests/app/notifications/rest/test_send_notification.py b/tests/app/notifications/rest/test_send_notification.py index 683b31abf..3949cee8f 100644 --- a/tests/app/notifications/rest/test_send_notification.py +++ b/tests/app/notifications/rest/test_send_notification.py @@ -99,8 +99,8 @@ def test_send_notification_invalid_template_id(notify_api, sample_template, mock json_resp = json.loads(response.get_data(as_text=True)) mocked.assert_not_called() - assert response.status_code == 404 - test_string = 'No result found' + assert response.status_code == 400 + test_string = 'Template not found' assert test_string in json_resp['message'] @@ -315,8 +315,8 @@ def test_should_not_allow_template_from_another_service(notify_api, json_resp = json.loads(response.get_data(as_text=True)) mocked.assert_not_called() - assert response.status_code == 404 - test_string = 'No result found' + assert response.status_code == 400 + test_string = 'Template not found' assert test_string in json_resp['message'] diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index f4ee4c6ee..2c7d813b5 100644 --- a/tests/app/notifications/test_validators.py +++ b/tests/app/notifications/test_validators.py @@ -8,12 +8,12 @@ from app.dao import templates_dao from app.models import SMS_TYPE, EMAIL_TYPE, LETTER_TYPE from app.notifications.process_notifications import create_content_for_notification from app.notifications.validators import ( + check_content_char_count, check_if_service_can_send_files_by_email, check_notification_content_is_not_empty, check_service_over_daily_message_limit, check_template_is_for_notification_type, check_template_is_active, - check_sms_content_char_count, check_service_over_api_rate_limit, check_service_email_reply_to_id, check_service_sms_sender_id, @@ -21,8 +21,9 @@ from app.notifications.validators import ( check_reply_to, service_can_send_to_recipient, validate_and_format_recipient, - validate_template + validate_template, ) +from app.utils import get_template_instance from app.v2.errors import ( BadRequestError, @@ -277,20 +278,40 @@ 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]) -def test_check_sms_content_char_count_passes(char_count, notify_api): - assert check_sms_content_char_count(char_count) is None +@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): + 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 @pytest.mark.parametrize('char_count', [613, 700, 6000]) -def test_check_sms_content_char_count_fails(char_count, notify_api): +@pytest.mark.parametrize('show_prefix', [True, False]) +def test_check_content_char_count_fails(notify_db_session, show_prefix, char_count): with pytest.raises(BadRequestError) as e: - check_sms_content_char_count(char_count) + 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) assert e.value.status_code == 400 - assert e.value.message == 'Content for template has a character count greater than the limit of {}'.format( - SMS_CHAR_COUNT_LIMIT) + assert e.value.message == f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \ + f'Your message is {char_count} characters' 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) + 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 + + def test_check_notification_content_is_not_empty_passes(notify_api, mocker, sample_service): template_id = create_template(sample_service, content="Content is not empty").id template = templates_dao.dao_get_template_by_id_and_service_id( @@ -321,7 +342,12 @@ def test_check_notification_content_is_not_empty_fails( assert e.value.fields == [] -def test_validate_template(mocker, fake_uuid, sample_service): +def test_validate_template(sample_service): + template = create_template(sample_service, template_type="email") + validate_template(template.id, {}, sample_service, "email") + + +def test_validate_template_calls_all_validators(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') @@ -329,12 +355,14 @@ def test_validate_template(mocker, fake_uuid, sample_service): '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') validate_template(template.id, {}, sample_service, "email") 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") @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 b70459fc8..db4bf7946 100644 --- a/tests/app/service/test_send_one_off_notification.py +++ b/tests/app/service/test_send_one_off_notification.py @@ -294,8 +294,8 @@ 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 == 'Content for template has a character count greater than the limit of {}'.format( - SMS_CHAR_COUNT_LIMIT) + assert e.value.message == f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \ + f'Your message is {729} characters' def test_send_one_off_notification_fails_if_created_by_other_service(sample_template):