Opened 6 years ago

Closed 3 years ago

#10711 closed New feature (fixed)

Add a hook for specifying whether a form in a formset should be deleted

Reported by: bretthoerner Owned by: jkocherhans
Component: Forms Version:
Severity: Normal Keywords: admin forms formset deletion
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

save_existing_objects currently uses form._raw_value to check the DELETION_FIELD_NAME,

 if self.can_delete:
     raw_delete_value = form._raw_value(DELETION_FIELD_NAME)
     should_delete = form.fields[DELETION_FIELD_NAME].clean(raw_delete_value)
     if should_delete:
         self.deleted_objects.append(obj)
         obj.delete()
         continue

Is there any reason it can't use form.cleaned_data.get(DELETION_FIELD_NAME, False) instead? By using _raw_value we prevent users from deciding to flag an inline for deletion during form validation.

 if self.can_delete:
     should_delete = form.cleaned_data.get(DELETION_FIELD_NAME, False)
     if should_delete:
         self.deleted_objects.append(obj)
         obj.delete()
         continue

Use case: Inline FormSet in the admin that detects certain inlines _should_ be deleted based on other information, and there is no need to bug to admin user for confirmation.

Definitely exists in 1.0.x and trunk.

Attachments (2)

delete-via-cleaned-rather-than-raw.diff (728 bytes) - added by bretthoerner 6 years ago.
10711-2.diff (1.6 KB) - added by claudep 3 years ago.
Use cleaned_data in _should_delete_form

Download all attachments as: .zip

Change History (14)

Changed 6 years ago by bretthoerner

comment:1 Changed 6 years ago by jkocherhans

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Yes, there is a reason. If a form you are marking for deletion does not validate then form.cleaned_data will not exist. Are you getting an actual exception here? What isn't working? Code examples would help.

comment:2 Changed 6 years ago by jkocherhans

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

I should also note that the very next line, should_delete = form.fields[DELETION_FIELD_NAME].clean(raw_delete_value) cleans the raw value, and that is what is used. You might be doing something I'm overlooking with the deletion field however, so if that's the case I'll see if I can figure out a way to accommodate your use case, but I'll need more specifics.

comment:3 follow-up: Changed 6 years ago by bretthoerner

No exception, but currently cleaned_data (while in the clean methods) displays the DELETION_FIELD_NAME as if the user can alter it with effect, ie:

print self.cleaned_data
[{'DELETE': False,
 'other_field': 0,
 ...},
 {'DELETE': False,
  'other_field': 1,
  ...}]

Now, in my clean method I'm detecting that certain inlines are no longer needed, and I want to flag them for deletion as if the user had,

for inline in self.cleaned_data:
    if inline.should_be_deleted():
        inline['DELETE'] = True

I just changed the value of DELETION_FIELD_NAME, but because form._raw_value(DELETION_FIELD_NAME) is used, nothing is actually deleted.

I don't understand why a form would be inside the save method, and thus the save_existing_objects method, if it weren't valid. But I'm new to 'newforms' (porting an old codebase now).

comment:4 in reply to: ↑ 3 ; follow-up: Changed 6 years ago by jkocherhans

  • milestone set to 1.2
  • Needs tests set
  • Patch needs improvement set
  • Summary changed from save_existing_objects in forms.models should use cleaned_data and not _raw_value to Add a hook for specifying whether a form in a formset should be deleted
  • Triage Stage changed from Unreviewed to Accepted

Replying to bretthoerner:

No exception, but currently cleaned_data (while in the clean methods) displays the DELETION_FIELD_NAME as if the user can alter it with effect, ie:

You're depending on an implementation detail here. cleaned_data is not something you can expect to muck around with to cause any effects.

Now, in my clean method I'm detecting that certain inlines are no longer needed, and I want to flag them for deletion as if the user had,

for inline in self.cleaned_data:
    if inline.should_be_deleted():
        inline['DELETE'] = True

