#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)
Change History (34)
comment:1 Changed 7 years ago by
comment:3 Changed 7 years ago by
Owner: | changed from nobody to Mathias Rav |
---|---|
Status: | new → assigned |
comment:4 Changed 7 years ago by
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 Changed 7 years ago by
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 Changed 7 years ago by
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 Changed 7 years ago by
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.
Changed 7 years ago by
Attachment: | 26142-test.diff added |
---|
comment:8 Changed 4 years ago by
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 Changed 4 years ago by
Here is a link to the PR for this ticket, please have a look
https://github.com/django/django/pull/11580
comment:10 Changed 4 years ago by
Has patch: | set |
---|
comment:11 Changed 4 years ago by
Needs documentation: | set |
---|---|
Patch needs improvement: | set |
comment:12 Changed 21 months ago by
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:13 Changed 19 months ago by
Patch needs improvement: | set |
---|
comment:14 Changed 19 months ago by
Easy pickings: | set |
---|---|
Owner: | Parth Patil deleted |
Status: | assigned → new |
comment:15 Changed 18 months ago by
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 Changed 18 months ago by
Patch needs improvement: | unset |
---|
comment:17 Changed 18 months ago by
Needs tests: | set |
---|---|
Owner: | set to Vlad |
Patch needs improvement: | set |
Status: | new → assigned |
comment:18 Changed 18 months ago by
Needs tests: | unset |
---|---|
Patch needs improvement: | unset |
comment:19 Changed 18 months ago by
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 Changed 18 months ago by
Needs tests: | unset |
---|
comment:21 Changed 18 months ago by
Patch needs improvement: | set |
---|
I agree with David's suggestion to include the new argument in modelformset_factory()
.
comment:22 Changed 17 months ago by
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 Changed 17 months ago by
Patch needs improvement: | set |
---|
comment:24 Changed 17 months ago by
Patch needs improvement: | unset |
---|
Added edit_only
argument to the formset_factory
.
comment:25 Changed 16 months ago by
Needs documentation: | set |
---|---|
Needs tests: | set |
Patch needs improvement: | set |
comment:26 Changed 14 months ago by
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 Changed 13 months ago by
Needs documentation: | set |
---|
comment:28 Changed 13 months ago by
Needs documentation: | unset |
---|
Added topic about edit_only
mode in Formsets and fixed other remarks.
comment:29 Changed 13 months ago by
Needs documentation: | set |
---|
comment:30 Changed 13 months ago by
Needs documentation: | unset |
---|
Fixed docs, where Sphinx raised warnings
comment:31 Changed 13 months ago by
Triage Stage: | Accepted → Ready for checkin |
---|
comment:33 Changed 12 months ago by
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.
In 8e6a08e: