Opened 14 years ago

Closed 12 years ago

Last modified 11 years ago

#13342 closed Bug (wontfix)

MultiValueField - incorrectly use 'required' attribute of MultiValueField for child field validation

Reported by: krejcik Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords: fields required MultiValueField
Cc: flisky Triage Stage: Unreviewed
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description (last modified by Russell Keith-Magee)

MultiValueField is composed from several child fields
Validation logic in it's clean method should be:

  • if MultiValueField is required, at least one of the child values must be non-empty (ok)
  • if a child field is required, it's value must be non-empty (bug in implementation, MultiValueField.required is used instead of child.required to check condition)
  • clean decompressed values by each child fields and compress together (ok)

Attachments (1)

fields.patch (613 bytes ) - added by krejcik 14 years ago.

Download all attachments as: .zip

Change History (6)

by krejcik, 14 years ago

Attachment: fields.patch added

comment:1 by Russell Keith-Magee, 14 years ago

Description: modified (diff)
milestone: 1.2
Resolution: wontfix
Status: newclosed

The proposed behavior strikes me as a fairly major change to the existing behavior, but it isn't clear to me that the existing behaviour is wrong, or that the proposed behavior is more correct.

The report and patch doesn't give any test cases to validate why the proposed change would be correct, so I'm closing wontfix. If you want to argue your case, please bring it up on Django-developers.

Regardless of the decision, the ticket isn't 1.2 critical.

comment:2 by fero, 12 years ago

Easy pickings: unset
Resolution: wontfix
Severity: Normal
Status: closedreopened
Type: Bug
UI/UX: unset

Is it allowed to reopen this ticket please?
Sorry if it is not the right procedure, but I'd like to post some notes about this ticket and consequent decision made.

Behaviour proposed by krejcik sounds good to me because when I have a required MultiValueField, my idea is to require that the whole stuff that resides inside validates. Not each single piece, but as one big picture. What there is inside is not myproblem.

Each field that are inside the MultiValueField knows by itself if it is required or not, and because of this they raises validation errors or not.

My example is a PlaceField(forms.MultiValueField) developed to set an address which is composed by:

  • Name
  • Address
  • ZIP
  • City
  • State

Name or address could be blank (not at the same time both), so can be ZIP.

My needing is that a place MUST be specificed, so PlaceField must be a required=True field,
BUT to specify a place is NOT REQUIRED to specify address and ZIP so I SHOULD have the ability
to tell that nested fields address and ZIP are not required, and, IMHO, Django must respect the constraint.

My PlaceField sounds like this

class PlaceField(forms.MultiValueField):
    widget = PlaceWidget
    def __init__(self, *args, **kw):
        fields = (
            forms.IntegerField(), # was: label=_("id")
            forms.CharField(label=_("name")),
            forms.CharField(label=_("address"), required=False),
            forms.CharField(label=_("zip"), required=False),
            forms.CharField(label=_("city")),
            forms.ChoiceField(label=_("state"),choices=PlaceField.STATE_CHOICES),
        )
        super(PlaceField, self).__init__(fields, *args, **kw)

or am I totally wrong?

in reply to:  2 comment:3 by Łukasz Rekucki, 12 years ago

Resolution: wontfix
Status: reopenedclosed

The best practice for "won't fix" tickets is what Russel said: "please bring it up on Django-developers". Especially with a ticket this old. Personally, I think that the current behaviour is at least non-obvious, so it should be documented more explicitly. But we can discuss this further on the mailing list.

comment:4 by flisky, 12 years ago

Cc: flisky added

comment:5 by Tai Lee, 11 years ago

Refs #15511.

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