Code

Opened 5 years ago

Closed 18 months ago

#10895 closed Bug (invalid)

Improvements in BRCPFField and BRCNPJField in localflavor.br.forms

Reported by: luizvital Owned by: waa
Component: contrib.localflavor Version: master
Severity: Normal Keywords: localflavor, brcpffield, brcpnjfield, br, localflavor, localflavorsplit
Cc: wiliamsouza83@…, semente+djangoproject@…, seocam, 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 5 years ago.
patch on r10620
localflavor_br_forms_r10620_updated.diff (7.4 KB) - added by luizvital 5 years ago.
Patch updated with ramalho's advices

Download all attachments as: .zip

Change History (17)

Changed 5 years ago by luizvital

patch on r10620

comment:1 Changed 5 years ago by luizvital

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

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 5 years ago by 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 follow-up: Changed 5 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 ; follow-ups: Changed 5 years ago by 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 5 years ago by 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 5 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 5 years ago by luizvital

Patch updated with ramalho's advices

comment:7 Changed 5 years ago by waa

  • Cc wiliamsouza83@… added
  • Keywords changed from brcpffield, brcpnjfield, br, localflavor to localflavor brcpffield, brcpnjfield, br, localflavor
  • Owner changed from nobody to waa
  • Status changed from new to assigned
  • Triage Stage changed from Unreviewed to Accepted

Good work!

comment:8 Changed 5 years ago by anonymous

  • Cc semente+djangoproject@… added

comment:9 Changed 5 years ago by seocam

  • Cc seocam added

comment:10 Changed 5 years ago by luizvital

  • Cc luiz.vital@… added

comment:11 Changed 4 years ago by chronos

  • Cc philipe.rp@… added

comment:12 Changed 3 years ago by julien

  • Patch needs improvement set
  • Severity set to Normal
  • Type set to Uncategorized

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

comment:13 Changed 3 years ago by julien

  • Type changed from Uncategorized to Bug

comment:14 Changed 3 years ago by semente

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

comment:15 Changed 18 months ago by aaugustin

  • Keywords localflavor, localflavor, localflavorsplit added; localflavor localflavor removed
  • Resolution set to invalid
  • Status changed from assigned to closed

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!

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.