Opened 6 years ago

Closed 6 years ago

Last modified 5 years ago

#14593 closed (fixed)

CZBirthNumberField expects two arguments to clean()

Reported by: Idan Gazit Owned by: Idan Gazit
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 Idan Gazit 6 years ago.
Deprecate gender checking in clean()
14593_PendingDeprecationWarning.diff (597 bytes) - added by Idan Gazit 6 years ago.
Changes issued warning to PendingDeprecationWarning

Download all attachments as: .zip

Change History (11)

comment:1 Changed 6 years ago by Idan Gazit

Status: newassigned

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 6 years ago by Honza Král

milestone: 1.3
Triage Stage: UnreviewedAccepted

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 6 years ago by Idan Gazit

Attachment: 14593.diff added

Deprecate gender checking in clean()

comment:3 Changed 6 years ago by Idan Gazit

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 6 years ago by Honza Král

Resolution: fixed
Status: assignedclosed

(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 6 years ago by Jannis Leidel

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 6 years ago by Idan Gazit

Resolution: fixed
Status: closedreopened

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

Changed 6 years ago by Idan Gazit

Changes issued warning to PendingDeprecationWarning

comment:7 Changed 6 years ago by Alex Gaynor

Resolution: fixed
Status: reopenedclosed

(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 5 years ago by Jacob

milestone: 1.3

Milestone 1.3 deleted

comment:11 Changed 5 years ago by Claude Paroz

In [17939]:

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

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