From 46d45d859545cf1c4e1a72d0dc51cdaafe6cf5c1 Mon Sep 17 00:00:00 2001 From: Chris Hill-Scott Date: Thu, 19 Oct 2017 11:06:28 +0100 Subject: [PATCH] Make international SMS on for new services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 | 15 +++++- tests/app/conftest.py | 4 +- tests/app/dao/test_services_dao.py | 48 ++++++++++++++----- tests/app/notifications/test_validators.py | 9 +++- tests/app/service/test_rest.py | 15 +++++- .../notifications/test_post_notifications.py | 31 ++++++++---- 6 files changed, 91 insertions(+), 31 deletions(-) diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index 5ae25c0e6..31b9e3300 100644 --- a/app/dao/services_dao.py +++ b/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) diff --git a/tests/app/conftest.py b/tests/app/conftest.py index d904c8ecf..7f3bd3008 100644 --- a/tests/app/conftest.py +++ b/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 diff --git a/tests/app/dao/test_services_dao.py b/tests/app/dao/test_services_dao.py index 1d01c4a3e..3396e52ca 100644 --- a/tests/app/dao/test_services_dao.py +++ b/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 set([SMS_TYPE, EMAIL_TYPE]) == set(p.permission for p in service.permissions) + assert len(service.permissions) == 3 + 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", - [(SMS_TYPE, EMAIL_TYPE), - (EMAIL_TYPE, SMS_TYPE)]) +@pytest.mark.parametrize("permission_to_remove, permission_remaining", [ + (SMS_TYPE, EMAIL_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 service.permissions[0].permission == permission_remaining + assert len(service.permissions) == 2 + 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 set([SMS_TYPE, EMAIL_TYPE, LETTER_TYPE]) == set([p.permission for p in service.permissions]) + assert len(service.permissions) == 4 + 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 set([SMS_TYPE, EMAIL_TYPE]) == set([p.permission for p in service.permissions]) + assert len(service.permissions) == 3 + 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 diff --git a/tests/app/notifications/test_validators.py b/tests/app/notifications/test_validators.py index 387da140e..fecfc4da5 100644 --- a/tests/app/notifications/test_validators.py +++ b/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 == [] diff --git a/tests/app/service/test_rest.py b/tests/app/service/test_rest.py index 21046702d..63381561c 100644 --- a/tests/app/service/test_rest.py +++ b/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): diff --git a/tests/app/v2/notifications/test_post_notifications.py b/tests/app/v2/notifications/test_post_notifications.py index 390ccac08..94fea27e5 100644 --- a/tests/app/v2/notifications/test_post_notifications.py +++ b/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): - - service = sample_service(notify_db, notify_db_session, permissions=[SMS_TYPE, INTERNATIONAL_SMS_TYPE]) - template = create_sample_template(notify_db, notify_db_session, service=service) +def test_post_sms_notification_returns_201_if_allowed_to_send_int_sms( + sample_service, + sample_template, + 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',