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

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

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/

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?

2. 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?

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

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.

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 ;)

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

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

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!

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.

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),

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.

Pardon, the last one was me.

#3342 marked as duplicate

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

Resynch'ed patch against trunk rev [4451]

added empty_label kwarg to the field, all tests pass now

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

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

(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@gmail.com and kilian.cavalotti