Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Landline validation #4131

Draft
wants to merge 14 commits into
base: main
Choose a base branch
from
13 changes: 0 additions & 13 deletions app/constants.py
Original file line number Diff line number Diff line change
Expand Up @@ -330,16 +330,3 @@ class CacheKeys:

# Admin API error codes
QR_CODE_TOO_LONG = "qr-code-too-long"


# We updated the content for phone number validation messages in https://github.com/alphagov/notifications-utils/pull/1054,
# but these are returned from our API. We don't want to make any breaking changes, so we will map them back to our
# original errors.
# We can decide as/when we want to remove this and update the messages to end users.
PHONE_NUMBER_VALIDATION_ERROR_MAP = {
"Mobile numbers can only include: 0 1 2 3 4 5 6 7 8 9 ( ) + -": "Must not contain letters or symbols",
"Mobile number is too long": "Too many digits",
"Mobile number is too short": "Not enough digits",
"Country code not found - double check the mobile number you entered": "Not a valid country prefix",
"This does not look like a UK mobile number – double check the mobile number you entered": "Not a UK mobile number",
}
5 changes: 0 additions & 5 deletions app/notifications/validators.py
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,6 @@
KEY_TYPE_TEAM,
KEY_TYPE_TEST,
LETTER_TYPE,
PHONE_NUMBER_VALIDATION_ERROR_MAP,
SMS_TYPE,
)
from app.dao.service_email_reply_to_dao import dao_get_reply_to_by_id
Expand Down Expand Up @@ -277,7 +276,3 @@ def check_template_can_contain_documents(template_type, personalisation):
isinstance(v, dict) and "file" in v for v in (personalisation or {}).values()
):
raise BadRequestError(message="Can only send a file by email")


def remap_phone_number_validation_messages(error_message):
return PHONE_NUMBER_VALIDATION_ERROR_MAP.get(error_message, error_message)
14 changes: 7 additions & 7 deletions app/schema_validation/__init__.py
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,7 @@
from jsonschema import Draft7Validator, FormatChecker, ValidationError
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 validate_phone_number

from app.notifications.validators import remap_phone_number_validation_messages
from notifications_utils.recipient_validation.phone_number import PhoneNumber

format_checker = FormatChecker()

Expand All @@ -21,13 +19,15 @@ def validate_uuid(instance):
return True


@format_checker.checks("phone_number", raises=InvalidPhoneError)
@format_checker.checks("phone_number", raises=ValidationError)
def validate_schema_phone_number(instance):

if isinstance(instance, str):
try:
validate_phone_number(instance, international=True)
except InvalidPhoneError as error:
raise InvalidPhoneError(remap_phone_number_validation_messages(str(error))) from error
PhoneNumber(instance, allow_international=True)
except InvalidPhoneError as e:
legacy_message = e.get_legacy_v2_api_error_message()
raise ValidationError(legacy_message) from None
return True


Expand Down
10 changes: 3 additions & 7 deletions app/schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@
from app import db, ma, models
from app.dao.permissions_dao import permission_dao
from app.models import ServicePermission
from app.notifications.validators import remap_phone_number_validation_messages
from app.utils import DATETIME_FORMAT_NO_TIMEZONE, get_template_instance


Expand Down Expand Up @@ -134,8 +133,7 @@ def validate_mobile_number(self, value):
if value is not None:
validate_phone_number(value, international=True)
except InvalidPhoneError as error:
error_message = remap_phone_number_validation_messages(str(error))
raise ValidationError(f"Invalid phone number: {error_message}") from error
raise ValidationError(f"Invalid phone number: {error.get_legacy_v2_api_error_message()}") from error


class UserUpdateAttributeSchema(BaseSchema):
Expand Down Expand Up @@ -175,8 +173,7 @@ def validate_mobile_number(self, value):
if value is not None:
validate_phone_number(value, international=True)
except InvalidPhoneError as error:
error_message = remap_phone_number_validation_messages(str(error))
raise ValidationError(f"Invalid phone number: {error_message}") from error
raise ValidationError(f"Invalid phone number: {error.get_legacy_v2_api_error_message()}") from error

@validates_schema(pass_original=True)
def check_unknown_fields(self, data, original_data, **kwargs):
Expand Down Expand Up @@ -578,8 +575,7 @@ def validate_to(self, value):
try:
validate_phone_number(value, international=True)
except InvalidPhoneError as error:
error_message = remap_phone_number_validation_messages(str(error))
raise ValidationError(f"Invalid phone number: {error_message}") from error
raise ValidationError(f"Invalid phone number: {error.get_legacy_v2_api_error_message()}") from error

@post_load
def format_phone_number(self, item, **kwargs):
Expand Down
15 changes: 8 additions & 7 deletions app/v2/errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

from flask import current_app, jsonify, request
from jsonschema import ValidationError as JsonSchemaValidationError
from notifications_utils.recipient_validation.errors import InvalidPhoneError, InvalidRecipientError
from notifications_utils.recipient_validation.errors import InvalidRecipientError
from sqlalchemy.exc import DataError
from sqlalchemy.orm.exc import NoResultFound

Expand Down Expand Up @@ -85,14 +85,15 @@ def to_dict_v2(self):
def register_errors(blueprint):
@blueprint.errorhandler(InvalidRecipientError)
def invalid_format(error):
if isinstance(error, InvalidPhoneError):
from app.notifications.validators import remap_phone_number_validation_messages

