Needing explanation about "reverse admin" checkin

[Malcolm Tredinnick]

I realise the confusion I'm about to cause here is predominantly my
fault for not working faster, but I thought I'd have a bit more time
than I did. So a massive mea culpa there.

Anyway, I have some questions and concerns about r9739 that Jacob
committed overnight (my time) that maybe can be cleared up with some
explanation or maybe are the flaws I suspect they might be.

The first problem is that I can't work out the answers to these
questions by reading the documentation and test changes that went in
with the patch, since there aren't any! That makes me sad. That was
actually predominantly why I thought the patch wasn't about to be
committed very quickly, since they're both pretty important components
of anything we're calling a feature addition.

More importantly, though, I'm not really sure what problem this is
trying to solve. At least, it looks like it's only addressing a very
restricted case. This is something that has been bugging me a bit
lately: proposed solutions to things that don't actually explain the
problem they're solving, so it's hard to evaluate if they solve the
intended problem or miss or if we're talking at cross-purposes about the
problem that needs to be solved.

I've been sketching out a solution on my own (and I agree, my fault for
not sharing) and it seems to come down to three main use-cases (there's
a fourth that I'm thinking is out of scope). Bear in mind, as you're
reading this, that this is intended to be not just admin specific, but
rather for any app that can be called as multiple instances.

(1) A template needs to refer to the admin instance that is
rendering that template (for templates inside the app itself).
They won't know the name under which the particular rendering
admin is called in the URLConf, since that is installer stuff.
So we need a generic, app-specific name that means "self".

(2) Some other template needs to be able to call a particular
admin instance. This can use the name (however that is specified
in any solution) that the admin is installed under in URLConf.
Mostly AppA will require that an admin (and, again, "admin" is a
proxy word for "multiply-instanced application") exists under
nameB. For example, an "edit this" link on a page that points to
the editor-specific version of the admin, as opposed to say the
content-publisher-specific version.

(3) A template needs to call *any* instance (call it the
"default" instance) of the admin. A reasonable example here are
bookmarklets. Again, this can use the default name of the admin
application to determine.

The use-case I kind of mentally decided was out of scope was some kind
of name overloading so that appA's templates say they want to call an
admin of nameB and somehow the person installing that app overloads an
existing admin version with nameB. That maybe could come later, but
initially, it would be a case of "just put in an entry with a name of
nameB".

As best I can work out, the current patch kind of does situation (1),
but only for a single admin? Every template will always call the primary
admin instance when you use the url tag? Actually, it looks like it's
all about reverse() and not much consideration given to the url tag,
but, again, I don't really understand how to use it at the moment.

For the record, the way I was thinking we could make all these
situations work is as follows (and I'll phrase it using templates and
the url tag, since the reverse() call is easy once that case is
handled). We can write namespace-aware (or rendering-context aware) urls
using a prefix to indicate that a portion has to be resolved using some
variable in the context. Specifically,

{% url admin:edit-bookmarklet %}

for example. The context will contain a variable called, say,
_resolvers, that is a dictionary containing key names mapped to
instances. So the _resolvers["admin"] key will either be missing (in
which case we use the object from the URLConf with the name of "admin"
-- use-case #2), or it maps to a particular admin site object that has a
"reverse" callable that can be used to do the specific reversal
(use-case #1). Use-case #3 is handled by having a default name for the
class that doesn't need to be expressed explicitly in the URLConf and is
the name anybody can use to mean "any admin, just point me at one of
them" or possibly "the first admin specified", although the latter
situation is harder to implement, I've found.

That all seems to extend quite nicely to other classes beyond admin (and
if we include a "resolve" callable on classes to specify what should be
used to resolve things, we probably don't need to overload include(),
but that's a relatively minor thing that can wait for the future.

Right now, the short thing I'd like to find out is what problem the
current patch is meant to be solving. Does it extend to url tags? If so,
how can it work for something like use-case #2, in particular (if I have
two-instances installed, how can I say "resolve with respect to the
current one")? Maybe the url tag changes still work on top of this --
the code I'd been trying to pull together used portions of the patch
from #6470 to get most of the admin-specific stuff done -- but I'm
really a bit confused.

I am *not* trying to denigrate the work in #6470 and Alex and Jacobs'
efforts to get something to completion (but .. no documentation?! no
tests?!). I'm completely to blame for not bringing this up earlier. But
now that it's landed, we need to fix any major problems fast or live
with it forever.

Regards,
Malcolm

[Alex Gaynor]

What this ticket allows is:
a) handling the admin site/model admin urls through the normal resolver method
b) reversing these URLs

The main problem you describe is that the multiple admin sites that any url tags we make use of in the admin will refer to a specific admin site instance's name.  The work around to this is to create another {% adminurl %} tag that takes the admin site(we can easily add this to all templates rendered from the admin and have it use it's name to just call the regular reverse tag, so the call would look like {% adminurl admin_site "login_page" %} (or whatever URL, the args would be the same as the regular URL tag).  Regular users can refer to a specific admin_site instance by using {% url %} with the admin site's name('' for the default).

I'd like to note, however, that {% url %} currently isn't used anywhere in the admin since it won't work for people still using root(), and that would be backwards incompatible.

Lastly, there is documentation: http://docs.djangoproject.com/en/dev/ref/contrib/admin/#get-urls-self .  It doesn't note, however that this is a new for 1.1 feature, which it should.

Hopefully this clears up some of the issues,
Alex

--
"I disapprove of what you say, but I will defend to the death your right to say it." --Voltaire
"The people's good is the highest law."--Cicero

[Malcolm Tredinnick]

On Wed, 2009-01-14 at 22:52 -0500, Alex Gaynor wrote:
[...]

> What this ticket allows is:
> a) handling the admin site/model admin urls through the normal
> resolver method
> b) reversing these URLs
>
> The main problem you describe is that the multiple admin sites that
> any url tags we make use of in the admin will refer to a specific
> admin site instance's name. The work around to this is to create
> another {% adminurl %} tag that takes the admin site(we can easily add
> this to all templates rendered from the admin and have it use it's
> name to just call the regular reverse tag, so the call would look like
> {% adminurl admin_site "login_page" %} (or whatever URL, the args
> would be the same as the regular URL tag). Regular users can refer to
> a specific admin_site instance by using {% url %} with the admin
> site's name('' for the default).

Hmm. The new tag idea feels a bit clunky, although it's roughly
equivalent to the namespaced idea. Don't think of it as adminurl,
though, since it's something that should work for every type of
multiply-instanced application). It would be a shame if we have to have
to use one sort of tag for referring to almost everything (the url tag)
and another sort for references to "self", though.

