Opened 6 years ago

Closed 5 years ago

#14608 closed New feature (fixed)

Adding a INPhoneNumberField to indian localflavor

Reported by: Kenneth Gonsalves Owned by: Kenneth Gonsalves
Component: contrib.localflavor Version: master
Severity: Normal Keywords: india phone
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

Indian landline phone numbers have an STD code prefixed by a '0' and followed by 2-4 digits. Followed by a 7 digit number that starts with the numbers 1-8. The two are separated by either a '-' or a space.

Attachments (6)

inphone.diff (823 bytes) - added by Kenneth Gonsalves 6 years ago.
diff between current forms.py and proposed forms.py
in.py (3.1 KB) - added by Kenneth Gonsalves 6 years ago.
in.py with tests for INPhoneNumberField
dj.diff (1.9 KB) - added by Kenneth Gonsalves 6 years ago.
diff between current forms.py and proposed forms.py
14608.INPhoneNumberField.diff (7.4 KB) - added by Julien Phalip 5 years ago.
Actually hooked up tests
INPhoneNumberField.diff (7.5 KB) - added by Kenneth Gonsalves 5 years ago.
fresh patch that fixes typo in the error message
14608.IndianPhoneNumber-LocalFlavor.diff (5.5 KB) - added by Julien Phalip 5 years ago.
Patch updated to latest trunk

Download all attachments as: .zip

Change History (41)

Changed 6 years ago by Kenneth Gonsalves

Attachment: inphone.diff added

diff between current forms.py and proposed forms.py

comment:1 Changed 6 years ago by Kenneth Gonsalves

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

After extensive discussion in Indian python circles it was found that the patch submitted is not fully accurate. A fresh regex has been formulated and tests are being rewritten, so this ticket is not yet ready for review.

comment:2 Changed 6 years ago by Simon Meers

Patch needs improvement: set
Triage Stage: UnreviewedAccepted

comment:3 Changed 6 years ago by Kenneth Gonsalves

Owner: changed from nobody to Kenneth Gonsalves
Patch needs improvement: unset
Status: newassigned

ok, now all is done. The documentation explains the format. Two files attached - the diff on django/contrib/localflavor/in_/forms.py and in.py which contains the tests. (why does the in directory have a trailing underscore?)

Changed 6 years ago by Kenneth Gonsalves

Attachment: in.py added

in.py with tests for INPhoneNumberField

Changed 6 years ago by Kenneth Gonsalves

Attachment: dj.diff added

diff between current forms.py and proposed forms.py

comment:4 Changed 5 years ago by Julien Phalip

Needs tests: set
Severity: Normal
Type: New feature

Thanks for this suggestion. Could you write some tests for this, either in source:django/trunk/tests/regressiontests/localflavor or source:django/trunk/tests/regressiontests/forms/localflavor ?

comment:5 Changed 5 years ago by Kenneth Gonsalves

Needs tests: unset

tests are there in 'in.py' uploaded 7 weeks back. I have unchecked the 'needs tests' field.

comment:6 Changed 5 years ago by Julien Phalip

Patch needs improvement: set

OK I see. Could you rewrite your tests using unittest, and also combine all changes within one single diff patch? Thanks!

comment:7 Changed 5 years ago by Kenneth Gonsalves

you want unittests in addition to doctests? or instead of doctests? I have the unittests, but since I could not find unittests for other phonenumber flavors, I did not put them.

comment:8 Changed 5 years ago by Russell Keith-Magee

One of the features of 1.3 is that we replaced almost all the doctests in Django with unit tests. Going forward, we're only adding unit tests.

So - any patch being proposed for trunk needs to include unittests not doctests.

comment:9 Changed 5 years ago by Julien Phalip

You may add your unittests either in source:django/trunk/tests/regressiontests/localflavor or source:django/trunk/tests/regressiontests/forms/localflavor , whichever makes more sense to you ;)

comment:10 Changed 5 years ago by Kenneth Gonsalves

ok - will do - good thing you are removing doctests - they are a pain to write.

comment:11 Changed 5 years ago by Kenneth Gonsalves

