feat: Changelist performance improvement #505
Open
Conversation
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
Contributor
Reviewer's GuideOptimizes the Versioning admin changelist for grouped content by adding targeted prefetching and cached version resolution, updates indicator state handling to work with multiple versions, and pins the query count with new admin tests while fixing a permission check bug in the Version model. Sequence diagram for optimized admin changelist indicator resolutionsequenceDiagram
actor AdminUser
participant DjangoAdmin as DjangoAdminSite
participant VersionAdmin as VersioningAdmin
participant Grouper as GrouperModel
participant Content as ContentModel
participant Version as VersionModel
participant Helpers as helpers
participant Indicators as indicators
AdminUser->>DjangoAdmin: Request grouper changelist
DjangoAdmin->>VersionAdmin: get_queryset(request)
activate VersionAdmin
VersionAdmin->>VersionAdmin: build base queryset with annotations
VersionAdmin->>VersionAdmin: reverse_name = content_model._meta.get_field(grouper_field_name).remote_field.get_accessor_name()
VersionAdmin->>Grouper: prefetch_related(reverse_name as _prefetched_contents)
note over Grouper,Content: Prefetch contents with admin_manager.filter(...) and versions as _prefetched_versions
VersionAdmin->>Grouper: prefetch_related(reverse_name as _current_contents)
VersionAdmin-->>DjangoAdmin: queryset with prefetched contents and versions
deactivate VersionAdmin
loop For each Grouper in changelist
DjangoAdmin->>VersionAdmin: get_indicator_column(request)
activate VersionAdmin
VersionAdmin->>VersionAdmin: indicator(obj)
alt Grouper model
VersionAdmin->>Helpers: get_latest_admin_viewable_content(grouper=obj, include_unpublished_archived=True, extra_grouping_fields)
activate Helpers
alt Prefetched contents available
Helpers->>Helpers: get_latest_content_from_cache(obj._prefetched_contents, include_unpublished_archived=True)
Helpers-->>VersionAdmin: content_obj
else No prefetched contents
Helpers-->>VersionAdmin: content_obj from database queryset
end
deactivate Helpers
VersionAdmin->>VersionAdmin: versions = [content._prefetched_versions[0] for content in obj._prefetched_contents]
else Content model
VersionAdmin->>VersionAdmin: content_obj = obj
VersionAdmin->>VersionAdmin: versions = None
end
VersionAdmin->>Indicators: content_indicator(content_obj, versions)
activate Indicators
Indicators->>Indicators: build signature {state: version}
Indicators->>Indicators: set content_obj._indicator_status
Indicators->>Indicators: set content_obj._versions
Indicators-->>VersionAdmin: status
deactivate Indicators
alt status is truthy
VersionAdmin->>Indicators: content_indicator_menu(request, status, content_obj._versions, back)
Indicators-->>VersionAdmin: menu
else
VersionAdmin->>VersionAdmin: menu = None
end
VersionAdmin-->>DjangoAdmin: HTML for indicator column
deactivate VersionAdmin
end
Class diagram for updated versioning and indicator structuresclassDiagram
class GrouperModel {
<<abstract>>
+_prefetched_contents list~ContentModel~
+_current_contents list~ContentModel~
}
class ContentModel {
<<abstract>>
+_prefetched_versions list~VersionModel~
+_indicator_status str
+_versions tuple~VersionModel~
}
class VersionModel {
+content ContentModel
+state int
+created datetime
+_has_permission(perm str, user User) bool
}
class VersioningAdmin {
+content_model ContentModel
+grouper_field_name str
+current_content_filters dict
+get_queryset(request HttpRequest) QuerySet
+get_indicator_column(request HttpRequest) function
+get_content_obj(obj Model) Model
}
class Helpers {
+get_latest_admin_viewable_content(grouper GrouperModel, include_unpublished_archived bool, extra_grouping_fields dict) ContentModel
+get_latest_content_from_cache(cache list~ContentModel~, include_unpublished_archived bool) ContentModel
}
class Indicators {
+content_indicator(content_obj ContentModel, versions list~VersionModel~) str
+content_indicator_menu(request HttpRequest, status str, versions tuple~VersionModel~, back str) Any
}
class CMSConfigAdapter {
+get_indicator_menu(request HttpRequest, page_content ContentModel) tuple
}
VersioningAdmin --> ContentModel : uses
VersioningAdmin --> GrouperModel : manages
GrouperModel "1" -- "*" ContentModel : groups
ContentModel "1" -- "*" VersionModel : has versions
Helpers ..> GrouperModel : processes
Helpers ..> ContentModel : returns
Indicators ..> ContentModel : updates
Indicators ..> VersionModel : reads state
CMSConfigAdapter ..> Indicators : delegates
VersionModel ..> ContentModel : references
VersioningAdmin ..> Helpers : calls
VersioningAdmin ..> Indicators : calls
Flow diagram for get_latest_content_from_cache helperflowchart TD
A[Start get_latest_content_from_cache] --> B{cache is empty?}
B -->|Yes| C[Return None]
B -->|No| D[Iterate cache and find first content with version state == DRAFT]
D --> E{Found draft?}
E -->|Yes| F[Return draft content]
E -->|No| G[Iterate cache and find first content with version state == PUBLISHED]
G --> H{Found published?}
H -->|Yes| I[Return published content]
H -->|No| J{include_unpublished_archived is True?}
J -->|No| C
J -->|Yes| K[Sort cache by version.created descending]
K --> L{Any content after sort?}
L -->|Yes| M[Return first content - most recent]
L -->|No| C
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.47% +2.91%
==========================================
Files 72 76 +4
Lines 2732 2728 -4
Branches 322 0 -322
==========================================
+ Hits 2474 2550 +76
+ Misses 182 178 -4
+ Partials 76 0 -76 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.