Open
Conversation
Contributor
Reviewer's GuideOptimizes the versioned content admin grouper changelist performance by prefetching related content and versions, reusing cached version data in indicator/menu helpers, tightening content resolution logic to use prefetched caches, and updating permission checks and admin wiring, backed by new query-pinning performance tests for poll groupers. Sequence diagram for optimized admin grouper changelist renderingsequenceDiagram
actor AdminUser
participant DjangoAdmin as DjangoAdminSite
participant GrouperAdmin as GrouperChangelistAdmin
participant DB as Database
participant Helpers as Helpers
participant Indicators as Indicators
AdminUser->>DjangoAdmin: Request grouper changelist
DjangoAdmin->>GrouperAdmin: get_queryset(request)
GrouperAdmin->>DB: Query grouper objects
GrouperAdmin->>DB: Prefetch related contents
Note right of GrouperAdmin: contents -> _prefetched_contents
GrouperAdmin->>DB: Prefetch versions for contents
Note right of GrouperAdmin: versions -> content._prefetched_versions
GrouperAdmin-->>DjangoAdmin: queryset with prefetched data
loop For each row in changelist
DjangoAdmin->>GrouperAdmin: get_indicator_column(request)
GrouperAdmin->>GrouperAdmin: indicator(obj)
alt Grouper object
GrouperAdmin->>Helpers: get_latest_admin_viewable_content(grouper, include_unpublished_archived)
alt Prefetched contents available
Helpers->>Helpers: get_latest_content_from_cache(grouper._prefetched_contents)
Helpers-->>GrouperAdmin: latest_content
else No prefetched contents
Helpers->>DB: Query related contents and versions
Helpers-->>GrouperAdmin: latest_content
end
GrouperAdmin->>Indicators: content_indicator(content_obj, versions from _prefetched_contents)
else Content object
GrouperAdmin->>Indicators: content_indicator(obj, versions)
end
Indicators-->>GrouperAdmin: status, content_obj._versions cached
GrouperAdmin->>Indicators: content_indicator_menu(request, status, content_obj._versions, back)
Indicators-->>GrouperAdmin: menu
GrouperAdmin-->>DjangoAdmin: rendered indicator cell
end
Updated class diagram for versioned admin and indicator cachingclassDiagram
class GrouperAdmin {
+content_model
+grouper_field_name
+current_content_filters
+get_queryset(request)
+get_indicator_column(request)
+get_content_obj(obj)
-_extra_grouping_fields
}
class ContentModel {
+admin_manager
+versions
+_prefetched_versions
+_indicator_status
+_versions
}
class Version {
+state
+created
+content
+content_type
+_has_permission(perm, user) bool
}
class HelpersModule {
+get_latest_admin_viewable_content(grouper, include_unpublished_archived, extra_grouping_fields) ContentModel
+get_latest_content_from_cache(cache, include_unpublished_archived) ContentModel
}
class IndicatorsModule {
+content_indicator(content_obj, versions) str
+content_indicator_menu(request, status, versions, back)
}
class CMSConfigApp {
+handle_admin_classes(cms_config)
}
GrouperAdmin --> ContentModel : lists as
ContentModel "1" --> "*" Version : versions
HelpersModule ..> ContentModel : returns
HelpersModule ..> Version : uses state, created
GrouperAdmin ..> HelpersModule : uses
GrouperAdmin ..> IndicatorsModule : uses
IndicatorsModule ..> ContentModel : sets _indicator_status,_versions
Version --> ContentModel : content
CMSConfigApp ..> GrouperAdmin : replaces admin classes
File-Level Changes
Possibly linked issues
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey there - I've reviewed your changes - here's some feedback:
- In
GrouperAdmin.get_queryset, you build twoPrefetchobjects on the samereverse_namewith very similar querysets; consider extracting a shared helper or at least documenting why both are needed to reduce future maintenance confusion. - Several places assume
_prefetched_versionsexists on prefetched content (e.g.,get_indicator_column,get_latest_content_from_cache); adding defensive checks or a clear failure mode will make this more robust if the queryset or prefetch configuration changes. - Since
_versionis replaced by_versionsfor indicator state, consider keeping_versionas a backward-compatible alias (or at least updating all known consumers in one place) to avoid subtle breakages in any external/custom code relying on the old attribute.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- In `GrouperAdmin.get_queryset`, you build two `Prefetch` objects on the same `reverse_name` with very similar querysets; consider extracting a shared helper or at least documenting why both are needed to reduce future maintenance confusion.
- Several places assume `_prefetched_versions` exists on prefetched content (e.g., `get_indicator_column`, `get_latest_content_from_cache`); adding defensive checks or a clear failure mode will make this more robust if the queryset or prefetch configuration changes.
- Since `_version` is replaced by `_versions` for indicator state, consider keeping `_version` as a backward-compatible alias (or at least updating all known consumers in one place) to avoid subtle breakages in any external/custom code relying on the old attribute.
## Individual Comments
### Comment 1
<location> `djangocms_versioning/admin.py:336` </location>
<code_context>
+ # 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(
+ Prefetch(
+ reverse_name,
</code_context>
<issue_to_address>
**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:
```python
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`.
</issue_to_address>
### Comment 2
<location> `tests/test_admin.py:3448-3446` </location>
<code_context>
+ def test_poll_changelist_with_many_versions_scales_well(self):
</code_context>
<issue_to_address>
**suggestion (testing):** The scalability test currently only checks query count for a single poll; consider expanding coverage to better prove the optimization scales with more groupers and different version mixes.
Consider adding a couple more scenarios to better exercise this: for example, multiple polls each with many versions, and polls with mixed states (draft+published, archived-only, unpublished). You could either extend this test or add parametrized cases that both keep query count constant and verify the changelist still shows the correct state/indicator for each row.
Suggested implementation:
```python
def test_poll_changelist_with_many_versions_scales_well(self):
"""
Ensure query count doesn't increase with more versions per poll and
that the changelist still reflects the correct state for each poll row.
"""
# ------------------------------------------------------------------
# Scenario 1: single poll with many versions
# ------------------------------------------------------------------
base_version = PollVersionFactory(state=constants.PUBLISHED)
base_poll = base_version.content.poll
# Add 10 more versions (archived) for the same poll
for _ in range(10):
PollVersionFactory(
content__poll=base_poll,
content__language=base_version.content.language,
state=constants.ARCHIVED,
)
request = self.factory.get(reverse("admin:polls_poll_changelist"))
request.user = self.superuser
poll_admin = PollAdmin(Poll, site)
# Pin the number of queries for the "many versions" case
with self.assertNumQueries(3):
response = poll_admin.changelist_view(request)
cl = response.context_data["cl"]
result_list = list(cl.result_list)
# There should still be only one row for the poll, with the latest published version visible
self.assertEqual(len(result_list), 1)
# Adjust attribute names here to match how "latest version state" is exposed on the row object
self.assertEqual(getattr(result_list[0], "latest_version_state"), constants.PUBLISHED)
# ------------------------------------------------------------------
# Scenario 2: multiple polls each with many versions
# ------------------------------------------------------------------
multi_polls = []
for _ in range(3):
latest = PollVersionFactory(state=constants.PUBLISHED)
poll = latest.content.poll
multi_polls.append((poll, latest))
for _ in range(5):
PollVersionFactory(
content__poll=poll,
content__language=latest.content.language,
state=constants.ARCHIVED,
)
with self.assertNumQueries(3):
response = poll_admin.changelist_view(request)
cl = response.context_data["cl"]
result_list = list(cl.result_list)
# Expect at least the base poll + the newly created multi_polls
ids = {obj.id for obj in result_list}
self.assertIn(base_poll.id, ids)
for poll, _ in multi_polls:
self.assertIn(poll.id, ids)
# ------------------------------------------------------------------
# Scenario 3: mixed states:
# - poll with draft + published versions
# - poll with archived-only versions
# - poll with no versions (unpublished)
# ------------------------------------------------------------------
# Draft + published
draft_version = PollVersionFactory(state=constants.DRAFT)
mixed_poll = draft_version.content.poll
published_mixed_version = PollVersionFactory(
content__poll=mixed_poll,
content__language=draft_version.content.language,
state=constants.PUBLISHED,
)
# Archived-only poll
archived_only_version = PollVersionFactory(state=constants.ARCHIVED)
archived_only_poll = archived_only_version.content.poll
for _ in range(3):
PollVersionFactory(
content__poll=archived_only_poll,
content__language=archived_only_version.content.language,
state=constants.ARCHIVED,
)
# Unpublished poll (no versions)
unpublished_poll = PollFactory()
with self.assertNumQueries(3):
response = poll_admin.changelist_view(request)
cl = response.context_data["cl"]
result_list = list(cl.result_list)
by_id = {row.id: row for row in result_list}
# We should have entries for each of the mixed-state polls
self.assertIn(mixed_poll.id, by_id)
self.assertIn(archived_only_poll.id, by_id)
self.assertIn(unpublished_poll.id, by_id)
mixed_row = by_id[mixed_poll.id]
archived_only_row = by_id[archived_only_poll.id]
unpublished_row = by_id[unpublished_poll.id]
# The following assertions assume the changelist rows expose some
# sort of "indicator" or "state" field. Adjust attribute names to your admin:
#
# mixed_row.latest_version_state -> state of the version that should be
# shown/used in the changelist (expected: PUBLISHED)
# archived_only_row.latest_version_state -> expected: ARCHIVED
# unpublished_row.latest_version_state -> expected: None or a sentinel
self.assertEqual(getattr(mixed_row, "latest_version_state"), constants.PUBLISHED)
self.assertEqual(getattr(archived_only_row, "latest_version_state"), constants.ARCHIVED)
self.assertIsNone(getattr(unpublished_row, "latest_version_state", None))
```
1. Ensure that `PollAdmin`, `Poll`, `PollFactory`, `PollVersionFactory`, `constants`, `reverse`, and `site` are imported/available in this test module (most are likely already present from the existing tests).
2. The example uses a `latest_version_state` attribute and assumes row objects can be keyed by `id`. Adjust those attribute names to whatever your admin changelist actually exposes (for example: an annotated `latest_state`, `current_state`, or a method field the admin adds). Update the three `getattr(..., "latest_version_state")` calls and any related expectations accordingly.
3. If your changelist uses a different primary key or representation for rows (e.g. `row.pk` instead of `row.id`), update the `ids` and `by_id` mappings to use the correct attribute.
4. If your existing tests rely on a different fixed query count than `3` (for example if the earlier optimization changed it), update the `assertNumQueries(3)` values to match the expected constant number for your setup.
</issue_to_address>
### Comment 3
<location> `djangocms_versioning/admin.py:185` </location>
<code_context>
def get_indicator_column(self, request):
def indicator(obj):
+ versions = None
if self._extra_grouping_fields is not None: # Grouper Model
</code_context>
<issue_to_address>
**issue (complexity):** Consider extracting small helper methods for handling prefetched versions, building repeated Prefetch querysets, and resolving latest prefetched content to simplify the admin logic and avoid duplication.
You can keep all the new behaviour but reduce complexity and duplication by extracting a few focused helpers and removing the outdated comment.
### 1. Extract helper for prefetched versions in `get_indicator_column`
Right now the nested `indicator` function is doing:
- grouper vs content branching
- iterating `_prefetched_contents`
- mutating `_prefetched_versions[0].content`
- building `versions` list
You can push that into a small helper that encapsulates the private-attr knowledge and mutation:
```python
def _get_prefetched_versions_for_grouper(self, grouper_obj):
"""Return list of latest versions from prefetched contents, or None."""
prefetched_contents = getattr(grouper_obj, "_prefetched_contents", None)
if not prefetched_contents:
return None
for content in prefetched_contents:
# Avoid fetching reverse by wiring the content onto the prefetched version
if getattr(content, "_prefetched_versions", None):
content._prefetched_versions[0].content = content
return [content._prefetched_versions[0] for content in prefetched_contents]
```
Then `indicator` becomes simpler and more focused on “indicator” logic:
```python
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},
)
versions = self._get_prefetched_versions_for_grouper(obj)
else: # Content Model
content_obj = obj
status = content_indicator(content_obj, versions)
menu = (
content_indicator_menu(
request,
status,
content_obj._versions,
back=f"{request.path_info}?{request.GET.urlencode()}",
)
if status
else None
)
...
```
This keeps the current behaviour (including the `.content` patch) but isolates it.
### 2. Extract helper for building content/versions `Prefetch`
You repeat the same queryset shape twice in `get_queryset`. You can centralise that:
```python
def _build_content_prefetch(self, to_attr, base_qs):
return Prefetch(
self.content_model._meta.get_field(self.grouper_field_name)
.remote_field
.get_accessor_name(),
to_attr=to_attr,
queryset=(
base_qs
.prefetch_related(Prefetch("versions", to_attr="_prefetched_versions"))
.order_by("-pk")
),
)
```
Then `get_queryset` can be reduced to:
```python
def get_queryset(self, request):
qs = super().get_queryset(request)
...
base_qs = self.content_model.admin_manager.filter(**self.current_content_filters)
current_qs = self.content_model.admin_manager.current_content(**self.current_content_filters)
qs = qs.prefetch_related(
self._build_content_prefetch("_prefetched_contents", base_qs)
)
qs = qs.prefetch_related(
self._build_content_prefetch("_current_contents", current_qs)
)
return qs
```
You can then safely remove or replace the now-misleading comment about prefetching not being implemented.
### 3. Extract helper for resolving latest prefetched content
To keep `get_content_obj` high-level, move the `_prefetched_contents` concern into a helper:
```python
def _get_latest_prefetched_content(self, obj):
if not hasattr(obj, "_prefetched_contents"):
return None
return get_latest_content_from_cache(
obj._prefetched_contents,
include_unpublished_archived=True,
)
```
Then `get_content_obj` reads as a simple decision point:
```python
def get_content_obj(self, obj: models.Model) -> models.Model:
if obj is None or self._is_content_obj(obj):
return super().get_content_obj(obj)
latest = self._get_latest_prefetched_content(obj)
if latest is not None:
return latest
return super().get_content_obj(obj)
```
This preserves the fast-path from prefetched data but localises the private-attr/caching knowledge in one helper.
</issue_to_address>
### Comment 4
<location> `tests/test_admin.py:3418-3426` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 5
<location> `tests/test_admin.py:3455-3460` </location>
<code_context>
</code_context>
<issue_to_address>
**issue (code-quality):** Avoid loops in tests. ([`no-loop-in-tests`](https://docs.sourcery.ai/Reference/Rules-and-In-Line-Suggestions/Python/Default-Rules/no-loop-in-tests))
<details><summary>Explanation</summary>Avoid complex code, like loops, in test functions.
Google's software engineering guidelines says:
"Clear tests are trivially correct upon inspection"
To reach that avoid complex code in tests:
* loops
* conditionals
Some ways to fix this:
* Use parametrized tests to get rid of the loop.
* Move the complex logic into helpers.
* Move the complex part into pytest fixtures.
> Complexity is most often introduced in the form of logic. Logic is defined via the imperative parts of programming languages such as operators, loops, and conditionals. When a piece of code contains logic, you need to do a bit of mental computation to determine its result instead of just reading it off of the screen. It doesn't take much logic to make a test more difficult to reason about.
Software Engineering at Google / [Don't Put Logic in Tests](https://abseil.io/resources/swe-book/html/ch12.html#donapostrophet_put_logic_in_tests)
</details>
</issue_to_address>
### Comment 6
<location> `djangocms_versioning/indicators.py:116` </location>
<code_context>
def content_indicator(
content_obj: models.Model,
versions: list[Version] | None = None
) -> str | None:
"""Translates available versions into status to be reflected by the indicator.
Function caches the result in the page_content object"""
if not content_obj:
return None # pragma: no cover
elif not hasattr(content_obj, "_indicator_status"):
if versions is None:
# Get all versions for the content object if not available
versions = Version.objects.filter_by_content_grouping_values(
content_obj
).order_by("-pk")
version_states = dict(VERSION_STATES)
signature = {
version.state: version
for version in versions if version.state in version_states
}
if DRAFT in signature and PUBLISHED not in signature:
content_obj._indicator_status = "draft"
content_obj._versions = signature[DRAFT],
elif DRAFT in signature and PUBLISHED in signature:
content_obj._indicator_status = "dirty"
content_obj._versions = (signature[DRAFT], signature[PUBLISHED])
elif PUBLISHED in signature:
content_obj._indicator_status = "published"
content_obj._versions = signature[PUBLISHED],
elif versions[0].state == UNPUBLISHED:
content_obj._indicator_status = "unpublished"
content_obj._versions = signature[UNPUBLISHED],
elif versions[0].state == ARCHIVED:
content_obj._indicator_status = "archived"
content_obj._versions = signature[ARCHIVED],
else: # pragma: no cover
content_obj._indicator_status = None
content_obj._versions = [None]
return content_obj._indicator_status
</code_context>
<issue_to_address>
**suggestion (code-quality):** Remove redundant conditional ([`remove-redundant-if`](https://docs.sourcery.ai/Reference/Default-Rules/refactorings/remove-redundant-if/))
```suggestion
elif DRAFT in signature:
```
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
| # 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( |
Contributor
There was a problem hiding this comment.
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:
get_content_objcurrently exists exactly as shown in the SEARCH block and thatget_latest_content_from_cacheaccepts a singleobjparameter (withinclude_unpublished_archivedeither optional or defaulting appropriately).- The queryset you showed is on the admin changelist for the same model whose instances are passed to
get_content_obj, and that_current_contentsis theto_attrused in the newPrefetch(...).
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 keepinclude_unpublished_archived=Trueif that is the intended behavior. - Ensure the
Prefetch(... to_attr="_current_contents")remains aligned with the attribute name used inget_content_obj.
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #505 +/- ##
==========================================
+ Coverage 90.55% 93.53% +2.97%
==========================================
Files 72 76 +4
Lines 2732 2736 +4
Branches 322 0 -322
==========================================
+ Hits 2474 2559 +85
+ Misses 182 177 -5
+ Partials 76 0 -76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Contributor
There was a problem hiding this comment.
Hey - I've found 1 issue, and left some high level feedback:
- Several new helpers rely on private, prefetch-specific attributes (e.g.
_prefetched_contents,_prefetched_versions) and index[_prefetched_versions[0]]without guards; consider adding defensive checks or fallbacks to avoid fragile failures when there are no versions or prefetching is bypassed. - In
GrouperAdmin.get_queryset, you prefetch the same reverse relation twice with differentto_attrvalues, butget_content_objonly reads_prefetched_contents; if_current_contentsis not used elsewhere, you can drop that second prefetch to simplify the queryset and reduce work. - The mutation
prefetched._prefetched_versions[0].content = prefetchedinsideget_indicator_columncreates hidden coupling between cached versions and their content; it may be more robust to rely on the actual relationship (orselect_related) instead of patching attributes on the fly.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Several new helpers rely on private, prefetch-specific attributes (e.g. `_prefetched_contents`, `_prefetched_versions`) and index `[_prefetched_versions[0]]` without guards; consider adding defensive checks or fallbacks to avoid fragile failures when there are no versions or prefetching is bypassed.
- In `GrouperAdmin.get_queryset`, you prefetch the same reverse relation twice with different `to_attr` values, but `get_content_obj` only reads `_prefetched_contents`; if `_current_contents` is not used elsewhere, you can drop that second prefetch to simplify the queryset and reduce work.
- The mutation `prefetched._prefetched_versions[0].content = prefetched` inside `get_indicator_column` creates hidden coupling between cached versions and their content; it may be more robust to rely on the actual relationship (or `select_related`) instead of patching attributes on the fly.
## Individual Comments
### Comment 1
<location> `djangocms_versioning/helpers.py:365-366` </location>
<code_context>
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
+ )
</code_context>
<issue_to_address>
**issue (bug_risk):** Prefetched-path lookup ignores extra grouping fields, which can return content for the wrong language or group.
In the non-prefetched path you filter by `extra_grouping_fields` (language, site, etc.) before selecting content, but when `_prefetched_contents` is present you just return the first draft/published/archived item from the cache without those filters.
If `_prefetched_contents` can contain multiple languages or other groupings, this may return the wrong item. You could either:
- pass `extra_grouping_fields` into `get_latest_content_from_cache` and filter there, or
- restrict prefetched contents so they only include the relevant grouping.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
Comment on lines
+365
to
+366
| if hasattr(grouper, "_prefetched_contents"): | ||
| return get_latest_content_from_cache( |
Contributor
There was a problem hiding this comment.
issue (bug_risk): Prefetched-path lookup ignores extra grouping fields, which can return content for the wrong language or group.
In the non-prefetched path you filter by extra_grouping_fields (language, site, etc.) before selecting content, but when _prefetched_contents is present you just return the first draft/published/archived item from the cache without those filters.
If _prefetched_contents can contain multiple languages or other groupings, this may return the wrong item. You could either:
- pass
extra_grouping_fieldsintoget_latest_content_from_cacheand filter there, or - restrict prefetched contents so they only include the relevant grouping.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Description
Improve versioned content admin changelist performance by caching and prefetching related content and versions while preserving existing behaviour.
Enhancements:
Tests:
Related resources
Checklist
masterSlack to find a “pr review buddy” who is going to review my pull request.
Summary by Sourcery
Optimize versioned content admin changelist performance and caching while preserving existing behavior and permissions.
Bug Fixes:
Enhancements:
Tests: