Merge pull request #2741 from alphagov/make-sms-char-count-consistent

Update validators to use is_message_too_long()

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:

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)
+        personalisation=notification_form.get('personalisation', {}),
-
+        service=authenticated_service,
-    check_template_is_for_notification_type(notification_type, template.template_type)
+        notification_type=notification_type
-    check_template_is_active(template)
+    )
-
-    template_object = create_template_object_for_notification(template, notification_form.get('personalisation', {}))
 
     _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
 
 

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):
+def check_content_char_count(template_with_content):
-    if content_count > SMS_CHAR_COUNT_LIMIT:
+    if template_with_content.is_message_too_long():
-        message = 'Content for template has a character count greater than the limit of {}'.format(SMS_CHAR_COUNT_LIMIT)
+        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
 

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

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

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
+            assert response.status_code == 400
-            test_string = 'No result found'
+            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
+            assert response.status_code == 400
-            test_string = 'No result found'
+            test_string = 'Template not found'
             assert test_string in json_resp['message']
 
 

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):
+@pytest.mark.parametrize('show_prefix', [True, False])
-    assert check_sms_content_char_count(char_count) is None
+@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(
+    assert e.value.message == f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \
-        SMS_CHAR_COUNT_LIMIT)
+                              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'])

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(
+    assert e.value.message == f'Text messages cannot be longer than {SMS_CHAR_COUNT_LIMIT} characters. ' \
-        SMS_CHAR_COUNT_LIMIT)
+                              f'Your message is {729} characters'
 
 
 def test_send_one_off_notification_fails_if_created_by_other_service(sample_template):