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
Refactor get_title_cache to be straightforward #6829
Changes from all commits
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 |
---|---|---|
|
@@ -902,32 +902,34 @@ def get_redirect(self, language=None, fallback=True, force_reload=False): | |
return self.get_page_content_obj_attribute("redirect", language, fallback, force_reload) | ||
|
||
def _get_title_cache(self, language, fallback, force_reload): | ||
if not language: | ||
language = get_language() | ||
|
||
force_reload = (force_reload or language not in self.title_cache) | ||
|
||
if fallback and not self.title_cache.get(language): | ||
# language can be in the cache but might be an EmptyPageContent instance | ||
def get_fallback_language(page, language): | ||
fallback_langs = i18n.get_fallback_languages(language) | ||
for lang in fallback_langs: | ||
if self.title_cache.get(lang): | ||
if page.title_cache.get(lang): | ||
return lang | ||
|
||
if not language: | ||
language = get_language() | ||
|
||
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. get_fallback_language would have been called here if fallback is true and would have run before the force_reload, this could have unforeseen side affects. I can see that this condition is ran below with the use_fallbacks option, I'm not sure how that could affect being run later after the force reload. 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. You are correct, you'll see the effects of this in the test that I provided. If you ran the test prior to this patch the test would fail. With this patch, the test passes. |
||
force_reload = (force_reload or language not in self.title_cache) | ||
if force_reload: | ||
from cms.models import PageContent | ||
|
||
titles = PageContent.objects.filter(page=self) | ||
for title in titles: | ||
self.title_cache[title.language] = title | ||
if self.title_cache.get(language): | ||
return language | ||
else: | ||
if fallback: | ||
fallback_langs = i18n.get_fallback_languages(language) | ||
for lang in fallback_langs: | ||
if self.title_cache.get(lang): | ||
return lang | ||
|
||
if self.title_cache.get(language): | ||
return language | ||
|
||
|
||
use_fallback = all([ | ||
fallback, | ||
not self.title_cache.get(language), | ||
get_fallback_language(self, language) | ||
]) | ||
if use_fallback: | ||
# language can be in the cache but might be an EmptyPageContent instance | ||
return get_fallback_language(self, language) | ||
return language | ||
|
||
@property | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -922,3 +922,19 @@ def test_move_node(self): | |
child.move_page(parent.node) | ||
child = child.reload() | ||
self.assertEqual(child.get_absolute_url(language='en'), '/en/parent/child/') | ||
|
||
|
||
class PageContentTests(CMSTestCase): | ||
|
||
def setUp(self): | ||
self.page = create_page("english-page", "nav_playground.html", "en") | ||
self.german_content = create_title("de", "german content", self.page) | ||
self.english_content = self.page.get_title_obj('en') | ||
|
||
def test_get_title_obj(self): | ||
""" | ||
Cache partially populated, if no hit it should try to populate it | ||
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. There is a test cache test file, this should live in there. I thought that we had a test_title file, seems that we don't. |
||
""" | ||
del self.page.title_cache['de'] | ||
german_content = self.page.get_title_obj('de') | ||
self.assertEqual(german_content.pk, self.german_content.pk) |
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.
makes sense, maybe as this is private it should be called: _get_fallback_language
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.
Was this created to attack it from versioning ?
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.
No, this method was made to reduce clutter. The same code was repeated twice. Also having a name
get_fallback_language
makes the code easier to parse than the block of code that does not have a name.