Merge pull request #5294 from alphagov/remove-old-phonenumber-validation

Remove old phonenumber validation

.pre-commit-config.yaml

@@ -1,4 +1,4 @@
-# This file was automatically copied from notifications-utils@94.0.1
+# This file was automatically copied from notifications-utils@95.0.0
 
 repos:
 - repo: https://github.com/pre-commit/pre-commit-hooks

app/__init__.py

@@ -32,13 +32,14 @@
     get_lines_with_normalised_whitespace,
 )
 from notifications_utils.logging import flask as utils_logging
-from notifications_utils.recipient_validation.phone_number import format_phone_number_human_readable
 from notifications_utils.safe_string import make_string_safe_for_email_local_part, make_string_safe_for_id
 from notifications_utils.sanitise_text import SanitiseASCII
 from werkzeug.exceptions import HTTPException as WerkzeugHTTPException
 from werkzeug.exceptions import abort
 from werkzeug.local import LocalProxy
 
+from app.formatters import format_phone_number_human_readable
+
 # must be declared before rest of app is imported to satisfy circular import
 # ruff: noqa: E402
 memo_resetters: list[Callable] = []

app/formatters.py

@@ -12,7 +12,7 @@
 from notifications_utils.field import Field
 from notifications_utils.formatters import make_quotes_smart
 from notifications_utils.formatters import nl2br as utils_nl2br
-from notifications_utils.recipient_validation.phone_number import InvalidPhoneError, validate_phone_number
+from notifications_utils.recipient_validation.phone_number import InvalidPhoneError, PhoneNumber
 from notifications_utils.take import Take
 from notifications_utils.timezones import utc_string_to_aware_gmt_datetime
 
@@ -139,7 +139,7 @@ def format_delta_days(date, numeric_prefix=""):
 
 def valid_phone_number(phone_number):
     try:
-        validate_phone_number(phone_number)
+        PhoneNumber(phone_number)
         return True
     except InvalidPhoneError:
         return False
@@ -441,3 +441,12 @@ def extract_path_from_url(url):
 def sentence_case(sentence):
     first_word, rest_of_sentence = (sentence + " ").split(" ", 1)
     return f"{first_word.title()} {rest_of_sentence}"[:-1]
+
+
+def format_phone_number_human_readable(number):
+    try:
+        phone_number = PhoneNumber(number)
+    except InvalidPhoneError:
+        # if there was a validation error, we want to shortcut out here, but still display the number on the front end
+        return number
+    return phone_number.get_human_readable_format()

app/main/forms.py

@@ -18,7 +18,7 @@
 from notifications_utils.insensitive_dict import InsensitiveDict
 from notifications_utils.recipient_validation.email_address import validate_email_address
 from notifications_utils.recipient_validation.errors import InvalidEmailError, InvalidPhoneError
-from notifications_utils.recipient_validation.phone_number import normalise_phone_number
+from notifications_utils.recipient_validation.phone_number import PhoneNumber as PhoneNumberUtils
 from notifications_utils.recipient_validation.postal_address import PostalAddress
 from notifications_utils.safe_string import make_string_safe_for_email_local_part
 from ordered_set import OrderedSet
@@ -1805,12 +1805,16 @@ def validate(self, *args, **kwargs):
             # and disallow emergency 3-digit numbers
             def valid_non_emergency_phone_number(self, num):
                 try:
-                    normalised_number = normalise_phone_number(num.data)
+                    PhoneNumberUtils(num.data, is_service_contact_number=True)
                 except InvalidPhoneError as e:
-                    raise ValidationError("Enter a phone number in the correct format") from e
-
-                if normalised_number in {"999", "112"}:
-                    raise ValidationError("Phone number cannot be an emergency number")
+                    if e.code == InvalidPhoneError.Codes.UNSUPPORTED_EMERGENCY_NUMBER:
+                        raise ValidationError(str(e)) from e
+                    elif e.code == InvalidPhoneError.Codes.TOO_LONG:
+                        # assume the number is an extension and return the number with minimal normalisation
+                        return True
+
+                    else:
+                        raise ValidationError("Enter a phone number in the correct format") from e
 
                 return True
 

app/main/validators.py

@@ -7,7 +7,7 @@
 from notifications_utils.formatters import formatted_list
 from notifications_utils.recipient_validation.email_address import validate_email_address
 from notifications_utils.recipient_validation.errors import InvalidEmailError, InvalidPhoneError
