Opened 13 years ago
Closed 13 years ago
#16202 closed New feature (fixed)
Localflavor support for Slovenia (si)
Reported by: | Owned by: | nobody | |
---|---|---|---|
Component: | contrib.localflavor | Version: | 1.3 |
Severity: | Normal | Keywords: | |
Cc: | Domen Kožar, vvangelovski@… | Triage Stage: | Ready for checkin |
Has patch: | yes | Needs documentation: | no |
Needs tests: | no | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description
Attached patch provides checking for: personal id number for slovenia, tax number, postal codes and a postal code widget.
Slovenian localflavor patch done in collaboration with Gasper Zejn and Domen Kozar.
Attachments (4)
Change History (23)
by , 13 years ago
Attachment: | ticket16202.diff added |
---|
comment:1 by , 13 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 13 years ago
Cc: | added |
---|
comment:3 by , 13 years ago
Cc: | added |
---|
comment:4 by , 13 years ago
This work looks good.
I would just comment on the SIEMSOField as it's often a cause for problems in practice. I gather this is a unique master citizen number. This was introduced around 1977, and is applied to citizens born since then or alive at the time. The implementation here assumes the year part 7xx translates to 17xx instead of 27xx, which I'm not too sure is the correct way to treat it. Also it's not checking if the date part of the UMCN is in the past which should be part of the validation IMO, because an imaginary UMCN that passes a checksum but has a date part in the future doesn't make sense.
I'd also want to caution the authors to lookup current laws regarding UMCNs given to people with a temporary residence in Slovenia and test this implementation with some real examples if they can.
Technically, when throwing ValidationErrors it would be better to throw more specific errors to notify the user which part of the field didn't pass validation.
For a reference this UMCNField implementation is taken from the Macedonian localflavor:
class UMCNField(RegexField): """ A form field that validates input as a unique master citizen number. The format of the unique master citizen number has been kept the same from Yugoslavia. It is still in use in other countries as well, it is not applicable solely in Macedonia. For more information see: https://secure.wikimedia.org/wikipedia/en/wiki/Unique_Master_Citizen_Number A value will pass validation if it complies to the following rules: * Consists of exactly 13 digits * The first 7 digits represent a valid past date in the format DDMMYYY * The last digit of the UMCN passes a checksum test """ default_error_messages = { 'invalid': _(u'This field should contain exactly 13 digits.'), 'date': _(u'The first 7 digits of the UMCN must represent a valid past date.'), 'checksum': _(u'The UMCN is not valid.'), } def __init__(self, *args, **kwargs): kwargs['min_length'] = None kwargs['max_length'] = 13 super(UMCNField, self).__init__(r'^\d{13}$', *args, **kwargs) def clean(self, value): value = super(UMCNField, self).clean(value) if value in EMPTY_VALUES: return u'' if not self._validate_date_part(value): raise ValidationError(self.error_messages['date']) if self._validate_checksum(value): return value else: raise ValidationError(self.error_messages['checksum']) def _validate_checksum(self, value): a,b,c,d,e,f,g,h,i,j,k,l,K = [int(digit) for digit in value] m = 11 - (( 7*(a+g) + 6*(b+h) + 5*(c+i) + 4*(d+j) + 3*(e+k) + 2*(f+l)) % 11) if (m >= 1 and m <= 9) and K == m: return True elif m == 11 and K == 0: return True else: return False def _validate_date_part(self, value): daypart, monthpart, yearpart = int(value[:2]), int(value[2:4]), int(value[4:7]) if yearpart >= 800: yearpart += 1000 else: yearpart += 2000 try: date = datetime.datetime(year = yearpart, month = monthpart, day = daypart).date() except ValueError: return False if date >= datetime.datetime.now().date(): return False return True
Regarding taking the birth date and gender from the entered value. In practice we have found that the best way to use this feature of the UMCN is to calculate the values with JavaScript and offer them as a choice but still allow users to change them. This is because there have been cases where a person has been given a UMCN with a wrong gender or birth date indication at birth. Another solution I would propose is to have a model field which populates other two fields on the model object for gender and birth date in a similar fashion as the ImageField populates fields for height and width. IMO this is better as it's a technique used in the core of Django.
comment:5 by , 13 years ago
Patch needs improvement: | set |
---|
Patch needs improvement as per vasiliyeah's comment.
follow-up: 7 comment:6 by , 13 years ago
Patch needs improvement: | unset |
---|
Hey vasiliyeah, good feedback.
First of all, since apparently UMCN has the same implementation across YU countries, we could share some code. Don't know how this touches Django's internal policy of handling localflavor.
I made following fixes to the patch:
- more PEP8 friendly
- additional tests for info dictionary
- additional postecode tests
- remove all whitespaces in phonenumber before validating
- assume year is <2000 if birth year is 890 or higher
- EMSO non-valid dates and dates in future raise validation error
- ESMO validation errors are now elaborate
- removed redundand error message in SIPostalCodeField
- removed trailing spaces in docs
According to http://www.uradni-list.si/1/objava.jsp?urlid=19998&stevilka=345, every resident in Slovenia should have a valid EMSO. And if not, valid one should be issued.
I would say storing computed information to self.info will give user most freedom how to use it further since there are so many use cases.
PS: It would remove lots of code django currently uses internally by switching to https://github.com/daviddrysdale/python-phonenumbers.
PPS: I wonder if it makes sense validating the date, since there is a slight possibility of incorrectly issued ESMO?
follow-up: 8 comment:7 by , 13 years ago
- Regarding the birth year, I'd just asume <2000 if it's 800 or higher just to be safe.
- Regarding validating that because of wrong birth date in the UMCN - these cases were rare and as I understand they were just one or two days off. I haven't heard of a case where someone was given a UMCN with a date years in the future.
One more thing I would suggest is applying ugettext_lazy to the regions/cities names in SI_POSTALCODES. That way if a region has a different name in English or German for example a user using a localized version of the site can see the appropriate names in their locale in the ChoiceField.
Regarding phone numbers. Some people like to put them in the localflavors. You can always use python-phonenumbers in your application, I don't think adding it as a dependency to Django will ever happen.
follow-up: 9 comment:8 by , 13 years ago
Replying to anonymous:
- Regarding the birth year, I'd just asume <2000 if it's 800 or higher just to be safe.
Even the oldest person still alive is not old enough, should be safe. If you insist, we can change it to 800.
- Regarding validating that because of wrong birth date in the UMCN - these cases were rare and as I understand they were just one or two days off. I haven't heard of a case where someone was given a UMCN with a date years in the future.
Roger.
One more thing I would suggest is applying ugettext_lazy to the regions/cities names in SI_POSTALCODES. That way if a region has a different name in English or German for example a user using a localized version of the site can see the appropriate names in their locale in the ChoiceField.
Posts are not translated to any other language, I would say i18n them is redundant.
Regarding phone numbers. Some people like to put them in the localflavors. You can always use python-phonenumbers in your application, I don't think adding it as a dependency to Django will ever happen.
Makes sense.
comment:9 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
Replying to iElectric:
Replying to anonymous:
- Regarding the birth year, I'd just asume <2000 if it's 800 or higher just to be safe.
Even the oldest person still alive is not old enough, should be safe. If you insist, we can change it to 800.
- Regarding validating that because of wrong birth date in the UMCN - these cases were rare and as I understand they were just one or two days off. I haven't heard of a case where someone was given a UMCN with a date years in the future.
Roger.
Your implementation works fine with 10k examples including what they give here to foreign nationals. Info gets extracted fine.
One more thing I would suggest is applying ugettext_lazy to the regions/cities names in SI_POSTALCODES. That way if a region has a different name in English or German for example a user using a localized version of the site can see the appropriate names in their locale in the ChoiceField.
Posts are not translated to any other language, I would say i18n them is redundant.
Maybe. That's up to you.
Patch applies and all tests run fine.
comment:10 by , 13 years ago
During client presentation I found out we should strip ' -/' in phone numbers before validating, giving *smart* users less chance to fail. Patch fixed and tests updated.
comment:13 by , 13 years ago
Patch needs improvement: | set |
---|---|
Resolution: | fixed |
Status: | closed → reopened |
Triage Stage: | Ready for checkin → Accepted |
In [16669]: There are a bunch of problems with this patch that I didn't pick up at time of commit -- tests don't pass, and it doesn't reflect some recent changes in the testing tools. Patch needs to be updated.
comment:14 by , 13 years ago
test_SIPostalCodeSelect
doesn't test anything, btw. (See https://code.djangoproject.com/browser/django/trunk/tests/regressiontests/forms/localflavor/si.py?rev=16662#L90)
follow-up: 16 comment:15 by , 13 years ago
Seems like last line was stripped. Should current patch be updated or create new one on top of trunk?
comment:16 by , 13 years ago
comment:17 by , 13 years ago
Patch updated to current trunk.
BTW, Domen Kozar and iElectric are the same person :)
Slovenian localflavor