Opened 3 years ago

Closed 2 years ago

Last modified 2 years ago

#18172 closed Bug (fixed)

ModelForm fails to generate proper ModelMultipleChoiceField when using iterable model

Reported by: eric.olstad@… Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: m.r.sopacua@…, patrys@…, bruscoob@… Triage Stage: Accepted
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When using default admin forms for a model with a ManyToManyField to an iterable model (defines __iter__), the select control generated will have options that have list values instead of the primary key on the model.

Example:

class MyIterableModel(models.Model):
   def __iter__(self):
      for item in self.items:
         yield item

class MyItem(models.Model):
   owner = models.ForeignKey(MyIterableModel, related_name=items)

class MyOtherModel(models.Model):
   myiterables = model.ManyToManyField(MyIterableModel)

In the admin:

admin.site.register(models.MyOtherModel)

When viewing the MyOtherModel page in the admin, the control for the ManyToManyField will have lists of primary keys as each option value.

<select multiple="multiple" name="myiterablemodels" id="id_myiterablemodels">
   <option value="[35636, 35383]">Yada Yada</option>
</select>

This of course generates "not a valid value for primary key" when submitting.

This is because django/forms/models.py prepare_value returns a list.

1035	    def prepare_value(self, value):
1036	        if hasattr(value, '__iter__'):
1037	            return [super(ModelMultipleChoiceField, self).prepare_value(v) for v in value]
1038	        return super(ModelMultipleChoiceField, self).prepare_value(value)

I'm not sure why prepare_value returns a list in that case, so I don't have suggestions for a fix. The workaround is to not use iterable models.

Attachments (1)

bug-18172-testcase.patch (3.0 KB) - added by msopacua 3 years ago.
Testcase

Download all attachments as: .zip

Change History (11)

comment:1 Changed 3 years ago by lgtm314159

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

Hi there,

I am afraid I've tried but failed in reproducing this bug. I created files containing almost identical code snippets you provided in this ticket (with some typos fixed) and tried it on Django 1.4, but when I clicked 'add' to add an object for table MyOtherModel on the admin interface, I immediately got

TypeError at /admin/bugfix/myothermodel/add/

'RelatedManager' object is not iterable

Not to mention getting into the change list page of a certain object to see the multiple-select box. Am I misunderstanding or missing anything?

Best regards

comment:2 Changed 3 years ago by anonymous

Sorry about all the typos. The 'RelatedManager object not iterable' is yet another typo. The loop in MyIterableObject's __iter__ method should have a .all().

class MyIterableModel(models.Model):
   def __iter__(self):
      for item in self.items.all():
         yield item

That should do it. Now when trying to add a new MyOtherModel, you should see that the values for each option in the select control is a list. Trying to save should show the primary key error.

http://i.imgur.com/xEkyu.png

comment:3 Changed 3 years ago by anonymous

Here is a working typo-free version of the models.py:

from django.db import models


class MyIterableModel(models.Model):
    def __iter__(self):
        for item in self.items.all():
            yield item
    name = models.CharField(max_length=1024)


class MyItem(models.Model):
    owner = models.ForeignKey(MyIterableModel, related_name='items')


class MyOtherModel(models.Model):
    myiterables = models.ManyToManyField(MyIterableModel)

Changed 3 years ago by msopacua

Testcase

comment:4 Changed 3 years ago by msopacua

  • Cc m.r.sopacua@… added
  • Summary changed from Admin form fails to generate proper ModelMultipleChoiceField when using iterable model to ModelForm fails to generate proper ModelMultipleChoiceField when using iterable model
  • Triage Stage changed from Unreviewed to Accepted
  • Type changed from Uncategorized to Bug
  • Version changed from 1.4 to master

Added test case that confirms the problem.

comment:5 Changed 2 years ago by patrys

  • Cc patrys@… added

comment:6 Changed 2 years ago by brook

  • Cc bruscoob@… added

comment:7 Changed 2 years ago by akaariai

I think this could be fixed by adding a check for models in the code:

1035	    def prepare_value(self, value):
1036	        if hasattr(value, '__iter__'):
1037	            return [super(ModelMultipleChoiceField, self).prepare_value(v) for v in value]
1038	        return super(ModelMultipleChoiceField, self).prepare_value(value)

the line 1036 should read:

1036	        if hasattr(value, '__iter__') and not isinstance(value, Model):

Anybody willing to write a full patch for this?

comment:8 Changed 2 years ago by patrys

comment:9 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

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

In 3989ce52ef78840eefe01541628daa220191c0ad:

Fixed #18172 -- Made models with iter usable in ModelMultipleChoiceField

Thanks to Patryk Zawadzki for the patch.

comment:10 Changed 2 years ago by Anssi Kääriäinen <akaariai@…>

In 9892919b0df0e7369ab793119342182cf91d549d:

[1.5.x] Fixed #18172 -- Made models with iter usable in ModelMultipleChoiceField

Thanks to Patryk Zawadzki for the patch.

Backpatch of 3989ce52ef78840eefe01541628daa220191c0ad

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