get_fieldsets not hooked in properly

[Melvyn Sopacua]

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.

[Melvyn Sopacua]

Shell archive with test case

[Preston Holmes]

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?

[Melvyn Sopacua]

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'...

[matt@…]

Replying to 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

[loic84]

PR ​https://github.com/django/django/pull/1016

[Tim Graham <timograham@…>]

In 23e1b59cf2ad6a75637dd0273973e657e48e317e:

Fixed #18681 -- BaseModelAdmin.get_form and InlineModelAdmin.get_formset no longer bypass get_fieldsets

Thanks msopacua for the report.

[Loic Bistuer <loic.bistuer@…>]

In 3a0022918989edfcb8d73f484b1b900935300abe:

Cleaned up UserAdmin.get_form() that worked around a bug fixed in 23e1b59.

Refs #18681.

[Tim Graham <timograham@…>]

In a0ed2f9260f995b0cdf145f2802fc8123c25db65:

Fixed #18681 -- GenericInlineModelAdmin.get_formset() no longer bypasses get_fieldsets().

Refs 23e1b59 which already fixed this issue for ModelAdmin and InlineModelAdmin.

[Tim Graham <timograham@…>]

In 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

[Tim Graham <timograham@…>]

In ebb3e50243448545c7314a1932a9067ddca5960b:

Introduced ModelAdmin.get_fields() and refactored get_fieldsets() to use it.

Refs #18681.

This also starts the deprecation of ModelAdmin.declared_fieldsets

[Tim Graham <timograham@…>]

In d4ee6cda5802adc5a38d266ccebe78fb67066179:

Removed ModelAdmin.declared_fieldsets per deprecation timeline; refs #18681.