Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

Uses mptt.managers.TreeManager as a base manager. #1492

Closed
wants to merge 2 commits into from

3 participants

@BertrandBordage

As I was trying to rebuild my Pages (which are pretty broken, since mptt sends me errors while trying to move some of them) using mptt.managers.TreeManager.rebuild, I realised Page.objects was not deriving from TreeManager.

This pull request tries to fix this lack, but I am not sure it will be properly able to rebuild the nodes (Page is such a complex model…). Doing a rebuild only partially fixed my problem, so that I can't be sure of its effects. While moving pages, I still get some Out of range value for column 'lft', but other problems like A node may not be made a sibling of any of its descendants. have disappeared.

Tests with this pull request: 101 failures, 523 errors.
Tests without this pull request: 101 failures, 523 errors.

@ojii ojii commented on the diff
cms/publisher/manager.py
@@ -15,11 +16,9 @@ def drafts(self):
def public(self):
return self.filter(publisher_is_draft=False)
-
- """
- def all(self):
- raise NotImplementedError, ("Calling all() on manager of publisher "
- "object is not allowed. Please use drafts() or public() method "
- "instead. If this isn't accident use get_query_set().all() for "
- "all instances.")
- """
+
+# def all(self):
+# raise NotImplementedError, ("Calling all() on manager of publisher "
+# "object is not allowed. Please use drafts() or public() method "
+# "instead. If this isn't accident use get_query_set().all() for "
+# "all instances.")
@ojii Collaborator
ojii added a note

why comment those lines out instead of removing them? And why remove them in the first place?

I commented this because it is the right way to... comment. This was "stringed" for a reason I ignore. And since I think it is OK to perform a .all() on a PublisherManager (to be able to do Page.objects.all() for example), removing them may be the best thing to do. I will update this pull request if you agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
@ojii
Collaborator

Just noticed the "Tests without this pull request: 101 failures, 523 errors.". What the heck ?! The test suite should pass! How did you test this?

@digi604
Collaborator

travis seams to like the PR

@BertrandBordage

@ojii, I just ran ./manage.py test cms in my project directory. I always got a whole bunch of errors while executing those tests. If you say I shouldn't encounter any failure or error, this is maybe why I experience some bugs while trying to move pages in enormous "categories" containing dozens and dozens of pages. I have to find the origin of this problem :\

@ojii
Collaborator

@BertrandBordage that's not really how you run our tests.

You check out the django CMS repo and run setup.py test

@BertrandBordage

OK, with the correct command I'm only getting this : SphinxWarning: /home/bertrand/depots_divers/django-cms/docs/index.rst:16: ERROR: Error in "warning" directive:
invalid option block.

@BertrandBordage BertrandBordage referenced this pull request
Merged

fix mptt command #1521

@digi604
Collaborator

closed as we have an mptt fix command now.

@digi604 digi604 closed this
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
Showing with 9 additions and 10 deletions.
  1. +9 −10 cms/publisher/manager.py
View
19 cms/publisher/manager.py
@@ -1,8 +1,9 @@
# -*- coding: utf-8 -*-
-from django.db import models
+from mptt import managers
from cms.publisher.query import PublisherQuerySet
-class PublisherManager(models.Manager):
+
+class PublisherManager(managers.TreeManager):
"""Manager with some support handling publisher.
"""
def get_query_set(self):
@@ -15,11 +16,9 @@ def drafts(self):
def public(self):
return self.filter(publisher_is_draft=False)
-
- """
- def all(self):
- raise NotImplementedError, ("Calling all() on manager of publisher "
- "object is not allowed. Please use drafts() or public() method "
- "instead. If this isn't accident use get_query_set().all() for "
- "all instances.")
- """
+
+# def all(self):
+# raise NotImplementedError, ("Calling all() on manager of publisher "
+# "object is not allowed. Please use drafts() or public() method "
+# "instead. If this isn't accident use get_query_set().all() for "
+# "all instances.")
@ojii Collaborator
ojii added a note

why comment those lines out instead of removing them? And why remove them in the first place?

I commented this because it is the right way to... comment. This was "stringed" for a reason I ignore. And since I think it is OK to perform a .all() on a PublisherManager (to be able to do Page.objects.all() for example), removing them may be the best thing to do. I will update this pull request if you agree.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Something went wrong with that request. Please try again.