-
Notifications
You must be signed in to change notification settings - Fork 34
feat: Changelist performance improvement #505
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: master
Are you sure you want to change the base?
Changes from all commits
954c10d
060ea5b
90d8bc0
f03f4f5
118d26b
3847042
6b3ca0d
3f2b4aa
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
|
|
@@ -362,6 +362,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( | ||
|
Comment on lines
+365
to
+366
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 If
|
||
| 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() | ||
|
|
||
|
|
@@ -375,6 +380,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 | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -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 | ||
There was a problem hiding this comment.
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_objwith the new_current_contentsprefetch.The queryset now prefetches the reverse relation twice: into
_prefetched_contents(alladmin_managercontents) and_current_contents(admin_manager.current_content), butget_content_objstill only uses_prefetched_contentsviaget_latest_content_from_cache(..., include_unpublished_archived=True). This both negates the benefit of the more selective_current_contentsprefetch 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_contentsinstead, or otherwise make the selection between_prefetched_contentsand_current_contentsexplicit and intentional.Suggested implementation:
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).get_content_obj, and that_current_contentsis theto_attrused in the newPrefetch(...).If your existing
get_content_objorget_latest_content_from_cachesignatures differ (for example, ifget_latest_content_from_cacherequires the admin instance or the content model, or ifinclude_unpublished_archivedmust be passed explicitly), you should:return get_latest_content_from_cache(self, obj)or keepinclude_unpublished_archived=Trueif that is the intended behavior.Prefetch(... to_attr="_current_contents")remains aligned with the attribute name used inget_content_obj.