fix #257 and test various url patterns

[PetrDlouhy]

fix #257

[idlesign]

Sorry can't accept it there's probably a ton of symbols that can brake Admin, besides, it seems that a URL resolution makes no sense in Admin at all.
We probably need to use current_app_is_admin or something similar.

[coveralls]

Coverage decreased (-4.6%) to 95.378% when pulling d5e6562 on PetrDlouhy:fix/url_pattern_quote into 1c4dd79 on idlesign:master.

[coveralls]

Coverage decreased (-4.6%) to 95.382% when pulling 55da7de on PetrDlouhy:fix/url_pattern_quote into 1c4dd79 on idlesign:master.

[coveralls]

Coverage decreased (-4.6%) to 95.382% when pulling 55da7de on PetrDlouhy:fix/url_pattern_quote into 1c4dd79 on idlesign:master.

[PetrDlouhy]

@idlesign What do you mean by that? If other symbols would break the Admin (I will try that), we need to try different approach to fix this. But I don't think, that it is acceptable that any value would break the admin in such way, that the user can't fix his mistake it anymore.

I am probably missing the point of the current_app_is_admin note. Those tests in this PR will fail without the fix. The bug doesn't depend on any specifics of my app.

[PetrDlouhy]

@idlesign And no, any other symbol that I tried, didn't break the admin, I tried: $%^čř%*#$@!§¨``°". I could add that into the tests also.

[PetrDlouhy]

@idlesign And that brings me to the question. Why django-sitetree uses rendering of url templatetag instead of simple reverse here: https://github.com/idlesign/django-sitetree/blob/master/sitetree/sitetreeapp.py#L694?

That is the problem - ' sign will be interpreted as end of string there.

[idlesign]

I am probably missing the point of the current_app_is_admin note.

Ok, let's try it the other way round: why do you need to resolve tree items URLs in admin at all?

Why django-sitetree uses rendering of url templatetag [...]

Hard to come back for the exact reasoning from 2010. Most probably it's due to the fact that to exactly mimic the behaviour you ended up with a code copies from various versions of Django (as back then url tag was not the one we have today).

Nowadays, when 1.7 is the lowest supported version we might indeed try to use URLNode or even reverse itself and gain some speed, but one should keep in mind that it needs to be done if a fully backward-compatible manner.

[PetrDlouhy]

@idlesign I think, this is some kind of misunderstanding. I don't use django-sitetree for generating links inside admin. I am just editing my sitetree there and end up with broken sitetree admin and broken frontend, without any way to take my edit back from admin. See the steps to reproduce in #257.

I cleaned the PR a bit. The problem is that the quotes was not cleaned from view_path if the pattern did not contain any parameters. I also added some more test cases.

PS: There is one failing test, but I don't think, that it is caused by this PR.

[idlesign]

I think, this is some kind of misunderstanding.

Probably. And I undertood #257 from the beginning.
The only way to get that exception is to hit https://github.com/idlesign/django-sitetree/blob/master/sitetree/sitetreeapp.py#L697
If you hit that line it means sitetree tries to resolve URLs. My point is: there's no need to resolve URL in Admin at all. That's why I've asked:

why do you need to resolve tree items URLs in admin at all?

Further: if we do not need to resolve in admin, we could bypass resovling, so there'd be no change to brake the Admin. Site itself still could be broken, but that can be fixed, as you've said in Admin.

[idlesign]

If you still really want to cure a symptom not the cause, you can do better: no need to strip chars on every call, sanitization on model save would suffice. We could probably also add validation on admin form save, but it won't really add value.
I'll accept that.

[timthelion]

@idlesign

Ok, let's try it the other way round: why do you need to resolve tree items URLs in admin at all?

URLs are being resolved in the tree.html admin template which is loaded by the change_form.html admin template which is loaded by sitetree's admin.py.

• https://github.com/idlesign/django-sitetree/blob/master/sitetree/admin.py#L112

• django-sitetree/sitetree/templates/admin/sitetree/tree/change_form.html

  Line 47 in aa25e29

  {% sitetree_tree from original.alias template "admin/sitetree/tree/tree.html" %}

• django-sitetree/sitetree/templates/admin/sitetree/tree/tree.html

  Line 30 in aa25e29

  <td>{{ item.url }}</td>

[idlesign]

@timthelion
Yeap, that confirms my assumption that URLs in admin are resolved, whereas I suspect they should rather not be. We probably need a change (as stated in #258 (comment)) in SiteTree.url that would prevent such resolution while in admin.

[idlesign]

reverse() based solution (just as @PetrDlouhy mentioned it in #258 (comment)) is in master at last (see #290). Sorry for such a long delay.

[PetrDlouhy]

@idlesign I forgot about this for some time, glad to see, that it is fixed now.
I thing the tests in this PR might be still helpful to ensure that this issue doesn't repeat in the future. Are you interested in porting the tests to newest master?

[idlesign]

@PetrDlouhy Sure, PR with test is welcome.