Opened 8 years ago

Closed 7 years ago

#6241 closed (fixed)

Use ModelForms in ModelAdmin. Refactor FormSets to work like ModelForms.

Reported by: brosner Owned by: brosner
Component: Forms Version: newforms-admin
Severity: Keywords: nfa-blocker
Cc: simon@…, jkocherhans@… Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: yes Patch needs improvement: yes
Easy pickings: UI/UX:

Description

This ticket really is two things, but they go hand in hand.

  1. We need to get ModelAdmin working with the ModelForm class. This will require some helper function to dynamically create a ModelForm class object at run-time.
  2. Refactor FormSet, BaseFormSet and InlineFormset to work like its ModelForm buddy.

With the correct refactoring of the formset code it will fix several other bugs found in newforms-admin.

Attachments (7)

formset_refactor_5.diff (61.2 KB) - added by brosner 8 years ago.
use ModelForm in ModelAdmin. refactors formset code.
formset_refactor_6_tests_fail.diff (47.5 KB) - added by Øyvind Saltvik <oyvind@…> 8 years ago.
attempt to update patch after merge with trunk
formset_refactor_7.diff (49.6 KB) - added by brosner 8 years ago.
updated to r6955 (this does not include any new changes since my last patch)
formset_refactor_8.diff (47.9 KB) - added by Øyvind Saltvik <oyvind@…> 8 years ago.
Slight change to BaseModelFormSet to allow queryset as a kwarg
formset_refactor_9.diff (52.0 KB) - added by brosner 8 years ago.
includes fixes recommended by malcolm, joseph and karen. also some various cleanups.
formset_refactor_10.diff (52.0 KB) - added by brosner 8 years ago.
removed the index parameter to get_form_class replaced with change=False
formset_refactor_7125.diff (49.5 KB) - added by jkocherhans 8 years ago.
Updated the patch to work with r7125. Still a couple of changes I'd like to make, but this is as far as I could get tonight.

Download all attachments as: .zip

Change History (19)

Changed 8 years ago by brosner

use ModelForm in ModelAdmin. refactors formset code.

comment:1 Changed 8 years ago by brosner

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Status changed from new to assigned

comment:2 Changed 8 years ago by brosner

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

The provide patch makes the FormSet class and its children work in a declarative way. I have introduced a new function named modelform_for_model which dynamically creates a ModelForm class. I believe that should be renamed to the current form_for_model and continue to depercate the form_for_instance helper function. But that can all be worked out later.

This patch also fixes inline file fields. I am also marking patch needs improvement and tests since I haven't 100% finish tests and there are some few corner cases that need to be looked at again.

comment:3 Changed 8 years ago by mtredinnick

From a quick read, I don't think modelform_for_model() should be exposed publicly (bury it in django.contrib.admin if it's needed). The idea is to move away from form_for_*() because everybody was wanting to add their own lever and button to customise them. Adding yet another method of this variety isn't going to help.

Normal "public" use-case is going to be creating a form for a specific model, so people should be using ModelForm directly. Admin is somewhat special in requiring dynamic form creation for unknown models, so it might need its own helper function, but don't leak that into the public API.

comment:4 Changed 8 years ago by brosner

Oh ok, that is a good point. I wasn't 100% sure whether or not these dynamic creation function should be publicly accessible. If anything not documented. Would be okay to just remove them from the __all__ tuple to prevent them at the django.newforms namespace? I don't think hiding them in the admin application would be best since the newforms version of the create/update generic views would then have that dependancy. See #3639.

Changed 8 years ago by Øyvind Saltvik <oyvind@…>

attempt to update patch after merge with trunk

Changed 8 years ago by brosner

updated to r6955 (this does not include any new changes since my last patch)

Changed 8 years ago by Øyvind Saltvik <oyvind@…>

Slight change to BaseModelFormSet to allow queryset as a kwarg

Changed 8 years ago by brosner

includes fixes recommended by malcolm, joseph and karen. also some various cleanups.

comment:5 Changed 8 years ago by brosner

  • Keywords nfa-blocker added

comment:6 Changed 8 years ago by brosner

  • Triage Stage changed from Design decision needed to Accepted

Changed 8 years ago by brosner

removed the index parameter to get_form_class replaced with change=False

comment:7 follow-up: Changed 8 years ago by mtredinnick

