From 7e56b83002cd9eb58120cb2c8d971e4450d6fa64 Mon Sep 17 00:00:00 2001 From: Eileen McNaughton Date: Wed, 11 May 2022 09:53:29 +1200 Subject: [PATCH] [Import][REf] Cleanup required field checking --- CRM/Contact/Import/Parser/Contact.php | 89 ++++-------------- CRM/Import/Parser.php | 93 +++++++++++++++++++ ...idual_invalid_external_identifier_only.csv | 2 + ...rganization_email_no_organization_name.csv | 2 + .../CRM/Contact/Import/Parser/ContactTest.php | 62 ++++++++++--- 5 files changed, 166 insertions(+), 82 deletions(-) create mode 100644 tests/phpunit/CRM/Contact/Import/Form/data/individual_invalid_external_identifier_only.csv create mode 100644 tests/phpunit/CRM/Contact/Import/Form/data/organization_email_no_organization_name.csv diff --git a/CRM/Contact/Import/Parser/Contact.php b/CRM/Contact/Import/Parser/Contact.php index 0a1b1fcd6c..8eefbf1686 100644 --- a/CRM/Contact/Import/Parser/Contact.php +++ b/CRM/Contact/Import/Parser/Contact.php @@ -33,10 +33,6 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { protected $_mapperRelatedContactDetails; protected $_relationships; - protected $_emailIndex; - - protected $_phoneIndex; - /** * Is update only permitted on an id match. * @@ -143,18 +139,10 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { $this->setActiveFields($this->_mapperKeys); - $this->_phoneIndex = -1; - $this->_emailIndex = -1; $this->_externalIdentifierIndex = -1; $index = 0; foreach ($this->_mapperKeys as $key) { - if (substr($key, 0, 5) == 'email' && substr($key, 0, 14) != 'email_greeting') { - $this->_emailIndex = $index; - } - if (substr($key, 0, 5) == 'phone') { - $this->_phoneIndex = $index; - } if ($key == 'external_identifier') { $this->_externalIdentifierIndex = $index; } @@ -162,16 +150,27 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { } $this->_updateWithId = FALSE; - if (in_array('id', $this->_mapperKeys) || ($this->_externalIdentifierIndex >= 0 && in_array($this->_onDuplicate, [ - CRM_Import_Parser::DUPLICATE_UPDATE, - CRM_Import_Parser::DUPLICATE_FILL, - ]))) { + if (in_array('id', $this->_mapperKeys) || ($this->_externalIdentifierIndex >= 0 && $this->isUpdateExistingContacts())) { $this->_updateWithId = TRUE; } $this->_parseStreetAddress = CRM_Utils_Array::value('street_address_parsing', CRM_Core_BAO_Setting::valueOptions(CRM_Core_BAO_Setting::SYSTEM_PREFERENCES_NAME, 'address_options'), FALSE); } + /** + * Is this a case where the user has opted to update existing contacts. + * + * @return bool + * + * @throws \API_Exception + */ + private function isUpdateExistingContacts(): bool { + return in_array((int) $this->getSubmittedValue('onDuplicate'), [ + CRM_Import_Parser::DUPLICATE_UPDATE, + CRM_Import_Parser::DUPLICATE_FILL, + ], TRUE); + } + /** * Gets the fields available for importing in a key-name, title format. * @@ -3087,66 +3086,14 @@ class CRM_Contact_Import_Parser_Contact extends CRM_Import_Parser { * @param array $values * * @throws \API_Exception + * @throws \CRM_Core_Exception */ public function validateValues(array $values): void { - $errorMessage = NULL; - $errorRequired = FALSE; $params = $this->getMappedRow($values); - $missingNames = []; - switch ($params['contact_type']) { - case 'Individual': - if (empty($params['first_name'])) { - $missingNames[] = ts('First Name'); - } - if (empty($params['last_name'])) { - $missingNames[] = ts('Last Name'); - } - break; - - case 'Household': - if (empty($params['household_name'])) { - $missingNames[] = ts('Missing required fields:') . ' ' . ts('Household Name'); - } - break; - - case 'Organization': - if (empty($params['organization_name'])) { - $missingNames[] = ts('Missing required fields:') . ' ' . ts('Organization Name'); - } - break; - } - if (!empty($missingNames)) { - $errorMessage = ts('Missing required fields:') . ' ' . implode(' ' . ts('and') . ' ', $missingNames); - $errorRequired = TRUE; - } - - if ($this->_emailIndex >= 0) { - /* If we don't have the required fields, bail */ - - if ($this->_contactType === 'Individual' && !$this->_updateWithId) { - if ($errorRequired && empty($values[$this->_emailIndex])) { - if ($errorMessage) { - $errorMessage .= ' ' . ts('OR') . ' ' . ts('Email Address'); - } - else { - $errorMessage = ts('Missing required field:') . ' ' . ts('Email Address'); - } - throw new CRM_Core_Exception($errorMessage); - } - } - } - elseif ($errorRequired && !$this->_updateWithId) { - if ($errorMessage) { - $errorMessage .= ' ' . ts('OR') . ' ' . ts('Email Address'); - } - else { - $errorMessage = ts('Missing required field:') . ' ' . ts('Email Address'); - } - throw new CRM_Core_Exception($errorMessage); - } + $this->validateRequiredContactFields($params['contact_type'], $params, $this->isUpdateExistingContacts()); //check for duplicate external Identifier - $externalID = $values[$this->_externalIdentifierIndex] ?? NULL; + $externalID = $params['external_identifier'] ?? NULL; if ($externalID) { /* If it's a dupe,external Identifier */ diff --git a/CRM/Import/Parser.php b/CRM/Import/Parser.php index d2860ce585..50109fe150 100644 --- a/CRM/Import/Parser.php +++ b/CRM/Import/Parser.php @@ -498,6 +498,47 @@ abstract class CRM_Import_Parser { $this->_maxLinesToProcess = $max; } + /** + * Validate that we have the required fields to create the contact or find it to update. + * + * Note that the users duplicate selection affects this as follows + * - if they did not select an update variant then the id field is not + * permitted in the mapping - so we can assume the presence of id means + * we should use it + * - the external_identifier field is valid in place of the other fields + * when they have chosen update or fill - in this case we are only looking + * to update an existing contact. + * + * @param string $contactType + * @param array $params + * @param bool $isPermitExistingMatchFields + * + * @return void + * @throws \CRM_Core_Exception + */ + protected function validateRequiredContactFields(string $contactType, array $params, bool $isPermitExistingMatchFields = TRUE): void { + if (!empty($params['id'])) { + return; + } + $requiredFields = [ + 'Individual' => [ + 'first_name_last_name' => ['first_name' => ts('First Name'), 'last_name' => ts('Last Name')], + 'email' => ts('Email Address'), + ], + 'Organization' => ['organization_name' => ts('Organization Name')], + 'Household' => ['household_name' => ts('Household Name')], + ][$contactType]; + if ($isPermitExistingMatchFields) { + $requiredFields['external_identifier'] = ts('External Identifier'); + // Historically just an email has been accepted as it is 'usually good enough' + // for a dedupe rule look up - but really this is a stand in for + // whatever is needed to find an existing matching contact using the + // specified dedupe rule (or the default Unsupervised if not specified). + $requiredFields['email'] = ts('Email Address'); + } + $this->validateRequiredFields($requiredFields, $params); + } + /** * Determines the file extension based on error code. * @@ -1073,4 +1114,56 @@ abstract class CRM_Import_Parser { } } + /** + * Validate that the field requirements are met in the params. + * + * @param array $requiredFields + * @param array $params + * An array of required fields (fieldName => label) + * - note this follows the and / or array nesting we see in permission checks + * eg. + * [ + * 'email' => ts('Email'), + * ['first_name' => ts('First Name'), 'last_name' => ts('Last Name')] + * ] + * Means 'email' OR 'first_name AND 'last_name'. + * + * @throws \CRM_Core_Exception + * Exception thrown if field requirements are not met. + */ + protected function validateRequiredFields(array $requiredFields, array $params): void { + $missingFields = []; + foreach ($requiredFields as $key => $required) { + if (!is_array($required)) { + $importParameter = $params[$key] ?? []; + if (!is_array($importParameter)) { + if (!empty($importParameter)) { + return; + } + } + else { + foreach ($importParameter as $locationValues) { + if (!empty($locationValues[$key])) { + return; + } + } + } + + $missingFields[$key] = $required; + } + else { + foreach ($required as $field => $label) { + if (empty($params[$field])) { + $missing[$field] = $label; + } + } + if (empty($missing)) { + return; + } + $missingFields[$key] = implode(' ' . ts('and') . ' ', $missing); + } + } + throw new CRM_Core_Exception(ts('Missing required fields:') . ' ' . implode(' ' . ts('OR') . ' ', $missingFields)); + } + } diff --git a/tests/phpunit/CRM/Contact/Import/Form/data/individual_invalid_external_identifier_only.csv b/tests/phpunit/CRM/Contact/Import/Form/data/individual_invalid_external_identifier_only.csv new file mode 100644 index 0000000000..db964d81f0 --- /dev/null +++ b/tests/phpunit/CRM/Contact/Import/Form/data/individual_invalid_external_identifier_only.csv @@ -0,0 +1,2 @@ +External Identifier,Gender +1234,Female diff --git a/tests/phpunit/CRM/Contact/Import/Form/data/organization_email_no_organization_name.csv b/tests/phpunit/CRM/Contact/Import/Form/data/organization_email_no_organization_name.csv new file mode 100644 index 0000000000..8c7b68314c --- /dev/null +++ b/tests/phpunit/CRM/Contact/Import/Form/data/organization_email_no_organization_name.csv @@ -0,0 +1,2 @@ +Email,Phone +my-org@example.com,0123 111 111 diff --git a/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php b/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php index a63ed3bde4..899de1339c 100644 --- a/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php +++ b/tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php @@ -94,6 +94,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * Test that import parser will not fail when same external_identifier found * of deleted contact. * + * @throws \API_Exception * @throws \CRM_Core_Exception * @throws \CiviCRM_API3_Exception */ @@ -117,7 +118,9 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * * In this case the contact has no external identifier. * + * @throws \API_Exception * @throws \CRM_Core_Exception + * @throws \CiviCRM_API3_Exception */ public function testImportParserWithUpdateWithoutExternalIdentifier(): void { [$originalValues, $result] = $this->setUpBaseContact(); @@ -135,7 +138,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * * @throws \Exception */ - public function testImportParserWithUpdateWithExternalIdentifier() { + public function testImportParserWithUpdateWithExternalIdentifier(): void { [$originalValues, $result] = $this->setUpBaseContact(['external_identifier' => 'windows']); $this->assertEquals($result['id'], CRM_Core_DAO::getFieldValue('CRM_Contact_DAO_Contact', 'windows', 'id', 'external_identifier', TRUE)); @@ -158,7 +161,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * * @throws \Exception */ - public function testImportParserWithUpdateWithExternalIdentifierButNoPrimaryMatch() { + public function testImportParserWithUpdateWithExternalIdentifierButNoPrimaryMatch(): void { [$originalValues, $result] = $this->setUpBaseContact([ 'external_identifier' => 'windows', 'email' => NULL, @@ -183,7 +186,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * * @throws \Exception */ - public function testImportParserWithUpdateWithContactID() { + public function testImportParserWithUpdateWithContactID(): void { [$originalValues, $result] = $this->setUpBaseContact([ 'external_identifier' => '', 'email' => NULL, @@ -203,7 +206,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * * @throws \Exception */ - public function testImportParserWithUpdateWithNoExternalIdentifier() { + public function testImportParserWithUpdateWithNoExternalIdentifier(): void { [$originalValues, $result] = $this->setUpBaseContact(); $originalValues['nick_name'] = 'Old Bill'; $originalValues['external_identifier'] = 'windows'; @@ -720,11 +723,16 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * * @dataProvider validateDataProvider * + * @param string $csv + * @param array $mapper + * @param string $expectedError + * @param array $submittedValues + * * @throws \API_Exception */ - public function testValidation($csv, $mapper, $expectedError): void { + public function testValidation(string $csv, array $mapper, string $expectedError = '', $submittedValues = []): void { try { - $this->validateCSV($csv, $mapper); + $this->validateCSV($csv, $mapper, $submittedValues); } catch (CRM_Core_Exception $e) { $this->assertSame($expectedError, $e->getMessage()); @@ -757,6 +765,33 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { 'mapper' => [['1_a_b', 'email', 1], ['first_name'], ['last_name']], 'expected_error' => 'Invalid value for field(s) : email', ], + 'individual_invalid_external_identifier_only' => [ + // External identifier is only enough in upgrade mode. + 'csv' => 'individual_invalid_external_identifier_only.csv', + 'mapper' => [['external_identifier'], ['gender_id']], + 'expected_error' => 'Missing required fields: First Name and Last Name OR Email Address', + ], + 'individual_invalid_external_identifier_only_update_mode' => [ + // External identifier only enough in upgrade mode, so no error here. + 'csv' => 'individual_invalid_external_identifier_only.csv', + 'mapper' => [['external_identifier'], ['gender_id']], + 'expected_error' => '', + 'submitted_values' => ['onDuplicate' => CRM_Import_Parser::DUPLICATE_UPDATE], + ], + 'organization_email_no_organization_name' => [ + // Email is only enough in upgrade mode. + 'csv' => 'organization_email_no_organization_name.csv', + 'mapper' => [['email'], ['phone', 1, 1]], + 'expected_error' => 'Missing required fields: Organization Name', + 'submitted_values' => ['onDuplicate' => CRM_Import_Parser::DUPLICATE_SKIP, 'contactType' => CRM_Import_Parser::CONTACT_ORGANIZATION], + ], + 'organization_email_no_organization_name_update_mode' => [ + // Email is enough in upgrade mode (at least to pass validate). + 'csv' => 'organization_email_no_organization_name.csv', + 'mapper' => [['email'], ['phone', 1, 1]], + 'expected_error' => '', + 'submitted_values' => ['onDuplicate' => CRM_Import_Parser::DUPLICATE_UPDATE, 'contactType' => CRM_Import_Parser::CONTACT_ORGANIZATION], + ], ]; } @@ -1019,11 +1054,10 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { foreach ($fields as $index => $field) { $mapper[] = [$field, $mapperLocType[$index] ?? NULL, $field === 'phone' ? 1 : NULL]; } - $userJobID = $this->getUserJobID(['mapper' => $mapper]); + $userJobID = $this->getUserJobID(['mapper' => $mapper, 'onDuplicate' => $onDuplicateAction]); $parser = new CRM_Contact_Import_Parser_Contact($fields, $mapperLocType); $parser->setUserJobID($userJobID); $parser->_dedupeRuleGroupID = $ruleGroupId; - $parser->_onDuplicate = $onDuplicateAction; $parser->init(); $this->assertEquals($expectedResult, $parser->import($onDuplicateAction, $values), 'Return code from parser import was not as expected'); } @@ -1163,6 +1197,7 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { 'doGeocodeAddress' => 0, 'dataSource' => 'CRM_Import_DataSource_SQL', 'sqlQuery' => 'SELECT first_name FROM civicrm_contact', + 'onDuplicate' => CRM_Import_Parser::DUPLICATE_SKIP, ], $submittedValues), ], 'status_id:name' => 'draft', @@ -1184,18 +1219,23 @@ class CRM_Contact_Import_Parser_ContactTest extends CiviUnitTestCase { * @param string $csv Name of csv file. * @param array $mapper Mapping as entered on MapField form. * e.g [['first_name']['email', 1]]. + * @param array $submittedValues + * Any submitted values overrides. * * @throws \API_Exception * @throws \CRM_Core_Exception */ - protected function validateCSV(string $csv, array $mapper): void { - $userJobID = $this->getUserJobID([ + protected function validateCSV(string $csv, array $mapper, $submittedValues): void { + $userJobID = $this->getUserJobID(array_merge([ 'uploadFile' => ['name' => __DIR__ . '/../Form/data/' . $csv], 'skipColumnHeader' => TRUE, 'fieldSeparator' => ',', + 'onDuplicate' => CRM_Import_Parser::DUPLICATE_SKIP, + 'contactType' => CRM_Import_Parser::CONTACT_INDIVIDUAL, 'mapper' => $mapper, 'dataSource' => 'CRM_Import_DataSource_CSV', - ]); + ], $submittedValues)); + $dataSource = new CRM_Import_DataSource_CSV($userJobID); $parser = new CRM_Contact_Import_Parser_Contact(); $parser->setUserJobID($userJobID); -- 2.25.1