Opened 8 years ago

Closed 4 years ago

#10895 closed Bug (invalid)

Improvements in BRCPFField and BRCNPJField in localflavor.br.forms

Reported by: luizvital Owned by: Wiliam Alves de Souza
Component: contrib.localflavor Version: master
Severity: Normal Keywords: localflavor, brcpffield, brcpnjfield, br, localflavor, localflavorsplit
Cc: wiliamsouza83@…, semente+djangoproject@…, Sergio Oliveira, luiz.vital@…, philipe.rp@…, semente+django@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

When using the fields BRCPFField and BRCNPJField in django.contrib.localflavor.br.forms I missed a uniform return format in the cleaned value of these fields, as they validate formatted and digits only inputs and the cleaned data preserves the same format as the entered value. If you're not enforcing a particular format with JavaScript, you may end up with a non standardized values on the database.

Ex.: For BRCPFField the values cleaned can be '999.999.999-99', '99999999999', '999999999-99', etc...

My suggestion is to keep the diversity of valid input formats, but provide a way to uniform the format of the cleaned value.

The implementation consists of adding two optional keyword arguments:

  • always_return_formatted: a boolean which indicates if the cleaned value must always be in a particular format, the default is False to prevent unexpected behavior to the current users of this FormField class.
  • return_format: a string to format the output, defaults to '%s.%s.%s-%s' in BRCPFField

In addition to these, I've changed the parent class of BRCNPJField from Field to CharField to take advantage of max_length and min_length validators.

Tests for these features included and fixed some errors in previous doctest that were expecting a unicode string.

Attachments (2)

localflavor_br_forms_r10620.diff (6.3 KB) - added by luizvital 8 years ago.
patch on r10620
localflavor_br_forms_r10620_updated.diff (7.4 KB) - added by luizvital 8 years ago.
Patch updated with ramalho's advices

Download all attachments as: .zip

Change History (17)

Changed 8 years ago by luizvital

patch on r10620

comment:1 Changed 8 years ago by luizvital

Forgot to mention that the patch includes additional validation of the BRCPFField by considering '000.000.000-00' and '999.999.999-99' as invalid inputs.

These rules can be checked in the Government site http://www.receita.fazenda.gov.br/Aplicacoes/ATCTA/cpf/ConsultaPublica.asp

comment:2 Changed 8 years ago by Luciano Ramalho

I support the patch, with two suggestions:

1) simplify the API and take only the return_format keyword arg, but make it default to None. If that argument is not given, then keep the current behaviour. No need for the other boolean keyword argument, and we are still backward compatible.

2) also reject 111.111.111-11 222.222.222-22... and so on, because any CPF composed of one digit repeated eleven times passes the check, but these are not real CPFs.

comment:3 Changed 8 years ago by luizvital

Ramalho,

1) I've thought about only having the parameter return_format defaulting to None, and the problem I see in this aproach is that there's no default return_format, obligating the user to think about the desired format, which unawered users may give wrong string format, leading to exceptions. Maybe a better error handling could solve this, as these exceptions could happen anyway. Any ideas?

2) Previously my code rejected that inputs too, but I checked http://www.receita.fazenda.gov.br/Aplicacoes/ATCTA/cpf/ConsultaPublica.asp for them and discovered they pass validation!!! Most of them are canceled or not used but they all pass the validation. Is it safe to reject them all?

comment:4 in reply to:  3 ; Changed 8 years ago by Luciano Ramalho

Replying to luizvital:

1) I've thought about only having the parameter return_format defaulting to None, and the problem I see in this aproach is that there's no default return_format, obligating the user to think about the desired format, which unawered users may give wrong string format, leading to exceptions. Maybe a better error handling could solve this, as these exceptions could happen anyway. Any ideas?


I agree, but this usability issue can be addressed by providing a couple of ready to use formatting strings as constants inside the field class definition, called DIGITS_ONLY and DOTS_AND_SLASH or something similar.


2) Previously my code rejected that inputs too, but I checked http://www.receita.fazenda.gov.br/Aplicacoes/ATCTA/cpf/ConsultaPublica.asp for them and discovered they pass validation!!! Most of them are canceled or not used but they all pass the validation. Is it safe to reject them all?


In this case "them all" amounts to exactly 8 CPFs (in addition to those two that you already exclude). I just checked and none of these 8 CPFs belong to real persons: 3 were cancelled (probably because the poor citizens could not use them) and the other 5 do not exist in the CPF database. But they are often abused by people who don't want to provide a real CPF, which partially defeats the purpose of validating the field. So I think the benefit of rejecting those 9 CPFs far outweights the remote possibility that a real person will assigned one of those numbers ever again.

comment:5 in reply to:  4 Changed 8 years ago by Luciano Ramalho

I the last phrase above I meant to say that I support "rejecting those 10 CPFs (including the 2 which the patched code already rejects)"

comment:6 in reply to:  4 Changed 8 years ago by luizvital

Replying to ramalho:

Replying to luizvital:

1) I've thought about only having the parameter return_format defaulting to None, and the problem I see in this aproach is that there's no default return_format, obligating the user to think about the desired format, which unawered users may give wrong string format, leading to exceptions. Maybe a better error handling could solve this, as these exceptions could happen anyway. Any ideas?


I agree, but this usability issue can be addressed by providing a couple of ready to use formatting strings as constants inside the field class definition, called DIGITS_ONLY and DOTS_AND_SLASH or something similar.


2) Previously my code rejected that inputs too, but I checked http://www.receita.fazenda.gov.br/Aplicacoes/ATCTA/cpf/ConsultaPublica.asp for them and discovered they pass validation!!! Most of them are canceled or not used but they all pass the validation. Is it safe to reject them all?


In this case "them all" amounts to exactly 8 CPFs (in addition to those two that you already exclude). I just checked and none of these 8 CPFs belong to real persons: 3 were cancelled (probably because the poor citizens could not use them) and the other 5 do not exist in the CPF database. But they are often abused by people who don't want to provide a real CPF, which partially defeats the purpose of validating the field. So I think the benefit of rejecting those 9 CPFs far outweights the remote possibility that a real person will assigned one of those numbers ever again.

I agree with all your points. Attaching an updated patch!
Thanks for the advices!

Changed 8 years ago by luizvital

Patch updated with ramalho's advices

comment:7 Changed 8 years ago by Wiliam Alves de Souza

Cc: wiliamsouza83@… added
Keywords: brcpffield, brcpnjfield, br, localflavorlocalflavor brcpffield, brcpnjfield, br, localflavor
Owner: changed from nobody to Wiliam Alves de Souza
Status: newassigned
Triage Stage: UnreviewedAccepted

Good work!

comment:8 Changed 8 years ago by anonymous

Cc: semente+djangoproject@… added

comment:9 Changed 7 years ago by Sergio Oliveira

Cc: Sergio Oliveira added

comment:10 Changed 7 years ago by luizvital

Cc: luiz.vital@… added

comment:11 Changed 7 years ago by Felipe 'chronos' Prenholato

Cc: philipe.rp@… added

comment:12 Changed 6 years ago by Julien Phalip

Patch needs improvement: set
Severity: Normal
Type: Uncategorized

Thanks for the great work. Could you now update the tests to use unittest?

comment:13 Changed 6 years ago by Julien Phalip

Type: UncategorizedBug

comment:14 Changed 5 years ago by Guilherme Gondim

Cc: semente+django@… added
Easy pickings: unset
UI/UX: unset

comment:15 Changed 4 years ago by Aymeric Augustin

Keywords: localflavorsplit added
Resolution: invalid
Status: assignedclosed

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!

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