Opened 18 years ago
Closed 18 years ago
#3257 closed defect (fixed)
Change form_for_model() to return correct field for ForeignKeys and ManyToManyFields
Reported by: | 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)
Change History (24)
comment:1 by , 18 years ago
comment:2 by , 18 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:3 by , 18 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 , 18 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:
- 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?
- 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?
- 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.
follow-up: 6 comment:5 by , 18 years ago
Cc: | added |
---|
comment:6 by , 18 years ago
Replying to floguy@gmail.com:
- i thoguht that since it's meant to return model,a failure to do so (non-existent pk) is error
- 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 therequired
because its handled by the ChoiceField.clean() - always good news ;)
thanks for working on the test, I really appreciate it.
follow-up: 8 comment:7 by , 18 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 ;)
comment:8 by , 18 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 , 18 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 , 18 years ago
Needs tests: | unset |
---|
follow-up: 14 comment:11 by , 18 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!
follow-up: 13 comment:12 by , 18 years ago
Summary: | [patch] added choice fields that return models → Change form_for_model() to return correct field for ForeignKeys and ManyToManyFields |
---|---|
Triage Stage: | Design decision needed → Accepted |
There are a couple of problems with this patch:
ModelChoiceField
andModelMultipleChoiceField
should not live innewforms/fields.py
. The only part of the newforms library that knows anything about models is the filenewforms/models.py
.ModelChoiceField
andModelMultipleChoiceField
should live either indjango/db/models/fields/related.py
ordjango/newforms/models.py
.
- Instead of taking
model
andchoices
arguments,ModelChoiceField
andModelMultipleChoiceField
should take aqueryset
argument, which would be aQuerySet
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 intests/modeltests
-- not intests/regressiontests
, as this one does.
comment:13 by , 18 years ago
Replying to adrian:
There are a couple of problems with this patch:
ModelChoiceField
andModelMultipleChoiceField
should not live innewforms/fields.py
. The only part of the newforms library that knows anything about models is the filenewforms/models.py
.ModelChoiceField
andModelMultipleChoiceField
should live either indjango/db/models/fields/related.py
ordjango/newforms/models.py
.
done, I moved the fields to django/newforms/models.py
.
- Instead of taking
model
andchoices
arguments,ModelChoiceField
andModelMultipleChoiceField
should take aqueryset
argument, which would be aQuerySet
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 intests/modeltests
-- not intests/regressiontests
, as this one does.
done - I am sorry, I removed some of the simple tests (number of arguments etc),
comment:14 by , 18 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:17 by , 18 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 , 18 years ago
Cc: | added |
---|
by , 18 years ago
Attachment: | related_choice_fields_moved_trunk_resync.patch added |
---|
Resynch'ed patch against trunk rev [4451]
by , 18 years ago
Attachment: | related_choice_field_empty_label.patch added |
---|
added empty_label kwarg to the field, all tests pass now
comment:19 by , 18 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 , 18 years ago
Cc: | added |
---|
comment:21 by , 18 years ago
(Removed some of the old attachments to this ticket, to avoid confusion with the latest-and-greatest patches.)
comment:22 by , 18 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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...