-
Notifications
You must be signed in to change notification settings - Fork 3
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
🎨 [#235] improve the way we validate phonenumbers #284
Conversation
5fb8a86
to
b4fa414
Compare
@@ -74,25 +74,37 @@ def test_validate_postal_code(self): | |||
self.assertIsNone(validate_postal_code("1015 cJ")) | |||
|
|||
def test_validate_phone_number(self): | |||
valid_phone_numbers = [ | |||
"+31612345678", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would/should something like 0612345678
still be valid?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, and I added it to the test to proof it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think this also requires validation on DigitaalAdres.adres
if soort_digitaal_adres
is telefoonnummer
(similar approach as done for emails #271)
@stevenbal I replaced the |
…tor, and added phonenumber validation to it.
ab31581
to
34b1ea4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Almost there! Whenever those subtests are split up this can be merged if you ask me.
@@ -123,10 +123,10 @@ def test_digitaal_adres_inline(self): | |||
@disable_admin_mfa() | |||
class DigitaalAdresAdminTests(WebTest): | |||
@tag("gh-234") | |||
def test_email_validation(self): | |||
def test_soort_digitaal_adres_validation(self): |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@bart-maykin could you split the three subtests into their own tests?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Time to press the green button 👍
Fixes #235
Changes
Improved the way we validate phonenumbers with the phonenumbers library