Opened 5 years ago

Closed 5 years ago

Last modified 5 years 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


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 5 years ago.
a tiny django project that reproduces the case

Download all attachments as: .zip

Change History (5)

Changed 5 years ago by soulne4ny

Attachment: report_admin_bug.tgz added

a tiny django project that reproduces the case

comment:1 Changed 5 years ago by Simon Meers

Resolution: invalid
Status: newclosed

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 5 years ago by Luke Plant

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:!topic/django-developers/FhNM9ajjKGA/discussion

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

comment:3 Changed 5 years ago by Simon Meers

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 5 years ago by Luke Plant

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:

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