Merge pull request #1184 from ae-utbm/page-N+1

fix: N+1 query on PageListView

core/models.py

@@ -1197,6 +1197,18 @@ class NotLocked(LockError):
     pass
 
 
+class PageQuerySet(models.QuerySet):
+    def viewable_by(self, user: User) -> Self:
+        if user.is_anonymous:
+            return self.filter(view_groups=settings.SITH_GROUP_PUBLIC_ID)
+        if user.has_perm("core.view_page"):
+            return self.all()
+        groups_ids = [g.id for g in user.cached_groups]
+        if user.is_subscribed:
+            groups_ids.append(settings.SITH_GROUP_SUBSCRIBERS_ID)
+        return self.filter(view_groups__in=groups_ids)
+
+
 # This function prevents generating migration upon settings change
 def get_default_owner_group():
     return settings.SITH_GROUP_ROOT_ID
@@ -1266,6 +1278,8 @@ class Page(models.Model):
         _("lock_timeout"), null=True, blank=True, default=None
     )
 
+    objects = PageQuerySet.as_manager()
+
     class Meta:
         unique_together = ("name", "parent")
         permissions = (

core/tests/test_page.py

@@ -1,11 +1,14 @@
 import pytest
+from django.conf import settings
+from django.contrib.auth.models import Permission
 from django.test import Client
 from django.urls import reverse
 from model_bakery import baker
 from pytest_django.asserts import assertRedirects
 
-from core.baker_recipes import board_user
+from core.baker_recipes import board_user, subscriber_user
-from core.models import Page
+from core.models import AnonymousUser, Page, User
+from sith.settings import SITH_GROUP_OLD_SUBSCRIBERS_ID, SITH_GROUP_SUBSCRIBERS_ID
 
 
 @pytest.mark.django_db
@@ -24,3 +27,32 @@ def test_edit_page(client: Client):
     assertRedirects(res, reverse("core:page", kwargs={"page_name": page._full_name}))
     revision = page.revisions.last()
     assert revision.content == "Hello World"
+
+
+@pytest.mark.django_db
+def test_viewable_by():
+    # remove existing pages to prevent side effect
+    Page.objects.all().delete()
+    view_groups = [
+        [settings.SITH_GROUP_PUBLIC_ID],
+        [settings.SITH_GROUP_PUBLIC_ID, SITH_GROUP_SUBSCRIBERS_ID],
+        [SITH_GROUP_SUBSCRIBERS_ID],
+        [SITH_GROUP_SUBSCRIBERS_ID, SITH_GROUP_OLD_SUBSCRIBERS_ID],
+        [],
+    ]
+    pages = baker.make(Page, _quantity=len(view_groups), _bulk_create=True)
+    for page, groups in zip(pages, view_groups, strict=True):
+        page.view_groups.set(groups)
+
+    viewable = Page.objects.viewable_by(AnonymousUser()).values_list("id", flat=True)
+    assert set(viewable) == {pages[0].id, pages[1].id}
+
+    subscriber = subscriber_user.make()
+    viewable = Page.objects.viewable_by(subscriber).values_list("id", flat=True)
+    assert set(viewable) == {p.id for p in pages[0:4]}
+
+    root_user = baker.make(
+        User, user_permissions=[Permission.objects.get(codename="view_page")]
+    )
+    viewable = Page.objects.viewable_by(root_user).values_list("id", flat=True)
+    assert set(viewable) == {p.id for p in pages}

core/views/page.py

@@ -43,11 +43,14 @@ class CanEditPagePropMixin(CanEditPropMixin):
         return res
 
 
-class PageListView(CanViewMixin, ListView):
+class PageListView(ListView):
     model = Page
     template_name = "core/page_list.jinja"
-    queryset = (
+
-        Page.objects.annotate(
+    def get_queryset(self):
+        return (
+            Page.objects.viewable_by(self.request.user)
+            .annotate(
                 display_name=Coalesce(
                     Subquery(
                         PageRev.objects.filter(page=OuterRef("id"))
@@ -57,7 +60,6 @@ class PageListView(CanViewMixin, ListView):
                     F("name"),
                 )
             )
-        .prefetch_related("view_groups")
             .select_related("parent")
         )