Make international SMS on for new services

International SMS is a mature, documented feature now. There’s no reason
it shouldn’t be available to everyone. If it’s turned off by default
then we’re relying on people finding it in the settings page to know
that it exists (which we found in research the other week that users,
who would have benefitted from having international SMS, were failing to
do).

This also fixes the problem whereby users signing up for Notify with an
international phone number (eg those working abroad for the Foreign and
Commonwealth Office) couldn’t get through the tour because they weren’t
able to send themselves the example text message (see
https://www.pivotaltracker.com/story/show/150705515).

app/dao/services_dao.py

@@ -35,11 +35,19 @@ from app.models import (
     JobStatistics,
     SMS_TYPE,
     EMAIL_TYPE,
-    ServiceSmsSender)
+    INTERNATIONAL_SMS_TYPE,
+    ServiceSmsSender,
+)
 from app.service.statistics import format_monthly_template_notification_stats
 from app.statsd_decorators import statsd
 from app.utils import get_london_month_from_utc_column, get_london_midnight_in_utc
 
+DEFAULT_SERVICE_PERMISSIONS = [
+    SMS_TYPE,
+    EMAIL_TYPE,
+    INTERNATIONAL_SMS_TYPE,
+]
+
 
 def dao_fetch_all_services(only_active=False):
     query = Service.query.order_by(
@@ -146,13 +154,16 @@ def dao_fetch_service_by_id_and_user(service_id, user_id):
 
 @transactional
 @version_class(Service)
-def dao_create_service(service, user, service_id=None, service_permissions=[SMS_TYPE, EMAIL_TYPE]):
+def dao_create_service(service, user, service_id=None, service_permissions=None):
     # the default property does not appear to work when there is a difference between the sqlalchemy schema and the
     # db schema (ie: during a migration), so we have to set sms_sender manually here. After the GOVUK sms_sender
     # migration is completed, this code should be able to be removed.
     if not service.sms_sender:
         service.sms_sender = current_app.config['FROM_NUMBER']
 
+    if service_permissions is None:
+        service_permissions = DEFAULT_SERVICE_PERMISSIONS
+
     from app.dao.permissions_dao import permission_dao
     service.users.append(user)
     permission_dao.add_default_service_permissions_for_user(user, service)

tests/app/conftest.py

@@ -137,7 +137,7 @@ def sample_service(
     restricted=False,
     limit=1000,
     email_from=None,
-    permissions=[SMS_TYPE, EMAIL_TYPE],
+    permissions=None,
     research_mode=None,
     free_sms_fragment_limit=250000
 ):
@@ -166,7 +166,7 @@ def sample_service(
         if user not in service.users:
             dao_add_user_to_service(service, user)
 
-    if INBOUND_SMS_TYPE in permissions:
+    if permissions and INBOUND_SMS_TYPE in permissions:
         create_inbound_number('12345', service_id=service.id)
 
     return service

tests/app/dao/test_services_dao.py

@@ -57,6 +57,7 @@ from app.models import (
     KEY_TYPE_TEST,
     EMAIL_TYPE,
     SMS_TYPE,
+    INTERNATIONAL_SMS_TYPE,
     LETTER_TYPE,
     SERVICE_PERMISSION_TYPES
 )
@@ -260,25 +261,38 @@ def test_create_service_returns_service_with_default_permissions(service_factory
     service = service_factory.get('testing', email_from='testing')
 
     service = dao_fetch_service_by_id(service.id)
-    assert len(service.permissions) == 2
+    assert len(service.permissions) == 3
-    assert set([SMS_TYPE, EMAIL_TYPE]) == set(p.permission for p in service.permissions)
+    assert set([
+        SMS_TYPE,
+        EMAIL_TYPE,
+        INTERNATIONAL_SMS_TYPE,
+    ]) == set(
+        p.permission for p in service.permissions
+    )
 
 
-@pytest.mark.parametrize("permission_to_remove, permission_remaining",
+@pytest.mark.parametrize("permission_to_remove, permission_remaining", [
-                         [(SMS_TYPE, EMAIL_TYPE),
+    (SMS_TYPE, EMAIL_TYPE),
-                          (EMAIL_TYPE, SMS_TYPE)])
+    (EMAIL_TYPE, SMS_TYPE),
+])
 def test_remove_permission_from_service_by_id_returns_service_with_correct_permissions(
-        sample_service, permission_to_remove, permission_remaining):
+    sample_service, permission_to_remove, permission_remaining
+):
     dao_remove_service_permission(service_id=sample_service.id, permission=permission_to_remove)
 
     service = dao_fetch_service_by_id(sample_service.id)
-    assert len(service.permissions) == 1
+    assert len(service.permissions) == 2
-    assert service.permissions[0].permission == permission_remaining
+    assert set(
+        p.permission for p in service.permissions
+    ) == set((
+        permission_remaining, INTERNATIONAL_SMS_TYPE
+    ))
 
 
 def test_removing_all_permission_returns_service_with_no_permissions(sample_service):
     dao_remove_service_permission(service_id=sample_service.id, permission=SMS_TYPE)
     dao_remove_service_permission(service_id=sample_service.id, permission=EMAIL_TYPE)
+    dao_remove_service_permission(service_id=sample_service.id, permission=INTERNATIONAL_SMS_TYPE)
 
     service = dao_fetch_service_by_id(sample_service.id)
     assert len(service.permissions) == 0
@@ -298,14 +312,22 @@ def test_create_service_by_id_adding_and_removing_letter_returns_service_without
     dao_add_service_permission(service_id=service.id, permission=LETTER_TYPE)
 
     service = dao_fetch_service_by_id(service.id)
-    assert len(service.permissions) == 3
+    assert len(service.permissions) == 4
-    assert set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]) == set([p.permission for p in service.permissions])
+    assert set([
+        SMS_TYPE, EMAIL_TYPE, LETTER_TYPE, INTERNATIONAL_SMS_TYPE,
+    ]) == set([
+        p.permission for p in service.permissions
+    ])
 
     dao_remove_service_permission(service_id=service.id, permission=LETTER_TYPE)
     service = dao_fetch_service_by_id(service.id)
 
