Opened 5 years ago

Closed 4 years ago

Last modified 3 years ago

#14593 closed (fixed)

CZBirthNumberField expects two arguments to clean()

Reported by: idangazit Owned by: idangazit
Component: contrib.localflavor Version: 1.2
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

See cz/forms.py, line 53. The field's clean() takes two arguments, but clean() should only take one (value).

From looking at the source, it seems that the conditional block on L72 can simply be excised along with the invalid_gender error message. Without explicit knowledge of the associated gender, it looks like the additional gender-based validation logic isn't possible.

Attachments (2)

14593.diff (1.1 KB) - added by idangazit 4 years ago.
Deprecate gender checking in clean()
14593_PendingDeprecationWarning.diff (597 bytes) - added by idangazit 4 years ago.
Changes issued warning to PendingDeprecationWarning

Download all attachments as: .zip

Change History (11)

comment:1 Changed 5 years ago by idangazit

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Status changed from new to assigned

Looking at this some more, it seems like a nice solution would be to use a ComboField, consisting of two charfields: one for the number, one for the gender. Most of the validation that takes place today would happen on the ID number field. The gender charfield would simply validate that the value is either "m" or "f". The combofield's validation would take care of the interaction between the two.

Talked this idea over with alex_gaynor on IRC, he liked it, generally.

comment:2 Changed 4 years ago by Honza_Kral

  • milestone set to 1.3
  • Triage Stage changed from Unreviewed to Accepted

gender is not necessary to determine whether a birth number is valid, it just validates some further restrictions and shouldn't be part of the field's validation. It should remain for some time with a deprecation warning and will be removed in the future (probably replaced by a method that anybody can call manually from their form.clean for convenience).

Changed 4 years ago by idangazit

Deprecate gender checking in clean()

comment:3 Changed 4 years ago by idangazit

Above patch issues the agreed-upon warning, new unittest updated to silence said warning: https://github.com/idangazit/django/compare/cz_clean_fix#diff-1

comment:4 Changed 4 years ago by Honza_Kral

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

(In [15048]) Fixed #14593 -- Deprecating CZBirthNumberField's second argument to clean()

Gender is not necessary to determine whether a birth number is valid, it
just validates some further restrictions and shouldn't be part of the
field's validation.

comment:5 Changed 4 years ago by jezdez

This needs to be a either PendingDeprecationWarning (and noted in the deprecation docs) or a backwards incompatible change (and noted in the release notes).

comment:6 Changed 4 years ago by idangazit

  • Resolution fixed deleted
  • Status changed from closed to reopened

Oops, my bad. Preparing new patch with proper warning.

Changed 4 years ago by idangazit

Changes issued warning to PendingDeprecationWarning

comment:7 Changed 4 years ago by Alex

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

(In [15062]) Fixed #14593 -- change this warning to be a PendingDeprecationWarning. Also converted the Czech localflavor tests to be unittests. We have always been at war with doctests. Thanks to Idan Gazit.

comment:8 Changed 4 years ago by jacob

  • milestone 1.3 deleted

Milestone 1.3 deleted

comment:11 Changed 3 years ago by claudep

In [17939]:

Removed deprecated gender check in cz localflavor. Refs #14593.

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