Opened 10 years ago

Closed 10 years ago

#3268 closed defect (fixed)

[patch] form_for_model() should use ChoiceField for any DB field with "choices" set

Reported by: brooks.travis@… Owned by: Adrian Holovaty
Component: Forms Version: master
Severity: normal Keywords:
Cc: kilian.cavalotti@…, larlet@…, brosner@…, real.human@…, espen@…, jm.bugtracking@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

I have a model with a models.CharField(choices=GENDER_CHOICES), but when I run newforms.models.form_for_model() on the model containing that field, and do a print on the resulting form, I get a plain <input type="text">.

Attachments (4)

__init__.diff (6.4 KB) - added by ryan.moe@… 10 years ago.
models.diff (1.4 KB) - added by ryan.moe@… 10 years ago.
select_widget_for_fields_with_choices.diff (8.0 KB) - added by Tai Lee 10 years ago.
updated *Field.formfield() methods, with tests.
choices.diff (837 bytes) - added by Baptiste <baptiste.goupil@…> 10 years ago.
Patch of the #3991, so much simpler (but maybe less powerful, I don't know)

Download all attachments as: .zip

Change History (23)

comment:1 Changed 10 years ago by Adrian Holovaty

priority: highnormal
Severity: majornormal

comment:2 Changed 10 years ago by Adrian Holovaty

Summary: newforms.models.form_for_model() does not print CharField with field option choices=CHOICES_LIST as a <select> itemform_for_model() should use ChoiceField for any DB field with "choices" set

Agreed -- good catch.

Changed 10 years ago by ryan.moe@…

Attachment: __init__.diff added

Changed 10 years ago by ryan.moe@…

Attachment: models.diff added

comment:3 Changed 10 years ago by ryan.moe@…

Summary: form_for_model() should use ChoiceField for any DB field with "choices" set[patch] form_for_model() should use ChoiceField for any DB field with "choices" set

I wasn't sure which fix would be best so I included both. One fixes this in django.db.models.fields by changing all the *Field.formfield() calls, while the other only changes the form_for_model and for_for_instance calls. Both ways worked for the forms I have where this problem was occurring.

comment:4 Changed 10 years ago by Adrian Holovaty

Triage Stage: UnreviewedAccepted

Thanks for the patches. I'll have a look.

comment:5 Changed 10 years ago by anonymous

Cc: kilian.cavalotti@… added

comment:6 Changed 10 years ago by Gary Wilson <gary.wilson@…>

Has patch: set
Needs tests: set

There is also a competing patch in #3401 that alters CharField.formfield() to use ChoiceField for a CharField that has choices.

comment:7 Changed 10 years ago by anonymous

Cc: larlet@… added

comment:8 Changed 10 years ago by brosner <brosner@…>

Cc: brosner@… added

comment:9 Changed 10 years ago by mrmachine <real dot human at mrmachine dot net>

Cc: real.human@… added

Changed 10 years ago by Tai Lee

updated *Field.formfield() methods, with tests.

comment:10 Changed 10 years ago by Tai Lee

Needs tests: unset

the last two patches were made out of date by recent changes to django/db/models/fields/init.py, so i've re-implemented this functionality by creating an appropriate Select widget (which can still be overridden if a custom widget is passed in directly) for any form fields that have choices.

i left the implementation in django/db/models/fields/init.py so that it would be useful for anyone wanting to return a form field from a model field, not only when generating a form object with the form_for_* methods.

i also updated tests/modeltests/model_forms/models.py by adding an integer field with choices to the models, updating the expected output, and a comment explaining that any fields with choices defined in the model are represented by a select list (same as foreignkeys).

comment:11 Changed 10 years ago by Chris Beaven

Triage Stage: AcceptedReady for checkin

Looks good

Changed 10 years ago by Baptiste <baptiste.goupil@…>

Attachment: choices.diff added

Patch of the #3991, so much simpler (but maybe less powerful, I don't know)

comment:12 Changed 10 years ago by brosner <brosner@…>

Baptiste,

Your patch only effects admin display of the widgets. This bug you discovered is an underlying newforms problem that affects form_for_instance and form_for_model. Which is used by newforms-admin (go figure, right) ;)

comment:13 Changed 10 years ago by Baptiste <baptiste.goupil@…>

So does the patch of mrmachine since the modified functions are the ones called by form_for_instance & form_for_model, no ?
I think that my patch just factorizes what mrmachine does in its... but I can be wrong !

comment:14 Changed 10 years ago by anonymous

Cc: espen@… added

comment:15 Changed 10 years ago by anonymous

Cc: jm.bugtracking@… added

comment:16 Changed 10 years ago by Jonas von Poser

just a heads-up that both attached patches don't work for me anymore at least as of rev. 5063. The widgets in newforms-admin remain simple text input fields with either one.

comment:17 Changed 10 years ago by Tai Lee

i've not used the newforms-admin branch. does it use form_for_model? is this a problem or conflict with that branch, or a problem with this patch? does it still work for your own form_for_models forms?

comment:18 Changed 10 years ago by Jonas von Poser

Oh, I'm sorry. Both patches work with newforms-admin, they're just off by a few lines each, so basically it was me being unable to use patch correctly. I should learn to double-check before updating any bugs. :-/ Apologies.

comment:19 Changed 10 years ago by Malcolm Tredinnick

Resolution: fixed
Status: newclosed

(In [5119]) Fixed #3268 -- Changed default model formfields to use a select widget when the
field has a choices attribute. Based on a patch from mrmachine.

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