Use named urls instead of hard coded ones in admin views

[Julien Phalip]

There are still a lot of hard coded urls in the admin views, generally used for redirection. To allow for more flexibility and robustness, those should be replaced by named urls. Where it might get tricky is with the urls that are provided in the view signatures (for example post_url_continue in response_add()).

[Ramiro Morales]

#11163 reported this problem for a specific case: ForeignKeyRawIdWidget, I closed it as duplicate of this one. Will try to update and upload some code Florian Apolloner and me had attached to #13588 (changes that were correctly marked as out of scope for that ticket, and that dealt precisely with what this ticker reports.)

[Julien Phalip]

See #8001 for a related feature request. Also, I think that for the urls present in the method signatures we'll have to wait till we get a lazy url resolver as proposed in #5925.

[Ramiro Morales]

Dario Ocles and I are working on this, see ​https://github.com/ramiro/django/compare/master...bug%2Ft15294

[Julien Phalip]

To clarify my comment above about #5925: it was specifically about the post_url_continue parameter in the response_add method's signature:

def response_add(self, request, obj, post_url_continue='../%s/'):
    ...

Ideally post_url_continue should take the reversed URL for the change view instead of the hardcoded '../%s/'. Initially I thought that the reverse_lazy() introduced by #5925 may help, but the problem is that there's no way of finding out the new object's pk or even it's app label or model name from the method's signature. Maybe an approach would be to allow passing a callable as post_url_continue, for example:

def post_url_continue(request, modeladmin, obj):
    opts = obj._meta
    return (reverse('admin:%s_%s_change' %
                            (opts.app_label, opts.module_name),
                            args=obj.pk,
                            current_app=modeladmin.admin_site.name))

class ModelAdmin(BaseModelAdmin):

    ...

    def response_add(self, request, obj, post_url_continue=post_url_continue):
        ...
        if callable(post_url_continue):
            post_url_continue = post_url_continue(request, self, obj)
        ...

Such an approach could be generalised for the other redirection urls suggested in #8001.

[Julien Phalip]

Or probably a better approach is to default post_url_continue to None:

def response_add(self, request, obj, post_url_continue=None):
    ...
    if post_url_continue is None:
        post_url_continue = reverse('admin:%s_%s_change' %
                                        (opts.app_label, module_name),
                                        args=pk_value,
                                        current_app=self.admin_site.name)
    ...

[Dario Ocles]

Replying to julien:

Or probably a better approach is to default post_url_continue to None:

def response_add(self, request, obj, post_url_continue=None):
    ...
    if post_url_continue is None:
        post_url_continue = reverse('admin:%s_%s_change' %
                                        (opts.app_label, module_name),
                                        args=pk_value,
                                        current_app=self.admin_site.name)
    ...

I did something like that here [0]. I changed the default value to None and I made an 'if' like this in order to maintain backward compatibility:

    if post_url_continue is not None:
        post_url_continue = post_url_continue % pk_value
    else:
        post_url_continue = reverse("admin:%s_%s_change" % (opts.app_label, opts.module_name), args=(pk_value,), current_app=self.admin_site.name)

​https://github.com/burzak/django/commit/cd275d5918aea8ee6d8dc0cdb2c93f2409c87637

[Julien Phalip]

Replying to burzak:

I did something like that here [0]. I changed the default value to None and I made an 'if' like this in order to maintain backward compatibility:

    if post_url_continue is not None:
        post_url_continue = post_url_continue % pk_value
    else:
        post_url_continue = reverse("admin:%s_%s_change" % (opts.app_label, opts.module_name), args=(pk_value,), current_app=self.admin_site.name)

Cool, that makes sense. However, this could be a bit limiting as you couldn't pass a pre-formatted string -- it would necessarily require a string format to insert pk_value.

I can think of 2 options, either/or:

