Code

Opened 4 years ago

Closed 2 years ago

Last modified 14 months 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: master
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 russellm)

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 4 years ago.

Download all attachments as: .zip

Change History (6)

Changed 4 years ago by krejcik

comment:1 Changed 4 years ago by russellm

  • Description modified (diff)
  • milestone 1.2 deleted
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to wontfix
  • Status changed from new to closed

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 follow-up: Changed 2 years ago by fero

  • Easy pickings unset
  • Resolution wontfix deleted
  • Severity set to Normal
  • Status changed from closed to reopened
  • Type set to 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?

comment:3 in reply to: ↑ 2 Changed 2 years ago by lrekucki

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

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 Changed 2 years ago by flisky

  • Cc flisky added

comment:5 Changed 14 months ago by mrmachine

Refs #15511.

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.