Opened 14 years ago

Closed 13 years ago

Last modified 12 years ago

#15294 closed New feature (fixed)

Use named urls instead of hard coded ones in admin views

Reported by: Julien Phalip Owned by: Ramiro Morales
Component: contrib.admin Version: dev
Severity: Normal Keywords:
Cc: dario.ocles@…, florian+django@… Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

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

Attachments (13)

15294.1.diff (64.5 KB ) - added by Ramiro Morales 14 years ago.
First iteration of a patch for this issue, so it can be reviewed
15294.2.diff (65.7 KB ) - added by Ramiro Morales 14 years ago.
Removed nested 'crumbs' template block
15294.3.diff (65.8 KB ) - added by Ramiro Morales 14 years ago.
Added one char to a init.py file in the git .diff so patch actually creates it
15294.4.diff (65.1 KB ) - added by Ramiro Morales 13 years ago.
Updated patch, also available at https://github.com/ramiro/django/compare/master...bug%2Ft15294
15294.10.diff (49.4 KB ) - added by Ramiro Morales 13 years ago.
Patch updated post [16575]/[16578], still no decision a the replacemente for the ugly model_url ttag.
15294.13.diff (50.5 KB ) - added by Florian Apolloner 13 years ago.
I really should learn git, let's see how that one works out
15294.14.diff (51.1 KB ) - added by Florian Apolloner 13 years ago.
New patch, has one broken testcase, but that test is utterly broken during it's missuse of admin view classes
15294-fix-comments-reverse.diff (1.0 KB ) - added by Florian Apolloner 13 years ago.
fixes ramiros comment bugs, just to illustrate the issue -- nicer solution anyone?
15294.15.diff (130.0 KB ) - added by Florian Apolloner 13 years ago.
all tests working, hopefully even for ramiro -- https://github.com/apollo13/django/compare/master...t15294
t15294.16.diff (130.1 KB ) - added by Florian Apolloner 13 years ago.
new patch against current trunk
15294.16.diff (131.1 KB ) - added by Florian Apolloner 13 years ago.
renamed admin_url filter to admin_urlname and added docs
admin_tests_urls_fixes.diff (77.6 KB ) - added by Ramiro Morales 13 years ago.
URL setup clueanup in admin-related tests by Florian Apolloner, isolated from the v16 patch for easier reviewing
15294.17.diff (48.6 KB ) - added by Ramiro Morales 13 years ago.
Patch RFC specific for this issue, apply after admin_tests_urls_fixes.diff

Download all attachments as: .zip

Change History (60)

comment:1 by Russell Keith-Magee, 14 years ago

Triage Stage: UnreviewedAccepted

comment:2 by Ramiro Morales, 14 years ago

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

comment:3 by Łukasz Rekucki, 14 years ago

Severity: Normal
Type: New feature

comment:4 by Julien Phalip, 14 years ago

Easy pickings: unset

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.

comment:5 by Ramiro Morales, 14 years ago

Owner: changed from nobody to Ramiro Morales

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

Last edited 14 years ago by Ramiro Morales (previous) (diff)

comment:6 by Julien Phalip, 14 years ago

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_changelist' %
                                   (opts.app_label, opts.module_name),
                                   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.

Version 0, edited 14 years ago by Julien Phalip (next)

comment:7 by Julien Phalip, 14 years ago

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

in reply to:  7 ; comment:8 by Dario Ocles, 14 years ago

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

comment:9 by Dario Ocles, 14 years ago

Cc: dario.ocles@… added

in reply to:  8 ; comment:10 by Julien Phalip, 14 years ago

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?

in reply to:  10 ; comment:11 by Dario Ocles, 14 years ago

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

in reply to:  11 comment:12 by Julien Phalip, 14 years ago

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.

comment:13 by Ramiro Morales, 14 years ago

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.

Last edited 14 years ago by Ramiro Morales (previous) (diff)

in reply to:  13 comment:14 by Julien Phalip, 14 years ago

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.

comment:15 by Julien Phalip, 14 years ago

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.

