Opened 15 years ago

Closed 12 years ago

#10828 closed Bug (fixed)

deleting all forms within a formset is not possible

Reported by: Patrick Kranzlmueller Owned by: nobody
Component: Forms Version: dev
Severity: Normal Keywords:
Cc: Vlastimil Zíma Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

when using a formset with can_delete and trying to delete _all_ items within the formset, I´m getting this error:
'MyForm' object has no attribute 'cleaned_data'

the problem here seems to be that formset.is_valid() is True, but cleaned_data (which is used afterwards, of course) is not available.
this could be solved with using a custom BaseFormSet and overriding the clean-method. unfortunately, the clean-method is not called in the case where all items are deleted.

I´ve done a bit of research on this issue and I found this related topic:
http://groups.google.com/group/django-developers/msg/b11309999f1b3e06

although I think it´s a pretty good idea that deleted items don´t throw validation errors, I´m not sure if the above behaviour is intended.

Attachments (4)

10828-test.diff (465 bytes ) - added by Karen Tracey 15 years ago.
10828.diff (2.1 KB ) - added by Colin Copeland 14 years ago.
10828-with-partial-cleaned_data.diff (4.8 KB ) - added by Colin Copeland 14 years ago.
10828-tests.diff (998 bytes ) - added by Ramiro Morales 12 years ago.
Regression tests by Karen and copelco converted to unit test, demonstrates the issue is still present

Download all attachments as: .zip

Change History (33)

by Karen Tracey, 15 years ago

Attachment: 10828-test.diff added

comment:1 by Karen Tracey, 15 years ago

milestone: 1.1
Triage Stage: UnreviewedAccepted

I added a patch to the test suite showing one way of triggering this error. If formset.is_valid() returns True, in the past one could expect to be able to safely iterate through form.cleaned_data for form in formset.forms...that's no longer the case. Setting milestone to 1.1 since this feels like a regression to me, so I think someone more up-to-speed in this area of the code should really take a look and make a decision on what to do about it before 1.1.

comment:2 by Patrick Kranzlmueller, 15 years ago

in the meantime, is there a workaround/solution?

in reply to:  2 comment:3 by Karen Tracey, 15 years ago

Replying to patrickk:

in the meantime, is there a workaround/solution?

Easiest: you can back off to a revision before the change that introduced the problem (see linked mailing list thread for specifics on what rev introduced it).

Harder, if you really need to run with a more recent level: you can look at what the changes were and un-do them in your more recent level, restoring the old behavior.

Even harder, but potentially more useful to others: you can try to come up with a real fix, one that preserves the intent of the changes without introducing this problem. One possibility for a solution to this particular side-effect for these changes is to fix #5524, so you might experiment with that and report on how that works out for you.

comment:4 by Patrick Craston, 15 years ago

I´ve found a pretty easy workaround (although it probably shouldn´t be mimicked). the print-statement makes it work ...

for attachment_form in attachment_formset.forms:

print attachment_form
if attachment_form.cleaned_data:

....


for attachment_form in attachment_formset.deleted_forms:

print attachment_form
if attachment_form.cleaned_data:

....

comment:5 by Patrick Craston, 15 years ago

sorry for not formatting ...

for attachment_form in attachment_formset.forms:
    print attachment_form
    if attachment_form.cleaned_data:
        ....
                    
for attachment_form in attachment_formset.deleted_forms:
    print attachment_form
    if attachment_form.cleaned_data:
        ....

comment:6 by jkocherhans, 15 years ago

It does seem like fixing #5524 (not deleting cleaned_data from invalid forms) would be the best thing to do here, but will probably have even more unintended consequences. In the meantime, is there a reason not to check form.is_valid() before trying to access form.cleaned_data? Is this problem showing up in the admin, or just in your own code?

comment:7 by anonymous, 15 years ago

@jkocherhans: the admin-interface works fine. I´m not exactly sure if I understand your question but I _do_ check form.is_valid(), resp. formset.is_valid() ...

I´ve found another workaround (for my usecase):

if form.is_valid() and amount_formset.is_valid():
    if len([fs_form.__getitem__('DELETE').data for fs_form in formset.forms if fs_form.__getitem__('DELETE').data == 'on']) != len(formset.forms):
        # save form, formsets, do stuff
    else:
        formset.non_form_errors = 'At least one item has to be saved.'

with this code example, the error won´t be thrown because it´s not allowed to delete _all_ items from the formset. of course, it´d be much better to use the formsets clean-method to perform this check (but that doesn´t work, unfortunately).

please let me know if I can help with more details or testing.

comment:8 by Jacob, 15 years ago

milestone: 1.11.2

Pushing to 1.2: there's a couple of workarounds available here.

comment:9 by Karen Tracey, 15 years ago

At a minimum, something about this needs to go in the release notes, particularly for 1.0.3. What used to be perfectly valid code (looping over cleaned_data for all the forms in a formset where is_valid() returns true) may now raise an exception.

comment:10 by TomA, 15 years ago

A quick workaround that works for me: wrapping it in a try/except block.

    try:
        for form in formset.deleted_forms:
            product = Product.objects.get(pk=form.cleaned_data['product'])
            cart.remove(product)
    except:
        cart.clear() # All forms were marked for deletion

