Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 5 commits into from
Aug 2, 2011
Merged
Show file tree
Hide file tree
Changes from 4 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 0 additions & 1 deletion cms/menu.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,7 +112,6 @@ def get_nodes(self, request):
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
home_children.append(page.pk)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I assume this means it was only used in get_absolute_url?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM :)

if (page.pk == home.pk and home.in_navigation) or page.pk != home.pk:
first = False
Expand Down
41 changes: 10 additions & 31 deletions cms/models/pagemodel.py
Original file line number Diff line number Diff line change
Expand Up @@ -117,6 +117,16 @@ def __unicode__(self):
if title is None:
title = u""
return u'%s' % (title,)

def get_absolute_url(self, language=None, fallback=True):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh yeah baby :)

if self.is_home():
return reverse('pages-root')
if settings.CMS_FLAT_URLS:
path = self.get_slug(language, fallback)
return urlutils.urljoin(reverse('pages-root'), path)
# else
path = self.get_path(language, fallback)
return urlutils.urljoin(reverse('pages-root'), path)

def move_page(self, target, position='first-child'):
"""Called from admin interface when page is moved. Should be used on
Expand Down Expand Up @@ -531,37 +541,6 @@ def get_languages(self):
self.all_languages = list(self.all_languages)
self.all_languages.sort()
return self.all_languages

def get_absolute_url(self, language=None, fallback=True):
try:
if self.is_home():
return reverse('pages-root')
except NoHomeFound:
pass
if settings.CMS_FLAT_URLS:
path = self.get_slug(language, fallback)
else:
path = self.get_path(language, fallback)
if hasattr(self, "home_cut_cache") and self.home_cut_cache:
if not self.get_title_obj_attribute("has_url_overwrite", language, fallback) and path:
path = "/".join(path.split("/")[1:])
else:
home_pk = None
try:
home_pk = self.home_pk_cache
except NoHomeFound:
pass
"""
this is pain slow! the code fetches all ancestors for all pages
and then checks if the root node is a home_pk, if yes, it cuts off the first path part.
"""
ancestors = self.get_cached_ancestors(ascending=True)
# sometimes there are no ancestors
if len(ancestors) != 0:
if self.parent_id and ancestors[-1].pk == home_pk and not self.get_title_obj_attribute("has_url_overwrite", language, fallback) and path:
path = "/".join(path.split("/")[1:])

return urlutils.urljoin(reverse('pages-root'), path)

def get_cached_ancestors(self, ascending=True):
if ascending:
Expand Down
26 changes: 18 additions & 8 deletions cms/signals.py
Original file line number Diff line number Diff line change
Expand Up @@ -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):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What does this have to do with the beans?

parent_page_id = title.page.parent_id
slug = u'%s' % title.slug

if title.page.is_home():
title.path = ''
else:
title.path = u'%s' % slug
if parent_page_id:
parent_title = Title.objects.get_title(parent_page_id, language=title.language, language_fallback=True)
if parent_title:
title.path = (u'%s/%s' % (parent_title.path, slug)).lstrip("/")

def pre_save_title(instance, raw, **kwargs):
"""Save old state to instance and setup path
"""
Expand All @@ -64,14 +77,7 @@ def pre_save_title(instance, raw, **kwargs):
if instance.has_url_overwrite and instance.path:
instance.path = instance.path.strip(" /")
else:
parent_page = instance.page.parent
slug = u'%s' % instance.slug

instance.path = u'%s' % slug
if parent_page:
parent_title = Title.objects.get_title(parent_page, language=instance.language, language_fallback=True)
if parent_title:
instance.path = (u'%s/%s' % (parent_title.path, slug)).lstrip("/")
update_title(instance)

signals.pre_save.connect(pre_save_title, sender=Title, dispatch_uid="cms.title.presave")

Expand Down Expand Up @@ -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)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, nevermind, makes sense now.

title.save()


def update_placeholders(instance, **kwargs):
Expand Down
4 changes: 2 additions & 2 deletions cms/tests/menu.py
Original file line number Diff line number Diff line change
Expand Up @@ -279,8 +279,8 @@ def test_home_not_in_menu(self):
tpl.render(context)
nodes = context['children']
self.assertEqual(len(nodes), 1)
self.assertEqual(nodes[0].get_absolute_url(), "%s%s/" % (self.get_pages_root(), self.get_page(2).get_slug()))
self.assertEqual(nodes[0].children[0].get_absolute_url(), "%s%s/%s/" % (self.get_pages_root(), self.get_page(2).get_slug(), self.get_page(3).get_slug()))
self.assertEqual(nodes[0].get_absolute_url(), self.get_page(2).get_absolute_url())
self.assertEqual(nodes[0].children[0].get_absolute_url(), self.get_page(3).get_absolute_url())
page4 = Page.objects.get(pk=self.get_page(4).pk)
page4.in_navigation = True
page4.save()
Expand Down
34 changes: 25 additions & 9 deletions cms/tests/page.py
Original file line number Diff line number Diff line change
Expand Up @@ -16,7 +16,7 @@
from django.contrib.auth.models import User
from django.contrib.sites.models import Site
from django.core.urlresolvers import reverse
from django.http import HttpRequest
from django.http import HttpRequest, HttpResponse, HttpResponseNotFound
import datetime
import os.path

Expand Down Expand Up @@ -201,9 +201,9 @@ def test_move_page(self):
page2 = Page.objects.all()[1]
page3 = Page.objects.all()[2]
# move pages
response = self.client.post("/admin/cms/page/%s/move-page/" % page3.pk, {"target":page2.pk, "position":"last-child" })
response = self.client.post("/admin/cms/page/%s/move-page/" % page3.pk, {"target": page2.pk, "position": "last-child"})
self.assertEqual(response.status_code, 200)
response = self.client.post("/admin/cms/page/%s/move-page/" % page2.pk, {"target":page1.pk, "position":"last-child" })
response = self.client.post("/admin/cms/page/%s/move-page/" % page2.pk, {"target": page1.pk, "position": "last-child"})
self.assertEqual(response.status_code, 200)
# check page2 path and url
page2 = Page.objects.get(pk=page2.pk)
Expand All @@ -214,17 +214,24 @@ def test_move_page(self):
self.assertEqual(page3.get_path(), page_data1['slug']+"/"+page_data2['slug']+"/"+page_data3['slug'])
self.assertEqual(page3.get_absolute_url(), self.get_pages_root()+page_data1['slug']+"/"+page_data2['slug']+"/"+page_data3['slug']+"/")
# publish page 1 (becomes home)
page1 = Page.objects.all()[0]
page1.published = True
page1.save()
page1 = Page.objects.get(pk=page1.pk)
page1.publish()
public_page1 = page1.publisher_public
self.assertEqual(public_page1.get_path(), '')
# check that page2 and page3 url have changed
page2 = Page.objects.get(pk=page2.pk)
self.assertEqual(page2.get_absolute_url(), self.get_pages_root()+page_data2['slug']+"/")
page2.publish()
public_page2 = page2.publisher_public
self.assertEqual(public_page2.get_absolute_url(), self.get_pages_root()+page_data2['slug']+"/")
page3 = Page.objects.get(pk=page3.pk)
self.assertEqual(page3.get_absolute_url(), self.get_pages_root()+page_data2['slug']+"/"+page_data3['slug']+"/")
page3.publish()
public_page3 = page3.publisher_public
self.assertEqual(public_page3.get_absolute_url(), self.get_pages_root()+page_data2['slug']+"/"+page_data3['slug']+"/")
# move page2 back to root and check path of 2 and 3
response = self.client.post("/admin/cms/page/%s/move-page/" % page2.pk, {"target":page1.pk, "position":"left" })
response = self.client.post("/admin/cms/page/%s/move-page/" % page2.pk, {"target": page1.pk, "position": "right"})
self.assertEqual(response.status_code, 200)
page1 = Page.objects.get(pk=page1.pk)
self.assertEqual(page1.get_path(), page_data1['slug'])
page2 = Page.objects.get(pk=page2.pk)
self.assertEqual(page2.get_path(), page_data2['slug'])
page3 = Page.objects.get(pk=page3.pk)
Expand Down Expand Up @@ -498,6 +505,15 @@ def test_page_urls(self):
page3 = self.move_page(page3, page4)
self.assertEqual(page3.get_absolute_url(),
self.get_pages_root()+'test-page-4/test-page-3/')

def test_home_slug_not_accessible(self):
with SettingsOverride(CMS_MODERATOR=False, CMS_PERMISSION=False):
page = create_page('page', 'nav_playground.html', 'en', published=True)
self.assertEqual(page.get_absolute_url('en'), '/')
resp = self.client.get('/en/')
self.assertEqual(resp.status_code, HttpResponse.status_code)
resp = self.client.get('/en/page/')
self.assertEqual(resp.status_code, HttpResponseNotFound.status_code)


class NoAdminPageTests(CMSTestCase):
Expand Down
25 changes: 14 additions & 11 deletions cms/tests/rendering.py
Original file line number Diff line number Diff line change
Expand Up @@ -130,7 +130,7 @@ def test_details_view(self):
"""
with SettingsOverride(**self.render_settings()):
from cms.views import details
response = details(self.get_request(self.test_page), slug=self.test_page.get_slug())
response = details(self.get_request(self.test_page), '')
r = self.strip_rendered(response.content)
self.assertEqual(r, u'|'+self.test_data['text_main']+u'|'+self.test_data['text_sub']+u'|')

