Code

Opened 12 months ago

Closed 11 months ago

Last modified 11 months ago

#20352 closed Bug (invalid)

ModelAdmin form validation does not check inherited get_readonly_fields()

Reported by: soulne4ny Owned by: nobody
Component: contrib.admin Version: 1.5
Severity: Normal Keywords: admin readonly fields validation
Cc: Triage Stage: Unreviewed
Has patch: no Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

While trying to create a custom _base_ class InlineModelAdmin with a set of predefined "evaluated"-fields I found that a _child_ inline model admin gets an error

ImproperlyConfigured: 'AnInlineModelAdmin.fields' refers to field 'base_field' that is missing from the form.

For more flexibility I didn't use the readonly_fields attribute, but the get_readonly_fields() method.

However, it works when the readonly_fields attribute is set. After looking in the admin validation code, I found that get_readonly_fields() works well with redefined get_form(), even if it just returns super().

It would be nice to have consistent behavoiur %)

Attachments (1)

report_admin_bug.tgz (2.9 KB) - added by soulne4ny 12 months ago.
a tiny django project that reproduces the case

Download all attachments as: .zip

Change History (5)

Changed 12 months ago by soulne4ny

a tiny django project that reproduces the case

comment:1 Changed 11 months ago by DrMeers

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Resolution set to invalid
  • Status changed from new to closed

The minimal sample project is appreciated, thank you. There were a few errors within it, but having remedied them I think I've reproduced your issue.

According to the / docs "The fields option, unlike list_display, may only contain names of fields on the model or the form specified by form. It may contain callables only if they are listed in readonly_fields."

So technically your use of 'a_field' in fields here is invalid if it is only returned by get_readonly_fields rather than defined in readonly_fields. Unlike your example, get_readonly_fields takes a request and optional model instance as arguments, which cannot be done by the validation code as it is not request-specific.

I believe your example works when you override the get_form method via the mixin because the super call to get_form bypasses the fields value specified at the MixIn level.

comment:2 Changed 11 months ago by lukeplant

I haven't looked into this, but I suspect this is a genuine bug with model validation - if validation is rejecting things that otherwise work. Discussion: https://groups.google.com/forum/#!topic/django-developers/FhNM9ajjKGA/discussion

There were similar bugs that were fixed in [1556b1c3b7a18cadf4d66d2ab6301d7fe8646ba5] - so this may have been fixed now.

comment:3 Changed 11 months ago by DrMeers

This is admin validation rather than model validation, and seems to boil down to whether or not get_readonly_fields should only be used for runtime request-specific overrides or should be considered during validation using mock requests or similar (or if we should relax the admin validation).

comment:4 Changed 11 months ago by lukeplant

Sorry - I was actually talking about admin validation, not model validation, and that discussion was about admin validation. We agreed that checks that attempt to check something statically that can only be checked really at run time are not helpful - especially if something can fail validation statically if it will actually work in practice. The validation should be replaced, where possible, with more helpful runtime error messages - like this: https://github.com/django/django/commit/1556b1c3b7a18cadf4d66d2ab6301d7fe8646ba5#L0L491

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.