fixing civil id validation for those ids starting with 300

[burhan]

Kuwait has issued ids starting with 300x that are valid; and these fail the birthdate check; this check and been given lower priority over the checksum validation.

[benkonrath]

Can you add test for an ID number that fails without this fix? This helps to ensure that this fix will continue to work with future changes. Thanks.

[benkonrath]

@burhan Sorry for the delay here. github doesn't give notifications when you add commits to a pull request. You have to actually make a comment for notifications to be sent.

[benkonrath]

I just looked over your latest version and I think it needs to be re-worked a bit. The problem is that a valid date based civil id number will never go through the date validation logic because it will always have a valid checksum which ends the validation. You can see this by looking at the code coverage after running the tests. The last line of the clean method is never executed.

From a high level, the validation logic could work something like this:

• Check that the value is all digits
  It might be a good idea to the django.forms.fields.RegexField instead of the regular Field. You can set the digit regex on that field.
• Check that the value has a valid checksum digit
• Only if the value doesn't start with 300, run the date based validation.
  The id_re variable should be renamed because it's not for all ids anymore.

This pull request also needs a couple of housekeeping items:

• Add an entry to docs/changelog.rst describing the change
• Add an entry for your name in docs/authors.rst file
• Add a link to some documentation about the format of the Kuwait Civil ID if possible.

Thanks for your contribution. I'm looking forward to reviewing your updated pull request.

[burhan]

After some testing around, I managed to solve the issue by adding a century prefix to the dates (since they are still valid) - this way, it ensures that 300xxx ids are validated correctly. Updated the test cases as well.

I think think should fix it; pending any travis blockers.

[benkonrath]

Nice solution. There are some issues with missing tests and an unnecessary duplicated regex but it's not related to addressing the validation bug for IDs starting with 3. If you want to work on fixing up these other issues, you can tackle that in a new PR.

The only thing left for you do is to rebase and squash your commits into 1 commit. Thanks.

[burhan]

HI @benkonrath - can you explain a bit more on the duplicated regex? I would like to close it up here. Also the tests were updated to check 3xx civil ids. Appreciate if you could clarify on this.

[benkonrath]

Sure. It's not necessary to have the first regex check because id_re already checks for only digits. You can safely delete these lines:

if not re.match(r'^\d{12}$', value):
    raise ValidationError(self.error_messages['invalid'])

Since there is only 1 regex, you can inherit from the RegexField instead of Field and adjust the code a bit to work with the RegexField. This change is not strictly required because the existing solution works.

It would also be nice to have tests for these cases:

• A civil id number with an invalid checksum
• A civil id number that has at least 1 non-digit (letter or special character)

Thanks for looking into this.

[burhan]

@benkonrath I'll raise another PR so it inherits from RegexField

[benkonrath]

Thanks. Merged via 7c26f09.