#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)
Change History (18)
by , 14 years ago
| Attachment: | in_states_normalized_path.diff added |
|---|
comment:1 by , 14 years ago
| Has patch: | set |
|---|---|
| Needs tests: | set |
| Triage Stage: | Unreviewed → Accepted |
comment:2 by , 14 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.
by , 14 years ago
| Attachment: | in_states_normalized_path_with_tests.diff added |
|---|
comment:3 by , 14 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.
comment:5 by , 14 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 , 14 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:
- Correct grammatical error in INStateField error message
- Extend INZipCodeField to allow "NNN NNN" and normalize to "NNNNNN" (see India in http://en.wikipedia.org/wiki/Postal_code#Formats)
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.
by , 14 years ago
| Attachment: | in_states_normalized_patch_with_better_tests.diff added |
|---|
by , 14 years ago
| Attachment: | in_states_normalized_patch_with_better_tests.v2.diff added |
|---|
comment:7 by , 14 years ago
by , 14 years ago
| Attachment: | in_states_normalized_patch_with_better_tests.v3.diff added |
|---|
comment:8 by , 14 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 , 14 years ago
| Cc: | 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 , 14 years ago
| Triage Stage: | Accepted → Ready 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 , 14 years ago
| Attachment: | 15813.indian-localflavor-fixes.diff added |
|---|
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.