Code

Opened 2 years ago

Closed 18 months ago

#17591 closed Bug (invalid)

USSocialSecurityNumberField doesn't validate the same when entered without hypens

Reported by: aaron.l.madison@… Owned by: nobody
Component: contrib.localflavor Version: 1.3
Severity: Normal Keywords: localflavor, ussocialsecuritynumberfield, localflavorsplit
Cc: aaron.l.madison@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: yes
Easy pickings: yes UI/UX: no

Description

In the us localflavor USSocialSecurityNumberField, if you enter an SSN without hyphens, the 'Woolworth' validations aren't run properly because they're explicity '078-05-1120' and '219-009-9999' instead of checking against the parts.

I refactored the field and created a patch that fixes it, plus added much better tests (in my opinion... added a test for each scenario).

Lastly, sometimes a user might NOT want to force the output to xxx-xx-xxxx. It might be useful to just leave it without hyphens. (in my case, need to pass ssn to a credit report service and will never need the dashes and I don't want to strip them out every time.) So, I added an optional keyword argument 'no_hyphen' to the ssn field which just forces the output to not have hyphens.

Please include or give feedback on the patch.

Thanks!

Attachments (2)

us_ssnfield_fix.patch (6.9 KB) - added by aaron.l.madison@… 2 years ago.
us ssn field patch
us_ssnfield_fix_v2.patch (66.2 KB) - added by anonymous 2 years ago.

Download all attachments as: .zip

Change History (6)

Changed 2 years ago by aaron.l.madison@…

us ssn field patch

comment:1 Changed 2 years ago by lrekucki

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

Few notes:

  • Python 2.5 (the minimal version supported right now) doesn't have "with" by default, you need from __future__ import with_statement. It also doesn't have the .format() method, so that won't work,
  • The tests are repetitive: you both added a case to the dictionary and a method. Just add all invalid cases to the dictionary, to keep it simple,
  • You don't have to write "bool(area == '666')": "area == '666'" is exactly the same. Same for any([]) - that just a needlesly complicated "or" expression,
  • The zeros_re - imho, there was nothing wrong with the previous check for zeroes, no reason to change it for a regexp,
  • Get rid of "_is_all_zeros()", "_raise_invalid", etc. methods. It doesn't save typing nor is more DRY, just makes the code longer.

If you want the "no_hypen" flag included, you also need to update the documentation for this field.

comment:2 Changed 2 years ago by anonymous

Thanks for your feedback. I've attached another patch.

To me the small methods are much less about DRY and much more about readability. A method that clearly says what it does doesn't need a comment to say what it does. Personal preference I'm sure.

Changed 2 years ago by anonymous

comment:3 Changed 2 years ago by aaron.l.madison@…

sorry for the spam... accidentally posted the patch and my comment as anonymous.

comment:4 Changed 18 months ago by aaugustin

  • Keywords localflavor, ussocialsecuritynumberfield, localflavorsplit added; localflavor ussocialsecuritynumberfield removed
  • Resolution set to invalid
  • Status changed from new 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.