diff --git a/admin/code/LeftAndMain.php b/admin/code/LeftAndMain.php index 9d5d667d0..3042ed4fb 100644 --- a/admin/code/LeftAndMain.php +++ b/admin/code/LeftAndMain.php @@ -799,7 +799,12 @@ class LeftAndMain extends Controller implements PermissionProvider { ? $filter->getChildrenMethod() : 'AllChildrenIncludingDeleted'; - if(!$numChildrenMethod) $numChildrenMethod = 'numChildren'; + if(!$numChildrenMethod) { + $numChildrenMethod = 'numChildren'; + if($filter && $filter->getNumChildrenMethod()) { + $numChildrenMethod = $filter->getNumChildrenMethod(); + } + } if(!$filterFunction) $filterFunction = ($filter) ? array($filter, 'isPageIncluded') : null; // Get the tree root @@ -812,7 +817,10 @@ class LeftAndMain extends Controller implements PermissionProvider { $obj->markPartialTree($nodeCountThreshold, $this, $childrenMethod, $numChildrenMethod); // Ensure current page is exposed - if($p = $this->currentPage()) $obj->markToExpose($p); + // This call flushes the Hierarchy::$marked cache when the current node is deleted + // @see CMSMain::getRecord() + // This will make it impossible to show children under a deleted parent page + // if($p = $this->currentPage()) $obj->markToExpose($p); // NOTE: SiteTree/CMSMain coupling :-( if(class_exists('SiteTree')) { @@ -823,9 +831,10 @@ class LeftAndMain extends Controller implements PermissionProvider { // getChildrenAsUL is a flexible and complex way of traversing the tree $controller = $this; $recordController = ($this->stat('tree_class') == 'SiteTree') ? singleton('CMSPageEditController') : $this; - $titleFn = function(&$child) use(&$controller, &$recordController) { + $titleFn = function(&$child, $numChildrenMethod) use(&$controller, &$recordController) { $link = Controller::join_links($recordController->Link("show"), $child->ID); - return LeftAndMain_TreeNode::create($child, $link, $controller->isCurrentPage($child))->forTemplate(); + $node = LeftAndMain_TreeNode::create($child, $link, $controller->isCurrentPage($child), $numChildrenMethod); + return $node->forTemplate(); }; // Limit the amount of nodes shown for performance reasons. @@ -1866,10 +1875,22 @@ class LeftAndMain_TreeNode extends ViewableData { */ protected $isCurrent; - public function __construct($obj, $link = null, $isCurrent = false) { + /** + * @var string + */ + protected $numChildrenMethod; + + /** + * @param $obj + * @param null $link + * @param bool $isCurrent + * @param $numChildrenMethod + */ + public function __construct($obj, $link = null, $isCurrent = false, $numChildrenMethod='numChildren') { $this->obj = $obj; $this->link = $link; $this->isCurrent = $isCurrent; + $this->numChildrenMethod = $numChildrenMethod; } /** @@ -1890,7 +1911,7 @@ class LeftAndMain_TreeNode extends ViewableData { } public function getClasses() { - $classes = $this->obj->CMSTreeClasses(); + $classes = $this->obj->CMSTreeClasses($this->numChildrenMethod); if($this->isCurrent) $classes .= " current"; $flags = $this->obj->hasMethod('getStatusFlags') ? $this->obj->getStatusFlags() : false; if ($flags) { diff --git a/model/Hierarchy.php b/model/Hierarchy.php index 3f20c1de9..d3c7070a5 100644 --- a/model/Hierarchy.php +++ b/model/Hierarchy.php @@ -115,7 +115,8 @@ class Hierarchy extends DataExtension { if($limitToMarked && $rootCall) { $this->markingFinished($numChildrenMethod); } - + + if($nodeCountCallback) { $nodeCountWarning = $nodeCountCallback($this->owner, $this->owner->$numChildrenMethod()); if($nodeCountWarning) return $nodeCountWarning; @@ -130,6 +131,7 @@ class Hierarchy extends DataExtension { } if($children) { + if($attributes) { $attributes = " $attributes"; } @@ -139,10 +141,15 @@ class Hierarchy extends DataExtension { foreach($children as $child) { if(!$limitToMarked || $child->isMarked()) { $foundAChild = true; - $output .= (is_callable($titleEval)) ? $titleEval($child) : eval("return $titleEval;"); + if(is_callable($titleEval)) { + $output .= $titleEval($child, $numChildrenMethod); + } else { + $output .= eval("return $titleEval;"); + } $output .= "\n"; $numChildren = $child->$numChildrenMethod(); + if( // Always traverse into opened nodes (they might be exposed as parents of search results) $child->isExpanded() @@ -158,7 +165,7 @@ class Hierarchy extends DataExtension { } else { $output .= $child->getChildrenAsUL("", $titleEval, $extraArg, $limitToMarked, $childrenMethod, $numChildrenMethod, false, $nodeCountThreshold); - } + } } elseif($child->isTreeOpened()) { // Since we're not loading children, don't mark it as open either $child->markClosed(); @@ -256,7 +263,7 @@ class Hierarchy extends DataExtension { return call_user_func($func, $node); } } - + /** * Mark all children of the given node that match the marking filter. * @param DataObject $node Parent node. @@ -276,11 +283,6 @@ class Hierarchy extends DataExtension { foreach($children as $child) { $markingMatches = $this->markingFilterMatches($child); if($markingMatches) { - // Filtered results should always show opened, since actual matches - // might be hidden by non-matching parent nodes. - if($this->markingFilter) { - $child->markOpened(); - } if($child->$numChildrenMethod()) { $child->markUnexpanded(); } else { @@ -315,14 +317,14 @@ class Hierarchy extends DataExtension { * * @return string */ - public function markingClasses() { + public function markingClasses($numChildrenMethod="numChildren") { $classes = ''; if(!$this->isExpanded()) { $classes .= " unexpanded"; } // Set jstree open state, or mark it as a leaf (closed) if there are no children - if(!$this->numChildren()) { + if(!$this->$numChildrenMethod()) { $classes .= " jstree-leaf closed"; } elseif($this->isTreeOpened()) { $classes .= " jstree-open"; diff --git a/tests/model/HierarchyTest.php b/tests/model/HierarchyTest.php index deb75dd96..0dcdbd8d5 100644 --- a/tests/model/HierarchyTest.php +++ b/tests/model/HierarchyTest.php @@ -391,6 +391,97 @@ class HierarchyTest extends SapphireTest { ); } + /** + * This test checks that deleted ('archived') child pages don't set a css class on the parent + * node that makes it look like it has children + */ + public function testGetChildrenAsULNodeDeletedOnLive() { + $obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2'); + $obj2a = $this->objFromFixture('HierarchyTest_Object', 'obj2a'); + $obj2aa = $this->objFromFixture('HierarchyTest_Object', 'obj2aa'); + $obj2ab = $this->objFromFixture('HierarchyTest_Object', 'obj2b'); + + // delete all children under obj2 + $obj2a->delete(); + $obj2aa->delete(); + $obj2ab->delete(); + // Don't pre-load all children + $nodeCountThreshold = 1; + + $childrenMethod = 'AllChildren'; + $numChildrenMethod = 'numChildren'; + + $root = new HierarchyTest_Object(); + $root->markPartialTree($nodeCountThreshold, null, $childrenMethod, $numChildrenMethod); + + // As in LeftAndMain::getSiteTreeFor() but simpler and more to the point for testing purposes + $titleFn = function(&$child, $numChildrenMethod="") { + return '
  • "' . $child->Title; + }; + + $html = $root->getChildrenAsUL( + "", + $titleFn, + null, + true, // limit to marked + $childrenMethod, + $numChildrenMethod, + true, + $nodeCountThreshold + ); + + // Get the class attribute from the $obj2 node in the sitetree, class 'jstree-leaf' means it's a leaf node + $nodeClass = $this->getNodeClassFromTree($html, $obj2); + $this->assertEquals('jstree-leaf closed', $nodeClass, 'object2 should not have children in the sitetree'); + } + + /** + * This test checks that deleted ('archived') child pages _do_ set a css class on the parent + * node that makes it look like it has children when getting all children including deleted + */ + public function testGetChildrenAsULNodeDeletedOnStage() { + $obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2'); + $obj2a = $this->objFromFixture('HierarchyTest_Object', 'obj2a'); + $obj2aa = $this->objFromFixture('HierarchyTest_Object', 'obj2aa'); + $obj2ab = $this->objFromFixture('HierarchyTest_Object', 'obj2b'); + + // delete all children under obj2 + $obj2a->delete(); + $obj2aa->delete(); + $obj2ab->delete(); + // Don't pre-load all children + $nodeCountThreshold = 1; + + $childrenMethod = 'AllChildrenIncludingDeleted'; + $numChildrenMethod = 'numHistoricalChildren'; + + $root = new HierarchyTest_Object(); + $root->markPartialTree($nodeCountThreshold, null, $childrenMethod, $numChildrenMethod); + + // As in LeftAndMain::getSiteTreeFor() but simpler and more to the point for testing purposes + $titleFn = function(&$child, $numChildrenMethod="") { + return '
  • "' . $child->Title; + }; + + $html = $root->getChildrenAsUL( + "", + $titleFn, + null, + true, // limit to marked + $childrenMethod, + $numChildrenMethod, + true, + $nodeCountThreshold + ); + + // Get the class attribute from the $obj2 node in the sitetree + $nodeClass = $this->getNodeClassFromTree($html, $obj2); + // Object2 can now be expanded + $this->assertEquals('unexpanded jstree-closed closed', $nodeClass, 'obj2 should have children in the sitetree'); + } + /** * @param String $html [description] * @param array $nodes Breadcrumb path as array @@ -417,6 +508,25 @@ class HierarchyTest extends SapphireTest { self::assertThat((bool)$match, self::isFalse(), $message); } + /** + * Get the HTML class attribute from a node in the sitetree + * + * @param $html + * @param $node + * @return string + */ + protected function getNodeClassFromTree($html, $node) { + $parser = new CSSContentParser($html); + $xpath = '//ul/li[@id="' . $node->ID . '"]'; + $object = $parser->getByXpath($xpath); + + foreach($object[0]->attributes() as $key => $attr) { + if($key == 'class') { + return (string)$attr; + } + } + return ''; + } } class HierarchyTest_Object extends DataObject implements TestOnly { @@ -428,4 +538,8 @@ class HierarchyTest_Object extends DataObject implements TestOnly { 'Hierarchy', "Versioned('Stage', 'Live')", ); -} \ No newline at end of file + + public function cmstreeclasses() { + return $this->markingClasses(); + } +}