Opened 8 years ago

Closed 5 years ago

Last modified 5 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 8 years ago.
model_formsets2.diff (7.5 KB) - added by Justin 8 years ago.
new patch that passes all but one test
test_model_formsets.diff (757 bytes) - added by Justin 8 years ago.
changes the save_as_new test so the new patch passes it
8160_test.diff (1.3 KB) - added by Claude Paroz 5 years ago.
Test showing working functionality

Download all attachments as: .zip

Change History (20)

comment:1 Changed 8 years ago by Eric Holscher

Triage Stage: UnreviewedDesign decision needed

comment:2 Changed 8 years ago by Brian Rosner

milestone: 1.0
Triage Stage: Design decision neededAccepted

Changed 8 years ago by Justin

Attachment: model_formsets.diff added

comment:3 Changed 8 years ago by Justin

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 Changed 8 years ago by Justin

Owner: changed from nobody to Justin

comment:5 Changed 8 years ago by Jacob

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.

Changed 8 years ago by Justin

Attachment: model_formsets2.diff added

new patch that passes all but one test

Changed 8 years ago by Justin

Attachment: test_model_formsets.diff added

changes the save_as_new test so the new patch passes it

comment:6 Changed 8 years ago by punteney

Cc: james@… added

comment:7 Changed 8 years ago by (none)

milestone: post-1.0

Milestone post-1.0 deleted

comment:8 Changed 8 years ago by jkocherhans

Owner: changed from Justin to jkocherhans
Status: newassigned

Taking this on with a few other related issues.

comment:9 Changed 7 years ago by Alan Justino da Silva

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 Changed 7 years ago by Alex Gaynor

milestone: 1.1

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

comment:11 Changed 7 years ago by Alex Gaynor

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

comment:12 in reply to:  9 Changed 7 years ago by Alan Justino da Silva

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 Changed 7 years ago by Alan Justino da Silva

Cc: alan.justino@… added

comment:14 Changed 6 years ago by Julien Phalip

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.

Changed 5 years ago by Claude Paroz

Attachment: 8160_test.diff added

Test showing working functionality

comment:15 Changed 5 years ago by Claude Paroz

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 Changed 5 years ago by Ramiro Morales

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