Opened 8 years ago

Closed 7 years ago

Last modified 7 years ago

#8306 closed (fixed)

BaseModelAdmin.formfield_for_dbfield() in desperate need of refactor

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


See attached patch, which moves all of the model-field-specific overrides out into a dictionary. This has the side effect of increasing customizability :)

Attachments (2)

admin_field_overrides.diff (3.7 KB) - added by ubernostrum 8 years ago.
admin-field-overrides.patch (17.9 KB) - added by jacob 7 years ago.

Download all attachments as: .zip

Change History (10)

Changed 8 years ago by ubernostrum

comment:1 Changed 8 years ago by ubernostrum

  • Has patch set
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Design decision needed

comment:2 Changed 8 years ago by mtredinnick

  • Triage Stage changed from Design decision needed to Accepted

I'd like to hear if brosner has any particular qualms about this, but it looks like a win to me. That function is a little scary and small children might accidentally look at it.

(Brian: wontfix if you have some really good reason why this is a bad idea.)

comment:3 Changed 8 years ago by jacob

  • milestone changed from 1.0 beta to 1.0

comment:4 Changed 8 years ago by Daniel Pope <dan@…>

This fixes #8326 but breaks admin widgets for subclasses of model fields.

I suggest the dictionary keys are not model.Fields but forms.Widgets. It is the widgets that we are replacing, so as long as the admin widgets are drop-in replacements, we can just call .formfield() and check for replacement widgets for the widget class in the forms.Field returned.

comment:5 Changed 8 years ago by jacob

  • milestone changed from 1.0 to post-1.0

This is an obvious win, but I see no reason to let the code churn around at this point in the 1.0 process. Let's do this later.

comment:6 Changed 7 years ago by jacob

I've attached a more complete patch with tests/docs; it's also on my github. Since James' version, I've done a couple things:

  • Broke what was left of formfield_for_dbfield into a handful of smaller methods to make those easier to screw around with.
  • Separated the defaults and the overrides so that it's easier for subclasses to override the defaults without accidentally emptying out the dictionary (as I did while testing).
  • Made sure that the ordering is correct (**kwargs overrides formfield_overrides which overrides the defaults) to fix #9148 while we at it.

Changed 7 years ago by jacob

comment:7 Changed 7 years ago by jacob

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

(In [9760]) Cleaned up and refactored ModelAdmin.formfield_for_dbfield:

  • The new method uses an admin configuration option (formfield_overrides); this makes custom admin widgets especially easy.
  • Refactored what was left of formfield_for_dbfield into a handful of smaller methods so that it's easier to hook in and return custom fields where needed.
  • These formfield_for_* methods now pass around request so that you can easily modify fields based on request (as in #3987).

Fixes #8306, #3987, #9148.

Thanks to James Bennet for the original patch; Alex Gaynor and Brian Rosner also contributed.

comment:8 Changed 7 years ago by anonymous

  • milestone post-1.0 deleted

Milestone post-1.0 deleted

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