Opened 6 years ago

Closed 6 years ago

Last modified 6 years ago

#14099 closed (fixed)

modelformset not using _should_delete_form

Reported by: kenth Owned by: nobody
Component: Forms Version: master
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: UI/UX:

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 6 years ago.
patch for BaseModelFormSet to use BaseFormSet._should_delete_form
14099-PEP8.patch (1.9 KB) - added by Mark Lavin 6 years ago.
Small PEP8 cleanup of kenth's original patch (vs [13749])
patch14099-with-tests.diff (7.0 KB) - added by kenth 6 years ago.
Added tests to #14099

Download all attachments as: .zip

Change History (13)

Changed 6 years ago by kenth

Attachment: model-formset-delete.diff added

patch for BaseModelFormSet to use BaseFormSet._should_delete_form

comment:1 Changed 6 years ago by kenth

Has patch: set
Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

comment:2 Changed 6 years ago by kenth

Component: UncategorizedForms
Version: 1.2SVN

comment:3 Changed 6 years ago by kenth

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

comment:4 Changed 6 years ago by Mark Lavin

Triage Stage: UnreviewedAccepted

Changed 6 years ago by Mark Lavin

Attachment: 14099-PEP8.patch added

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

comment:5 Changed 6 years ago by Mark Lavin

Keywords: sprintSep2010 added
Triage Stage: AcceptedReady 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 Changed 6 years ago by Russell Keith-Magee

Needs tests: set
Triage Stage: Ready for checkinAccepted

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

Changed 6 years ago by kenth

Attachment: patch14099-with-tests.diff added

Added tests to #14099

comment:7 Changed 6 years ago by kenth

Needs tests: unset

comment:8 Changed 6 years ago by Russell Keith-Magee

Triage Stage: AcceptedReady for checkin

New patch has tests; back to RFC.

comment:9 Changed 6 years ago by Luke Plant

Resolution: fixed
Status: newclosed

In [15612]:

Fixed #14099 - BaseModelFormSet should use _should_delete_form

Thanks to kenth for the report and patch.

comment:10 Changed 6 years ago by Luke Plant

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