Opened 15 years ago

Closed 15 years ago

Last modified 12 years ago

#10061 closed (fixed)

incorrect logout link in admin

Reported by: Tim Miller Owned by: Malcolm Tredinnick
Component: contrib.admin Version: dev
Severity: Keywords:
Cc: jarek.zgoda@…, David Larlet, mgarcia@…, sebastian.rahlf@…, djjordaan@…, idan@…, xian@…, Sergey Belov, mikko@…, django@…, siddhartag@…, rodrigo@…, grf@…, DrMeers@…, mariano.mara@…, cmawebsite@…, uptimebox@…, andy@…, dtamborelli@…, beathan@… Triage Stage: Accepted
Has patch: yes Needs documentation: yes
Needs tests: yes Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

I'm using revision 9772 and the development server. After starting a new project, activating the admin interface and logging in.. admin displays an incorrect logout link.

http://127.0.0.1:8000/admin/admin/logout/

Logs out successfully if I remove the duplicate admin.

Attachments (18)

admin-urls.diff (8.5 KB ) - added by Alex Gaynor 15 years ago.
attached patch should solve the issue
admin-urls.2.diff (8.5 KB ) - added by Alex Gaynor 15 years ago.
fixed a stupid typo, I've tested this under every conceivable admin setup and it seems to work, the only thing that really needs review(IMO) is the scoping stuff in the url tag
admin-urls.3.diff (9.5 KB ) - added by Alex Gaynor 15 years ago.
added tests
admin-urls.4.diff (11.9 KB ) - added by Alex Gaynor 15 years ago.
fixed issues for admin and auth, also cleaned up the URL tag, it shouldn't alter it's own state if not necessary
admin-urls.5.diff (13.3 KB ) - added by Alex Gaynor 15 years ago.
admin-urls.6.diff (16.7 KB ) - added by Alex Gaynor 15 years ago.
admin-urls.7.diff (17.1 KB ) - added by Alex Gaynor 15 years ago.
admin-urls.8.diff (17.1 KB ) - added by Alex Gaynor 15 years ago.
admin-urls.9.diff (1.9 KB ) - added by Johan 15 years ago.
I've attempted my first try at a patch. Here its is. Its much simpler than the other patches. Am i missing something ?
admin-urls.10.diff (17.2 KB ) - added by Alex Gaynor 15 years ago.
updated for merge conflicts
admin-urls.11.diff (18.0 KB ) - added by Alex Gaynor 15 years ago.
a few extra lines of documentation
admin-urls.12.diff (14.4 KB ) - added by Alex Gaynor 15 years ago.
admin-urls.13.diff (14.4 KB ) - added by Alex Gaynor 15 years ago.
Probably just a whitespace change, but I can't remember if I've had to resolve any merge conflicts
t10061-r11201.diff (29.1 KB ) - added by Russell Keith-Magee 15 years ago.
First attempt at a complete namespace lookup fix for redeployed applications
t10061-r11201.patch1.diff (1.7 KB ) - added by Rob Hudson <treborhudson@…> 15 years ago.
patch on t10061-r11201
t10061-r11201.v2.diff (34.0 KB ) - added by Russell Keith-Magee 15 years ago.
Updated patch including suggestions from Alex and Rob
t10061-r11201.v3.diff (36.8 KB ) - added by Russell Keith-Magee 15 years ago.
Third pass - now passing full test suite.
t10061-r11021.v3.diff (36.9 KB ) - added by Russell Keith-Magee 15 years ago.
Cleanup of some naming issues and other minor tweaks.

Download all attachments as: .zip

Change History (98)

comment:1 by Tim Miller, 15 years ago

Component: Uncategorizeddjango.contrib.admin

comment:2 by Malcolm Tredinnick, 15 years ago

Triage Stage: UnreviewedAccepted

Confirmed. Only happens when one uses the include(admin.site.urls) form, so it's something introduced around r9739.

comment:3 by Alex Gaynor, 15 years ago

Owner: changed from nobody to Alex Gaynor
Status: newassigned

The issue is with the way root_path is handled, this may require some of the reversing stuff malcolm and I discussed on the mailing list.

by Alex Gaynor, 15 years ago

Attachment: admin-urls.diff added

attached patch should solve the issue

