Code

Opened 5 years ago

Closed 3 years ago

#11163 closed (duplicate)

admin ForeignKeyRawIdWidget contains incorrect link when used in change list view

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

Description (last modified by ramiro)

When the user specifies in their admin.py that a field is both editable and a raw_id, like this:

    list_editable = ('owner',)
    raw_id_fields = ('owner',)

the ForeignKeyRawIdWidget is used in the change list view for that field. However, the code in the ForeignKeyRawIdWidget's render() method is hardcoded to be called from the edit form, with the following code:

        related_url = '../../../%s/%s/' % (self.rel.to._meta.app_label, self.rel.to._meta.object_name.lower())

When called from the change list, the url has one less entry than when called from the edit form. As a result, the link is wrong when called from the change list. For example, the url for the changelist is something like this:

http://mysite.com/admin/taskmanager/task/

while the url from the edit form is like this:

http://mysite.com/admin/taskmanager/task/1

So when used in the change list, the raw id widget's link ends up pointing to http://mysite.com/auth/user/?t=id rather than http://mysite.com/admin/auth/user/?t=id

Attachments (2)

11163.patch.1 (2.3 KB) - added by akaihola 5 years ago.
Patch against r11173: imitating what RelatedFieldWidgetWrapper does
11163.patch.2 (9.0 KB) - added by akaihola 5 years ago.
Fixed many-to-many fields and tests, too (still against r11173).

Download all attachments as: .zip

Change History (17)

comment:1 Changed 5 years ago by ramiro

  • Description modified (diff)

(reformatted description)

comment:2 Changed 5 years ago by Alex

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

This is fixed as long as you use the include(admin.site.urls) method for setting up the admin.

comment:3 Changed 5 years ago by margieroginski@…

I am including admin.site.urls. My urls.py looks like this:

from django.contrib import admin
admin.autodiscover()

import os
urlpatterns = patterns(,

(r'admin/', include(admin.site.urls)),

)

if settings.SERVE_MEDIA:

urlpatterns += patterns(,

(r'site_media/(?P<path>.*)$', 'django.views.static.serve',

{'document_root': os.path.join(os.path.dirname(file), "site_media")}),

)

I don't know if this is useful, but here is the value that site.get_urls() returns (newlines added by me):

(Pdb) from django.contrib.admin.sites import site
(Pdb) site
<django.contrib.admin.sites.AdminSite object at 0x2a9a5bf2d0>
(Pdb) site.get_urls()
[<RegexURLPattern admin_index $>,
<RegexURLPattern %sadmin_logout
logout/$>,
<RegexURLPattern admin_password_change password_change/$>,
<RegexURLPattern admin_password_change_done
password_change/done/$>,
<RegexURLPattern admin_jsi18n jsi18n/$>,
<RegexURLPattern None
r/(?P<content_type_id>\d+)/(?P<object_id>.+)/$>,
<RegexURLPattern admin_app_list (?P<app_label>\w+)/$>,
<RegexURLResolver [<RegexURLPattern admin_auth_group_changelist
$>,
<RegexURLPattern admin_auth_group_add add/$>,
<RegexURLPattern admin_auth_group_history
(.+)/history/$>,
<RegexURLPattern admin_auth_group_delete (.+)/delete/$>,
<RegexURLPattern admin_auth_group_change
(.+)/$>] auth/group/>,
<RegexURLResolver [<RegexURLPattern None
(\d+)/password/$>,
<RegexURLPattern admin_auth_user_changelist $>,
<RegexURLPattern admin_auth_user_add
add/$>,
<RegexURLPattern admin_auth_user_history (.+)/history/$>,
<RegexURLPattern admin_auth_user_delete
(.+)/delete/$>,
<RegexURLPattern admin_auth_user_change (.+)/$>] auth/user/>,
<RegexURLResolver [<RegexURLPattern admin_sites_site_changelist $>,
<RegexURLPattern admin_sites_site_add
add/$>,
<RegexURLPattern admin_sites_site_history (.+)/history/$>,
<RegexURLPattern admin_sites_site_delete
(.+)/delete/$>,
<RegexURLPattern admin_sites_site_change (.+)/$>] sites/site/>,
<RegexURLResolver [<RegexURLPattern admin_taskmanager_task_changelist $>,
<RegexURLPattern admin_taskmanager_task_add
add/$>,
<RegexURLPattern admin_taskmanager_task_history (.+)/history/$>,
<RegexURLPattern admin_taskmanager_task_delete
(.+)/delete/$>,
<RegexURLPattern admin_taskmanager_task_change (.+)/$>] taskmanager/task/>]
(Pdb)