> I'd like to note, however, that {% url %} currently isn't used
> anywhere in the admin since it won't work for people still using
> root(), and that would be backwards incompatible.

That was one of the advantages of the namespaced scheme and a "reverse"
attribute: we'd set reverse=root on the admin class and things would
fallback nicely if anybody had overridden root. I'll have a look if
that's still possible.

> Lastly, there is documentation:
> http://docs.djangoproject.com/en/dev/ref/contrib/admin/#get-urls-self . It doesn't note, however that this is a new for 1.1 feature, which it should.

Somehow, despite checking about four times, I still missed that portion.
My apologies.

I was predominantly looking for something that explained how to use this
and that isn't present (I actually grepped for the string "admin/" to
find the example usage and it didn't show up). As usual, though, we can
incrementally improve that.

Thanks for the explanation. You've confirmed/explained a couple of
things.

Regards,
Malcolm

[Jacob Kaplan-Moss]

Hey Malcolm --

Sorry if I missed some steps on this one; I've been working on it
pretty hard for the past few days and thought it was done (though I've
already noticed an oversight with the password change link... grr...).

I should point out that there is docs
(http://docs.djangoproject.com/en/dev/ref/contrib/admin/#get-urls-self),
and also tests (``regressiontests.admin_views`` covers ``get_urls()``
and registration of multiple admins). I did forget to clean up
whitespace when I merged the patch, so that makes the diff really hard
to read; sorry!

Reversing does work fine with multiple admins, BTW, as long as you
"name" the AdminSite; views get registered prefixed with that name.
So, given::

class MyAdmin(admin.AdminSite):
...

myadmin = MyAdmin(name="myadmin")

You can then do things like ``{% url myadmin_admin_logout %}``. I
hadn't really thought of doing any namespacing, though we could easily
make that change, I think. I don't feel strongly either way. Of
course, if you can figure out a way to modify the admin templates to
use ``{% url %}`` and still work with the old-style ``root``-ed views,
that'd be awesome; I tried and failed.

You're right that this part isn't documented: I'm working on a more
comprehensive rewrite of the admin docs, and didn't want to bother
writing the same content twice. I'll be landing that tomorrow, so you
won't have to wait long.

So -- are there major problems? If so, again I'm very sorry, but I do
think this solves the immediate problem and makes the admin use a URL
resolver correctly; any changes from herein ought to be a lot easier.

Jacob

[Malcolm Tredinnick]

I suspect there probably aren't major problems now that I've read it a
few more times and combined it with Alex's comments. It feels a bit
hard-coded at the admin level with the prefix appending (thus,
admin-specific and I was working towards something that would work more
generally on multiple levels), but it sounds like it fixes the admin
problem. I keep picturing what the equivalent stuff would look like for
other applications.

Hey, you guys got it done. So I can't complain and should thank you
(which I do). That is good news.

I'll look at the url tag and root stuff. I hate deprecating things if we
can avoid it. And I'll bring up more general reversing/resolving for
1.2.

Malcolm

[Alex Gaynor]

Actually all we need to do is add a prefix argument to the url tag, and then just pass admin_site.name to prefix, all this kwarg has to do is add the string of the view name and the prefix together and tada-namespaces.  I think this is a good, comprehensive solution.

[Malcolm Tredinnick]

On Thu, 2009-01-15 at 00:49 -0500, Alex Gaynor wrote:
[...]

> Actually all we need to do is add a prefix argument to the url tag,
> and then just pass admin_site.name to prefix, all this kwarg has to do
> is add the string of the view name and the prefix together and
> tada-namespaces. I think this is a good, comprehensive solution.

If you mean adding a keyword argument to the template tag that is
specified by the person using it in the template, that will get ugly
rapidly, since it introduces both the very likely possibility of
collisions with existing url patterns keyword arguments (at least one
app that a client of mine has deployed in production will break, for
example, since we use "prefix" as the name of a parameter) and mixing
keyword and positional arguments (for any url tag usage that is using
positional arguments at the moment).

Malcolm

[Alex Gaynor]

Ok, then perhaps using the namespace syntax you suggested would work nicely

{% url admin_site:"admin_logout" %}

most importantly it seems that we have satisfied most of the original concerns that you had about htis implementation.

[Malcolm Tredinnick]

On Thu, 2009-01-15 at 01:25 -0500, Alex Gaynor wrote:
[...]

> Ok, then perhaps using the namespace syntax you suggested would work
> nicely
>
> {% url admin_site:"admin_logout" %}

Ideally, the url tag would operate on the names of things, rather than
jamming objects and strings together like this, but that's the idea,
yes.

I had a quick look over lunch and I think I can merge the stuff I've
been whacking on into the existing implementation and then extend it a
bit more in the Django 1.2 development period to fit into a more general
pattern or resolve/reverse handling on objects.

> most importantly it seems that we have satisfied most of the original
> concerns that you had about htis implementation.

You've satisfied some of the concerns about the API, which is more
important, since that's the bit we're stuck with forever. I'm still
unhappy that it intended to deprecate a documented usage, which means
that in Djagno 1.3 code that worked in Django 1.0 will cease to work. As
mentioned in a reply to Jacob, I want to try and fix that.

Malcolm

[Alex Gaynor]

Jacob and I discussed this earlier, and what I concluded was that we can actually make root transparently create a RegexURLResolver and use self.urls therefore alleviating the need to maintain parallel implementations, however the issue is anyone who has overridden root or __call__ to add extra urls would then be SOL.

[Alex Gaynor]

In order to fix #10061[1] I've needed to implement the url tag scoping that has been discussed.  As implemented in the ticket it just does splits the viewname on ":" and then resolves the first portion of it and combines the 2 strings.  That definitely needs further review however it does solve the issue under the 4 setups I tested this under:
/admin/ -- new urls
/admin5/ -- new urls
/special/admin/ -- new urls
/other/admin/ -- old urls

I'm going to write some tests for it now.

Alex

[1]: http://code.djangoproject.com/ticket/10061