by Alex Gaynor, 15 years ago

Attachment: admin-urls.2.diff added

fixed a stupid typo, I've tested this under every conceivable admin setup and it seems to work, the only thing that really needs review(IMO) is the scoping stuff in the url tag

by Alex Gaynor, 15 years ago

Attachment: admin-urls.3.diff added

added tests

by Alex Gaynor, 15 years ago

Attachment: admin-urls.4.diff added

fixed issues for admin and auth, also cleaned up the URL tag, it shouldn't alter it's own state if not necessary

comment:4 by jleingang, 15 years ago

Applying this patch fixes the logout/password change urls but breaks a custom admin view:

class SupportAdmin(admin.AdminSite):
    def get_urls(self):
        from django.conf.urls.defaults import patterns, url
        urls = super(SupportAdmin, self).get_urls()
        my_urls = patterns('',
            url(r'^pledge/teamapproach/$', self.admin_view(self.team_approach), name="support_admin_teamapproach"),
        )
        
        return my_urls + urls
    def team_approach(self, request, *args, **kwargs):    
        ...       
        context = {
            'adminform': selector_form,
            'media': selector_form.media,
            'admin_site': self.name
        }
        return render_to_response("admin/pledge/team_approach_selector.html", context, context_instance=RequestContext(request))

This view wouldn't work unless admin_site was added to the context. Just wanted to mention this change appears to be necessary for any custom admin view

comment:5 by Alex Gaynor, 15 years ago

You are correct, I want to wait for a bit of review to the patch itself before I add that to the docs, but it should be noted.

by Alex Gaynor, 15 years ago

Attachment: admin-urls.5.diff added

comment:6 by Jarek Zgoda, 15 years ago

Cc: jarek.zgoda@… added

comment:7 by Batiste Bieler, 15 years ago

Hello, I have the same error with this test:

from django.test import TestCase

from datetime import datetime



class PostBugTestCase(TestCase):



    fixtures = ['tests.json']



    def test_stuff(self):



        from django.test.client import Client

        c = Client()

        data = {'question':'test', 'pub_date':datetime.now()}

        print c.login(username='batiste', password='b')

        response = c.post('/admin/toto/poll/add/?toto=true', data)

        #print response.content

        self.assertRedirects(response, '/admin/toto/poll/')



I applied the patch without luck.

comment:8 by Thomas Schreiber, 15 years ago

Resolution: worksforme
Status: assignedclosed

Upon patching the logout link now works for me with a second custom admin. I have one at /admin/ and another at /manage/ I tried with both regular users and superuers.

However going to /admin/logout/ (or /manage/logout/) if already logged out returns a sign-in page that when signed into immediately logs out, and returns to /admin/ (/manage/) with the regular login page. That is different than behaviour when running one admin site and a logged out user visits /admin/logout/. That would show the 'successful logout template'.

comment:9 by Thomas Schreiber, 15 years ago

Resolution: worksforme
Status: closedreopened

sorry, didn't mean to close it

by Alex Gaynor, 15 years ago

Attachment: admin-urls.6.diff added

by Alex Gaynor, 15 years ago

Attachment: admin-urls.7.diff added

by Alex Gaynor, 15 years ago

Attachment: admin-urls.8.diff added

comment:10 by Alex Gaynor, 15 years ago

Oh hey, just uploaded the same patch again, that was smart.

comment:11 by David Larlet, 15 years ago

Cc: David Larlet added

comment:12 by Russell Keith-Magee, 15 years ago

milestone: 1.1

comment:13 by Alex Ryabov, 15 years ago

Cc: alex.ryabov@… added

comment:14 by Marc Garcia, 15 years ago

Cc: mgarcia@… added

comment:15 by treworld, 15 years ago

To fix this problem:

1) Open django_directory/contrib/admin/sites.py.

2) Find "self.root_path = 'admin/'" (around line 41).

3) Replace it with "self.root_path = '/admin/'".

4) Save and restart your app/server.

comment:16 by anonymous, 15 years ago

Cc: sebastian.rahlf@… added

in reply to:  15 comment:17 by Johan, 15 years ago

This does not fix the issue. If i add my admin site on another url like so:

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

Then the admin url's are wrong again.

Replying to treworld:

To fix this problem:

1) Open django_directory/contrib/admin/sites.py.

2) Find "self.root_path = 'admin/'" (around line 41).

3) Replace it with "self.root_path = '/admin/'".

4) Save and restart your app/server.

by Johan, 15 years ago

Attachment: admin-urls.9.diff added

I've attempted my first try at a patch. Here its is. Its much simpler than the other patches. Am i missing something ?

comment:18 by Johan, 15 years ago

Cc: djjordaan@… added

comment:19 by Alex Gaynor, 15 years ago

That's a) backwards incompatible, and b) doesn't work with multiple admin sites.

by Alex Gaynor, 15 years ago

Attachment: admin-urls.10.diff added

updated for merge conflicts

by Alex Gaynor, 15 years ago

Attachment: admin-urls.11.diff added

a few extra lines of documentation

comment:20 by anonymous, 15 years ago

Has patch: set

comment:21 by Matthew Jacobi, 15 years ago

I've had this patch applied for a while now since I've been tracking trunk. Did my weekly update to trunk and was getting errors with urlresolvers.py. I reverted this patch and it fixed the errors.

Also with the latest trunk (with this patch removed), the admin urls are working again. So I think this patch might not be needed.

comment:22 by anonymous, 15 years ago

Cc: idan@… added

in reply to:  21 comment:23 by Carl Meyer, 15 years ago

Replying to dalore:

Also with the latest trunk (with this patch removed), the admin urls are working again. So I think this patch might not be needed.

This problem does still exist in Django trunk r10526.

In case it's helpful to others: the simplest and least invasive temporary workaround is to just add this line in your urls.py:

admin.site.root_path = '/admin/'

If you run the admin at an alternate URL, just use the correct one here; if you run multiple admin instances, you can set the root_path for each one in the same way.

by Alex Gaynor, 15 years ago

Attachment: admin-urls.12.diff added

comment:24 by Jacob, 15 years ago

Triage Stage: AcceptedReady for checkin

comment:25 by dc, 15 years ago

Needs documentation: set
Needs tests: set
Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

admin-urls.12.diff has lost all documentation.
Changes in core/urlresolvers.py and url template tag are not documented and do not have tests.

viewname = ''.join(viewname.split(':'))

is especially ugly. Whole thing looks like dirty hack and this line can be replaced by

viewname = viewname.replace(':', '')

anyway.

Is this ticket really so complicated that it needs special infrastructure in resolvers?

comment:26 by dc, 15 years ago

Also change in core/urlresolvers.py is backwards-incompatible as someone can use ':' in view names.

comment:27 by Malcolm Tredinnick, 15 years ago

Owner: changed from Alex Gaynor to Malcolm Tredinnick
Status: reopenednew

@dc, you're missing a whole bunch of context here, I suspect. Yes, it will be slightly backwards-incompatible and the colon was chosen for precisely that reason. We need namespaces in URL reversing now. it's basically a bug that it isn't there at the moment. We've chosen a character that is both readable, common to the majority of keyboards and relatively unlikely to appear in names (as opposed to, say, dashes or underscores, which are more commonly used or commas, which are going to be a readability hassle). The infrastructure in place here is designed to handle things a bit more broad than just the admin case.

So, yes, this approach is necessary. The particular line you point at is indeed a bug in the patch.

Since I'm in the neighbourhood now, assigning to me so that I remember to commit this darn thing over the weekend.

comment:28 by dc, 15 years ago

Maybe i'm wrong, but it looks like that you are inventing namespaces as workaround for url tag inability to work with template variables. I still think that you are breaking butterfly on the wheel (and "just strip colon and reverse" is not a very good thing IMHO). But if core developers really think that this is the best way...
At least document it please.

comment:29 by Malcolm Tredinnick, 15 years ago

Yes, you are mistaken. That's not why we're using namespaces. Unsurprisingly, it will be documented.

comment:30 by dc, 15 years ago

ok but how "namespaces" in admin-urls.12.diff differ from for e.g. {% url var1|concat:var2|concat:var3 %} Why do you want to make all this more implicit and fragile?

comment:31 by Malcolm Tredinnick, 15 years ago