error = InvalidPhoneError(remap_phone_number_validation_messages(str(error)))

current_app.logger.info(error)

return jsonify(status_code=400, errors=[{"error": error.__class__.__name__, "message": str(error)}]), 400
return (
jsonify(
status_code=400,
errors=[{"error": error.__class__.__name__, "message": error.get_legacy_v2_api_error_message()}],
),
400,
)

@blueprint.errorhandler(InvalidRequest)
def invalid_data(error):
Expand Down
2 changes: 1 addition & 1 deletion requirements.in
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,7 @@ lxml==4.9.3

notifications-python-client==8.0.1

notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@80.0.1
notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@81.1.1

# gds-metrics requires prometheseus 0.2.0, override that requirement as 0.7.1 brings significant performance gains
prometheus-client==0.14.1
Expand Down
2 changes: 1 addition & 1 deletion requirements.txt
Original file line number Diff line number Diff line change
Expand Up @@ -151,7 +151,7 @@ mistune==0.8.4
# via notifications-utils
notifications-python-client==8.0.1
# via -r requirements.in
notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@80.0.1
notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@81.1.1
# via -r requirements.in
ordered-set==4.1.0
# via notifications-utils
Expand Down
9 changes: 7 additions & 2 deletions tests/app/v2/notifications/test_notification_schemas.py
Original file line number Diff line number Diff line change
Expand Up @@ -120,6 +120,11 @@ def test_post_sms_schema_is_valid(input):
assert validate(input, post_sms_request_schema) == input


def test_post_sms_schema_is_valid_for_landline_if_service_can_send_to_landlines():
sms_data = {"phone_number": "0117 496 0860", "template_id": str(uuid.uuid4())}
assert validate(sms_data, post_sms_request_schema) == sms_data


@pytest.mark.parametrize(
"template_id",
[
Expand Down Expand Up @@ -175,7 +180,7 @@ def test_post_sms_schema_with_personalisation_that_is_not_a_dict():
@pytest.mark.parametrize(
"invalid_phone_number, err_msg",
[
("08515111111", "phone_number Not a UK mobile number"),
("08515111111", "phone_number Number is not valid – double check the phone number you entered"),
("07515111*11", "phone_number Must not contain letters or symbols"),
("notaphoneumber", "phone_number Must not contain letters or symbols"),
(7700900001, "phone_number 7700900001 is not of type string"),
Expand All @@ -201,7 +206,7 @@ def test_post_sms_request_schema_invalid_phone_number_and_missing_template():
validate(j, post_sms_request_schema)
errors = json.loads(str(e.value)).get("errors")
assert len(errors) == 2
assert {"error": "ValidationError", "message": "phone_number Not a UK mobile number"} in errors
assert {"error": "ValidationError", "message": "phone_number Number is not valid – double check the phone number you entered"} in errors
assert {"error": "ValidationError", "message": "template_id is a required property"} in errors


Expand Down
2 changes: 1 addition & 1 deletion tests/app/v2/notifications/test_post_notifications.py
Original file line number Diff line number Diff line change
Expand Up @@ -840,7 +840,7 @@ def test_post_sms_notification_returns_400_if_number_not_in_guest_list(
template = create_template(service=service)

data = {
"phone_number": "+327700900855",
"phone_number": "+3225484211",
"template_id": template.id,
}

Expand Down
19 changes: 6 additions & 13 deletions tests/app/v2/test_errors.py
Original file line number Diff line number Diff line change
Expand Up @@ -49,14 +49,7 @@ def raising_data_error():

@blue.route("raise_phone_error/<error_id>", methods=["GET"])
def raising_invalid_phone_error(error_id):
errors = {
"symbol": "Mobile numbers can only include: 0 1 2 3 4 5 6 7 8 9 ( ) + -",
"too-short": "Mobile number is too long",
"too-long": "Mobile number is too short",
"invalid-country": "Country code not found - double check the mobile number you entered",
"invalid-uk": "This does not look like a UK mobile number – double check the mobile number you entered",
}
raise InvalidPhoneError(errors[error_id])
raise InvalidPhoneError(code=error_id)

@blue.route("raise_exception", methods=["GET"])
def raising_exception():
Expand Down Expand Up @@ -151,11 +144,11 @@ def test_bad_method(app_for_test):
@pytest.mark.parametrize(
"error_id, expected_response",
(
("symbol", "Must not contain letters or symbols"),
("too-short", "Too many digits"),
("too-long", "Not enough digits"),
("invalid-country", "Not a valid country prefix"),
("invalid-uk", "Not a UK mobile number"),
(InvalidPhoneError.Codes.UNKNOWN_CHARACTER, "Must not contain letters or symbols"),
(InvalidPhoneError.Codes.TOO_LONG, "Too many digits"),
(InvalidPhoneError.Codes.TOO_SHORT, "Not enough digits"),
(InvalidPhoneError.Codes.UNSUPPORTED_COUNTRY_CODE, "Not a valid country prefix"),
(InvalidPhoneError.Codes.NOT_A_UK_MOBILE, "Not a UK mobile number"),
),
)
def test_invalid_phone_error(app_for_test, error_id, expected_response):
Expand Down