Opened 17 years ago

Closed 13 years ago

Last modified 7 years ago

#5247 closed (worksforme)

ModelMultipleChoiceField doesn't select initial choices

Reported by: uhlr@… Owned by: Alex Robbins
Component: Forms Version: dev
Severity: Keywords: ModelMultipleChoiceField SelectMultiple
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

When using a ModelMultipleChoiceField, initial values are not selected. After some investigation, I discovered that this was because SelectMultiple.render() was comparing the primary key of the choices to the unicode string of each value; this obviously only works if one's model returns the primary key, which is near-universally suboptimal.

I have modified SelectMultiple to call a method value_to_str() (perhaps not the best name) which defaults to returning the force_unicode() of the value; I've added a subclass ModelSelectMultiple which overrides this to return SelectMultiple.value_to_str(value._get_pk_val()). Finally, I modified ModelMultipleChoiceField to use a ModelSelectMultiple widget rather than a SelectMultiple.

This does what's expected, and is extensible in the future.

Attachments (5)

multipatch.patch (2.4 KB ) - added by uhlr@… 17 years ago.
svn diff for patch application
patch6702.diff (2.8 KB ) - added by Peter Baumgartner 16 years ago.
patch against #6702 that passes tests
patch.diff (1.6 KB ) - added by Peter Baumgartner 16 years ago.
better patch
test_patch_ticket5247_r11751.diff (1.7 KB ) - added by Tim Kersten 14 years ago.
patch_ticket5247_r11751.diff (922 bytes ) - added by Tim Kersten 14 years ago.

Download all attachments as: .zip

Change History (25)

by uhlr@…, 17 years ago

Attachment: multipatch.patch added

svn diff for patch application

comment:1 by uhlr@…, 17 years ago

Note also that in my SVN branch smart_unicode() is used; in my installed version force_unicode() is used instead. In my patch I use smart_unicode(); if it has gone away then simply replace it with force_unicode().

comment:2 by Philippe Raoult, 17 years ago

might be a duplicate of #5481 which also has a patch.

comment:3 by Robert Coup, 17 years ago

It's not a duplicate, but its semi-relevant. In #5481, whatever ends up as keys in ChoiceField.choices gets faithfully returned as cleaned_data - be it integer, string, object, etc.

Should creating keys be the widgets responsibility or the fields?

by Peter Baumgartner, 16 years ago

Attachment: patch6702.diff added

patch against #6702 that passes tests

comment:4 by Luke Plant, 16 years ago

Needs tests: set
Patch needs improvement: set

Just by looking at them, you can see the patches can't work. ModelMultipleChoiceField is defined as a function when it should be a class. In the latest patch, it is then imported into another model and never used!

To determine correctness, there should be a test which fails without the patch, and passes with it.

comment:5 by Peter Baumgartner, 16 years ago

Needs tests: unset
Patch needs improvement: unset

OK, here's a better patch, although I'm not sure it is the right one. It works for me but I'm not sure that testing for _get_pk_val is the best way to determine whether we're working with a model instance or not. Unfortunately, django.db.models can't be imported here to use isinstance

Here is a simple test case for the problem:

models.py:

from django.db import models

class MyModel(models.Model):
    name = models.CharField(max_length=100)

forms.py:

from django import newforms as forms
from myapp.models import MyModel

class MyForm(forms.Form):
    my_field = forms.ModelMultipleChoiceField(queryset=MyModel.objects.all())
from myapp import forms
from myapp.models import MyModel
MyModel.objects.create(name="one")
f = forms.MyForm({'my_field':MyModel.objects.all()})
f['my_field'].data #correct
f['my_field'].as_widget() #incorrect, without patch nothing is selected

by Peter Baumgartner, 16 years ago

Attachment: patch.diff added

better patch

comment:6 by Brian Rosner, 16 years ago

I really don't think special casing objects is the right thing to do here (Especially a hacky type checking using hasattr). While what is described doesn't work, I feel this is a very uncommon use-case that can be easily fixed by simple using list comprehension:

