Opened 3 years ago

Closed 2 years ago

Last modified 6 months ago

#18681 closed Bug (fixed)

get_fieldsets not hooked in properly

Reported by: msopacua Owned by: loic84
Component: contrib.admin Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

django-bug-get_fieldsets.shar (3.2 KB) - added by msopacua 3 years ago.
Shell archive with test case

Download all attachments as: .zip

Change History (12)

Changed 3 years ago by msopacua

Shell archive with test case

comment:1 Changed 3 years ago by ptone

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

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?

comment:2 follow-up: Changed 3 years ago 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'...

comment:3 in reply to: ↑ 2 Changed 3 years ago by matt@…

  • Triage Stage changed from Unreviewed to Accepted

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

comment:5 Changed 2 years ago by loic84

  • Has patch set
  • Owner changed from nobody to loic84
  • Status changed from new to assigned

comment:6 Changed 2 years ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from assigned to closed

In 23e1b59cf2ad6a75637dd0273973e657e48e317e:

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

Thanks msopacua for the report.

comment:7 Changed 2 years ago by Loic Bistuer <loic.bistuer@…>

In 3a0022918989edfcb8d73f484b1b900935300abe:

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

Refs #18681.

comment:8 Changed 2 years ago by 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.

comment:9 Changed 2 years ago by 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

comment:10 Changed 2 years ago by 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

comment:11 Changed 6 months ago by Tim Graham <timograham@…>

In d4ee6cda5802adc5a38d266ccebe78fb67066179:

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

Note: See TracTickets for help on using tickets.
Back to Top