*sigh* Please let's not have design discussions in tickets. Also, give us, the core commiters, some credit. We don't want to make things more implicit and fragile. That's simply a ridiculous assertion. We go for solid code, as well as neatness and usability.

As I noted in my original comment, the replacing of colons by nothing is simply a bug in the patch. It actually leads to ambiguous situations. I doubt that I'll be committing that particular line, for example. That's why patches are uploaded and reviewed with great care and why this ticket has not yet been closed. It's a very subtle area (before you post a comment pointing out that jacob moved it to "ready for checkin" .. yes, I noticed. Reasonable people can disagree on the state and timing of things and I'm sure Jacob would have carefully examined things before finally committing as well.)

Just wait a little bit and this will be resolved, please. There's a long history of the development of this. Search the django-dev archives for a lot fo the design discussion if you really care to look at it.

comment:32 by dc, 15 years ago

Have misunderstood you. I have nothing against namespaces but I just do not like this particular implementation. That was my point. I believe that you will solve problem in more elegant way. I'm sorry for disturbance.

comment:33 by Alex Gaynor, 15 years ago

We also didn't lose any docs, jacob just committed them separately(which was really the right way to handle it in the first place).

comment:34 by anonymous, 15 years ago

Cc: xian@… added

comment:35 by Sergey Belov, 15 years ago

Cc: Sergey Belov added

comment:36 by sorl, 15 years ago

Cc: mikko@… added

What about adding something like:

    def _get_root_path(self):
        if not hasattr(self, '_root_path'):
            self._root_path = reverse('%sadmin_index' % self.name)
        return self._root_path
    root_path = property(_get_root_path)

to the AdminSite class?

comment:37 by sorl, 15 years ago

sorry I didn't reed up properly, discard my suggestion, just use it if you want something working now.

comment:38 by chr, 15 years ago

Cc: django@… added

comment:39 by siddhi, 15 years ago

Cc: siddhartag@… added

comment:40 by anonymous, 15 years ago

Cc: rodrigo@… added

comment:41 by leemarrett, 15 years ago

this problem is still happening in revision 10865, and it's happening with the change password link as well.

if urls.py is setting the admin url to (r'admin/', include(admin.site.urls)), the links are adding an extra "admin/" to the path and breaking because "admin/admin/logout" does not exist.

comment:42 by mmara, 15 years ago

I can confirm the problem still exists with rev10980

comment:43 by anonymous, 15 years ago

people, you don't want to release 1.1 with this embarassing bug, really!

comment:44 by David Larlet, 15 years ago

No, and that's why it's tagged with 1.1 milestone.

comment:45 by anonymous, 15 years ago

Cc: grf@… added

by Alex Gaynor, 15 years ago

Attachment: admin-urls.13.diff added

Probably just a whitespace change, but I can't remember if I've had to resolve any merge conflicts

comment:46 by Simon Meers, 15 years ago

Cc: DrMeers@… added

comment:47 by Alex Ryabov, 15 years ago

Cc: alex.ryabov@… removed

comment:48 by mmara, 15 years ago

Cc: mariano.mara@… added

comment:49 by Collin Anderson, 15 years ago

Cc: cmawebsite@… added

Sorry for the noise.

comment:50 by Mikhail, 15 years ago

Cc: uptimebox@… added

comment:51 by anonymous, 15 years ago

Cc: andy@… added

comment:52 by anonymous, 15 years ago

Is anyone working on this?

comment:53 by Russell Keith-Magee, 15 years ago

Yes - I'm working on it. We know the problem exists, and it needs to be fixed - that's why it's the last blocker on v1.1. The issue is that the fix isn't obvious - Alex's solution is closer to a workaround than a real fix.

I have a patch locally which I'm almost ready to share - there is still a couple of edge cases that I need to resolve.

comment:54 by Juan, 15 years ago

First time I saw this bug, I though was a simple typo/confusion error, I never realized that it would be the last (and probabbly the hardest) one.

BTW, I have 1 year using Python and 6 months using Django (wonderful couple) I hope some day I get the skills to make contributions.
Congratulations.

comment:55 by André Cruz, 15 years ago

Cc: andre.cruz@… added

by Russell Keith-Magee, 15 years ago

Attachment: t10061-r11201.diff added

First attempt at a complete namespace lookup fix for redeployed applications

