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.

Sign up for GitHub

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

Jump to bottom

Remove old phonenumber validation #5294

Merged
merged 4 commits into from
Feb 5, 2025
Merged

Remove old phonenumber validation #5294

merged 4 commits into from
Feb 5, 2025

Conversation

rparke
Copy link
Contributor

@rparke rparke commented Nov 12, 2024

No description provided.

@rparke rparke force-pushed the remove-old-phonenumber-validation branch from 07014d8 to f096108 Compare November 12, 2024 13:54
CrystalPea
CrystalPea reviewed Nov 13, 2024
View reviewed changes
Copy link
Contributor

@CrystalPea CrystalPea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey, nice work!

I think we just need to look into there 3-digit numbers for service contact details - as we probably still want to support these?

tests/app/main/forms/test_service_contact_details_form.py Outdated
@pytest.mark.parametrize(
"short_number, allowed",
(
("119", True),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think the question should be - are there valid cases where service's contact number is a three-digit number?

It looks like it might be from this list? https://www.inyourarea.co.uk/news/10-three-digit-phone-numbers-and-what-theyre-for

For example Track and Trace had a 3-digit number, and there are some other ones.

I wonder how we previously validated this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is an easy fix, and it's an exception to pure libphonenumber validation I'm okay with adding. We can either have an allow list, or just say "any 3 digit number that successfully parses to a "possible" UK number, we are just going to say is valid"

@rparke rparke force-pushed the remove-old-phonenumber-validation branch 2 times, most recently from bf3a83f to 6fa938c Compare November 20, 2024 18:57
rparke
rparke commented Nov 21, 2024
View reviewed changes
requirements.in Outdated
@@ -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@f676a72f212ba9d8b1e8cc4dc5fd3211a2214960
notifications-utils @ git+https://github.com/alphagov/notifications-utils.git@de4d6573a0dfb5969156b5a7595f00c54f492cc0
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

needs updating to semver'd tag once that has been merged

CrystalPea
CrystalPea approved these changes Nov 22, 2024
View reviewed changes
Copy link
Contributor

@CrystalPea CrystalPea left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great, nicely done with contact phone numbers validation 🙌🏼

Ready to merge when utils are merged and requirements updated 👍🏼

@rparke rparke force-pushed the remove-old-phonenumber-validation branch 3 times, most recently from 6f49f33 to 2e3ef21 Compare November 25, 2024 13:12
@rparke rparke marked this pull request as ready for review November 25, 2024 13:12
@rparke rparke force-pushed the remove-old-phonenumber-validation branch from 2e3ef21 to b951ef6 Compare November 28, 2024 16:06
@rparke rparke force-pushed the remove-old-phonenumber-validation branch from 145b99f to b2787c3 Compare January 10, 2025 16:53
quis
quis reviewed Jan 30, 2025
View reviewed changes
app/utils/validation.py Outdated Show resolved Hide resolved
@rparke rparke force-pushed the remove-old-phonenumber-validation branch from 4b47593 to d1bbfb0 Compare January 31, 2025 10:23
@rparke rparke force-pushed the remove-old-phonenumber-validation branch from d1bbfb0 to 965511a Compare January 31, 2025 16:21
rparke added 3 commits January 31, 2025 17:17
@rparke
Replaced old phonenumber validation code with new libphonenumber back…
44134f5 
…ed code

utils 95.0.0 removes the old custom phone number validation code and replaces it with a rich class backed against libphonenumbe. This makes our phone number validation more readable and more robust.
@rparke
Updated tests for libphonenumber backed phone number validation
7d756c5 
Some of the numbers used in our unit tests are actually invalid when validated against libphonenumbers metadata catalogues. These have been changed to actually valid numbers to make sure our tests pass. Also adds a test for non-emergency numbers with extensions as libphonenumber does not natively accept extensions and these need to be supported for emergency contact lists.
@rparke
Handle invalid numbers when sending to emergency contact lists
5c45eb0 
legacy emergency contact lists are known to contain invalid numbers. We do not want this to cause sending to the entire list to fail, so this will bypass validation against libphonenumber entirely for emergency contact lists while ensuring new uploads of all CSVs are validated properly
@rparke rparke force-pushed the remove-old-phonenumber-validation branch from 965511a to 5c45eb0 Compare January 31, 2025 17:21
@rparke rparke merged commit ebe2137 into main Feb 5, 2025
3 checks passed
@rparke rparke deleted the remove-old-phonenumber-validation branch February 5, 2025 13:47
rparke added a commit that referenced this pull request Feb 5, 2025
@rparke
Revert "Merge pull request #5294 from alphagov/remove-old-phonenumber…
726c7b6 
…-validation"

This reverts commit ebe2137, reversing
changes made to 88f8ec5.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@quis quis quis left review comments

@CrystalPea CrystalPea CrystalPea approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@rparke @quis @CrystalPea