Code

Opened 6 years ago

Closed 21 months ago

#7023 closed New feature (invalid)

Improve validation and add extra attributes to PLNationalIdentificationNumberField

Reported by: Piotr Lewandowski <django@…> Owned by: nobody
Component: contrib.localflavor Version: master
Severity: Normal Keywords: pl, polish, pesel, localflavorsplit
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

Attached patch includes:

  • PLNationalIdentificationNumberField:
    • gender recognition (PLNationalIdentificationNumberField.gender),
    • birthday calculation (PLNationalIdentificationNumberField.birthday),
    • birthday-based validation,
  • tests,
  • Polish locale.

Attachments (1)

pesel-validation.diff (6.2 KB) - added by Piotr Lewandowski <django@…> 6 years ago.

Download all attachments as: .zip

Change History (10)

Changed 6 years ago by Piotr Lewandowski <django@…>

comment:1 follow-up: Changed 6 years ago by mtredinnick

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

This looks pretty good. Thanks for all the patches lately to improve Polish Django.

Is there a real use-case for not doing the birth date checks all the time? Are there known invalid Polish ID numbers or something?

Generally, we try to enforce valid data all the time, so unless there's a real-world case where this wouldn't be appropriate, we can probably just remove the option.

comment:2 in reply to: ↑ 1 Changed 6 years ago by anonymous

Replying to mtredinnick:

This looks pretty good. Thanks for all the patches lately to improve Polish Django.

You're welcome - get ready for more. :-)

Is there a real use-case for not doing the birth date checks all the time? Are there known invalid Polish ID numbers or something?
Generally, we try to enforce valid data all the time, so unless there's a real-world case where this wouldn't be appropriate, we can probably just remove the option.

Unfortunately there are some numbers „in the wild” which are not valid - PESEL (http://en.wikipedia.org/wiki/PESEL) was created in 1979 (written in S/360 assembler) and is working till now in almost unchanged form (only bugfixes are applied). AFAIR in the late 70's some numbers were generated „manually” and this was the most likely reason for errors - there are even numbers with incorrect checksum! But the most annoying part of this problem is the standpoint of Polish authorities which are not willing to change those incorrect IDs - exceptions are only made for sex-change operations (since PESEL contains gender-info) and witness protection programs.

Nonetheless, those cases are rather rare and I had serious doubts if the birthday_check option is really necessary. Anyway, I've decided to put it in the code with sensible default. In order to be consistent I should probably add checksum_check as well to face the real-world situations.

comment:3 Changed 6 years ago by mtredinnick

Thanks for the extra information. In that case, though, the extra validation should never be applied. If I'm writing a site that accepts PESEL numbers, I have no way of knowing when I should be expecting invalid numbers or not. 1979 is hardly prehistoric times. I was born before then!

Certainly people could have numbers in the bad range, so you must always expect them, right? Sometimes you just have to let validation slip a little in order to allow all the valid cases (although bad checksums means you can't really validate anything, which is kind of silly). Given what you've said, if you really wanted birthdate_check in there, it should probably default to False so that nobody is excluded just because they had an ID number created in 1979.

comment:4 Changed 6 years ago by Piotr Lewandowski <django@…>

In order to be consistent we should provide also an option for checksum validation since people do have numbers which are formally incorrect.

I propose:

  • set default value for birthdate_check to X,
  • add checksum_check with defaults to X,
  • document the problem in the documentation (docstring).


Now the question is whether X should be True or False. Since numbers we are talking about are really rare (and both errors can occur with approx. the same probability) I'm for True as sensible default for doing checks.

Fortunately, PESEL number always consists of 11 digits - and that can be validated without doubts.

comment:5 Changed 4 years ago by imbaczek@…

what's the status of this ticket? suggest committing and closing.

comment:6 Changed 3 years ago by lukeplant

  • Severity set to Normal
  • Type set to New feature

comment:7 Changed 3 years ago by julien

  • Easy pickings unset
  • Patch needs improvement set

The tests would need to be rewritten using unittests since this is now Django's preferred way. Please also do _not_ include translation strings in the patch.

comment:8 Changed 2 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:9 Changed 21 months ago by aaugustin

  • Keywords pl, polish, pesel, localflavorsplit added; pl polish pesel removed
  • 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!

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.