[Django] #18681: get_fieldsets not hooked in properly
Django
-------------------------------+--------------------
Reporter: msopacua | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------+--------------------
The get_fieldsets hook on a ModelAdmin class is not hooked in properly.
When a ModelAdmin does not declare fieldsets nor fields, but only
constructs
it's fieldsets using the get_fieldsets method, get_form() does not
initialize
fieldsets nor ModelAdmin.form.fields which results in all Model fields
being
validated and cleaned before one gets to save_model.
The error is caused as follows:
- in ModelAdmin.add_view, a call is made to form.is_valid() which in turn
calls
django.forms.Form.full_clean()
- This gets to the _post_clean() method of django.forms.models.ModelForm.
- Since the ModelForm._meta has fields set to none, this results in
django.forms.models.construct_instance being called with fields set to
None
- construct_instance now loops over /all/ of the model's fields and
validates
them before assigning the value, because the continue statement is never
reached:
{{{
if fields is not None and f.name not in fields:
continue
}}}
A solution is not so simple because of the way _declared_fieldsets and
get_fieldsets are implemented and a decision has to be made on who is
going to
be authoritative for "setting the fieldsets attribute and populating the
fields meta attribute".
The real world case that triggered this error implements the following
logic:
- dynamically generate fieldset using several method calls each returning
one
'fieldset section tuple' to allow models derived from the same base
model to
add fields to a section.
- if the instance for get_fieldset is None (and thus the add_view) do not
add the fields that can be determined automatically.
- attach automatically determined values as properties to the model in
save_model().
Test case exposing the bug is attached.
--
Ticket URL: <https://code.djangoproject.com/ticket/18681>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by ptone):
* needs_better_patch: => 0
* needs_tests: => 0
* needs_docs: => 0
Comment:
other than your sample missing a line in the admin.py file:
{{{admin.site.register(Bookmark, BookmarkAdminHack)}}}
I'm not sure how to demonstrate the bug you are seeing.
On creating a new bookmark, the url field is not shown
on editing a bookmark it is shown
as the models are created - you get an integrity error for a blank URL,
but that is unrelated to the formset issue.
Can you comment further?
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:1>
Django
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Unreviewed
Comment (by msopacua):
There's a testcase. Reading it explains the lack of the
admin.site.register line. Running it, shows the problem:
{{{
Creating test database for alias 'default'...
F.
======================================================================
FAIL: test_admin_get_fields (bookmarks.tests.SimpleTest)
----------------------------------------------------------------------
Traceback (most recent call last):
File "/home/msopacua/hg/django-dev/sites/testarea/bookmarks/tests.py",
line 31, in test_admin_get_fields
self.do_admin_get_fields_test(adm)
File "/home/msopacua/hg/django-dev/sites/testarea/bookmarks/tests.py",
line 22, in do_admin_get_fields_test
self.assertEqual(form.errors, {})
AssertionError: {'added_by': [u'This field is required.']} != {}
----------------------------------------------------------------------
Ran 2 tests in 0.014s
FAILED (failures=1)
Destroying test database for alias 'default'...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:2>
Django
Reporter: msopacua | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by matt@…):
* stage: Unreviewed => Accepted
Comment:
Replying to [comment:2 msopacua]:
> There's a testcase. Reading it explains the lack of the
admin.site.register line. Running it, shows the problem:
> {{{
> Creating test database for alias 'default'...
> F.
> ======================================================================
> FAIL: test_admin_get_fields (bookmarks.tests.SimpleTest)
> ----------------------------------------------------------------------
> Traceback (most recent call last):
> File "/home/msopacua/hg/django-dev/sites/testarea/bookmarks/tests.py",
line 31, in test_admin_get_fields
> self.do_admin_get_fields_test(adm)
> File "/home/msopacua/hg/django-dev/sites/testarea/bookmarks/tests.py",
line 22, in do_admin_get_fields_test
> self.assertEqual(form.errors, {})
> AssertionError: {'added_by': [u'This field is required.']} != {}
>
> ----------------------------------------------------------------------
> Ran 2 tests in 0.014s
>
> FAILED (failures=1)
> Destroying test database for alias 'default'...
> }}}
I get the same result when running the above
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:3>
Django
Reporter: msopacua | Owner: nobody
Type: Bug | Status: new
Component: contrib.admin | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by loic84):
PR https://github.com/django/django/pull/1016
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:4>
Django
Reporter: msopacua | Owner: loic84
Type: Bug | Status: assigned
Component: contrib.admin | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by loic84):
* owner: nobody => loic84
* status: new => assigned
* has_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:5>
Django
Reporter: msopacua | Owner: loic84
Component: contrib.admin | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"23e1b59cf2ad6a75637dd0273973e657e48e317e"]:
{{{
#!CommitTicketReference repository=""
revision="23e1b59cf2ad6a75637dd0273973e657e48e317e"
Fixed #18681 -- BaseModelAdmin.get_form and InlineModelAdmin.get_formset
no longer bypass get_fieldsets
Thanks msopacua for the report.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:6>
Django
Reporter: msopacua | Owner: loic84
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Loic Bistuer <loic.bistuer@…>):
In [changeset:"3a0022918989edfcb8d73f484b1b900935300abe"]:
{{{
#!CommitTicketReference repository=""
revision="3a0022918989edfcb8d73f484b1b900935300abe"
Cleaned up UserAdmin.get_form() that worked around a bug fixed in 23e1b59.
Refs #18681.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:7>
Django
Reporter: msopacua | Owner: loic84
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"a0ed2f9260f995b0cdf145f2802fc8123c25db65"]:
{{{
#!CommitTicketReference repository=""
revision="a0ed2f9260f995b0cdf145f2802fc8123c25db65"
Fixed #18681 -- GenericInlineModelAdmin.get_formset() no longer bypasses
get_fieldsets().
Refs 23e1b59 which already fixed this issue for ModelAdmin and
InlineModelAdmin.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:8>
Django
Reporter: msopacua | Owner: loic84
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"4f8fb199948eab417961a8df66e5c41354d9fd0d"]:
{{{
#!CommitTicketReference repository=""
revision="4f8fb199948eab417961a8df66e5c41354d9fd0d"
[1.6.x] Fixed #18681 -- GenericInlineModelAdmin.get_formset() no longer
bypasses get_fieldsets().
Refs 23e1b59 which already fixed this issue for ModelAdmin and
InlineModelAdmin.
Backport of a0ed2f9260 from master
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:9>
Django
Reporter: msopacua | Owner: loic84
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"ebb3e50243448545c7314a1932a9067ddca5960b"]:
{{{
#!CommitTicketReference repository=""
revision="ebb3e50243448545c7314a1932a9067ddca5960b"
Introduced ModelAdmin.get_fields() and refactored get_fieldsets() to use
it.
Refs #18681.
This also starts the deprecation of ModelAdmin.declared_fieldsets
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:10>
Django
Reporter: msopacua | Owner: loic84
Type: Bug | Status: closed
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"d4ee6cda5802adc5a38d266ccebe78fb67066179"]:
{{{
#!CommitTicketReference repository=""
revision="d4ee6cda5802adc5a38d266ccebe78fb67066179"
Removed ModelAdmin.declared_fieldsets per deprecation timeline; refs
#18681.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/18681#comment:11>