general: force URLs to reflect real page hierarchy (#8522)

combo/data/models.py

@@ -94,9 +94,11 @@ class Page(models.Model):
 
     def get_online_url(self):
         parts = [x.slug for x in self.get_parents_and_self()]
-        if parts == ['index']:
+        if parts[0] == 'index':
+            parts = parts[1:]
+        if not parts:
             return '/'
-        return '/' + '/'.join(parts)
+        return '/' + '/'.join(parts) + '/'
 
     def get_page_of_level(self, level):
         '''Return page of given level in the page hierarchy.'''

combo/public/views.py

@@ -170,23 +170,28 @@ def empty_site(request):
 def page(request):
     url = request.path_info
     parts = [x for x in request.path_info.strip('/').split('/') if x]
+    if len(parts) == 1 and parts[0] == 'index':
+        return HttpResponsePermanentRedirect('/')
     if not parts:
         parts = ['index']
     try:
-        pages = [Page.objects.get(slug=x) for x in parts]
+        page = Page.objects.get(slug=parts[-1])
     except Page.DoesNotExist:
-        if not url.endswith('/') and settings.APPEND_SLASH:
-            return HttpResponsePermanentRedirect(url + '/')
         if Page.objects.count() == 0 and parts == ['index']:
             return empty_site(request)
         else:
+            if not url.endswith('/') and settings.APPEND_SLASH:
+                # this is useful to allow /login, /manage, and other non-page
+                # URLs to work.
+                return HttpResponsePermanentRedirect(url + '/')
             raise Http404()
 
-    for i, page in enumerate(pages[1:]):
-        if page.parent_id != pages[i].id:
-            raise Http404()
+    pages = list(page.get_parents_and_self())
 
-    page = pages[-1]
+    if page.get_online_url() != url:
+        if not url.endswith('/') and settings.APPEND_SLASH:
+            return HttpResponsePermanentRedirect(url + '/')
+        raise Http404()
 
     if not page.is_visible(request.user):
         if not request.user.is_authenticated():

tests/test_cells.py

@@ -76,15 +76,15 @@ def test_link_cell():
 
     cell.link_page = page
     cell.save()
-    assert cell.render(ctx).strip() == '<a href="/example-page">example page</a>'
+    assert cell.render(ctx).strip() == '<a href="/example-page/">example page</a>'
 
     cell.title = 'altertitle'
     cell.save()
-    assert cell.render(ctx).strip() == '<a href="/example-page">altertitle</a>'
+    assert cell.render(ctx).strip() == '<a href="/example-page/">altertitle</a>'
 
     cell.anchor = 'anchor'
     cell.save()
-    assert cell.render(ctx).strip() == '<a href="/example-page#anchor">altertitle</a>'
+    assert cell.render(ctx).strip() == '<a href="/example-page/#anchor">altertitle</a>'
 
     cell.link_page = None
     cell.save()

tests/test_pages.py

@@ -19,11 +19,11 @@ def test_page_url():
     page = Page()
     page.slug = 'foo'
     page.save()
-    assert page.get_online_url() == '/foo'
+    assert page.get_online_url() == '/foo/'
     page2 = Page()
     page2.slug = 'bar'
     page2.parent = page
-    assert page2.get_online_url() == '/foo/bar'
+    assert page2.get_online_url() == '/foo/bar/'
 
 def test_page_of_level():
     parent_page = None

tests/test_public.py

@@ -21,7 +21,8 @@ def test_index():
     page = Page(title='Home', slug='index', template_name='standard')
     page.save()
     app = TestApp(application)
-    resp = app.get('/index', status=200)
+    resp = app.get('/index', status=301)
+    assert resp.location == 'http://localhost:80/'
     resp = app.get('/', status=200)
 
     # check {% now %} inside a skeleton_extra_placeholder is interpreted
@@ -49,7 +50,9 @@ def test_page_footer_acquisition():
 
     page = Page(title='Second', slug='second', template_name='standard')
     page.save()
-    resp = app.get('/second', status=200)
+    resp = app.get('/second', status=301)
+    assert resp.location == 'http://localhost:80/second/'
+    resp = app.get('/second/', status=200)
     assert 'BARFOO' in resp.body
 
 def test_page_redirect():
@@ -58,7 +61,7 @@ def test_page_redirect():
             redirect_url='http://example.net')
     page.save()
     app = TestApp(application)
-    resp = app.get('/elsewhere', status=302)
+    resp = app.get('/elsewhere/', status=302)
     assert resp.location == 'http://example.net'
 
 def test_page_private_unlogged():
@@ -106,3 +109,51 @@ def test_page_skeleton():
 
     # check {% now %} inside a skeleton_extra_placeholder is not interpreted
     assert '{%now' in resp.body
+
+def test_subpage_location():
+    Page.objects.all().delete()
+
+    page_index = Page(title='Home Page', slug='index', template_name='standard')
+    page_index.save()
+
+    page_sibling = Page(title='Second top level page', slug='second', template_name='standard')
+    page_sibling.save()
+
+    page = Page(title='Child of Home', slug='child-home',
+            template_name='standard', parent=page_index)
+    page.save()
+
+    page = Page(title='Grand child of home', slug='grand-child-home',
+            template_name='standard', parent=page)
+    page.save()
+
+    page = Page(title='Child of second', slug='child-second',
+            template_name='standard', parent=page_sibling)
+    page.save()
+
+    page = Page(title='Grand child of second', slug='grand-child-second',
+            template_name='standard', parent=page)
+    page.save()
+
+    app = TestApp(application)
+    resp = app.get('/', status=200)
+    assert 'Home Page' in resp.body
+
+    resp = app.get('/child-home/', status=200)
+    assert 'Child of Home' in resp.body
+
+    resp = app.get('/child-home/grand-child-home/', status=200)
+    assert 'Grand child of home' in resp.body
+
+    assert app.get('/child-home/grand-child-home', status=301).location == \
+            'http://localhost:80/child-home/grand-child-home/'
+    app.get('/grand-child-home/', status=404)
+
+    resp = app.get('/second/', status=200)
+    assert 'Second top level page' in resp.body
+
+    resp = app.get('/second/child-second/', status=200)
+    assert 'Child of second' in resp.body
+
+    resp = app.get('/second/child-second/grand-child-second/', status=200)
+    assert 'Grand child of second' in resp.body