comment:11 by Colin Copeland, 14 years ago

Owner: changed from nobody to Colin Copeland

comment:12 by Colin Copeland, 14 years ago

Has patch: set
Patch needs improvement: set

What do we want to have happen? Since we're already short circuiting formset.is_valid() to skip forms marked for deletion, we could go ahead and validate the form and set the cleaned_data attribute to allow iteration over formset.forms. I'll attach a patch, but I'm not sure it's the best solution.

comment:13 by Colin Copeland, 14 years ago

Has patch: unset
Owner: changed from Colin Copeland to nobody
Patch needs improvement: unset

Actually, this prevents iteration of formset.deleted_forms, reverting...

by Colin Copeland, 14 years ago

Attachment: 10828.diff added

comment:14 by Colin Copeland, 14 years ago

I duplicated the logic to locate the deleted forms in is_valid() to formset._get_deleted_forms and added a test to check it.

comment:15 by Colin Copeland, 14 years ago

Due to #5524 similarities, I updated my patch to not delete form.cleaned_data if invalid, but rather only populate it with the valid fields.

by Colin Copeland, 14 years ago

comment:16 by Colin Copeland, 14 years ago

Has patch: set
Needs documentation: set

Some changes may still be required, like docs, but I'll put this up for review to see if it's at least going in the right direction.

comment:17 by Paul Garner, 14 years ago

I think I've encountered this bug...

I have code in a view like this:

        img_formset = ImageFormset(instance=gallery, data=request.POST, prefix='gallery')
        if img_formset.is_valid():
            images = img_formset.save(commit=False)
            for image in images:
                image.gallery = gallery
                image.save()
            
            del_count = len(img_formset.deleted_forms)

It works ok if I have several rows in the formset and go to delete one of them, but if there's only one and I try to delete it I get:
'GalleryImageForm' object has no attribute 'cleaned_data'

Which is caused by my last line, trying to get len(img_formset.deleted_forms) - the error is raised by this line of formsets.py _get_deleted_forms():
if form.cleaned_data[DELETION_FIELD_NAME]:

According to the docs 'cleaned_data' should always be available if the form is_valid(), no? This is fairly basic form stuff not working. Someone above said #5524 needs to be fixed for this, but that's been sitting there for 2 years with no progress apparently.

Also, um, anyone know how to actually download eg copelco's diff file? Or do I have to look at the html formatted diff page linked above and make the changes manually?

in reply to:  17 comment:18 by Luke Plant, 14 years ago

Replying to anentropic:

Also, um, anyone know how to actually download eg copelco's diff file? Or do I have to look at the html formatted diff page linked above and make the changes manually?

There is a download link at the bottom of the page.

comment:19 by Paul Garner, 14 years ago

Ok thanks. Copelco's patch works great for me. Hope it can go in a release soon?

comment:20 by James Bennett, 14 years ago

milestone: 1.2

1.2 is feature-frozen, moving this feature request off the milestone.

comment:21 by Paul Garner, 14 years ago

This isn't a feature request... if you can't delete the last row of a formset that is a bug surely?

still broken in current 1.2 beta

in reply to:  21 comment:22 by Paul Garner, 14 years ago

Replying to anentropic:

This isn't a feature request... if you can't delete the last row of a formset that is a bug surely?

still broken in current 1.2 beta

Actually it looks like there might be some code in the current svn head to handle this (?) fingers crossed!

comment:23 by Vlastimil Zíma, 14 years ago

I found similar problem when I created formsets:

When deleting invalid form from formset _get_deleted_forms() method raises AttributeError at line 147 because invalid deleted forms does not have cleaned_data.

I would recommend include parts of patches from this ticket that changes line 147 in forms/formsets.py

if form.cleaned_data[DELETION_FIELD_NAME]:

so _get_deleted_forms() method work at all cases.

comment:24 by Vlastimil Zíma, 14 years ago

Cc: Vlastimil Zíma added

comment:25 by Chris Beaven, 13 years ago

Severity: Normal
Type: Bug

comment:26 by Claude Paroz, 12 years ago

Easy pickings: unset
UI/UX: unset

I cannot reproduce this bug. Can someone confirm that the issue is now fixed? Or provide a failing test?

by Ramiro Morales, 12 years ago

Attachment: 10828-tests.diff added

Regression tests by Karen and copelco converted to unit test, demonstrates the issue is still present

in reply to:  26 comment:27 by Ramiro Morales, 12 years ago

Replying to claudep:

I cannot reproduce this bug. Can someone confirm that the issue is now fixed? Or provide a failing test?

I've simply converted the tests by Karen and copelco and it is still failing. See 10828-tests.diff

comment:28 by Claude Paroz, 12 years ago

Thanks ramiro, I was probably misled by the title of the ticket which might rather be "Unable to access cleaned_data for deleted forms of a formset"...

Anyway, the patch I proposed in #5524 should resolve this issue as well.

comment:29 by Claude Paroz, 12 years ago

Resolution: fixed
Status: newclosed

This issue should be solved now that the patch for #5524 has been committed. cleaned_data is no more deleted when a form fails validation.

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