[Django] #20522: Admin formset validation cannot take submitted model instance into account when form not valid.
Django
when form not valid.
-------------------------------+--------------------------------------
Reporter: meshy | Owner: nobody
Type: Uncategorized | Status: new
Component: Uncategorized | Version: 1.5
Severity: Normal | Keywords: admin formset validation
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 1 | UI/UX: 0
-------------------------------+--------------------------------------
In the admin, if the validation on an inline formset depends upon the data
from the main form it cannot be correctly validated when the main form is
not valid. This is because the instance from the incomplete form is not
passed into the formset unless it is completely valid. Instead, on add
(https://github.com/django/django/blob/1.5.1/django/contrib/admin/options.py#L988-L994),
we get a completely blank new instance, or on change
(https://github.com/django/django/blob/1.5.1/django/contrib/admin/options.py#L1085-L1091),
we are passed the original instance.
I understand that the current way of doing things may stop invalid data
being sent to the formsets but I believe, as this is the data that has
been submitted, and the majority of it will likely be correct, that this
would be better than the nothing we currently get.
I propose that lines 994 and 1091 both change to `new_object =
form.instance`. This would mean that in a fair number of cases the errors
on the main form would not need to be fixed before the errors on the
inline form become visible.
I hope that my concerns/intention are clear, but if not, please ask and I
will produce a working example and patch for examination.
--
Ticket URL: <https://code.djangoproject.com/ticket/20522>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
when form not valid.
Reporter: meshy | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by mjtamlyn):
* needs_better_patch: => 0
* component: Uncategorized => contrib.admin
* needs_tests: => 0
* needs_docs: => 0
* type: Uncategorized => Bug
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:1>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* status: new => assigned
* owner: nobody => kamni
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:2>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by kamni):
Please disregard the screenshot, as there was a mistake in the setup.
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:3>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: 1.5
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by kamni):
I was able to confirm the first part of the bug (add_view), but could not
replicate any bug for change_view. Please see the second screenshot
attached to this bug for an example of the latter. I have also created a
repository for test code to manually duplicate the issue
(https://github.com/kamni/django_20522). Instructions for duplicating
these findings are in the README, and can be used later to confirm that
the bug has been fixed.
There is a pull request that includes the automated tests to reproduce the
bug at: https://github.com/kamni/django/pull/1
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:4>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 0 | Patch needs improvement: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* version: 1.5 => master
Comment:
Pull request for tests at https://github.com/django/django/pull/1590
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:5>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Changes (by kamni):
* has_patch: 0 => 1
Comment:
Bug fixed. Patch for review: https://github.com/django/django/pull/1590
Please provide feedback on whether/how the commits should be squashed.
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:6>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* needs_better_patch: 0 => 1
Comment:
If we can reuse existing models for the tests that would be ideal.
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:7>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by kamni):
In order to test this bug, we need two models where the validation of one
of the child's fields is contingent on the validation of one of the
parent's fields, with a special admin that handled this validation. I was
hesitant to alter an existing set of models/admin out of concern of
breaking other testing.
I already confirmed that adding the new models was valid in this
particular case with EvilDMP and one other core dev at the sprints before
submitting this patch. If you have suggestions about which models I should
change instead, please let me know.
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:8>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
* cc: EvilDMP (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:9>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by timo):
I sympathize with that concern and am ok adding the additional models. I
left a couple more comments on the PR.
In wondering why the code wasn't written like this in the first place, I
recalled a warning in
[https://docs.djangoproject.com/en/1.6/topics/forms/modelforms
/#validation-on-a-modelform Validation on a ModelForm].
"The cleaning process modifies the model instance passed to the
ModelForm constructor in various ways. For instance, any date fields on
the model are converted into actual date objects. Failed validation may
leave the underlying model instance in an inconsistent state and therefore
it’s not recommended to reuse it."
As the OP points out, hopefully "this will be better than the nothing we
currently get", but I mention it in case a documentation update might be
helpful somewhere. It seems like it could actually break existing formset
validation code if model attributes are different types when the form is
invalid which makes me a little nervous.
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:10>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by EvilDMP):
kamni: I've merged your branch with recent updates to django master, at
https://github.com/evildmp/django/tree/kamni-bug/20522; tests still pass.
It may be worth merging those updates into your branch as it will make it
easier for others to work with it.
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:11>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
timo: would the
[https://docs.djangoproject.com/en/1.6/topics/forms/modelforms
/#validation-on-a-modelform Validation on a ModelForm] link be the
appropriate place for documentation, or would you recommend somewhere
else?
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:12>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 1 |
-------------------------------------+-------------------------------------
Comment (by kamni):
Replying to [comment:12 anonymous]:
> timo: would the
[https://docs.djangoproject.com/en/1.6/topics/forms/modelforms
/#validation-on-a-modelform Validation on a ModelForm] link be the
appropriate place for documentation, or would you recommend somewhere
else?
This comment is mine.
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:13>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Changes (by timo):
* easy: 1 => 0
Comment:
This change would be specific to contrib.admin so the documentation would
go there (along with a backwards incompatible note in the release notes).
I guess I'm -0 on this change for the backwards compatibility concern and
the advice in our own docs that we shouldn't reuse model instances that
have failed validation. Feel free to persuade me otherwise (or find
another core dev who really wants this).
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:14>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by anonymous):
So one thing to consider: as you pointed out for the tests, the
change_view tests already pass with the old code (i.e., the bug is only
for the 'add' view -- see comment 4). I even considered the possibility
that my tests were running validation against the old version of the model
and that's why it was passing; but when I updated the tests so that the
formset matched the expected data from the old version but the updated
version would cause invalid fields (i.e., the tests should fail if the
code wasn't reusing a model with failed validation), the tests still
passed with the old options.py.
This seems to imply that the change_view is already disregarding the
"Don't re-use models that have failed validation". Clearly we have
inconsistent behavior between two views that needs to be remedied in one
way or another.
So should we move forward with this ticket and make the add_view
consistent with what change_view already seems to be doing, or should we
open a new ticket to revert the change_view's behavior to be more
consistent with
[https://docs.djangoproject.com/en/1.6/topics/forms/modelforms
/#validation-on-a-modelform Validation on a ModelForm]?
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:15>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by timo):
Good point. In that case, I'm comfortable with this change. Please go
ahead and squash your commits and make sure your commit message confirms
to our [https://docs.djangoproject.com/en/dev/internals/contributing
/committing-code/#committing-guidelines guidelines].
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:16>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by kamni):
The bug fix is now a single commit.
I also added a second commit with the requested documentation regarding
[https://docs.djangoproject.com/en/1.6/topics/forms/modelforms
/#validation-on-a-modelform Validation on a ModelForm]. It can be found in
contrib.admin.index under the InlineModelAdmin.form section. A review for
the docs would be much appreciated.
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:17>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by kamni):
Pull request at [https://github.com/django/django/pull/1960]
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:18>
Django
when form not valid.
Reporter: meshy | Owner: kamni
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: admin formset | Triage Stage: Accepted
validation | Needs documentation: 0
Has patch: 1 | Patch needs improvement: 1
Needs tests: 0 | UI/UX: 0
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"c74504c2dd21974571ab72805fbfc8d4d76ce151"]:
{{{
#!CommitTicketReference repository=""
revision="c74504c2dd21974571ab72805fbfc8d4d76ce151"
Fixed #20522 - Allowed use of partially validated object in
ModelAdmin.add_view formset validation.
Updated ModelAdmin to use form.instance when passing parent model to
child inlines for add_view. There is effectively no change in the
change_view since the previously passed 'obj' is the same as
form.instance.
Thanks to meshy for report, and EvilDMP and timo for review.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/20522#comment:19>