Opened 9 years ago

Closed 9 years ago

#24908 closed Bug (fixed)

Overridden field in modelform used in modeladmin which name is returned by get_readonly_fields

Reported by: James Pic Owned by: Tim Graham <timograham@…>
Component: contrib.admin Version: 1.8
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

A Model ForeignKey field will appear twice in the admin if the following conditions are met:

  • its ModelAdmin uses a custom ModelForm,
  • the ModelForm overrides that field,
  • its ModelAdmin.get_readonly_fields returns that field in edit mode.

Example:
https://github.com/jpic/double_readonly_field/raw/master/demo.png

Test project reproducing this issue:

Related:

Change History (9)

comment:1 by Tim Graham, 9 years ago

Triage Stage: UnreviewedAccepted
Version: 1.8

comment:2 by James Pic, 9 years ago

This is fixed by : https://github.com/django/django/pull/4747/

Ready for review and probably for merge, since it's really trivial.

comment:3 by David Gouldin, 9 years ago

Owner: changed from nobody to David Gouldin
Status: newassigned

Looks like this is happening because ModelFormMetaclass.__new__ doesn't expect the form class's declared fields to also be in its exclude opts. Normally, that would be a reasonable assumption. (Why would you exclude a field you've bothered to explicitly deeclared?) But in this case, it's the admin which is excluding the field, so the assumption breaks. Patch coming.

comment:4 by David Gouldin, 9 years ago

Owner: David Gouldin removed
Status: assignednew

Oops, just realized there's already a PR for this. Ignore me.

comment:5 by James Pic, 9 years ago

Probably because I forgot to assign the task, my bad, lesson learned: do not sprint so much during a sprint !

This new PR fixes the bug inside ModelAdmin: https://github.com/django/django/pull/4822

Version 2, edited 9 years ago by James Pic (previous) (next) (diff)

comment:6 by James Pic, 9 years ago

Closed PR 4822 in favor of 4824: https://github.com/django/django/pull/4824

It's the same but rebased on master. It seems like the right implementation indeed, which is "encapsulated in the class that provides the readonly feature" (and with all the awesomeness that's in type())

Last edited 9 years ago by James Pic (previous) (diff)

comment:7 by Tim Graham, 9 years ago

Has patch: set

comment:8 by Tim Graham, 9 years ago

Triage Stage: AcceptedReady for checkin

comment:9 by Tim Graham <timograham@…>, 9 years ago

Owner: set to Tim Graham <timograham@…>
Resolution: fixed
Status: newclosed

In fedef7b2:

Fixed #24908 -- Fixed duplicate readonly field rendering.

ModelAdmin added readonly_fields to exclude, but would not undeclare
them if they were overridden.

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