Opened 7 years ago

Closed 5 years ago

Last modified 4 years ago

#7287 closed Uncategorized (fixed)

Newforms' initial values as models, for ModelChoiceField etc

Reported by: Simon Litchfield <simon@…> Owned by: nobody
Component: Documentation Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: no Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

ModelChoiceField cleans form data into model instances, but forms don't accept model instances as initial values. This seems a bit inconsistent to me. Patch attached.

Attachments (1)

newforms-initial-model.diff (821 bytes) - added by Simon Litchfield <simon@…> 7 years ago.
Cleans initial values that are model instances into their pk values

Download all attachments as: .zip

Change History (16)

Changed 7 years ago by Simon Litchfield <simon@…>

Cleans initial values that are model instances into their pk values

comment:1 Changed 7 years ago by Simon Litchfield <simon@…>

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

Another inconsistency. If my choices are of type int, clean should bring them back to int. At the moment, it seems to leave them as unicode.

comment:2 Changed 7 years ago by ericholscher

  • milestone set to 1.0
  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 7 years ago by mtredinnick

  • Has patch set
  • milestone changed from 1.0 to 1.0 maybe
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Design decision needed to Accepted

That patch is not cool. Importing all of django.db.models just for rendering a widget isn't a good idea, since you might be using forms standalone. Plus it's a leaky abstraction. We've tried very hard to keep that leakiness in django.forms.models in that part of the code. If that import is the price to be paid for using instances as default data, then you can't use instances there. If you can do it without having to import models, the enhancement has a chance (although a hasattr test would look a bit ugly, too, so I'm coming up a bit blank on how to do it).

comment:4 Changed 7 years ago by sime

Yep no worries wasn't aware of that. Maybe we throw it into django.forms.models instead then?

comment:5 Changed 7 years ago by wwinham

I've been bitten by this inconsistency myself. I think maybe an easy way to clear it up would be to edit http://www.djangoproject.com/documentation/forms/#initial and add some initial values for non-text fields so that it's obvious that you can't just pass in models.

comment:6 Changed 7 years ago by jacob

  • Component changed from Forms to Documentation
  • milestone changed from 1.0 maybe to post-1.0

A note in the docs is probably the right approach here.

comment:7 Changed 7 years ago by mrmachine

Also bitten by this, and took a while to track down the real cause. I'm storing cleaned data for a multi-page form in the session, and using that as initial data when users go back to a previous page. When returning to a previous page, ModelChoiceField's were suddenly using the cleaned model instance's __unicode__ as the form widget's value, which raised ValueError: invalid literal for int() with base 10 (see #8974). It makes sense that I should be able to pass back in the cleaned_data that I'm getting out of a form.

comment:8 Changed 7 years ago by bradcater

I don't think that this is going to be solved by documentation. I've had the "ValueError: invalid literal for int() with base 10" error on a multi-form page (not a multi-page form, in my case :) ), also, and I managed to get rid of it by changing db/models/forms/init.py (line ~356) to:

def get_db_prep_value(self, value):
    if value is None:
        return None
        try:
            return int(value)
        except:
            return int(value.pk)

I then had a problem with the Select widget comparing each c in "choices" to the contents of the set "selected_choices." The real issue was that c could be any one of several types, but you have to compare to the db values (primary keys, in my case) for equality (list membership). I got around that by subclassing the Select widget with something like this:

class GoodSelect(Select):
    """ Fix the ForeignKey bug.
        It's a known issue: http://code.djangoproject.com/ticket/7287 """
    def render_option(self, option_value, option_label, selected_choices):
        selected_html = (option_value in selected_choices) and u' selected="selected"' or ''
        return u'<option value="%s"%s>%s</option>' % (
            escape(option_value), selected_html,
            conditional_escape(force_unicode(option_label)))

    def render_options(self, choices, selected_choices):
        # Normalize to strings.
        selected_choices = set([self.__pk_or_unicode(v) for v in selected_choices])
        output = []
        for option_value, option_label in chain(self.choices, choices):
            if isinstance(option_label, (list, tuple)):
                output.append(u'<optgroup label="%s">' % escape(force_unicode(option_value)))
                for option in option_label:
                    output.append(self.render_option(*option))
                output.append(u'</optgroup>')
            else:
                output.append(self.render_option(option_value, option_label, selected_choices))
        return u'\n'.join(output)

    def __pk_or_unicode(self,obj):
        # If the object is already unicode, try to convert it to an int first.
        # If that doesn't work, then just return the force_unicode bit.
        # This is *probably* safe because when we try to put stuff in the db, we convert it anyway.
        if isinstance(obj,unicode):
            try:
                return int(obj)
            except:
                return force_unicode(obj)
        elif isinstance(obj,str):
            return force_unicode(obj)
        else:
            return obj.pk

I'm still exploring to come up with a better fix, but for now, this is functioning well for me.

comment:9 Changed 7 years ago by guettli

  • Cc hv@… added

comment:10 Changed 7 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:11 Changed 5 years ago by timo

  • Has patch unset
  • Needs documentation set
  • Needs tests unset
  • Patch needs improvement unset

removing has_patch since the patch isn't a doc patch

comment:12 Changed 5 years ago by zimnyx

Patch should be aware of "to_field_name" param for ModelChoiceField, which is pk by default, but can be overriden by this param.

comment:13 Changed 5 years ago by russellm

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

(In [13577]) Fixed #13679, #13231, #7287 -- Ensured that models that have ForeignKeys/ManyToManyField can use a a callable default that returns a model instance/queryset. #13679 was a regression in behavior; the other two tickets are pleasant side effects. Thanks to 3point2 for the report.

comment:14 Changed 5 years ago by russellm

(In [13578]) [1.2.X] Fixed #13679, #13231, #7287 -- Ensured that models that have ForeignKeys/ManyToManyField can use a a callable default that returns a model instance/queryset. #13679 was a regression in behavior; the other two tickets are pleasant side effects. Thanks to 3point2 for the report.

Backport of r13577 from trunk.

comment:15 Changed 4 years ago by guettli

  • Cc hv@… removed
  • Easy pickings unset
  • Severity set to Normal
  • Type set to Uncategorized
  • UI/UX unset
Note: See TracTickets for help on using tickets.
Back to Top