From 92414567d9f0bc11e3a223166ce829c34ef2d6b5 Mon Sep 17 00:00:00 2001 From: Coleman Watts Date: Sat, 29 Oct 2022 11:08:24 -0400 Subject: [PATCH] Afform - Refactor required fields validation to use an Event --- .../Civi/Afform/AfformMetadataInjector.php | 61 +-------- .../Civi/Afform/Event/AfformPrefillEvent.php | 2 +- .../Civi/Afform/Event/AfformValidateEvent.php | 54 ++++++++ ext/afform/core/Civi/Afform/FormDataModel.php | 68 ++++++++++ .../core/Civi/Api4/Action/Afform/Submit.php | 126 ++++++++---------- ext/afform/core/afform.php | 1 + .../phpunit/api/v4/AfformContactUsageTest.php | 23 ++-- 7 files changed, 198 insertions(+), 137 deletions(-) create mode 100644 ext/afform/core/Civi/Afform/Event/AfformValidateEvent.php diff --git a/ext/afform/core/Civi/Afform/AfformMetadataInjector.php b/ext/afform/core/Civi/Afform/AfformMetadataInjector.php index 48c9264173..5810e62a17 100644 --- a/ext/afform/core/Civi/Afform/AfformMetadataInjector.php +++ b/ext/afform/core/Civi/Afform/AfformMetadataInjector.php @@ -11,7 +11,6 @@ namespace Civi\Afform; -use Civi\Api4\Utils\CoreUtil; use CRM_Afform_ExtensionUtil as E; /** @@ -87,7 +86,7 @@ class AfformMetadataInjector { */ private static function getFieldMetadata($entityNames, string $action, string $fieldName):? array { foreach ((array) $entityNames as $entityName) { - $fieldInfo = self::getField($entityName, $fieldName, $action); + $fieldInfo = FormDataModel::getField($entityName, $fieldName, $action); if ($fieldInfo) { return $fieldInfo; } @@ -177,64 +176,6 @@ class AfformMetadataInjector { } } - /** - * @param string $entityName - * @param string $fieldName - * @param string $action - * @return array|NULL - */ - private static function getField(string $entityName, string $fieldName, string $action):? array { - // For explicit joins, strip the alias off the field name - if (strpos($entityName, ' AS ')) { - [$entityName, $alias] = explode(' AS ', $entityName); - $fieldName = preg_replace('/^' . preg_quote($alias . '.', '/') . '/', '', $fieldName); - } - $namesToMatch = [$fieldName]; - // Also match base field if this is an implicit join - if ($action === 'get' && strpos($fieldName, '.')) { - $namesToMatch[] = substr($fieldName, 0, strrpos($fieldName, '.')); - } - $params = [ - 'action' => $action, - 'where' => [['name', 'IN', $namesToMatch]], - 'select' => ['name', 'label', 'input_type', 'input_attrs', 'help_pre', 'help_post', 'options', 'fk_entity', 'required'], - 'loadOptions' => ['id', 'label'], - // If the admin included this field on the form, then it's OK to get metadata about the field regardless of user permissions. - 'checkPermissions' => FALSE, - ]; - if (in_array($entityName, \CRM_Contact_BAO_ContactType::basicTypes(TRUE))) { - $params['values'] = ['contact_type' => $entityName]; - $entityName = 'Contact'; - } - foreach (civicrm_api4($entityName, 'getFields', $params) as $field) { - // In the highly unlikely event of 2 fields returned, prefer the exact match - if ($field['name'] === $fieldName) { - break; - } - } - if (!isset($field)) { - return NULL; - } - // Id field for selecting existing entity - if ($action === 'create' && $field['name'] === CoreUtil::getIdFieldName($entityName)) { - $entityTitle = CoreUtil::getInfoItem($entityName, 'title'); - $field['input_type'] = 'Existing'; - $field['entity'] = $entityName; - $field['label'] = E::ts('Existing %1', [1 => $entityTitle]); - $field['input_attrs']['placeholder'] = E::ts('Select %1', [1 => $entityTitle]); - } - // If this is an implicit join, get new field from fk entity - if ($field['name'] !== $fieldName && $field['fk_entity']) { - $params['where'] = [['name', '=', substr($fieldName, 1 + strrpos($fieldName, '.'))]]; - $originalField = $field; - $field = civicrm_api4($field['fk_entity'], 'getFields', $params)->first(); - if ($field) { - $field['label'] = $originalField['label'] . ' ' . $field['label']; - } - } - return $field; - } - /** * Determines name of the api entit(ies) based on the field name prefix * diff --git a/ext/afform/core/Civi/Afform/Event/AfformPrefillEvent.php b/ext/afform/core/Civi/Afform/Event/AfformPrefillEvent.php index 7f02c9423e..df2ec46579 100644 --- a/ext/afform/core/Civi/Afform/Event/AfformPrefillEvent.php +++ b/ext/afform/core/Civi/Afform/Event/AfformPrefillEvent.php @@ -9,7 +9,7 @@ class AfformPrefillEvent extends AfformBaseEvent { use AfformEventEntityTrait; /** - * AfformSubmitEvent constructor. + * AfformPrefillEvent constructor. * * @param array $afform * @param \Civi\Afform\FormDataModel $formDataModel diff --git a/ext/afform/core/Civi/Afform/Event/AfformValidateEvent.php b/ext/afform/core/Civi/Afform/Event/AfformValidateEvent.php new file mode 100644 index 0000000000..29b2a306cb --- /dev/null +++ b/ext/afform/core/Civi/Afform/Event/AfformValidateEvent.php @@ -0,0 +1,54 @@ +entityValues = $entityValues; + parent::__construct($afform, $formDataModel, $apiRequest); + } + + /** + * @param string $errorMsg + */ + public function setError(string $errorMsg) { + $this->errors[] = $errorMsg; + } + + /** + * @return array + */ + public function getErrors(): array { + return $this->errors; + } + + /** + * @return array + */ + public function getEntityValues(): array { + return $this->entityValues; + } + +} diff --git a/ext/afform/core/Civi/Afform/FormDataModel.php b/ext/afform/core/Civi/Afform/FormDataModel.php index 8d99733f8c..18cc358689 100644 --- a/ext/afform/core/Civi/Afform/FormDataModel.php +++ b/ext/afform/core/Civi/Afform/FormDataModel.php @@ -4,6 +4,8 @@ namespace Civi\Afform; use Civi\API\Exception\UnauthorizedException; use Civi\Api4\Afform; +use Civi\Api4\Utils\CoreUtil; +use CRM_Afform_ExtensionUtil as E; /** * Class FormDataModel @@ -131,6 +133,12 @@ class FormDataModel { } /** + * Fills $this->entities[*]['fields'] and $this->['entities'][*]['joins'][*]['fields'] + * and $this->searchDisplays[*]['fields'] + * + * Note that it does not fill in fields metadata from the schema, only the markup in the form. + * To fetch field's schema definition, use the getFields function. + * * @param array $nodes * @param string $entity * @param string $join @@ -180,6 +188,66 @@ class FormDataModel { } } + /** + * Loads a field definition from the schema + * + * @param string $entityName + * @param string $fieldName + * @param string $action + * @return array|NULL + */ + public static function getField(string $entityName, string $fieldName, string $action): ?array { + // For explicit joins, strip the alias off the field name + if (strpos($entityName, ' AS ')) { + [$entityName, $alias] = explode(' AS ', $entityName); + $fieldName = preg_replace('/^' . preg_quote($alias . '.', '/') . '/', '', $fieldName); + } + $namesToMatch = [$fieldName]; + // Also match base field if this is an implicit join + if ($action === 'get' && strpos($fieldName, '.')) { + $namesToMatch[] = substr($fieldName, 0, strrpos($fieldName, '.')); + } + $params = [ + 'action' => $action, + 'where' => [['name', 'IN', $namesToMatch]], + 'select' => ['name', 'label', 'input_type', 'input_attrs', 'help_pre', 'help_post', 'options', 'fk_entity', 'required'], + 'loadOptions' => ['id', 'label'], + // If the admin included this field on the form, then it's OK to get metadata about the field regardless of user permissions. + 'checkPermissions' => FALSE, + ]; + if (in_array($entityName, \CRM_Contact_BAO_ContactType::basicTypes(TRUE))) { + $params['values'] = ['contact_type' => $entityName]; + $entityName = 'Contact'; + } + foreach (civicrm_api4($entityName, 'getFields', $params) as $field) { + // In the highly unlikely event of 2 fields returned, prefer the exact match + if ($field['name'] === $fieldName) { + break; + } + } + if (!isset($field)) { + return NULL; + } + // Id field for selecting existing entity + if ($action === 'create' && $field['name'] === CoreUtil::getIdFieldName($entityName)) { + $entityTitle = CoreUtil::getInfoItem($entityName, 'title'); + $field['input_type'] = 'Existing'; + $field['entity'] = $entityName; + $field['label'] = E::ts('Existing %1', [1 => $entityTitle]); + $field['input_attrs']['placeholder'] = E::ts('Select %1', [1 => $entityTitle]); + } + // If this is an implicit join, get new field from fk entity + if ($field['name'] !== $fieldName && $field['fk_entity']) { + $params['where'] = [['name', '=', substr($fieldName, 1 + strrpos($fieldName, '.'))]]; + $originalField = $field; + $field = civicrm_api4($field['fk_entity'], 'getFields', $params)->first(); + if ($field) { + $field['label'] = $originalField['label'] . ' ' . $field['label']; + } + } + return $field; + } + /** * Finds a search display within a fieldset * diff --git a/ext/afform/core/Civi/Api4/Action/Afform/Submit.php b/ext/afform/core/Civi/Api4/Action/Afform/Submit.php index 8844060d01..f9c301cb1b 100644 --- a/ext/afform/core/Civi/Api4/Action/Afform/Submit.php +++ b/ext/afform/core/Civi/Api4/Action/Afform/Submit.php @@ -2,7 +2,10 @@ namespace Civi\Api4\Action\Afform; +use CRM_Afform_ExtensionUtil as E; use Civi\Afform\Event\AfformSubmitEvent; +use Civi\Afform\Event\AfformValidateEvent; +use Civi\Afform\FormDataModel; use Civi\Api4\AfformSubmission; use Civi\Api4\RelationshipType; use Civi\Api4\Utils\CoreUtil; @@ -26,15 +29,7 @@ class Submit extends AbstractProcessor { protected $values; protected function processForm() { - // Save submission record - if (!empty($this->_afform['create_submission'])) { - $submission = AfformSubmission::create(FALSE) - ->addValue('contact_id', \CRM_Core_Session::getLoggedInContactID()) - ->addValue('afform_name', $this->name) - ->addValue('data', $this->getValues()) - ->execute()->first(); - } - + // Preprocess submitted values $entityValues = []; foreach ($this->_formDataModel->getEntities() as $entityName => $entity) { $entityValues[$entityName] = []; @@ -68,11 +63,24 @@ class Submit extends AbstractProcessor { } } - // validate the submitted values for required - if (!$this->validateForm()) { - throw new \Exception(ts('Please fill all required fields.')); + // Call validation handlers + $event = new AfformValidateEvent($this->_afform, $this->_formDataModel, $this, $entityValues); + \Civi::dispatcher()->dispatch('civi.afform.validate', $event); + $errors = $event->getErrors(); + if ($errors) { + throw new \CRM_Core_Exception(ts('Validation Error', ['plural' => '%1 Validation Errors', 'count' => count($errors)]), 0, ['validation' => $errors]); } + // Save submission record + if (!empty($this->_afform['create_submission'])) { + $submission = AfformSubmission::create(FALSE) + ->addValue('contact_id', \CRM_Core_Session::getLoggedInContactID()) + ->addValue('afform_name', $this->name) + ->addValue('data', $this->getValues()) + ->execute()->first(); + } + + // Call submit handlers $entityWeights = \Civi\Afform\Utils::getEntityWeights($this->_formDataModel->getEntities(), $entityValues); foreach ($entityWeights as $entityName) { $entityType = $this->_formDataModel->getEntity($entityName)['type']; @@ -117,75 +125,59 @@ class Submit extends AbstractProcessor { } /** - * Function to validate the submitted values + * Validate required field values * - * @return boolean + * @param \Civi\Afform\Event\AfformValidateEvent $event */ - private function validateForm() { - $isValid = TRUE; - - // get feild flatterned defintions - $fieldDefinitions = $this->getAllFieldDefinitions(); - - // loops through each field and validates the submitted values - foreach ($fieldDefinitions as $entityName => $field) { - foreach ($this->values[$entityName] as $index => $values) { - foreach ($field as $fieldName => $fieldDefinition) { - if ($fieldName !== 'joins') { - if (!empty($fieldDefinition['required']) && empty($values['fields'][$fieldName])) { - $isValid = FALSE; - } + public static function validateRequiredFields(AfformValidateEvent $event): void { + foreach ($event->getFormDataModel()->getEntities() as $entityName => $entity) { + $entityValues = $event->getEntityValues()[$entityName] ?? []; + foreach ($entityValues as $values) { + foreach ($entity['fields'] as $fieldName => $attributes) { + $error = self::getRequiredFieldError($entity['type'], $fieldName, $attributes, $values['fields'][$fieldName] ?? NULL); + if ($error) { + $event->setError($error); } - else { - // loop through each joins and validates the submitted values - foreach ($field['joins'] as $joinName => $joinField) { - foreach ($values['joins'][$joinName] as $joinIndex => $joinValues) { - foreach ($joinField as $joinFieldName => $joinFieldDefinition) { - if (!empty($joinFieldDefinition['required']) && empty($joinValues[$joinFieldName])) { - $isValid = FALSE; - } - } + } + foreach ($entity['joins'] as $joinEntity => $join) { + foreach ($values['joins'][$joinEntity] ?? [] as $joinIndex => $joinValues) { + foreach ($join['fields'] ?? [] as $fieldName => $attributes) { + $error = self::getRequiredFieldError($joinEntity, $fieldName, $attributes, $joinValues[$fieldName] ?? NULL); + if ($error) { + $event->setError($error); } } } } } } - - return $isValid; } /** - * Get all field definitions for this form + * If a required field is missing a value, return an error message * - * @return array $fieldDefinitions + * @param string $apiEntity + * @param string $fieldName + * @param array $attributes + * @param mixed $value + * @return string|null */ - private function getAllFieldDefinitions() { - $fieldDefinitions = []; - - foreach ($this->_formDataModel->getEntities() as $entityName => $entity) { - // main entity fields - foreach ($entity['fields'] as $field => $attributes) { - if (!empty($attributes['defn'])) { - $fieldDefinitions[$entityName][$field] = array_merge(['name' => $field], $attributes['defn']); - } - } - - // join entity fields - if (!empty($entity['joins'])) { - foreach ($entity['joins'] as $joinEntityName => $joinEntity) { - if (!empty($joinEntity['fields'])) { - foreach ($joinEntity['fields'] as $joinField => $joinAttributes) { - if (!empty($joinAttributes['defn'])) { - $fieldDefinitions[$entityName]['joins'][$joinEntityName][$joinField] = array_merge(['name' => $joinField], $joinAttributes['defn']); - } - } - } - } - } + private static function getRequiredFieldError(string $apiEntity, string $fieldName, $attributes, $value) { + // If we have a value, no need to check if required + if ($value || is_numeric($value) || is_bool($value)) { + return NULL; } - - return $fieldDefinitions; + // Required set to false, no need to validate + if (isset($attributes['defn']['required']) && !$attributes['defn']['required']) { + return NULL; + } + $fullDefn = FormDataModel::getField($apiEntity, $fieldName, 'create'); + $isRequired = $attributes['defn']['required'] ?? $fullDefn['required'] ?? FALSE; + if ($isRequired) { + $label = $attributes['defn']['label'] ?? $fullDefn['label']; + return E::ts('%1 is a required field.', [1 => $label]); + } + return NULL; } /** @@ -217,7 +209,7 @@ class Submit extends AbstractProcessor { } /** - * Validate contact(s) meet the minimum requirements to be created (name and/or email). + * Check if contact(s) meet the minimum requirements to be created (name and/or email). * * This requires a function because simple required fields validation won't work * across multiple entities (contact + n email addresses). diff --git a/ext/afform/core/afform.php b/ext/afform/core/afform.php index c546334c83..5041b7509d 100644 --- a/ext/afform/core/afform.php +++ b/ext/afform/core/afform.php @@ -49,6 +49,7 @@ function afform_civicrm_config(&$config) { Civi::$statics[__FUNCTION__] = 1; $dispatcher = Civi::dispatcher(); + $dispatcher->addListener('civi.afform.validate', ['\Civi\Api4\Action\Afform\Submit', 'validateRequiredFields'], 10); $dispatcher->addListener('civi.afform.submit', ['\Civi\Api4\Action\Afform\Submit', 'processGenericEntity'], 0); $dispatcher->addListener('civi.afform.submit', ['\Civi\Api4\Action\Afform\Submit', 'preprocessContact'], 10); $dispatcher->addListener('civi.afform.submit', ['\Civi\Api4\Action\Afform\Submit', 'processRelationships'], 1); diff --git a/ext/afform/mock/tests/phpunit/api/v4/AfformContactUsageTest.php b/ext/afform/mock/tests/phpunit/api/v4/AfformContactUsageTest.php index ac5203e4dd..33245f63c9 100644 --- a/ext/afform/mock/tests/phpunit/api/v4/AfformContactUsageTest.php +++ b/ext/afform/mock/tests/phpunit/api/v4/AfformContactUsageTest.php @@ -63,10 +63,11 @@ EOHTML;
- + +
- +
@@ -328,7 +329,7 @@ EOHTML; ], 'joins' => [ 'Email' => [ - ['email' => $individualEmail, 'location_type_id' => $locationType], + ['email' => $individualEmail, 'location_type_id' => $locationType, 'is_primary' => TRUE], ], ], ], @@ -338,7 +339,7 @@ EOHTML; 'fields' => [], 'joins' => [ 'Email' => [ - ['email' => $orgEmail, 'location_type_id' => $locationType], + ['email' => $orgEmail, 'location_type_id' => $locationType, 'is_primary' => TRUE], ], ], ], @@ -406,7 +407,7 @@ EOHTML; ], 'joins' => [ 'Email' => [ - ['email' => '123@example.com', 'location_type_id' => $locationType], + ['email' => '123@example.com', 'location_type_id' => $locationType, 'is_primary' => TRUE], ], ], ], @@ -437,22 +438,25 @@ EOHTML; 'joins' => [ 'Email' => [ ['email' => 'test@example.org'], + [], ], ], ], ], ]; - $this->expectException(\Exception::class); - try { Civi\Api4\Afform::submit() ->setName($this->formName) ->setValues($values) ->execute(); + $this->fail('Should have thrown exception'); } catch (\CRM_Core_Exception $e) { // Should fail required fields missing + $this->assertCount(2, $e->getErrorData()['validation']); + $this->assertEquals('First Name is a required field.', $e->getErrorData()['validation'][0]); + $this->assertEquals('Email is a required field.', $e->getErrorData()['validation'][1]); } } @@ -477,16 +481,17 @@ EOHTML; ], ]; - $this->expectException(\Exception::class); - try { Civi\Api4\Afform::submit() ->setName($this->formName) ->setValues($values) ->execute(); + $this->fail('Should have thrown exception'); } catch (\CRM_Core_Exception $e) { // Should fail required fields missing + $this->assertCount(1, $e->getErrorData()['validation']); + $this->assertEquals('Email is a required field.', $e->getErrorData()['validation'][0]); } } -- 2.25.1