Skip to content
This repository
Browse code

Rewrote show_menu template tag to be much more efficient in use of qu…

…eries.

- Fixes sub issue of Issue #47: Query count reduction
- Fixes sub issue of Issue #12: Too many DB queries
  • Loading branch information...
commit d8f3b344928758467ba27281c45175bcbe329461 1 parent 07c8c9c
mvdwaeter authored January 19, 2012
21  fiber/managers.py
@@ -3,6 +3,7 @@
3 3
 
4 4
 from django.db import models
5 5
 from django.utils.translation import ugettext_lazy as _
  6
+from mptt.managers import TreeManager
6 7
 
7 8
 from fiber import editor
8 9
 
@@ -112,3 +113,23 @@ def resort():
112 113
                 next_index = page_content_items.index(next_item)
113 114
                 page_content_items.insert(next_index, item)
114 115
                 resort()
  116
+
  117
+
  118
+class PageManager(TreeManager):
  119
+
  120
+    def link_parent_objects(self, pages):
  121
+        """
  122
+        Given an iterable of page objects which includes all ancestors
  123
+        of any contained pages, unifies the 'parent' objects
  124
+        using items in the iterable.
  125
+        """
  126
+        pages = list(pages)
  127
+        page_dict = {}
  128
+        for p in pages:
  129
+            page_dict[p.id] = p
  130
+        for p in pages:
  131
+            if p.parent_id is None:
  132
+                p.parent = None
  133
+            else:
  134
+                p.parent = page_dict[p.parent_id]
  135
+        return pages
11  fiber/models.py
@@ -7,7 +7,6 @@
7 7
 from django.utils.translation import ugettext
8 8
 from django.utils.translation import ugettext_lazy as _
9 9
 
10  
-from mptt.managers import TreeManager
11 10
 from mptt.models import MPTTModel
12 11
 
13 12
 from app_settings import IMAGES_DIR, FILES_DIR, METADATA_PAGE_SCHEMA, METADATA_CONTENT_SCHEMA
@@ -86,7 +85,7 @@ class Page(MPTTModel):
86 85
     content_items = models.ManyToManyField(ContentItem, through='PageContentItem', verbose_name=_('content items'))
87 86
     metadata = JSONField(blank=True, null=True, schema=METADATA_PAGE_SCHEMA, prefill_from='fiber.models.Page')
88 87
 
89  
-    tree = TreeManager()
  88
+    objects = managers.PageManager()
90 89
 
91 90
     class Meta:
92 91
         verbose_name = _('page')
@@ -152,6 +151,14 @@ def is_last_child(self):
152 151
             return True
153 152
         return self.parent and (self.rght + 1 == self.parent.rght)
154 153
 
  154
+    def is_child_of(self, node):
  155
+        """
  156
+        Returns True if this is a child of the given node.
  157
+        """
  158
+        return (self.tree_id == node.tree_id and
  159
+                self.lft > node.lft and
  160
+                self.rght < node.rght)
  161
+
155 162
     def get_ancestors_include_self(self):
156 163
         """
157 164
         This method is currently used because there is no include_self
87  fiber/templatetags/fiber_tags.py
... ...
@@ -1,3 +1,5 @@
  1
+import operator
  2
+
1 3
 from django import template
2 4
 from django.utils import simplejson
3 5
 
@@ -17,47 +19,74 @@ def show_menu(context, menu_name, min_level, max_level, expand=None):
17 19
     except Page.DoesNotExist:
18 20
         raise Page.DoesNotExist("Menu does not exist.\nNo top-level page found with the title '%s'." % menu_name)
19 21
 
  22
+    # Page.get_absolute_url() accesses self.parent recursively to build URLs
  23
+    # (assuming relative URLs).
  24
+    # This means that to render any menu item, we need all the ancestors up to
  25
+    # the root. Therefore it is more efficient to pull back this tree, without
  26
+    # min_level applied, and apply it just to decide which items to render.
  27
+
20 28
     current_page = None
21 29
     if 'fiber_page' in context:
22 30
         current_page = context['fiber_page']
23 31
 
24  
-    if current_page and current_page.get_root() == root_page:
25  
-        """
26  
-        Get all siblings of the pages in the path from the root_node to the current page,
27  
-        plus the pages one level below the current page,
28  
-        but stay inside min_level and max_level
29  
-        """
30  
-        for page in current_page.get_ancestors_include_self():
31  
-            if min_level <= page.level:
32  
-                if page.level <= max_level:
33  
-                    menu_pages.extend(page.get_siblings(include_self=True))
34  
-                else:
35  
-                    break
36  
-            elif min_level == page.level + 1:
37  
-                if expand == 'all':
38  
-                    menu_pages.extend(page.get_descendants().filter(level__range=(min_level, max_level)))
39  
-                    break
40  
-
41  
-        if min_level <= (current_page.level + 1) <= max_level:
42  
-            if not expand:
43  
-                menu_pages.extend(current_page.get_children())
44  
-            elif expand == 'all_descendants':
45  
-                menu_pages.extend(current_page.get_descendants().filter(level__range=(min_level, max_level)))
  32
+    if current_page and current_page.is_child_of(root_page):
  33
+        tree = root_page.get_descendants(include_self=True).filter(level__lte=max_level)
  34
+        if expand == 'all':
  35
+            needed_pages = tree
  36
+        else:
  37
+            if current_page.level + 1 < min_level:
  38
+                # Nothing to do
  39
+                needed_pages = []
  40
+            else:
  41
+                # We need the 'route' nodes, the 'sibling' nodes and the children
  42
+                route = tree.filter(lft__lt=current_page.lft,
  43
+                                    rght__gt=current_page.rght)
  44
+
  45
+                # We show any siblings of anything in the route to the current page.
  46
+                # The logic here is that if the user drills down, menu items
  47
+                # shown previously should not disappear.
  48
+
  49
+                # The following assumes that accessing .parent is cheap, which
  50
+                # it can be if current_page was loaded correctly.
  51
+                p = current_page
  52
+                sibling_qs = []
  53
+                while p.parent_id is not None:
  54
+                    sibling_qs.append(tree.filter(level=p.level,
  55
+                                                  lft__gt=p.parent.lft,
  56
+                                                  rght__lt=p.parent.rght))
  57
+                    p = p.parent
  58
+                route_siblings = reduce(operator.or_, sibling_qs)
  59
+
  60
+                children = tree.filter(lft__gt=current_page.lft,
  61
+                                       rght__lt=current_page.rght)
  62
+                if expand != 'all_descendants':
  63
+                    # only want immediate children:
  64
+                    children = children.filter(level=current_page.level + 1)
  65
+
  66
+                needed_pages = route | route_siblings | children
46 67
 
47 68
     else:
48  
-        """
49  
-        Only show menus that start at the first level (min_level == 1)
50  
-        when the current page is not in the menu tree.
51  
-        """
  69
+        # Only show menus that start at the first level (min_level == 1)
  70
+        # when the current page is not in the menu tree.
52 71
         if min_level == 1:
53 72
             if not expand:
54  
-                menu_pages.extend(Page.objects.filter(tree_id=root_page.tree_id).filter(level=min_level))
  73
+                needed_pages = Page.objects.filter(tree_id=root_page.tree_id).filter(level__lte=1)
55 74
             elif expand == 'all':
56  
-                menu_pages.extend(Page.objects.filter(tree_id=root_page.tree_id).filter(level__range=(min_level, max_level)))
  75
+                needed_pages = Page.objects.filter(tree_id=root_page.tree_id).filter(level__lte=max_level)
  76
+            else:
  77
+                needed_pages = []
  78
+
  79
+    needed_pages = Page.objects.link_parent_objects(needed_pages)
  80
+
  81
+    # Now we need to do min_level filtering
  82
+    for p in needed_pages:
  83
+        if p.level >= min_level:
  84
+            menu_pages.append(p)
57 85
 
58 86
     # Remove pages that shouldn't be shown in the menu for the current user.
59 87
     user = context['user']
60  
-    menu_pages = [p for p in menu_pages if (p.is_public_for_user(user) and p.show_in_menu)]
  88
+    menu_pages = [p for p in menu_pages if (p.is_public_for_user(user)
  89
+                                            and p.show_in_menu)]
61 90
 
62 91
     """