comment:56 by Russell Keith-Magee, 15 years ago

Ok - here's a first pass at revised fix for this. No guarantees that this is the final solution, or that details won't change between now and final commit. Feedback welcome.

comment:57 by anonymous, 15 years ago

Cc: dtamborelli@… added

comment:58 by Rob Hudson <treborhudson@…>, 15 years ago

I'm playing around with it a bit on a project that this bug was showing up with. Here are a few notes. Hopefully you find them helpful:

  • There's a print statement in django.core.urlresolvers L412
  • In django.contrib.admin.sites L235 reverse is used but it's not imported
  • Traversing into the /admin/docs/ area I'm getting an error 'function' object has no attribute 'split' at django/core/urlresolvers.py in reverse, line 298

I've fixed each of these in the patch to be posted next. Is a patch of a patch the best here?

by Rob Hudson <treborhudson@…>, 15 years ago

Attachment: t10061-r11201.patch1.diff added

patch on t10061-r11201

comment:59 by Alex Gaynor, 15 years ago

Couples thoughts:

1) I don't see any tests of the admin itself to prove this really works.
2) I don't think Rob's fix for the split() error is correct, reversing a function itself is legal, there should be a conditional arround the view.split() line in the event it isn't a basestring.
3) I'm not sure I follow how {% url admin:logout %} works, as best I can see there's no admin in the context, thus that will always reverse the string "admin:logout" which is only correct in the case where the name of the admin_site is in fact "admin", and thus the reverse fails on custom AdminSites in a multi-adminsite scenario.

in reply to:  59 comment:60 by Russell Keith-Magee, 15 years ago

Replying to Alex:

Couples thoughts:

1) I don't see any tests of the admin itself to prove this really works.

No - it's testing the more general problem - that URL namespace lookup. A test of the admin probably wouldn't go astray as a literal regression for the ticket, but once you have proved 1+1=2, there isn't a whole lot of gain in proving 1+2 = 3.

3) I'm not sure I follow how {% url admin:logout %} works, as best I can see there's no admin in the context, thus that will always reverse the string "admin:logout" which is only correct in the case where the name of the admin_site is in fact "admin", and thus the reverse fails on custom AdminSites in a multi-adminsite scenario.

Did you try the patch, Alex? If you do, you'll find that it works for the case you describe.

You are correct there's no admin mentioned in the public context. That's the point. However, the context is tracking the name of the current admin instance.

The reason this approach works is that admin:logout isn't doing a simple lookup for the namespace 'admin' - it's looking for a registered application called admin. The reverse function is provided an argument (app_name - probably better called 'current_app') that lets the application rendering the template specify the currently active application instance. When rendering the views for 'customadmin', the current app is set to 'customadmin', and so admin:logout is transormed to customadmin:logout, which resolves as /customadmin/logout. If the current app is 'admin', then admin:logout is transformed to admin:logout, which resolves as a /admin/logout.

in reply to:  58 comment:61 by Russell Keith-Magee, 15 years ago

Replying to Rob Hudson <treborhudson@gmail.com>:

I'm playing around with it a bit on a project that this bug was showing up with. Here are a few notes. Hopefully you find them helpful:

  • There's a print statement in django.core.urlresolvers L412
  • In django.contrib.admin.sites L235 reverse is used but it's not imported
  • Traversing into the /admin/docs/ area I'm getting an error 'function' object has no attribute 'split' at django/core/urlresolvers.py in reverse, line 298

Thanks for those pickups, Rob. 1+2 are no brainers; I'll have to check 3 to see that it isn't masking something more sinister.

I've fixed each of these in the patch to be posted next. Is a patch of a patch the best here?

As good as anything.

in reply to:  59 ; comment:62 by Russell Keith-Magee, 15 years ago

Replying to Alex:

Couples thoughts:

2) I don't think Rob's fix for the split() error is correct, reversing a function itself is legal, there should be a conditional arround the view.split() line in the event it isn't a basestring.

What evidence can you provide to support this assertion, Alex? As best as I can make out, the line that Rob has identified has historically fallen through to the NoReverseMatch exception, rather than being handled as a callable. I can't find any example in code or docs where a a reverse lookup is based on a callable. All the usage (documented and actual) that I can see uses reverse(string), which makes sense to me given the purpose of the function.

