From 5986a650052b71a37bbffe1e609ba6eb8d6dce86 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 24 Feb 2021 14:38:06 +0000 Subject: [PATCH 1/3] Check international number for alpha: NO if true then use number to send SMS. This is not a catch all for international SMS, the rules are quite complex and still not completely understood. We are talking with our provider who maybe able to sort this out for us. But in the meantime, this should solve for the case that we understand. --- app/user/rest.py | 20 +++++++++++--------- requirements-app.txt | 2 +- tests/app/user/test_rest.py | 2 +- 3 files changed, 13 insertions(+), 11 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index 32aa7b532..0f9c90ad3 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -4,7 +4,7 @@ from datetime import datetime from urllib.parse import urlencode from flask import (jsonify, request, Blueprint, current_app, abort) -from notifications_utils.recipients import is_uk_phone_number +from notifications_utils.recipients import is_uk_phone_number, use_numeric_sender from sqlalchemy.exc import IntegrityError from app.config import QueueNames @@ -105,10 +105,7 @@ def update_user_attribute(user_id): elif 'mobile_number' in update_dct: template = dao_get_template_by_id(current_app.config['TEAM_MEMBER_EDIT_MOBILE_TEMPLATE_ID']) recipient = user_to_update.mobile_number - if is_uk_phone_number(recipient): - reply_to = template.service.get_default_sms_sender() - else: - reply_to = current_app.config['NOTIFY_INTERNATIONAL_SMS_SENDER'] + reply_to = set_reply_to(recipient, template) else: return jsonify(data=user_to_update.serialize()), 200 service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) @@ -133,6 +130,14 @@ def update_user_attribute(user_id): return jsonify(data=user_to_update.serialize()), 200 +def set_reply_to(recipient, template): + if not is_uk_phone_number(recipient) and use_numeric_sender(recipient): + reply_to = current_app.config['NOTIFY_INTERNATIONAL_SMS_SENDER'] + else: + reply_to = template.service.get_default_sms_sender() + return reply_to + + @user_blueprint.route('//archive', methods=['POST']) def archive_user(user_id): user = get_user_by_id(user_id) @@ -271,10 +276,7 @@ def create_2fa_code(template_id, user_to_send_to, secret_code, recipient, person create_user_code(user_to_send_to, secret_code, template.template_type) reply_to = None if template.template_type == SMS_TYPE: - if is_uk_phone_number(recipient): - reply_to = template.service.get_default_sms_sender() - else: - reply_to = current_app.config['NOTIFY_INTERNATIONAL_SMS_SENDER'] + reply_to = set_reply_to(recipient, template) elif template.template_type == EMAIL_TYPE: reply_to = template.service.get_default_reply_to_email_address() saved_notification = persist_notification( diff --git a/requirements-app.txt b/requirements-app.txt index 8d99c0d8f..80933ce40 100644 --- a/requirements-app.txt +++ b/requirements-app.txt @@ -33,7 +33,7 @@ notifications-python-client==5.7.1 # PaaS awscli-cwlogs==1.4.6 -git+https://github.com/alphagov/notifications-utils.git@43.9.0#egg=notifications-utils==43.9.0 +git+https://github.com/alphagov/notifications-utils.git@43.9.1#egg=notifications-utils==43.9.1 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.9.0 diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 64ad80d15..7c17f9e83 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -315,7 +315,7 @@ def test_post_user_attribute_with_updated_by_sends_notification_to_international ): updater = create_user(name="Service Manago") update_dict = { - 'mobile_number': '601117224412', + 'mobile_number': '+601117224412', 'updated_by': str(updater.id) } auth_header = create_authorization_header() From 11bd906338f3b356a0723045c39f922a74e68a0d Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Wed, 24 Feb 2021 16:56:33 +0000 Subject: [PATCH 2/3] Update utils to get the new function --- requirements.txt | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/requirements.txt b/requirements.txt index 41242987a..6bedf5a41 100644 --- a/requirements.txt +++ b/requirements.txt @@ -35,25 +35,25 @@ notifications-python-client==5.7.1 # PaaS awscli-cwlogs==1.4.6 -git+https://github.com/alphagov/notifications-utils.git@43.9.0#egg=notifications-utils==43.9.0 +git+https://github.com/alphagov/notifications-utils.git@43.9.1#egg=notifications-utils==43.9.1 # gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains prometheus-client==0.9.0 gds-metrics==0.2.4 ## The following requirements were added by pip freeze: -alembic==1.5.4 +alembic==1.5.5 amqp==1.4.9 anyjson==0.3.3 attrs==20.3.0 -awscli==1.19.8 +awscli==1.19.14 bcrypt==3.2.0 billiard==3.3.0.23 bleach==3.3.0 blinker==1.4 boto==2.49.0 -boto3==1.17.8 -botocore==1.20.8 +boto3==1.17.14 +botocore==1.20.14 certifi==2020.12.5 chardet==4.0.0 click==7.1.2 @@ -66,7 +66,7 @@ geojson==2.5.0 govuk-bank-holidays==0.8 greenlet==1.0.0 idna==2.10 -importlib-metadata==3.4.0 +importlib-metadata==3.6.0 Jinja2==2.11.3 jmespath==0.10.0 kombu==3.0.37 @@ -86,10 +86,10 @@ python-dateutil==2.8.1 python-editor==1.0.4 python-json-logger==2.0.1 pytz==2021.1 -PyYAML==5.3.1 +PyYAML==5.4.1 redis==3.5.3 requests==2.25.1 -rsa==4.5 +rsa==4.7.2 s3transfer==0.3.4 Shapely==1.7.1 six==1.15.0 From 97d1bfaee8b6ff10ffadd95c456a002574d56357 Mon Sep 17 00:00:00 2001 From: Rebecca Law Date: Thu, 25 Feb 2021 08:10:52 +0000 Subject: [PATCH 3/3] Rename method for clarity Added unit test for new method. --- app/user/rest.py | 6 +++--- tests/app/user/test_rest.py | 17 +++++++++++++++++ 2 files changed, 20 insertions(+), 3 deletions(-) diff --git a/app/user/rest.py b/app/user/rest.py index 0f9c90ad3..6867e58b6 100644 --- a/app/user/rest.py +++ b/app/user/rest.py @@ -105,7 +105,7 @@ def update_user_attribute(user_id): elif 'mobile_number' in update_dct: template = dao_get_template_by_id(current_app.config['TEAM_MEMBER_EDIT_MOBILE_TEMPLATE_ID']) recipient = user_to_update.mobile_number - reply_to = set_reply_to(recipient, template) + reply_to = get_sms_reply_to_for_notify_service(recipient, template) else: return jsonify(data=user_to_update.serialize()), 200 service = Service.query.get(current_app.config['NOTIFY_SERVICE_ID']) @@ -130,7 +130,7 @@ def update_user_attribute(user_id): return jsonify(data=user_to_update.serialize()), 200 -def set_reply_to(recipient, template): +def get_sms_reply_to_for_notify_service(recipient, template): if not is_uk_phone_number(recipient) and use_numeric_sender(recipient): reply_to = current_app.config['NOTIFY_INTERNATIONAL_SMS_SENDER'] else: @@ -276,7 +276,7 @@ def create_2fa_code(template_id, user_to_send_to, secret_code, recipient, person create_user_code(user_to_send_to, secret_code, template.template_type) reply_to = None if template.template_type == SMS_TYPE: - reply_to = set_reply_to(recipient, template) + reply_to = get_sms_reply_to_for_notify_service(recipient, template) elif template.template_type == EMAIL_TYPE: reply_to = template.service.get_default_reply_to_email_address() saved_notification = persist_notification( diff --git a/tests/app/user/test_rest.py b/tests/app/user/test_rest.py index 7c17f9e83..07fa440c2 100644 --- a/tests/app/user/test_rest.py +++ b/tests/app/user/test_rest.py @@ -1086,3 +1086,20 @@ def test_search_for_users_by_email_handles_incorrect_data_format(notify_db, clie assert response.status_code == 400 assert json.loads(response.get_data(as_text=True))['message'] == {'email': ['Not a valid string.']} + + +@pytest.mark.parametrize('number, expected_reply_to', + [ + ("1-403-123-4567", "notify_international_sender"), + ("27 123 4569 2312", "notify_international_sender"), + ("30 123 4567 7890", "Notify"), + ("+20 123 4567 7890", "Notify"), + ]) +def test_get_sms_reply_to_for_notify_service(team_member_mobile_edit_template, number, expected_reply_to): + # need to import locally to avoid db session errors, + # if this import is with the other imports at the top of the file + # the imports happen in the wrong order and you'll see "dummy session" errors + from app.user.rest import get_sms_reply_to_for_notify_service + reply_to = get_sms_reply_to_for_notify_service(number, team_member_mobile_edit_template) + assert reply_to == current_app.config['NOTIFY_INTERNATIONAL_SMS_SENDER'] \ + if expected_reply_to == 'notify_international_sender' else current_app.config['FROM_NUMBER']