Opened 18 years ago
Closed 16 years ago
#3987 closed (fixed)
ModelAdmin should allow for overriding of ForeignKey/ManyToMany options
Reported by: | Owned by: | nobody | |
---|---|---|---|
Component: | contrib.admin | Version: | newforms-admin |
Severity: | Keywords: | nfa-someday | |
Cc: | brosner@…, Florian Apolloner, Martin Diers | Triage Stage: | Design decision needed |
Has patch: | yes | Needs documentation: | no |
Needs tests: | yes | Patch needs improvement: | yes |
Easy pickings: | no | UI/UX: | no |
Description (last modified by )
See discussion in comments.
Attachments (11)
Change History (44)
by , 18 years ago
Attachment: | customfilters.diff added |
---|
comment:1 by , 18 years ago
Has patch: | set |
---|---|
Patch needs improvement: | set |
comment:2 by , 18 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
comment:3 by , 17 years ago
Version: | new-admin → SVN |
---|
comment:4 by , 17 years ago
Version: | SVN → newforms-admin |
---|
comment:5 by , 17 years ago
Description: | modified (diff) |
---|---|
Summary: | Customs filters for related keys in the admin (newforms-admin) → ModelAdmin should allow for overriding of ForeignKey/ManyToMany options |
Triage Stage: | Design decision needed → Accepted |
I like the idea, but the implementation is a bit brittle. What about adding method hooks on ModelAdmin
that would allow you to specify the options for a given ForeignKey
field? These methods could get passed the request object, so you could do per-user options. Something like this:
# Model class Book(models.Model): title = models.CharField(maxlength=100) author = models.ForeignKey(Author) # Admin class BookAdmin(ModelAdmin): def dynamic_author_choices(self, request, book): # Default implementation: # return book.author_set.all() return book.author_set.filter(use_in_admin=True)
comment:6 by , 17 years ago
If dynamic_field_choices() is automatically called for every field when admin is displayed, that would be great, but should also work for ManyToManyField.
comment:7 by , 17 years ago
Cc: | added |
---|
comment:8 by , 17 years ago
Patch needs improvement: | unset |
---|
Okay, here is a new patch that I hope cleaner.
It works almost like Adrian suggested it :
# Model class Book(models.Model): title = models.CharField(maxlength=100) author = models.ForeignKey(Author) # Admin class BookAdmin(ModelAdmin): def dynamic_author_choices(self, request, model): # Default implementation: # return book.author_set.all() return model.objects.filter(use_in_admin=True)
And also allows:
class BookAdmin(ModelAdmin): def dynamic_author_choices(self, request, model): if request.user.is_superuser: return model.objects.all() return model.objects.filter(use_in_admin=True)
by , 17 years ago
Attachment: | diff-dynamic.3.diff added |
---|
security fix : the choice of an item not in the list of choices must fail
comment:9 by , 17 years ago
Cc: | added |
---|
comment:11 by , 17 years ago
I attached an updated version of patch. It now works with revision: 6426.
A little change I did to the patch is removal of all occurences of:
defaults = {'form_class': forms.ModelChoiceField} #To prevent an useless query if not 'queryset' in kwargs: defaults['queryset'] = self.rel.to._default_manager.all()
in favour of original code:
defaults = {'form_class': forms.ModelChoiceField, 'queryset': self.rel.to._default_manager.all()}
The reason is that QuerySets are lazy so there is no "useless query".
by , 17 years ago
Attachment: | dynamic_related_3987.diff added |
---|
Patch updated to work with version 6426
comment:12 by , 17 years ago
I replaced the previous version of the patch, because it had a small bug. I did simple replacement in line 203 of models.py (s/value/val/)
comment:13 by , 17 years ago
Keywords: | nfa-someday added |
---|---|
Needs documentation: | set |
Needs tests: | set |
New function so it shouldn't block newforms-admin merge. Also it needs docs and tests, I think.
follow-ups: 15 16 comment:14 by , 17 years ago
It doesn't block the merge since it is a newforms-admin patch.
But I am not going to write doc and tests since I haven't had any feedback about this and I have no idea if it has a chance to be accepted! If someone said it could be accepted, I would do it, sure.
comment:15 by , 17 years ago
Replying to Baptiste:
But I am not going to write doc and tests since I haven't had any feedback about this and I have no idea if it has a chance to be accepted! If someone said it could be accepted, I would do it, sure.
Triage stage is Accepted, and you've updated the patch to be in line with Adrian's suggestion, so I was thinking it had been accepted. I believe a complete package is more likely than just a code patch to get the review needed to move on to ready for checkin.
comment:16 by , 17 years ago
Replying to Baptiste:
But I am not going to write doc and tests since I haven't had any feedback about this and I have no idea if it has a chance to be accepted! If someone said it could be accepted, I would do it, sure.
It should be accepted. Enough? ;) Hey! This patch is very important to me. It would be great to have it merged with the trunk!
follow-up: 18 comment:17 by , 16 years ago
Replying to Baptiste <baptiste.goupil@gmail.com>:
See discussion in comments.
I think I have upgraded the patch to work with latest SVN copy. First time looking into the code!
Would greatly appreciate it if this patch was included in 1.0 as it's an incredibly handy feature.
comment:18 by , 16 years ago
Replying to mikeblake:
Would greatly appreciate it if this patch was included in 1.0 as it's an incredibly handy feature.
It was accepted by Adrian and seems to have multiple people interested in it, however it's got no docs nor tests. That's probably preventing it from moving along in the process.
by , 16 years ago
New patch (sorry, fisrt try) , forget/delete the preceding one if possible
comment:19 by , 16 years ago
First proposal for a documentation.
Forget the first patch, totally wrong.
comment:20 by , 16 years ago
Cc: | added; removed |
---|
comment:21 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:22 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:23 by , 16 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Resolution: | fixed |
Status: | closed → reopened |
Triage Stage: | Accepted → Ready for checkin |
comment:24 by , 16 years ago
Owner: | changed from | to
---|---|
Status: | reopened → new |
Triage Stage: | Ready for checkin → Someday/Maybe |
comment:25 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
comment:26 by , 16 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
comment:27 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → closed |
comment:28 by , 16 years ago
Resolution: | fixed |
---|---|
Status: | closed → reopened |
Looks like Mr Anonymous is having a bit of fun here...
comment:29 by , 16 years ago
I'ld like to show in model A list_filter model B fields that have a ForeignKey to model A.
I've applied dynamic_related_3987.2.diff to django svn trunk.
I've add a method get_b() to the A admin class ("class AAdmin(admin.ModelAdmin):") that returns a.b_set.all(), but I don't know how I've to include it in list_filter. I've put "list_filter = (...,'get_b')" but get the error:
Error while importing URLconf 'myapp.urls': `AAdmin.list_filter[9]` refers to field `get_b` that is missing from model `A`.
What should be solve the error?
Thanks in advance.
by , 16 years ago
Attachment: | dynamic_choice_9014.diff added |
---|
Updated patch to rev 9014. This will work with 1.0 also.
comment:30 by , 16 years ago
Cc: | added |
---|
comment:31 by , 16 years ago
There is a serious problem with this patch. It changes the formfield_for_dbfield method of ModelAdmin, requiring a request object to passed in as a positional argument. This method is not in the docs, but it is commonly overridden, and represents a significant change to the API for the Admin.
There is a way to do what this patch does on 1.0, without any patching:
class SiteAdmin(ModelAdmin): def __call__(self, request, url): #Add in the request object, so that it may be referenced #later in the formfield_for_dbfield function. self.request = request return super(SiteAdmin, self).__call__(request, url) def get_form(self, request, obj=None): form = super(SiteAdmin, self).get_form(request, obj) #print form return form def formfield_for_dbfield(self, db_field, **kwargs): field = super(SiteAdmin, self).formfield_for_dbfield(db_field, **kwargs) # Get the default field if db_field.name == 'home_page': #Add the null object my_choices = [('', '---------')] #Grab the current site id from the URL. my_choices.extend(Page.objects.filter(site=self.request.META['PATH_INFO'].split('/')[-2]).values_list('id','name')) print my_choices field.choices = my_choices return field
The trick is the override of _ _call_ _, adding the request object as an attribute of the instance. Once you have this, you can do pretty much anything you need in the formfield_for_dbfield override. Granted, this is not very elegant. But it works in 1.0, and our users are already accustomed to overriding formfield_for_dbfield to make custom form tweaks.
comment:32 by , 16 years ago
Needs tests: | set |
---|---|
Patch needs improvement: | set |
Triage Stage: | Someday/Maybe → Design decision needed |
Sorry. Had some superfluous code in that example. Updated version for reference:
class SiteAdmin(ModelAdmin): def __call__(self, request, url): #Add in the request object, so that it may be referenced #later in the formfield_for_dbfield function. self.request = request return super(SiteAdmin, self).__call__(request, url) def formfield_for_dbfield(self, db_field, **kwargs): field = super(SiteAdmin, self).formfield_for_dbfield(db_field, **kwargs) # Get the default field if db_field.name == 'home_page': #Add the null object my_choices = [('', '---------')] #Grab the current site id from the URL. my_choices.extend(Page.objects.filter(site=self.request.META['PATH_INFO'].split('/')[-2]).values_list('id','name')) print my_choices field.choices = my_choices return field
comment:33 by , 16 years ago
Resolution: | → fixed |
---|---|
Status: | reopened → 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 aroundrequest
so that you can easily modify fields based on request (as in #3987).
Thanks to James Bennet for the original patch; Alex Gaynor and Brian Rosner also contributed.
the patch