all ready, but I am confused as how to combine everything in one patch. svn diff gives me the changes in forms.py. I have also created an in.py in http://code.djangoproject.com/browser/django/trunk/tests/regressiontests/forms/localflavor - how do I add this to the patch?

comment:12 Changed 5 years ago by Julien Phalip

My svn skills are a bit rusty but I think you need to 'svn add' the new file before running diff. I've also found this: http://stackoverflow.com/questions/4248768/including-new-files-in-svn-diff

If you're still struggling, I suggest you ask for help on the django-users list. Thank you!

comment:13 Changed 5 years ago by Kenneth Gonsalves

cool - I haven't used svn for some years now. svn add did the trick. I will just test everything out and upload tomorrow. Thanks for you patience

comment:14 Changed 5 years ago by Kenneth Gonsalves

Patch needs improvement: unset
Resolution: fixed
Status: assignedclosed

attached the patch with tests

comment:15 Changed 5 years ago by Alex Gaynor

Resolution: fixed
Status: closedreopened

A ticket isn't closed when a patch is uploaded.

comment:16 Changed 5 years ago by Kenneth Gonsalves

apologies - my first time

comment:17 Changed 5 years ago by Julien Phalip

Needs documentation: set

Thank you for your own patience, lawgon. Could you also please write some documentation for this new feature? :)

comment:18 Changed 5 years ago by Kenneth Gonsalves

class in_.forms.INPhoneNumberField validates that the data is a valid Indian phone number, including the STD code. It's normalised to 0XXX-XXXXXXX or 0XXX XXXXXXX format. The first string is the STD code which is a '0' followed by 2-4 digits. The second string is 8 digits if the STD code is 3 digits, 7 digits if the STD code is 4 digits and 6 digits if the STD code is 5 digits. The second string will start with numbers between 1 and 6. The separator is either a space or a hyphen.

This is the documentation (I had put it in the doctests file which has now been left out) - It is now in the docstring. Will upload the latest patch.

comment:19 Changed 5 years ago by Julien Phalip

Sorry, I should have been clearer. You need to write the doc in source:django/trunk/docs/ref/contrib/localflavor.txt so that it appears in http://docs.djangoproject.com/en/dev/ref/contrib/localflavor/#india-in

By the way, what contribution did "Steve Fernandez" do in this patch? You may add his name and yours -- or maybe YOU are Steve :-) -- in source:django/trunk/AUTHORS if you like.

comment:20 Changed 5 years ago by Kenneth Gonsalves

Needs documentation: unset

documentation added. Steve removed. Fresh patch attached. Changed type in docs - the directory is 'in_' and not 'in' (wonder why?)

Changed 5 years ago by Julien Phalip

Actually hooked up tests

comment:21 Changed 5 years ago by Julien Phalip

Patch needs improvement: set

I tried your patch was the test wasn't run because it wasn't actually hooked up to the test suite. I've fixed that in the attached patch by updating tests/regressiontests/forms/localflavortests.py and tests/regressiontests/forms/tests/__init__.py

Now, the test can be run but it raises this error:

======================================================================
ERROR: test_INPhoneNumberField (regressiontests.forms.localflavor.in_.INLocalFlavorTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/julien/.virtualenvs/djangotests/src/django/tests/regressiontests/forms/localflavor/in_.py", line 24, in test_INPhoneNumberField
    self.assertFieldOutput(INPhoneNumberField, valid, invalid)
  File "/Users/julien/.virtualenvs/djangotests/src/django/tests/regressiontests/forms/localflavor/utils.py", line 41, in assertFieldOutput
    required.clean, input
  File "/Users/julien/.virtualenvs/djangotests/src/django/django/utils/unittest/case.py", line 991, in assertRaisesRegexp
    expected_regexp = re.compile(expected_regexp)
  File "/opt/local/lib/python2.5/re.py", line 188, in compile
    return _compile(pattern, flags)
  File "/opt/local/lib/python2.5/re.py", line 241, in _compile
    raise error, v # invalid expression
error: bad character range

----------------------------------------------------------------------

Needs fixing :)

I'm not sure if that's the cause, but tests need to pass on Python 2.5.

comment:22 Changed 5 years ago by Russell Keith-Magee

@lawgon -- the directory is called in_ because "in" is a keyword in Python.

