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

[#1558] Add phonenumber validation #695

Merged

Conversation

vaszig
Copy link
Contributor

@vaszig vaszig commented Jun 23, 2023

No description provided.

@vaszig vaszig force-pushed the feature/1558-add-phonenumber-validation-to-contactform branch 3 times, most recently from 8a6e1ce to a2d3bc4 Compare June 26, 2023 07:54
@codecov-commenter
Copy link

codecov-commenter commented Jun 26, 2023

Codecov Report

Merging #695 (c900c85) into develop (604e946) will increase coverage by 0.00%.
The diff coverage is 97.18%.

@@            Coverage Diff            @@
##           develop     #695    +/-   ##
=========================================
  Coverage    96.20%   96.20%            
=========================================
  Files          639      642     +3     
  Lines        22799    22934   +135     
=========================================
+ Hits         21934    22064   +130     
- Misses         865      870     +5     
Impacted Files Coverage Δ
src/open_inwoner/utils/validators.py 84.14% <92.59%> (+4.14%) ⬆️
src/open_inwoner/accounts/forms.py 97.75% <100.00%> (-0.05%) ⬇️
...accounts/migrations/0063_alter_user_phonenumber.py 100.00% <100.00%> (ø)
src/open_inwoner/accounts/models.py 97.66% <100.00%> (ø)
src/open_inwoner/accounts/tests/factories.py 100.00% <100.00%> (ø)
...c/open_inwoner/accounts/tests/test_auth_2fa_sms.py 100.00% <100.00%> (ø)
.../open_inwoner/accounts/tests/test_profile_views.py 100.00% <100.00%> (ø)
...r_siteconfiguration_footer_visiting_phonenumber.py 100.00% <100.00%> (ø)
src/open_inwoner/configurations/models.py 97.61% <100.00%> (ø)
src/open_inwoner/openklant/forms.py 100.00% <100.00%> (ø)
... and 5 more

... and 3 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

@vaszig vaszig requested a review from Bartvaderkin June 26, 2023 09:05
Copy link
Contributor

@Bartvaderkin Bartvaderkin left a comment

Choose a reason for hiding this comment

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

I'm not 100% sure this is accurate and has usability:

  • Do we need the distinction for international numbers? Can we still enter local dutch numbers (eg: without country code) if a field has international validator?
  • Do we support input of humanized numbers? Eg, with dashes and spaces for readability? Many people write their numbers in groups (06 1234 5678 or 020 123 45 67 or 020-1234567)

@vaszig
Copy link
Contributor Author

vaszig commented Jun 26, 2023

I'm not 100% sure this is accurate and has usability:

* Do we need the distinction for international numbers? Can we still enter local dutch numbers (eg: without country code) if a field has international validator?

* Do we support input of humanized numbers? Eg, with dashes and spaces for readability? Many people write their numbers in groups (06 1234 5678 or 020 123 45 67 or 020-1234567)

If we want make use of international numbers (imo we maybe only need them in the contact form) we need the distinction. When we enter such a number we have to use the country code indeed. Should I remove it and use only dutch numbers? @alextreme what do you think?

@alextreme
Copy link
Member

I would recommend to only allow Dutch numbers (10 digits, no spaces, dashes etc). I'd prefer being too strict when it comes to data sent through to a secondary system.

@vaszig vaszig force-pushed the feature/1558-add-phonenumber-validation-to-contactform branch from a2d3bc4 to 1637e69 Compare June 27, 2023 11:36
@vaszig
Copy link
Contributor Author

vaszig commented Jun 27, 2023

Removed international phone validator and now there is only the dutch one. I also added a method for spaces and dashes.

@vaszig vaszig requested review from alextreme and pi-sigma June 27, 2023 13:58
Copy link
Contributor

@pi-sigma pi-sigma left a comment

Choose a reason for hiding this comment

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

Couple of minor changes and we're good to go.

@alextreme alextreme merged commit f451a98 into develop Jun 28, 2023
@alextreme alextreme deleted the feature/1558-add-phonenumber-validation-to-contactform branch June 28, 2023 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants