[Django] #24541: save_new_objects in ModelFromsets do not handle intitial data
Django
-------------------------------+--------------------------
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Severity: Normal | Keywords: modelformset
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------------
If you create a `Formset` and pass `initial` data for some forms, the
forms are evaluated as `has_changed` False.
All in all that is true, but it has the the side effect that the forms in
the formset don't get saved, tho they contain data.
The error is here:
https://github.com/django/django/blob/master/django/forms/models.py#L777-L778
It should not only be checked for `has_changed` but also for initial data.
--
Ticket URL: <https://code.djangoproject.com/ticket/24541>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: closed
Component: Forms | Version: master
Severity: Normal | Resolution: invalid
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 1 | UI/UX: 0
Changes (by timgraham):
* status: new => closed
* needs_better_patch: => 0
* resolution: => invalid
* needs_tests: => 0
* needs_docs: => 0
Comment:
You should use initial forms instead of extra forms if you require those
populated forms to be submitted. Changing the behavior to validate and
save unchanged extra forms would be backwards incompatible. Feel free to
reopen if I missed something.
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:1>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Easy pickings: 1 | UI/UX: 0
Changes (by codingjoe):
* status: closed => new
* resolution: invalid =>
Comment:
That does not work for `ModelFormSets` as the pop `initial` to
`initial_extra` and only except existing objects using the Queryset.
There currently is no way to create objects using a `ModelFormSet` with
initial data.
I get your point, about compatibility, but you should decide if this odd
behavior is a bug or a feature. Personally it feels like a bug. Especially
as Model and regular form sets handle initial data so differently. There
is no consistent behavior nor any documentation about this.
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:2>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
-------------------------------+--------------------------------------
Changes (by timgraham):
* easy: 1 => 0
Comment:
I had no trouble with this view:
{{{
from django.forms.models import modelformset_factory
def test_view(request):
qs = Poll.objects.none()
PollFormSet = modelformset_factory(Poll, fields='__all__')
if request.method == 'POST':
formset = PollFormSet(request.POST, queryset=qs)
if formset.is_valid():
formset.save()
else:
formset = PollFormSet(queryset=qs, initial=[
{'question': 'initial', 'pub_date': timezone.now()}])
return render(request, 'polls/manage.html', {'formset': formset})
}}}
Can you provide a snippet that demonstrates your problem?
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:3>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by codingjoe):
As I said:
It does not work if your objects are not yet created and you can't pass a
queryset.
{{{
from django.forms.models import modelformset_factory
def test_view(request):
qs = Poll.objects.none()
PollFormSet = modelformset_factory(Poll, fields='__all__')
if request.method == 'POST':
formset = PollFormSet(request.POST, inital=[{'vote': 2},
{'vote':4}])
if formset.is_valid():
obj_list = formset.save()
assert len(obj_list) == 2
else:
formset = PollFormSet(queryset=qs, initial=[
{'question': 'initial', 'pub_date': timezone.now()}])
return render(request, 'polls/manage.html', {'formset': formset})
}}}
This will fail.
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:4>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Old description:
> If you create a `Formset` and pass `initial` data for some forms, the
> forms are evaluated as `has_changed` False.
> All in all that is true, but it has the the side effect that the forms in
> the formset don't get saved, tho they contain data.
>
> The error is here:
> https://github.com/django/django/blob/master/django/forms/models.py#L777-L778
>
> It should not only be checked for `has_changed` but also for initial
> data.
New description:
If you create a `ModelFormSet` and pass `initial` data for some forms, the
forms are evaluated as `has_changed` False.
All in all that is true, but it has the the side effect that the forms in
the formset don't get saved, tho they contain data.
It should not only be checked for `has_changed` but also for initial data.
--
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:5>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by timgraham):
I'm using the models from the tutorial. `Poll` doesn't have a `vote` field
also `inital` is a typo in your example. Maybe you could explain your
actual use case instead of modifying my example as I don't understand what
the expected behavior or usefulness of that snippet is.
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:6>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by codingjoe):
Sorry, I think I fixed the example.
Let me try to be a little more specific to what's wrong:
There are two ways to pre fill a ModelFormSet, by passing a queryset or
initial data. The later is described here:
https://docs.djangoproject.com/en/dev/topics/forms/formsets/#using-
initial-data-with-a-formset
Sadly a ModelFormSet only takes `queryset` in account for
`initial_form_count`
https://github.com/django/django/blob/master/django/forms/models.py#L579
Therefore initial data is used for form construction but not accounted for
form saving, which is very much unexpected.
https://github.com/django/django/blob/master/django/forms/models.py#L606-L611
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:7>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by codingjoe):
The fix would be to change
https://github.com/django/django/blob/master/django/forms/models.py#L777
to:
{{{
if not (form.has_changed() or form.initial):
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:8>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by timgraham):
What are you trying to accomplish by passing `initial` to `PollFormSet()`
along with `request.POST`? Do you want to override `request.POST` with the
initial values or use them if no value is provided in `request.POST`?
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:9>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by codingjoe):
Well it should work in a FormView right? The form view passes the
`initial` as well as `request.POST`.
https://github.com/django/django/blob/master/django/views/generic/edit.py#L76-L90
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:10>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by timgraham):
If you want the initial data to be considered "changed", then don't pass
it to the bound FormSet as in my example. Is there a problem with that
solution?
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:11>
Django
Reporter: codingjoe | Owner: nobody
Type: Uncategorized | Status: new
Component: Forms | Version: master
Keywords: modelformset | Triage Stage: Unreviewed
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------------------------
Comment (by codingjoe):
I know that I can make it work, I'm just asking myself the questions if it
shouldn't just work out of the box.
A ModelFormSet would be the only model, that has a different behavior
regarding initial data and therefore also doesn't work with the generic
FormView.
This really doesn't sound like a feature, it's a bug.
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:12>
Django
--------------------------------------+------------------------------------
Reporter: codingjoe | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: modelformset | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* type: Uncategorized => Cleanup/optimization
* has_patch: 0 => 1
* component: Forms => Documentation
* stage: Unreviewed => Accepted
Comment:
Accepting as a documentation issue. As I said before, I don't think
changing the behavior is acceptable given backwards compatibility. For
example, your patch assumes that the initial data includes all fields of
the model. Since unchanged formsets aren't validated, saving would fail
with a database `IntegrityError` if a required field is omitted.
Let me know if anything is unclear or if you think something additional
should be added to the documentation. I've attached a proposed patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:13>
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: modelformset | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Changes (by timgraham):
* Attachment "24541.diff" added.
Django
--------------------------------------+------------------------------------
Type: Cleanup/optimization | Status: new
Component: Documentation | Version: master
Severity: Normal | Resolution:
Keywords: modelformset | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
--------------------------------------+------------------------------------
Comment (by codingjoe):
Fair enough, thanks.
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:14>
Django
-------------------------------------+-------------------------------------
Reporter: codingjoe | Owner: nobody
Type: | Status: new
Cleanup/optimization |
Component: Documentation | Version: master
Severity: Normal | Resolution:
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:15>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: master
Keywords: modelformset | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"6de3a1e2c34ae5bfcdec3ebbf3d682aa578ecae0" 6de3a1e2]:
{{{
#!CommitTicketReference repository=""
revision="6de3a1e2c34ae5bfcdec3ebbf3d682aa578ecae0"
Fixed #24541 -- Clarified ModelFormSet's handling of initial data.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:16>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: modelformset | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"7c7d29adbeb078655f9133a14d865955d1d26048" 7c7d29a]:
{{{
#!CommitTicketReference repository=""
revision="7c7d29adbeb078655f9133a14d865955d1d26048"
[1.7.x] Fixed #24541 -- Clarified ModelFormSet's handling of initial data.
Backport of 6de3a1e2c34ae5bfcdec3ebbf3d682aa578ecae0 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:17>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization |
Component: Documentation | Version: master
Severity: Normal | Resolution: fixed
Keywords: modelformset | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by Tim Graham <timograham@…>):
In [changeset:"a70dea2c06cf834e24c4b63237af669c143a8e03" a70dea2c]:
{{{
#!CommitTicketReference repository=""
revision="a70dea2c06cf834e24c4b63237af669c143a8e03"
[1.8.x] Fixed #24541 -- Clarified ModelFormSet's handling of initial data.
Backport of 6de3a1e2c34ae5bfcdec3ebbf3d682aa578ecae0 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24541#comment:18>