I just changed the value of DELETION_FIELD_NAME, but because form._raw_value(DELETION_FIELD_NAME) is used, nothing is actually deleted.

I'd need to see more context to be able to tell what self.cleaned_data actually refers to, but you might be better off using a custom subclass of BaseInlineFormSet, and change the save_existing_objects to do what you need. This is more of a feature request than a bug (even though I know it must not seem that way.) I'll put it in my queue for 1.2, but 1.1 is at a feature freeze right now, so it won't get into trunk for a little while. I need to think about it a bit more too.

I don't understand why a form would be inside the save method, and thus the save_existing_objects method, if it weren't valid. But I'm new to 'newforms' (porting an old codebase now).

Because save_existing_objects processes all the existing objects, don't read too much into the "save" part of that method.

You can check out #9587 and [10283] to see why things were changed.

comment:5 in reply to: ↑ 4 Changed 6 years ago by bretthoerner

Replying to jkocherhans:

You're depending on an implementation detail here. cleaned_data is not something you can expect to muck around with to cause any effects.

Really? It's definitely an assumption on my part, but I think a reasonable one. Because my clean_foo methods are expected to return the value, and my clean method is expected to return a dict (for cleaned_data) I would think that means "change it how you want, return the value you want it to use." So if I change the DELETION_FIELD_NAME to True ... why isn't that respected? I'm not actually asking, but rather explaining the reasonable (I think) thoughts behind my assumption.

I'll override save_existing_objects as you said, but I think other people may run into this based on the above assumption.

If it helps to understand the problem,

from django.contrib import admin
from django.forms.formsets import DELETION_FIELD_NAME
from django.forms.models import BaseInlineFormSet

from myapp.bar import models

class FooFormSet(BaseInlineFormSet):
    def clean(self):
        # at this point self.cleaned_data is something like:
        # [{'DELETED': False,
        #   'field1': 'data1'},
        #  {'DELETED': False,
        #   'field1': 'data1'}]

        for i, foo in enumerate(self.cleaned_data):
            # in reality this is some logic to decide
            # whether we want to delete this inline for
            # some reason
            if foo:
                self.cleaned_data[i][DELETION_FIELD_NAME] = True

        # at this point self.cleaned_data is something like:
        # [{'DELETED': True,
        #   'field1': 'data1'},
        #  {'DELETED': True,
        #   'field1': 'data1'}]

        # And we return that, assuming the values we changed
        # will be used.
        return self.cleaned_data

class FooInline(admin.StackedInline):
    formset = FooFormSet
    model = models.Foo

class BarAdmin(admin.ModelAdmin):
    inlines = (FooInline,)
admin.site.register(models.Bar, BarAdmin)

Thanks so much for your quick help.

comment:6 Changed 5 years ago by jkocherhans

  • milestone changed from 1.2 to 1.3
  • Version 1.0 deleted

Another feature that probably should have made it to 1.2. Punt! Yell at me about a week after 1.2 comes out.

comment:7 Changed 5 years ago by ubernostrum

  • milestone 1.3 deleted

1.3 is feature-frozen now. Also, per his comment, someone should yell at Joseph.

Hey, Joseph! Fix this (for 1.4)!

comment:8 Changed 4 years ago by SmileyChris

  • Severity set to Normal
  • Type set to New feature

Hey Joseph, 1.3 is out! Fix this!

comment:9 Changed 3 years ago by aaugustin

  • UI/UX unset

Change UI/UX from NULL to False.

comment:10 Changed 3 years ago by aaugustin

  • Easy pickings unset

Change Easy pickings from NULL to False.

Changed 3 years ago by claudep

Use cleaned_data in _should_delete_form

comment:11 Changed 3 years ago by claudep

  • Needs tests unset
  • Patch needs improvement unset

Code has moved but now that cleaned_data is not deleted any more, we can use it in _should_delete_form.

comment:12 Changed 3 years ago by claudep

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

I'm closing it as the original ticket subject is resolved (_should_delete_form). Cleaning up is tracked in #18751.

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