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

Improve phone handling #1433

Merged
merged 22 commits into from Dec 13, 2017
Merged
Show file tree
Hide file tree
Changes from 12 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
10 changes: 7 additions & 3 deletions saleor/userprofile/forms.py
Expand Up @@ -3,26 +3,30 @@

from django.conf import settings
from django.contrib.auth import forms as django_forms, update_session_auth_hash
from phonenumbers.phonenumberutil import country_code_for_region

from .i18n import AddressMetaForm, get_address_form_class


def get_address_form(data, country_code, initial=None, instance=None, **kwargs):
country_form = AddressMetaForm(data, initial=initial)
preview = False

if country_form.is_valid():
country_code = country_form.cleaned_data['country']
preview = country_form.cleaned_data['preview']

if initial is None and country_code:
initial = {}
if country_code:
initial['phone'] = '+{}'.format(country_code_for_region(country_code))

address_form_class = get_address_form_class(country_code)

if not preview and instance is not None:
address_form_class = get_address_form_class(
instance.country.code)
address_form = address_form_class(
data, instance=instance,
**kwargs)
data, instance=instance, **kwargs)
else:
initial_address = (
initial if not preview
Expand Down
33 changes: 32 additions & 1 deletion saleor/userprofile/i18n.py
Expand Up @@ -5,14 +5,27 @@
import i18naddress
from django import forms
from django.forms.forms import BoundField
from django.utils.translation import pgettext, pgettext_lazy
from django.forms import Select, TextInput
from django_countries.data import COUNTRIES
from django.utils.translation import pgettext_lazy
from phonenumber_field.widgets import (
PhoneNumberPrefixWidget, PhonePrefixSelect)
from phonenumber_field.formfields import PhoneNumberField
from phonenumbers import COUNTRY_CODE_TO_REGION_CODE

from .models import Address
from .validators import validate_possible_number


PhoneNumberField.default_validators = [validate_possible_number]
PhoneNumberField.to_python = lambda self, value: value
Copy link
Member

Choose a reason for hiding this comment

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

Instead of this kind of patching, we should rather extend the original class and override the methods or fields that we want to customize. This would be also a good place to include relevant docstrings, explaining how the customization is different from the original implementation.


COUNTRY_FORMS = {}
UNKNOWN_COUNTRIES = set()

phone_prefixes = [
('+{}'.format(k), '+{}'.format(k)) for
(k, v) in COUNTRY_CODE_TO_REGION_CODE.items()]

AREA_TYPE_TRANSLATIONS = {
'area': pgettext_lazy('Address field', 'Area'),
Expand All @@ -37,6 +50,22 @@
'zip': pgettext_lazy('Address field', 'ZIP code')}


class CustomPhonePrefix(PhonePrefixSelect):
Copy link
Member

Choose a reason for hiding this comment

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

It's a good practice to include the widget type in the class name so in this case, CustomPhonePrefixSelect would be a better name. Also, the word "custom" doesn't say much. What makes this widget custom? It would be great if we could reflect the purpose of this class in the name.


def __init__(self, initial=None):
choices = phone_prefixes
Copy link
Member

Choose a reason for hiding this comment

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

I think there's no need to assign phone_prefixes to a new variable here.

if initial:
self.initial = phone_prefixes[initial]

Copy link
Member

Choose a reason for hiding this comment

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

Empty blank lines in simple functions aren't necessary, it doesn't add much to readability.

super(PhonePrefixSelect, self).__init__(choices=choices)


class PhonePrefixWidget(PhoneNumberPrefixWidget):
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I decided to overwrite this one as there are issues with default widget value:
stefanfoulis/django-phonenumber-field#82
Quick example: choosing "United Kingdom +44 -> continue -> select new address makes field being populated by Guernsey as it has the same phone code, but it's alphabetically before UK.

Copy link
Member

Choose a reason for hiding this comment

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

Missing docstring.

def __init__(self, attrs=None):
widgets = (Select(attrs=attrs, choices=phone_prefixes), TextInput())
super(PhoneNumberPrefixWidget, self).__init__(widgets, attrs)


class AddressMetaForm(forms.ModelForm):
# This field is never visible in UI
preview = forms.BooleanField(initial=False, required=False)
Expand Down Expand Up @@ -74,6 +103,8 @@ class Meta:
model = Address
exclude = []

phone = PhoneNumberField(widget=PhonePrefixWidget, required=False)

def __init__(self, *args, **kwargs):
autocomplete_type = kwargs.pop('autocomplete_type', None)
super(AddressForm, self).__init__(*args, **kwargs)
Expand Down
6 changes: 5 additions & 1 deletion saleor/userprofile/models.py
Expand Up @@ -10,6 +10,10 @@
from django_countries.fields import Country, CountryField
from phonenumber_field.modelfields import PhoneNumberField

from .validators import validate_possible_number

PhoneNumberField.default_validators = [validate_possible_number]


class AddressManager(models.Manager):

Expand Down Expand Up @@ -94,7 +98,7 @@ def __repr__(self):
self.first_name, self.last_name, self.company_name,
self.street_address_1, self.street_address_2, self.city,
self.postal_code, self.country, self.country_area,
str(self.phone)))
self.phone))


