Opened 8 years ago

Closed 8 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: Matias Hermanrud Fjeld <mhf@…> Owned by: adrian
Component: Forms Version: master
Severity: Keywords:
Cc: Triage Stage: Design decision needed
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

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)

newforms_models_for_instance_exclude.diff (4.2 KB) - added by Matias Hermanrud Fjeld <mhf@…> 8 years ago.
diff against r4607.
newforms_save_instance.diff (642 bytes) - added by Matias Hermanrud Fjeld <mhf@…> 8 years ago.
diff against r4741

Download all attachments as: .zip

Change History (8)

Changed 8 years ago by Matias Hermanrud Fjeld <mhf@…>

diff against r4607.

comment:1 Changed 8 years ago by Matias Hermanrud Fjeld <mhf@…>

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to 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 Changed 8 years ago by ubernostrum

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.

Changed 8 years ago by Matias Hermanrud Fjeld <mhf@…>

diff against r4741

comment:3 Changed 8 years ago by Matias Hermanrud Fjeld <mhf@…>

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 Changed 8 years ago by Matias Hermanrud Fjeld <mhf@…>

  • Summary changed from form_for_model and form_for_instance should be able to exclude certain fields to save_instance raises KeyError when form_from_{model,instance} is supplied a formfield_callback that does not return a formfield

comment:5 Changed 8 years ago by Matias Hermanrud Fjeld <mhf@…>

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 Changed 8 years ago by Matias Hermanrud Fjeld <mhf@…>

  • Resolution set to worksforme
  • Status changed from new to 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.

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