Opened 9 years ago

Closed 8 years ago

#3257 closed defect (fixed)

Change form_for_model() to return correct field for ForeignKeys and ManyToManyFields

Reported by: Honza Král <Honza.Kral@…> Owned by: adrian
Component: Forms Version:
Severity: normal Keywords:
Cc: Honza.Kral@…, floguy@…, gandalf@…, kilian.cavalotti@…, yatiohi@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

Its a standard ChoiceField with the added value that clean() returns instance of a model with the pk equals to value - designed to work with form_for_model... it takes model class as a first argument to __init__, ForeignKey.formfield() etc. has been updated to use it...

all tests pass

Attachments (2)

related_choice_fields_moved_trunk_resync.patch (7.6 KB) - added by kilian.cavalotti@… 9 years ago.
Resynch'ed patch against trunk rev [4451]
related_choice_field_empty_label.patch (7.8 KB) - added by Honza Král <Honza.Kral@…> 9 years ago.
added empty_label kwarg to the field, all tests pass now

Download all attachments as: .zip

Change History (24)

comment:1 Changed 9 years ago by Honza Král <Honza.Kral@…>

if anybody feels that this is too tightly bound to django models (which it is) to be integral part of newforms, feel free to move it to some other location (db.models.formfields ?? ) in order to keep the decoupling...

comment:2 Changed 9 years ago by Honza Král <Honza.Kral@…>

  • Triage Stage changed from Unreviewed to Design decision needed

comment:3 Changed 9 years ago by mir@…

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set

Hi Honza--thanks for your patch! But, please don't put spaces around parentheses like in return self.model._default_manager.filter( pk__in=new_value ). There is a brief spec for the usual coding style at http://www.djangoproject.com/documentation/contributing/

comment:4 Changed 9 years ago by floguy@…

I have started working on tests for this patch, however I do have a few questions after writing most of the tests for the two new Fields:

  1. Currently, even if the value passed to the clean function is in the choices list, but is not a pk for the model, it will raise an error. Is this expected behavior? Do we want some sort of check on field construction to check that all of the choices are in fact pks for the given model?
  1. Setting required=False in either of the new fields does nothing, because when the clean function is called, if it doesn't find a result for that value, it will raise an exception. Should required=False be an option on these fields, or should the code be changed to allow for it?
  1. The section of the patch regarding django/newforms/models.py is no longer needed as far as I can tell.

Other than those few fairly minor questions, test creation is going quite well.

comment:5 follow-up: Changed 9 years ago by floguy@…

  • Cc floguy@… added

comment:6 in reply to: ↑ 5 Changed 9 years ago by Honza Kral <honza.kral@…>

Replying to floguy@gmail.com:

  1. i thoguht that since it's meant to return model,a failure to do so (non-existent pk) is error
  2. my bad - if you add if not new_value: return None (or [] for Multi...) after the call to (Multi)ChoiceField.clean(), it should do the trick... you don't have to worry explicitly about the required because its handled by the ChoiceField.clean()
  3. always good news ;)

thanks for working on the test, I really appreciate it.

comment:7 follow-up: Changed 9 years ago by floguy@…

Firstly, thanks for the quick response to my first batch of questions!

Secondly, I have uploaded an updated patch with new unit tests for the new Fields themselves, but I'm having a problem with getting everything to work correctly. When a form is gotten using form_for_instance, then calling the .save() function on that form throws errors. I tried tracing it through, and am having problems finding out what is going wrong. To recreate this, try running runtests.py model_forms.

Please let me know if I've got my environment set up properly, or if this is a legitimate error. Sorry if I'm moving slowly on this, but I'm relatively inexperienced with this process ;)

comment:8 in reply to: ↑ 7 Changed 9 years ago by Honza Kral <honza.kral@…>

Replying to floguy@gmail.com:

Firstly, thanks for the quick response to my first batch of questions!

Secondly, I have uploaded an updated patch with new unit tests for the new Fields themselves, but I'm having a problem with getting everything to work correctly. When a form is gotten using form_for_instance, then calling the .save() function on that form throws errors. I tried tracing it through, and am having problems finding out what is going wrong. To recreate this, try running runtests.py model_forms.

Please let me know if I've got my environment set up properly, or if this is a legitimate error. Sorry if I'm moving slowly on this, but I'm relatively inexperienced with this process ;)

I don't have access to a server right now, but try re-adding the section of the patch regarding django/newforms/models.py

