Opened 8 years ago

Closed 4 years ago

#5247 closed (worksforme)

ModelMultipleChoiceField doesn't select initial choices

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

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@… 8 years ago.
svn diff for patch application
patch6702.diff (2.8 KB) - added by baumer1122 8 years ago.
patch against #6702 that passes tests
patch.diff (1.6 KB) - added by baumer1122 8 years ago.
better patch
test_patch_ticket5247_r11751.diff (1.7 KB) - added by timkersten 6 years ago.
patch_ticket5247_r11751.diff (922 bytes) - added by timkersten 6 years ago.

Download all attachments as: .zip

Change History (25)

Changed 8 years ago by uhlr@…

svn diff for patch application

comment:1 Changed 8 years ago by uhlr@…

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 Changed 8 years ago by PhiR

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

comment:3 Changed 8 years ago by rcoup

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?

Changed 8 years ago by baumer1122

patch against #6702 that passes tests

comment:4 Changed 8 years ago by lukeplant

  • 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 Changed 8 years ago by baumer1122

  • 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

Changed 8 years ago by baumer1122

better patch

comment:6 Changed 8 years ago by brosner

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 Changed 8 years ago by jacob

  • Triage Stage changed from Unreviewed to Accepted

comment:8 Changed 8 years ago by brosner

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 Changed 8 years ago by brosner

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 Changed 8 years ago by jacob

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

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

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

comment:13 in reply to: ↑ 12 Changed 7 years ago by kmtracey

  • 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.

Changed 6 years ago by timkersten

Changed 6 years ago by timkersten

comment:14 Changed 6 years ago by timkersten

  • 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 Changed 6 years ago by timkersten

  • Patch needs improvement set

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

comment:16 Changed 6 years ago by timkersten

  • Needs tests set

comment:17 Changed 5 years ago by anonymous

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.

comment:18 Changed 5 years ago by davidcooper

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

comment:19 Changed 4 years ago by alexr

  • Owner changed from nobody to alexr

comment:20 Changed 4 years ago by alexr

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

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