-from notifications_utils.recipient_validation.phone_number import PhoneNumber, validate_phone_number
+from notifications_utils.recipient_validation.phone_number import PhoneNumber
 from notifications_utils.sanitise_text import SanitiseSMS
 from ordered_set import OrderedSet
 from wtforms import ValidationError
@@ -95,19 +95,17 @@ def __init__(
         InvalidPhoneError.Codes.NOT_A_UK_MOBILE: "%s does not look like a UK mobile number",
         InvalidPhoneError.Codes.UNSUPPORTED_COUNTRY_CODE: "Country code for %s not found",
         InvalidPhoneError.Codes.UNKNOWN_CHARACTER: "%s can only include: 0 1 2 3 4 5 6 7 8 9 ( ) + -",
+        InvalidPhoneError.Codes.INVALID_NUMBER: "%s is not valid – double check the phone number you entered",
     }
 
     def __call__(self, form, field):
         try:
             if field.data:
-                if self.allow_sms_to_uk_landlines:
-                    number = PhoneNumber(field.data)
-                    number.validate(
-                        allow_international_number=self.allow_international_sms,
-                        allow_uk_landline=self.allow_sms_to_uk_landlines,
-                    )
-                else:
-                    validate_phone_number(field.data, international=self.allow_international_sms)
+                number = PhoneNumber(field.data)
+                number.validate(
+                    allow_international_number=self.allow_international_sms,
+                    allow_uk_landline=self.allow_sms_to_uk_landlines,
+                )
         except InvalidPhoneError as e:
             error_message = str(e)
             if hasattr(field, "error_summary_messages"):

app/main/views/conversation.py

@@ -1,10 +1,10 @@
 from flask import jsonify, redirect, render_template, request, session, url_for
 from flask_login import current_user
 from notifications_python_client.errors import HTTPError
-from notifications_utils.recipient_validation.phone_number import format_phone_number_human_readable
 from notifications_utils.template import SMSPreviewTemplate
 
 from app import current_service, notification_api_client, service_api_client
+from app.formatters import format_phone_number_human_readable
 from app.main import json_updates, main
 from app.main.forms import SearchByNameForm
 from app.models.notification import InboundSMSMessage, InboundSMSMessages, Notifications

app/main/views/dashboard.py

@@ -5,7 +5,6 @@
 
 from flask import Response, abort, jsonify, render_template, request, session, url_for
 from flask_login import current_user
-from notifications_utils.recipient_validation.phone_number import format_phone_number_human_readable
 from werkzeug.utils import redirect
 
 from app import (
@@ -17,7 +16,7 @@
 )
 from app.constants import MAX_NOTIFICATION_FOR_DOWNLOAD
 from app.extensions import redis_client
-from app.formatters import format_date_numeric, format_datetime_numeric
+from app.formatters import format_date_numeric, format_datetime_numeric, format_phone_number_human_readable
 from app.main import json_updates, main
 from app.main.forms import SearchNotificationsForm
 from app.models.notification import InboundSMSMessages, Notifications

app/main/views/send.py

@@ -600,7 +600,6 @@ def _check_messages(service_id, template_id, upload_id, preview_row):
     except HTTPError as e:
         if e.status_code != 404:
             raise
