Merge pull request #27 from tractorcow/pulls/fix-submission

BUG Fix form submission

code/extensions/UserFormFieldEditorExtension.php

@@ -37,7 +37,7 @@ class UserFormFieldEditorExtension extends DataExtension {
 		$this->createInitialFormStep(true);
 
 		$editableColumns = new GridFieldEditableColumns();
-		$fieldClasses = $this->getEditableFieldClasses();
+		$fieldClasses = singleton('EditableFormField')->getEditableFieldClasses();
 		$editableColumns->setDisplayFields(array(
 			'ClassName' => function($record, $column, $grid) use ($fieldClasses) {
 				if($record instanceof EditableFormField) {
@@ -121,32 +121,6 @@ class UserFormFieldEditorExtension extends DataExtension {
 		$fields->add($step);
 	}
 
-	/**
-	 * @return array
-	 */
-	public function getEditableFieldClasses() {
-		$classes = ClassInfo::getValidSubClasses('EditableFormField');
-
-		// Remove classes we don't want to display in the dropdown.
-		$editableFieldClasses = array();
-		foreach ($classes as $class) {
-			if(in_array($class, array('EditableFormField', 'EditableMultipleOptionField'))
-				|| Config::inst()->get($class, 'hidden')
-			) {
-				continue;
-			}
-			
-			$singleton = singleton($class);
-			if(!$singleton->canCreate()) {
-				continue;
-			}
-
-			$editableFieldClasses[$class] = $singleton->i18n_singular_name();
-		}
-
-		return $editableFieldClasses;
-	}
-
 	/**
 	 * Ensure that at least one page exists at the start
 	 */

code/formfields/UserFormsStepField.php

@@ -4,4 +4,41 @@
  * Represents a page step in a form, which may contain form fields or other groups
  */
 class UserFormsStepField extends UserFormsCompositeField {
+
+	private static $casting = array(
+		'StepNumber' => 'Int'
+	);
+
+	/**
+	 * Numeric index (1 based) of this step
+	 *
+	 * Null if unassigned
+	 *
+	 * @var int|null
+	 */
+	protected $number = null;
+
+	public function FieldHolder($properties = array()) {
+		return $this->Field($properties);
+	}
+
+	/**
+	 * Get the step number
+	 *
+	 * @return int|null
+	 */
+	public function getStepNumber() {
+		return $this->number;
+	}
+
+	/**
+	 * Re-assign this step to another number
+	 *
+	 * @param type $number
+	 * @return $this
+	 */
+	public function setStepNumber($number) {
+		$this->number = $number;
+		return $this;
+	}
 }

code/forms/UserForm.php

@@ -22,6 +22,12 @@ class UserForm extends Form {
 			$this->getRequiredFields()
 		);
 
+		// Number each page
+		$stepNumber = 1;
+		foreach($this->getSteps() as $step) {
+			$step->setStepNumber($stepNumber++);
+		}
+
 		if($controller->DisableCsrfSecurityToken) {
 			$this->disableSecurityToken();
 		}
@@ -45,24 +51,21 @@ class UserForm extends Form {
 	}
 
 	/**
-	 * @return int
+	 * @return bool
 	 */
 	public function getDisplayErrorMessagesAtTop() {
-		return $this->controller->DisplayErrorMessagesAtTop;
+		return (bool)$this->controller->DisplayErrorMessagesAtTop;
 	}
 
 	/**
-	 * @return array
+	 * Return the fieldlist, filtered to only contain steps
+	 *
+	 * @return ArrayList
 	 */
-	public function getNumberOfSteps() {
-		$steps = new ArrayList();
-		$numberOfSteps = $this->controller->Fields()->filter('ClassName', 'EditableFormStep')->Count();
-
-		for($i = 0; $i < $numberOfSteps; $i++) {
-			$steps->push($i);
-		}
-
-		return $steps;
+	public function getSteps() {
+		return $this->Fields()->filterByCallback(function($field) {
+			return $field instanceof UserFormsStepField;
+		});
 	}
 
 	/**
@@ -123,9 +126,7 @@ class UserForm extends Form {
 			->filter('Required', true)
 			->column('Name');
 		$required = new RequiredFields($requiredNames);
-		
 		$this->extend('updateRequiredFields', $required);
-
 		return $required;
 	}
 

code/model/UserDefinedForm.php

@@ -394,15 +394,11 @@ class UserDefinedForm_Controller extends Page_Controller {
 
 		if($this->Fields()) {
 			foreach($this->Fields() as $field) {
-				$fieldId = $field->Name;
-
-				if($field instanceof EditableFormHeading) {
-					$fieldId = 'UserForm_Form_' . $field->Name;
-				}
+				$holderSelector = $field->getSelectorHolder();
 
 				// Is this Field Show by Default
 				if(!$field->ShowOnLoad) {
-					$default .= "$(\"#" . $fieldId . "\").hide();\n";
+					$default .= "{$holderSelector}.hide();\n";
 				}
 
 				// Check for field dependencies / default
@@ -411,22 +407,8 @@ class UserDefinedForm_Controller extends Page_Controller {
 					// Get the field which is effected
 					$formFieldWatch = EditableFormField::get()->byId($rule->ConditionFieldID);
 
-					if($formFieldWatch->RecordClassName == 'EditableDropdown') {
-						// watch out for multiselect options - radios and check boxes
-						$fieldToWatch = "$(\"select[name='" . $formFieldWatch->Name . "']\")";
-						$fieldToWatchOnLoad = $fieldToWatch;
-					} else if($formFieldWatch->RecordClassName == 'EditableCheckboxGroupField') {
-						// watch out for checkboxs as the inputs don't have values but are 'checked
-						$fieldToWatch = "$(\"input[name='" . $formFieldWatch->Name . "[" . $rule->FieldValue . "]']\")";
-						$fieldToWatchOnLoad = $fieldToWatch;
-					} else if($formFieldWatch->RecordClassName == 'EditableRadioField') {
-						$fieldToWatch = "$(\"input[name='" . $formFieldWatch->Name . "']\")";
-						// We only want to trigger on load once for the radio group - hence we focus on the first option only.
-						$fieldToWatchOnLoad = "$(\"input[name='" . $formFieldWatch->Name . "']:first\")";
-					} else {
-						$fieldToWatch = "$(\"input[name='" . $formFieldWatch->Name . "']\")";
-						$fieldToWatchOnLoad = $fieldToWatch;
-					}
+					$fieldToWatch = $formFieldWatch->getSelectorField($rule);
+					$fieldToWatchOnLoad = $formFieldWatch->getSelectorField($rule, true);
 
 					// show or hide?
 					$view = ($rule->Display == 'Hide') ? 'hide' : 'show';
@@ -436,7 +418,7 @@ class UserDefinedForm_Controller extends Page_Controller {
 					// @todo encapulsation
 					$action = "change";
 
-					if($formFieldWatch->ClassName == "EditableTextField") {
+					if($formFieldWatch instanceof EditableTextField) {
 						$action = "keyup";
 					}
 
@@ -454,11 +436,11 @@ class UserDefinedForm_Controller extends Page_Controller {
 					// and what should we evaluate
 					switch($rule->ConditionOption) {
 						case 'IsNotBlank':
-							$expression = ($checkboxField || $radioField) ? '$(this).prop("checked")' :'$(this).val() != ""';
+							$expression = ($checkboxField || $radioField) ? '$(this).is(":checked")' :'$(this).val() != ""';
 
 							break;
 						case 'IsBlank':
-							$expression = ($checkboxField || $radioField) ? '!($(this).prop("checked"))' : '$(this).val() == ""';
+							$expression = ($checkboxField || $radioField) ? '!($(this).is(":checked"))' : '$(this).val() == ""';
 							
 							break;
 						case 'HasValue':
@@ -507,7 +489,7 @@ class UserDefinedForm_Controller extends Page_Controller {
 
 					$watch[$fieldToWatch][] =  array(
 						'expression' => $expression,
-						'field_id' => $fieldId,
+						'holder_selector' => $holderSelector,
 						'view' => $view,
 						'opposite' => $opposite,
 						'action' => $action
@@ -526,9 +508,9 @@ class UserDefinedForm_Controller extends Page_Controller {
 				foreach($values as $rule) {
 					// Register conditional behaviour with an element, so it can be triggered from many places.
 					$logic[] = sprintf(
-						'if(%s) { $("#%s").%s(); } else { $("#%2$s").%s(); }', 
+						'if(%s) { %s.%s(); } else { %2$s.%s(); }', 
 						$rule['expression'], 
-						$rule['field_id'], 
+						$rule['holder_selector'],
 						$rule['view'], 
 						$rule['opposite']
 					);

code/model/editableformfields/EditableCheckboxGroupField.php

@@ -44,4 +44,14 @@ class EditableCheckboxGroupField extends EditableMultipleOptionField {
 		}
 		return $result;
 	}
+
+	public function getSelectorField(EditableCustomRule $rule, $forOnLoad = false) {
+		// watch out for checkboxs as the inputs don't have values but are 'checked
+		// @todo - Test this
+		if($rule->FieldValue) {
+			return "$(\"input[name='{$this->Name}[]'][value='{$rule->FieldValue}']\")";
+		} else {
+			return "$(\"input[name='{$this->Name}[]']:first\")";
+		}
+	}
 }

code/model/editableformfields/EditableCountryDropdownField.php

@@ -38,4 +38,8 @@ class EditableCountryDropdownField extends EditableFormField {
 	public function getIcon() {
 		return  USERFORMS_DIR . '/images/editabledropdown.png';
 	}
+
+	public function getSelectorField(EditableCustomRule $rule, $forOnLoad = false) {
+		return "$(\"select[name='{$this->Name}']\")";
+	}
 }

code/model/editableformfields/EditableDropdown.php

@@ -38,4 +38,8 @@ class EditableDropdown extends EditableMultipleOptionField {
 		$this->doUpdateFormField($field);
 		return $field;
 	}
+
+	public function getSelectorField(EditableCustomRule $rule, $forOnLoad = false) {
+		return "$(\"select[name='{$this->Name}']\")";
+	}
 }

code/model/editableformfields/EditableFieldGroup.php

@@ -17,6 +17,13 @@ class EditableFieldGroup extends EditableFormField {
 	 */
 	private static $hidden = true;
 
+	/**
+	 * Non-data field type
+	 *
+	 * @var type
+	 */
+	private static $literal = true;
+
 	public function getCMSFields() {
 		$fields = parent::getCMSFields();
 		$fields->removeByName(array('MergeField', 'Default', 'Validation', 'DisplayRules'));

code/model/editableformfields/EditableFieldGroupEnd.php

@@ -16,6 +16,14 @@ class EditableFieldGroupEnd extends EditableFormField {
 	 * @var bool
 	 */
 	private static $hidden = true;
+	
+	/**
+	 * Non-data type
+	 *
+	 * @config
+	 * @var bool
+	 */
+	private static $literal = true;
 
 	public function getCMSTitle() {
 		$group = $this->Group();

code/model/editableformfields/EditableFormField.php

@@ -16,6 +16,22 @@ class EditableFormField extends DataObject {
 	 */
 	private static $hidden = false;
 
+	/**
+	 * Define this field as abstract (not inherited)
+	 *
+	 * @config
+	 * @var bool
+	 */
+	private static $abstract = true;
+
+	/**
+	 * Flag this field type as non-data (e.g. literal, header, html)
+	 *
+	 * @config
+	 * @var bool
+	 */
+	private static $literal = false;
+
 	/**
 	 * Default sort order
 	 *
@@ -176,21 +192,25 @@ class EditableFormField extends DataObject {
 		}
 
 		// Validation
-		$fields->addFieldsToTab(
-			'Root.Validation',
-			$this->getFieldValidationOptions()
-		);
-
+		$validationFields = $this->getFieldValidationOptions();
+		if($validationFields) {
+			$fields->addFieldsToTab(
+				'Root.Validation',
+				$this->getFieldValidationOptions()
+			);
+		}
+		$allowedClasses = array_keys($this->getEditableFieldClasses(false));
 		$editableColumns = new GridFieldEditableColumns();
 		$editableColumns->setDisplayFields(array(
 			'Display' => '',
-			'ConditionFieldID' => function($record, $column, $grid) {
+			'ConditionFieldID' => function($record, $column, $grid) use ($allowedClasses) {
 				return DropdownField::create(
 					$column,
 					'',
 					EditableFormField::get()
 						->filter(array(
-							'ParentID' => $this->ParentID
+							'ParentID' => $this->ParentID,
+							'ClassName' => $allowedClasses
 						))
 						->exclude(array(
 							'ID' => $this->ID
@@ -651,4 +671,57 @@ class EditableFormField extends DataObject {
 			->setAttribute('placeholder', _t('EditableFormField.TITLE', 'Title'))
 			->setAttribute('data-placeholder', _t('EditableFormField.TITLE', 'Title'));
 	}
+
+	/**
+	 * Get the JS expression for selecting the holder for this field
+	 *
+	 * @return string
+	 */
+	public function getSelectorHolder() {
+		return "$(\"#{$this->Name}\")";
+	}
+
+	/**
+	 * Gets the JS expression for selecting the value for this field
+	 *
+	 * @param EditableCustomRule $rule Custom rule this selector will be used with
+	 * @param bool $forOnLoad Set to true if this will be invoked on load
+	 */
+	public function getSelectorField(EditableCustomRule $rule, $forOnLoad = false) {
+		return "$(\"input[name='{$this->Name}']\")";
+	}
+
+
+	/**
+	 * Get the list of classes that can be selected and used as data-values
+	 *
+	 * @param $includeLiterals Set to false to exclude non-data fields
+	 * @return array
+	 */
+	public function getEditableFieldClasses($includeLiterals = true) {
+		$classes = ClassInfo::getValidSubClasses('EditableFormField');
+
+		// Remove classes we don't want to display in the dropdown.
+		$editableFieldClasses = array();
+		foreach ($classes as $class) {
+			// Skip abstract / hidden classes
+			if(Config::inst()->get($class, 'abstract', Config::UNINHERITED) || Config::inst()->get($class, 'hidden')
+			) {
+				continue;
+			}
+
+			if(!$includeLiterals && Config::inst()->get($class, 'literal')) {
+				continue;
+			}
+
+			$singleton = singleton($class);
+			if(!$singleton->canCreate()) {
+				continue;
+			}
+
+			$editableFieldClasses[$class] = $singleton->i18n_singular_name();
+		}
+
+		return $editableFieldClasses;
+	}
 }

code/model/editableformfields/EditableFormHeading.php

@@ -11,6 +11,8 @@ class EditableFormHeading extends EditableFormField {
 	
 	private static $plural_name = 'Headings';
 
+	private static $literal = true;
+
 	private static $db = array(
 		'Level' => 'Int(3)', // From CustomSettings
 		'HideFromReports' => 'Boolean(0)' // from CustomSettings
@@ -57,6 +59,7 @@ class EditableFormHeading extends EditableFormField {
 	public function getFormField() {
 		$labelField = new HeaderField($this->Name, $this->EscapedTitle, $this->Level);
 		$labelField->addExtraClass('FormHeading');
+		$labelField->setAttribute('data-id', $this->Name);
 		$this->doUpdateFormField($labelField);
 		return $labelField;
 	}
@@ -80,4 +83,8 @@ class EditableFormHeading extends EditableFormField {
 	public function getFieldValidationOptions() {
 		return false;
 	}
+
+	public function getSelectorHolder() {
+		return "$(\":header[data-id='{$this->Name}']\")";
+	}
 }

code/model/editableformfields/EditableLiteralField.php

@@ -13,6 +13,14 @@ class EditableLiteralField extends EditableFormField {
 	
 	private static $plural_name = 'HTML Blocks';
 
+	/**
+	 * Mark as literal only
+	 *
+	 * @config
+	 * @var bool
+	 */
+	private static $literal = true;
+
 	/**
 	 * Get the name of the editor config to use for HTML sanitisation. Defaults to the active config.
 	 *

code/model/editableformfields/EditableMultipleOptionField.php

@@ -14,6 +14,14 @@
  */
 
 class EditableMultipleOptionField extends EditableFormField {
+
+	/**
+	 * Define this field as abstract (not inherited)
+	 *
+	 * @config
+	 * @var bool
+	 */
+	private static $abstract = true;
 	
 	private static $has_many = array(
 		"Options" => "EditableOption"

code/model/editableformfields/EditableRadioField.php

@@ -35,4 +35,10 @@ class EditableRadioField extends EditableMultipleOptionField {
 		$this->doUpdateFormField($field);
 		return $field;
 	}
+
+	public function getSelectorField(EditableCustomRule $rule, $forOnLoad = false) {
+		// We only want to trigger on load once for the radio group - hence we focus on the first option only.
+		$first = $forOnLoad ? ':first' : '';
+		return "$(\"input[name='{$this->Name}']{$first}\")";
+	}
 }

javascript/UserForm.js

@@ -139,7 +139,7 @@ jQuery(function ($) {
 	 */
 	UserForm.prototype.setCurrentStep = function (step) {
 		// Make sure we're dealing with a form step.
-		if (!step instanceof FormStep) {
+		if (!(step instanceof FormStep)) {
 			return;
 		}
 

templates/Includes/UserFormProgress.ss

@@ -1,20 +1,18 @@
-<% cached "UserForms_Navigation", $LastEdited %>
-<% if $NumberOfSteps.Count > "1" %>
-<div id="userform-progress" class="userform-progress" aria-hidden="true" style="display:none;">
-	<h2 class="progress-title"></h2>
-	<p>Step <span class="current-step-number">1</span> of $NumberOfSteps.Count</p>
-	<div class="progress">
-		<div class="progress-bar" role="progressbar" aria-valuenow="1" aria-valuemin="1" aria-valuemax="$NumberOfSteps.Count"></div>
-	</div>
-	<nav>
-		<ul class="step-buttons">
-			<% loop $NumberOfSteps %>
-				<li class="step-button-wrapper<% if $Pos == '1' %> current<% end_if %>">
+<% if $Steps.Count > 1 %>
+	<div id="userform-progress" class="userform-progress" aria-hidden="true" style="display:none;">
+		<h2 class="progress-title"></h2>
+		<p>Step <span class="current-step-number">1</span> of $NumberOfSteps.Count</p>
+		<div class="progress">
+			<div class="progress-bar" role="progressbar" aria-valuenow="1" aria-valuemin="1" aria-valuemax="$NumberOfSteps.Count"></div>
+		</div>
+		<nav>
+			<ul class="step-buttons">
+				<% loop $Steps %>
+					<li class="step-button-wrapper<% if $First %> current<% end_if %>">
 					<button class="step-button-jump js-align" disabled="disabled">$Pos</button><!-- Remove js-align class to remove javascript positioning -->
-				</li>
-			<% end_loop %>
-		<ul>
-	</nav>
-</div>
+					</li>
+				<% end_loop %>
+			</ul>
+		</nav>
+	</div>
 <% end_if %>
-<% end_cached %>

templates/Includes/UserFormStepErrors.ss

@@ -0,0 +1,8 @@
+<% if $Steps.Count > 1 %>
+	<fieldset class="error-container form-wide-errors" aria-hidden="true" style="display: none;">
+		<div>
+			<h4></h4>
+			<ul class="error-list"></ul>
+		</div>
+	</fieldset>
+<% end_if %>

templates/Includes/UserFormStepNav.ss

@@ -1,29 +1,21 @@
-<% if $FirstLast == "first last" %>
-<% else %>
-<nav class="step-navigation" aria-hidden="true" style="display:none;">
-	<ul class="step-buttons">
-		<% if $FirstLast == "first" %>
-		<% else %>
-		<li class="step-button-wrapper">
-			<button class="step-button-prev">Prev</button>
-		</li>
-		<% end_if %>
+<% if $Form.Steps.Count > 1 %>
+	<nav class="step-navigation" aria-hidden="true" style="display:none;">
+		<ul class="step-buttons">
+			<% if $StepNumber > 1 %>
+				<li class="step-button-wrapper">
+					<button class="step-button-prev">Prev</button>
+				</li>
+			<% end_if %>
 
-		<% if $FirstLast == "last" %>
-
-		<% if $ContainingPage.Actions %>
-		<% loop $ContainingPage.Actions %>
-		<li class="step-button-wrapper">
-			$Field
-		</li>
-		<% end_loop %>
-		<% end_if %>
-
-		<% else %>
-		<li class="step-button-wrapper">
-			<button class="step-button-next">Next</button>
-		</li>
-		<% end_if %>
-	</ul>
-</nav>
+			<% if $StepNumber < $Form.Steps.Count %>
+				<li class="step-button-wrapper">
+					<button class="step-button-next">Next</button>
+				</li>
+			<% else_if $Form.Actions %><% loop $Form.Actions %>
+				<li class="step-button-wrapper">
+					$Field
+				</li>
+			<% end_loop %><% end_if %>
+		</ul>
+	</nav>
 <% end_if %>

templates/UserForm.ss

@@ -1,44 +1,19 @@
-<% include UserFormProgress %>
-
 <form $AttributesHTML>
+	
+<% include UserFormProgress %>
+<% include UserFormStepErrors %>
 
 <% if $Message %>
-<p id="{$FormName}_error" class="message $MessageType">$Message</p>
+	<p id="{$FormName}_error" class="message $MessageType">$Message</p>
 <% else %>
-<p id="{$FormName}_error" class="message $MessageType" aria-hidden="true" style="display: none;"></p>
-<% end_if %>
-
-<% if $NumberOfSteps.Count > "1" %>
-<fieldset class="error-container form-wide-errors" aria-hidden="true" style="display: none;">
-	<div>
-		<h4></h4>
-		<ul class="error-list"></ul>
-	</div>
-</fieldset>
+	<p id="{$FormName}_error" class="message $MessageType" aria-hidden="true" style="display: none;"></p>
 <% end_if %>
 
 <fieldset>
 	<% if $Legend %><legend>$Legend</legend><% end_if %>
-
-	<% if $FormFields%><% loop $FormFields %>
-		<fieldset id="step-$Pos" class="form-step" data-title="$Title">
-			<% if $Top.DisplayErrorMessagesAtTop %>
-				<fieldset class="error-container" aria-hidden="true" style="display: none;">
-					<div>
-						<h4></h4>
-						<ul class="error-list"></ul>
-					</div>
-				</fieldset>
-			<% end_if %>
-
-			<% loop $Children %>
-			$FieldHolder
-			<% end_loop %>
-
-			<% include UserFormStepNav ContainingPage=$Top %>
-		</fieldset>
-	<% end_loop %><% end_if %>
-
+	<% loop $Fields %>
+		$FieldHolder
+	<% end_loop %>
 	<div class="clear"><!-- --></div>
 </fieldset>
 

templates/UserFormsStepField.ss

@@ -0,0 +1,16 @@
+<fieldset id="$Name" class="form-step $extraClass" data-title="$Title">
+	<% if $Form.DisplayErrorMessagesAtTop %>
+		<fieldset class="error-container" aria-hidden="true" style="display: none;">
+			<div>
+				<h4></h4>
+				<ul class="error-list"></ul>
+			</div>
+		</fieldset>
+	<% end_if %>
+	
+	<% loop $Children %>
+		$FieldHolder
+	<% end_loop %>
+
+	<% include UserFormStepNav %>
+</fieldset>