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

Added support for international phone numbers in volunteer model #846

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

gotham13
Copy link

Issue Reference

This PR addresses the Issue : Fixes #841

Summarize

Please, describe what the PR does, the changes you have made.

Support for indian and international numbers
Added dependency django-phonenumber-field
Made changes in volunteer modal

@gotham13
Copy link
Author

Just made changes for the volunteer model for now. Will change rest on furthur discussion

Copy link

@atmb4u atmb4u left a comment

Choose a reason for hiding this comment

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

LGTM
Please verify this will NOT affect some of the dependencies on phone field which is already in production - once you can confirm on that, we can merge this to upstream.

@gotham13
Copy link
Author

gotham13 commented Aug 21, 2018

One thing that can break with this commit is + infront of international numbers . We need to verify wether it breaks the calling or sms feature. @atmb4u

@gotham13
Copy link
Author

As + was present in the previous regex. I dont think it will break the app. This should be good @atmb4u

@shajith
Copy link

shajith commented Aug 21, 2018

@Gotham13121997 I think this is a good idea in principle. Can you confirm what kinds of numbers are now valid/invalid? We had a nasty bug once that prevented data entry because of a wrong regex, just trying to understand what the change is.

@gotham13
Copy link
Author

gotham13 commented Aug 21, 2018

I have tested on indian numbers with 0 without 0 and with 91. and on international number that was mentioned in the issue. It seems legit. I have not done furthur checking on international numbers though. This was ported from libphonenumber by google so I trusted it should work

@KManiKumarReddy
Copy link

It's allowing alphabets d+420602123456 is allowing successful registration

@KManiKumarReddy
Copy link

Test against this list http://www.iaug.org/p/fo/et/thread=3486

@gotham13
Copy link
Author

@KManiKumarReddy value going into database is a number though. I think it removes the alphabets by itself

@gotham13
Copy link
Author

2018-08-22

@biswaz biswaz added future Features for making KR resourceful for future deployments and removed Testing required rebase-needed labels Sep 13, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
future Features for making KR resourceful for future deployments
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants