Merge pull request #3309 from stojg/issue/cms-1049

Delete parent page, child pages cannot be found anymore

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) {

model/Hierarchy.php

@@ -116,6 +116,7 @@ class Hierarchy extends DataExtension {
 			$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()
@@ -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";

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 '<li class="' . $child->markingClasses($numChildrenMethod).
+				'" id="' . $child->ID . '">"' . $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 '<li class="' . $child->markingClasses($numChildrenMethod).
+				'" id="' . $child->ID . '">"' . $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')",
 	);
+
+	public function cmstreeclasses() {
+		return $this->markingClasses();
+	}
 }