-
     contents = s3download(service_id, upload_id)
 
     template = current_service.get_template_with_user_permission_or_403(
@@ -625,7 +624,6 @@ def _check_messages(service_id, template_id, upload_id, preview_row):
     elif template.template_type == "sms":
         template.sender = get_sms_sender_from_session()
         template.show_sender = bool(template.sender)
-
     recipients = RecipientCSV(
         contents,
         template=template,
@@ -642,8 +640,8 @@ def _check_messages(service_id, template_id, upload_id, preview_row):
         allow_international_sms=current_service.has_permission("international_sms"),
         allow_sms_to_uk_landline=current_service.has_permission("sms_to_uk_landlines"),
         allow_international_letters=current_service.has_permission("international_letters"),
+        should_validate_phone_number=False,
     )
-
     if request.args.get("from_test"):
         # only happens if generating a letter preview test
         back_link = url_for("main.send_one_off", service_id=service_id, template_id=template.id)
@@ -659,9 +657,7 @@ def _check_messages(service_id, template_id, upload_id, preview_row):
         template.values = recipients[preview_row - 2].recipient_and_personalisation
     elif preview_row > 2:
         abort(404)
-
     original_file_name = get_csv_metadata(service_id, upload_id).get("original_file_name", "")
-
     return {
         "recipients": recipients,
         "template": template,
@@ -699,7 +695,6 @@ def _check_messages(service_id, template_id, upload_id, preview_row):
 def check_messages(service_id, template_id, upload_id, row_index=2):
     data = _check_messages(service_id, template_id, upload_id, row_index)
     data["allowed_file_extensions"] = Spreadsheet.ALLOWED_FILE_EXTENSIONS
-
     if (
         data["recipients"].too_many_rows
         or not data["count_of_recipients"]

requirements.in

@@ -17,7 +17,7 @@ notifications-python-client==10.0.0
 fido2==1.1.3
 
 # Run `make bump-utils` to update to the latest version
-notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@94.0.1
+notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@95.0.0
 
 govuk-frontend-jinja==3.4.0
 

requirements.txt

@@ -103,15 +103,15 @@ mistune==0.8.4
     # via notifications-utils
 notifications-python-client==10.0.0
     # via -r requirements.in
-notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@d6311c3ab83b8225265277f781157c48f9318ae9
+notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@e89dcc53c7f88ef23053d1efb2c00b8ef2ea377e
     # via -r requirements.in
 openpyxl==3.0.10
     # via pyexcel-xlsx
 ordered-set==4.1.0
     # via notifications-utils
 packaging==23.1
     # via gunicorn
-phonenumbers==8.13.52
+phonenumbers==8.13.50
     # via notifications-utils
 pillow==10.3.0
     # via -r requirements.in

requirements_for_test.txt

@@ -169,7 +169,7 @@ moto==4.0.9
     # via -r requirements_for_test.in
 notifications-python-client==10.0.0
     # via -r requirements.txt
-notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@d6311c3ab83b8225265277f781157c48f9318ae9
+notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@e89dcc53c7f88ef23053d1efb2c00b8ef2ea377e
     # via -r requirements.txt
 openpyxl==3.0.10
     # via
@@ -184,7 +184,9 @@ packaging==23.1
     #   -r requirements.txt
     #   gunicorn
     #   pytest
-phonenumbers==8.13.52
+pathspec==0.12.1
+    # via black
+phonenumbers==8.13.50
     # via
     #   -r requirements.txt
     #   notifications-utils

requirements_for_test_common.in

@@ -1,4 +1,4 @@
-# This file was automatically copied from notifications-utils@94.0.1
+# This file was automatically copied from notifications-utils@95.0.0
 
 beautifulsoup4==4.12.3
 pytest==8.3.4

ruff.toml

@@ -1,4 +1,4 @@
-# This file was automatically copied from notifications-utils@94.0.1
+# This file was automatically copied from notifications-utils@95.0.0
 
 exclude = [
     "migrations/versions/",

tests/__init__.py

@@ -426,7 +426,7 @@ def inbound_sms_json():
                 (4, 5, "33(0)1 12345678"),  # France
                 (5, 7, "1-202-555-0104"),  # USA in one format
                 (6, 9, "+12025550104"),  # USA in another format
-                (7, 9, "+68212345"),  # Cook Islands
+                (7, 9, "+68223001"),  # Cook Islands
             )
         ],
     }

tests/app/main/forms/test_service_contact_details_form.py

@@ -113,7 +113,6 @@ def test_form_phone_number_allows_non_emergency_3_digit_numbers(notify_admin, sh
 
     with notify_admin.test_request_context(method="POST", data=data):
         form = ServiceContactDetailsForm()
-
         if allowed:
             assert form.validate_on_submit()
             assert len(form.errors) == 0
@@ -122,3 +121,17 @@ def test_form_phone_number_allows_non_emergency_3_digit_numbers(notify_admin, sh
             assert not form.validate_on_submit()
             assert len(form.errors) == 1
             assert form.errors["phone_number"] == ["Phone number cannot be an emergency number"]
+
+
+@pytest.mark.parametrize(
+    "short_number, allowed",
+    (("01572 812241 7081", True),),
+)
+def test_form_phone_number_allows_non_emergency_numbers_with_extensions(notify_admin, short_number, allowed):
+    data = {"contact_details_type": "phone_number", "phone_number": short_number}
+
+    with notify_admin.test_request_context(method="POST", data=data):
+        form = ServiceContactDetailsForm()
+        assert form.validate_on_submit()
+        assert len(form.errors) == 0
+        assert form.errors == {}

tests/app/main/test_validators.py

@@ -91,7 +91,7 @@ def test_invalid_list_of_white_list_email_domains(
 def test_uk_mobile_number_validation_messages_match(mocker):
     mock_field = _gen_mock_field("notanumber", error_summary_messages=[])
     mocker.patch(
-        "app.main.validators.validate_phone_number",
+        "app.main.validators.PhoneNumber",
         side_effect=InvalidPhoneError(code=InvalidPhoneError.Codes.UNKNOWN_CHARACTER),
     )
     with pytest.raises(ValidationError) as error:

tests/app/main/views/service_settings/test_service_settings.py

@@ -4917,7 +4917,7 @@ def test_send_files_by_email_contact_details_prefills_the_form_with_the_existing
     [
         ("url", "http://example.com/", "http://new-link.com/"),
         ("email_address", "[email protected]", "[email protected]"),
-        ("phone_number", "0207 12345", "0207 56789"),
+        ("phone_number", "020 3451 9002", "020 3451 9001"),
     ],
 )
 def test_send_files_by_email_contact_details_updates_contact_details_and_redirects_to_settings_page(

tests/app/main/views/test_api_integration.py

@@ -558,8 +558,8 @@ def test_should_update_guestlist(
     data = {
         "email_addresses-1": "[email protected]",
         "email_addresses-3": "[email protected]",
-        "phone_numbers-0": "07900900000",
-        "phone_numbers-2": "+1800-555-555",
+        "phone_numbers-0": "07988057616",
+        "phone_numbers-2": "+1 202-555-0104",
     }
 
     client_request.post(
@@ -572,7 +572,7 @@ def test_should_update_guestlist(
         SERVICE_ONE_ID,
         {
             "email_addresses": ["[email protected]", "[email protected]"],
-            "phone_numbers": ["07900900000", "+1800-555-555"],
+            "phone_numbers": ["07988057616", "+1 202-555-0104"],
         },
     )
 

tests/app/main/views/test_code_not_received.py

@@ -77,7 +77,7 @@ def test_should_render_correct_resend_template_for_pending_user(
     "phone_number_to_register_with",
     [
         "+447700900460",
-        "+1800-555-555",
+        "+1 202-555-0104",
     ],
 )
 def test_should_resend_verify_code_and_update_mobile_for_pending_user(

tests/app/main/views/test_conversation.py

@@ -40,9 +40,9 @@ def test_get_user_phone_number_when_only_outbound_exists(notify_admin, mocker):
         side_effect=HTTPError(response=Mock(status_code=404)),
     )
     mock_get_notification = mocker.patch(
-        "app.main.views.conversation.notification_api_client.get_notification", return_value={"to": "15550000000"}
+        "app.main.views.conversation.notification_api_client.get_notification", return_value={"to": "14157711401"}
     )
-    assert get_user_number("service", "notification") == "+1 555-000-0000"
+    assert get_user_number("service", "notification") == "+1 415-771-1401"
     mock_get_inbound_sms.assert_called_once_with("service", "notification")
     mock_get_notification.assert_called_once_with("service", "notification")
 

tests/app/main/views/test_dashboard.py

@@ -269,7 +269,7 @@ def test_inbound_messages_shows_count_of_messages_when_there_are_no_messages(
             "+33 1 12 34 56 78 message-5 5 hours ago",
             "+1 202-555-0104 message-6 7 hours ago",
             "+1 202-555-0104 message-7 9 hours ago",
-            "+682 12345 message-8 9 hours ago",
+            "+682 23 001 message-8 9 hours ago",
         ]
     ),
 )
@@ -437,7 +437,7 @@ def test_download_inbox(
         "+33 1 12 34 56 78,message-5,2016-07-01 08:59\r\n"
         "+1 202-555-0104,message-6,2016-07-01 06:59\r\n"
         "+1 202-555-0104,message-7,2016-07-01 04:59\r\n"
-        "+682 12345,message-8,2016-07-01 04:59\r\n"
+        "+682 23 001,message-8,2016-07-01 04:59\r\n"
     )
 
 

tests/app/main/views/test_register.py

@@ -38,7 +38,7 @@ def test_logged_in_user_redirects_to_account(
     "phone_number_to_register_with",
     [
         "+4407700900460",
-        "+1800-555-555",
+        "+1 202-555-0104",
     ],
 )
 @pytest.mark.parametrize(