From 43dcde5197e0995225235f69937ed57bec0cd7e2 Mon Sep 17 00:00:00 2001
From: madmatt <signups@madman.net.nz>
Date: Wed, 20 Apr 2016 11:21:14 +1200
Subject: [PATCH] FIX: Hierarchy was incorrectly unexpanding nodes that had
 been previously expanded

---
 model/Hierarchy.php           |  3 +-
 tests/model/HierarchyTest.php | 59 ++++++++++++++++++++++++++++++++---
 tests/model/HierarchyTest.yml | 14 ++++++++-
 3 files changed, 69 insertions(+), 7 deletions(-)

diff --git a/model/Hierarchy.php b/model/Hierarchy.php
index 6ccc6ebc5..3e8cf1c67 100644
--- a/model/Hierarchy.php
+++ b/model/Hierarchy.php
@@ -274,7 +274,8 @@ class Hierarchy extends DataExtension {
 			foreach($children as $child) {
 				$markingMatches = $this->markingFilterMatches($child);
 				if($markingMatches) {
-					if($child->$numChildrenMethod()) {
+					// Mark a child node as unexpanded if it has children and has not already been expanded
+					if($child->$numChildrenMethod() && !$child->isExpanded()) {
 						$child->markUnexpanded();
 					} else {
 						$child->markExpanded();
diff --git a/tests/model/HierarchyTest.php b/tests/model/HierarchyTest.php
index 735845529..5fee85703 100644
--- a/tests/model/HierarchyTest.php
+++ b/tests/model/HierarchyTest.php
@@ -59,12 +59,12 @@ class HierarchyTest extends SapphireTest {
 		// Obj 3 has been deleted; let's bring it back from the grave
 		$obj3 = Versioned::get_including_deleted("HierarchyTest_Object", "\"Title\" = 'Obj 3'")->First();
 
-		// Check that both obj 3 children are returned
-		$this->assertEquals(array("Obj 3a", "Obj 3b", "Obj 3c"),
+		// Check that all obj 3 children are returned
+		$this->assertEquals(array("Obj 3a", "Obj 3b", "Obj 3c", "Obj 3d"),
 			$obj3->AllHistoricalChildren()->column('Title'));
 
 		// Check numHistoricalChildren
-		$this->assertEquals(3, $obj3->numHistoricalChildren());
+		$this->assertEquals(4, $obj3->numHistoricalChildren());
 
 	}
 
@@ -94,11 +94,11 @@ class HierarchyTest extends SapphireTest {
 	public function testNumChildren() {
 		$this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj1')->numChildren(), 0);
 		$this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj2')->numChildren(), 2);
-		$this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj3')->numChildren(), 3);
+		$this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj3')->numChildren(), 4);
 		$this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj2a')->numChildren(), 2);
 		$this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj2b')->numChildren(), 0);
 		$this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj3a')->numChildren(), 2);
-		$this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj3b')->numChildren(), 0);
+		$this->assertEquals($this->objFromFixture('HierarchyTest_Object', 'obj3d')->numChildren(), 0);
 
 		$obj1 = $this->objFromFixture('HierarchyTest_Object', 'obj1');
 		$this->assertEquals($obj1->numChildren(), 0);
@@ -180,6 +180,53 @@ class HierarchyTest extends SapphireTest {
 		$this->assertEquals('Obj 2 &raquo; Obj 2a &raquo; Obj 2aa', $obj2aa->getBreadcrumbs());
 	}
 
+	/**
+	 * @covers Hierarchy::markChildren()
+	 */
+	public function testMarkChildrenDoesntUnmarkPreviouslyMarked() {
+		$obj3 = $this->objFromFixture('HierarchyTest_Object', 'obj3');
+		$obj3aa = $this->objFromFixture('HierarchyTest_Object', 'obj3aa');
+		$obj3ba = $this->objFromFixture('HierarchyTest_Object', 'obj3ba');
+		$obj3ca = $this->objFromFixture('HierarchyTest_Object', 'obj3ca');
+
+		$obj3->markPartialTree();
+		$obj3->markToExpose($obj3aa);
+		$obj3->markToExpose($obj3ba);
+		$obj3->markToExpose($obj3ca);
+
+		$expected = <<<EOT
+<ul>
+<li>Obj 3a
+<ul>
+<li>Obj 3aa
+</li>
+<li>Obj 3ab
+</li>
+</ul>
+</li>
+<li>Obj 3b
+<ul>
+<li>Obj 3ba
+</li>
+<li>Obj 3bb
+</li>
+</ul>
+</li>
+<li>Obj 3c
+<ul>
+<li>Obj 3c
+</li>
+</ul>
+</li>
+<li>Obj 3d
+</li>
+</ul>
+
+EOT;
+
+		$this->assertSame($expected, $obj3->getChildrenAsUL());
+	}
+
 	public function testGetChildrenAsUL() {
 		$obj1 = $this->objFromFixture('HierarchyTest_Object', 'obj1');
 		$obj2 = $this->objFromFixture('HierarchyTest_Object', 'obj2');
@@ -539,6 +586,8 @@ class HierarchyTest_Object extends DataObject implements TestOnly {
 		"Versioned('Stage', 'Live')",
 	);
 
+	private static $default_sort = 'Title ASC';
+
 	public function cmstreeclasses() {
 		return $this->markingClasses();
 	}
diff --git a/tests/model/HierarchyTest.yml b/tests/model/HierarchyTest.yml
index b3cdd8231..cb5da66d1 100644
--- a/tests/model/HierarchyTest.yml
+++ b/tests/model/HierarchyTest.yml
@@ -20,6 +20,9 @@ HierarchyTest_Object:
    obj3c:
       Parent: =>HierarchyTest_Object.obj3
       Title: Obj 3c
+   obj3d:
+      Parent: =>HierarchyTest_Object.obj3
+      Title: Obj 3d
    obj2aa:
       Parent: =>HierarchyTest_Object.obj2a
       Title: Obj 2aa
@@ -31,4 +34,13 @@ HierarchyTest_Object:
       Title: Obj 3aa
    obj3ab:
       Parent: =>HierarchyTest_Object.obj3a
-      Title: Obj 3ab
\ No newline at end of file
+      Title: Obj 3ab
+   obj3ba:
+      Parent: =>HierarchyTest_Object.obj3b
+      Title: Obj 3ba
+   obj3bb:
+      Parent: =>HierarchyTest_Object.obj3b
+      Title: Obj 3bb
+   obj3ca:
+      Parent: =>HierarchyTest_Object.obj3c
+      Title: Obj 3c