class UserManager(BaseUserManager):
Expand Down
12 changes: 12 additions & 0 deletions saleor/userprofile/validators.py
@@ -0,0 +1,12 @@
from django.core.exceptions import ValidationError
from django.utils.translation import ugettext_lazy as _
from phonenumber_field.phonenumber import to_python
from phonenumbers.phonenumberutil import is_possible_number


def validate_possible_number(value):
phone_number = to_python(value)
if phone_number and not is_possible_number(phone_number):
raise ValidationError(
_('The phone number entered is not valid.'),
code='invalid_phone_number')
2 changes: 1 addition & 1 deletion templates/userprofile/snippets/address-form.html
@@ -1,6 +1,6 @@
{% load bootstrap_field from bootstrap3 %}
<div class="i18n-address">
{% bootstrap_field address_form.phone %}
{% bootstrap_field address_form.phone%}
{% with address_form_lines=address_form.i18n_fields_order %}
{% if address_form_lines %}
{% for line in address_form_lines %}
Expand Down
2 changes: 1 addition & 1 deletion templates/userprofile/snippets/address-short.html
Expand Up @@ -11,7 +11,7 @@
{{ address.street_address_1 }} {% if address.street_address_2 %} {{ address.street_address_2 }} {% endif %}<br>
{{ address.postal_code }} {{ address.city }}
{% if address.country_area %}
{{ address.country_area }},
{{ address.country_area }},
{% endif %}<br>
{{ address.get_country_display }}<br>
{{ address.phone }}</p>
Expand Down
21 changes: 20 additions & 1 deletion tests/test_userprofile.py
Expand Up @@ -10,9 +10,11 @@

import i18naddress
import pytest
from django.core.exceptions import ValidationError
from django.http import QueryDict

from saleor.userprofile import forms, models, i18n
from saleor.userprofile.validators import validate_possible_number

@pytest.fixture
def billing_address(db):
Expand All @@ -30,7 +32,9 @@ def test_address_form_for_country(country):
data = {
'first_name': 'John',
'last_name': 'Doe',
'country': country}
'country': country,
'phone': '123456789'}

form = forms.get_address_form(data, country_code=country)[0]
errors = form.errors
rules = i18naddress.get_validation_rules({'country_code': country})
Expand Down Expand Up @@ -105,3 +109,18 @@ def test_country_aware_form_has_only_supported_countries():
for country in i18n.UNKNOWN_COUNTRIES:
assert country not in i18n.COUNTRY_FORMS
assert country not in country_choices


@pytest.mark.parametrize("input,exception", [
('123', ValidationError),
('+48123456789', None),
('+12025550169', None),
('+481234567890', ValidationError),
('testext', ValidationError),
])
def test_validate_possible_number(input, exception):
if exception is not None:
with pytest.raises(exception):
validate_possible_number(input)
else:
validate_possible_number(input)