On top of that, admin.site.root is marked as deprecated, so we would be well served to avoid using it.

by Russell Keith-Magee, 15 years ago

Attachment: t10061-r11201.v2.diff added

Updated patch including suggestions from Alex and Rob

comment:63 by Russell Keith-Magee, 15 years ago

A updated fix - this addresses the issues found by Rob, including some related problems found with the admindocs app. It also adds regression tests for the admin site. The full test suite is still known to throw 18 errors in comment_tests, and 1 failure in admin_widgets. I'm looking into these failures.

in reply to:  62 comment:64 by Russell Keith-Magee, 15 years ago

Replying to russellm:

Replying to Alex:

Couples thoughts:

2) I don't think Rob's fix for the split() error is correct, reversing a function itself is legal, there should be a conditional arround the view.split() line in the event it isn't a basestring.

What evidence can you provide to support this assertion, Alex? As best as I can make out, the line that Rob has identified has historically fallen through to the NoReverseMatch exception, rather than being handled as a callable.

For my next trick, I will put my entire lower leg into my mouth. Apologies, Alex - you are correct: a callable should be allowed to reverse, the docs say so, and so do the comment tests (hence the 18 errors). I'm about to attach a patch that passes the full test suite.

by Russell Keith-Magee, 15 years ago

Attachment: t10061-r11201.v3.diff added

Third pass - now passing full test suite.

comment:65 by anonymous, 15 years ago

Cc: beathan@… added

comment:66 by dc, 15 years ago

Russell, may i suggest using isinstance() for type checking?
Better replace

type(view) == tuple

with

isinstance(view, tuple)

Not critical but more pythonic.

in reply to:  66 comment:67 by anonymous, 15 years ago

Replying to dc:

Russell, may i suggest using isinstance() for type checking?
Better replace

type(view) == tuple

with

isinstance(view, tuple)

Not critical but more pythonic.

And more PEP8

comment:68 by Alex Gaynor, 15 years ago

Another possible bug: The password_change_done method on AdminSites reverses using the form reverse('%s:password_change_done' % self.name), while the render method on RelatedFieldWidgetWrapper uses the form reverse('admin:%s_%s_add' % info, app_name=self.admin_site.name). My understanding is that the 2nd form would be correct, however it seems that the term app_name is being used inconsistently, AdminSite instances have both a name and an app_name, here we are passing the name as a parameter named app_name, which is rather confusing.

The get_root_path method will, as far as I can tell, fail using the old admin.site.root form.

Another concern is the use of app_name on Context, I think this attribute should be exclusively on RequestContext, as this is the object that knows about the rest of the Django environ. The only change that needs to be made in light of this is having the URLNode.render use getattr(context, 'app_name', None) in place of context.app_name.

Finally the new form for extending admin urls needs to be documented (as does the namespacing infrastructure probably).

in reply to:  66 comment:69 by Russell Keith-Magee, 15 years ago

Replying to dc:

Russell, may i suggest using isinstance() for type checking?

This was more an accident of history, rather than intent. I've corrected it in my local checkout, and made the check a little more liberal (checking for tuple and list, not just tuple).

in reply to:  68 comment:70 by Russell Keith-Magee, 15 years ago

Replying to Alex:

Another possible bug: The password_change_done method on AdminSites reverses using the form reverse('%s:password_change_done' % self.name), while the render method on RelatedFieldWidgetWrapper uses the form reverse('admin:%s_%s_add' % info, app_name=self.admin_site.name). My understanding is that the 2nd form would be correct,

You are correct. I've fixed this in my local branch.

however it seems that the term app_name is being used inconsistently, AdminSite instances have both a name and an app_name, here we are passing the name as a parameter named app_name, which is rather confusing.

As I noted in comment 60, the app_name argument to Context and reverse should probably be something like 'current_app'. I'll make this change.

The get_root_path method will, as far as I can tell, fail using the old admin.site.root form.

Can you elaborate on the error case here? The only way I can generate problems is if I mix old-style and new-style admin registrations, which is understandable given that the old-form doesn't define (or utilize) the app_name. I don't see any problem with a standard "old-admin registration plus admin docs" setup, or a 'multiple new-admin plus admin docs".

