Opened 8 years ago

Closed 7 years ago

Last modified 5 years ago

#10180 closed (wontfix)

Fixed bug saving inlineformset_factory formsets

Reported by: Simon Litchfield Owned by: nobody
Component: Forms Version: 1.0
Severity: Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: no
Easy pickings: UI/UX:

Description

The inlineformset_factory formset save() method was setting it's fk using the 'myfk_id' no problem, but then overwriting it with form data that used 'myfk'. For new records, the form's 'myfk' is empty, so up comes an IntegrityError since null fk's were attempting to be saved. This same code used to work fine, so I assume something has fallen inconsistent around ModelForm field naming, or fk.name/fk.get_attname().

Example --

class App(models.Model):
    company = models.CharField(max_length=100, verbose_name='company name')
...
class AppMember(models.Model):
    app = models.ForeignKey(App, related_name='members')
    name = models.CharField(max_length=50)
...
AppMemberFormSet = inlineformset_factory(App, AppMember, extra=10)
...
formset = AppMemberFormSet(data=request.POST or None)
...
formset.instance = app  # A saved app instance with an ID
formset.save()  # Error

Attachments (1)

django-inlineformset-save-new.diff (688 bytes) - added by Simon Litchfield 8 years ago.

Download all attachments as: .zip

Change History (6)

Changed 8 years ago by Simon Litchfield

comment:1 Changed 8 years ago by Simon Litchfield

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

I also needed to pass the formset an empty fk instance.

Broken --

formset = AppMemberFormSet(data=request.POST or None)

Worked --

formset = AppMemberFormSet(instance=App(), data=request.POST or None)

Looks like formsets are still a bit broken (been trying to use em since early newforms-admin branch days).

comment:2 Changed 8 years ago by Karen Tracey

Needs tests: set

For the initial problem description, a complete test, preferably one integrated into the existing test suite, would help immensely. See the commits associated with #9865 #10075 for examples. As this code is modified to handle each new corner case that wasn't previously explicitly tested for, we really need to add a test to ensure that it is not broken by any subsequent changes to this code. If an integrated test is more than you are able to do, it isn't entirely clear to me how the case you are reporting differs from ones we already test, so please spell it out. Is it that you are setting the instance for the formset after creating the formset? I'm not sure that is valid...why is the instance not passed in on the call to create the formset? Changing the instance after-the-fact strikes me as something likely to cause problems, though I can't say that it will for sure without delving more into the code than I have time for at the moment.

For the added comment, is this really the same or a different problem? It looks like it may be related to #9462 but I am not sure -- you don't say what "Broken" means. It looks to me like the fix for #9462 was intended to implicitly do what you are doing in the "Worked" version, perhaps you are reporting the same thing as the comment by tobias but since neither of you have given any specifics on the nature of any failures resulting from the current code it is very hard to be sure what either of you are reporting.

comment:3 Changed 8 years ago by Jacob

milestone: 1.1
Triage Stage: UnreviewedAccepted

comment:4 Changed 7 years ago by jkocherhans

Resolution: wontfix
Status: newclosed

If I understand your example correctly, you should be doing this instead:

formset = AppMemberFormSet(request.POST, instance=app)

If you have a reason for assigning formset.instance, or if I'm misunderstanding your problem, please reopen this ticket with more details.

comment:5 Changed 5 years ago by Jacob

milestone: 1.1

Milestone 1.1 deleted

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