comment:16 by Florian Apolloner, 14 years ago

Cc: florian+django@… added

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…

Last edited 14 years ago by Ramiro Morales (previous) (diff)

comment:17 by Florian Apolloner, 14 years ago

Version: 1.2SVN

comment:18 by anonymous, 14 years ago

Another question; can we finally get rid of root_path?

in reply to:  18 comment:19 by Dario Ocles, 14 years ago

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

Last edited 14 years ago by Dario Ocles (previous) (diff)

comment:20 by Julien Phalip, 14 years ago

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

by Ramiro Morales, 14 years ago

Attachment: 15294.1.diff added

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

comment:21 by Ramiro Morales, 14 years ago

Has patch: set

comment:22 by Jannis Leidel, 14 years ago

Patch needs improvement: set

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

comment:23 by Florian Apolloner, 14 years ago

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…

comment:24 by Dario Ocles, 14 years ago

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.

comment:25 by Florian Apolloner, 14 years ago

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?

comment:26 by Jannis Leidel, 14 years ago

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

by Ramiro Morales, 14 years ago

Attachment: 15294.2.diff added

Removed nested 'crumbs' template block

by Ramiro Morales, 14 years ago

Attachment: 15294.3.diff added

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

by Ramiro Morales, 13 years ago

Attachment: 15294.4.diff added

comment:27 by Jannis Leidel, 13 years ago

UI/UX: unset

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.

Last edited 13 years ago by Jannis Leidel (previous) (diff)

comment:28 by Ramiro Morales, 13 years ago

In [16575]:

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

comment:29 by Ramiro Morales, 13 years ago

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.

by Ramiro Morales, 13 years ago

Attachment: 15294.10.diff added

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

comment:30 by Florian Apolloner, 13 years ago

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

comment:31 by Jannis Leidel, 13 years ago

Needs documentation: set
Patch needs improvement: unset

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

by Florian Apolloner, 13 years ago

Attachment: 15294.13.diff added

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

by Florian Apolloner, 13 years ago

Attachment: 15294.14.diff added

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

by Florian Apolloner, 13 years ago

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

comment:32 by Julien Phalip, 13 years ago

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!

by Florian Apolloner, 13 years ago

Attachment: 15294.15.diff added

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

comment:33 by Julien Phalip, 13 years ago

Patch needs improvement: set

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! :-)

by Florian Apolloner, 13 years ago

Attachment: t15294.16.diff added

new patch against current trunk

comment:34 by Julien Phalip, 13 years ago

The failures are gone with this new patch, thanks!

comment:35 by Julien Phalip, 13 years ago

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.

comment:36 by Jannis Leidel, 13 years ago

admin_urlname sounds good +1

by Florian Apolloner, 13 years ago

Attachment: 15294.16.diff added

renamed admin_url filter to admin_urlname and added docs

comment:37 by Florian Apolloner, 13 years ago

Needs documentation: unset
Patch needs improvement: unset

comment:38 by Jannis Leidel, 13 years ago

Triage Stage: AcceptedReady for checkin

by Ramiro Morales, 13 years ago

Attachment: admin_tests_urls_fixes.diff added

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

by Ramiro Morales, 13 years ago

Attachment: 15294.17.diff added

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

comment:39 by Ramiro Morales, 13 years ago

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

comment:40 by Ramiro Morales, 13 years ago

Resolution: fixed
Status: newclosed

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.

comment:41 by kingtut, 13 years ago

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?

Last edited 13 years ago by Ramiro Morales (previous) (diff)

in reply to:  41 comment:42 by Julien Phalip, 13 years ago

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!

in reply to:  41 ; comment:43 by Ramiro Morales, 13 years ago

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.

in reply to:  43 comment:44 by Ramiro Morales, 13 years ago

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.

comment:45 by Ramiro Morales, 13 years ago

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.

comment:46 by Ramiro Morales <cramm0@…>, 12 years ago

In [5a9e127efc37490b2a1caa605e987a693245b5fa]:

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

comment:47 by Ramiro Morales <cramm0@…>, 12 years ago

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.

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