Code

Opened 5 years ago

Closed 5 years ago

Last modified 3 years ago

#10061 closed (fixed)

incorrect logout link in admin

Reported by: lashni Owned by: mtredinnick
Component: contrib.admin Version: master
Severity: Keywords:
Cc: jarek.zgoda@…, david, mgarcia@…, sebastian.rahlf@…, djjordaan@…, idan@…, xian@…, arikon, 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: UI/UX:

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 5 years ago.
attached patch should solve the issue
admin-urls.2.diff (8.5 KB) - added by Alex 5 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 5 years ago.
added tests
admin-urls.4.diff (11.9 KB) - added by Alex 5 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 5 years ago.
admin-urls.6.diff (16.7 KB) - added by Alex 5 years ago.
admin-urls.7.diff (17.1 KB) - added by Alex 5 years ago.
admin-urls.8.diff (17.1 KB) - added by Alex 5 years ago.
admin-urls.9.diff (1.9 KB) - added by Johan 5 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 5 years ago.
updated for merge conflicts
admin-urls.11.diff (18.0 KB) - added by Alex 5 years ago.
a few extra lines of documentation
admin-urls.12.diff (14.4 KB) - added by Alex 5 years ago.
admin-urls.13.diff (14.4 KB) - added by Alex 5 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 russellm 5 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@…> 5 years ago.
patch on t10061-r11201
t10061-r11201.v2.diff (34.0 KB) - added by russellm 5 years ago.
Updated patch including suggestions from Alex and Rob
t10061-r11201.v3.diff (36.8 KB) - added by russellm 5 years ago.
Third pass - now passing full test suite.
t10061-r11021.v3.diff (36.9 KB) - added by russellm 5 years ago.
Cleanup of some naming issues and other minor tweaks.

Download all attachments as: .zip

Change History (98)

comment:1 Changed 5 years ago by lashni

  • Component changed from Uncategorized to django.contrib.admin
  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 5 years ago by mtredinnick

  • Triage Stage changed from Unreviewed to Accepted

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

comment:3 Changed 5 years ago by Alex

  • Owner changed from nobody to Alex
  • Status changed from new to assigned

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.

Changed 5 years ago by Alex

attached patch should solve the issue

Changed 5 years ago by Alex

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

Changed 5 years ago by Alex

added tests

Changed 5 years ago by Alex

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 Changed 5 years ago by jleingang

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 Changed 5 years ago by Alex

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.

Changed 5 years ago by Alex

comment:6 Changed 5 years ago by zgoda

  • Cc jarek.zgoda@… added

comment:7 Changed 5 years ago by batiste

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 Changed 5 years ago by rizumu

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

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 Changed 5 years ago by rizumu

  • Resolution worksforme deleted
  • Status changed from closed to reopened

sorry, didn't mean to close it

Changed 5 years ago by Alex

Changed 5 years ago by Alex

Changed 5 years ago by Alex

comment:10 Changed 5 years ago by Alex

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

comment:11 Changed 5 years ago by david

  • Cc david added

comment:12 Changed 5 years ago by russellm

  • milestone set to 1.1

comment:13 Changed 5 years ago by haron

  • Cc alex.ryabov@… added

comment:14 Changed 5 years ago by garcia_marc

  • Cc mgarcia@… added

comment:15 follow-up: Changed 5 years ago by 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.

comment:16 Changed 5 years ago by anonymous

  • Cc sebastian.rahlf@… added

comment:17 in reply to: ↑ 15 Changed 5 years ago by Johan

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.

Changed 5 years ago by Johan

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 Changed 5 years ago by Johan

  • Cc djjordaan@… added

comment:19 Changed 5 years ago by Alex

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

Changed 5 years ago by Alex

updated for merge conflicts

Changed 5 years ago by Alex

a few extra lines of documentation

comment:20 Changed 5 years ago by anonymous

  • Has patch set

comment:21 follow-up: Changed 5 years ago by dalore

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 Changed 5 years ago by anonymous

  • Cc idan@… added

comment:23 in reply to: ↑ 21 Changed 5 years ago by carljm

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.

Changed 5 years ago by Alex

comment:24 Changed 5 years ago by jacob

  • Triage Stage changed from Accepted to Ready for checkin

comment:25 Changed 5 years ago by dc

  • Needs documentation set
  • Needs tests set
  • Patch needs improvement set
  • Triage Stage changed from Ready for checkin to Accepted

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 Changed 5 years ago by dc

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

