Opened 6 years ago

Closed 3 years ago

#10456 closed Bug (invalid)

Canada localflavor improvements/fixes

Reported by: mmulley Owned by: nobody
Component: contrib.localflavor Version: master
Severity: Normal Keywords: localflavor, ca, localflavorsplit
Cc: sbull@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

The Canadian localflavor package uses incorrect postal abbreviations for Newfoundland (NF, should be NL) and the Yukon (YK, should be YT). Previous bugs: #10365, #8478.

There's a discussion in #10365 about the merits of changing the abbreviation and potentially breaking existing data. It's a tough question, since the abbreviation isn't simply an internal representation: it's a very common use case to use the postal abbreviation in addresses, and so the current version of ca.localflavor will generate incorrect output. This doesn't help with the issue of existing data, though, and I'm not quite sure what to do there.

Two patches are attached. One updates to the correct abbreviations. The other maintains the current, incorrect abbreviations but normalizes the correct abbreviations (NL, YT) to the incorrect ones (NF, YK) instead of rejecting them, as the current version does.

Both patches also include some other fixes and improvements:

  • More abbreviations and misspellings are accepted and normalized for province names
  • French province names are now accepted
  • CAPostalCodeField accepts and normalizes common postal code formats ('h4u5h7' -> 'H4U 5H7')

Attachments (3)

calocal.diff (5.1 KB) - added by mmulley 6 years ago.
Patch (using correct postal abbreviations)
calocal-oldabbrevs.diff (4.1 KB) - added by mmulley 6 years ago.
Patch (maintains old, incorrect abbreviations)
calocal-compatible.diff (4.7 KB) - added by mmulley 6 years ago.
Maintains backwards compatibility; adds note to documentation

Download all attachments as: .zip

Change History (16)

Changed 6 years ago by mmulley

Patch (using correct postal abbreviations)

Changed 6 years ago by mmulley

Patch (maintains old, incorrect abbreviations)

comment:1 Changed 6 years ago by mtredinnick

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

The data value used in the choices list that is destined to be stored in the database is not a difficult issue at all: it's not used for user-displayable data. If somebody is using it for that, then they're using it incorrectly. The first element in those tuples are simply the database storage values. They could be integers or anything. So let's leave that portion in the previous ticket, but it will almost certainly be wontfixed, since breaking existing database data is bad.

Please create a patch without the duplication from #10365, so that we can deal with the improvements here in isolation.

comment:2 Changed 6 years ago by dharris

I vote for fixing it.

Changing existing data is bad, but storing the wrong code when there's an official proper code sanctioned by a national government is bad as well. Continuing to store the incorrect code means a future of one-off hacks for data export scripts, address printing utilities and other tools which would use these data outside of Django.

My guess is that many sites that actually store Canadian postal codes have already patched their code and fixed their data -- I have -- so the impact on existing data is probably not as bad as you fear.

Fix the code and add an entry to the BackwardsIncompatibleChanges page. Include a sample update query for those who are unclear about how to write the simple SQL required to fix the data.

comment:3 Changed 6 years ago by mmulley

  • Patch needs improvement unset

To clarify, the second patch (labelled -oldabbrevs) doesn't duplicate #10365 and does not include any backwards-incompatible changes to database representations. Clearing needs_better_patch for now, unless/until there are specific issues with that patch. Given that the first patch is controversial, I'd suggest looking at the second patch (or rather the mildly updated -compatible version), which at least accepts the correct postal abbreviations as input, for the moment.

I would argue that the user-displayable data issue isn't entirely cut-and-dried. For example, the documentation text for USStateField is "A form field that validates input as a U.S. state name or abbreviation. It normalizes the input to the standard two-letter postal service abbreviation for the given state." Not some random non-displayable could-be-an-integer internal representation, but "the standard two-letter postal service abbreviation", implying that this is standardized and displayable data. And it's certainly the case that, in the US and Canada, the postal abbreviation is displayed to users just as if not more frequently than the full state/province name.

But, yes, breaking database data is certainly bad. Anyone have a suggestion on how to provide access to correct postal abbreviations (a very, very common use case) without breaking compatibility or violating design principles w/r/t user-displayable data?

(Uploading a slightly modified patch which adds a note about the postal abbreviations to the documentation.)

Changed 6 years ago by mmulley

Maintains backwards compatibility; adds note to documentation

comment:4 Changed 6 years ago by dharris

It looks like your patches also address formatting of Canadian postal codes. I opened a ticket and added a similar patch as #8527 -- you might want to either (a) reduce your patch for this issue to only those changes for the NF/NL, YK/YT issues, or (b) associate this ticket with #8527 so that these fixes are applied together.

comment:5 Changed 6 years ago by dharris

(ah, you mentioned the additional fixes in your description already -- in any case, I've now mentioned #8527 so that this can potentially be closed also)

comment:6 Changed 6 years ago by mtredinnick

I think there might have been some cross-purposes discussion going on here, now that I've tried to sort out the various patches on the tickets. There are two separate issues at play.

  1. The PROVINCE_CHOICES list. In that list, the first tuple is a value stored in the database that is used for lookups and is not a display string. That's the one I'd prefer not to change, since it's not a functionality bug and changing it requires end-user code changes.
  2. The normalised value returned from CAProvinceField. That is a user-displayable field (it's something they can input directly) and that's a functionality bug, since we're normalising to the wrong value. We can and probably should break backwards-compatibility for that.

The problem is then that CAProvinceSelect can potentially be layered over CAProvinceField, which adds weight to the option of changing them both. Going to think about it a bit more to see if any good solution pops up, because there are competing and contradictory requirements here. I'm leaning slightly towards changing both at the moment.

Given how intertwined everything is, I'm going to close #10365 as a dupe of this, since it's going to be hard to close one and not the other and the more relevant comments are here.

comment:7 Changed 6 years ago by kmtracey

#11326 reports the problem with NF vs. NL again.

comment:8 Changed 5 years ago by SamBull

  • Cc sbull@… added

I just wanted to pipe in and note that this bug makes PROVINCE_CHOICES misleading and can lead to frustration and wasted time (because users may assume that the standard province codes were used without checking to confirm). Beyond that, the current PROVINCE_CHOICES just isn't very useful. A list of provinces with arbitrary codes isn't worth nearly as much as a list of provinces with official two-letter province codes.

I propose that the existing PROVINCE_CHOICES be renamed to OLD_PROVINCE_CHOICES and a new PROVINCE_CHOICES be defined that uses the correct codes. This would introduce a backwards incompatibility, but would allow for an easy fix for any users who were using the old choices.

comment:9 Changed 4 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Bug

comment:10 Changed 4 years ago by julien

  • Patch needs improvement set

This looks great, although the patch is now a bit old and would need to be updated. Tests should also be rewritten using unittest.

comment:11 Changed 4 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:12 Changed 4 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

comment:13 Changed 3 years ago by aaugustin

  • Keywords localflavorsplit added
  • Resolution set to invalid
  • Status changed from new to closed

django.contrib.localflavor is now deprecated — see https://docs.djangoproject.com/en/dev/ref/contrib/localflavor/

A repository was created for each localflavor at https://github.com/django/django-localflavor-? (Replace with the country code.)

If you're still interested in this ticket, could you create a pull request on that repository?

Sorry for not resolving this issue earlier, and thanks for your input!

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