Warn about SMS failures when it's cost effective

Previously we warned when as little as 100 SMS messages failed, which
equates to £1.60 in cost. This increases the absolute threshold so that
we avoid taking action when the cost in developer time is greater than
the money wasted. If it takes us an hour to solve the ticket, then that
equates to roughly £100 in people costs.

In future, we should also consider simplifying this check to be about
permanent failures in general.

app/broadcast_message/broadcast_message_schema.py

@@ -15,7 +15,7 @@ create_broadcast_message_schema = {
         'finishes_at': {'type': 'string', 'format': 'datetime'},
         'areas': {"type": "array", "items": {"type": "string"}},
         'simple_polygons': {"type": "array", "items": {"type": "array"}},
-        'content': {'type': 'string', 'minLength': 1, 'maxLength': 1395},
+        'content': {'type': 'string', 'minLength': 1},
         'reference': {'type': 'string', 'minLength': 1, 'maxLength': 255},
     },
     'required': ['service_id', 'created_by'],

app/broadcast_message/rest.py

@@ -2,6 +2,7 @@ from datetime import datetime
 
 import iso8601
 from flask import Blueprint, current_app, jsonify, request
+from notifications_utils.template import BroadcastMessageTemplate
 
 from app.broadcast_message.broadcast_message_schema import (
     create_broadcast_message_schema,
@@ -116,6 +117,19 @@ def create_broadcast_message(service_id):
         reference = None
     else:
         template, content, reference = None, data['content'], data['reference']
+        temporary_template = BroadcastMessageTemplate.from_content(content)
+        if temporary_template.content_too_long:
+            raise InvalidRequest(
+                (
+                    f'Content must be '
+                    f'{temporary_template.max_content_count:,.0f} '
+                    f'characters or fewer'
+                ) + (
+                    ' (because it could not be GSM7 encoded)'
+                    if temporary_template.non_gsm_characters else ''
+                ),
+                status_code=400,
+            )
 
     broadcast_message = BroadcastMessage(
         service_id=service.id,

app/celery/broadcast_message_tasks.py

@@ -163,8 +163,8 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider):
         formatted_message_number = format_sequential_number(broadcast_provider_message.message_number)
 
     current_app.logger.info(
-        f'invoking cbc proxy to send '
-        f'broadcast_event {broadcast_event.reference} '
+        f'Invoking cbc proxy to send broadcast_provider_message with ID of {broadcast_provider_message.id} '
+        f'and broadcast_event ID of {broadcast_event_id} '
         f'msgType {broadcast_event.message_type}'
     )
 
@@ -215,8 +215,8 @@ def send_broadcast_provider_message(self, broadcast_event_id, provider):
     except CBCProxyRetryableException as exc:
         delay = get_retry_delay(self.request.retries)
         current_app.logger.exception(
-            f'Retrying send_broadcast_provider_message for broadcast_event {broadcast_event_id} and ' +
-            f'provider {provider} in {delay} seconds'
+            f'Retrying send_broadcast_provider_message for broadcast event {broadcast_event_id}, '
+            f'provider message {broadcast_provider_message.id}, provider {provider} in {delay} seconds'
         )
 
         self.retry(
@@ -236,6 +236,6 @@ def trigger_link_test(provider):
         sequence = Sequence('broadcast_provider_message_number_seq')
         sequential_number = db.session.connection().execute(sequence)
         formatted_seq_number = format_sequential_number(sequential_number)
-    message = f"Sending a link test to CBC proxy for provider {provider} with ID {identifier}"
+    message = f"Sending a link test to CBC proxy for provider {provider}. Identifier in payload is {identifier}"
     current_app.logger.info(message)
     cbc_proxy_client.get_proxy(provider).send_link_test(identifier, formatted_seq_number)

app/celery/scheduled_tasks.py

@@ -274,7 +274,7 @@ def check_for_services_with_high_failure_rates_or_sending_to_tv_numbers():
             )
             message += "service: {} failure rate: {},\n".format(service_dashboard, service.permanent_failure_rate)
     elif services_sending_to_tv_numbers:
-        message += "{} service(s) have sent over 500 sms messages to tv numbers in last 24 hours:\n".format(
+        message += "{} service(s) have sent high volumes of sms messages to tv numbers in last 24 hours:\n".format(
             len(services_sending_to_tv_numbers)
         )
         for service in services_sending_to_tv_numbers:

app/clients/sms/firetext.py

@@ -66,6 +66,7 @@ class FiretextClient(SmsClient):
         super(SmsClient, self).__init__(*args, **kwargs)
         self.current_app = current_app
         self.api_key = current_app.config.get('FIRETEXT_API_KEY')
+        self.international_api_key = current_app.config.get('FIRETEXT_INTERNATIONAL_API_KEY')
         self.from_number = current_app.config.get('FROM_NUMBER')
         self.name = 'firetext'
         self.url = current_app.config.get('FIRETEXT_URL')
@@ -91,10 +92,9 @@ class FiretextClient(SmsClient):
             self.statsd_client.incr("clients.firetext.error")
             self.current_app.logger.warning(log_message)
 
-    def send_sms(self, to, content, reference, sender=None):
-
+    def send_sms(self, to, content, reference, international, sender=None):
         data = {
-            "apiKey": self.api_key,
+            "apiKey": self.international_api_key if international else self.api_key,
             "from": self.from_number if sender is None else sender,
             "to": to.replace('+', ''),
             "message": content,

app/clients/sms/mmg.py

@@ -97,7 +97,7 @@ class MMGClient(SmsClient):
     def get_name(self):
         return self.name
 
-    def send_sms(self, to, content, reference, multi=True, sender=None):
+    def send_sms(self, to, content, reference, international, multi=True, sender=None):
         data = {
             "reqType": "BULK",
             "MSISDN": to,

app/config.py

@@ -99,6 +99,7 @@ class Config(object):
 
     # Firetext API Key
     FIRETEXT_API_KEY = os.getenv("FIRETEXT_API_KEY")
+    FIRETEXT_INTERNATIONAL_API_KEY = os.getenv("FIRETEXT_INTERNATIONAL_API_KEY", "placeholder")
 
     # Prefix to identify queues in SQS
     NOTIFICATION_QUEUE_PREFIX = os.getenv('NOTIFICATION_QUEUE_PREFIX')

app/dao/services_dao.py

@@ -542,7 +542,7 @@ def dao_fetch_active_users_for_service(service_id):
     return query.all()
 
 
-def dao_find_services_sending_to_tv_numbers(start_date, end_date, threshold=500):
+def dao_find_services_sending_to_tv_numbers(start_date, end_date, threshold=62_500):
     return db.session.query(
         Notification.service_id.label('service_id'),
         func.count(Notification.id).label('notification_count')
@@ -563,7 +563,7 @@ def dao_find_services_sending_to_tv_numbers(start_date, end_date, threshold=500)
     ).all()
 
 
-def dao_find_services_with_high_failure_rates(start_date, end_date, threshold=100):
+def dao_find_services_with_high_failure_rates(start_date, end_date, threshold=62_500):
     subquery = db.session.query(
         func.count(Notification.id).label('total_count'),
         Notification.service_id.label('service_id')

app/delivery/send_to_providers.py

@@ -68,7 +68,8 @@ def send_sms_to_provider(notification):
                     to=notification.normalised_to,
                     content=str(template),
                     reference=str(notification.id),
-                    sender=notification.reply_to_text
+                    sender=notification.reply_to_text,
+                    international=notification.international
                 )
             except Exception as e:
                 notification.billable_units = template.fragment_count

app/template/rest.py

@@ -5,7 +5,10 @@ import botocore
 from flask import Blueprint, current_app, jsonify, request
 from notifications_utils import SMS_CHAR_COUNT_LIMIT
 from notifications_utils.pdf import extract_page_from_pdf
-from notifications_utils.template import SMSMessageTemplate
+from notifications_utils.template import (
+    BroadcastMessageTemplate,
+    SMSMessageTemplate,
+)
 from PyPDF2.utils import PdfReadError
 from requests import post as requests_post
 from sqlalchemy.orm.exc import NoResultFound
@@ -28,7 +31,13 @@ from app.dao.templates_dao import (
 )
 from app.errors import InvalidRequest, register_errors
 from app.letters.utils import get_letter_pdf_and_metadata
-from app.models import LETTER_TYPE, SECOND_CLASS, SMS_TYPE, Template
+from app.models import (
+    BROADCAST_TYPE,
+    LETTER_TYPE,
+    SECOND_CLASS,
+    SMS_TYPE,
+    Template,
+)
 from app.notifications.validators import check_reply_to, service_has_permission
 from app.schema_validation import validate
 from app.schemas import (
@@ -48,10 +57,13 @@ register_errors(template_blueprint)
 
 
 def _content_count_greater_than_limit(content, template_type):
-    if template_type != SMS_TYPE:
-        return False
-    template = SMSMessageTemplate({'content': content, 'template_type': template_type})
-    return template.is_message_too_long()
+    if template_type == SMS_TYPE:
+        template = SMSMessageTemplate({'content': content, 'template_type': template_type})
+        return template.is_message_too_long()
+    if template_type == BROADCAST_TYPE:
+        template = BroadcastMessageTemplate({'content': content, 'template_type': template_type})
+        return template.is_message_too_long()
+    return False
 
 
 def validate_parent_folder(template_json):

app/v2/notifications/post_notifications.py

@@ -23,6 +23,7 @@ from app.celery.research_mode_tasks import create_fake_letter_response_file
 from app.celery.tasks import save_api_email, save_api_sms
 from app.clients.document_download import DocumentDownloadError
 from app.config import QueueNames, TaskNames
+from app.dao.dao_utils import transaction
 from app.dao.templates_dao import get_precompiled_letter_template
 from app.letters.utils import upload_letter_pdf
 from app.models import (
@@ -412,12 +413,14 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, service, t
     except ValueError:
         raise BadRequestError(message='Cannot decode letter content (invalid base64 encoding)', status_code=400)
 
-    notification = create_letter_notification(letter_data=letter_data,
-                                              service=service,
-                                              template=template,
-                                              api_key=api_key,
-                                              status=status,
-                                              reply_to_text=reply_to_text)
+    with transaction():
+        notification = create_letter_notification(letter_data=letter_data,
+                                                  service=service,
+                                                  template=template,
+                                                  api_key=api_key,
+                                                  status=status,
+                                                  reply_to_text=reply_to_text)
+        filename = upload_letter_pdf(notification, letter_content, precompiled=True)
 
     resp = {
         'id': notification.id,
@@ -425,8 +428,6 @@ def process_precompiled_letter_notifications(*, letter_data, api_key, service, t
         'postage': notification.postage
     }
 
-    filename = upload_letter_pdf(notification, letter_content, precompiled=True)
-
     current_app.logger.info('Calling task scan-file for {}'.format(filename))
 
     # call task to add the filename to anti virus queue

manifest.yml.j2

@@ -138,6 +138,7 @@ applications:
       MMG_INBOUND_SMS_USERNAME: '{{ MMG_INBOUND_SMS_USERNAME | tojson }}'
 
       FIRETEXT_API_KEY: '{{ FIRETEXT_API_KEY }}'
+      FIRETEXT_INTERNATIONAL_API_KEY: '{{ FIRETEXT_INTERNATIONAL_API_KEY }}'
       FIRETEXT_INBOUND_SMS_AUTH: '{{ FIRETEXT_INBOUND_SMS_AUTH | tojson }}'
 
       REDIS_ENABLED: '{{ REDIS_ENABLED }}'

tests/app/broadcast_message/test_rest.py

@@ -192,6 +192,40 @@ def test_create_broadcast_message_400s_if_json_schema_fails_validation(
     assert response['errors'] == expected_errors
 
 
+@pytest.mark.parametrize('content, expected_status, expected_errors', (
+    ('a', 201, None),
+    ('a' * 1_395, 201, None),
+    ('a\r\n' * 697, 201, None),  # 1,394 chars – new lines normalised to \n
+    ('a' * 1_396, 400, (
+        'Content must be 1,395 characters or fewer'
+    )),
+    ('ŵ' * 615, 201, None),
+    ('ŵ' * 616, 400, (
+        'Content must be 615 characters or fewer '
+        '(because it could not be GSM7 encoded)'
+    )),
+))
+def test_create_broadcast_message_400s_if_content_too_long(
+    admin_request,
+    sample_broadcast_service,
+    content,
+    expected_status,
+    expected_errors,
+):
+    response = admin_request.post(
+        'broadcast_message.create_broadcast_message',
+        service_id=sample_broadcast_service.id,
+        _data={
+            'service_id': str(sample_broadcast_service.id),
+            'created_by': str(sample_broadcast_service.created_by_id),
+            'reference': 'abc123',
+            'content': content,
+        },
+        _expected_status=expected_status,
+    )
+    assert response.get('message') == expected_errors
+
+
 @freeze_time('2020-01-01')
 def test_create_broadcast_message_can_be_created_from_content(admin_request, sample_broadcast_service):
     response = admin_request.post(

tests/app/celery/test_scheduled_tasks.py

@@ -564,9 +564,9 @@ MockServicesWithHighFailureRate = namedtuple(
     ],
     [
         [],
-        [MockServicesSendingToTVNumbers("123", 300)],
-        "1 service(s) have sent over 500 sms messages to tv numbers in last 24 hours:\n"
-        "service: {}/services/{} count of sms to tv numbers: 300,\n".format(
+        [MockServicesSendingToTVNumbers("123", 63_000)],
+        "1 service(s) have sent high volumes of sms messages to tv numbers in last 24 hours:\n"
+        "service: {}/services/{} count of sms to tv numbers: 63000,\n".format(
             Config.ADMIN_BASE_URL, "123"
         )
     ]

tests/app/clients/test_firetext.py

@@ -47,7 +47,7 @@ def test_send_sms_successful_returns_firetext_response(mocker, mock_firetext_cli
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        response = mock_firetext_client.send_sms(to, content, reference)
+        response = mock_firetext_client.send_sms(to, content, reference, False)
 
     response_json = response.json()
     assert response.status_code == 200
@@ -65,7 +65,7 @@ def test_send_sms_calls_firetext_correctly(mocker, mock_firetext_client):
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        mock_firetext_client.send_sms(to, content, reference)
+        mock_firetext_client.send_sms(to, content, reference, False)
 
     assert request_mock.call_count == 1
     assert request_mock.request_history[0].url == 'https://example.com/firetext'
@@ -79,6 +79,30 @@ def test_send_sms_calls_firetext_correctly(mocker, mock_firetext_client):
     assert request_args['reference'][0] == reference
 
 
+def test_send_sms_calls_firetext_correctly_for_international(mocker, mock_firetext_client):
+    to = '+607234567890'
+    content = 'my message'
+    reference = 'my reference'
+    response_dict = {
+        'code': 0,
+    }
+
+    with requests_mock.Mocker() as request_mock:
+        request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
+        mock_firetext_client.send_sms(to, content, reference, True)
+
+    assert request_mock.call_count == 1
+    assert request_mock.request_history[0].url == 'https://example.com/firetext'
+    assert request_mock.request_history[0].method == 'POST'
+
+    request_args = parse_qs(request_mock.request_history[0].text)
+    assert request_args['apiKey'][0] == 'international'
+    assert request_args['from'][0] == 'bar'
+    assert request_args['to'][0] == '607234567890'
+    assert request_args['message'][0] == content
+    assert request_args['reference'][0] == reference
+
+
 def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
     to = content = reference = 'foo'
     response_dict = {
@@ -90,7 +114,7 @@ def test_send_sms_raises_if_firetext_rejects(mocker, mock_firetext_client):
 
     with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        mock_firetext_client.send_sms(to, content, reference)
+        mock_firetext_client.send_sms(to, content, reference, False)
 
     assert exc.value.status_code == 200
     assert '"description": "Some kind of error"' in exc.value.text
@@ -103,7 +127,7 @@ def test_send_sms_raises_if_firetext_rejects_with_unexpected_data(mocker, mock_f
 
     with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=400)
-        mock_firetext_client.send_sms(to, content, reference)
+        mock_firetext_client.send_sms(to, content, reference, False)
 
     assert exc.value.status_code == 400
     assert exc.value.text == '{"something": "gone bad"}'
@@ -121,7 +145,7 @@ def test_send_sms_override_configured_shortcode_with_sender(mocker, mock_firetex
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/firetext', json=response_dict, status_code=200)
-        mock_firetext_client.send_sms(to, content, reference, sender=sender)
+        mock_firetext_client.send_sms(to, content, reference, False, sender=sender)
 
     request_args = parse_qs(request_mock.request_history[0].text)
     assert request_args['from'][0] == 'fromservice'
@@ -132,7 +156,7 @@ def test_send_sms_raises_if_firetext_rejects_with_connect_timeout(rmock, mock_fi
 
     with pytest.raises(FiretextClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/firetext', exc=ConnectTimeout)
-        mock_firetext_client.send_sms(to, content, reference)
+        mock_firetext_client.send_sms(to, content, reference, False)
 
     assert exc.value.status_code == 504
     assert exc.value.text == 'Gateway Time-out'
@@ -143,7 +167,7 @@ def test_send_sms_raises_if_firetext_rejects_with_read_timeout(rmock, mock_firet
 
     with pytest.raises(FiretextClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/firetext', exc=ReadTimeout)
-        mock_firetext_client.send_sms(to, content, reference)
+        mock_firetext_client.send_sms(to, content, reference, False)
 
     assert exc.value.status_code == 504
     assert exc.value.text == 'Gateway Time-out'

tests/app/clients/test_mmg.py

@@ -44,7 +44,7 @@ def test_send_sms_successful_returns_mmg_response(notify_api, mocker):
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', json=response_dict, status_code=200)
-        response = mmg_client.send_sms(to, content, reference)
+        response = mmg_client.send_sms(to, content, reference, False)
 
     response_json = response.json()
     assert response.status_code == 200
@@ -59,7 +59,7 @@ def test_send_sms_calls_mmg_correctly(notify_api, mocker):
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', json=response_dict, status_code=200)
-        mmg_client.send_sms(to, content, reference)
+        mmg_client.send_sms(to, content, reference, False)
 
     assert request_mock.call_count == 1
     assert request_mock.request_history[0].url == 'https://example.com/mmg'
@@ -83,7 +83,7 @@ def test_send_sms_raises_if_mmg_rejects(notify_api, mocker):
 
     with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', json=response_dict, status_code=400)
-        mmg_client.send_sms(to, content, reference)
+        mmg_client.send_sms(to, content, reference, False)
 
     assert exc.value.status_code == 400
     assert '"Error": 206' in exc.value.text
@@ -100,7 +100,7 @@ def test_send_sms_override_configured_shortcode_with_sender(notify_api, mocker):
 
     with requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', json=response_dict, status_code=200)
-        mmg_client.send_sms(to, content, reference, sender=sender)
+        mmg_client.send_sms(to, content, reference, False, sender=sender)
 
     request_args = request_mock.request_history[0].json()
     assert request_args['sender'] == 'fromservice'
@@ -112,7 +112,7 @@ def test_send_sms_raises_if_mmg_fails_to_return_json(notify_api, mocker):
 
     with pytest.raises(SmsClientResponseException) as exc, requests_mock.Mocker() as request_mock:
         request_mock.post('https://example.com/mmg', text=response_dict, status_code=200)
-        mmg_client.send_sms(to, content, reference)
+        mmg_client.send_sms(to, content, reference, False)
 
     assert 'Code 200 text NOT AT ALL VALID JSON {"key" : "value"}} exception Expecting value: line 1 column 1 (char 0)' in str(exc.value)  # noqa
     assert exc.value.status_code == 200
@@ -124,7 +124,7 @@ def test_send_sms_raises_if_mmg_rejects_with_connect_timeout(rmock):
 
     with pytest.raises(MMGClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/mmg', exc=ConnectTimeout)
-        mmg_client.send_sms(to, content, reference)
+        mmg_client.send_sms(to, content, reference, False)
 
     assert exc.value.status_code == 504
     assert exc.value.text == 'Gateway Time-out'
@@ -135,7 +135,7 @@ def test_send_sms_raises_if_mmg_rejects_with_read_timeout(rmock):
 
     with pytest.raises(MMGClientResponseException) as exc:
         rmock.register_uri('POST', 'https://example.com/mmg', exc=ReadTimeout)
-        mmg_client.send_sms(to, content, reference)
+        mmg_client.send_sms(to, content, reference, False)
 
     assert exc.value.status_code == 504
     assert exc.value.text == 'Gateway Time-out'

tests/app/conftest.py

@@ -594,6 +594,7 @@ def mock_firetext_client(mocker):
     current_app = mocker.Mock(config={
         'FIRETEXT_URL': 'https://example.com/firetext',
         'FIRETEXT_API_KEY': 'foo',
+        'FIRETEXT_INTERNATIONAL_API_KEY': 'international',
         'FROM_NUMBER': 'bar'
     })
     client.init_app(current_app, statsd_client)

tests/app/delivery/test_send_to_providers.py

@@ -123,7 +123,8 @@ def test_should_send_personalised_template_to_correct_sms_provider_and_persist(
         to="447234123123",
         content="Sample service: Hello Jo\nHere is <em>some HTML</em> & entities",
         reference=str(db_notification.id),
-        sender=current_app.config['FROM_NUMBER']
+        sender=current_app.config['FROM_NUMBER'],
+        international=False
     )
 
     notification = Notification.query.filter_by(id=db_notification.id).one()
@@ -158,7 +159,7 @@ def test_should_send_personalised_template_to_correct_email_provider_and_persist
         'Jo <em>some HTML</em>',
         body='Hello Jo\nThis is an email from GOV.\u200bUK with <em>some HTML</em>\n',
         html_body=ANY,
-        reply_to_address=None
+        reply_to_address=None,
     )
 
     assert '<!DOCTYPE html' in app.aws_ses_client.send_email.call_args[1]['html_body']
@@ -185,7 +186,7 @@ def test_should_not_send_email_message_when_service_is_inactive_notifcation_is_i
 
 
 @pytest.mark.parametrize("client_send", ["app.mmg_client.send_sms", "app.firetext_client.send_sms"])
-def test_should_not_send_sms_message_when_service_is_inactive_notifcation_is_in_tech_failure(
+def test_should_not_send_sms_message_when_service_is_inactive_notification_is_in_tech_failure(
         sample_service, sample_notification, mocker, client_send):
     sample_service.active = False
     send_mock = mocker.patch(client_send, return_value='reference')
@@ -226,7 +227,8 @@ def test_send_sms_should_use_template_version_from_notification_not_latest(
         to=validate_and_format_phone_number("+447234123123"),
         content="Sample service: This is a template:\nwith a newline",
         reference=str(db_notification.id),
-        sender=current_app.config['FROM_NUMBER']
+        sender=current_app.config['FROM_NUMBER'],
+        international=False
     )
 
     persisted_notification = notifications_dao.get_notification_by_id(db_notification.id)
@@ -324,7 +326,8 @@ def test_should_send_sms_with_downgraded_content(notify_db_session, mocker):
         to=ANY,
         content=gsm_message,
         reference=ANY,
-        sender=ANY
+        sender=ANY,
+        international=False
     )
 
 
@@ -345,7 +348,8 @@ def test_send_sms_should_use_service_sms_sender(
         to=ANY,
         content=ANY,
         reference=ANY,
-        sender=sms_sender.sms_sender
+        sender=sms_sender.sms_sender,
+        international=False
     )
 
 
@@ -666,7 +670,8 @@ def test_should_send_sms_to_international_providers(
         to="447234123999",
         content=ANY,
         reference=str(notification_uk.id),
-        sender=current_app.config['FROM_NUMBER']
+        sender=current_app.config['FROM_NUMBER'],
+        international=False
     )
 
     send_to_providers.send_sms_to_provider(
@@ -677,7 +682,8 @@ def test_should_send_sms_to_international_providers(
         to="601117224412",
         content=ANY,
         reference=str(notification_international.id),
-        sender=current_app.config['FROM_NUMBER']
+        sender=current_app.config['FROM_NUMBER'],
+        international=True
     )
 
     assert notification_uk.status == 'sending'
@@ -715,6 +721,7 @@ def test_should_handle_sms_sender_and_prefix_message(
         sender=expected_sender,
         to=ANY,
         reference=ANY,
+        international=False
     )
 
 
@@ -750,7 +757,8 @@ def test_send_sms_to_provider_should_use_normalised_to(
     send_mock.assert_called_once_with(to=notification.normalised_to,
                                       content=ANY,
                                       reference=str(notification.id),
-                                      sender=notification.reply_to_text)
+                                      sender=notification.reply_to_text,
+                                      international=False)
 
 
 def test_send_email_to_provider_should_user_normalised_to(
@@ -801,7 +809,8 @@ def test_send_sms_to_provider_should_return_template_if_found_in_redis(
     send_mock.assert_called_once_with(to=notification.normalised_to,
                                       content=ANY,
                                       reference=str(notification.id),
-                                      sender=notification.reply_to_text)
+                                      sender=notification.reply_to_text,
+                                      international=False)
 
 
 def test_send_email_to_provider_should_return_template_if_found_in_redis(

tests/app/template/test_rest.py

@@ -731,14 +731,24 @@ def test_should_return_404_if_no_templates_for_service_with_id(client, sample_se
     assert json_resp['message'] == 'No result found'
 
 
-def test_create_400_for_over_limit_content(client, notify_api, sample_user, sample_service, fake_uuid):
+@pytest.mark.parametrize('template_type', (
+    SMS_TYPE, BROADCAST_TYPE,
+))
+def test_create_400_for_over_limit_content(
+    client,
+    notify_api,
+    sample_user,
+    fake_uuid,
+    template_type,
+):
+    sample_service = create_service(service_permissions=[template_type])
     content = ''.join(random.choice(string.ascii_uppercase + string.digits) for _ in range(SMS_CHAR_COUNT_LIMIT + 1))
     data = {
         'name': 'too big template',
-        'template_type': SMS_TYPE,
+        'template_type': template_type,
         'content': content,
         'service': str(sample_service.id),
-        'created_by': str(sample_user.id)
+        'created_by': str(sample_service.created_by.id)
     }
     data = json.dumps(data)
     auth_header = create_authorization_header()

tests/app/v2/notifications/test_post_letter_notifications.py

@@ -20,6 +20,9 @@ from app.models import (
     Job,
     Notification,
 )
+from app.notifications.process_letter_notifications import (
+    create_letter_notification,
+)
 from app.schema_validation import validate
 from app.v2.errors import RateLimitError
 from app.v2.notifications.notification_schemas import post_letter_response
@@ -673,6 +676,31 @@ def test_post_precompiled_letter_notification_returns_201(
     assert resp_json == {'id': str(notification.id), 'reference': 'letter-reference', 'postage': expected_postage}
 
 
+def test_post_precompiled_letter_notification_if_s3_upload_fails_notification_is_not_persisted(
+    client, notify_user, mocker
+):
+    sample_service = create_service(service_permissions=['letter'])
+    persist_letter_mock = mocker.patch('app.v2.notifications.post_notifications.create_letter_notification',
+                                       side_effect=create_letter_notification)
+    s3mock = mocker.patch('app.v2.notifications.post_notifications.upload_letter_pdf', side_effect=Exception())
+    mocker.patch('app.celery.letters_pdf_tasks.notify_celery.send_task')
+    data = {
+        "reference": "letter-reference",
+        "content": "bGV0dGVyLWNvbnRlbnQ="
+    }
+
+    auth_header = create_authorization_header(service_id=sample_service.id)
+    with pytest.raises(expected_exception=Exception):
+        client.post(
+            path="v2/notifications/letter",
+            data=json.dumps(data),
+            headers=[('Content-Type', 'application/json'), auth_header])
+
+    assert s3mock.called
+    assert persist_letter_mock.called
+    assert Notification.query.count() == 0
+
+
 def test_post_letter_notification_throws_error_for_invalid_postage(client, notify_user, mocker):
     sample_service = create_service(service_permissions=['letter'])
     data = {