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()
silverstripe · Dec 3, 2012 · f9a5601 · f9a5601
1 parent 98cd09a
commit f9a5601
Show file tree

Hide file tree

Showing 6 changed files with 180 additions and 70 deletions.
diff --git a/code/controllers/CMSMain.php b/code/controllers/CMSMain.php
@@ -364,7 +364,6 @@ public function Breadcrumbs($unlinked = false) {
 	 */
 	public function SiteTreeHints() {
 		$json = '';
-
 		$classes = SiteTree::page_type_classes();
 
 	 	$cacheCanCreate = array();
@@ -377,55 +376,68 @@ public function SiteTreeHints() {
 	 	$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);
-				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();
+				$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;
+				}
 
-				// Exclude SiteTree from possible Children
-				$possibleChildren = array_diff($allowedChildren, array("SiteTree"));
+				if(
+					($obj instanceof HiddenClass)
+					|| (!array_key_exists($class, $cacheCanCreate) || !$cacheCanCreate[$class])
+					|| ($needsPerm && !$this->can($needsPerm))
+				) {
+					$globalDisallowed[] = $class;
+					$def['Root']['disallowedChildren'][] = $class;
+				}
+			}
 
-				// Find i18n - names and build allowed children array
-				foreach($possibleChildren as $child) {
-					$instance = singleton($child);
-
-					if($instance instanceof HiddenClass) continue;
+			// Set disallows by class
+			foreach($classes as $class) {
+				$obj = singleton($class);
+				if($obj instanceof HiddenClass) continue;
 
-					if(!array_key_exists($child, $cacheCanCreate) || !$cacheCanCreate[$child]) continue;
+				$def[$class] = array();
 
-					// skip this type if it is restricted
-					if($instance->stat('need_permission') && !$this->can(singleton($class)->stat('need_permission'))) continue;
+				$allowed = $obj->allowedChildren();
+				if($pos = array_search('SiteTree', $allowed)) unset($allowed[$pos]);
 
-					$title = $instance->i18n_singular_name();
+				// 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;
 
-					$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
@@ -41,9 +41,11 @@ public function AddForm() {
 		$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('<span class="hints" data-hints="%s"></span>', $this->SiteTreeHints())),
+			$hintsField = new LiteralField(
+				'Hints', 
+				sprintf('<span class="hints" data-hints="%s"></span>', 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 @@ public function AddForm() {
 				sprintf($numericLabelTmpl, 2, _t('CMSMain.ChoosePageType', 'Choose page type')), 
 				$pageTypes, 
 				'Page'
+			),
+			new LiteralField(
+				'RestrictedNote',
+				sprintf(
+					'<p class="message notice message-restricted">%s</p>',
+					_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
@@ -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
@@ -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': '<span class="jstree-pageicon"></span>' + 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<disallowedChildren.length; i++) {
+								disallowedClass = disallowedChildren[i];
+								if(allowedChildren[disallowedClass]) {
+									delete allowedChildren[disallowedClass];
+								}
 							}
-						);
-						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')
-										));
-									}
+						}
+
+						// Convert to menu entries
+						$.each(allowedChildren, function(klass, klassData){
+							hasAllowedChildren = true;
+							menuAllowedChildren["allowedchildren-" + klass ] = {
+								'label': '<span class="jstree-pageicon"></span>' + 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
@@ -23,7 +23,7 @@ $ExtraTreeTools
 	</div>
 	<% end_if %>
 
-	<div class="cms-tree" data-url-tree="$Link(getsubtree)" data-url-savetreenode="$Link(savetreenode)" data-url-updatetreenodes="$Link(updatetreenodes)" data-url-addpage="{$LinkPageAdd('AddForm/?action_doAdd=1')}&amp;ParentID=%s&amp;PageType=%s&amp;SecurityID=$SecurityID" data-url-editpage="$LinkPageEdit('%s')" data-hints="$SiteTreeHints">
+	<div class="cms-tree" data-url-tree="$Link(getsubtree)" data-url-savetreenode="$Link(savetreenode)" data-url-updatetreenodes="$Link(updatetreenodes)" data-url-addpage="{$LinkPageAdd('AddForm/?action_doAdd=1')}&amp;ParentID=%s&amp;PageType=%s&amp;SecurityID=$SecurityID" data-url-editpage="$LinkPageEdit('%s')" data-hints="$SiteTreeHints.XML">
 		$SiteTreeAsUL
 	</div>
 </div>
diff --git a/tests/controller/CMSMainTest.php b/tests/controller/CMSMainTest.php
@@ -25,6 +25,62 @@ public function tearDownOnce() {
 
 		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 {
+
 }