Opened 5 years ago

Closed 4 years ago

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

Download all attachments as: .zip

Change History (13)

Changed 5 years ago by kenth

patch for BaseModelFormSet to use BaseFormSet._should_delete_form

comment:1 Changed 5 years ago by kenth

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

comment:2 Changed 5 years ago by kenth

  • Component changed from Uncategorized to Forms
  • Version changed from 1.2 to SVN

comment:3 Changed 5 years ago by kenth

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

comment:4 Changed 5 years ago by mlavin

  • Triage Stage changed from Unreviewed to Accepted

Changed 5 years ago by mlavin

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

comment:5 Changed 5 years ago by mlavin

  • Keywords sprintSep2010 added
  • Triage Stage changed from Accepted to 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 Changed 4 years ago by russellm

  • Needs tests set
  • Triage Stage changed from Ready for checkin to Accepted

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

Changed 4 years ago by kenth

Added tests to #14099

comment:7 Changed 4 years ago by kenth

  • Needs tests unset

comment:8 Changed 4 years ago by russellm

  • Triage Stage changed from Accepted to Ready for checkin

New patch has tests; back to RFC.

comment:9 Changed 4 years ago by lukeplant

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

In [15612]:

Fixed #14099 - BaseModelFormSet should use _should_delete_form

Thanks to kenth for the report and patch.

comment:10 Changed 4 years ago by lukeplant

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