Opened 18 years ago
Closed 18 years ago
#3727 closed (worksforme)
save_instance raises KeyError when form_from_{model,instance} is supplied a formfield_callback that does not return a formfield
Reported by: | Owned by: | Adrian Holovaty | |
---|---|---|---|
Component: | Forms | Version: | dev |
Severity: | Keywords: | ||
Cc: | Triage Stage: | Design decision needed | |
Has patch: | yes | Needs documentation: | yes |
Needs tests: | yes | Patch needs improvement: | no |
Easy pickings: | no | UI/UX: | no |
Description
I have a custom user class:
class User(model.Model): first_name = models.CharField(maxlength=50) last_name = models.CharField(maxlength=50) [snip] company = models.ForeignKey(Company)
Users should be able to change all fields except company. Attached is a patch implementing an exclude-keyword to django.newforms.models.form_for_{model,instance}.
Attachments (2)
Change History (8)
by , 18 years ago
Attachment: | newforms_models_for_instance_exclude.diff added |
---|
comment:1 by , 18 years ago
Needs documentation: | set |
---|---|
Needs tests: | set |
Triage Stage: | Unreviewed → Design decision needed |
If/when this is approved by the devs, I'll write tests and documentation.
(I hope this is the right way to proceed. If not, I apologize.)
comment:2 by , 18 years ago
I'm extremely wary of this -- in the old manipulator system we used follow
, and it's been the bane of many a debugger (see #999 and all its related tickets, for example, which took far too long to finally nail down). And where writing a manipulator was pretty tedious, writing a custom form in newforms to grant fine-grained control over fields and behavior is pretty easy... I'm tempted to vote for form_for_model
and form_for_instance
just being fairly "dumb" default forms, and recommending custom forms when custom behavior is desired.
comment:3 by , 18 years ago
I've attached a new patch, newforms_save_instance.diff. This changes django.newforms.models.save_instance to only update fields that have corresponding data in clean_data. With this applied, I can write:
form_for_model(User, formfield_callback=lambda f, **kwargs: f.name in ('company', 'someotherfield') and f.formfield(kwargs**))
It seems to me that adrian intended the the formfield callback to be able to keep fields out of the form by returning a value evaluating to false. From django/newforms/models.py, lines 97-99:
formfield = formfield_callback(f, initial=current_value) if formfield: field_list.append((f.name, formfield))
The problem is that if there is no field in the form corresponding to a field in the model, save_instance raises a KeyError. This patch fixes this, but would break code that depends on this behavior.
Maybe adrian could comment on how he intended this to work?
comment:4 by , 18 years ago
Summary: | form_for_model and form_for_instance should be able to exclude certain fields → save_instance raises KeyError when form_from_{model,instance} is supplied a formfield_callback that does not return a formfield |
---|
comment:5 by , 18 years ago
I have found a better way for writing this. More code, but more flexible:
class MyForm(forms.Form): field1 = MyModel._meta.get_field('field1').formfield() field2 = MyModel._meta.get_field('field2').formfield() def save(self, **kwargs): instance = MyModel(**dict(self.clean_data, **kwargs)) instance.save() return instance
Then views can do:
form = MyForm(request.POST) if form.is_valid(): instance = form.save(user=request.User) ...
comment:6 by , 18 years ago
Resolution: | → worksforme |
---|---|
Status: | new → closed |
Ubernostrum: You are absolutely right, form_for_{model,instance} should be just that, nothing more. I'm closing this, but it would be nice to update the newforms-documentation with something like the example above.
diff against r4607.