Opened 13 years ago

Closed 13 years ago

Last modified 12 years ago

#15813 closed Cleanup/optimization (fixed)

STATES_NORMALIZED dict for India does not include all states

Reported by: Jim Dalton Owned by: nobody
Component: contrib.localflavor Version: dev
Severity: Normal Keywords:
Cc: Jim Dalton 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

I got bit today by an inconsitency in the implementation of the STATES_NORMALIZED dictionary for India as compared with the U.S. and Canada. (These are the only three countries which offer this dictionary.

My assumption is that the purpose of this dictionary is to help normalize user input to the correct two letter state abbreviation. For both the U.S. and Canada, this dictionary maps every two-letter abbreviation to the normalized version (e.g.. "bc" -> "BC"), every complete spelling to the normalized version (e.g. "british columbia" -> "BC") and common misspellings or abbreviations to the normalized version (e.g. "calif" -> "CA"). Makes sense.

The STATES_NORMALIZED dict in India does not do this though. It *only* maps a handful of abbreviations and misspellings, but does not comprehensively map each two letter abbreviation and each state.

I implemented some logic that assumed it worked like the U.S. and Canada and got a complaint from a user registering from Gujarat who could not do so, because no mapping is included for it. Yikes!

I humbly propose we extend STATES_NORMALIZED for India with all two letter abbreviations and spelled out state names. If there is a reason not to do this, I'd like to hear it. Otherwise, my patch is attached.

Attachments (6)

in_states_normalized_path.diff (2.4 KB ) - added by Jim Dalton 13 years ago.
in_states_normalized_path_with_tests.diff (4.2 KB ) - added by Jim Dalton 13 years ago.
in_states_normalized_patch_with_better_tests.diff (9.8 KB ) - added by Jim Dalton 13 years ago.
in_states_normalized_patch_with_better_tests.v2.diff (10.4 KB ) - added by Jim Dalton 13 years ago.
in_states_normalized_patch_with_better_tests.v3.diff (9.8 KB ) - added by Jim Dalton 13 years ago.
15813.indian-localflavor-fixes.diff (10.1 KB ) - added by Julien Phalip 13 years ago.

Download all attachments as: .zip

Change History (18)

by Jim Dalton, 13 years ago

comment:1 by Julien Phalip, 13 years ago

Has patch: set
Needs tests: set
Triage Stage: UnreviewedAccepted

I see no reason why we shouldn't allow this. Every country is different, which is why we have the localflavors in place. Your patch looks good, although you need to add tests before it can be checked in. I see there already is some documentation for this feature so no needs to add more.

comment:2 by Jim Dalton, 13 years ago

Okay, patch with tests included. There were no tests whatsoever for India previously, but I only added tests to cover the changes I made.

comment:3 by Jim Dalton, 13 years ago

Just a note on the patch, for some reason svn diff did not include the __init__.py file in the in_ directory I created, presumably because the file is blank? Anyhow that file obviously needs to be created for the tests to work.

Last edited 13 years ago by Jim Dalton (previous) (diff)

comment:4 by Jim Dalton, 13 years ago

Needs tests: unset

Forgot to unset the "needs tests" flag last week.

comment:5 by Julien Phalip, 13 years ago

Easy pickings: unset
Patch needs improvement: set

Thanks a lot for your patch. Everything looks great but the tests still look a bit thin. The tests should be made more thorough by checking that *all* the values return the expected outcome. Testing some cases where the form field is invalid would also be useful. This would prevent regressions caused, for example, by typos inserted in the STATES_NORMALIZED or STATE_CHOICES contants in the future.

You can find lots of inspiration in the existing tests for other local flavors in source:django/trunk/tests/regressiontests/forms/localflavor/

I'll review your patch again if you can re-post it with more comprehensive tests. Thanks!

comment:6 by anonymous, 13 years ago

Okay, I've uploaded a new patch. Thank you for pointing me to the tests in regressiontests/ btw; I had only seen the single test in modeltests/ previously.

The India localflavor actually had not test module at all, so I added tests for my original change plus the other fields in the India module. In doing so, I came across two small issues which I tested for and corrected:

Patch with new tests is included. I removed the old tests since modeltests/ wasn't really the right place for them.

Two other items of note:

  • "Zip code field" is really a misnamed field. Indian postal codes are called "pin codes" or more generically postal codes. Probably too inconsequential of an issue to warrant an API change at this point, but just thought I'd note it.
  • I'm going to open a separate ticket for this, but in constructing these tests I observed that none of the localflavor tests are correctly checking for a match in the error message for validation errors. The error message is incorrectly being passed as a list instead of a string and therefore the regex check is being bypassed.

comment:7 by Jim Dalton, 13 years ago

Just saw #15805, which addresses my last comment above. I fixed the patch to use the "list" format for checking the error message, assuming that #15805 gets committed.

comment:8 by Jim Dalton, 13 years ago

Once more with feeling. Got rid of an error I introduced while separately investigating #15805. Sorry to pollute the message space.

comment:9 by Jim Dalton, 13 years ago

Cc: Jim Dalton added
Patch needs improvement: unset

Noticed I left the patch needs improvement flag checked. I'm unchecking it, since this patch should be ready for check in. If a reviewer finds any issues, please feel free to recheck and I will address any problems.

comment:10 by Julien Phalip, 13 years ago

Triage Stage: AcceptedReady for checkin

Thank you, the patch looks great. Good catch with the allowed space inside the postal code -- I've just updated your patch to reflect that in the error message, and also fixed the related small issue reported in #16132.

by Julien Phalip, 13 years ago

comment:11 by Jannis Leidel, 13 years ago

Resolution: fixed
Status: newclosed

In [16480]:

Fixed #15813 -- Updated Indian localflavor to use correct state choices and fixed various other bugs. Thanks, jsdalton.

comment:12 by Jacob, 12 years ago

milestone: 1.4

Milestone 1.4 deleted

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