Another concern is the use of app_name on Context, I think this attribute should be exclusively on RequestContext, as this is the object that knows about the rest of the Django environ. The only change that needs to be made in light of this is having the URLNode.render use getattr(context, 'app_name', None) in place of context.app_name.

Hrm.. Not sure I agree here. RequestContext is the context that knows about the request, not necessarily the entire environment (although, as a web app, a lot of the current environment is contained or associated with the request). I don't think there's any examples in admin, but I can see of situations where a simple help page in a redeployable app might return a Context, rather than a RequestContext, but it would still be appropriate to be able to reverse a name to that help page.

Finally the new form for extending admin urls needs to be documented (as does the namespacing infrastructure probably).

Granted. I'm waiting until the syntax settles down before I start writing docs.

I would, however, make some sort of comment about glass houses... the include(object.urls) notation that you contributed wasn't documented until last night, and you didn't write the docs :-)

comment:71 by Russell Keith-Magee, 15 years ago

For those interested, I'm pushing my modifications into my public git repository: http://github.com/freakboy3742/django/tree/t10061-rkm

comment:72 by Ramiro Morales, 15 years ago

Russell: (simple nitpick) If the user isn't supposed to modify the app_name and name attibutes inside AdminSite.get_urls(), wouldn't it be better to leave it to return an urlpattern collection as it does currently and make the urls method return self.get_urls(), self.app_name, self.name?.

This would simplfy the admin customization API and would keep the method consistent with its ModelAdmin counterpart.

comment:73 by Alex Gaynor, 15 years ago

ramiro's suggestion sounds look a good one. And failing that then the auth UserAdmin needs to be updated for the new syntax.

in reply to:  72 comment:74 by Russell Keith-Magee, 15 years ago

Replying to ramiro:

Russell: (simple nitpick) If the user isn't supposed to modify the app_name and name attibutes inside AdminSite.get_urls(), wouldn't it be better to leave it to return an urlpattern collection as it does currently and make the urls method return self.get_urls(), self.app_name, self.name?.

This would simplfy the admin customization API and would keep the method consistent with its ModelAdmin counterpart.

Good idea, Ramiro. I've made this change on Github. I'm about to upload a new patch to reflect all the recent changes for those that aren't git-enabled.

by Russell Keith-Magee, 15 years ago

Attachment: t10061-r11021.v3.diff added

Cleanup of some naming issues and other minor tweaks.

comment:75 by Russell Keith-Magee, 15 years ago

Resolution: fixed
Status: newclosed

(In [11250]) Fixed #10061 -- Added namespacing for named URLs - most importantly, for the admin site, where the absence of this facility was causing problems. Thanks to the many people who contributed to and helped review this patch.

This change is backwards incompatible for anyone that is using the named URLs
introduced in [9739]. Any usage of the old admin_XXX names need to be modified
to use the new namespaced format; in many cases this will be as simple as a
search & replace for "admin_" -> "admin:". See the docs for more details on
the new URL names, and the namespace resolution strategy.

comment:76 by mjs7231, 15 years ago

Did this fix make it into the django-1.1 release?
It's hard for me to tell by the dates.

comment:77 by André Cruz, 15 years ago

Cc: andre.cruz@… removed

in reply to:  76 comment:78 by Karen Tracey, 15 years ago

Replying to mjs7231:

Did this fix make it into the django-1.1 release?
It's hard for me to tell by the dates.

Yes. A clue is it's got a 1.1 milestone and it's fixed. If it had been deferred from 1.1 the milestone would have been changed. If it didn't have a milestone or if you didn't trust it the sure way to check is to see here:

http://code.djangoproject.com/log/django/tags/releases/1.1

that 1.1 was tagged at r11366, which is after the r11250 changeset that fixed this ticket. Thus the fix is in the 1.1 release.

comment:79 by Antti Kaihola, 14 years ago

For cross reference: r11250 broke patch 2 for #11163 (wrong ForeignKeyRawIdWidget link in admin change list view). I haven't figured out what the exact reason is, but once I do, I'll prepare a new patch for #11163. Any ideas?

comment:80 by Jacob, 12 years ago

milestone: 1.1

Milestone 1.1 deleted

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