Skip to content
Open
Show file tree
Hide file tree
Changes from all 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
45 changes: 42 additions & 3 deletions djangocms_versioning/admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@
from django.contrib.contenttypes.models import ContentType
from django.core.exceptions import ImproperlyConfigured, ObjectDoesNotExist, PermissionDenied
from django.db import models
from django.db.models import OuterRef, Subquery
from django.db.models import OuterRef, Prefetch, Subquery
from django.db.models.functions import Cast, Lower
from django.forms import MediaDefiningClass
from django.http import (
Expand Down Expand Up @@ -50,6 +50,7 @@
get_current_site,
get_editable_url,
get_latest_admin_viewable_content,
get_latest_content_from_cache,
get_object_live_url,
get_preview_url,
proxy_model,
Expand Down Expand Up @@ -182,20 +183,29 @@ def _extra_grouping_fields(self):

def get_indicator_column(self, request):
def indicator(obj):
versions = None
if self._extra_grouping_fields is not None: # Grouper Model
content_obj = get_latest_admin_viewable_content(
obj,
include_unpublished_archived=True,
**{field: getattr(self, field) for field in self._extra_grouping_fields},
)
for prefetched in getattr(obj, "_prefetched_contents", []):
prefetched._prefetched_versions[0].content = prefetched # Avoid fetching reverse
versions = (
[content._prefetched_versions[0] for content in obj._prefetched_contents]
if hasattr(obj, "_prefetched_contents")
else None
)
else: # Content Model
content_obj = obj
status = content_indicator(content_obj)

status = content_indicator(content_obj, versions)
menu = (
content_indicator_menu(
request,
status,
content_obj._version,
content_obj._versions,
back=f"{request.path_info}?{request.GET.urlencode()}",
)
if status
Expand Down Expand Up @@ -318,8 +328,37 @@ def get_queryset(self, request: HttpRequest) -> models.QuerySet:
# cast is necessary for mysql
content_modified=Cast(Subquery(contents.values("content_modified")[:1]), models.DateTimeField()),
)
# Prefetching is not implemented here; you may want to use Prefetch for related objects if needed.
# To get the reverse name for self.grouper_field_name:
# It's usually: <related_model>_set or the related_name defined on the ForeignKey.
# For a ForeignKey field, you can get the reverse accessor name like this:
reverse_name = self.content_model._meta.get_field(self.grouper_field_name).remote_field.get_accessor_name()
qs = qs.prefetch_related(
Copy link
Contributor

Choose a reason for hiding this comment

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

suggestion (bug_risk): Reconcile get_content_obj with the new _current_contents prefetch.

The queryset now prefetches the reverse relation twice: into _prefetched_contents (all admin_manager contents) and _current_contents (admin_manager.current_content), but get_content_obj still only uses _prefetched_contents via get_latest_content_from_cache(..., include_unpublished_archived=True). This both negates the benefit of the more selective _current_contents prefetch and changes behavior to always include unpublished/archived content when a cache exists. If this method is meant to return the current content, consider using _current_contents instead, or otherwise make the selection between _prefetched_contents and _current_contents explicit and intentional.

Suggested implementation:

    def get_content_obj(self, obj):
        """
        Returns the "current" content object that belongs to the grouper.

        Priority:
        1. Use the `_current_contents` prefetch (admin_manager.current_content) when available.
        2. Fall back to the cached lookup, which in turn uses `_prefetched_contents`
           if that prefetch was done, or hits the database otherwise.
        """
        # When the changelist queryset has prefetch_related(... to_attr="_current_contents")
        # this will be a list-like of the current content(s) for this grouper.
        current_contents = getattr(obj, "_current_contents", None)
        if current_contents:
            # `_current_contents` should contain at most a single object, but we
            # defensively take the first element.
            return current_contents[0]

        # Fallback: use the existing cache-based helper, which may rely on
        # `_prefetched_contents` if it has been prefetched.
        # We intentionally do NOT force `include_unpublished_archived=True` here so that
        # the helper’s default behavior (usually "current" or "latest" valid content)
        # is preserved unless explicitly requested elsewhere.
        return get_latest_content_from_cache(obj)

This edit assumes that:

  1. get_content_obj currently exists exactly as shown in the SEARCH block and that get_latest_content_from_cache accepts a single obj parameter (with include_unpublished_archived either optional or defaulting appropriately).
  2. The queryset you showed is on the admin changelist for the same model whose instances are passed to get_content_obj, and that _current_contents is the to_attr used in the new Prefetch(...).

If your existing get_content_obj or get_latest_content_from_cache signatures differ (for example, if get_latest_content_from_cache requires the admin instance or the content model, or if include_unpublished_archived must be passed explicitly), you should:

  • Adjust the fallback call accordingly, e.g. return get_latest_content_from_cache(self, obj) or keep include_unpublished_archived=True if that is the intended behavior.
  • Ensure the Prefetch(... to_attr="_current_contents") remains aligned with the attribute name used in get_content_obj.

Prefetch(
reverse_name,
to_attr="_prefetched_contents", # Needed for state indicators
queryset=self.content_model.admin_manager.filter(**self.current_content_filters)
.prefetch_related(Prefetch("versions", to_attr="_prefetched_versions"))
.order_by("-pk"),
)
)
qs = qs.prefetch_related(
Prefetch(
reverse_name,
to_attr="_current_contents", # Service for get_content_obj
queryset=self.content_model.admin_manager.current_content(**self.current_content_filters)
.prefetch_related(Prefetch("versions", to_attr="_prefetched_versions"))
.order_by("-pk"),
)
)
return qs

def get_content_obj(self, obj: models.Model) -> models.Model:
"""Returns the latest content object for the given grouper object."""
if obj is None or self._is_content_obj(obj) or not hasattr(obj, "_prefetched_contents"):
return super().get_content_obj(obj)
return get_latest_content_from_cache(obj._prefetched_contents, include_unpublished_archived=True)

@admin.display(
description=_("State"),
ordering="content_state",
Expand Down
5 changes: 4 additions & 1 deletion djangocms_versioning/cms_config.py
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,9 @@ def handle_admin_classes(self, cms_config):
"""Replaces admin model classes for all registered content types
with an admin model class that inherits from `versionable.content_admin_mixin`.
"""
from django.contrib.admin import autodiscover

autodiscover()
replace_admin_for_models(
[(versionable.content_model, versionable.content_admin_mixin) for versionable in cms_config.versioning]
)
Expand Down Expand Up @@ -328,7 +331,7 @@ def get_indicator_menu(cls, request, page_content):
status = page_content.content_indicator()
if not status or status == "empty": # pragma: no cover
return super().get_indicator_menu(request, page_content)
versions = page_content._version # Cache from .content_indicator()
versions = page_content._versions # Cache from .content_indicator()
back = admin_reverse("cms_pagecontent_changelist") + f"?language={request.GET.get('language')}"
menu = indicators.content_indicator_menu(request, status, versions, back=back)
return menu_template if menu else "", menu
Expand Down
21 changes: 20 additions & 1 deletion djangocms_versioning/helpers.py
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@

from . import versionables
from .conf import EMAIL_NOTIFICATIONS_FAIL_SILENTLY
from .constants import DRAFT
from .constants import DRAFT, PUBLISHED

try:
from djangocms_internalsearch.helpers import emit_content_change
Expand Down Expand Up @@ -360,6 +360,11 @@ def get_latest_admin_viewable_content(
f"Grouping field(s) {missing_fields} required for {versionable.grouper_model}."
)

if hasattr(grouper, "_prefetched_contents"):
return get_latest_content_from_cache(
grouper._prefetched_contents, include_unpublished_archived
)

# Get the name of the content_set (e.g., "pagecontent_set") from the versionable
content_set = versionable.grouper_field.remote_field.get_accessor_name()

Expand All @@ -373,6 +378,20 @@ def get_latest_admin_viewable_content(
return qs.filter(**extra_grouping_fields).current_content().first()


def get_latest_content_from_cache(cache, include_unpublished_archived: bool = False) -> models.Model | None:
# Evaluate prefetched contents in python
for content in cache:
if content._prefetched_versions[0].state == DRAFT:
return content
for content in cache:
if content._prefetched_versions[0].state == PUBLISHED:
return content
if include_unpublished_archived:
order_by_date = sorted(cache, key=lambda v: v._prefetched_versions[0].created, reverse=True)
return order_by_date[0] if order_by_date else None
return None


def get_latest_admin_viewable_page_content(
page: Page, language: str
) -> PageContent: # pragma: no cover
Expand Down
12 changes: 6 additions & 6 deletions djangocms_versioning/indicators.py
Original file line number Diff line number Diff line change
Expand Up @@ -112,20 +112,20 @@ def content_indicator(
}
if DRAFT in signature and PUBLISHED not in signature:
content_obj._indicator_status = "draft"
content_obj._version = signature[DRAFT],
content_obj._versions = signature[DRAFT],
elif DRAFT in signature and PUBLISHED in signature:
content_obj._indicator_status = "dirty"
content_obj._version = (signature[DRAFT], signature[PUBLISHED])
content_obj._versions = (signature[DRAFT], signature[PUBLISHED])
elif PUBLISHED in signature:
content_obj._indicator_status = "published"
content_obj._version = signature[PUBLISHED],
content_obj._versions = signature[PUBLISHED],
elif versions[0].state == UNPUBLISHED:
content_obj._indicator_status = "unpublished"
content_obj._version = signature[UNPUBLISHED],
content_obj._versions = signature[UNPUBLISHED],
elif versions[0].state == ARCHIVED:
content_obj._indicator_status = "archived"
content_obj._version = signature[ARCHIVED],
content_obj._versions = signature[ARCHIVED],
else: # pragma: no cover
content_obj._indicator_status = None
content_obj._version = [None]
content_obj._versions = [None]
return content_obj._indicator_status
2 changes: 1 addition & 1 deletion djangocms_versioning/models.py
Original file line number Diff line number Diff line change
Expand Up @@ -493,7 +493,7 @@ def _has_permission(self, perm: str, user) -> bool:
# Second fallback: placeholder change permissions - works for PageContent
return self.content.has_placeholder_change_permission(user)
# final fallback: Django perms
return user.has_perm(f"{self.content_type.app_label}.change_{self.content_type.model}")
return user.has_perm(f"{self.content._meta.app_label}.change_{self.content._meta.model_name}")

check_modify = Conditions(
[
Expand Down
2 changes: 1 addition & 1 deletion tests/requirements/dj60_cms50.txt
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,7 @@

django-cms>=5.0,<5.1

Django>=6.0a1,<6.1
Django>=6.0,<6.1
django-classy-tags
django-fsm-2==4.1.0
django-sekizai
73 changes: 73 additions & 0 deletions tests/test_admin.py
Original file line number Diff line number Diff line change
Expand Up @@ -3402,3 +3402,76 @@ def test_buttons_in_archived_changeview(self):
self.assertNotContains(response, "New Draft")
self.assertNotContains(response, "Publish")


class GrouperAdminPerformanceTestCase(CMSTestCase):
"""Test to ensure Poll GrouperAdmin changelist_view queries are optimized"""

def setUp(self):
super().setUp()
# Pre-cache site to get consistent query counts across tests
from django.contrib.sites.models import Site
Site.objects.get_current()

def test_poll_grouper_changelist_queries_are_optimized(self):
"""Pin the number of queries for Poll GrouperAdmin changelist to prevent regressions"""
# Create test data: 5 polls with 2 versions each (draft + published)
for _i in range(5):
# Create published version
published = PollVersionFactory(state=constants.PUBLISHED)
# Create draft version for same poll
PollVersionFactory(
content__poll=published.content.poll,
content__language=published.content.language,
state=constants.DRAFT,
)

from djangocms_versioning.test_utils.polls.admin import PollAdmin
from djangocms_versioning.test_utils.polls.models import Poll

poll_admin = PollAdmin(Poll, admin.site)

# Create request
factory = RequestFactory()
request = factory.get("/admin/polls/poll/")
request.session = {}
request.user = self.get_superuser()

# Pin the number of queries
# Expected queries should remain constant due to prefetch optimization
# 1. Count query for pagination
# 2. Count query (duplicate from admin)
# 3. Main queryset with subqueries for content annotations
with self.assertNumQueries(3):
response = poll_admin.changelist_view(request)
# Force evaluation of queryset
list(response.context_data["cl"].result_list)

def test_poll_changelist_with_many_versions_scales_well(self):
"""Ensure query count doesn't increase with more versions per poll"""
# Create 1 poll with many versions
poll_version = PollVersionFactory(state=constants.PUBLISHED)
poll = poll_version.content.poll

# Add 10 more versions (archived)
for _ in range(10):
PollVersionFactory(
content__poll=poll,
content__language=poll_version.content.language,
state=constants.ARCHIVED,
)

from djangocms_versioning.test_utils.polls.admin import PollAdmin
from djangocms_versioning.test_utils.polls.models import Poll

poll_admin = PollAdmin(Poll, admin.site)

factory = RequestFactory()
request = factory.get("/admin/polls/poll/")
request.user = self.get_superuser()
request.session = {}

# Query count should remain the same regardless of version count
# because of prefetch optimization
with self.assertNumQueries(3):
response = poll_admin.changelist_view(request)
list(response.context_data["cl"].result_list)
Loading