Code

Opened 7 years ago

Closed 4 years ago

Last modified 4 years ago

#5327 closed (fixed)

ModelChoiceField and ChoiceField clean methods behave differently

Reported by: danielrubio Owned by: PhiR
Component: Documentation Version: master
Severity: Keywords: ChoiceField clean method id
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description (last modified by mtredinnick)

Using both ChoiceField and ModelChoiceField, I discovered a bug in ChoiceField clean method ( or a discrepancy in behaviour)

ModelChoiceField seems to be working as expected, when I call the clean method in a template like so: form.clean.city , I get the city name(e.g New York), or what would be string inside the tag <select id="5"> New York </select>

This behaviour is different if the values are inside a ChoiceField, if I use the following in the template: form.clean.city, I get the city id (e.g 5 ), not the expected string or behaviour as using ModelChoiceField.

[NOTE: Not calling the clean field, in either !ChoiceField or !ModelChoiceField works identcally, generating a select list ]

I modified the fields.py file in django/newforms, the clean method on the ChoiceField class[line 466], would now read:

 def clean(self, value):
        """
        Validates that the input is in self.choices.
        """
        value = super(ChoiceField, self).clean(value)
        if value in EMPTY_VALUES:
            value = u''
        value = smart_unicode(value)
        if value == u'':
            return value
        valid_values = set([smart_unicode(k) for k, v in self.choices])
        if value not in valid_values:
            raise ValidationError(ugettext(u'Select a valid choice. That choice is not one of the available choices.'))
        else:
            value = self._choices[int(value)][1]
        return value

Only modification is the 'else' at the end, which would assign the value to the actual string.


Attachments (1)

5327.diff (1.6 KB) - added by PhiR 7 years ago.
clarification of the docs concerning ModelChoices

Download all attachments as: .zip

Change History (14)

comment:1 Changed 7 years ago by danielrubio

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Did some more testing, the previous fix assumed the option keys were in order 1,2,3,4,5,6,7,8,9.etc.. I had a list ordered by name, and the previous code obviosuly gave the wrong key value. The following snippet will fix this.

if value not in valid_values:

raise ValidationError(ugettext(u'Select a valid choice. That choice is not one of the available choices.'))

else:

for k, v in self.choices:

if(smart_unicode(k) == value):

return smart_unicode(v)

comment:2 Changed 7 years ago by PhiR

  • Needs documentation set
  • Owner changed from nobody to PhiR
  • Triage Stage changed from Unreviewed to Design decision needed

Logically the model choicefield should be able to return either an object or just its id, but that should be more explicit in the docs (it is not explicitely listed in the FormField list). Also, the ChoiceField doc doesn't say if the first or second member of the choice tuple is used as the normalized value.

Changed 7 years ago by PhiR

clarification of the docs concerning ModelChoices

comment:3 Changed 7 years ago by PhiR

  • Has patch set
  • Needs documentation unset
  • Status changed from new to assigned
  • Triage Stage changed from Design decision needed to Ready for checkin

comment:4 Changed 7 years ago by mtredinnick

  • Description modified (diff)

(fixed description formatting)

comment:5 Changed 7 years ago by mtredinnick

  • Summary changed from ChoiceField clean method to ModelChoiceField and ChoiceField clean methods behave differently

We should resolve the inconsistency here, I suspect. My gut feeling is that cleaning to the id value is more correct, because that is what you would assign to a field that had "choices" as an option. But whichever way we go, I'm not too comfortable with the inconsistency. ModelChoiceField subclasses ChoiceField, so it shouldn't behave wildly differently.

comment:6 Changed 7 years ago by mtredinnick

  • Triage Stage changed from Ready for checkin to Design decision needed

comment:7 Changed 7 years ago by PhiR

ModelChoiceField will NOT return a name, it will return the object associated with the id you selected. The only difference with a ChoiceField is that ModelChoiceField will resolve the object from the id in the queryset, which is to be expected given the field name. There is no inconsistency here, and the patch only clarifies the current documentation.

comment:8 Changed 7 years ago by Ramiro Morales

From what I idnerstand from the description, Daniel isn't reporting about ModelChoiceField but about what component of the relevant choices member the clean method of ChoiceField is returning.

Could this be then related to #5481?

comment:9 Changed 7 years ago by rcoup

#5481 for fixes ChoiceField choices keys in the general case.

IMHO PhiR's doc patch is correct:

  • ChoiceField normalises to the key, that is the generally expected behaviour - otherwise you get a translated string or something else back.
  • ModelChoiceField normalises to the model object. It will use the queryset to initialize the keys/strings and then returns back the model object from clean()

I'd suggest that using form.cleaned_data.city in a template isn't the best way to get a choice label. It would be relatively easy to create a filter or a tag to do it:

@register.simple_tag
def choice_value_label(form, field_name):
   return dict(form.fields[field_name].choices)[form.cleaned_data[field_name]

<span>{% choice_value form "city" %}</span>

comment:10 Changed 4 years ago by russellm

  • Component changed from Forms to Documentation
  • Triage Stage changed from Design decision needed to Accepted

Accepted that the documentation is what needs to be fixed here.

comment:11 Changed 4 years ago by gabrielhurley

Most of this patch (and ticket) was taken care of years ago. I'll merge in the last tiny bits and call it done.

comment:12 Changed 4 years ago by gabrielhurley

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

(In [14214]) Fixed #5327 -- Added standardized field information to ModelChoiceField and ModelMultipleChoiceField documentation. Thanks to danielrubio for the report and PhiR for the text.

comment:13 Changed 4 years ago by gabrielhurley

(In [14215]) [1.2.X] Fixed #5327 -- Added standardized field information to ModelChoiceField and ModelMultipleChoiceField documentation. Thanks to danielrubio for the report and PhiR for the text.

Backport of [14214] from trunk.

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.