Skip to content

Fix for 885 and potentially also 493, 770. #912

Merged
merged 5 commits into from Aug 2, 2011

2 participants

@ojii
ojii commented Aug 2, 2011

PLEASE REVIEW EXTREMELY CAREFULLY. This pull request does some heavy changes to how get_absolute_url works!

Okay this is a big one:

I wanted to fix #885, which I did.
However it appeared that Page.get_absolute_url was a complete mess.
So I ended up fixing that, by moving page-path (url) logic to post-save (Title/Page) signals and compute them there, and store them in Title.path.
This means Page.get_absolute_url() should be A LOT faster now (=faster menus?), however someone better check that first.
It also means that page/title writes (create,update,move,blah) will be SLOWER, but in my opinion, optimizing reads over writes is preferable.

As a little side effect this commit also potentially fixes #493 due to also fixing #770.

And a further side effect, it will HIDE but NOT FIX #760. Potentially making that bug even more obscure (however it might still show up in the signals).

As you can see, I've changed a lot of tests (because they looked broken in my opinion), so this patch should get extra care.

Jonas Obrist added some commits Jun 23, 2011
Jonas Obrist Added test for #885 234f773
Jonas Obrist fixed #885, but broke tons of other tests.... 54b2815
Jonas Obrist Merge branch 'develop' of github.com:divio/django-cms into feature/fi…
…x-885
dd83195
Jonas Obrist Okay this is a big one:
I wanted to fix #885, which I did.
However it appeared that Page.get_absolute_url was a complete mess.
So I ended up fixing that, by moving page-path (url) logic to post-save (Title/Page) signals and compute them there, and store them in Title.path.
This means Page.get_absolute_url() should be A LOT faster now (=faster menus?), however someone better check that first.
It also means that page/title writes (create,update,move,blah) will be SLOWER, but in my opinion, optimizing reads over writes is preferable.

As a little side effect this commit also potentially fixes #493  due to also fixing #770.

And a further side effect, it will HIDE but NOT FIX #760. Potentially making that bug even more obscure (however it might still show up in the signals).

As you can see, I've changed a lot of tests (because they looked broken in my opinion), so this patch should get extra care.
ba38d2e
@chrisglass chrisglass commented on the diff Aug 2, 2011
cms/menu.py
@@ -112,7 +112,6 @@ class CMSMenu(Menu):
elif not settings.CMS_PUBLIC_FOR == 'all':
continue
if (page.parent_id == home.pk or page.parent_id in home_children) and home_cut:
- page.home_cut_cache = True
@chrisglass
chrisglass added a note Aug 2, 2011

I assume this means it was only used in get_absolute_url?

@ojii
ojii added a note Aug 2, 2011

indeed, a short search over the project showed that it was set here and then used by get_absolute_url. It basically does the same I wanted to do for 885

@chrisglass
chrisglass added a note Aug 2, 2011

LGTM :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chrisglass chrisglass commented on the diff Aug 2, 2011
cms/models/pagemodel.py
@@ -117,6 +117,16 @@ class Page(MPTTModel):
if title is None:
title = u""
return u'%s' % (title,)
+
+ def get_absolute_url(self, language=None, fallback=True):
@chrisglass
chrisglass added a note Aug 2, 2011

oh yeah baby :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chrisglass chrisglass commented on the diff Aug 2, 2011
cms/signals.py
@@ -43,6 +43,19 @@ def update_title_paths(instance, **kwargs):
page_moved.connect(update_title_paths, sender=Page, dispatch_uid="cms.title.update_path")
+def update_title(title):
@chrisglass
chrisglass added a note Aug 2, 2011

What does this have to do with the beans?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chrisglass chrisglass commented on the diff Aug 2, 2011
cms/signals.py
@@ -198,6 +204,10 @@ def post_save_page(instance, raw, created, **kwargs):
# tell moderator something was happen with this page
from cms.utils.moderator import page_changed
page_changed(instance, old_page)
+ for page in instance.get_descendants():
+ for title in page.title_set.all():
+ update_title(title)
@chrisglass
chrisglass added a note Aug 2, 2011

Ah, nevermind, makes sense now.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chrisglass chrisglass commented on the diff Aug 2, 2011
cms/tests/rendering.py
@@ -231,11 +231,13 @@ class RenderingTestCase(SettingsOverrideTestCase):
def test_detail_view_404_when_no_language_is_found(self):
with SettingsOverride(TEMPLATE_CONTEXT_PROCESSORS=[],
- CMS_LANGUAGES=[( 'klingon', 'Klingon' ),
- ( 'elvish', 'Elvish' )]):
+ CMS_LANGUAGES=[
+ ('x-klingon', 'Klingon'),
@chrisglass
chrisglass added a note Aug 2, 2011

Nit picker ;o)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@chrisglass chrisglass commented on an outdated diff Aug 2, 2011
cms/utils/page_resolver.py
@@ -65,15 +65,15 @@ def get_page_from_path(path, preview=False):
q = Q(title_set__slug=path)
else:
q = Q(title_set__path=path)
- if home:
- # if we have a home, also search for all paths prefixed with the
- # home slug that are on the same tree as home, since home isn't ussually
- # called with it's slug, thus it's children don't have the home bit in
- # the request either, thus we need to re-add it.
- q2 = Q()
- q2 = Q(title_set__path='%s/%s' % (home.get_slug(), path))
- q2 &= Q(tree_id=home.tree_id)
- q |= q2
+# if home:
@chrisglass
chrisglass added a note Aug 2, 2011

Why not simply delete the block?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Jonas Obrist Removed silly comments to make @chrisglass happy (and also for sanity…
…s sake)
e78d9a6
@chrisglass

Looks very good. I'll quickly take it for a spin before I merge you in.

@chrisglass chrisglass merged commit 2150f1c into divio:develop Aug 2, 2011
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.