Opened 16 years ago

Closed 12 years ago

Last modified 12 years ago

#8160 closed Bug (fixed)

ModelFormset ignores form properties

Reported by: andrew.mcmurry@… Owned by: jkocherhans
Component: Forms Version: 1.1-beta
Severity: Normal Keywords: formset
Cc: justin1@…, james@…, alan.justino@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The implementation of modelformset_factory and inlineformset_factory ignore 'fields' and 'excludes' attributes of the supplied form (in fact all of Meta), and only use the values passed to the factory function.
Also the ModelFormset and InlineFormset 'save' method ignores the 'save' method of the supplied form.

class MyModelForm (django.forms.models.ModelForm):
    class Meta:
        model = models.MyModel
        fields = ('field1', 'field2')

    def save(self):
        m = super(MyModelForm, self).save()
        do_something_with(m)
        return m

MyFormSet = modelformset_factory(models.MyModel, MyModelForm)
fset = MyFormSet()

The forms that are part of fset have all fields of models.MyModel and not just 'field1' and 'field2' as expected.
Also fset.save() will not call 'do_something_with' on the models saved.

Much of the functionality of the ModelForm is ignored when used as part of a formset. This may be intended behavior, but it is not mentioned in the manual as far as I can see, and is not what one might expect.

Attachments (4)

model_formsets.diff (6.2 KB ) - added by Justin 16 years ago.
model_formsets2.diff (7.5 KB ) - added by Justin 16 years ago.
new patch that passes all but one test
test_model_formsets.diff (757 bytes ) - added by Justin 16 years ago.
changes the save_as_new test so the new patch passes it
8160_test.diff (1.3 KB ) - added by Claude Paroz 12 years ago.
Test showing working functionality

Download all attachments as: .zip

Change History (20)

comment:1 by Eric Holscher, 16 years ago

Triage Stage: UnreviewedDesign decision needed

comment:2 by Brian Rosner, 16 years ago

milestone: 1.0
Triage Stage: Design decision neededAccepted

by Justin, 16 years ago

Attachment: model_formsets.diff added

comment:3 by Justin, 16 years ago

Cc: justin1@… added
Has patch: set
Needs tests: set

I attached a rough initial patch that fixes both issues in this ticket.

modelform_factory() is changed so that the form argument's Meta is used to construct the model form. if 'fields' or 'exclude' are passed in, they override Meta. This fixes the first issue for both modelformset_factory and inlineformset_factory.

To get save() called on the forms, I rewrote BaseModelFormSet.save() to call it's forms.save(). I got rid of save_existing_objects() and save_new_objects(), but I'm not sure if those are considered part of the public API. BaseModelFormSet also now overrides _construct_form() in order to set the instance attribute of the forms. This also care makes the forms in a model formset behave more like normal model forms.

There's one nasty hack where a {{QuerySet}} is passed as the 'initial' argument for {{BaseFormSet.init()}} so that _initial_form_count and _total_form_count are set properly. Fixing that will probably require changes to {{BaseFormSet}}.

I think this patch fixes #8071 also.

comment:4 by Justin, 16 years ago

Owner: changed from nobody to Justin

comment:5 by Jacob, 16 years ago

milestone: 1.0post-1.0

I'm going to stretch the term a little bit and call this a "feature" and postpone until after 1.0. It obviously needs a fix, but stuffing this in at the last moment is a recipe for other subtle bugs.

by Justin, 16 years ago

Attachment: model_formsets2.diff added

new patch that passes all but one test

by Justin, 16 years ago

Attachment: test_model_formsets.diff added

changes the save_as_new test so the new patch passes it

comment:6 by punteney, 15 years ago

Cc: james@… added

comment:7 by (none), 15 years ago

milestone: post-1.0

Milestone post-1.0 deleted

comment:8 by jkocherhans, 15 years ago

Owner: changed from Justin to jkocherhans
Status: newassigned

Taking this on with a few other related issues.

comment:9 by Alan Justino da Silva, 15 years ago

milestone: 1.1
Version: SVN1.1-beta-1

Attached patch appears to target a very old revision. I will fix this bug in next 3 weeks (because I need it fixed), but for that I'm asking some guiedance to make not much mistakes. I will patch against 1.1-beta1.

Roadmap: (to source:django/forms/models.py#8691 )

  • Compare rev8691 with 1.1-beta1
  • Merge diff into rev8691 and study results
  • Merge diff into 1.1-beta1 and run tests
  • Force tests to pass

I found no tag for 1.1-beta1. So, if someone knows rev. number for this, please comment or msg me.

Thanks in advance.

comment:10 by Alex Gaynor, 15 years ago

milestone: 1.1

This is not a critical bug for 1.1, removing milestone.

comment:11 by Alex Gaynor, 15 years ago

I'll also note that this looks like a dupe of #10208.

in reply to:  9 comment:12 by Alan Justino da Silva, 14 years ago

It looks like fixed at 1.1.1, but I cant run unittests.

Replying to alanjds:

Attached patch appears to target a very old revision. I will fix this bug in next 3 weeks (because I need it fixed), but for that I'm asking some guiedance to make not much mistakes. I will patch against 1.1-beta1.

comment:13 by Alan Justino da Silva, 14 years ago

Cc: alan.justino@… added

comment:14 by Julien Phalip, 13 years ago

Easy pickings: unset
Severity: Normal
Type: Bug

#8071 is very likely a symptom of the same problem. Both tickets should probably be addressed at the same time.

by Claude Paroz, 12 years ago

Attachment: 8160_test.diff added

Test showing working functionality

comment:15 by Claude Paroz, 12 years ago

Needs tests: unset
UI/UX: unset

This works on current trunk. I suggest to commit the attached test and then close this ticket.

comment:16 by Ramiro Morales, 12 years ago

Resolution: fixed
Status: assignedclosed

In [16918]:

Fixed #8160 -- Made sure modelformset_factory takes in account fields' and exclude` ModelForm options.

Thanks Andrew McMurry for the report and Claude Paroz for creating these tests.

(Actually, this had been fixed in r10619 but the tests added then exercise the
code in the context of ModelAdmin. This commit adds more generic tests.)

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