comment:9 Changed 9 years ago by anonymous

You were right about that, I must have been crazy not to see it before. Now all tests pass including the new ones!

comment:10 Changed 9 years ago by anonymous

  • Needs tests unset

comment:11 follow-up: Changed 9 years ago by Philipp Keller <philipp.keller@…>

Why is this in triage stage "design decision needed"? I don't find any open question.
I'm looking forward for this patch to be accepted. This improves form_for_model usage of newforms a lot!

comment:12 follow-up: Changed 9 years ago by adrian

  • Summary changed from [patch] added choice fields that return models to Change form_for_model() to return correct field for ForeignKeys and ManyToManyFields
  • Triage Stage changed from Design decision needed to Accepted

There are a couple of problems with this patch:

  • ModelChoiceField and ModelMultipleChoiceField should not live in newforms/fields.py. The only part of the newforms library that knows anything about models is the file newforms/models.py. ModelChoiceField and ModelMultipleChoiceField should live either in django/db/models/fields/related.py or django/newforms/models.py.
  • Instead of taking model and choices arguments, ModelChoiceField and ModelMultipleChoiceField should take a queryset argument, which would be a QuerySet instance.
  • The unit tests in this patch should be moved to tests/modeltests/model_forms/models.py. Any test that requires a working database connection should live in tests/modeltests -- not in tests/regressiontests, as this one does.

comment:13 in reply to: ↑ 12 Changed 9 years ago by Honza Král <Honza.Kral@…>

Replying to adrian:

There are a couple of problems with this patch:

  • ModelChoiceField and ModelMultipleChoiceField should not live in newforms/fields.py. The only part of the newforms library that knows anything about models is the file newforms/models.py. ModelChoiceField and ModelMultipleChoiceField should live either in django/db/models/fields/related.py or django/newforms/models.py.

done, I moved the fields to django/newforms/models.py.

  • Instead of taking model and choices arguments, ModelChoiceField and ModelMultipleChoiceField should take a queryset argument, which would be a QuerySet instance.

done - problem here now is, if the field is not required, how do you add the ("", "------") option? I don't like the idea of it magically appearing in the choices, although it can be simply done.

I did't do anything about this, so the tests still fail for this feature...

  • The unit tests in this patch should be moved to tests/modeltests/model_forms/models.py. Any test that requires a working database connection should live in tests/modeltests -- not in tests/regressiontests, as this one does.

done - I am sorry, I removed some of the simple tests (number of arguments etc),

comment:14 in reply to: ↑ 11 Changed 9 years ago by anonymous

Replying to Philipp Keller <philipp.keller@gmail.com>:

Why is this in triage stage "design decision needed"? I don't find any open question.
I'm looking forward for this patch to be accepted. This improves form_for_model usage of newforms a lot!

Well, this has been accepted by Adrian in the meantime. My rationale was that this is adding a new class, ModelChoiceField, so I counted it as something that needs a decision whether it's wanted. Actually, it's a corner case, and I don't use the newforms, so had to make a kind of summary decision.

comment:15 Changed 9 years ago by mir@…

Pardon, the last one was me.

comment:16 Changed 9 years ago by Michael Radziej <mir@…>

  • Cc gandalf@… added

#3342 marked as duplicate

comment:17 Changed 9 years ago by Michael Radziej <mir@…>

Hmm, the patch doesn't apply to current trunk, and the merge isn't trivial for me. Honza, can you take a look?

comment:18 Changed 9 years ago by anonymous

  • Cc kilian.cavalotti@… added

Changed 9 years ago by kilian.cavalotti@…

Resynch'ed patch against trunk rev [4451]

Changed 9 years ago by Honza Král <Honza.Kral@…>

added empty_label kwarg to the field, all tests pass now

comment:19 Changed 9 years ago by Honza Král <Honza.Kral@…>

  • Patch needs improvement unset

the new patch contains everything necessary to pass the remaining tests, it applies against current trunk...

comment:20 Changed 8 years ago by anonymous

  • Cc yatiohi@… added

comment:21 Changed 8 years ago by adrian

(Removed some of the old attachments to this ticket, to avoid confusion with the latest-and-greatest patches.)

comment:22 Changed 8 years ago by adrian

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

(In [4547]) Fixed #3257 -- Added newforms ModelChoiceField and ModelMultipleChoiceField, which are now used by form_for_model() and form_for_instance(). Thanks for the patch, Honza Kral, floguy@… and kilian.cavalotti

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