[Django] #28316: ModelChoiceField with to_field_name fails to render a widget properly
Django
------------------------------------------+------------------------
Reporter: László Károlyi | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------+------------------------
Hey guys,
I have two models something along the following:
{{{#!python
class M1(Model):
code = CharField()
name = CharField()
class M2(Model):
m1 = ForeignKey(to=M1)
}}}
I have a custom modelform, something along:
{{{#!python
class MyForm(ModelForm):
m1 = ModelChoiceField(to_field_name='code')
class Meta(object):
model = M2
exclude = ()
}}}
When I try to render this form with an instance of `M2`, because of
`to_field_name` not being `pk` of `m1`, the form will render without the
option pointing to `M1` selected.
I managed to track the problem back to `BoundField.value()`, which calls
`self.field.prepare_value(data)` with the `PK` passed to the instance of
the `ModelChoiceField`. There, `ModelChoiceField.prepare_value()` should
be able to know how to resolve the passed `PK` to the parameter in
`to_field_name`, but it doesn't do it.
Because of that, I'm forced not to pass the `to_field_name` parameter, and
let the select widget render itself with `PK`s instead of the wished
`code` fields.
Please look into this, should be fairly easy to reproduce. As I could see
from googling, [https://stackoverflow.com/questions/9827057/django-
modelchoicefield-use-something-other-than-id#comment21720211_11522820 this
has formerly already been a problem], and now it's present again.
--
Ticket URL: <https://code.djangoproject.com/ticket/28316>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: László Károlyi | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by László Károlyi):
Update: it seems that checking out the django project source and testing
within can't reproduce this bug.
I'm trying to hunt down the exact source of it. Still, the problem is that
BoundField gets an integer value instead of the looked up model in the
foreign key.
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:1>
Django
Reporter: László Károlyi | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by László Károlyi):
So I guess I just opened a huge can of worms. In a nutshell:
ModelForm ForeignKey data is initialized on ModelForms at `__init__` time,
and then, only the PKs are put into the `self.initial` dict. Hence, the
time the `BoundField` wants to pass it over to
`ModelChoiceField.prepare_value()`, it only has the PK.
There are two 'workarounds', neither of which is good looking, but fixes
the problem:
1: Patch `ModelChoiceField.prepare_value()`:
{{{#!python
def prepare_value(self, value):
if hasattr(value, '_meta'):
if self.to_field_name:
return value.serializable_value(self.to_field_name)
else:
return value.pk
if self.to_field_name:
# Case for lookup when only a PK is passed
selected_value = [x for x in self.queryset if x.pk == value]
if selected_value:
return getattr(selected_value[0], self.to_field_name)
return super().prepare_value(value)
}}}
2: Patch `BoundField.value()`:
{{{#!python
def value(self):
"""
Return the value for this BoundField, using the initial value if
the form is not bound or the data otherwise.
"""
from django.forms.models import ModelChoiceField
data = self.initial
if self.form.is_bound:
data = self.field.bound_data(self.data, data)
elif hasattr(self.form, 'instance') and self.form.instance.pk and
type(self.field) is ModelChoiceField:
# First display: form non-bound but has an existing
# instance, override any initials
data = getattr(self.form.instance, self.name)
return self.field.prepare_value(data)
}}}
I got a test, the whole test suite runs with both patches, posted in a
pull request (the first version is commented out but it's there):
https://github.com/django/django/pull/8650
This part of Django with the forms seems to be seriously not well thought
out, to put it mildly.
Any suggestions are welcome, I spent ~8 hrs pondering on the solution, and
traversing the Django source.
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:2>
Django
--------------------------------+------------------------------------
Reporter: László Károlyi | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* stage: Unreviewed => Accepted
Comment:
You might look at #17657 which was a fix for `to_field_name` and
`ModelMultipleChoiceField`. In that case, I think
`ModelMultipleChoiceField._check_values()` is doing the conversion from
initial pk values to `to_field_name` values. A proper fix most likely lies
in `ModelChoiceField` rather than `BoundField`; the latter shouldn't have
knowledge of specific fields.
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:3>
Django
--------------------------------+------------------------------------
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by László Károlyi):
Replying to [comment:3 Tim Graham]:
> You might look at #17657 which was a fix for `to_field_name` and
`ModelMultipleChoiceField`. In that case, I think
`ModelMultipleChoiceField._check_values()` is doing the conversion from
initial pk values to `to_field_name` values. A proper fix most likely lies
in `ModelChoiceField` rather than `BoundField`; the latter shouldn't have
knowledge of specific fields.
Just took a look at it, it's doing basically the same what I implemented
in `ModelChoiceField.value()`.
For me it really doesn't matter where the underlying logic is, as long as
it works. Do you want me to uncomment the logic in `ModelChoiceField` and
update the pull request?
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:4>
Django
--------------------------------+------------------------------------
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
Yes, I'll have to examine that solution in more detail but I would say
that's more on the right track.
The test can probably be more minimal as in
81963b37a92ef583b9f725f1a78dd2ca97c1ab95 -- check `BoundField.value()`
rather than rendered HTML.
Please rebase your branch so that the unrelated commits don't appear and
revert the unrelated blank line changes that I presume your IDE is making.
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:5>
Django
--------------------------------+------------------------------------
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by László Károlyi):
Replying to [comment:5 Tim Graham]:
> Yes, I'll have to examine that solution in more detail but I would say
that's more on the right track.
>
> The test can probably be more minimal as in
81963b37a92ef583b9f725f1a78dd2ca97c1ab95 -- check `BoundField.value()`
rather than rendered HTML.
>
> Please rebase your branch so that the unrelated commits don't appear and
revert the unrelated blank line changes that I presume your IDE is making.
OK, I'll get that done in the upcoming days. Are the new lines such a big
problem? It's PEP8 enforced by Anaconda/ST3.
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:6>
Django
--------------------------------+------------------------------------
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham):
We don't want unrelated whitespace changes in a patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:7>
Django
model field's to_field
--------------------------------+------------------------------------
Reporter: László Károlyi | Owner: nobody
Type: Bug | Status: new
Component: Forms | Version: master
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* needs_better_patch: 0 => 1
* has_patch: 0 => 1
Comment:
I updated [https://github.com/django/django/pull/8650 the patch], however,
I feel like the solution isn't ideal and there's still a case that doesn't
work (see the failing test on the patch) -- if the form field's
`to_field_name` is different from the model form's `to_field`.
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:8>
Django
model field's to_field
Reporter: László Károlyi | Owner: László Károlyi
Type: Bug | Status: assigned
Component: Forms | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by László Károlyi):
* owner: nobody => László Károlyi
* status: new => assigned
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:9>
Django
model field's to_field
--------------------------------+------------------------------------------
Reporter: László Károlyi | Owner: László Károlyi
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by László Károlyi):
So the generic idea of the fix is to also fix the
> ModelForm ForeignKey data is initialized on ModelForms at __init__ time,
and then, only the PKs are put into the self.initial dict.
part. After that, the value that will be passed to
`ModelChoiceField.prepare_value()`. My opinion is still that this part is
too complicated and should be rewritten at a later stage.
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:10>
Django
model field's to_field
--------------------------------+------------------------------------------
Reporter: László Károlyi | Owner: László Károlyi
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by George Khaburzaniya):
Original PR was closed, new PR here
https://github.com/django/django/pull/8745
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:11>
Django
model field's to_field
--------------------------------+------------------------------------------
Reporter: László Károlyi | Owner: László Károlyi
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Comment (by Marc Gibbons):
Taking a stab at a fix for this. Looked at few different options –
including `ModelChoiceField` and `model_to_dict`, all of which (including
this one) feeling a bit hacky.
It really comes down to a problem of generating initial values to the
`ModelForm` when an instance is provided and is used as the source of
initial values. Getting the full context of the ForeignKey, its
`to_field`, and whether or not the "initial" value came from the
`instance` or from the `initial` kwargs of the constructor, isn't feasible
from the `ModelChoiceField` class in isolation.
PR: https://github.com/django/django/pull/15025
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:12>
Django
model field's to_field
--------------------------------+----------------------------------------
Type: Bug | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* owner: László Károlyi => Marc Gibbons
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:13>
Django
model field's to_field
--------------------------------+----------------------------------------
Reporter: László Károlyi | Owner: Marc Gibbons
Type: Bug | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Carlton Gibson):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:14>
Django
model field's to_field
--------------------------------+----------------------------------------
Reporter: László Károlyi | Owner: Marc Gibbons
Type: Bug | Status: assigned
Component: Forms | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by Dmytro Litvinov):
* cc: Dmytro Litvinov (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/28316#comment:15>