Code

Opened 4 years ago

Closed 3 years ago

#14608 closed New feature (fixed)

Adding a INPhoneNumberField to indian localflavor

Reported by: lawgon Owned by: lawgon
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 lawgon 4 years ago.
diff between current forms.py and proposed forms.py
in.py (3.1 KB) - added by lawgon 3 years ago.
in.py with tests for INPhoneNumberField
dj.diff (1.9 KB) - added by lawgon 3 years ago.
diff between current forms.py and proposed forms.py
14608.INPhoneNumberField.diff (7.4 KB) - added by julien 3 years ago.
Actually hooked up tests
INPhoneNumberField.diff (7.5 KB) - added by lawgon 3 years ago.
fresh patch that fixes typo in the error message
14608.IndianPhoneNumber-LocalFlavor.diff (5.5 KB) - added by julien 3 years ago.
Patch updated to latest trunk

Download all attachments as: .zip

Change History (41)

Changed 4 years ago by lawgon

diff between current forms.py and proposed forms.py

comment:1 Changed 4 years ago by lawgon

  • 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 4 years ago by DrMeers

  • Patch needs improvement set
  • Triage Stage changed from Unreviewed to Accepted

comment:3 Changed 3 years ago by lawgon

  • Owner changed from nobody to lawgon
  • Patch needs improvement unset
  • Status changed from new to assigned

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 3 years ago by lawgon

in.py with tests for INPhoneNumberField

Changed 3 years ago by lawgon

diff between current forms.py and proposed forms.py

comment:4 Changed 3 years ago by julien

  • Needs tests set
  • Severity set to Normal
  • Type set to 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 3 years ago by lawgon

  • Needs tests unset

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

comment:6 Changed 3 years ago by julien

  • 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 3 years ago by lawgon

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 3 years ago by russellm

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 3 years ago by julien

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 3 years ago by lawgon

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

comment:11 Changed 3 years ago by lawgon

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 3 years ago by julien

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 3 years ago by lawgon

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 3 years ago by lawgon

  • Patch needs improvement unset
  • Resolution set to fixed
  • Status changed from assigned to closed

attached the patch with tests

comment:15 Changed 3 years ago by Alex

  • Resolution fixed deleted
  • Status changed from closed to reopened

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

comment:16 Changed 3 years ago by lawgon

apologies - my first time

comment:17 Changed 3 years ago by julien

  • Needs documentation set

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

comment:18 Changed 3 years ago by lawgon

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 3 years ago by julien

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 3 years ago by lawgon

  • Needs documentation unset

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

Changed 3 years ago by julien

Actually hooked up tests

comment:21 Changed 3 years ago by julien

  • 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 3 years ago by russellm

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

comment:23 Changed 3 years ago by lawgon

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 3 years ago by lawgon

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

comment:25 Changed 3 years ago by lawgon

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 3 years ago by lawgon

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 3 years ago by julien

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 3 years ago by lawgon

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 3 years ago by julien

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 3 years ago by lawgon

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

Changed 3 years ago by lawgon

fresh patch that fixes typo in the error message

comment:31 Changed 3 years ago by julien

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 3 years ago by lawgon

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

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

comment:33 Changed 3 years ago by julien

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

Changed 3 years ago by julien

Patch updated to latest trunk

comment:34 Changed 3 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

Good to go, thanks!

comment:35 Changed 3 years ago by jezdez

  • Resolution set to fixed
  • Status changed from reopened to closed

In [16495]:

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

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.