Code

Opened 8 years ago

Closed 7 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
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@… 7 years ago.
models.diff (1.4 KB) - added by ryan.moe@… 7 years ago.
select_widget_for_fields_with_choices.diff (8.0 KB) - added by mrmachine 7 years ago.
updated *Field.formfield() methods, with tests.
choices.diff (837 bytes) - added by Baptiste <baptiste.goupil@…> 7 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 8 years ago by adrian

  • priority changed from high to normal
  • Severity changed from major to normal

comment:2 Changed 8 years ago by adrian

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

Agreed -- good catch.

Changed 7 years ago by ryan.moe@…

Changed 7 years ago by ryan.moe@…

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

  • Summary changed from form_for_model() should use ChoiceField for any DB field with "choices" set to [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 7 years ago by adrian

  • Triage Stage changed from Unreviewed to Accepted

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

comment:5 Changed 7 years ago by anonymous

  • Cc kilian.cavalotti@… added

comment:6 Changed 7 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 7 years ago by anonymous

  • Cc larlet@… added

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

  • Cc brosner@… added

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

  • Cc real.human@… added

Changed 7 years ago by mrmachine

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

comment:10 Changed 7 years ago by mrmachine

  • 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 7 years ago by SmileyChris

  • Triage Stage changed from Accepted to Ready for checkin

Looks good

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

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

comment:12 Changed 7 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 7 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 7 years ago by anonymous

  • Cc espen@… added

comment:15 Changed 7 years ago by anonymous

  • Cc jm.bugtracking@… added

comment:16 Changed 7 years ago by Jonas

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 7 years ago by mrmachine

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 7 years ago by Jonas

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 7 years ago by mtredinnick

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

(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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


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

 
Note: See TracTickets for help on using tickets.