Opened 8 years ago

Closed 2 years ago

Last modified 2 years ago

#26142 closed New feature (fixed)

Provide a way for model formsets to disallow new object creation

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

Description

Model formsets don't provide a way to create an "edit only" view of objects. We see users trying to use extra=0 to accomplish this, but that's not reliable as extra is merely meant for the extra number of forms to display. You can add more forms with Javascript (or just send additional post data).

Attachments (1)

26142-test.diff (1.2 KB ) - added by Tim Graham 8 years ago.

Download all attachments as: .zip

Change History (34)

comment:1 by Tim Graham <timograham@…>, 8 years ago

In 8e6a08e:

Refs #26142 -- Documented that Formset's extra=0 doesn't prevent creating objects.

comment:2 by Tim Graham <timograham@…>, 8 years ago

In 204d31c:

[1.9.x] Refs #26142 -- Documented that Formset's extra=0 doesn't prevent creating objects.

Backport of 8e6a08e937272f088902cdbec65a9f2e919783bf from master

comment:3 by Mathias Rav, 8 years ago

Owner: changed from nobody to Mathias Rav
Status: newassigned

comment:4 by David Sanders, 8 years ago

Doesn't max_num already allow this? Testing on master, if you have max_num set to 0 and validate_max to True, then the formset is effectively edit-only for existing objects. From the documentation:

max_num does not prevent existing objects from being displayed

Is there a corner case of functionality that's not covered by these two settings?

comment:5 by Tim Graham, 8 years ago

Yes, I think it's that sending data that references pks that don't appear in the queryset creates new objects for that data.

comment:6 by David Sanders, 8 years ago

Can you expand on that a little? Looking at the formset code, if the form number is less than the initial form count, it uses save_existing_object. Since validate_max prevents any forms above initial form count from being a valid formset, the only way a new object could be created is if save_existing_object could create new objects, which would be a bug in itself probably.

Playing with it in Chrome, providing a blank value for the PK of one of the objects in the formset will cause an error when trying to use to_python on the PK value since the value is blank and an integer is expected. Even if that didn't fail, it would fail to find the object by its PK in the queryset.

Providing a bogus value for the PK will also fail to look up in the queryset on the formset. This also occurs if you use a PK for an existing object that isn't in the queryset, which is good, otherwise you'd be able to modify objects outside the queryset which would be very bad.

So, I'm not sure I see the path where new objects can be created if validate_max is set and max_num is 0. Doesn't mean it's not there, but it's not immediately obvious how that could occur.

comment:7 by Tim Graham, 8 years ago

I don't think max_num=0 allows editing existing instances. Such a formset will fail with "Please submit 0 or fewer forms." won't it?

The idea I had to try to accomplish this is setting max_num=len(queryset) but I'm attaching a regression test demonstrating that an extra instance is created if the user edits the 'form-INITIAL_FORMS' form value.

by Tim Graham, 8 years ago

Attachment: 26142-test.diff added

comment:8 by Parth Patil, 5 years ago

Owner: changed from Mathias Rav to Parth Patil

Hey Mathias, I have started working on this ticket, hope it's not a problem that I'm assigning this to myself. Found no other to contact you so am directly assigning it.

I will try to make an edit_only mode for the ModelFormSet which will fix this issue.

comment:9 by Parth Patil, 5 years ago

Here is a link to the PR for this ticket, please have a look
https://github.com/django/django/pull/11580

comment:10 by Mariusz Felisiak, 5 years ago

Has patch: set

comment:11 by Mariusz Felisiak, 5 years ago

Needs documentation: set
Patch needs improvement: set

comment:12 by Jacob Walls, 3 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:13 by Carlton Gibson, 3 years ago

Patch needs improvement: set

comment:14 by David Smith, 3 years ago

Easy pickings: set
Owner: Parth Patil removed
Status: assignednew

comment:15 by Vlad, 3 years ago

I added a PR (https://github.com/django/django/pull/14725) for this ticket. It's based on previous patch that Parth Patil made. I cleaned up git artifacts and changed tests.

comment:16 by Vlad, 3 years ago

Patch needs improvement: unset

comment:17 by Mariusz Felisiak, 3 years ago

Needs tests: set
Owner: set to Vlad
Patch needs improvement: set
Status: newassigned

comment:18 by Vlad, 3 years ago

Needs tests: unset
Patch needs improvement: unset

comment:19 by Jacob Walls, 3 years ago

Needs tests: set

Patch still lacks a regression test that fails on main (upon removing the keyword introduced by the PR). Luckily, there is one we can use in the previous PR. Vlad, can you update your patch with additional tests from the previous PR?

comment:20 by Vlad, 3 years ago

Needs tests: unset

comment:21 by Jacob Walls, 3 years ago

Patch needs improvement: set

I agree with David's suggestion to include the new argument in modelformset_factory().

comment:22 by Vlad, 3 years ago

Patch needs improvement: unset

I added *edit_only* argument for the modelfomset_factory and fixed tests as discussed with Jacob. Also, I documented changes for the modelformset_factory in release notes and topic about modelformsets.

comment:23 by Jacob Walls, 3 years ago

Patch needs improvement: set

comment:24 by Vlad, 2 years ago

Patch needs improvement: unset

Added edit_only argument to the formset_factory.

comment:25 by Mariusz Felisiak, 2 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set

comment:26 by Vlad, 2 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

Added argument for inlineformset_factory, tests for inlineformset_factory and formset_factory, changed docs and release notes

comment:27 by Jacob Walls, 2 years ago

Needs documentation: set

comment:28 by Vlad, 2 years ago

Needs documentation: unset

Added topic about edit_only mode in Formsets and fixed other remarks.

comment:29 by Jacob Walls, 2 years ago

Needs documentation: set

comment:30 by Vlad, 2 years ago

Needs documentation: unset

Fixed docs, where Sphinx raised warnings

comment:31 by Mariusz Felisiak, 2 years ago

Triage Stage: AcceptedReady for checkin

comment:32 by Mariusz Felisiak <felisiak.mariusz@…>, 2 years ago

Resolution: fixed
Status: assignedclosed

In e87f57f:

Fixed #26142 -- Allowed model formsets to prevent new object creation.

Thanks Jacob Walls, David Smith, and Mariusz Felisiak for reviews.

Co-authored-by: parth <parthvin@…>

comment:33 by Matthias Kestenholz, 2 years ago

Hi all

Great new feature! Thanks. I wanted to comment on the naming of the new parameter; maybe can_add=True would be a better default than edit_only=False? There's a precedent for using can_* in Django's codebase and it would be a bit more consistent.

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