Opened 8 years ago

Closed 8 years ago

Last modified 7 years ago

#4036 closed (fixed)

Spanish (es) localflavor

Reported by: ricardojbarrios@… Owned by: garcia_marc
Component: contrib.localflavor Version: master
Severity: Keywords: spanish es l10n i18n localflavor
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Hi, I've cerated some spanish localflavour code. The tests are in test.py.

I'm working in some extra code, but i'll release it when it's done.

Attachments (7)

es.diff (16.3 KB) - added by Simon G. <dev@…> 8 years ago.
Patch from ricardojbarrios@.. that I've just svn diff'ed
es_provinces.py (1.3 KB) - added by garcia_marc 8 years ago.
contrib/localflavor/es/es_provinces.py (Spain provinces file)
es_regions.py (600 bytes) - added by garcia_marc 8 years ago.
contrib/localflavor/es/es_regions.py (Spain regions file)
forms.py (1.9 KB) - added by garcia_marc 8 years ago.
contrib/localflavor/es/forms.py (Spain NIF validation function)
4036.diff (15.5 KB) - added by oggie_rob 8 years ago.
New patch with simplified Identity number field & error handling
4036_2.diff (9.1 KB) - added by garcia_marc 8 years ago.
spanish localflavor files
4036_3.diff (23.8 KB) - added by garcia_marc 8 years ago.
spanish localflavor files

Download all attachments as: .zip

Change History (22)

comment:1 Changed 8 years ago by ricardojbarrios@…

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

The system doesn't leave me upload a file, so i've upload it to an external server, in http://www.organicadtm.com/localflavour_es.zip

Changed 8 years ago by Simon G. <dev@…>

Patch from ricardojbarrios@.. that I've just svn diff'ed

comment:2 Changed 8 years ago by Simon G. <dev@…>

  • Has patch set
  • Keywords spanish l10n i18n localflavor added; spanis removed
  • Summary changed from Spanish localflavour to Spanish (es) localflavour
  • Triage Stage changed from Unreviewed to Ready for checkin

comment:3 Changed 8 years ago by mtredinnick

  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

I don't like the error messages in the identity card number class. If they enter an invalid number, you can't guess at what the right number should be -- the error messages here are assuming it's the check digit that is wrong. That's not a valid assumption, since they may have enterred any one or more of the previous characters incorrectly. It should report that the number is incorrect, only. Then the person entering the data can enter the correct value. Prompting the user with a potentially still incorrect value will increase the chances that they'll just use that wrong one instead.

Before we can check this in:

  • Please remove the show_correct option.
  • Raise an AssertionError (or just use an "assert" statement) if both nif and cif are not specified, which was one of the "fixme" items in the code.
  • Some of the validation statements assume that taking int() of one of the entries will never fail. This isn't true: int('a'), for example. A test that checks that f.clean("elephant") for each of the card types gives a validation error (rather than a Python programming error) would be a good idea.

comment:4 Changed 8 years ago by ricardojbarrios@…

The problems with show_correct option can be easily overrided with a more clean error mesage.

The checks are done because previously we have a regex that check that it's one of the correct format. For example, if we use f.clean("elephant") it will raise a u'Please, enter a valid CIF, NIF or NIE.', so, when i use a int i know that it ALWAIS is going to be a int. ;)

I'll add the AssertionError and the new checks today or tomorrow.

comment:5 Changed 8 years ago by mtredinnick

Unfortunately, the show_correct option cannot be fixed. This is because there are many possible correct options for every incorrect string. Recomputing the check digit is only one possibility. So show_correct really means "show a guess", which is misleading.

comment:6 Changed 8 years ago by oggie_rob

  • Owner changed from nobody to oggie_rob

I should be able to make those changes & fix the test location.

Changed 8 years ago by garcia_marc

contrib/localflavor/es/es_provinces.py (Spain provinces file)

Changed 8 years ago by garcia_marc

contrib/localflavor/es/es_regions.py (Spain regions file)

Changed 8 years ago by garcia_marc

contrib/localflavor/es/forms.py (Spain NIF validation function)

comment:7 Changed 8 years ago by garcia_marc

  • Needs tests set
  • Summary changed from Spanish (es) localflavour to Spanish (es) localflavor

I've upload some files for this ticket.
es_provinces.py and es_regions.py should replace existing es_province.py. That's because old file has spanish names, new ones have translatable english names. Also I've changed ISO codes for oficial codes. Regions file didn't exist.

forms.py has a new implementation of NIF validation, that (hopefully) solves old one issues. It should be merged to old forms.py and tested again if after discusing, this one is better for checking in.

oggie_rob I can do that if you want.

Changed 8 years ago by oggie_rob

New patch with simplified Identity number field & error handling

comment:8 Changed 8 years ago by oggie_rob

Using Malcolm's comments and starting with ricardojbarrios' code, and looking through Wikipedia for examples of how this is supposed to work, I think I've come up with reasonable validation methods.

It would help if somebody who actually has a NIE or knows how they work took a look - I wasn't sure whether 7 or 8 digits were the norm.

Unfortunately I wasn't able to post the changes in before garcia_marc submitted patches... oh well. Somewhere in there is an appropriate solution.

comment:9 follow-up: Changed 8 years ago by garcia_marc

Hi oggie_rob, firs of all sorry for submitting a patch without letting you know that I was working on that, but the fact is that I couldn't find this ticket until I finished my work.

I've checked your code and there is something that IMHO shouldn't be like that, and it's arguments Nif and Cif. Whan you have to validate a NIF field, usually is the customer or the provider in an invoice, and you cannot know in advance if you are going to sell something to a company (that has a CIF code), or to a individual (that has a NIF/NIE code). So I think that you cannot pass an argument to specify it, you have to check it inside the function.

