general: refine navigation/menu page visibility (#28963)

combo/public/menu.py

@@ -17,9 +17,10 @@
 from django.template.loader import get_template
 
 from combo.data.models import Page
+from combo.utils import NothingInCacheException
 
 
-def render_menu(context, level=0, root_page=None, depth=1):
+def render_menu(context, level=0, root_page=None, depth=1, ignore_visibility=False):
     context['root_page'] = root_page
     if root_page:
         level = len(root_page.get_parents_and_self())
@@ -27,7 +28,7 @@ def render_menu(context, level=0, root_page=None, depth=1):
     template = get_template('combo/menu.html')
     return template.render(context)
 
-def get_menu_context(context, level=0, current_page=None, depth=1):
+def get_menu_context(context, level=0, current_page=None, depth=1, ignore_visibility=False):
     if 'hierarchical_pages_by_id' not in context:
         context['hierarchical_pages_by_id'] = Page.get_with_hierarchy_attributes()
     pages_by_id = context['hierarchical_pages_by_id']
@@ -68,7 +69,11 @@ def get_menu_context(context, level=0, current_page=None, depth=1):
     for element in elements:
         if element.exclude_from_navigation:
             continue
-        if not context.get('render_skeleton'):
+        if not ignore_visibility:
+            if context.get('render_skeleton'):
+                if not element.public:
+                    # mark the menu cell for asynchronous rendering.
+                    raise NothingInCacheException()
             if not element.is_visible(context['request'].user):
                 continue
         menuitem = {'page': element}

combo/public/templates/combo/page_template.html

@@ -15,7 +15,7 @@
        {% if check_badges %}data-check-badges="true"{% endif %}
        data-api-root="{{ site_base }}/api/">
  <div id="title"><h1>{{ page.title }}</h1></div>
- <div id="menu">{% block menu %}{% show_menu %}{% endblock %}</div>
+ <div id="menu">{% block menu %}{% show_menu ignore_visibility=True %}{% endblock %}</div>
  <div id="content">
 
   {% block messages %}

combo/public/templatetags/combo.py

@@ -148,7 +148,7 @@ class ExtraPlaceholderNode(template.Node):
         return skeleton_text(context, self.placeholder_name, content=self.content)
 
 @register.inclusion_tag('combo/menu.html', takes_context=True)
-def show_menu(context, level=0, current_page=None, depth=1, reduce_depth=False):
+def show_menu(context, level=0, current_page=None, depth=1, ignore_visibility=False, reduce_depth=False):
     if reduce_depth:
         depth -= 1
     new_context = {
@@ -156,7 +156,7 @@ def show_menu(context, level=0, current_page=None, depth=1, reduce_depth=False):
         'render_skeleton': context.get('render_skeleton'),
         'request': context['request']}
     return get_menu_context(new_context, level=level, current_page=current_page,
-            depth=depth)
+            depth=depth, ignore_visibility=ignore_visibility)
 
 @register.simple_tag(takes_context=True)
 def page_absolute_url(context, page):

tests/test_cells.py

@@ -4,7 +4,7 @@ import os
 import pytest
 import requests
 
-from combo.data.models import Page, CellBase, TextCell, LinkCell, JsonCellBase, JsonCell, ConfigJsonCell
+from combo.data.models import Page, CellBase, TextCell, LinkCell, MenuCell, JsonCellBase, JsonCell, ConfigJsonCell
 from django.conf import settings
 from django.db import connection
 from django.forms.widgets import Media
@@ -109,6 +109,41 @@ def test_link_cell():
     assert cell.render(ctx).strip() == '<a href="http://example.net/#anchor">altertitle</a>'
 
 
+def test_menu_cell():
+    Page.objects.all().delete()
+    parent = page = Page(title='Page1', slug='page1', template_name='standard')
+    page.save()
+    page = Page(title='Page2', slug='page2', template_name='standard',
+            parent=parent)
+    page.save()
+    page = Page(title='Page3', slug='page3', template_name='standard',
+            parent=parent, public=False)
+    page.save()
+    cell = MenuCell(root_page=parent, order=0, page=parent)
+    cell.save()
+    request = RequestFactory().get('/page1/')
+    request.user = None
+    ctx = {'page': parent, 'request': request}
+    assert 'menu-page2' in cell.render(ctx)
+    assert 'menu-page3' not in cell.render(ctx)
+
+    request.user = User(username='foo', email='foo@example.net')
+    assert 'menu-page2' in cell.render(ctx)
+    assert 'menu-page3' in cell.render(ctx)
+
+    # check cell is deferred in skeletons if some pages are private
+    ctx = {'page': parent, 'request': request, 'render_skeleton': True}
+    with pytest.raises(NothingInCacheException):
+        cell.render(ctx)
+
+    # and that it's rendered directly if all pages are public
+    page.public = True
+    page.save()
+    ctx = {'page': parent, 'request': request, 'render_skeleton': True}
+    assert 'menu-page2' in cell.render(ctx)
+    assert 'menu-page3' in cell.render(ctx)
+
+
 def test_variant_templates():
     page = Page(title='example page', slug='example-page')
     page.save()

tests/test_public.py

@@ -268,6 +268,13 @@ def test_page_skeleton(app):
     resp = app.get('/__skeleton__/?source=%s' % quote('http://example.net/foo/bar'))
     resp = app.get('/__skeleton__/?source=%s' % quote('http://example.net/badredir'))
 
+    # add a page with restricted visibility
+    page = Page(title='RestrictedVisibility', slug='restrictedvisibilit',
+            template_name='standard', public=False)
+    page.save()
+    resp = app.get('/__skeleton__/?source=%s' % quote('http://127.0.0.1:8999/'))
+    assert 'RestrictedVisibility' in resp.text
+
 
 def test_subpage_location(app):
     Page.objects.all().delete()
@@ -316,6 +323,21 @@ def test_subpage_location(app):
     resp = app.get('/second/child-second/grand-child-second/', status=200)
     assert 'Grand child of second' in resp.text
 
+
+def test_menu(app):
+    Page.objects.all().delete()
+    page = Page(title='Page1', slug='index', template_name='standard')
+    page.save()
+    page = Page(title='Page2', slug='page2', template_name='standard')
+    page.save()
+    page = Page(title='Page3', slug='page3', template_name='standard', public=False)
+    page.save()
+    resp = app.get('/', status=200)
+    assert 'menu-index' in resp.text
+    assert 'menu-page2' in resp.text
+    assert 'menu-page3' in resp.text
+
+
 def test_404(app):
     Page.objects.all().delete()
     resp = app.get('/foobar/', status=404)