Code

Opened 5 years ago

Closed 21 months ago

#10828 closed Bug (fixed)

deleting all forms within a formset is not possible

Reported by: patrickk Owned by: nobody
Component: Forms Version: master
Severity: Normal Keywords:
Cc: vzima 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 kmtracey 5 years ago.
10828.diff (2.1 KB) - added by copelco 4 years ago.
10828-with-partial-cleaned_data.diff (4.8 KB) - added by copelco 4 years ago.
10828-tests.diff (998 bytes) - added by ramiro 3 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)

Changed 5 years ago by kmtracey

comment:1 Changed 5 years ago by kmtracey

  • milestone set to 1.1
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

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 follow-up: Changed 5 years ago by patrickk

in the meantime, is there a workaround/solution?

comment:3 in reply to: ↑ 2 Changed 5 years ago by kmtracey

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

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

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

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

@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 Changed 5 years ago by jacob

  • milestone changed from 1.1 to 1.2

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

comment:9 Changed 5 years ago by kmtracey

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

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 Changed 4 years ago by copelco

  • Owner changed from nobody to copelco

comment:12 Changed 4 years ago by copelco

  • 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 Changed 4 years ago by copelco

  • Has patch unset
  • Owner changed from copelco to nobody
  • Patch needs improvement unset

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

Changed 4 years ago by copelco

comment:14 Changed 4 years ago by copelco

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 Changed 4 years ago by copelco

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.

Changed 4 years ago by copelco

comment:16 Changed 4 years ago by copelco

  • 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 follow-up: Changed 4 years ago by anentropic

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?

comment:18 in reply to: ↑ 17 Changed 4 years ago by lukeplant

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 Changed 4 years ago by anentropic

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

comment:20 Changed 4 years ago by ubernostrum

  • milestone 1.2 deleted

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

comment:21 follow-up: Changed 4 years ago by 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

comment:22 in reply to: ↑ 21 Changed 4 years ago by anentropic

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 Changed 4 years ago by vzima

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 Changed 4 years ago by vzima

  • Cc vzima added

comment:25 Changed 3 years ago by SmileyChris

  • Severity set to Normal
  • Type set to Bug

comment:26 follow-up: Changed 3 years ago by claudep

  • 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?

Changed 3 years ago by ramiro

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

comment:27 in reply to: ↑ 26 Changed 3 years ago by ramiro

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 Changed 3 years ago by claudep

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 Changed 21 months ago by claudep

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

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.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.