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

Closed
wants to merge 2 commits into
from

Projects

None yet

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 Oct 26, 2012
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
ojii Oct 26, 2012

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

@BertrandBordage
BertrandBordage Nov 12, 2012

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.

@ojii

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
Divio AG member

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

@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 Nov 19, 2012
Merged

fix mptt command #1521

@digi604
Divio AG member

closed as we have an mptt fix command now.

@digi604 digi604 closed this Nov 27, 2012
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment