fix: Show fallback languages if changelist view#334
Open
Conversation
Contributor
Reviewer's guide (collapsed on small PRs)Reviewer's GuideAdjust alias content resolution to better handle missing languages in the changelist view and make alias naming resilient when versioning is absent or content lacks versions, while caching contents per language for multi-language lookups. Class diagram for updated Alias content and naming logicclassDiagram
class Alias {
+int pk
+dict _content_cache
+get_name(language)
+get_content(language, show_draft_content)
+get_placeholder(language, show_draft_content)
}
class AliasContent {
+int id
+string language
+string name
+versions
}
class Version {
+int id
+string state
}
class DjangocmsVersioningConstants {
<<module>>
+string DRAFT
}
Alias "1" o-- "*" AliasContent : contents
AliasContent "1" o-- "*" Version : versions
Alias ..> DjangocmsVersioningConstants : uses
%% Method behavior notes encoded via additional pseudo methods
Alias : get_name(language)
Alias : 1. content = get_content(language, show_draft_content=True)
Alias : 2. if no content use first _content_cache value as fallback
Alias : 3. name = content.name or Alias_pk_No_content
Alias : 4. try import DRAFT from djangocms_versioning.constants
Alias : 5. if version exists and version.state == DRAFT
Alias : return name_Not_published
Alias : 6. on ImportError ModuleNotFoundError AttributeError ignore
Alias : 7. return name
Alias : get_content(language, show_draft_content)
Alias : A. if language in _content_cache return cached
Alias : B. else build qs via admin_manager.latest_content or all
Alias : C. iterate qs and cache first content per language
Alias : D. return _content_cache.get(language)
File-Level Changes
Assessment against linked issues
Possibly linked issues
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}
| if version.state == DRAFT: | ||
| return f"{name} (Not published)" | ||
|
|
||
| except (ImportError, ModuleNotFoundError, AttributeError): |
Check notice
Code scanning / CodeQL
Empty except Note
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
This autofix suggestion was applied.
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI about 24 hours ago
In general, empty except blocks should be replaced with explicit, documented handling: narrow the exceptions where possible, add comments explaining why they are safely ignored, and/or log unexpected issues. Here, the goal is to annotate alias names as “(Not published)” when draft versioning is available, and otherwise silently fall back to the plain name. We can preserve that behavior while removing the “empty except” smell by (1) guarding against content being None before entering the try, and (2) adding a short explanatory comment in the except block to clarify that missing versioning or versions is expected and intentionally ignored.
Concretely, in Alias.get_name in djangocms_alias/models.py, we will:
- Add an early
if content is None: return nameto avoidAttributeErrorfromcontent.versions. - Keep the
tryimportingDRAFTand accessingcontent.versions.first(). - Keep catching the same exceptions, but add a brief comment explaining that the project may not have versioning installed or the content may not be versioned, and that in those cases we intentionally fall back to the base name.
This removes the “empty except” issue while keeping the same observable behavior (plainnamewhen versioning is unavailable or when any of the anticipated exceptions is raised).
No new imports or helper methods are required.
Suggested changeset
1
djangocms_alias/models.py
| @@ -160,15 +160,18 @@ | ||
| if not content: | ||
| content = next(iter(self._content_cache.values()), None) | ||
| name = getattr(content, "name", f"Alias {self.pk} (No content)") | ||
| if content is None: | ||
| return name | ||
| try: | ||
| from djangocms_versioning.constants import DRAFT | ||
|
|
||
| version = content.versions.first() | ||
|
|
||
| if version.state == DRAFT: | ||
| if version and version.state == DRAFT: | ||
| return f"{name} (Not published)" | ||
| except (ImportError, ModuleNotFoundError, AttributeError): | ||
| pass | ||
| # Versioning is not installed or content is not versioned; fall back to the base name. | ||
| return name | ||
| return name | ||
|
|
||
| def get_content(self, language=None, show_draft_content=False): |
Copilot is powered by AI and may make mistakes. Always verify output.
fsbraun
committed this autofix suggestion about 24 hours ago.
![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 1 issue, and left some high level feedback:
- The new
get_contentimplementation populates_content_cachewithout considering theshow_draft_contentflag, so once the cache is filled by a call with one flag value it will be reused for subsequent calls with the other flag value, which may return inconsistent draft/published content; consider scoping the cache byshow_draft_contentor avoiding cross-flag reuse. - When populating
_content_cacheinget_content, usingsetdefaulton the unfiltered queryset relies on its implicit ordering to decide which content per language is cached; to make the behavior deterministic and closer to the previous.first()semantics, it would be safer to add an explicit ordering (e.g. by-pkor-created) or to reuselatest_content()for both branches. - In
get_name, falling back tonext(iter(self._content_cache.values()), None)may pick an arbitrary language rather than a predictable one; if possible, prefer a specific fallback strategy (e.g. the current active language, default language, or a defined priority list) to avoid surprising alias names in multi-language setups.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- The new `get_content` implementation populates `_content_cache` without considering the `show_draft_content` flag, so once the cache is filled by a call with one flag value it will be reused for subsequent calls with the other flag value, which may return inconsistent draft/published content; consider scoping the cache by `show_draft_content` or avoiding cross-flag reuse.
- When populating `_content_cache` in `get_content`, using `setdefault` on the unfiltered queryset relies on its implicit ordering to decide which content per language is cached; to make the behavior deterministic and closer to the previous `.first()` semantics, it would be safer to add an explicit ordering (e.g. by `-pk` or `-created`) or to reuse `latest_content()` for both branches.
- In `get_name`, falling back to `next(iter(self._content_cache.values()), None)` may pick an arbitrary language rather than a predictable one; if possible, prefer a specific fallback strategy (e.g. the current active language, default language, or a defined priority list) to avoid surprising alias names in multi-language setups.
## Individual Comments
### Comment 1
<location> `djangocms_alias/models.py:174` </location>
<code_context>
-
- self._content_cache[language] = qs.first()
- return self._content_cache[language]
+ for content in qs:
+ self._content_cache.setdefault(content.language, content)
+ return self._content_cache.get(language)
def get_placeholder(self, language=None, show_draft_content=False):
</code_context>
<issue_to_address>
**suggestion (performance):** Missing negative caching for languages with no content can cause repeated DB hits.
With the new loop, when `qs` has no items for a `language`, nothing is stored in `_content_cache` for that key, so every `get_content(language)` call re-queries the DB. To preserve negative caching, also cache the no-content case, e.g. by setting `self._content_cache.setdefault(language, None)` when `language` is not present in `qs`.
```suggestion
def get_content(self, language=None, show_draft_content=False):
qs = self.contents(manager="admin_manager").latest_content()
else:
qs = self.contents.all()
for content in qs:
self._content_cache.setdefault(content.language, content)
if language not in self._content_cache:
# Negative cache: remember that this language has no content
self._content_cache[language] = None
return self._content_cache.get(language)
```
</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
force-pushed
the
fix/no-content
branch
from
dcf0cef to
4cb0230
Compare
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 alias content resolution and naming when a requested language or versioning integration is unavailable.
Bug Fixes:
Enhancements:
fixes bug: aliases show "missing language" #331
Related resources
Checklist