>>> form = forms.MyForm(initial={"my_field": [o.pk for o in MyModel.objects.all()]})
>>> print form["my_field"]
<select multiple="multiple" name="my_field" id="id_my_field">
<option value="1" selected="selected">Test 1</option>
<option value="2" selected="selected">Test 2</option>
<option value="3" selected="selected">Test 3</option>
<option value="4" selected="selected">Test 4</option>

comment:7 by Jacob, 16 years ago

Triage Stage: UnreviewedAccepted

comment:8 by Brian Rosner, 16 years ago

Jacob,

I meant to close this ticket as a wontfix, but looks like I forgot to do that, per my reasoning above. Do you see that the current patch something that is acceptable or another way to solve this? I feel a simple list comprehension above would be fine the correct way to solve this.

comment:9 by Brian Rosner, 16 years ago

Ugh, ok, I thought about this a bit more on my way into work. I am completely missing the point ;) Discard what I said earlier. I am going to look into this a bit more.

comment:10 by Jacob, 16 years ago

Yeah, I would expect if I've got a ModelMultipleChoiceField I should be able to do initial={'f': MyModel.objects.filter(....)} and have that Just Work™.

comment:11 by drano, 16 years ago

I have the same problem with initial choice selected. I have applied the patch, and the problem is solved. Then, why this patch is not included in the trunk ?

comment:12 by draxus, 15 years ago

Please, add this patch to the trunk. I was geting mad.

in reply to:  12 comment:13 by Karen Tracey, 15 years ago

Needs tests: set
Patch needs improvement: set

Replying to draxus:

Please, add this patch to the trunk. I was geting mad.

Patch is very out of date -- if you've updated it to work with current trunk please attach it, since the latest patch here makes changes to code I can't even find in the current file. Also, a test integrated into the current Django test suite that demonstrates the problem (fails on current trunk code, passes with the patch) would go a long way to helping move this one along. Unfortunately, updating the patch and adapting the test that is mentioned in the comments to fit in the test suite requires more work than I have time for at the moment. If, however, a current patch with integrated test were to appear, it would be much more likely to get review and be checked in.

by Tim Kersten, 14 years ago

by Tim Kersten, 14 years ago

comment:14 by Tim Kersten, 14 years ago

Needs tests: unset
Patch needs improvement: unset

I've attached a patch against the current trunk.
I've also attached a patch adding a test, which fails if the fix isn't applied.
Could you please review this and give feedback if it cannot be applied to trunk?

Thanks,
Tim

comment:15 by Tim Kersten, 14 years ago

Patch needs improvement: set

Um, actually scratch my patch. I didn't read the original bug report correctly :)

comment:16 by Tim Kersten, 14 years ago

Needs tests: set

comment:17 by anonymous, 13 years ago

Having ran into a similar problem I am wondering on the status of this ticket. My situation is using a CheckboxSelectMultiple for a ModelMultipleChoiceField with CheckboxSelectMultiple. Behaviour of this combination seems fine with the notable exception of initial values, much like the problem mentioned in this ticket. My fix is trivial to subclass the checkbox widget thus:

class ModelCheckboxSelectMultiple(CheckboxSelectMultiple):
    def render(self, name, value, attrs=None, choices=()):
        # convert the 'value' into a list of pk values before rendering forces the instances to unicode
        if value is None:
            value = []
        selected_keys = []
        for v in value:
            selected_keys.append(v.pk)
        return super(ModelCheckboxSelectMultiple, self).render(name, selected_keys, attrs, choices)

Looking for direction on this and I would be happy to open a new ticket if that makes sense.

Last edited 7 years ago by Tim Graham (previous) (diff)

comment:18 by davidcooper, 13 years ago

Apologies on the anonymous post about CheckboxSelectMultiple, forgot to pay attention to my login state.

comment:19 by Alex Robbins, 13 years ago

Owner: changed from nobody to Alex Robbins

comment:20 by Alex Robbins, 13 years ago

Resolution: worksforme
Status: newclosed

I tested the original problem a couple different ways and couldn't get it to show up.

I tested it with a ModelForm, which correctly highlighted values when given an existing instance. I also tested with a normal Form and passed a queryset as the initial argument. This worked too.

Marking this as worksforme. David, if you have a problem that is similar but different, it'd probably be good to make a new ticket so it doesn't get lost in the 4 year history of this ticket.

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