I've just read through this and a lot of flags go up. I'm not really loving the design: it smells too complicated. I'm just writing down my first thoughts, mostly for Joseph's benefit (feel free to go whichever way you like, though, Joseph; it's your baby). Trying to predict the future, this doesn't feel very maintainable (or, rather, it feels like only a few people are ever going to motivated enough to understand it to maintain it).

There's no need to use warnings in any of this code. It isn't in trunk yet. Anybody writing production code against the branch has to realise all bets are off as far as any kind of stability goes.

Do we really need Yet Another Metaclass(tm) here? Why not put the __new__ method on the formset class directly if it's really needed? Django seems to be going overboard with metaclasses at the moment, but apparently only because we can. There isn't really a huge use-case for it being replaced that can't be done just by class subclassing. Metaclasses make sense when they apply to multiple classes; they aren't intended as a holder for __new__ methods.

I'm not sure putting the num_extra attribute into _meta is a good plan. That hides it from the template. Anything in a leading underscore variable isn't intended to be accessed by public code, but the number of empty instances of an "edit inline" equivalent is useful information to have access to publically.

Replacing the ModelFormsOptions class feels wrong somehow, too. I suspect it should be possible to use it more directly. I think it's probably because trying to use a ModelForm as something like a FormSet -- which looks like what the code is trying to do -- doesn't sit well. A FormSet can be a collection of ModelForms (the main model's form, plus one for each of the related instances), but I don't think a FormSet "is a" ModelForm (or a variation on that). Trying to keep ModelForms corresponding to a single model is going to stop our brains leaking out our ears at a later date.

comment:8 Changed 8 years ago by mtredinnick

Brian, Can you please note in a comment here what the "other bugs" are that you are trying to fix here. So that we can check whatever final design we do end up with either fixes them or leaves them on the table.

comment:9 in reply to: ↑ 7 Changed 8 years ago by brosner

Replying to mtredinnick:

I've just read through this and a lot of flags go up. I'm not really loving the design: it smells too complicated. I'm just writing down my first thoughts, mostly for Joseph's benefit (feel free to go whichever way you like, though, Joseph; it's your baby). Trying to predict the future, this doesn't feel very maintainable (or, rather, it feels like only a few people are ever going to motivated enough to understand it to maintain it).

There's no need to use warnings in any of this code. It isn't in trunk yet. Anybody writing production code against the branch has to realise all bets are off as far as any kind of stability goes.

Makes sense. Have noted to remove it.

Do we really need Yet Another Metaclass(tm) here? Why not put the __new__ method on the formset class directly if it's really needed? Django seems to be going overboard with metaclasses at the moment, but apparently only because we can. There isn't really a huge use-case for it being replaced that can't be done just by class subclassing. Metaclasses make sense when they apply to multiple classes; they aren't intended as a holder for __new__ methods.

I do tend to feel similar, but I did it this to make formsets work like the Form and ModelForm class with the declarative syntax for defining fields. This was something that Joseph and I talked about and seemed to be the consensus of our discussions. InlineFormset has a decent use-case for a metaclass which would allow for generic relations to define its own metaclass to perform generic foreign key resolution.

I'm not sure putting the num_extra attribute into _meta is a good plan. That hides it from the template. Anything in a leading underscore variable isn't intended to be accessed by public code, but the number of empty instances of an "edit inline" equivalent is useful information to have access to publically.

This was done to follow the convention of how all the other metaclass type classes worked with options relating to itself. This would have to be made publicly usable similar to how orderable and deletable are accessible. It was never intended that the user would being using _meta directly in the template due to its underscore convention.

Replacing the ModelFormsOptions class feels wrong somehow, too. I suspect it should be possible to use it more directly. I think it's probably because trying to use a ModelForm as something like a FormSet -- which looks like what the code is trying to do -- doesn't sit well. A FormSet can be a collection of ModelForms (the main model's form, plus one for each of the related instances), but I don't think a FormSet "is a" ModelForm (or a variation on that). Trying to keep ModelForms corresponding to a single model is going to stop our brains leaking out our ears at a later date.

Not sure what you mean by replacing it? It was moved to options.py and subclassed by the other option classes since they shared common options. Can you explain a bit more about what you mean by using a ModelForm like a FormSet?

To answer the "other bugs" here is what I have in mind that my patch doesn't seem to address well enough.

  1. The use of form in the FormSetOptions is slightly confusing, but has a reason. It is used to define the form that the FormSet class uses. I think this needs to be addressed somehow. Not sure if this is something that needs to be fixed up in FormSet or what.
  1. Customizing the form used in either the ModelAdmin or InlineModelAdmin still seems very hairy. Right now with my patch there isn't much in the way really being able to do that nicely.
  1. In writing this patch it become apparent the inheritance used for generating the ModelForm class is slightly flawed. See #6337 for more information.

comment:10 Changed 8 years ago by Simon Litchfield <simon@…>

  • Cc simon@… added

Changed 8 years ago by jkocherhans

Updated the patch to work with r7125. Still a couple of changes I'd like to make, but this is as far as I could get tonight.

comment:11 Changed 8 years ago by jkocherhans

  • Cc jkocherhans@… added

Ugh. I can't even keep my head wrapped around this code most of the time (not just the patch, but all the model-formset integration stuff in general). It feels like django.contrib.admin.options felt a few months ago (only this mess is mostly of my own creation) :). I'm going to take a step back and try to think about this with fewer assumptions.

comment:12 Changed 7 years ago by jkocherhans

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

Fixed in [7270].

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