Opened 4 years ago

Closed 4 years ago

#16202 closed New feature (fixed)

Localflavor support for Slovenia (si)

Reported by: Jure Cuhalev <gandalf@…> Owned by: nobody
Component: contrib.localflavor Version: 1.3
Severity: Normal Keywords:
Cc: iElectric, 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)

ticket16202.diff (42.9 KB) - added by Jure Cuhalev <gandalf@…> 4 years ago.
Slovenian localflavor
ticket160202_2.diff (44.0 KB) - added by iElectric 4 years ago.
Slovenian localflavor
ticket160202_3.diff (44.1 KB) - added by iElectric 4 years ago.
More freedom specifying phone numbers
ticket16202_4.diff (43.5 KB) - added by iElectric 4 years ago.
Update patch to fix upstream tests changes

Download all attachments as: .zip

Change History (23)

Changed 4 years ago by Jure Cuhalev <gandalf@…>

Slovenian localflavor

comment:1 Changed 4 years ago by aaugustin

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

comment:2 Changed 4 years ago by iElectric

  • Cc iElectric added

comment:3 Changed 4 years ago by vasiliyeah

  • Cc vvangelovski@… added

comment:4 Changed 4 years ago by vasiliyeah

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 Changed 4 years ago by julien

  • Patch needs improvement set

Patch needs improvement as per vasiliyeah's comment.

Changed 4 years ago by iElectric

Slovenian localflavor

comment:6 follow-up: Changed 4 years ago by iElectric

  • 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?

comment:7 in reply to: ↑ 6 ; follow-up: Changed 4 years ago by anonymous

  • 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.

comment:8 in reply to: ↑ 7 ; follow-up: Changed 4 years ago by 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.


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 in reply to: ↑ 8 Changed 4 years ago by vasiliyeah

  • Triage Stage changed from Accepted to 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 Changed 4 years ago by anonymous

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.

Changed 4 years ago by iElectric

More freedom specifying phone numbers

comment:11 Changed 4 years ago by anonymous

Anything else to be done here? I guess targeted release is 1.4

comment:12 Changed 4 years ago by russellm

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

In [16662]:

Fixed #16202 -- Added a Slovenian localflavor. Thanks to Jure Cuhalev <gandalf@…>, Gasper Zejn, Domen Kozar and iElectric for the patch.

comment:13 Changed 4 years ago by russellm

  • Patch needs improvement set
  • Resolution fixed deleted
  • Status changed from closed to reopened
  • Triage Stage changed from Ready for checkin to 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:15 follow-up: Changed 4 years ago by iElectric

Seems like last line was stripped. Should current patch be updated or create new one on top of trunk?

comment:16 in reply to: ↑ 15 Changed 4 years ago by julien

Replying to iElectric:

Seems like last line was stripped. Should current patch be updated or create new one on top of trunk?

Please resubmit an updated patch. Also note that, as of r16680, the dedicated place for localflavor tests is source:django/trunk/tests/regressiontests/localflavor/

Changed 4 years ago by iElectric

Update patch to fix upstream tests changes

comment:17 Changed 4 years ago by iElectric

Patch updated to current trunk.

BTW, Domen Kozar and iElectric are the same person :)

comment:18 Changed 4 years ago by julien

  • Triage Stage changed from Accepted to Ready for checkin

Thanks!

comment:19 Changed 4 years ago by julien

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

In [16706]:

Fixed #16202 -- Added a Slovenian localflavor. Thanks to Jure Cuhalev < gandalf@…>, Gasper Zejn and Domen Kozar for the patch.

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