• we consider that response_add is not part of the official API (it's not documented) and therefore we don't worry about breaking backwards compatibility. We then just assume that post_url_continue is either None or a pre-formatted string.
• we first try to insert pk_value into post_url_continue, but if an exception is raised then we assumed it's a pre-formatted string and move on with it.

Hope that makes sense. What do you think?

[Dario Ocles]

Replying to julien:

Replying to burzak:

I did something like that here [0]. I changed the default value to None and I made an 'if' like this in order to maintain backward compatibility:

    if post_url_continue is not None:
        post_url_continue = post_url_continue % pk_value
    else:
        post_url_continue = reverse("admin:%s_%s_change" % (opts.app_label, opts.module_name), args=(pk_value,), current_app=self.admin_site.name)

Cool, that makes sense. However, this could be a bit limiting as you couldn't pass a pre-formatted string -- it would necessarily require a string format to insert pk_value.

I can think of 2 options, either/or:

• we consider that response_add is not part of the official API (it's not documented) and therefore we don't worry about breaking backwards compatibility. We then just assume that post_url_continue is either None or a pre-formatted string.
• we first try to insert pk_value into post_url_continue, but if an exception is raised then we assumed it's a pre-formatted string and move on with it.

Hope that makes sense. What do you think?

I tend to think that if somebody pass a post_url_continue this have to be pre-formatted since the user must know where to go.

I like the idea of keeping the code clean and do not add any try/catch. If the backward compatibility is not so much important in this situation I would be happy removing the pk_value.

I like this...

    if post_url_continue is None:
        post_url_continue = reverse("admin:%s_%s_change" % (opts.app_label, opts.module_name), args=(pk_value,), current_app=self.admin_site.name)

rather than:

    if post_url_continue is None:
        post_url_continue = reverse("admin:%s_%s_change" % (opts.app_label, opts.module_name), args=(pk_value,), current_app=self.admin_site.name)
    else:
        try:
            post_url_continue = post_url_continue % pk_value
        except TypeError:
            pass

[Julien Phalip]

Replying to burzak:

I like this...

    if post_url_continue is None:
        post_url_continue = reverse("admin:%s_%s_change" % (opts.app_label, opts.module_name), args=(pk_value,), current_app=self.admin_site.name)

I like this better too. It all depends on whether we're allowing this or not (based on the backward compatibility policy). I think it's worth breaking compatibility in this case, as it would make the implementation a lot cleaner, and the response_add method is not documented yet anyway.

Maybe this is worth asking on the dev-list for other opinions.

[Ramiro Morales]

Am I wrong if I say that comment:4 and comments from comment:6 onwards belong in #8001 and not here? Ths ticket is only about making current handling on intra-admin site handling of URLs not use hard-coded ones.

[Julien Phalip]

Replying to ramiro:

Am I wrong if I say that comment:4 and comments from comment:6 onwards belong in #8001 and not here? Ths ticket is only about making current handling on intra-admin site handling of URLs.

I was wrong in linking this issue to #5925 -- I initially thought that reverse_lazy may help here, but in fact it can't.

However, I think that the other comments about post_url_continue are still relevant (this was even mentioned in the original report). If we can find a neat implementation for dealing with post_url_continue then that would be helpful for #8001 too. But, regardless of #8001, I think we should still do something to remove the hardcoded value of post_url_continue at the same time as the other things in the admin views and templates.

[Julien Phalip]

I've just discussed the post_url_continue issue with Ramiro on IRC and it was decided that it won't be tackled by the work in this ticket. It will be done as part of #8001 instead.

[Florian Apolloner]

Hey, I am just looking through the diff (will take a deeper look at it that weekend) and noticed some inconsistencies: ​https://github.com/ramiro/django/compare/master...bug%2Ft15294#L4R13 model_url uses "" as quotes while you use with url, imo both should be "".

Looking at: ​https://github.com/ramiro/django/compare/master...bug%2Ft15294#L5R36 Do we need the '{% if password_change_url %}' at all? The previous version always produced an url, so I guess we can just drop the if and put the url tag directly into the href attribute. Same for logout url…

[anonymous]

Another question; can we finally get rid of root_path?

[Dario Ocles]

Replying to anonymous:

Another question; can we finally get rid of root_path?

I did it here [0] on my github branch.

[0] ​https://github.com/burzak/django/commit/3d289bb31cf4e6debf4145d94358f67cdfa8d3c9

[Julien Phalip]

Just linking to #5625, which would get fixed by the patch in this ticket.

[Ramiro Morales]

First iteration of a patch for this issue, so it can be reviewed

[Jannis Leidel]

What's the purpose of the crumbs block? Isn't that a bit OT for this ticket and would make upgrading harder?

[Florian Apolloner]

I agree, wasn't in my inital patch either. Ramiro: any objections against removing that again? I there is a compelling reason we could do {% block breadcrumbs %}{% block crumbs %} in the base template I guess…

[Dario Ocles]

jedez, apollo13: The idea of the crumbs block is to avoid many url repetition. Inside of many template there are many urls defines on breadcrumbs block. When you extend these templates to add some link in breadcrumb block, you have to include the urls defines in the template you are extending of. With crumbs block you only add the link you want and that's it.

PS: Sorry for my English.

[Florian Apolloner]

Ah, you do mean cause breadcrumbs have the div inside it -- so essentially you have to duplicate this block everywhere. From a quick read the old breadcrumbs block is still there, so everyone should be able to use the new codepath without any changes to his templates right?

[Jannis Leidel]

Understood, but this is definitely a separate feature, so please open a new ticket and remove it from this patch.

[Ramiro Morales]

Removed nested 'crumbs' template block

[Ramiro Morales]

Added one char to a init.py file in the git .diff so patch actually creates it

[Ramiro Morales]

Updated patch, also available at ​https://github.com/ramiro/django/compare/master...bug%2Ft15294

[Jannis Leidel]

Hm, so I'm not a big fan of the model_url template tag, {% model_url 'admin:%s_%s_changelist' opts.app_label opts.module_name %} doesn't look very legible to me, in fact it looks like Pseudo Python. Why not put that in the actual Python code instead?

Other than that this looks pretty awesome.

[Ramiro Morales]

In [16575]:

Removed deprecated admin contrib app AdminSite root_path attribute. Refs #15294, r11250, r16136.

[Ramiro Morales]

In [16578]:

Fixed #16542 -- Made Raw ID form widgets shipped with the admin app render the related object lookup tool only when the related model is effectively registered with the AdminSite.

Also, converted these widgets to reverse named URLs instead of hard-coded '../../...'-style links, refs #15294.
Thanks Florian Apolloner for the initial patch.

[Ramiro Morales]

Patch updated post [16575]/[16578], still no decision a the replacemente for the ugly model_url ttag.

[Florian Apolloner]

Attached a new diff with the new proposed syntax, needs docs for the new filter.

[Jannis Leidel]

This looks great, yay for not needing a model_url tag!

[Florian Apolloner]

I really should learn git, let's see how that one works out

[Florian Apolloner]

New patch, has one broken testcase, but that test is utterly broken during it's missuse of admin view classes

[Florian Apolloner]

fixes ramiros comment bugs, just to illustrate the issue -- nicer solution anyone?

[Julien Phalip]

The "model_tag" replacement is very elegant, well done! I'm just a bit ambivalent with the name between "admin_url" (shorter and sexier) and "admin_url_name" (more explicit and accurate) -- although it's no big deal either way.

Unfortunately I still can't reproduce those test errors with any permutations of generic_inline_admin, admin_views and comment_tests. However, from what you've described in IRC it does feel like there could be some conflicts relating to the order in which the models are registered in the admin. Perhaps those errors could be avoided by using a context manager:

def test_blah(self):
    with override_admin((MyModel, MyModelAdmin), (MyAwesomeModel, MyAwesomeModelAdmin),):
        # Tests go here
        ...

On __enter__() the CM would back up potential modeladmins that are already registered, then unregister them, then register the ones provided as arguments. And on __exit__() the CM would re-register the backed up modeladmins.

Just a thought!

[Florian Apolloner]

all tests working, hopefully even for ramiro -- ​https://github.com/apollo13/django/compare/master...t15294

[Julien Phalip]

Great effort cleaning up the admin tests! It looks sane and much more in line with best practices. It seems to fix the errors that Ramiro had (and which I finally managed to see too) but I'm now getting 2 failures:

======================================================================
FAIL: testAddView (regressiontests.admin_views.tests.AdminViewPermissionsTest)
Test add view restricts access and actually adds items.
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/julien/Documents/Development/eclipse/workspace/DjangoCore/git/tests/regressiontests/admin_views/tests.py", line 863, in testAddView
    'Unrestricted user is not given link to change list view in breadcrumbs.')
AssertionError: True is not False : Unrestricted user is not given link to change list view in breadcrumbs.

======================================================================
FAIL: test_save_as_display (regressiontests.admin_views.tests.SaveAsTests)
----------------------------------------------------------------------
Traceback (most recent call last):
  File "/Users/julien/Documents/Development/eclipse/workspace/DjangoCore/git/tests/regressiontests/admin_views/tests.py", line 600, in test_save_as_display
    self.assertEqual(response.context['form_url'], '/test_admin/admin/admin_views/person/add/')
AssertionError: '../add/' != '/test_admin/admin/admin_views/person/add/'

Also, I've got a slight preference for admin_url_name instead of admin_url simply because that filter does not return a url. Another approach could be a new template tag: {% admin_url opts 'changelist' %} -- It would feel more natural to me, since it's an admin-specific thing. I'm not too precious about this though.

Apart from that, I think it's getting really close now! :-)

[Florian Apolloner]

new patch against current trunk

[Julien Phalip]

The failures are gone with this new patch, thanks!

[Julien Phalip]

OK, personally my final vote goes to keeping it as a filter and to naming it admin_urlname. I'm happy to write the doc once an executive decision is taken on the name.

[Jannis Leidel]

admin_urlname sounds good +1

[Florian Apolloner]

renamed admin_url filter to admin_urlname and added docs

[Ramiro Morales]

URL setup clueanup in admin-related tests by Florian Apolloner, isolated from the v16 patch for easier reviewing

[Ramiro Morales]

Patch RFC specific for this issue, apply after admin_tests_urls_fixes.diff

[Ramiro Morales]

In [16856]:

Improved test isolation of the admin tests and assigned custom admin sites to
prevent test order dependant failures.

This involves introducing usage of TestCase.urls and implementing proper
admin.py modules for some of the test apps.

Thanks Florian Apolloner for finding the issue and contributing the patch.

Refs #15294 (it solves these problems so the fix for that ticket we are going
to commit doesn't introduce obscure and hard to reproduce test failures when
running the Django test suite.)