Anyway, I think that you're using some code better than mine, so any kind of merge of both functions would be great. If you want me to assign this ticket to myself, and make those final stepe, please let me know.

comment:10 in reply to: ↑ 9 ; follow-up: Changed 8 years ago by oggie_rob

Replying to garcia_marc:

Hi oggie_rob, firs of all sorry for submitting a patch without letting you know that I was working on that, but the fact is that I couldn't find this ticket until I finished my work.

I've checked your code and there is something that IMHO shouldn't be like that, and it's arguments Nif and Cif. Whan you have to validate a NIF field, usually is the customer or the provider in an invoice, and you cannot know in advance if you are going to sell something to a company (that has a CIF code), or to a individual (that has a NIF/NIE code). So I think that you cannot pass an argument to specify it, you have to check it inside the function.

Anyway, I think that you're using some code better than mine, so any kind of merge of both functions would be great. If you want me to assign this ticket to myself, and make those final stepe, please let me know.

Hi garcia_marc,
My greatest concern about this is that I'm not sure I got the specs right. I learned about these values by looking at the original patch and by searching online for specifications (mainly from Wikipedia, as I couldn't find many technical references). Also Spanish is not my first language, although I feel that I understood most of the articles well enough. Having said that, it seems to match up with what I had seen online.

Here are some of the things I considered when I was writing this:

  1. As opposed to the original code, normalization and validation should be the only functions of the Identity card field. We shouldn't suggest any possible answers, as people really should know their own NIF in full
  2. Translating the province names seems unnecessary and potentially a lot of work, so I left them in unicode (rather than ugettext)
  3. There are situations where users know if they will expect a NIF/NIS or CIF, and so the author should be able to limit those choices. However, by default both schemes are allowed. Your example of an invoice is one where you need to allow both types, but in another example (e.g. an employee form) you might only expect a NIF/NIS. It doesn't hurt to leave as-is.

If you would like to take it over, that would be fine by me. I think that including the two-digit province code is an improvement, although the two-character lookup is also handy. Something that allows searching of both of those would be ideal.

Thanks,

-rob

comment:11 in reply to: ↑ 10 ; follow-up: Changed 8 years ago by garcia_marc

  • Owner changed from oggie_rob to garcia_marc

Hi,

I absolutly agree on point 1, of course nobody can guess which character is wrong when checksum fails (may be in more sofisticated algorithms, but not this one). I didn't guess it in my code.

I disagree on point 2, for example, I think that an english speaker want to see "Basque Country" as the region, he wouldn't understant spanish or basque name, alse people from Spain usually say it as "País Vasco", that often doesn't like to basques, that prefer to say it "Euskadi", the basque name. And many other cases like that. In my case I use django in catalan, and it will be annoying for me having spanish names for catalan places in django, after the huge work of translating application. I know that it's further work for translators; I recently translated many swedish, german, holland... city names to Catalan and Spanish, so I know it closely, but I think that it is a must.

About point 3, I just misunderstood that there were an extra feature, I thought that you had to specify it; so I agree with you.

I've assigned this ticket to me, so I want to review code (so as far as I understand I'm more confortable with spanish stuff than you), and if you agree on point 2, I'll replace provinces and regions files.

Anyway I hope having your help on it, so I'm sure that your knowledge on hacking django is better than mine.

Thanks,

Marc

comment:12 in reply to: ↑ 11 Changed 8 years ago by oggie_rob

Replying to garcia_marc:

I disagree on point 2, for example, I think that an english speaker want to see "Basque Country" as the region, he wouldn't understant spanish or basque name, alse people from Spain usually say it as "País Vasco", that often doesn't like to basques, that prefer to say it "Euskadi", the basque name. And many other cases like that. In my case I use django in catalan, and it will be annoying for me having spanish names for catalan places in django, after the huge work of translating application. I know that it's further work for translators; I recently translated many swedish, german, holland... city names to Catalan and Spanish, so I know it closely, but I think that it is a must.

Yep, those are good reasons for translating!

I've assigned this ticket to me, so I want to review code (so as far as I understand I'm more confortable with spanish stuff than you), and if you agree on point 2, I'll replace provinces and regions files.

That would be great. Thanks.

-rob

Changed 8 years ago by garcia_marc

spanish localflavor files

comment:13 Changed 8 years ago by garcia_marc

  • Patch needs improvement unset

Finally I uploaded an updated patch for this ticket.

I've added some new fields (phone number and bank account), and I modified some features of old NIF/CIF functions. Main changes are:

  • When NIF or NIE, it does not check number length (most NIFs have 8 digits, but actually it is a sequential number that started on 1).
  • When CIF, it is valid both number or letter checksum, so there is no official documentation on it, and different authors have different opinions on which company types can have both checksum types.
  • I left just a parameter for allowing only nif or nie values (not cif). Allowing only cif values is meaningless, so if idea is restricting just companies, there is a type that uses owners nif as identity number. I cannot imagine any case that somebody would want to avoid this case of companies in validation.
  • Message errors are more detiled now.

This patch is well tested using forms, but there are missing regression tests, so I don't know if there are based on any documentation that I cannot found, or using any script. Could anybody help me on it please? After that and any commiter review it'll be ready for check in.

Changed 8 years ago by garcia_marc

spanish localflavor files

comment:14 Changed 8 years ago by garcia_marc

  • Needs tests unset
  • Triage Stage changed from Accepted to Ready for checkin

Development is finished and testing is done, so just are missing a commiter review and check it in.

comment:15 Changed 8 years ago by mtredinnick

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

(In [6555]) Fixed #4036 -- Added Spanish localflavor. Thanks, ricardojbarrios@… and
oggie_rob.

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