-    assert len(service.permissions) == 2
+    assert len(service.permissions) == 3
-    assert set([SMS_TYPE, EMAIL_TYPE]) == set([p.permission for p in service.permissions])
+    assert set([
+        SMS_TYPE, EMAIL_TYPE, INTERNATIONAL_SMS_TYPE,
+    ]) == set([
+        p.permission for p in service.permissions
+    ])
 
 
 def test_create_service_creates_a_history_record_with_current_data(sample_user):
@@ -431,7 +453,7 @@ def test_delete_service_and_associated_objects(notify_db,
                                                sample_permission,
                                                sample_provider_statistics):
     # Default service permissions of Email and SMS
-    assert ServicePermission.query.count() == 2
+    assert ServicePermission.query.count() == 3
 
     delete_service_and_all_associated_db_objects(sample_service)
     assert NotificationStatistics.query.count() == 0

tests/app/notifications/test_validators.py

@@ -310,9 +310,14 @@ def test_should_not_rate_limit_if_limiting_is_disabled(
 
 
 @pytest.mark.parametrize('key_type', ['test', 'normal'])
-def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_int_sms(sample_service, key_type):
+def test_rejects_api_calls_with_international_numbers_if_service_does_not_allow_int_sms(
+    key_type,
+    notify_db,
+    notify_db_session,
+):
+    service = create_service(notify_db, notify_db_session, permissions=[SMS_TYPE])
     with pytest.raises(BadRequestError) as e:
-        validate_and_format_recipient('20-12-1234-1234', key_type, sample_service, SMS_TYPE)
+        validate_and_format_recipient('20-12-1234-1234', key_type, service, SMS_TYPE)
     assert e.value.status_code == 400
     assert e.value.message == 'Cannot send to international mobile numbers'
     assert e.value.fields == []

tests/app/service/test_rest.py

@@ -203,7 +203,14 @@ def test_get_service_list_has_default_permissions(client, service_factory):
     assert response.status_code == 200
     json_resp = json.loads(response.get_data(as_text=True))
     assert len(json_resp['data']) == 3
-    assert all([set(json['permissions']) == set([EMAIL_TYPE, SMS_TYPE]) for json in json_resp['data']])
+    assert all(
+        set(
+            json['permissions']
+        ) == set([
+            EMAIL_TYPE, SMS_TYPE, INTERNATIONAL_SMS_TYPE,
+        ])
+        for json in json_resp['data']
+    )
 
 
 def test_get_service_by_id_has_default_service_permissions(client, sample_service):
@@ -214,7 +221,11 @@ def test_get_service_by_id_has_default_service_permissions(client, sample_servic
     )
     json_resp = json.loads(resp.get_data(as_text=True))
 
-    assert set(json_resp['data']['permissions']) == set([EMAIL_TYPE, SMS_TYPE])
+    assert set(
+        json_resp['data']['permissions']
+    ) == set([
+        EMAIL_TYPE, SMS_TYPE, INTERNATIONAL_SMS_TYPE,
+    ])
 
 
 def test_get_service_by_id_should_404_if_no_service(notify_api, notify_db):

tests/app/v2/notifications/test_post_notifications.py

@@ -328,17 +328,26 @@ def test_returns_a_429_limit_exceeded_if_rate_limit_exceeded(
     assert not deliver_mock.called
 
 
-def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms(client, sample_service, sample_template):
+def test_post_sms_notification_returns_400_if_not_allowed_to_send_int_sms(
+    client,
+    notify_db,
+    notify_db_session,
+):
+
+    service = sample_service(notify_db, notify_db_session, permissions=[SMS_TYPE])
+    template = create_sample_template(notify_db, notify_db_session, service=service)
+
     data = {
         'phone_number': '20-12-1234-1234',
-        'template_id': sample_template.id
+        'template_id': template.id
     }
-    auth_header = create_authorization_header(service_id=sample_service.id)
+    auth_header = create_authorization_header(service_id=service.id)
 
     response = client.post(
         path='/v2/notifications/sms',
         data=json.dumps(data),
-        headers=[('Content-Type', 'application/json'), auth_header])
+        headers=[('Content-Type', 'application/json'), auth_header]
+    )
 
     assert response.status_code == 400
     assert response.headers['Content-type'] == 'application/json'
@@ -378,18 +387,20 @@ def test_post_sms_notification_returns_400_if_not_allowed_to_send_notification(
     ]
 
 
-def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(notify_db, notify_db_session, client, mocker):
+def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms(
-
+    sample_service,
-    service = sample_service(notify_db, notify_db_session, permissions=[SMS_TYPE, INTERNATIONAL_SMS_TYPE])
+    sample_template,
-    template = create_sample_template(notify_db, notify_db_session, service=service)
+    client,
+    mocker,
+):
 
     mocker.patch('app.celery.provider_tasks.deliver_sms.apply_async')
 
     data = {
         'phone_number': '20-12-1234-1234',
-        'template_id': template.id
+        'template_id': sample_template.id
     }
-    auth_header = create_authorization_header(service_id=service.id)
+    auth_header = create_authorization_header(service_id=sample_service.id)
 
     response = client.post(
         path='/v2/notifications/sms',