[Ramiro Morales]

In [16857]:

Converted internal link generation in the admin and admin document generator to use named URLs.

Thanks to Florian Apolloner for both the initial patch and his final push to get
this fixed, to Dario Ocles for his great work on the admin templates and
switching the admin_doc application to also use named URLs, to Mikko Hellsing
for his comments and to Jannis and Julien for their review and design guidance.

Fixes #15294.

[kingtut]

This is breaking apps that do not have add permissions (NoReverseMatch), e.g. django-mailchimp campaigns are always added remotely.

 if True in perms.values(): 
     info = (app_label, model._meta.module_name) 
     model_dict = { 
         'name': capfirst(model._meta.verbose_name_plural), 
-        'admin_url': mark_safe('%s/%s/' % (app_label, model.__name__.lower())),
+        'admin_url': reverse('admin:%s_%s_changelist' % info, current_app=self.name), 
+        'add_url': reverse('admin:%s_%s_add' % info, current_app=self.name), 

If the add or change permission is false, why bother resolving the url. Perhaps only include the 'add_url' when the 'add' permission exists?

Or is is mandatory now that apps implement add, change and delete urls?

[Julien Phalip]

Replying to kingtut:

This is breaking apps that do not have add permissions (NoReverseMatch), e.g. django-mailchimp campaigns are always added remotely.

Could you open a new ticket with a failing test case for this? Thanks!

[Ramiro Morales]

Replying to kingtut:

This is breaking apps that do not have add permissions (NoReverseMatch), e.g. django-mailchimp campaigns are always added remotely.

 if True in perms.values(): 
     info = (app_label, model._meta.module_name) 
     model_dict = { 
         'name': capfirst(model._meta.verbose_name_plural), 
-        'admin_url': mark_safe('%s/%s/' % (app_label, model.__name__.lower())),
+        'admin_url': reverse('admin:%s_%s_changelist' % info, current_app=self.name), 
+        'add_url': reverse('admin:%s_%s_add' % info, current_app=self.name), 

If the add or change permission is false, why bother resolving the url. Perhaps only include the 'add_url' when the 'add' permission exists?

Or is is mandatory now that apps implement add, change and delete urls?

I've opened #17333 for this.

[Ramiro Morales]

Replying to ramiro:

Replying to kingtut:

This is breaking apps that do not have add permissions (NoReverseMatch), e.g. django-mailchimp campaigns are always added remotely.

I've opened #17333 for this.

.. and I've closed it. kingtut, If you are reading this please help us to help you by modifying the regression tes case attached there because I can't reproduce your report.

[Ramiro Morales]

In [17237]:

Stopped unconditionally reversing admin model add/change URLs.

Starting with [16857] this could cause HTTP 500 errors when
ModelAdmin.get_urls() has been customized to the point it doesn't
provide these standard URLs.

Fixes #17333. Refs #15294.

[Ramiro Morales <cramm0@…>]

In [5a9e127efc37490b2a1caa605e987a693245b5fa]:

Made model instance history admin view link not hard-coded. Refs #15294.

[Ramiro Morales <cramm0@…>]

In f51eab796d087439eedcb768cdd312517780940e:

Fixed #18072 -- Made more admin links use reverse() instead of hard-coded relative URLs.

Thanks kmike for the report and initial patch for the changelist->edit
object view link URL.

Other affected links include the delete object one and object history
one (in this case the change had been implemented in commit 5a9e127, this
commit adds admin-quoting of the object PK in a way similar to a222d6e.)

Refs #15294.