Code

Opened 2 years ago

Last modified 13 months ago

#17657 new Bug

ModelForm does not respect ModelMultipleChoiceField's to_field_name attribute

Reported by: andreyfedoseev Owned by: nobody
Component: Forms Version: 1.3
Severity: Normal Keywords:
Cc: marc.tamlyn@…, bmispelon@…, valtron2000@…, nav@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

When extracting data from existing model instance ModelForm ignores to_field_name attribute of ModelMultipleChoiceField fields and always extracts values as a list of primary keys (see https://code.djangoproject.com/browser/django/trunk/django/forms/models.py?rev=17434#L127)

One of the problems caused by this behaviour is that options in multiple select widget are not selected. Here's a small test case to illustrate this:

# models.py

from django.db import models


class Foo(models.Model):

    slug = models.CharField(max_length=40, unique=True)
    title = models.CharField(max_length=40, unique=True)


class Bar(models.Model):

    foos = models.ManyToManyField(Foo)

# tests.py

from django.test.testcases import TestCase
from django import forms
from models import Foo, Bar


class TestModelMultipleChoiceWithFieldName(TestCase):

    def setUp(self):
        self.instance = Bar.objects.create()

        self.instance.foos.add(Foo.objects.create(title="Spam", slug="spam"))
        self.instance.foos.add(Foo.objects.create(title="Ham", slug="ham"))
        self.instance.foos.add(Foo.objects.create(title="Eggs", slug="eggs"))


    def test_without_field_name(self):

        class Form(forms.ModelForm):

            foos = forms.ModelMultipleChoiceField(Foo.objects.all())

            class Meta:
                model = Bar

        form = Form(instance=self.instance)

        self.assertEquals(
            str(form["foos"]),
            '<select multiple="multiple" name="foos" id="id_foos">\n'
            '<option value="1" selected="selected">Spam</option>\n'
            '<option value="2" selected="selected">Ham</option>\n'
            '<option value="3" selected="selected">Eggs</option>\n'
            '</select>'
        )

    def test_with_field_name(self):

        class Form(forms.ModelForm):

            foos = forms.ModelMultipleChoiceField(Foo.objects.all(), to_field_name="slug")

            class Meta:
                model = Bar

        form = Form(instance=self.instance)

        # Fails! Options aren't selected.
        self.assertEquals(
            str(form["foos"]),
            '<select multiple="multiple" name="foos" id="id_foos">\n'
            '<option value="spam" selected="selected">Spam</option>\n'
            '<option value="ham" selected="selected">Ham</option>\n'
            '<option value="eggs" selected="selected">Eggs</option>\n'
            '</select>'
        )

I am preparing a patch now.

Attachments (1)

issue17657.diff (3.3 KB) - added by andreyfedoseev 2 years ago.

Download all attachments as: .zip

Change History (9)

Changed 2 years ago by andreyfedoseev

comment:1 Changed 2 years ago by andreyfedoseev

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

comment:2 Changed 2 years ago by aaugustin

  • Triage Stage changed from Unreviewed to Design decision needed

I'm not sure what to do with this ticket.

to_field_name is an internal API. I'd like to know if it's possible to trigger a bug by using only public APIs. But the patch appears to be an improvement.

This needs more thought.

Version 0, edited 2 years ago by aaugustin (next)

comment:3 Changed 18 months ago by mjtamlyn

  • Cc marc.tamlyn@… added

I came across this today - I would like the patch to be applied. I know I'm messing with an internal API but it'd be good if it 'just worked'.

comment:4 Changed 17 months ago by poswald

I think there is a pretty strong case to be made that this kind of functionality should be documented and added to Django's public API. Even without going that far, it certainly seems more clean to not make the assumption that the pk is the way field names are looked up, if an attribute is being used elsewhere in the code.

comment:5 Changed 16 months ago by jacob

  • Triage Stage changed from Design decision needed to Accepted

The patch is an improvement, so internal API or not I think it should go in.

comment:6 Changed 15 months ago by bmispelon

  • Cc bmispelon@… added
  • Patch needs improvement set

This was reported again recently with a slightly different perspective (the goal was to hide the pk from the end-user): #20202

In that ticket, I came to the conclusion that it was a documentation issue and proposed the following patch: https://github.com/bmispelon/django/compare/ticket-20202

The existing patch does not apply cleanly anymore because of commit e44ab5bb4fd3aa826ca4243a8ea9fd7125800da2 which was done as an optimization.

The optimization is unfortunately not compatible with the OP's proposed fix.

From my understanding, model_to_dict needs access to the form's field's to_field_name which is a problem because the current implementation is only passed a model instance and two lists of field names (one to include and one to exclude).

It's possible to hack something that makes the OP's test pass but it's quite ugly: https://gist.github.com/bmispelon/5329758

comment:7 Changed 15 months ago by valtron2000@…

  • Cc valtron2000@… added

comment:8 Changed 13 months ago by nav@…

  • Cc nav@… added

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as new
The owner will be changed from nobody to anonymous. Next status will be 'assigned'
as The resolution will be set. Next status will be 'closed'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.