63 92
     Order menu_pages for use with tree_info template tag.
52  fiber/tests.py
@@ -327,29 +327,30 @@ def test_show_user_menu_all(self):
327 327
             'user': self.get_non_staff_user(),
328 328
             'fiber_page': Page.objects.get(title='home'),
329 329
         })
330  
-        self.assertEquals(
331  
-            strip_whitespace(t.render(c)),
332  
-            ('<ul>'
333  
-               '<li class="home first last">'
334  
-                 '<a href="/">home</a>'
335  
-                 '<ul>'
336  
-                   '<li class="section1 first">'
337  
-                     '<a href="/section1/">section1</a>'
  330
+        with self.assertNumQueries(2):
  331
+            self.assertEquals(
  332
+                strip_whitespace(t.render(c)),
  333
+                ('<ul>'
  334
+                   '<li class="home first last">'
  335
+                     '<a href="/">home</a>'
338 336
                      '<ul>'
339  
-                       '<li class="sub1 first">'
340  
-                         '<a href="/section1/sub1/">sub1</a>'
  337
+                       '<li class="section1 first">'
  338
+                         '<a href="/section1/">section1</a>'
  339
+                         '<ul>'
  340
+                           '<li class="sub1 first">'
  341
+                             '<a href="/section1/sub1/">sub1</a>'
  342
+                           '</li>'
  343
+                           '<li class="sub2 last">'
  344
+                             '<a href="/section1/sub2/">sub2</a>'
  345
+                           '</li>'
  346
+                         '</ul>'
341 347
                        '</li>'
342  
-                       '<li class="sub2 last">'
343  
-                         '<a href="/section1/sub2/">sub2</a>'
  348
+                       '<li class="section2 last">'
  349
+                         '<a href="/section2/">section2</a>'
344 350
                        '</li>'
345 351
                      '</ul>'
346 352
                    '</li>'
347  
-                   '<li class="section2 last">'
348  
-                     '<a href="/section2/">section2</a>'
349  
-                   '</li>'
350  
-                 '</ul>'
351  
-               '</li>'
352  
-             '</ul>'))
  353
+                 '</ul>'))
353 354
 
354 355
     def test_show_user_menu_all_descendants(self):
355 356
         """
@@ -464,13 +465,14 @@ def test_show_user_menu_different_root(self):
464 465
             'user': self.get_non_staff_user(),
465 466
             'fiber_page': other_root,
466 467
         })
467  
-        self.assertEquals(
468  
-            strip_whitespace(t.render(c)),
469  
-            ('<ul>'
470  
-             '<li class="home first last">'
471  
-             '<a href="/">home</a>'
472  
-             '</li>'
473  
-             '</ul>'))
  468
+        with self.assertNumQueries(2):
  469
+            self.assertEquals(
  470
+                strip_whitespace(t.render(c)),
  471
+                ('<ul>'
  472
+                 '<li class="home first last">'
  473
+                 '<a href="/">home</a>'
  474
+                 '</li>'
  475
+                 '</ul>'))
474 476
 
475 477
 
476 478
     def test_show_admin_menu_all(self):

0 notes on commit d8f3b34

Please sign in to comment.
Something went wrong with that request. Please try again.