Code

Opened 3 years ago

Closed 3 years ago

Last modified 22 months ago

#15294 closed New feature (fixed)

Use named urls instead of hard coded ones in admin views

Reported by: julien Owned by: ramiro
Component: contrib.admin Version: master
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 3 years ago.
First iteration of a patch for this issue, so it can be reviewed
15294.2.diff (65.7 KB) - added by ramiro 3 years ago.
Removed nested 'crumbs' template block
15294.3.diff (65.8 KB) - added by ramiro 3 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 3 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 3 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 apollo13 3 years ago.
I really should learn git, let's see how that one works out
15294.14.diff (51.1 KB) - added by apollo13 3 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 apollo13 3 years ago.
fixes ramiros comment bugs, just to illustrate the issue -- nicer solution anyone?
15294.15.diff (130.0 KB) - added by apollo13 3 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 apollo13 3 years ago.
new patch against current trunk
15294.16.diff (131.1 KB) - added by apollo13 3 years ago.
renamed admin_url filter to admin_urlname and added docs
admin_tests_urls_fixes.diff (77.6 KB) - added by ramiro 3 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 3 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 Changed 3 years ago by russellm

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset
  • Triage Stage changed from Unreviewed to Accepted

comment:2 Changed 3 years ago by ramiro

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

  • Severity set to Normal
  • Type set to New feature

comment:4 Changed 3 years ago by julien

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

  • Owner changed from nobody to ramiro

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

Last edited 3 years ago by ramiro (previous) (diff)

comment:6 Changed 3 years ago by julien

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.

Last edited 3 years ago by julien (previous) (diff)

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

comment:8 in reply to: ↑ 7 ; follow-up: Changed 3 years ago by burzak

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

  • Cc dario.ocles@… added

comment:10 in reply to: ↑ 8 ; follow-up: Changed 3 years ago by 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?

comment:11 in reply to: ↑ 10 ; follow-up: Changed 3 years ago by burzak

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

comment:12 in reply to: ↑ 11 Changed 3 years ago by julien

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 follow-up: Changed 3 years ago by 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 not use hard-coded ones.

Last edited 3 years ago by ramiro (previous) (diff)

comment:14 in reply to: ↑ 13 Changed 3 years ago by julien

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

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

  • 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 3 years ago by ramiro (previous) (diff)

comment:17 Changed 3 years ago by apollo13

  • Version changed from 1.2 to SVN

comment:18 follow-up: Changed 3 years ago by anonymous

Another question; can we finally get rid of root_path?

comment:19 in reply to: ↑ 18 Changed 3 years ago by burzak

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 3 years ago by burzak (previous) (diff)

comment:20 Changed 3 years ago by julien

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

Changed 3 years ago by ramiro

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

comment:21 Changed 3 years ago by ramiro

  • Has patch set

comment:22 Changed 3 years ago by jezdez

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

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

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

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

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

Changed 3 years ago by ramiro

Removed nested 'crumbs' template block

Changed 3 years ago by ramiro

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

Changed 3 years ago by ramiro

comment:27 Changed 3 years ago by jezdez

  • 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 3 years ago by jezdez (previous) (diff)

comment:28 Changed 3 years ago by ramiro

In [16575]:

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

comment:29 Changed 3 years ago by ramiro

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.

Changed 3 years ago by ramiro

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

comment:30 Changed 3 years ago by apollo13

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

comment:31 Changed 3 years ago by jezdez

  • Needs documentation set
  • Patch needs improvement unset

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

Changed 3 years ago by apollo13

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

Changed 3 years ago by apollo13

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

Changed 3 years ago by apollo13

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

comment:32 Changed 3 years ago by julien

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!

Changed 3 years ago by apollo13

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

comment:33 Changed 3 years ago by julien

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

Changed 3 years ago by apollo13

new patch against current trunk

comment:34 Changed 3 years ago by julien

The failures are gone with this new patch, thanks!

comment:35 Changed 3 years ago by julien

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

admin_urlname sounds good +1

Changed 3 years ago by apollo13

renamed admin_url filter to admin_urlname and added docs

comment:37 Changed 3 years ago by apollo13

  • Needs documentation unset
  • Patch needs improvement unset

comment:38 Changed 3 years ago by jezdez

  • Triage Stage changed from Accepted to Ready for checkin

Changed 3 years ago by ramiro

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

Changed 3 years ago by ramiro

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

comment:39 Changed 3 years ago by ramiro

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

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

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 follow-ups: Changed 3 years ago by 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?

Last edited 3 years ago by ramiro (previous) (diff)

comment:42 in reply to: ↑ 41 Changed 3 years ago by julien

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!

comment:43 in reply to: ↑ 41 ; follow-up: Changed 3 years ago by ramiro

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.

comment:44 in reply to: ↑ 43 Changed 3 years ago by ramiro

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

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 Changed 23 months ago by Ramiro Morales <cramm0@…>

In [5a9e127efc37490b2a1caa605e987a693245b5fa]:

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

comment:47 Changed 22 months ago by 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.

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.