comment:23 Changed 5 years ago by Kenneth Gonsalves

I have only tested on 2.7 - will check on 2.6 and 2.5 and revert. Nice to know that our country has been honoured with a python keyword

comment:24 Changed 5 years ago by Kenneth Gonsalves

it fails in 2.6 also. However the unit tests of the re expression pass in 2.6. I will investigate.

comment:25 Changed 5 years ago by Kenneth Gonsalves

I am unable to fix this. Import of INPhoneNumberField into a form works as advertised. I also tried with a single line re (without VERBOSE) but the same re error comes up in the tests. Finally I tried with a very simple re - r'\d*'. This works. My conclusion is that in testing, an re beyond a certain length (or maybe beyond a certain level of complexity) will cause a syntax error although the same works in django itself. This is beyond my competence to correct. Should I file a bug?

comment:26 Changed 5 years ago by Kenneth Gonsalves

I now tried with a simple re containing the '\d' repeated 85 times (this makes it the same length of my re). The tests accept it. So it must lie in the complexity.

comment:27 Changed 5 years ago by Julien Phalip

Wow, it took me a long time to track this down. In fact, there's nothing really wrong with your patch. I think the problem is in the way that django.tests.regressiontests.forms.localflavor.utils.assertFieldOutput works.

assertFieldOutput calls assertRaisesRegexp and passes the expected errors as a unicode string:

In source:django/trunk/tests/regressiontests/forms/localflavor/utils.py#L40 :

self.assertRaisesRegexp(ValidationError, unicode(errors),
                required.clean, input
            )

... and further down the track, in assertRaisesRegexp, that string is compiled as a regular expression:

In source:django/trunk/django/utils/unittest/case.py#L991 :

expected_regexp = re.compile(expected_regexp)

Now, the issue here is that the error message in your patch contains some '-' characters, which are interpreted as a range, hence the 'bad character range' error that is raised when running the tests:

class INLocalFlavorTests(LocalFlavorTestCase):
    def test_INPhoneNumberField(self):
        error_format = [u'Phone numbers must be in 02X-7X or 03X-6X or 04X-5X format.']
    ...

Remove those '-' characters and the tests will pass... Now of course, this is not satisfactory. I tend to think it's a bug in the test framework. Let me know if somebody has an opinion on this, otherwise I might bring this up on django-dev.

comment:28 Changed 5 years ago by Kenneth Gonsalves

Given that the framework itself does not barf when the patch is used, I think that this bug needs to be fixed. The world will not end if django does not yet have an Indian Phone number field, but I have a few more of this type of thing in the Queue and some of them also use '-' in the error messages. So please take it up on the dev list. Frankly I do not think that error messages should compile as regex.

comment:29 Changed 5 years ago by Julien Phalip

I think I've found the root cause for this problem and posted a proposed fix in #15805. If that fix gets checked in then your patch will be ready for checkin.

comment:30 Changed 5 years ago by Kenneth Gonsalves

found a typo in my patch. Attached a fresh patch.

Changed 5 years ago by Kenneth Gonsalves

Attachment: INPhoneNumberField.diff added

fresh patch that fixes typo in the error message

comment:31 Changed 5 years ago by Julien Phalip

Thanks for the update. By the way, feel free to add your name to the AUTHORS file, if only in memory of this epic ticket :)

comment:32 Changed 5 years ago by Kenneth Gonsalves

Easy pickings: unset
Patch needs improvement: unset
UI/UX: unset

as #15805 is fixed, can this be checked in?

comment:33 Changed 5 years ago by Julien Phalip

This one can easily proceed as soon as #15813 gets checked in. I'm keeping an eye on it ;)

Changed 5 years ago by Julien Phalip

Patch updated to latest trunk

comment:34 Changed 5 years ago by Julien Phalip

Triage Stage: AcceptedReady for checkin

Good to go, thanks!

comment:35 Changed 5 years ago by Jannis Leidel

Resolution: fixed
Status: reopenedclosed

In [16495]:

Fixed #14608 -- Added phone number field to Indian local flavor. Thanks, lawgon and Julien Phalip.

Note: See TracTickets for help on using tickets.
Back to Top