comment:4 Changed 5 years ago by Alex

  • Resolution invalid deleted
  • Status changed from closed to reopened

Reopening because I may have confused this with the bug fixed in r10235, I'll reclose if it turns out I was indeed correct.

comment:5 follow-up: Changed 5 years ago by anonymous

I think this needs a similar fix to the one made for r10235 (but in ForeignKeyRawIdWidget). I think a similar fix may also be needed in RelatedFieldWidgetWraper, where there is another hardcoded url

related_url = '../../../%s/%s/' % (rel_to._meta.app_label, rel_to._meta.object_name.lower())

Sorry I can't jump in and fix it. I am overloaded with my own work right now, but as I get more familiar with the source and how to do patches, I will hopefully get to the point where I can.

comment:6 Changed 5 years ago by margieroginski@…

Oops. That anonymous response above is from me, Margie ...

Changed 5 years ago by akaihola

Patch against r11173: imitating what RelatedFieldWidgetWrapper does

Changed 5 years ago by akaihola

Fixed many-to-many fields and tests, too (still against r11173).

comment:7 in reply to: ↑ 5 Changed 5 years ago by akaihola

  • Component changed from Uncategorized to django.contrib.admin
  • Has patch set
  • Keywords raw_id_fields added
  • Version changed from 1.0 to SVN

Replying to margieroginski@yahoo.com:

I think this needs a similar fix to the one made for r10235 (but in ForeignKeyRawIdWidget). I think a similar fix may also be needed in RelatedFieldWidgetWraper, where there is another hardcoded url

related_url = '../../../%s/%s/' % (rel_to._meta.app_label, rel_to._meta.object_name.lower())

That looks like a fallback when reverse URL resolution fails. I tried my patch without that fallback in ForeignKeyRawIdWidget and all tests were good. I didn't try the same for RelatedFieldWidgetWrapper though.

comment:8 Changed 5 years ago by akaihola

My patch is backwards-incompatible for anyone using ForeignKeyRawIdWidget or ManyToManyRawIdWidget directly. That could be fixed by defaulting admin_site=None and falling back to old behavior if the caller omits admin_site. On the other hand, that would make the code unnecessarily messy.

comment:9 Changed 5 years ago by Guilherme Gondim <semente@…>

  • Cc semente@… added

comment:10 Changed 5 years ago by Alex

  • Triage Stage changed from Unreviewed to Accepted

comment:11 Changed 4 years ago by mykhul@…

  • Has patch unset

I am experiencing this problem on the latest trunk 1.2 pre-alpha SVN-11731

What can I do?

I was told to:

Replace the ForeignKeyRawIdWidget for your changelist formset's forms.

..what does this mean? is this the right fix? i'm completely confused

comment:12 Changed 4 years ago by akaihola

  • Has patch set
  • Patch needs improvement set

Looks like my patch is incompatible with Russell's changeset for named URL namespacing ([11250]). Unfortunately I can't dig deeper into this right now, but I hope to do it soon.

comment:13 Changed 4 years ago by bcurtu

I have tried this solution, I think it works. Just add the name of the admin view to reverse:

related_url = reverse('admin:%s_%s_changelist' % info, current_app=self.admin_site.name)

comment:14 Changed 4 years ago by zbyte64

Caution about the reverse method, if the model that the foreignkey does not exist in the current admin (possibly registered in a different admin for instance) then it will raise an error. If this error occurs we should catch it and not render the magnifying glass or fallback to original functionality. Aside from that the url reverse solution I like the most.

comment:15 Changed 3 years ago by ramiro

  • Resolution set to duplicate
  • Status changed from reopened to closed

I'm going to close this as a duplicate of #15294 that, althugh being newer, approaches the problem in a more holistic fashion. Also, refs #13588.

Add Comment

Modify Ticket

Change Properties
<Author field>
Action
as closed
as The resolution will be set. Next status will be 'closed'
The resolution will be deleted. Next status will be 'new'
Author


E-mail address and user name can be saved in the Preferences.

 
Note: See TracTickets for help on using tickets.