#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)
Change History (60)
comment:1 by , 14 years ago
Triage Stage: | Unreviewed → Accepted |
---|
comment:2 by , 14 years ago
comment:3 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:4 by , 14 years ago
Easy pickings: | unset |
---|
comment:5 by , 14 years ago
Owner: | changed from | to
---|
Dario Ocles and I are working on this, see https://github.com/ramiro/django/compare/master...bug%2Ft15294
comment:6 by , 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_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.
follow-up: 8 comment:7 by , 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) ...
follow-up: 10 comment:8 by , 14 years ago
Replying to julien:
Or probably a better approach is to default
post_url_continue
toNone
:
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 , 14 years ago
Cc: | added |
---|
follow-up: 11 comment:10 by , 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 thatpost_url_continue
is eitherNone
or a pre-formatted string. - we first try to insert
pk_value
intopost_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?
follow-up: 12 comment:11 by , 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 thatpost_url_continue
is eitherNone
or a pre-formatted string.- we first try to insert
pk_value
intopost_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 by , 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.
follow-up: 14 comment:13 by , 14 years ago
comment:14 by , 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 , 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 , 14 years ago
Cc: | 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…
comment:17 by , 14 years ago
Version: | 1.2 → SVN |
---|
comment:19 by , 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
comment:20 by , 13 years ago
Just linking to #5625, which would get fixed by the patch in this ticket.
by , 13 years ago
Attachment: | 15294.1.diff added |
---|
First iteration of a patch for this issue, so it can be reviewed
comment:21 by , 13 years ago
Has patch: | set |
---|
comment:22 by , 13 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 , 13 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 , 13 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 , 13 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 , 13 years ago
Understood, but this is definitely a separate feature, so please open a new ticket and remove it from this patch.
by , 13 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 , 13 years ago
Attachment: | 15294.4.diff added |
---|
Updated patch, also available at https://github.com/ramiro/django/compare/master...bug%2Ft15294
comment:27 by , 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.
by , 13 years ago
Attachment: | 15294.10.diff added |
---|
comment:30 by , 13 years ago
Attached a new diff with the new proposed syntax, needs docs for the new filter.
comment:31 by , 13 years ago
Needs documentation: | set |
---|---|
Patch needs improvement: | unset |
This looks great, yay for not needing a model_url
tag!
by , 13 years ago
Attachment: | 15294.13.diff added |
---|
I really should learn git, let's see how that one works out
by , 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 , 13 years ago
Attachment: | 15294-fix-comments-reverse.diff added |
---|
fixes ramiros comment bugs, just to illustrate the issue -- nicer solution anyone?
comment:32 by , 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 , 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 , 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! :-)
comment:35 by , 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.
by , 13 years ago
Attachment: | 15294.16.diff added |
---|
renamed admin_url filter to admin_urlname and added docs
comment:37 by , 13 years ago
Needs documentation: | unset |
---|---|
Patch needs improvement: | unset |
comment:38 by , 13 years ago
Triage Stage: | Accepted → Ready for checkin |
---|
by , 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 , 13 years ago
Attachment: | 15294.17.diff added |
---|
Patch RFC specific for this issue, apply after admin_tests_urls_fixes.diff
follow-ups: 42 43 comment:41 by , 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?
comment:42 by , 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!
follow-up: 44 comment:43 by , 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.
comment:44 by , 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.
#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.)