Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Fix #985, typos in comments #991

Closed
wants to merge 1 commit into from

2 participants

@brente

Fixed #985 and added a test

I think it would be safer to also add a filter on the site, at least for root nodes (non-root nodes rely on the parent).

Also, I think there's something wrong with this test (in Page._publisher_save_public):

            not (self.level > 0 and self.parent.publisher_public == self.old_public.parent)

This is always true for root nodes, so the branch for moved nodes is always taken. I didn't touch this, though, because the final result seems to be the same if a root node is modified but not moved.

Finally, why not include filters on node status (and site) as defaults in get_*_filtered_sibling?
After all we're looking for the next/previous sibling among nodes of the same kind (draft or public).
I can't think of a case where we might need to find a sibling for a public node including drafts (or nodes from a different site).

@ojii
Collaborator

thanks for the pull request. For the future please do not mix cleanup (comments/codestyle) and actual fixes in a single pull request, since that just makes it a lot harder for us to review. Instead, please send two pull requests (one with the fix, one with cleanup).

@ojii
Collaborator

Didn't you say in #985 that get_previous_filtered_siblings should also filter by site?

@brente
@ojii
Collaborator

If you hop on IRC (#django-cms on freenode) I might be able to help you with that test

@ojii
Collaborator

the test you added doesn't actually seem to test what this issue is about...

@brente

Why not?
I opened #985 because the home page was losing its status when the user updated it, which is exactly what PagesTestCase.test_public_home_page_replaced tests.
Probably it's too specific, and I agree that other test cases are missing (e.g. multi-site, non-root nodes with unpublished siblings, ...). I hope I'll be able to include them in the next submission.

@ojii
Collaborator

I've written a test and fix for it now, nvm

@brente

OK.
Thank you.

@ojii ojii closed this pull request from a commit
@ojii ojii Added a fix for #985
Fixes #985
Fixes #991
f573a24
@ojii ojii closed this in f573a24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Sep 3, 2011
  1. @brente

    Fix #985, typos in comments

    brente authored
This page is out of date. Refresh to see the latest.
Showing with 54 additions and 37 deletions.
  1. +30 −28 cms/models/pagemodel.py
  2. +23 −8 cms/tests/page.py
  3. +1 −1  cms/tests/publisher.py
View
58 cms/models/pagemodel.py
@@ -154,7 +154,7 @@ def copy_page(self, target, site, position='first-child',
public_copy=False):
"""
copy a page [ and all its descendants to a new location ]
- Doesn't checks for add page permissions anymore, this is done in PageAdmin.
+ Doesn't check for add page permissions anymore, this is done in PageAdmin.
Note: public_copy was added in order to enable the creation of a copy for creating the public page during
the publish operation as it sets the publisher_is_draft=False.
@@ -166,7 +166,7 @@ def copy_page(self, target, site, position='first-child',
if public_copy:
# create a copy of the draft page - existing code loops through pages so added it to a list
- pages = [copy.copy(self)]
+ pages = [copy.copy(self)]
else:
pages = [self] + list(self.get_descendants().order_by('-rght'))
@@ -331,9 +331,9 @@ def save(self, no_signals=False, change_state=True, commit=True,
elif change_state:
self.moderator_state = Page.MODERATOR_CHANGED
- #publish_directly = True - no publisher, no publishing!! - we just
- # use draft models in this case
-
+ # publish_directly = True - no publisher, no publishing!!
+ # we just use draft models in this case
+
if force_state is not None:
self.moderator_state = force_state
@@ -367,7 +367,7 @@ def save(self, no_signals=False, change_state=True, commit=True,
self.publish()
def save_base(self, *args, **kwargs):
- """Overriden save_base. If an instance is draft, and was changed, mark
+ """Overridden save_base. If an instance is draft, and was changed, mark
it as dirty.
Dirty flag is used for changed nodes identification when publish method
@@ -386,7 +386,7 @@ def save_base(self, *args, **kwargs):
def publish(self):
"""Overrides Publisher method, because there may be some descendants, which
- are waiting for parent to publish, so publish them if possible.
+ are waiting for parent to publish, so publish them if possible.
IMPORTANT: @See utils.moderator.approve_page for publishing permissions
@@ -457,15 +457,15 @@ def publish(self):
# reload old_public to get correct tree attrs
old_public = Page.objects.get(pk=old_public.pk)
old_public.move_to(None, 'last-child')
- # moving the object out of the way berore deleting works, but why?
+ # moving the object out of the way before deleting works, but why?
# finally delete the old public page
old_public.delete()
- # page was published, check if there are some childs, which are waiting
- # for publishing (because of the parent)
+ # page was published, check if there are children waiting for publishing
+ # (because of the parent)
publish_set = self.children.filter(moderator_state = Page.MODERATOR_APPROVED_WAITING_FOR_PARENTS)
for page in publish_set:
- # recursive call to all childrens....
+ # recursive call to all children....
page.moderator_state = Page.MODERATOR_APPROVED
page.save(change_state=False)
page.publish()
@@ -522,13 +522,13 @@ def get_languages(self):
if not hasattr(self, "all_languages"):
self.all_languages = Title.objects.filter(page=self).values_list("language", flat=True).distinct()
self.all_languages = list(self.all_languages)
- self.all_languages.sort()
+ self.all_languages.sort()
return self.all_languages
def get_cached_ancestors(self, ascending=True):
if ascending:
if not hasattr(self, "ancestors_ascending"):
- self.ancestors_ascending = list(self.get_ancestors(ascending))
+ self.ancestors_ascending = list(self.get_ancestors(ascending))
return self.ancestors_ascending
else:
if not hasattr(self, "ancestors_descending"):
@@ -537,7 +537,7 @@ def get_cached_ancestors(self, ascending=True):
def get_title_obj(self, language=None, fallback=True, version_id=None, force_reload=False):
"""Helper function for accessing wanted / current title.
- If wanted title doesn't exists, EmptyTitle instance will be returned.
+ If wanted title doesn't exist, EmptyTitle instance will be returned.
"""
language = self._get_title_cache(language, fallback, version_id, force_reload)
@@ -628,8 +628,8 @@ def _get_title_cache(self, language, fallback, version_id, force_reload):
fallback_langs = i18n.get_fallback_languages(language)
for lang in fallback_langs:
if lang in self.title_cache:
- return lang
- load = True
+ return lang
+ load = True
if load:
from cms.models.titlemodels import Title
if version_id:
@@ -643,10 +643,10 @@ def _get_title_cache(self, language, fallback, version_id, force_reload):
else:
title = Title.objects.get_title(self, language, language_fallback=fallback)
if title:
- self.title_cache[title.language] = title
+ self.title_cache[title.language] = title
language = title.language
return language
-
+
def get_template(self):
"""
get the template of this page if defined or if closer parent if
@@ -674,7 +674,7 @@ def get_template_name(self):
template = self.get_template()
for t in settings.CMS_TEMPLATES:
if t[0] == template:
- return t[1]
+ return t[1]
return _("default")
def has_view_permission(self, request):
@@ -857,7 +857,7 @@ def is_public_published(self):
if hasattr(self, 'public_published_cache'):
# if it was cached in change list, return cached value
return self.public_published_cache
- # othervise make db lookup
+ # otherwise make db lookup
if self.publisher_public_id:
return self.publisher_public.published
#return is_public_published(self)
@@ -912,13 +912,13 @@ def _publisher_get_public_copy(self):
inheritance.
eg. Text.objects.get(pk=1).publisher_public returns instance of CMSPlugin
- instead of instance of Text, thats why this method must be overriden in
+ instead of instance of Text, that's why this method must be overridden in
CMSPlugin.
"""
return self.publisher_public
def get_next_filtered_sibling(self, **filters):
- """Very simillar to original mptt method, but adds support for filters.
+ """Very similar to original mptt method, but adds support for filters.
Returns this model instance's next sibling in the tree, or
``None`` if it doesn't have a next sibling.
"""
@@ -942,7 +942,7 @@ def get_next_filtered_sibling(self, **filters):
return sibling
def get_previous_filtered_sibling(self, **filters):
- """Very simillar to original mptt method, but adds support for filters.
+ """Very similar to original mptt method, but adds support for filters.
Returns this model instance's previous sibling in the tree, or
``None`` if it doesn't have a previous sibling.
"""
@@ -990,7 +990,7 @@ def _publisher_save_public(self, obj):
obj.insert_at(public_parent, save=False)
else:
# check if object was moved / structural tree change
- prev_public_sibling = self.old_public.get_previous_filtered_sibling()
+ prev_public_sibling = self.old_public.get_previous_filtered_sibling(publisher_is_draft=False)
if not self.level == self.old_public.level or \
not (self.level > 0 and self.parent.publisher_public == self.old_public.parent) or \
@@ -1005,12 +1005,14 @@ def _publisher_save_public(self, obj):
obj.insert_at(target, position='first-child')
else:
# it is a move from the right side or just save
- next_sibling = self.get_next_filtered_sibling()
+ next_sibling = self.get_next_filtered_sibling(
+ publisher_is_draft=True, publisher_public__isnull=False
+ )
if next_sibling and next_sibling.publisher_public_id:
obj.insert_at(next_sibling.publisher_public, position="left")
else:
# insert at last public position
- prev_sibling = self.old_public.get_previous_filtered_sibling()
+ prev_sibling = self.old_public.get_previous_filtered_sibling(publisher_is_draft=False)
if prev_sibling:
obj.insert_at(prev_sibling, position="right")
@@ -1020,7 +1022,7 @@ def _publisher_save_public(self, obj):
obj.insert_at(target, position='first-child')
else:
# it is a move from the right side or just save
- next_sibling = self.old_public.get_next_filtered_sibling()
+ next_sibling = self.old_public.get_next_filtered_sibling(publisher_is_draft=False)
if next_sibling and next_sibling.publisher_public_id:
obj.insert_at(next_sibling, position="left")
# or none structural change, just save
@@ -1046,7 +1048,7 @@ def rescan_placeholders(self):
def _reversion():
exclude_fields = ['publisher_is_draft', 'publisher_public', 'publisher_state']
-
+
reversion_register(
Page,
follow=["title_set", "placeholders", "pagepermission_set"],
View
31 cms/tests/page.py
@@ -50,10 +50,10 @@ def test_create_page(self):
self.assertEqual(page.get_slug(), page_data['slug'])
self.assertEqual(page.placeholders.all().count(), 2)
- # were public instanes created?
+ # were public instances created?
title = Title.objects.drafts().get(slug=page_data['slug'])
-
+
def test_slug_collision(self):
"""
Test a slug collision
@@ -78,8 +78,8 @@ def test_slug_collision(self):
self.assertEqual(response.status_code, 200)
self.assertEqual(response['Location'].endswith(URL_CMS_PAGE_ADD), True)
# TODO: check for slug collisions after move
- # TODO: check for slug collisions with different settings
-
+ # TODO: check for slug collisions with different settings
+
def test_details_view(self):
"""
Test the details view
@@ -150,7 +150,7 @@ def test_meta_description_and_keywords_from_template_tags(self):
req = HttpRequest()
page.published = True
page.save()
- req.current_page = page
+ req.current_page = page
req.REQUEST = {}
self.assertEqual(t.render(template.Context({"request": req})), "Hello I am a page page,cms,stuff")
@@ -349,12 +349,12 @@ def test_delete_with_plugins(self):
placeholder = page.placeholders.all()[0]
plugin_base = CMSPlugin(
plugin_type='TextPlugin',
- placeholder=placeholder,
- position=1,
+ placeholder=placeholder,
+ position=1,
language=settings.LANGUAGES[0][0]
)
plugin_base.insert_at(None, position='last-child', save=False)
-
+
plugin = Text(body='')
plugin_base.set_base_attr(plugin)
plugin.save()
@@ -515,6 +515,21 @@ def test_home_slug_not_accessible(self):
resp = self.client.get('/en/page/')
self.assertEqual(resp.status_code, HttpResponseNotFound.status_code)
+ def test_public_home_page_replaced(self):
+ """Test that publishing changes to the home page doesn't move the public version"""
+ home = create_page('home', 'nav_playground.html', 'en', published = True, slug = 'home')
+ self.assertEqual(Page.objects.drafts().get_home().get_slug(), 'home')
+ home.publish()
+ self.assertEqual(Page.objects.public().get_home().get_slug(), 'home')
+ other = create_page('other', 'nav_playground.html', 'en', published = True, slug = 'other')
+ other.publish()
+ self.assertEqual(Page.objects.drafts().get_home().get_slug(), 'home')
+ self.assertEqual(Page.objects.public().get_home().get_slug(), 'home')
+ home.in_navigation = True
+ home.save()
+ home.publish()
+ self.assertEqual(Page.objects.drafts().get_home().get_slug(), 'home')
+ self.assertEqual(Page.objects.public().get_home().get_slug(), 'home')
class NoAdminPageTests(CMSTestCase):
urls = 'project.noadmin_urls'
View
2  cms/tests/publisher.py
@@ -16,7 +16,7 @@ class PublisherTestCase(CMSTestCase):
def test_simple_publisher(self):
'''
- Creates the stuff needed for theses tests.
+ Creates the stuff needed for these tests.
Please keep this up-to-date (the docstring!)
A
Something went wrong with that request. Please try again.