Opened 14 years ago

Closed 13 years ago

Last modified 13 years ago

#14099 closed (fixed)

modelformset not using _should_delete_form

Reported by: kenth Owned by: nobody
Component: Forms Version: dev
Severity: Keywords: sprintSep2010
Cc: kenth Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

The BaseModelFormSet class doesn't use the BaseFormSet method _should_delete_form when deciding if a form should be deleted. Instead it roots around in the form raw_data for the field that BaseFormSet added. This also makes it difficult to override _should_delete_form.

This patch makes BaseModelFormSet use the BaseFormSet method.

Attachments (3)

model-formset-delete.diff​ (2.1 KB ) - added by kenth 14 years ago.
patch for BaseModelFormSet to use BaseFormSet._should_delete_form
14099-PEP8.patch​ (1.9 KB ) - added by Mark Lavin 14 years ago.
Small PEP8 cleanup of kenth's original patch (vs [13749])
patch14099-with-tests.diff​ (7.0 KB ) - added by kenth 13 years ago.
Added tests to #14099

Download all attachments as: .zip

Change History (13)

by kenth, 14 years ago

Attachment: model-formset-delete.diff​ added

patch for BaseModelFormSet to use BaseFormSet._should_delete_form

comment:1 by kenth, 14 years ago

Has patch: set

comment:2 by kenth, 14 years ago

Component: Uncategorized → Forms
Version: 1.2 → SVN

comment:3 by kenth, 14 years ago

In researching, this change was made to BaseFormSet in [12772].

comment:4 by Mark Lavin, 14 years ago

Triage Stage: Unreviewed → Accepted

by Mark Lavin, 14 years ago

Attachment: 14099-PEP8.patch​ added

Small PEP8 cleanup of kenth's original patch (vs [13749])

comment:5 by Mark Lavin, 14 years ago

Keywords: sprintSep2010 added
Triage Stage: Accepted → Ready for checkin

I did remove a couple stray spaces in the patch but it looks good. No regression tests are broken by the change and it makes the internals of BaseModelFormSet consistent with BaseFormSet as expected given the inheritance. I would say this is RFC.

comment:6 by Russell Keith-Magee, 13 years ago

Needs tests: set
Triage Stage: Ready for checkin → Accepted

This isn't RFC. It doesn't contain any tests.

by kenth, 13 years ago

Attachment: patch14099-with-tests.diff​ added

Added tests to #14099

comment:7 by kenth, 13 years ago

Needs tests: unset

comment:8 by Russell Keith-Magee, 13 years ago

Triage Stage: Accepted → Ready for checkin

New patch has tests; back to RFC.

comment:9 by Luke Plant, 13 years ago

Resolution: → fixed
Status: new → closed

In [15612]:

Fixed #14099 - BaseModelFormSet should use _should_delete_form

Thanks to kenth for the report and patch.

comment:10 by Luke Plant, 13 years ago

In [15613]:

[1.2.X] Fixed #14099 - BaseModelFormSet should use _should_delete_form

Thanks to kenth for the report and patch.

Backport of [15612] from trunk.

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