Code

Opened 3 years ago

Closed 3 years ago

Last modified 3 years ago

#15813 closed Cleanup/optimization (fixed)

STATES_NORMALIZED dict for India does not include all states

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

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)

Changed 3 years ago by jsdalton

comment:1 Changed 3 years ago by julien

  • Has patch set
  • Needs documentation unset
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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

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

Changed 3 years ago by jsdalton

comment:3 Changed 3 years ago by jsdalton

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.

Version 0, edited 3 years ago by jsdalton (next)

comment:4 Changed 3 years ago by jsdalton

  • Needs tests unset

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

comment:5 Changed 3 years ago by julien

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

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

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

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

comment:9 Changed 3 years ago by jsdalton

  • Cc jsdalton 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 Changed 3 years ago by julien

  • Triage Stage changed from Accepted to 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.

Changed 3 years ago by julien

comment:11 Changed 3 years ago by jezdez

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

In [16480]:

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

comment:12 Changed 3 years ago by jacob

  • milestone 1.4 deleted

Milestone 1.4 deleted

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.