From f9a5601fa378646afe7f94ff2363241971f0748e Mon Sep 17 00:00:00 2001 From: Ingo Schommer Date: Mon, 3 Dec 2012 16:33:06 +0100 Subject: [PATCH] BUG Enforce "add page" restrictions, improve UI (fixes #7879) - Fix support for $allowed_children - Added note when type selection is restricted - Removed overly complex specs for "allowed children", the data should be inferred from "disallowed children" - Added support for SiteTree::$can_be_root - Return raw JSON (not entity-encoded) from CMSMain->SiteTreeHints() - Added tests for CMSMain->SiteTreeHints() --- code/controllers/CMSMain.php | 80 +++++++++++++---------- code/controllers/CMSPageAddController.php | 16 ++++- javascript/CMSMain.AddForm.js | 15 +++-- javascript/CMSMain.Tree.js | 75 ++++++++++++--------- templates/Includes/CMSMain_TreeView.ss | 2 +- tests/controller/CMSMainTest.php | 64 ++++++++++++++++++ 6 files changed, 181 insertions(+), 71 deletions(-) diff --git a/code/controllers/CMSMain.php b/code/controllers/CMSMain.php index bf6904ff..f846055e 100644 --- a/code/controllers/CMSMain.php +++ b/code/controllers/CMSMain.php @@ -364,7 +364,6 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr */ public function SiteTreeHints() { $json = ''; - $classes = SiteTree::page_type_classes(); $cacheCanCreate = array(); @@ -377,55 +376,68 @@ class CMSMain extends LeftAndMain implements CurrentPageIdentifier, PermissionPr $json = $cache->load($cacheKey); if(!$json) { $def['Root'] = array(); - $def['Root']['disallowedParents'] = array(); + $def['Root']['disallowedChildren'] = array(); + // Contains all possible classes to support UI controls listing them all, + // such as the "add page here" context menu. + $def['All'] = array(); + + // Identify disallows and set globals + $globalDisallowed = array(); + foreach($classes as $class) { + $obj = singleton($class); + $needsPerm = $obj->stat('need_permission'); + + if(!($obj instanceof HiddenClass)) { + $def['All'][$class] = array( + 'title' => $obj->i18n_singular_name() + ); + } + + if(!$obj->stat('can_be_root')) { + $def['Root']['disallowedChildren'][] = $class; + } + + if( + ($obj instanceof HiddenClass) + || (!array_key_exists($class, $cacheCanCreate) || !$cacheCanCreate[$class]) + || ($needsPerm && !$this->can($needsPerm)) + ) { + $globalDisallowed[] = $class; + $def['Root']['disallowedChildren'][] = $class; + } + } + + // Set disallows by class foreach($classes as $class) { $obj = singleton($class); if($obj instanceof HiddenClass) continue; - - $allowedChildren = $obj->allowedChildren(); - - // SiteTree::allowedChildren() returns null rather than an empty array if SiteTree::allowed_chldren == 'none' - if($allowedChildren == null) $allowedChildren = array(); - - // Exclude SiteTree from possible Children - $possibleChildren = array_diff($allowedChildren, array("SiteTree")); - // Find i18n - names and build allowed children array - foreach($possibleChildren as $child) { - $instance = singleton($child); - - if($instance instanceof HiddenClass) continue; + $def[$class] = array(); - if(!array_key_exists($child, $cacheCanCreate) || !$cacheCanCreate[$child]) continue; + $allowed = $obj->allowedChildren(); + if($pos = array_search('SiteTree', $allowed)) unset($allowed[$pos]); - // skip this type if it is restricted - if($instance->stat('need_permission') && !$this->can(singleton($class)->stat('need_permission'))) continue; + // Start by disallowing all classes which aren't specifically allowed, + // then add the ones which are globally disallowed. + $disallowed = array_diff($classes, (array)$allowed); + $disallowed = array_unique(array_merge($disallowed, $globalDisallowed)); + if($disallowed) $def[$class]['disallowedChildren'] = $disallowed; - $title = $instance->i18n_singular_name(); - - $def[$class]['allowedChildren'][] = array("ssclass" => $child, "ssname" => $title); + $defaultChild = $obj->defaultChild(); + if($defaultChild != 'Page' && $defaultChild != null) { + $def[$class]['defaultChild'] = $defaultChild; } - $allowedChildren = array_keys(array_diff($classes, $allowedChildren)); - if($allowedChildren) $def[$class]['disallowedChildren'] = $allowedChildren; - $defaultChild = $obj->defaultChild(); - if($defaultChild != 'Page' && $defaultChild != null) $def[$class]['defaultChild'] = $defaultChild; $defaultParent = $obj->defaultParent(); $parent = SiteTree::get_by_link($defaultParent); $id = $parent ? $parent->id : null; - if ($defaultParent != 1 && $defaultParent != null) $def[$class]['defaultParent'] = $defaultParent; - if(isset($def[$class]['disallowedChildren'])) { - foreach($def[$class]['disallowedChildren'] as $disallowedChild) { - $def[$disallowedChild]['disallowedParents'][] = $class; - } + if ($defaultParent != 1 && $defaultParent != null) { + $def[$class]['defaultParent'] = $defaultParent; } - - // Are any classes allowed to be parents of root? - $def['Root']['disallowedParents'][] = $class; } - $json = Convert::raw2xml(Convert::raw2json($def)); + $json = Convert::raw2json($def); $cache->save($json, $cacheKey); } return $json; diff --git a/code/controllers/CMSPageAddController.php b/code/controllers/CMSPageAddController.php index 0f7743c9..49749426 100644 --- a/code/controllers/CMSPageAddController.php +++ b/code/controllers/CMSPageAddController.php @@ -41,9 +41,11 @@ class CMSPageAddController extends CMSPageEditController { $fields = new FieldList( // new HiddenField("ParentID", false, ($this->parentRecord) ? $this->parentRecord->ID : null), // TODO Should be part of the form attribute, but not possible in current form API - $hintsField = new LiteralField('Hints', sprintf('', $this->SiteTreeHints())), + $hintsField = new LiteralField( + 'Hints', + sprintf('', Convert::raw2xml($this->SiteTreeHints())) + ), new LiteralField('PageModeHeader', sprintf($numericLabelTmpl, 1, _t('CMSMain.ChoosePageParentMode', 'Choose where to create this page'))), - $parentModeField = new SelectionGroup( "ParentModeField", array( @@ -62,6 +64,16 @@ class CMSPageAddController extends CMSPageEditController { sprintf($numericLabelTmpl, 2, _t('CMSMain.ChoosePageType', 'Choose page type')), $pageTypes, 'Page' + ), + new LiteralField( + 'RestrictedNote', + sprintf( + '

%s

', + _t( + 'CMSMain.AddPageRestriction', + 'Note: Some page types are not allowed for this selection' + ) + ) ) ); // TODO Re-enable search once it allows for HTML title display, diff --git a/javascript/CMSMain.AddForm.js b/javascript/CMSMain.AddForm.js index d62e906b..c1a9511b 100644 --- a/javascript/CMSMain.AddForm.js +++ b/javascript/CMSMain.AddForm.js @@ -34,17 +34,22 @@ var hints = this.find('.hints').data('hints'), metadata = this.find('#ParentID .TreeDropdownField').data('metadata'), id = this.find('#ParentID .TreeDropdownField').getValue(), - newClassName = metadata ? metadata.ClassName : null, - hintKey = newClassName ? newClassName : 'Root', - hint = (typeof hints[hintKey] != 'undefined') ? hints[hintKey] : null; + newClassName = (id && metadata) ? metadata.ClassName : null, + hintKey = (newClassName) ? newClassName : 'Root', + hint = (typeof hints[hintKey] != 'undefined') ? hints[hintKey] : null, + allAllowed = true; var disallowedChildren = (hint && typeof hint.disallowedChildren != 'undefined') ? hint.disallowedChildren : [], defaultChildClass = (hint && typeof hint.defaultChild != 'undefined') ? hint.defaultChild : null; // Limit selection this.find('#PageType li').each(function() { - var className = $(this).find('input').val(), isAllowed = ($.inArray(className, disallowedChildren) == -1); + var className = $(this).find('input').val(), + isAllowed = ($.inArray(className, disallowedChildren) == -1); + $(this).setEnabled(isAllowed); + if(!isAllowed) $(this).setSelected(false); + allAllowed = allAllowed && isAllowed; }); // Set default child selection, or fall back to first available option @@ -59,6 +64,8 @@ // Disable the "Create" button if none of the pagetypes are available var buttonState = (this.find('#PageType li:not(.disabled)').length) ? 'enable' : 'disable'; this.find('button[name=action_doAdd]').button(buttonState); + + this.find('.message-restricted')[allAllowed ? 'hide' : 'show'](); } }); diff --git a/javascript/CMSMain.Tree.js b/javascript/CMSMain.Tree.js index 73b41c5c..285f82fc 100644 --- a/javascript/CMSMain.Tree.js +++ b/javascript/CMSMain.Tree.js @@ -7,43 +7,58 @@ config.plugins.push('contextmenu'); config.contextmenu = { 'items': function(node) { + + var menuitems = { + 'edit': { + 'label': ss.i18n._t('Tree.EditPage'), + 'action': function(obj) { + $('.cms-container').entwine('.ss').loadPanel(ss.i18n.sprintf( + self.data('urlEditpage'), obj.data('id') + )); + } + } + }; // Build a list for allowed children as submenu entries - var pagetype = node.data('pagetype'); - var id = node.data('id'); + var pagetype = node.data('pagetype'), + id = node.data('id'), + disallowedChildren = hints[pagetype].disallowedChildren, + allowedChildren = $.extend(true, {}, hints['All']), // clone + disallowedClass, + menuAllowedChildren = {}, + hasAllowedChildren = false; - var allowedChildren = new Object; - $(hints[pagetype].allowedChildren).each( - function(key, val){ - allowedChildren["allowedchildren-" + key ] = { - 'label': '' + val.ssname, - '_class': 'class-' + val.ssclass, - 'action': function(obj) { - $('.cms-container').entwine('.ss').loadPanel(ss.i18n.sprintf( - self.data('urlAddpage'), id, val.ssclass - )); - } - }; + // Filter allowed + if(disallowedChildren) { + for(var i=0; i' + klassData.title, + '_class': 'class-' + klass, + 'action': function(obj) { + $('.cms-container').entwine('.ss').loadPanel(ss.i18n.sprintf( + self.data('urlAddpage'), id, klass + )); } }; - // Test if there are any allowed Children and thus the possibility of adding some - if(allowedChildren.hasOwnProperty('allowedchildren-0')) { + }); + + if(hasAllowedChildren) { menuitems['addsubpage'] = { - 'label': ss.i18n._t('Tree.AddSubPage'), - 'submenu': allowedChildren - }; - } + 'label': ss.i18n._t('Tree.AddSubPage'), + 'submenu': menuAllowedChildren + }; + } + return menuitems; } }; diff --git a/templates/Includes/CMSMain_TreeView.ss b/templates/Includes/CMSMain_TreeView.ss index b2cf9e0f..dfb54060 100644 --- a/templates/Includes/CMSMain_TreeView.ss +++ b/templates/Includes/CMSMain_TreeView.ss @@ -23,7 +23,7 @@ $ExtraTreeTools <% end_if %> -
+
$SiteTreeAsUL
diff --git a/tests/controller/CMSMainTest.php b/tests/controller/CMSMainTest.php index b087841e..f3d3ac78 100644 --- a/tests/controller/CMSMainTest.php +++ b/tests/controller/CMSMainTest.php @@ -25,6 +25,62 @@ class CMSMainTest extends FunctionalTest { parent::tearDownOnce(); } + + function testSiteTreeHints() { + $cache = SS_Cache::factory('CMSMain_SiteTreeHints'); + $cache->clean(Zend_Cache::CLEANING_MODE_ALL); + + $rawHints = singleton('CMSMain')->SiteTreeHints(); + $this->assertNotNull($rawHints); + + $rawHints = preg_replace('/^"(.*)"$/', '$1', Convert::xml2raw($rawHints)); + $hints = Convert::json2array($rawHints); + + $this->assertArrayHasKey('Root', $hints); + $this->assertArrayHasKey('Page', $hints); + $this->assertArrayHasKey('All', $hints); + + $this->assertArrayHasKey( + 'CMSMainTest_ClassA', + $hints['All'], + 'Global list shows allowed classes' + ); + + $this->assertArrayNotHasKey( + 'CMSMainTest_HiddenClass', + $hints['All'], + 'Global list does not list hidden classes' + ); + + $this->assertNotContains( + 'CMSMainTest_ClassA', + $hints['Root']['disallowedChildren'], + 'Limits root classes' + ); + + $this->assertContains( + 'CMSMainTest_NotRoot', + $hints['Root']['disallowedChildren'], + 'Limits root classes' + ); + $this->assertNotContains( + 'CMSMainTest_ClassA', + // Lenient checks because other modules might influence state + (array)@$hints['Page']['disallowedChildren'], + 'Does not limit types on unlimited parent' + ); + $this->assertContains( + 'Page', + $hints['CMSMainTest_ClassA']['disallowedChildren'], + 'Limited parent lists disallowed classes' + ); + $this->assertNotContains( + 'CMSMainTest_ClassB', + $hints['CMSMainTest_ClassA']['disallowedChildren'], + 'Limited parent omits explicitly allowed classes in disallowedChildren' + ); + + } /** * @todo Test the results of a publication better @@ -293,4 +349,12 @@ class CMSMainTest_ClassA extends Page implements TestOnly { class CMSMainTest_ClassB extends Page implements TestOnly { +} + +class CMSMainTest_NotRoot extends Page implements TestOnly { + static $can_be_root = false; +} + +class CMSMainTest_HiddenClass extends Page implements TestOnly, HiddenClass { + } \ No newline at end of file