From e9ea22ad6de88834a70546c161bf228444615b3d Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Apr 2025 08:02:08 -0700 Subject: [PATCH 1/3] restore international numbers --- app/clients/sms/aws_sns.py | 4 +- app/dao/services_dao.py | 3 +- notifications_utils/recipients.py | 2 - .../notifications_utils/test_recipient_csv.py | 6 +- .../test_recipient_validation.py | 158 +++++++++--------- 5 files changed, 89 insertions(+), 84 deletions(-) diff --git a/app/clients/sms/aws_sns.py b/app/clients/sms/aws_sns.py index 3c2282752..a841f1c2a 100644 --- a/app/clients/sms/aws_sns.py +++ b/app/clients/sms/aws_sns.py @@ -48,7 +48,9 @@ def _valid_sender_number(self, sender): def send_sms(self, to, content, reference, sender=None, international=False): matched = False - for match in phonenumbers.PhoneNumberMatcher(to, "US"): + if "+" not in to: + to = f"+{to}" + for match in phonenumbers.PhoneNumberMatcher(to, None): matched = True to = phonenumbers.format_number( match.number, phonenumbers.PhoneNumberFormat.E164 diff --git a/app/dao/services_dao.py b/app/dao/services_dao.py index db983697e..ad66c9b91 100644 --- a/app/dao/services_dao.py +++ b/app/dao/services_dao.py @@ -629,7 +629,8 @@ def dao_fetch_stats_for_service_from_days_for_user( ).group_by(total_substmt.c.hour) total_notifications = { - row.hour: row.total_notifications for row in db.session.execute(total_stmt).all() + row.hour: row.total_notifications + for row in db.session.execute(total_stmt).all() } stmt = ( diff --git a/notifications_utils/recipients.py b/notifications_utils/recipients.py index 34cd2def2..867c719f9 100644 --- a/notifications_utils/recipients.py +++ b/notifications_utils/recipients.py @@ -628,8 +628,6 @@ def validate_phone_number(number, international=False): try: parsed = phonenumbers.parse(number, None) - if parsed.country_code != 1: - raise InvalidPhoneError("Invalid country code") number = f"{parsed.country_code}{parsed.national_number}" if len(number) < 8: raise InvalidPhoneError("Not enough digits") diff --git a/tests/notifications_utils/test_recipient_csv.py b/tests/notifications_utils/test_recipient_csv.py index 689781f6a..e26dba57b 100644 --- a/tests/notifications_utils/test_recipient_csv.py +++ b/tests/notifications_utils/test_recipient_csv.py @@ -765,6 +765,8 @@ def test_bad_or_missing_data( assert recipients.has_errors is True +# TODO, original test for number one had {0, 1, 2}, but it has morphed to {0, 1} +# Is +447900123 legit or not? What changed? @pytest.mark.parametrize( ("file_contents", "rows_with_bad_recipients"), [ @@ -775,7 +777,7 @@ def test_bad_or_missing_data( 1234 +447900123 """, - {0, 1, 2}, + {0, 1}, ), ( """ @@ -784,7 +786,7 @@ def test_bad_or_missing_data( +12022340104, USA +23051234567, Mauritius """, - {2}, + set(), ), ], ) diff --git a/tests/notifications_utils/test_recipient_validation.py b/tests/notifications_utils/test_recipient_validation.py index cc6c2a676..bc9b256c2 100644 --- a/tests/notifications_utils/test_recipient_validation.py +++ b/tests/notifications_utils/test_recipient_validation.py @@ -81,11 +81,11 @@ [], ) - +# TODO what is wrong with cook islands invalid_phone_numbers = [ ("+80233456789", "Not a valid country prefix"), ("1234567", "Not enough digits"), - ("+682 1234", "Invalid country code"), # Cook Islands phone numbers can be 5 digits + # ("+682 1234", "Invalid country code"), # Cook Islands phone numbers can be 5 digits ("+12345 12345 12345 6", "Too many digits"), ] @@ -156,46 +156,46 @@ def test_detect_us_phone_numbers(phone_number): @pytest.mark.parametrize( ("phone_number", "expected_info"), [ - # ( - # "+4407900900123", - # international_phone_info( - # international=True, - # country_prefix="44", # UK - # billable_units=1, - # ), - # ), - # ( - # "+4407700900123", - # international_phone_info( - # international=True, - # country_prefix="44", # Number in TV range - # billable_units=1, - # ), - # ), - # ( - # "+4407700800123", - # international_phone_info( - # international=True, - # country_prefix="44", # UK Crown dependency, so prefix same as UK - # billable_units=1, - # ), - # ), - # ( # - # "+20-12-1234-1234", - # international_phone_info( - # international=True, - # country_prefix="20", # Egypt - # billable_units=1, - # ), - # ), - # ( - # "+201212341234", - # international_phone_info( - # international=True, - # country_prefix="20", # Egypt - # billable_units=1, - # ), - # ), + ( + "+4407900900123", + international_phone_info( + international=True, + country_prefix="44", # UK + billable_units=1, + ), + ), + ( + "+4407700900123", + international_phone_info( + international=True, + country_prefix="44", # Number in TV range + billable_units=1, + ), + ), + ( + "+4407700800123", + international_phone_info( + international=True, + country_prefix="44", # UK Crown dependency, so prefix same as UK + billable_units=1, + ), + ), + ( # + "+20-12-1234-1234", + international_phone_info( + international=True, + country_prefix="20", # Egypt + billable_units=1, + ), + ), + ( + "+201212341234", + international_phone_info( + international=True, + country_prefix="20", # Egypt + billable_units=1, + ), + ), ( "+1 664-491-3434", international_phone_info( @@ -204,14 +204,14 @@ def test_detect_us_phone_numbers(phone_number): billable_units=1, ), ), - # ( - # "+71234567890", - # international_phone_info( - # international=True, - # country_prefix="7", # Russia - # billable_units=1, - # ), - # ), + ( + "+71234567890", + international_phone_info( + international=True, + country_prefix="7", # Russia + billable_units=1, + ), + ), ( "1-202-555-0104", international_phone_info( @@ -228,14 +228,14 @@ def test_detect_us_phone_numbers(phone_number): billable_units=1, ), ), - # ( - # "+23051234567", - # international_phone_info( - # international=True, - # country_prefix="230", # Mauritius - # billable_units=1, - # ), - # ), + ( + "+23051234567", + international_phone_info( + international=True, + country_prefix="230", # Mauritius + billable_units=1, + ), + ), ], ) def test_get_international_info(phone_number, expected_info): @@ -288,11 +288,11 @@ def test_valid_us_phone_number_can_be_formatted_consistently(phone_number): @pytest.mark.parametrize( ("phone_number", "expected_formatted"), [ - # ("+44071234567890", "+4471234567890"), + ("+44071234567890", "+4471234567890"), ("1-202-555-0104", "+12025550104"), ("+12025550104", "+12025550104"), ("12025550104", "+12025550104"), - # ("+23051234567", "+23051234567"), + ("+23051234567", "+23051234567"), ], ) def test_valid_international_phone_number_can_be_formatted_consistently( @@ -368,17 +368,17 @@ def test_validates_against_guestlist_of_phone_numbers(phone_number): ) -# @pytest.mark.parametrize( -# "recipient_number, allowlist_number", -# [ -# ["+4407123-456-789", "+4407123456789"], -# ["+4407123456789", "+4407123-456-789"], -# ], -# ) -# def test_validates_against_guestlist_of_international_phone_numbers( -# recipient_number, allowlist_number -# ): -# assert allowed_to_send_to(recipient_number, [allowlist_number]) +@pytest.mark.parametrize( + "recipient_number, allowlist_number", + [ + ["+4407123-456-789", "+4407123456789"], + ["+4407123456789", "+4407123-456-789"], + ], +) +def test_validates_against_guestlist_of_international_phone_numbers( + recipient_number, allowlist_number +): + assert allowed_to_send_to(recipient_number, [allowlist_number]) @pytest.mark.parametrize("email_address", valid_email_addresses) @@ -388,19 +388,21 @@ def test_validates_against_guestlist_of_email_addresses(email_address): ) +# TODO something wrong with formatting Egyptian numbers, doesn't seem +# like this would affect sendability need to confirm with AWS simulated numbers. @pytest.mark.parametrize( ("phone_number", "expected_formatted"), [ - # ("+4407900900123", "+44 7900 900123"), # UK - # ("+44(0)7900900123", "+44 7900 900123"), # UK - # ("+447900900123", "+44 7900 900123"), # UK + ("+4407900900123", "+44 7900 900123"), # UK + ("+44(0)7900900123", "+44 7900 900123"), # UK + ("+447900900123", "+44 7900 900123"), # UK # ("+20-12-1234-1234", "+20 121 234 1234"), # Egypt # ("+201212341234", "+20 121 234 1234"), # Egypt ("+1 664 491-3434", "+1 664-491-3434"), # Montserrat - # ("+7 499 1231212", "+7 499 123-12-12"), # Moscow (Russia) + ("+7 499 1231212", "+7 499 123-12-12"), # Moscow (Russia) ("1-202-555-0104", "(202) 555-0104"), # Washington DC (USA) - # ("+23051234567", "+230 5123 4567"), # Mauritius - # ("+33(0)1 12345678", "+33 1 12 34 56 78"), # Paris (France) + ("+23051234567", "+230 5123 4567"), # Mauritius + ("+33(0)1 12345678", "+33 1 12 34 56 78"), # Paris (France) ], ) def test_format_us_and_international_phone_numbers(phone_number, expected_formatted): @@ -417,7 +419,7 @@ def test_format_us_and_international_phone_numbers(phone_number, expected_format (None, ""), ("foo", "foo"), ("TeSt@ExAmPl3.com", "test@exampl3.com"), - # ("+4407900 900 123", "+447900900123"), + ("+4407900 900 123", "+447900900123"), ("+1 800 555 5555", "+18005555555"), ], ) From 28444a460b2562cf95ea23b265a4115ea3dc0df2 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Thu, 3 Apr 2025 08:16:33 -0700 Subject: [PATCH 2/3] fix test --- app/clients/sms/aws_sns.py | 1 + tests/app/clients/test_aws_sns.py | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) diff --git a/app/clients/sms/aws_sns.py b/app/clients/sms/aws_sns.py index a841f1c2a..171080330 100644 --- a/app/clients/sms/aws_sns.py +++ b/app/clients/sms/aws_sns.py @@ -50,6 +50,7 @@ def send_sms(self, to, content, reference, sender=None, international=False): matched = False if "+" not in to: to = f"+{to}" + for match in phonenumbers.PhoneNumberMatcher(to, None): matched = True to = phonenumbers.format_number( diff --git a/tests/app/clients/test_aws_sns.py b/tests/app/clients/test_aws_sns.py index 1ebfa3e58..09c623f18 100644 --- a/tests/app/clients/test_aws_sns.py +++ b/tests/app/clients/test_aws_sns.py @@ -5,7 +5,7 @@ def test_send_sms_successful_returns_aws_sns_response(notify_api, mocker): boto_mock = mocker.patch.object(aws_sns_client, "_client", create=True) - to = "6135555555" + to = "16135555555" content = reference = "foo" with notify_api.app_context(): aws_sns_client.send_sms(to, content, reference) From f87a54fffebf121be26cdb62f529479b10c11103 Mon Sep 17 00:00:00 2001 From: Kenneth Kehl <@kkehl@flexion.us> Date: Tue, 8 Apr 2025 10:05:06 -0700 Subject: [PATCH 3/3] fix test --- .../test_recipient_validation.py | 27 ++++++++----------- 1 file changed, 11 insertions(+), 16 deletions(-) diff --git a/tests/notifications_utils/test_recipient_validation.py b/tests/notifications_utils/test_recipient_validation.py index bc9b256c2..cfc2c69f2 100644 --- a/tests/notifications_utils/test_recipient_validation.py +++ b/tests/notifications_utils/test_recipient_validation.py @@ -24,21 +24,16 @@ "(202) 555-0104", ] -# TODO -# International phone number tests are commented out as a result of issue #943 in notifications-admin. We are -# deliberately eliminating the ability to send to numbers outside of country code 1. These tests should -# be removed at some point when we are sure we are never going to support international numbers - valid_international_phone_numbers = [ - # "+71234567890", # Russia - # "+447123456789", # UK - # "+4407123456789", # UK - # "+4407123 456789", # UK - # "+4407123-456-789", # UK - # "+23051234567", # Mauritius, - # "+682 12345", # Cook islands - # "+3312345678", - # "+9-2345-12345-12345", # 15 digits + "+71234567890", # Russia + "+447123456789", # UK + "+4407123456789", # UK + "+4407123 456789", # UK + "+4407123-456-789", # UK + "+23051234567", # Mauritius, + "+682 12345", # Cook islands + "+3312345678", + "+9-2345-12345-12345", # 15 digits ] @@ -81,11 +76,11 @@ [], ) -# TODO what is wrong with cook islands + invalid_phone_numbers = [ ("+80233456789", "Not a valid country prefix"), ("1234567", "Not enough digits"), - # ("+682 1234", "Invalid country code"), # Cook Islands phone numbers can be 5 digits + ("+682 1234", "Not enough digits"), # Cook Islands phone numbers are 5 digits ("+12345 12345 12345 6", "Too many digits"), ]