From 5349bb7d3b508eae832b241dea13df82bd46477e Mon Sep 17 00:00:00 2001 From: Damian Mooyman Date: Fri, 11 Dec 2015 17:38:31 +1300 Subject: [PATCH] API Use core validation for form submission Fixes #350 and fixes #404 This change abandons validation via EditableFormField::validateField, as it bypassed too many core validation mechanisms (RequiredFields, etc). In order to enforce consistency of editable field validation, display rules have been hard-disabled when a field is marked as required. Since this removes functionality, I have incremented the major version number --- code/extensions/UserFormValidator.php | 2 +- code/forms/UserForm.php | 41 +++--- code/model/UserDefinedForm.php | 2 +- .../editableformfields/EditableFormField.php | 124 ++++++++++-------- composer.json | 2 +- tests/EditableFormFieldTest.php | 7 +- tests/EditableLiteralFieldTest.php | 7 - tests/UserDefinedFormControllerTest.php | 71 +++++++--- tests/UserDefinedFormTest.yml | 17 ++- 9 files changed, 164 insertions(+), 109 deletions(-) diff --git a/code/extensions/UserFormValidator.php b/code/extensions/UserFormValidator.php index e817715..72440aa 100644 --- a/code/extensions/UserFormValidator.php +++ b/code/extensions/UserFormValidator.php @@ -22,7 +22,7 @@ class UserFormValidator extends RequiredFields { // Page at top level, or after another page is ok if(empty($stack) || (count($stack) === 1 && $stack[0] instanceof EditableFormStep)) { $stack = array($field); - $conditionalStep = $field->DisplayRules()->count() > 0; + $conditionalStep = $field->EffectiveDisplayRules()->count() > 0; continue; } diff --git a/code/forms/UserForm.php b/code/forms/UserForm.php index 2b69da5..c13ea0e 100644 --- a/code/forms/UserForm.php +++ b/code/forms/UserForm.php @@ -27,6 +27,9 @@ class UserForm extends Form { $actions->setForm($this); $this->setValidator($this->getRequiredFields()); + // This needs to be re-evaluated since fields have been assigned + $this->setupFormErrors(); + // Number each page $stepNumber = 1; foreach($this->getSteps() as $step) { @@ -46,6 +49,16 @@ class UserForm extends Form { $this->extend('updateForm'); } + public function setupFormErrors() + { + // Suppress setupFormErrors if fields haven't been bootstrapped + if($this->fields && $this->fields->exists()) { + return parent::setupFormErrors(); + } + + return $this; + } + /** * Used for partial caching in the template. * @@ -90,6 +103,7 @@ class UserForm extends Form { } $fields->clearEmptySteps(); $this->extend('updateFormFields', $fields); + $fields->setForm($this); return $fields; } @@ -115,7 +129,7 @@ class UserForm extends Form { } $this->extend('updateFormActions', $actions); - + $actions->setForm($this); return $actions; } @@ -127,37 +141,16 @@ class UserForm extends Form { public function getRequiredFields() { // Generate required field validator $requiredNames = $this - ->controller + ->getController() ->Fields() ->filter('Required', true) ->column('Name'); $required = new RequiredFields($requiredNames); $this->extend('updateRequiredFields', $required); + $required->setForm($this); return $required; } - /** - * Override validation so conditional fields can be validated correctly. - * - * @return boolean - */ - public function validate() { - $data = $this->getData(); - - Session::set("FormInfo.{$this->FormName()}.data", $data); - Session::clear("FormInfo.{$this->FormName()}.errors"); - - foreach ($this->controller->Fields() as $key => $field) { - $field->validateField($data, $this); - } - - if(Session::get("FormInfo.{$this->FormName()}.errors")) { - return false; - } - - return true; - } - /** * Override some we can add UserForm specific attributes to the form. * diff --git a/code/model/UserDefinedForm.php b/code/model/UserDefinedForm.php index 2592526..a50c165 100755 --- a/code/model/UserDefinedForm.php +++ b/code/model/UserDefinedForm.php @@ -426,7 +426,7 @@ class UserDefinedForm_Controller extends Page_Controller { } // Check for field dependencies / default - foreach($field->DisplayRules() as $rule) { + foreach($field->EffectiveDisplayRules() as $rule) { // Get the field which is effected $formFieldWatch = EditableFormField::get()->byId($rule->ConditionFieldID); diff --git a/code/model/editableformfields/EditableFormField.php b/code/model/editableformfields/EditableFormField.php index 40c6095..cee82c1 100755 --- a/code/model/editableformfields/EditableFormField.php +++ b/code/model/editableformfields/EditableFormField.php @@ -8,7 +8,12 @@ use SilverStripe\Forms\SegmentField; * * @package userforms * - * @property string Name + * @property string $Name + * @property string $Title + * @property string $Default + * @property int $Sort + * @property bool $Required + * @property string $CustomErrorMessage * @method UserDefinedForm Parent() Parent page * @method DataList DisplayRules() List of EditableCustomRule objects */ @@ -203,14 +208,39 @@ class EditableFormField extends DataObject { // Validation $validationFields = $this->getFieldValidationOptions(); - if($validationFields) { - $fields->addFieldsToTab( - 'Root.Validation', - $this->getFieldValidationOptions() + if($validationFields && $validationFields->count()) { + $fields->addFieldsToTab('Root.Validation', $validationFields); + } + + // Add display rule fields + $displayFields = $this->getDisplayRuleFields(); + if($displayFields && $displayFields->count()) { + $fields->addFieldsToTab('Root.DisplayRules', $displayFields); + } + + $this->extend('updateCMSFields', $fields); + + return $fields; + } + + /** + * Return fields to display on the 'Display Rules' tab + * + * @return FieldList + */ + protected function getDisplayRuleFields() { + // Check display rules + if($this->Required) { + return new FieldList( + LabelField::create(_t( + 'EditableFormField.DISPLAY_RULES_DISABLED', + 'Display rules are not enabled for required fields. ' . + 'Please uncheck "Is this field Required?" under "Validation" to re-enable.' + ))->addExtraClass('message warning') ); } - $allowedClasses = array_keys($this->getEditableFieldClasses(false)); $self = $this; + $allowedClasses = array_keys($this->getEditableFieldClasses(false)); $editableColumns = new GridFieldEditableColumns(); $editableColumns->setDisplayFields(array( 'Display' => '', @@ -251,7 +281,7 @@ class EditableFormField extends DataObject { new GridFieldDeleteAction() ); - $fields->addFieldsToTab('Root.DisplayRules', array( + return new FieldList( CheckboxField::create('ShowOnLoad') ->setDescription(_t( 'EditableFormField.SHOWONLOAD', @@ -263,16 +293,9 @@ class EditableFormField extends DataObject { $this->DisplayRules(), $customRulesConfig ) - )); - - $this->extend('updateCMSFields', $fields); - - return $fields; + ); } - /** - * @return void - */ public function onBeforeWrite() { parent::onBeforeWrite(); @@ -320,24 +343,24 @@ class EditableFormField extends DataObject { return false; } - /** - * Return whether a user can delete this form field - * based on whether they can edit the page - * + /** + * Return whether a user can delete this form field + * based on whether they can edit the page + * * @param Member $member - * @return bool - */ + * @return bool + */ public function canDelete($member = null) { return $this->canEdit($member); - } + } - /** - * Return whether a user can edit this form field - * based on whether they can edit the page - * + /** + * Return whether a user can edit this form field + * based on whether they can edit the page + * * @param Member $member - * @return bool - */ + * @return bool + */ public function canEdit($member = null) { $parent = $this->Parent(); if($parent && $parent->exists()) { @@ -346,7 +369,7 @@ class EditableFormField extends DataObject { // Fallback to secure admin permissions return parent::canEdit($member); - } + } /** * Return whether a user can view this form field @@ -678,7 +701,7 @@ class EditableFormField extends DataObject { */ protected function updateFormField($field) { // set the error / formatting messages - $field->setCustomValidationMessage($this->getErrorMessage()); + $field->setCustomValidationMessage($this->getErrorMessage()->RAW()); // set the right title on this field if($this->RightTitle) { @@ -741,34 +764,6 @@ class EditableFormField extends DataObject { return DBField::create_field('Varchar', $errorMessage); } - /** - * Validate the field taking into account its custom rules. - * - * @param Array $data - * @param UserForm $form - * - * @return boolean - */ - public function validateField($data, $form) { - if($this->Required && $this->DisplayRules()->Count() == 0) { - $formField = $this->getFormField(); - - if(isset($data[$this->Name])) { - $formField->setValue($data[$this->Name]); - } - - if( - !isset($data[$this->Name]) || - !$data[$this->Name] || - !$formField->validate($form->getValidator()) - ) { - $form->addErrorMessage($this->Name, $this->getErrorMessage()->HTML(), 'error', false); - } - } - - return true; - } - /** * Invoked by UserFormUpgradeService to migrate settings specific to this field from CustomSettings * to the field proper @@ -874,4 +869,17 @@ class EditableFormField extends DataObject { return EditableFormFieldValidator::create() ->setRecord($this); } + + /** + * Determine effective display rules for this field. + * + * @return SS_List + */ + public function EffectiveDisplayRules() { + if($this->Required) { + return new ArrayList(); + } + return $this->DisplayRules(); + } + } diff --git a/composer.json b/composer.json index f6cc752..048ca1c 100644 --- a/composer.json +++ b/composer.json @@ -38,7 +38,7 @@ }, "extra": { "branch-alias": { - "dev-master": "3.1.x-dev" + "dev-master": "4.0.x-dev" } } } diff --git a/tests/EditableFormFieldTest.php b/tests/EditableFormFieldTest.php index 5033e63..484492d 100644 --- a/tests/EditableFormFieldTest.php +++ b/tests/EditableFormFieldTest.php @@ -55,7 +55,8 @@ class EditableFormFieldTest extends FunctionalTest { // form has 2 fields - a checkbox and a text field // it has 1 rule - when ticked the checkbox hides the text field - $this->assertEquals($rules->Count(), 1); + $this->assertEquals(1, $rules->Count()); + $this->assertEquals($rules, $checkbox->EffectiveDisplayRules()); $checkboxRule = $rules->First(); $checkboxRule->ConditionFieldID = $field->ID; @@ -63,6 +64,10 @@ class EditableFormFieldTest extends FunctionalTest { $this->assertEquals($checkboxRule->Display, 'Hide'); $this->assertEquals($checkboxRule->ConditionOption, 'HasValue'); $this->assertEquals($checkboxRule->FieldValue, '6'); + + // If field is required then all custom rules are disabled + $checkbox->Required = true; + $this->assertEquals(0, $checkbox->EffectiveDisplayRules()->count()); } function testEditableDropdownField() { diff --git a/tests/EditableLiteralFieldTest.php b/tests/EditableLiteralFieldTest.php index 85508df..b4cfa4b 100644 --- a/tests/EditableLiteralFieldTest.php +++ b/tests/EditableLiteralFieldTest.php @@ -7,16 +7,9 @@ class EditableLiteralFieldTest extends SapphireTest { public function setUp() { parent::setUp(); - Config::nest(); HtmlEditorConfig::set_active('cms'); } - - public function tearDown() { - Config::unnest(); - parent::tearDown(); - } - /** * Tests the sanitisation of HTML content */ diff --git a/tests/UserDefinedFormControllerTest.php b/tests/UserDefinedFormControllerTest.php index 0d3f5a2..a43cd64 100644 --- a/tests/UserDefinedFormControllerTest.php +++ b/tests/UserDefinedFormControllerTest.php @@ -8,7 +8,7 @@ class UserDefinedFormControllerTest extends FunctionalTest { static $fixture_file = 'UserDefinedFormTest.yml'; - function testProcess() { + public function testProcess() { $form = $this->setupFormFrontend(); $controller = new UserDefinedFormControllerTest_Controller($form); @@ -60,7 +60,40 @@ class UserDefinedFormControllerTest extends FunctionalTest { $this->assertStringEndsWith('finished#uff', $response->getHeader('Location')); } - function testFinished() { + public function testValidation() { + $form = $this->setupFormFrontend('email-form'); + + // Post with no fields + $this->get($form->URLSegment); + $response = $this->submitForm('UserForm_Form', null, array()); + $this->assertPartialMatchBySelector( + '.field .message', + array('This field is required') + ); + + // Post with all fields, but invalid email + $this->get($form->URLSegment); + $this->submitForm('UserForm_Form', null, array( + 'required-email' => 'invalid', + 'required-text' => 'bob' + )); + $this->assertPartialMatchBySelector( + '.field .message', + array('Please enter an email address') + ); + + // Post with only required + $this->get($form->URLSegment); + $this->submitForm('UserForm_Form', null, array( + 'required-text' => 'bob' + )); + $this->assertPartialMatchBySelector( + 'p', + array("Thanks, we've received your submission.") + ); + } + + public function testFinished() { $form = $this->setupFormFrontend(); // set formProcessed and SecurityID to replicate the form being filled out @@ -72,7 +105,7 @@ class UserDefinedFormControllerTest extends FunctionalTest { $this->assertContains($form->OnCompleteMessage ,$response->getBody()); } - function testAppendingFinished() { + public function testAppendingFinished() { $form = $this->setupFormFrontend(); // replicate finished being added to the end of the form URL without the form being filled out @@ -84,7 +117,7 @@ class UserDefinedFormControllerTest extends FunctionalTest { $this->assertNotContains($form->OnCompleteMessage ,$response->getBody()); } - function testForm() { + public function testForm() { $form = $this->objFromFixture('UserDefinedForm', 'basic-form-page'); $controller = new UserDefinedFormControllerTest_Controller($form); @@ -103,7 +136,7 @@ class UserDefinedFormControllerTest extends FunctionalTest { $this->assertEquals(count($controller->Form()->getValidator()->getRequired()), 1); } - function testGetFormFields() { + public function testGetFormFields() { // generating the fieldset of fields $form = $this->objFromFixture('UserDefinedForm', 'basic-form-page'); @@ -125,7 +158,7 @@ class UserDefinedFormControllerTest extends FunctionalTest { $firstStep = $formSteps->first(); $firstField = $firstStep->getChildren()->first(); - $this->assertEquals('Custom Error Message', $firstField->getCustomValidationMessage()->getValue()); + $this->assertEquals('Custom Error Message', $firstField->getCustomValidationMessage()); $this->assertEquals($firstField->Title(), 'Required Text Field *'); // test custom right title @@ -146,7 +179,8 @@ class UserDefinedFormControllerTest extends FunctionalTest { $this->assertFalse($controller->Form()->getFormFields()->exists()); } - function testGetFormActions() { + public function testGetFormActions() + { // generating the fieldset of actions $form = $this->objFromFixture('UserDefinedForm', 'basic-form-page'); @@ -155,6 +189,7 @@ class UserDefinedFormControllerTest extends FunctionalTest { // by default will have 1 submit button which links to process $expected = new FieldList(new FormAction('process', 'Submit')); + $expected->setForm($controller->Form()); $this->assertEquals($actions, $expected); @@ -165,11 +200,12 @@ class UserDefinedFormControllerTest extends FunctionalTest { $expected = new FieldList(new FormAction('process', 'Custom Button')); $expected->push(new ResetFormAction("clearForm", "Clear")); + $expected->setForm($controller->Form()); $this->assertEquals($actions, $expected); } - function testRenderingIntoFormTemplate() { + public function testRenderingIntoFormTemplate() { $form = $this->setupFormFrontend(); $form->Content = 'This is some content without a form nested between it'; @@ -184,7 +220,7 @@ class UserDefinedFormControllerTest extends FunctionalTest { $this->checkTemplateIsCorrect($parser); } - function testRenderingIntoTemplateWithSubstringReplacement() { + public function testRenderingIntoTemplateWithSubstringReplacement() { $form = $this->setupFormFrontend(); $controller = new UserDefinedFormControllerTest_Controller($form); @@ -195,9 +231,14 @@ class UserDefinedFormControllerTest extends FunctionalTest { $this->checkTemplateIsCorrect($parser); } - - function setupFormFrontend() { - $form = $this->objFromFixture('UserDefinedForm', 'basic-form-page'); + /** + * Publish a form for use on the frontend + * + * @param string $fixtureName + * @return UserDefinedForm + */ + protected function setupFormFrontend($fixtureName = 'basic-form-page') { + $form = $this->objFromFixture('UserDefinedForm', $fixtureName); $this->logInWithPermission('ADMIN'); $form->doPublish(); @@ -208,7 +249,7 @@ class UserDefinedFormControllerTest extends FunctionalTest { return $form; } - function checkTemplateIsCorrect($parser) { + public function checkTemplateIsCorrect($parser) { $this->assertArrayHasKey(0, $parser->getBySelector('form#UserForm_Form')); // check for the input @@ -228,12 +269,12 @@ class UserDefinedFormControllerTest extends FunctionalTest { } } -class UserDefinedFormControllerTest_Controller extends UserDefinedForM_Controller implements TestOnly { +class UserDefinedFormControllerTest_Controller extends UserDefinedForm_Controller implements TestOnly { /** * Overloaded to avoid inconsistencies between 2.4.2 and 2.4.3 (disables all security tokens in unit tests by default) */ - function Form() { + public function Form() { $form = parent::Form(); if($form) $form->disableSecurityToken(); diff --git a/tests/UserDefinedFormTest.yml b/tests/UserDefinedFormTest.yml index a69385b..aed9982 100644 --- a/tests/UserDefinedFormTest.yml +++ b/tests/UserDefinedFormTest.yml @@ -100,7 +100,13 @@ EditableTextField: some-field: Name: SomeField - + + another-required: + Name: required-text + Title: Required Text Field + Required: true + CustomErrorMessage: 'This field is required' + EditableDropdown: basic-dropdown: Name: basic-dropdown @@ -137,6 +143,11 @@ EditableEmailField: Name: email-field Title: Email + another-email-field: + Name: required-email + Title: Enter your email + CustomErrorMessage: 'That email is not valid' + EditableRadioField: radio-field: Name: radio-option @@ -251,3 +262,7 @@ UserDefinedForm: empty-page: Title: 'Page with empty step' Fields: =>EditableFormStep.form6step1, =>EditableTextField.field-1, =>EditableFormStep.form6step2, =>EditableTextField.field-2, =>EditableFormStep.form6step3 + + email-form: + Title: 'Page with email field' + Fields: =>EditableEmailField.another-email-field, =>EditableTextField.another-required \ No newline at end of file