Open
Conversation
Contributor
Reviewer's GuideRefactors djangocms_alias to use the djangocms-versioning contract API and to be more defensive when versioning is absent, including wiring versioning configuration via the CMS app config constructor, introducing a contract-based helper to obtain the VersionableItem, relaxing the versioning-enabled detection, and making Alias name/content resolution robust to missing content or versioning imports. Sequence diagram for djangocms_alias versioning contract resolutionsequenceDiagram
actor DjangoAppRegistry
participant AliasCMSConfig
participant Utils
participant VersioningContract as VersioningContractProvider
participant VersioningLib as DjangocmsVersioning
DjangoAppRegistry->>AliasCMSConfig: __init__(app_config)
activate AliasCMSConfig
AliasCMSConfig->>Utils: get_versionable_item(cms_config)
activate Utils
alt cms_config provides contract API
Utils->>AliasCMSConfig: has attribute get_contract
Utils->>VersioningContract: get_contract(djangocms_versioning)
VersioningContract-->>Utils: VersionableItem or null
else no contract API
Utils-->>AliasCMSConfig: None
end
Utils-->>AliasCMSConfig: VersionableItem or None
deactivate Utils
AliasCMSConfig->>AliasCMSConfig: set djangocms_moderation_enabled
AliasCMSConfig->>AliasCMSConfig: set djangocms_versioning_enabled
alt djangocms_versioning_enabled and VersionableItem is null
AliasCMSConfig->>VersioningLib: import __version__, VersionableItem
VersioningLib-->>AliasCMSConfig: __version__, VersionableItem
AliasCMSConfig->>AliasCMSConfig: validate djangocms_versioning_version >= 2.4
end
alt versioning is available
AliasCMSConfig->>AliasCMSConfig: build VersionableItem config for AliasContent
AliasCMSConfig->>AliasCMSConfig: assign versioning list
end
deactivate AliasCMSConfig
Class diagram for updated djangocms_alias configuration and modelsclassDiagram
class CMSAppConfig {
}
class AliasCMSConfig {
+bool cms_enabled
+list moderated_models
+list cms_wizards
+bool djangocms_moderation_enabled
+bool djangocms_versioning_enabled
+list versioning
+__init__(app_config)
}
class Alias {
+dict _content_cache
+get_name(language)
+get_content(language, show_draft_content)
+get_placeholder(language, show_draft_content)
}
class AliasContent {
}
class VersionableItem {
+model content_model
+str grouper_field_name
+list extra_grouping_fields
+dict version_list_filter_lookups
+callable copy_function
+callable grouper_selector_option_label
+str grouper_admin_mixin
}
class Utils {
+get_versionable_item(cms_config)
+is_versioning_enabled()
+emit_content_change(objs, sender)
}
CMSAppConfig <|-- AliasCMSConfig
AliasCMSConfig ..> Utils : uses
AliasCMSConfig ..> VersionableItem : configures
VersionableItem ..> AliasContent : versions
Alias o--> AliasContent : contents
Alias ..> Utils : uses (indirectly via versioning)
File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Hey - I've found 3 issues, and left some high level feedback:
- The new
AliasCMSConfig.__init__overrides the base implementation but never callssuper().__init__(app_config), which risks leaving the underlyingCMSAppConfiguninitialized and may break other cms app config features. get_versionable_itemis decorated with@cachebut takes acms_configinstance argument, which is likely unhashable and will cause aTypeError; consider removing the cache or changing the API to use a hashable key (e.g. app label) instead.- The revised
is_versioning_enablednow iterates all app configs, returns on the first withcms_extension, and includes aprint(app_config)debug statement; this changes semantics from checking thedjangocms_versioningapp specifically and may give incorrect results and noisy output—restrict the lookup to the versioning app and remove the print.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `AliasCMSConfig.__init__` overrides the base implementation but never calls `super().__init__(app_config)`, which risks leaving the underlying `CMSAppConfig` uninitialized and may break other cms app config features.
- `get_versionable_item` is decorated with `@cache` but takes a `cms_config` instance argument, which is likely unhashable and will cause a `TypeError`; consider removing the cache or changing the API to use a hashable key (e.g. app label) instead.
- The revised `is_versioning_enabled` now iterates all app configs, returns on the first with `cms_extension`, and includes a `print(app_config)` debug statement; this changes semantics from checking the `djangocms_versioning` app specifically and may give incorrect results and noisy output—restrict the lookup to the versioning app and remove the print.
## Individual Comments
### Comment 1
<location> `djangocms_alias/cms_config.py:25-34` </location>
<code_context>
+ def __init__(self, app_config):
</code_context>
<issue_to_address>
**issue (bug_risk):** Using an overridden __init__ and instance attributes for CMS config is likely to break Django CMS/CMSAppConfig integration.
Django CMS expects app config options to be defined as class attributes and introspected without instantiating the class. A custom `__init__` that doesn’t match the standard `__init__(self, *args, **kwargs)` and doesn’t call `super().__init__` can break its initialization flow, and using `self.versioning` instead of a class-level `versioning` means the versioning extension may never see the config. Please move `djangocms_moderation_enabled`, `djangocms_versioning_enabled`, and `versioning` back to class attributes (optionally computed via helpers), or at minimum restore the standard `__init__` signature and call `super().__init__`.
</issue_to_address>
### Comment 2
<location> `djangocms_alias/cms_config.py:26-35` </location>
<code_context>
+ VersionableItem = get_versionable_item(self)
</code_context>
<issue_to_address>
**suggestion:** The `VersionableItem` name is reused for two different concepts, which makes the control flow hard to reason about and easy to break.
Here `VersionableItem` is first bound to the result of `get_versionable_item(self)` (an instance or `None`), and later reused for the imported `VersionableItem` class. This name shadowing is brittle (e.g. if a false-y but non-`None` value is ever returned) and hard to follow. Use distinct names (e.g. `versionable_item = get_versionable_item(self)`) and import the `VersionableItem` class under its own name if needed, without relying on truthiness for control flow.
Suggested implementation:
```python
def __init__(self, app_config):
versionable_item = get_versionable_item(self)
self.djangocms_moderation_enabled = getattr(settings, "MODERATING_ALIAS_MODELS_ENABLED", True)
self.djangocms_versioning_enabled = getattr(
settings, "VERSIONING_ALIAS_MODELS_ENABLED", versionable_item is not None
)
if self.djangocms_versioning_enabled:
```
1. Ensure that elsewhere in this file, the `VersionableItem` class is imported and referenced only as a class (e.g. from `djangocms_versioning.helpers import VersionableItem`), and not reused as a local variable name.
2. If there are later references in this method that were relying on the local `VersionableItem` binding (e.g. `if VersionableItem:` or passing it around), update them to use `versionable_item` instead, and keep the `VersionableItem` name reserved for the class only.
</issue_to_address>
### Comment 3
<location> `djangocms_alias/utils.py:6-9` </location>
<code_context>
-def is_versioning_enabled():
+@cache
+def get_versionable_item(cms_config) -> type | None:
+ if hasattr(cms_config, "get_contract"):
+ return cms_config.get_contract("djangocms_versioning")
+ return None
+
+
+@cache
+def is_versioning_enabled() -> bool:
from .models import AliasContent
- try:
- app_config = apps.get_app_config("djangocms_versioning")
- return app_config.cms_extension.is_content_model_versioned(AliasContent)
- except LookupError:
- return False
</code_context>
<issue_to_address>
**issue (bug_risk):** New `is_versioning_enabled` logic can return the wrong result and includes a stray `print`.
The loop will return on the first `app_config` with a `cms_extension`, even if it’s not `djangocms_versioning`, so the result can be wrong when another CMS app is encountered first. `print(app_config)` will also pollute stdout in production. Please either restore the explicit `apps.get_app_config("djangocms_versioning")` logic or ensure you filter by `app_config.label`/`name` before calling `cms_extension.is_content_model_versioned`, and remove the `print`.
</issue_to_address>Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
fsbraun
commented
Feb 4, 2026
fsbraun
commented
Feb 4, 2026
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
Add support for the djangocms-versioning contract and make alias versioning integration more robust and configuration-driven.
New Features:
Bug Fixes:
Enhancements:
Related resources
Checklist