comment:27 Changed 5 years ago by mtredinnick

  • Owner changed from Alex to mtredinnick
  • Status changed from reopened to new

@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 Changed 5 years ago by dc

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 Changed 5 years ago by mtredinnick

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

comment:30 Changed 5 years ago by dc

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 Changed 5 years ago by mtredinnick

*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 Changed 5 years ago by dc

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 Changed 5 years ago by Alex

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 Changed 5 years ago by anonymous

  • Cc xian@… added

comment:35 Changed 5 years ago by arikon

  • Cc arikon added

comment:36 Changed 5 years ago by sorl

  • 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 Changed 5 years ago by sorl

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

comment:38 Changed 5 years ago by chr

  • Cc django@… added

comment:39 Changed 5 years ago by siddhi

  • Cc siddhartag@… added

comment:40 Changed 5 years ago by anonymous

  • Cc rodrigo@… added

comment:41 Changed 5 years ago by leemarrett

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 Changed 5 years ago by mmara

I can confirm the problem still exists with rev10980

comment:43 Changed 5 years ago by anonymous

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

comment:44 Changed 5 years ago by david

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

comment:45 Changed 5 years ago by anonymous

  • Cc grf@… added

Changed 5 years ago by Alex

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

comment:46 Changed 5 years ago by DrMeers

  • Cc DrMeers@… added

comment:47 Changed 5 years ago by haron

  • Cc alex.ryabov@… removed

comment:48 Changed 5 years ago by mmara

  • Cc mariano.mara@… added

comment:49 Changed 5 years ago by CollinAnderson

  • Cc cmawebsite@… added

Sorry for the noise.

comment:50 Changed 5 years ago by uptimebox

  • Cc uptimebox@… added

comment:51 Changed 5 years ago by anonymous

  • Cc andy@… added

comment:52 Changed 5 years ago by anonymous

Is anyone working on this?

comment:53 Changed 5 years ago by russellm

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 Changed 5 years ago by Juan

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 Changed 5 years ago by edevil

  • Cc andre.cruz@… added

Changed 5 years ago by russellm

First attempt at a complete namespace lookup fix for redeployed applications

comment:56 Changed 5 years ago by russellm

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 Changed 5 years ago by anonymous

  • Cc dtamborelli@… added

comment:58 follow-up: Changed 5 years ago by Rob Hudson <treborhudson@…>

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?

Changed 5 years ago by Rob Hudson <treborhudson@…>

patch on t10061-r11201

comment:59 follow-ups: Changed 5 years ago by Alex

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.

comment:60 in reply to: ↑ 59 Changed 5 years ago by russellm

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.

comment:61 in reply to: ↑ 58 Changed 5 years ago by russellm

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.

comment:62 in reply to: ↑ 59 ; follow-up: Changed 5 years ago by 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. 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.

Changed 5 years ago by russellm

Updated patch including suggestions from Alex and Rob

comment:63 Changed 5 years ago by russellm

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.

comment:64 in reply to: ↑ 62 Changed 5 years ago by russellm

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.

Changed 5 years ago by russellm

Third pass - now passing full test suite.

comment:65 Changed 5 years ago by anonymous

  • Cc beathan@… added

comment:66 follow-ups: Changed 5 years ago by dc

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

type(view) == tuple

with

isinstance(view, tuple)

Not critical but more pythonic.

comment:67 in reply to: ↑ 66 Changed 5 years ago by anonymous

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 follow-up: Changed 5 years ago by 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, 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).

comment:69 in reply to: ↑ 66 Changed 5 years ago by russellm

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).

comment:70 in reply to: ↑ 68 Changed 5 years ago by russellm

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 Changed 5 years ago by russellm

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

comment:72 follow-up: Changed 5 years ago by 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.

comment:73 Changed 5 years ago by Alex

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

comment:74 in reply to: ↑ 72 Changed 5 years ago by russellm

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.

Changed 5 years ago by russellm

Cleanup of some naming issues and other minor tweaks.

comment:75 Changed 5 years ago by russellm

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

(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 follow-up: Changed 5 years ago by mjs7231

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

comment:77 Changed 5 years ago by edevil

  • Cc andre.cruz@… removed

comment:78 in reply to: ↑ 76 Changed 5 years ago by kmtracey

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 Changed 4 years ago by akaihola

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 Changed 3 years ago by jacob

  • milestone 1.1 deleted

Milestone 1.1 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.