Initialising disabled ModelChoiceField yields 'Select a valid choice'-error despite initialised option being valid

[Thomas Hamann]

I have a form with a ModelChoiceField that gets initialised to a specific value using get_initial in that form's View. This value is a valid choice for that Model. I don't want the user to be able to change the option on the form, but it needs to be displayed nonetheless.

When I set disabled=True on that field in forms.py, submitting the form yields the following error:

<ul class="errorlist"><li>fieldname<ul class="errorlist"><li>Select a valid choice. That choice is not one of the available choices.</li></ul></li></ul>.

Firstly, I would like to comment on the general quality of the error message, as it is not very useful: It does not return which choice it considers invalid. Including this information would make the message much more informative, and would avoid sending people on a wild goose chase to discover what the message could possibly mean.

Secondly, if a field is disabled but does contain a valid choice, validating the form should work and not trigger an error.

Edit: Adding the to_field_name option to the form field fixes the problem. However, when disabled=True is not present, this is not required.

This is probably related to the bugfix for this bug: #28387

[Tim Graham]

Can you please include code to reproduce the issue? (or ideally, a test for Django's test suite). Also, you should verify the issue against Django 2.1 or master, if possible.

[Thomas Hamann]

Because this is for work on a commercial project, I can't give the exact code, but I'll try to provide generic examples.

The form field get intialised in the FormView via:

    def get_initial(self):
        """
        Returns the initial data to use for forms on this view.
        """
        initial = super(FormView, self).get_initial()
        if self.formfield1 is not None:
            initial['formfield1'] = Model.objects.get(pk=self.formfield1)
        return initial

In this case a primary key value is passed, but the problem also occurs when I pass a value for another field in the model.

The working code in the form is:

formfield1 = forms.ModelChoiceField(queryset=Model.objects.all(), to_field_name='corresponding field name in the model', label='item to use', disabled=True)

If I leave out the disabled=True I can leave out to_field_name:

formfield1 = forms.ModelChoiceField(queryset=Model.objects.all(), label='item to use')

Then the field initialises properly, too.

This however will fail:

formfield1 = forms.ModelChoiceField(queryset=Model.objects.all(), label='item to use', disabled=True)

The problem does not occur if I initialise the field via a custom init in the form itself. In that case I can leave out to_field_name:

selected_record = Model.objects.filter(some_flag=True).first() 
selected_field_item = selected_record.field1 
self.fields['formfield1'] = forms.CharField(max_length=64,  label='Field 1', initial=selected_field_item, disabled=True)

[Carlton Gibson]

Can you reduce this to a minimal example?

As far as I can see the underlying behaviour between disabled and initial is correct:

>>> from django import forms
>>>
>>> class AForm(forms.Form):
...     c = forms.ChoiceField(choices=(("a","A"), ("b", "B")), disabled=True)
... 
>>> a_form = AForm(data={}, initial={"c":"a"})
>>> a_form.is_bound
True
>>> a_form.is_valid()
True
>>> a_form.errors
{}
>>> a_form.cleaned_data
{'c': 'a'}

Thus there must be something more particular about your case.

Happy to review if we can pin that down.

[Thomas Hamann]

It seems to only occur when I try to use get_initial in views.py. If I use a custom init in forms.py I can do whatever I want, but get_initial tends to be glitchy. I'm afraid I can't get any closer than that.

Even if the problem with get_initial can't be tracked down, fixing the unhelpful error message itself would be an improvement, too.

[Simon Charette]

Even if the problem with get_initial can't be tracked down, fixing the unhelpful error message itself would be an improvement, too.

thoha, the main problem here is that we can't reproduce your problem with the details you provided. If you can provide a minimal test project that reproduces your issue against Django 2.1 we'll certainly investigate more but at this point nothing proves Django is at fault.

[Etienne Chove]

Here is the test example I wrote in tests/forms_tests/field_tests/test_modelchoicefield.py reproducing this bug:

from django.forms import ModelChoiceField, Form
from django.test import TestCase

from ..models import ChoiceOptionModel


class ModelChoiceFieldTest(TestCase):

    def test_disabled_field_1(self):
        opt1 = ChoiceOptionModel(12345)
        opt1.save()
        class MyForm(Form):
            field = ModelChoiceField(
                queryset=ChoiceOptionModel.objects.all(), disabled=True,
                initial=opt1)
        self.assertTrue(MyForm({}).is_valid())

When disabled is True, the function to_python is called with initial value already having model type.

Here's new version of function ModelChoiceField.to_python in django/forms/models.py :

=    def to_python(self, value):
=        if value in self.empty_values:
=             return None
+         if type(value) is self.queryset.model:
+             return value
=         try:
=             key = self.to_field_name or 'pk'
=             value = self.queryset.get(**{key: value})
=         except (ValueError, TypeError, self.queryset.model.DoesNotExist):
=             raise ValidationError(self.error_messages['invalid_choice'], code='invalid_choice')
=         return value

[Carlton Gibson]

Hi Etienne.

Thanks for the test case. It's not quite working as expected I think...

I get exactly the same failure with disabled True or False. (i.e. changing it doesn't make the test pass.)

Can you double check and adjust?
(I'll leave it open for the moment.)

[Etienne Chove]

Replying to Carlton Gibson:

Hi Etienne.

Thanks for the test case. It's not quite working as expected I think...

I get exactly the same failure with disabled True or False. (i.e. changing it doesn't make the test pass.)

Can you double check and adjust?
(I'll leave it open for the moment.)

Hi Carlton,

Tahnk for your analysis.

If you write disabled=True, the test fails due to the bug described here. If you set disabled=False, the test fails because there're no data given to the form.

Here's the code failing only for disabled=True :

from django.forms import ModelChoiceField, Form
from django.test import TestCase

from ..models import ChoiceOptionModel


class ModelChoiceFieldTest(TestCase):

    def test_disabled_field_1(self):
        opt1 = ChoiceOptionModel(12345)
        opt1.save()

        class MyForm(Form):
            field = ModelChoiceField(
                queryset=ChoiceOptionModel.objects.all(), disabled=False,
                initial=opt1)

        self.assertTrue(MyForm({"field": str(opt1.pk)}).is_valid())

[Tim Graham]

This might be solved by ​a documentation clarification regarding what values should be used for initial data (primary keys rather than model instances).

While it seems ​some efforts were made to support model instances an initial values, I'm not certain if that's a good path to continue down given the increased complexity.

[Etienne Chove]

I think either we implement completely the initial paramter with a model instance (adding the 2-lines as proposed) or we do not implement at all, remove code and throw exception if user set it not as a pk.

Leaving it work in enabled but not in disabled fields is, in my mind, a bad solution because developpers will have same issue in the future.

[Etienne Chove]

I really disagree just adding notice in the doc.

In this case, when we want inial from a blanked instance field, we should add many lines each time to test if ther's a pk or if it is None:

class MyForm(Form):

    if the_object.the_filed is None:
        initial = None
    else:
        initial = the_object.the_filed.pk
    # or initial = the_object.the_filed and the_object.the_filed.pk or None

     field = ModelChoiceField(
        queryset=ChoiceOptionModel.objects.all(), disabled=False,
        initial=initial)

Allowing initial as a model instance is a really good idea, it reduces 4 lines each time.

Another patch, if we do not want to use model object everywhere, would be to evaluate type in the init method of the ModelChoiceField so we just store pk.

[Carlton Gibson]

Hi Etienne.

I am sympathetic to your point. Looking at your test-case, it is a little-strange that it accepts the model instance in the one-case but not in the other.

The addition Tim offers for the docs is right too: don't give a form data that it would never normally receive and expect it to work. (Garbage in, garbage out.)

So the thought about mapping to the pk in init seems a possibility. (We want to allow passing instances, but lets move from that as quickly as possible.)

The other thought is, yes, don't allow passing instances at all...

The difficulty here is "why is it as it is? & what breaks?" Answering that definitively is why this is still "Unreviewed" (We have looked at it. 🙂)
How about you put your patch (with test) in a PR on Github? That way the CI will tell us the "what breaks?" bit.

[Etienne Chove]

Hi,

I'll be on hollidays next week. I'll try to build a pull request when back.

Etienne

[Etienne Chove]

Pull request posted ​here