Opened 17 years ago

Closed 17 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 Holovaty
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: no UI/UX: no

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@… 17 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@…> 17 years ago.
added empty_label kwarg to the field, all tests pass now

Download all attachments as: .zip

Change History (24)

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

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 by Honza Král <Honza.Kral@…>, 17 years ago

Triage Stage: UnreviewedDesign decision needed

comment:3 by mir@…, 17 years ago

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 by floguy@…, 17 years ago

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 by floguy@…, 17 years ago

Cc: floguy@… added

in reply to:  5 comment:6 by Honza Kral <honza.kral@…>, 17 years ago

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 by floguy@…, 17 years ago

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

in reply to:  7 comment:8 by Honza Kral <honza.kral@…>, 17 years ago

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 by anonymous, 17 years ago

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 by anonymous, 17 years ago

Needs tests: unset

comment:11 by Philipp Keller <philipp.keller@…>, 17 years ago

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 by Adrian Holovaty, 17 years ago

Summary: [patch] added choice fields that return modelsChange form_for_model() to return correct field for ForeignKeys and ManyToManyFields
Triage Stage: Design decision neededAccepted

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.

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

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

in reply to:  11 comment:14 by anonymous, 17 years ago

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 by mir@…, 17 years ago

Pardon, the last one was me.

comment:16 by Michael Radziej <mir@…>, 17 years ago

Cc: gandalf@… added

#3342 marked as duplicate

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

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 by anonymous, 17 years ago

Cc: kilian.cavalotti@… added

by kilian.cavalotti@…, 17 years ago

Resynch'ed patch against trunk rev [4451]

by Honza Král <Honza.Kral@…>, 17 years ago

added empty_label kwarg to the field, all tests pass now

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

Patch needs improvement: unset

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

comment:20 by anonymous, 17 years ago

Cc: yatiohi@… added

comment:21 by Adrian Holovaty, 17 years ago

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

comment:22 by Adrian Holovaty, 17 years ago

Resolution: fixed
Status: newclosed

(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