Opened 7 years ago

Closed 3 years ago

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

Download all attachments as: .zip

Change History (20)

comment:1 Changed 7 years ago by ericholscher

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 7 years ago by brosner

  • milestone set to 1.0
  • Triage Stage changed from Design decision needed to Accepted

Changed 7 years ago by justinf

comment:3 Changed 7 years ago by justinf

  • 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 7 years ago by justinf

  • Owner changed from nobody to justinf

comment:5 Changed 7 years ago by jacob

  • milestone changed from 1.0 to post-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 7 years ago by justinf

new patch that passes all but one test

Changed 7 years ago by justinf

changes the save_as_new test so the new patch passes it

comment:6 Changed 6 years ago by punteney

  • Cc james@… added

comment:7 Changed 6 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

comment:8 Changed 6 years ago by jkocherhans

  • Owner changed from justinf to jkocherhans
  • Status changed from new to assigned

Taking this on with a few other related issues.

comment:9 follow-up: Changed 6 years ago by alanjds

  • milestone set to 1.1
  • Version changed from SVN to 1.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 6 years ago by Alex

  • milestone 1.1 deleted

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

comment:11 Changed 6 years ago by Alex

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

comment:12 in reply to: ↑ 9 Changed 5 years ago by alanjds

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 5 years ago by alanjds

  • Cc alan.justino@… added

comment:14 Changed 4 years ago by julien

  • Easy pickings unset
  • Severity set to Normal
  • Type set to Bug

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

Changed 3 years ago by claudep

Test showing working functionality

comment:15 Changed 3 years ago by claudep

  • 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 3 years ago by ramiro

  • Resolution set to fixed
  • Status changed from assigned to closed

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