Simplify and increase test coverage for PhoneNumberField

PR #1645 pointed to an issue with phone numbers. After digging into the
code for a while, I opted to redo the phone number field implementation
according to the Django documentation. It always takes me a bit of time
to grok descriptors; the new implementation avoids them.

src/oscar/core/phonenumber.py

@@ -120,35 +120,6 @@ def to_python(value):
     return phone_number
 
 
-class PhoneNumberDescriptor(object):
-    """
-    The descriptor for the phone number attribute on the model instance.
-    Returns a PhoneNumber when accessed so you can do stuff like::
-
-        >>> instance.phone_number.as_international
-
-    Assigns a phone number object on assignment so you can do::
-
-        >>> instance.phone_number = PhoneNumber(...)
-    or
-        >>> instance.phone_number = '+414204242'
-
-    """
-
-    def __init__(self, field):
-        self.field = field
-
-    def __get__(self, instance=None, owner=None):
-        if instance is None:
-            raise AttributeError(
-                "The '%s' attribute can only be accessed from %s instances."
-                % (self.field.name, owner.__name__))
-        return instance.__dict__[self.field.name]
-
-    def __set__(self, instance, value):
-        instance.__dict__[self.field.name] = to_python(value)
-
-
 def validate_international_phonenumber(value):
     phone_number = to_python(value)
     if phone_number and not phone_number.is_valid():

src/oscar/models/fields/__init__.py

@@ -137,14 +137,12 @@ def deconstruct(self):
         return name, path, args, kwargs
 
 
-class PhoneNumberField(Field):
+class PhoneNumberField(six.with_metaclass(SubfieldBase, CharField)):
     """
     An international phone number.
 
     * Validates a wide range of phone number formats
     * Displays it nicely formatted
-    * Can be given a hint for the country, so that it can accept local numbers,
-      that are not in an international format
 
     Notes
     -----
@@ -155,22 +153,20 @@ class PhoneNumberField(Field):
     permission notice.
     """
 
-    attr_class = phonenumber.PhoneNumber
-    descriptor_class = phonenumber.PhoneNumberDescriptor
     default_validators = [phonenumber.validate_international_phonenumber]
 
     description = _("Phone number")
 
     def __init__(self, *args, **kwargs):
+        # There's no useful distinction between '' and None for a phone
+        # number. To avoid running into issues that are similar to what
+        # NullCharField tries to solve, we just forbid settings null=True.
         if kwargs.get('null', False):
             raise ImproperlyConfigured(
                 "null=True is not supported on PhoneNumberField")
+        # Set a default max_length.
         kwargs['max_length'] = kwargs.get('max_length', 128)
         super(PhoneNumberField, self).__init__(*args, **kwargs)
-        self.validators.append(MaxLengthValidator(self.max_length))
-
-    def get_internal_type(self):
-        return "CharField"
 
     def get_prep_value(self, value):
         """
@@ -181,15 +177,22 @@ def get_prep_value(self, value):
             return u''
         return value.as_e164 if value.is_valid() else value.raw_input
 
-    def contribute_to_class(self, cls, name):
-        super(PhoneNumberField, self).contribute_to_class(cls, name)
-        setattr(cls, self.name, self.descriptor_class(self))
+    def to_python(self, value):
+        return phonenumber.to_python(value)
+
+    def value_to_string(self, obj):
+        """
+        Used when the field is serialized. See Django docs.
+        """
+        value = self._get_val_from_obj(obj)
+        return self.get_prep_value(value)
 
     def deconstruct(self):
         """
-        deconstruct() is needed by Django's migration framework
+        deconstruct() is needed by Django's migration framework.
         """
         name, path, args, kwargs = super(PhoneNumberField, self).deconstruct()
+        # Delete kwargs at default value.
         if self.max_length == 128:
             del kwargs['max_length']
         return name, path, args, kwargs

tests/unit/core/phonenumber_tests.py

@@ -1,6 +1,7 @@
 #-*- coding: utf-8 -*-
 from django.test.testcases import TestCase
 from django.db import models
+from django import forms
 
 from oscar.core.phonenumber import PhoneNumber
 from oscar.models.fields import PhoneNumberField
@@ -14,7 +15,13 @@ class OptionalPhoneNumber(models.Model):
     phone_number = PhoneNumberField(blank=True, default='')
 
 
-test_number_1 = '+414204242'
+class PhoneNumberForm(forms.ModelForm):
+
+    class Meta:
+        model = MandatoryPhoneNumber
+
+
+valid_number = '+4917696842671'
 equal_number_strings = ['+44 113 8921113', '+441138921113']
 local_numbers = [
     ('GB', '01606 751 78'),
@@ -27,7 +34,7 @@ class TestPhoneNumberTestCase(TestCase):
 
     def test_valid_numbers_are_valid(self):
         numbers = [PhoneNumber.from_string(number_string)
-                   for number_string in equal_number_strings]
+                   for number_string in equal_number_strings + [valid_number]]
         self.assertTrue(all([number.is_valid() for number in numbers]))
         numbers = [PhoneNumber.from_string(number_string, region=region)
                    for region, number_string in local_numbers]
@@ -50,11 +57,21 @@ def test_objects_with_same_number_are_equal(self):
     def test_field_returns_correct_type(self):
         model = OptionalPhoneNumber()
         self.assertEqual(model.phone_number, None)
-        model.phone_number = '+49 176 96842671'
+        model.phone_number = valid_number
         self.assertEqual(type(model.phone_number), PhoneNumber)
 
     def test_can_assign_string_phone_number(self):
         opt_phone = OptionalPhoneNumber()
-        opt_phone.phone_number = test_number_1
+        opt_phone.phone_number = valid_number
         self.assertEqual(type(opt_phone.phone_number), PhoneNumber)
-        self.assertEqual(opt_phone.phone_number.as_e164, test_number_1)
+        self.assertEqual(opt_phone.phone_number.as_e164, valid_number)
+
+
+class TestPhoneNumberFormFieldTestCase(TestCase):
+
+    def test_form_flow(self):
+        form = PhoneNumberForm({'phone_number': valid_number})
+        self.assertTrue(form.is_valid())
+        instance = form.save(commit=False)
+        self.assertTrue(type(instance.phone_number), PhoneNumber)
+        self.assertEqual(instance.phone_number.as_e164, valid_number)