Code

Opened 6 years ago

Closed 6 years ago

Last modified 3 years ago

#6962 closed (wontfix)

admin/doc pages aren't handled by admin.site.root

Reported by: Karen Tracey <kmtracey@…> Owned by: nobody
Component: contrib.admin Version: newforms-admin
Severity: Keywords: nfa-blocker
Cc: v.oostveen@…, cmawebsite@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: UI/UX:

Description

newforms-admin branch page says to convert to newforms-admin change your urls.py to include:

urlpatterns = patterns('',
    ('^admin/(.*)', admin.site.root),
)

but admin.site.root doesn't handle the admin documentation pages that begin with admin/doc/, so the "Documentation" link at top right of every admin page results in a page not found error. It's easy enough to fix if you include:

    (r'^admin/doc/', include('django.contrib.admindocs.urls')),

in urls.py ahead of the more general admin spec, but given that that is not mentioned on the branch page I'm not sure if the intent is to have people do that or to have it be handled by admin.site.root. Personally I'd prefer a single-line addition to urls.py that makes the entire admin site work, not multiple lines, so I'd rather admin.site.root be changed to handle these versus updating the transition doc.

Attachments (2)

newforms-admin-admindocs.diff (966 bytes) - added by trbs 6 years ago.
6962_admindoc_url_fix.1.diff (1.8 KB) - added by brosner 6 years ago.
use a named url for admin doc

Download all attachments as: .zip

Change History (16)

comment:1 Changed 6 years ago by preston clark

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

Hello.

I'm fairly new here, both to Django and Python, apologies if I'm wrong. As far as I can tell, running the admin site in newforms-admin as prescribed in the wiki, admin.urls is being bypassed altogether and delegation of urls is going straight to the admin.site.root view. Would it be wrong to more closely tie admin and admindocs together in the following way?

In your urls.py:

urlpatterns = patterns('',
    ('^admin/(.*)', include('django.contrib.admin.urls')),
)

and change django.contrib.admin.urls to:

from django.contrib import admin

urlpatterns = patterns('',
    ('^doc/', include('django.contrib.admindocs.urls')),
    ('(.*)', admin.site.root),
)

This way, admin.site.root again fields everything except documentation urls, and the end-user only has to specify an admin url pattern once.

comment:2 Changed 6 years ago by Karen Tracey <kmtracey@…>

That sounds like a good approach to me. One less thing people have to do when migrating to newforms-admin, since the admin entry in the app's urls.py stays the same (perhaps that was always the intent and what's currently on the branch page is just interim?). Looks like the urls.py that is currently in newforms-admin/django/contrib/admin needs a little cleanup to do this and make sure all the old entries that haven't been moved elsewhere are covered by some means in the new code.

Changed 6 years ago by trbs

comment:3 Changed 6 years ago by trbs

  • Cc v.oostveen@… added
  • Has patch set

added a patch to include admindocs inside of AdminSite.root

it resolves the urlpatterns from django.contrib.admindocs.urls so there's no duplicated url configuration.

comment:4 Changed 6 years ago by Simon Greenhill

  • Triage Stage changed from Unreviewed to Ready for checkin

comment:5 Changed 6 years ago by garcia_marc

  • milestone set to 1.0 alpha

Changed 6 years ago by brosner

use a named url for admin doc

comment:6 Changed 6 years ago by brosner

The original idea was to keep the admindocs app separated from the admin app. Does it seem sensible to just give a named url?

comment:7 Changed 6 years ago by shanx

To me that seems to be a nice way of doing it. I'll have a look at this as well.

comment:8 Changed 6 years ago by shanx

Yeah I personally think it is the best way of solving this, but what about the 'documentation' link when admindocs app is not installed? I think it would be best for it to not be displayed in the admin.

comment:9 Changed 6 years ago by anonymous

  • Cc cmawebsite@… added

comment:10 Changed 6 years ago by brosner

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

The general issue described here is being handled with #7466 which will fix the bad relative paths in the documentation app. However, in looking at this fully I can't just do a named URL for documentation because Change Password and Logout need their routes as well. I will be committing a fix to #7466 shortly which keeps admindocs decoupled from the admin code.

comment:11 follow-up: Changed 6 years ago by shanx

  • Resolution wontfix deleted
  • Status changed from closed to reopened

I'm not sure if I can fully agree with this. There is in fact always a coupling between the admin and the admindocs app, because you refer users to the docs in the admin interface. The way it is solved now in #7466 is that I'm forced to do this in the main urls.py file:

    (r'^admin/doc/', include('django.contrib.admindocs.urls')),

While I may prefer to have the admindocs app be available on a different url like so:

    (r'^admin/documentation/', include('django.contrib.admindocs.urls')),

This is not possible the way it is fixed in #7466.

comment:12 in reply to: ↑ 11 Changed 6 years ago by Karen Tracey <kmtracey@…>

Replying to shanx:

While I may prefer to have the admindocs app be available on a different url like so:

[snipped]

This is not possible the way it is fixed in #7466.

But you couldn't do that in old admin either, could you?

My objection is that I do not like that users now have to add two lines to their urls.py file, I think that is trouble waiting to happen when they only add one of them. The admin pages all have this "Documentation" link in the upper right corner, so admindocs in my mind ARE tied to the basic admin code. Users should not have to do something other than the basic "activate admin" in order to make that "Documentation" link work.

I don't understand what is the problem with the fix recommended by preston clark above? If that approach is taken then users don't have to change anything in their urls.py when migrating to newforms-admin.

BTW not all relative links are working after the related fix but I will address that in its own ticket.

comment:13 Changed 6 years ago by brosner

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

While I understand the argument for making it simple to setup, but the admindocs are explicitly defined in their own URL for good reasons.

  1. The admin docs have no way to filter models, it is meant for project wide documentation and coupling them to the AdminSite would really show undesirable results plus having multiple URLs to get at them.
  1. It lets the user customize them exactly how shanx suggested. In which I broke and will fix.

It is a known backward incompatibility on http://code.djangoproject.com/wiki/NewformsAdminBranch. Please take the discussion to django-dev for further consideration. Also I am wontfixing again, so shanx can you please open a new ticket for the issue you described.

comment:14 Changed 3 years ago by jacob

  • milestone 1.0 alpha deleted

Milestone 1.0 alpha deleted

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.