From 53dbbb7982429dfd75c5d7ae265087fc54dcc04b Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Wed, 6 Aug 2014 15:00:48 +1200 Subject: [PATCH] BUG Fix CMSMain::getList to correctly respect filter result Fixes #1064 CMSSiteTreeFilter refactored to allow SS_List of filtered pages to be returned --- code/controllers/CMSMain.php | 45 ++++++++------ code/controllers/CMSSiteTreeFilter.php | 54 +++++++++------- tests/controller/CMSMainTest.php | 86 ++++++++++++++++++++++++++ 3 files changed, 142 insertions(+), 43 deletions(-) diff --git a/code/controllers/CMSMain.php b/code/controllers/CMSMain.php index 340529ad..ebb326cd 100644 --- a/code/controllers/CMSMain.php +++ b/code/controllers/CMSMain.php @@ -697,35 +697,40 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr return $this->renderWith($this->getTemplatesWithSuffix('_ListView')); } + /** + * Safely reconstruct a selected filter from a given set of query parameters + * + * @param array $params Query parameters to use + * @return CMSSiteTreeFilter The filter class, or null if none present + * @throws InvalidArgumentException if invalid filter class is passed. + */ + protected function getQueryFilter($params) { + if(empty($params['FilterClass'])) return null; + $filterClass = $params['FilterClass']; + if(!is_subclass_of($filterClass, 'CMSSiteTreeFilter')) { + throw new InvalidArgumentException("Invalid filter class passed: {$filterClass}"); + } + return $filterClass::create($params); + } + /** * Returns the pages meet a certain criteria as {@see CMSSiteTreeFilter} or the subpages of a parent page * defaulting to no filter and show all pages in first level. * Doubles as search results, if any search parameters are set through {@link SearchForm()}. * - * @param Array $params Search filter criteria - * @param Int $parentID Optional parent node to filter on (can't be combined with other search criteria) + * @param array $params Search filter criteria + * @param int $parentID Optional parent node to filter on (can't be combined with other search criteria) * @return SS_List - * @throws Exception if invalid filter class is passed. + * @throws InvalidArgumentException if invalid filter class is passed. */ - public function getList($params, $parentID = 0) { - $list = new DataList($this->stat('tree_class')); - $filter = null; - $ids = array(); - if(isset($params['FilterClass']) && $filterClass = $params['FilterClass']){ - if(!is_subclass_of($filterClass, 'CMSSiteTreeFilter')) { - throw new Exception(sprintf('Invalid filter class passed: %s', $filterClass)); - } - $filter = new $filterClass($params); - $filterOn = true; - foreach($pages=$filter->pagesIncluded() as $pageMap){ - $ids[] = $pageMap['ID']; - } - if(count($ids)) $list = $list->where('"'.$this->stat('tree_class').'"."ID" IN ('.implode(",", $ids).')'); + public function getList($params = array(), $parentID = 0) { + if($filter = $this->getQueryFilter($params)) { + return $filter->getFilteredPages(); } else { - $list = $list->filter("ParentID", is_numeric($parentID) ? $parentID : 0); + $list = DataList::create($this->stat('tree_class')); + $parentID = is_numeric($parentID) ? $parentID : 0; + return $list->filter("ParentID", $parentID); } - - return $list; } public function ListViewForm() { diff --git a/code/controllers/CMSSiteTreeFilter.php b/code/controllers/CMSSiteTreeFilter.php index 1c13947f..f6e2384b 100644 --- a/code/controllers/CMSSiteTreeFilter.php +++ b/code/controllers/CMSSiteTreeFilter.php @@ -92,9 +92,19 @@ abstract class CMSSiteTreeFilter extends Object { } /** - * @return Array Map of Page IDs to their respective ParentID values. + * Gets the list of filtered pages + * + * @see {@link SiteTree::getStatusFlags()} + * @return SS_List */ - public function pagesIncluded() {} + abstract public function getFilteredPages(); + + /** + * @return array Map of Page IDs to their respective ParentID values. + */ + public function pagesIncluded() { + return $this->mapIDs($this->getFilteredPages()); + } /** * Populate the IDs of the pages returned by pagesIncluded(), also including @@ -231,15 +241,15 @@ class CMSSIteTreeFilter_PublishedPages extends CMSSiteTreeFilter { * Filters out all pages who's status who's status that doesn't exist on live * * @see {@link SiteTree::getStatusFlags()} - * @return array + * @return SS_List */ - public function pagesIncluded() { + public function getFilteredPages() { $pages = Versioned::get_including_deleted('SiteTree'); $pages = $this->applyDefaultFilters($pages); $pages = $pages->filterByCallback(function($page) { return $page->ExistsOnLive; }); - return $this->mapIDs($pages); + return $pages; } } @@ -267,10 +277,10 @@ class CMSSiteTreeFilter_DeletedPages extends CMSSiteTreeFilter { return _t('CMSSiteTreeFilter_DeletedPages.Title', "All pages, including deleted"); } - public function pagesIncluded() { + public function getFilteredPages() { $pages = Versioned::get_including_deleted('SiteTree'); $pages = $this->applyDefaultFilters($pages); - return $this->mapIDs($pages); + return $pages; } } @@ -286,12 +296,12 @@ class CMSSiteTreeFilter_ChangedPages extends CMSSiteTreeFilter { return _t('CMSSiteTreeFilter_ChangedPages.Title', "Changed pages"); } - public function pagesIncluded() { + public function getFilteredPages() { $pages = Versioned::get_by_stage('SiteTree', 'Stage'); $pages = $this->applyDefaultFilters($pages) ->leftJoin('SiteTree_Live', '"SiteTree_Live"."ID" = "SiteTree"."ID"') ->where('"SiteTree"."Version" > "SiteTree_Live"."Version"'); - return $this->mapIDs($pages); + return $pages; } } @@ -310,17 +320,16 @@ class CMSSiteTreeFilter_StatusRemovedFromDraftPages extends CMSSiteTreeFilter { /** * Filters out all pages who's status is set to "Removed from draft". * - * @see {@link SiteTree::getStatusFlags()} - * @return array + * @return SS_List */ - public function pagesIncluded() { + public function getFilteredPages() { $pages = Versioned::get_including_deleted('SiteTree'); $pages = $this->applyDefaultFilters($pages); $pages = $pages->filterByCallback(function($page) { // If page is removed from stage but not live return $page->IsDeletedFromStage && $page->ExistsOnLive; }); - return $this->mapIDs($pages); + return $pages; } } @@ -340,16 +349,16 @@ class CMSSiteTreeFilter_StatusDraftPages extends CMSSiteTreeFilter { * Filters out all pages who's status is set to "Draft". * * @see {@link SiteTree::getStatusFlags()} - * @return array + * @return SS_List */ - public function pagesIncluded() { + public function getFilteredPages() { $pages = Versioned::get_by_stage('SiteTree', 'Stage'); $pages = $this->applyDefaultFilters($pages); $pages = $pages->filterByCallback(function($page) { // If page exists on stage but not on live return (!$page->IsDeletedFromStage && $page->IsAddedToStage); }); - return $this->mapIDs($pages); + return $pages; } } @@ -379,9 +388,9 @@ class CMSSiteTreeFilter_StatusDeletedPages extends CMSSiteTreeFilter { * Filters out all pages who's status is set to "Deleted". * * @see {@link SiteTree::getStatusFlags()} - * @return array + * @return SS_List */ - public function pagesIncluded() { + public function getFilteredPages() { $pages = Versioned::get_including_deleted('SiteTree'); $pages = $this->applyDefaultFilters($pages); @@ -389,7 +398,7 @@ class CMSSiteTreeFilter_StatusDeletedPages extends CMSSiteTreeFilter { // Doesn't exist on either stage or live return $page->IsDeletedFromStage && !$page->ExistsOnLive; }); - return $this->mapIDs($pages); + return $pages; } } @@ -407,13 +416,12 @@ class CMSSiteTreeFilter_Search extends CMSSiteTreeFilter { * Retun an array of maps containing the keys, 'ID' and 'ParentID' for each page to be displayed * in the search. * - * @return Array + * @return SS_List */ - public function pagesIncluded() { - + public function getFilteredPages() { // Filter default records $pages = Versioned::get_by_stage('SiteTree', 'Stage'); $pages = $this->applyDefaultFilters($pages); - return $this->mapIDs($pages); + return $pages; } } diff --git a/tests/controller/CMSMainTest.php b/tests/controller/CMSMainTest.php index 644cc858..5bcf119a 100644 --- a/tests/controller/CMSMainTest.php +++ b/tests/controller/CMSMainTest.php @@ -349,6 +349,92 @@ class CMSMainTest extends FunctionalTest { $this->assertEquals($controller->getResponse()->getStatusCode(), 302); } } + + /** + * Tests filtering in {@see CMSMain::getList()} + */ + public function testGetList() { + $controller = new CMSMain(); + + // Test all pages (stage) + $pages = $controller->getList()->sort('Title'); + $this->assertEquals(28, $pages->count()); + $this->assertEquals( + array('Home', 'Page 1', 'Page 10', 'Page 11', 'Page 12'), + $pages->Limit(5)->column('Title') + ); + + // Change state of tree + $page1 = $this->objFromFixture('Page', 'page1'); + $page3 = $this->objFromFixture('Page', 'page3'); + $page11 = $this->objFromFixture('Page', 'page11'); + $page12 = $this->objFromFixture('Page', 'page12'); + // Deleted + $page1->doUnpublish(); + $page1->delete(); + // Live and draft + $page11->publish('Stage', 'Live'); + // Live only + $page12->publish('Stage', 'Live'); + $page12->delete(); + + // Re-test all pages (stage) + $pages = $controller->getList()->sort('Title'); + $this->assertEquals(26, $pages->count()); + $this->assertEquals( + array('Home', 'Page 10', 'Page 11', 'Page 13', 'Page 14'), + $pages->Limit(5)->column('Title') + ); + + // Test deleted page filter + $params = array( + 'FilterClass' => 'CMSSiteTreeFilter_StatusDeletedPages' + ); + $pages = $controller->getList($params); + $this->assertEquals(1, $pages->count()); + $this->assertEquals( + array('Page 1'), + $pages->column('Title') + ); + + // Test live, but not on draft filter + $params = array( + 'FilterClass' => 'CMSSiteTreeFilter_StatusRemovedFromDraftPages' + ); + $pages = $controller->getList($params); + $this->assertEquals(1, $pages->count()); + $this->assertEquals( + array('Page 12'), + $pages->column('Title') + ); + + // Test live pages filter + $params = array( + 'FilterClass' => 'CMSSIteTreeFilter_PublishedPages' + ); + $pages = $controller->getList($params); + $this->assertEquals(2, $pages->count()); + $this->assertEquals( + array('Page 11', 'Page 12'), + $pages->column('Title') + ); + + // Test that parentID is ignored when filtering + $pages = $controller->getList($params, $page3->ID); + $this->assertEquals(2, $pages->count()); + $this->assertEquals( + array('Page 11', 'Page 12'), + $pages->column('Title') + ); + + // Test that parentID is respected when not filtering + $pages = $controller->getList(array(), $page3->ID); + $this->assertEquals(2, $pages->count()); + $this->assertEquals( + array('Page 3.1', 'Page 3.2'), + $pages->column('Title') + ); + } } class CMSMainTest_ClassA extends Page implements TestOnly {