Expand Down Expand Up @@ -231,35 +231,38 @@ def test_inherit_placeholder(self):

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'),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit picker ;o)

('x-elvish', 'Elvish')
]):
from cms.views import details
request = AttributeObject(
REQUEST={'language': 'elvish'},
REQUEST={'language': 'x-elvish'},
GET=[],
session={},
path='/',
user=self.test_user,
current_page=None,
method='GET',
)
self.assertRaises(Http404, details, request, slug=self.test_page.get_slug())
self.assertRaises(Http404, details, request, '')

def test_detail_view_fallsback_language(self):
def test_detail_view_fallback_language(self):
'''
Ask for a page in elvish (doesn't exist), and assert that it fallsback
to English
'''
with SettingsOverride(TEMPLATE_CONTEXT_PROCESSORS=[],
CMS_LANGUAGE_CONF={
'elvish': ['klingon', 'en',]
'x-elvish': ['x-klingon', 'en',]
},
CMS_LANGUAGES=[( 'klingon', 'Klingon' ),
( 'elvish', 'Elvish' )
CMS_LANGUAGES=[
('x-klingon', 'Klingon'),
('x-elvish', 'Elvish'),
]):
from cms.views import details
request = AttributeObject(
REQUEST={'language': 'elvish'},
REQUEST={'language': 'x-elvish'},
GET=[],
session={},
path='/',
Expand All @@ -268,7 +271,7 @@ def test_detail_view_fallsback_language(self):
method='GET',
)

response = details(request, slug=self.test_page.get_slug())
response = details(request, '')
self.assertTrue(isinstance(response,HttpResponseRedirect))

def test_extra_context_isolation(self):
Expand Down
16 changes: 12 additions & 4 deletions cms/tests/views.py
Original file line number Diff line number Diff line change
Expand Up @@ -63,11 +63,9 @@ def test_apphook_not_hooked(self):
self.assertEqual(response.status_code, 200)
apphook_pool.clear()

def test_redirect(self):
redirect_one = 'https://www.django-cms.org/'
redirect_two = '/'
redirect_three = '/en/'
def test_external_redirect(self):
# test external redirect
redirect_one = 'https://www.django-cms.org/'
one = create_page("one", "nav_playground.html", "en", published=True,
redirect=redirect_one)
url = one.get_absolute_url()
Expand All @@ -76,7 +74,12 @@ def test_redirect(self):
self.assertEqual(response.status_code, 302)
self.assertEqual(response['Location'], redirect_one)

def test_internal_neutral_redirect(self):
# test internal language neutral redirect
redirect_one = 'https://www.django-cms.org/'
redirect_two = '/'
one = create_page("one", "nav_playground.html", "en", published=True,
redirect=redirect_one)
two = create_page("two", "nav_playground.html", "en", parent=one,
published=True, redirect=redirect_two)
url = two.get_absolute_url()
Expand All @@ -85,7 +88,12 @@ def test_redirect(self):
self.assertEqual(response.status_code, 302)
self.assertEqual(response['Location'], '/en/')

def test_internal_forced_redirect(self):
# test internal forced language redirect
redirect_one = 'https://www.django-cms.org/'
redirect_three = '/en/'
one = create_page("one", "nav_playground.html", "en", published=True,
redirect=redirect_one)
three = create_page("three", "nav_playground.html", "en", parent=one,
published=True, redirect=redirect_three)
url = three.get_absolute_url()
Expand Down
18 changes: 9 additions & 9 deletions cms/utils/page_resolver.py
Original file line number Diff line number Diff line change
Expand Up @@ -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:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not simply delete the block?

# # 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
try:
page = pages.filter(q).distinct().get()
except Page.DoesNotExist:
Expand Down