Skip to content

Commit

Permalink
[Import][REf] Cleanup required field checking
Browse files Browse the repository at this point in the history
  • Loading branch information
@eileenmcnaughton
eileenmcnaughton committed May 13, 2022
1 parent 7a50c81 commit 7e56b83
Show file tree
Hide file tree
Showing 5 changed files with 166 additions and 82 deletions.
89 changes: 18 additions & 71 deletions CRM/Contact/Import/Parser/Contact.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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.
*
Expand Down Expand Up @@ -143,35 +139,38 @@ public function init() {

$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;
}
$index++;
}

$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.
*
Expand Down Expand Up @@ -3087,66 +3086,14 @@ public function getMappedRow(array $values): array {
* @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 */

Expand Down
93 changes: 93 additions & 0 deletions CRM/Import/Parser.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -498,6 +498,47 @@ public function setMaxLinesToProcess($max) {
$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.
*
Expand Down Expand Up @@ -1073,4 +1114,56 @@ protected function getIdsOfMatchingContacts(array $formatted):array {
}
}

/**
* 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));
}

}
2 changes: 2 additions & 0 deletions tests/phpunit/CRM/Contact/Import/Form/data/individual_invalid_external_identifier_only.csv
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
External Identifier,Gender
1234,Female
2 changes: 2 additions & 0 deletions tests/phpunit/CRM/Contact/Import/Form/data/organization_email_no_organization_name.csv
View file
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
Email,Phone
my-org@example.com,0123 111 111
62 changes: 51 additions & 11 deletions tests/phpunit/CRM/Contact/Import/Parser/ContactTest.php
View file
Original file line number Diff line number Diff line change
Expand Up @@ -94,6 +94,7 @@ public function testImportParserWithEmployeeOfRelationship(): void {
* 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
*/
Expand All @@ -117,7 +118,9 @@ public function testImportParserWithDeletedContactExternalIdentifier(): void {
*
* 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();
Expand All @@ -135,7 +138,7 @@ public function testImportParserWithUpdateWithoutExternalIdentifier(): void {
*
* @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));
Expand All @@ -158,7 +161,7 @@ public function testImportParserWithUpdateWithExternalIdentifier() {
*
* @throws \Exception
*/
public function testImportParserWithUpdateWithExternalIdentifierButNoPrimaryMatch() {
public function testImportParserWithUpdateWithExternalIdentifierButNoPrimaryMatch(): void {
[$originalValues, $result] = $this->setUpBaseContact([
'external_identifier' => 'windows',
'email' => NULL,
Expand All @@ -183,7 +186,7 @@ public function testImportParserWithUpdateWithExternalIdentifierButNoPrimaryMatc
*
* @throws \Exception
*/
public function testImportParserWithUpdateWithContactID() {
public function testImportParserWithUpdateWithContactID(): void {
[$originalValues, $result] = $this->setUpBaseContact([
'external_identifier' => '',
'email' => NULL,
Expand All @@ -203,7 +206,7 @@ public function testImportParserWithUpdateWithContactID() {
*
* @throws \Exception
*/
public function testImportParserWithUpdateWithNoExternalIdentifier() {
public function testImportParserWithUpdateWithNoExternalIdentifier(): void {
[$originalValues, $result] = $this->setUpBaseContact();
$originalValues['nick_name'] = 'Old Bill';
$originalValues['external_identifier'] = 'windows';
Expand Down Expand Up @@ -720,11 +723,16 @@ public function testCustomFieldValidation(): void {
*
* @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());
Expand Down Expand Up @@ -757,6 +765,33 @@ public function validateDataProvider(): array {
'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],
],
];
}

Expand Down Expand Up @@ -1019,11 +1054,10 @@ protected function runImport(array $originalValues, $onDuplicateAction, $expecte
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');
}
Expand Down Expand Up @@ -1163,6 +1197,7 @@ protected function getUserJobID($submittedValues = []) {
'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',
Expand All @@ -1184,18 +1219,23 @@ protected function getUserJobID($submittedValues = []) {
* @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);
Expand Down

0 comments on commit 7e56b83

Please sign in to comment.